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
next prev parent 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).