[PATCH] drm/simple-kms: Drop drm_simple_kms_format_mod_supported.

Mon Jan 25 15:32:48 UTC 2021

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.

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.
-mario
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20210125/138c17de/attachment.htm>