[PATCH v2 0/2] drm: fix and cleanup legacy gamma support

[PATCH v2 0/2] drm: fix and cleanup legacy gamma support

[Tomi Valkeinen]

Hi,

This is v2 of the series.

The first patch fixes legacy gamma table for HW which have a degamma lut
block before CTM block, but no gamma lut after the CTM. This will allow
us to add CTM support to omapdrm, which only has "pre-gamma" i.e.
de-gamma table.

The second one cleans up the legacy gamma support a bit by handling
legacy gamma for modern drivers in the drm core.

Changes in v2:
- Use bitfields for has_gamma_prop and has_degamma_prop
- Drop use_degamma variable
- Also fix gamma/degamma handling in setcmap_atomic, which I had missed
  earlier.
- Fix comments that were still referring to drm_atomic_helper_legacy_gamma_set
- Drop drm_atomic_helper_legacy_gamma_set use from intel_display.c which
  I had missed earlier.
- Move the declaration of the new functions to drm_crtc_internal.h

I didn't add Laurent's RBs as I felt the changes were too big to keep
the RB.

Ville has a WIP branch at

git://github.com/vsyrjala/linux.git fb_helper_c8_lut_4

which does more extensive cleanups to the gamma handling. That work is
slightly overlapping with this series, but afaics the concepts do not
conflict as such (but the code changes do conflict).

 Tomi

Tomi Valkeinen (2):
  drm: add legacy support for using degamma for gamma
  drm: automatic legacy gamma support

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   1 -
 .../gpu/drm/arm/display/komeda/komeda_crtc.c  |   1 -
 drivers/gpu/drm/arm/malidp_crtc.c             |   1 -
 drivers/gpu/drm/armada/armada_crtc.c          |   1 -
 drivers/gpu/drm/ast/ast_mode.c                |   1 -
 .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c    |   1 -
 drivers/gpu/drm/drm_atomic_helper.c           |  70 ----------
 drivers/gpu/drm/drm_color_mgmt.c              | 122 ++++++++++++++++--
 drivers/gpu/drm/drm_crtc_internal.h           |   6 +
 drivers/gpu/drm/drm_fb_helper.c               |  21 +--
 drivers/gpu/drm/i915/display/intel_display.c  |   1 -
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c     |   2 -
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c       |   1 -
 drivers/gpu/drm/nouveau/dispnv50/head.c       |   2 -
 drivers/gpu/drm/omapdrm/omap_crtc.c           |   1 -
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c        |   1 -
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |   1 -
 drivers/gpu/drm/stm/ltdc.c                    |   1 -
 drivers/gpu/drm/vc4/vc4_crtc.c                |   1 -
 drivers/gpu/drm/vc4/vc4_txp.c                 |   1 -
 include/drm/drm_atomic_helper.h               |   4 -
 include/drm/drm_crtc.h                        |   3 +
 22 files changed, 135 insertions(+), 109 deletions(-)

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/2] drm: add legacy support for using degamma for gamma

[Tomi Valkeinen]

We currently have drm_atomic_helper_legacy_gamma_set() helper which can
be used to handle legacy gamma-set ioctl.
drm_atomic_helper_legacy_gamma_set() sets GAMMA_LUT, and clears
CTM and DEGAMMA_LUT. This works fine on HW where we have either:

degamma -> ctm -> gamma -> out

or

ctm -> gamma -> out

However, if the HW has gamma table before ctm, the atomic property
should be DEGAMMA_LUT, and thus we have:

degamma -> ctm -> out

This is fine for userspace which sets gamma table using the properties,
as the userspace can check for the existence of gamma & degamma, but the
legacy gamma-set ioctl does not work.

This patch fixes the issue by changing
drm_atomic_helper_legacy_gamma_set() so that GAMMA_LUT will be used if
it exists, and DEGAMMA_LUT will be used as a fallback.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++++++---
 drivers/gpu/drm/drm_color_mgmt.c    |  4 ++++
 drivers/gpu/drm/drm_fb_helper.c     |  8 ++++++--
 include/drm/drm_crtc.h              |  3 +++
 4 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ba1507036f26..117b186fe646 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3512,6 +3512,10 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
  * that support color management through the DEGAMMA_LUT/GAMMA_LUT
  * properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
  * how the atomic color management and gamma tables work.
+ *
+ * This function uses the GAMMA_LUT or DEGAMMA_LUT property for the gamma table.
+ * GAMMA_LUT property is used if it exists, and DEGAMMA_LUT property is used as
+ * a fallback.
  */
 int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
 				       u16 *red, u16 *green, u16 *blue,
@@ -3526,6 +3530,9 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
 	int i, ret = 0;
 	bool replaced;
 
+	if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
+		return -ENODEV;
+
 	state = drm_atomic_state_alloc(crtc->dev);
 	if (!state)
 		return -ENOMEM;
@@ -3554,10 +3561,12 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
 		goto fail;
 	}
 
-	/* Reset DEGAMMA_LUT and CTM properties. */
-	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
+	/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
+	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
+					      crtc->has_gamma_prop ? NULL : blob);
 	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
-	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
+	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
+					      crtc->has_gamma_prop ? blob : NULL);
 	crtc_state->color_mgmt_changed |= replaced;
 
 	ret = drm_atomic_commit(state);
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 3bcabc2f6e0e..956e59d5f6a7 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -176,6 +176,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 					   degamma_lut_size);
 	}
 
+	crtc->has_degamma_prop = !!degamma_lut_size;
+
 	if (has_ctm)
 		drm_object_attach_property(&crtc->base,
 					   config->ctm_property, 0);
@@ -187,6 +189,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 					   config->gamma_lut_size_property,
 					   gamma_lut_size);
 	}
+
+	crtc->has_gamma_prop = !!gamma_lut_size;
 }
 EXPORT_SYMBOL(drm_crtc_enable_color_mgmt);
 
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 25edf670867c..b0906ef97617 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1001,6 +1001,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
 	drm_client_for_each_modeset(modeset, &fb_helper->client) {
 		crtc = modeset->crtc;
 
+		if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
+			continue;
+
 		if (!gamma_lut)
 			gamma_lut = setcmap_new_gamma_lut(crtc, cmap);
 		if (IS_ERR(gamma_lut)) {
@@ -1015,11 +1018,12 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
 			goto out_state;
 		}
 
+		/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
 		replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
-						      NULL);
+						      crtc->has_gamma_prop ? NULL : gamma_lut);
 		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
 		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
-						      gamma_lut);
+						      crtc->has_gamma_prop ? gamma_lut : NULL);
 		crtc_state->color_mgmt_changed |= replaced;
 	}
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index ba839e5e357d..4d9e217e5040 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1084,6 +1084,9 @@ struct drm_crtc {
 	 */
 	uint16_t *gamma_store;
 
+	bool has_gamma_prop : 1;
+	bool has_degamma_prop : 1;
+
 	/** @helper_private: mid-layer private data */
 	const struct drm_crtc_helper_funcs *helper_private;
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/2] drm: automatic legacy gamma support

[Tomi Valkeinen]

To support legacy gamma ioctls the drivers need to set
drm_crtc_funcs.gamma_set either to a custom implementation or to
drm_atomic_helper_legacy_gamma_set. Most of the atomic drivers do the
latter.

We can simplify this by making the core handle it automatically.

Add two functions: drm_crtc_supports_legacy_gamma() which tells if the
legacy gamma table can be set, and drm_crtc_legacy_gamma_set() which
does the work by either calling the drm_crtc_funcs.gamma_set or using
GAMMA_LUT or DEGAMMA_LUT.

