ozone: Create GbmBuffers from fourcc formats and gbm flags.

ozone: Create GbmBuffers from fourcc formats and gbm flags.

This CL changes the interface of GbmBuffer to use fourcc formats and
gbm flags instead of gfx::Buffer{Format,Usage}.

In this way we can create modeset buffers that can be mapped and can
be scanned out without the need to add a new gfx::BufferUsage.

BUG=chrome-os-partner:56407
Committed: https://crrev.com/2be041a887dffe27d886225be13d69160115dd12
Cr-Commit-Position: refs/heads/master@{#435368}

[Daniele Castagna, 2016-11-29 00:04:05]

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

[commit-bot: I haz the power, 2016-11-29 00:05:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniele Castagna, 2016-11-29 00:08:10]

Description was changed from

==========
ozone: Create GbmBuffers from fourcc format and gbm flags.

This CL changes the interface of GbmBuffer to use fourcc format and
gbm flags instead of gfx::Buffer{Format,Usage}.

In this was we can create modeset buffers that can be mapped and can
be scanned out without the need to add a new gfx::BufferUsage.

BUG=chrome-os-partner:56407
==========

to

==========
ozone: Create GbmBuffers from fourcc formats and gbm flags.

This CL changes the interface of GbmBuffer to use fourcc formats and
gbm flags instead of gfx::Buffer{Format,Usage}.

In this way we can create modeset buffers that can be mapped and can
be scanned out without the need to add a new gfx::BufferUsage.

BUG=chrome-os-partner:56407
==========

[Daniele Castagna, 2016-11-29 00:22:37]

dcastagna@chromium.org changed reviewers:
+ hoegsberg@chromium.org, reveman@chromium.org

[Daniele Castagna, 2016-11-29 01:37:07]

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

[commit-bot: I haz the power, 2016-11-29 01:37:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 03:10:49]

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

[commit-bot: I haz the power, 2016-11-29 03:10:50]

Dry run: This issue passed the CQ dry run.

[hoegsberg1, 2016-11-30 01:06:17]

hoegsberg@google.com changed reviewers:
+ hoegsberg@google.com

[hoegsberg1, 2016-11-30 01:06:18]

This looks good.

[Daniele Castagna, 2016-11-30 01:33:44]

dcastagna@chromium.org changed reviewers:
+ dnicoara@chromium.org

[Daniele Castagna, 2016-11-30 01:33:45]

Thank you for taking a look Kristian.

+dnicoara for review.
crrev.com/2535613002 has some context on why we decided not to add a
gfx::BufferUsage but we preferred to change GbmBuffer API in this way instead.

[dnicoara, 2016-11-30 15:12:02]

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

https://codereview.chromium.org/2533163002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:114: unsigned flags = 0;
nit: Make this a uint32_t.

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

https://codereview.chromium.org/2533163002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:204: for (size_t i = 0; i <
gfx::NumberOfPlanesForBufferFormat(format); ++i) {
Wondering why gbm_bo_get_num_planes() can't be used right now? Especially since
it is being used above in line 111.

[Daniele Castagna, 2016-11-30 18:41:34]

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

[Daniele Castagna, 2016-11-30 18:41:38]

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

https://codereview.chromium.org/2533163002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:114: unsigned flags = 0;
On 2016/11/30 at 15:12:01, dnicoara wrote:
> nit: Make this a uint32_t.

Done.

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

https://codereview.chromium.org/2533163002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:204: for (size_t i = 0; i <
gfx::NumberOfPlanesForBufferFormat(format); ++i) {
On 2016/11/30 at 15:12:01, dnicoara wrote:
> Wondering why gbm_bo_get_num_planes() can't be used right now? Especially
since it is being used above in line 111.

We don't always have a gbm bo pointer. There are some formats that we import but
can't be gbm imported, so no bo for those.

We can use gbm_bo_get_num_planes at line 111 since that is after allocating a
buffer via gbm, and we know for sure we have a bo at that point.

Updated the TODO for clarity.

[commit-bot: I haz the power, 2016-11-30 18:42:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dnicoara, 2016-11-30 18:44:15]

lgtm

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

https://codereview.chromium.org/2533163002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:204: for (size_t i = 0; i <
gfx::NumberOfPlanesForBufferFormat(format); ++i) {
On 2016/11/30 18:41:38, Daniele Castagna wrote:
> On 2016/11/30 at 15:12:01, dnicoara wrote:
> > Wondering why gbm_bo_get_num_planes() can't be used right now? Especially
> since it is being used above in line 111.
> 
> We don't always have a gbm bo pointer. There are some formats that we import
but
> can't be gbm imported, so no bo for those.
> 
> We can use gbm_bo_get_num_planes at line 111 since that is after allocating a
> buffer via gbm, and we know for sure we have a bo at that point.
> 
> Updated the TODO for clarity.

Ahh, thank you for clarifying.

[commit-bot: I haz the power, 2016-11-30 19:06:19]

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

[commit-bot: I haz the power, 2016-11-30 19:06:20]

Dry run: This issue passed the CQ dry run.

[Daniele Castagna, 2016-11-30 19:06:57]

The CQ bit was checked by dcastagna@chromium.org

[commit-bot: I haz the power, 2016-11-30 19:07:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 19:15:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480532817886320,
"parent_rev": "61bef364057cf5fb2761fcbdc9ba33ffbe62ca26", "commit_rev":
"64674b9223f3d45f233161df447c4ca74dc8ef5c"}

[commit-bot: I haz the power, 2016-11-30 19:16:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-30 19:21:28]

Description was changed from

==========
ozone: Create GbmBuffers from fourcc formats and gbm flags.

This CL changes the interface of GbmBuffer to use fourcc formats and
gbm flags instead of gfx::Buffer{Format,Usage}.

In this way we can create modeset buffers that can be mapped and can
be scanned out without the need to add a new gfx::BufferUsage.

BUG=chrome-os-partner:56407
==========

to

==========
ozone: Create GbmBuffers from fourcc formats and gbm flags.

This CL changes the interface of GbmBuffer to use fourcc formats and
gbm flags instead of gfx::Buffer{Format,Usage}.

In this way we can create modeset buffers that can be mapped and can
be scanned out without the need to add a new gfx::BufferUsage.

BUG=chrome-os-partner:56407

Committed: https://crrev.com/2be041a887dffe27d886225be13d69160115dd12
Cr-Commit-Position: refs/heads/master@{#435368}
==========

[commit-bot: I haz the power, 2016-11-30 19:21:29]

Patchset 3 (id:??) landed as
https://crrev.com/2be041a887dffe27d886225be13d69160115dd12
Cr-Commit-Position: refs/heads/master@{#435368}

[tfiga, 2016-12-06 06:16:31]

tfiga@chromium.org changed reviewers:
+ tfiga@chromium.org

[tfiga, 2016-12-06 06:16:32]

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

https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:44: GBM_BO_USE_SCANOUT |
GBM_BO_USE_LINEAR);
Why GBM_BO_USE_SCANOUT | GBM_BO_USE_LINEAR? First of all, GBM_BO_USE_RENDERING
should be necessary for the GPU to actually write anything to this buffer. Then
some platforms (e.g. Intel) can actually scan out of tiled buffers, which is a
significant performance optimization, so forcing linear doesn't make much sense.

[Daniele Castagna, 2016-12-06 06:31:01]

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

https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:44: GBM_BO_USE_SCANOUT |
GBM_BO_USE_LINEAR);
On 2016/12/06 at 06:16:32, tfiga wrote:
> Why GBM_BO_USE_SCANOUT | GBM_BO_USE_LINEAR? First of all, GBM_BO_USE_RENDERING
should be necessary for the GPU to actually write anything to this buffer. Then
some platforms (e.g. Intel) can actually scan out of tiled buffers, which is a
significant performance optimization, so forcing linear doesn't make much sense.

The GPU is not writing to this buffer. We're mapping it and writing to it via SW
skia.

I don't think we care about performance in this case, this allocation is for
buffers that are used only for modesetting after we change the display mode.

[tfiga, 2016-12-06 06:50:57]

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

https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:44: GBM_BO_USE_SCANOUT |
GBM_BO_USE_LINEAR);
On 2016/12/06 06:31:01, Daniele Castagna wrote:
> On 2016/12/06 at 06:16:32, tfiga wrote:
> > Why GBM_BO_USE_SCANOUT | GBM_BO_USE_LINEAR? First of all,
GBM_BO_USE_RENDERING
> should be necessary for the GPU to actually write anything to this buffer.
Then
> some platforms (e.g. Intel) can actually scan out of tiled buffers, which is a
> significant performance optimization, so forcing linear doesn't make much
sense.
> 
> The GPU is not writing to this buffer. We're mapping it and writing to it via
SW
> skia.
> 
> I don't think we care about performance in this case, this allocation is for
> buffers that are used only for modesetting after we change the display mode.

