From: Mark Yacoub <[email protected]>
[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 <[email protected]>
---
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 Tue, Oct 26, 2021 at 03:21:00PM -0400, Mark Yacoub wrote:
> From: Mark Yacoub <[email protected]>
>
> [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 <[email protected]>
> ---
> 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 8:42 PM Sean Paul <[email protected]> wrote:
>
> On Tue, Oct 26, 2021 at 03:21:00PM -0400, Mark Yacoub wrote:
> > From: Mark Yacoub <[email protected]>
> >
> > [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 <[email protected]>
> > ---
> > 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 <[email protected]> wrote:
> >
> > On Tue, Oct 26, 2021 at 03:21:00PM -0400, Mark Yacoub wrote:
> > > From: Mark Yacoub <[email protected]>
> > >
> > > [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 <[email protected]>
> > > ---
> > > 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
--
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 <[email protected]> wrote:
>>>
>>> On Tue, Oct 26, 2021 at 03:21:00PM -0400, Mark Yacoub wrote:
>>>> From: Mark Yacoub <[email protected]>
>>>>
>>>> [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 <[email protected]>
>>>> ---
>>>> 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
>