[Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout

[Jason Ekstrand]

On Wed, Feb 14, 2018 at 8:43 AM, Daniel Stone <daniel at fooishbar.org> wrote:

> On 14 February 2018 at 16:27, Jason Ekstrand <jason at jlekstrand.net> wrote:
> > On Wed, Feb 14, 2018 at 3:39 AM, Daniel Stone <daniel at fooishbar.org>
> wrote:
> >> > On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone <daniel at fooishbar.org>
> wrote:
> >> >> For actual scanout, the kernel very specifically demands that if the
> >> >> BO is X-tiled, then set_tiling be called with TILING_X. This applies
> >> >> even if we explicitly allocate with MOD_X_TILED and pass that in via
> >> >> drmModeAddFB2WithModifiers: there is an explicit check for
> >> >> !!(bo_tiling == TILING_X) == !!(modifier == MOD_X_TILED).
> >>
> >> You missed this bit. Suggested fixup: https://hastebin.com/xikopobiza
> >
> > I was really hoping I'd read wrong.  I'm going with "that's a kernel
> bug".
> > Modifiers is supposed to separate tiling from the BO.  Tying them back
> > together in drmModeAddFB2WithModifiers is wrong.  This is especially true
> > since SET_TILING is an inherently racy operation and we really need to
> stop
> > using it entirely.
> >
> > If what you wrote above really is immutably true, then this patch is
> dead.
>
> It is immutably true, but I think this patch (with ugly fixup) still
> makes sense.
>

That's unfortunate.  Yeah, I agree that we should only set_tiling in the
minimum case.


> Linear is unchanged as it's implicit. X tiling has to take the
> workaround, in case anyone wants to display it.


I'm not sure I buy that.  From a userspace perspective, you shouldn't be
using modifiers unless everything supports them.  Why?  Because no
userspace that isn't an Intel driver should look at I915_FORMAT_MOD_X_TILED
and say "I can hand that off without modifiers just fine".


> Y tiling can skip it
> though, and future Yf/etc tiling modes don't need to either extend the
> I915_TILING_* enum (eww), or be explicitly set to linear when they're
> not. I think there's value in having the code clarify, rather than it
> doing set_tiling everywhere, so people assuming it's still required.
>

Agreed.  We should set the minimum amount of tiling.


> I can kind of see why that ABI makes sense though. Having userspace
> explicitly call set_tiling(X) had the kernel magically imply that the
> FB created from that BO should be X-tiled. Given that ABI still
> unfortunately exists, enforcing coherency between old and new ways to
> declare an FB should be tiled, doesn't seem unreasonable.
>

Eh.  The whole point of modifiers is as an alternate mechanism.  Insisting
that they "match" is, to me, equivalent to insisting that you use both
which is silly.  Of course, anyone is free to argue in the opposite
direction.


> > What's the failure mode here?  We just don't flip?  Or the compositor
> wigs
> > out and crashes?
>
> AddFB never succeeds, so we can never flip.
>

Ok.  That helps.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180214/af465b97/attachment.html>