Re: [PATCH] drm/exynos: Set supported modifiers during plane init

[Inki Dae]

Sorry for late review,

2026년 8월 3일 (월) 오후 11:22, Chen-Yu Tsai <wenst@xxxxxxxxxxxx>님이 작성:
>
> The exynos driver currently checks the modifier in a helper called by
> the plane atomic check helper callback. This check only concerns whether
> the selected plane supports the tiled format or not. This is tied to
> the plane's hardware capability.
>
> Turns out the tiled support was added around the same time modifiers
> were being plumbed through DRM planes. The latter provided a way to
> describe per-plane modifier support.
>
> Add a field to the hardware plane capability struct to allow adding
> an optional list of supported modifiers. This is passed to
> drm_universal_plane_init(). This also allows the core to generate
> and pass to userspace a list of supported modifiers for each plane.
>
> EXYNOS_DRM_PLANE_CAP_TILE and the existing helper are removed, as the
> check is now down by the DRM atomic check.
>
> Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.h   |  2 +-
>  drivers/gpu/drm/exynos/exynos_drm_plane.c | 30 +----------------------
>  drivers/gpu/drm/exynos/exynos_mixer.c     |  8 +++++-
>  3 files changed, 9 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index b126cd129944..ab0edb826261 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -92,7 +92,6 @@ struct exynos_drm_plane {
>  #define EXYNOS_DRM_PLANE_CAP_DOUBLE    (1 << 0)
>  #define EXYNOS_DRM_PLANE_CAP_SCALE     (1 << 1)
>  #define EXYNOS_DRM_PLANE_CAP_ZPOS      (1 << 2)
> -#define EXYNOS_DRM_PLANE_CAP_TILE      (1 << 3)
>  #define EXYNOS_DRM_PLANE_CAP_PIX_BLEND (1 << 4)
>  #define EXYNOS_DRM_PLANE_CAP_WIN_BLEND (1 << 5)
>
> @@ -112,6 +111,7 @@ struct exynos_drm_plane_config {
>         const uint32_t *pixel_formats;
>         unsigned int num_pixel_formats;
>         unsigned int capabilities;
> +       const uint64_t *modifiers;
>  };
>
>  /*
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> index 5c04ef87e400..31f5f0941154 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> @@ -174,30 +174,6 @@ static struct drm_plane_funcs exynos_plane_funcs = {
>         .atomic_destroy_state = exynos_drm_plane_destroy_state,
>  };
>
> -static int
> -exynos_drm_plane_check_format(const struct exynos_drm_plane_config *config,
> -                             struct exynos_drm_plane_state *state)
> -{
> -       struct drm_framebuffer *fb = state->base.fb;
> -       struct drm_device *dev = fb->dev;
> -
> -       switch (fb->modifier) {
> -       case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
> -               if (!(config->capabilities & EXYNOS_DRM_PLANE_CAP_TILE))
> -                       return -ENOTSUPP;
> -               break;
> -
> -       case DRM_FORMAT_MOD_LINEAR:
> -               break;
> -
> -       default:
> -               DRM_DEV_ERROR(dev->dev, "unsupported pixel format modifier");
> -               return -ENOTSUPP;
> -       }
> -
> -       return 0;
> -}
> -
>  static int
>  exynos_drm_plane_check_size(const struct exynos_drm_plane_config *config,
>                             struct exynos_drm_plane_state *state)
> @@ -245,10 +221,6 @@ static int exynos_plane_atomic_check(struct drm_plane *plane,
>         /* translate state into exynos_state */
>         exynos_plane_mode_set(exynos_state);
>
> -       ret = exynos_drm_plane_check_format(exynos_plane->config, exynos_state);
> -       if (ret)
> -               return ret;
> -
>         ret = exynos_drm_plane_check_size(exynos_plane->config, exynos_state);
>         return ret;
>  }
> @@ -312,7 +284,7 @@ int exynos_plane_init(struct drm_device *dev,
>                                        &exynos_plane_funcs,
>                                        config->pixel_formats,
>                                        config->num_pixel_formats,
> -                                      NULL, config->type, NULL);
> +                                      config->modifiers, config->type, NULL);
>         if (err) {
>                 DRM_DEV_ERROR(dev->dev, "failed to initialize plane\n");
>                 return err;
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 69dea5049309..d74e605a452b 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -93,6 +93,12 @@ static const uint32_t vp_formats[] = {
>         DRM_FORMAT_NV21,
>  };
>
> +static const uint64_t vp_modifiers[] = {
> +       DRM_FORMAT_MOD_LINEAR,
> +       DRM_FORMAT_MOD_SAMSUNG_64_32_TILE,
> +       DRM_FORMAT_MOD_INVALID
> +};
> +
>  struct mixer_context {
>         struct platform_device *pdev;
>         struct device           *dev;
> @@ -148,8 +154,8 @@ static const struct exynos_drm_plane_config plane_configs[MIXER_WIN_NR] = {
>                 .num_pixel_formats = ARRAY_SIZE(vp_formats),
>                 .capabilities = EXYNOS_DRM_PLANE_CAP_SCALE |
>                                 EXYNOS_DRM_PLANE_CAP_ZPOS |
> -                               EXYNOS_DRM_PLANE_CAP_TILE |
>                                 EXYNOS_DRM_PLANE_CAP_WIN_BLEND,
> +               .modifiers = vp_modifiers,

The VP plane advertises the tiled modifier for both NV12 and NV21, but
framebuffer_check() accepts it only for NV12[1]. Could you add a
format_mod_supported callback to restrict that combination? For
example, the callback could check that the modifier is present in
plane->modifiers, then reject DRM_FORMAT_MOD_SAMSUNG_64_32_TILE when
the format is not NV12:
static bool exynos_plane_format_mod_supported(struct drm_plane *plane,
                                              u32 format, u64 modifier)
{
        unsigned int i;

        for (i = 0; i < plane->modifier_count; i++)
        if (plane->modifiers[i] == modifier)
                break;

        if (i == plane->modifier_count)
                return false;

       return modifier != DRM_FORMAT_MOD_SAMSUNG_64_32_TILE ||
                 format == DRM_FORMAT_NV12;
}

This can be assigned to exynos_plane_funcs.format_mod_supported.
Checking the modifier list matters here because DRM delegates that
validation to the callback when one is present.

Thanks,
Inki Dae


[1] in drm_framebuffers.c
static int framebuffer_check(struct drm_device *dev,
    const struct drm_format_info *info,
    const struct drm_mode_fb_cmd2 *r)
{
...
        /* modifier specific checks: */
        switch (r->modifier[i]) {
        case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
               /* NOTE: the pitch restriction may be lifted later if it turns
                * out that no hw has this restriction:
                */
                if (r->pixel_format != DRM_FORMAT_NV12 ||
                         width % 128 || height % 32 ||
                          r->pitches[i] % 128) {
                       drm_dbg_kms(dev, "bad modifier data for plane %d\n", i);
                       return -EINVAL;
        }
...

>         },
>  };
>
> --
> 2.55.0.571.g244d577d93-goog
>
>