Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2254103002:
ozone: Allow full screen overlay optimization for BGR formats. (Closed)
|
Created:
4 years, 4 months ago by Daniele Castagna Modified:
4 years, 4 months ago CC:
chromium-reviews, ozone-reviews_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionozone: Allow full screen overlay optimization for BGR formats.
Allow full screen overlay optimization for buffers coming via
wayland (typically BGR).
Note: this CL could cause the format of the primary plane framebuffer
used by drmModePageFlip to change (RGB<->BGR), this currently works
on rockchip with a 3.14 kernel and won't work with newer kernels.
This optimization will be initially enabled only those devices.
The long term plan is to enable the optimization only when the overlay
plane matches the format of the primary plane, and to switch the
primary plane format to BGR (crbug.com/639111).
BUG=b/29430506
Committed: https://crrev.com/58f021db4c3e656bd88b23b907b5675f7f55fd48
Cr-Commit-Position: refs/heads/master@{#412960}
Patch Set 1 #Patch Set 2 : Allow both RGB and BGR. #Patch Set 3 : Remove "only" in the comment. #
Total comments: 2
Messages
Total messages: 33 (18 generated)
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== ozone: Single full screen overlay optimization for BGR formats. This optimization is for buffers coming from wayland (typically BGR), while cros primary plane format is currently RGB. This CL changes the format of the primary plane framebuffer used by drmModePageFlip, this currently works only on rockchip with a 3.14 kernel. This optimization will be initially enabled only those devices. The long term plan is to enable the optimization only when the overlay plane matches the format of the primary plane, and to switch the primary plane format to BGR. BUG=b/29430506 ========== to ========== ozone: Single full screen overlay optimization for BGR formats. This optimization is for buffers coming via wayland (typically BGR), while cros primary plane format is currently RGB. This CL changes the format of the primary plane framebuffer used by drmModePageFlip, this currently works only on rockchip with a 3.14 kernel. This optimization will be initially enabled only those devices. The long term plan is to enable the optimization only when the overlay plane matches the format of the primary plane, and to switch the primary plane format to BGR. BUG=b/29430506 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== ozone: Single full screen overlay optimization for BGR formats. This optimization is for buffers coming via wayland (typically BGR), while cros primary plane format is currently RGB. This CL changes the format of the primary plane framebuffer used by drmModePageFlip, this currently works only on rockchip with a 3.14 kernel. This optimization will be initially enabled only those devices. The long term plan is to enable the optimization only when the overlay plane matches the format of the primary plane, and to switch the primary plane format to BGR. BUG=b/29430506 ========== to ========== ozone: Allow full screen overlay optimization for BGR formats. Allow full screen overlay optimization for buffers coming via wayland (typically BGR). Note: this CL could cause the format of the primary plane framebuffer used by drmModePageFlip to change (RGB<->BGR), this currently works only on rockchip with a 3.14 kernel. This optimization will be initially enabled only those devices. The long term plan is to enable the optimization only when the overlay plane matches the format of the primary plane, and to switch the primary plane format to BGR. BUG=b/29430506 ==========
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== ozone: Allow full screen overlay optimization for BGR formats. Allow full screen overlay optimization for buffers coming via wayland (typically BGR). Note: this CL could cause the format of the primary plane framebuffer used by drmModePageFlip to change (RGB<->BGR), this currently works only on rockchip with a 3.14 kernel. This optimization will be initially enabled only those devices. The long term plan is to enable the optimization only when the overlay plane matches the format of the primary plane, and to switch the primary plane format to BGR. BUG=b/29430506 ========== to ========== ozone: Allow full screen overlay optimization for BGR formats. Allow full screen overlay optimization for buffers coming via wayland (typically BGR). Note: this CL could cause the format of the primary plane framebuffer used by drmModePageFlip to change (RGB<->BGR), this currently works on rockchip with a 3.14 kernel and won't work with newer kernels. This optimization will be initially enabled only those devices. The long term plan is to enable the optimization only when the overlay plane matches the format of the primary plane, and to switch the primary plane format to BGR. BUG=b/29430506 ==========
dcastagna@chromium.org changed reviewers: + dnicoara@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
How do you guarantee that this is only used on rockchip?
On 2016/08/18 at 13:55:53, dnicoara wrote: > How do you guarantee that this is only used on rockchip? Currently overlays are not enabled on any ozone drm device, so we never receive more than on plane in hardware_display_plane_manager.cc and this optimization is actually never used. The plan is to start enabling fullscreen overlay only on minnie with this CL: https://chromium-review.googlesource.com/c/372402/
On 2016/08/18 17:12:26, Daniele Castagna wrote: > On 2016/08/18 at 13:55:53, dnicoara wrote: > > How do you guarantee that this is only used on rockchip? > > Currently overlays are not enabled on any ozone drm device, so we never receive > more than on plane in hardware_display_plane_manager.cc and this optimization is > actually never used. We have an enable-overlay flag (or something similar) which would actually enable overlay path and thats how this code was tested. > The plan is to start enabling fullscreen overlay only on minnie with this CL: > https://chromium-review.googlesource.com/c/372402/ Any reason why only on Minnie ? Idea was to keep this as hardware agnostic as possible.
On 2016/08/18 at 17:16:04, kalyan.kondapally wrote: > On 2016/08/18 17:12:26, Daniele Castagna wrote: > > On 2016/08/18 at 13:55:53, dnicoara wrote: > > > How do you guarantee that this is only used on rockchip? > > > > Currently overlays are not enabled on any ozone drm device, so we never receive > > more than on plane in hardware_display_plane_manager.cc and this optimization is > > actually never used. > > We have an enable-overlay flag (or something similar) which would actually enable > overlay path and thats how this code was tested. > That's the flag we're adding in https://chromium-review.googlesource.com/c/372402/ > > The plan is to start enabling fullscreen overlay only on minnie with this CL: > > https://chromium-review.googlesource.com/c/372402/ > > Any reason why only on Minnie ? Idea was to keep this as hardware agnostic as > possible. Currently the primary plane of a CRTC on intel can be only *RGB and not *BGR. https://chromium-review.googlesource.com/c/366910/ will change that.
kalyan.kondapally@intel.com changed reviewers: + kalyan.kondapally@intel.com
https://codereview.chromium.org/2254103002/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/hardware_display_plane_manager.cc (right): https://codereview.chromium.org/2254103002/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/hardware_display_plane_manager.cc:273: // plane currently works on rockchip with 3.14 kernel and won't work with Ideally what we want is if Kernel/hardware can swap the formats, than Kernel should report as the supported format for the primary plane. Making this assumptions here is something we should really avoid. That way we should be able to enable this for all Ozone platforms.
On 2016/08/18 17:12:26, Daniele Castagna wrote: > On 2016/08/18 at 13:55:53, dnicoara wrote: > > How do you guarantee that this is only used on rockchip? > > Currently overlays are not enabled on any ozone drm device, so we never receive > more than on plane in hardware_display_plane_manager.cc and this optimization is > actually never used. > > The plan is to start enabling fullscreen overlay only on minnie with this CL: > https://chromium-review.googlesource.com/c/372402/ Thanks, just wanted to make sure this wasn't enabled by default everywhere! Sorry, one more thing: you were looking at changing the default color from RGB to BGR, did that not pan out?
Also, what are the limitation past kernel 3.14 for minnie that won't make this work? What about other systems we support? Past the hardware support for the format, we may need to worry about buffer layouts being different. As a more robust suggestion, you could query the list of supported formats for the primary plane and replace the if-statement with a more accurate check based on actual hardware support.
On 2016/08/18 at 17:26:39, dnicoara wrote: > On 2016/08/18 17:12:26, Daniele Castagna wrote: > > On 2016/08/18 at 13:55:53, dnicoara wrote: > > > How do you guarantee that this is only used on rockchip? > > > > Currently overlays are not enabled on any ozone drm device, so we never receive > > more than on plane in hardware_display_plane_manager.cc and this optimization is > > actually never used. > > > > The plan is to start enabling fullscreen overlay only on minnie with this CL: > > https://chromium-review.googlesource.com/c/372402/ > > Thanks, just wanted to make sure this wasn't enabled by default everywhere! > > Sorry, one more thing: you were looking at changing the default color from RGB to BGR, did that not pan out? That's still the plan. On the Chrome side it is just a matter of changing the format in one place. Before we can do that we need to add BGR to rockchip minigbm https://chromium-review.googlesource.com/c/366900/ and BGR to intel drm: https://chromium-review.googlesource.com/c/366910/ We'd like to start enabling this on minnie to start exercising the code, and because it saves ~6 ms GPU time when we can hit this optimization. On 2016/08/18 at 17:33:59, dnicoara wrote: > Also, what are the limitation past kernel 3.14 for minnie that won't make this work? What about other systems we support? Past the hardware support for the format, we may need to worry about buffer layouts being different. > Past kernel 3.14 drmModePageFlip checks that the format of the primary framebuffer stays the same: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.... line 5370 > As a more robust suggestion, you could query the list of supported formats for the primary plane and replace the if-statement with a more accurate check based on actual hardware support. That sounds good, even if practically we'd be just checking against the hard-coded list we're changing in https://chromium-review.googlesource.com/c/366910/. I'd still prefer to leave this ugly hack with a comment that says to remove it ASAP, and we already have a plan for that, instead of having a more robust solution that still isn't correct. I'm happy to change it as you suggested if you prefer that way though.
On 2016/08/18 20:21:14, Daniele Castagna wrote: > On 2016/08/18 at 17:26:39, dnicoara wrote: > > On 2016/08/18 17:12:26, Daniele Castagna wrote: > > > On 2016/08/18 at 13:55:53, dnicoara wrote: > > > > How do you guarantee that this is only used on rockchip? > > > > > > Currently overlays are not enabled on any ozone drm device, so we never > receive > > > more than on plane in hardware_display_plane_manager.cc and this > optimization is > > > actually never used. > > > > > > The plan is to start enabling fullscreen overlay only on minnie with this > CL: > > > https://chromium-review.googlesource.com/c/372402/ > > > > Thanks, just wanted to make sure this wasn't enabled by default everywhere! > > > > Sorry, one more thing: you were looking at changing the default color from RGB > to BGR, did that not pan out? > > That's still the plan. > On the Chrome side it is just a matter of changing the format in one place. > Before we can do that we need to add BGR to rockchip minigbm > https://chromium-review.googlesource.com/c/366900/ and BGR to intel drm: > https://chromium-review.googlesource.com/c/366910/ > > We'd like to start enabling this on minnie to start exercising the code, and > because it saves ~6 ms GPU time when we can hit this optimization. > > > On 2016/08/18 at 17:33:59, dnicoara wrote: > > Also, what are the limitation past kernel 3.14 for minnie that won't make this > work? What about other systems we support? Past the hardware support for the > format, we may need to worry about buffer layouts being different. > > > > Past kernel 3.14 drmModePageFlip checks that the format of the primary > framebuffer stays the same: > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.... > line 5370 > Thanks for the reference. I was expecting that considering that color formats may not be interchangeable. > > As a more robust suggestion, you could query the list of supported formats for > the primary plane and replace the if-statement with a more accurate check based > on actual hardware support. > > That sounds good, even if practically we'd be just checking against the > hard-coded list we're changing in > https://chromium-review.googlesource.com/c/366910/. > I'd still prefer to leave this ugly hack with a comment that says to remove it > ASAP, and we already have a plan for that, instead of having a more robust > solution that still isn't correct. I'm happy to change it as you suggested if > you prefer that way though. lgtm with nit ... Could you please file a bug for this?
On 2016/08/18 at 21:46:09, dnicoara wrote: > On 2016/08/18 20:21:14, Daniele Castagna wrote: > > On 2016/08/18 at 17:26:39, dnicoara wrote: > > > On 2016/08/18 17:12:26, Daniele Castagna wrote: > > > > On 2016/08/18 at 13:55:53, dnicoara wrote: > > > > > How do you guarantee that this is only used on rockchip? > > > > > > > > Currently overlays are not enabled on any ozone drm device, so we never > > receive > > > > more than on plane in hardware_display_plane_manager.cc and this > > optimization is > > > > actually never used. > > > > > > > > The plan is to start enabling fullscreen overlay only on minnie with this > > CL: > > > > https://chromium-review.googlesource.com/c/372402/ > > > > > > Thanks, just wanted to make sure this wasn't enabled by default everywhere! > > > > > > Sorry, one more thing: you were looking at changing the default color from RGB > > to BGR, did that not pan out? > > > > That's still the plan. > > On the Chrome side it is just a matter of changing the format in one place. > > Before we can do that we need to add BGR to rockchip minigbm > > https://chromium-review.googlesource.com/c/366900/ and BGR to intel drm: > > https://chromium-review.googlesource.com/c/366910/ > > > > We'd like to start enabling this on minnie to start exercising the code, and > > because it saves ~6 ms GPU time when we can hit this optimization. > > > > > > On 2016/08/18 at 17:33:59, dnicoara wrote: > > > Also, what are the limitation past kernel 3.14 for minnie that won't make this > > work? What about other systems we support? Past the hardware support for the > > format, we may need to worry about buffer layouts being different. > > > > > > > Past kernel 3.14 drmModePageFlip checks that the format of the primary > > framebuffer stays the same: > > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.... > > line 5370 > > > > Thanks for the reference. I was expecting that considering that color formats may not be interchangeable. > > > > As a more robust suggestion, you could query the list of supported formats for > > the primary plane and replace the if-statement with a more accurate check based > > on actual hardware support. > > > > That sounds good, even if practically we'd be just checking against the > > hard-coded list we're changing in > > https://chromium-review.googlesource.com/c/366910/. > > I'd still prefer to leave this ugly hack with a comment that says to remove it > > ASAP, and we already have a plan for that, instead of having a more robust > > solution that still isn't correct. I'm happy to change it as you suggested if > > you prefer that way though. > > lgtm with nit ... Could you please file a bug for this? crbug.com/639111
Description was changed from ========== ozone: Allow full screen overlay optimization for BGR formats. Allow full screen overlay optimization for buffers coming via wayland (typically BGR). Note: this CL could cause the format of the primary plane framebuffer used by drmModePageFlip to change (RGB<->BGR), this currently works on rockchip with a 3.14 kernel and won't work with newer kernels. This optimization will be initially enabled only those devices. The long term plan is to enable the optimization only when the overlay plane matches the format of the primary plane, and to switch the primary plane format to BGR. BUG=b/29430506 ========== to ========== ozone: Allow full screen overlay optimization for BGR formats. Allow full screen overlay optimization for buffers coming via wayland (typically BGR). Note: this CL could cause the format of the primary plane framebuffer used by drmModePageFlip to change (RGB<->BGR), this currently works on rockchip with a 3.14 kernel and won't work with newer kernels. This optimization will be initially enabled only those devices. The long term plan is to enable the optimization only when the overlay plane matches the format of the primary plane, and to switch the primary plane format to BGR (crbug.com/639111). BUG=b/29430506 ==========
https://codereview.chromium.org/2254103002/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/hardware_display_plane_manager.cc (right): https://codereview.chromium.org/2254103002/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/hardware_display_plane_manager.cc:273: // plane currently works on rockchip with 3.14 kernel and won't work with On 2016/08/18 at 17:23:10, kalyank wrote: > Ideally what we want is if Kernel/hardware can swap the formats, than Kernel should report as the supported format for the primary plane. Making this assumptions here is something we should really avoid. That way we should be able to enable this for all Ozone platforms. AFAIK there is no interface from the kernel to know if we can swap between different formats. The kernel currently reports that on Intel we can't use XBGR, and we're changing that. This hack should go away as soon as we address crbug.com/639111, and at that point we'll have it on all ozone drm devices.
The CQ bit was checked by dcastagna@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== ozone: Allow full screen overlay optimization for BGR formats. Allow full screen overlay optimization for buffers coming via wayland (typically BGR). Note: this CL could cause the format of the primary plane framebuffer used by drmModePageFlip to change (RGB<->BGR), this currently works on rockchip with a 3.14 kernel and won't work with newer kernels. This optimization will be initially enabled only those devices. The long term plan is to enable the optimization only when the overlay plane matches the format of the primary plane, and to switch the primary plane format to BGR (crbug.com/639111). BUG=b/29430506 ========== to ========== ozone: Allow full screen overlay optimization for BGR formats. Allow full screen overlay optimization for buffers coming via wayland (typically BGR). Note: this CL could cause the format of the primary plane framebuffer used by drmModePageFlip to change (RGB<->BGR), this currently works on rockchip with a 3.14 kernel and won't work with newer kernels. This optimization will be initially enabled only those devices. The long term plan is to enable the optimization only when the overlay plane matches the format of the primary plane, and to switch the primary plane format to BGR (crbug.com/639111). BUG=b/29430506 Committed: https://crrev.com/58f021db4c3e656bd88b23b907b5675f7f55fd48 Cr-Commit-Position: refs/heads/master@{#412960} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/58f021db4c3e656bd88b23b907b5675f7f55fd48 Cr-Commit-Position: refs/heads/master@{#412960} |