We can then drop drm_atomic_helper_legacy_gamma_set() and remove all its
uses.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   1 -
 .../gpu/drm/arm/display/komeda/komeda_crtc.c  |   1 -
 drivers/gpu/drm/arm/malidp_crtc.c             |   1 -
 drivers/gpu/drm/armada/armada_crtc.c          |   1 -
 drivers/gpu/drm/ast/ast_mode.c                |   1 -
 .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c    |   1 -
 drivers/gpu/drm/drm_atomic_helper.c           |  79 ------------
 drivers/gpu/drm/drm_color_mgmt.c              | 118 ++++++++++++++++--
 drivers/gpu/drm/drm_crtc_internal.h           |   6 +
 drivers/gpu/drm/drm_fb_helper.c               |  13 +-
 drivers/gpu/drm/i915/display/intel_display.c  |   1 -
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c     |   2 -
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c       |   1 -
 drivers/gpu/drm/nouveau/dispnv50/head.c       |   2 -
 drivers/gpu/drm/omapdrm/omap_crtc.c           |   1 -
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c        |   1 -
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |   1 -
 drivers/gpu/drm/stm/ltdc.c                    |   1 -
 drivers/gpu/drm/vc4/vc4_crtc.c                |   1 -
 drivers/gpu/drm/vc4/vc4_txp.c                 |   1 -
 include/drm/drm_atomic_helper.h               |   4 -
 21 files changed, 122 insertions(+), 116 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 2855bb918535..848b06c51b0e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5396,7 +5396,6 @@ static void dm_disable_vblank(struct drm_crtc *crtc)
 static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
 	.reset = dm_crtc_reset_state,
 	.destroy = amdgpu_dm_crtc_destroy,
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
 	.atomic_duplicate_state = dm_crtc_duplicate_state,
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 4b485eb512e2..59172acb9738 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -550,7 +550,6 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc)
 }
 
 static const struct drm_crtc_funcs komeda_crtc_funcs = {
-	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
 	.destroy		= drm_crtc_cleanup,
 	.set_config		= drm_atomic_helper_set_config,
 	.page_flip		= drm_atomic_helper_page_flip,
diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
index 108e7a31bd26..494075ddbef6 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -510,7 +510,6 @@ static void malidp_crtc_disable_vblank(struct drm_crtc *crtc)
 }
 
 static const struct drm_crtc_funcs malidp_crtc_funcs = {
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 	.destroy = drm_crtc_cleanup,
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
index 3ebcf5a52c8b..b7bb90ae787f 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -820,7 +820,6 @@ static const struct drm_crtc_funcs armada_crtc_funcs = {
 	.cursor_set	= armada_drm_crtc_cursor_set,
 	.cursor_move	= armada_drm_crtc_cursor_move,
 	.destroy	= armada_drm_crtc_destroy,
-	.gamma_set	= drm_atomic_helper_legacy_gamma_set,
 	.set_config	= drm_atomic_helper_set_config,
 	.page_flip	= drm_atomic_helper_page_flip,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 9db371f4054f..5b0ec785c516 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -903,7 +903,6 @@ static void ast_crtc_atomic_destroy_state(struct drm_crtc *crtc,
 
 static const struct drm_crtc_funcs ast_crtc_funcs = {
 	.reset = ast_crtc_reset,
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 	.destroy = drm_crtc_cleanup,
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index c58fa00b4848..05ad75d155e8 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -473,7 +473,6 @@ static const struct drm_crtc_funcs atmel_hlcdc_crtc_funcs = {
 	.atomic_destroy_state = atmel_hlcdc_crtc_destroy_state,
 	.enable_vblank = atmel_hlcdc_crtc_enable_vblank,
 	.disable_vblank = atmel_hlcdc_crtc_disable_vblank,
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 };
 
 int atmel_hlcdc_crtc_create(struct drm_device *dev)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 117b186fe646..b114658100b3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3499,85 +3499,6 @@ int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc,
 }
 EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
 
-/**
- * drm_atomic_helper_legacy_gamma_set - set the legacy gamma correction table
- * @crtc: CRTC object
- * @red: red correction table
- * @green: green correction table
- * @blue: green correction table
- * @size: size of the tables
- * @ctx: lock acquire context
- *
- * Implements support for legacy gamma correction table for drivers
- * that support color management through the DEGAMMA_LUT/GAMMA_LUT
- * properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
- * how the atomic color management and gamma tables work.
- *
- * This function uses the GAMMA_LUT or DEGAMMA_LUT property for the gamma table.
- * GAMMA_LUT property is used if it exists, and DEGAMMA_LUT property is used as
- * a fallback.
- */
-int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
-				       u16 *red, u16 *green, u16 *blue,
-				       uint32_t size,
-				       struct drm_modeset_acquire_ctx *ctx)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_atomic_state *state;
-	struct drm_crtc_state *crtc_state;
-	struct drm_property_blob *blob = NULL;
-	struct drm_color_lut *blob_data;
-	int i, ret = 0;
-	bool replaced;
-
-	if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
-		return -ENODEV;
-
-	state = drm_atomic_state_alloc(crtc->dev);
-	if (!state)
-		return -ENOMEM;
-
-	blob = drm_property_create_blob(dev,
-					sizeof(struct drm_color_lut) * size,
-					NULL);
-	if (IS_ERR(blob)) {
-		ret = PTR_ERR(blob);
-		blob = NULL;
-		goto fail;
-	}
-
-	/* Prepare GAMMA_LUT with the legacy values. */
-	blob_data = blob->data;
-	for (i = 0; i < size; i++) {
-		blob_data[i].red = red[i];
-		blob_data[i].green = green[i];
-		blob_data[i].blue = blue[i];
-	}
-
-	state->acquire_ctx = ctx;
-	crtc_state = drm_atomic_get_crtc_state(state, crtc);
-	if (IS_ERR(crtc_state)) {
-		ret = PTR_ERR(crtc_state);
-		goto fail;
-	}
-
-	/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
-	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
-					      crtc->has_gamma_prop ? NULL : blob);
-	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
-	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
-					      crtc->has_gamma_prop ? blob : NULL);
-	crtc_state->color_mgmt_changed |= replaced;
-
-	ret = drm_atomic_commit(state);
-
-fail:
-	drm_atomic_state_put(state);
-	drm_property_blob_put(blob);
-	return ret;
-}
-EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
-
 /**
  * drm_atomic_helper_bridge_propagate_bus_fmt() - Propagate output format to
  *						  the input end of a bridge
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 956e59d5f6a7..78b3a9325f7a 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -22,6 +22,7 @@
 
 #include <linux/uaccess.h>
 
+#include <drm/drm_atomic.h>
 #include <drm/drm_color_mgmt.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_device.h>
@@ -89,9 +90,8 @@
  *	modes) appropriately.
  *
  * There is also support for a legacy gamma table, which is set up by calling
- * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
- * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
- * "GAMMA_LUT" property above.
+ * drm_mode_crtc_set_gamma_size(). The DRM core will then alias the legacy gamma
+ * ramp with "GAMMA_LUT", or if that is unavailable "DEGAMMA_LUT".
  *
  * Support for different non RGB color encodings is controlled through
  * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties. They
@@ -156,9 +156,6 @@ EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
  * optional. The gamma and degamma properties are only attached if
  * their size is not 0 and ctm_property is only attached if has_ctm is
  * true.
- *
- * Drivers should use drm_atomic_helper_legacy_gamma_set() to implement the
- * legacy &drm_crtc_funcs.gamma_set callback.
  */
 void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 				uint degamma_lut_size,
@@ -235,6 +232,109 @@ int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 }
 EXPORT_SYMBOL(drm_mode_crtc_set_gamma_size);
 
+/**
+ * drm_crtc_supports_legacy_gamma - does the crtc support legacy gamma correction table
+ * @crtc: CRTC object
+ *
+ * Returns true/false if the given crtc supports setting the legacy gamma
+ * correction table.
+ */
+bool drm_crtc_supports_legacy_gamma(struct drm_crtc *crtc)
+{
+	if (!crtc->gamma_size)
+		return false;
+
+	if (crtc->funcs->gamma_set)
+		return true;
+
+	return crtc->has_gamma_prop || crtc->has_degamma_prop;
+}
+EXPORT_SYMBOL(drm_crtc_supports_legacy_gamma);
+
+/**
+ * drm_crtc_legacy_gamma_set - set the legacy gamma correction table
+ * @crtc: CRTC object
+ * @red: red correction table
+ * @green: green correction table
+ * @blue: green correction table
+ * @size: size of the tables
+ * @ctx: lock acquire context
+ *
+ * Implements support for legacy gamma correction table for drivers
+ * that have set drm_crtc_funcs.gamma_set or that support color management
+ * through the DEGAMMA_LUT/GAMMA_LUT properties. See
+ * drm_crtc_enable_color_mgmt() and the containing chapter for
+ * how the atomic color management and gamma tables work.
+ *
+ * This function sets the gamma using the first one available:
+ * - drm_crtc_funcs.gamma_set()
+ * - GAMMA_LUT
+ * - DEGAMMA_LUT
+ */
+int drm_crtc_legacy_gamma_set(struct drm_crtc *crtc,
+			      u16 *red, u16 *green, u16 *blue,
+			      uint32_t size,
+			      struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_atomic_state *state;
+	struct drm_crtc_state *crtc_state;
+	struct drm_property_blob *blob = NULL;
+	struct drm_color_lut *blob_data;
+	int i, ret = 0;
+	bool replaced;
+
+	if (crtc->funcs->gamma_set)
+		return crtc->funcs->gamma_set(crtc, red, green, blue, size, ctx);
+
+	if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
+		return -ENODEV;
+
+	state = drm_atomic_state_alloc(crtc->dev);
+	if (!state)
+		return -ENOMEM;
+
+	blob = drm_property_create_blob(dev,
+					sizeof(struct drm_color_lut) * size,
+					NULL);
+	if (IS_ERR(blob)) {
+		ret = PTR_ERR(blob);
+		blob = NULL;
+		goto fail;
+	}
+
+	/* Prepare GAMMA_LUT with the legacy values. */
+	blob_data = blob->data;
+	for (i = 0; i < size; i++) {
+		blob_data[i].red = red[i];
+		blob_data[i].green = green[i];
+		blob_data[i].blue = blue[i];
+	}
+
+	state->acquire_ctx = ctx;
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state)) {
+		ret = PTR_ERR(crtc_state);
+		goto fail;
+	}
+
+	/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
+	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
+					      crtc->has_gamma_prop ? NULL : blob);
+	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
+	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
+					      crtc->has_gamma_prop ? blob : NULL);
+	crtc_state->color_mgmt_changed |= replaced;
+
+	ret = drm_atomic_commit(state);
+
+fail:
+	drm_atomic_state_put(state);
+	drm_property_blob_put(blob);
+	return ret;
+}
+EXPORT_SYMBOL(drm_crtc_legacy_gamma_set);
+
 /**
  * drm_mode_gamma_set_ioctl - set the gamma table
  * @dev: DRM device
@@ -266,7 +366,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
 	if (!crtc)
 		return -ENOENT;
 
-	if (crtc->funcs->gamma_set == NULL)
+	if (!drm_crtc_supports_legacy_gamma(crtc))
 		return -ENOSYS;
 
 	/* memcpy into gamma store */
@@ -294,8 +394,8 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
-	ret = crtc->funcs->gamma_set(crtc, r_base, g_base, b_base,
-				     crtc->gamma_size, &ctx);
+	ret = drm_crtc_legacy_gamma_set(crtc, r_base, g_base, b_base,
+					crtc->gamma_size, &ctx);
 
 out:
 	DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 54d4cf1233e9..dd1cca024b16 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -114,6 +114,12 @@ int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
 const char *drm_get_color_encoding_name(enum drm_color_encoding encoding);
 const char *drm_get_color_range_name(enum drm_color_range range);
 
+bool drm_crtc_supports_legacy_gamma(struct drm_crtc *crtc);
+int drm_crtc_legacy_gamma_set(struct drm_crtc *crtc,
+			      u16 *red, u16 *green, u16 *blue,
+			      uint32_t size,
+			      struct drm_modeset_acquire_ctx *ctx);
+
 /* IOCTLs */
 int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 			     void *data, struct drm_file *file_priv);
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index b0906ef97617..0fb29b1a1f98 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -46,6 +46,7 @@
 #include <drm/drm_print.h>
 #include <drm/drm_vblank.h>
 
+#include "drm_crtc_internal.h"
 #include "drm_crtc_helper_internal.h"
 #include "drm_internal.h"
 
@@ -136,15 +137,15 @@ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
 {
 	uint16_t *r_base, *g_base, *b_base;
 
-	if (crtc->funcs->gamma_set == NULL)
+	if (!drm_crtc_supports_legacy_gamma(crtc))
 		return;
 
 	r_base = crtc->gamma_store;
 	g_base = r_base + crtc->gamma_size;
 	b_base = g_base + crtc->gamma_size;
 
-	crtc->funcs->gamma_set(crtc, r_base, g_base, b_base,
-			       crtc->gamma_size, NULL);
+	drm_crtc_legacy_gamma_set(crtc, r_base, g_base, b_base,
+				  crtc->gamma_size, NULL);
 }
 
 /**
@@ -907,7 +908,7 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
 	drm_modeset_lock_all(fb_helper->dev);
 	drm_client_for_each_modeset(modeset, &fb_helper->client) {
 		crtc = modeset->crtc;
-		if (!crtc->funcs->gamma_set || !crtc->gamma_size)
+		if (!drm_crtc_supports_legacy_gamma(crtc))
 			return -EINVAL;
 
 		if (cmap->start + cmap->len > crtc->gamma_size)
@@ -921,8 +922,8 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
 		memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g));
 		memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
 
-		ret = crtc->funcs->gamma_set(crtc, r, g, b,
-					     crtc->gamma_size, NULL);
+		ret = drm_crtc_legacy_gamma_set(crtc, r, g, b, crtc->gamma_size,
+						NULL);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index cddbda5303ff..1e90b8c3b1c7 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -16879,7 +16879,6 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
 }
 
 #define INTEL_CRTC_FUNCS \
-	.gamma_set = drm_atomic_helper_legacy_gamma_set, \
 	.set_config = drm_atomic_helper_set_config, \
 	.destroy = intel_crtc_destroy, \
 	.page_flip = drm_atomic_helper_page_flip, \
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 368bfef8b340..75f948d41b0a 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -755,8 +755,6 @@ static const struct drm_crtc_funcs ingenic_drm_crtc_funcs = {
 
 	.enable_vblank		= ingenic_drm_enable_vblank,
 	.disable_vblank		= ingenic_drm_disable_vblank,
-
-	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
 };
 
 static const struct drm_plane_helper_funcs ingenic_drm_plane_helper_funcs = {
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index bfe994230543..b665bd498a4a 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -619,7 +619,6 @@ static const struct drm_crtc_funcs mtk_crtc_funcs = {
 	.reset			= mtk_drm_crtc_reset,
 	.atomic_duplicate_state	= mtk_drm_crtc_duplicate_state,
 	.atomic_destroy_state	= mtk_drm_crtc_destroy_state,
-	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
 	.enable_vblank		= mtk_drm_crtc_enable_vblank,
 	.disable_vblank		= mtk_drm_crtc_disable_vblank,
 };
diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c b/drivers/gpu/drm/nouveau/dispnv50/head.c
index 537c1ef2e464..ec361d17e900 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/head.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
@@ -503,7 +503,6 @@ nv50_head_destroy(struct drm_crtc *crtc)
 static const struct drm_crtc_funcs
 nv50_head_func = {
 	.reset = nv50_head_reset,
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 	.destroy = nv50_head_destroy,
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
@@ -518,7 +517,6 @@ nv50_head_func = {
 static const struct drm_crtc_funcs
 nvd9_head_func = {
 	.reset = nv50_head_reset,
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 	.destroy = nv50_head_destroy,
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 7d66269ad998..b97f21cf4a91 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -741,7 +741,6 @@ static const struct drm_crtc_funcs omap_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = omap_crtc_destroy,
 	.page_flip = drm_atomic_helper_page_flip,
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 	.atomic_duplicate_state = omap_crtc_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 	.atomic_set_property = omap_crtc_atomic_set_property,
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index b5fb941e0f53..f93e0750431d 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -1144,7 +1144,6 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
 	.set_crc_source = rcar_du_crtc_set_crc_source,
 	.verify_crc_source = rcar_du_crtc_verify_crc_source,
 	.get_crc_sources = rcar_du_crtc_get_crc_sources,
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 };
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index d1e05482641b..8d15cabdcb02 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1643,7 +1643,6 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
 	.disable_vblank = vop_crtc_disable_vblank,
 	.set_crc_source = vop_crtc_set_crc_source,
 	.verify_crc_source = vop_crtc_verify_crc_source,
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 };
 
 static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 3980677435cb..7812094f93d6 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -713,7 +713,6 @@ static const struct drm_crtc_funcs ltdc_crtc_funcs = {
 	.enable_vblank = ltdc_crtc_enable_vblank,
 	.disable_vblank = ltdc_crtc_disable_vblank,
 	.get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 };
 
 /*
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index ea710beb8e00..7495d70d6a32 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -884,7 +884,6 @@ static const struct drm_crtc_funcs vc4_crtc_funcs = {
 	.reset = vc4_crtc_reset,
 	.atomic_duplicate_state = vc4_crtc_duplicate_state,
 	.atomic_destroy_state = vc4_crtc_destroy_state,
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 	.enable_vblank = vc4_enable_vblank,
 	.disable_vblank = vc4_disable_vblank,
 	.get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 8aa5220885f4..e4b782e42f69 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -382,7 +382,6 @@ static const struct drm_crtc_funcs vc4_txp_crtc_funcs = {
 	.reset			= vc4_crtc_reset,
 	.atomic_duplicate_state	= vc4_crtc_duplicate_state,
 	.atomic_destroy_state	= vc4_crtc_destroy_state,
-	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
 	.enable_vblank		= vc4_txp_enable_vblank,
 	.disable_vblank		= vc4_txp_disable_vblank,
 };
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 5f47720440fa..4045e2507e11 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -147,10 +147,6 @@ int drm_atomic_helper_page_flip_target(
 				uint32_t flags,
 				uint32_t target,
 				struct drm_modeset_acquire_ctx *ctx);
-int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
-				       u16 *red, u16 *green, u16 *blue,
-				       uint32_t size,
-				       struct drm_modeset_acquire_ctx *ctx);
 
 /**
  * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm: add legacy support for using degamma for gamma

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Dec 08, 2020 at 03:57:58PM +0200, Tomi Valkeinen wrote:
> We currently have drm_atomic_helper_legacy_gamma_set() helper which can
> be used to handle legacy gamma-set ioctl.
> drm_atomic_helper_legacy_gamma_set() sets GAMMA_LUT, and clears
> CTM and DEGAMMA_LUT. This works fine on HW where we have either:
> 
> degamma -> ctm -> gamma -> out
> 
> or
> 
> ctm -> gamma -> out
> 
> However, if the HW has gamma table before ctm, the atomic property
> should be DEGAMMA_LUT, and thus we have:
> 
> degamma -> ctm -> out
> 
> This is fine for userspace which sets gamma table using the properties,
> as the userspace can check for the existence of gamma & degamma, but the
> legacy gamma-set ioctl does not work.
> 
> This patch fixes the issue by changing
> drm_atomic_helper_legacy_gamma_set() so that GAMMA_LUT will be used if
> it exists, and DEGAMMA_LUT will be used as a fallback.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++++++---
>  drivers/gpu/drm/drm_color_mgmt.c    |  4 ++++
>  drivers/gpu/drm/drm_fb_helper.c     |  8 ++++++--
>  include/drm/drm_crtc.h              |  3 +++
>  4 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ba1507036f26..117b186fe646 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3512,6 +3512,10 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
>   * that support color management through the DEGAMMA_LUT/GAMMA_LUT
>   * properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
>   * how the atomic color management and gamma tables work.
> + *
> + * This function uses the GAMMA_LUT or DEGAMMA_LUT property for the gamma table.
> + * GAMMA_LUT property is used if it exists, and DEGAMMA_LUT property is used as
> + * a fallback.
>   */
>  int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>  				       u16 *red, u16 *green, u16 *blue,
> @@ -3526,6 +3530,9 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>  	int i, ret = 0;
>  	bool replaced;
>  
> +	if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
> +		return -ENODEV;
> +
>  	state = drm_atomic_state_alloc(crtc->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -3554,10 +3561,12 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>  		goto fail;
>  	}
>  
> -	/* Reset DEGAMMA_LUT and CTM properties. */
> -	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
> +	/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
> +	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> +					      crtc->has_gamma_prop ? NULL : blob);
>  	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> -	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
> +	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> +					      crtc->has_gamma_prop ? blob : NULL);
>  	crtc_state->color_mgmt_changed |= replaced;
>  
>  	ret = drm_atomic_commit(state);
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 3bcabc2f6e0e..956e59d5f6a7 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -176,6 +176,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>  					   degamma_lut_size);
>  	}
>  
> +	crtc->has_degamma_prop = !!degamma_lut_size;
> +
>  	if (has_ctm)
>  		drm_object_attach_property(&crtc->base,
>  					   config->ctm_property, 0);
> @@ -187,6 +189,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>  					   config->gamma_lut_size_property,
>  					   gamma_lut_size);
>  	}
> +
> +	crtc->has_gamma_prop = !!gamma_lut_size;
>  }
>  EXPORT_SYMBOL(drm_crtc_enable_color_mgmt);
>  
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 25edf670867c..b0906ef97617 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1001,6 +1001,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>  	drm_client_for_each_modeset(modeset, &fb_helper->client) {
>  		crtc = modeset->crtc;
>  
> +		if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
> +			continue;
> +
>  		if (!gamma_lut)
>  			gamma_lut = setcmap_new_gamma_lut(crtc, cmap);
>  		if (IS_ERR(gamma_lut)) {
> @@ -1015,11 +1018,12 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>  			goto out_state;
>  		}
>  
> +		/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
>  		replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> -						      NULL);
> +						      crtc->has_gamma_prop ? NULL : gamma_lut);
>  		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
>  		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> -						      gamma_lut);
> +						      crtc->has_gamma_prop ? gamma_lut : NULL);
>  		crtc_state->color_mgmt_changed |= replaced;
>  	}
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index ba839e5e357d..4d9e217e5040 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1084,6 +1084,9 @@ struct drm_crtc {
>  	 */
>  	uint16_t *gamma_store;
>  
> +	bool has_gamma_prop : 1;
> +	bool has_degamma_prop : 1;
> +
>  	/** @helper_private: mid-layer private data */
>  	const struct drm_crtc_helper_funcs *helper_private;
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Dec 08, 2020 at 03:57:59PM +0200, Tomi Valkeinen wrote:
> To support legacy gamma ioctls the drivers need to set
> drm_crtc_funcs.gamma_set either to a custom implementation or to
> drm_atomic_helper_legacy_gamma_set. Most of the atomic drivers do the
> latter.
> 
> We can simplify this by making the core handle it automatically.
> 
> Add two functions: drm_crtc_supports_legacy_gamma() which tells if the
> legacy gamma table can be set, and drm_crtc_legacy_gamma_set() which
> does the work by either calling the drm_crtc_funcs.gamma_set or using
> GAMMA_LUT or DEGAMMA_LUT.
> 
> We can then drop drm_atomic_helper_legacy_gamma_set() and remove all its
> uses.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   1 -
>  .../gpu/drm/arm/display/komeda/komeda_crtc.c  |   1 -
>  drivers/gpu/drm/arm/malidp_crtc.c             |   1 -
>  drivers/gpu/drm/armada/armada_crtc.c          |   1 -
>  drivers/gpu/drm/ast/ast_mode.c                |   1 -
>  .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c    |   1 -
>  drivers/gpu/drm/drm_atomic_helper.c           |  79 ------------
>  drivers/gpu/drm/drm_color_mgmt.c              | 118 ++++++++++++++++--
>  drivers/gpu/drm/drm_crtc_internal.h           |   6 +
>  drivers/gpu/drm/drm_fb_helper.c               |  13 +-
>  drivers/gpu/drm/i915/display/intel_display.c  |   1 -
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c     |   2 -
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c       |   1 -
>  drivers/gpu/drm/nouveau/dispnv50/head.c       |   2 -
>  drivers/gpu/drm/omapdrm/omap_crtc.c           |   1 -
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c        |   1 -
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |   1 -
>  drivers/gpu/drm/stm/ltdc.c                    |   1 -
>  drivers/gpu/drm/vc4/vc4_crtc.c                |   1 -
>  drivers/gpu/drm/vc4/vc4_txp.c                 |   1 -
>  include/drm/drm_atomic_helper.h               |   4 -
>  21 files changed, 122 insertions(+), 116 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 2855bb918535..848b06c51b0e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5396,7 +5396,6 @@ static void dm_disable_vblank(struct drm_crtc *crtc)
>  static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
>  	.reset = dm_crtc_reset_state,
>  	.destroy = amdgpu_dm_crtc_destroy,
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
>  	.atomic_duplicate_state = dm_crtc_duplicate_state,
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 4b485eb512e2..59172acb9738 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -550,7 +550,6 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc)
>  }
>  
>  static const struct drm_crtc_funcs komeda_crtc_funcs = {
> -	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
>  	.destroy		= drm_crtc_cleanup,
>  	.set_config		= drm_atomic_helper_set_config,
>  	.page_flip		= drm_atomic_helper_page_flip,
> diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
> index 108e7a31bd26..494075ddbef6 100644
> --- a/drivers/gpu/drm/arm/malidp_crtc.c
> +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> @@ -510,7 +510,6 @@ static void malidp_crtc_disable_vblank(struct drm_crtc *crtc)
>  }
>  
>  static const struct drm_crtc_funcs malidp_crtc_funcs = {
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  	.destroy = drm_crtc_cleanup,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
> diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> index 3ebcf5a52c8b..b7bb90ae787f 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -820,7 +820,6 @@ static const struct drm_crtc_funcs armada_crtc_funcs = {
>  	.cursor_set	= armada_drm_crtc_cursor_set,
>  	.cursor_move	= armada_drm_crtc_cursor_move,
>  	.destroy	= armada_drm_crtc_destroy,
> -	.gamma_set	= drm_atomic_helper_legacy_gamma_set,
>  	.set_config	= drm_atomic_helper_set_config,
>  	.page_flip	= drm_atomic_helper_page_flip,
>  	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 9db371f4054f..5b0ec785c516 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -903,7 +903,6 @@ static void ast_crtc_atomic_destroy_state(struct drm_crtc *crtc,
>  
>  static const struct drm_crtc_funcs ast_crtc_funcs = {
>  	.reset = ast_crtc_reset,
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  	.destroy = drm_crtc_cleanup,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index c58fa00b4848..05ad75d155e8 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -473,7 +473,6 @@ static const struct drm_crtc_funcs atmel_hlcdc_crtc_funcs = {
>  	.atomic_destroy_state = atmel_hlcdc_crtc_destroy_state,
>  	.enable_vblank = atmel_hlcdc_crtc_enable_vblank,
>  	.disable_vblank = atmel_hlcdc_crtc_disable_vblank,
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  };
>  
>  int atmel_hlcdc_crtc_create(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 117b186fe646..b114658100b3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3499,85 +3499,6 @@ int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
>  
> -/**
> - * drm_atomic_helper_legacy_gamma_set - set the legacy gamma correction table
> - * @crtc: CRTC object
> - * @red: red correction table
> - * @green: green correction table
> - * @blue: green correction table
> - * @size: size of the tables
> - * @ctx: lock acquire context
> - *
> - * Implements support for legacy gamma correction table for drivers
> - * that support color management through the DEGAMMA_LUT/GAMMA_LUT
> - * properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
> - * how the atomic color management and gamma tables work.
> - *
> - * This function uses the GAMMA_LUT or DEGAMMA_LUT property for the gamma table.
> - * GAMMA_LUT property is used if it exists, and DEGAMMA_LUT property is used as
> - * a fallback.
> - */
> -int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> -				       u16 *red, u16 *green, u16 *blue,
> -				       uint32_t size,
> -				       struct drm_modeset_acquire_ctx *ctx)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_atomic_state *state;
> -	struct drm_crtc_state *crtc_state;
> -	struct drm_property_blob *blob = NULL;
> -	struct drm_color_lut *blob_data;
> -	int i, ret = 0;
> -	bool replaced;
> -
> -	if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
> -		return -ENODEV;
> -
> -	state = drm_atomic_state_alloc(crtc->dev);
> -	if (!state)
> -		return -ENOMEM;
> -
> -	blob = drm_property_create_blob(dev,
> -					sizeof(struct drm_color_lut) * size,
> -					NULL);
> -	if (IS_ERR(blob)) {
> -		ret = PTR_ERR(blob);
> -		blob = NULL;
> -		goto fail;
> -	}
> -
> -	/* Prepare GAMMA_LUT with the legacy values. */
> -	blob_data = blob->data;
> -	for (i = 0; i < size; i++) {
> -		blob_data[i].red = red[i];
> -		blob_data[i].green = green[i];
> -		blob_data[i].blue = blue[i];
> -	}
> -
> -	state->acquire_ctx = ctx;
> -	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -	if (IS_ERR(crtc_state)) {
> -		ret = PTR_ERR(crtc_state);
> -		goto fail;
> -	}
> -
> -	/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
> -	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> -					      crtc->has_gamma_prop ? NULL : blob);
> -	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> -	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> -					      crtc->has_gamma_prop ? blob : NULL);
> -	crtc_state->color_mgmt_changed |= replaced;
> -
> -	ret = drm_atomic_commit(state);
> -
> -fail:
> -	drm_atomic_state_put(state);
> -	drm_property_blob_put(blob);
> -	return ret;
> -}
> -EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
> -
>  /**
>   * drm_atomic_helper_bridge_propagate_bus_fmt() - Propagate output format to
>   *						  the input end of a bridge
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 956e59d5f6a7..78b3a9325f7a 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -22,6 +22,7 @@
>  
>  #include <linux/uaccess.h>
>  
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_color_mgmt.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_device.h>
> @@ -89,9 +90,8 @@
>   *	modes) appropriately.
>   *
>   * There is also support for a legacy gamma table, which is set up by calling
> - * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
> - * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
> - * "GAMMA_LUT" property above.
> + * drm_mode_crtc_set_gamma_size(). The DRM core will then alias the legacy gamma
> + * ramp with "GAMMA_LUT", or if that is unavailable "DEGAMMA_LUT".
>   *
>   * Support for different non RGB color encodings is controlled through
>   * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties. They
> @@ -156,9 +156,6 @@ EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
>   * optional. The gamma and degamma properties are only attached if
>   * their size is not 0 and ctm_property is only attached if has_ctm is
>   * true.
> - *
> - * Drivers should use drm_atomic_helper_legacy_gamma_set() to implement the
> - * legacy &drm_crtc_funcs.gamma_set callback.
>   */
>  void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>  				uint degamma_lut_size,
> @@ -235,6 +232,109 @@ int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>  }
>  EXPORT_SYMBOL(drm_mode_crtc_set_gamma_size);
>  
> +/**
> + * drm_crtc_supports_legacy_gamma - does the crtc support legacy gamma correction table
> + * @crtc: CRTC object
> + *
> + * Returns true/false if the given crtc supports setting the legacy gamma
> + * correction table.
> + */
> +bool drm_crtc_supports_legacy_gamma(struct drm_crtc *crtc)
> +{
> +	if (!crtc->gamma_size)
> +		return false;
> +
> +	if (crtc->funcs->gamma_set)
> +		return true;
> +
> +	return crtc->has_gamma_prop || crtc->has_degamma_prop;
> +}
> +EXPORT_SYMBOL(drm_crtc_supports_legacy_gamma);
> +
> +/**
> + * drm_crtc_legacy_gamma_set - set the legacy gamma correction table
> + * @crtc: CRTC object
> + * @red: red correction table
> + * @green: green correction table
> + * @blue: green correction table
> + * @size: size of the tables
> + * @ctx: lock acquire context
> + *
> + * Implements support for legacy gamma correction table for drivers
> + * that have set drm_crtc_funcs.gamma_set or that support color management
> + * through the DEGAMMA_LUT/GAMMA_LUT properties. See
> + * drm_crtc_enable_color_mgmt() and the containing chapter for
> + * how the atomic color management and gamma tables work.
> + *
> + * This function sets the gamma using the first one available:
> + * - drm_crtc_funcs.gamma_set()
> + * - GAMMA_LUT
> + * - DEGAMMA_LUT
> + */
> +int drm_crtc_legacy_gamma_set(struct drm_crtc *crtc,
> +			      u16 *red, u16 *green, u16 *blue,
> +			      uint32_t size,
> +			      struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_atomic_state *state;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_property_blob *blob = NULL;

No need to assign NULL to this variable.

> +	struct drm_color_lut *blob_data;
> +	int i, ret = 0;
> +	bool replaced;
> +
> +	if (crtc->funcs->gamma_set)
> +		return crtc->funcs->gamma_set(crtc, red, green, blue, size, ctx);
> +
> +	if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
> +		return -ENODEV;
> +
> +	state = drm_atomic_state_alloc(crtc->dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	blob = drm_property_create_blob(dev,
> +					sizeof(struct drm_color_lut) * size,
> +					NULL);
> +	if (IS_ERR(blob)) {
> +		ret = PTR_ERR(blob);
> +		blob = NULL;
> +		goto fail;
> +	}
> +
> +	/* Prepare GAMMA_LUT with the legacy values. */
> +	blob_data = blob->data;
> +	for (i = 0; i < size; i++) {
> +		blob_data[i].red = red[i];
> +		blob_data[i].green = green[i];
> +		blob_data[i].blue = blue[i];
> +	}
> +
> +	state->acquire_ctx = ctx;
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state)) {
> +		ret = PTR_ERR(crtc_state);
> +		goto fail;
> +	}
> +
> +	/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
> +	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> +					      crtc->has_gamma_prop ? NULL : blob);
> +	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> +	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> +					      crtc->has_gamma_prop ? blob : NULL);
> +	crtc_state->color_mgmt_changed |= replaced;
> +
> +	ret = drm_atomic_commit(state);
> +
> +fail:
> +	drm_atomic_state_put(state);
> +	drm_property_blob_put(blob);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_crtc_legacy_gamma_set);
> +
>  /**
>   * drm_mode_gamma_set_ioctl - set the gamma table
>   * @dev: DRM device
> @@ -266,7 +366,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
>  	if (!crtc)
>  		return -ENOENT;
>  
> -	if (crtc->funcs->gamma_set == NULL)
> +	if (!drm_crtc_supports_legacy_gamma(crtc))
>  		return -ENOSYS;
>  
>  	/* memcpy into gamma store */
> @@ -294,8 +394,8 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
>  		goto out;
>  	}
>  
> -	ret = crtc->funcs->gamma_set(crtc, r_base, g_base, b_base,
> -				     crtc->gamma_size, &ctx);
> +	ret = drm_crtc_legacy_gamma_set(crtc, r_base, g_base, b_base,
> +					crtc->gamma_size, &ctx);
>  
>  out:
>  	DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 54d4cf1233e9..dd1cca024b16 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -114,6 +114,12 @@ int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
>  const char *drm_get_color_encoding_name(enum drm_color_encoding encoding);
>  const char *drm_get_color_range_name(enum drm_color_range range);
>  
> +bool drm_crtc_supports_legacy_gamma(struct drm_crtc *crtc);
> +int drm_crtc_legacy_gamma_set(struct drm_crtc *crtc,
> +			      u16 *red, u16 *green, u16 *blue,
> +			      uint32_t size,
> +			      struct drm_modeset_acquire_ctx *ctx);
> +
>  /* IOCTLs */
>  int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>  			     void *data, struct drm_file *file_priv);
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index b0906ef97617..0fb29b1a1f98 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -46,6 +46,7 @@
>  #include <drm/drm_print.h>
>  #include <drm/drm_vblank.h>
>  
> +#include "drm_crtc_internal.h"
>  #include "drm_crtc_helper_internal.h"

i goes after h.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  #include "drm_internal.h"
>  
> @@ -136,15 +137,15 @@ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
>  {
>  	uint16_t *r_base, *g_base, *b_base;
>  
> -	if (crtc->funcs->gamma_set == NULL)
> +	if (!drm_crtc_supports_legacy_gamma(crtc))
>  		return;
>  
>  	r_base = crtc->gamma_store;
>  	g_base = r_base + crtc->gamma_size;
>  	b_base = g_base + crtc->gamma_size;
>  
> -	crtc->funcs->gamma_set(crtc, r_base, g_base, b_base,
> -			       crtc->gamma_size, NULL);
> +	drm_crtc_legacy_gamma_set(crtc, r_base, g_base, b_base,
> +				  crtc->gamma_size, NULL);
>  }
>  
>  /**
> @@ -907,7 +908,7 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
>  	drm_modeset_lock_all(fb_helper->dev);
>  	drm_client_for_each_modeset(modeset, &fb_helper->client) {
>  		crtc = modeset->crtc;
> -		if (!crtc->funcs->gamma_set || !crtc->gamma_size)
> +		if (!drm_crtc_supports_legacy_gamma(crtc))
>  			return -EINVAL;
>  
>  		if (cmap->start + cmap->len > crtc->gamma_size)
> @@ -921,8 +922,8 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
>  		memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g));
>  		memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
>  
> -		ret = crtc->funcs->gamma_set(crtc, r, g, b,
> -					     crtc->gamma_size, NULL);
> +		ret = drm_crtc_legacy_gamma_set(crtc, r, g, b, crtc->gamma_size,
> +						NULL);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index cddbda5303ff..1e90b8c3b1c7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -16879,7 +16879,6 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
>  }
>  
>  #define INTEL_CRTC_FUNCS \
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set, \
>  	.set_config = drm_atomic_helper_set_config, \
>  	.destroy = intel_crtc_destroy, \
>  	.page_flip = drm_atomic_helper_page_flip, \
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 368bfef8b340..75f948d41b0a 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -755,8 +755,6 @@ static const struct drm_crtc_funcs ingenic_drm_crtc_funcs = {
>  
>  	.enable_vblank		= ingenic_drm_enable_vblank,
>  	.disable_vblank		= ingenic_drm_disable_vblank,
> -
> -	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
>  };
>  
>  static const struct drm_plane_helper_funcs ingenic_drm_plane_helper_funcs = {
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index bfe994230543..b665bd498a4a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -619,7 +619,6 @@ static const struct drm_crtc_funcs mtk_crtc_funcs = {
>  	.reset			= mtk_drm_crtc_reset,
>  	.atomic_duplicate_state	= mtk_drm_crtc_duplicate_state,
>  	.atomic_destroy_state	= mtk_drm_crtc_destroy_state,
> -	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
>  	.enable_vblank		= mtk_drm_crtc_enable_vblank,
>  	.disable_vblank		= mtk_drm_crtc_disable_vblank,
>  };
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c b/drivers/gpu/drm/nouveau/dispnv50/head.c
> index 537c1ef2e464..ec361d17e900 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/head.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
> @@ -503,7 +503,6 @@ nv50_head_destroy(struct drm_crtc *crtc)
>  static const struct drm_crtc_funcs
>  nv50_head_func = {
>  	.reset = nv50_head_reset,
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  	.destroy = nv50_head_destroy,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
> @@ -518,7 +517,6 @@ nv50_head_func = {
>  static const struct drm_crtc_funcs
>  nvd9_head_func = {
>  	.reset = nv50_head_reset,
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  	.destroy = nv50_head_destroy,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 7d66269ad998..b97f21cf4a91 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -741,7 +741,6 @@ static const struct drm_crtc_funcs omap_crtc_funcs = {
>  	.set_config = drm_atomic_helper_set_config,
>  	.destroy = omap_crtc_destroy,
>  	.page_flip = drm_atomic_helper_page_flip,
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  	.atomic_duplicate_state = omap_crtc_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>  	.atomic_set_property = omap_crtc_atomic_set_property,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index b5fb941e0f53..f93e0750431d 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -1144,7 +1144,6 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
>  	.set_crc_source = rcar_du_crtc_set_crc_source,
>  	.verify_crc_source = rcar_du_crtc_verify_crc_source,
>  	.get_crc_sources = rcar_du_crtc_get_crc_sources,
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  };
>  
>  /* -----------------------------------------------------------------------------
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index d1e05482641b..8d15cabdcb02 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1643,7 +1643,6 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
>  	.disable_vblank = vop_crtc_disable_vblank,
>  	.set_crc_source = vop_crtc_set_crc_source,
>  	.verify_crc_source = vop_crtc_verify_crc_source,
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  };
>  
>  static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 3980677435cb..7812094f93d6 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -713,7 +713,6 @@ static const struct drm_crtc_funcs ltdc_crtc_funcs = {
>  	.enable_vblank = ltdc_crtc_enable_vblank,
>  	.disable_vblank = ltdc_crtc_disable_vblank,
>  	.get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  };
>  
>  /*
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index ea710beb8e00..7495d70d6a32 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -884,7 +884,6 @@ static const struct drm_crtc_funcs vc4_crtc_funcs = {
>  	.reset = vc4_crtc_reset,
>  	.atomic_duplicate_state = vc4_crtc_duplicate_state,
>  	.atomic_destroy_state = vc4_crtc_destroy_state,
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  	.enable_vblank = vc4_enable_vblank,
>  	.disable_vblank = vc4_disable_vblank,
>  	.get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index 8aa5220885f4..e4b782e42f69 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> @@ -382,7 +382,6 @@ static const struct drm_crtc_funcs vc4_txp_crtc_funcs = {
>  	.reset			= vc4_crtc_reset,
>  	.atomic_duplicate_state	= vc4_crtc_duplicate_state,
>  	.atomic_destroy_state	= vc4_crtc_destroy_state,
> -	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
>  	.enable_vblank		= vc4_txp_enable_vblank,
>  	.disable_vblank		= vc4_txp_disable_vblank,
>  };
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 5f47720440fa..4045e2507e11 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -147,10 +147,6 @@ int drm_atomic_helper_page_flip_target(
>  				uint32_t flags,
>  				uint32_t target,
>  				struct drm_modeset_acquire_ctx *ctx);
> -int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> -				       u16 *red, u16 *green, u16 *blue,
> -				       uint32_t size,
> -				       struct drm_modeset_acquire_ctx *ctx);
>  
>  /**
>   * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm: add legacy support for using degamma for gamma

[Daniel Vetter]

On Tue, Dec 08, 2020 at 05:55:11PM +0200, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Dec 08, 2020 at 03:57:58PM +0200, Tomi Valkeinen wrote:
> > We currently have drm_atomic_helper_legacy_gamma_set() helper which can
> > be used to handle legacy gamma-set ioctl.
> > drm_atomic_helper_legacy_gamma_set() sets GAMMA_LUT, and clears
> > CTM and DEGAMMA_LUT. This works fine on HW where we have either:
> > 
> > degamma -> ctm -> gamma -> out
> > 
> > or
> > 
> > ctm -> gamma -> out
> > 
> > However, if the HW has gamma table before ctm, the atomic property
> > should be DEGAMMA_LUT, and thus we have:
> > 
> > degamma -> ctm -> out
> > 
> > This is fine for userspace which sets gamma table using the properties,
> > as the userspace can check for the existence of gamma & degamma, but the
> > legacy gamma-set ioctl does not work.
> > 
> > This patch fixes the issue by changing
> > drm_atomic_helper_legacy_gamma_set() so that GAMMA_LUT will be used if
> > it exists, and DEGAMMA_LUT will be used as a fallback.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++++++---
> >  drivers/gpu/drm/drm_color_mgmt.c    |  4 ++++
> >  drivers/gpu/drm/drm_fb_helper.c     |  8 ++++++--
> >  include/drm/drm_crtc.h              |  3 +++
> >  4 files changed, 25 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index ba1507036f26..117b186fe646 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3512,6 +3512,10 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
> >   * that support color management through the DEGAMMA_LUT/GAMMA_LUT
> >   * properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
> >   * how the atomic color management and gamma tables work.
> > + *
> > + * This function uses the GAMMA_LUT or DEGAMMA_LUT property for the gamma table.
> > + * GAMMA_LUT property is used if it exists, and DEGAMMA_LUT property is used as
> > + * a fallback.
> >   */
> >  int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> >  				       u16 *red, u16 *green, u16 *blue,
> > @@ -3526,6 +3530,9 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> >  	int i, ret = 0;
> >  	bool replaced;
> >  
> > +	if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
> > +		return -ENODEV;
> > +
> >  	state = drm_atomic_state_alloc(crtc->dev);
> >  	if (!state)
> >  		return -ENOMEM;
> > @@ -3554,10 +3561,12 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> >  		goto fail;
> >  	}
> >  
> > -	/* Reset DEGAMMA_LUT and CTM properties. */
> > -	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
> > +	/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
> > +	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> > +					      crtc->has_gamma_prop ? NULL : blob);
> >  	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> > -	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
> > +	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> > +					      crtc->has_gamma_prop ? blob : NULL);
> >  	crtc_state->color_mgmt_changed |= replaced;
> >  
> >  	ret = drm_atomic_commit(state);
> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > index 3bcabc2f6e0e..956e59d5f6a7 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -176,6 +176,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> >  					   degamma_lut_size);
> >  	}
> >  
> > +	crtc->has_degamma_prop = !!degamma_lut_size;
> > +
> >  	if (has_ctm)
> >  		drm_object_attach_property(&crtc->base,
> >  					   config->ctm_property, 0);
> > @@ -187,6 +189,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> >  					   config->gamma_lut_size_property,
> >  					   gamma_lut_size);
> >  	}
> > +
> > +	crtc->has_gamma_prop = !!gamma_lut_size;
> >  }
> >  EXPORT_SYMBOL(drm_crtc_enable_color_mgmt);
> >  
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 25edf670867c..b0906ef97617 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -1001,6 +1001,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> >  	drm_client_for_each_modeset(modeset, &fb_helper->client) {
> >  		crtc = modeset->crtc;
> >  
> > +		if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
> > +			continue;
> > +
> >  		if (!gamma_lut)
> >  			gamma_lut = setcmap_new_gamma_lut(crtc, cmap);
> >  		if (IS_ERR(gamma_lut)) {
> > @@ -1015,11 +1018,12 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> >  			goto out_state;
> >  		}
> >  
> > +		/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
> >  		replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> > -						      NULL);
> > +						      crtc->has_gamma_prop ? NULL : gamma_lut);
> >  		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> >  		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> > -						      gamma_lut);
> > +						      crtc->has_gamma_prop ? gamma_lut : NULL);
> >  		crtc_state->color_mgmt_changed |= replaced;
> >  	}
> >  
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index ba839e5e357d..4d9e217e5040 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1084,6 +1084,9 @@ struct drm_crtc {
> >  	 */
> >  	uint16_t *gamma_store;
> >  
> > +	bool has_gamma_prop : 1;
> > +	bool has_degamma_prop : 1;

I'm a bit behind on patches, but in case this got missed please remove
this and replace with the (obj, prop) lookup function thing or something
like that. Makes sure everything stays in sync, plus like I said atomic
uses this a ton. So not a problem here.
-Daniel

> > +
> >  	/** @helper_private: mid-layer private data */
> >  	const struct drm_crtc_helper_funcs *helper_private;
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2207 bytes --]

Hi Tomi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on linus/master v5.10-rc7]
[cannot apply to drm-tip/drm-tip anholt/for-next next-20201208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-s021-20201208 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-179-ga00755aa-dirty
        # https://github.com/0day-ci/linux/commit/82a704a10c400f58d4b9b44e6def07cc7d6bab20
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917
        git checkout 82a704a10c400f58d4b9b44e6def07cc7d6bab20
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/drm_ioc32.c:38:
>> drivers/gpu/drm/drm_crtc_internal.h:121:17: warning: 'struct drm_modeset_acquire_ctx' declared inside parameter list will not be visible outside of this definition or declaration
     121 |          struct drm_modeset_acquire_ctx *ctx);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~

vim +121 drivers/gpu/drm/drm_crtc_internal.h

   116	
   117	bool drm_crtc_supports_legacy_gamma(struct drm_crtc *crtc);
   118	int drm_crtc_legacy_gamma_set(struct drm_crtc *crtc,
   119				      u16 *red, u16 *green, u16 *blue,
   120				      uint32_t size,
 > 121				      struct drm_modeset_acquire_ctx *ctx);
   122	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39242 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2257 bytes --]

Hi Tomi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on linus/master v5.10-rc7]
[cannot apply to drm-tip/drm-tip anholt/for-next next-20201208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-s021-20201208 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-179-ga00755aa-dirty
        # https://github.com/0day-ci/linux/commit/82a704a10c400f58d4b9b44e6def07cc7d6bab20
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917
        git checkout 82a704a10c400f58d4b9b44e6def07cc7d6bab20
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/drm_ioc32.c:38:
>> drivers/gpu/drm/drm_crtc_internal.h:121:17: warning: 'struct drm_modeset_acquire_ctx' declared inside parameter list will not be visible outside of this definition or declaration
     121 |          struct drm_modeset_acquire_ctx *ctx);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~

vim +121 drivers/gpu/drm/drm_crtc_internal.h

   116	
   117	bool drm_crtc_supports_legacy_gamma(struct drm_crtc *crtc);
   118	int drm_crtc_legacy_gamma_set(struct drm_crtc *crtc,
   119				      u16 *red, u16 *green, u16 *blue,
   120				      uint32_t size,
 > 121				      struct drm_modeset_acquire_ctx *ctx);
   122	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 39242 bytes --]

Re: [PATCH v2 1/2] drm: add legacy support for using degamma for gamma

[Tomi Valkeinen]

Hi Daniel,

On 09/12/2020 02:51, Daniel Vetter wrote:

>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>> index ba839e5e357d..4d9e217e5040 100644
>>> --- a/include/drm/drm_crtc.h
>>> +++ b/include/drm/drm_crtc.h
>>> @@ -1084,6 +1084,9 @@ struct drm_crtc {
>>>  	 */
>>>  	uint16_t *gamma_store;
>>>  
>>> +	bool has_gamma_prop : 1;
>>> +	bool has_degamma_prop : 1;
> 
> I'm a bit behind on patches, but in case this got missed please remove
> this and replace with the (obj, prop) lookup function thing or something
> like that. Makes sure everything stays in sync, plus like I said atomic
> uses this a ton. So not a problem here.

The drm_mode_obj_find_prop_id() is in core drm.ko, and not exported, but I need it also from
drm_kms_helper.ko.

drm_mode_obj_find_prop_id() is declared in drm_crtc_internal.h. Are those functions supposed to be
not exported from drm.ko?

So, is it fine to just export drm_mode_obj_find_prop_id? If I export, should I move it to
drm_mode_object.h?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm: add legacy support for using degamma for gamma

[Daniel Vetter]

On Wed, Dec 9, 2020 at 12:17 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Hi Daniel,
>
> On 09/12/2020 02:51, Daniel Vetter wrote:
>
> >>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >>> index ba839e5e357d..4d9e217e5040 100644
> >>> --- a/include/drm/drm_crtc.h
> >>> +++ b/include/drm/drm_crtc.h
> >>> @@ -1084,6 +1084,9 @@ struct drm_crtc {
> >>>      */
> >>>     uint16_t *gamma_store;
> >>>
> >>> +   bool has_gamma_prop : 1;
> >>> +   bool has_degamma_prop : 1;
> >
> > I'm a bit behind on patches, but in case this got missed please remove
> > this and replace with the (obj, prop) lookup function thing or something
> > like that. Makes sure everything stays in sync, plus like I said atomic
> > uses this a ton. So not a problem here.
>
> The drm_mode_obj_find_prop_id() is in core drm.ko, and not exported, but I need it also from
> drm_kms_helper.ko.
>
> drm_mode_obj_find_prop_id() is declared in drm_crtc_internal.h. Are those functions supposed to be
> not exported from drm.ko?
>
> So, is it fine to just export drm_mode_obj_find_prop_id? If I export, should I move it to
> drm_mode_object.h?

It's not exported to drivers so they don't play games in their own
ioctls or do something else than the atomic set/get_property hooks. I
guess we could export, but I think here the better solution is to lift
the legacy gamma compat function to core code status, since I think
that's the direction we want to go anyway. Bit more churn since the
name changes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4308 bytes --]

Hi Tomi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on linus/master v5.10-rc7]
[cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-m021-20201209 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

New smatch warnings:
drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)

Old smatch warnings:
drivers/gpu/drm/drm_color_mgmt.c:214 drm_mode_crtc_set_gamma_size() warn: double check that we're allocating correct size: 2 vs 6

vim +/blob +307 drivers/gpu/drm/drm_color_mgmt.c

   253	
   254	/**
   255	 * drm_crtc_legacy_gamma_set - set the legacy gamma correction table
   256	 * @crtc: CRTC object
   257	 * @red: red correction table
   258	 * @green: green correction table
   259	 * @blue: green correction table
   260	 * @size: size of the tables
   261	 * @ctx: lock acquire context
   262	 *
   263	 * Implements support for legacy gamma correction table for drivers
   264	 * that have set drm_crtc_funcs.gamma_set or that support color management
   265	 * through the DEGAMMA_LUT/GAMMA_LUT properties. See
   266	 * drm_crtc_enable_color_mgmt() and the containing chapter for
   267	 * how the atomic color management and gamma tables work.
   268	 *
   269	 * This function sets the gamma using the first one available:
   270	 * - drm_crtc_funcs.gamma_set()
   271	 * - GAMMA_LUT
   272	 * - DEGAMMA_LUT
   273	 */
   274	int drm_crtc_legacy_gamma_set(struct drm_crtc *crtc,
   275				      u16 *red, u16 *green, u16 *blue,
   276				      uint32_t size,
   277				      struct drm_modeset_acquire_ctx *ctx)
   278	{
   279		struct drm_device *dev = crtc->dev;
   280		struct drm_atomic_state *state;
   281		struct drm_crtc_state *crtc_state;
   282		struct drm_property_blob *blob = NULL;
   283		struct drm_color_lut *blob_data;
   284		int i, ret = 0;
   285		bool replaced;
   286	
   287		if (crtc->funcs->gamma_set)
   288			return crtc->funcs->gamma_set(crtc, red, green, blue, size, ctx);
   289	
   290		if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
   291			return -ENODEV;
   292	
   293		state = drm_atomic_state_alloc(crtc->dev);
   294		if (!state)
   295			return -ENOMEM;
   296	
   297		blob = drm_property_create_blob(dev,
   298						sizeof(struct drm_color_lut) * size,
   299						NULL);
   300		if (IS_ERR(blob)) {
   301			ret = PTR_ERR(blob);
   302			blob = NULL;
   303			goto fail;
   304		}
   305	
   306		/* Prepare GAMMA_LUT with the legacy values. */
 > 307		blob_data = blob->data;
   308		for (i = 0; i < size; i++) {
   309			blob_data[i].red = red[i];
   310			blob_data[i].green = green[i];
   311			blob_data[i].blue = blue[i];
   312		}
   313	
   314		state->acquire_ctx = ctx;
   315		crtc_state = drm_atomic_get_crtc_state(state, crtc);
   316		if (IS_ERR(crtc_state)) {
   317			ret = PTR_ERR(crtc_state);
   318			goto fail;
   319		}
   320	
   321		/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
   322		replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
   323						      crtc->has_gamma_prop ? NULL : blob);
   324		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
   325		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
   326						      crtc->has_gamma_prop ? blob : NULL);
   327		crtc_state->color_mgmt_changed |= replaced;
   328	
   329		ret = drm_atomic_commit(state);
   330	
   331	fail:
   332		drm_atomic_state_put(state);
   333		drm_property_blob_put(blob);
   334		return ret;
   335	}
   336	EXPORT_SYMBOL(drm_crtc_legacy_gamma_set);
   337	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36289 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4426 bytes --]

Hi Tomi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on linus/master v5.10-rc7]
[cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-m021-20201209 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

New smatch warnings:
drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)

Old smatch warnings:
drivers/gpu/drm/drm_color_mgmt.c:214 drm_mode_crtc_set_gamma_size() warn: double check that we're allocating correct size: 2 vs 6

vim +/blob +307 drivers/gpu/drm/drm_color_mgmt.c

   253	
   254	/**
   255	 * drm_crtc_legacy_gamma_set - set the legacy gamma correction table
   256	 * @crtc: CRTC object
   257	 * @red: red correction table
   258	 * @green: green correction table
   259	 * @blue: green correction table
   260	 * @size: size of the tables
   261	 * @ctx: lock acquire context
   262	 *
   263	 * Implements support for legacy gamma correction table for drivers
   264	 * that have set drm_crtc_funcs.gamma_set or that support color management
   265	 * through the DEGAMMA_LUT/GAMMA_LUT properties. See
   266	 * drm_crtc_enable_color_mgmt() and the containing chapter for
   267	 * how the atomic color management and gamma tables work.
   268	 *
   269	 * This function sets the gamma using the first one available:
   270	 * - drm_crtc_funcs.gamma_set()
   271	 * - GAMMA_LUT
   272	 * - DEGAMMA_LUT
   273	 */
   274	int drm_crtc_legacy_gamma_set(struct drm_crtc *crtc,
   275				      u16 *red, u16 *green, u16 *blue,
   276				      uint32_t size,
   277				      struct drm_modeset_acquire_ctx *ctx)
   278	{
   279		struct drm_device *dev = crtc->dev;
   280		struct drm_atomic_state *state;
   281		struct drm_crtc_state *crtc_state;
   282		struct drm_property_blob *blob = NULL;
   283		struct drm_color_lut *blob_data;
   284		int i, ret = 0;
   285		bool replaced;
   286	
   287		if (crtc->funcs->gamma_set)
   288			return crtc->funcs->gamma_set(crtc, red, green, blue, size, ctx);
   289	
   290		if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
   291			return -ENODEV;
   292	
   293		state = drm_atomic_state_alloc(crtc->dev);
   294		if (!state)
   295			return -ENOMEM;
   296	
   297		blob = drm_property_create_blob(dev,
   298						sizeof(struct drm_color_lut) * size,
   299						NULL);
   300		if (IS_ERR(blob)) {
   301			ret = PTR_ERR(blob);
   302			blob = NULL;
   303			goto fail;
   304		}
   305	
   306		/* Prepare GAMMA_LUT with the legacy values. */
 > 307		blob_data = blob->data;
   308		for (i = 0; i < size; i++) {
   309			blob_data[i].red = red[i];
   310			blob_data[i].green = green[i];
   311			blob_data[i].blue = blue[i];
   312		}
   313	
   314		state->acquire_ctx = ctx;
   315		crtc_state = drm_atomic_get_crtc_state(state, crtc);
   316		if (IS_ERR(crtc_state)) {
   317			ret = PTR_ERR(crtc_state);
   318			goto fail;
   319		}
   320	
   321		/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
   322		replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
   323						      crtc->has_gamma_prop ? NULL : blob);
   324		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
   325		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
   326						      crtc->has_gamma_prop ? blob : NULL);
   327		crtc_state->color_mgmt_changed |= replaced;
   328	
   329		ret = drm_atomic_commit(state);
   330	
   331	fail:
   332		drm_atomic_state_put(state);
   333		drm_property_blob_put(blob);
   334		return ret;
   335	}
   336	EXPORT_SYMBOL(drm_crtc_legacy_gamma_set);
   337	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 36289 bytes --]

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Tomi Valkeinen]

On 10/12/2020 20:06, kernel test robot wrote:
> Hi Tomi,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on drm-intel/for-linux-next]
> [also build test WARNING on linus/master v5.10-rc7]
> [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917
> base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> config: i386-randconfig-m021-20201209 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> New smatch warnings:
> drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)

I don't see how this could happen. There's no code path I see where drm_property_create_blob could
return null...

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]

On 10/12/2020 20:06, kernel test robot wrote:
> Hi Tomi,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on drm-intel/for-linux-next]
> [also build test WARNING on linus/master v5.10-rc7]
> [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917
> base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> config: i386-randconfig-m021-20201209 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> New smatch warnings:
> drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)

I don't see how this could happen. There's no code path I see where drm_property_create_blob could
return null...

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Ville Syrjälä]

On Fri, Dec 11, 2020 at 01:24:49PM +0200, Tomi Valkeinen wrote:
> On 10/12/2020 20:06, kernel test robot wrote:
> > Hi Tomi,
> > 
> > I love your patch! Perhaps something to improve:
> > 
> > [auto build test WARNING on drm-intel/for-linux-next]
> > [also build test WARNING on linus/master v5.10-rc7]
> > [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917
> > base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> > config: i386-randconfig-m021-20201209 (attached as .config)
> > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > New smatch warnings:
> > drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)
> 
> I don't see how this could happen. There's no code path I see where drm_property_create_blob could
> return null...

IIRC we've received multiple similar nonsense reports from lkp, but
no explanation why it thinks it could ever be null. Hmm, maybe there
is a codepath somewhere that has a null check on the return value?

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Ville Syrjälä]

[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]

On Fri, Dec 11, 2020 at 01:24:49PM +0200, Tomi Valkeinen wrote:
> On 10/12/2020 20:06, kernel test robot wrote:
> > Hi Tomi,
> > 
> > I love your patch! Perhaps something to improve:
> > 
> > [auto build test WARNING on drm-intel/for-linux-next]
> > [also build test WARNING on linus/master v5.10-rc7]
> > [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917
> > base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> > config: i386-randconfig-m021-20201209 (attached as .config)
> > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > New smatch warnings:
> > drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)
> 
> I don't see how this could happen. There's no code path I see where drm_property_create_blob could
> return null...

IIRC we've received multiple similar nonsense reports from lkp, but
no explanation why it thinks it could ever be null. Hmm, maybe there
is a codepath somewhere that has a null check on the return value?

-- 
Ville Syrjälä
Intel

Re: [kbuild-all] Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Philip Li]

On Fri, Dec 11, 2020 at 04:42:00PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 11, 2020 at 01:24:49PM +0200, Tomi Valkeinen wrote:
> > On 10/12/2020 20:06, kernel test robot wrote:
> > > Hi Tomi,
> > > 
> > > I love your patch! Perhaps something to improve:
> > > 
> > > [auto build test WARNING on drm-intel/for-linux-next]
> > > [also build test WARNING on linus/master v5.10-rc7]
> > > [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
> > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > And when submitting patch, we suggest to use '--base' as documented in
> > > https://git-scm.com/docs/git-format-patch]
> > > 
> > > url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917
> > > base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> > > config: i386-randconfig-m021-20201209 (attached as .config)
> > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > > 
> > > If you fix the issue, kindly add following tag as appropriate
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > 
> > > New smatch warnings:
> > > drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)
> > 
> > I don't see how this could happen. There's no code path I see where drm_property_create_blob could
> > return null...
> 
> IIRC we've received multiple similar nonsense reports from lkp, but
> no explanation why it thinks it could ever be null. Hmm, maybe there
> is a codepath somewhere that has a null check on the return value?
hi Dan, can you help on this to share some idea?

The original report is at
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/F3MVBRGF2R4URBNLNY3GMTSDZUCBX6RF/

Thanks


> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> kbuild-all mailing list -- kbuild-all@lists.01.org
> To unsubscribe send an email to kbuild-all-leave@lists.01.org
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Philip Li]

[-- Attachment #1: Type: text/plain, Size: 2068 bytes --]

On Fri, Dec 11, 2020 at 04:42:00PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 11, 2020 at 01:24:49PM +0200, Tomi Valkeinen wrote:
> > On 10/12/2020 20:06, kernel test robot wrote:
> > > Hi Tomi,
> > > 
> > > I love your patch! Perhaps something to improve:
> > > 
> > > [auto build test WARNING on drm-intel/for-linux-next]
> > > [also build test WARNING on linus/master v5.10-rc7]
> > > [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
> > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > And when submitting patch, we suggest to use '--base' as documented in
> > > https://git-scm.com/docs/git-format-patch]
> > > 
> > > url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917
> > > base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> > > config: i386-randconfig-m021-20201209 (attached as .config)
> > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > > 
> > > If you fix the issue, kindly add following tag as appropriate
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > 
> > > New smatch warnings:
> > > drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)
> > 
> > I don't see how this could happen. There's no code path I see where drm_property_create_blob could
> > return null...
> 
> IIRC we've received multiple similar nonsense reports from lkp, but
> no explanation why it thinks it could ever be null. Hmm, maybe there
> is a codepath somewhere that has a null check on the return value?
hi Dan, can you help on this to share some idea?

The original report is at
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org/thread/F3MVBRGF2R4URBNLNY3GMTSDZUCBX6RF/

Thanks


> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> kbuild-all mailing list -- kbuild-all(a)lists.01.org
> To unsubscribe send an email to kbuild-all-leave(a)lists.01.org

Re: [kbuild-all] Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Dan Carpenter]

On Sat, Dec 12, 2020 at 04:54:45PM +0800, Philip Li wrote:
> On Fri, Dec 11, 2020 at 04:42:00PM +0200, Ville Syrjälä wrote:
> > On Fri, Dec 11, 2020 at 01:24:49PM +0200, Tomi Valkeinen wrote:
> > > On 10/12/2020 20:06, kernel test robot wrote:
> > > > Hi Tomi,
> > > > 
> > > > I love your patch! Perhaps something to improve:
> > > > 
> > > > [auto build test WARNING on drm-intel/for-linux-next]
> > > > [also build test WARNING on linus/master v5.10-rc7]
> > > > [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
> > > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > > And when submitting patch, we suggest to use '--base' as documented in
> > > > https://git-scm.com/docs/git-format-patch ]
> > > > 
> > > > url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917 
> > > > base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> > > > config: i386-randconfig-m021-20201209 (attached as .config)
> > > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > > > 
> > > > If you fix the issue, kindly add following tag as appropriate
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > 
> > > > New smatch warnings:
> > > > drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)
> > > 
> > > I don't see how this could happen. There's no code path I see where drm_property_create_blob could
> > > return null...
> > 
> > IIRC we've received multiple similar nonsense reports from lkp, but
> > no explanation why it thinks it could ever be null. Hmm, maybe there
> > is a codepath somewhere that has a null check on the return value?
> hi Dan, can you help on this to share some idea?
> 
> The original report is at
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/F3MVBRGF2R4URBNLNY3GMTSDZUCBX6RF/ 

Thanks, Philip.  I've pushed a fix for this.

It didn't show up if you had the cross function DB built, so it's not
something I would see in my testing.

regards,
dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

On Sat, Dec 12, 2020 at 04:54:45PM +0800, Philip Li wrote:
> On Fri, Dec 11, 2020 at 04:42:00PM +0200, Ville Syrjälä wrote:
> > On Fri, Dec 11, 2020 at 01:24:49PM +0200, Tomi Valkeinen wrote:
> > > On 10/12/2020 20:06, kernel test robot wrote:
> > > > Hi Tomi,
> > > > 
> > > > I love your patch! Perhaps something to improve:
> > > > 
> > > > [auto build test WARNING on drm-intel/for-linux-next]
> > > > [also build test WARNING on linus/master v5.10-rc7]
> > > > [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
> > > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > > And when submitting patch, we suggest to use '--base' as documented in
> > > > https://git-scm.com/docs/git-format-patch ]
> > > > 
> > > > url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917 
> > > > base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> > > > config: i386-randconfig-m021-20201209 (attached as .config)
> > > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > > > 
> > > > If you fix the issue, kindly add following tag as appropriate
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > 
> > > > New smatch warnings:
> > > > drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)
> > > 
> > > I don't see how this could happen. There's no code path I see where drm_property_create_blob could
> > > return null...
> > 
> > IIRC we've received multiple similar nonsense reports from lkp, but
> > no explanation why it thinks it could ever be null. Hmm, maybe there
> > is a codepath somewhere that has a null check on the return value?
> hi Dan, can you help on this to share some idea?
> 
> The original report is at
> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org/thread/F3MVBRGF2R4URBNLNY3GMTSDZUCBX6RF/ 

Thanks, Philip.  I've pushed a fix for this.

It didn't show up if you had the cross function DB built, so it's not
something I would see in my testing.

regards,
dan carpenter

Re: [kbuild-all] Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Philip Li]

On Mon, Dec 14, 2020 at 09:49:17PM +0300, Dan Carpenter wrote:
> On Sat, Dec 12, 2020 at 04:54:45PM +0800, Philip Li wrote:
> > On Fri, Dec 11, 2020 at 04:42:00PM +0200, Ville Syrjälä wrote:
> > > On Fri, Dec 11, 2020 at 01:24:49PM +0200, Tomi Valkeinen wrote:
> > > > On 10/12/2020 20:06, kernel test robot wrote:
> > > > > Hi Tomi,
> > > > > 
> > > > > I love your patch! Perhaps something to improve:
> > > > > 
> > > > > [auto build test WARNING on drm-intel/for-linux-next]
> > > > > [also build test WARNING on linus/master v5.10-rc7]
> > > > > [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
> > > > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > > > And when submitting patch, we suggest to use '--base' as documented in
> > > > > https://git-scm.com/docs/git-format-patch ]
> > > > > 
> > > > > url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917 
> > > > > base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> > > > > config: i386-randconfig-m021-20201209 (attached as .config)
> > > > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > > > > 
> > > > > If you fix the issue, kindly add following tag as appropriate
> > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > > 
> > > > > New smatch warnings:
> > > > > drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)
> > > > 
> > > > I don't see how this could happen. There's no code path I see where drm_property_create_blob could
> > > > return null...
> > > 
> > > IIRC we've received multiple similar nonsense reports from lkp, but
> > > no explanation why it thinks it could ever be null. Hmm, maybe there
> > > is a codepath somewhere that has a null check on the return value?
> > hi Dan, can you help on this to share some idea?
> > 
> > The original report is at
> > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/F3MVBRGF2R4URBNLNY3GMTSDZUCBX6RF/ 
> 
> Thanks, Philip.  I've pushed a fix for this.
Thanks Dan for this, we will pack the new code to run in the robot.

> 
> It didn't show up if you had the cross function DB built, so it's not
> something I would see in my testing.
Got it, do you recommend this "cross function DB" is necessary for us to
setup or not a must?

Thanks

> 
> regards,
> dan carpenter
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Philip Li]

[-- Attachment #1: Type: text/plain, Size: 2495 bytes --]

On Mon, Dec 14, 2020 at 09:49:17PM +0300, Dan Carpenter wrote:
> On Sat, Dec 12, 2020 at 04:54:45PM +0800, Philip Li wrote:
> > On Fri, Dec 11, 2020 at 04:42:00PM +0200, Ville Syrjälä wrote:
> > > On Fri, Dec 11, 2020 at 01:24:49PM +0200, Tomi Valkeinen wrote:
> > > > On 10/12/2020 20:06, kernel test robot wrote:
> > > > > Hi Tomi,
> > > > > 
> > > > > I love your patch! Perhaps something to improve:
> > > > > 
> > > > > [auto build test WARNING on drm-intel/for-linux-next]
> > > > > [also build test WARNING on linus/master v5.10-rc7]
> > > > > [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
> > > > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > > > And when submitting patch, we suggest to use '--base' as documented in
> > > > > https://git-scm.com/docs/git-format-patch ]
> > > > > 
> > > > > url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917 
> > > > > base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> > > > > config: i386-randconfig-m021-20201209 (attached as .config)
> > > > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > > > > 
> > > > > If you fix the issue, kindly add following tag as appropriate
> > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > > 
> > > > > New smatch warnings:
> > > > > drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)
> > > > 
> > > > I don't see how this could happen. There's no code path I see where drm_property_create_blob could
> > > > return null...
> > > 
> > > IIRC we've received multiple similar nonsense reports from lkp, but
> > > no explanation why it thinks it could ever be null. Hmm, maybe there
> > > is a codepath somewhere that has a null check on the return value?
> > hi Dan, can you help on this to share some idea?
> > 
> > The original report is at
> > https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org/thread/F3MVBRGF2R4URBNLNY3GMTSDZUCBX6RF/ 
> 
> Thanks, Philip.  I've pushed a fix for this.
Thanks Dan for this, we will pack the new code to run in the robot.

> 
> It didn't show up if you had the cross function DB built, so it's not
> something I would see in my testing.
Got it, do you recommend this "cross function DB" is necessary for us to
setup or not a must?

Thanks

> 
> regards,
> dan carpenter
> 

Re: [kbuild-all] Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Dan Carpenter]

On Wed, Dec 16, 2020 at 09:06:50AM +0800, Philip Li wrote:
> On Mon, Dec 14, 2020 at 09:49:17PM +0300, Dan Carpenter wrote:
> > On Sat, Dec 12, 2020 at 04:54:45PM +0800, Philip Li wrote:
> > > On Fri, Dec 11, 2020 at 04:42:00PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Dec 11, 2020 at 01:24:49PM +0200, Tomi Valkeinen wrote:
> > > > > On 10/12/2020 20:06, kernel test robot wrote:
> > > > > > Hi Tomi,
> > > > > > 
> > > > > > I love your patch! Perhaps something to improve:
> > > > > > 
> > > > > > [auto build test WARNING on drm-intel/for-linux-next]
> > > > > > [also build test WARNING on linus/master v5.10-rc7]
> > > > > > [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
> > > > > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > > > > And when submitting patch, we suggest to use '--base' as documented in
> > > > > > https://git-scm.com/docs/git-format-patch  ]
> > > > > > 
> > > > > > url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917  
> > > > > > base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> > > > > > config: i386-randconfig-m021-20201209 (attached as .config)
> > > > > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > > > > > 
> > > > > > If you fix the issue, kindly add following tag as appropriate
> > > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > > > 
> > > > > > New smatch warnings:
> > > > > > drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)
> > > > > 
> > > > > I don't see how this could happen. There's no code path I see where drm_property_create_blob could
> > > > > return null...
> > > > 
> > > > IIRC we've received multiple similar nonsense reports from lkp, but
> > > > no explanation why it thinks it could ever be null. Hmm, maybe there
> > > > is a codepath somewhere that has a null check on the return value?
> > > hi Dan, can you help on this to share some idea?
> > > 
> > > The original report is at
> > > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/F3MVBRGF2R4URBNLNY3GMTSDZUCBX6RF/  
> > 
> > Thanks, Philip.  I've pushed a fix for this.
> Thanks Dan for this, we will pack the new code to run in the robot.
> 
> > 
> > It didn't show up if you had the cross function DB built, so it's not
> > something I would see in my testing.
> Got it, do you recommend this "cross function DB" is necessary for us to
> setup or not a must?

It's not a must.  Smatch is supposed to work fine without it.

regards,
dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: automatic legacy gamma support

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]

On Wed, Dec 16, 2020 at 09:06:50AM +0800, Philip Li wrote:
> On Mon, Dec 14, 2020 at 09:49:17PM +0300, Dan Carpenter wrote:
> > On Sat, Dec 12, 2020 at 04:54:45PM +0800, Philip Li wrote:
> > > On Fri, Dec 11, 2020 at 04:42:00PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Dec 11, 2020 at 01:24:49PM +0200, Tomi Valkeinen wrote:
> > > > > On 10/12/2020 20:06, kernel test robot wrote:
> > > > > > Hi Tomi,
> > > > > > 
> > > > > > I love your patch! Perhaps something to improve:
> > > > > > 
> > > > > > [auto build test WARNING on drm-intel/for-linux-next]
> > > > > > [also build test WARNING on linus/master v5.10-rc7]
> > > > > > [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210]
> > > > > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > > > > And when submitting patch, we suggest to use '--base' as documented in
> > > > > > https://git-scm.com/docs/git-format-patch  ]
> > > > > > 
> > > > > > url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917  
> > > > > > base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> > > > > > config: i386-randconfig-m021-20201209 (attached as .config)
> > > > > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > > > > > 
> > > > > > If you fix the issue, kindly add following tag as appropriate
> > > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > > > 
> > > > > > New smatch warnings:
> > > > > > drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: potential null dereference 'blob'.  (drm_property_create_blob returns null)
> > > > > 
> > > > > I don't see how this could happen. There's no code path I see where drm_property_create_blob could
> > > > > return null...
> > > > 
> > > > IIRC we've received multiple similar nonsense reports from lkp, but
> > > > no explanation why it thinks it could ever be null. Hmm, maybe there
> > > > is a codepath somewhere that has a null check on the return value?
> > > hi Dan, can you help on this to share some idea?
> > > 
> > > The original report is at
> > > https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org/thread/F3MVBRGF2R4URBNLNY3GMTSDZUCBX6RF/  
> > 
> > Thanks, Philip.  I've pushed a fix for this.
> Thanks Dan for this, we will pack the new code to run in the robot.
> 
> > 
> > It didn't show up if you had the cross function DB built, so it's not
> > something I would see in my testing.
> Got it, do you recommend this "cross function DB" is necessary for us to
> setup or not a must?

It's not a must.  Smatch is supposed to work fine without it.

regards,
dan carpenter