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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2021-06-07 10:53 a.m., Mark Yacoub wrote:
> On Fri, Jun 4, 2021 at 4:17 PM Harry Wentland <harry.wentland@xxxxxxx> wrote:
>>
>>
>>
>> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
>>> From: Mark Yacoub <markyacoub@xxxxxxxxxx>
>>>
>>> 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 <markyacoub@xxxxxxxxxxxx>
>>> 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?
> Is it supposed to be variable?
> I'm basing my knowledge of LUTs on this IGT Test:
> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_color_helper.c#L281>> It does check for invalid sizes and for the exact size, giving me the
> impression that it's not too flexible.
> Is variability of size an AMD specific behavior or should it be a DRM behavior?
>>
>> Legacy X-based userspace will give us 256 size LUTs. We can't break support for
>> that. See MAX_COLOR_LEGACY_LUT_ENTRIES.
> In the new function `amdgpu_dm_verify_lut_sizes`, I maintained parity
> with the old behavior. In `amdgpu_dm_update_crtc_color_mgmt`, the
> degamma size is only checked against `MAX_COLOR_LUT_ENTRIES` while
> regamma_size size is checked against both MAX_COLOR_LUT_ENTRIES and
> MAX_COLOR_LEGACY_LUT_ENTRIES:
> https://gitlab.freedesktop.org/agd5f/linux/-/blob/amd-staging-drm-next/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c#L321>> Also, in the definition of MAX_COLOR_LEGACY_LUT_ENTRIES, it mentions
> "Legacy gamm[sic] LUT" not degamma:
> https://gitlab.freedesktop.org/agd5f/linux/-/blame/amd-staging-drm-next/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h#L616>> As well as the commit when it was introduced, it seems to be handling
> gammas rather than degamma LUTs:
> https://gitlab.freedesktop.org/agd5f/linux/-/commit/086247a4b2fba49800b27807f22bb894cd8363fb>> Let me know if this would be a bug in the old behavior and I can fix
> it, or if i'm missing something.

Ah, yes, you're right, of course. Thanks for walking me through it. :)

Reviewed-by: Harry Wentland <harry.wentland@xxxxxxx>

Harry

>>
>> 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);
>>>
>>
> -Mark
> 




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux