Propagate supported scanout formats from driver to SurfaceFactoryOzone.

Propagate supported scanout formats from driver to SurfaceFactoryOzone.

BUG=None

Committed: https://crrev.com/f9e3001545d09eb9579a13ca3fc76331c2d61546
Cr-Commit-Position: refs/heads/master@{#371396}

[william.xie, 2016-01-16 01:49:14]

Description was changed from

==========
Propagate supported formats from driver to ClientNativePixmapFactoryGbm.

BUG=None
==========

to

==========
Propagate supported formats from driver to ClientNativePixmapFactoryGbm.

BUG=None
==========

[william.xie, 2016-01-16 01:49:14]

william.xie@chromium.org changed reviewers:
+ alexst@chromium.org, dnicoara@chromium.org, kalyan.kondapally@intel.com,
marcheu@chromium.org, reveman@chromium.org, spang@chromium.org

[william.xie, 2016-01-16 01:49:34]

PTAL

[spang, 2016-01-18 18:14:31]

https://codereview.chromium.org/1598573002/diff/1/ui/ozone/platform/drm/clien...
File ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc (right):

https://codereview.chromium.org/1598573002/diff/1/ui/ozone/platform/drm/clien...
ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc:58:
ui::OzonePlatform::GetInstance()->GetSupportedFormats(&support_formats);
The renderer process is allowed to have a ClientNativePixmapFactory, but doesn't
have an OzonePlatform. 

You'll need to use IPC to support this in the renderer case (adding an
initialization handshake between gpu process & renderer/browser).

The gpu process format query probably needs to be moved from
ClientNativePixmapFactory to SurfaceFactoryOzone; After that I don't think the
gpu process will use ClientNativePixmapFactory.

[william.xie, 2016-01-19 04:39:42]

Description was changed from

==========
Propagate supported formats from driver to ClientNativePixmapFactoryGbm.

BUG=None
==========

to

==========
Propagate supported formats from driver to SurfaceFactoryOzone.

BUG=None
==========

[william.xie, 2016-01-19 04:40:16]

Thanks Michael, PTAL

https://codereview.chromium.org/1598573002/diff/1/ui/ozone/platform/drm/clien...
File ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc (right):

https://codereview.chromium.org/1598573002/diff/1/ui/ozone/platform/drm/clien...
ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc:58:
ui::OzonePlatform::GetInstance()->GetSupportedFormats(&support_formats);
On 2016/01/18 18:14:31, spang wrote:
> The renderer process is allowed to have a ClientNativePixmapFactory, but
doesn't
> have an OzonePlatform. 
> 
> You'll need to use IPC to support this in the renderer case (adding an
> initialization handshake between gpu process & renderer/browser).
> 
> The gpu process format query probably needs to be moved from
> ClientNativePixmapFactory to SurfaceFactoryOzone; After that I don't think the
> gpu process will use ClientNativePixmapFactory.

Done.

[william.xie, 2016-01-19 04:43:33]

The CQ bit was checked by william.xie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-19 04:43:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1598573002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1598573002/20001

[commit-bot: I haz the power, 2016-01-19 04:43:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-19 04:43:45]

Dry run: The author william.xie@chromium.org has not signed Google Contributor
License Agreement. Please visit https://cla.developers.google.com to sign and
manage CLA.

[william.xie, 2016-01-19 04:46:50]

The CQ bit was checked by william.xie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-19 04:46:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1598573002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1598573002/20001

[commit-bot: I haz the power, 2016-01-19 05:15:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-19 05:15:22]

Dry run: This issue passed the CQ dry run.

[kalyank, 2016-01-19 07:34:22]

https://codereview.chromium.org/1598573002/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/1598573002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:109:
drm_thread_->GetSupportedFormats(support_formats);
We can cache them here? no need to post a synchronous task for DrmThread every
time.

https://codereview.chromium.org/1598573002/diff/20001/ui/ozone/public/surface...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/1598573002/diff/20001/ui/ozone/public/surface...
ui/ozone/public/surface_factory_ozone.h:104: virtual void
GetSupportedFormats(std::vector<uint32_t>* support_formats);
We want to pass in the usage too i.e. BufferUsage ? The formats will differ
depending on intended usage.

