[PATCH] drm/gem: Check for valid formats

[Rob Clark]

drive-by thought/concern, what are the odds that there is some wayland
compositor out there that creates an fb for every window surface, even
if it later decides to composite on the GPU because the display does
not support the format?  It seems like there is a non-zero chance of
breaking userspace..

BR,
-R

On Tue, Jan 3, 2023 at 4:55 AM Maíra Canal <mcanal at igalia.com> wrote:
>
> Currently, drm_gem_fb_create() doesn't check if the pixel format is
> supported, which can lead to the acceptance of invalid pixel formats
> e.g. the acceptance of invalid modifiers. Therefore, add a check for
> valid formats on drm_gem_fb_create().
>
> Moreover, note that this check is only valid for atomic drivers,
> because, for non-atomic drivers, checking drm_any_plane_has_format() is
> not possible since the format list for the primary plane is fake, and
> we'd therefor reject valid formats.
>
> Suggested-by: Thomas Zimmermann <tzimmermann at suse.de>
> Signed-off-by: Maíra Canal <mcanal at igalia.com>
> ---
>  Documentation/gpu/todo.rst                   | 7 ++-----
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 9 +++++++++
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 1f8a5ebe188e..68bdafa0284f 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -276,11 +276,8 @@ Various hold-ups:
>  - Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
>    setup code can't be deleted.
>
> -- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
> -  atomic drivers we could check for valid formats by calling
> -  drm_plane_check_pixel_format() against all planes, and pass if any plane
> -  supports the format. For non-atomic that's not possible since like the format
> -  list for the primary plane is fake and we'd therefor reject valid formats.
> +- Need to switch to drm_gem_fb_create(), as now drm_gem_fb_create() checks for
> +  valid formats for atomic drivers.
>
>  - Many drivers subclass drm_framebuffer, we'd need a embedding compatible
>    version of the varios drm_gem_fb_create functions. Maybe called
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index e93533b86037..b8a615a138cd 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -9,6 +9,7 @@
>  #include <linux/module.h>
>
>  #include <drm/drm_damage_helper.h>
> +#include <drm/drm_drv.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_gem.h>
> @@ -164,6 +165,14 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
>                 return -EINVAL;
>         }
>
> +       if (drm_drv_uses_atomic_modeset(dev) &&
> +           !drm_any_plane_has_format(dev, mode_cmd->pixel_format,
> +                                     mode_cmd->modifier[0])) {
> +               drm_dbg(dev, "Unsupported pixel format %p4cc / modifier 0x%llx\n",
> +                       &mode_cmd->pixel_format, mode_cmd->modifier[0]);
> +               return -EINVAL;
> +       }
> +
>         for (i = 0; i < info->num_planes; i++) {
>                 unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
>                 unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> --
> 2.38.1
>