ozone/drm: add support for psr2 damage rects [chromium/src : main]
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
Femi Adegunloye would like Mark PMNKHME, Gil Dekel and Peter McNeeley to review this change.
ozone/drm: add support for psr2 damage rects
Pass along damage rects to optional FB_DAMAGE_CLIPS property,
enabling support for PSR-2 on AMD and Intel.
BUG=b:232858482
TEST=build and deploy to volteer delbin
Change-Id: I5b0e66958620242386835319f96a7cd9fd493ae3
---
M components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc
M ui/ozone/platform/drm/common/scoped_drm_types.h
M ui/ozone/platform/drm/gpu/drm_gpu_util.cc
M ui/ozone/platform/drm/gpu/drm_gpu_util.h
M ui/ozone/platform/drm/gpu/drm_overlay_plane.cc
M ui/ozone/platform/drm/gpu/drm_overlay_plane.h
M ui/ozone/platform/drm/gpu/drm_overlay_validator.cc
M ui/ozone/platform/drm/gpu/hardware_display_plane.cc
M ui/ozone/platform/drm/gpu/hardware_display_plane.h
M ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc
M ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc
11 files changed, 68 insertions(+), 7 deletions(-)
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME, Peter McNeeley.
1 comment:
File ui/ozone/platform/drm/gpu/hardware_display_plane.h:
Patch Set #9, Line 59:
ScopedDrmPropertyBlob fb_damage_clips_blob;Why is this public? This feels a bit unorthodox.
Can you have a private member and a set function that can control for input validation and timing (if needed) instead?
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
1 comment:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
Patch Set #9, Line 2288:
damage_rectIf we assign zero it will crash?
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
2 comments:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
Patch Set #9, Line 2288:
damage_rectIf we assign zero it will crash?
The screen will go black on new tab with high probability, then eventually get stuck that way.
I don't remember seeing an actual crash message in the logs though.
File ui/ozone/platform/drm/gpu/hardware_display_plane.h:
Patch Set #9, Line 59:
ScopedDrmPropertyBlob fb_damage_clips_blob;
Why is this public? This feels a bit unorthodox. […]
I moved it to a protected member, and created a setter function. Not sure what input validation would be needed aside from the one already done by drm->CreatePropertyBlob
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME, Peter McNeeley.
1 comment:
File ui/ozone/platform/drm/gpu/hardware_display_plane.h:
Patch Set #9, Line 59:
ScopedDrmPropertyBlob fb_damage_clips_blob;
I moved it to a protected member, and created a setter function. […]
What about the coordinates that are fed into the blob? would it make sense for them to be all 0s? or negative ones? or larger than the overlay's size?
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
1 comment:
File ui/ozone/platform/drm/gpu/hardware_display_plane.h:
Patch Set #9, Line 59:
ScopedDrmPropertyBlob fb_damage_clips_blob;
What about the coordinates that are fed into the blob? would it make sense for them to be all 0s? or […]
That's fair, I was thinking of the blob pointer as kind of opaque but I realize now there is a GetPropertyBlob method in drm.
I'll reupload in a bit with some input checks
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME, Peter McNeeley.
1 comment:
File ui/ozone/platform/drm/gpu/hardware_display_plane.h:
Patch Set #9, Line 59:
ScopedDrmPropertyBlob fb_damage_clips_blob;
That's fair, I was thinking of the blob pointer as kind of opaque but I realize now there is a GetPr […]
These are checks that should be added as DCHECKS if you expect never to run into them, or otherwise have LOG(ERRORS) around them when the input is unexpected.
Also, it implies you need to add unittests around the logic that produces all these damage rects and feed them down to DRM, but I assume you plan to add these in a follow-up CL with this logic.
Also2, why protected and not private? Especially if you expose a set method? In general, it's better to force clients of the API to use set methods so they go through your validation hoops.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
1 comment:
File ui/ozone/platform/drm/gpu/drm_overlay_validator.cc:
Patch Set #10, Line 98:
gfx::Rect(),Shouldnt the default here be the entire plane?
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
File ui/ozone/platform/drm/gpu/drm_overlay_plane.cc:
Patch Set #10, Line 34:
gfx::Point(),I am not sure it is required to do this point/size.
I think you can construct from a size or or cast to a rect.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
for (auto& overlay : overlays_) {
if (frame->sub_buffer_rect &&
!frame->sub_buffer_rect->size().IsZero()) {
overlay.damage_rect = gfx::RectF(*frame->sub_buffer_rect);
}
}This code is actually elsewhere with the exception of the primary plane.
You should not need this code:
https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:components/viz/service/display/overlay_candidate_factory.cc;l=687;drc=568ec8fcdbcff502c6271882630e4da990019e05;bpv=1;bpt=1
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Mark PMNKHME, Peter McNeeley.
1 comment:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
for (auto& overlay : overlays_) {
if (frame->sub_buffer_rect &&
!frame->sub_buffer_rect->size().IsZero()) {
overlay.damage_rect = gfx::RectF(*frame->sub_buffer_rect);
}
}
This code is actually elsewhere with the exception of the primary plane. […]
hm, maybe not. From the logs it seemed like the damage was only ever 1080p without this code.
Maybe things have changed? I'll take a closer look and come back with a clearer answer
I guess my other question is, wouldn't the primary plane still need damage information?
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
4 comments:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
for (auto& overlay : overlays_) {
if (frame->sub_buffer_rect &&
!frame->sub_buffer_rect->size().IsZero()) {
overlay.damage_rect = gfx::RectF(*frame->sub_buffer_rect);
}
}
hm, maybe not. From the logs it seemed like the damage was only ever 1080p without this code. […]
After removing this block of code, the damages actually look a bit better.
(The code seemed to introduce MORE full screen damage, even when nothing was happening).
I removed it. But will also keep an eye on the damages while testing.
File ui/ozone/platform/drm/gpu/drm_overlay_plane.cc:
Patch Set #10, Line 34:
gfx::Point(),
I am not sure it is required to do this point/size. […]
Done
File ui/ozone/platform/drm/gpu/drm_overlay_validator.cc:
Patch Set #10, Line 98:
gfx::Rect(),Shouldnt the default here be the entire plane?
Acknowledged
File ui/ozone/platform/drm/gpu/hardware_display_plane.h:
Patch Set #9, Line 59:
ScopedDrmPropertyBlob fb_damage_clips_blob;
These are checks that should be added as DCHECKS if you expect never to run into them, or otherwise […]
I added the validation/DCHECKS to CreateDCTestBlob because that's where the object is constructed and the ScopedDrmPropertyBlob value should be coming from there.
--
I created fb_damage_clips_blob member variable just so I could get around the fact that AssignPlaneProps didn't already pass a drm pointer. After thinking about it there was no real need to restrict myself in this way.So I've instead updated AssignPlaneProps to additionally accept drm, and damage_clip args. Which was probably always the cleanest/simplest approach.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
Attention is currently required from: Femi Adegunloye, Gil Dekel, Mark PMNKHME.
1 comment:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
if (overlays_.size()) {
TRACE_EVENT1("viz", "SkiaOutputDevice->ScheduleOverlays()",
"num_overlays", overlays_.size());
constexpr base:I tested your change on redrix. The damage coming through there is correct.
I think that we have made a refactor and cleanup such that no change here is required.I could not discover a positive power result on redrix so perhaps redrix doesnt support this? Is this feature hidden behind a flag that is not present in this cl?
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
Attention is currently required from: Femi Adegunloye, Gil Dekel, Mark PMNKHME.
1 comment:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
TRACE_EVENT1("viz", "SkiaOutputDevice->ScheduleOverlays()",
"num_overlays", overlays_.size());
You can add these here for debugging
for(auto& each: overlays_){
DBG_DRAW_RECT("output.overlay.rect", each.display_rect);
DBG_DRAW_RECT("output.overlay.damage", each.damage_rect);
}
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
1 comment:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
if (overlays_.size()) {
TRACE_EVENT1("viz", "SkiaOutputDevice->ScheduleOverlays()",
"num_overlays", overlays_.size());
constexpr base:
I tested your change on redrix. The damage coming through there is correct. […]
I haven't noticed any difference on Brya either. I'll check again on the volteer. The Brya was on kernel 5.15 and the volteer is 5.4. What version is your redrix on?
The feature should not be hidden behind any other flag than #panel-self-refresh-2.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
Attention is currently required from: Femi Adegunloye, Gil Dekel, Mark PMNKHME.
1 comment:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
if (overlays_.size()) {
TRACE_EVENT1("viz", "SkiaOutputDevice->ScheduleOverlays()",
"num_overlays", overlays_.size());
constexpr base:
I haven't noticed any difference on Brya either. I'll check again on the volteer. […]
I might have notice a difference for non overlay (aka only plane damage)
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
1 comment:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
if (overlays_.size()) {
TRACE_EVENT1("viz", "SkiaOutputDevice->ScheduleOverlays()",
"num_overlays", overlays_.size());
constexpr base:
I might have notice a difference for non overlay (aka only plane damage)
Hm, I guess it makes sense that overlays might be harder to do a partial swap for.
But won't a lot of non-video content be composited anyway? Like browsing email or viewing that ticking clock website.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
1 comment:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
TRACE_EVENT1("viz", "SkiaOutputDevice->ScheduleOverlays()",
"num_overlays", overlays_.size());
You can add these here for debugging […]
Done
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
1 comment:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
if (overlays_.size()) {
TRACE_EVENT1("viz", "SkiaOutputDevice->ScheduleOverlays()",
"num_overlays", overlays_.size());
constexpr base:
I might have notice a difference for non overlay (aka only plane damage)
This test page does 60fps damage locally to the plane with no other overlay
https://codepen.io/LindsayMac/full/MwrONL
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
1 comment:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
if (overlays_.size()) {
TRACE_EVENT1("viz", "SkiaOutputDevice->ScheduleOverlays()",
"num_overlays", overlays_.size());
constexpr base:
This test page does 60fps damage locally to the plane with no other overlay […]
Apparently for brya devices, psr needs to be enabled in the kernel.
With i915.enable_psr=2
https://chromium.googlesource.com/chromiumos/docs/+/HEAD/kernel_development.md#Modifying-the-kernel-command-lineHaven't done this myself yet, so don't know the exact process.
And it should be possible to leave some debug messages too:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/display/intel_psr.c#L376Gonna give it a try on my osiris.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME, Peter McNeeley.
2 comments:
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
DCHECK((x2 - x1) > 0 && (y2 - y1) > 0);
DCHECK(x1 >= 0 && x2 >= 0 && y1 >= 0 && y2 >= 0);Two things:
1) I'd recommend each condition has its own DCHECK. That way if anything breaks, you'd know exactly which coordinate is the culprit.
2) DCHECKs help during development and testing, but these will fail silently in production. Is that acceptable? In other words, do we expect clients of `CreateDCBlob()` to never break these rules via input validation around the call-sites? Because otherwise these should be `if` statements with error messages.
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #12, Line 101:
DrmDevice* drm,This should be s refcount ptr
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
3 comments:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
if (overlays_.size()) {
TRACE_EVENT1("viz", "SkiaOutputDevice->ScheduleOverlays()",
"num_overlays", overlays_.size());
constexpr base:
Apparently for brya devices, psr needs to be enabled in the kernel. […]
I can confirm from logging intel_psr.c that brya osiris with 5.15 kernel does not support PSR2,
But my volteer drobit with 5.4 kernel does.
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
DCHECK((x2 - x1) > 0 && (y2 - y1) > 0);
DCHECK(x1 >= 0 && x2 >= 0 && y1 >= 0 && y2 >= 0);
Two things: […]
Yeah, probably better to have these as if statements then.
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #12, Line 101:
DrmDevice* drm,This should be s refcount ptr
How come? the passed in drm isn't a Scoped_Refptr (it is raw_ptr<DrmDevice, ExperimentalAsh>) and the variable is not stored for use outside of this function
https://www.chromium.org/developers/smart-pointer-guidelines/ I also get the impression that scoped_refptr should generally be avoided
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME, Peter McNeeley.
6 comments:
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
Patch Set #13, Line 95:
LOG(ERROR) << "Damage rect is empty: " << x2 << " - " << x1 << " <= 0";nit: consider replacing with:
```
LOG(ERROR) << "Damage rect width is invalid: x2=" << x2 << " - x1=" << x1 << " = " << x2 - x1;
```Patch Set #13, Line 99:
LOG(ERROR) << "Damage rect is empty: " << y2 << " - " << y1 << " <= 0";ditto
Copy-paste got ya.
s/x1/y1
x2
y2
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #12, Line 101:
DrmDevice* drm,
How come? the passed in drm isn't a Scoped_Refptr (it is raw_ptr<DrmDevice, ExperimentalAsh>) and th […]
Well, my concern was that the DRM instance may be destroyed while this is being executed.. but I took a closer look at the code and `HardwareDisplayPlaneAtomic` seems to be handled by `HardwareDisplayPlaneManagerAtomic`, which is owned by `DrmDevice`[[1]](https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:ui/ozone/platform/drm/gpu/drm_device.h;drc=df097ef9b722789243248f807ad66c7085a14099;bpv=0;bpt=1;l=85). So, you're right. This should be safe.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
5 comments:
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
Patch Set #13, Line 95:
LOG(ERROR) << "Damage rect is empty: " << x2 << " - " << x1 << " <= 0";
nit: consider replacing with: […]
Done
Patch Set #13, Line 99:
LOG(ERROR) << "Damage rect is empty: " << y2 << " - " << y1 << " <= 0";ditto
Done
Copy-paste got ya. […]
Always gotta be careful with the copy-paste T-T. Thanks!
Done
Done
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
Attention is currently required from: Femi Adegunloye, Gil Dekel, Mark PMNKHME.
1 comment:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
if (overlays_.size()) {
TRACE_EVENT1("viz", "SkiaOutputDevice->ScheduleOverlays()",
"num_overlays", overlays_.size());
constexpr base:
I can confirm from logging intel_psr.c that brya osiris with 5.15 kernel does not support PSR2, […]
Any notes on power differences?
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
1 comment:
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
if (overlays_.size()) {
TRACE_EVENT1("viz", "SkiaOutputDevice->ScheduleOverlays()",
"num_overlays", overlays_.size());
constexpr base:
Any notes on power differences?
I'll need to crunch the numbers on turbostat, but just from looking it over the PkgWatt was twice as likely to be over 3.00. It would also dip below 2.8 when ON but never when OFF.
-CorWatt and GFXWatt are a bit harder to read at a glance, but my impression is there is no difference with PSR on/off.Battery_sample.py: I think it's a bit easier to say PSR is saving power here.
I just did a quick run. for 250 samples at 1 second each.
PSR On - median =4.7003845000000002
PSR Off - median =5.5256259999999999
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME, Peter McNeeley.
2 comments:
Patchset:
Last comment. Everything else LGTM. Will +1 once it's resolved.
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
Patch Set #14, Line 89:
int x1, int y1, int x2, int y2Thinking about this further.. is there a reason why we don't pass a `const gfx::Rect&` here, given that you have one available at the call-site?
It will simplify the call-sites and provide validation API: `IsEmpty()`, `width()`, `height()`, and `ToString()` that can help with all these checks (IIUC, width/height would be negative if the x's and y's were flipped).
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
1 comment:
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
Patch Set #14, Line 89:
int x1, int y1, int x2, int y2
Thinking about this further.. […]
You're right. I'm not really sure why I was hesitant to use the gfx::Rect directly, but I switched things over and it made things cleaner.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
2 comments:
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
I removed IsEmpty since it just checks width/height != 0.
removed rect.bottom() and rect.right() checks because they are impossible to hit if width/height, x/y are already checked.
changed to only print rect.ToString()
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #17, Line 138:
auto clip_in_bounds = src_rect.Contains(damage_rect);Added a check here that the damage clip is contained within the source plane.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME, Peter McNeeley.
Patch set 17:Code-Review +1
4 comments:
Patchset:
LGTM % last comments.
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #17, Line 138:
auto clip_in_bounds = src_rect.Contains(damage_rect);Added a check here that the damage clip is contained within the source plane.
Thanks!
`const bool` - no need to leave the reader guessing what type is returned here.
So damage rects that do not have an id or are outside of the plane are just ignored? Is that ok? no need to log it somehow?
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Mark PMNKHME, Peter McNeeley.
2 comments:
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
`const bool` - no need to leave the reader guessing what type is returned here.
Done
So damage rects that do not have an id or are outside of the plane are just ignored? Is that ok? no […]
I added logging for out of bounds damage rects, but missing ID only means the feature isn't enabled or supported. It's not an error.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME, Peter McNeeley.
Patch set 18:Code-Review +1
1 comment:
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #18, Line 140:
LOG(ERROR) << "Damage clip not contained inside source plane";Sorry, I know I suggested logging, but let's be mindful here. If this is a likely/common case, this log will become very spammy, and we should avoid that.
Depending on the likelihood of this case, you should consider changing this to `VLOG(1|2)` instead. But if this should never happen, `ERROR` is good here.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Mark PMNKHME, Peter McNeeley.
1 comment:
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #18, Line 140:
LOG(ERROR) << "Damage clip not contained inside source plane";
Sorry, I know I suggested logging, but let's be mindful here. […]
I don't expect this to be hit ever unless there's a bug in damage tracking. The damaged area should represent a region inside of the screen, and I've also never seen it occur in testing.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
Patch set 18:-Code-Review
The change is no longer submittable: Code-Owners is unsatisfied now.
Peter McNeeley (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Mark PMNKHME (Gerrit)
Attention is currently required from: Femi Adegunloye.
4 comments:
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
ScopedDrmModeRectPtr dmg_rect(
static_cast<drm_mode_rect*>(malloc(sizeof(drm_mode_rect))));opt: personal opinion: allocate this just before it's used on L110
Patch Set #18, Line 99:
positienit: positive
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
LOG_IF(ERROR, properties_.plane_fb_damage_clips.id == 0)
<< "FB Damage Clips Property not found";is this a required property that we have to report everytime if not found?
File ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc:
Patch Set #18, Line 339:
nullptrwhy aren't you passing drm_ here?
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
File ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc:
Patch Set #18, Line 339:
nullptrwhy aren't you passing drm_ here?
+1
That's a very good point. Even if the intention here is to get some sort of an "empty state", `drm` is used without checking it's valid in `AssignPlaneProps()`...
So you either have to pass it here, or add guards against a null `drm` within `AssignPlaneProps()` before using it.
I wonder if a better approach here would be to implement something like `AssignEmptyPlaneProps()` that does that internally since we're doing it both here and in ln 229..
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Mark PMNKHME.
4 comments:
File ui/ozone/platform/drm/gpu/drm_gpu_util.cc:
ScopedDrmModeRectPtr dmg_rect(
static_cast<drm_mode_rect*>(malloc(sizeof(drm_mode_rect))));opt: personal opinion: allocate this just before it's used on L110
Done
Patch Set #18, Line 99:
positienit: positive
Done
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
LOG_IF(ERROR, properties_.plane_fb_damage_clips.id == 0)
<< "FB Damage Clips Property not found";is this a required property that we have to report everytime if not found?
It's an optional property, so I guess it doesn't need to be reported. It's definitely not an error if it's missing. I removed the log statement.
File ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc:
Patch Set #18, Line 339:
nullptr
+1 […]
Yeah a drm ptr check is a straightforward fix since the function's not actually needed in the empty state.
I'm not sure an "AssignEmptyPlaneProps()" would be anything more than duplicate code or a helper function that calls AssignPlaneProps() with empty values anyway, so I think the drm ptr check is the cleanest solution.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye, Mark PMNKHME.
Patch set 19:-Code-Review
2 comments:
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
nit: `drm` should be checked first ([short-circuit](https://en.wikipedia.org/wiki/Short-circuit_evaluation)), since nothing else matters if it's invalid.
File ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc:
Patch Set #18, Line 339:
nullptr
Yeah a drm ptr check is a straightforward fix since the function's not actually needed in the empty […]
I'd say that a `ClearPlaneProps()` utility function may be outside the scope of your work, but its purpose is to actually _reduce_ duplicate code (since we're doing it in at least two call-sites in `ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc`; lns. 229 and 339), and ensure that the planes are cleared within the plane safely and on its own terms. The issue with `AssignPlaneProps()` is that it is designed to check and work on valid inputs, and may end up breaking if invalid input is provided. It's hard to catch all edge-cases..
To view, visit change 4554495. 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/hardware_display_plane_atomic.cc:
nit: `drm` should be checked first ([short-circuit](https://en.wikipedia. […]
Sure I moved it, but short-circuit is more about avoiding side effects of evaluation. None of the three expressions have side effects, and can evaluate false independently so I don't know that there's any real advantage except for having a structurally logical ordering.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Mark PMNKHME (Gerrit)
Attention is currently required from: Femi Adegunloye, Gil Dekel.
Patch set 20:Code-Review +1
1 comment:
Patchset:
lgtm % approval of functionalities of others
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
Attention is currently required from: Femi Adegunloye, Gil Dekel.
Patch set 20:Code-Review +1
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel.
Patch set 20:Commit-Queue +2
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
Patch set 21:Commit-Queue +1
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel, Mark PMNKHME, Peter McNeeley.
Mark can approve the changes to drm
Acknowledged
Patchset:
Hi, can everyone reapprove.
I updated the unit tests to compile with the new function defs.I will add damage clip specific tests in the next CL
File components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:
if (overlays_.size()) {
TRACE_EVENT1("viz", "SkiaOutputDevice->ScheduleOverlays()",
"num_overlays", overlays_.size());
constexpr base:
I'll need to crunch the numbers on turbostat, but just from looking it over the PkgWatt was twice as […]
Done
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Peter McNeeley (Gerrit)
Attention is currently required from: Femi Adegunloye, Gil Dekel, Mark PMNKHME.
Patch set 21:Code-Review +1
Mark PMNKHME (Gerrit)
Attention is currently required from: Femi Adegunloye, Gil Dekel.
Patch set 21:Code-Review +1
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel.
Patch set 21:Commit-Queue +2
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye.
1 comment:
Patchset:
I am curious.. why are the unit tests not added here?
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Attention is currently required from: Gil Dekel.
1 comment:
Patchset:
I am curious.. […]
I plan to add them in the next CL. This code is currently disabled and I don't plan on enabling it until after unit tests have been added and I've run a finch trial.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye.
Patch set 21:Code-Review +1
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
ozone/drm: add support for psr2 damage rects
Pass along damage rects to optional FB_DAMAGE_CLIPS property,
enabling support for PSR-2 on AMD and Intel.
BUG=b:232858482
TEST=build and deploy to volteer delbin
Change-Id: I5b0e66958620242386835319f96a7cd9fd493ae3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4554495
Reviewed-by: Peter McNeeley <peterm...@chromium.org>
Commit-Queue: Femi Adegunloye <mrf...@google.com>
Reviewed-by: Gil Dekel <gild...@chromium.org>
Reviewed-by: Mark PMNKHME <marky...@google.com>
Cr-Commit-Position: refs/heads/main@{#1186051}
---
M components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc
M ui/ozone/platform/drm/common/scoped_drm_types.h
M ui/ozone/platform/drm/gpu/drm_gpu_util.cc
M ui/ozone/platform/drm/gpu/drm_gpu_util.h
M ui/ozone/platform/drm/gpu/drm_overlay_plane.cc
M ui/ozone/platform/drm/gpu/drm_overlay_plane.h
M ui/ozone/platform/drm/gpu/drm_overlay_validator.cc
M ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc
M ui/ozone/platform/drm/gpu/hardware_display_controller_unittest.cc
M ui/ozone/platform/drm/gpu/hardware_display_plane.cc
M ui/ozone/platform/drm/gpu/hardware_display_plane.h
M ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc
M ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.h
M ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc
M ui/ozone/platform/drm/gpu/hardware_display_plane_manager_unittest.cc
15 files changed, 129 insertions(+), 46 deletions(-)
Miriam Polzer (Gerrit)
Attention is currently required from: Femi Adegunloye.
1 comment:
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #22, Line 137:
LOG(ERROR) << "Damage clip not contained inside source plane";This is clogging logs in one of my test runs:https://cros-test-analytics.appspot.com/p/chromeos/file/view/invocations/u-chromeos-test-2023-09-20-18-57-00-2fb79a1536677680/?file=cros-test%2Fartifact%2Ftauto%2Fresults-1-tast.medium-low-tier-shard-3%2Ftast%2Fresults%2Ftests%2Frollback.EnterpriseRollbackEnrolled%2Fchrome-2023-09-20T18%3A53%3A55Z%2Fchrome_20230920-184816&test=tast.rollback.EnterpriseRollbackEnrolled&accountId=1
Any chance we can emit it a bit less often? Or is this such a huge error that it's fine and there's something seriously broken here?
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #22, Line 137:
LOG(ERROR) << "Damage clip not contained inside source plane";
This is clogging logs in one of my test runs:https://cros-test-analytics.appspot. […]
We discussed this possibility here: https://chromium-review.googlesource.com/c/chromium/src/+/4554495/comment/7cda03d6_858d8369/
According to mrfemi@'s respond, it seems like there's something wrong going on with the test, or worse..
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Femi Adegunloye (Gerrit)
Patch Set #22, Line 137:
LOG(ERROR) << "Damage clip not contained inside source plane";
We discussed this possibility here: https://chromium-review.googlesource. […]
I'm guessing this is a tast test or unit test? I can understand why damage might be incorrect if it's not the priority for the test, but I think out of bounds damage would be a decent indicator of a bug in real environments.
I did take a look at the KMS documentation on fb_damage_clips though, and it does sound like it will just ignore out of bounds clips:
https://docs.kernel.org/gpu/drm-kms.html#c.drm_atomic_helper_damage_iter_next
"This iterator will skip damage clips outside of plane src."@Gil, one option here is that I limit logging to only when plane_fb_damage_clips is being used. Another one is that I don't log, or switch to VLOG like you originally suggested in your comment.
@Miriam, is there any chance you test can be modified to send a damage without bounds, or empty damage?
Actually now that I think about it, maybe the test has an empty src_rect, and that's why clip_in_bounds is returning false? That could be a test specific issue where empty src_rect should be ignored.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Gil Dekel (Gerrit)
Attention is currently required from: Femi Adegunloye.
Patch Set #22, Line 137:
LOG(ERROR) << "Damage clip not contained inside source plane";
Well, you're introducing a new behavior to Chrome - so it's up to you to define user-behavior and logging around this new behavior.
>I did take a look at the KMS documentation on fb_damage_clips though, and it does sound like it will just ignore out of bounds clips:
It's good that KMS handles/ignores misbehaving damages, but it doesn't change the fact that we may still have a failure in Chrome.
If we should never have out of bounds damage clips, then an ERROR or WARNING log is appropriate and will help catching all current and future mishandling of damage clips (and also debugging). Whether or not we should emit one for every occurrence is up to how critical or abnormal mishandling damage rects is and the amount of useful information we get from each log. Since we're not printing any variables here, like plane size vs. damage size, the current information value is very low.
If all users must handle damage rects one way or another, then the test should be fixed to set up and define a damage rect correctly, and so is any other future discoveries of damage rect mishandling due to this ERROR.
If performance is going to suffer due to Chrome not handling damage clips correctly, thus disabling psr2 or under-utilizing it, then having these logs could prove important.
Another one is that I don't log, or switch to VLOG like you originally suggested in your comment.
Thinking about this further, I don't think moving it to VLOG is better, since we'll be potentially hiding a crucial error signal here, and/or spam the logs when VLOG is enabled.
My suggestion is:
1) Fix any and all tests to do the right/trivial thing around damage rects (might want to run all ozone/display unittests and look for all cases in which you spam this log.
2) Modify the log to provide more useful information (plane vs. dmg size?, maybe frame ID if that's a thing?)
3) If possible, emit a log only if it's different from the previous one (i.e. ID and damage are different?)
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Miriam Polzer (Gerrit)
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #22, Line 137:
LOG(ERROR) << "Damage clip not contained inside source plane";
Well, you're introducing a new behavior to Chrome - so it's up to you to define user-behavior and lo […]
My test only runs through OOBE, just as any user does. It doesn't create or see anything a prod user doesn't see.
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.
Brian Ho (Gerrit)
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:
Patch Set #22, Line 137:
LOG(ERROR) << "Damage clip not contained inside source plane";
My test only runs through OOBE, just as any user does. […]
+1 here
I ran into this when moving a window with a video partially offscreen, and this error gets logged once per frame. Nothing else seems broken, but it makes debugging crashes and local development kinda annoying. Can we suppress these logs or challenge the assumption that this is an error state?
To view, visit change 4554495. To unsubscribe, or for help writing mail filters, visit settings.