RE: [i-g-t 03/14] kms_color_helper: Add helper functions for plane color mgmt

From: Pekka Paalanen ppaalanen@gmail.com Sent: Thursday, November 18, 2021 2:11 PM To: Modem, Bhanuprakash bhanuprakash.modem@intel.com Cc: igt-dev@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Juha-Pekka Heikkila juhapekka.heikkila@gmail.com; Shankar, Uma uma.shankar@intel.com Subject: Re: [i-g-t 03/14] kms_color_helper: Add helper functions for plane color mgmt

On Mon, 15 Nov 2021 15:17:48 +0530 Bhanuprakash Modem bhanuprakash.modem@intel.com wrote:

...

Add helper functions to support Plane color management properties.

Cc: Harry Wentland harry.wentland@amd.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Juha-Pekka Heikkila juhapekka.heikkila@gmail.com Cc: Uma Shankar uma.shankar@intel.com Signed-off-by: Bhanuprakash Modem bhanuprakash.modem@intel.com

---

tests/kms_color_helper.c | 173 +++++++++++++++++++++++++++++++++++++++ tests/kms_color_helper.h | 29 +++++++ 2 files changed, 202 insertions(+)

diff --git a/tests/kms_color_helper.c b/tests/kms_color_helper.c index d71e7bb2e6..c65b7a0f50 100644 --- a/tests/kms_color_helper.c +++ b/tests/kms_color_helper.c @@ -241,6 +241,150 @@ void disable_prop(igt_pipe_t *pipe, enum

igt_atomic_crtc_properties prop)

...

igt_pipe_obj_replace_prop_blob(pipe, prop, NULL, 0);

}

+drmModePropertyPtr get_plane_gamma_degamma_mode(igt_plane_t *plane,

• 		enum igt_atomic_plane_properties prop)
  

+{

• igt_display_t *display = plane->pipe->display;
• uint32_t prop_id = plane->props[prop];
• drmModePropertyPtr drmProp;
• igt_assert(prop_id);
• drmProp = drmModeGetProperty(display->drm_fd, prop_id);
• igt_assert(drmProp);
• igt_assert(drmProp->count_enums);
• return drmProp;

+}

+struct drm_color_lut_ext *create_linear_lut(segment_data_t *info)

Hi,

this actually creates an identity transformation as a LUT, does it not? If so, it should be named that way. A linear transformation represented by a LUT may not be identity transformation.

I'll float a new rev by renaming it to identity.

Identity is quite a bad case to test with, because it is a no-op, and simply accidentally disabling the whole LUT will pass a writeback test.

...

+{

• uint32_t val, segment, entry, index = 0;
• uint32_t max_val = 0xffffffff;
• struct drm_color_lut_ext *lut = malloc(sizeof(struct drm_color_lut_ext)

• info->entries_count);

...

• igt_assert(lut);
• for (segment = 0; segment < info->segment_count; segment++) {

• uint32_t entry_count = info->segment_data[segment].count;
  

• uint32_t start = info->segment_data[segment].start;
  

• uint32_t end = info->segment_data[segment].end;
  

• for (entry = 1; entry <= entry_count; entry++) {
  

• 	val = (index == 0) ? /* First entry is Zero. */
  

• 		0 : start + entry * ((end - start) * 1.0 / entry_count);
  

Having to explicitly special-case index zero feels odd to me. Why does it need explicit special-casing?

To me it's a hint that the definitions of .start and .end are somehow inconsistent.

Intel hardware is expecting the first value as zero for any kind of LUT (Please refer CRTC implementation), and yes we can remove this logic from this helper function & should be handled in individual tests. I'll float a new rev with this change.

...

• 	lut[index].red = lut[index].green = lut[index].blue =
  

MIN(val, max_val);

There are tests that use different curves for each of red, green and blue and check that they also work, right?

...

• 	index++;
  

• }
  

• }
• return lut;

+}

+struct drm_color_lut_ext *create_max_lut(segment_data_t *info)

What is the point of testing this pathological case?

The intension of this function is to convert gradient colors to solid colors. (Please refer the patch 04/14 in this series)

Compare "gradient colors + maxed out plane gamma LUT" and "solid color + unity plane gamma LUT"

...

+{

• int i;
• struct drm_color_lut_ext *lut;
• uint32_t max_val = 0xffffffff;
• lut = malloc(sizeof(struct drm_color_lut_ext) * info->entries_count);
• igt_assert(lut);
• lut[0].red = lut[0].green = lut[0].blue = 0; /* First entry is Zero. */

It's not a max LUT, if the first entry is zero.

Please check the previous comments

...

• for (i = 1; i < info->entries_count; i++)

• lut[i].red = lut[i].green = lut[i].blue = max_val;
  

• return lut;

+}

+void clear_segment_data(segment_data_t *info) +{

• if (!info)

• return;
  

• free(info->segment_data);
• free(info);

+}

+segment_data_t *get_segment_data(data_t *data,

• 		uint64_t blob_id, char *mode)
  

+{

• drmModePropertyBlobPtr blob;
• struct drm_color_lut_range *lut_range = NULL;
• segment_data_t *info = NULL;
• uint32_t i;
• blob = drmModeGetPropertyBlob(data->drm_fd, blob_id);
• igt_assert(blob);
• igt_assert(blob->length);
• info = malloc(sizeof(segment_data_t));
• igt_assert(info);
• lut_range = (struct drm_color_lut_range *) blob->data;
• info->segment_count = blob->length / sizeof(lut_range[0]);
• igt_assert(info->segment_count);
• info->segment_data = malloc(sizeof(struct drm_color_lut_range) * info-

segment_count);

• igt_assert(info->segment_data);
• for (i = 0; i < info->segment_count; i++) {

• info->entries_count += lut_range[i].count;
  

• info->segment_data[i] = lut_range[i];
  

• }
• drmModeFreePropertyBlob(blob);
• return info;

+}

+void set_plane_gamma(igt_plane_t *plane,

• char *mode,
  

• struct drm_color_lut_ext *lut,
  

• uint32_t size)
  

+{

• igt_plane_set_prop_enum(plane, IGT_PLANE_GAMMA_MODE, mode);
• igt_plane_replace_prop_blob(plane, IGT_PLANE_GAMMA_LUT, lut, size);

+}

+void set_plane_degamma(igt_plane_t *plane,

• char *mode,
  

• struct drm_color_lut_ext *lut,
  

• uint32_t size)
  

+{

• igt_plane_set_prop_enum(plane, IGT_PLANE_DEGAMMA_MODE, mode);
• igt_plane_replace_prop_blob(plane, IGT_PLANE_DEGAMMA_LUT, lut, size);

+}

+void set_plane_ctm(igt_plane_t *plane, const double *coefficients) +{

• struct drm_color_ctm ctm;
• int i;
• for (i = 0; i < ARRAY_SIZE(ctm.matrix); i++) {

• if (coefficients[i] < 0) {
  

• 	ctm.matrix[i] =
  

• 		(int64_t) (-coefficients[i] *
  

• 		((int64_t) 1L << 32));
  

• 	ctm.matrix[i] |= 1ULL << 63;
  

• } else
  

• 	ctm.matrix[i] =
  

• 		(int64_t) (coefficients[i] *
  

• 		((int64_t) 1L << 32));
  

• }

Is this literally the only function that converts double to the KMS matrix element type? If not, I think the value conversion should be a separate function that just converts a single value, and be called from all sites that need the same conversion.

Sure, I'll float a new rev by adding a new function

...

• igt_plane_replace_prop_blob(plane, IGT_PLANE_CTM, &ctm, sizeof(ctm));

+}

+void disable_plane_prop(igt_plane_t *plane, enum

igt_atomic_plane_properties prop)

...

+{

• if (igt_plane_has_prop(plane, prop))

• igt_plane_replace_prop_blob(plane, prop, NULL, 0);
  

+}

drmModePropertyBlobPtr get_blob(data_t *data, igt_pipe_t *pipe, enum igt_atomic_crtc_properties

prop)

...

{ @@ -274,6 +418,19 @@ pipe_set_property_blob_id(igt_pipe_t *pipe, return ret; }

+static int +plane_set_property_blob(igt_display_t *display,

• 	igt_plane_t *plane,
  

• 	enum igt_atomic_plane_properties prop,
  

• 	void *ptr, size_t length)
  

+{

• igt_plane_replace_prop_blob(plane, prop, ptr, length);
• return igt_display_try_commit2(display,

• 		       display->is_atomic ?
  

• 		       COMMIT_ATOMIC : COMMIT_LEGACY);
  

+}

int pipe_set_property_blob(igt_pipe_t *pipe, enum igt_atomic_crtc_properties prop, @@ -319,6 +476,22 @@ invalid_lut_sizes(data_t *data, enum pipe p, free(lut); }

+void +invalid_plane_lut_sizes(igt_display_t *display,

• 	igt_plane_t *plane,
  

• 	enum igt_atomic_plane_properties prop,
  

• 	size_t lut_size)
  

+{

• void *lut = malloc(lut_size * 2);

Feeding garbage data does not seem like a good idea. Maybe it can lead to EINVAL even when the driver misses the size check because the values are illegal?

I think this needs to craft a real and good lut data blob, with the only problem that it is of the wrong size.

Yes, we can do that. But my intension is to validate the size check only. Read the lut_size from uapi & try with "lut_size +/- 1"

I think for now we can go with zero lut: `memset(lut, 0, (lut_size *2));` I am also planning to add few tests for invalid LUTs in next phase later.

- Bhanu

...

• igt_assert(lut);
• igt_assert_eq(plane_set_property_blob(display, plane, prop, lut, 1), -

EINVAL);

...

• igt_assert_eq(plane_set_property_blob(display, plane, prop, lut,

lut_size + 1), -EINVAL);

...

• igt_assert_eq(plane_set_property_blob(display, plane, prop, lut,

lut_size - 1), -EINVAL);

...

• free(lut);

+}

Thanks, pq

...

void invalid_gamma_lut_sizes(data_t *data, enum pipe p) { diff --git a/tests/kms_color_helper.h b/tests/kms_color_helper.h index bb6f0054f3..5a35dcaac1 100644 --- a/tests/kms_color_helper.h +++ b/tests/kms_color_helper.h @@ -64,6 +64,14 @@ typedef struct { color_t coeffs[]; } gamma_lut_t;

+typedef struct {

• uint32_t segment_count;
• struct drm_color_lut_range *segment_data;
• uint32_t entries_count;

+} segment_data_t;

+#define MIN(a, b) ((a) < (b) ? (a) : (b))

void paint_gradient_rectangles(data_t *data, drmModeModeInfo *mode, color_t *colors, @@ -90,10 +98,31 @@ void set_gamma(data_t *data, void set_ctm(igt_pipe_t *pipe, const double *coefficients); void disable_prop(igt_pipe_t *pipe, enum igt_atomic_crtc_properties prop);

+drmModePropertyPtr get_plane_gamma_degamma_mode(igt_plane_t *plane,

• enum igt_atomic_plane_properties prop);

+void clear_segment_data(segment_data_t *info); +struct drm_color_lut_ext *create_linear_lut(segment_data_t *info); +struct drm_color_lut_ext *create_max_lut(segment_data_t *info); +segment_data_t *get_segment_data(data_t *data, uint64_t blob_id, char

*mode);

...

+void set_plane_gamma(igt_plane_t *plane,

• char *mode, struct drm_color_lut_ext *lut, uint32_t size);

+void set_plane_degamma(igt_plane_t *plane,

• char *mode, struct drm_color_lut_ext *lut, uint32_t size);

+void set_plane_ctm(igt_plane_t *plane, const double *coefficients); +void disable_plane_prop(igt_plane_t *plane, enum

igt_atomic_plane_properties prop);

...

+void invalid_plane_lut_sizes(igt_display_t *display,

• 	   igt_plane_t *plane,
  

• 	   enum igt_atomic_plane_properties prop,
  

• 	   size_t lut_size);
  

#define disable_degamma(pipe) disable_prop(pipe, IGT_CRTC_DEGAMMA_LUT) #define disable_gamma(pipe) disable_prop(pipe, IGT_CRTC_GAMMA_LUT) #define disable_ctm(pipe) disable_prop(pipe, IGT_CRTC_CTM)

+#define disable_plane_degamma(plane) disable_plane_prop(plane,

IGT_PLANE_DEGAMMA_LUT)

...

+#define disable_plane_gamma(plane) disable_plane_prop(plane,

IGT_PLANE_GAMMA_LUT)

...

+#define disable_plane_ctm(plane) disable_plane_prop(plane, IGT_PLANE_CTM)

drmModePropertyBlobPtr get_blob(data_t *data, igt_pipe_t *pipe, enum igt_atomic_crtc_properties prop); bool crc_equal(igt_crc_t *a, igt_crc_t *b);