[psr2]: remove spammy damage log messages [chromium/src : main]
Femi Adegunloye (Gerrit)
Femi Adegunloye has uploaded this change for review.
[psr2]: remove spammy damage log messages
Empty damage clips should not be passed to drm or it might cause
artifacts and black screens, but they don't need to be logged because
it's not an error for chrome to return an empty damage rect.
Also, fix a bug when checking if damage rect is within display bounds.
And restrict the logging to only when PSR2 is enabled.
BUG=b/307353217
TEST=build and deploy to DUT
Change-Id: I6788aeef3b770ec8c544fe8b401d1f2b686146b0
---
M ui/ozone/platform/drm/gpu/drm_gpu_util.cc
M ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc
2 files changed, 8 insertions(+), 10 deletions(-)
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel.
Femi Adegunloye would like Gil Dekel to review this change.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel.
2 comments:
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
Patch Set #1, Line 90:
// Damage rect can be empty, but sending empty or negative rects can result inLooking through the parts of code that calculate sub_buffer_rect/damage, there's nothing really restricting the existence of empty damages. And Rect::Union seems pretty tolerant of empty rects.
I do not believe it is helpful to log instances of empty rects.
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #1, Line 135:
crtc_rectClip_in_bounds now mostly works as I expect, except when playing youtube videos. When video is playing:
- Damage Rect is 0x0 with origin at (0,0)
- But display bounds has positive size and origin (ex. 1600x800 at (425, 300))
- Damage rect is not in bounds
I don't know if this is good, bad, or weird. But it'd be good to silence the logging while PSR2 is disabled as I figure things out.
It is also now purely informational since I set fb_damage_clips anyway. I have not noticed any effects either way.
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Miriam Polzer (Gerrit)
Attention is currently required from: Gil Dekel.
1 comment:
Patchset:
Thank you for fixing this!
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye.
1 comment:
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #1, Line 135:
crtc_rect
Clip_in_bounds now mostly works as I expect, except when playing youtube videos. […]
I guess that silencing this for now while figuring out a long term solution is fine.
Thinking out loud here.. If damage is not guaranteed to be present (i.e. could be empty), shouldn't we be passing around an optional instead of an object?
That way we get a clearer signal when a damage is not needed.
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel.
1 comment:
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #1, Line 135:
crtc_rect
I guess that silencing this for now while figuring out a long term solution is fine. […]
Perhaps!
I was thinking about that too. Though using empty rects does give one advantage over optional rects that I can see - it's very easy to get the Union() of an empty rect and another rect. Also an empty rect can be the output of the rect.Intersect() function.
Also there can be empty rects with length on one side. So they still carry some information, even if they're useless from a damage perspective.
It might not be worth switching to optional if IsEmpty() already exists and conveys the same information.
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye.
1 comment:
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #1, Line 135:
crtc_rect
Perhaps! […]
Ok, I see what you mean. And it has merits. But I see the challenges an Optional introduces as a good thing. Tossing around an empty rect could hide issues, and make debugging difficult. We'll always have to doubt or fact check our call-sites (which there are many in Ozone and Viz), whereas an Optional will clearly state whether or not a damage is present and will force code that checks for value before usage (i.e. implementers will be required to think about edge cases.. I hope).
I am not going to insist on this, as this is a compositor decision to make, but I advise towards a more strict marshaling of the damage input.
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel.
Patch set 1:Commit-Queue +1
1 comment:
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #1, Line 135:
crtc_rect
Ok, I see what you mean. And it has merits. […]
I'll open a bug to investigate the issue. I think having an optional could be informative, and the damage starts out as optional in skia_renderer.cc anyway.
b/315355778But converting all the necessary places to accept or return <optional> might take some effort, and even then we may still end up with empty rects that we'll have to handle. For now I can stop the log spam
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME.
Femi Adegunloye would like Mark PMNKHME to review this change.
[psr2]: remove spammy damage log messages
Empty damage clips should not be passed to drm or it might cause
artifacts and black screens, but they don't need to be logged because
it's not an error for chrome to return an empty damage rect.
Also, fix a bug when checking if damage rect is within display bounds.
And restrict the logging to only when PSR2 is enabled.
BUG=b/307353217
TEST=build and deploy to DUT
Change-Id: I6788aeef3b770ec8c544fe8b401d1f2b686146b0
---
M ui/ozone/platform/drm/gpu/drm_gpu_util.cc
M ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc
2 files changed, 8 insertions(+), 10 deletions(-)
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME.
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
3 comments:
Patchset:
Adding Mark since Gil will be OOO until the new year
Sorry, our system marked me OOO before I got a chance to adjust it to when my OOO begins.
I'll be around until Dec. 15th.
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
Patch Set #1, Line 93:
LOG(ERROR) << "Damage rect width must be positive: " << rect.ToString();Were these spammy? I think we can leave these alone..
The main culprit was ln. 137 in `ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc`.
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #1, Line 135:
crtc_rect
I'll open a bug to investigate the issue. […]
Ack. Thanks Femi!
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
1 comment:
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #1, Line 136:
if (!clip_in_bounds && assigned_props_.plane_fb_damage_clips.id) {According to your commit message, you're restricting this to when PSR2 is enabled.
It's not clear to me by the code here that that's what is done here. Can you add a comment that clearly state this?
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
Femi Adegunloye uploaded patch set #2 to this change.
[psr2]: remove spammy damage log messages
Empty damage clips should not be passed to drm or it might cause
artifacts and black screens, but they don't need to be logged because
it's not an error for chrome to return an empty damage rect.
Also, fix a bug when checking if damage rect is within display bounds.
And restrict the logging to only when PSR2 is enabled.
BUG=b/307353217
TEST=build and deploy to DUT
Change-Id: I6788aeef3b770ec8c544fe8b401d1f2b686146b0
---
M ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc
1 file changed, 8 insertions(+), 4 deletions(-)
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME.
Patch set 2:Commit-Queue +1
2 comments:
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
Patch Set #1, Line 93:
LOG(ERROR) << "Damage rect width must be positive: " << rect.ToString();
Were these spammy? I think we can leave these alone.. […]
It's spammy when PSR2 is enabled. But I can leave it alone for now, and resort to a LOG ONCE solution in a later CL perhaps.
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #1, Line 136:
if (!clip_in_bounds && assigned_props_.plane_fb_damage_clips.id) {
According to your commit message, you're restricting this to when PSR2 is enabled. […]
Done
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
Patch set 2:Code-Review +1
2 comments:
Patchset:
LGTM % last comment about the logs in `ui/ozone/platform/drm/gpu/drm_gpu_util.cc`
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
Patch Set #1, Line 93:
LOG(ERROR) << "Damage rect width must be positive: " << rect.ToString();
It's spammy when PSR2 is enabled. […]
Well, I am working off of Miriam's logs provided in the first comment in
https://chromium-review.googlesource.com/c/chromium/src/+/4554495/comment/d03bf878_ee7956c1/I didn't see any of these logs spamming her test logs.
If you are positive these will get spammy to the point they render a log file useless, then remove them. Otherwise we should keep them.
For your discretion.
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Femi Adegunloye, Gil Dekel, Mark PMNKHME.
Femi Adegunloye uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Code-Review+1 by Gil Dekel
[psr2]: remove spammy damage log messages
Empty damage clips should not be passed to drm or it might cause
artifacts and black screens, but they don't need to be logged because
it's not an error for chrome to return an empty damage rect.
Also, fix a bug when checking if damage rect is within display bounds.
And restrict the logging to only when PSR2 is enabled.
BUG=b/307353217
TEST=build and deploy to DUT
Change-Id: I6788aeef3b770ec8c544fe8b401d1f2b686146b0
---
M ui/ozone/platform/drm/gpu/drm_gpu_util.cc
M ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc
2 files changed, 10 insertions(+), 10 deletions(-)
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME.
1 comment:
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
Patch Set #1, Line 93:
LOG(ERROR) << "Damage rect width must be positive: " << rect.ToString();
Well, I am working off of Miriam's logs provided in the first comment in […]
Okay, I added them back in, since they will become spammy when PSR is enabled.
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
3 comments:
Patchset:
Sorry, last two nits - then LGTM.
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
if (rect.width() <= 0) {
return nullptr;
}
if (rect.height() <= 0) {
return nullptr;
}
if (rect.x() < 0) {
return nullptr;
}
if (rect.y() < 0) {
return nullptr;
}nit: since there's now no difference in the outcome of all these cases, they can be consolidated into one `if` statement.
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
// Log an error if the clip is not in bounds. Restrict logging to when PSR2 is
// enabled. (plane_fb_damage_clips has non-zero ID).nit: move above `clip_in_bounds` initialization in ln. 135
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
Femi Adegunloye uploaded patch set #4 to this change.
[psr2]: remove spammy damage log messages
Empty damage clips should not be passed to drm or it might cause
artifacts and black screens, but they don't need to be logged because
it's not an error for chrome to return an empty damage rect.
Also, fix a bug when checking if damage rect is within display bounds.
And restrict the logging to only when PSR2 is enabled.
BUG=b/307353217
TEST=build and deploy to DUT
Change-Id: I6788aeef3b770ec8c544fe8b401d1f2b686146b0
---
M ui/ozone/platform/drm/gpu/drm_gpu_util.cc
M ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc
2 files changed, 11 insertions(+), 20 deletions(-)
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME.
2 comments:
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
if (rect.width() <= 0) {
return nullptr;
}
if (rect.height() <= 0) {
return nullptr;
}
if (rect.x() < 0) {
return nullptr;
}
if (rect.y() < 0) {
return nullptr;
}
nit: since there's now no difference in the outcome of all these cases, they can be consolidated int […]
Done
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
// Log an error if the clip is not in bounds. Restrict logging to when PSR2 is
// enabled. (plane_fb_damage_clips has non-zero ID).
nit: move above `clip_in_bounds` initialization in ln. […]
Done
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Mark PMNKHME (Gerrit)
Attention is currently required from: Femi Adegunloye.
To view, visit change 5063711. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Patch set 4:Commit-Queue +2
findit-for-me@appspot.gserviceaccount.com (Gerrit)
This change will be blocked from submission as there are files which do not meet the coverage criteria. Following files have incremental coverage(all tests) < 70%.
- //ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc
Please add tests for uncovered lines, or add Low-Coverage-Reason:<reason> in the change description to bypass. See https://bit.ly/46jhjS9 to understand when it is okay to bypass. If you think coverage is underreported, file a bug at https://bit.ly/3ENM7Pe
Patch set 4:Code-Coverage -1
The change is no longer submittable: Code-Coverage is unsatisfied now.
Femi Adegunloye (Gerrit)
Femi Adegunloye uploaded new patchset![]( "Open in Gerrit")
- Femi Adegunloye
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Femi Adegunloye (Gerrit)
Femi Adegunloye voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Femi Adegunloye (Gerrit)
Femi Adegunloye removed a vote from this change![]( "Open in Gerrit")
Removed Code-Coverage-1 by findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>
Related details
- Femi Adegunloye
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Femi Adegunloye (Gerrit)
Femi Adegunloye voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
4 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[psr2]: remove spammy damage log messages
Empty damage clips should not be passed to drm or it might cause
artifacts and black screens, but they don't need to be logged because
it's not an error for chrome to return an empty damage rect.
Also, fix a bug when checking if damage rect is within display bounds.
And restrict the logging to only when PSR2 is enabled.
Low-Coverage-Reason: TRIVIAL_CHANGE This CL only affects logging.
BUG=b/307353217
TEST=build and deploy to DUT
- M ui/ozone/platform/drm/gpu/drm_gpu_util.cc
- M ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc
Code-Coverage: +0 by findit...@appspot.gserviceaccount.com
Code-Review: +1 by Mark PMNKHME, +1 by Gil Dekel
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |