[PATCH] drm/simple-kms: Drop drm_simple_kms_format_mod_supported.
Mario Kleiner
mario.kleiner.de at gmail.com
Mon Jan 25 18:19:44 UTC 2021
On Mon, Jan 25, 2021 at 5:34 PM Ville Syrjälä <ville.syrjala at linux.intel.com>
wrote:
> On Mon, Jan 25, 2021 at 04:32:48PM +0100, Mario Kleiner wrote:
> > On Mon, Jan 25, 2021 at 1:13 PM Ville Syrjälä <
> ville.syrjala at linux.intel.com>
> > wrote:
> >
> > > On Sun, Jan 24, 2021 at 09:47:48PM +0100, Mario Kleiner wrote:
> > > > The check was introduced to make sure that only the
> > > > DRM_FORMAT_MOD_LINEAR modifier is accepted by tinydrm.
> > > >
> > > > However, if .format_mod_supported is not hooked up to
> > > > drm_simple_kms_format_mod_supported then the core will
> > > > simply validate modifiers against the format_modifiers
> > > > list passed into drm_simple_display_pipe_init() or
> > > > drm_universal_plane_init() and perform the same validation
> > > > as drm_simple_kms_format_mod_supported() would have done.
> > > >
> > > > Additionally, if a kms driver / plane does not support
> > > > modifiers, it will not reject fb updates with no modifiers/
> > > > DRM_FORMAT_MOD_INVALID. This is important, because some
> > > > simple drm drivers, e.g., pl111, pass NULL as format_modifiers
> > > > list, so modifier support is disabled for these drivers,
> > > > userspace would fall back to drmAddFB() without modifiers,
> > > > and ergo the current drm_simple_kms_format_mod_supported()
> > > > function would reject valid modifier-less fb's.
> > > >
> > > > So this should fix display on non-tinydrm drivers like
> > > > pl111, and probably also for non-atomic clients?
> > > >
> > > > The Mesa vc4 gallium driver mentions pl111 as one possible
> > > > display driver in render_only mode, so i assume this matters
> > > > for some SoC's?
> > > >
> > > > The background for the patch that introduced this was to
> > > > fix atomic modesetting in the X-Servers modesetting-ddx,
> > > > but with atomic modesetting and format modifiers disabled
> > > > in modesetting-ddx (and also current kernels when interacting
> > > > with modesetting-ddx), i assume this should fix some panels.
> > > >
> > > > Note that i don't have any of the hw required for testing
> > > > this, this is purely based on looking at the code, so this
> > > > patch is only compile-tested.
> > > >
> > > > For more reference, this fix was motivated by some discussions
> > > > around broken page-flipping on VideoCore6 / RaspberryPi 4
> > > > with current Raspbian OS, so the experts may want to weigh
> > > > in on that Mesa bug report as well, under the following link:
> > > >
> > > > https://gitlab.freedesktop.org/mesa/mesa/-/issues/3601
> > > >
> > > > Fixes: dff906c3f91c ("drm/tinydrm: Advertise that we can do only
> > > DRM_FORMAT_MOD_LINEAR.")
> > > > Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
> > > > Cc: Eric Anholt <eric at anholt.net>
> > > > Cc: Noralf Trønnes <noralf at tronnes.org>
> > > > Cc: Maxime Ripard <mripard at kernel.org>
> > > > ---
> > > > drivers/gpu/drm/drm_simple_kms_helper.c | 8 --------
> > > > 1 file changed, 8 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
> > > b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > index 743e57c1b44f..5f3e30553172 100644
> > > > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > @@ -229,13 +229,6 @@ static void
> drm_simple_kms_plane_cleanup_fb(struct
> > > drm_plane *plane,
> > > > pipe->funcs->cleanup_fb(pipe, state);
> > > > }
> > > >
> > > > -static bool drm_simple_kms_format_mod_supported(struct drm_plane
> *plane,
> > > > - uint32_t format,
> > > > - uint64_t modifier)
> > > > -{
> > > > - return modifier == DRM_FORMAT_MOD_LINEAR;
> > > > -}
> > > > -
> > > > static const struct drm_plane_helper_funcs
> > > drm_simple_kms_plane_helper_funcs = {
> > > > .prepare_fb = drm_simple_kms_plane_prepare_fb,
> > > > .cleanup_fb = drm_simple_kms_plane_cleanup_fb,
> > > > @@ -250,7 +243,6 @@ static const struct drm_plane_funcs
> > > drm_simple_kms_plane_funcs = {
> > > > .reset = drm_atomic_helper_plane_reset,
> > > > .atomic_duplicate_state =
> drm_atomic_helper_plane_duplicate_state,
> > > > .atomic_destroy_state =
> drm_atomic_helper_plane_destroy_state,
> > > > - .format_mod_supported = drm_simple_kms_format_mod_supported,
> > >
> > > This will now cause also this driver to report a totally borked
> > > IN_FORMATS blob.
> > >
> > > Would need this
> > > https://patchwork.freedesktop.org/series/83018/
> > >
> > >
> > The slight problem with that (see my comments in the linked Mesa bug
> > report), is that at least one common userspace driver - modesetting-ddx -
> > treat a lack of an IN_FORMATS blob not as "don't use modifiers for
> > drm_framebuffers", but as "everything goes" -- Use every modifier and
> > tiling format that the graphics driver exposes also for scanout buffers.
> > I'm arguing in that bug report that modesetting-ddx shouldn't use atomic
> or
> > modifiers at all, given how broken that driver is atm. in that area, so
> i'm
> > not sure if my argument here is valid. Just saying that doing the "every
> > modifier is valid for every format" in absence of format_mod_supported()
> > would probably be less harmful to some existing userspace. Ofc. then
> > there's a reason why atomic gets rejected by the kernel for current
> > modesetting-ddx...
> >
> > I'm not sure if I'm arguing pro or contra your patch here btw. Just
> > pointing out one possible victim if it were applied.
>
> I have no idea how anything would get broken by it.
>
> Currently:
> - the broken IN_FORMAT blob says nothing is supported at all,
> so if someone consults it they won't be able to find a working
> pixel format
> - if they ignore the broken IN_FORMAT blob then it doesn't matter
> if it's present or not.
>
>
Yes. What modesetting-ddx does iff xorg.conf Option "Debug"
"dmabuf_capable" is set, is it tries to get the list of display capable
modifiers. If the list is empty (due to broken IN_FORMAT blob, because
.format_mod_supported == NULL for a kms driver in the current kernel), or
because the IN_FORMAT blob doesn't exist (if your patch is applied and
.format_mod_supported == NULL) or if it can't query in the first place
(because use of atomic uapi is disabled or rejected by the kernel - as is
the case for current kernels on modesetting-ddx), then it will
unfortunately not decide not to use modifiers, but instead will consider
all dmabuf modifiers exposed by the render driver as scanout capable, also
tiled formats and pass those back to Mesa's DRI3/Present backend or
Vulkan/WSI-X11 where things can go sideways, at least as so far shown for
v3d / Broadcom VideoCore6 / RPi-4.
Anyway, yes it probably could only break a broken userspace driver more.
But that is what modesetting-ddx is atm. iff one decided to enable the
dmabuf_capable option, which is what Raspbian currently does, for an
underwhelming RPi4 out of the box visual experience.
Looking at your patch i'd suggest to also reset
config->allow_fb_modifiers = false;
if .format_mod_supported == NULL, so clients that check for the
DRM_CAP_ADDFB2_MODIFIERS capability will more likely stay away from
modifiers. At least modesetting-ddx would fall back to drmModeAddFB instead
of drmModeAddFBWithModifiers, which would be better, but probably still
broken. It's complicated...
I guess the only way somehting might be affected is if they just use
> the presence of the IN_FORMATS blob as a hint for something but never
> actually look at the contents.
>
Or at DRM_CAP_ADDFB2_MODIFIERS query result, see above.
>
> >
> > but IIRC the bikeshed around that kinda suggested we should just
> > > implement .format_mod_support() always. Can't quite recall the
> > > details anymore.
> > >
> > >
> > I see. But if .format_mod_supported() is always implemented, then we'd
> need
> > to handle the case modifier == DRM_FORMAT_MOD_INVALID in the core or in
> > each format_mod_supported() implementation, because currently iff this is
> > hooked up, it gets always used, even if the user-space does not use
> > modifiers. The X-Servers modesetting-ddx, e.g., does not use atomic or
> > modifiers by default, and the linked Mesa bug report shows why - or at
> > least why it shouldn't atm. I think none of the X drivers does.
> >
> > The softer alternative solution instead of my patch would be to also
> accept
> > modifier == DRM_FORMAT_MOD_INVALID as valid for simple kms drivers.
>
> DRM_FORMAT_MOD_INVALID is not a valid modifier.
> What kind of broken code is using it?
>
>
Oh sorry, i meant DRM_FORMAT_MOD_NONE (==0) ie. what is in fb->modifier if
userspace doesn't assign a modifier, e.g., drmModeAddFB().
But now after looking again, i just realized that DRM_FORMAT_MOD_NONE is
identical to DRM_FORMAT_MOD_LINEAR, ie. both are zero, so i guess my point
is moot.
That actually means this whole patch of mine is not needed and I was
hunting a ghost :/. It also means that simple drm drivers should probably
work with RPi if they revert that dmabuf_capable patch on Raspbian's
modesetting-ddx with no need for extra kernel fixes, so this is not wasted.
Thanks for helping me think!
-mario
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20210125/43fb6d13/attachment-0001.htm>
More information about the dri-devel
mailing list