<div dir="ltr"><div dir="ltr">On Mon, Jan 25, 2021 at 1:13 PM Ville Syrjälä <<a href="mailto:ville.syrjala@linux.intel.com">ville.syrjala@linux.intel.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sun, Jan 24, 2021 at 09:47:48PM +0100, Mario Kleiner wrote:<br> > The check was introduced to make sure that only the<br> > DRM_FORMAT_MOD_LINEAR modifier is accepted by tinydrm.<br> > <br> > However, if .format_mod_supported is not hooked up to<br> > drm_simple_kms_format_mod_supported then the core will<br> > simply validate modifiers against the format_modifiers<br> > list passed into drm_simple_display_pipe_init() or<br> > drm_universal_plane_init() and perform the same validation<br> > as drm_simple_kms_format_mod_supported() would have done.<br> > <br> > Additionally, if a kms driver / plane does not support<br> > modifiers, it will not reject fb updates with no modifiers/<br> > DRM_FORMAT_MOD_INVALID. This is important, because some<br> > simple drm drivers, e.g., pl111, pass NULL as format_modifiers<br> > list, so modifier support is disabled for these drivers,<br> > userspace would fall back to drmAddFB() without modifiers,<br> > and ergo the current drm_simple_kms_format_mod_supported()<br> > function would reject valid modifier-less fb's.<br> > <br> > So this should fix display on non-tinydrm drivers like<br> > pl111, and probably also for non-atomic clients?<br> > <br> > The Mesa vc4 gallium driver mentions pl111 as one possible<br> > display driver in render_only mode, so i assume this matters<br> > for some SoC's?<br> > <br> > The background for the patch that introduced this was to<br> > fix atomic modesetting in the X-Servers modesetting-ddx,<br> > but with atomic modesetting and format modifiers disabled<br> > in modesetting-ddx (and also current kernels when interacting<br> > with modesetting-ddx), i assume this should fix some panels.<br> > <br> > Note that i don't have any of the hw required for testing<br> > this, this is purely based on looking at the code, so this<br> > patch is only compile-tested.<br> > <br> > For more reference, this fix was motivated by some discussions<br> > around broken page-flipping on VideoCore6 / RaspberryPi 4<br> > with current Raspbian OS, so the experts may want to weigh<br> > in on that Mesa bug report as well, under the following link:<br> > <br> > <a href="https://gitlab.freedesktop.org/mesa/mesa/-/issues/3601" rel="noreferrer" target="_blank">https://gitlab.freedesktop.org/mesa/mesa/-/issues/3601</a><br> > <br> > Fixes: dff906c3f91c ("drm/tinydrm: Advertise that we can do only DRM_FORMAT_MOD_LINEAR.")<br> > Signed-off-by: Mario Kleiner <<a href="mailto:mario.kleiner.de@gmail.com" target="_blank">mario.kleiner.de@gmail.com</a>><br> > Cc: Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>><br> > Cc: Noralf Trønnes <<a href="mailto:noralf@tronnes.org" target="_blank">noralf@tronnes.org</a>><br> > Cc: Maxime Ripard <<a href="mailto:mripard@kernel.org" target="_blank">mripard@kernel.org</a>><br> > ---<br> > drivers/gpu/drm/drm_simple_kms_helper.c | 8 --------<br> > 1 file changed, 8 deletions(-)<br> > <br> > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c<br> > index 743e57c1b44f..5f3e30553172 100644<br> > --- a/drivers/gpu/drm/drm_simple_kms_helper.c<br> > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c<br> > @@ -229,13 +229,6 @@ static void drm_simple_kms_plane_cleanup_fb(struct drm_plane *plane,<br> > pipe->funcs->cleanup_fb(pipe, state);<br> > }<br> > <br> > -static bool drm_simple_kms_format_mod_supported(struct drm_plane *plane,<br> > - uint32_t format,<br> > - uint64_t modifier)<br> > -{<br> > - return modifier == DRM_FORMAT_MOD_LINEAR;<br> > -}<br> > -<br> > static const struct drm_plane_helper_funcs drm_simple_kms_plane_helper_funcs = {<br> > .prepare_fb = drm_simple_kms_plane_prepare_fb,<br> > .cleanup_fb = drm_simple_kms_plane_cleanup_fb,<br> > @@ -250,7 +243,6 @@ static const struct drm_plane_funcs drm_simple_kms_plane_funcs = {<br> > .reset = drm_atomic_helper_plane_reset,<br> > .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,<br> > .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,<br> > - .format_mod_supported = drm_simple_kms_format_mod_supported,<br> <br> This will now cause also this driver to report a totally borked<br> IN_FORMATS blob.<br> <br> Would need this<br> <a href="https://patchwork.freedesktop.org/series/83018/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.org/series/83018/</a><br> <br></blockquote><div><br></div><div>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...<br></div><div><br></div><div>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.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> but IIRC the bikeshed around that kinda suggested we should just<br> implement .format_mod_support() always. Can't quite recall the<br> details anymore.<br> <br></blockquote><div> </div><div>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.<br></div><div><br></div><div>The softer alternative solution instead of my patch would be to also accept modifier == DRM_FORMAT_MOD_INVALID as valid for simple kms drivers.<br></div><div>-mario<br></div><div> </div></div></div>