[PATCH 0/4] drm: rcar-du: Add cubic LUT support

[PATCH 0/4] drm: rcar-du: Add cubic LUT support

[Laurent Pinchart]

Hello,

This patch series adds cubic (3D) look up table support to the CMM and
DU drivers, and extend the KMS userspace API to expose the cubic LUT to
userspace.

The code is fairly straightforward. Patch 1/4 refactors the CMM (color
management module, the Renesas R-Car IP core that handles 1D and 3D
lookup tables for the display) driver, which currently supports the 1D
(a.k.a. gamma) table only, to prepare for 3D LUT support (including a
modification to the API between the CMM and DU drivers). The CMM driver
is then extended in patch 2/4 to support the 3D LUT.

Patch 3/4 adds support for the 3D LUT in the KMS core and the KMS
userspace API, in the form of two new properties. I expect this to be
the most controversial part of the series, not so much for the feature
itself, but for when it is inserted in the color management pipeline.

Finally, patch 4/4 wires the KMS extension to the DU driver.

The R-Car CMM applies the 3D LUT at the output of the display, where
data is transmitted in RGB space (when outputting YUV data to the
display the CMM can't be used according to the documentation, but I
wouldn't be entirely surprised if this limitation could be worked
around), before the 1D LUT. I've located the 3D LUT between the CTM and
the gamma LUT, but it could equally be placed before the degamma LUT or
between the degamma LUT and the CTM in my case, as the R-Car color
management pipeline has no CTM and has a single 1D LUT on the output
side (there's provision for 1D LUT on the input side for some of the
planes, but that's a separate topic).

I however don't expect this to necessarily match all hardware though,
and this feature may require us to give up on a fixed, one size fits
them all, color management pipeline exposed to userspace. Whether this
would mean device-specific APIs (not necessarily in the form of
device-specific properties, but in how they map to hardware features, as
I think helpers to handle a 3D LUT property in the KMS core can save
code duplication in drivers), or the need for a new property to expose
the order in which color management operations are implemented, I don't
know.

I started having a look at userspace to see how this could be handled,
searching for color management support in weston, kwin and wlroots/sway.
All three support setting the gamma table when using the DRM/KMS
backend, weston and kwin through the legacy API, and wlroots through the
atomic API. Weston reads an ICC profile using Little CMS and applies the
gamma table. kwin is a bit more elaborate, it also uses Little CMS to
read an ICC profile, but additionally supports setting the brightness
and color temperature. It however only sets a gamma table in the end.
Wlroots seems to have an API to set the gamma table, but I haven't seen
where sway uses it (I may have missed that though). In any case, there's
limited support there for color management.

Inputs would be appreciated on this, for instance with feedback on how
X.org and Android handle color management, on how 3D LUTs are
implemented on other platforms, or in general on how we would like to
use them. I don't mind doing some work in userspace to prototype this,
but I won't have the bandwidth to design a completely new framework.

Kieran Bingham (3):
  drm: rcar-du: cmm: Provide 3D-CLU support
  drm: Extend color correction to support 3D-CLU
  drm: rcar-du: kms: Configure the CLU

Laurent Pinchart (1):
  drm: rcar-du: cmm: Refactor LUT configuration

 drivers/gpu/drm/drm_atomic_helper.c       |   1 +
 drivers/gpu/drm/drm_atomic_state_helper.c |   3 +
 drivers/gpu/drm/drm_atomic_uapi.c         |  10 ++
 drivers/gpu/drm/drm_color_mgmt.c          |  41 ++++++--
 drivers/gpu/drm/drm_mode_config.c         |  14 +++
 drivers/gpu/drm/rcar-du/rcar_cmm.c        | 110 +++++++++++++++-------
 drivers/gpu/drm/rcar-du/rcar_cmm.h        |  22 +++--
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c    |  45 ++++++---
 include/drm/drm_crtc.h                    |   9 ++
 include/drm/drm_mode_config.h             |  11 +++
 10 files changed, 207 insertions(+), 59 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[PATCH 1/4] drm: rcar-du: cmm: Refactor LUT configuration

[Laurent Pinchart]

To prepare for CLU support, expend the CMM API exposed to the DU driver
to separate the LUT table pointer from the LUT update decision. This
will be required, as we will need to update the LUT and CLU
independently.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_cmm.c     | 60 ++++++++++++--------------
 drivers/gpu/drm/rcar-du/rcar_cmm.h     | 19 +++++---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 22 +++++++---
 3 files changed, 55 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
index 382d53f8a22e..ccc8c8b03bac 100644
--- a/drivers/gpu/drm/rcar-du/rcar_cmm.c
+++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
@@ -42,23 +42,33 @@ static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
 	iowrite32(data, rcmm->base + reg);
 }
 
-/*
- * rcar_cmm_lut_write() - Scale the DRM LUT table entries to hardware precision
- *			  and write to the CMM registers
- * @rcmm: Pointer to the CMM device
- * @drm_lut: Pointer to the DRM LUT table
- */
-static void rcar_cmm_lut_write(struct rcar_cmm *rcmm,
-			       const struct drm_color_lut *drm_lut)
+static void rcar_cmm_lut_configure(struct rcar_cmm *rcmm,
+				   const struct drm_color_lut *table)
 {
-	unsigned int i;
+	bool enable = !!table;
 
-	for (i = 0; i < CM2_LUT_SIZE; ++i) {
-		u32 entry = drm_color_lut_extract(drm_lut[i].red, 8) << 16
-			  | drm_color_lut_extract(drm_lut[i].green, 8) << 8
-			  | drm_color_lut_extract(drm_lut[i].blue, 8);
+	if (rcmm->lut.enabled != enable) {
+		rcar_cmm_write(rcmm, CM2_LUT_CTRL,
+			       enable ? CM2_LUT_CTRL_LUT_EN : 0);
+		rcmm->lut.enabled = enable;
+	}
 
-		rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry);
+	if (table) {
+		unsigned int i;
+
+		/*
+		 * Scale the DRM LUT table entries to the hardware precision
+		 * and program it.
+		 */
+		for (i = 0; i < CM2_LUT_SIZE; ++i) {
+			const struct drm_color_lut *lut = &table[i];
+
+			u32 entry = drm_color_lut_extract(lut->red, 8) << 16
+				  | drm_color_lut_extract(lut->green, 8) << 8
+				  | drm_color_lut_extract(lut->blue, 8);
+
+			rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry);
+		}
 	}
 }
 
@@ -83,23 +93,8 @@ int rcar_cmm_setup(struct platform_device *pdev,
 {
 	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
 
-	/* Disable LUT if no table is provided. */
-	if (!config->lut.table) {
-		if (rcmm->lut.enabled) {
-			rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
-			rcmm->lut.enabled = false;
-		}
-
-		return 0;
-	}
-
-	/* Enable LUT and program the new gamma table values. */
-	if (!rcmm->lut.enabled) {
-		rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);
-		rcmm->lut.enabled = true;
-	}
-
-	rcar_cmm_lut_write(rcmm, config->lut.table);
+	if (config->lut.update)
+		rcar_cmm_lut_configure(rcmm, config->lut.table);
 
 	return 0;
 }
@@ -144,8 +139,7 @@ void rcar_cmm_disable(struct platform_device *pdev)
 {
 	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
 
-	rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
-	rcmm->lut.enabled = false;
+	rcar_cmm_lut_configure(rcmm, NULL);
 
 	pm_runtime_put(&pdev->dev);
 }
diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h
index b5f7ec6db04a..f4b16535ec16 100644
--- a/drivers/gpu/drm/rcar-du/rcar_cmm.h
+++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
@@ -13,16 +13,23 @@
 struct drm_color_lut;
 struct platform_device;
 
+/**
+ * struct rcar_cmm_table_config - CMM LUT configuration
+ * @update: When true, update the LUT configuration.
+ * @table: Table data. The LUT is enabled if non-NULL, and disabled
+ *	otherwise. The value is ignored if @update is false.
+ */
+struct rcar_cmm_table_config {
+	bool update;
+	struct drm_color_lut *table;
+};
+
 /**
  * struct rcar_cmm_config - CMM configuration
- *
- * @lut:	1D-LUT configuration
- * @lut.table:	1D-LUT table entries. Disable LUT operations when NULL
+ * @lut: 1D-LUT configuration
  */
 struct rcar_cmm_config {
-	struct {
-		struct drm_color_lut *table;
-	} lut;
+	struct rcar_cmm_table_config lut;
 };
 
 #if IS_ENABLED(CONFIG_DRM_RCAR_CMM)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 9a099c0fe1d4..426b1870b3cb 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -500,17 +500,23 @@ static int rcar_du_cmm_check(struct drm_crtc *crtc,
 	return 0;
 }
 
-static void rcar_du_cmm_setup(struct drm_crtc *crtc)
+static void rcar_du_cmm_setup(struct rcar_du_crtc *rcrtc,
+			      const struct drm_crtc_state *old_state,
+			      const struct drm_crtc_state *new_state)
 {
-	struct drm_property_blob *drm_lut = crtc->state->gamma_lut;
-	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 	struct rcar_cmm_config cmm_config = {};
 
 	if (!rcrtc->cmm)
 		return;
 
-	if (drm_lut)
-		cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;
+	if (!old_state ||
+	    !old_state->gamma_lut != !new_state->gamma_lut ||
+	    (old_state->gamma_lut && new_state->gamma_lut &&
+	     old_state->gamma_lut->base.id != new_state->gamma_lut->base.id)) {
+		cmm_config.lut.update = true;
+		cmm_config.lut.table = new_state->gamma_lut
+				     ? new_state->gamma_lut->data : NULL;
+	}
 
 	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
 }
@@ -744,7 +750,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 	 * after the DU channel has been activated. Investigate the impact
 	 * of this restriction on the first displayed frame.
 	 */
-	rcar_du_cmm_setup(crtc);
+	rcar_du_cmm_setup(rcrtc, NULL, crtc->state);
 }
 
 static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
@@ -781,6 +787,8 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
 static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
 				      struct drm_atomic_state *state)
 {
+	struct drm_crtc_state *old_state = drm_atomic_get_old_crtc_state(state,
+									 crtc);
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 
 	WARN_ON(!crtc->state->enable);
@@ -801,7 +809,7 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
 
 	/* If the active state changed, we let .atomic_enable handle CMM. */
 	if (crtc->state->color_mgmt_changed && !crtc->state->active_changed)
-		rcar_du_cmm_setup(crtc);
+		rcar_du_cmm_setup(rcrtc, old_state, crtc->state);
 
 	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
 		rcar_du_vsp_atomic_begin(rcrtc);
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/4] drm: rcar-du: cmm: Refactor LUT configuration

[Kieran Bingham]

Hi Laurent,

On 21/12/2020 01:57, Laurent Pinchart wrote:
> To prepare for CLU support, expend the CMM API exposed to the DU driver

s/expend/extend/ ...?

> to separate the LUT table pointer from the LUT update decision. This
> will be required, as we will need to update the LUT and CLU
> independently.
> 

Aha, I see this has changed a little since I originally looked at this,
but that's probably a good thing.


> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_cmm.c     | 60 ++++++++++++--------------
>  drivers/gpu/drm/rcar-du/rcar_cmm.h     | 19 +++++---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 22 +++++++---
>  3 files changed, 55 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> index 382d53f8a22e..ccc8c8b03bac 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_cmm.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> @@ -42,23 +42,33 @@ static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
>  	iowrite32(data, rcmm->base + reg);
>  }
>  
> -/*
> - * rcar_cmm_lut_write() - Scale the DRM LUT table entries to hardware precision
> - *			  and write to the CMM registers
> - * @rcmm: Pointer to the CMM device
> - * @drm_lut: Pointer to the DRM LUT table
> - */
> -static void rcar_cmm_lut_write(struct rcar_cmm *rcmm,
> -			       const struct drm_color_lut *drm_lut)
> +static void rcar_cmm_lut_configure(struct rcar_cmm *rcmm,
> +				   const struct drm_color_lut *table)
>  {
> -	unsigned int i;
> +	bool enable = !!table;
>  

Ahh good, handling both enable and disable in here makes more sense. I
like it.


> -	for (i = 0; i < CM2_LUT_SIZE; ++i) {
> -		u32 entry = drm_color_lut_extract(drm_lut[i].red, 8) << 16
> -			  | drm_color_lut_extract(drm_lut[i].green, 8) << 8
> -			  | drm_color_lut_extract(drm_lut[i].blue, 8);
> +	if (rcmm->lut.enabled != enable) {
> +		rcar_cmm_write(rcmm, CM2_LUT_CTRL,
> +			       enable ? CM2_LUT_CTRL_LUT_EN : 0);
> +		rcmm->lut.enabled = enable;
> +	}
>  
> -		rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry);
> +	if (table) {
> +		unsigned int i;
> +
> +		/*
> +		 * Scale the DRM LUT table entries to the hardware precision
> +		 * and program it.
> +		 */
> +		for (i = 0; i < CM2_LUT_SIZE; ++i) {
> +			const struct drm_color_lut *lut = &table[i];
> +
> +			u32 entry = drm_color_lut_extract(lut->red, 8) << 16
> +				  | drm_color_lut_extract(lut->green, 8) << 8
> +				  | drm_color_lut_extract(lut->blue, 8);
> +
> +			rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry);
> +		}
>  	}
>  }
>  
> @@ -83,23 +93,8 @@ int rcar_cmm_setup(struct platform_device *pdev,
>  {
>  	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
>  
> -	/* Disable LUT if no table is provided. */
> -	if (!config->lut.table) {
> -		if (rcmm->lut.enabled) {
> -			rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> -			rcmm->lut.enabled = false;
> -		}
> -
> -		return 0;
> -	}
> -
> -	/* Enable LUT and program the new gamma table values. */
> -	if (!rcmm->lut.enabled) {
> -		rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);
> -		rcmm->lut.enabled = true;
> -	}
> -
> -	rcar_cmm_lut_write(rcmm, config->lut.table);
> +	if (config->lut.update)
> +		rcar_cmm_lut_configure(rcmm, config->lut.table);

Does something need to reset config->lut.update to false?
Or is this structure reset / cleaned on each call?

Never mind, looks like this is always used from a fresh initialised
structure in rcar_du_cmm_setup().


>  
>  	return 0;
>  }
> @@ -144,8 +139,7 @@ void rcar_cmm_disable(struct platform_device *pdev)
>  {
>  	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
>  
> -	rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> -	rcmm->lut.enabled = false;
> +	rcar_cmm_lut_configure(rcmm, NULL);
>  
>  	pm_runtime_put(&pdev->dev);
>  }
> diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> index b5f7ec6db04a..f4b16535ec16 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_cmm.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> @@ -13,16 +13,23 @@
>  struct drm_color_lut;
>  struct platform_device;
>  
> +/**
> + * struct rcar_cmm_table_config - CMM LUT configuration
> + * @update: When true, update the LUT configuration.
> + * @table: Table data. The LUT is enabled if non-NULL, and disabled
> + *	otherwise. The value is ignored if @update is false.
> + */
> +struct rcar_cmm_table_config {
> +	bool update;
> +	struct drm_color_lut *table;
> +};
> +
>  /**
>   * struct rcar_cmm_config - CMM configuration
> - *
> - * @lut:	1D-LUT configuration
> - * @lut.table:	1D-LUT table entries. Disable LUT operations when NULL
> + * @lut: 1D-LUT configuration
>   */
>  struct rcar_cmm_config {
> -	struct {
> -		struct drm_color_lut *table;
> -	} lut;
> +	struct rcar_cmm_table_config lut;
>  };
>  
>  #if IS_ENABLED(CONFIG_DRM_RCAR_CMM)
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 9a099c0fe1d4..426b1870b3cb 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -500,17 +500,23 @@ static int rcar_du_cmm_check(struct drm_crtc *crtc,
>  	return 0;
>  }
>  
> -static void rcar_du_cmm_setup(struct drm_crtc *crtc)
> +static void rcar_du_cmm_setup(struct rcar_du_crtc *rcrtc,
> +			      const struct drm_crtc_state *old_state,
> +			      const struct drm_crtc_state *new_state)
>  {
> -	struct drm_property_blob *drm_lut = crtc->state->gamma_lut;
> -	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>  	struct rcar_cmm_config cmm_config = {};
>  
>  	if (!rcrtc->cmm)
>  		return;
>  
> -	if (drm_lut)
> -		cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;
> +	if (!old_state ||
> +	    !old_state->gamma_lut != !new_state->gamma_lut ||
> +	    (old_state->gamma_lut && new_state->gamma_lut &&
> +	     old_state->gamma_lut->base.id != new_state->gamma_lut->base.id)) {


The conditional looks a bit terse, but it looks like it does the expected.

Everything else looks good to me.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> +		cmm_config.lut.update = true;
> +		cmm_config.lut.table = new_state->gamma_lut
> +				     ? new_state->gamma_lut->data : NULL;
> +	}
>  
>  	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
>  }
> @@ -744,7 +750,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>  	 * after the DU channel has been activated. Investigate the impact
>  	 * of this restriction on the first displayed frame.
>  	 */
> -	rcar_du_cmm_setup(crtc);
> +	rcar_du_cmm_setup(rcrtc, NULL, crtc->state);
>  }
>  
>  static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
> @@ -781,6 +787,8 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
>  static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
>  				      struct drm_atomic_state *state)
>  {
> +	struct drm_crtc_state *old_state = drm_atomic_get_old_crtc_state(state,
> +									 crtc);
>  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>  
>  	WARN_ON(!crtc->state->enable);
> @@ -801,7 +809,7 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
>  
>  	/* If the active state changed, we let .atomic_enable handle CMM. */
>  	if (crtc->state->color_mgmt_changed && !crtc->state->active_changed)
> -		rcar_du_cmm_setup(crtc);
> +		rcar_du_cmm_setup(rcrtc, old_state, crtc->state);
>  
>  	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		rcar_du_vsp_atomic_begin(rcrtc);
> 

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

[PATCH 2/4] drm: rcar-du: cmm: Provide 3D-CLU support

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The CMM module provides a three-dimensional cubic look up table that
converts three-color-component data into desired three color components
by use of a lookup table.

While the 1D-LUT can only control each of three color components
separately, the 3D-CLU can be used for specific color adjustment.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_cmm.c | 52 ++++++++++++++++++++++++++++--
 drivers/gpu/drm/rcar-du/rcar_cmm.h | 11 ++++---
 2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
index ccc8c8b03bac..9a20728a3534 100644
--- a/drivers/gpu/drm/rcar-du/rcar_cmm.c
+++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
@@ -17,9 +17,18 @@
 
 #define CM2_LUT_CTRL		0x0000
 #define CM2_LUT_CTRL_LUT_EN	BIT(0)
+
+#define CM2_CLU_CTRL		0x0100
+#define CM2_CLU_CTRL_CLU_EN	BIT(1)
+#define CM2_CLU_CTRL_MVS	BIT(24)
+#define CM2_CLU_CTRL_AAI	BIT(28)
+
 #define CM2_LUT_TBL_BASE	0x0600
 #define CM2_LUT_TBL(__i)	(CM2_LUT_TBL_BASE + (__i) * 4)
 
+#define CM2_CLU_ADDR		0x0a00
+#define CM2_CLU_DATA		0x0a04
+
 struct rcar_cmm {
 	void __iomem *base;
 
@@ -30,6 +39,10 @@ struct rcar_cmm {
 	struct {
 		bool enabled;
 	} lut;
+
+	struct {
+		bool enabled;
+	} clu;
 };
 
 static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
@@ -72,13 +85,44 @@ static void rcar_cmm_lut_configure(struct rcar_cmm *rcmm,
 	}
 }
 
+static void rcar_cmm_clu_configure(struct rcar_cmm *rcmm,
+				   const struct drm_color_lut *table)
+{
+	static const u32 cfg = CM2_CLU_CTRL_AAI
+			     | CM2_CLU_CTRL_MVS
+			     | CM2_CLU_CTRL_CLU_EN;
+	bool enable = !!table;
+
+	if (rcmm->clu.enabled != enable) {
+		rcar_cmm_write(rcmm, CM2_CLU_CTRL, enable ? cfg : 0);
+		rcmm->clu.enabled = enable;
+	}
+
+	if (table) {
+		unsigned int i;
+
+		/* Utilise CM2_CLU_CTRL_AAI (auto-increment). */
+		rcar_cmm_write(rcmm, CM2_CLU_ADDR, 0);
+
+		for (i = 0; i < CM2_CLU_SIZE; ++i) {
+			const struct drm_color_lut *lut = &table[i];
+
+			u32 entry = drm_color_lut_extract(lut->red, 8) << 16
+				  | drm_color_lut_extract(lut->green, 8) << 8
+				  | drm_color_lut_extract(lut->blue, 8);
+
+			rcar_cmm_write(rcmm, CM2_CLU_DATA, entry);
+		}
+	}
+}
+
 /*
  * rcar_cmm_setup() - Configure the CMM unit
  * @pdev: The platform device associated with the CMM instance
  * @config: The CMM unit configuration
  *
- * Configure the CMM unit with the given configuration. Currently enabling,
- * disabling and programming of the 1-D LUT unit is supported.
+ * Configure the CMM unit with the given configuration, handling both the
+ * 1-D LUT and the 3-D CLU.
  *
  * As rcar_cmm_setup() accesses the CMM registers the unit should be powered
  * and its functional clock enabled. To guarantee this, before any call to
@@ -96,6 +140,9 @@ int rcar_cmm_setup(struct platform_device *pdev,
 	if (config->lut.update)
 		rcar_cmm_lut_configure(rcmm, config->lut.table);
 
+	if (config->clu.update)
+		rcar_cmm_clu_configure(rcmm, config->clu.table);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(rcar_cmm_setup);
@@ -140,6 +187,7 @@ void rcar_cmm_disable(struct platform_device *pdev)
 	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
 
 	rcar_cmm_lut_configure(rcmm, NULL);
+	rcar_cmm_clu_configure(rcmm, NULL);
 
 	pm_runtime_put(&pdev->dev);
 }
diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h
index f4b16535ec16..35f901158cec 100644
--- a/drivers/gpu/drm/rcar-du/rcar_cmm.h
+++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
@@ -9,14 +9,15 @@
 #define __RCAR_CMM_H__
 
 #define CM2_LUT_SIZE		256
+#define CM2_CLU_SIZE		(17 * 17 * 17)
 
 struct drm_color_lut;
 struct platform_device;
 
 /**
- * struct rcar_cmm_table_config - CMM LUT configuration
- * @update: When true, update the LUT configuration.
- * @table: Table data. The LUT is enabled if non-NULL, and disabled
+ * struct rcar_cmm_table_config - CMM LUT and CLU configuration
+ * @update: When true, update the LUT or CLU configuration.
+ * @table: Table data. The LUT or CLU is enabled if non-NULL, and disabled
  *	otherwise. The value is ignored if @update is false.
  */
 struct rcar_cmm_table_config {
@@ -26,10 +27,12 @@ struct rcar_cmm_table_config {
 
 /**
  * struct rcar_cmm_config - CMM configuration
- * @lut: 1D-LUT configuration
+ * @lut: 1D LUT configuration
+ * @clu: 3D (cubic) LUT configuration
  */
 struct rcar_cmm_config {
 	struct rcar_cmm_table_config lut;
+	struct rcar_cmm_table_config clu;
 };
 
 #if IS_ENABLED(CONFIG_DRM_RCAR_CMM)
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 2/4] drm: rcar-du: cmm: Provide 3D-CLU support

[Kieran Bingham]

Hi Laurent,

On 21/12/2020 01:57, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The CMM module provides a three-dimensional cubic look up table that
> converts three-color-component data into desired three color components
> by use of a lookup table.
> 
> While the 1D-LUT can only control each of three color components
> separately, the 3D-CLU can be used for specific color adjustment.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

For the updates since I wrote the patch:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/rcar-du/rcar_cmm.c | 52 ++++++++++++++++++++++++++++--
>  drivers/gpu/drm/rcar-du/rcar_cmm.h | 11 ++++---
>  2 files changed, 57 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> index ccc8c8b03bac..9a20728a3534 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_cmm.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> @@ -17,9 +17,18 @@
>  
>  #define CM2_LUT_CTRL		0x0000
>  #define CM2_LUT_CTRL_LUT_EN	BIT(0)
> +
> +#define CM2_CLU_CTRL		0x0100
> +#define CM2_CLU_CTRL_CLU_EN	BIT(1)
> +#define CM2_CLU_CTRL_MVS	BIT(24)
> +#define CM2_CLU_CTRL_AAI	BIT(28)
> +
>  #define CM2_LUT_TBL_BASE	0x0600
>  #define CM2_LUT_TBL(__i)	(CM2_LUT_TBL_BASE + (__i) * 4)
>  
> +#define CM2_CLU_ADDR		0x0a00
> +#define CM2_CLU_DATA		0x0a04
> +
>  struct rcar_cmm {
>  	void __iomem *base;
>  
> @@ -30,6 +39,10 @@ struct rcar_cmm {
>  	struct {
>  		bool enabled;
>  	} lut;
> +
> +	struct {
> +		bool enabled;
> +	} clu;
>  };
>  
>  static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> @@ -72,13 +85,44 @@ static void rcar_cmm_lut_configure(struct rcar_cmm *rcmm,
>  	}
>  }
>  
> +static void rcar_cmm_clu_configure(struct rcar_cmm *rcmm,
> +				   const struct drm_color_lut *table)
> +{
> +	static const u32 cfg = CM2_CLU_CTRL_AAI
> +			     | CM2_CLU_CTRL_MVS
> +			     | CM2_CLU_CTRL_CLU_EN;
> +	bool enable = !!table;

Good, not sure why I was calculating this outside of the call.


> +
> +	if (rcmm->clu.enabled != enable) {
> +		rcar_cmm_write(rcmm, CM2_CLU_CTRL, enable ? cfg : 0);
> +		rcmm->clu.enabled = enable;
> +	}
> +
> +	if (table) {
> +		unsigned int i;
> +
> +		/* Utilise CM2_CLU_CTRL_AAI (auto-increment). */
> +		rcar_cmm_write(rcmm, CM2_CLU_ADDR, 0);
> +
> +		for (i = 0; i < CM2_CLU_SIZE; ++i) {
> +			const struct drm_color_lut *lut = &table[i];
> +
> +			u32 entry = drm_color_lut_extract(lut->red, 8) << 16
> +				  | drm_color_lut_extract(lut->green, 8) << 8
> +				  | drm_color_lut_extract(lut->blue, 8);
> +
> +			rcar_cmm_write(rcmm, CM2_CLU_DATA, entry);
> +		}
> +	}
> +}
> +
>  /*
>   * rcar_cmm_setup() - Configure the CMM unit
>   * @pdev: The platform device associated with the CMM instance
>   * @config: The CMM unit configuration
>   *
> - * Configure the CMM unit with the given configuration. Currently enabling,
> - * disabling and programming of the 1-D LUT unit is supported.
> + * Configure the CMM unit with the given configuration, handling both the
> + * 1-D LUT and the 3-D CLU.
>   *
>   * As rcar_cmm_setup() accesses the CMM registers the unit should be powered
>   * and its functional clock enabled. To guarantee this, before any call to
> @@ -96,6 +140,9 @@ int rcar_cmm_setup(struct platform_device *pdev,
>  	if (config->lut.update)
>  		rcar_cmm_lut_configure(rcmm, config->lut.table);
>  
> +	if (config->clu.update)
> +		rcar_cmm_clu_configure(rcmm, config->clu.table);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(rcar_cmm_setup);
> @@ -140,6 +187,7 @@ void rcar_cmm_disable(struct platform_device *pdev)
>  	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
>  
>  	rcar_cmm_lut_configure(rcmm, NULL);
> +	rcar_cmm_clu_configure(rcmm, NULL);
>  
>  	pm_runtime_put(&pdev->dev);
>  }
> diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> index f4b16535ec16..35f901158cec 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_cmm.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> @@ -9,14 +9,15 @@
>  #define __RCAR_CMM_H__
>  
>  #define CM2_LUT_SIZE		256
> +#define CM2_CLU_SIZE		(17 * 17 * 17)
>  
>  struct drm_color_lut;
>  struct platform_device;
>  
>  /**
> - * struct rcar_cmm_table_config - CMM LUT configuration
> - * @update: When true, update the LUT configuration.
> - * @table: Table data. The LUT is enabled if non-NULL, and disabled
> + * struct rcar_cmm_table_config - CMM LUT and CLU configuration
> + * @update: When true, update the LUT or CLU configuration.
> + * @table: Table data. The LUT or CLU is enabled if non-NULL, and disabled
>   *	otherwise. The value is ignored if @update is false.
>   */
>  struct rcar_cmm_table_config {
> @@ -26,10 +27,12 @@ struct rcar_cmm_table_config {
>  
>  /**
>   * struct rcar_cmm_config - CMM configuration
> - * @lut: 1D-LUT configuration
> + * @lut: 1D LUT configuration
> + * @clu: 3D (cubic) LUT configuration
>   */
>  struct rcar_cmm_config {
>  	struct rcar_cmm_table_config lut;
> +	struct rcar_cmm_table_config clu;
>  };
>  
>  #if IS_ENABLED(CONFIG_DRM_RCAR_CMM)
> 

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

[PATCH 3/4] drm: Extend color correction to support 3D-CLU

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Extend the existing color management properties to support provision
of a 3D cubic look up table, allowing for color specific adjustments.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Co-developed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/drm_atomic_helper.c       |  1 +
 drivers/gpu/drm/drm_atomic_state_helper.c |  3 ++
 drivers/gpu/drm/drm_atomic_uapi.c         | 10 ++++++
 drivers/gpu/drm/drm_color_mgmt.c          | 41 +++++++++++++++++++----
 drivers/gpu/drm/drm_mode_config.c         | 14 ++++++++
 include/drm/drm_crtc.h                    |  9 +++++
 include/drm/drm_mode_config.h             | 11 ++++++
 7 files changed, 82 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ba1507036f26..0f54897d3c8d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
 	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
 	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->cubic_lut, NULL);
 	crtc_state->color_mgmt_changed |= replaced;
 
 	ret = drm_atomic_commit(state);
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index ddcf5c2c8e6a..61c685b50677 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 		drm_property_blob_get(state->ctm);
 	if (state->gamma_lut)
 		drm_property_blob_get(state->gamma_lut);
+	if (state->cubic_lut)
+		drm_property_blob_get(state->cubic_lut);
 	state->mode_changed = false;
 	state->active_changed = false;
 	state->planes_changed = false;
@@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
 	drm_property_blob_put(state->degamma_lut);
 	drm_property_blob_put(state->ctm);
 	drm_property_blob_put(state->gamma_lut);
+	drm_property_blob_put(state->cubic_lut);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 268bb69c2e2f..07229acab71c 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 					&replaced);
 		state->color_mgmt_changed |= replaced;
 		return ret;
+	} else if (property == config->cubic_lut_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+					&state->cubic_lut,
+					val,
+					-1, sizeof(struct drm_color_lut),
+					&replaced);
+		state->color_mgmt_changed |= replaced;
+		return ret;
 	} else if (property == config->prop_out_fence_ptr) {
 		s32 __user *fence_ptr = u64_to_user_ptr(val);
 
@@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 		*val = (state->ctm) ? state->ctm->base.id : 0;
 	else if (property == config->gamma_lut_property)
 		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
+	else if (property == config->cubic_lut_property)
+		*val = (state->cubic_lut) ? state->cubic_lut->base.id : 0;
 	else if (property == config->prop_out_fence_ptr)
 		*val = 0;
 	else if (property == crtc->scaling_filter_property)
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 3bcabc2f6e0e..85bbbc8ce8e5 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -33,7 +33,7 @@
 /**
  * DOC: overview
  *
- * Color management or color space adjustments is supported through a set of 5
+ * Color management or color space adjustments is supported through a set of 7
  * properties on the &drm_crtc object. They are set up by calling
  * drm_crtc_enable_color_mgmt().
  *
@@ -60,7 +60,7 @@
  * “CTM”:
  *	Blob property to set the current transformation matrix (CTM) apply to
  *	pixel data after the lookup through the degamma LUT and before the
- *	lookup through the gamma LUT. The data is interpreted as a struct
+ *	lookup through the cubic LUT. The data is interpreted as a struct
  *	&drm_color_ctm.
  *
  *	Setting this to NULL (blob property value set to 0) means a
@@ -68,13 +68,40 @@
  *	boot-up state too. Drivers can access the blob for the color conversion
  *	matrix through &drm_crtc_state.ctm.
  *
+ * ”CUBIC_LUT”:
+ *	Blob property to set the cubic (3D) lookup table performing color
+ *	mapping after the transformation matrix and before the lookup through
+ *	the gamma LUT. Unlike the degamma and gamma LUTs that map color
+ *	components independently, the 3D LUT converts an input color to an
+ *	output color by indexing into the 3D table using the color components
+ *	as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
+ *	would require too much storage space in the hardware, so the precision
+ *	of the color components is reduced before the look up, and the low
+ *	order bits may be used to interpolate between the nearest points in 3D
+ *	space.
+ *
+ *	The data is interpreted as an array of &struct drm_color_lut elements.
+ *	Hardware might choose not to use the full precision of the LUT
+ *	elements.
+ *
+ *	Setting this to NULL (blob property value set to 0) means the output
+ *	color is identical to the input color. This is generally the driver
+ *	boot-up state too. Drivers can access this blob through
+ *	&drm_crtc_state.cubic_lut.
+ *
+ * ”CUBIC_LUT_SIZE”:
+ *	Unsigned range property to give the size of the lookup table to be set
+ *	on the CUBIC_LUT property (the size depends on the underlying hardware).
+ *	If drivers support multiple LUT sizes then they should publish the
+ *	largest size, and sub-sample smaller sized LUTs appropriately.
+ *
  * “GAMMA_LUT”:
  *	Blob property to set the gamma lookup table (LUT) mapping pixel data
- *	after the transformation matrix to data sent to the connector. The
- *	data is interpreted as an array of &struct drm_color_lut elements.
- *	Hardware might choose not to use the full precision of the LUT elements
- *	nor use all the elements of the LUT (for example the hardware might
- *	choose to interpolate between LUT[0] and LUT[4]).
+ *	after the cubic LUT to data sent to the connector. The data is
+ *	interpreted as an array of &struct drm_color_lut elements. Hardware
+ *	might choose not to use the full precision of the LUT elements nor use
+ *	all the elements of the LUT (for example the hardware might choose to
+ *	interpolate between LUT[0] and LUT[4]).
  *
  *	Setting this to NULL (blob property value set to 0) means a
  *	linear/pass-thru gamma table should be used. This is generally the
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index f1affc1bb679..6c3324f60e7d 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.gamma_lut_size_property = prop;
 
+	prop = drm_property_create(dev,
+			DRM_MODE_PROP_BLOB,
+			"CUBIC_LUT", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.cubic_lut_property = prop;
+
+	prop = drm_property_create_range(dev,
+			DRM_MODE_PROP_IMMUTABLE,
+			"CUBIC_LUT_SIZE", 0, UINT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.cubic_lut_size_property = prop;
+
 	prop = drm_property_create(dev,
 				   DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
 				   "IN_FORMATS", 0);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 5f43d64d2a07..df5cc2239adb 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -288,6 +288,15 @@ struct drm_crtc_state {
 	 */
 	struct drm_property_blob *gamma_lut;
 
+	/**
+	 * @cubic_lut:
+	 *
+	 * Cubic Lookup table for converting pixel data. See
+	 * drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array
+	 * of &struct drm_color_lut.
+	 */
+	struct drm_property_blob *cubic_lut;
+
 	/**
 	 * @target_vblank:
 	 *
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index ab424ddd7665..8edb0094e5a7 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -800,6 +800,17 @@ struct drm_mode_config {
 	 */
 	struct drm_property *gamma_lut_size_property;
 
+	/**
+	 * @cubic_lut_property: Optional CRTC property to set the 3D LUT used to
+	 * convert color spaces.
+	 */
+	struct drm_property *cubic_lut_property;
+	/**
+	 * @cubic_lut_size_property: Optional CRTC property for the size of the
+	 * 3D LUT as supported by the driver (read-only).
+	 */
+	struct drm_property *cubic_lut_size_property;
+
 	/**
 	 * @suggested_x_property: Optional connector property with a hint for
 	 * the position of the output on the host's screen.
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 3/4] drm: Extend color correction to support 3D-CLU

[Kieran Bingham]

Hi Laurent,

On 21/12/2020 01:57, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Extend the existing color management properties to support provision
> of a 3D cubic look up table, allowing for color specific adjustments.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Co-developed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Again, for the updates since I created the patch:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

A bit of a question on clarifying the added documentation but I don't
think it's major.

> ---
>  drivers/gpu/drm/drm_atomic_helper.c       |  1 +
>  drivers/gpu/drm/drm_atomic_state_helper.c |  3 ++
>  drivers/gpu/drm/drm_atomic_uapi.c         | 10 ++++++
>  drivers/gpu/drm/drm_color_mgmt.c          | 41 +++++++++++++++++++----
>  drivers/gpu/drm/drm_mode_config.c         | 14 ++++++++
>  include/drm/drm_crtc.h                    |  9 +++++
>  include/drm/drm_mode_config.h             | 11 ++++++
>  7 files changed, 82 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ba1507036f26..0f54897d3c8d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>  	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
>  	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->cubic_lut, NULL);
>  	crtc_state->color_mgmt_changed |= replaced;
>  
>  	ret = drm_atomic_commit(state);
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index ddcf5c2c8e6a..61c685b50677 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>  		drm_property_blob_get(state->ctm);
>  	if (state->gamma_lut)
>  		drm_property_blob_get(state->gamma_lut);
> +	if (state->cubic_lut)
> +		drm_property_blob_get(state->cubic_lut);
>  	state->mode_changed = false;
>  	state->active_changed = false;
>  	state->planes_changed = false;
> @@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
>  	drm_property_blob_put(state->degamma_lut);
>  	drm_property_blob_put(state->ctm);
>  	drm_property_blob_put(state->gamma_lut);
> +	drm_property_blob_put(state->cubic_lut);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 268bb69c2e2f..07229acab71c 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  					&replaced);
>  		state->color_mgmt_changed |= replaced;
>  		return ret;
> +	} else if (property == config->cubic_lut_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> +					&state->cubic_lut,
> +					val,
> +					-1, sizeof(struct drm_color_lut),
> +					&replaced);
> +		state->color_mgmt_changed |= replaced;
> +		return ret;
>  	} else if (property == config->prop_out_fence_ptr) {
>  		s32 __user *fence_ptr = u64_to_user_ptr(val);
>  
> @@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>  		*val = (state->ctm) ? state->ctm->base.id : 0;
>  	else if (property == config->gamma_lut_property)
>  		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> +	else if (property == config->cubic_lut_property)
> +		*val = (state->cubic_lut) ? state->cubic_lut->base.id : 0;
>  	else if (property == config->prop_out_fence_ptr)
>  		*val = 0;
>  	else if (property == crtc->scaling_filter_property)
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 3bcabc2f6e0e..85bbbc8ce8e5 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -33,7 +33,7 @@
>  /**
>   * DOC: overview
>   *
> - * Color management or color space adjustments is supported through a set of 5
> + * Color management or color space adjustments is supported through a set of 7
>   * properties on the &drm_crtc object. They are set up by calling
>   * drm_crtc_enable_color_mgmt().
>   *
> @@ -60,7 +60,7 @@
>   * “CTM”:
>   *	Blob property to set the current transformation matrix (CTM) apply to
>   *	pixel data after the lookup through the degamma LUT and before the
> - *	lookup through the gamma LUT. The data is interpreted as a struct
> + *	lookup through the cubic LUT. The data is interpreted as a struct
>   *	&drm_color_ctm.
>   *
>   *	Setting this to NULL (blob property value set to 0) means a
> @@ -68,13 +68,40 @@
>   *	boot-up state too. Drivers can access the blob for the color conversion
>   *	matrix through &drm_crtc_state.ctm.
>   *
> + * ”CUBIC_LUT”:
> + *	Blob property to set the cubic (3D) lookup table performing color
> + *	mapping after the transformation matrix and before the lookup through
> + *	the gamma LUT. Unlike the degamma and gamma LUTs that map color
> + *	components independently, the 3D LUT converts an input color to an
> + *	output color by indexing into the 3D table using the color components
> + *	as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
> + *	would require too much storage space in the hardware, so the precision

This sentence is a bit confusing. What bit depth is actually used, is it
mandated to a specific subsampling? Or restricted to 8-bit....


> + *	of the color components is reduced before the look up, and the low
> + *	order bits may be used to interpolate between the nearest points in 3D
> + *	space.
> + *
> + *	The data is interpreted as an array of &struct drm_color_lut elements.
> + *	Hardware might choose not to use the full precision of the LUT
> + *	elements.

Is the table already reduced precision though ?


> + *
> + *	Setting this to NULL (blob property value set to 0) means the output
> + *	color is identical to the input color. This is generally the driver
> + *	boot-up state too. Drivers can access this blob through
> + *	&drm_crtc_state.cubic_lut.
> + *
> + * ”CUBIC_LUT_SIZE”:
> + *	Unsigned range property to give the size of the lookup table to be set
> + *	on the CUBIC_LUT property (the size depends on the underlying hardware).
> + *	If drivers support multiple LUT sizes then they should publish the
> + *	largest size, and sub-sample smaller sized LUTs appropriately.
> + *
>   * “GAMMA_LUT”:
>   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
> - *	after the transformation matrix to data sent to the connector. The
> - *	data is interpreted as an array of &struct drm_color_lut elements.
> - *	Hardware might choose not to use the full precision of the LUT elements
> - *	nor use all the elements of the LUT (for example the hardware might
> - *	choose to interpolate between LUT[0] and LUT[4]).
> + *	after the cubic LUT to data sent to the connector. The data is
> + *	interpreted as an array of &struct drm_color_lut elements. Hardware
> + *	might choose not to use the full precision of the LUT elements nor use
> + *	all the elements of the LUT (for example the hardware might choose to
> + *	interpolate between LUT[0] and LUT[4]).
>   *
>   *	Setting this to NULL (blob property value set to 0) means a
>   *	linear/pass-thru gamma table should be used. This is generally the
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index f1affc1bb679..6c3324f60e7d 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.gamma_lut_size_property = prop;
>  
> +	prop = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB,
> +			"CUBIC_LUT", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cubic_lut_property = prop;
> +
> +	prop = drm_property_create_range(dev,
> +			DRM_MODE_PROP_IMMUTABLE,
> +			"CUBIC_LUT_SIZE", 0, UINT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cubic_lut_size_property = prop;
> +
>  	prop = drm_property_create(dev,
>  				   DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
>  				   "IN_FORMATS", 0);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 5f43d64d2a07..df5cc2239adb 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -288,6 +288,15 @@ struct drm_crtc_state {
>  	 */
>  	struct drm_property_blob *gamma_lut;
>  
> +	/**
> +	 * @cubic_lut:
> +	 *
> +	 * Cubic Lookup table for converting pixel data. See
> +	 * drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array
> +	 * of &struct drm_color_lut.
> +	 */
> +	struct drm_property_blob *cubic_lut;
> +
>  	/**
>  	 * @target_vblank:
>  	 *
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index ab424ddd7665..8edb0094e5a7 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -800,6 +800,17 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *gamma_lut_size_property;
>  
> +	/**
> +	 * @cubic_lut_property: Optional CRTC property to set the 3D LUT used to
> +	 * convert color spaces.
> +	 */
> +	struct drm_property *cubic_lut_property;
> +	/**
> +	 * @cubic_lut_size_property: Optional CRTC property for the size of the
> +	 * 3D LUT as supported by the driver (read-only).
> +	 */
> +	struct drm_property *cubic_lut_size_property;
> +
>  	/**
>  	 * @suggested_x_property: Optional connector property with a hint for
>  	 * the position of the output on the host's screen.
> 

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

Re: [PATCH 3/4] drm: Extend color correction to support 3D-CLU

[Laurent Pinchart]

Hi Kieran,

On Mon, Dec 21, 2020 at 02:09:09PM +0000, Kieran Bingham wrote:
> On 21/12/2020 01:57, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > Extend the existing color management properties to support provision
> > of a 3D cubic look up table, allowing for color specific adjustments.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Co-developed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Again, for the updates since I created the patch:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> A bit of a question on clarifying the added documentation but I don't
> think it's major.
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c       |  1 +
> >  drivers/gpu/drm/drm_atomic_state_helper.c |  3 ++
> >  drivers/gpu/drm/drm_atomic_uapi.c         | 10 ++++++
> >  drivers/gpu/drm/drm_color_mgmt.c          | 41 +++++++++++++++++++----
> >  drivers/gpu/drm/drm_mode_config.c         | 14 ++++++++
> >  include/drm/drm_crtc.h                    |  9 +++++
> >  include/drm/drm_mode_config.h             | 11 ++++++
> >  7 files changed, 82 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index ba1507036f26..0f54897d3c8d 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> >  	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
> >  	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->cubic_lut, NULL);
> >  	crtc_state->color_mgmt_changed |= replaced;
> >  
> >  	ret = drm_atomic_commit(state);
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index ddcf5c2c8e6a..61c685b50677 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> >  		drm_property_blob_get(state->ctm);
> >  	if (state->gamma_lut)
> >  		drm_property_blob_get(state->gamma_lut);
> > +	if (state->cubic_lut)
> > +		drm_property_blob_get(state->cubic_lut);
> >  	state->mode_changed = false;
> >  	state->active_changed = false;
> >  	state->planes_changed = false;
> > @@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
> >  	drm_property_blob_put(state->degamma_lut);
> >  	drm_property_blob_put(state->ctm);
> >  	drm_property_blob_put(state->gamma_lut);
> > +	drm_property_blob_put(state->cubic_lut);
> >  }
> >  EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
> >  
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 268bb69c2e2f..07229acab71c 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> >  					&replaced);
> >  		state->color_mgmt_changed |= replaced;
> >  		return ret;
> > +	} else if (property == config->cubic_lut_property) {
> > +		ret = drm_atomic_replace_property_blob_from_id(dev,
> > +					&state->cubic_lut,
> > +					val,
> > +					-1, sizeof(struct drm_color_lut),
> > +					&replaced);
> > +		state->color_mgmt_changed |= replaced;
> > +		return ret;
> >  	} else if (property == config->prop_out_fence_ptr) {
> >  		s32 __user *fence_ptr = u64_to_user_ptr(val);
> >  
> > @@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> >  		*val = (state->ctm) ? state->ctm->base.id : 0;
> >  	else if (property == config->gamma_lut_property)
> >  		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> > +	else if (property == config->cubic_lut_property)
> > +		*val = (state->cubic_lut) ? state->cubic_lut->base.id : 0;
> >  	else if (property == config->prop_out_fence_ptr)
> >  		*val = 0;
> >  	else if (property == crtc->scaling_filter_property)
> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > index 3bcabc2f6e0e..85bbbc8ce8e5 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -33,7 +33,7 @@
> >  /**
> >   * DOC: overview
> >   *
> > - * Color management or color space adjustments is supported through a set of 5
> > + * Color management or color space adjustments is supported through a set of 7
> >   * properties on the &drm_crtc object. They are set up by calling
> >   * drm_crtc_enable_color_mgmt().
> >   *
> > @@ -60,7 +60,7 @@
> >   * “CTM”:
> >   *	Blob property to set the current transformation matrix (CTM) apply to
> >   *	pixel data after the lookup through the degamma LUT and before the
> > - *	lookup through the gamma LUT. The data is interpreted as a struct
> > + *	lookup through the cubic LUT. The data is interpreted as a struct
> >   *	&drm_color_ctm.
> >   *
> >   *	Setting this to NULL (blob property value set to 0) means a
> > @@ -68,13 +68,40 @@
> >   *	boot-up state too. Drivers can access the blob for the color conversion
> >   *	matrix through &drm_crtc_state.ctm.
> >   *
> > + * ”CUBIC_LUT”:
> > + *	Blob property to set the cubic (3D) lookup table performing color
> > + *	mapping after the transformation matrix and before the lookup through
> > + *	the gamma LUT. Unlike the degamma and gamma LUTs that map color
> > + *	components independently, the 3D LUT converts an input color to an
> > + *	output color by indexing into the 3D table using the color components
> > + *	as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
> > + *	would require too much storage space in the hardware, so the precision
> 
> This sentence is a bit confusing. What bit depth is actually used, is it
> mandated to a specific subsampling? Or restricted to 8-bit....

We're both confused then, because I don't think I understand the
question :-)

> > + *	of the color components is reduced before the look up, and the low
> > + *	order bits may be used to interpolate between the nearest points in 3D
> > + *	space.
> > + *
> > + *	The data is interpreted as an array of &struct drm_color_lut elements.
> > + *	Hardware might choose not to use the full precision of the LUT
> > + *	elements.
> 
> Is the table already reduced precision though ?

drm_color_lut has 16-bit precision. The precision of the table on the
hardware side varies.

> > + *
> > + *	Setting this to NULL (blob property value set to 0) means the output
> > + *	color is identical to the input color. This is generally the driver
> > + *	boot-up state too. Drivers can access this blob through
> > + *	&drm_crtc_state.cubic_lut.
> > + *
> > + * ”CUBIC_LUT_SIZE”:
> > + *	Unsigned range property to give the size of the lookup table to be set
> > + *	on the CUBIC_LUT property (the size depends on the underlying hardware).
> > + *	If drivers support multiple LUT sizes then they should publish the
> > + *	largest size, and sub-sample smaller sized LUTs appropriately.
> > + *
> >   * “GAMMA_LUT”:
> >   *	Blob property to set the gamma lookup table (LUT) mapping pixel data
> > - *	after the transformation matrix to data sent to the connector. The
> > - *	data is interpreted as an array of &struct drm_color_lut elements.
> > - *	Hardware might choose not to use the full precision of the LUT elements
> > - *	nor use all the elements of the LUT (for example the hardware might
> > - *	choose to interpolate between LUT[0] and LUT[4]).
> > + *	after the cubic LUT to data sent to the connector. The data is
> > + *	interpreted as an array of &struct drm_color_lut elements. Hardware
> > + *	might choose not to use the full precision of the LUT elements nor use
> > + *	all the elements of the LUT (for example the hardware might choose to
> > + *	interpolate between LUT[0] and LUT[4]).
> >   *
> >   *	Setting this to NULL (blob property value set to 0) means a
> >   *	linear/pass-thru gamma table should be used. This is generally the
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index f1affc1bb679..6c3324f60e7d 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> >  		return -ENOMEM;
> >  	dev->mode_config.gamma_lut_size_property = prop;
> >  
> > +	prop = drm_property_create(dev,
> > +			DRM_MODE_PROP_BLOB,
> > +			"CUBIC_LUT", 0);
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	dev->mode_config.cubic_lut_property = prop;
> > +
> > +	prop = drm_property_create_range(dev,
> > +			DRM_MODE_PROP_IMMUTABLE,
> > +			"CUBIC_LUT_SIZE", 0, UINT_MAX);
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	dev->mode_config.cubic_lut_size_property = prop;
> > +
> >  	prop = drm_property_create(dev,
> >  				   DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
> >  				   "IN_FORMATS", 0);
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 5f43d64d2a07..df5cc2239adb 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -288,6 +288,15 @@ struct drm_crtc_state {
> >  	 */
> >  	struct drm_property_blob *gamma_lut;
> >  
> > +	/**
> > +	 * @cubic_lut:
> > +	 *
> > +	 * Cubic Lookup table for converting pixel data. See
> > +	 * drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array
> > +	 * of &struct drm_color_lut.
> > +	 */
> > +	struct drm_property_blob *cubic_lut;
> > +
> >  	/**
> >  	 * @target_vblank:
> >  	 *
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index ab424ddd7665..8edb0094e5a7 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -800,6 +800,17 @@ struct drm_mode_config {
> >  	 */
> >  	struct drm_property *gamma_lut_size_property;
> >  
> > +	/**
> > +	 * @cubic_lut_property: Optional CRTC property to set the 3D LUT used to
> > +	 * convert color spaces.
> > +	 */
> > +	struct drm_property *cubic_lut_property;
> > +	/**
> > +	 * @cubic_lut_size_property: Optional CRTC property for the size of the
> > +	 * 3D LUT as supported by the driver (read-only).
> > +	 */
> > +	struct drm_property *cubic_lut_size_property;
> > +
> >  	/**
> >  	 * @suggested_x_property: Optional connector property with a hint for
> >  	 * the position of the output on the host's screen.

-- 
Regards,

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

Re: [PATCH 3/4] drm: Extend color correction to support 3D-CLU

[Daniel Vetter]

On Mon, Dec 21, 2020 at 2:57 AM Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
>
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> Extend the existing color management properties to support provision
> of a 3D cubic look up table, allowing for color specific adjustments.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Co-developed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Assuming this is meant for merging to upstream: Needs igt + open
userspace in a compositor that cares enough.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c       |  1 +
>  drivers/gpu/drm/drm_atomic_state_helper.c |  3 ++
>  drivers/gpu/drm/drm_atomic_uapi.c         | 10 ++++++
>  drivers/gpu/drm/drm_color_mgmt.c          | 41 +++++++++++++++++++----
>  drivers/gpu/drm/drm_mode_config.c         | 14 ++++++++
>  include/drm/drm_crtc.h                    |  9 +++++
>  include/drm/drm_mode_config.h             | 11 ++++++
>  7 files changed, 82 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ba1507036f26..0f54897d3c8d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>         replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
>         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->cubic_lut, NULL);
>         crtc_state->color_mgmt_changed |= replaced;
>
>         ret = drm_atomic_commit(state);
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index ddcf5c2c8e6a..61c685b50677 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>                 drm_property_blob_get(state->ctm);
>         if (state->gamma_lut)
>                 drm_property_blob_get(state->gamma_lut);
> +       if (state->cubic_lut)
> +               drm_property_blob_get(state->cubic_lut);
>         state->mode_changed = false;
>         state->active_changed = false;
>         state->planes_changed = false;
> @@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
>         drm_property_blob_put(state->degamma_lut);
>         drm_property_blob_put(state->ctm);
>         drm_property_blob_put(state->gamma_lut);
> +       drm_property_blob_put(state->cubic_lut);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 268bb69c2e2f..07229acab71c 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>                                         &replaced);
>                 state->color_mgmt_changed |= replaced;
>                 return ret;
> +       } else if (property == config->cubic_lut_property) {
> +               ret = drm_atomic_replace_property_blob_from_id(dev,
> +                                       &state->cubic_lut,
> +                                       val,
> +                                       -1, sizeof(struct drm_color_lut),
> +                                       &replaced);
> +               state->color_mgmt_changed |= replaced;
> +               return ret;
>         } else if (property == config->prop_out_fence_ptr) {
>                 s32 __user *fence_ptr = u64_to_user_ptr(val);
>
> @@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>                 *val = (state->ctm) ? state->ctm->base.id : 0;
>         else if (property == config->gamma_lut_property)
>                 *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> +       else if (property == config->cubic_lut_property)
> +               *val = (state->cubic_lut) ? state->cubic_lut->base.id : 0;
>         else if (property == config->prop_out_fence_ptr)
>                 *val = 0;
>         else if (property == crtc->scaling_filter_property)
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 3bcabc2f6e0e..85bbbc8ce8e5 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -33,7 +33,7 @@
>  /**
>   * DOC: overview
>   *
> - * Color management or color space adjustments is supported through a set of 5
> + * Color management or color space adjustments is supported through a set of 7
>   * properties on the &drm_crtc object. They are set up by calling
>   * drm_crtc_enable_color_mgmt().
>   *
> @@ -60,7 +60,7 @@
>   * “CTM”:
>   *     Blob property to set the current transformation matrix (CTM) apply to
>   *     pixel data after the lookup through the degamma LUT and before the
> - *     lookup through the gamma LUT. The data is interpreted as a struct
> + *     lookup through the cubic LUT. The data is interpreted as a struct
>   *     &drm_color_ctm.
>   *
>   *     Setting this to NULL (blob property value set to 0) means a
> @@ -68,13 +68,40 @@
>   *     boot-up state too. Drivers can access the blob for the color conversion
>   *     matrix through &drm_crtc_state.ctm.
>   *
> + * ”CUBIC_LUT”:
> + *     Blob property to set the cubic (3D) lookup table performing color
> + *     mapping after the transformation matrix and before the lookup through
> + *     the gamma LUT. Unlike the degamma and gamma LUTs that map color
> + *     components independently, the 3D LUT converts an input color to an
> + *     output color by indexing into the 3D table using the color components
> + *     as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
> + *     would require too much storage space in the hardware, so the precision
> + *     of the color components is reduced before the look up, and the low
> + *     order bits may be used to interpolate between the nearest points in 3D
> + *     space.
> + *
> + *     The data is interpreted as an array of &struct drm_color_lut elements.
> + *     Hardware might choose not to use the full precision of the LUT
> + *     elements.
> + *
> + *     Setting this to NULL (blob property value set to 0) means the output
> + *     color is identical to the input color. This is generally the driver
> + *     boot-up state too. Drivers can access this blob through
> + *     &drm_crtc_state.cubic_lut.
> + *
> + * ”CUBIC_LUT_SIZE”:
> + *     Unsigned range property to give the size of the lookup table to be set
> + *     on the CUBIC_LUT property (the size depends on the underlying hardware).
> + *     If drivers support multiple LUT sizes then they should publish the
> + *     largest size, and sub-sample smaller sized LUTs appropriately.
> + *
>   * “GAMMA_LUT”:
>   *     Blob property to set the gamma lookup table (LUT) mapping pixel data
> - *     after the transformation matrix to data sent to the connector. The
> - *     data is interpreted as an array of &struct drm_color_lut elements.
> - *     Hardware might choose not to use the full precision of the LUT elements
> - *     nor use all the elements of the LUT (for example the hardware might
> - *     choose to interpolate between LUT[0] and LUT[4]).
> + *     after the cubic LUT to data sent to the connector. The data is
> + *     interpreted as an array of &struct drm_color_lut elements. Hardware
> + *     might choose not to use the full precision of the LUT elements nor use
> + *     all the elements of the LUT (for example the hardware might choose to
> + *     interpolate between LUT[0] and LUT[4]).
>   *
>   *     Setting this to NULL (blob property value set to 0) means a
>   *     linear/pass-thru gamma table should be used. This is generally the
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index f1affc1bb679..6c3324f60e7d 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>                 return -ENOMEM;
>         dev->mode_config.gamma_lut_size_property = prop;
>
> +       prop = drm_property_create(dev,
> +                       DRM_MODE_PROP_BLOB,
> +                       "CUBIC_LUT", 0);
> +       if (!prop)
> +               return -ENOMEM;
> +       dev->mode_config.cubic_lut_property = prop;
> +
> +       prop = drm_property_create_range(dev,
> +                       DRM_MODE_PROP_IMMUTABLE,
> +                       "CUBIC_LUT_SIZE", 0, UINT_MAX);
> +       if (!prop)
> +               return -ENOMEM;
> +       dev->mode_config.cubic_lut_size_property = prop;
> +
>         prop = drm_property_create(dev,
>                                    DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
>                                    "IN_FORMATS", 0);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 5f43d64d2a07..df5cc2239adb 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -288,6 +288,15 @@ struct drm_crtc_state {
>          */
>         struct drm_property_blob *gamma_lut;
>
> +       /**
> +        * @cubic_lut:
> +        *
> +        * Cubic Lookup table for converting pixel data. See
> +        * drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array
> +        * of &struct drm_color_lut.
> +        */
> +       struct drm_property_blob *cubic_lut;
> +
>         /**
>          * @target_vblank:
>          *
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index ab424ddd7665..8edb0094e5a7 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -800,6 +800,17 @@ struct drm_mode_config {
>          */
>         struct drm_property *gamma_lut_size_property;
>
> +       /**
> +        * @cubic_lut_property: Optional CRTC property to set the 3D LUT used to
> +        * convert color spaces.
> +        */
> +       struct drm_property *cubic_lut_property;
> +       /**
> +        * @cubic_lut_size_property: Optional CRTC property for the size of the
> +        * 3D LUT as supported by the driver (read-only).
> +        */
> +       struct drm_property *cubic_lut_size_property;
> +
>         /**
>          * @suggested_x_property: Optional connector property with a hint for
>          * the position of the output on the host's screen.
> --
> 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 3/4] drm: Extend color correction to support 3D-CLU

[Laurent Pinchart]

Hi Daniel,

On Mon, Dec 21, 2020 at 07:36:22PM +0100, Daniel Vetter wrote:
> On Mon, Dec 21, 2020 at 2:57 AM Laurent Pinchart wrote:
> >
> > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >
> > Extend the existing color management properties to support provision
> > of a 3D cubic look up table, allowing for color specific adjustments.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Co-developed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Assuming this is meant for merging to upstream: Needs igt + open
> userspace in a compositor that cares enough.

Please see the cover letter :-) Feedback on what an appropriate
userspace would be would be appreciated.

> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c       |  1 +
> >  drivers/gpu/drm/drm_atomic_state_helper.c |  3 ++
> >  drivers/gpu/drm/drm_atomic_uapi.c         | 10 ++++++
> >  drivers/gpu/drm/drm_color_mgmt.c          | 41 +++++++++++++++++++----
> >  drivers/gpu/drm/drm_mode_config.c         | 14 ++++++++
> >  include/drm/drm_crtc.h                    |  9 +++++
> >  include/drm/drm_mode_config.h             | 11 ++++++
> >  7 files changed, 82 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index ba1507036f26..0f54897d3c8d 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> >         replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
> >         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->cubic_lut, NULL);
> >         crtc_state->color_mgmt_changed |= replaced;
> >
> >         ret = drm_atomic_commit(state);
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index ddcf5c2c8e6a..61c685b50677 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> >                 drm_property_blob_get(state->ctm);
> >         if (state->gamma_lut)
> >                 drm_property_blob_get(state->gamma_lut);
> > +       if (state->cubic_lut)
> > +               drm_property_blob_get(state->cubic_lut);
> >         state->mode_changed = false;
> >         state->active_changed = false;
> >         state->planes_changed = false;
> > @@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
> >         drm_property_blob_put(state->degamma_lut);
> >         drm_property_blob_put(state->ctm);
> >         drm_property_blob_put(state->gamma_lut);
> > +       drm_property_blob_put(state->cubic_lut);
> >  }
> >  EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 268bb69c2e2f..07229acab71c 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> >                                         &replaced);
> >                 state->color_mgmt_changed |= replaced;
> >                 return ret;
> > +       } else if (property == config->cubic_lut_property) {
> > +               ret = drm_atomic_replace_property_blob_from_id(dev,
> > +                                       &state->cubic_lut,
> > +                                       val,
> > +                                       -1, sizeof(struct drm_color_lut),
> > +                                       &replaced);
> > +               state->color_mgmt_changed |= replaced;
> > +               return ret;
> >         } else if (property == config->prop_out_fence_ptr) {
> >                 s32 __user *fence_ptr = u64_to_user_ptr(val);
> >
> > @@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> >                 *val = (state->ctm) ? state->ctm->base.id : 0;
> >         else if (property == config->gamma_lut_property)
> >                 *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> > +       else if (property == config->cubic_lut_property)
> > +               *val = (state->cubic_lut) ? state->cubic_lut->base.id : 0;
> >         else if (property == config->prop_out_fence_ptr)
> >                 *val = 0;
> >         else if (property == crtc->scaling_filter_property)
> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > index 3bcabc2f6e0e..85bbbc8ce8e5 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -33,7 +33,7 @@
> >  /**
> >   * DOC: overview
> >   *
> > - * Color management or color space adjustments is supported through a set of 5
> > + * Color management or color space adjustments is supported through a set of 7
> >   * properties on the &drm_crtc object. They are set up by calling
> >   * drm_crtc_enable_color_mgmt().
> >   *
> > @@ -60,7 +60,7 @@
> >   * “CTM”:
> >   *     Blob property to set the current transformation matrix (CTM) apply to
> >   *     pixel data after the lookup through the degamma LUT and before the
> > - *     lookup through the gamma LUT. The data is interpreted as a struct
> > + *     lookup through the cubic LUT. The data is interpreted as a struct
> >   *     &drm_color_ctm.
> >   *
> >   *     Setting this to NULL (blob property value set to 0) means a
> > @@ -68,13 +68,40 @@
> >   *     boot-up state too. Drivers can access the blob for the color conversion
> >   *     matrix through &drm_crtc_state.ctm.
> >   *
> > + * ”CUBIC_LUT”:
> > + *     Blob property to set the cubic (3D) lookup table performing color
> > + *     mapping after the transformation matrix and before the lookup through
> > + *     the gamma LUT. Unlike the degamma and gamma LUTs that map color
> > + *     components independently, the 3D LUT converts an input color to an
> > + *     output color by indexing into the 3D table using the color components
> > + *     as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
> > + *     would require too much storage space in the hardware, so the precision
> > + *     of the color components is reduced before the look up, and the low
> > + *     order bits may be used to interpolate between the nearest points in 3D
> > + *     space.
> > + *
> > + *     The data is interpreted as an array of &struct drm_color_lut elements.
> > + *     Hardware might choose not to use the full precision of the LUT
> > + *     elements.
> > + *
> > + *     Setting this to NULL (blob property value set to 0) means the output
> > + *     color is identical to the input color. This is generally the driver
> > + *     boot-up state too. Drivers can access this blob through
> > + *     &drm_crtc_state.cubic_lut.
> > + *
> > + * ”CUBIC_LUT_SIZE”:
> > + *     Unsigned range property to give the size of the lookup table to be set
> > + *     on the CUBIC_LUT property (the size depends on the underlying hardware).
> > + *     If drivers support multiple LUT sizes then they should publish the
> > + *     largest size, and sub-sample smaller sized LUTs appropriately.
> > + *
> >   * “GAMMA_LUT”:
> >   *     Blob property to set the gamma lookup table (LUT) mapping pixel data
> > - *     after the transformation matrix to data sent to the connector. The
> > - *     data is interpreted as an array of &struct drm_color_lut elements.
> > - *     Hardware might choose not to use the full precision of the LUT elements
> > - *     nor use all the elements of the LUT (for example the hardware might
> > - *     choose to interpolate between LUT[0] and LUT[4]).
> > + *     after the cubic LUT to data sent to the connector. The data is
> > + *     interpreted as an array of &struct drm_color_lut elements. Hardware
> > + *     might choose not to use the full precision of the LUT elements nor use
> > + *     all the elements of the LUT (for example the hardware might choose to
> > + *     interpolate between LUT[0] and LUT[4]).
> >   *
> >   *     Setting this to NULL (blob property value set to 0) means a
> >   *     linear/pass-thru gamma table should be used. This is generally the
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index f1affc1bb679..6c3324f60e7d 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> >                 return -ENOMEM;
> >         dev->mode_config.gamma_lut_size_property = prop;
> >
> > +       prop = drm_property_create(dev,
> > +                       DRM_MODE_PROP_BLOB,
> > +                       "CUBIC_LUT", 0);
> > +       if (!prop)
> > +               return -ENOMEM;
> > +       dev->mode_config.cubic_lut_property = prop;
> > +
> > +       prop = drm_property_create_range(dev,
> > +                       DRM_MODE_PROP_IMMUTABLE,
> > +                       "CUBIC_LUT_SIZE", 0, UINT_MAX);
> > +       if (!prop)
> > +               return -ENOMEM;
> > +       dev->mode_config.cubic_lut_size_property = prop;
> > +
> >         prop = drm_property_create(dev,
> >                                    DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
> >                                    "IN_FORMATS", 0);
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 5f43d64d2a07..df5cc2239adb 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -288,6 +288,15 @@ struct drm_crtc_state {
> >          */
> >         struct drm_property_blob *gamma_lut;
> >
> > +       /**
> > +        * @cubic_lut:
> > +        *
> > +        * Cubic Lookup table for converting pixel data. See
> > +        * drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array
> > +        * of &struct drm_color_lut.
> > +        */
> > +       struct drm_property_blob *cubic_lut;
> > +
> >         /**
> >          * @target_vblank:
> >          *
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index ab424ddd7665..8edb0094e5a7 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -800,6 +800,17 @@ struct drm_mode_config {
> >          */
> >         struct drm_property *gamma_lut_size_property;
> >
> > +       /**
> > +        * @cubic_lut_property: Optional CRTC property to set the 3D LUT used to
> > +        * convert color spaces.
> > +        */
> > +       struct drm_property *cubic_lut_property;
> > +       /**
> > +        * @cubic_lut_size_property: Optional CRTC property for the size of the
> > +        * 3D LUT as supported by the driver (read-only).
> > +        */
> > +       struct drm_property *cubic_lut_size_property;
> > +
> >         /**
> >          * @suggested_x_property: Optional connector property with a hint for
> >          * the position of the output on the host's screen.

-- 
Regards,

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

Re: [PATCH 3/4] drm: Extend color correction to support 3D-CLU

[Daniel Vetter]

On Mon, Dec 21, 2020 at 08:38:44PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Mon, Dec 21, 2020 at 07:36:22PM +0100, Daniel Vetter wrote:
> > On Mon, Dec 21, 2020 at 2:57 AM Laurent Pinchart wrote:
> > >
> > > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > >
> > > Extend the existing color management properties to support provision
> > > of a 3D cubic look up table, allowing for color specific adjustments.
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > Co-developed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > Assuming this is meant for merging to upstream: Needs igt + open
> > userspace in a compositor that cares enough.
> 
> Please see the cover letter :-) Feedback on what an appropriate
> userspace would be would be appreciated.

Oops sorry.

Wrt userspace CrOS was the one originally used to merge this, they do the
full ICC color correction for their panels with degamm + ctm + lut. So if
you somewhat care about that (or can make google care about 3d/cube luts)
then that might be simplest.

The Kodi people also care quite a lot about color correction stuff, and
iirc some of the per-plane color management is being prototyped with code
(so that the movie and the UI both have their correct colors).

Otherwise I guess weston is the closest with real color management, but
nothing merged yet.
-Daniel

> 
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c       |  1 +
> > >  drivers/gpu/drm/drm_atomic_state_helper.c |  3 ++
> > >  drivers/gpu/drm/drm_atomic_uapi.c         | 10 ++++++
> > >  drivers/gpu/drm/drm_color_mgmt.c          | 41 +++++++++++++++++++----
> > >  drivers/gpu/drm/drm_mode_config.c         | 14 ++++++++
> > >  include/drm/drm_crtc.h                    |  9 +++++
> > >  include/drm/drm_mode_config.h             | 11 ++++++
> > >  7 files changed, 82 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index ba1507036f26..0f54897d3c8d 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> > >         replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
> > >         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->cubic_lut, NULL);
> > >         crtc_state->color_mgmt_changed |= replaced;
> > >
> > >         ret = drm_atomic_commit(state);
> > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > index ddcf5c2c8e6a..61c685b50677 100644
> > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> > >                 drm_property_blob_get(state->ctm);
> > >         if (state->gamma_lut)
> > >                 drm_property_blob_get(state->gamma_lut);
> > > +       if (state->cubic_lut)
> > > +               drm_property_blob_get(state->cubic_lut);
> > >         state->mode_changed = false;
> > >         state->active_changed = false;
> > >         state->planes_changed = false;
> > > @@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
> > >         drm_property_blob_put(state->degamma_lut);
> > >         drm_property_blob_put(state->ctm);
> > >         drm_property_blob_put(state->gamma_lut);
> > > +       drm_property_blob_put(state->cubic_lut);
> > >  }
> > >  EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index 268bb69c2e2f..07229acab71c 100644
> > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> > >                                         &replaced);
> > >                 state->color_mgmt_changed |= replaced;
> > >                 return ret;
> > > +       } else if (property == config->cubic_lut_property) {
> > > +               ret = drm_atomic_replace_property_blob_from_id(dev,
> > > +                                       &state->cubic_lut,
> > > +                                       val,
> > > +                                       -1, sizeof(struct drm_color_lut),
> > > +                                       &replaced);
> > > +               state->color_mgmt_changed |= replaced;
> > > +               return ret;
> > >         } else if (property == config->prop_out_fence_ptr) {
> > >                 s32 __user *fence_ptr = u64_to_user_ptr(val);
> > >
> > > @@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> > >                 *val = (state->ctm) ? state->ctm->base.id : 0;
> > >         else if (property == config->gamma_lut_property)
> > >                 *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> > > +       else if (property == config->cubic_lut_property)
> > > +               *val = (state->cubic_lut) ? state->cubic_lut->base.id : 0;
> > >         else if (property == config->prop_out_fence_ptr)
> > >                 *val = 0;
> > >         else if (property == crtc->scaling_filter_property)
> > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > > index 3bcabc2f6e0e..85bbbc8ce8e5 100644
> > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > @@ -33,7 +33,7 @@
> > >  /**
> > >   * DOC: overview
> > >   *
> > > - * Color management or color space adjustments is supported through a set of 5
> > > + * Color management or color space adjustments is supported through a set of 7
> > >   * properties on the &drm_crtc object. They are set up by calling
> > >   * drm_crtc_enable_color_mgmt().
> > >   *
> > > @@ -60,7 +60,7 @@
> > >   * “CTM”:
> > >   *     Blob property to set the current transformation matrix (CTM) apply to
> > >   *     pixel data after the lookup through the degamma LUT and before the
> > > - *     lookup through the gamma LUT. The data is interpreted as a struct
> > > + *     lookup through the cubic LUT. The data is interpreted as a struct
> > >   *     &drm_color_ctm.
> > >   *
> > >   *     Setting this to NULL (blob property value set to 0) means a
> > > @@ -68,13 +68,40 @@
> > >   *     boot-up state too. Drivers can access the blob for the color conversion
> > >   *     matrix through &drm_crtc_state.ctm.
> > >   *
> > > + * ”CUBIC_LUT”:
> > > + *     Blob property to set the cubic (3D) lookup table performing color
> > > + *     mapping after the transformation matrix and before the lookup through
> > > + *     the gamma LUT. Unlike the degamma and gamma LUTs that map color
> > > + *     components independently, the 3D LUT converts an input color to an
> > > + *     output color by indexing into the 3D table using the color components
> > > + *     as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
> > > + *     would require too much storage space in the hardware, so the precision
> > > + *     of the color components is reduced before the look up, and the low
> > > + *     order bits may be used to interpolate between the nearest points in 3D
> > > + *     space.
> > > + *
> > > + *     The data is interpreted as an array of &struct drm_color_lut elements.
> > > + *     Hardware might choose not to use the full precision of the LUT
> > > + *     elements.
> > > + *
> > > + *     Setting this to NULL (blob property value set to 0) means the output
> > > + *     color is identical to the input color. This is generally the driver
> > > + *     boot-up state too. Drivers can access this blob through
> > > + *     &drm_crtc_state.cubic_lut.
> > > + *
> > > + * ”CUBIC_LUT_SIZE”:
> > > + *     Unsigned range property to give the size of the lookup table to be set
> > > + *     on the CUBIC_LUT property (the size depends on the underlying hardware).
> > > + *     If drivers support multiple LUT sizes then they should publish the
> > > + *     largest size, and sub-sample smaller sized LUTs appropriately.
> > > + *
> > >   * “GAMMA_LUT”:
> > >   *     Blob property to set the gamma lookup table (LUT) mapping pixel data
> > > - *     after the transformation matrix to data sent to the connector. The
> > > - *     data is interpreted as an array of &struct drm_color_lut elements.
> > > - *     Hardware might choose not to use the full precision of the LUT elements
> > > - *     nor use all the elements of the LUT (for example the hardware might
> > > - *     choose to interpolate between LUT[0] and LUT[4]).
> > > + *     after the cubic LUT to data sent to the connector. The data is
> > > + *     interpreted as an array of &struct drm_color_lut elements. Hardware
> > > + *     might choose not to use the full precision of the LUT elements nor use
> > > + *     all the elements of the LUT (for example the hardware might choose to
> > > + *     interpolate between LUT[0] and LUT[4]).
> > >   *
> > >   *     Setting this to NULL (blob property value set to 0) means a
> > >   *     linear/pass-thru gamma table should be used. This is generally the
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index f1affc1bb679..6c3324f60e7d 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> > >                 return -ENOMEM;
> > >         dev->mode_config.gamma_lut_size_property = prop;
> > >
> > > +       prop = drm_property_create(dev,
> > > +                       DRM_MODE_PROP_BLOB,
> > > +                       "CUBIC_LUT", 0);
> > > +       if (!prop)
> > > +               return -ENOMEM;
> > > +       dev->mode_config.cubic_lut_property = prop;
> > > +
> > > +       prop = drm_property_create_range(dev,
> > > +                       DRM_MODE_PROP_IMMUTABLE,
> > > +                       "CUBIC_LUT_SIZE", 0, UINT_MAX);
> > > +       if (!prop)
> > > +               return -ENOMEM;
> > > +       dev->mode_config.cubic_lut_size_property = prop;
> > > +
> > >         prop = drm_property_create(dev,
> > >                                    DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
> > >                                    "IN_FORMATS", 0);
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index 5f43d64d2a07..df5cc2239adb 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -288,6 +288,15 @@ struct drm_crtc_state {
> > >          */
> > >         struct drm_property_blob *gamma_lut;
> > >
> > > +       /**
> > > +        * @cubic_lut:
> > > +        *
> > > +        * Cubic Lookup table for converting pixel data. See
> > > +        * drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array
> > > +        * of &struct drm_color_lut.
> > > +        */
> > > +       struct drm_property_blob *cubic_lut;
> > > +
> > >         /**
> > >          * @target_vblank:
> > >          *
> > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > > index ab424ddd7665..8edb0094e5a7 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -800,6 +800,17 @@ struct drm_mode_config {
> > >          */
> > >         struct drm_property *gamma_lut_size_property;
> > >
> > > +       /**
> > > +        * @cubic_lut_property: Optional CRTC property to set the 3D LUT used to
> > > +        * convert color spaces.
> > > +        */
> > > +       struct drm_property *cubic_lut_property;
> > > +       /**
> > > +        * @cubic_lut_size_property: Optional CRTC property for the size of the
> > > +        * 3D LUT as supported by the driver (read-only).
> > > +        */
> > > +       struct drm_property *cubic_lut_size_property;
> > > +
> > >         /**
> > >          * @suggested_x_property: Optional connector property with a hint for
> > >          * the position of the output on the host's screen.
> 
> -- 
> 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 3/4] drm: Extend color correction to support 3D-CLU

[Ville Syrjälä]

On Mon, Dec 21, 2020 at 03:57:29AM +0200, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Extend the existing color management properties to support provision
> of a 3D cubic look up table, allowing for color specific adjustments.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Co-developed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

FYI I've got a WIP 3D LUT implementation for i915 here:
git://github.com/vsyrjala/linux.git 3dlut

I named the prop GAMMA_LUT_3D to indicate its position in the
pipeline (on our hw it's postioned after the normal gamma LUT).
Alas no userspace for it yet, so can't really do anything more 
with it for the time being.

Rudimentary tests also available here:
git://github.com/vsyrjala/intel-gpu-tools.git 3dlut

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

Re: [PATCH 3/4] drm: Extend color correction to support 3D-CLU

[Sandy Huang]

Hi Laurent and Kieran,

Do you forget to attach cubic_lut_property with crtc?

drm_object_attach_property(&crtc->base, config->cubic_lut_property, cubic_lut_size);

在 2020/12/21 9:57, Laurent Pinchart 写道:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> Extend the existing color management properties to support provision
> of a 3D cubic look up table, allowing for color specific adjustments.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Co-developed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   drivers/gpu/drm/drm_atomic_helper.c       |  1 +
>   drivers/gpu/drm/drm_atomic_state_helper.c |  3 ++
>   drivers/gpu/drm/drm_atomic_uapi.c         | 10 ++++++
>   drivers/gpu/drm/drm_color_mgmt.c          | 41 +++++++++++++++++++----
>   drivers/gpu/drm/drm_mode_config.c         | 14 ++++++++
>   include/drm/drm_crtc.h                    |  9 +++++
>   include/drm/drm_mode_config.h             | 11 ++++++
>   7 files changed, 82 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ba1507036f26..0f54897d3c8d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>   	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
>   	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->cubic_lut, NULL);
>   	crtc_state->color_mgmt_changed |= replaced;
>   
>   	ret = drm_atomic_commit(state);
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index ddcf5c2c8e6a..61c685b50677 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>   		drm_property_blob_get(state->ctm);
>   	if (state->gamma_lut)
>   		drm_property_blob_get(state->gamma_lut);
> +	if (state->cubic_lut)
> +		drm_property_blob_get(state->cubic_lut);
>   	state->mode_changed = false;
>   	state->active_changed = false;
>   	state->planes_changed = false;
> @@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
>   	drm_property_blob_put(state->degamma_lut);
>   	drm_property_blob_put(state->ctm);
>   	drm_property_blob_put(state->gamma_lut);
> +	drm_property_blob_put(state->cubic_lut);
>   }
>   EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
>   
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 268bb69c2e2f..07229acab71c 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>   					&replaced);
>   		state->color_mgmt_changed |= replaced;
>   		return ret;
> +	} else if (property == config->cubic_lut_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> +					&state->cubic_lut,
> +					val,
> +					-1, sizeof(struct drm_color_lut),
> +					&replaced);
> +		state->color_mgmt_changed |= replaced;
> +		return ret;
>   	} else if (property == config->prop_out_fence_ptr) {
>   		s32 __user *fence_ptr = u64_to_user_ptr(val);
>   
> @@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>   		*val = (state->ctm) ? state->ctm->base.id : 0;
>   	else if (property == config->gamma_lut_property)
>   		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> +	else if (property == config->cubic_lut_property)
> +		*val = (state->cubic_lut) ? state->cubic_lut->base.id : 0;
>   	else if (property == config->prop_out_fence_ptr)
>   		*val = 0;
>   	else if (property == crtc->scaling_filter_property)
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 3bcabc2f6e0e..85bbbc8ce8e5 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -33,7 +33,7 @@
>   /**
>    * DOC: overview
>    *
> - * Color management or color space adjustments is supported through a set of 5
> + * Color management or color space adjustments is supported through a set of 7
>    * properties on the &drm_crtc object. They are set up by calling
>    * drm_crtc_enable_color_mgmt().
>    *
> @@ -60,7 +60,7 @@
>    * “CTM”:
>    *	Blob property to set the current transformation matrix (CTM) apply to
>    *	pixel data after the lookup through the degamma LUT and before the
> - *	lookup through the gamma LUT. The data is interpreted as a struct
> + *	lookup through the cubic LUT. The data is interpreted as a struct
>    *	&drm_color_ctm.
>    *
>    *	Setting this to NULL (blob property value set to 0) means a
> @@ -68,13 +68,40 @@
>    *	boot-up state too. Drivers can access the blob for the color conversion
>    *	matrix through &drm_crtc_state.ctm.
>    *
> + * ”CUBIC_LUT”:
> + *	Blob property to set the cubic (3D) lookup table performing color
> + *	mapping after the transformation matrix and before the lookup through
> + *	the gamma LUT. Unlike the degamma and gamma LUTs that map color
> + *	components independently, the 3D LUT converts an input color to an
> + *	output color by indexing into the 3D table using the color components
> + *	as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
> + *	would require too much storage space in the hardware, so the precision
> + *	of the color components is reduced before the look up, and the low
> + *	order bits may be used to interpolate between the nearest points in 3D
> + *	space.
> + *
> + *	The data is interpreted as an array of &struct drm_color_lut elements.
> + *	Hardware might choose not to use the full precision of the LUT
> + *	elements.
> + *
> + *	Setting this to NULL (blob property value set to 0) means the output
> + *	color is identical to the input color. This is generally the driver
> + *	boot-up state too. Drivers can access this blob through
> + *	&drm_crtc_state.cubic_lut.
> + *
> + * ”CUBIC_LUT_SIZE”:
> + *	Unsigned range property to give the size of the lookup table to be set
> + *	on the CUBIC_LUT property (the size depends on the underlying hardware).
> + *	If drivers support multiple LUT sizes then they should publish the
> + *	largest size, and sub-sample smaller sized LUTs appropriately.
> + *
>    * “GAMMA_LUT”:
>    *	Blob property to set the gamma lookup table (LUT) mapping pixel data
> - *	after the transformation matrix to data sent to the connector. The
> - *	data is interpreted as an array of &struct drm_color_lut elements.
> - *	Hardware might choose not to use the full precision of the LUT elements
> - *	nor use all the elements of the LUT (for example the hardware might
> - *	choose to interpolate between LUT[0] and LUT[4]).
> + *	after the cubic LUT to data sent to the connector. The data is
> + *	interpreted as an array of &struct drm_color_lut elements. Hardware
> + *	might choose not to use the full precision of the LUT elements nor use
> + *	all the elements of the LUT (for example the hardware might choose to
> + *	interpolate between LUT[0] and LUT[4]).
>    *
>    *	Setting this to NULL (blob property value set to 0) means a
>    *	linear/pass-thru gamma table should be used. This is generally the
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index f1affc1bb679..6c3324f60e7d 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>   		return -ENOMEM;
>   	dev->mode_config.gamma_lut_size_property = prop;
>   
> +	prop = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB,
> +			"CUBIC_LUT", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cubic_lut_property = prop;
> +
> +	prop = drm_property_create_range(dev,
> +			DRM_MODE_PROP_IMMUTABLE,
> +			"CUBIC_LUT_SIZE", 0, UINT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cubic_lut_size_property = prop;
> +
>   	prop = drm_property_create(dev,
>   				   DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
>   				   "IN_FORMATS", 0);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 5f43d64d2a07..df5cc2239adb 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -288,6 +288,15 @@ struct drm_crtc_state {
>   	 */
>   	struct drm_property_blob *gamma_lut;
>   
> +	/**
> +	 * @cubic_lut:
> +	 *
> +	 * Cubic Lookup table for converting pixel data. See
> +	 * drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array
> +	 * of &struct drm_color_lut.
> +	 */
> +	struct drm_property_blob *cubic_lut;
> +
>   	/**
>   	 * @target_vblank:
>   	 *
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index ab424ddd7665..8edb0094e5a7 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -800,6 +800,17 @@ struct drm_mode_config {
>   	 */
>   	struct drm_property *gamma_lut_size_property;
>   
> +	/**
> +	 * @cubic_lut_property: Optional CRTC property to set the 3D LUT used to
> +	 * convert color spaces.
> +	 */
> +	struct drm_property *cubic_lut_property;
> +	/**
> +	 * @cubic_lut_size_property: Optional CRTC property for the size of the
> +	 * 3D LUT as supported by the driver (read-only).
> +	 */
> +	struct drm_property *cubic_lut_size_property;
> +
>   	/**
>   	 * @suggested_x_property: Optional connector property with a hint for
>   	 * the position of the output on the host's screen.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm: Extend color correction to support 3D-CLU

[Sandy Huang]

Hi Laurent and Kieran,

Do you forget to attach cubic_lut_property with crtc?

drm_object_attach_property(&crtc->base, config->cubic_lut_property, 
cubic_lut_size);

在 2020/12/21 9:57, Laurent Pinchart 写道:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Extend the existing color management properties to support provision
> of a 3D cubic look up table, allowing for color specific adjustments.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Co-developed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   drivers/gpu/drm/drm_atomic_helper.c       |  1 +
>   drivers/gpu/drm/drm_atomic_state_helper.c |  3 ++
>   drivers/gpu/drm/drm_atomic_uapi.c         | 10 ++++++
>   drivers/gpu/drm/drm_color_mgmt.c          | 41 +++++++++++++++++++----
>   drivers/gpu/drm/drm_mode_config.c         | 14 ++++++++
>   include/drm/drm_crtc.h                    |  9 +++++
>   include/drm/drm_mode_config.h             | 11 ++++++
>   7 files changed, 82 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ba1507036f26..0f54897d3c8d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>   	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
>   	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->cubic_lut, NULL);
>   	crtc_state->color_mgmt_changed |= replaced;
>   
>   	ret = drm_atomic_commit(state);
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index ddcf5c2c8e6a..61c685b50677 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>   		drm_property_blob_get(state->ctm);
>   	if (state->gamma_lut)
>   		drm_property_blob_get(state->gamma_lut);
> +	if (state->cubic_lut)
> +		drm_property_blob_get(state->cubic_lut);
>   	state->mode_changed = false;
>   	state->active_changed = false;
>   	state->planes_changed = false;
> @@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
>   	drm_property_blob_put(state->degamma_lut);
>   	drm_property_blob_put(state->ctm);
>   	drm_property_blob_put(state->gamma_lut);
> +	drm_property_blob_put(state->cubic_lut);
>   }
>   EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
>   
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 268bb69c2e2f..07229acab71c 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>   					&replaced);
>   		state->color_mgmt_changed |= replaced;
>   		return ret;
> +	} else if (property == config->cubic_lut_property) {
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> +					&state->cubic_lut,
> +					val,
> +					-1, sizeof(struct drm_color_lut),
> +					&replaced);
> +		state->color_mgmt_changed |= replaced;
> +		return ret;
>   	} else if (property == config->prop_out_fence_ptr) {
>   		s32 __user *fence_ptr = u64_to_user_ptr(val);
>   
> @@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>   		*val = (state->ctm) ? state->ctm->base.id : 0;
>   	else if (property == config->gamma_lut_property)
>   		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> +	else if (property == config->cubic_lut_property)
> +		*val = (state->cubic_lut) ? state->cubic_lut->base.id : 0;
>   	else if (property == config->prop_out_fence_ptr)
>   		*val = 0;
>   	else if (property == crtc->scaling_filter_property)
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 3bcabc2f6e0e..85bbbc8ce8e5 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -33,7 +33,7 @@
>   /**
>    * DOC: overview
>    *
> - * Color management or color space adjustments is supported through a set of 5
> + * Color management or color space adjustments is supported through a set of 7
>    * properties on the &drm_crtc object. They are set up by calling
>    * drm_crtc_enable_color_mgmt().
>    *
> @@ -60,7 +60,7 @@
>    * “CTM”:
>    *	Blob property to set the current transformation matrix (CTM) apply to
>    *	pixel data after the lookup through the degamma LUT and before the
> - *	lookup through the gamma LUT. The data is interpreted as a struct
> + *	lookup through the cubic LUT. The data is interpreted as a struct
>    *	&drm_color_ctm.
>    *
>    *	Setting this to NULL (blob property value set to 0) means a
> @@ -68,13 +68,40 @@
>    *	boot-up state too. Drivers can access the blob for the color conversion
>    *	matrix through &drm_crtc_state.ctm.
>    *
> + * ”CUBIC_LUT”:
> + *	Blob property to set the cubic (3D) lookup table performing color
> + *	mapping after the transformation matrix and before the lookup through
> + *	the gamma LUT. Unlike the degamma and gamma LUTs that map color
> + *	components independently, the 3D LUT converts an input color to an
> + *	output color by indexing into the 3D table using the color components
> + *	as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
> + *	would require too much storage space in the hardware, so the precision
> + *	of the color components is reduced before the look up, and the low
> + *	order bits may be used to interpolate between the nearest points in 3D
> + *	space.
> + *
> + *	The data is interpreted as an array of &struct drm_color_lut elements.
> + *	Hardware might choose not to use the full precision of the LUT
> + *	elements.
> + *
> + *	Setting this to NULL (blob property value set to 0) means the output
> + *	color is identical to the input color. This is generally the driver
> + *	boot-up state too. Drivers can access this blob through
> + *	&drm_crtc_state.cubic_lut.
> + *
> + * ”CUBIC_LUT_SIZE”:
> + *	Unsigned range property to give the size of the lookup table to be set
> + *	on the CUBIC_LUT property (the size depends on the underlying hardware).
> + *	If drivers support multiple LUT sizes then they should publish the
> + *	largest size, and sub-sample smaller sized LUTs appropriately.
> + *
>    * “GAMMA_LUT”:
>    *	Blob property to set the gamma lookup table (LUT) mapping pixel data
> - *	after the transformation matrix to data sent to the connector. The
> - *	data is interpreted as an array of &struct drm_color_lut elements.
> - *	Hardware might choose not to use the full precision of the LUT elements
> - *	nor use all the elements of the LUT (for example the hardware might
> - *	choose to interpolate between LUT[0] and LUT[4]).
> + *	after the cubic LUT to data sent to the connector. The data is
> + *	interpreted as an array of &struct drm_color_lut elements. Hardware
> + *	might choose not to use the full precision of the LUT elements nor use
> + *	all the elements of the LUT (for example the hardware might choose to
> + *	interpolate between LUT[0] and LUT[4]).
>    *
>    *	Setting this to NULL (blob property value set to 0) means a
>    *	linear/pass-thru gamma table should be used. This is generally the
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index f1affc1bb679..6c3324f60e7d 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>   		return -ENOMEM;
>   	dev->mode_config.gamma_lut_size_property = prop;
>   
> +	prop = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB,
> +			"CUBIC_LUT", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cubic_lut_property = prop;
> +
> +	prop = drm_property_create_range(dev,
> +			DRM_MODE_PROP_IMMUTABLE,
> +			"CUBIC_LUT_SIZE", 0, UINT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cubic_lut_size_property = prop;
> +
>   	prop = drm_property_create(dev,
>   				   DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
>   				   "IN_FORMATS", 0);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 5f43d64d2a07..df5cc2239adb 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -288,6 +288,15 @@ struct drm_crtc_state {
>   	 */
>   	struct drm_property_blob *gamma_lut;
>   
> +	/**
> +	 * @cubic_lut:
> +	 *
> +	 * Cubic Lookup table for converting pixel data. See
> +	 * drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array
> +	 * of &struct drm_color_lut.
> +	 */
> +	struct drm_property_blob *cubic_lut;
> +
>   	/**
>   	 * @target_vblank:
>   	 *
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index ab424ddd7665..8edb0094e5a7 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -800,6 +800,17 @@ struct drm_mode_config {
>   	 */
>   	struct drm_property *gamma_lut_size_property;
>   
> +	/**
> +	 * @cubic_lut_property: Optional CRTC property to set the 3D LUT used to
> +	 * convert color spaces.
> +	 */
> +	struct drm_property *cubic_lut_property;
> +	/**
> +	 * @cubic_lut_size_property: Optional CRTC property for the size of the
> +	 * 3D LUT as supported by the driver (read-only).
> +	 */
> +	struct drm_property *cubic_lut_size_property;
> +
>   	/**
>   	 * @suggested_x_property: Optional connector property with a hint for
>   	 * the position of the output on the host's screen.
> 

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

[PATCH 4/4] drm: rcar-du: kms: Configure the CLU

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Link the DRM 3D-CLU configuration to the CMM setup configuration.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 426b1870b3cb..5c77017084ed 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -484,19 +484,23 @@ static int rcar_du_cmm_check(struct drm_crtc *crtc,
 			     struct drm_crtc_state *state)
 {
 	struct drm_property_blob *drm_lut = state->gamma_lut;
+	struct drm_property_blob *drm_clu = state->cubic_lut;
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 	struct device *dev = rcrtc->dev->dev;
 
-	if (!drm_lut)
-		return 0;
-
-	/* We only accept fully populated LUT tables. */
-	if (drm_color_lut_size(drm_lut) != CM2_LUT_SIZE) {
+	/* We only accept fully populated LUTs. */
+	if (drm_lut && drm_color_lut_size(drm_lut) != CM2_LUT_SIZE) {
 		dev_err(dev, "invalid gamma lut size: %zu bytes\n",
 			drm_lut->length);
 		return -EINVAL;
 	}
 
+	if (drm_clu && drm_color_lut_size(drm_clu) != CM2_CLU_SIZE) {
+		dev_err(dev, "invalid cubic lut size: %zu bytes\n",
+			drm_clu->length);
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -518,6 +522,15 @@ static void rcar_du_cmm_setup(struct rcar_du_crtc *rcrtc,
 				     ? new_state->gamma_lut->data : NULL;
 	}
 
+	if (!old_state ||
+	    !old_state->cubic_lut != !new_state->cubic_lut ||
+	    (old_state->cubic_lut && new_state->cubic_lut &&
+	     old_state->cubic_lut->base.id != new_state->cubic_lut->base.id)) {
+		cmm_config.clu.update = true;
+		cmm_config.clu.table = new_state->cubic_lut
+				     ? new_state->cubic_lut->data : NULL;
+	}
+
 	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
 }
 
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 4/4] drm: rcar-du: kms: Configure the CLU

[Kieran Bingham]

Hi Laurent,

On 21/12/2020 01:57, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Link the DRM 3D-CLU configuration to the CMM setup configuration.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

And for the updates from my original patch (variable rename, and
property blob rework, and that ... obvious bug ... from my patch that's
now obviously gone :-D ...)

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 426b1870b3cb..5c77017084ed 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -484,19 +484,23 @@ static int rcar_du_cmm_check(struct drm_crtc *crtc,
>  			     struct drm_crtc_state *state)
>  {
>  	struct drm_property_blob *drm_lut = state->gamma_lut;
> +	struct drm_property_blob *drm_clu = state->cubic_lut;
>  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>  	struct device *dev = rcrtc->dev->dev;
>  
> -	if (!drm_lut)
> -		return 0;
> -
> -	/* We only accept fully populated LUT tables. */
> -	if (drm_color_lut_size(drm_lut) != CM2_LUT_SIZE) {
> +	/* We only accept fully populated LUTs. */
> +	if (drm_lut && drm_color_lut_size(drm_lut) != CM2_LUT_SIZE) {
>  		dev_err(dev, "invalid gamma lut size: %zu bytes\n",
>  			drm_lut->length);
>  		return -EINVAL;
>  	}
>  
> +	if (drm_clu && drm_color_lut_size(drm_clu) != CM2_CLU_SIZE) {
> +		dev_err(dev, "invalid cubic lut size: %zu bytes\n",
> +			drm_clu->length);
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -518,6 +522,15 @@ static void rcar_du_cmm_setup(struct rcar_du_crtc *rcrtc,
>  				     ? new_state->gamma_lut->data : NULL;
>  	}
>  
> +	if (!old_state ||
> +	    !old_state->cubic_lut != !new_state->cubic_lut ||
> +	    (old_state->cubic_lut && new_state->cubic_lut &&
> +	     old_state->cubic_lut->base.id != new_state->cubic_lut->base.id)) {
> +		cmm_config.clu.update = true;
> +		cmm_config.clu.table = new_state->cubic_lut
> +				     ? new_state->cubic_lut->data : NULL;
> +	}
> +
>  	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
>  }
>  
> 

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

Re: [PATCH 0/4] drm: rcar-du: Add cubic LUT support

[Simon Ser]

Hi,

On Monday, December 21st, 2020 at 2:57 AM, Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:

> I started having a look at userspace to see how this could be handled,
> searching for color management support in weston, kwin and wlroots/sway.
> All three support setting the gamma table when using the DRM/KMS
> backend, weston and kwin through the legacy API, and wlroots through the
> atomic API. Weston reads an ICC profile using Little CMS and applies the
> gamma table. kwin is a bit more elaborate, it also uses Little CMS to
> read an ICC profile, but additionally supports setting the brightness
> and color temperature. It however only sets a gamma table in the end.
> Wlroots seems to have an API to set the gamma table, but I haven't seen
> where sway uses it (I may have missed that though)

wlroots delegates setting the gamma table to a privileged client, to allow
users to set it to whatever they want. Use-cases include setting the color
temperature and setting the brightness. wlroots doesn't support ICC profiles
(and I don't know of a client setting the gamma LUT from an ICC profile).

> In any case, there's limited support there for color management.

That's correct.

> Inputs would be appreciated on this, for instance with feedback on how
> X.org and Android handle color management, on how 3D LUTs are
> implemented on other platforms, or in general on how we would like to
> use them. I don't mind doing some work in userspace to prototype this,
> but I won't have the bandwidth to design a completely new framework.

Weston is working on improving color management support [1] [2]. I think it's
still a little early, but maybe see with Pekka if something can be worked out?

Other than that, maybe some media players have support for some color
management and would need to blend in multiple buffers and standardize
protocols. Maybe look into Kodi or mpv?

Simon

[1]: https://www.collabora.com/news-and-blog/blog/2020/11/19/developing-wayland-color-management-and-high-dynamic-range/
[2]: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/14
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/4] drm: rcar-du: Add cubic LUT support

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 9046 bytes --]

On Mon, 21 Dec 2020 03:57:26 +0200
Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:

> Hello,
> 
> This patch series adds cubic (3D) look up table support to the CMM and
> DU drivers, and extend the KMS userspace API to expose the cubic LUT to
> userspace.

Hi,

when you say "cubic" I immediately think "polynomial of third degree",
and got really curious how that works. But it seems that is not at all
what you have here, instead you have a 3D LUT with probably
trilinear(?) interpolation.

I would suggest to stop using the misleading term "cubic" (e.g. cubic
interpolation is a thing).

Where does the abbreviation CLU come from? If that refers to cubic as
well, it would be best to change that too to avoid misleading.

Unless your hardware actually does cubic interpolation in the 3D LUT?

> The code is fairly straightforward. Patch 1/4 refactors the CMM (color
> management module, the Renesas R-Car IP core that handles 1D and 3D
> lookup tables for the display) driver, which currently supports the 1D
> (a.k.a. gamma) table only, to prepare for 3D LUT support (including a
> modification to the API between the CMM and DU drivers). The CMM driver
> is then extended in patch 2/4 to support the 3D LUT.
> 
> Patch 3/4 adds support for the 3D LUT in the KMS core and the KMS
> userspace API, in the form of two new properties. I expect this to be
> the most controversial part of the series, not so much for the feature
> itself, but for when it is inserted in the color management pipeline.
> 
> Finally, patch 4/4 wires the KMS extension to the DU driver.
> 
> The R-Car CMM applies the 3D LUT at the output of the display, where
> data is transmitted in RGB space (when outputting YUV data to the
> display the CMM can't be used according to the documentation, but I
> wouldn't be entirely surprised if this limitation could be worked
> around), before the 1D LUT. I've located the 3D LUT between the CTM and
> the gamma LUT, but it could equally be placed before the degamma LUT or
> between the degamma LUT and the CTM in my case, as the R-Car color
> management pipeline has no CTM and has a single 1D LUT on the output
> side (there's provision for 1D LUT on the input side for some of the
> planes, but that's a separate topic).
> 
> I however don't expect this to necessarily match all hardware though,
> and this feature may require us to give up on a fixed, one size fits
> them all, color management pipeline exposed to userspace. Whether this
> would mean device-specific APIs (not necessarily in the form of
> device-specific properties, but in how they map to hardware features, as
> I think helpers to handle a 3D LUT property in the KMS core can save
> code duplication in drivers), or the need for a new property to expose
> the order in which color management operations are implemented, I don't
> know.

That is a difficult problem indeed. Userspace must know everything what
happens to the pixel values exactly, beyond that I have no suggestions
there.

> I started having a look at userspace to see how this could be handled,
> searching for color management support in weston, kwin and wlroots/sway.
> All three support setting the gamma table when using the DRM/KMS
> backend, weston and kwin through the legacy API, and wlroots through the
> atomic API. Weston reads an ICC profile using Little CMS and applies the
> gamma table. kwin is a bit more elaborate, it also uses Little CMS to
> read an ICC profile, but additionally supports setting the brightness
> and color temperature. It however only sets a gamma table in the end.
> Wlroots seems to have an API to set the gamma table, but I haven't seen
> where sway uses it (I may have missed that though). In any case, there's
> limited support there for color management.
> 
> Inputs would be appreciated on this, for instance with feedback on how
> X.org and Android handle color management, on how 3D LUTs are
> implemented on other platforms, or in general on how we would like to
> use them. I don't mind doing some work in userspace to prototype this,
> but I won't have the bandwidth to design a completely new framework.

The idea for Weston (and Wayland in general) is that the display server
uses a CMM to compute a transformation it needs to apply, based on the
display and content color properties (e.g. ICC profiles) and more. What
that transformation exactly is depends on the CMM, and it may further
depend on what kind of ICC profiles are being used (an ICC file may
contain different kinds of transformation definitions, from
parameterised standard formulas to 1D and 3D LUTs and chains of those).
So a display server gets a more or less opaque transformation object
from a CMM and needs to implement what it describes somehow.

Implementing the transformation depends on what kind of API the CMM
offers. The good thing with a 3D LUT is that, AFAIU, no matter what the
actual transformation is, it can always be represented as a 3D LUT. The
only open question then is the size and precision of the 3D LUT, and
how it is interpolated. If the precision is sufficient, a display
server may choose to use the hardware 3D LUT.

All this I believe should be internal to a Wayland display server.

Mind, that everything I talk about in the above is done in *addition*
to "the LUT" (VCGT tag in ICC files). In the X11 model of color
management, "the LUT" is considered calibration and is 3 x 1D. If some
LUT was in place when profiling a monitor, the same LUT must also be in
place when using that monitor color profile. Confusingly, calibration
values like "the LUT" are not considered as part of the color profile,
even though technically "the LUT" can be saved in an ICC file. For more
information, see:
https://ninedegreesbelow.com/photography/monitor-profile-calibrate-confuse.html
https://gitlab.freedesktop.org/swick/wayland-protocols/-/blob/color/unstable/color-management/color.rst

The code you saw in Weston loading a LUT from an ICC profile does the
VCGT tag part and nothing else. IOW, it loads a calibration, not a
monitor color profile. A 3D LUT could be regarded as calibration as it
is in the old-school model, or it could be actively computed and used
like the Wayland model being designed.

Loading a LUT from the VCGT tag is not really color management. It's
just calibration, like tuning the monitor brightness knob.

Simon already pointed you to
https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/14
and Weston work is underway as well, but it may be quite some time
before Weston could actually take advantage of a KMS 3D LUT. In Weston
we consider hardware features like the 3D LUT as optional optimisations
that can be used if the circumstances are right. This means that we
want the "software path", that is, GL shaders, to work first. Shaders
we can run and test everywhere, but the 3D LUT is not always available
and requires a writeback connector to test in an automated test suite..

As a summary, I could say that X.org does not do color management. Period.
X.org does allow loading "the LUT" (VCGT), but it does nothing with
color profiles. Applying color profiles is left for each X11 app to do
on their own to their own pixels. If X11 protocol extensions do not
already support setting a 3D LUT, then you have to add that to the
protocol.

Wayland is being designed differently: the display server is
responsible for color management, unless Wayland applications
explicitly ask to do it themselves. Wayland protocol is intended to
not allow free-to-all access to hardware LUTs like X11 does.

Therefore, I would say that while adding X11 protocol to set a 3D LUT
in hardware does mechanically exercise the UAPI, it does not in my
opinion prove anything about the usability of the UAPI, because it
lacks an actual use case. You would need something to meaningfully and
purposefully to set the 3D LUT to some particular values to reach some
specific effect or goal for a real world use case. This may be an
unpopular opinion, but IMO X11 RandR is little more than a library ABI
exposing KMS UAPI as is, similar to libdrm: an interface, not a full
application alone.

CrOS and Kodi mentioned by danvet sound like much better proving
vehicles than X.org, Weston I'm afraid might be quite some months away
still.


A consideration from KMS UAPI point of view: for the 3D LUT to be
usable for Wayland compositors, setting the 3D LUT must be either
glitch-free, or if it can cause a hickup, glitch, stall, or anything
else not observed during a normal pageflip, it must require
ALLOW_MODESET.

If setting any KMS property is possible without ALLOW_MODESET but it
may cause a visible glitch, pageflip timings included, then that KMS
property is much less useful to a Wayland display server as it can only
be used as if it always required ALLOW_MODESET, even with drivers where
it is glitch-free.


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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