Add support to send optimal format as part of ScheduleOverlayPlane

Add support to send optimal format as part of ScheduleOverlayPlane

On ChromeOS, during swap we try to convert the storage formats of
Video buffers to the most optimal one (i.e. YUV etc) which can save
Memroy bandwidth during scan out. This is currently hardcoded in
GbmBuffer, however this format support can vary on different hardware
and hence we should check first if this format is supported before
using it. This CL adds basic support to pass two additional parameters
storage_format and handle_scaling as part of ScheduleOverlayPlane.
These act as hints about the optimal format supported by the hardware
and if scaling needs to be handled in chrome before the composition
(i.e. Display Controller cannot handle plane scaling). Currently these
are hardcoded in DrmOverlayCandidateHost, in a followup patch I will add
support to actually query these from driver and pass it on here.

BUG=553264
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[kalyank, 2015-12-10 16:40:39]

Description was changed from

==========
Add support to send optimal format as part of ScheduleOverlayPlane

On ChromeOS, during swap we try to convert the storage formats of
Video buffers to the most optimal one (i.e. YUV etc) which can save
Memroy bandwidth during scan out. This is currently hardcoded in
GbmBuffer, however this format support can vary on different hardware
and hence we should check first if this format is supported before
using it. This CL adds basic support to pass two additional parameters
storage_format and handle_scaling as part of ScheduleOverlayPlane.
These act as hints about the optimal format supported by the hardware
and if scaling needs to be handled in chrome before the composition
(i.e. Display Controller cannot handle plane scaling). Currently these
are hardcoded in DrmOverlayCandidateHost, in a followup patch I will add
support to actually query these from driver and pass it on here.

BUG=553264
==========

to

==========
Add support to send optimal format as part of ScheduleOverlayPlane

On ChromeOS, during swap we try to convert the storage formats of
Video buffers to the most optimal one (i.e. YUV etc) which can save
Memroy bandwidth during scan out. This is currently hardcoded in
GbmBuffer, however this format support can vary on different hardware
and hence we should check first if this format is supported before
using it. This CL adds basic support to pass two additional parameters
storage_format and handle_scaling as part of ScheduleOverlayPlane.
These act as hints about the optimal format supported by the hardware
and if scaling needs to be handled in chrome before the composition
(i.e. Display Controller cannot handle plane scaling). Currently these
are hardcoded in DrmOverlayCandidateHost, in a followup patch I will add
support to actually query these from driver and pass it on here.

BUG=553264
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[kalyank, 2015-12-10 16:42:28]

Description was changed from

==========
Add support to send optimal format as part of ScheduleOverlayPlane

On ChromeOS, during swap we try to convert the storage formats of
Video buffers to the most optimal one (i.e. YUV etc) which can save
Memroy bandwidth during scan out. This is currently hardcoded in
GbmBuffer, however this format support can vary on different hardware
and hence we should check first if this format is supported before
using it. This CL adds basic support to pass two additional parameters
storage_format and handle_scaling as part of ScheduleOverlayPlane.
These act as hints about the optimal format supported by the hardware
and if scaling needs to be handled in chrome before the composition
(i.e. Display Controller cannot handle plane scaling). Currently these
are hardcoded in DrmOverlayCandidateHost, in a followup patch I will add
support to actually query these from driver and pass it on here.

BUG=553264
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add support to send optimal format as part of ScheduleOverlayPlane

On ChromeOS, during swap we try to convert the storage formats of
Video buffers to the most optimal one (i.e. YUV etc) which can save
Memroy bandwidth during scan out. This is currently hardcoded in
GbmBuffer, however this format support can vary on different hardware
and hence we should check first if this format is supported before
using it. This CL adds basic support to pass two additional parameters
storage_format and handle_scaling as part of ScheduleOverlayPlane.
These act as hints about the optimal format supported by the hardware
and if scaling needs to be handled in chrome before the composition
(i.e. Display Controller cannot handle plane scaling). Currently these
are hardcoded in DrmOverlayCandidateHost, in a followup patch I will add
support to actually query these from driver and pass it on here.

BUG=553264
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[kalyank, 2015-12-10 16:42:28]

kalyan.kondapally@intel.com changed reviewers:
+ alexst@chromium.org, piman@chromium.org

[kalyank, 2015-12-10 16:45:55]

kalyan.kondapally@intel.com changed reviewers:
+ reveman@chromium.org

[kalyank, 2015-12-10 16:47:12]

@reveman @piman @alexst PTAL

[reveman, 2015-12-10 16:58:31]

Can we implement this using CopyTexImage instead? See
https://codereview.chromium.org/1419733005 for an example of how that's done to
convert between different YUV formats on Mac. As an explicit copy operation the
client gets more control over when it happens and can take advantage of the fact
that the source buffer can reused as soon as the copy completes.

[kalyank, 2015-12-10 18:49:34]

On 2015/12/10 16:58:31, reveman wrote:
> Can we implement this using CopyTexImage instead? See
> https://codereview.chromium.org/1419733005 for an example of how that's done
to
> convert between different YUV formats on Mac. As an explicit copy operation
the
> client gets more control over when it happens and can take advantage of the
fact
> that the source buffer can reused as soon as the copy completes.

This wouldn't help in this case as we might want to convert to YUV, NV12
depending
on platform support and factors like Video being played in FullScreen or not
(Most
platforms don't support YUV on primary plane). This information needs to be
passed
from Ozone side to GLImage. Also, this conversion happens quite late (During
swap).
However, I think we want to add similar support in CopyTexImage for ChromeOS
too,
so that Media can create buffers with YUV/NV12 formats.

[piman, 2015-12-11 08:32:25]

I'm not sure I understand this CL. It looks like we're getting redundant
information from the client, or at least I don't understand what is supposed to
happen if they don't match.

Separately, it looks like you manually changed auto-generated files. Please make
sure to fix gpu/command_buffer/cmd_buffer_functions.txt first, and re-run the
code generator (gpu/command_buffer/build_gles2_cmd_buffer.py)

https://codereview.chromium.org/1513283002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1513283002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9450:
GetGFXOverlayStorageFormat(c.storage_format),
When would this be different than the image's BufferFormat?
What happens if they don't match? We can't trust the client.

https://codereview.chromium.org/1513283002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9454:
static_cast<GLboolean>(c.handle_scaling))) {
Same here, what happens if handle_scaling doesn't match whether
(uv_width,uv_height)*image->GetSize() is equal to (bounds_width,bounds_height)?

[kalyank, 2015-12-11 19:22:36]

https://codereview.chromium.org/1513283002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1513283002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9450:
GetGFXOverlayStorageFormat(c.storage_format),
On 2015/12/11 08:32:25, piman (OOO back 12-11) wrote:
> When would this be different than the image's BufferFormat?
> What happens if they don't match? We can't trust the client.

This might be different in case HardwarePlanes support formats
which can reduce the bandwidth. In ChromeOS case we hardcode these
checks in GbmPixmap 
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...

https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...

We decide if we need to do any format conversions or handle any scaling requests
and do it here. This is wrong as some hardware might not support these
formats or can handle plane scaling (would help reduce bandwidth in upscaling
cases). This information needs to come from HardwareDisplayPlaneManager and will
vary depending on the combination of layers using HardwarePlanes during a page
flip request. The other option would be to query this from PlaneManager while
scheduling the plane, however this would mean having to maintain a cache of
Overlay configurations being in use and making sure it's in sync with the
pending swaps in GLSurfaceOzone.

https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...

[kalyank, 2015-12-11 19:31:55]

On 2015/12/11 08:32:25, piman (OOO back 12-11) wrote:
> I'm not sure I understand this CL. It looks like we're getting redundant
> information from the client, or at least I don't understand what is supposed
to
> happen if they don't match.

They are just hints. In ChromeOS case if they dont match, we try to do any 
necessary processing while scheduling the overlay plane. On other platforms
they should be same always and can be ignored.

> Separately, it looks like you manually changed auto-generated files. Please
make
> sure to fix gpu/command_buffer/cmd_buffer_functions.txt first, and re-run the
> code generator (gpu/command_buffer/build_gles2_cmd_buffer.py)
> 

Done.

[piman, 2015-12-14 11:37:12]

On Sat, Dec 12, 2015 at 4:22 AM, <kalyan.kondapally@intel.com> wrote:

>
>
>
https://codereview.chromium.org/1513283002/diff/1/gpu/command_buffer/service/...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>
>
>
https://codereview.chromium.org/1513283002/diff/1/gpu/command_buffer/service/...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:9450:
> GetGFXOverlayStorageFormat(c.storage_format),
> On 2015/12/11 08:32:25, piman (OOO back 12-11) wrote:
>
>> When would this be different than the image's BufferFormat?
>> What happens if they don't match? We can't trust the client.
>>
>
> This might be different in case HardwarePlanes support formats
> which can reduce the bandwidth. In ChromeOS case we hardcode these
> checks in GbmPixmap
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...
>
> We decide if we need to do any format conversions or handle any scaling
> requests and do it here. This is wrong as some hardware might not
> support these
> formats or can handle plane scaling (would help reduce bandwidth in
> upscaling cases). This information needs to come from
> HardwareDisplayPlaneManager and will vary depending on the combination
> of layers using HardwarePlanes during a page flip request. The other
> option would be to query this from PlaneManager while scheduling the
> plane, however this would mean having to maintain a cache of Overlay
> configurations being in use and making sure it's in sync with the
> pending swaps in GLSurfaceOzone.
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...
>
>
Sorry, I still don't understand. All the code you link to is service-side.
Why does the (untrusted) client need to be involved to pass format data
between the place where it's allocated and the place where it's used?

https://codereview.chromium.org/1513283002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kalyank, 2015-12-14 18:15:50]

> >
> Sorry, I still don't understand. All the code you link to is service-side.
> Why does the (untrusted) client need to be involved to pass format data
> between the place where it's allocated and the place where it's used?

When processing Overlays, DrmOverlayCandidateHost will validate if a particular
layer can be promoted to Overlay and Cache the result. The validation happens
in GPU process side.

https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...

During the validation in GPU side, we do a test commit to check if the hardware
can support this layer on a Hardware plane or not. Idea is during this phase we
should also be able to determine optimal format and if scaling can be supported 
by the hardware. These values need to be available when compositing that given 
layer using a hardware plane. One solution was to cache the Overlaycheck_params(
which contains needed data about the layer and hold these values also) in GPU 
Process side and during swap retrieve these values for that given layer. 
However, as DrmOverlayCandidateHost already maintains cache of
OverlayCheck_params and knows which layers are going to be scheduled, 
I was thinking we could use that to retrieve these values and pass it as part of
ScheduleOverlay. This avoids need for maintaining another cache in GPU side and
having to make sure it's in sync with the cache in DrmOverlayCandidateHost.Do
you think we should try to handle this in server side completely without 
DrmOverlayCandidateHost having the need to pass this information.

> https://codereview.chromium.org/1513283002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-12-15 06:55:10]

On 2015/12/14 18:15:50, kalyank wrote:
> > >
> > Sorry, I still don't understand. All the code you link to is service-side.
> > Why does the (untrusted) client need to be involved to pass format data
> > between the place where it's allocated and the place where it's used?
> 
> When processing Overlays, DrmOverlayCandidateHost will validate if a
particular
> layer can be promoted to Overlay and Cache the result. The validation happens
> in GPU process side.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...
> 
> During the validation in GPU side, we do a test commit to check if the
hardware
> can support this layer on a Hardware plane or not. Idea is during this phase
we
> should also be able to determine optimal format and if scaling can be
supported 
> by the hardware. These values need to be available when compositing that given

> layer using a hardware plane. One solution was to cache the
Overlaycheck_params(
> which contains needed data about the layer and hold these values also) in GPU 
> Process side and during swap retrieve these values for that given layer. 
> However, as DrmOverlayCandidateHost already maintains cache of
> OverlayCheck_params and knows which layers are going to be scheduled, 
> I was thinking we could use that to retrieve these values and pass it as part
of
> ScheduleOverlay. This avoids need for maintaining another cache in GPU side
and
> having to make sure it's in sync with the cache in DrmOverlayCandidateHost.Do
> you think we should try to handle this in server side completely without 
> DrmOverlayCandidateHost having the need to pass this information.
> 
> > https://codereview.chromium.org/1513283002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-12-15 07:03:08]

On Tue, Dec 15, 2015 at 3:55 PM, <piman@chromium.org> wrote:

> On 2015/12/14 18:15:50, kalyank wrote:
>
>> > >
>> > Sorry, I still don't understand. All the code you link to is
>> service-side.
>> > Why does the (untrusted) client need to be involved to pass format data
>> > between the place where it's allocated and the place where it's used?
>>
>
> When processing Overlays, DrmOverlayCandidateHost will validate if a
>>
> particular
>
>> layer can be promoted to Overlay and Cache the result. The validation
>> happens
>> in GPU process side.
>>
>
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/.
> ..
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/.
> ..
>
> During the validation in GPU side, we do a test commit to check if the
>>
> hardware
>
>> can support this layer on a Hardware plane or not. Idea is during this
>> phase
>>
> we
>
>> should also be able to determine optimal format and if scaling can be
>>
> supported
>
>> by the hardware. These values need to be available when compositing that
>> given
>>
>
> layer using a hardware plane. One solution was to cache the
>>
> Overlaycheck_params(
>
>> which contains needed data about the layer and hold these values also) in
>> GPU
>> Process side and during swap retrieve these values for that given layer.
>> However, as DrmOverlayCandidateHost already maintains cache of
>> OverlayCheck_params and knows which layers are going to be scheduled,
>> I was thinking we could use that to retrieve these values and pass it as
>> part
>>
> of
>
>> ScheduleOverlay. This avoids need for maintaining another cache in GPU
>> side
>>
> and
>
>> having to make sure it's in sync with the cache in
>> DrmOverlayCandidateHost.Do
>> you think we should try to handle this in server side completely without
>> DrmOverlayCandidateHost having the need to pass this information.
>>
>
Keeping the cache on the GPU side too seems a lot simpler than moving this
data around, given the potential trust issues and loose semantics.


>
> > https://codereview.chromium.org/1513283002/
>> > >
>> >
>> > --
>> > You received this message because you are subscribed to the Google
>> Groups
>> > "Chromium-reviews" group.
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/1513283002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.