Acknowledged.

[tfiga, 2016-12-06 07:20:37]

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

https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/screen_manager_unittest.cc:503: drm_,
DRM_FORMAT_XRGB8888, GetPrimaryBounds().size());
Shouldn't the new format be DRM_FORMAT_ARGB8888?

[Daniele Castagna, 2016-12-06 18:31:10]

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

https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/screen_manager_unittest.cc:503: drm_,
DRM_FORMAT_XRGB8888, GetPrimaryBounds().size());
On 2016/12/06 at 07:20:37, tfiga wrote:
> Shouldn't the new format be DRM_FORMAT_ARGB8888?

At this point Chrome doesn't support overlays with alpha. That means if a gfx
format with alpha arrives here we know we can discard the alpha channel and we
use a fourcc format without alpha to create the framebuffer. You can see that in
drm_util.cc GetFourCCFormatForFramebuffer.

Since we changed the gbmbuffer format from gfx::BufferFormat to a fourcc format,
this change reflects that.

[marcheu1, 2016-12-13 00:07:05]

marcheu@google.com changed reviewers:
+ marcheu@google.com

[marcheu1, 2016-12-13 00:07:06]

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

https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:44: GBM_BO_USE_SCANOUT |
GBM_BO_USE_LINEAR);
On 2016/12/06 06:50:57, tfiga wrote:
> On 2016/12/06 06:31:01, Daniele Castagna wrote:
> > On 2016/12/06 at 06:16:32, tfiga wrote:
> > > Why GBM_BO_USE_SCANOUT | GBM_BO_USE_LINEAR? First of all,
> GBM_BO_USE_RENDERING
> > should be necessary for the GPU to actually write anything to this buffer.
> Then
> > some platforms (e.g. Intel) can actually scan out of tiled buffers, which is
a
> > significant performance optimization, so forcing linear doesn't make much
> sense.
> > 
> > The GPU is not writing to this buffer. We're mapping it and writing to it
via
> SW
> > skia.
> > 
> > I don't think we care about performance in this case, this allocation is for
> > buffers that are used only for modesetting after we change the display mode.
> 
> Acknowledged.

