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

[Jason Ekstrand]

On Wed, Feb 14, 2018 at 9:42 AM, Ville Syrjälä <
ville.syrj...@linux.intel.com> wrote:

> On Wed, Feb 14, 2018 at 04:43:15PM +, Daniel Stone wrote:
> > On 14 February 2018 at 16:27, Jason Ekstrand 
> wrote:
> > > On Wed, Feb 14, 2018 at 3:39 AM, Daniel Stone 
> wrote:
> > >> > On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone <
> dan...@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".
>
> Old kernels required TILING_X with MOD_X.


How many kernels is that?


> I changed that at some
> point to allow TILING_NONE with any modifier, but otherwise we
> require the BO tiling to match the modifier. Ie. you still can't
> mix TILING_Y + MOD_X for example.
>

Right.  That's perfectly reasonable.  It should be either TILING_NONE or
match.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ville Syrjälä]

On Wed, Feb 14, 2018 at 04:43:15PM +, Daniel Stone wrote:
> On 14 February 2018 at 16:27, Jason Ekstrand  wrote:
> > On Wed, Feb 14, 2018 at 3:39 AM, Daniel Stone  wrote:
> >> > On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone  
> >> > 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".

Old kernels required TILING_X with MOD_X. I changed that at some
point to allow TILING_NONE with any modifier, but otherwise we
require the BO tiling to match the modifier. Ie. you still can't
mix TILING_Y + MOD_X for example.

-- 
Ville Syrjälä
Intel OTC
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Daniel Stone]

On 14 February 2018 at 17:23, Jason Ekstrand  wrote:
> On Wed, Feb 14, 2018 at 8:43 AM, Daniel Stone  wrote:
>> 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".

Take a sprite plane which only advertises LINEAR + X_TILED at the
moment (watermark allocation bugs seem to prevent Y_TILED). Hand that
set of two modifiers to Vulkan for allocation, render into the
resulting image. Call AddFB on it with I915_FORMAT_MOD_X_TILED. That
will get rejected because set_tiling hasn't been called, even though
you _have_ been explicit with modifiers everywhere.

>> 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.

*shrug*. Looked at another way, it's just about having your implicit
and explicit mechanisms match exactly. Given the implicit mechanism
will stay around forever, keeping them consistent makes sure that
no-one slip up. Either way, we're stuck with it forever, and if we
ever want to display X_TILED then we need it, so ...

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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  wrote:

> On 14 February 2018 at 16:27, Jason Ekstrand  wrote:
> > On Wed, Feb 14, 2018 at 3:39 AM, Daniel Stone 
> wrote:
> >> > On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone 
> 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.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Daniel Stone]

On 14 February 2018 at 16:27, Jason Ekstrand  wrote:
> On Wed, Feb 14, 2018 at 3:39 AM, Daniel Stone  wrote:
>> > On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone  
>> > 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.

Linear is unchanged as it's implicit. X tiling has to take the
workaround, in case anyone wants to display it. 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.

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.

> 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.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jason Ekstrand]

On Wed, Feb 14, 2018 at 3:39 AM, Daniel Stone  wrote:

> Hi,
>
> On 13 February 2018 at 22:15, Jason Ekstrand  wrote:
> > On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone 
> wrote:
> >> On 9 February 2018 at 23:43, Jason Ekstrand 
> 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.

What's the failure mode here?  We just don't flip?  Or the compositor wigs
out and crashes?

--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Daniel Stone]

On 14 February 2018 at 11:39, Daniel Stone  wrote:
> On 13 February 2018 at 22:15, Jason Ekstrand  wrote:
>> On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone  wrote:
>>> On 9 February 2018 at 23:43, Jason Ekstrand  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

Er, except it should be (1 << b) everywhere, not b.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Daniel Stone]

Hi,

On 13 February 2018 at 22:15, Jason Ekstrand  wrote:
> On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone  wrote:
>> On 9 February 2018 at 23:43, Jason Ekstrand  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

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jason Ekstrand]

On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone  wrote:

