<div dir="ltr"><div>So this series is back in the review process. Only the last 4 patches need an Rb and it's good to go. </div>Marek </div><div class="gmail_extra"> <div class="gmail_quote">On Fri, Apr 27, 2018 at 4:29 PM, Nicolai Hähnle <<a href="mailto:nhaehnle@gmail.com" target="_blank">nhaehnle@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 27.04.2018 19:21, Gurchetan Singh wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Fri, Apr 27, 2018 at 2:00 AM, Nicolai Hähnle <<a href="mailto:nhaehnle@gmail.com" target="_blank">nhaehnle@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote> The DRI backend maps the entire texture currently: <a href="https://chromium.googlesource.com/chromiumos/platform/minigbm/+/master/dri.c#266" rel="noreferrer" target="_blank">https://chromium.googlesource.com/chromiumos/platform/minigbm/+/master/dri.c#266</a> This is consistent with what other drivers do as well, since GEM ioctls (i.e, DRM_IOCTL_I915_GEM_MMAP_GTT) don't support partial mappings. I assume the AMD map ioctl also maps the entire buffer, but the shadow buffer is smaller. </blockquote> I assume that those other drivers don't have to copy pixels as part of their map / unmap, so for them mapping the entire texture is okay, since there is no performance/power difference either way. We do have to actually copy pixels during map / unmap, so mapping the entire texture is a no-go if we're not actually going to use it all. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote> We modified minigbm to not cache mappings for AMD devices (<a href="http://crrev.com/c/990892" rel="noreferrer" target="_blank">crrev.com/c/990892</a>). </blockquote> Yes, I see now, thanks for that. Cheers, Nicolai<div class="HOEnZb"><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Cheers, Nicolai -- Lerne, wie die Welt wirklich ist, Aber vergiss niemals, wie sie sein sollte. </blockquote></blockquote> -- Lerne, wie die Welt wirklich ist, Aber vergiss niemals, wie sie sein sollte. </div></div></blockquote></div> </div>