Just a heads up, your sender email and your signed-off-by don't match
and you had some assignments in if clauses. I've fixed those up.
Alex
On Fri, Jun 11, 2021 at 1:35 PM Alex Deucher <alexdeuc...@gmail.com> wrote:
>
> Applied. Thanks!
>
> On Thu, Jun 10, 2021 at 5:14 PM Harry Wentland <harry.wentl...@amd.com> wrote:
> >
> >
> >
> > On 2021-06-07 10:53 a.m., Mark Yacoub wrote:
> > > On Fri, Jun 4, 2021 at 4:17 PM Harry Wentland <harry.wentl...@amd.com>
> > > wrote:
> > >>
> > >>
> > >>
> > >> 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?
> > > 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.wentl...@amd.com>
> >
> > 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,
> > >>> °amma_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,
> > >>> °amma_size);
> > >>> regamma_lut = __extract_blob_lut(crtc->base.gamma_lut,
> > >>> ®amma_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
> > >
> >
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
