Re: [PATCH v5 2/3] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
On Thu, Nov 04, 2021 at 05:15:43PM -0400, Ilia Mirkin wrote:
> On Thu, Nov 4, 2021 at 4:03 PM Mark Yacoub wrote:
> >
> > From: Mark Yacoub
> >
> > [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.
> > 3. As the LUT size check now happens in drm_atomic_helper_check, remove
> > the lut check in intel_color.c
> >
> > Resolves: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK
> > Tested on Zork (amdgpu) and Jacuzzi (mediatek), volteer (TGL)
> >
> > v3:
> > 1. Check for lut pointer inside drm_check_lut_size.
> >
> > v2:
> > 1. Remove the rename to a parent commit.
> > 2. Create a drm drm_check_lut_size instead of intel only function.
> >
> > v1:
> > 1. Fix typos
> > 2. Remove the LUT size check from intel driver
> > 3. Rename old LUT check to indicate it's a channel change
> >
> > Signed-off-by: Mark Yacoub
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c| 52 ++
> > drivers/gpu/drm/drm_color_mgmt.c | 19
> > drivers/gpu/drm/i915/display/intel_color.c | 32 ++---
> > include/drm/drm_atomic_helper.h| 1 +
> > include/drm/drm_color_mgmt.h | 3 ++
> > include/drm/drm_crtc.h | 11 +
> > 6 files changed, 99 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index bc3487964fb5e..548e5d8221fb4 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -929,6 +929,54 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_check_planes);
> >
> > +/**
> > + * drm_atomic_helper_check_crtcs - validate state object for CRTC changes
> > + * @state: the driver state object
> > + *
> > + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the
> > new
> > + * state holds them.
> > + *
> > + * RETURNS:
> > + * Zero for success or -errno
> > + */
> > +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state)
> > +{
> > + 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) {
> > + if (!new_crtc_state->color_mgmt_changed)
> > + continue;
> > +
> > + if (drm_check_lut_size(new_crtc_state->gamma_lut,
> > + crtc->gamma_lut_size) ||
> > + drm_check_lut_size(new_crtc_state->gamma_lut,
> > + crtc->gamma_size)) {
> > + drm_dbg_state(
> > + state->dev,
> > + "Invalid Gamma LUT size. Expected %u/%u,
> > got %u.\n",
> > + crtc->gamma_lut_size, crtc->gamma_size,
> > +
> > drm_color_lut_size(new_crtc_state->gamma_lut));
> > + return -EINVAL;
> > + }
> > +
> > + if (drm_check_lut_size(new_crtc_state->degamma_lut,
> > + crtc->degamma_lut_size)) {
> > + drm_dbg_state(
> > + state->dev,
> > + "Invalid Degamma LUT size. Expected %u, got
> > %u.\n",
> > + crtc->degamma_lut_size,
> > + drm_color_lut_size(
> > + new_crtc_state->degamma_lut));
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs);
> > +
> > /**
> > * drm_atomic_helper_check - validate state object
> > * @dev: DRM device
> > @@ -974,6 +1022,10 @@ int drm_atomic_helper_check(struct drm_device *dev,
> > if (ret)
> > return ret;
> >
> > + ret = drm_atomic_helper_check_crtcs(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 16a07f84948f3..c85094223b535 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@
Re: [PATCH v5 2/3] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
On Thu, Nov 4, 2021 at 4:03 PM Mark Yacoub wrote:
>
> From: Mark Yacoub
>
> [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.
> 3. As the LUT size check now happens in drm_atomic_helper_check, remove
> the lut check in intel_color.c
>
> Resolves: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK
> Tested on Zork (amdgpu) and Jacuzzi (mediatek), volteer (TGL)
>
> v3:
> 1. Check for lut pointer inside drm_check_lut_size.
>
> v2:
> 1. Remove the rename to a parent commit.
> 2. Create a drm drm_check_lut_size instead of intel only function.
>
> v1:
> 1. Fix typos
> 2. Remove the LUT size check from intel driver
> 3. Rename old LUT check to indicate it's a channel change
>
> Signed-off-by: Mark Yacoub
> ---
> drivers/gpu/drm/drm_atomic_helper.c| 52 ++
> drivers/gpu/drm/drm_color_mgmt.c | 19
> drivers/gpu/drm/i915/display/intel_color.c | 32 ++---
> include/drm/drm_atomic_helper.h| 1 +
> include/drm/drm_color_mgmt.h | 3 ++
> include/drm/drm_crtc.h | 11 +
> 6 files changed, 99 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index bc3487964fb5e..548e5d8221fb4 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -929,6 +929,54 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_helper_check_planes);
>
> +/**
> + * drm_atomic_helper_check_crtcs - validate state object for CRTC changes
> + * @state: the driver state object
> + *
> + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new
> + * state holds them.
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state)
> +{
> + 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) {
> + if (!new_crtc_state->color_mgmt_changed)
> + continue;
> +
> + if (drm_check_lut_size(new_crtc_state->gamma_lut,
> + crtc->gamma_lut_size) ||
> + drm_check_lut_size(new_crtc_state->gamma_lut,
> + crtc->gamma_size)) {
> + drm_dbg_state(
> + state->dev,
> + "Invalid Gamma LUT size. Expected %u/%u, got
> %u.\n",
> + crtc->gamma_lut_size, crtc->gamma_size,
> +
> drm_color_lut_size(new_crtc_state->gamma_lut));
> + return -EINVAL;
> + }
> +
> + if (drm_check_lut_size(new_crtc_state->degamma_lut,
> + crtc->degamma_lut_size)) {
> + drm_dbg_state(
> + state->dev,
> + "Invalid Degamma LUT size. Expected %u, got
> %u.\n",
> + crtc->degamma_lut_size,
> + drm_color_lut_size(
> + new_crtc_state->degamma_lut));
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs);
> +
> /**
> * drm_atomic_helper_check - validate state object
> * @dev: DRM device
> @@ -974,6 +1022,10 @@ int drm_atomic_helper_check(struct drm_device *dev,
> if (ret)
> return ret;
>
> + ret = drm_atomic_helper_check_crtcs(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 16a07f84948f3..c85094223b535 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 = >mode_config;
>
> if (degamma_lut_size) {
> + crtc->degamma_lut_size = degamma_lut_size;
> drm_object_attach_property(>base,
>
[PATCH v5 2/3] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
From: Mark Yacoub
[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.
3. As the LUT size check now happens in drm_atomic_helper_check, remove
the lut check in intel_color.c
Resolves: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK
Tested on Zork (amdgpu) and Jacuzzi (mediatek), volteer (TGL)
v3:
1. Check for lut pointer inside drm_check_lut_size.
v2:
1. Remove the rename to a parent commit.
2. Create a drm drm_check_lut_size instead of intel only function.
v1:
1. Fix typos
2. Remove the LUT size check from intel driver
3. Rename old LUT check to indicate it's a channel change
Signed-off-by: Mark Yacoub
---
drivers/gpu/drm/drm_atomic_helper.c| 52 ++
drivers/gpu/drm/drm_color_mgmt.c | 19
drivers/gpu/drm/i915/display/intel_color.c | 32 ++---
include/drm/drm_atomic_helper.h| 1 +
include/drm/drm_color_mgmt.h | 3 ++
include/drm/drm_crtc.h | 11 +
6 files changed, 99 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index bc3487964fb5e..548e5d8221fb4 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -929,6 +929,54 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_atomic_helper_check_planes);
+/**
+ * drm_atomic_helper_check_crtcs - validate state object for CRTC changes
+ * @state: the driver state object
+ *
+ * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new
+ * state holds them.
+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state)
+{
+ 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) {
+ if (!new_crtc_state->color_mgmt_changed)
+ continue;
+
+ if (drm_check_lut_size(new_crtc_state->gamma_lut,
+ crtc->gamma_lut_size) ||
+ drm_check_lut_size(new_crtc_state->gamma_lut,
+ crtc->gamma_size)) {
+ drm_dbg_state(
+ state->dev,
+ "Invalid Gamma LUT size. Expected %u/%u, got
%u.\n",
+ crtc->gamma_lut_size, crtc->gamma_size,
+ drm_color_lut_size(new_crtc_state->gamma_lut));
+ return -EINVAL;
+ }
+
+ if (drm_check_lut_size(new_crtc_state->degamma_lut,
+ crtc->degamma_lut_size)) {
+ drm_dbg_state(
+ state->dev,
+ "Invalid Degamma LUT size. Expected %u, got
%u.\n",
+ crtc->degamma_lut_size,
+ drm_color_lut_size(
+ new_crtc_state->degamma_lut));
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_atomic_helper_check_crtcs);
+
/**
* drm_atomic_helper_check - validate state object
* @dev: DRM device
@@ -974,6 +1022,10 @@ int drm_atomic_helper_check(struct drm_device *dev,
if (ret)
return ret;
+ ret = drm_atomic_helper_check_crtcs(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 16a07f84948f3..c85094223b535 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 = >mode_config;
if (degamma_lut_size) {
+ crtc->degamma_lut_size = degamma_lut_size;
drm_object_attach_property(>base,
config->degamma_lut_property, 0);
drm_object_attach_property(>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 =
