On 26.04.2018 04:30, Marek Olšák wrote:
On Wed, Apr 25, 2018 at 6:56 PM, Gurchetan Singh
<gurchetansi...@chromium.org <mailto:gurchetansi...@chromium.org>> wrote:
On Wed, Apr 25, 2018 at 2:16 PM, Marek Olšák <mar...@gmail.com
<mailto:mar...@gmail.com>> wrote:
> From: Nicolai Hähnle <nicolai.haeh...@amd.com
<mailto:nicolai.haeh...@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?
I don't see how this approach can possibly work unless we always map the
whole texture.
The whole problem with the fixed stride is that in many cases, we
allocate a staging texture. The reasonable thing to do for a sane API
(i.e., everything except ChromeOS) is to allocate a staging texture that
is sized correctly for the area that you want to map. But since we don't
know at texture allocation time what sizes of the texture will be
mapped, we cannot do that.
That was the whole point of the USER_STRIDE business. There are two
alternatives I can see:
1. Change minigbm so that it always maps the entire texture regardless
of what the caller requests. This is a very simple fix, but it has the
downside that the driver will always blit the entire surface to staging
even if only a single pixel is needed. That can obviously bite us for
performance and power, which is why I didn't want to go down that route.
2. Instead of having a USER_STRIDE interface, just simplify the story to
saying that whenever a transfer allocates a staging texture, it should
allocate a texture sized for the entire texture, but perform blits only
for the region that the user has requested.
This ends up being the same thing in the end, but perhaps it's better as
an interface because it's more explicit about what's happening.
By the way: the fact that gralloc wants to *cache* mappings will also
cause us pain, but that's perhaps something we should really just change
in gralloc.
Cheers,
Nicolai
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
