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

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

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/

but IIRC the bikeshed around that kinda suggested we should just
implement .format_mod_support() always. Can't quite recall the
details anymore.

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