On Mon, 15 Nov 2021 15:17:48 +0530
Bhanuprakash Modem [off-list ref] wrote:

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.

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