chromeos: gpu video decoder on Intel decodes video in NV12 format, instead of RGBA. [chromium/src : master]
Dongseong Hwang (Gerrit)
Dongseong Hwang posted comments on this change.
Patch set 1:
Set Ready For Review
To view, visit change 569144. To unsubscribe, visit settings.
Dongseong Hwang (Gerrit)
Dongseong Hwang would like Daniel Nicoara, Daniele Castagna, Pawel Osciak and Gurchetan Singh to review this change.
chromeos: gpu video decoder on Intel decodes video in NV12 format, instead of RGBA.
YUYV is 1.5 bytes per pixel, while BGRA is 4 bytes per pixel. It speeds up and
saves power consumption.
This CL is based on Johnson Lin's implementaion
https://codereview.chromium.org/2636433003/
Skylake Acer Chromebook 14 on http://browsertests.herokuapp.com/media/h264.html
* BGRA: 6.6462 W
* YUYV: 6.1729 W
YUYV saves 7.1% power compared to BGRA
In addition, YUYV plane is overlayable in ChromeOS. DrmOverlayValidator checks overlayable format in runtime.
If enabling experimental --enable-hardware-overlays, it saves further power consumption.
* BGRA with overlay: 6.1734 W
* YUYV with overlay: 6.0593 W
Say in English
* BGRA with overlay saves 7.1% power compared to BGRA
* NV12 with overlay saves 1.8% power compared to NV12
BUG=683347
TEST=cc_unittests VideoResourceUpdaterTest.CreateForHardwarePlanes_YUV422Texture
run samus chromeos on http://www.quirksmode.org/html5/tests/video.html with
following configurations
* overlay configurations
* without overlay: by default
* atomic mode setting: --enable-hardware-overlays --enable-drm-atomic
* legacy mode setting: --enable-hardware-overlays
Change-Id: Ibbfea506fe686f9ee3097c9bbd1e41becb6141ca
---
M cc/resources/video_resource_updater.cc
M cc/resources/video_resource_updater_unittest.cc
M media/gpu/vaapi_drm_picture.cc
M media/gpu/vaapi_video_decode_accelerator.cc
M media/gpu/vaapi_wrapper.cc
M ui/gfx/linux/client_native_pixmap_factory_dmabuf.cc
M ui/gfx/native_pixmap.h
M ui/ozone/platform/cast/surface_factory_cast.cc
M ui/ozone/platform/drm/common/drm_util.cc
M ui/ozone/platform/drm/common/drm_util.h
M ui/ozone/platform/drm/gpu/drm_thread.cc
M ui/ozone/platform/drm/gpu/gbm_buffer.cc
M ui/ozone/platform/drm/gpu/gbm_buffer.h
M ui/ozone/platform/headless/headless_surface_factory.cc
14 files changed, 123 insertions(+), 30 deletions(-)
Dongseong Hwang (Gerrit)
Dongseong Hwang posted comments on this change.
Patch set 1:Code-Review +1
Hi could you review?
This CL is relocated from https://codereview.chromium.org/2678343011/
This CL includes https://chromium-review.googlesource.com/c/549095/. When the CL is landed, I'll rebase this on it.
Dongseong Hwang (Gerrit)
Dongseong Hwang uploaded patch set #2 to this change.
chromeos: decode video into NV12 format instead of RGBA in vaapi decoder
NV12 is 1.5 bytes per pixel, while BGRA is 4 bytes per pixel. It speeds up and
saves power consumption.
There is 2 reasons why NV12 is the most desirable format.
1. The display controller hardware in Intel SoCs support NV12 [1] and YUV422
for hardware overlay.
2. Intel vaapi gpu decoder decodes the encoded video to NV12 first. If we use
YVU420 or YUYV, gpu decoder needs to do additional color conversion or swizzling.
This CL is based on Johnson Lin's implementaion
https://codereview.chromium.org/2636433003/
Electro (Apollo Lake) on H264 1080p 30FPS video
* BGRA: 7.87 W
* NV12: 7.41 W
NV12 saves 5.8% power compared to BGRA for H264
Electro (Apollo Lake) on VP9 1080p 30FPS video
* BGRA: 8.34 W
* NV12: 8.03 W
NV12 saves 3.7% power compared to BGRA for VP9
[1] https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-kbl-vol12-display.pdf
BUG=683347
TEST=cc_unittests VideoResourceUpdaterTest.CreateForNV12HardwarePlanes
run samus, lars and reef chromeos on http://www.quirksmode.org/html5/tests/video.html
Change-Id: Ibbfea506fe686f9ee3097c9bbd1e41becb6141ca
---
M cc/resources/video_resource_updater.cc
M cc/resources/video_resource_updater_unittest.cc
M media/gpu/vaapi_drm_picture.cc
M media/gpu/vaapi_video_decode_accelerator.cc
M media/gpu/vaapi_wrapper.cc
M ui/gfx/linux/client_native_pixmap_factory_dmabuf.cc
M ui/gfx/native_pixmap.h
M ui/ozone/platform/cast/surface_factory_cast.cc
M ui/ozone/platform/drm/common/drm_util.cc
M ui/ozone/platform/drm/common/drm_util.h
M ui/ozone/platform/drm/gpu/drm_thread.cc
M ui/ozone/platform/drm/gpu/gbm_buffer.cc
M ui/ozone/platform/drm/gpu/gbm_buffer.h
M ui/ozone/platform/headless/headless_surface_factory.cc
14 files changed, 123 insertions(+), 30 deletions(-)
To view, visit change 569144. To unsubscribe, visit settings.
Daniele Castagna (Gerrit)
Daniele Castagna posted comments on this change.
Patch set 2:
(1 comment)
File media/gpu/vaapi_drm_picture.cc:
Patch Set #2, Line 151:
return pixmap_->GetBufferUsage() == gfx::BufferUsage::SCANOUT;I already commented on this method in the other CL and I'm not quite sure why you didn't address this already.
AllowOverlay returning true at this level is completely fine. As you well know, it just means that the compositor will consider the VideoFrame originating from this picture as an overlay. DrmOverlayValidator will be used to determine if it can actually be scanned out. If leaving this to true break anything, we need to fix whatever is broken in ozone/overlay validation.
To view, visit change 569144. To unsubscribe, visit settings.
Daniele Castagna (Gerrit)
(3 comments)
File cc/resources/video_resource_updater.cc:
Patch Set #2, Line 68:
bool is_dual_gmb = video_frame->mailbox_holder(1).texture_target ==Why are you comparing the texture_target of the two mailboxes here? Where is the producer that produces these kind of VideoFrames?
In your case, don't you have only one mailbox with one NV12 texture? Are you relying on the fact that mailbox_holder.texture_target is initialized to TEXTURE_2D?
Patch Set #2, Line 35:
virtual gfx::BufferUsage GetBufferUsage() const = 0;Please remove this, it's not needed.
File ui/ozone/platform/drm/common/drm_util.cc:
Patch Set #2, Line 536:
gfx::BufferUsage GetBufferUsageFromGbmFlags(int flags) {I really think it's a bad idea to convert from gbmflags back to BufferUsage and it shouldn't be needed. Please remove.
To view, visit change 569144. To unsubscribe, visit settings.
Pawel Osciak (Gerrit)
Pawel Osciak posted comments on this change.
Patch set 2:
Thank you for working on this!
Are NV12 overlays supported on all Intel platforms, including the other platforms mentioned here (for samus, lars, etc.)? Could you perhaps share performance numbers from your tests on these platforms as well please?
Could you also test your changes with the video_VideoDecodeAccelerator unittest (https://chromium.googlesource.com/chromiumos/third_party/autotest/+/HEAD/client/site_tests/video_VideoDecodeAccelerator/) on all relevant platforms and update the TEST= line please? The test mentioned here in TEST= line is not enough to verify hardware video acceleration.
Thank you.
Dongseong Hwang (Gerrit)
Dongseong Hwang posted comments on this change.
Patch set 3:Commit-Queue +1
Dongseong Hwang (Gerrit)
Set Ready For Review
Dongseong Hwang (Gerrit)
Patch Set 2:
Thank you for working on this!
Are NV12 overlays supported on all Intel platforms, including the other platforms mentioned here (for samus, lars, etc.)?
NV12 decoding is supported by all platform.
NV12 overlays is supported by Apollo lake and Kaby Lake or newer generation.
NOTE: hardware overlays will be enabled on only Apollo lake and Kaby Lake or newer generation, mainly because of kernel v4.4 and native display scaler (Skylake or newer).
Could you perhaps share performance numbers from your tests on these platforms as well please?
I'll provide soon. Anyway it must be better because Intel GPU decoder always decode video to NV12 first and then do color conversion.
Could you also test your changes with the video_VideoDecodeAccelerator unittest (https://chromium.googlesource.com/chromiumos/third_party/autotest/+/HEAD/client/site_tests/video_VideoDecodeAccelerator/) on all relevant platforms and update the TEST= line please? The test mentioned here in TEST= line is not enough to verify hardware video acceleration.
Yes, done.
(3 comments)
Patch Set #2, Line 68:
bool is_dual_gmb = video_frame->mailbox_holder(1).texture_target ==Why are you comparing the texture_target of the two mailboxes here? Where i
Check whether
NV12_SINGLE_GMB, // One NV12 GMB
NV12_DUAL_GMB, // One R8, one RG88 GMB
https://cs.chromium.org/chromium/src/media/renderers/gpu_video_accelerator_factories.h?q=NV12_DUAL_GMB&sq=package:chromium&l=67media::PIXEL_FORMAT_NV12 can be either of them, but we must use YUV_RESOURCE for NV12_DUAL_GMB.
File media/gpu/vaapi_drm_picture.cc:
I already commented on this method in the other CL and I'm not quite sure w
Sorry for the stale comment. I just removed the comment. And I think current code is better than hardcoding 'true', because we know enough information.
In addition, it's not true that AllowOverlay returning true at this level is completely fine. OverlayCandidateValidator validates format, size and position, instead of actual buffer.
DrmOverlayValidator reports to cc it's fine, and then when actuall flip happen, HardwareDisplayPlaneManager::AssignOverlayPlanes() fails because this plane isn't scanout and doesn't have fb id.[4523:4533:0714/153410.712048:ERROR:hardware_display_plane_manager.cc(249)] Failed to find a free plane for crtc 35
[4523:4533:0714/153410.712125:ERROR:crtc_controller.cc(102)] Failed to assign overlay planes for crtc 35: Invalid argument
File ui/ozone/platform/drm/common/drm_util.cc:
Patch Set #2, Line 536:
gfx::BufferUsage GetBufferUsageFromGbmFlags(int flags) {I really think it's a bad idea to convert from gbmflags back to BufferUsage
As I replied above, we need the way to figure out whether the GBM is scanout or not.
To view, visit change 569144. To unsubscribe, visit settings.
Daniele Castagna (Gerrit)
Daniele Castagna posted comments on this change.
Patch set 3:
(1 comment)
Sorry for the stale comment. I just removed the comment. And I think curren
We should change the overlay validator to use actual buffers instead of some generated ones that are similar, but not the same as the actual buffers we'll scanout.
If a buffer doesn't have an fb, it should fail validation.
To view, visit change 569144. To unsubscribe, visit settings.
Dongseong Hwang (Gerrit)
Daniele, Pawel, could you review again?
Sorry for delay updating. Worked on hardware overlay issues for while.
1 comment:
We should change the overlay validator to use actual buffers instead of som
I'll. On the other hands, new CL doesn't change gbm_flags to BufferUsage, because now gbm_buffer keeps BufferUsage.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Daniele Castagna (Gerrit)
Patch Set 6:
(1 comment)
Daniele, Pawel, could you review again?
Sorry for delay updating. Worked on hardware overlay issues for while.
We should really revisit the power consumption numbers that you have in the CL description.
We're now decoding into RGBX (https://chromium-review.googlesource.com/c/590772/) and we hit the overlay case for videos now, skipping compositing completely in many cases.
We might still be better off with NV12 and compositing, we need to get updated numbers though.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Dongseong Hwang (Gerrit)
Patch Set 6:
Patch Set 6:
(1 comment)
Daniele, Pawel, could you review again?
Sorry for delay updating. Worked on hardware overlay issues for while.
We should really revisit the power consumption numbers that you have in the CL description.
We're now decoding into RGBX (https://chromium-review.googlesource.com/c/590772/) and we hit the overlay case for videos now, skipping compositing completely in many cases.
We might still be better off with NV12 and compositing, we need to get updated numbers though.
Agreed.
The pixel of RGBX is 4 bytes, but can be overlayed.
The pixle of NV12 is 1.5bytes, but now cannot be overlayed. (NOTE: NV12 overlay will be supported)
I'll update PnP number.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Dongseong Hwang (Gerrit)
Hi Pawel and Daniele, I updated PnP result in the description.
PnP result indicates NV12 is better than BGRX, even though only BGRX can be overlayed now.
NOTE: NV12 overlay will be supported. https://chromium-review.googlesource.com/c/602570
Could you review again?
1 comment:
Patch Set #14, Line 63:
NV12 saves 3.8% than BGRX and -1.8% than BGRX with overlay.Although VP9 720p has small regression, I think NV12 is much better thna BGRX because
- VP9 4k and H.264 4k has ~10% power saving.
- H.264 720p has 5% power saving.
- 4 to 1.5 memory saving.
- NV12 overlay will be supported soon.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Daniele Castagna (Gerrit)
Patch Set 14:
(1 comment)
Hi Pawel and Daniele, I updated PnP result in the description.
PnP result indicates NV12 is better than BGRX, even though only BGRX can be overlayed now.
NOTE: NV12 overlay will be supported. https://chromium-review.googlesource.com/c/602570Could you review again?
A description of the methodology you used to get power numbers would be really appreciated.
Did you test a windowed video? A fullscreen one? How was the brightness of the screen?
Why has the power consumption reduced more now than it used to in your fist analysis when we were not using HW overlays for RGBX?
That seems hard to believe considering the RGBX with HW overlays is already a significant improvement vs what we had before.
2 comments:
File media/gpu/vaapi_drm_picture.cc:
Patch Set #14, Line 147:
return pixmap_->GetBufferUsage() == gfx::BufferUsage::SCANOUT;Again, as I already commented twice, this is not needed an can just return true.
In your tests we would accept the configuration with an NV12 buffer and not fail because you are not correctly creating the resource in video_resource_updater.cc.I cced you to https://chromium-review.googlesource.com/c/602908, that correctly creates the resource (specifying the buffer_format) from a NV12 VideoFrame so that the DrmOverlayValidator will reject NV12 as an overlay.
File ui/ozone/platform/drm/gpu/gbm_buffer.cc:
Patch Set #14, Line 38:
case gfx::BufferUsage::GPU_READ:Please remove all of this.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Daniele Castagna (Gerrit)
Would you mind rebasing this patch on master so we can test it locally too?
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Dongseong Hwang (Gerrit)
Patch Set 14:
(2 comments)
Patch Set 14:
(1 comment)
Hi Pawel and Daniele, I updated PnP result in the description.
PnP result indicates NV12 is better than BGRX, even though only BGRX can be overlayed now.
NOTE: NV12 overlay will be supported. https://chromium-review.googlesource.com/c/602570Could you review again?
A description of the methodology you used to get power numbers would be really appreciated.
It's my pleasure.
Did you test a windowed video? A fullscreen one? How was the brightness of the screen?
windowed video whose page has only one video element. the brightness is as low as possible with lcd on.
Why has the power consumption reduced more now than it used to in your fist analysis when we were not using HW overlays for RGBX?
The first analysis was done in different device which is Electro. This time, Ren use Reef and Soraka. I think RGBX and RGBA supposed to consume the same power.
That seems hard to believe considering the RGBX with HW overlays is already a significant improvement vs what we had before.
So the absolute value is not very meaningful. Sorry for inconsistence.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Daniele Castagna (Gerrit)
Patch Set 14:
Patch Set 14:
(2 comments)
Patch Set 14:
(1 comment)
Hi Pawel and Daniele, I updated PnP result in the description.
PnP result indicates NV12 is better than BGRX, even though only BGRX can be overlayed now.
NOTE: NV12 overlay will be supported. https://chromium-review.googlesource.com/c/602570Could you review again?
A description of the methodology you used to get power numbers would be really appreciated.
It's my pleasure.
Did you test a windowed video? A fullscreen one? How was the brightness of the screen?
windowed video whose page has only one video element. the brightness is as low as possible with lcd on.
Why has the power consumption reduced more now than it used to in your fist analysis when we were not using HW overlays for RGBX?
The first analysis was done in different device which is Electro. This time, Ren use Reef and Soraka. I think RGBX and RGBA supposed to consume the same power.
RGBX should consume less power because we don't draw the background. When we were using RGBA we were drawing behind the video for every frame (the compositor didn't know the video would end up being opaque).
It made a huge difference when HW overlays were enabled since with RGBX we'd end up with an empty damage rect and we could just update the overlay.
> That seems hard to believe considering the RGBX with HW overlays is already a significant improvement vs what we had before.So the absolute value is not very meaningful. Sorry for inconsistence.
That's why I would like to reproduce and validate these experiments here too.
Please let us know when you rebase this patch.
At this point I'd expect only changes in media/gpu/* to be needed.
Btw, most of the time seems to be spent in vaapi_wrapper_->BlitSurface(va_surface, va_surface_). The big win will be to avoid that blit completely.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Dongseong Hwang (Gerrit)
Daniele, could you review again? :)
2 comments:
Again, as I already commented twice, this is not needed an can just return
Done. awesome! thank you for the preparation CL!
File ui/ozone/platform/drm/gpu/gbm_buffer.cc:
Patch Set #14, Line 38:
const std::vector<gfx::NativePixmapPlane>&& planes)
Please remove all of this.
Done
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Daniele Castagna (Gerrit)
4 comments:
File cc/resources/video_resource_updater_unittest.cc:
Patch Set #17, Line 526:
TEST_F(VideoResourceUpdaterTest, CreateForNV12HardwarePlanes) {VideoResourceUpdaterTest.CreateForHardwarePlanes_SingleNV12 is already testing what we need.
Please revert this file.
File gpu/ipc/service/gpu_init.cc:
Patch Set #17, Line 225:
std::unique_ptr<gfx::ClientNativePixmapFactory> client_native_pixmap_factory =Why is this needed and how is this related to this CL?
File media/gpu/vaapi_drm_picture.cc:
Patch Set #17, Line 114:
pixmap_ = factory->CreateNativePixmap(gfx::kNullAcceleratedWidget, size_,Please, leave this as it was. SCANOUT should work.
File media/gpu/vaapi_wrapper.cc:
Patch Set #17, Line 657:
va_attrib_extbuf.flags = VA_SURFACE_EXTBUF_DESC_ENABLE_TILING;Why is this needed? Are NV12 buffers always tiled? If that's the case, should we write an helper function that returns the flags given the buffer format?
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Dongseong Hwang (Gerrit)
Daniele, thank you for review. Could you review again?
4 comments:
File cc/resources/video_resource_updater_unittest.cc:
Patch Set #17, Line 526:
context3d_->ResetTextureCreationCount();VideoResourceUpdaterTest.CreateForHardwarePlanes_SingleNV12 is already test
Done. Added CreateForHardwarePlanes_DoubleNV12 not covered by current test.
File gpu/ipc/service/gpu_init.cc:
Patch Set #17, Line 225:
std::unique_ptr<gfx::ClientNativePixmapFactory> client_native_pixmap_factory =Why is this needed and how is this related to this CL?
gpu::IsNativeGpuMemoryBufferConfigurationSupported(), which is used by VaapiDrmPicture::Allocate(), requires ClientNativePixmapFactory singleton initialization.
This CL needs to use it in gpu process.
File media/gpu/vaapi_drm_picture.cc:
Patch Set #17, Line 114:
pixmap_ = factory->CreateNativePixmap(gfx::kNullAcceleratedWidget, size_,Please, leave this as it was. SCANOUT should work.
No, gbm_bo_create(DRM_FORMAT_NV12, GBM_BO_USE_SCANOUT) fails without kernel support.
NOTE: i915 in minigbm queries format candidate for scanout buffer in runtime.
File media/gpu/vaapi_wrapper.cc:
Patch Set #17, Line 657:
va_attrib_extbuf.flags = VA_SURFACE_EXTBUF_DESC_ENABLE_TILING;Why is this needed? Are NV12 buffers always tiled? If that's the case, shou
NV12 tiling is up to chromium. This CL allocates NV12 buffer as gfx::BufferUsage::GPU_READ or gfx::BufferUsage::SCANOUT, which is tiled.
BTW, this CL works with and without this change. I choose more correct one.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Daniele Castagna (Gerrit)
I just tried to get some numbers on eve. And my numbers don't match with yours (you tried on other devices though.)
Power consumption is significantly better with RGBX and HW overlays than with NV12.
NV12 is slightly better than RGBX with compositing.
I used a 4k 60fps youtube video, windowed, theater mode and fullscreen.
The savings with RGBX and HW overlays vs NV12 for the windowed case I observed are:
720p60: 7.1 vs 9.7
1080p60: 8.4 vs 10.4
1440p60: 10.5 vs 11.5
2160p60: 14.5 vs 13.5
Basically, only 4k is an advantage with NV12 (and compositing).
I'd wait for NV12 support in the kernel and minigbm before landing this patch since I'm guessing most of the users are not watching 4k videos (we can check this guess if needed), and 4k videos are already running pretty smoothly at this point.
WDYT?
Another thing we might want to look at is to remove the blit we do in VaapiWrapper::BlitSurface.
If we could remove that blit NV12 would probably be a win even with compositing.
5 comments:
Patch Set #17, Line 526:
context3d_->ResetTextureCreationCount();
Done. Added CreateForHardwarePlanes_DoubleNV12 not covered by current test.
Can you use the same name convention used in the rest of the file then? This should be CreateForHardwarePlanes_DualNV12. That'd be the same name you mentioned in the comment.
File gpu/ipc/service/gpu_init.cc:
Patch Set #17, Line 225:
std::unique_ptr<gfx::ClientNativePixmapFactory> client_native_pixmap_factory =
gpu::IsNativeGpuMemoryBufferConfigurationSupported(), which is used by Vaap
Isn't the client native pixmap factory what we use for importing/mmapping on the client side?
Why would we use something related to that to check if a format/usage is supported?
File media/gpu/vaapi_drm_picture.cc:
Patch Set #17, Line 114:
pixmap_ = factory->CreateNativePixmap(gfx::kNullAcceleratedWidget, size_,
No, gbm_bo_create(DRM_FORMAT_NV12, GBM_BO_USE_SCANOUT) fails without kernel
Are you sure it has anything to do with kernel support?
Have we added NV12 SCANOUT to minigbm?
David was suggesting to have anothe USAGE for this. I'll discuss this wit him and give an update soon.Is NV12 with TEXTURING supported on all the devices with vaapi?
File media/gpu/vaapi_wrapper.cc:
Patch Set #17, Line 657:
va_attrib_extbuf.flags = VA_SURFACE_EXTBUF_DESC_ENABLE_TILING;
NV12 tiling is up to chromium. This CL allocates NV12 buffer as gfx::Buffer
After reading your reply, I'm still not sure why we need this.
File ui/gfx/linux/client_native_pixmap_factory_dmabuf.cc:
Patch Set #17, Line 64:
format == gfx::BufferFormat::YUV_420_BIPLANAR ||This check should be for buffers that we try to map in the client. We shouldn't need to add this format here at this point.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Daniele Castagna (Gerrit)
2 comments:
File cc/resources/video_resource_updater_unittest.cc:
Patch Set #20, Line 694:
TEST_F(VideoResourceUpdaterTest, CreateForHardwarePlanes_DoubleNV12) {This test would be nice to have independently from this CL, and while related to this CL, it is not really testing anything specific to the newly added code.
Would you mind uploading another CL just to add this test?
File media/gpu/vaapi_drm_picture.cc:
Patch Set #20, Line 113:
usage = gfx::BufferUsage::SCANOUT;We are about to add a specific usage for this: crrev.com/c/612761
I'm also confused how you tested this with SCANOUT, since that would trainslate to GBM_BO_USE_RENDERING | GBM_BO_USE_SCANOUT | GBM_BO_USE_TEXTURING. And NV12 with USE_RENDERING is not supported.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Daniele Castagna (Gerrit)
DS, you should be able to revert all the files but media/gpu/vaapi_*, use the new usage SCANOUT_VDA_WRITE in VaapiDrmPicture::Allocate and this patch should be ready to land after we get NV12 framebuffer support in the kernel.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Dongseong Hwang (Gerrit)
Patch Set 20:
DS, you should be able to revert all the files but media/gpu/vaapi_*, use the new usage SCANOUT_VDA_WRITE in VaapiDrmPicture::Allocate and this patch should be ready to land after we get NV12 framebuffer support in the kernel.
Hi Daniele, thank you for reviewing, introducing SCANOUT_VDA_WRITE and benchmarking.
New patch set uses SCANOUT_VDA_WRITE and there is one important change. As you found RGBX with overlay is better than NV12 in small video, this CL makes chromium work like
- use RGBX if NV12 overlay is not supported.
- use NV12 if NV12 overlay is supported.
The old patch set makes chromium work like
- use NV12 and GPU_READ if NV12 overlay is not supported.
- use NV12 and SCANOUT if NV12 overlay is supported.
- However, you found it will cause regression. We priotize overlay to format.
Now this CL doesn't need to wait kernel change, because RGBX will be used as-is without kernel change.
Could you review and can we make it land?
7 comments:
File cc/resources/video_resource_updater_unittest.cc:
Can you use the same name convention used in the rest of the file then? This should be CreateForHard […]
Done
File cc/resources/video_resource_updater_unittest.cc:
This test would be nice to have independently from this CL, and while related to this CL, it is not […]
Done
File gpu/ipc/service/gpu_init.cc:
Isn't the client native pixmap factory what we use for importing/mmapping on the client side? […]
I agree. removed.
File media/gpu/vaapi_drm_picture.cc:
Have we added NV12 SCANOUT to minigbm?
minigbm queries scanout format in runtime in following code. It check supported format by atomic plane. NV12 scanout requires kernel support
https://chromium.googlesource.com/chromiumos/platform/minigbm/+/master/i915.c#167
Is NV12 with TEXTURING supported on all the devices with vaapi?
Yes, NV12 texturing is just software implementation by mesa. Kernel doesn't do nothing for it.
From 1st generation intel core to Kaby Lake all supported.
File media/gpu/vaapi_drm_picture.cc:
Patch Set #20, Line 113:
return false;We are about to add a specific usage for this: crrev.com/c/612761 […]
Use SCANOUT_VDA_WRITE now. Thank you for introducing.
File media/gpu/vaapi_wrapper.cc:
Patch Set #17, Line 657:
va_attrib_extbuf.flags = 0;
After reading your reply, I'm still not sure why we need this.
reverted
File ui/gfx/linux/client_native_pixmap_factory_dmabuf.cc:
Patch Set #17, Line 64:
return format == gfx::BufferFormat::BGRX_8888 ||This check should be for buffers that we try to map in the client. […]
You're right. Removed.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Daniele Castagna (Gerrit)
Patch Set 22:
(7 comments)
Patch Set 20:
DS, you should be able to revert all the files but media/gpu/vaapi_*, use the new usage SCANOUT_VDA_WRITE in VaapiDrmPicture::Allocate and this patch should be ready to land after we get NV12 framebuffer support in the kernel.
Hi Daniele, thank you for reviewing, introducing SCANOUT_VDA_WRITE and benchmarking.
New patch set uses SCANOUT_VDA_WRITE and there is one important change. As you found RGBX with overlay is better than NV12 in small video, this CL makes chromium work like
- use RGBX if NV12 overlay is not supported.
- use NV12 if NV12 overlay is supported.
The old patch set makes chromium work like
- use NV12 and GPU_READ if NV12 overlay is not supported.
- use NV12 and SCANOUT if NV12 overlay is supported.
- However, you found it will cause regression. We priotize overlay to format.
Now this CL doesn't need to wait kernel change, because RGBX will be used as-is without kernel change.
Could you review and can we make it land?
What's the benefit of landing this now instead of waiting for NV12 in the kernel?
I'd prefer to land this when NV12 framebuffer is supported an avoid picking a format at runtime.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Dongseong Hwang (Gerrit)
Patch Set 22:
Patch Set 22:
(7 comments)
Patch Set 20:
DS, you should be able to revert all the files but media/gpu/vaapi_*, use the new usage SCANOUT_VDA_WRITE in VaapiDrmPicture::Allocate and this patch should be ready to land after we get NV12 framebuffer support in the kernel.
Hi Daniele, thank you for reviewing, introducing SCANOUT_VDA_WRITE and benchmarking.
New patch set uses SCANOUT_VDA_WRITE and there is one important change. As you found RGBX with overlay is better than NV12 in small video, this CL makes chromium work like
- use RGBX if NV12 overlay is not supported.
- use NV12 if NV12 overlay is supported.
The old patch set makes chromium work like
- use NV12 and GPU_READ if NV12 overlay is not supported.
- use NV12 and SCANOUT if NV12 overlay is supported.
- However, you found it will cause regression. We priotize overlay to format.
Now this CL doesn't need to wait kernel change, because RGBX will be used as-is without kernel change.
Could you review and can we make it land?What's the benefit of landing this now instead of waiting for NV12 in the kernel?
No benefit for users. However, we can test the kernel change easily.
I'd prefer to land this when NV12 framebuffer is supported an avoid picking a format at runtime.
Even though the kernel change is landed, we still need to pick it up in runtime, because NV12 + SCANOUT isn't supported for old kernel as well as old device. gbm buffer allocation fails on old devices.
gbm succeeds to allocate the scanout buffer only if the underline display supports the format for OVL plane.
For Intel, NV12 plane is supported by APL, KBL and newer generation.
For Kernel, only after chromeos-v4.4
The old patch set intends to use NV12 + GPU_READ for old devices, but your benchmark change it to use RGBX + SCANOUT for old devices.
Either BufferFormat or BufferUsage must be selected in runtime.
Dongseong Hwang (Gerrit)
Daniele, do you have a chance to take a look again?
Daniele Castagna (Gerrit)
Patch Set 22:
Patch Set 22:
Patch Set 22:
(7 comments)
Patch Set 20:
DS, you should be able to revert all the files but media/gpu/vaapi_*, use the new usage SCANOUT_VDA_WRITE in VaapiDrmPicture::Allocate and this patch should be ready to land after we get NV12 framebuffer support in the kernel.
Hi Daniele, thank you for reviewing, introducing SCANOUT_VDA_WRITE and benchmarking.
New patch set uses SCANOUT_VDA_WRITE and there is one important change. As you found RGBX with overlay is better than NV12 in small video, this CL makes chromium work like
- use RGBX if NV12 overlay is not supported.
- use NV12 if NV12 overlay is supported.
The old patch set makes chromium work like
- use NV12 and GPU_READ if NV12 overlay is not supported.
- use NV12 and SCANOUT if NV12 overlay is supported.
- However, you found it will cause regression. We priotize overlay to format.
Now this CL doesn't need to wait kernel change, because RGBX will be used as-is without kernel change.
Could you review and can we make it land?What's the benefit of landing this now instead of waiting for NV12 in the kernel?
No benefit for users. However, we can test the kernel change easily.
I'd prefer to land this when NV12 framebuffer is supported an avoid picking a format at runtime.
Even though the kernel change is landed, we still need to pick it up in runtime, because NV12 + SCANOUT isn't supported for old kernel as well as old device. gbm buffer allocation fails on old devices.
gbm succeeds to allocate the scanout buffer only if the underline display supports the format for OVL plane.
This is not an issue. If gbm fails to allocate a BO_SCANOUT buffer, chrome will retry to allocate buffer without BO_SCANOUT usage. So, we should always be able to allocate an NV12 buffer with SCANOUT_VDA_WRITE usage.
For Intel, NV12 plane is supported by APL, KBL and newer generation.
For Kernel, only after chromeos-v4.4The old patch set intends to use NV12 + GPU_READ for old devices, but your benchmark change it to use RGBX + SCANOUT for old devices.
What are the platforms where we enabled overlays but we don't have NV12 scanout support? Is it worth fragmenting the format just to have a marginal benefit there?
Either BufferFormat or BufferUsage must be selected in runtime.
Dongseong Hwang (Gerrit)
Patch Set 22:
Patch Set 22:
Patch Set 22:
Patch Set 22:
(7 comments)
Patch Set 20:
DS, you should be able to revert all the files but media/gpu/vaapi_*, use the new usage SCANOUT_VDA_WRITE in VaapiDrmPicture::Allocate and this patch should be ready to land after we get NV12 framebuffer support in the kernel.
Hi Daniele, thank you for reviewing, introducing SCANOUT_VDA_WRITE and benchmarking.
New patch set uses SCANOUT_VDA_WRITE and there is one important change. As you found RGBX with overlay is better than NV12 in small video, this CL makes chromium work like
- use RGBX if NV12 overlay is not supported.
- use NV12 if NV12 overlay is supported.
The old patch set makes chromium work like
- use NV12 and GPU_READ if NV12 overlay is not supported.
- use NV12 and SCANOUT if NV12 overlay is supported.
- However, you found it will cause regression. We priotize overlay to format.
Now this CL doesn't need to wait kernel change, because RGBX will be used as-is without kernel change.
Could you review and can we make it land?What's the benefit of landing this now instead of waiting for NV12 in the kernel?
No benefit for users. However, we can test the kernel change easily.
I'd prefer to land this when NV12 framebuffer is supported an avoid picking a format at runtime.
Even though the kernel change is landed, we still need to pick it up in runtime, because NV12 + SCANOUT isn't supported for old kernel as well as old device. gbm buffer allocation fails on old devices.
gbm succeeds to allocate the scanout buffer only if the underline display supports the format for OVL plane.
This is not an issue. If gbm fails to allocate a BO_SCANOUT buffer, chrome will retry to allocate buffer without BO_SCANOUT usage. So, we should always be able to allocate an NV12 buffer with SCANOUT_VDA_WRITE usage.
Current chromium doesn't behave like the explanation. https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/gbm_buffer.cc?l=241 Furthermore, I'm not sure if it's good idea, since
- client code can know whether it's possible to be scanout or not
- explicit code would be better than implicit fallback.
- even though scanout gbm bo is created without BO_SCANOUT, it's another story to handle AddFramebuffer2 call.
For Intel, NV12 plane is supported by APL, KBL and newer generation.
For Kernel, only after chromeos-v4.4The old patch set intends to use NV12 + GPU_READ for old devices, but your benchmark change it to use RGBX + SCANOUT for old devices.
What are the platforms where we enabled overlays but we don't have NV12 scanout support? Is it worth fragmenting the format just to have a marginal benefit there?
That's good point. Nope, we don't have. We enable overlay since KabyLake and ApolloLake which support NV12 scanout. It's why I more like old patch's branch
- use NV12 and GPU_READ if NV12 overlay is not supported.
- use NV12 and SCANOUT if NV12 overlay is supported.
Either BufferFormat or BufferUsage must be selected in runtime.
Daniele Castagna (Gerrit)
Please look at DrmThread::CreateBuffer.
Furthermore, I'm not sure if it's good idea, since
- client code can know whether it's possible to be scanout or not
- explicit code would be better than implicit fallback.
- even though scanout gbm bo is created without BO_SCANOUT, it's another story to handle AddFramebuffer2 call.
For Intel, NV12 plane is supported by APL, KBL and newer generation.
For Kernel, only after chromeos-v4.4The old patch set intends to use NV12 + GPU_READ for old devices, but your benchmark change it to use RGBX + SCANOUT for old devices.
What are the platforms where we enabled overlays but we don't have NV12 scanout support? Is it worth fragmenting the format just to have a marginal benefit there?
That's good point. Nope, we don't have. We enable overlay since KabyLake and ApolloLake which support NV12 scanout.
If we don't have any of those configurations. NV12 is the best format on all intel boards once we have NV12 fb, so, let's switch all of them to NV12.
It's why I more like old patch's branch
- use NV12 and GPU_READ if NV12 overlay is not supported.
- use NV12 and SCANOUT if NV12 overlay is supported.
This already happens with the new usage we just added.
Either BufferFormat or BufferUsage must be selected in runtime.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Dongseong Hwang (Gerrit)
Ah, it's changed recently. I see.
I'll wait for upstream kernel review. Fortunately, Vidya rebased the patch set today to land. The review is almost done.
https://patchwork.freedesktop.org/series/28103/
Furthermore, I'm not sure if it's good idea, since
- client code can know whether it's possible to be scanout or not
- explicit code would be better than implicit fallback.
- even though scanout gbm bo is created without BO_SCANOUT, it's another story to handle AddFramebuffer2 call.
For Intel, NV12 plane is supported by APL, KBL and newer generation.
For Kernel, only after chromeos-v4.4The old patch set intends to use NV12 + GPU_READ for old devices, but your benchmark change it to use RGBX + SCANOUT for old devices.
What are the platforms where we enabled overlays but we don't have NV12 scanout support? Is it worth fragmenting the format just to have a marginal benefit there?
That's good point. Nope, we don't have. We enable overlay since KabyLake and ApolloLake which support NV12 scanout.
If we don't have any of those configurations. NV12 is the best format on all intel boards once we have NV12 fb, so, let's switch all of them to NV12.
It's why I more like old patch's branch
- use NV12 and GPU_READ if NV12 overlay is not supported.
- use NV12 and SCANOUT if NV12 overlay is supported.
This already happens with the new usage we just added.
ACK
Either BufferFormat or BufferUsage must be selected in runtime.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.
Daniele Castagna (Gerrit)
Hi Dongseong, do you mind cleaning up this patch as suggested so we can use it for testing and it'll be ready to land as soon as we have the changes needed in the kernel?
Dongseong Hwang (Gerrit)
Dongseong Hwang abandoned this change.
To view, visit change 569144. To unsubscribe, or for help writing mail filters, visit settings.