This is still wrong. You can't allocate a gem BO and expect to be able to map it
like a dumb buffer since these are two different things. There are two possible
ways to make this consistent:

1. allocate an actual dumb buffer with DRM_IOCTL_MODE_CREATE_DUMB (not through
gbm), then map it like a dumb buffer (which is what the code currently does).

2. allocate a GBM buffer with the LINEAR flag. In theory the LINEAR flag is not
needed because gbm_bo_map() gives you a linear view, except on Tegra where
that's not working because nvidia didn't implement it, and on Kevin where that
might not work because we don't know how to decompress AFBC. You need to then
change the code which maps the buffer to use gbm_bo_map() instead of
DRM_IOCTL_MODE_MAP_DUMB.

[marcheu1, 2016-12-13 00:09:42]

On 2016/12/13 00:07:06, marcheu1 wrote:
>
https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/drm_thread.cc (right):
> 
>
https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/drm_thread.cc:44: GBM_BO_USE_SCANOUT |
> GBM_BO_USE_LINEAR);
> On 2016/12/06 06:50:57, tfiga wrote:
> > On 2016/12/06 06:31:01, Daniele Castagna wrote:
> > > On 2016/12/06 at 06:16:32, tfiga wrote:
> > > > Why GBM_BO_USE_SCANOUT | GBM_BO_USE_LINEAR? First of all,
> > GBM_BO_USE_RENDERING
> > > should be necessary for the GPU to actually write anything to this buffer.
> > Then
> > > some platforms (e.g. Intel) can actually scan out of tiled buffers, which
is
> a
> > > significant performance optimization, so forcing linear doesn't make much
> > sense.
> > > 
> > > The GPU is not writing to this buffer. We're mapping it and writing to it
> via
> > SW
> > > skia.
> > > 
> > > I don't think we care about performance in this case, this allocation is
for
> > > buffers that are used only for modesetting after we change the display
mode.
> > 
> > Acknowledged.
> 
> This is still wrong. You can't allocate a gem BO and expect to be able to map
it
> like a dumb buffer since these are two different things. There are two
possible
> ways to make this consistent:
> 
> 1. allocate an actual dumb buffer with DRM_IOCTL_MODE_CREATE_DUMB (not through
> gbm), then map it like a dumb buffer (which is what the code currently does).

Oh and I forgot to add here, since the dumb buffer and the gbm buffer do not
necessarily have the same sizes, we can't page flip from one to the other. So
solution 1. will also have to include a postponed modeset to switch over to gbm
buffers.

> 
> 2. allocate a GBM buffer with the LINEAR flag. In theory the LINEAR flag is
not
> needed because gbm_bo_map() gives you a linear view, except on Tegra where
> that's not working because nvidia didn't implement it, and on Kevin where that
> might not work because we don't know how to decompress AFBC. You need to then
> change the code which maps the buffer to use gbm_bo_map() instead of
> DRM_IOCTL_MODE_MAP_DUMB.

[hoegsberg1, 2016-12-13 00:11:42]

On 2016/12/13 00:07:06, marcheu1 wrote:
>
https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/drm_thread.cc (right):
> 
>
https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/drm_thread.cc:44: GBM_BO_USE_SCANOUT |
> GBM_BO_USE_LINEAR);
> On 2016/12/06 06:50:57, tfiga wrote:
> > On 2016/12/06 06:31:01, Daniele Castagna wrote:
> > > On 2016/12/06 at 06:16:32, tfiga wrote:
> > > > Why GBM_BO_USE_SCANOUT | GBM_BO_USE_LINEAR? First of all,
> > GBM_BO_USE_RENDERING
> > > should be necessary for the GPU to actually write anything to this buffer.
> > Then
> > > some platforms (e.g. Intel) can actually scan out of tiled buffers, which
is
> a
> > > significant performance optimization, so forcing linear doesn't make much
> > sense.
> > > 
> > > The GPU is not writing to this buffer. We're mapping it and writing to it
> via
> > SW
> > > skia.
> > > 
> > > I don't think we care about performance in this case, this allocation is
for
> > > buffers that are used only for modesetting after we change the display
mode.
> > 
> > Acknowledged.
> 
> This is still wrong. You can't allocate a gem BO and expect to be able to map
it
> like a dumb buffer since these are two different things. There are two
possible
> ways to make this consistent:
> 
> 1. allocate an actual dumb buffer with DRM_IOCTL_MODE_CREATE_DUMB (not through
> gbm), then map it like a dumb buffer (which is what the code currently does).
> 
> 2. allocate a GBM buffer with the LINEAR flag. In theory the LINEAR flag is
not
> needed because gbm_bo_map() gives you a linear view, except on Tegra where
> that's not working because nvidia didn't implement it, and on Kevin where that
> might not work because we don't know how to decompress AFBC. You need to then
> change the code which maps the buffer to use gbm_bo_map() instead of
> DRM_IOCTL_MODE_MAP_DUMB.

Are you saying the patch is wrong? It's doing what you describe in 2. Also the
LINEAR flag is required to be able to map it at all. That's what the use flags
are for: they describe way you intend to use the buffer; mapping it is one way
to use it.  As we've discussed before, maybe LINEAR is the wrong name for the
flag it should be GBM_BO_USE_MAP or something like that.

[marcheu1, 2016-12-13 00:15:47]

On 2016/12/13 00:11:42, hoegsberg1 wrote:
> On 2016/12/13 00:07:06, marcheu1 wrote:
> >
>
https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
> > File ui/ozone/platform/drm/gpu/drm_thread.cc (right):
> > 
> >
>
https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
> > ui/ozone/platform/drm/gpu/drm_thread.cc:44: GBM_BO_USE_SCANOUT |
> > GBM_BO_USE_LINEAR);
> > On 2016/12/06 06:50:57, tfiga wrote:
> > > On 2016/12/06 06:31:01, Daniele Castagna wrote:
> > > > On 2016/12/06 at 06:16:32, tfiga wrote:
> > > > > Why GBM_BO_USE_SCANOUT | GBM_BO_USE_LINEAR? First of all,
> > > GBM_BO_USE_RENDERING
> > > > should be necessary for the GPU to actually write anything to this
buffer.
> > > Then
> > > > some platforms (e.g. Intel) can actually scan out of tiled buffers,
which
> is
> > a
> > > > significant performance optimization, so forcing linear doesn't make
much
> > > sense.
> > > > 
> > > > The GPU is not writing to this buffer. We're mapping it and writing to
it
> > via
> > > SW
> > > > skia.
> > > > 
> > > > I don't think we care about performance in this case, this allocation is
> for
> > > > buffers that are used only for modesetting after we change the display
> mode.
> > > 
> > > Acknowledged.
> > 
> > This is still wrong. You can't allocate a gem BO and expect to be able to
map
> it
> > like a dumb buffer since these are two different things. There are two
> possible
> > ways to make this consistent:
> > 
> > 1. allocate an actual dumb buffer with DRM_IOCTL_MODE_CREATE_DUMB (not
through
> > gbm), then map it like a dumb buffer (which is what the code currently
does).
> > 
> > 2. allocate a GBM buffer with the LINEAR flag. In theory the LINEAR flag is
> not
> > needed because gbm_bo_map() gives you a linear view, except on Tegra where
> > that's not working because nvidia didn't implement it, and on Kevin where
that
> > might not work because we don't know how to decompress AFBC. You need to
then
> > change the code which maps the buffer to use gbm_bo_map() instead of
> > DRM_IOCTL_MODE_MAP_DUMB.
> 
> Are you saying the patch is wrong? It's doing what you describe in 2. Also the
> LINEAR flag is required to be able to map it at all. That's what the use flags
> are for: they describe way you intend to use the buffer; mapping it is one way
> to use it.  As we've discussed before, maybe LINEAR is the wrong name for the
> flag it should be GBM_BO_USE_MAP or something like that.

IIRC the code that maps the existing screen and paints to it with skia is using
dumb buffers.

[hoegsberg1, 2016-12-13 00:22:36]

