From: Sean Paul <sean@poorly.run> To: Mark Yacoub <markyacoub@chromium.org> Cc: dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org, seanpaul@chromium.org, Rodrigo.Siqueira@amd.com, anson.jacob@amd.com, Mark Yacoub <markyacoub@google.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Matthias Brugger <matthias.bgg@gmail.com>, linux-mediatek@lists.infradead.org Subject: Re: [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate. Date: Fri, 1 Oct 2021 16:34:34 -0400 [thread overview] Message-ID: <20211001203434.GY2515@art_vandelay> (raw) In-Reply-To: <20210929194012.3433306-1-markyacoub@chromium.org> On Wed, Sep 29, 2021 at 03:39:25PM -0400, Mark Yacoub wrote: > From: Mark Yacoub <markyacoub@google.com> > > [Why] > 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma > or Degamma props in the new CRTC state, allowing any invalid size to > be passed on. > 2. Each driver has its own LUT size, which could also be different for > legacy users. > > [How] > 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes > assigned by the driver when it's initializing its color and CTM > management. > 2. Create drm_atomic_helper_check_crtc which is called by > drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that > they match the sizes in the new CRTC state. > Did you consider extending drm_color_lut_check() with the size checks? > Fixes: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK > Tested on Zork(amdgpu) and Jacuzzi(mediatek) > > Signed-off-by: Mark Yacoub<markyacoub@chromium.org> nit: missing a space between name and email > --- > drivers/gpu/drm/drm_atomic_helper.c | 56 +++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_color_mgmt.c | 2 ++ > include/drm/drm_atomic_helper.h | 1 + > include/drm/drm_crtc.h | 11 ++++++ > 4 files changed, 70 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 2c0c6ec928200..265b9747250d1 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -930,6 +930,58 @@ drm_atomic_helper_check_planes(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_atomic_helper_check_planes); > > +/** > + * drm_atomic_helper_check_planes - validate state object for CRTC changes Ctrl+c/Ctrl+v error here > + * @state: the driver state object > + * > + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new Are there missing words between "object" and "such"? > + * state holds them. > + * > + * RETURNS: > + * Zero for success or -errno > + */ > +int drm_atomic_helper_check_crtc(struct drm_atomic_state *state) drm_atomic_helper_check_crtcs to be consistent with drm_atomic_helper_check_planes > +{ > + struct drm_crtc *crtc; > + struct drm_crtc_state *new_crtc_state; > + int i; > + > + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) { no space before ( > + if (new_crtc_state->gamma_lut) { Perhaps gate these with a check of state->color_mgmt_changed first? > + uint64_t supported_lut_size = crtc->gamma_lut_size; > + uint32_t supported_legacy_lut_size = crtc->gamma_size; > + uint32_t new_state_lut_size = > + drm_color_lut_size(new_crtc_state->gamma_lut); nit: new_state_lut_size and supported_lut_size can be pulled out to top level scope to avoid re-instantiation on each iteration > + > + if (new_state_lut_size != supported_lut_size && > + new_state_lut_size != supported_legacy_lut_size) { According to the docbook, "If drivers support multiple LUT sizes then they should publish the largest size, and sub-sample smaller sized LUTs". So should this check be > instead of != ? > + DRM_DEBUG_DRIVER( drm_dbg_state() is probably more appropriate > + "Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n", > + supported_lut_size, > + supported_legacy_lut_size, > + new_state_lut_size); > + return -EINVAL; > + } > + } > + > + if (new_crtc_state->degamma_lut) { > + uint32_t new_state_lut_size = > + drm_color_lut_size(new_crtc_state->degamma_lut); > + uint64_t supported_lut_size = crtc->degamma_lut_size; > + > + if (new_state_lut_size != supported_lut_size) { > + DRM_DEBUG_DRIVER( drm_dbg_state() > + "Invalid Degamma LUT size. Should be %u but got %u.\n", > + supported_lut_size, new_state_lut_size); > + return -EINVAL; > + } > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_atomic_helper_check_crtc); > + > /** > * drm_atomic_helper_check - validate state object > * @dev: DRM device > @@ -975,6 +1027,10 @@ int drm_atomic_helper_check(struct drm_device *dev, > if (ret) > return ret; > > + ret = drm_atomic_helper_check_crtc(state); > + if (ret) > + return ret; > + > if (state->legacy_cursor_update) > state->async_update = !drm_atomic_helper_async_check(dev, state); > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c > index bb14f488c8f6c..72a1b628e7cdd 100644 > --- a/drivers/gpu/drm/drm_color_mgmt.c > +++ b/drivers/gpu/drm/drm_color_mgmt.c > @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > struct drm_mode_config *config = &dev->mode_config; > > if (degamma_lut_size) { > + crtc->degamma_lut_size = degamma_lut_size; > drm_object_attach_property(&crtc->base, > config->degamma_lut_property, 0); > drm_object_attach_property(&crtc->base, > @@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > config->ctm_property, 0); > > if (gamma_lut_size) { > + crtc->gamma_lut_size = gamma_lut_size; > drm_object_attach_property(&crtc->base, > config->gamma_lut_property, 0); > drm_object_attach_property(&crtc->base, > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 4045e2507e11c..3eda13622ca1e 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -38,6 +38,7 @@ struct drm_atomic_state; > struct drm_private_obj; > struct drm_private_state; > > +int drm_atomic_helper_check_crtc(struct drm_atomic_state *state); > int drm_atomic_helper_check_modeset(struct drm_device *dev, > struct drm_atomic_state *state); > int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 13eeba2a750af..c602be2cafca9 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1072,6 +1072,17 @@ struct drm_crtc { > /** @funcs: CRTC control functions */ > const struct drm_crtc_funcs *funcs; > > + /** > + * @degamma_lut_size: Size of degamma LUT. > + */ > + uint32_t degamma_lut_size; > + > + /** > + * @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace such as > + * X, which doesn't support large lut sizes. > + */ > + uint32_t gamma_lut_size; > + Above, you're checking if (new_state_lut_size != gamma_size && new_state_lut_size != gamma_lut_size) fail; doesn't that imply that gamma_size and gamma_lut_size must always be equal? If so, perhaps turf this new state and rename degamma_lut_size to degamma_size to be consistent. De-duping this and initializing crtc->gamma_size in the initialization would mean the if (crtc->gamma_size) check in drm_crtc_supports_legacy_check() is no longer useful (and possibly other similar checks), so some care will need to be taken to avoid regression. I think the effort is worthwhile to avoid introducing new state. > /** > * @gamma_size: Size of legacy gamma ramp reported to userspace. Set up > * by calling drm_mode_crtc_set_gamma_size(). > -- > 2.33.0.685.g46640cef36-goog > -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek
WARNING: multiple messages have this Message-ID (diff)
From: Sean Paul <sean@poorly.run> To: Mark Yacoub <markyacoub@chromium.org> Cc: dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org, seanpaul@chromium.org, Rodrigo.Siqueira@amd.com, anson.jacob@amd.com, Mark Yacoub <markyacoub@google.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Matthias Brugger <matthias.bgg@gmail.com>, linux-mediatek@lists.infradead.org Subject: Re: [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate. Date: Fri, 1 Oct 2021 16:34:34 -0400 [thread overview] Message-ID: <20211001203434.GY2515@art_vandelay> (raw) In-Reply-To: <20210929194012.3433306-1-markyacoub@chromium.org> On Wed, Sep 29, 2021 at 03:39:25PM -0400, Mark Yacoub wrote: > From: Mark Yacoub <markyacoub@google.com> > > [Why] > 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma > or Degamma props in the new CRTC state, allowing any invalid size to > be passed on. > 2. Each driver has its own LUT size, which could also be different for > legacy users. > > [How] > 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes > assigned by the driver when it's initializing its color and CTM > management. > 2. Create drm_atomic_helper_check_crtc which is called by > drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that > they match the sizes in the new CRTC state. > Did you consider extending drm_color_lut_check() with the size checks? > Fixes: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK > Tested on Zork(amdgpu) and Jacuzzi(mediatek) > > Signed-off-by: Mark Yacoub<markyacoub@chromium.org> nit: missing a space between name and email > --- > drivers/gpu/drm/drm_atomic_helper.c | 56 +++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_color_mgmt.c | 2 ++ > include/drm/drm_atomic_helper.h | 1 + > include/drm/drm_crtc.h | 11 ++++++ > 4 files changed, 70 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 2c0c6ec928200..265b9747250d1 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -930,6 +930,58 @@ drm_atomic_helper_check_planes(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_atomic_helper_check_planes); > > +/** > + * drm_atomic_helper_check_planes - validate state object for CRTC changes Ctrl+c/Ctrl+v error here > + * @state: the driver state object > + * > + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new Are there missing words between "object" and "such"? > + * state holds them. > + * > + * RETURNS: > + * Zero for success or -errno > + */ > +int drm_atomic_helper_check_crtc(struct drm_atomic_state *state) drm_atomic_helper_check_crtcs to be consistent with drm_atomic_helper_check_planes > +{ > + struct drm_crtc *crtc; > + struct drm_crtc_state *new_crtc_state; > + int i; > + > + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) { no space before ( > + if (new_crtc_state->gamma_lut) { Perhaps gate these with a check of state->color_mgmt_changed first? > + uint64_t supported_lut_size = crtc->gamma_lut_size; > + uint32_t supported_legacy_lut_size = crtc->gamma_size; > + uint32_t new_state_lut_size = > + drm_color_lut_size(new_crtc_state->gamma_lut); nit: new_state_lut_size and supported_lut_size can be pulled out to top level scope to avoid re-instantiation on each iteration > + > + if (new_state_lut_size != supported_lut_size && > + new_state_lut_size != supported_legacy_lut_size) { According to the docbook, "If drivers support multiple LUT sizes then they should publish the largest size, and sub-sample smaller sized LUTs". So should this check be > instead of != ? > + DRM_DEBUG_DRIVER( drm_dbg_state() is probably more appropriate > + "Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n", > + supported_lut_size, > + supported_legacy_lut_size, > + new_state_lut_size); > + return -EINVAL; > + } > + } > + > + if (new_crtc_state->degamma_lut) { > + uint32_t new_state_lut_size = > + drm_color_lut_size(new_crtc_state->degamma_lut); > + uint64_t supported_lut_size = crtc->degamma_lut_size; > + > + if (new_state_lut_size != supported_lut_size) { > + DRM_DEBUG_DRIVER( drm_dbg_state() > + "Invalid Degamma LUT size. Should be %u but got %u.\n", > + supported_lut_size, new_state_lut_size); > + return -EINVAL; > + } > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_atomic_helper_check_crtc); > + > /** > * drm_atomic_helper_check - validate state object > * @dev: DRM device > @@ -975,6 +1027,10 @@ int drm_atomic_helper_check(struct drm_device *dev, > if (ret) > return ret; > > + ret = drm_atomic_helper_check_crtc(state); > + if (ret) > + return ret; > + > if (state->legacy_cursor_update) > state->async_update = !drm_atomic_helper_async_check(dev, state); > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c > index bb14f488c8f6c..72a1b628e7cdd 100644 > --- a/drivers/gpu/drm/drm_color_mgmt.c > +++ b/drivers/gpu/drm/drm_color_mgmt.c > @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > struct drm_mode_config *config = &dev->mode_config; > > if (degamma_lut_size) { > + crtc->degamma_lut_size = degamma_lut_size; > drm_object_attach_property(&crtc->base, > config->degamma_lut_property, 0); > drm_object_attach_property(&crtc->base, > @@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > config->ctm_property, 0); > > if (gamma_lut_size) { > + crtc->gamma_lut_size = gamma_lut_size; > drm_object_attach_property(&crtc->base, > config->gamma_lut_property, 0); > drm_object_attach_property(&crtc->base, > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 4045e2507e11c..3eda13622ca1e 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -38,6 +38,7 @@ struct drm_atomic_state; > struct drm_private_obj; > struct drm_private_state; > > +int drm_atomic_helper_check_crtc(struct drm_atomic_state *state); > int drm_atomic_helper_check_modeset(struct drm_device *dev, > struct drm_atomic_state *state); > int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 13eeba2a750af..c602be2cafca9 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1072,6 +1072,17 @@ struct drm_crtc { > /** @funcs: CRTC control functions */ > const struct drm_crtc_funcs *funcs; > > + /** > + * @degamma_lut_size: Size of degamma LUT. > + */ > + uint32_t degamma_lut_size; > + > + /** > + * @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace such as > + * X, which doesn't support large lut sizes. > + */ > + uint32_t gamma_lut_size; > + Above, you're checking if (new_state_lut_size != gamma_size && new_state_lut_size != gamma_lut_size) fail; doesn't that imply that gamma_size and gamma_lut_size must always be equal? If so, perhaps turf this new state and rename degamma_lut_size to degamma_size to be consistent. De-duping this and initializing crtc->gamma_size in the initialization would mean the if (crtc->gamma_size) check in drm_crtc_supports_legacy_check() is no longer useful (and possibly other similar checks), so some care will need to be taken to avoid regression. I think the effort is worthwhile to avoid introducing new state. > /** > * @gamma_size: Size of legacy gamma ramp reported to userspace. Set up > * by calling drm_mode_crtc_set_gamma_size(). > -- > 2.33.0.685.g46640cef36-goog > -- Sean Paul, Software Engineer, Google / Chromium OS
next prev parent reply other threads:[~2021-10-01 20:34 UTC|newest] Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-29 19:39 [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate Mark Yacoub 2021-09-29 19:39 ` Mark Yacoub 2021-09-29 19:39 ` [PATCH 2/2] amd/amdgpu_dm: Verify Gamma and Degamma LUT sizes using DRM Core check Mark Yacoub 2021-10-01 19:56 ` Sean Paul 2021-10-04 14:12 ` Harry Wentland 2021-10-01 20:34 ` Sean Paul [this message] 2021-10-01 20:34 ` [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate Sean Paul 2021-10-02 14:28 ` Sean Paul 2021-10-02 14:28 ` Sean Paul 2021-10-13 18:18 ` Mark Yacoub 2021-10-13 18:18 ` Mark Yacoub 2021-10-13 18:12 ` Mark Yacoub 2021-10-13 18:12 ` Mark Yacoub 2021-10-13 18:12 ` [Intel-gfx] " Mark Yacoub 2021-10-13 18:12 ` Mark Yacoub 2021-10-13 18:12 ` [PATCH 2/2] amd/amdgpu_dm: Verify Gamma and Degamma LUT sizes using DRM Core check Mark Yacoub 2021-10-13 18:12 ` [Intel-gfx] " Mark Yacoub 2021-10-14 23:42 ` Sean Paul 2021-10-14 23:42 ` [Intel-gfx] " Sean Paul 2021-10-26 1:26 ` [Intel-gfx] [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate Sean Paul 2021-10-26 1:26 ` Sean Paul 2021-10-26 1:26 ` Sean Paul 2021-10-26 19:25 ` Mark Yacoub 2021-10-26 19:25 ` Mark Yacoub 2021-10-26 19:25 ` Mark Yacoub 2021-10-26 12:02 ` Paul Menzel 2021-10-26 12:02 ` [Intel-gfx] " Paul Menzel 2021-10-26 12:02 ` Paul Menzel 2021-10-26 12:02 ` Paul Menzel 2021-10-26 19:24 ` Mark Yacoub 2021-10-26 19:24 ` Mark Yacoub 2021-10-26 19:24 ` [Intel-gfx] " Mark Yacoub 2021-10-26 19:24 ` Mark Yacoub 2021-10-26 19:26 ` [Intel-gfx] " Mark Yacoub 2021-10-26 19:26 ` Mark Yacoub 2021-10-26 19:26 ` Mark Yacoub 2021-10-26 19:26 ` Mark Yacoub
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20211001203434.GY2515@art_vandelay \ --to=sean@poorly.run \ --cc=Rodrigo.Siqueira@amd.com \ --cc=airlied@linux.ie \ --cc=amd-gfx@lists.freedesktop.org \ --cc=anson.jacob@amd.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=markyacoub@chromium.org \ --cc=markyacoub@google.com \ --cc=matthias.bgg@gmail.com \ --cc=mripard@kernel.org \ --cc=seanpaul@chromium.org \ --cc=tzimmermann@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.