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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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@gmail.com>
> > > Cc: Eric Anholt <eric@anholt.net>
> > > Cc: Noralf Trønnes <noralf@tronnes.org>
> > > Cc: Maxime Ripard <mripard@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.

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.

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

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel