[Mesa-dev] [PATCH 02/10] dri_interface: add __DRI_IMAGE_TRANSFER_USER_STRIDE

[Gurchetan Singh]

On Thu, Apr 26, 2018 at 10:23 AM, Marek Olšák <maraeo at gmail.com> wrote:
> Gurchetan, can you confirm that the stride returned at alloc() is only used
> for CPU access and not for display programming?

It really depends on how you couple your HWComposer and gralloc HALs
together.  For example, drm_hwcomposer calls drmModeAddFB2 from with
the allocation time stride:

https://anongit.freedesktop.org/git/drm_hwcomposer.git

For the minigbm + wayland_hwcomposer use case, we have a
GRALLOC_DRM_GET_STRIDE call that returns the allocation time pixel
stride to the Wayland client.

We could:
      - Return the stride that works for display with GRALLOC_DRM_GET_STRIDE
      - Return the stride that works for CPU access with the alloc() call.

Just curious -- which Android app and/or test case is running into this issue?

> Thanks,
> Marek
>
> On Wed, Apr 25, 2018 at 11:23 PM, Marek Olšák <maraeo at gmail.com> wrote:
>>
>> Why would you want to modify it? It's exactly what you get when you map
>> it, but that stride can't be used for modesetting.
>>
>> Marek
>>
>>
>> On Wed, Apr 25, 2018 at 11:14 PM, Gurchetan Singh
>> <gurchetansingh at chromium.org> wrote:
>>>
>>> That sounds fine to me.  We can just modify the stride after
>>> dri_bo_create(..).
>>>
>>> On Wed, Apr 25, 2018 at 7:30 PM, Marek Olšák <maraeo at gmail.com> wrote:
>>> > On Wed, Apr 25, 2018 at 6:56 PM, Gurchetan Singh
>>> > <gurchetansingh at chromium.org> wrote:
>>> >>
>>> >> On Wed, Apr 25, 2018 at 2:16 PM, Marek Olšák <maraeo at gmail.com> wrote:
>>> >> > From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>> >> >
>>> >> > Allow the caller to specify the row stride (in bytes) with which an
>>> >> > image
>>> >> > should be mapped. Note that completely ignoring USER_STRIDE is a
>>> >> > valid
>>> >> > implementation of mapImage.
>>> >> >
>>> >> > This is horrible API design. Unfortunately, cros_gralloc does indeed
>>> >> > have
>>> >> > a horrible API design -- in that arbitrary images should be allowed
>>> >> > to
>>> >> > be
>>> >> > mapped with the stride that a linear image of the same width would
>>> >> > have.
>>> >>
>>> >> Yes, unfortunately the gralloc API doesn't return the stride when
>>> >> (*lock) is called.  However, the stride is returned during (*alloc).
>>> >> Currently, for the dri backend, minigbm uses
>>> >> __DRI_IMAGE_ATTRIB_STRIDE to compute the pixel stride given to
>>> >> Android.
>>> >>
>>> >> Is AMD seeing problems with the current approach (I haven't seen any
>>> >> bugs filed for this issue)?
>>> >>
>>> >> Another possible solution is to call mapImage()/unmapImage right after
>>> >> allocating the image, and use the stride returned by mapImage() to
>>> >> compute the pixel stride.  That could also fix whatever bugs AMD is
>>> >> seeing.
>>> >
>>> >
>>> > Thanks. You cleared it up to me. It looks like that everything we've
>>> > been
>>> > told so far is BS. This series isn't needed.
>>> >
>>> > The solution is to do this in the amdgpu minigbm backend at alloc time:
>>> >    stride = align(width * Bpp, 64);
>>> >
>>> > Later chips should change that to:
>>> >    stride = align(width * Bpp, 256);
>>> >
>>> > No querying needed. What do you think?
>>> >
>>> > Marek
>>
>>
>