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.

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.

• 	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?

+{

• 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.

• 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.

• 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.

• 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);

On 2021-11-15 04:47, Bhanuprakash Modem 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) +{

• 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);
  

• 	lut[index].red = lut[index].green = lut[index].blue = MIN(val, max_val);
  

• 	index++;
  

• }
  

• }
• return lut;

+}

+struct drm_color_lut_ext *create_max_lut(segment_data_t *info) +{

• 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. */
• 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));
  

• }
• 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);
• 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);

+}

This might make more sense as part of patch 7, though I don't have a strong preference.

Harry

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);

From: Pekka Paalanen ppaalanen@gmail.com Sent: Thursday, November 18, 2021 2:33 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 04/14] tests/kms_color: New subtests for Plane gamma

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

...

To verify Plane gamma, draw 3 gradient rectangles in red, green and blue, with a maxed out gamma LUT and verify we have the same CRC as drawing solid color rectangles.

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.c | 179 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 178 insertions(+), 1 deletion(-)

diff --git a/tests/kms_color.c b/tests/kms_color.c index 775f35964f..b45d66762f 100644 --- a/tests/kms_color.c +++ b/tests/kms_color.c @@ -24,7 +24,34 @@

#include "kms_color_helper.h"

-IGT_TEST_DESCRIPTION("Test Color Features at Pipe level"); +IGT_TEST_DESCRIPTION("Test Color Features at Pipe & Plane level");

+#define MAX_SUPPORTED_PLANES 7 +#define SDR_PLANE_BASE 3

+typedef bool (*test_t)(data_t*, igt_plane_t*);

+static bool is_hdr_plane(const igt_plane_t *plane) +{

• return plane->index >= 0 && plane->index < SDR_PLANE_BASE;

How can this be right for all KMS drivers?

What is a HDR plane?

...

+}

+static bool is_valid_plane(igt_plane_t *plane) +{

• int index = plane->index;
• if (plane->type != DRM_PLANE_TYPE_PRIMARY)

• return false;
  

• /*

• * Test 1 HDR plane, 1 SDR plane.
  

• *
  

• * 0,1,2 HDR planes
  

• * 3,4,5,6 SDR planes
  

As above, where does this come from? Is this your hardware?

It is specific to Intel hardware, I'll make this generic in next rev.

Extended mode: By default to optimize the CI, we can run these tests on first & last planes only, If we want to cover all the planes, we need to pass an argument in commandline

Example: ./kms_color --run-subtest pipe-A-plane-gamma ./kms_color --extend --run-subtest pipe-A-plane-gamma

...

• *
  

• */
  

• return index >= 0 && index < MAX_SUPPORTED_PLANES;

+}

static void test_pipe_degamma(data_t *data, igt_plane_t *primary) @@ -638,6 +665,122 @@ static void test_pipe_limited_range_ctm(data_t *data, } #endif

+static bool plane_gamma_test(data_t *data, igt_plane_t *plane) +{

• igt_output_t *output;
• igt_display_t *display = &data->display;
• drmModeModeInfo *mode;
• struct igt_fb fb;
• drmModePropertyPtr gamma_mode = NULL;
• uint32_t i;
• bool ret = true;
• igt_pipe_crc_t *pipe_crc = NULL;
• color_t red_green_blue[] = {

• { 1.0, 0.0, 0.0 },
  

• { 0.0, 1.0, 0.0 },
  

• { 0.0, 0.0, 1.0 }
  

• };
• igt_info("Plane gamma test is running on pipe-%s plane-%s(%s)\n",

• 	kmstest_pipe_name(plane->pipe->pipe),
  

• 	kmstest_plane_type_name(plane->type),
  

• 	is_hdr_plane(plane) ? "hdr":"sdr");
  

• igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_MODE));
• igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_LUT));
• pipe_crc = igt_pipe_crc_new(data->drm_fd,

• 		  plane->pipe->pipe,
  

• 		  INTEL_PIPE_CRC_SOURCE_AUTO);
  

• output = igt_get_single_output_for_pipe(display, plane->pipe->pipe);
• igt_assert(output);
• igt_output_set_pipe(output, plane->pipe->pipe);
• mode = igt_output_get_mode(output);
• /* Create a framebuffer at the size of the output. */
• igt_assert(igt_create_fb(data->drm_fd,

• 	      mode->hdisplay,
  

• 	      mode->vdisplay,
  

• 	      DRM_FORMAT_XRGB8888,
  

• 	      DRM_FORMAT_MOD_LINEAR,
  

• 	      &fb));
  

• igt_plane_set_fb(plane, &fb);
• /* Disable Pipe color props. */
• disable_ctm(plane->pipe);
• disable_degamma(plane->pipe);
• disable_gamma(plane->pipe);
• disable_plane_ctm(plane);
• disable_plane_degamma(plane);
• igt_display_commit2(display, display->is_atomic ?

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• gamma_mode = get_plane_gamma_degamma_mode(plane, IGT_PLANE_GAMMA_MODE);
• /* Iterate all supported gamma modes. */
• for (i = 0; i < gamma_mode->count_enums; i++) {

• igt_crc_t crc_gamma, crc_fullcolors;
  

• segment_data_t *segment_info = NULL;
  

• struct drm_color_lut_ext *lut = NULL;
  

• uint32_t lut_size = 0;
  

• /* Ignore 'no gamma' from enum list. */
  

• if (!strcmp(gamma_mode->enums[i].name, "no gamma"))
  

• 	continue;
  

• igt_info("Trying to use gamma mode: \'%s\'\n", gamma_mode-
  

enums[i].name);

• /* Draw solid colors with no gamma transformation. */
  

• disable_plane_gamma(plane);
  

• paint_rectangles(data, mode, red_green_blue, &fb);
  

• igt_plane_set_fb(plane, &fb);
  

• igt_display_commit2(display, display->is_atomic ?
  

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• igt_wait_for_vblank(data->drm_fd,
  

• 	display->pipes[plane->pipe->pipe].crtc_offset);
  

• igt_pipe_crc_collect_crc(pipe_crc, &crc_fullcolors);
  

• /* Draw gradient colors with gamma LUT to remap all
  

•  * values to max red/green/blue.
  

•  */
  

• segment_info = get_segment_data(data, gamma_mode->enums[i].value,
  

• 		gamma_mode->enums[i].name);
  

• lut_size = sizeof(struct drm_color_lut_ext) * segment_info-
  

entries_count;

• lut = create_max_lut(segment_info);
  

Using max LUT seems like a weak test. I recall seeing problem reports related to alpha blending where trying to display an alpha gradient essentially resulted in what max LUT would produce.

Due to the hardware roundups, we are getting crc mismatches for any LUTs. So, by default we can compare "gradient colors + max LUT" and "solid colors + No/unity LUT"

In extend mode (Please check above comments) Other h/w vendors can extend the support for different LUTs

- Bhanu

Thanks, pq

...

• set_plane_gamma(plane, gamma_mode->enums[i].name, lut, lut_size);
  

• paint_gradient_rectangles(data, mode, red_green_blue, &fb);
  

• igt_plane_set_fb(plane, &fb);
  

• igt_display_commit2(display, display->is_atomic ?
  

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• igt_wait_for_vblank(data->drm_fd,
  

• 	display->pipes[plane->pipe->pipe].crtc_offset);
  

• igt_pipe_crc_collect_crc(pipe_crc, &crc_gamma);
  

• /* Verify that the CRC of the software computed output is
  

•  * equal to the CRC of the gamma LUT transformation output.
  

•  */
  

• ret &= igt_check_crc_equal(&crc_gamma, &crc_fullcolors);
  

• free(lut);
  

• clear_segment_data(segment_info);
  

• }
• disable_plane_gamma(plane);
• igt_plane_set_fb(plane, NULL);
• igt_output_set_pipe(output, PIPE_NONE);
• igt_display_commit2(display, display->is_atomic ?

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• igt_pipe_crc_free(pipe_crc);
• drmModeFreeProperty(gamma_mode);
• return ret;

+}

static void prep_pipe(data_t *data, enum pipe p) { @@ -890,6 +1033,37 @@ run_invalid_tests_for_pipe(data_t *data, enum pipe p) invalid_ctm_matrix_sizes(data, p); }

+static void run_plane_color_test(data_t *data, enum pipe pipe, test_t test) +{

• igt_plane_t *plane;
• int count = 0;
• for_each_plane_on_pipe(&data->display, pipe, plane) {

• if (!is_valid_plane(plane))
  

• 	continue;
  

• igt_assert(test(data, plane));
  

• count++;
  

• }
• igt_require_f(count, "No valid planes found.\n");

+}

+static void run_tests_for_plane(data_t *data, enum pipe pipe) +{

• igt_fixture {

• igt_require_pipe(&data->display, pipe);
  

• igt_require_pipe_crc(data->drm_fd);
  

• igt_require(data->display.pipes[pipe].n_planes > 0);
  

• igt_display_require_output_on_pipe(&data->display, pipe);
  

• }
• igt_describe("Compare maxed out plane gamma LUT and solid color linear

LUT");

...

• igt_subtest_f("pipe-%s-plane-gamma", kmstest_pipe_name(pipe))

• run_plane_color_test(data, pipe, plane_gamma_test);
  

+}

igt_main { data_t data = {}; @@ -910,6 +1084,9 @@ igt_main

igt_subtest_group
	run_invalid_tests_for_pipe(&data, pipe);

• igt_subtest_group
  

• 	run_tests_for_plane(&data, pipe);
  

  }

  igt_fixture {

From: Harry Wentland harry.wentland@amd.com Sent: Friday, November 26, 2021 10:25 PM To: Modem, Bhanuprakash bhanuprakash.modem@intel.com; igt- dev@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Ville Syrjälä ville.syrjala@linux.intel.com; Juha-Pekka Heikkila juhapekka.heikkila@gmail.com; Shankar, Uma uma.shankar@intel.com Subject: Re: [i-g-t 04/14] tests/kms_color: New subtests for Plane gamma

On 2021-11-15 04:47, Bhanuprakash Modem wrote:

...

To verify Plane gamma, draw 3 gradient rectangles in red, green and blue, with a maxed out gamma LUT and verify we have the same CRC as drawing solid color rectangles.

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.c | 179 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 178 insertions(+), 1 deletion(-)

diff --git a/tests/kms_color.c b/tests/kms_color.c index 775f35964f..b45d66762f 100644 --- a/tests/kms_color.c +++ b/tests/kms_color.c @@ -24,7 +24,34 @@

#include "kms_color_helper.h"

-IGT_TEST_DESCRIPTION("Test Color Features at Pipe level"); +IGT_TEST_DESCRIPTION("Test Color Features at Pipe & Plane level");

+#define MAX_SUPPORTED_PLANES 7 +#define SDR_PLANE_BASE 3

+typedef bool (*test_t)(data_t*, igt_plane_t*);

+static bool is_hdr_plane(const igt_plane_t *plane) +{

• return plane->index >= 0 && plane->index < SDR_PLANE_BASE;

+}

+static bool is_valid_plane(igt_plane_t *plane) +{

• int index = plane->index;
• if (plane->type != DRM_PLANE_TYPE_PRIMARY)

• return false;
  

• /*

• * Test 1 HDR plane, 1 SDR plane.
  

• *
  

• * 0,1,2 HDR planes
  

• * 3,4,5,6 SDR planes
  

• *
  

• */
  

This seems to be about Intel HW. AMD HW for example does not have HDR or SDR planes, only one generic plane.

...

• return index >= 0 && index < MAX_SUPPORTED_PLANES;

+}

static void test_pipe_degamma(data_t *data, igt_plane_t *primary) @@ -638,6 +665,122 @@ static void test_pipe_limited_range_ctm(data_t *data, } #endif

+static bool plane_gamma_test(data_t *data, igt_plane_t *plane) +{

• igt_output_t *output;
• igt_display_t *display = &data->display;
• drmModeModeInfo *mode;
• struct igt_fb fb;
• drmModePropertyPtr gamma_mode = NULL;
• uint32_t i;
• bool ret = true;
• igt_pipe_crc_t *pipe_crc = NULL;
• color_t red_green_blue[] = {

• { 1.0, 0.0, 0.0 },
  

• { 0.0, 1.0, 0.0 },
  

• { 0.0, 0.0, 1.0 }
  

• };
• igt_info("Plane gamma test is running on pipe-%s plane-%s(%s)\n",

• 	kmstest_pipe_name(plane->pipe->pipe),
  

• 	kmstest_plane_type_name(plane->type),
  

• 	is_hdr_plane(plane) ? "hdr":"sdr");
  

is_hdr_plane is Intel-specific.

...

• igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_MODE));
• igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_LUT));
• pipe_crc = igt_pipe_crc_new(data->drm_fd,

• 		  plane->pipe->pipe,
  

• 		  INTEL_PIPE_CRC_SOURCE_AUTO);
  

• output = igt_get_single_output_for_pipe(display, plane->pipe->pipe);
• igt_assert(output);
• igt_output_set_pipe(output, plane->pipe->pipe);
• mode = igt_output_get_mode(output);
• /* Create a framebuffer at the size of the output. */
• igt_assert(igt_create_fb(data->drm_fd,

• 	      mode->hdisplay,
  

• 	      mode->vdisplay,
  

• 	      DRM_FORMAT_XRGB8888,
  

• 	      DRM_FORMAT_MOD_LINEAR,
  

• 	      &fb));
  

• igt_plane_set_fb(plane, &fb);
• /* Disable Pipe color props. */
• disable_ctm(plane->pipe);
• disable_degamma(plane->pipe);
• disable_gamma(plane->pipe);
• disable_plane_ctm(plane);
• disable_plane_degamma(plane);
• igt_display_commit2(display, display->is_atomic ?

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• gamma_mode = get_plane_gamma_degamma_mode(plane, IGT_PLANE_GAMMA_MODE);
• /* Iterate all supported gamma modes. */
• for (i = 0; i < gamma_mode->count_enums; i++) {

• igt_crc_t crc_gamma, crc_fullcolors;
  

• segment_data_t *segment_info = NULL;
  

• struct drm_color_lut_ext *lut = NULL;
  

• uint32_t lut_size = 0;
  

• /* Ignore 'no gamma' from enum list. */
  

• if (!strcmp(gamma_mode->enums[i].name, "no gamma"))
  

• 	continue;
  

It might still make sense to test that this is doing bypass.

As we know gamma_mode->enum[i].name represents the name of the gamma mode and gamma_mode->enum[i].value would be the LUT blob address of that particular gamma_mode.

For "no gamma" case the blob address is NULL, so we can't commit this mode. Hence skipping this mode.

...

• igt_info("Trying to use gamma mode: \'%s\'\n", gamma_mode-
  

enums[i].name);

• /* Draw solid colors with no gamma transformation. */
  

• disable_plane_gamma(plane);
  

• paint_rectangles(data, mode, red_green_blue, &fb);
  

• igt_plane_set_fb(plane, &fb);
  

• igt_display_commit2(display, display->is_atomic ?
  

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• igt_wait_for_vblank(data->drm_fd,
  

• 	display->pipes[plane->pipe->pipe].crtc_offset);
  

• igt_pipe_crc_collect_crc(pipe_crc, &crc_fullcolors);
  

• /* Draw gradient colors with gamma LUT to remap all
  

•  * values to max red/green/blue.
  

•  */
  

• segment_info = get_segment_data(data, gamma_mode->enums[i].value,
  

• 		gamma_mode->enums[i].name);
  

• lut_size = sizeof(struct drm_color_lut_ext) * segment_info-
  

entries_count;

• lut = create_max_lut(segment_info);
  

• set_plane_gamma(plane, gamma_mode->enums[i].name, lut, lut_size);
  

• paint_gradient_rectangles(data, mode, red_green_blue, &fb);
  

• igt_plane_set_fb(plane, &fb);
  

• igt_display_commit2(display, display->is_atomic ?
  

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• igt_wait_for_vblank(data->drm_fd,
  

• 	display->pipes[plane->pipe->pipe].crtc_offset);
  

• igt_pipe_crc_collect_crc(pipe_crc, &crc_gamma);
  

• /* Verify that the CRC of the software computed output is
  

•  * equal to the CRC of the gamma LUT transformation output.
  

•  */
  

• ret &= igt_check_crc_equal(&crc_gamma, &crc_fullcolors);
  

• free(lut);
  

• clear_segment_data(segment_info);
  

• }
• disable_plane_gamma(plane);
• igt_plane_set_fb(plane, NULL);
• igt_output_set_pipe(output, PIPE_NONE);
• igt_display_commit2(display, display->is_atomic ?

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• igt_pipe_crc_free(pipe_crc);
• drmModeFreeProperty(gamma_mode);
• return ret;

+}

static void prep_pipe(data_t *data, enum pipe p) { @@ -890,6 +1033,37 @@ run_invalid_tests_for_pipe(data_t *data, enum pipe p) invalid_ctm_matrix_sizes(data, p); }

+static void run_plane_color_test(data_t *data, enum pipe pipe, test_t test) +{

• igt_plane_t *plane;
• int count = 0;
• for_each_plane_on_pipe(&data->display, pipe, plane) {

• if (!is_valid_plane(plane))
  

• 	continue;
  

• igt_assert(test(data, plane));
  

• count++;
  

• }
• igt_require_f(count, "No valid planes found.\n");

+}

+static void run_tests_for_plane(data_t *data, enum pipe pipe) +{

• igt_fixture {

• igt_require_pipe(&data->display, pipe);
  

• igt_require_pipe_crc(data->drm_fd);
  

• igt_require(data->display.pipes[pipe].n_planes > 0);
  

• igt_display_require_output_on_pipe(&data->display, pipe);
  

• }
• igt_describe("Compare maxed out plane gamma LUT and solid color linear

LUT");

I can't seem to see the linear LUT test. This only seems to test the max LUT.

Harry

...

• igt_subtest_f("pipe-%s-plane-gamma", kmstest_pipe_name(pipe))

• run_plane_color_test(data, pipe, plane_gamma_test);
  

+}

igt_main { data_t data = {}; @@ -910,6 +1084,9 @@ igt_main

igt_subtest_group
	run_invalid_tests_for_pipe(&data, pipe);

• igt_subtest_group
  

• 	run_tests_for_plane(&data, pipe);
  

  }

  igt_fixture {

From: Harry Wentland harry.wentland@amd.com Sent: Wednesday, January 5, 2022 2:49 AM To: Modem, Bhanuprakash bhanuprakash.modem@intel.com; igt- dev@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Ville Syrjälä ville.syrjala@linux.intel.com; Shankar, Uma uma.shankar@intel.com; ppaalanen@gmail.com Subject: Re: [i-g-t 04/14] tests/kms_color: New subtests for Plane gamma

On 2022-01-02 23:05, Modem, Bhanuprakash wrote:

...

...

From: Harry Wentland harry.wentland@amd.com Sent: Friday, November 26, 2021 10:25 PM To: Modem, Bhanuprakash bhanuprakash.modem@intel.com; igt- dev@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Ville Syrjälä ville.syrjala@linux.intel.com; Juha-Pekka Heikkila juhapekka.heikkila@gmail.com; Shankar, Uma uma.shankar@intel.com Subject: Re: [i-g-t 04/14] tests/kms_color: New subtests for Plane gamma

On 2021-11-15 04:47, Bhanuprakash Modem wrote:

...

To verify Plane gamma, draw 3 gradient rectangles in red, green and blue, with a maxed out gamma LUT and verify we have the same CRC as drawing

solid

...

...

...

color rectangles.

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.c | 179 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 178 insertions(+), 1 deletion(-)

diff --git a/tests/kms_color.c b/tests/kms_color.c index 775f35964f..b45d66762f 100644 --- a/tests/kms_color.c +++ b/tests/kms_color.c @@ -24,7 +24,34 @@

#include "kms_color_helper.h"

-IGT_TEST_DESCRIPTION("Test Color Features at Pipe level"); +IGT_TEST_DESCRIPTION("Test Color Features at Pipe & Plane level");

+#define MAX_SUPPORTED_PLANES 7 +#define SDR_PLANE_BASE 3

+typedef bool (*test_t)(data_t*, igt_plane_t*);

+static bool is_hdr_plane(const igt_plane_t *plane) +{

• return plane->index >= 0 && plane->index < SDR_PLANE_BASE;

+}

+static bool is_valid_plane(igt_plane_t *plane) +{

• int index = plane->index;
• if (plane->type != DRM_PLANE_TYPE_PRIMARY)

• return false;
  

• /*

• * Test 1 HDR plane, 1 SDR plane.
  

• *
  

• * 0,1,2 HDR planes
  

• * 3,4,5,6 SDR planes
  

• *
  

• */
  

This seems to be about Intel HW. AMD HW for example does not have HDR or SDR planes, only one generic plane.

...

• return index >= 0 && index < MAX_SUPPORTED_PLANES;

+}

static void test_pipe_degamma(data_t *data, igt_plane_t *primary) @@ -638,6 +665,122 @@ static void test_pipe_limited_range_ctm(data_t

*data,

...

...

...

} #endif

+static bool plane_gamma_test(data_t *data, igt_plane_t *plane) +{

• igt_output_t *output;
• igt_display_t *display = &data->display;
• drmModeModeInfo *mode;
• struct igt_fb fb;
• drmModePropertyPtr gamma_mode = NULL;
• uint32_t i;
• bool ret = true;
• igt_pipe_crc_t *pipe_crc = NULL;
• color_t red_green_blue[] = {

• { 1.0, 0.0, 0.0 },
  

• { 0.0, 1.0, 0.0 },
  

• { 0.0, 0.0, 1.0 }
  

• };
• igt_info("Plane gamma test is running on pipe-%s plane-%s(%s)\n",

• 	kmstest_pipe_name(plane->pipe->pipe),
  

• 	kmstest_plane_type_name(plane->type),
  

• 	is_hdr_plane(plane) ? "hdr":"sdr");
  

is_hdr_plane is Intel-specific.

...

• igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_MODE));
• igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_LUT));
• pipe_crc = igt_pipe_crc_new(data->drm_fd,

• 		  plane->pipe->pipe,
  

• 		  INTEL_PIPE_CRC_SOURCE_AUTO);
  

• output = igt_get_single_output_for_pipe(display, plane->pipe->pipe);
• igt_assert(output);
• igt_output_set_pipe(output, plane->pipe->pipe);
• mode = igt_output_get_mode(output);
• /* Create a framebuffer at the size of the output. */
• igt_assert(igt_create_fb(data->drm_fd,

• 	      mode->hdisplay,
  

• 	      mode->vdisplay,
  

• 	      DRM_FORMAT_XRGB8888,
  

• 	      DRM_FORMAT_MOD_LINEAR,
  

• 	      &fb));
  

• igt_plane_set_fb(plane, &fb);
• /* Disable Pipe color props. */
• disable_ctm(plane->pipe);
• disable_degamma(plane->pipe);
• disable_gamma(plane->pipe);
• disable_plane_ctm(plane);
• disable_plane_degamma(plane);
• igt_display_commit2(display, display->is_atomic ?

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• gamma_mode = get_plane_gamma_degamma_mode(plane, IGT_PLANE_GAMMA_MODE);
• /* Iterate all supported gamma modes. */
• for (i = 0; i < gamma_mode->count_enums; i++) {

• igt_crc_t crc_gamma, crc_fullcolors;
  

• segment_data_t *segment_info = NULL;
  

• struct drm_color_lut_ext *lut = NULL;
  

• uint32_t lut_size = 0;
  

• /* Ignore 'no gamma' from enum list. */
  

• if (!strcmp(gamma_mode->enums[i].name, "no gamma"))
  

• 	continue;
  

It might still make sense to test that this is doing bypass.

As we know gamma_mode->enum[i].name represents the name of the gamma mode and gamma_mode->enum[i].value would be the LUT blob address of that particular gamma_mode.

For "no gamma" case the blob address is NULL, so we can't commit this mode. Hence skipping this mode.

I was thinking it'd be good to test the "no gamma" case as well here, i.e. the case were we set a NULL blob. I'm not sure what you mean when you say "we can't commit this mode."

Sorry for the confusion, "no gamma" is intentional to disable the gamma. I think, we just need to check that we can able to flip with this mode. So, we need to update disable_plane_gamma() to use this mode.

Or are you thinking any specific usecase for this?

- Bhanu

Harry

...

...

...

• igt_info("Trying to use gamma mode: \'%s\'\n", gamma_mode-
  

enums[i].name);

• /* Draw solid colors with no gamma transformation. */
  

• disable_plane_gamma(plane);
  

• paint_rectangles(data, mode, red_green_blue, &fb);
  

• igt_plane_set_fb(plane, &fb);
  

• igt_display_commit2(display, display->is_atomic ?
  

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• igt_wait_for_vblank(data->drm_fd,
  

• 	display->pipes[plane->pipe->pipe].crtc_offset);
  

• igt_pipe_crc_collect_crc(pipe_crc, &crc_fullcolors);
  

• /* Draw gradient colors with gamma LUT to remap all
  

•  * values to max red/green/blue.
  

•  */
  

• segment_info = get_segment_data(data, gamma_mode->enums[i].value,
  

• 		gamma_mode->enums[i].name);
  

• lut_size = sizeof(struct drm_color_lut_ext) * segment_info-
  

entries_count;

• lut = create_max_lut(segment_info);
  

• set_plane_gamma(plane, gamma_mode->enums[i].name, lut, lut_size);
  

• paint_gradient_rectangles(data, mode, red_green_blue, &fb);
  

• igt_plane_set_fb(plane, &fb);
  

• igt_display_commit2(display, display->is_atomic ?
  

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• igt_wait_for_vblank(data->drm_fd,
  

• 	display->pipes[plane->pipe->pipe].crtc_offset);
  

• igt_pipe_crc_collect_crc(pipe_crc, &crc_gamma);
  

• /* Verify that the CRC of the software computed output is
  

•  * equal to the CRC of the gamma LUT transformation output.
  

•  */
  

• ret &= igt_check_crc_equal(&crc_gamma, &crc_fullcolors);
  

• free(lut);
  

• clear_segment_data(segment_info);
  

• }
• disable_plane_gamma(plane);
• igt_plane_set_fb(plane, NULL);
• igt_output_set_pipe(output, PIPE_NONE);
• igt_display_commit2(display, display->is_atomic ?

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• igt_pipe_crc_free(pipe_crc);
• drmModeFreeProperty(gamma_mode);
• return ret;

+}

static void prep_pipe(data_t *data, enum pipe p) { @@ -890,6 +1033,37 @@ run_invalid_tests_for_pipe(data_t *data, enum pipe

p)

...

...

...

invalid_ctm_matrix_sizes(data, p);

}

+static void run_plane_color_test(data_t *data, enum pipe pipe, test_t

test)

...

...

...

+{

• igt_plane_t *plane;
• int count = 0;
• for_each_plane_on_pipe(&data->display, pipe, plane) {

• if (!is_valid_plane(plane))
  

• 	continue;
  

• igt_assert(test(data, plane));
  

• count++;
  

• }
• igt_require_f(count, "No valid planes found.\n");

+}

+static void run_tests_for_plane(data_t *data, enum pipe pipe) +{

• igt_fixture {

• igt_require_pipe(&data->display, pipe);
  

• igt_require_pipe_crc(data->drm_fd);
  

• igt_require(data->display.pipes[pipe].n_planes > 0);
  

• igt_display_require_output_on_pipe(&data->display, pipe);
  

• }
• igt_describe("Compare maxed out plane gamma LUT and solid color linear

LUT");

I can't seem to see the linear LUT test. This only seems to test the max LUT.

Harry

...

• igt_subtest_f("pipe-%s-plane-gamma", kmstest_pipe_name(pipe))

• run_plane_color_test(data, pipe, plane_gamma_test);
  

+}

igt_main { data_t data = {}; @@ -910,6 +1084,9 @@ igt_main

igt_subtest_group
	run_invalid_tests_for_pipe(&data, pipe);

• igt_subtest_group
  

• 	run_tests_for_plane(&data, pipe);
  

  }

  igt_fixture {

On 2021-11-15 04:47, Bhanuprakash Modem wrote:

To verify plane CTM, draw 3 rectangles using before colors with the ctm matrix applied and verify the CRC is equal to using after colors with an identify ctm matrix.

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.c | 225 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 225 insertions(+)

diff --git a/tests/kms_color.c b/tests/kms_color.c index 920a5eaadd..e14b37cb6f 100644 --- a/tests/kms_color.c +++ b/tests/kms_color.c @@ -53,6 +53,77 @@ static bool is_valid_plane(igt_plane_t *plane) return index >= 0 && index < MAX_SUPPORTED_PLANES; }

+struct {

• const char *test_name;
• int iter;
• color_t expected_colors[3];
• double ctm[9];

+} ctm_tests[] = {

• {"plane-ctm-red-to-blue", 0,

• 			{{ 0.0, 0.0, 1.0 },
  

• 			 { 0.0, 1.0, 0.0 },
  

• 			 { 0.0, 0.0, 1.0 }},
  

• { 0.0, 0.0, 0.0,
  

•   0.0, 1.0, 0.0,
  

•   1.0, 0.0, 1.0 },
  

• },
• {"plane-ctm-green-to-red", 0,

• 			{{ 1.0, 0.0, 0.0 },
  

• 			 { 1.0, 0.0, 0.0 },
  

• 			 { 0.0, 0.0, 1.0 }},
  

• { 1.0, 1.0, 0.0,
  

•   0.0, 0.0, 0.0,
  

•   0.0, 0.0, 1.0 },
  

• },
• {"plane-ctm-blue-to-red", 0,

• 			{{ 1.0, 0.0, 0.0 },
  

• 			 { 0.0, 1.0, 0.0 },
  

• 			 { 1.0, 0.0, 0.0 }},
  

• { 1.0, 0.0, 1.0,
  

•   0.0, 1.0, 0.0,
  

•   0.0, 0.0, 0.0 },
  

• },
• {"plane-ctm-max", 0,

• 			{{ 1.0, 0.0, 0.0 },
  

• 			 { 0.0, 1.0, 0.0 },
  

• 			 { 0.0, 0.0, 1.0 }},
  

• { 100.0, 0.0, 0.0,
  

•   0.0, 100.0, 0.0,
  

•   0.0, 0.0, 100.0 },
  

• },
• {"plane-ctm-negative", 0,

• 			{{ 0.0, 0.0, 0.0 },
  

• 			 { 0.0, 0.0, 0.0 },
  

• 			 { 0.0, 0.0, 0.0 }},
  

• { -1.0, 0.0, 0.0,
  

•    0.0, -1.0, 0.0,
  

•    0.0, 0.0, -1.0 },
  

• },
• /* We tests a few values around the expected result because

• * it depends on the hardware we're dealing with, we can
  

• * either get clamped or rounded values and we also need to
  

• * account for odd number of items in the LUTs.
  

• */
  

• {"plane-ctm-0-25", 5,

• 			{{ 0.0, }, { 0.0, }, { 0.0, }},
  

• { 0.25, 0.0,  0.0,
  

•   0.0,  0.25, 0.0,
  

•   0.0,  0.0,  0.25 },
  

• },
• {"plane-ctm-0-50", 5,

• 			{{ 0.0, }, { 0.0, }, { 0.0, }},
  

• { 0.5,  0.0,  0.0,
  

•   0.0,  0.5,  0.0,
  

•   0.0,  0.0,  0.5 },
  

• },
• {"plane-ctm-0-75", 7,

• 			{{ 0.0, }, { 0.0, }, { 0.0, }},
  

• { 0.75, 0.0,  0.0,
  

•   0.0,  0.75, 0.0,
  

•   0.0,  0.0,  0.75 },
  

• },

+};

static void test_pipe_degamma(data_t *data, igt_plane_t *primary) { @@ -900,6 +971,99 @@ static bool plane_degamma_test(data_t *data, igt_plane_t *plane) return ret; }

+static bool test_plane_ctm(data_t *data,

• 	  igt_plane_t *plane,
  

• 	  color_t *before,
  

• 	  color_t *after,
  

• 	  double *ctm_matrix)
  

+{

• const double ctm_identity[] = {

• 1.0, 0.0, 0.0,
  

• 0.0, 1.0, 0.0,
  

• 0.0, 0.0, 1.0
  

• };
• igt_output_t *output;
• igt_display_t *display = &data->display;
• drmModeModeInfo *mode;
• struct igt_fb fb;
• igt_crc_t crc_software, crc_hardware;
• igt_pipe_crc_t *pipe_crc = NULL;
• bool ret = true;
• igt_info("Plane CTM test is running on pipe-%s plane-%s(%s)\n",

• 	kmstest_pipe_name(plane->pipe->pipe),
  

• 	kmstest_plane_type_name(plane->type),
  

• 	is_hdr_plane(plane) ? "hdr":"sdr");
  

• igt_require(igt_plane_has_prop(plane, IGT_PLANE_CTM));
• pipe_crc = igt_pipe_crc_new(data->drm_fd,

• 		  plane->pipe->pipe,
  

• 		  INTEL_PIPE_CRC_SOURCE_AUTO);
  

• output = igt_get_single_output_for_pipe(display, plane->pipe->pipe);
• igt_assert(output);
• igt_output_set_pipe(output, plane->pipe->pipe);
• mode = igt_output_get_mode(output);
• /* Create a framebuffer at the size of the output. */
• igt_assert(igt_create_fb(data->drm_fd,

• 	      mode->hdisplay,
  

• 	      mode->vdisplay,
  

• 	      DRM_FORMAT_XRGB8888,
  

• 	      DRM_FORMAT_MOD_LINEAR,
  

• 	      &fb));
  

• igt_plane_set_fb(plane, &fb);
• /* Disable Pipe color props. */
• disable_degamma(plane->pipe);
• disable_gamma(plane->pipe);
• disable_ctm(plane->pipe);
• disable_plane_gamma(plane);
• disable_plane_degamma(plane);
• igt_display_commit2(display, display->is_atomic ?

• 		COMMIT_ATOMIC : COMMIT_LEGACY);
  

• /* Without CTM transformation. */
• paint_rectangles(data, mode, after, &fb);
• igt_plane_set_fb(plane, &fb);
• set_plane_ctm(plane, ctm_identity);
• igt_display_commit2(display, display->is_atomic ?

• 		COMMIT_ATOMIC : COMMIT_LEGACY);
  

• igt_wait_for_vblank(data->drm_fd,

• 	display->pipes[plane->pipe->pipe].crtc_offset);
  

• igt_pipe_crc_collect_crc(pipe_crc, &crc_software);
• /* With CTM transformation. */
• paint_rectangles(data, mode, before, &fb);
• igt_plane_set_fb(plane, &fb);
• set_plane_ctm(plane, ctm_matrix);
• igt_display_commit2(display, display->is_atomic ?

• 		COMMIT_ATOMIC : COMMIT_LEGACY);
  

• igt_wait_for_vblank(data->drm_fd,

• 	display->pipes[plane->pipe->pipe].crtc_offset);
  

• igt_pipe_crc_collect_crc(pipe_crc, &crc_hardware);
• /* Verify that the CRC of the software computed ouutput

• * is equal to the CRC of the CTM matrix transformation
  

• * output.
  

• */
  

• ret = igt_check_crc_equal(&crc_software, &crc_hardware);
• disable_plane_ctm(plane);
• igt_plane_set_fb(plane, NULL);
• igt_output_set_pipe(output, PIPE_NONE);
• igt_display_commit2(display, display->is_atomic ?

• 			COMMIT_ATOMIC : COMMIT_LEGACY);
  

• igt_pipe_crc_free(pipe_crc);
• return ret;

+}

static void prep_pipe(data_t *data, enum pipe p) { @@ -1169,8 +1333,57 @@ static void run_plane_color_test(data_t *data, enum pipe pipe, test_t test) igt_require_f(count, "No valid planes found.\n"); }

+static void run_plane_ctm_test(data_t *data,

• 		enum pipe pipe,
  

• 		color_t *expected,
  

• 		double *ctm,
  

• 		int iter)
  

+{

• igt_plane_t *plane;
• bool result;
• int i, count = 0;
• double delta = 1.0 / (1 << 8);
• color_t red_green_blue[] = {

• { 1.0, 0.0, 0.0 },
  

• { 0.0, 1.0, 0.0 },
  

• { 0.0, 0.0, 1.0 }
  

• };
• for_each_plane_on_pipe(&data->display, pipe, plane) {

• if (!is_valid_plane(plane))
  

• 	continue;
  

• result = false;
  

• if (!iter)
  

• 	result |= test_plane_ctm(data, plane,
  

• 			red_green_blue, expected,
  

• 			ctm);
  

• for (i = 0; i < iter; i++) {
  

• 	expected[0].r =
  

• 	expected[1].g =
  

• 	expected[2].b =
  

• 		ctm[0] + delta * (i - (iter/2));
  

This bracketing likely depends closely on HW. I am curious if we can get this to work on various HW.

Harry

• 	result |= test_plane_ctm(data, plane,
  

• 			red_green_blue,	expected,
  

• 			ctm);
  

• 	if (result)
  

• 		break;
  

• }
  

• igt_assert(result);
  

• count++;
  

• }
• igt_require_f(count, "No valid planes found.\n");

+}

static void run_tests_for_plane(data_t *data, enum pipe pipe) {

• int i;
• igt_fixture { igt_require_pipe(&data->display, pipe); igt_require_pipe_crc(data->drm_fd);

@@ -1186,6 +1399,18 @@ static void run_tests_for_plane(data_t *data, enum pipe pipe) igt_subtest_f("pipe-%s-plane-degamma", kmstest_pipe_name(pipe)) run_plane_color_test(data, pipe, plane_degamma_test);

• for (i = 0; i < ARRAY_SIZE(ctm_tests); i++) {

• igt_describe("Compare after applying ctm matrix & identity matrix");
  

• igt_subtest_f("pipe-%s-%s",
  

• 		kmstest_pipe_name(pipe),
  

• 		ctm_tests[i].test_name) {
  

• 	run_plane_ctm_test(data, pipe,
  

• 			ctm_tests[i].expected_colors,
  

• 			ctm_tests[i].ctm,
  

• 			ctm_tests[i].iter);
  

• }
  

• }

}

igt_main

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

Negative check for:

• plane gamma lut sizes
• plane degamma lut sizes
• plane ctm matrix sizes

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.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+)

diff --git a/tests/kms_color.c b/tests/kms_color.c index e14b37cb6f..d9fe417ba9 100644 --- a/tests/kms_color.c +++ b/tests/kms_color.c @@ -736,6 +736,118 @@ static void test_pipe_limited_range_ctm(data_t *data, } #endif

+static bool invalid_plane_gamma_test(data_t *data, igt_plane_t *plane) +{

• igt_display_t *display = &data->display;
• drmModePropertyPtr gamma_mode = NULL;
• uint32_t i;
• igt_info("Plane invalid gamma test is running on pipe-%s plane-%s(%s)\n",

• 	kmstest_pipe_name(plane->pipe->pipe),
  

• 	kmstest_plane_type_name(plane->type),
  

• 	is_hdr_plane(plane) ? "hdr":"sdr");
  

• igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_MODE));
• igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_LUT));
• gamma_mode = get_plane_gamma_degamma_mode(plane, IGT_PLANE_GAMMA_MODE);
• /* Iterate all supported gamma modes. */
• for (i = 0; i < gamma_mode->count_enums; i++) {

• segment_data_t *segment_info = NULL;
  

• size_t lut_size = 0;
  

• /* Ignore 'no gamma' from enum list. */
  

• if (!strcmp(gamma_mode->enums[i].name, "no gamma"))
  

• 	continue;
  

• igt_info("Trying to use gamma mode: \'%s\'\n", gamma_mode->enums[i].name);
  

• segment_info = get_segment_data(data, gamma_mode->enums[i].value,
  

• 		gamma_mode->enums[i].name);
  

• lut_size = sizeof(struct drm_color_lut_ext) * segment_info->entries_count;
  

• igt_plane_set_prop_enum(plane, IGT_PLANE_GAMMA_MODE, gamma_mode->enums[i].name);
  

• invalid_plane_lut_sizes(display, plane,
  

• 			IGT_PLANE_GAMMA_LUT,
  

• 			lut_size);
  

• clear_segment_data(segment_info);
  

• /* One enum is enough. */
  

• break;
  

• }
• drmModeFreeProperty(gamma_mode);
• return true;

+}

+static bool invalid_plane_degamma_test(data_t *data, igt_plane_t *plane) +{

• igt_display_t *display = &data->display;
• drmModePropertyPtr degamma_mode = NULL;
• uint32_t i;
• igt_info("Plane invalid degamma test is running on pipe-%s plane-%s(%s)\n",

• 	kmstest_pipe_name(plane->pipe->pipe),
  

• 	kmstest_plane_type_name(plane->type),
  

• 	is_hdr_plane(plane) ? "hdr":"sdr");
  

• igt_require(igt_plane_has_prop(plane, IGT_PLANE_DEGAMMA_MODE));
• igt_require(igt_plane_has_prop(plane, IGT_PLANE_DEGAMMA_LUT));
• degamma_mode = get_plane_gamma_degamma_mode(plane, IGT_PLANE_DEGAMMA_MODE);
• /* Iterate all supported degamma modes. */
• for (i = 0; i < degamma_mode->count_enums; i++) {

• segment_data_t *segment_info = NULL;
  

• size_t lut_size = 0;
  

• /* Ignore 'no degamma' from enum list. */
  

• if (!strcmp(degamma_mode->enums[i].name, "no degamma"))
  

• 	continue;
  

• igt_info("Trying to use degamma mode: \'%s\'\n", degamma_mode->enums[i].name);
  

• segment_info = get_segment_data(data,
  

• 				degamma_mode->enums[i].value,
  

• 				degamma_mode->enums[i].name);
  

• lut_size = sizeof(struct drm_color_lut_ext) * segment_info->entries_count * 2;
  

• igt_plane_set_prop_enum(plane,
  

• 			IGT_PLANE_DEGAMMA_MODE,
  

• 			degamma_mode->enums[i].name);
  

• invalid_plane_lut_sizes(display, plane,
  

• 			IGT_PLANE_DEGAMMA_LUT,
  

• 			lut_size);
  

• clear_segment_data(segment_info);
  

• /* One enum is enough. */
  

• break;
  

Why is one enum enough?

The same question for the other case in this patch.

• }
• drmModeFreeProperty(degamma_mode);
• return true;

+}

+static bool invalid_plane_ctm_test(data_t *data, igt_plane_t *plane) +{

• igt_info("Plane invalid CTM test is running on pipe-%s plane-%s(%s)\n",

• 	kmstest_pipe_name(plane->pipe->pipe),
  

• 	kmstest_plane_type_name(plane->type),
  

• 	is_hdr_plane(plane) ? "hdr":"sdr");
  

• igt_require(igt_plane_has_prop(plane, IGT_PLANE_CTM));
• invalid_plane_lut_sizes(&data->display, plane,

• 		IGT_PLANE_CTM,
  

• 		sizeof(struct drm_color_ctm));
  

The code says you're trying shove a LUT into a CTM blob. I understand that mechanically this is test you want to do, feed a wrong sized blob, and in this case the contents do not matter (unlike with actual LUTs), but reading this code is completely misleading and does not make sense. It takes a while to think about what you actually want to test here, and then reverse-engineer the code to understand that that is what actually happens, too. That is too much mental burden for the reader to realize that this piece of code actually works.

Thanks, pq

• return true;

+}

static bool plane_gamma_test(data_t *data, igt_plane_t *plane) { igt_output_t *output; @@ -1411,6 +1523,21 @@ static void run_tests_for_plane(data_t *data, enum pipe pipe) ctm_tests[i].iter); } }

• igt_describe("Negative check for invalid plane gamma lut sizes");
• igt_subtest_f("pipe-%s-invalid-plane-gamma-lut-sizes",

• 	kmstest_pipe_name(pipe))
  

• run_plane_color_test(data, pipe, invalid_plane_gamma_test);
  

• igt_describe("Negative check for invalid plane degamma lut sizes");
• igt_subtest_f("pipe-%s-invalid-plane-degamma-lut-sizes",

• 	kmstest_pipe_name(pipe))
  

• run_plane_color_test(data, pipe, invalid_plane_degamma_test);
  

• igt_describe("Negative check for invalid plane ctm matrix sizes");
• igt_subtest_f("pipe-%s-invalid-plane-ctm-matrix-sizes",

• 	kmstest_pipe_name(pipe))
  

• run_plane_color_test(data, pipe, invalid_plane_ctm_test);
  

}

igt_main

From: Pekka Paalanen ppaalanen@gmail.com Sent: Thursday, November 18, 2021 2:50 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 07/14] tests/kms_color: New negative tests for plane level color mgmt

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

...

Negative check for:

• plane gamma lut sizes
• plane degamma lut sizes
• plane ctm matrix sizes

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.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+)

diff --git a/tests/kms_color.c b/tests/kms_color.c index e14b37cb6f..d9fe417ba9 100644 --- a/tests/kms_color.c +++ b/tests/kms_color.c @@ -736,6 +736,118 @@ static void test_pipe_limited_range_ctm(data_t *data, } #endif

+static bool invalid_plane_gamma_test(data_t *data, igt_plane_t *plane) +{

• igt_display_t *display = &data->display;
• drmModePropertyPtr gamma_mode = NULL;
• uint32_t i;
• igt_info("Plane invalid gamma test is running on pipe-%s plane-

%s(%s)\n",

...

• 	kmstest_pipe_name(plane->pipe->pipe),
  

• 	kmstest_plane_type_name(plane->type),
  

• 	is_hdr_plane(plane) ? "hdr":"sdr");
  

• igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_MODE));
• igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_LUT));
• gamma_mode = get_plane_gamma_degamma_mode(plane, IGT_PLANE_GAMMA_MODE);
• /* Iterate all supported gamma modes. */
• for (i = 0; i < gamma_mode->count_enums; i++) {

• segment_data_t *segment_info = NULL;
  

• size_t lut_size = 0;
  

• /* Ignore 'no gamma' from enum list. */
  

• if (!strcmp(gamma_mode->enums[i].name, "no gamma"))
  

• 	continue;
  

• igt_info("Trying to use gamma mode: \'%s\'\n", gamma_mode-
  

enums[i].name);

• segment_info = get_segment_data(data, gamma_mode->enums[i].value,
  

• 		gamma_mode->enums[i].name);
  

• lut_size = sizeof(struct drm_color_lut_ext) * segment_info-
  

entries_count;

• igt_plane_set_prop_enum(plane, IGT_PLANE_GAMMA_MODE, gamma_mode-
  

enums[i].name);

• invalid_plane_lut_sizes(display, plane,
  

• 			IGT_PLANE_GAMMA_LUT,
  

• 			lut_size);
  

• clear_segment_data(segment_info);
  

• /* One enum is enough. */
  

• break;
  

• }
• drmModeFreeProperty(gamma_mode);
• return true;

+}

+static bool invalid_plane_degamma_test(data_t *data, igt_plane_t *plane) +{

• igt_display_t *display = &data->display;
• drmModePropertyPtr degamma_mode = NULL;
• uint32_t i;
• igt_info("Plane invalid degamma test is running on pipe-%s plane-

%s(%s)\n",

...

• 	kmstest_pipe_name(plane->pipe->pipe),
  

• 	kmstest_plane_type_name(plane->type),
  

• 	is_hdr_plane(plane) ? "hdr":"sdr");
  

• igt_require(igt_plane_has_prop(plane, IGT_PLANE_DEGAMMA_MODE));
• igt_require(igt_plane_has_prop(plane, IGT_PLANE_DEGAMMA_LUT));
• degamma_mode = get_plane_gamma_degamma_mode(plane,

IGT_PLANE_DEGAMMA_MODE);

...

• /* Iterate all supported degamma modes. */
• for (i = 0; i < degamma_mode->count_enums; i++) {

• segment_data_t *segment_info = NULL;
  

• size_t lut_size = 0;
  

• /* Ignore 'no degamma' from enum list. */
  

• if (!strcmp(degamma_mode->enums[i].name, "no degamma"))
  

• 	continue;
  

• igt_info("Trying to use degamma mode: \'%s\'\n", degamma_mode-
  

enums[i].name);

• segment_info = get_segment_data(data,
  

• 				degamma_mode->enums[i].value,
  

• 				degamma_mode->enums[i].name);
  

• lut_size = sizeof(struct drm_color_lut_ext) * segment_info-
  

entries_count * 2;

• igt_plane_set_prop_enum(plane,
  

• 			IGT_PLANE_DEGAMMA_MODE,
  

• 			degamma_mode->enums[i].name);
  

• invalid_plane_lut_sizes(display, plane,
  

• 			IGT_PLANE_DEGAMMA_LUT,
  

• 			lut_size);
  

• clear_segment_data(segment_info);
  

• /* One enum is enough. */
  

• break;
  

Why is one enum enough?

The same question for the other case in this patch.

This is just for CI time optimization, seems we don't save much CI time I'll remove this & float a new rev.

...

• }
• drmModeFreeProperty(degamma_mode);
• return true;

+}

+static bool invalid_plane_ctm_test(data_t *data, igt_plane_t *plane) +{

• igt_info("Plane invalid CTM test is running on pipe-%s plane-%s(%s)\n",

• 	kmstest_pipe_name(plane->pipe->pipe),
  

• 	kmstest_plane_type_name(plane->type),
  

• 	is_hdr_plane(plane) ? "hdr":"sdr");
  

• igt_require(igt_plane_has_prop(plane, IGT_PLANE_CTM));
• invalid_plane_lut_sizes(&data->display, plane,

• 		IGT_PLANE_CTM,
  

• 		sizeof(struct drm_color_ctm));
  

The code says you're trying shove a LUT into a CTM blob. I understand that mechanically this is test you want to do, feed a wrong sized blob, and in this case the contents do not matter (unlike with actual LUTs), but reading this code is completely misleading and does not make sense. It takes a while to think about what you actually want to test here, and then reverse-engineer the code to understand that that is what actually happens, too. That is too much mental burden for the reader to realize that this piece of code actually works.

Sorry for the poor documentation, I'll try to add some comments.

- Bhanu

Thanks, pq

...

• return true;

+}

static bool plane_gamma_test(data_t *data, igt_plane_t *plane) { igt_output_t *output; @@ -1411,6 +1523,21 @@ static void run_tests_for_plane(data_t *data, enum

pipe pipe)

...

			ctm_tests[i].iter);
}

}

• igt_describe("Negative check for invalid plane gamma lut sizes");
• igt_subtest_f("pipe-%s-invalid-plane-gamma-lut-sizes",

• 	kmstest_pipe_name(pipe))
  

• run_plane_color_test(data, pipe, invalid_plane_gamma_test);
  

• igt_describe("Negative check for invalid plane degamma lut sizes");
• igt_subtest_f("pipe-%s-invalid-plane-degamma-lut-sizes",

• 	kmstest_pipe_name(pipe))
  

• run_plane_color_test(data, pipe, invalid_plane_degamma_test);
  

• igt_describe("Negative check for invalid plane ctm matrix sizes");
• igt_subtest_f("pipe-%s-invalid-plane-ctm-matrix-sizes",

• 	kmstest_pipe_name(pipe))
  

• run_plane_color_test(data, pipe, invalid_plane_ctm_test);
  

}

igt_main

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

To verify Plane gamma, draw 3 gradient rectangles in red, green and blue, with a maxed out gamma LUT and verify we have the same frame dump as drawing solid color rectangles.

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 Cc: Kunal Joshi kunal1.joshi@intel.com Signed-off-by: Bhanuprakash Modem bhanuprakash.modem@intel.com

---

tests/kms_color_chamelium.c | 188 +++++++++++++++++++++++++++++++++++- 1 file changed, 187 insertions(+), 1 deletion(-)

diff --git a/tests/kms_color_chamelium.c b/tests/kms_color_chamelium.c index 76f82d6d35..b506109271 100644 --- a/tests/kms_color_chamelium.c +++ b/tests/kms_color_chamelium.c @@ -24,7 +24,34 @@

#include "kms_color_helper.h"

-IGT_TEST_DESCRIPTION("Test Color Features at Pipe level using Chamelium to verify instead of CRC"); +IGT_TEST_DESCRIPTION("Test Color Features at Pipe & Plane level using Chamelium to verify instead of CRC");

Now that you actually can get a captured image of the result with Chamelium, I think the tests should be more ambitious. Do not rely on identity curves or matrices, nor max LUT, because now you can use a difference threshold per pixel when comparing the result with the reference.

Use various non-trivial curves, different for each of red, green and blue. Use non-trivial matrices that actually compute mixtures instead of just moving red value to the green channel. Use multiple planes simultaneously. Use different framebuffer formats, particularly with higher than 8 bits per channel, and check the capture has the same precision and not truncated to 8 bit.

That kind of tests would have much more proving power, and they also help assess the precision of the hardware. Precision is important to userspace.

These are also tests that userspace projects cannot really execute, they do not have labs with Chamelium boards and not all drivers/hardware support writeback connectors.

+#define MAX_SUPPORTED_PLANES 7 +#define SDR_PLANE_BASE 3

+typedef bool (*test_t)(data_t*, igt_plane_t*);

+static bool is_hdr_plane(const igt_plane_t *plane) +{

• return plane->index >= 0 && plane->index < SDR_PLANE_BASE;

This here again. I guess the previous definition of this function was never used?

The same questions.

+}

+static bool is_valid_plane(igt_plane_t *plane) +{

• int index = plane->index;
• if (plane->type != DRM_PLANE_TYPE_PRIMARY)

• return false;
  

• /*

• * Test 1 HDR plane, 1 SDR plane.
  

• *
  

• * 0,1,2 HDR planes
  

• * 3,4,5,6 SDR planes
  

• *
  

• */
  

• return index >= 0 && index < MAX_SUPPORTED_PLANES;

+}

Thanks, pq

From: Pekka Paalanen ppaalanen@gmail.com Sent: Thursday, November 18, 2021 3:15 PM To: Modem, Bhanuprakash bhanuprakash.modem@intel.com Cc: igt-dev@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Kumar, Mukunda Pramodh mukunda.pramodh.kumar@intel.com; Juha-Pekka Heikkila juhapekka.heikkila@gmail.com; Shankar, Uma uma.shankar@intel.com Subject: Re: [i-g-t 12/14] kms_color_helper: Add helper functions to support logarithmic gamma mode

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

...

From: Mukunda Pramodh Kumar mukunda.pramodh.kumar@intel.com

Add helper functions to support logarithmic gamma mode

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: Mukunda Pramodh Kumar mukunda.pramodh.kumar@intel.com Signed-off-by: Bhanuprakash Modem bhanuprakash.modem@intel.com

---

tests/kms_color_helper.c | 127 +++++++++++++++++++++++++++++++++++++++ tests/kms_color_helper.h | 16 +++++ 2 files changed, 143 insertions(+)

diff --git a/tests/kms_color_helper.c b/tests/kms_color_helper.c index c65b7a0f50..7ea8282df3 100644 --- a/tests/kms_color_helper.c +++ b/tests/kms_color_helper.c @@ -190,6 +190,33 @@ struct drm_color_lut *coeffs_to_lut(data_t *data, return lut; }

+struct drm_color_lut *coeffs_to_logarithmic_lut(data_t *data,

• 				const gamma_lut_t *gamma,
  

• 				uint32_t color_depth,
  

• 				int off)
  

+{

• struct drm_color_lut *lut;
• int i, lut_size = gamma->size;
• /* This is the maximum value due to 16 bit precision in hardware. */

In whose hardware?

Are igt tests not supposed to be generic for everything that exposes the particular KMS properties?

This also hints that the UAPI design is lacking, because userspace needs to know hardware specific things out of thin air. Display servers are not going to have hardware-specific code. They specialise based on the existence of KMS properties instead.

Yeah, the comment is misleading and variable names also bit confusing.

As uapi supports U0.16 precision the max supported value would be (1 << 16) - 1 and Intel h/w supports max value of (1 << 24) so we need to scale the values accordingly.

So, need to drop h/w specific hardcoded stuff and read from uapi & rename the variables as below:

max_uapi_value = (1 << 16) - 1; max_hw_value = /* Read from the uapi. */

...

• uint32_t max_hw_value = (1 << 16) - 1;
• unsigned int max_segment_value = 1 << 24;
• lut = malloc(sizeof(struct drm_color_lut) * lut_size);
• for (i = 0; i < lut_size; i++) {

• double scaling_factor = (double)max_hw_value /
  

(double)max_segment_value;

...

• uint32_t r = MIN((gamma->coeffs[i].r * scaling_factor),
  

max_hw_value);

...

• uint32_t g = MIN((gamma->coeffs[i].g * scaling_factor),
  

max_hw_value);

...

• uint32_t b = MIN((gamma->coeffs[i].b * scaling_factor),
  

max_hw_value);

...

• lut[i].red = r;
  

• lut[i].green = g;
  

• lut[i].blue = b;
  

• }
• return lut;

+}

void set_degamma(data_t *data, igt_pipe_t *pipe, const gamma_lut_t *gamma) @@ -203,6 +230,15 @@ void set_degamma(data_t *data, free(lut); }

+void set_pipe_gamma(igt_pipe_t *pipe,

•     uint64_t value,
  

•     struct drm_color_lut *lut,
  

•     uint32_t size)
  

+{

• igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_GAMMA_MODE, value);
• igt_pipe_obj_replace_prop_blob(pipe, IGT_CRTC_GAMMA_LUT, lut, size);

+}

void set_gamma(data_t *data, igt_pipe_t *pipe, const gamma_lut_t *gamma) { @@ -241,6 +277,51 @@ 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_pipe_gamma_degamma_mode(igt_pipe_t *pipe,

• 			       enum igt_atomic_crtc_properties prop)
  

+{

• igt_display_t *display = pipe->display;
• uint32_t prop_id = pipe->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;

+}

+gamma_lut_t *pipe_create_linear_lut(segment_data_t *info)

Identity transformation?

...

+{

• uint32_t segment, entry, index = 0;
• double val;
• int i = 0;
• gamma_lut_t *gamma = alloc_lut(info->entries_count);
• igt_assert(gamma);
• gamma->size = info->entries_count;
• for (segment = 0; segment < info->segment_count; segment++) {

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

• uint32_t start = (segment == 0) ? 0 : (1 << (segment - 1));
  

• uint32_t end = 1 << segment;
  

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

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

• 		0 : start + entry *
  

• 		((end - start) * 1.0 / entry_count);
  

• 	set_rgb(&gamma->coeffs[i++], val);
  

• 	index++;
  

• }
  

• }
• return gamma;

+}

drmModePropertyPtr get_plane_gamma_degamma_mode(igt_plane_t *plane, enum igt_atomic_plane_properties prop) { @@ -331,6 +412,7 @@ segment_data_t *get_segment_data(data_t *data, info->segment_data = malloc(sizeof(struct drm_color_lut_range) * info- segment_count); igt_assert(info->segment_data);

• info->entries_count = 0;

What's this?

...

for (i = 0; i < info->segment_count; i++) { info->entries_count += lut_range[i].count; info->segment_data[i] = lut_range[i]; @@ -341,6 +423,51 @@ segment_data_t *get_segment_data(data_t *data, return info; }

+void set_advance_gamma(data_t *data, igt_pipe_t *pipe, enum gamma_type

type)

...

+{

• igt_display_t *display = &data->display;
• gamma_lut_t *gamma_log;
• drmModePropertyPtr gamma_mode = NULL;
• segment_data_t *segment_info = NULL;
• struct drm_color_lut *lut = NULL;
• int lut_size = 0;
• drmSetClientCap(data->drm_fd, DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES, 1);

Is this how we are going to do cross-software DRM-master hand-over or switching compatibility in general?

Add a new client cap for every new KMS property, and if the KMS client does not set the property, the kernel will magically reset it to ensure the client's expectations are met? Is that how it works?

Or why does this exist?