> Hi Jason,
>
> On 9 February 2018 at 23:43, Jason Ekstrand  wrote:
> > - /* For images using modifiers, we require a dedicated
> allocation
> > -  * and we set the BO tiling to match the tiling of the
> underlying
> > -  * modifier.  This is a bit unfortunate as this is completely
> > -  * pointless for Vulkan.  However, GL needs to be able to map
> things
> > -  * so it needs the tiling to be set.  The only way to do this
> in a
> > -  * non-racy way is to set the tiling in the creator of the BO
> so that
> > -  * makes it our job.
> > -  *
> > -  * One of these days, once the GL driver learns to not map
> things
> > -  * through the GTT in random places, we can drop this and start
> > -  * allowing multiple modified images in the same BO.
> > + /* For legacy scanout images, no tiling information is passed
> along
> > +  * directly.  Instead, consumers pull the tiling from BO.
> >*/
> > - if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) {
> > -assert(isl_drm_modifier_get_info(image->drm_format_mod)->tiling
> ==
> > -   image->planes[0].surface.isl.tiling);
> > + if (image->legacy_scanout) {
>
> Hm. There's a couple of things here. There's no-modifier protocols
> like DRI2, current DRI3 (see patches to improve this), and wl_drm
> which don't carry modifier information. For those, we set the tiling
> so the importer can know what we've done. This is either linear or
> X-tiled, and is more legacy-winsys than legacy-scanout.
>
> 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).
>
> I think this would be better called image->needs_set_tiling or
> similar, which enforced dedicated allocation and a set_tiling call,
> and was called if we have no modifier information, _or_ if the buffer
> is X-tiled.
>

Yeah, image->needs_set_tiling seems reasonable to me.  I'll make the change.


> (Is there a port of vkcube to the new modifier bits somewhere?)
>
> > @@ -2217,7 +2206,7 @@ void anv_GetImageMemoryRequirements2KHR(
> >switch (ext->sType) {
> >case VK_STRUCTURE_TYPE_MEMORY_DEDICATED_REQUIREMENTS_KHR: {
> >   VkMemoryDedicatedRequirementsKHR *requirements = (void *)ext;
> > - if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) {
> > + if (image->legacy_scanout) {
> >  /* Require a dedicated allocation for images with modifiers.
>
> Also this comment is stale.
>

Yup.


> > +   /** True if this is a legacy scanout image */
> > +   bool legacy_scanout;
>
> The comment in the header could also perhaps go a brief way to
> explaining the bear trap outlined above.
>

I've written the following:

   /** True if this is needs to be bound to an appropriately tiled BO.
*
* When not using modifiers, consumers such as X11, Wayland, and KMS need
* the tiling passed via I915_GEM_SET_TILING.  When exporting these
buffers
* we require a dedicated allocation so that we can know to allocate a
* tiled buffer.
*/
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Daniel Stone]

Hi Jason,

On 9 February 2018 at 23:43, Jason Ekstrand  wrote:
> - /* For images using modifiers, we require a dedicated allocation
> -  * and we set the BO tiling to match the tiling of the underlying
> -  * modifier.  This is a bit unfortunate as this is completely
> -  * pointless for Vulkan.  However, GL needs to be able to map things
> -  * so it needs the tiling to be set.  The only way to do this in a
> -  * non-racy way is to set the tiling in the creator of the BO so 
> that
> -  * makes it our job.
> -  *
> -  * One of these days, once the GL driver learns to not map things
> -  * through the GTT in random places, we can drop this and start
> -  * allowing multiple modified images in the same BO.
> + /* For legacy scanout images, no tiling information is passed along
> +  * directly.  Instead, consumers pull the tiling from BO.
>*/
> - if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) {
> -assert(isl_drm_modifier_get_info(image->drm_format_mod)->tiling 
> ==
> -   image->planes[0].surface.isl.tiling);
> + if (image->legacy_scanout) {

Hm. There's a couple of things here. There's no-modifier protocols
like DRI2, current DRI3 (see patches to improve this), and wl_drm
which don't carry modifier information. For those, we set the tiling
so the importer can know what we've done. This is either linear or
X-tiled, and is more legacy-winsys than legacy-scanout.

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).

I think this would be better called image->needs_set_tiling or
similar, which enforced dedicated allocation and a set_tiling call,
and was called if we have no modifier information, _or_ if the buffer
is X-tiled.

(Is there a port of vkcube to the new modifier bits somewhere?)

> @@ -2217,7 +2206,7 @@ void anv_GetImageMemoryRequirements2KHR(
>switch (ext->sType) {
>case VK_STRUCTURE_TYPE_MEMORY_DEDICATED_REQUIREMENTS_KHR: {
>   VkMemoryDedicatedRequirementsKHR *requirements = (void *)ext;
> - if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) {
> + if (image->legacy_scanout) {
>  /* Require a dedicated allocation for images with modifiers.

Also this comment is stale.

> +   /** True if this is a legacy scanout image */
> +   bool legacy_scanout;

The comment in the header could also perhaps go a brief way to
explaining the bear trap outlined above.

Rest looks good though.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev