Re: [V7 33/45] drm/colorop: Add 1D Curve Custom LUT type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 1/15/25 01:14, Simon Ser wrote:
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index a3e1fcad47ad..4744c12e429d 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -701,6 +701,9 @@ static int drm_atomic_color_set_data_property(struct drm_colorop *colorop,
  	bool replaced = false;
switch (colorop->type) {
+	case DRM_COLOROP_1D_LUT:
+		size = colorop->lut_size * sizeof(struct drm_color_lut);

Should we set the element size and the number of elements instead of
multiplying? Or is that only useful when either of these are controlled by
user-space to avoid integer overflows?

This multiplication here is to calculate the total size for the data blob.

The user-space communicates the lut_size (which is read-only) without multiplying sizeof(drm_color_lut) in drm_atomic_colorop_get_property, i.e.,

+	} else if (property == colorop->lut_size_property) {
+		*val = colorop->lut_size;

Is this what you meant?


+		break;
  	case DRM_COLOROP_CTM_3X4:
  		size = sizeof(struct drm_color_ctm_3x4);
  		break;
@@ -750,6 +753,8 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
  		*val = state->bypass;
  	} else if (property == colorop->curve_1d_type_property) {
  		*val = state->curve_1d_type;
+	} else if (property == colorop->lut_size_property) {
+		*val = colorop->lut_size;
  	} else if (property == colorop->data_property) {
  		*val = (state->data) ? state->data->base.id : 0;
  	} else {
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index 665b23900cc0..e6dea2713490 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -64,6 +64,7 @@
static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
  	{ DRM_COLOROP_1D_CURVE, "1D Curve" },
+	{ DRM_COLOROP_1D_LUT, "1D Curve Custom LUT" },

Since we now have both a "normal" 1D curve, and a "special" one… Would it make
sense to change our minds regarding the naming of the former? For instance, we
could rename it to DRM_COLOROP_FIXED_1D_CURVE. Or is the current name clear
enough (and only the human-readable name can be switched to "1D Fixed Curve")?

How about keeping "1D Curve" and simplifying "1D Curve Custom LUT" to "1D LUT" such as the following?

   	{ DRM_COLOROP_1D_CURVE, "1D Curve" },
+	{ DRM_COLOROP_1D_LUT, "1D LUT" },




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux