Message ID | 20181217224415.12848-1-matthew.d.roper@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v4,1/2] drm: Add color management LUT validation helper (v4) | expand |
Dave, Daniel - any concerns if we merge this drm core patch through the Intel tree? The second patch in the series doesn't apply cleanly in drm-misc-next. Matt On Mon, Dec 17, 2018 at 02:44:14PM -0800, Matt Roper wrote: > Some hardware may place additional restrictions on the gamma/degamma > curves described by our LUT properties. E.g., that a gamma curve never > decreases or that the red/green/blue channels of a LUT's entries must be > equal. Let's add a helper function that drivers can use to test that a > userspace-provided LUT is valid and doesn't violate hardware > requirements. > > v2: > - Combine into a single helper that just takes a bitmask of the tests > to apply. (Brian Starkey) > - Add additional check (always performed) that LUT property blob size > is always a multiple of the LUT entry size. (stolen from ARM driver) > > v3: > - Drop the LUT size check again since > drm_atomic_replace_property_blob_from_id() already covers this for > us. (Alexandru Gheorghe) > > v4: > - Use an enum to describe possible test values rather than #define's; > this is cleaner to provide kerneldoc for. (Daniel Vetter) > - s/DRM_COLOR_LUT_INCREASING/DRM_COLOR_LUT_NON_DECREASING/. (Ville) > > Cc: Uma Shankar <uma.shankar@intel.com> > Cc: Swati Sharma <swati2.sharma@intel.com> > Cc: Brian Starkey <Brian.Starkey@arm.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > Reviewed-by(v1): Brian Starkey <brian.starkey@arm.com> > Reviewed-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > Reviewed-by: Uma Shankar <uma.shankar@intel.com> > --- > drivers/gpu/drm/drm_color_mgmt.c | 44 ++++++++++++++++++++++++++++++++++++++++ > include/drm/drm_color_mgmt.h | 29 ++++++++++++++++++++++++++ > 2 files changed, 73 insertions(+) > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c > index 07dcf47daafe..968ca7c91ad8 100644 > --- a/drivers/gpu/drm/drm_color_mgmt.c > +++ b/drivers/gpu/drm/drm_color_mgmt.c > @@ -462,3 +462,47 @@ int drm_plane_create_color_properties(struct drm_plane *plane, > return 0; > } > EXPORT_SYMBOL(drm_plane_create_color_properties); > + > +/** > + * drm_color_lut_check - check validity of 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. > + * > + * Returns 0 on success, -EINVAL on failure. > + */ > +int drm_color_lut_check(struct drm_property_blob *lut, > + uint32_t tests) > +{ > + struct drm_color_lut *entry; > + int i; > + > + if (!lut || !tests) > + return 0; > + > + entry = lut->data; > + for (i = 0; i < drm_color_lut_size(lut); i++) { > + if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) { > + if (entry[i].red != entry[i].blue || > + entry[i].red != entry[i].green) { > + DRM_DEBUG_KMS("All LUT entries must have equal r/g/b\n"); > + return -EINVAL; > + } > + } > + > + if (i > 0 && tests & DRM_COLOR_LUT_NON_DECREASING) { > + if (entry[i].red < entry[i - 1].red || > + entry[i].green < entry[i - 1].green || > + entry[i].blue < entry[i - 1].blue) { > + DRM_DEBUG_KMS("LUT entries must never decrease.\n"); > + return -EINVAL; > + } > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_color_lut_check); > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h > index 90ef9996d9a4..6affbda6d9cb 100644 > --- a/include/drm/drm_color_mgmt.h > +++ b/include/drm/drm_color_mgmt.h > @@ -69,4 +69,33 @@ int drm_plane_create_color_properties(struct drm_plane *plane, > u32 supported_ranges, > enum drm_color_encoding default_encoding, > enum drm_color_range default_range); > + > +/** > + * enum drm_color_lut_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 { > + /** > + * @DRM_COLOR_LUT_EQUAL_CHANNELS: > + * > + * Checks whether the entries of a LUT all have equal values for the > + * red, green, and blue channels. Intended for hardware that only > + * accepts a single value per LUT entry and assumes that value applies > + * to all three color components. > + */ > + DRM_COLOR_LUT_EQUAL_CHANNELS = BIT(0), > + > + /** > + * @DRM_COLOR_LUT_NON_DECREASING: > + * > + * Checks whether the entries of a LUT are always flat or increasing > + * (never decreasing). > + */ > + DRM_COLOR_LUT_NON_DECREASING = BIT(1), > +}; > + > +int drm_color_lut_check(struct drm_property_blob *lut, > + uint32_t tests); > #endif > -- > 2.14.4 >
On Fri, Jan 11, 2019 at 02:27:00PM -0800, Matt Roper wrote: > Dave, Daniel - any concerns if we merge this drm core patch through the > Intel tree? The second patch in the series doesn't apply cleanly in > drm-misc-next. Hm, need to discuss with Dave how good my ack powers are now. In the past we had the rule that I asked him for an explicit ack, but back then I was still maintainer of i915, so a bit a too obvious conflict there. I think with the separate roles me acking should be good enough. Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Dave, sounds reasonable? -Daniel > > > Matt > > > On Mon, Dec 17, 2018 at 02:44:14PM -0800, Matt Roper wrote: > > Some hardware may place additional restrictions on the gamma/degamma > > curves described by our LUT properties. E.g., that a gamma curve never > > decreases or that the red/green/blue channels of a LUT's entries must be > > equal. Let's add a helper function that drivers can use to test that a > > userspace-provided LUT is valid and doesn't violate hardware > > requirements. > > > > v2: > > - Combine into a single helper that just takes a bitmask of the tests > > to apply. (Brian Starkey) > > - Add additional check (always performed) that LUT property blob size > > is always a multiple of the LUT entry size. (stolen from ARM driver) > > > > v3: > > - Drop the LUT size check again since > > drm_atomic_replace_property_blob_from_id() already covers this for > > us. (Alexandru Gheorghe) > > > > v4: > > - Use an enum to describe possible test values rather than #define's; > > this is cleaner to provide kerneldoc for. (Daniel Vetter) > > - s/DRM_COLOR_LUT_INCREASING/DRM_COLOR_LUT_NON_DECREASING/. (Ville) > > > > Cc: Uma Shankar <uma.shankar@intel.com> > > Cc: Swati Sharma <swati2.sharma@intel.com> > > Cc: Brian Starkey <Brian.Starkey@arm.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > Reviewed-by(v1): Brian Starkey <brian.starkey@arm.com> > > Reviewed-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > > Reviewed-by: Uma Shankar <uma.shankar@intel.com> > > --- > > drivers/gpu/drm/drm_color_mgmt.c | 44 ++++++++++++++++++++++++++++++++++++++++ > > include/drm/drm_color_mgmt.h | 29 ++++++++++++++++++++++++++ > > 2 files changed, 73 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c > > index 07dcf47daafe..968ca7c91ad8 100644 > > --- a/drivers/gpu/drm/drm_color_mgmt.c > > +++ b/drivers/gpu/drm/drm_color_mgmt.c > > @@ -462,3 +462,47 @@ int drm_plane_create_color_properties(struct drm_plane *plane, > > return 0; > > } > > EXPORT_SYMBOL(drm_plane_create_color_properties); > > + > > +/** > > + * drm_color_lut_check - check validity of 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. > > + * > > + * Returns 0 on success, -EINVAL on failure. > > + */ > > +int drm_color_lut_check(struct drm_property_blob *lut, > > + uint32_t tests) > > +{ > > + struct drm_color_lut *entry; > > + int i; > > + > > + if (!lut || !tests) > > + return 0; > > + > > + entry = lut->data; > > + for (i = 0; i < drm_color_lut_size(lut); i++) { > > + if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) { > > + if (entry[i].red != entry[i].blue || > > + entry[i].red != entry[i].green) { > > + DRM_DEBUG_KMS("All LUT entries must have equal r/g/b\n"); > > + return -EINVAL; > > + } > > + } > > + > > + if (i > 0 && tests & DRM_COLOR_LUT_NON_DECREASING) { > > + if (entry[i].red < entry[i - 1].red || > > + entry[i].green < entry[i - 1].green || > > + entry[i].blue < entry[i - 1].blue) { > > + DRM_DEBUG_KMS("LUT entries must never decrease.\n"); > > + return -EINVAL; > > + } > > + } > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(drm_color_lut_check); > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h > > index 90ef9996d9a4..6affbda6d9cb 100644 > > --- a/include/drm/drm_color_mgmt.h > > +++ b/include/drm/drm_color_mgmt.h > > @@ -69,4 +69,33 @@ int drm_plane_create_color_properties(struct drm_plane *plane, > > u32 supported_ranges, > > enum drm_color_encoding default_encoding, > > enum drm_color_range default_range); > > + > > +/** > > + * enum drm_color_lut_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 { > > + /** > > + * @DRM_COLOR_LUT_EQUAL_CHANNELS: > > + * > > + * Checks whether the entries of a LUT all have equal values for the > > + * red, green, and blue channels. Intended for hardware that only > > + * accepts a single value per LUT entry and assumes that value applies > > + * to all three color components. > > + */ > > + DRM_COLOR_LUT_EQUAL_CHANNELS = BIT(0), > > + > > + /** > > + * @DRM_COLOR_LUT_NON_DECREASING: > > + * > > + * Checks whether the entries of a LUT are always flat or increasing > > + * (never decreasing). > > + */ > > + DRM_COLOR_LUT_NON_DECREASING = BIT(1), > > +}; > > + > > +int drm_color_lut_check(struct drm_property_blob *lut, > > + uint32_t tests); > > #endif > > -- > > 2.14.4 > > > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
On Sat, Jan 12, 2019 at 01:07:14PM +0100, Daniel Vetter wrote: > On Fri, Jan 11, 2019 at 02:27:00PM -0800, Matt Roper wrote: > > Dave, Daniel - any concerns if we merge this drm core patch through the > > Intel tree? The second patch in the series doesn't apply cleanly in > > drm-misc-next. > > Hm, need to discuss with Dave how good my ack powers are now. In the past > we had the rule that I asked him for an explicit ack, but back then I was > still maintainer of i915, so a bit a too obvious conflict there. I think > with the separate roles me acking should be good enough. > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Dave, sounds reasonable? > -Daniel Dave confirmed on IRC that Daniel's ack here is sufficient. Merged both patches through the Intel tree (dinq); thanks everyone for the reviews. Matt > > > > > > > Matt > > > > > > On Mon, Dec 17, 2018 at 02:44:14PM -0800, Matt Roper wrote: > > > Some hardware may place additional restrictions on the gamma/degamma > > > curves described by our LUT properties. E.g., that a gamma curve never > > > decreases or that the red/green/blue channels of a LUT's entries must be > > > equal. Let's add a helper function that drivers can use to test that a > > > userspace-provided LUT is valid and doesn't violate hardware > > > requirements. > > > > > > v2: > > > - Combine into a single helper that just takes a bitmask of the tests > > > to apply. (Brian Starkey) > > > - Add additional check (always performed) that LUT property blob size > > > is always a multiple of the LUT entry size. (stolen from ARM driver) > > > > > > v3: > > > - Drop the LUT size check again since > > > drm_atomic_replace_property_blob_from_id() already covers this for > > > us. (Alexandru Gheorghe) > > > > > > v4: > > > - Use an enum to describe possible test values rather than #define's; > > > this is cleaner to provide kerneldoc for. (Daniel Vetter) > > > - s/DRM_COLOR_LUT_INCREASING/DRM_COLOR_LUT_NON_DECREASING/. (Ville) > > > > > > Cc: Uma Shankar <uma.shankar@intel.com> > > > Cc: Swati Sharma <swati2.sharma@intel.com> > > > Cc: Brian Starkey <Brian.Starkey@arm.com> > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > Reviewed-by(v1): Brian Starkey <brian.starkey@arm.com> > > > Reviewed-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > > > Reviewed-by: Uma Shankar <uma.shankar@intel.com> > > > --- > > > drivers/gpu/drm/drm_color_mgmt.c | 44 ++++++++++++++++++++++++++++++++++++++++ > > > include/drm/drm_color_mgmt.h | 29 ++++++++++++++++++++++++++ > > > 2 files changed, 73 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c > > > index 07dcf47daafe..968ca7c91ad8 100644 > > > --- a/drivers/gpu/drm/drm_color_mgmt.c > > > +++ b/drivers/gpu/drm/drm_color_mgmt.c > > > @@ -462,3 +462,47 @@ int drm_plane_create_color_properties(struct drm_plane *plane, > > > return 0; > > > } > > > EXPORT_SYMBOL(drm_plane_create_color_properties); > > > + > > > +/** > > > + * drm_color_lut_check - check validity of 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. > > > + * > > > + * Returns 0 on success, -EINVAL on failure. > > > + */ > > > +int drm_color_lut_check(struct drm_property_blob *lut, > > > + uint32_t tests) > > > +{ > > > + struct drm_color_lut *entry; > > > + int i; > > > + > > > + if (!lut || !tests) > > > + return 0; > > > + > > > + entry = lut->data; > > > + for (i = 0; i < drm_color_lut_size(lut); i++) { > > > + if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) { > > > + if (entry[i].red != entry[i].blue || > > > + entry[i].red != entry[i].green) { > > > + DRM_DEBUG_KMS("All LUT entries must have equal r/g/b\n"); > > > + return -EINVAL; > > > + } > > > + } > > > + > > > + if (i > 0 && tests & DRM_COLOR_LUT_NON_DECREASING) { > > > + if (entry[i].red < entry[i - 1].red || > > > + entry[i].green < entry[i - 1].green || > > > + entry[i].blue < entry[i - 1].blue) { > > > + DRM_DEBUG_KMS("LUT entries must never decrease.\n"); > > > + return -EINVAL; > > > + } > > > + } > > > + } > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL(drm_color_lut_check); > > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h > > > index 90ef9996d9a4..6affbda6d9cb 100644 > > > --- a/include/drm/drm_color_mgmt.h > > > +++ b/include/drm/drm_color_mgmt.h > > > @@ -69,4 +69,33 @@ int drm_plane_create_color_properties(struct drm_plane *plane, > > > u32 supported_ranges, > > > enum drm_color_encoding default_encoding, > > > enum drm_color_range default_range); > > > + > > > +/** > > > + * enum drm_color_lut_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 { > > > + /** > > > + * @DRM_COLOR_LUT_EQUAL_CHANNELS: > > > + * > > > + * Checks whether the entries of a LUT all have equal values for the > > > + * red, green, and blue channels. Intended for hardware that only > > > + * accepts a single value per LUT entry and assumes that value applies > > > + * to all three color components. > > > + */ > > > + DRM_COLOR_LUT_EQUAL_CHANNELS = BIT(0), > > > + > > > + /** > > > + * @DRM_COLOR_LUT_NON_DECREASING: > > > + * > > > + * Checks whether the entries of a LUT are always flat or increasing > > > + * (never decreasing). > > > + */ > > > + DRM_COLOR_LUT_NON_DECREASING = BIT(1), > > > +}; > > > + > > > +int drm_color_lut_check(struct drm_property_blob *lut, > > > + uint32_t tests); > > > #endif > > > -- > > > 2.14.4 > > > > > > > -- > > Matt Roper > > Graphics Software Engineer > > IoTG Platform Enabling & Development > > Intel Corporation > > (916) 356-2795 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 07dcf47daafe..968ca7c91ad8 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -462,3 +462,47 @@ int drm_plane_create_color_properties(struct drm_plane *plane, return 0; } EXPORT_SYMBOL(drm_plane_create_color_properties); + +/** + * drm_color_lut_check - check validity of 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. + * + * Returns 0 on success, -EINVAL on failure. + */ +int drm_color_lut_check(struct drm_property_blob *lut, + uint32_t tests) +{ + struct drm_color_lut *entry; + int i; + + if (!lut || !tests) + return 0; + + entry = lut->data; + for (i = 0; i < drm_color_lut_size(lut); i++) { + if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) { + if (entry[i].red != entry[i].blue || + entry[i].red != entry[i].green) { + DRM_DEBUG_KMS("All LUT entries must have equal r/g/b\n"); + return -EINVAL; + } + } + + if (i > 0 && tests & DRM_COLOR_LUT_NON_DECREASING) { + if (entry[i].red < entry[i - 1].red || + entry[i].green < entry[i - 1].green || + entry[i].blue < entry[i - 1].blue) { + DRM_DEBUG_KMS("LUT entries must never decrease.\n"); + return -EINVAL; + } + } + } + + return 0; +} +EXPORT_SYMBOL(drm_color_lut_check); diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h index 90ef9996d9a4..6affbda6d9cb 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -69,4 +69,33 @@ int drm_plane_create_color_properties(struct drm_plane *plane, u32 supported_ranges, enum drm_color_encoding default_encoding, enum drm_color_range default_range); + +/** + * enum drm_color_lut_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 { + /** + * @DRM_COLOR_LUT_EQUAL_CHANNELS: + * + * Checks whether the entries of a LUT all have equal values for the + * red, green, and blue channels. Intended for hardware that only + * accepts a single value per LUT entry and assumes that value applies + * to all three color components. + */ + DRM_COLOR_LUT_EQUAL_CHANNELS = BIT(0), + + /** + * @DRM_COLOR_LUT_NON_DECREASING: + * + * Checks whether the entries of a LUT are always flat or increasing + * (never decreasing). + */ + DRM_COLOR_LUT_NON_DECREASING = BIT(1), +}; + +int drm_color_lut_check(struct drm_property_blob *lut, + uint32_t tests); #endif