dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: James Jones <jajones@nvidia.com>
To: Daniel Stone <daniel@fooishbar.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	LKML <linux-kernel@vger.kernel.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Ben Skeggs <bskeggs@redhat.com>,
	"Kirill A. Shutemov" <kirill@shutemov.name>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [git pull] drm for 5.8-rc1
Date: Thu, 2 Jul 2020 14:14:01 -0700	[thread overview]
Message-ID: <5ffa32db-4383-80f6-c0cf-a9bb12e729aa@nvidia.com> (raw)
In-Reply-To: <CAPj87rOrUHBZZR3cw+iqUMtZL1ZQyjd=RoM2yHt5oUeRO5uDTA@mail.gmail.com>

On 7/2/20 1:22 AM, Daniel Stone wrote:
> Hi,
> 
> On Wed, 1 Jul 2020 at 20:45, James Jones <jajones@nvidia.com> wrote:
>> OK, I think I see what's going on.  In the Xorg modesetting driver, the
>> logic is basically:
>>
>> if (gbm_has_modifiers && DRM_CAP_ADDFB2_MODIFIERS != 0) {
>>     drmModeAddFB2WithModifiers(..., gbm_bo_get_modifier(bo->gbm));
>> } else {
>>     drmModeAddFB(...);
>> }
> 
> I read this thread expecting to explain the correct behaviour we
> implement in Weston and how modesetting needs to be fixed, but ...
> that seems OK to me? As long as `gbm_has_modifiers` is a proxy for 'we
> used gbm_(bo|surface)_create_with_modifiers to allocate the buffer'.

Yes, the hazards of reporting findings before verifying.  I now see 
modesetting does query the DRM-KMS modifiers and attempt to allocate 
with them if it found any.  However, I still see a lot of ways things 
can go wrong, but I'm not going to share my speculation again until I've 
actually verified it, which is taking a frustratingly long time.  The 
modesetting driver is not my friend right now.

>> There's no attempt to verify the DRM-KMS device supports the modifier,
>> but then, why would there be?  GBM presumably chose a supported modifier
>> at buffer creation time, and we don't know which plane the FB is going
>> to be used with yet.  GBM doesn't actually ask the kernel which
>> modifiers it supports here either though.
> 
> Right, it doesn't ask, because userspace tells it which modifiers to
> use. The correct behaviour is to take the list from the KMS
> `IN_FORMATS` property and then pass that to
> `gbm_(bo|surface)_create_with_modifiers`; GBM must then select from
> that list and only that list. If that call does not succeed and Xorg
> falls back to `gbm_surface_create`, then it must not call
> `gbm_bo_get_modifier` - so that would be a modesetting bug. If that
> call does succeed and `gbm_bo_get_modifier` subsequently reports a
> modifier which was not in the list, that's a Mesa driver bug.
> 
>> It just goes into Mesa via
>> DRI and reports the modifier (unpatched) Mesa chose on its own.  Mesa
>> just hard-codes the modifiers in its driver backends since its thinking
>> in terms of a device's 3D engine, not display.  In theory, Mesa's DRI
>> drivers could query KMS for supported modifiers if allocating from GBM
>> using the non-modifiers path and the SCANOUT flag is set (perhaps some
>> drivers do this or its equivalent?  Haven't checked.), but that seems
>> pretty gnarly and doesn't fix the modifier-based GBM allocation path
>> AFAIK.  Bit of a mess.
> 
> Two options for GBM users:
> * call gbm_*_create_with_modifiers, it succeeds, call
> gbm_bo_get_modifier, pass modifier into AddFB
> * call gbm_*_create (without modifiers), it succeeds, do not call
> gbm_bo_get_modifier, do not pass a modifier into AddFB
> 
> Anything else is a bug in the user. Note that falling back from 1 to 2
> is fine: if `gbm_*_create_with_modifiers()` fails, you can fall back
> to the non-modifier path, provided you don't later try to get a
> modifier back out.
>
>> For a quick userspace fix that could probably be pushed out everywhere
>> (Only affects Xorg server 1.20+ AFAIK), just retrying
>> drmModeAddFB2WithModifiers() without the DRM_MODE_FB_MODIFIERS flag on
>> failure should be sufficient.
> 
> This would break other drivers.

I think this could be done in a way that wouldn't, though it wouldn't be 
quite as simple.  Let's see what the true root cause is first though.

>> Still need to verify as I'm having
>> trouble wrangling my Xorg build at the moment and I'm pressed for time.
>> A more complete fix would be quite involved, as modesetting isn't really
>> properly plumbed to validate GBM's modifiers against KMS planes, and it
>> doesn't seem like GBM/Mesa/DRI should be responsible for this as noted
>> above given the general modifier workflow/design.
>>
>> Most importantly, options I've considered for fixing from the kernel side:
>>
>> -Accept "legacy" modifiers in nouveau in addition to the new modifiers,
>> though avoid reporting them to userspace as supported to avoid further
>> proliferation.  This is pretty straightforward.  I'll need to modify
>> both the AddFB2 handler (nouveau_validate_decode_mod) and the mode set
>> plane validation logic (nv50_plane_format_mod_supported), but it should
>> end up just being a few lines of code.
> 
> I do think that they should also be reported to userspace if they are
> accepted. Other users can and do look at the modifier list to see if
> the buffer is acceptable for a given plane, so the consistency is good
> here. Of course, in Mesa you would want to prioritise the new
> modifiers over the legacy ones, and not allocate or return the legacy
> ones unless that was all you were asked for. This would involve
> tracking the used modifier explicitly through Mesa, rather than
> throwing it away at alloc time and then later divining it from the
> tiling mode.

Reporting them as supported is equivalent to reporting support for a 
memory layout the chips don't actually support (It corresponds to a 
valid layout on Tegra chips, but not on discrete NV chips).  This is 
what the new modifiers are trying to avoid in the first place: Implying 
buffers can be shared between these Tegra chips and discrete NV GPUs.

Thanks,
-James

> Cheers,
> Daniel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2020-07-02 21:13 UTC|newest]

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-02  6:06 [git pull] drm for 5.8-rc1 Dave Airlie
2020-06-02 21:21 ` Linus Torvalds
2020-06-02 21:22   ` Linus Torvalds
2020-06-02 21:56   ` Linus Torvalds
2020-06-03  7:18     ` Thomas Zimmermann
2020-06-03  7:43       ` Daniel Vetter
2020-06-02 22:14 ` Linus Torvalds
2020-06-02 23:03   ` Dave Airlie
2020-06-02 22:20 ` pr-tracker-bot
2020-06-03 20:13 ` Jason Gunthorpe
2020-06-04  8:10   ` Christian König
2020-06-30 23:08 ` Kirill A. Shutemov
2020-07-01  4:40   ` James Jones
2020-07-01  7:57     ` Kirill A. Shutemov
2020-07-01  7:59       ` Kirill A. Shutemov
2020-07-01 19:45         ` James Jones
2020-07-02  7:36           ` Daniel Vetter
2020-07-02  7:59           ` Pekka Paalanen
2020-07-02  8:22           ` Daniel Stone
2020-07-02 21:14             ` James Jones [this message]
2020-07-03  6:01               ` James Jones
2020-07-03  7:16                 ` Daniel Vetter
2020-07-13  1:37                   ` Dave Airlie
2020-07-14 14:31                     ` James Jones
2020-08-04  8:58                       ` Karol Herbst
2020-08-12  0:19                         ` James Jones
2020-08-12 10:27                           ` Karol Herbst
2020-08-12 10:43                             ` Karol Herbst
2020-08-12 12:24                               ` Karol Herbst
2020-08-12 12:37                                 ` Ilia Mirkin
2020-08-12 17:03                                   ` James Jones
2020-08-12 17:10                                     ` Karol Herbst
2020-08-12 17:19                                       ` James Jones
2020-08-12 17:40                                         ` Alyssa Rosenzweig
2020-08-12 18:24                                           ` James Jones
2020-08-12 18:51                                             ` Karol Herbst
2020-08-13 13:00                                               ` Karol Herbst
2020-08-13 15:39                                                 ` Karol Herbst
2020-08-13 17:19                                                   ` Karol Herbst
2020-08-13 17:45                                                     ` James Jones
2020-08-13 17:48                                                       ` Karol Herbst
2020-08-14 13:57                                                         ` Thierry Reding
2020-08-14 13:59                                                           ` Karol Herbst
2020-08-14 14:10                                                             ` Thierry Reding
2020-08-14 14:05                                                       ` Thierry Reding
2020-08-14 14:44                                                         ` Karol Herbst
2020-08-14 15:34                                                           ` Thierry Reding
2020-08-14 15:40                                                             ` Karol Herbst
2020-08-14 16:06                                                               ` Thierry Reding
2020-08-14 16:12                                                                 ` Karol Herbst
2020-08-14 16:22                                                                   ` Thierry Reding
2020-08-14 17:17                                                                     ` Daniel Stone
2020-08-14 17:25                                                                       ` Daniel Vetter
2020-08-18 14:37                                                                         ` Thierry Reding
2020-09-01  7:13                                                                           ` Daniel Vetter
2020-09-01 10:42                                                                             ` Daniel Stone
2020-09-01 10:59                                                                             ` Karol Herbst
2020-09-01 14:42                                                                               ` James Jones
2020-08-14 14:08                                                   ` Thierry Reding
2020-08-14 14:45                                                     ` Karol Herbst
2020-08-14 15:24                                                       ` Thierry Reding
2020-08-14 15:43                                                         ` Karol Herbst
2020-08-14 13:54                                                 ` Thierry Reding
2020-08-14 13:40                                     ` Thierry Reding
2020-08-14 13:56                                       ` Karol Herbst
2020-08-12 15:05                               ` Thierry Reding
2020-08-12 15:20                                 ` Karol Herbst
2020-08-12 15:49                                   ` Karol Herbst
2020-07-01 11:24     ` Karol Herbst
2020-07-01 15:51       ` James Jones
2020-07-01 16:01         ` Daniel Vetter
2020-07-01 17:04           ` Karol Herbst
2020-07-01 17:37             ` James Jones
2020-07-01 18:08               ` Karol Herbst

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5ffa32db-4383-80f6-c0cf-a9bb12e729aa@nvidia.com \
    --to=jajones@nvidia.com \
    --cc=bskeggs@redhat.com \
    --cc=daniel.vetter@ffwll.ch \
    --cc=daniel@fooishbar.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=kirill@shutemov.name \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).