Re: [PATCH] drm/amd/display: Verify Gamma & Degamma LUT sizes in amdgpu_dm_atomic_check

[Harry Wentland]

On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
> From: Mark Yacoub <markyac...@google.com>
> 
> For each CRTC state, check the size of Gamma and Degamma LUTs  so
> unexpected and larger sizes wouldn't slip through.
> 
> TEST: IGT:kms_color::pipe-invalid-gamma-lut-sizes
> 
> Signed-off-by: Mark Yacoub <markyac...@chromium.org>
> Change-Id: I9d513a38e8ac2af1b4bf802e1feb1a4d726fba4c
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  3 ++
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  1 +
>  .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 40 ++++++++++++++++---
>  3 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 38d497d30dba8..f6cd522b42a80 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -9402,6 +9402,9 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>                       dm_old_crtc_state->dsc_force_changed == false)
>                       continue;
>  
> +             if ((ret = amdgpu_dm_verify_lut_sizes(new_crtc_state)))
> +                     goto fail;
> +
>               if (!new_crtc_state->enable)
>                       continue;
>  
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 8bfe901cf2374..1b77cd2612691 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -541,6 +541,7 @@ void amdgpu_dm_trigger_timing_sync(struct drm_device 
> *dev);
>  #define MAX_COLOR_LEGACY_LUT_ENTRIES 256
>  
>  void amdgpu_dm_init_color_mod(void);
> +int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state);
>  int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc);
>  int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
>                                     struct dc_plane_state *dc_plane_state);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index 157fe4efbb599..da6f9fcc0b415 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> @@ -284,6 +284,37 @@ static int __set_input_tf(struct dc_transfer_func *func,
>       return res ? 0 : -ENOMEM;
>  }
>  
> +/**
> + * Verifies that the Degamma and Gamma LUTs attached to the |crtc_state| are 
> of
> + * the expected size.
> + * Returns 0 on success.
> + */
> +int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state)
> +{
> +     const struct drm_color_lut *lut = NULL;
> +     uint32_t size = 0;
> +
> +     lut = __extract_blob_lut(crtc_state->degamma_lut, &size);
> +     if (lut && size != MAX_COLOR_LUT_ENTRIES) {

Isn't the point of the LUT size that it can be variable? Did you observe any
problems with LUTs that are not of size 4096?

Legacy X-based userspace will give us 256 size LUTs. We can't break support for
that. See MAX_COLOR_LEGACY_LUT_ENTRIES.

Harry

> +             DRM_DEBUG_DRIVER(
> +                     "Invalid Degamma LUT size. Should be %u but got %u.\n",
> +                     MAX_COLOR_LUT_ENTRIES, size);
> +             return -EINVAL;
> +     }
> +
> +     lut = __extract_blob_lut(crtc_state->gamma_lut, &size);
> +     if (lut && size != MAX_COLOR_LUT_ENTRIES &&
> +         size != MAX_COLOR_LEGACY_LUT_ENTRIES) {
> +             DRM_DEBUG_DRIVER(
> +                     "Invalid Gamma LUT size. Should be %u (or %u for 
> legacy) but got %u.\n",
> +                     MAX_COLOR_LUT_ENTRIES, MAX_COLOR_LEGACY_LUT_ENTRIES,
> +                     size);
> +             return -EINVAL;
> +     }
> +
> +     return 0;
> +}
> +
>  /**
>   * amdgpu_dm_update_crtc_color_mgmt: Maps DRM color management to DC stream.
>   * @crtc: amdgpu_dm crtc state
> @@ -317,14 +348,11 @@ int amdgpu_dm_update_crtc_color_mgmt(struct 
> dm_crtc_state *crtc)
>       bool is_legacy;
>       int r;
>  
> -     degamma_lut = __extract_blob_lut(crtc->base.degamma_lut, &degamma_size);
> -     if (degamma_lut && degamma_size != MAX_COLOR_LUT_ENTRIES)
> -             return -EINVAL;
> +     if ((r = amdgpu_dm_verify_lut_sizes(&crtc->base)))
> +             return r;
>  
> +     degamma_lut = __extract_blob_lut(crtc->base.degamma_lut, &degamma_size);
>       regamma_lut = __extract_blob_lut(crtc->base.gamma_lut, &regamma_size);
> -     if (regamma_lut && regamma_size != MAX_COLOR_LUT_ENTRIES &&
> -         regamma_size != MAX_COLOR_LEGACY_LUT_ENTRIES)
> -             return -EINVAL;
>  
>       has_degamma =
>               degamma_lut && !__is_lut_linear(degamma_lut, degamma_size);
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx