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

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

[Mark Yacoub]

From: Mark Yacoub <markyacoub@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 <markyacoub@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) {
+		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);
-- 
2.32.0.rc1.229.g3e70b5a671-goog

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

[Mark Yacoub]

From: Mark Yacoub <markyacoub@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 <markyacoub@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) {
+		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);
-- 
2.32.0.rc1.229.g3e70b5a671-goog

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

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 <markyacoub@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 <markyacoub@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);
> 

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 <markyacoub@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 <markyacoub@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

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

[Mark Yacoub]

On Fri, Jun 4, 2021 at 4:17 PM Harry Wentland <harry.wentland@amd.com> wrote:
>
>
>
> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
> > From: Mark Yacoub <markyacoub@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 <markyacoub@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.
>
> 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

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

[Mark Yacoub]

On Fri, Jun 4, 2021 at 4:17 PM Harry Wentland <harry.wentland@amd.com> wrote:
>
>
>
> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
> > From: Mark Yacoub <markyacoub@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 <markyacoub@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.
>
> 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Harry Wentland]

On 2021-06-07 10:53 a.m., Mark Yacoub wrote:
> On Fri, Jun 4, 2021 at 4:17 PM Harry Wentland <harry.wentland@amd.com> wrote:
>>
>>
>>
>> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
>>> From: Mark Yacoub <markyacoub@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 <markyacoub@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.wentland@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, &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
> 

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

[Harry Wentland]

On 2021-06-07 10:53 a.m., Mark Yacoub wrote:
> On Fri, Jun 4, 2021 at 4:17 PM Harry Wentland <harry.wentland@amd.com> wrote:
>>
>>
>>
>> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
>>> From: Mark Yacoub <markyacoub@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 <markyacoub@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.wentland@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, &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
> 

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

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

[Alex Deucher]

Applied.  Thanks!

On Thu, Jun 10, 2021 at 5:14 PM Harry Wentland <harry.wentland@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.wentland@amd.com> wrote:
> >>
> >>
> >>
> >> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
> >>> From: Mark Yacoub <markyacoub@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 <markyacoub@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.wentland@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, &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
> >
>

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

[Alex Deucher]

Applied.  Thanks!

On Thu, Jun 10, 2021 at 5:14 PM Harry Wentland <harry.wentland@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.wentland@amd.com> wrote:
> >>
> >>
> >>
> >> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
> >>> From: Mark Yacoub <markyacoub@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 <markyacoub@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.wentland@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, &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
> >
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Alex Deucher]

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 <alexdeucher@gmail.com> wrote:
>
> Applied.  Thanks!
>
> On Thu, Jun 10, 2021 at 5:14 PM Harry Wentland <harry.wentland@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.wentland@amd.com> wrote:
> > >>
> > >>
> > >>
> > >> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
> > >>> From: Mark Yacoub <markyacoub@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 <markyacoub@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.wentland@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, &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
> > >
> >

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

[Alex Deucher]

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 <alexdeucher@gmail.com> wrote:
>
> Applied.  Thanks!
>
> On Thu, Jun 10, 2021 at 5:14 PM Harry Wentland <harry.wentland@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.wentland@amd.com> wrote:
> > >>
> > >>
> > >>
> > >> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
> > >>> From: Mark Yacoub <markyacoub@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 <markyacoub@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.wentland@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, &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
> > >
> >
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Mark Yacoub]

hmm I see, thanks for the heads up, I'll double check why it uses
google email for sending.
wrt the assignment in the if clauses, are those typically frowned upon?
Thanks!

On Fri, Jun 11, 2021 at 4:17 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> 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 <alexdeucher@gmail.com> wrote:
> >
> > Applied.  Thanks!
> >
> > On Thu, Jun 10, 2021 at 5:14 PM Harry Wentland <harry.wentland@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.wentland@amd.com> wrote:
> > > >>
> > > >>
> > > >>
> > > >> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
> > > >>> From: Mark Yacoub <markyacoub@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 <markyacoub@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.wentland@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, &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
> > > >
> > >

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

[Mark Yacoub]

hmm I see, thanks for the heads up, I'll double check why it uses
google email for sending.
wrt the assignment in the if clauses, are those typically frowned upon?
Thanks!

On Fri, Jun 11, 2021 at 4:17 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> 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 <alexdeucher@gmail.com> wrote:
> >
> > Applied.  Thanks!
> >
> > On Thu, Jun 10, 2021 at 5:14 PM Harry Wentland <harry.wentland@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.wentland@amd.com> wrote:
> > > >>
> > > >>
> > > >>
> > > >> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
> > > >>> From: Mark Yacoub <markyacoub@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 <markyacoub@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.wentland@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, &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
> > > >
> > >
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Alex Deucher]

On Mon, Jun 14, 2021 at 10:49 AM Mark Yacoub <markyacoub@chromium.org> wrote:
>
> hmm I see, thanks for the heads up, I'll double check why it uses
> google email for sending.
> wrt the assignment in the if clauses, are those typically frowned upon?

Yes, checkpatch complains about them.

Alex


> Thanks!
>
> On Fri, Jun 11, 2021 at 4:17 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > 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 <alexdeucher@gmail.com> wrote:
> > >
> > > Applied.  Thanks!
> > >
> > > On Thu, Jun 10, 2021 at 5:14 PM Harry Wentland <harry.wentland@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.wentland@amd.com> wrote:
> > > > >>
> > > > >>
> > > > >>
> > > > >> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
> > > > >>> From: Mark Yacoub <markyacoub@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 <markyacoub@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.wentland@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, &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
> > > > >
> > > >

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

[Alex Deucher]

On Mon, Jun 14, 2021 at 10:49 AM Mark Yacoub <markyacoub@chromium.org> wrote:
>
> hmm I see, thanks for the heads up, I'll double check why it uses
> google email for sending.
> wrt the assignment in the if clauses, are those typically frowned upon?

Yes, checkpatch complains about them.

Alex


> Thanks!
>
> On Fri, Jun 11, 2021 at 4:17 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > 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 <alexdeucher@gmail.com> wrote:
> > >
> > > Applied.  Thanks!
> > >
> > > On Thu, Jun 10, 2021 at 5:14 PM Harry Wentland <harry.wentland@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.wentland@amd.com> wrote:
> > > > >>
> > > > >>
> > > > >>
> > > > >> On 2021-06-04 1:01 p.m., Mark Yacoub wrote:
> > > > >>> From: Mark Yacoub <markyacoub@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 <markyacoub@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.wentland@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, &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
> > > > >
> > > >
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx