From: Mark Yacoub markyacoub@google.com
[Why] This function and enum do not do generic checking on the luts but they test color channels in the LUTs. Keeping the name explicit as more generic LUT checks will follow.
Tested on Eldrid ChromeOS (TGL).
Signed-off-by: Mark Yacoub markyacoub@chromium.org --- drivers/gpu/drm/drm_color_mgmt.c | 12 ++++++------ drivers/gpu/drm/i915/display/intel_color.c | 10 +++++----- include/drm/drm_color_mgmt.h | 7 ++++--- 3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index bb14f488c8f6c..6f4e04746d90f 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -585,17 +585,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 +625,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..4bb1bc76c4de9 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,15 @@ 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_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
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. As the LUT size check now happens in drm_atomic_helper_check, remove the lut check in intel_color.c
Resolves: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK Tested on Zork(amdgpu) and Jacuzzi(mediatek), volteer(TGL)
v2: 1. Remove the rename to a parent commit. 2. Create a drm drm_check_lut_size instead of intel only function.
v1: 1. Fix typos 2. Remove the LUT size check from intel driver 3. Rename old LUT check to indicate it's a channel change
Signed-off-by: Mark Yacoub markyacoub@chromium.org --- drivers/gpu/drm/drm_atomic_helper.c | 56 ++++++++++++++++++++++ drivers/gpu/drm/drm_color_mgmt.c | 2 + drivers/gpu/drm/i915/display/intel_color.c | 39 ++++++++------- include/drm/drm_atomic_helper.h | 1 + include/drm/drm_color_mgmt.h | 13 +++++ include/drm/drm_crtc.h | 11 +++++ 6 files changed, 102 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index bc3487964fb5e..c565b3516cce9 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -929,6 +929,58 @@ 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) { + if (drm_check_lut_size(new_crtc_state->gamma_lut, + crtc->gamma_lut_size) || + drm_check_lut_size(new_crtc_state->gamma_lut, + crtc->gamma_size)) { + drm_dbg_state( + state->dev, + "Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n", + crtc->gamma_lut_size, crtc->gamma_size, + drm_color_lut_size( + new_crtc_state->gamma_lut)); + return -EINVAL; + } + } + + if (new_crtc_state->color_mgmt_changed && + new_crtc_state->degamma_lut) { + if (drm_check_lut_size(new_crtc_state->degamma_lut, + crtc->degamma_lut_size)) { + drm_dbg_state( + state->dev, + "Invalid DeGamma LUT size. Should be %u but got %u.\n", + crtc->degamma_lut_size, + drm_color_lut_size( + new_crtc_state->degamma_lut)); + return -EINVAL; + } + } + } + + return 0; +} +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs); + /** * drm_atomic_helper_check - validate state object * @dev: DRM device @@ -974,6 +1026,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 6f4e04746d90f..6bb59645a75bc 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/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index 4bb1bc76c4de9..888886d2936f8 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -1262,23 +1262,6 @@ intel_color_add_affected_planes(struct intel_crtc_state *new_crtc_state) return 0; }
-static int check_lut_size(const struct drm_property_blob *lut, int expected) -{ - int len; - - if (!lut) - return 0; - - len = drm_color_lut_size(lut); - if (len != expected) { - DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n", - len, expected); - return -EINVAL; - } - - return 0; -} - static int check_luts(const struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); @@ -1303,9 +1286,25 @@ static int check_luts(const struct intel_crtc_state *crtc_state) 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 (degamma_lut) { + if (drm_check_lut_size(degamma_lut, degamma_length)) { + drm_dbg_state( + &dev_priv->drm, + "Invalid DeGamma LUT size. Should be %u but got %u.\n", + degamma_length, + drm_color_lut_size(degamma_lut)); + return -EINVAL; + } + } + if (gamma_lut) { + if (drm_check_lut_size(gamma_lut, degamma_length)) { + drm_dbg_state( + &dev_priv->drm, + "Invalid Gamma LUT size. Should be %u but got %u.\n", + degamma_length, drm_color_lut_size(gamma_lut)); + return -EINVAL; + } + }
if (drm_color_lut_channels_check(degamma_lut, degamma_channels_tests) || drm_color_lut_channels_check(gamma_lut, gamma_channels_tests)) 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 cb1bf361ad3e3..c214812d1b7a5 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -74,6 +74,19 @@ static inline int drm_color_lut_size(const struct drm_property_blob *blob) return blob->length / sizeof(struct drm_color_lut); }
+/** + * drm_check_lut_size - Checks if LUT size matches the driver expected size. + * @lut: blob containing the LUT + * @expected_size: driver expected LUT size + * + * Returns -EINVAL on mismatch, 0 on match. + */ +static inline int drm_check_lut_size(const struct drm_property_blob *lut, + uint64_t expected_size) +{ + return drm_color_lut_size(lut) != expected_size ? -EINVAL : 0; +} + enum drm_color_encoding { DRM_COLOR_YCBCR_BT601, DRM_COLOR_YCBCR_BT709, 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().
On Tue, Oct 26, 2021 at 03:21:01PM -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. As the LUT size check now happens in drm_atomic_helper_check, remove the lut check in intel_color.c
Resolves: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK Tested on Zork(amdgpu) and Jacuzzi(mediatek), volteer(TGL)
v2:
- Remove the rename to a parent commit.
- Create a drm drm_check_lut_size instead of intel only function.
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 | 56 ++++++++++++++++++++++ drivers/gpu/drm/drm_color_mgmt.c | 2 + drivers/gpu/drm/i915/display/intel_color.c | 39 ++++++++------- include/drm/drm_atomic_helper.h | 1 + include/drm/drm_color_mgmt.h | 13 +++++ include/drm/drm_crtc.h | 11 +++++ 6 files changed, 102 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index bc3487964fb5e..c565b3516cce9 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -929,6 +929,58 @@ 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) {if (drm_check_lut_size(new_crtc_state->gamma_lut,crtc->gamma_lut_size) ||drm_check_lut_size(new_crtc_state->gamma_lut,crtc->gamma_size)) {
I think you could save a level of indentation by re-organizing the conditionals:
if (!new_crtc_state->color_mgmt_changed) continue;
if (drm_check_lut_size(new_crtc_state->gamma_lut, crtc->gamma_lut_size) || drm_check_lut_size(new_crtc_state->gamma_lut, crtc->gamma_size)) { drm_dbg_state(...); return -EINVAL; }
if (drm_check_lut_size(new_crtc_state->degamma_lut, crtc->degamma_lut_size)) { drm_dbg_state(...); return -EINVAL; }
drm_dbg_state(state->dev,"Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n",
With the indent lower, you could also make this message a bit more terse to fit in 80 chars:
"Invalid gamma lut size. Expected %u/%u, got %u\n"
crtc->gamma_lut_size, crtc->gamma_size,drm_color_lut_size(new_crtc_state->gamma_lut));return -EINVAL;}}if (new_crtc_state->color_mgmt_changed &&new_crtc_state->degamma_lut) {if (drm_check_lut_size(new_crtc_state->degamma_lut,crtc->degamma_lut_size)) {drm_dbg_state(state->dev,"Invalid DeGamma LUT size. Should be %u but got %u.\n",crtc->degamma_lut_size,drm_color_lut_size(new_crtc_state->degamma_lut));return -EINVAL;}}- }
- return 0;
+} +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs);
/**
- drm_atomic_helper_check - validate state object
- @dev: DRM device
@@ -974,6 +1026,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 6f4e04746d90f..6bb59645a75bc 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/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index 4bb1bc76c4de9..888886d2936f8 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -1262,23 +1262,6 @@ intel_color_add_affected_planes(struct intel_crtc_state *new_crtc_state) return 0; }
-static int check_lut_size(const struct drm_property_blob *lut, int expected) -{
- int len;
- if (!lut)
return 0;- len = drm_color_lut_size(lut);
- if (len != expected) {
DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",len, expected);return -EINVAL;- }
- return 0;
-}
static int check_luts(const struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); @@ -1303,9 +1286,25 @@ static int check_luts(const struct intel_crtc_state *crtc_state) 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 (degamma_lut) {
if (drm_check_lut_size(degamma_lut, degamma_length)) {drm_dbg_state(&dev_priv->drm,"Invalid DeGamma LUT size. Should be %u but got %u.\n",degamma_length,drm_color_lut_size(degamma_lut));return -EINVAL;}}
if (gamma_lut) {
if (drm_check_lut_size(gamma_lut, degamma_length)) {drm_dbg_state(&dev_priv->drm,"Invalid Gamma LUT size. Should be %u but got %u.\n",degamma_length, drm_color_lut_size(gamma_lut));return -EINVAL;}}
if (drm_color_lut_channels_check(degamma_lut, degamma_channels_tests) || drm_color_lut_channels_check(gamma_lut, gamma_channels_tests))
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 cb1bf361ad3e3..c214812d1b7a5 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -74,6 +74,19 @@ static inline int drm_color_lut_size(const struct drm_property_blob *blob) return blob->length / sizeof(struct drm_color_lut); }
+/**
- drm_check_lut_size - Checks if LUT size matches the driver expected size.
- @lut: blob containing the LUT
- @expected_size: driver expected LUT size
- Returns -EINVAL on mismatch, 0 on match.
- */
+static inline int drm_check_lut_size(const struct drm_property_blob *lut,
uint64_t expected_size)+{
If you add back in the check for !lut which existed in the intel version, you can remove all the NULL checks which precede this call above and this function will be less of a toy.
I'd probably just lift the intel function into drm_color_mgmt.c with the improved docbook and export it.
- return drm_color_lut_size(lut) != expected_size ? -EINVAL : 0;
+}
enum drm_color_encoding { DRM_COLOR_YCBCR_BT601, DRM_COLOR_YCBCR_BT709, 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.1079.g6e70778dc9-goog
Dear Mark,
On 26.10.21 21:21, 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. As the LUT size check now happens in drm_atomic_helper_check, remove the lut check in intel_color.c
Resolves: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK Tested on Zork(amdgpu) and Jacuzzi(mediatek), volteer(TGL)
If you should sent another iteration, only a minor thing, could you please add a space before the (.
v2:
- Remove the rename to a parent commit.
- Create a drm drm_check_lut_size instead of intel only function.
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 | 56 ++++++++++++++++++++++ drivers/gpu/drm/drm_color_mgmt.c | 2 + drivers/gpu/drm/i915/display/intel_color.c | 39 ++++++++------- include/drm/drm_atomic_helper.h | 1 + include/drm/drm_color_mgmt.h | 13 +++++ include/drm/drm_crtc.h | 11 +++++ 6 files changed, 102 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index bc3487964fb5e..c565b3516cce9 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -929,6 +929,58 @@ 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) {if (drm_check_lut_size(new_crtc_state->gamma_lut,crtc->gamma_lut_size) ||drm_check_lut_size(new_crtc_state->gamma_lut,crtc->gamma_size)) {drm_dbg_state(state->dev,"Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n",crtc->gamma_lut_size, crtc->gamma_size,drm_color_lut_size(new_crtc_state->gamma_lut));return -EINVAL;}}if (new_crtc_state->color_mgmt_changed &&new_crtc_state->degamma_lut) {if (drm_check_lut_size(new_crtc_state->degamma_lut,crtc->degamma_lut_size)) {drm_dbg_state(state->dev,"Invalid DeGamma LUT size. Should be %u but got %u.\n",crtc->degamma_lut_size,drm_color_lut_size(new_crtc_state->degamma_lut));return -EINVAL;}}- }
- return 0;
+} +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs);
- /**
- drm_atomic_helper_check - validate state object
- @dev: DRM device
@@ -974,6 +1026,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 6f4e04746d90f..6bb59645a75bc 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/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index 4bb1bc76c4de9..888886d2936f8 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -1262,23 +1262,6 @@ intel_color_add_affected_planes(struct intel_crtc_state *new_crtc_state) return 0; }
-static int check_lut_size(const struct drm_property_blob *lut, int expected) -{
- int len;
- if (!lut)
return 0;- len = drm_color_lut_size(lut);
- if (len != expected) {
DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",len, expected);return -EINVAL;- }
- return 0;
-}
- static int check_luts(const struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
@@ -1303,9 +1286,25 @@ static int check_luts(const struct intel_crtc_state *crtc_state) 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 (degamma_lut) {
if (drm_check_lut_size(degamma_lut, degamma_length)) {drm_dbg_state(&dev_priv->drm,"Invalid DeGamma LUT size. Should be %u but got %u.\n",degamma_length,drm_color_lut_size(degamma_lut));return -EINVAL;}}
if (gamma_lut) {
if (drm_check_lut_size(gamma_lut, degamma_length)) {drm_dbg_state(&dev_priv->drm,"Invalid Gamma LUT size. Should be %u but got %u.\n",degamma_length, drm_color_lut_size(gamma_lut));return -EINVAL;}}
if (drm_color_lut_channels_check(degamma_lut, degamma_channels_tests) || drm_color_lut_channels_check(gamma_lut, gamma_channels_tests))
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 cb1bf361ad3e3..c214812d1b7a5 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -74,6 +74,19 @@ static inline int drm_color_lut_size(const struct drm_property_blob *blob) return blob->length / sizeof(struct drm_color_lut); }
+/**
- drm_check_lut_size - Checks if LUT size matches the driver expected size.
- @lut: blob containing the LUT
- @expected_size: driver expected LUT size
- Returns -EINVAL on mismatch, 0 on match.
- */
+static inline int drm_check_lut_size(const struct drm_property_blob *lut,
uint64_t expected_size)+{
- return drm_color_lut_size(lut) != expected_size ? -EINVAL : 0;
+}
- enum drm_color_encoding { DRM_COLOR_YCBCR_BT601, DRM_COLOR_YCBCR_BT709,
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;
Why not use size_t?
- /**
* @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().
Kind regards,
Paul
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 Reviewed-by: Sean Paul seanpaul@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 Tue, Oct 26, 2021 at 03:21:00PM -0400, Mark Yacoub wrote:
From: Mark Yacoub markyacoub@google.com
[Why] This function and enum do not do generic checking on the luts but they test color channels in the LUTs.
I'm not sure there's anything inherently specific to channels, it seems like one could add a new test to reflect a HW limitation and it would fit pretty well in the lut check function. I wonder if it would be better to expose the types of tests required by the crtc such that the atomic_check could also do the test?
Sean
Keeping the name explicit as more generic LUT checks will follow.
Tested on Eldrid ChromeOS (TGL).
Signed-off-by: Mark Yacoub markyacoub@chromium.org
drivers/gpu/drm/drm_color_mgmt.c | 12 ++++++------ drivers/gpu/drm/i915/display/intel_color.c | 10 +++++----- include/drm/drm_color_mgmt.h | 7 ++++--- 3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index bb14f488c8f6c..6f4e04746d90f 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -585,17 +585,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 +625,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..4bb1bc76c4de9 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,15 @@ 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_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
2.33.0.1079.g6e70778dc9-goog
On Thu, Oct 28, 2021 at 8:42 PM Sean Paul sean@poorly.run wrote:
On Tue, Oct 26, 2021 at 03:21:00PM -0400, Mark Yacoub wrote:
From: Mark Yacoub markyacoub@google.com
[Why] This function and enum do not do generic checking on the luts but they test color channels in the LUTs.
I'm not sure there's anything inherently specific to channels, it seems like one could add a new test to reflect a HW limitation and it would fit pretty well in the lut check function. I wonder if it would be better to expose the types of tests required by the crtc such that the atomic_check could also do the test?
So the tests of the color are pretty unique to intel devices, no other device is using it so I didn't think it adds a lot of benefit adding it to the lut check. However, it's still in DRM because technically it can be supported by any driver. But once it is, the driver will have to expose the tests it wants so we can check it in atomic_check. but given that no one does expose any test but intel, i just left it only used by them.
Sean
Keeping the name explicit as more generic LUT checks will follow.
Tested on Eldrid ChromeOS (TGL).
Signed-off-by: Mark Yacoub markyacoub@chromium.org
drivers/gpu/drm/drm_color_mgmt.c | 12 ++++++------ drivers/gpu/drm/i915/display/intel_color.c | 10 +++++----- include/drm/drm_color_mgmt.h | 7 ++++--- 3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index bb14f488c8f6c..6f4e04746d90f 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -585,17 +585,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 +625,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..4bb1bc76c4de9 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,15 @@ 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_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
2.33.0.1079.g6e70778dc9-goog
-- Sean Paul, Software Engineer, Google / Chromium OS
On Thu, Oct 28, 2021 at 11:03:54PM -0400, Mark Yacoub wrote:
On Thu, Oct 28, 2021 at 8:42 PM Sean Paul sean@poorly.run wrote:
On Tue, Oct 26, 2021 at 03:21:00PM -0400, Mark Yacoub wrote:
From: Mark Yacoub markyacoub@google.com
[Why] This function and enum do not do generic checking on the luts but they test color channels in the LUTs.
I'm not sure there's anything inherently specific to channels, it seems like one could add a new test to reflect a HW limitation and it would fit pretty well in the lut check function. I wonder if it would be better to expose the types of tests required by the crtc such that the atomic_check could also do the test?
So the tests of the color are pretty unique to intel devices, no other device is using it so I didn't think it adds a lot of benefit adding it to the lut check. However, it's still in DRM because technically it can be supported by any driver. But once it is, the driver will have to expose the tests it wants so we can check it in atomic_check. but given that no one does expose any test but intel, i just left it only used by them.
Yeah, understood. Regardless of that, the spirit of the function is not specific to the color channels in the LUT, so renaming as channels_check is probably not the correct fix. I'd probably lean towards stuffing this in i915 as a driver-specific check instead of renaming it.
Sean
Sean
Keeping the name explicit as more generic LUT checks will follow.
Tested on Eldrid ChromeOS (TGL).
Signed-off-by: Mark Yacoub markyacoub@chromium.org
drivers/gpu/drm/drm_color_mgmt.c | 12 ++++++------ drivers/gpu/drm/i915/display/intel_color.c | 10 +++++----- include/drm/drm_color_mgmt.h | 7 ++++--- 3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index bb14f488c8f6c..6f4e04746d90f 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -585,17 +585,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 +625,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..4bb1bc76c4de9 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,15 @@ 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_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
2.33.0.1079.g6e70778dc9-goog
-- Sean Paul, Software Engineer, Google / Chromium OS
On 2021-10-29 09:43, Sean Paul wrote:
On Thu, Oct 28, 2021 at 11:03:54PM -0400, Mark Yacoub wrote:
On Thu, Oct 28, 2021 at 8:42 PM Sean Paul sean@poorly.run wrote:
On Tue, Oct 26, 2021 at 03:21:00PM -0400, Mark Yacoub wrote:
From: Mark Yacoub markyacoub@google.com
[Why] This function and enum do not do generic checking on the luts but they test color channels in the LUTs.
I'm not sure there's anything inherently specific to channels, it seems like one could add a new test to reflect a HW limitation and it would fit pretty well in the lut check function. I wonder if it would be better to expose the types of tests required by the crtc such that the atomic_check could also do the test?
So the tests of the color are pretty unique to intel devices, no other device is using it so I didn't think it adds a lot of benefit adding it to the lut check. However, it's still in DRM because technically it can be supported by any driver. But once it is, the driver will have to expose the tests it wants so we can check it in atomic_check. but given that no one does expose any test but intel, i just left it only used by them.
Yeah, understood. Regardless of that, the spirit of the function is not specific to the color channels in the LUT, so renaming as channels_check is probably not the correct fix. I'd probably lean towards stuffing this in i915 as a driver-specific check instead of renaming it.
The checks could be generally useful for other drivers. I assume a lot of HW wants the LUT to be non-decreasing and there might be other HW out there that implements LUTs with a single channel that applies to all colors. Since this is only used by i915 at the moment I don't have a strong opinion about keeping it in DRM core or stuffing it into i915.
I agree with Sean that this function isn't specifically about color channels. The function seems to be designed as a generic check for LUTs, which is why the "tests" flag is passed in. DRM_COLOR_LUT_EQUAL_CHANNELS is checking the channels but DRM_COLOR_LUT_NON_DECREASING doesn't particularly have anything to do with the channels.
Harry
Sean
Sean
Keeping the name explicit as more generic LUT checks will follow.
Tested on Eldrid ChromeOS (TGL).
Signed-off-by: Mark Yacoub markyacoub@chromium.org
drivers/gpu/drm/drm_color_mgmt.c | 12 ++++++------ drivers/gpu/drm/i915/display/intel_color.c | 10 +++++----- include/drm/drm_color_mgmt.h | 7 ++++--- 3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index bb14f488c8f6c..6f4e04746d90f 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -585,17 +585,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 +625,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..4bb1bc76c4de9 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,15 @@ 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_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
2.33.0.1079.g6e70778dc9-goog
-- Sean Paul, Software Engineer, Google / Chromium OS
dri-devel@lists.freedesktop.org
-
Harry Wentland -
Mark Yacoub -
Paul Menzel -
Sean Paul