* [PATCH 2/2] amd/amdgpu_dm: Verify Gamma and Degamma LUT sizes using DRM Core check
2021-10-13 18:12 ` [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate Mark Yacoub
@ 2021-10-13 18:12 ` Mark Yacoub
2021-10-14 23:42 ` Sean Paul
2021-10-26 1:26 ` [Intel-gfx] [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate Sean Paul
2021-10-26 12:02 ` Paul Menzel
2 siblings, 1 reply; 8+ messages in thread
From: Mark Yacoub @ 2021-10-13 18:12 UTC (permalink / raw)
To: dri-devel, amd-gfx, intel-gfx
Cc: seanpaul, harry.wentland, Mark Yacoub, Mark Yacoub, Leo Li,
Alex Deucher, Christian König, Pan, Xinhui, David Airlie,
Daniel Vetter, linux-kernel
From: Mark Yacoub <markyacoub@google.com>
[Why]
drm_atomic_helper_check_crtc now verifies both legacy and non-legacy LUT
sizes. There is no need to check it within amdgpu_dm_atomic_check.
[How]
Remove the local call to verify LUT sizes and use DRM Core function
instead.
Tested on ChromeOS Zork.
v1:
Remove amdgpu_dm_verify_lut_sizes everywhere.
Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
.../amd/display/amdgpu_dm/amdgpu_dm_color.c | 35 -------------------
3 files changed, 4 insertions(+), 40 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 f74663b6b046e..47f8de1cfc3a5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10244,6 +10244,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
}
}
#endif
+ ret = drm_atomic_helper_check_crtcs(state);
+ if (ret)
+ return ret;
+
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
@@ -10253,10 +10257,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
dm_old_crtc_state->dsc_force_changed == false)
continue;
- ret = amdgpu_dm_verify_lut_sizes(new_crtc_state);
- if (ret)
- 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 fcb9c4a629c32..22730e5542092 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -617,7 +617,6 @@ 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 a022e5bb30a5c..319f8a8a89835 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,37 +284,6 @@ 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
@@ -348,10 +317,6 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
bool is_legacy;
int r;
- r = amdgpu_dm_verify_lut_sizes(&crtc->base);
- if (r)
- return r;
-
degamma_lut = __extract_blob_lut(crtc->base.degamma_lut, °amma_size);
regamma_lut = __extract_blob_lut(crtc->base.gamma_lut, ®amma_size);
--
2.33.0.882.g93a45727a2-goog
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] amd/amdgpu_dm: Verify Gamma and Degamma LUT sizes using DRM Core check
2021-10-13 18:12 ` [PATCH 2/2] amd/amdgpu_dm: Verify Gamma and Degamma LUT sizes using DRM Core check Mark Yacoub
@ 2021-10-14 23:42 ` Sean Paul
0 siblings, 0 replies; 8+ messages in thread
From: Sean Paul @ 2021-10-14 23:42 UTC (permalink / raw)
To: Mark Yacoub
Cc: dri-devel, amd-gfx, intel-gfx, harry.wentland, Mark Yacoub,
Leo Li, Alex Deucher, Christian König, Pan, Xinhui,
David Airlie, Daniel Vetter, linux-kernel
On Wed, Oct 13, 2021 at 2:12 PM Mark Yacoub <markyacoub@chromium.org> wrote:
>
> From: Mark Yacoub <markyacoub@google.com>
>
> [Why]
> drm_atomic_helper_check_crtc now verifies both legacy and non-legacy LUT
> sizes. There is no need to check it within amdgpu_dm_atomic_check.
>
> [How]
> Remove the local call to verify LUT sizes and use DRM Core function
> instead.
>
> Tested on ChromeOS Zork.
>
> v1:
> Remove amdgpu_dm_verify_lut_sizes everywhere.
>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 35 -------------------
> 3 files changed, 4 insertions(+), 40 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 f74663b6b046e..47f8de1cfc3a5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10244,6 +10244,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> }
> }
> #endif
> + ret = drm_atomic_helper_check_crtcs(state);
> + if (ret)
> + return ret;
> +
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
>
> @@ -10253,10 +10257,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> dm_old_crtc_state->dsc_force_changed == false)
> continue;
>
> - ret = amdgpu_dm_verify_lut_sizes(new_crtc_state);
> - if (ret)
> - 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 fcb9c4a629c32..22730e5542092 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -617,7 +617,6 @@ 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 a022e5bb30a5c..319f8a8a89835 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,37 +284,6 @@ 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
> @@ -348,10 +317,6 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
> bool is_legacy;
> int r;
>
> - r = amdgpu_dm_verify_lut_sizes(&crtc->base);
> - if (r)
> - return r;
> -
> degamma_lut = __extract_blob_lut(crtc->base.degamma_lut, °amma_size);
> regamma_lut = __extract_blob_lut(crtc->base.gamma_lut, ®amma_size);
>
> --
> 2.33.0.882.g93a45727a2-goog
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Intel-gfx] [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
2021-10-13 18:12 ` [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate Mark Yacoub
2021-10-13 18:12 ` [PATCH 2/2] amd/amdgpu_dm: Verify Gamma and Degamma LUT sizes using DRM Core check Mark Yacoub
@ 2021-10-26 1:26 ` Sean Paul
2021-10-26 19:25 ` Mark Yacoub
2021-10-26 12:02 ` Paul Menzel
2 siblings, 1 reply; 8+ messages in thread
From: Sean Paul @ 2021-10-26 1:26 UTC (permalink / raw)
To: Mark Yacoub
Cc: dri-devel, amd-gfx, intel-gfx, seanpaul, harry.wentland,
Mark Yacoub, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Matthias Brugger, linux-kernel, linux-arm-kernel,
linux-mediatek
On Wed, Oct 13, 2021 at 02:12:20PM -0400, Mark Yacoub wrote:
> From: Mark Yacoub <markyacoub@google.com>
>
> [Why]
> 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma
> or Degamma props in the new CRTC state, allowing any invalid size to
> be passed on.
> 2. Each driver has its own LUT size, which could also be different for
> legacy users.
>
> [How]
> 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes
> assigned by the driver when it's initializing its color and CTM
> management.
> 2. Create drm_atomic_helper_check_crtc which is called by
> drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that
> they match the sizes in the new CRTC state.
> 3. Rename older lut checks that test for the color channels to indicate
> it's a channel check. It's not included in drm_atomic_helper_check_crtc
> as it's hardware specific and is to be called by the driver.
> 4. As the LUT size check now happens in drm_atomic_helper_check, remove
> the lut check in intel_color.c
I'd probably split the rename out from the crtc check since they're only
tangentially related.
>
> Fixes: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK
> Tested on Zork(amdgpu) and Jacuzzi(mediatek), volteer(TGL)
>
> v1:
> 1. Fix typos
> 2. Remove the LUT size check from intel driver
> 3. Rename old LUT check to indicate it's a channel change
>
> Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 60 ++++++++++++++++++++++
> drivers/gpu/drm/drm_color_mgmt.c | 14 ++---
> drivers/gpu/drm/i915/display/intel_color.c | 14 ++---
> include/drm/drm_atomic_helper.h | 1 +
> include/drm/drm_color_mgmt.h | 7 +--
> include/drm/drm_crtc.h | 11 ++++
> 6 files changed, 89 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index bc3487964fb5e..5feb2ad0209c3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -929,6 +929,62 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_helper_check_planes);
>
> +/**
> + * drm_atomic_helper_check_crtcs - validate state object for CRTC changes
> + * @state: the driver state object
> + *
> + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new
> + * state holds them.
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state)
> +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *new_crtc_state;
> + int i;
> +
> + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
> + if (new_crtc_state->color_mgmt_changed &&
> + new_crtc_state->gamma_lut) {
> + uint64_t supported_lut_size = crtc->gamma_lut_size;
> + uint32_t supported_legacy_lut_size = crtc->gamma_size;
> + uint32_t new_state_lut_size =
> + drm_color_lut_size(new_crtc_state->gamma_lut);
> +
> + if (new_state_lut_size != supported_lut_size &&
> + new_state_lut_size != supported_legacy_lut_size) {
> + drm_dbg_state(
> + state->dev,
> + "Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n",
> + supported_lut_size,
> + supported_legacy_lut_size,
> + new_state_lut_size);
> + return -EINVAL;
> + }
> + }
> +
> + if (new_crtc_state->color_mgmt_changed &&
> + new_crtc_state->degamma_lut) {
> + uint32_t new_state_lut_size =
> + drm_color_lut_size(new_crtc_state->degamma_lut);
> + uint64_t supported_lut_size = crtc->degamma_lut_size;
> +
> + if (new_state_lut_size != supported_lut_size) {
> + drm_dbg_state(
> + state->dev,
> + "Invalid Degamma LUT size. Should be %u but got %u.\n",
> + supported_lut_size, new_state_lut_size);
> + return -EINVAL;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs);
> +
> /**
> * drm_atomic_helper_check - validate state object
> * @dev: DRM device
> @@ -974,6 +1030,10 @@ int drm_atomic_helper_check(struct drm_device *dev,
> if (ret)
> return ret;
>
> + ret = drm_atomic_helper_check_crtcs(state);
> + if (ret)
> + return ret;
> +
> if (state->legacy_cursor_update)
> state->async_update = !drm_atomic_helper_async_check(dev, state);
>
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index bb14f488c8f6c..e5b820ce823bf 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> struct drm_mode_config *config = &dev->mode_config;
>
> if (degamma_lut_size) {
> + crtc->degamma_lut_size = degamma_lut_size;
> drm_object_attach_property(&crtc->base,
> config->degamma_lut_property, 0);
> drm_object_attach_property(&crtc->base,
> @@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> config->ctm_property, 0);
>
> if (gamma_lut_size) {
> + crtc->gamma_lut_size = gamma_lut_size;
> drm_object_attach_property(&crtc->base,
> config->gamma_lut_property, 0);
> drm_object_attach_property(&crtc->base,
> @@ -585,17 +587,17 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> EXPORT_SYMBOL(drm_plane_create_color_properties);
>
> /**
> - * drm_color_lut_check - check validity of lookup table
> + * drm_color_lut_channels_check - check validity of the channels in the lookup table
> * @lut: property blob containing LUT to check
> * @tests: bitmask of tests to run
> *
> - * Helper to check whether a userspace-provided lookup table is valid and
> - * satisfies hardware requirements. Drivers pass a bitmask indicating which of
> - * the tests in &drm_color_lut_tests should be performed.
> + * Helper to check whether each color channel of userspace-provided lookup table is valid and
> + * satisfies hardware requirements. Drivers pass a bitmask indicating which of in
> + * &drm_color_lut_channels_tests should be performed.
> *
> * Returns 0 on success, -EINVAL on failure.
> */
> -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
> +int drm_color_lut_channels_check(const struct drm_property_blob *lut, u32 tests)
> {
> const struct drm_color_lut *entry;
> int i;
> @@ -625,4 +627,4 @@ int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
>
> return 0;
> }
> -EXPORT_SYMBOL(drm_color_lut_check);
> +EXPORT_SYMBOL(drm_color_lut_channels_check);
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index dab892d2251ba..a308fe52746ac 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1285,7 +1285,7 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
> const struct drm_property_blob *gamma_lut = crtc_state->hw.gamma_lut;
> const struct drm_property_blob *degamma_lut = crtc_state->hw.degamma_lut;
> int gamma_length, degamma_length;
> - u32 gamma_tests, degamma_tests;
> + u32 gamma_channels_tests, degamma_channels_tests;
>
> /* Always allow legacy gamma LUT with no further checking. */
> if (crtc_state_is_legacy_gamma(crtc_state))
> @@ -1300,15 +1300,11 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
>
> degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> + degamma_channels_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> + gamma_channels_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
>
> - if (check_lut_size(degamma_lut, degamma_length) ||
> - check_lut_size(gamma_lut, gamma_length))
> - return -EINVAL;
Can you remove check_lut_size() now?
> -
> - if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> - drm_color_lut_check(gamma_lut, gamma_tests))
> + if (drm_color_lut_channels_check(degamma_lut, degamma_channels_tests) ||
> + drm_color_lut_channels_check(gamma_lut, gamma_channels_tests))
> return -EINVAL;
>
> return 0;
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 4045e2507e11c..a22d32a7a8719 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -38,6 +38,7 @@ struct drm_atomic_state;
> struct drm_private_obj;
> struct drm_private_state;
>
> +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state);
> int drm_atomic_helper_check_modeset(struct drm_device *dev,
> struct drm_atomic_state *state);
> int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 81c298488b0c8..cb1bf361ad3e3 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -94,12 +94,12 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> enum drm_color_range default_range);
>
> /**
> - * enum drm_color_lut_tests - hw-specific LUT tests to perform
> + * enum drm_color_lut_channels_tests - hw-specific LUT tests to perform
> *
> * The drm_color_lut_check() function takes a bitmask of the values here to
> * determine which tests to apply to a userspace-provided LUT.
> */
> -enum drm_color_lut_tests {
> +enum drm_color_lut_channels_tests {
> /**
> * @DRM_COLOR_LUT_EQUAL_CHANNELS:
> *
> @@ -119,5 +119,6 @@ enum drm_color_lut_tests {
> DRM_COLOR_LUT_NON_DECREASING = BIT(1),
> };
>
> -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests);
> +int drm_color_lut_channels_check(const struct drm_property_blob *lut,
> + u32 tests);
> #endif
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 2deb15d7e1610..cabd3ef1a6e32 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1072,6 +1072,17 @@ struct drm_crtc {
> /** @funcs: CRTC control functions */
> const struct drm_crtc_funcs *funcs;
>
> + /**
> + * @degamma_lut_size: Size of degamma LUT.
> + */
> + uint32_t degamma_lut_size;
> +
> + /**
> + * @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace such as
> + * X, which doesn't support large lut sizes.
> + */
> + uint32_t gamma_lut_size;
> +
> /**
> * @gamma_size: Size of legacy gamma ramp reported to userspace. Set up
> * by calling drm_mode_crtc_set_gamma_size().
> --
> 2.33.0.882.g93a45727a2-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Intel-gfx] [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
2021-10-26 1:26 ` [Intel-gfx] [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate Sean Paul
@ 2021-10-26 19:25 ` Mark Yacoub
0 siblings, 0 replies; 8+ messages in thread
From: Mark Yacoub @ 2021-10-26 19:25 UTC (permalink / raw)
To: Sean Paul
Cc: dri-devel, amd-gfx, intel-gfx, seanpaul, harry.wentland,
Mark Yacoub, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Matthias Brugger, linux-kernel, linux-arm-kernel,
linux-mediatek
On Mon, Oct 25, 2021 at 9:26 PM Sean Paul <sean@poorly.run> wrote:
>
> On Wed, Oct 13, 2021 at 02:12:20PM -0400, Mark Yacoub wrote:
> > From: Mark Yacoub <markyacoub@google.com>
> >
> > [Why]
> > 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma
> > or Degamma props in the new CRTC state, allowing any invalid size to
> > be passed on.
> > 2. Each driver has its own LUT size, which could also be different for
> > legacy users.
> >
> > [How]
> > 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes
> > assigned by the driver when it's initializing its color and CTM
> > management.
> > 2. Create drm_atomic_helper_check_crtc which is called by
> > drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that
> > they match the sizes in the new CRTC state.
> > 3. Rename older lut checks that test for the color channels to indicate
> > it's a channel check. It's not included in drm_atomic_helper_check_crtc
> > as it's hardware specific and is to be called by the driver.
> > 4. As the LUT size check now happens in drm_atomic_helper_check, remove
> > the lut check in intel_color.c
>
> I'd probably split the rename out from the crtc check since they're only
> tangentially related.
done.
>
> >
> > Fixes: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK
> > Tested on Zork(amdgpu) and Jacuzzi(mediatek), volteer(TGL)
> >
> > v1:
> > 1. Fix typos
> > 2. Remove the LUT size check from intel driver
> > 3. Rename old LUT check to indicate it's a channel change
> >
> > Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 60 ++++++++++++++++++++++
> > drivers/gpu/drm/drm_color_mgmt.c | 14 ++---
> > drivers/gpu/drm/i915/display/intel_color.c | 14 ++---
> > include/drm/drm_atomic_helper.h | 1 +
> > include/drm/drm_color_mgmt.h | 7 +--
> > include/drm/drm_crtc.h | 11 ++++
> > 6 files changed, 89 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index bc3487964fb5e..5feb2ad0209c3 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -929,6 +929,62 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_check_planes);
> >
> > +/**
> > + * drm_atomic_helper_check_crtcs - validate state object for CRTC changes
> > + * @state: the driver state object
> > + *
> > + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new
> > + * state holds them.
> > + *
> > + * RETURNS:
> > + * Zero for success or -errno
> > + */
> > +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state)
> > +{
> > + struct drm_crtc *crtc;
> > + struct drm_crtc_state *new_crtc_state;
> > + int i;
> > +
> > + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
> > + if (new_crtc_state->color_mgmt_changed &&
> > + new_crtc_state->gamma_lut) {
> > + uint64_t supported_lut_size = crtc->gamma_lut_size;
> > + uint32_t supported_legacy_lut_size = crtc->gamma_size;
> > + uint32_t new_state_lut_size =
> > + drm_color_lut_size(new_crtc_state->gamma_lut);
> > +
> > + if (new_state_lut_size != supported_lut_size &&
> > + new_state_lut_size != supported_legacy_lut_size) {
> > + drm_dbg_state(
> > + state->dev,
> > + "Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n",
> > + supported_lut_size,
> > + supported_legacy_lut_size,
> > + new_state_lut_size);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + if (new_crtc_state->color_mgmt_changed &&
> > + new_crtc_state->degamma_lut) {
> > + uint32_t new_state_lut_size =
> > + drm_color_lut_size(new_crtc_state->degamma_lut);
> > + uint64_t supported_lut_size = crtc->degamma_lut_size;
> > +
> > + if (new_state_lut_size != supported_lut_size) {
> > + drm_dbg_state(
> > + state->dev,
> > + "Invalid Degamma LUT size. Should be %u but got %u.\n",
> > + supported_lut_size, new_state_lut_size);
> > + return -EINVAL;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs);
> > +
> > /**
> > * drm_atomic_helper_check - validate state object
> > * @dev: DRM device
> > @@ -974,6 +1030,10 @@ int drm_atomic_helper_check(struct drm_device *dev,
> > if (ret)
> > return ret;
> >
> > + ret = drm_atomic_helper_check_crtcs(state);
> > + if (ret)
> > + return ret;
> > +
> > if (state->legacy_cursor_update)
> > state->async_update = !drm_atomic_helper_async_check(dev, state);
> >
> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > index bb14f488c8f6c..e5b820ce823bf 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> > struct drm_mode_config *config = &dev->mode_config;
> >
> > if (degamma_lut_size) {
> > + crtc->degamma_lut_size = degamma_lut_size;
> > drm_object_attach_property(&crtc->base,
> > config->degamma_lut_property, 0);
> > drm_object_attach_property(&crtc->base,
> > @@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> > config->ctm_property, 0);
> >
> > if (gamma_lut_size) {
> > + crtc->gamma_lut_size = gamma_lut_size;
> > drm_object_attach_property(&crtc->base,
> > config->gamma_lut_property, 0);
> > drm_object_attach_property(&crtc->base,
> > @@ -585,17 +587,17 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> > EXPORT_SYMBOL(drm_plane_create_color_properties);
> >
> > /**
> > - * drm_color_lut_check - check validity of lookup table
> > + * drm_color_lut_channels_check - check validity of the channels in the lookup table
> > * @lut: property blob containing LUT to check
> > * @tests: bitmask of tests to run
> > *
> > - * Helper to check whether a userspace-provided lookup table is valid and
> > - * satisfies hardware requirements. Drivers pass a bitmask indicating which of
> > - * the tests in &drm_color_lut_tests should be performed.
> > + * Helper to check whether each color channel of userspace-provided lookup table is valid and
> > + * satisfies hardware requirements. Drivers pass a bitmask indicating which of in
> > + * &drm_color_lut_channels_tests should be performed.
> > *
> > * Returns 0 on success, -EINVAL on failure.
> > */
> > -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
> > +int drm_color_lut_channels_check(const struct drm_property_blob *lut, u32 tests)
> > {
> > const struct drm_color_lut *entry;
> > int i;
> > @@ -625,4 +627,4 @@ int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
> >
> > return 0;
> > }
> > -EXPORT_SYMBOL(drm_color_lut_check);
> > +EXPORT_SYMBOL(drm_color_lut_channels_check);
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> > index dab892d2251ba..a308fe52746ac 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -1285,7 +1285,7 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
> > const struct drm_property_blob *gamma_lut = crtc_state->hw.gamma_lut;
> > const struct drm_property_blob *degamma_lut = crtc_state->hw.degamma_lut;
> > int gamma_length, degamma_length;
> > - u32 gamma_tests, degamma_tests;
> > + u32 gamma_channels_tests, degamma_channels_tests;
> >
> > /* Always allow legacy gamma LUT with no further checking. */
> > if (crtc_state_is_legacy_gamma(crtc_state))
> > @@ -1300,15 +1300,11 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
> >
> > degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > + degamma_channels_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > + gamma_channels_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> >
> > - if (check_lut_size(degamma_lut, degamma_length) ||
> > - check_lut_size(gamma_lut, gamma_length))
> > - return -EINVAL;
>
> Can you remove check_lut_size() now?
>
replace by drm_ check_lut_size
still gotta keep it cause you reach this both using set prop, not
necessarily through an atomic commit only.
> > -
> > - if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > - drm_color_lut_check(gamma_lut, gamma_tests))
> > + if (drm_color_lut_channels_check(degamma_lut, degamma_channels_tests) ||
> > + drm_color_lut_channels_check(gamma_lut, gamma_channels_tests))
> > return -EINVAL;
> >
> > return 0;
> > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > index 4045e2507e11c..a22d32a7a8719 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -38,6 +38,7 @@ struct drm_atomic_state;
> > struct drm_private_obj;
> > struct drm_private_state;
> >
> > +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state);
> > int drm_atomic_helper_check_modeset(struct drm_device *dev,
> > struct drm_atomic_state *state);
> > int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > index 81c298488b0c8..cb1bf361ad3e3 100644
> > --- a/include/drm/drm_color_mgmt.h
> > +++ b/include/drm/drm_color_mgmt.h
> > @@ -94,12 +94,12 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> > enum drm_color_range default_range);
> >
> > /**
> > - * enum drm_color_lut_tests - hw-specific LUT tests to perform
> > + * enum drm_color_lut_channels_tests - hw-specific LUT tests to perform
> > *
> > * The drm_color_lut_check() function takes a bitmask of the values here to
> > * determine which tests to apply to a userspace-provided LUT.
> > */
> > -enum drm_color_lut_tests {
> > +enum drm_color_lut_channels_tests {
> > /**
> > * @DRM_COLOR_LUT_EQUAL_CHANNELS:
> > *
> > @@ -119,5 +119,6 @@ enum drm_color_lut_tests {
> > DRM_COLOR_LUT_NON_DECREASING = BIT(1),
> > };
> >
> > -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests);
> > +int drm_color_lut_channels_check(const struct drm_property_blob *lut,
> > + u32 tests);
> > #endif
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 2deb15d7e1610..cabd3ef1a6e32 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1072,6 +1072,17 @@ struct drm_crtc {
> > /** @funcs: CRTC control functions */
> > const struct drm_crtc_funcs *funcs;
> >
> > + /**
> > + * @degamma_lut_size: Size of degamma LUT.
> > + */
> > + uint32_t degamma_lut_size;
> > +
> > + /**
> > + * @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace such as
> > + * X, which doesn't support large lut sizes.
> > + */
> > + uint32_t gamma_lut_size;
> > +
> > /**
> > * @gamma_size: Size of legacy gamma ramp reported to userspace. Set up
> > * by calling drm_mode_crtc_set_gamma_size().
> > --
> > 2.33.0.882.g93a45727a2-goog
> >
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
2021-10-13 18:12 ` [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate Mark Yacoub
2021-10-13 18:12 ` [PATCH 2/2] amd/amdgpu_dm: Verify Gamma and Degamma LUT sizes using DRM Core check Mark Yacoub
2021-10-26 1:26 ` [Intel-gfx] [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate Sean Paul
@ 2021-10-26 12:02 ` Paul Menzel
2021-10-26 19:24 ` Mark Yacoub
2 siblings, 1 reply; 8+ messages in thread
From: Paul Menzel @ 2021-10-26 12:02 UTC (permalink / raw)
To: Mark Yacoub
Cc: seanpaul, harry.wentland, Mark Yacoub, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Matthias Brugger,
linux-kernel, linux-arm-kernel, linux-mediatek, dri-devel,
amd-gfx, intel-gfx
Dear Mark,
Thank you for your patch.
On 13.10.21 20:12, Mark Yacoub wrote:
> From: Mark Yacoub <markyacoub@google.com>
>
> [Why]
> 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma
> or Degamma props in the new CRTC state, allowing any invalid size to
> be passed on.
> 2. Each driver has its own LUT size, which could also be different for
> legacy users.
How can the problem be reproduced?
> [How]
> 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes
> assigned by the driver when it's initializing its color and CTM
> management.
> 2. Create drm_atomic_helper_check_crtc which is called by
> drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that
> they match the sizes in the new CRTC state.
> 3. Rename older lut checks that test for the color channels to indicate
> it's a channel check. It's not included in drm_atomic_helper_check_crtc
> as it's hardware specific and is to be called by the driver.
> 4. As the LUT size check now happens in drm_atomic_helper_check, remove
> the lut check in intel_color.c
>
> Fixes: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK
If I am not mistaken, the Fixes tag is used for commits I believe. Maybe
use Resolves or something similar?
> Tested on Zork(amdgpu) and Jacuzzi(mediatek), volteer(TGL)
Please add a space before the (.
How did you test this?
> v1:
> 1. Fix typos
> 2. Remove the LUT size check from intel driver
> 3. Rename old LUT check to indicate it's a channel change
>
> Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 60 ++++++++++++++++++++++
> drivers/gpu/drm/drm_color_mgmt.c | 14 ++---
> drivers/gpu/drm/i915/display/intel_color.c | 14 ++---
> include/drm/drm_atomic_helper.h | 1 +
> include/drm/drm_color_mgmt.h | 7 +--
> include/drm/drm_crtc.h | 11 ++++
> 6 files changed, 89 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index bc3487964fb5e..5feb2ad0209c3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -929,6 +929,62 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_helper_check_planes);
>
> +/**
> + * drm_atomic_helper_check_crtcs - validate state object for CRTC changes
> + * @state: the driver state object
> + *
> + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new
> + * state holds them.
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state)
> +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *new_crtc_state;
> + int i;
> +
> + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
> + if (new_crtc_state->color_mgmt_changed &&
> + new_crtc_state->gamma_lut) {
> + uint64_t supported_lut_size = crtc->gamma_lut_size;
> + uint32_t supported_legacy_lut_size = crtc->gamma_size;
> + uint32_t new_state_lut_size =
> + drm_color_lut_size(new_crtc_state->gamma_lut);
> +
> + if (new_state_lut_size != supported_lut_size &&
> + new_state_lut_size != supported_legacy_lut_size) {
> + drm_dbg_state(
> + state->dev,
> + "Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n",
> + supported_lut_size,
> + supported_legacy_lut_size,
> + new_state_lut_size);
> + return -EINVAL;
> + }
> + }
> +
> + if (new_crtc_state->color_mgmt_changed &&
> + new_crtc_state->degamma_lut) {
> + uint32_t new_state_lut_size =
> + drm_color_lut_size(new_crtc_state->degamma_lut);
> + uint64_t supported_lut_size = crtc->degamma_lut_size;
> +
> + if (new_state_lut_size != supported_lut_size) {
> + drm_dbg_state(
> + state->dev,
> + "Invalid Degamma LUT size. Should be %u but got %u.\n",
> + supported_lut_size, new_state_lut_size);
> + return -EINVAL;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs);
> +
> /**
> * drm_atomic_helper_check - validate state object
> * @dev: DRM device
> @@ -974,6 +1030,10 @@ int drm_atomic_helper_check(struct drm_device *dev,
> if (ret)
> return ret;
>
> + ret = drm_atomic_helper_check_crtcs(state);
> + if (ret)
> + return ret;
> +
> if (state->legacy_cursor_update)
> state->async_update = !drm_atomic_helper_async_check(dev, state);
>
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index bb14f488c8f6c..e5b820ce823bf 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> struct drm_mode_config *config = &dev->mode_config;
>
> if (degamma_lut_size) {
> + crtc->degamma_lut_size = degamma_lut_size;
> drm_object_attach_property(&crtc->base,
> config->degamma_lut_property, 0);
> drm_object_attach_property(&crtc->base,
> @@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> config->ctm_property, 0);
>
> if (gamma_lut_size) {
> + crtc->gamma_lut_size = gamma_lut_size;
> drm_object_attach_property(&crtc->base,
> config->gamma_lut_property, 0);
> drm_object_attach_property(&crtc->base,
> @@ -585,17 +587,17 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> EXPORT_SYMBOL(drm_plane_create_color_properties);
>
> /**
> - * drm_color_lut_check - check validity of lookup table
> + * drm_color_lut_channels_check - check validity of the channels in the lookup table
> * @lut: property blob containing LUT to check
> * @tests: bitmask of tests to run
> *
> - * Helper to check whether a userspace-provided lookup table is valid and
> - * satisfies hardware requirements. Drivers pass a bitmask indicating which of
> - * the tests in &drm_color_lut_tests should be performed.
> + * Helper to check whether each color channel of userspace-provided lookup table is valid and
> + * satisfies hardware requirements. Drivers pass a bitmask indicating which of in
> + * &drm_color_lut_channels_tests should be performed.
> *
> * Returns 0 on success, -EINVAL on failure.
> */
> -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
> +int drm_color_lut_channels_check(const struct drm_property_blob *lut, u32 tests)
> {
> const struct drm_color_lut *entry;
> int i;
> @@ -625,4 +627,4 @@ int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
>
> return 0;
> }
> -EXPORT_SYMBOL(drm_color_lut_check);
> +EXPORT_SYMBOL(drm_color_lut_channels_check);
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index dab892d2251ba..a308fe52746ac 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1285,7 +1285,7 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
> const struct drm_property_blob *gamma_lut = crtc_state->hw.gamma_lut;
> const struct drm_property_blob *degamma_lut = crtc_state->hw.degamma_lut;
> int gamma_length, degamma_length;
> - u32 gamma_tests, degamma_tests;
> + u32 gamma_channels_tests, degamma_channels_tests;
>
> /* Always allow legacy gamma LUT with no further checking. */
> if (crtc_state_is_legacy_gamma(crtc_state))
> @@ -1300,15 +1300,11 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
>
> degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> + degamma_channels_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> + gamma_channels_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
>
> - if (check_lut_size(degamma_lut, degamma_length) ||
> - check_lut_size(gamma_lut, gamma_length))
> - return -EINVAL;
> -
> - if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> - drm_color_lut_check(gamma_lut, gamma_tests))
> + if (drm_color_lut_channels_check(degamma_lut, degamma_channels_tests) ||
> + drm_color_lut_channels_check(gamma_lut, gamma_channels_tests))
> return -EINVAL;
>
> return 0;
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 4045e2507e11c..a22d32a7a8719 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -38,6 +38,7 @@ struct drm_atomic_state;
> struct drm_private_obj;
> struct drm_private_state;
>
> +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state);
> int drm_atomic_helper_check_modeset(struct drm_device *dev,
> struct drm_atomic_state *state);
> int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 81c298488b0c8..cb1bf361ad3e3 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -94,12 +94,12 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> enum drm_color_range default_range);
>
> /**
> - * enum drm_color_lut_tests - hw-specific LUT tests to perform
> + * enum drm_color_lut_channels_tests - hw-specific LUT tests to perform
> *
> * The drm_color_lut_check() function takes a bitmask of the values here to
> * determine which tests to apply to a userspace-provided LUT.
> */
> -enum drm_color_lut_tests {
> +enum drm_color_lut_channels_tests {
> /**
> * @DRM_COLOR_LUT_EQUAL_CHANNELS:
> *
> @@ -119,5 +119,6 @@ enum drm_color_lut_tests {
> DRM_COLOR_LUT_NON_DECREASING = BIT(1),
> };
>
> -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests);
> +int drm_color_lut_channels_check(const struct drm_property_blob *lut,
> + u32 tests);
> #endif
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 2deb15d7e1610..cabd3ef1a6e32 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1072,6 +1072,17 @@ struct drm_crtc {
> /** @funcs: CRTC control functions */
> const struct drm_crtc_funcs *funcs;
>
> + /**
> + * @degamma_lut_size: Size of degamma LUT.
> + */
> + uint32_t degamma_lut_size;
> +
> + /**
> + * @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace such as
> + * X, which doesn't support large lut sizes.
> + */
> + uint32_t gamma_lut_size;
> +
> /**
> * @gamma_size: Size of legacy gamma ramp reported to userspace. Set up
> * by calling drm_mode_crtc_set_gamma_size().
>
Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>
Kind regards,
Paul
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
2021-10-26 12:02 ` Paul Menzel
@ 2021-10-26 19:24 ` Mark Yacoub
2021-10-26 19:26 ` Mark Yacoub
0 siblings, 1 reply; 8+ messages in thread
From: Mark Yacoub @ 2021-10-26 19:24 UTC (permalink / raw)
To: Paul Menzel
Cc: seanpaul, harry.wentland, Mark Yacoub, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Matthias Brugger,
linux-kernel, linux-arm-kernel, linux-mediatek, dri-devel,
amd-gfx, intel-gfx
On Tue, Oct 26, 2021 at 8:02 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Mark,
>
>
> Thank you for your patch.
>
> On 13.10.21 20:12, Mark Yacoub wrote:
> > From: Mark Yacoub <markyacoub@google.com>
> >
> > [Why]
> > 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma
> > or Degamma props in the new CRTC state, allowing any invalid size to
> > be passed on.
> > 2. Each driver has its own LUT size, which could also be different for
> > legacy users.
>
> How can the problem be reproduced?
It was caught using igt@kms_color@pipe-A-invalid-gamma-lut-sizes.
it validates that drivers will only LUTs of their expected size not
any random (smaller or larger) number.
>
> > [How]
> > 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes
> > assigned by the driver when it's initializing its color and CTM
> > management.
> > 2. Create drm_atomic_helper_check_crtc which is called by
> > drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that
> > they match the sizes in the new CRTC state.
> > 3. Rename older lut checks that test for the color channels to indicate
> > it's a channel check. It's not included in drm_atomic_helper_check_crtc
> > as it's hardware specific and is to be called by the driver.
> > 4. As the LUT size check now happens in drm_atomic_helper_check, remove
> > the lut check in intel_color.c
> >
> > Fixes: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK
>
> If I am not mistaken, the Fixes tag is used for commits I believe. Maybe
> use Resolves or something similar?
fixed!
>
> > Tested on Zork(amdgpu) and Jacuzzi(mediatek), volteer(TGL)
>
> Please add a space before the (.
>
> How did you test this?
smoke test on both MTK and TGL devices along with running
igt@kms_color on both devices.
>
> > v1:
> > 1. Fix typos
> > 2. Remove the LUT size check from intel driver
> > 3. Rename old LUT check to indicate it's a channel change
> >
> > Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 60 ++++++++++++++++++++++
> > drivers/gpu/drm/drm_color_mgmt.c | 14 ++---
> > drivers/gpu/drm/i915/display/intel_color.c | 14 ++---
> > include/drm/drm_atomic_helper.h | 1 +
> > include/drm/drm_color_mgmt.h | 7 +--
> > include/drm/drm_crtc.h | 11 ++++
> > 6 files changed, 89 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index bc3487964fb5e..5feb2ad0209c3 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -929,6 +929,62 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_check_planes);
> >
> > +/**
> > + * drm_atomic_helper_check_crtcs - validate state object for CRTC changes
> > + * @state: the driver state object
> > + *
> > + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new
> > + * state holds them.
> > + *
> > + * RETURNS:
> > + * Zero for success or -errno
> > + */
> > +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state)
> > +{
> > + struct drm_crtc *crtc;
> > + struct drm_crtc_state *new_crtc_state;
> > + int i;
> > +
> > + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
> > + if (new_crtc_state->color_mgmt_changed &&
> > + new_crtc_state->gamma_lut) {
> > + uint64_t supported_lut_size = crtc->gamma_lut_size;
> > + uint32_t supported_legacy_lut_size = crtc->gamma_size;
> > + uint32_t new_state_lut_size =
> > + drm_color_lut_size(new_crtc_state->gamma_lut);
> > +
> > + if (new_state_lut_size != supported_lut_size &&
> > + new_state_lut_size != supported_legacy_lut_size) {
> > + drm_dbg_state(
> > + state->dev,
> > + "Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n",
> > + supported_lut_size,
> > + supported_legacy_lut_size,
> > + new_state_lut_size);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + if (new_crtc_state->color_mgmt_changed &&
> > + new_crtc_state->degamma_lut) {
> > + uint32_t new_state_lut_size =
> > + drm_color_lut_size(new_crtc_state->degamma_lut);
> > + uint64_t supported_lut_size = crtc->degamma_lut_size;
> > +
> > + if (new_state_lut_size != supported_lut_size) {
> > + drm_dbg_state(
> > + state->dev,
> > + "Invalid Degamma LUT size. Should be %u but got %u.\n",
> > + supported_lut_size, new_state_lut_size);
> > + return -EINVAL;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs);
> > +
> > /**
> > * drm_atomic_helper_check - validate state object
> > * @dev: DRM device
> > @@ -974,6 +1030,10 @@ int drm_atomic_helper_check(struct drm_device *dev,
> > if (ret)
> > return ret;
> >
> > + ret = drm_atomic_helper_check_crtcs(state);
> > + if (ret)
> > + return ret;
> > +
> > if (state->legacy_cursor_update)
> > state->async_update = !drm_atomic_helper_async_check(dev, state);
> >
> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > index bb14f488c8f6c..e5b820ce823bf 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> > struct drm_mode_config *config = &dev->mode_config;
> >
> > if (degamma_lut_size) {
> > + crtc->degamma_lut_size = degamma_lut_size;
> > drm_object_attach_property(&crtc->base,
> > config->degamma_lut_property, 0);
> > drm_object_attach_property(&crtc->base,
> > @@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> > config->ctm_property, 0);
> >
> > if (gamma_lut_size) {
> > + crtc->gamma_lut_size = gamma_lut_size;
> > drm_object_attach_property(&crtc->base,
> > config->gamma_lut_property, 0);
> > drm_object_attach_property(&crtc->base,
> > @@ -585,17 +587,17 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> > EXPORT_SYMBOL(drm_plane_create_color_properties);
> >
> > /**
> > - * drm_color_lut_check - check validity of lookup table
> > + * drm_color_lut_channels_check - check validity of the channels in the lookup table
> > * @lut: property blob containing LUT to check
> > * @tests: bitmask of tests to run
> > *
> > - * Helper to check whether a userspace-provided lookup table is valid and
> > - * satisfies hardware requirements. Drivers pass a bitmask indicating which of
> > - * the tests in &drm_color_lut_tests should be performed.
> > + * Helper to check whether each color channel of userspace-provided lookup table is valid and
> > + * satisfies hardware requirements. Drivers pass a bitmask indicating which of in
> > + * &drm_color_lut_channels_tests should be performed.
> > *
> > * Returns 0 on success, -EINVAL on failure.
> > */
> > -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
> > +int drm_color_lut_channels_check(const struct drm_property_blob *lut, u32 tests)
> > {
> > const struct drm_color_lut *entry;
> > int i;
> > @@ -625,4 +627,4 @@ int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
> >
> > return 0;
> > }
> > -EXPORT_SYMBOL(drm_color_lut_check);
> > +EXPORT_SYMBOL(drm_color_lut_channels_check);
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> > index dab892d2251ba..a308fe52746ac 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -1285,7 +1285,7 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
> > const struct drm_property_blob *gamma_lut = crtc_state->hw.gamma_lut;
> > const struct drm_property_blob *degamma_lut = crtc_state->hw.degamma_lut;
> > int gamma_length, degamma_length;
> > - u32 gamma_tests, degamma_tests;
> > + u32 gamma_channels_tests, degamma_channels_tests;
> >
> > /* Always allow legacy gamma LUT with no further checking. */
> > if (crtc_state_is_legacy_gamma(crtc_state))
> > @@ -1300,15 +1300,11 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
> >
> > degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > + degamma_channels_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > + gamma_channels_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> >
> > - if (check_lut_size(degamma_lut, degamma_length) ||
> > - check_lut_size(gamma_lut, gamma_length))
> > - return -EINVAL;
> > -
> > - if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > - drm_color_lut_check(gamma_lut, gamma_tests))
> > + if (drm_color_lut_channels_check(degamma_lut, degamma_channels_tests) ||
> > + drm_color_lut_channels_check(gamma_lut, gamma_channels_tests))
> > return -EINVAL;
> >
> > return 0;
> > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > index 4045e2507e11c..a22d32a7a8719 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -38,6 +38,7 @@ struct drm_atomic_state;
> > struct drm_private_obj;
> > struct drm_private_state;
> >
> > +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state);
> > int drm_atomic_helper_check_modeset(struct drm_device *dev,
> > struct drm_atomic_state *state);
> > int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > index 81c298488b0c8..cb1bf361ad3e3 100644
> > --- a/include/drm/drm_color_mgmt.h
> > +++ b/include/drm/drm_color_mgmt.h
> > @@ -94,12 +94,12 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> > enum drm_color_range default_range);
> >
> > /**
> > - * enum drm_color_lut_tests - hw-specific LUT tests to perform
> > + * enum drm_color_lut_channels_tests - hw-specific LUT tests to perform
> > *
> > * The drm_color_lut_check() function takes a bitmask of the values here to
> > * determine which tests to apply to a userspace-provided LUT.
> > */
> > -enum drm_color_lut_tests {
> > +enum drm_color_lut_channels_tests {
> > /**
> > * @DRM_COLOR_LUT_EQUAL_CHANNELS:
> > *
> > @@ -119,5 +119,6 @@ enum drm_color_lut_tests {
> > DRM_COLOR_LUT_NON_DECREASING = BIT(1),
> > };
> >
> > -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests);
> > +int drm_color_lut_channels_check(const struct drm_property_blob *lut,
> > + u32 tests);
> > #endif
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 2deb15d7e1610..cabd3ef1a6e32 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1072,6 +1072,17 @@ struct drm_crtc {
> > /** @funcs: CRTC control functions */
> > const struct drm_crtc_funcs *funcs;
> >
> > + /**
> > + * @degamma_lut_size: Size of degamma LUT.
> > + */
> > + uint32_t degamma_lut_size;
> > +
> > + /**
> > + * @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace such as
> > + * X, which doesn't support large lut sizes.
> > + */
> > + uint32_t gamma_lut_size;
> > +
> > /**
> > * @gamma_size: Size of legacy gamma ramp reported to userspace. Set up
> > * by calling drm_mode_crtc_set_gamma_size().
> >
>
> Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>
>
>
> Kind regards,
>
> Paul
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
2021-10-26 19:24 ` Mark Yacoub
@ 2021-10-26 19:26 ` Mark Yacoub
0 siblings, 0 replies; 8+ messages in thread
From: Mark Yacoub @ 2021-10-26 19:26 UTC (permalink / raw)
To: Paul Menzel
Cc: seanpaul, harry.wentland, Mark Yacoub, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Matthias Brugger,
linux-kernel, linux-arm-kernel, linux-mediatek, dri-devel,
amd-gfx, intel-gfx
new patch: https://patchwork.freedesktop.org/series/96314/
On Tue, Oct 26, 2021 at 3:24 PM Mark Yacoub <markyacoub@chromium.org> wrote:
>
> On Tue, Oct 26, 2021 at 8:02 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> >
> > Dear Mark,
> >
> >
> > Thank you for your patch.
> >
> > On 13.10.21 20:12, Mark Yacoub wrote:
> > > From: Mark Yacoub <markyacoub@google.com>
> > >
> > > [Why]
> > > 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma
> > > or Degamma props in the new CRTC state, allowing any invalid size to
> > > be passed on.
> > > 2. Each driver has its own LUT size, which could also be different for
> > > legacy users.
> >
> > How can the problem be reproduced?
> It was caught using igt@kms_color@pipe-A-invalid-gamma-lut-sizes.
> it validates that drivers will only LUTs of their expected size not
> any random (smaller or larger) number.
> >
> > > [How]
> > > 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes
> > > assigned by the driver when it's initializing its color and CTM
> > > management.
> > > 2. Create drm_atomic_helper_check_crtc which is called by
> > > drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that
> > > they match the sizes in the new CRTC state.
> > > 3. Rename older lut checks that test for the color channels to indicate
> > > it's a channel check. It's not included in drm_atomic_helper_check_crtc
> > > as it's hardware specific and is to be called by the driver.
> > > 4. As the LUT size check now happens in drm_atomic_helper_check, remove
> > > the lut check in intel_color.c
> > >
> > > Fixes: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK
> >
> > If I am not mistaken, the Fixes tag is used for commits I believe. Maybe
> > use Resolves or something similar?
> fixed!
> >
> > > Tested on Zork(amdgpu) and Jacuzzi(mediatek), volteer(TGL)
> >
> > Please add a space before the (.
my apologies i missed this. I'll update it if another version is
needed or if the committer can do it for me when they apply it.
> >
> > How did you test this?
> smoke test on both MTK and TGL devices along with running
> igt@kms_color on both devices.
> >
> > > v1:
> > > 1. Fix typos
> > > 2. Remove the LUT size check from intel driver
> > > 3. Rename old LUT check to indicate it's a channel change
> > >
> > > Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> > > ---
> > > drivers/gpu/drm/drm_atomic_helper.c | 60 ++++++++++++++++++++++
> > > drivers/gpu/drm/drm_color_mgmt.c | 14 ++---
> > > drivers/gpu/drm/i915/display/intel_color.c | 14 ++---
> > > include/drm/drm_atomic_helper.h | 1 +
> > > include/drm/drm_color_mgmt.h | 7 +--
> > > include/drm/drm_crtc.h | 11 ++++
> > > 6 files changed, 89 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index bc3487964fb5e..5feb2ad0209c3 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -929,6 +929,62 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> > > }
> > > EXPORT_SYMBOL(drm_atomic_helper_check_planes);
> > >
> > > +/**
> > > + * drm_atomic_helper_check_crtcs - validate state object for CRTC changes
> > > + * @state: the driver state object
> > > + *
> > > + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new
> > > + * state holds them.
> > > + *
> > > + * RETURNS:
> > > + * Zero for success or -errno
> > > + */
> > > +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state)
> > > +{
> > > + struct drm_crtc *crtc;
> > > + struct drm_crtc_state *new_crtc_state;
> > > + int i;
> > > +
> > > + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
> > > + if (new_crtc_state->color_mgmt_changed &&
> > > + new_crtc_state->gamma_lut) {
> > > + uint64_t supported_lut_size = crtc->gamma_lut_size;
> > > + uint32_t supported_legacy_lut_size = crtc->gamma_size;
> > > + uint32_t new_state_lut_size =
> > > + drm_color_lut_size(new_crtc_state->gamma_lut);
> > > +
> > > + if (new_state_lut_size != supported_lut_size &&
> > > + new_state_lut_size != supported_legacy_lut_size) {
> > > + drm_dbg_state(
> > > + state->dev,
> > > + "Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n",
> > > + supported_lut_size,
> > > + supported_legacy_lut_size,
> > > + new_state_lut_size);
> > > + return -EINVAL;
> > > + }
> > > + }
> > > +
> > > + if (new_crtc_state->color_mgmt_changed &&
> > > + new_crtc_state->degamma_lut) {
> > > + uint32_t new_state_lut_size =
> > > + drm_color_lut_size(new_crtc_state->degamma_lut);
> > > + uint64_t supported_lut_size = crtc->degamma_lut_size;
> > > +
> > > + if (new_state_lut_size != supported_lut_size) {
> > > + drm_dbg_state(
> > > + state->dev,
> > > + "Invalid Degamma LUT size. Should be %u but got %u.\n",
> > > + supported_lut_size, new_state_lut_size);
> > > + return -EINVAL;
> > > + }
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs);
> > > +
> > > /**
> > > * drm_atomic_helper_check - validate state object
> > > * @dev: DRM device
> > > @@ -974,6 +1030,10 @@ int drm_atomic_helper_check(struct drm_device *dev,
> > > if (ret)
> > > return ret;
> > >
> > > + ret = drm_atomic_helper_check_crtcs(state);
> > > + if (ret)
> > > + return ret;
> > > +
> > > if (state->legacy_cursor_update)
> > > state->async_update = !drm_atomic_helper_async_check(dev, state);
> > >
> > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > > index bb14f488c8f6c..e5b820ce823bf 100644
> > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> > > struct drm_mode_config *config = &dev->mode_config;
> > >
> > > if (degamma_lut_size) {
> > > + crtc->degamma_lut_size = degamma_lut_size;
> > > drm_object_attach_property(&crtc->base,
> > > config->degamma_lut_property, 0);
> > > drm_object_attach_property(&crtc->base,
> > > @@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> > > config->ctm_property, 0);
> > >
> > > if (gamma_lut_size) {
> > > + crtc->gamma_lut_size = gamma_lut_size;
> > > drm_object_attach_property(&crtc->base,
> > > config->gamma_lut_property, 0);
> > > drm_object_attach_property(&crtc->base,
> > > @@ -585,17 +587,17 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> > > EXPORT_SYMBOL(drm_plane_create_color_properties);
> > >
> > > /**
> > > - * drm_color_lut_check - check validity of lookup table
> > > + * drm_color_lut_channels_check - check validity of the channels in the lookup table
> > > * @lut: property blob containing LUT to check
> > > * @tests: bitmask of tests to run
> > > *
> > > - * Helper to check whether a userspace-provided lookup table is valid and
> > > - * satisfies hardware requirements. Drivers pass a bitmask indicating which of
> > > - * the tests in &drm_color_lut_tests should be performed.
> > > + * Helper to check whether each color channel of userspace-provided lookup table is valid and
> > > + * satisfies hardware requirements. Drivers pass a bitmask indicating which of in
> > > + * &drm_color_lut_channels_tests should be performed.
> > > *
> > > * Returns 0 on success, -EINVAL on failure.
> > > */
> > > -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
> > > +int drm_color_lut_channels_check(const struct drm_property_blob *lut, u32 tests)
> > > {
> > > const struct drm_color_lut *entry;
> > > int i;
> > > @@ -625,4 +627,4 @@ int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
> > >
> > > return 0;
> > > }
> > > -EXPORT_SYMBOL(drm_color_lut_check);
> > > +EXPORT_SYMBOL(drm_color_lut_channels_check);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> > > index dab892d2251ba..a308fe52746ac 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > > @@ -1285,7 +1285,7 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
> > > const struct drm_property_blob *gamma_lut = crtc_state->hw.gamma_lut;
> > > const struct drm_property_blob *degamma_lut = crtc_state->hw.degamma_lut;
> > > int gamma_length, degamma_length;
> > > - u32 gamma_tests, degamma_tests;
> > > + u32 gamma_channels_tests, degamma_channels_tests;
> > >
> > > /* Always allow legacy gamma LUT with no further checking. */
> > > if (crtc_state_is_legacy_gamma(crtc_state))
> > > @@ -1300,15 +1300,11 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
> > >
> > > degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > > gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > > - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > > - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > > + degamma_channels_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > > + gamma_channels_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > >
> > > - if (check_lut_size(degamma_lut, degamma_length) ||
> > > - check_lut_size(gamma_lut, gamma_length))
> > > - return -EINVAL;
> > > -
> > > - if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > > - drm_color_lut_check(gamma_lut, gamma_tests))
> > > + if (drm_color_lut_channels_check(degamma_lut, degamma_channels_tests) ||
> > > + drm_color_lut_channels_check(gamma_lut, gamma_channels_tests))
> > > return -EINVAL;
> > >
> > > return 0;
> > > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > > index 4045e2507e11c..a22d32a7a8719 100644
> > > --- a/include/drm/drm_atomic_helper.h
> > > +++ b/include/drm/drm_atomic_helper.h
> > > @@ -38,6 +38,7 @@ struct drm_atomic_state;
> > > struct drm_private_obj;
> > > struct drm_private_state;
> > >
> > > +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state);
> > > int drm_atomic_helper_check_modeset(struct drm_device *dev,
> > > struct drm_atomic_state *state);
> > > int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > > index 81c298488b0c8..cb1bf361ad3e3 100644
> > > --- a/include/drm/drm_color_mgmt.h
> > > +++ b/include/drm/drm_color_mgmt.h
> > > @@ -94,12 +94,12 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> > > enum drm_color_range default_range);
> > >
> > > /**
> > > - * enum drm_color_lut_tests - hw-specific LUT tests to perform
> > > + * enum drm_color_lut_channels_tests - hw-specific LUT tests to perform
> > > *
> > > * The drm_color_lut_check() function takes a bitmask of the values here to
> > > * determine which tests to apply to a userspace-provided LUT.
> > > */
> > > -enum drm_color_lut_tests {
> > > +enum drm_color_lut_channels_tests {
> > > /**
> > > * @DRM_COLOR_LUT_EQUAL_CHANNELS:
> > > *
> > > @@ -119,5 +119,6 @@ enum drm_color_lut_tests {
> > > DRM_COLOR_LUT_NON_DECREASING = BIT(1),
> > > };
> > >
> > > -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests);
> > > +int drm_color_lut_channels_check(const struct drm_property_blob *lut,
> > > + u32 tests);
> > > #endif
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index 2deb15d7e1610..cabd3ef1a6e32 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -1072,6 +1072,17 @@ struct drm_crtc {
> > > /** @funcs: CRTC control functions */
> > > const struct drm_crtc_funcs *funcs;
> > >
> > > + /**
> > > + * @degamma_lut_size: Size of degamma LUT.
> > > + */
> > > + uint32_t degamma_lut_size;
> > > +
> > > + /**
> > > + * @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace such as
> > > + * X, which doesn't support large lut sizes.
> > > + */
> > > + uint32_t gamma_lut_size;
> > > +
> > > /**
> > > * @gamma_size: Size of legacy gamma ramp reported to userspace. Set up
> > > * by calling drm_mode_crtc_set_gamma_size().
> > >
> >
> > Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>
> >
> >
> > Kind regards,
> >
> > Paul
^ permalink raw reply [flat|nested] 8+ messages in thread