On Mon, Jan 25, 2021 at 5:34 PM Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> > > Cc: Eric Anholt <eric@xxxxxxxxxx>
> > > Cc: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > > ---
> > > 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
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