[william.xie, 2016-01-19 10:07:07]

Thanks Kalyan, PTAL

https://codereview.chromium.org/1598573002/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/1598573002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:109:
drm_thread_->GetSupportedFormats(support_formats);
On 2016/01/19 07:34:22, kalyank wrote:
> We can cache them here? no need to post a synchronous task for DrmThread every
> time. 

Done.

https://codereview.chromium.org/1598573002/diff/20001/ui/ozone/public/surface...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/1598573002/diff/20001/ui/ozone/public/surface...
ui/ozone/public/surface_factory_ozone.h:104: virtual void
GetSupportedFormats(std::vector<uint32_t>* support_formats);
On 2016/01/19 07:34:22, kalyank wrote:
> We want to pass in the usage too i.e. BufferUsage ? The formats will differ
> depending on intended usage.
> 

Done.

[kalyank, 2016-01-19 16:00:37]

https://codereview.chromium.org/1598573002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/1598573002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:118: case
gfx::BufferUsage::SCANOUT:
I understand we currently return same format list for GPU_READ and SCANOUT, but
I think we should fix it. For SCANOUT we should return whatever PlaneManager
gives us. GPU_READ should be list of formats we can texture from ? @reveman?

[william.xie, 2016-01-20 01:53:49]

PTAL

https://codereview.chromium.org/1598573002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/1598573002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:118: case
gfx::BufferUsage::SCANOUT:
On 2016/01/19 16:00:37, kalyank wrote:
> I understand we currently return same format list for GPU_READ and SCANOUT,
but
> I think we should fix it. For SCANOUT we should return whatever PlaneManager
> gives us. GPU_READ should be list of formats we can texture from ? @reveman?

How about handling SCANOUT usage only for now and add other usage implementation
later?

[reveman, 2016-01-20 04:01:44]

https://codereview.chromium.org/1598573002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/1598573002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:118: case
gfx::BufferUsage::SCANOUT:
On 2016/01/20 at 01:53:49, william.xie wrote:
> On 2016/01/19 16:00:37, kalyank wrote:
> > I understand we currently return same format list for GPU_READ and SCANOUT,
but
> > I think we should fix it. For SCANOUT we should return whatever PlaneManager
> > gives us. GPU_READ should be list of formats we can texture from ? @reveman?
> 
> How about handling SCANOUT usage only for now and add other usage
implementation later?

That would break exo/ usage of native buffers so not really an option.

[william.xie, 2016-01-20 15:14:41]

On 2016/01/20 04:01:44, reveman wrote:
>
https://codereview.chromium.org/1598573002/diff/40001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):
> 
>
https://codereview.chromium.org/1598573002/diff/40001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:118: case
> gfx::BufferUsage::SCANOUT:
> On 2016/01/20 at 01:53:49, william.xie wrote:
> > On 2016/01/19 16:00:37, kalyank wrote:
> > > I understand we currently return same format list for GPU_READ and
SCANOUT,
> but
> > > I think we should fix it. For SCANOUT we should return whatever
PlaneManager
> > > gives us. GPU_READ should be list of formats we can texture from ?
@reveman?
> > 
> > How about handling SCANOUT usage only for now and add other usage
> implementation later?
> 
> That would break exo/ usage of native buffers so not really an option.

Thanks @reveman, can we just return the same list for GPU_READ as SCANOUT?

[spang, 2016-01-20 18:52:24]

https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_thread.h (right):

https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.h:62: void
GetSupportedFormats(std::vector<uint32_t>* support_formats);
Document what the uint32_t means.

In fact, could this API just use gfx::BufferFormat?

https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_thread_proxy.cc (right):

https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread_proxy.cc:43: std::vector<uint32_t>*
support_formats) {
Be consistent with naming: supported_formats

https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:107: bool
GbmSurfaceFactory::GetSupportedFormats(
Why does this take a usage parameter?

This is checking formats supported by the display controller, so it's only
relevant for scanout.

It seems like 

  std::vector<gfx::BufferFormat> GetScanoutFormats();

is what you want.

But it's concerning that this API is not parameterized by display device. It'll
break if some secondary device doesn't have all the formats of the primary
device, right?

[william.xie, 2016-01-21 02:14:02]

PTAL

https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_thread.h (right):

https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.h:62: void
GetSupportedFormats(std::vector<uint32_t>* support_formats);
On 2016/01/20 18:52:24, spang wrote:
> Document what the uint32_t means.
> 
> In fact, could this API just use gfx::BufferFormat?

Done.

https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_thread_proxy.cc (right):

https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread_proxy.cc:43: std::vector<uint32_t>*
support_formats) {
On 2016/01/20 18:52:24, spang wrote:
> Be consistent with naming: supported_formats

Done.

https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:107: bool
GbmSurfaceFactory::GetSupportedFormats(
On 2016/01/20 18:52:24, spang wrote:
> Why does this take a usage parameter?
> 
> This is checking formats supported by the display controller, so it's only
> relevant for scanout.
> 
> It seems like 
> 
>   std::vector<gfx::BufferFormat> GetScanoutFormats();
> 
> is what you want.
> 
> But it's concerning that this API is not parameterized by display device.
It'll
> break if some secondary device doesn't have all the formats of the primary
> device, right?

Thanks Michael, I documented the concern in the API header file to notify the
user be careful for that.

[spang, 2016-01-21 14:59:41]

On 2016/01/21 02:14:02, william.xie wrote:
> PTAL
> 
>
https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/drm_thread.h (right):
> 
>
https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/drm_thread.h:62: void
> GetSupportedFormats(std::vector<uint32_t>* support_formats);
> On 2016/01/20 18:52:24, spang wrote:
> > Document what the uint32_t means.
> > 
> > In fact, could this API just use gfx::BufferFormat?
> 
> Done.
> 
>
https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/drm_thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/drm_thread_proxy.cc:43: std::vector<uint32_t>*
> support_formats) {
> On 2016/01/20 18:52:24, spang wrote:
> > Be consistent with naming: supported_formats
> 
> Done.
> 
>
https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):
> 
>
https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:107: bool
> GbmSurfaceFactory::GetSupportedFormats(
> On 2016/01/20 18:52:24, spang wrote:
> > Why does this take a usage parameter?
> > 
> > This is checking formats supported by the display controller, so it's only
> > relevant for scanout.
> > 
> > It seems like 
> > 
> >   std::vector<gfx::BufferFormat> GetScanoutFormats();
> > 
> > is what you want.
> > 
> > But it's concerning that this API is not parameterized by display device.
> It'll
> > break if some secondary device doesn't have all the formats of the primary
> > device, right?
> 
> Thanks Michael, I documented the concern in the API header file to notify the
> user be careful for that.

Why not add a gfx::AcceleratedWidget parameter? Then at least the device is
unambiguous.

[william.xie, 2016-01-21 15:25:21]

On 2016/01/21 14:59:41, spang wrote:
> On 2016/01/21 02:14:02, william.xie wrote:
> > PTAL
> > 
> >
>
https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
> > File ui/ozone/platform/drm/gpu/drm_thread.h (right):
> > 
> >
>
https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
> > ui/ozone/platform/drm/gpu/drm_thread.h:62: void
> > GetSupportedFormats(std::vector<uint32_t>* support_formats);
> > On 2016/01/20 18:52:24, spang wrote:
> > > Document what the uint32_t means.
> > > 
> > > In fact, could this API just use gfx::BufferFormat?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
> > File ui/ozone/platform/drm/gpu/drm_thread_proxy.cc (right):
> > 
> >
>
https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
> > ui/ozone/platform/drm/gpu/drm_thread_proxy.cc:43: std::vector<uint32_t>*
> > support_formats) {
> > On 2016/01/20 18:52:24, spang wrote:
> > > Be consistent with naming: supported_formats
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
> > File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/1598573002/diff/60001/ui/ozone/platform/drm/g...
> > ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:107: bool
> > GbmSurfaceFactory::GetSupportedFormats(
> > On 2016/01/20 18:52:24, spang wrote:
> > > Why does this take a usage parameter?
> > > 
> > > This is checking formats supported by the display controller, so it's only
> > > relevant for scanout.
> > > 
> > > It seems like 
> > > 
> > >   std::vector<gfx::BufferFormat> GetScanoutFormats();
> > > 
> > > is what you want.
> > > 
> > > But it's concerning that this API is not parameterized by display device.
> > It'll
> > > break if some secondary device doesn't have all the formats of the primary
> > > device, right?
> > 
> > Thanks Michael, I documented the concern in the API header file to notify
the
> > user be careful for that.
> 
> Why not add a gfx::AcceleratedWidget parameter? Then at least the device is
> unambiguous.

Good suggestion, thanks Michael, PTAL

[william.xie, 2016-01-21 15:25:32]

PTAL

[william.xie, 2016-01-21 15:29:58]

Patchset #6 (id:100001) has been deleted

[kalyank, 2016-01-22 06:41:25]

https://codereview.chromium.org/1598573002/diff/120001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/1598573002/diff/120001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:109: if
(scanout_formats_.empty())
This now looks wrong as widget might be associated with a different device and
cached formats might not be supported. We would need to query this every time by
posting a task to DrmThread, like in your initial patch.

https://codereview.chromium.org/1598573002/diff/120001/ui/ozone/public/surfac...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/1598573002/diff/120001/ui/ozone/public/surfac...
ui/ozone/public/surface_factory_ozone.h:105: // have all the formats of the
primary device.
Nit: I think you can remove the comment about secondary device as we now check
for the display controller to which |window| is associated and return the
formats supported by it. Also, PlaneManager is bit confusing here and everywhere
else, comments similar to what we have for CreateNativePixmap below should be
good ?

[william.xie, 2016-01-23 01:51:53]

Thanks Kalyan, PTAL

https://codereview.chromium.org/1598573002/diff/120001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/1598573002/diff/120001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:109: if
(scanout_formats_.empty())
On 2016/01/22 06:41:25, kalyank wrote:
> This now looks wrong as widget might be associated with a different device and
> cached formats might not be supported. We would need to query this every time
by
> posting a task to DrmThread, like in your initial patch. 

Done.

https://codereview.chromium.org/1598573002/diff/120001/ui/ozone/public/surfac...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/1598573002/diff/120001/ui/ozone/public/surfac...
ui/ozone/public/surface_factory_ozone.h:105: // have all the formats of the
primary device.
On 2016/01/22 06:41:25, kalyank wrote:
> Nit: I think you can remove the comment about secondary device as we now check
> for the display controller to which |window| is associated and return the
> formats supported by it. Also, PlaneManager is bit confusing here and
everywhere
> else, comments similar to what we have for CreateNativePixmap below should be
> good ?

Done.

[william.xie, 2016-01-23 01:52:30]

Description was changed from

==========
Propagate supported formats from driver to SurfaceFactoryOzone.

BUG=None
==========

to

==========
Propagate supported scanout formats from driver to SurfaceFactoryOzone.

BUG=None
==========

[spang, 2016-01-25 18:35:14]

lgtm

[reveman, 2016-01-25 18:59:18]

lgtm

[kalyank, 2016-01-25 20:11:19]

non owner lgtm

[william.xie, 2016-01-26 00:43:10]

The CQ bit was checked by william.xie@chromium.org

[commit-bot: I haz the power, 2016-01-26 00:44:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1598573002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1598573002/140001

[commit-bot: I haz the power, 2016-01-26 01:14:58]

Description was changed from

==========
Propagate supported scanout formats from driver to SurfaceFactoryOzone.

BUG=None
==========

to

==========
Propagate supported scanout formats from driver to SurfaceFactoryOzone.

BUG=None
==========

[commit-bot: I haz the power, 2016-01-26 01:14:59]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-01-26 01:17:08]

Description was changed from

==========
Propagate supported scanout formats from driver to SurfaceFactoryOzone.

BUG=None
==========

to

==========
Propagate supported scanout formats from driver to SurfaceFactoryOzone.

BUG=None

Committed: https://crrev.com/f9e3001545d09eb9579a13ca3fc76331c2d61546
Cr-Commit-Position: refs/heads/master@{#371396}
==========

[commit-bot: I haz the power, 2016-01-26 01:17:09]

Patchset 7 (id:??) landed as
https://crrev.com/f9e3001545d09eb9579a13ca3fc76331c2d61546
Cr-Commit-Position: refs/heads/master@{#371396}