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.
Fixes: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK Tested on Zork(amdgpu) and Jacuzzi(mediatek)
Signed-off-by: Mark Yacoubmarkyacoub@chromium.org --- drivers/gpu/drm/drm_atomic_helper.c | 56 +++++++++++++++++++++++++++++ drivers/gpu/drm/drm_color_mgmt.c | 2 ++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 11 ++++++ 4 files changed, 70 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 2c0c6ec928200..265b9747250d1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -930,6 +930,58 @@ drm_atomic_helper_check_planes(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_helper_check_planes);
+/** + * drm_atomic_helper_check_planes - validate state object for CRTC changes + * @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_crtc(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->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_DEBUG_DRIVER( + "Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n", + supported_lut_size, + supported_legacy_lut_size, + new_state_lut_size); + return -EINVAL; + } + } + + if (new_crtc_state->degamma_lut) { + uint32_t new_state_lut_size = + drm_color_lut_size(new_crtc_state->degamma_lut); + uint64_t supported_lut_size = crtc->degamma_lut_size; + + if (new_state_lut_size != supported_lut_size) { + DRM_DEBUG_DRIVER( + "Invalid Degamma LUT size. Should be %u but got %u.\n", + supported_lut_size, new_state_lut_size); + return -EINVAL; + } + } + } + + return 0; +} +EXPORT_SYMBOL(drm_atomic_helper_check_crtc); + /** * drm_atomic_helper_check - validate state object * @dev: DRM device @@ -975,6 +1027,10 @@ int drm_atomic_helper_check(struct drm_device *dev, if (ret) return ret;
+ ret = drm_atomic_helper_check_crtc(state); + if (ret) + return ret; + if (state->legacy_cursor_update) state->async_update = !drm_atomic_helper_async_check(dev, state);
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index bb14f488c8f6c..72a1b628e7cdd 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, struct drm_mode_config *config = &dev->mode_config;
if (degamma_lut_size) { + crtc->degamma_lut_size = degamma_lut_size; drm_object_attach_property(&crtc->base, config->degamma_lut_property, 0); drm_object_attach_property(&crtc->base, @@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, config->ctm_property, 0);
if (gamma_lut_size) { + crtc->gamma_lut_size = gamma_lut_size; drm_object_attach_property(&crtc->base, config->gamma_lut_property, 0); drm_object_attach_property(&crtc->base, diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 4045e2507e11c..3eda13622ca1e 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -38,6 +38,7 @@ struct drm_atomic_state; struct drm_private_obj; struct drm_private_state;
+int drm_atomic_helper_check_crtc(struct drm_atomic_state *state); int drm_atomic_helper_check_modeset(struct drm_device *dev, struct drm_atomic_state *state); int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 13eeba2a750af..c602be2cafca9 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1072,6 +1072,17 @@ struct drm_crtc { /** @funcs: CRTC control functions */ const struct drm_crtc_funcs *funcs;
+ /** + * @degamma_lut_size: Size of degamma LUT. + */ + uint32_t degamma_lut_size; + + /** + * @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace such as + * X, which doesn't support large lut sizes. + */ + uint32_t gamma_lut_size; + /** * @gamma_size: Size of legacy gamma ramp reported to userspace. Set up * by calling drm_mode_crtc_set_gamma_size().
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.
Signed-off-by: Mark Yacoub markyacoub@chromium.org --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 07adac1a8c42b..96a1d006b777e 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10683,6 +10683,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, } } #endif + ret = drm_atomic_helper_check_crtc(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);
@@ -10692,10 +10696,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;
On Wed, Sep 29, 2021 at 03:39:26PM -0400, Mark Yacoub 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.
Signed-off-by: Mark Yacoub markyacoub@chromium.org
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 07adac1a8c42b..96a1d006b777e 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10683,6 +10683,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, } } #endif
- ret = drm_atomic_helper_check_crtc(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);
@@ -10692,10 +10696,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);
From a quick glance, I think you can now delete this function. It's called from
amdgpu_dm_update_crtc_color_mgmt() which is part of the commit, so the lut sizes should have already been checked.
If the call from amdgpu_dm_update_crtc_color_mgmt() is not possible to remove, you could replace it with a call to the new helper function. And if _that_ is not possible, please make amdgpu_dm_verify_lut_sizes() static :-)
Sean
if (ret)goto fail;- if (!new_crtc_state->enable) continue;
-- 2.33.0.685.g46640cef36-goog
On 2021-10-01 15:56, Sean Paul wrote:
On Wed, Sep 29, 2021 at 03:39:26PM -0400, Mark Yacoub 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.
Signed-off-by: Mark Yacoub markyacoub@chromium.org
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 07adac1a8c42b..96a1d006b777e 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10683,6 +10683,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, } } #endif
- ret = drm_atomic_helper_check_crtc(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);
@@ -10692,10 +10696,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);From a quick glance, I think you can now delete this function. It's called from amdgpu_dm_update_crtc_color_mgmt() which is part of the commit, so the lut sizes should have already been checked.
I agree. Would be nice if we could drop the extra call in the CM function (after confirming that it isn't needed).
Harry
If the call from amdgpu_dm_update_crtc_color_mgmt() is not possible to remove, you could replace it with a call to the new helper function. And if _that_ is not possible, please make amdgpu_dm_verify_lut_sizes() static :-)
Sean
if (ret)goto fail;- if (!new_crtc_state->enable) continue;
-- 2.33.0.685.g46640cef36-goog
On Wed, Sep 29, 2021 at 03:39:25PM -0400, Mark Yacoub wrote:
From: Mark Yacoub markyacoub@google.com
[Why]
- 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]
- Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes
assigned by the driver when it's initializing its color and CTM management. 2. Create drm_atomic_helper_check_crtc which is called by drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that they match the sizes in the new CRTC state.
Did you consider extending drm_color_lut_check() with the size checks?
Fixes: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK Tested on Zork(amdgpu) and Jacuzzi(mediatek)
Signed-off-by: Mark Yacoubmarkyacoub@chromium.org
nit: missing a space between name and email
drivers/gpu/drm/drm_atomic_helper.c | 56 +++++++++++++++++++++++++++++ drivers/gpu/drm/drm_color_mgmt.c | 2 ++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 11 ++++++ 4 files changed, 70 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 2c0c6ec928200..265b9747250d1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -930,6 +930,58 @@ drm_atomic_helper_check_planes(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_helper_check_planes);
+/**
- drm_atomic_helper_check_planes - validate state object for CRTC changes
Ctrl+c/Ctrl+v error here
- @state: the driver state object
- Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new
Are there missing words between "object" and "such"?
- state holds them.
- RETURNS:
- Zero for success or -errno
- */
+int drm_atomic_helper_check_crtc(struct drm_atomic_state *state)
drm_atomic_helper_check_crtcs to be consistent with drm_atomic_helper_check_planes
+{
- struct drm_crtc *crtc;
- struct drm_crtc_state *new_crtc_state;
- int i;
- for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
no space before (
if (new_crtc_state->gamma_lut) {
Perhaps gate these with a check of state->color_mgmt_changed first?
uint64_t supported_lut_size = crtc->gamma_lut_size;uint32_t supported_legacy_lut_size = crtc->gamma_size;uint32_t new_state_lut_size =drm_color_lut_size(new_crtc_state->gamma_lut);
nit: new_state_lut_size and supported_lut_size can be pulled out to top level scope to avoid re-instantiation on each iteration
if (new_state_lut_size != supported_lut_size &&new_state_lut_size != supported_legacy_lut_size) {
According to the docbook, "If drivers support multiple LUT sizes then they should publish the largest size, and sub-sample smaller sized LUTs". So should this check be > instead of != ?
DRM_DEBUG_DRIVER(
drm_dbg_state() is probably more appropriate
"Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n",supported_lut_size,supported_legacy_lut_size,new_state_lut_size);return -EINVAL;}}if (new_crtc_state->degamma_lut) {uint32_t new_state_lut_size =drm_color_lut_size(new_crtc_state->degamma_lut);uint64_t supported_lut_size = crtc->degamma_lut_size;if (new_state_lut_size != supported_lut_size) {DRM_DEBUG_DRIVER(
drm_dbg_state()
"Invalid Degamma LUT size. Should be %u but got %u.\n",supported_lut_size, new_state_lut_size);return -EINVAL;}}- }
- return 0;
+} +EXPORT_SYMBOL(drm_atomic_helper_check_crtc);
/**
- drm_atomic_helper_check - validate state object
- @dev: DRM device
@@ -975,6 +1027,10 @@ int drm_atomic_helper_check(struct drm_device *dev, if (ret) return ret;
- ret = drm_atomic_helper_check_crtc(state);
- if (ret)
return ret;- if (state->legacy_cursor_update) state->async_update = !drm_atomic_helper_async_check(dev, state);
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index bb14f488c8f6c..72a1b628e7cdd 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, struct drm_mode_config *config = &dev->mode_config;
if (degamma_lut_size) {
crtc->degamma_lut_size = degamma_lut_size;@@ -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;diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 4045e2507e11c..3eda13622ca1e 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -38,6 +38,7 @@ struct drm_atomic_state; struct drm_private_obj; struct drm_private_state;
+int drm_atomic_helper_check_crtc(struct drm_atomic_state *state); int drm_atomic_helper_check_modeset(struct drm_device *dev, struct drm_atomic_state *state); int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 13eeba2a750af..c602be2cafca9 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1072,6 +1072,17 @@ struct drm_crtc { /** @funcs: CRTC control functions */ const struct drm_crtc_funcs *funcs;
- /**
* @degamma_lut_size: Size of degamma LUT.*/- uint32_t degamma_lut_size;
- /**
* @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace such as* X, which doesn't support large lut sizes.*/- uint32_t gamma_lut_size;
Above, you're checking
if (new_state_lut_size != gamma_size && new_state_lut_size != gamma_lut_size) fail;
doesn't that imply that gamma_size and gamma_lut_size must always be equal? If so, perhaps turf this new state and rename degamma_lut_size to degamma_size to be consistent.
De-duping this and initializing crtc->gamma_size in the initialization would mean the if (crtc->gamma_size) check in drm_crtc_supports_legacy_check() is no longer useful (and possibly other similar checks), so some care will need to be taken to avoid regression. I think the effort is worthwhile to avoid introducing new state.
/** * @gamma_size: Size of legacy gamma ramp reported to userspace. Set up * by calling drm_mode_crtc_set_gamma_size(). -- 2.33.0.685.g46640cef36-goog
On Fri, Oct 01, 2021 at 04:34:34PM -0400, Sean Paul wrote:
On Wed, Sep 29, 2021 at 03:39:25PM -0400, Mark Yacoub wrote:
From: Mark Yacoub markyacoub@google.com
[Why]
- 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]
- Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes
assigned by the driver when it's initializing its color and CTM management. 2. Create drm_atomic_helper_check_crtc which is called by drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that they match the sizes in the new CRTC state.
Did you consider extending drm_color_lut_check() with the size checks?
Fixes: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK Tested on Zork(amdgpu) and Jacuzzi(mediatek)
Signed-off-by: Mark Yacoubmarkyacoub@chromium.org
nit: missing a space between name and email
drivers/gpu/drm/drm_atomic_helper.c | 56 +++++++++++++++++++++++++++++ drivers/gpu/drm/drm_color_mgmt.c | 2 ++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 11 ++++++ 4 files changed, 70 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 2c0c6ec928200..265b9747250d1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -930,6 +930,58 @@ drm_atomic_helper_check_planes(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_helper_check_planes);
+/**
- drm_atomic_helper_check_planes - validate state object for CRTC changes
Ctrl+c/Ctrl+v error here
- @state: the driver state object
- Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new
Are there missing words between "object" and "such"?
- state holds them.
- RETURNS:
- Zero for success or -errno
- */
+int drm_atomic_helper_check_crtc(struct drm_atomic_state *state)
drm_atomic_helper_check_crtcs to be consistent with drm_atomic_helper_check_planes
+{
- struct drm_crtc *crtc;
- struct drm_crtc_state *new_crtc_state;
- int i;
- for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
no space before (
Ignore this, parsing error on my behalf.
if (new_crtc_state->gamma_lut) {Perhaps gate these with a check of state->color_mgmt_changed first?
uint64_t supported_lut_size = crtc->gamma_lut_size;uint32_t supported_legacy_lut_size = crtc->gamma_size;uint32_t new_state_lut_size =drm_color_lut_size(new_crtc_state->gamma_lut);nit: new_state_lut_size and supported_lut_size can be pulled out to top level scope to avoid re-instantiation on each iteration
if (new_state_lut_size != supported_lut_size &&new_state_lut_size != supported_legacy_lut_size) {According to the docbook, "If drivers support multiple LUT sizes then they should publish the largest size, and sub-sample smaller sized LUTs". So should this check be > instead of != ?
DRM_DEBUG_DRIVER(drm_dbg_state() is probably more appropriate
"Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n",supported_lut_size,supported_legacy_lut_size,new_state_lut_size);return -EINVAL;}}if (new_crtc_state->degamma_lut) {uint32_t new_state_lut_size =drm_color_lut_size(new_crtc_state->degamma_lut);uint64_t supported_lut_size = crtc->degamma_lut_size;if (new_state_lut_size != supported_lut_size) {DRM_DEBUG_DRIVER(drm_dbg_state()
"Invalid Degamma LUT size. Should be %u but got %u.\n",supported_lut_size, new_state_lut_size);return -EINVAL;}}- }
- return 0;
+} +EXPORT_SYMBOL(drm_atomic_helper_check_crtc);
/**
- drm_atomic_helper_check - validate state object
- @dev: DRM device
@@ -975,6 +1027,10 @@ int drm_atomic_helper_check(struct drm_device *dev, if (ret) return ret;
- ret = drm_atomic_helper_check_crtc(state);
- if (ret)
return ret;- if (state->legacy_cursor_update) state->async_update = !drm_atomic_helper_async_check(dev, state);
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index bb14f488c8f6c..72a1b628e7cdd 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, struct drm_mode_config *config = &dev->mode_config;
if (degamma_lut_size) {
crtc->degamma_lut_size = degamma_lut_size;@@ -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;diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 4045e2507e11c..3eda13622ca1e 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -38,6 +38,7 @@ struct drm_atomic_state; struct drm_private_obj; struct drm_private_state;
+int drm_atomic_helper_check_crtc(struct drm_atomic_state *state); int drm_atomic_helper_check_modeset(struct drm_device *dev, struct drm_atomic_state *state); int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 13eeba2a750af..c602be2cafca9 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1072,6 +1072,17 @@ struct drm_crtc { /** @funcs: CRTC control functions */ const struct drm_crtc_funcs *funcs;
- /**
* @degamma_lut_size: Size of degamma LUT.*/- uint32_t degamma_lut_size;
- /**
* @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace such as* X, which doesn't support large lut sizes.*/- uint32_t gamma_lut_size;
Above, you're checking
if (new_state_lut_size != gamma_size && new_state_lut_size != gamma_lut_size) fail;
doesn't that imply that gamma_size and gamma_lut_size must always be equal? If so, perhaps turf this new state and rename degamma_lut_size to degamma_size to be consistent.
Yeah... so not sure what I was thinking when I wrote this, but my brain decided to remind me this was wrong last night while I was trying to fall asleep (thanks for checking in, brain). I still think perhaps the naming could be improved here to more clearly delineate legacy from current.
Apologies for the churn.
De-duping this and initializing crtc->gamma_size in the initialization would mean the if (crtc->gamma_size) check in drm_crtc_supports_legacy_check() is no longer useful (and possibly other similar checks), so some care will need to be taken to avoid regression. I think the effort is worthwhile to avoid introducing new state.
/** * @gamma_size: Size of legacy gamma ramp reported to userspace. Set up * by calling drm_mode_crtc_set_gamma_size(). -- 2.33.0.685.g46640cef36-goog
-- Sean Paul, Software Engineer, Google / Chromium OS
On Fri, Oct 1, 2021 at 4:34 PM Sean Paul sean@poorly.run wrote:
On Wed, Sep 29, 2021 at 03:39:25PM -0400, Mark Yacoub wrote:
From: Mark Yacoub markyacoub@google.com
[Why]
- 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]
- Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes
assigned by the driver when it's initializing its color and CTM management. 2. Create drm_atomic_helper_check_crtc which is called by drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that they match the sizes in the new CRTC state.
Did you consider extending drm_color_lut_check() with the size checks?
renamed it to be specific to channels. It's HW specific so i thought of keeping it a separate check if the driver chooses to check it. Removed the LUT size check that intel uses though.
Fixes: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK Tested on Zork(amdgpu) and Jacuzzi(mediatek)
Signed-off-by: Mark Yacoubmarkyacoub@chromium.org
nit: missing a space between name and email
drivers/gpu/drm/drm_atomic_helper.c | 56 +++++++++++++++++++++++++++++ drivers/gpu/drm/drm_color_mgmt.c | 2 ++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 11 ++++++ 4 files changed, 70 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 2c0c6ec928200..265b9747250d1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -930,6 +930,58 @@ drm_atomic_helper_check_planes(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_helper_check_planes);
+/**
- drm_atomic_helper_check_planes - validate state object for CRTC changes
Ctrl+c/Ctrl+v error here
- @state: the driver state object
- Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new
Are there missing words between "object" and "such"?
not really. I was thinking of how to reword it without being too verbose and nothing sounded good. I mean I'm checking the object, such as the LUT which is part of this object.
- state holds them.
- RETURNS:
- Zero for success or -errno
- */
+int drm_atomic_helper_check_crtc(struct drm_atomic_state *state)
drm_atomic_helper_check_crtcs to be consistent with drm_atomic_helper_check_planes
+{
struct drm_crtc *crtc;struct drm_crtc_state *new_crtc_state;int i;for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {no space before (
if (new_crtc_state->gamma_lut) {Perhaps gate these with a check of state->color_mgmt_changed first?
done . did it for each check so you can easily expand in the future and squeeze in more things around those checks as it loops the CRTC states.
uint64_t supported_lut_size = crtc->gamma_lut_size;uint32_t supported_legacy_lut_size = crtc->gamma_size;uint32_t new_state_lut_size =drm_color_lut_size(new_crtc_state->gamma_lut);nit: new_state_lut_size and supported_lut_size can be pulled out to top level scope to avoid re-instantiation on each iteration
CRTC is an iterator, so it changes within the loop.
if (new_state_lut_size != supported_lut_size &&new_state_lut_size != supported_legacy_lut_size) {According to the docbook, "If drivers support multiple LUT sizes then they should publish the largest size, and sub-sample smaller sized LUTs". So should this check be > instead of != ?
so IGT tests see it differently, they check for a very specific size, rather than a range. so if the legacy size is 256 and regular is 1024, 1000 isn't a valid size.
DRM_DEBUG_DRIVER(drm_dbg_state() is probably more appropriate
"Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n",supported_lut_size,supported_legacy_lut_size,new_state_lut_size);return -EINVAL;}}if (new_crtc_state->degamma_lut) {uint32_t new_state_lut_size =drm_color_lut_size(new_crtc_state->degamma_lut);uint64_t supported_lut_size = crtc->degamma_lut_size;if (new_state_lut_size != supported_lut_size) {DRM_DEBUG_DRIVER(drm_dbg_state()
"Invalid Degamma LUT size. Should be %u but got %u.\n",supported_lut_size, new_state_lut_size);return -EINVAL;}}}return 0;+} +EXPORT_SYMBOL(drm_atomic_helper_check_crtc);
/**
- drm_atomic_helper_check - validate state object
- @dev: DRM device
@@ -975,6 +1027,10 @@ int drm_atomic_helper_check(struct drm_device *dev, if (ret) return ret;
ret = drm_atomic_helper_check_crtc(state);if (ret)return ret;if (state->legacy_cursor_update) state->async_update = !drm_atomic_helper_async_check(dev, state);diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index bb14f488c8f6c..72a1b628e7cdd 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, struct drm_mode_config *config = &dev->mode_config;
if (degamma_lut_size) {
crtc->degamma_lut_size = degamma_lut_size; drm_object_attach_property(&crtc->base, config->degamma_lut_property, 0); drm_object_attach_property(&crtc->base,@@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, config->ctm_property, 0);
if (gamma_lut_size) {
crtc->gamma_lut_size = gamma_lut_size; drm_object_attach_property(&crtc->base, config->gamma_lut_property, 0); drm_object_attach_property(&crtc->base,diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 4045e2507e11c..3eda13622ca1e 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -38,6 +38,7 @@ struct drm_atomic_state; struct drm_private_obj; struct drm_private_state;
+int drm_atomic_helper_check_crtc(struct drm_atomic_state *state); int drm_atomic_helper_check_modeset(struct drm_device *dev, struct drm_atomic_state *state); int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 13eeba2a750af..c602be2cafca9 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1072,6 +1072,17 @@ struct drm_crtc { /** @funcs: CRTC control functions */ const struct drm_crtc_funcs *funcs;
/*** @degamma_lut_size: Size of degamma LUT.*/uint32_t degamma_lut_size;/*** @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace such as* X, which doesn't support large lut sizes.*/uint32_t gamma_lut_size;Above, you're checking
if (new_state_lut_size != gamma_size && new_state_lut_size != gamma_lut_size) fail;
doesn't that imply that gamma_size and gamma_lut_size must always be equal? If so, perhaps turf this new state and rename degamma_lut_size to degamma_size to be consistent.
De-duping this and initializing crtc->gamma_size in the initialization would mean the if (crtc->gamma_size) check in drm_crtc_supports_legacy_check() is no longer useful (and possibly other similar checks), so some care will need to be taken to avoid regression. I think the effort is worthwhile to avoid introducing new state.
/** * @gamma_size: Size of legacy gamma ramp reported to userspace. Set up * by calling drm_mode_crtc_set_gamma_size().-- 2.33.0.685.g46640cef36-goog
-- Sean Paul, Software Engineer, Google / Chromium OS
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
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; - - 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().
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);
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
On Wed, Oct 13, 2021 at 02:12:20PM -0400, Mark Yacoub wrote:
From: Mark Yacoub markyacoub@google.com
[Why]
- 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]
- 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:
- Fix typos
- Remove the LUT size check from intel driver
- 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;@@ -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;@@ -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
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]
- 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]
- 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:
- Fix typos
- Remove the LUT size check from intel driver
- 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
Dear Mark,
Thank you for your patch.
On 13.10.21 20:12, Mark Yacoub wrote:
From: Mark Yacoub markyacoub@google.com
[Why]
- 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]
- 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:
- Fix typos
- Remove the LUT size check from intel driver
- 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;@@ -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;@@ -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);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
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]
- 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]
- 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:
- Fix typos
- Remove the LUT size check from intel driver
- 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);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
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]
- 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]
- 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:
- Fix typos
- Remove the LUT size check from intel driver
- 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);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
dri-devel@lists.freedesktop.org
-
Harry Wentland -
Mark Yacoub -
Paul Menzel -
Sean Paul -
Sean Paul