On 2016/12/13 00:15:47, marcheu1 wrote:
> On 2016/12/13 00:11:42, hoegsberg1 wrote:
> > On 2016/12/13 00:07:06, marcheu1 wrote:
> > >
> >
>
https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
> > > File ui/ozone/platform/drm/gpu/drm_thread.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
> > > ui/ozone/platform/drm/gpu/drm_thread.cc:44: GBM_BO_USE_SCANOUT |
> > > GBM_BO_USE_LINEAR);
> > > On 2016/12/06 06:50:57, tfiga wrote:
> > > > On 2016/12/06 06:31:01, Daniele Castagna wrote:
> > > > > On 2016/12/06 at 06:16:32, tfiga wrote:
> > > > > > Why GBM_BO_USE_SCANOUT | GBM_BO_USE_LINEAR? First of all,
> > > > GBM_BO_USE_RENDERING
> > > > > should be necessary for the GPU to actually write anything to this
> buffer.
> > > > Then
> > > > > some platforms (e.g. Intel) can actually scan out of tiled buffers,
> which
> > is
> > > a
> > > > > significant performance optimization, so forcing linear doesn't make
> much
> > > > sense.
> > > > > 
> > > > > The GPU is not writing to this buffer. We're mapping it and writing to
> it
> > > via
> > > > SW
> > > > > skia.
> > > > > 
> > > > > I don't think we care about performance in this case, this allocation
is
> > for
> > > > > buffers that are used only for modesetting after we change the display
> > mode.
> > > > 
> > > > Acknowledged.
> > > 
> > > This is still wrong. You can't allocate a gem BO and expect to be able to
> map
> > it
> > > like a dumb buffer since these are two different things. There are two
> > possible
> > > ways to make this consistent:
> > > 
> > > 1. allocate an actual dumb buffer with DRM_IOCTL_MODE_CREATE_DUMB (not
> through
> > > gbm), then map it like a dumb buffer (which is what the code currently
> does).
> > > 
> > > 2. allocate a GBM buffer with the LINEAR flag. In theory the LINEAR flag
is
> > not
> > > needed because gbm_bo_map() gives you a linear view, except on Tegra where
> > > that's not working because nvidia didn't implement it, and on Kevin where
> that
> > > might not work because we don't know how to decompress AFBC. You need to
> then
> > > change the code which maps the buffer to use gbm_bo_map() instead of
> > > DRM_IOCTL_MODE_MAP_DUMB.
> > 
> > Are you saying the patch is wrong? It's doing what you describe in 2. Also
the
> > LINEAR flag is required to be able to map it at all. That's what the use
flags
> > are for: they describe way you intend to use the buffer; mapping it is one
way
> > to use it.  As we've discussed before, maybe LINEAR is the wrong name for
the
> > flag it should be GBM_BO_USE_MAP or something like that.
> 
> IIRC the code that maps the existing screen and paints to it with skia is
using
> dumb buffers.

Ah right, we talked about that a bit when Daniele was here last week. The code
does assume that the existing framebuffer is a plain linear buffer that can be
mapped. That's not generally safe, but for the initial modeset we're coming from
frecon and it's safe. It does break down when we plug in an external monitor and
try to reuse the AFBC encoded previous fb on little VOP on eDP. We get a flicker
raw AFBC data. We're still trying to figure out what the best approach is - the
delayed modeset idea would solve these issues...

[Daniele Castagna, 2016-12-13 02:28:59]

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

https://codereview.chromium.org/2533163002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:44: GBM_BO_USE_SCANOUT |
GBM_BO_USE_LINEAR);
On 2016/12/13 at 00:07:06, marcheu1 wrote:
> On 2016/12/06 06:50:57, tfiga wrote:
> > On 2016/12/06 06:31:01, Daniele Castagna wrote:
> > > On 2016/12/06 at 06:16:32, tfiga wrote:
> > > > Why GBM_BO_USE_SCANOUT | GBM_BO_USE_LINEAR? First of all,
> > GBM_BO_USE_RENDERING
> > > should be necessary for the GPU to actually write anything to this buffer.
> > Then
> > > some platforms (e.g. Intel) can actually scan out of tiled buffers, which
is a
> > > significant performance optimization, so forcing linear doesn't make much
> > sense.
> > > 
> > > The GPU is not writing to this buffer. We're mapping it and writing to it
via
> > SW
> > > skia.
> > > 
> > > I don't think we care about performance in this case, this allocation is
for
> > > buffers that are used only for modesetting after we change the display
mode.
> > 
> > Acknowledged.
> 
> This is still wrong. You can't allocate a gem BO and expect to be able to map
it like a dumb buffer since these are two different things. There are two
possible ways to make this consistent:
> 
> 1. allocate an actual dumb buffer with DRM_IOCTL_MODE_CREATE_DUMB (not through
gbm), then map it like a dumb buffer (which is what the code currently does).
> 
> 2. allocate a GBM buffer with the LINEAR flag. In theory the LINEAR flag is
not needed because gbm_bo_map() gives you a linear view, except on Tegra where
that's not working because nvidia didn't implement it, and on Kevin where that
might not work because we don't know how to decompress AFBC. You need to then
change the code which maps the buffer to use gbm_bo_map() instead of
DRM_IOCTL_MODE_MAP_DUMB.

As discussed offline, let's start landing crrev.com/2568243004 as a temporary
fix for the regression described in crbug.com/673026