[PATCH v2 0/4] drm/komeda: Enable layer/plane color mgmt

[PATCH v2 0/4] drm/komeda: Enable layer/plane color mgmt

[james qian wang (Arm Technology China)]

This patch series enabled layer/plane color management for komeda driver

v2: Rebase it on drm-misc-next

james qian wang (Arm Technology China) (4):
  drm/komeda: Introduce komeda_coeffs_table/manager
  drm/komeda: Introduce komeda_color_manager/state
  drm: Increase DRM_OBJECT_MAX_PROPERTY to 32
  drm/komeda: Enable Layer color management for komeda

 drivers/gpu/drm/arm/display/komeda/Makefile   |   1 +
 .../arm/display/komeda/d71/d71_component.c    |  64 +++++++++
 .../gpu/drm/arm/display/komeda/d71/d71_dev.c  |   5 +-
 .../gpu/drm/arm/display/komeda/d71/d71_dev.h  |   2 +
 .../drm/arm/display/komeda/komeda_coeffs.c    | 119 +++++++++++++++++
 .../drm/arm/display/komeda/komeda_coeffs.h    |  74 ++++++++++
 .../arm/display/komeda/komeda_color_mgmt.c    | 126 ++++++++++++++++++
 .../arm/display/komeda/komeda_color_mgmt.h    |  32 ++++-
 .../drm/arm/display/komeda/komeda_pipeline.h  |   4 +-
 .../display/komeda/komeda_pipeline_state.c    |  53 +++++++-
 .../gpu/drm/arm/display/komeda/komeda_plane.c |  12 ++
 .../arm/display/komeda/komeda_private_obj.c   |   4 +
 include/drm/drm_mode_object.h                 |   2 +-
 13 files changed, 490 insertions(+), 8 deletions(-)
 create mode 100644 drivers/gpu/drm/arm/display/komeda/komeda_coeffs.c
 create mode 100644 drivers/gpu/drm/arm/display/komeda/komeda_coeffs.h

--
2.20.1

[PATCH v2 1/4] drm/komeda: Introduce komeda_coeffs_table/manager

[james qian wang (Arm Technology China)]

komeda display HWs have kinds of coefficient tables for various purposes
like gamma/degamma. ususally these tables are shared by multiple HW
component and have limited number.
Introduce komeda_coeffs_table/manager for describing and managing these
tables, like table reuse, racing.

Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
---
 drivers/gpu/drm/arm/display/komeda/Makefile   |   1 +
 .../drm/arm/display/komeda/komeda_coeffs.c    | 119 ++++++++++++++++++
 .../drm/arm/display/komeda/komeda_coeffs.h    |  74 +++++++++++
 3 files changed, 194 insertions(+)
 create mode 100644 drivers/gpu/drm/arm/display/komeda/komeda_coeffs.c
 create mode 100644 drivers/gpu/drm/arm/display/komeda/komeda_coeffs.h

diff --git a/drivers/gpu/drm/arm/display/komeda/Makefile b/drivers/gpu/drm/arm/display/komeda/Makefile
index 5c3900c2e764..38aa5843c03a 100644
--- a/drivers/gpu/drm/arm/display/komeda/Makefile
+++ b/drivers/gpu/drm/arm/display/komeda/Makefile
@@ -8,6 +8,7 @@ komeda-y := \
 	komeda_drv.o \
 	komeda_dev.o \
 	komeda_format_caps.o \
+	komeda_coeffs.o \
 	komeda_color_mgmt.o \
 	komeda_pipeline.o \
 	komeda_pipeline_state.o \
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_coeffs.c b/drivers/gpu/drm/arm/display/komeda/komeda_coeffs.c
new file mode 100644
index 000000000000..d9d35e23003c
--- /dev/null
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_coeffs.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * (C) COPYRIGHT 2019 ARM Limited. All rights reserved.
+ * Author: James.Qian.Wang <james.qian.wang@arm.com>
+ *
+ */
+#include <linux/slab.h>
+#include "komeda_coeffs.h"
+
+static inline bool is_table_in_using(struct komeda_coeffs_table *table)
+{
+	return refcount_read(&table->refcount) > 1;
+}
+
+/* request a coeffs table for the coeffs data specified by argument coeffs */
+struct komeda_coeffs_table *
+komeda_coeffs_request(struct komeda_coeffs_manager *mgr, void *coeffs)
+{
+	struct komeda_coeffs_table *table;
+	u32 i;
+
+	mutex_lock(&mgr->mutex);
+
+	/* search table list to find if there is a in-using table with the
+	 * same coefficient content, if find, reuse this table.
+	 */
+	for (i = 0; i < mgr->n_tables; i++) {
+		table = mgr->tables[i];
+
+		/* skip the unused table */
+		if (!is_table_in_using(table))
+			continue;
+
+		if (!memcmp(table->coeffs, coeffs, mgr->coeffs_sz))
+			goto found;
+	}
+
+	/* can not reuse the existing in-using table, loop for a new one */
+	for (i = 0; i < mgr->n_tables; i++) {
+		table = mgr->tables[i];
+
+		if (!is_table_in_using(table)) {
+			memcpy(table->coeffs, coeffs, mgr->coeffs_sz);
+			table->needs_update = true;
+			goto found;
+		}
+	}
+
+	/* Since previous two search loop will directly goto found if found an
+	 * available table, so once program ran here means search failed.
+	 * clear the table to NULL, unlock(mgr->mutex) and return NULL.
+	 */
+	table = NULL;
+
+found:
+	komeda_coeffs_get(table);
+	mutex_unlock(&mgr->mutex);
+	return table;
+}
+
+/* Add a coeffs table to manager */
+int komeda_coeffs_add(struct komeda_coeffs_manager *mgr,
+		      u32 hw_id, u32 __iomem *reg,
+		      void (*update)(struct komeda_coeffs_table *table))
+{
+	struct komeda_coeffs_table *table;
+
+	if (mgr->n_tables >= ARRAY_SIZE(mgr->tables))
+		return -ENOSPC;
+
+	table = kzalloc(sizeof(*table), GFP_KERNEL);
+	if (!table)
+		return -ENOMEM;
+
+	table->coeffs = kzalloc(mgr->coeffs_sz, GFP_KERNEL);
+	if (!table->coeffs) {
+		kfree(table);
+		return -ENOMEM;
+	}
+
+	refcount_set(&table->refcount, 1);
+	table->mgr = mgr;
+	table->hw_id = hw_id;
+	table->update = update;
+	table->reg = reg;
+
+	mgr->tables[mgr->n_tables++] = table;
+	return 0;
+}
+
+struct komeda_coeffs_manager *komeda_coeffs_create_manager(u32 coeffs_sz)
+{
+	struct komeda_coeffs_manager *mgr;
+
+	mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_init(&mgr->mutex);
+	mgr->coeffs_sz = coeffs_sz;
+
+	return mgr;
+}
+
+void komeda_coeffs_destroy_manager(struct komeda_coeffs_manager *mgr)
+{
+	u32 i;
+
+	if (!mgr)
+		return;
+
+	for (i = 0; i < mgr->n_tables; i++) {
+		WARN_ON(is_table_in_using(mgr->tables[i]));
+		kfree(mgr->tables[i]->coeffs);
+		kfree(mgr->tables[i]);
+	}
+
+	kfree(mgr);
+}
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_coeffs.h b/drivers/gpu/drm/arm/display/komeda/komeda_coeffs.h
new file mode 100644
index 000000000000..172ac2ea17ba
--- /dev/null
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_coeffs.h
@@ -0,0 +1,74 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * (C) COPYRIGHT 2019 ARM Limited. All rights reserved.
+ * Author: James.Qian.Wang <james.qian.wang@arm.com>
+ *
+ */
+#ifndef _KOMEDA_COEFFS_H_
+#define _KOMEDA_COEFFS_H_
+
+#include <linux/refcount.h>
+
+/* Komeda display HWs have kinds of coefficient tables for various purposes,
+ * like gamma/degamma. ususally these tables are shared by multiple HW component
+ * and limited number.
+ * The komeda_coeffs_table/manager are imported for describing and managing
+ * these tables for table reuse and racing.
+ */
+struct komeda_coeffs_table {
+	struct komeda_coeffs_manager *mgr;
+	refcount_t refcount;
+	bool needs_update;
+	u32 hw_id;
+	void *coeffs;
+	u32 __iomem *reg;
+	void (*update)(struct komeda_coeffs_table *table);
+};
+
+struct komeda_coeffs_manager {
+	struct mutex mutex; /* for tables accessing */
+	u32 n_tables;
+	u32 coeffs_sz;
+	struct komeda_coeffs_table *tables[8];
+};
+
+static inline struct komeda_coeffs_table *
+komeda_coeffs_get(struct komeda_coeffs_table *table)
+{
+	if (table)
+		refcount_inc(&table->refcount);
+
+	return table;
+}
+
+static inline void __komeda_coeffs_put(struct komeda_coeffs_table *table)
+{
+	if (table)
+		refcount_dec(&table->refcount);
+}
+
+#define komeda_coeffs_put(table) \
+do { \
+	__komeda_coeffs_put(table); \
+	(table) = NULL; \
+} while (0)
+
+static inline void komeda_coeffs_update(struct komeda_coeffs_table *table)
+{
+	if (!table || !table->needs_update)
+		return;
+
+	table->update(table);
+	table->needs_update = false;
+}
+
+struct komeda_coeffs_manager *komeda_coeffs_create_manager(u32 coeffs_sz);
+void komeda_coeffs_destroy_manager(struct komeda_coeffs_manager *mgr);
+
+int komeda_coeffs_add(struct komeda_coeffs_manager *mgr,
+		      u32 hw_id, u32 __iomem *reg,
+		      void (*update)(struct komeda_coeffs_table *table));
+struct komeda_coeffs_table *
+komeda_coeffs_request(struct komeda_coeffs_manager *mgr, void *coeffs);
+
+#endif /*_KOMEDA_COEFFS_H_*/
-- 
2.20.1

[PATCH v2 2/4] drm/komeda: Introduce komeda_color_manager/state

[james qian wang (Arm Technology China)]

Many komeda component support color management like layer and IPS, so
komeda_color_manager/state are introduced to manager gamma, csc and degamma
together for easily share it to multiple componpent.

And for komeda_color_manager which:
- convert drm 3d gamma lut to komeda specific gamma coeffs
- gamma table management and hide the HW difference for komeda-CORE

Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
---
 .../arm/display/komeda/komeda_color_mgmt.c    | 126 ++++++++++++++++++
 .../arm/display/komeda/komeda_color_mgmt.h    |  32 ++++-
 2 files changed, 156 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
index 9d14a92dbb17..bf2388d641b9 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
@@ -4,7 +4,9 @@
  * Author: James.Qian.Wang <james.qian.wang@arm.com>
  *
  */
+#include <drm/drm_print.h>
 
+#include "malidp_utils.h"
 #include "komeda_color_mgmt.h"
 
 /* 10bit precision YUV2RGB matrix */
@@ -65,3 +67,127 @@ const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range)
 
 	return coeffs;
 }
+
+struct gamma_curve_sector {
+	u32 boundary_start;
+	u32 num_of_segments;
+	u32 segment_width;
+};
+
+struct gamma_curve_segment {
+	u32 start;
+	u32 end;
+};
+
+static struct gamma_curve_sector sector_tbl[] = {
+	{ 0,    4,  4   },
+	{ 16,   4,  4   },
+	{ 32,   4,  8   },
+	{ 64,   4,  16  },
+	{ 128,  4,  32  },
+	{ 256,  4,  64  },
+	{ 512,  16, 32  },
+	{ 1024, 24, 128 },
+};
+
+static struct gamma_curve_sector igamma_sector_tbl[] = {
+	{0, 64, 64},
+};
+
+static void
+drm_lut_to_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs,
+		  struct gamma_curve_sector *sector_tbl, u32 num_sectors)
+{
+	struct drm_color_lut *lut;
+	u32 i, j, in, num = 0;
+
+	if (!lut_blob)
+		return;
+
+	lut = lut_blob->data;
+
+	for (i = 0; i < num_sectors; i++) {
+		for (j = 0; j < sector_tbl[i].num_of_segments; j++) {
+			in = sector_tbl[i].boundary_start +
+			     j * sector_tbl[i].segment_width;
+
+			coeffs[num++] = drm_color_lut_extract(lut[in].red,
+						KOMEDA_COLOR_PRECISION);
+		}
+	}
+
+	coeffs[num] = BIT(KOMEDA_COLOR_PRECISION);
+}
+
+void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
+{
+	drm_lut_to_coeffs(lut_blob, coeffs,
+			  igamma_sector_tbl, ARRAY_SIZE(igamma_sector_tbl));
+}
+
+void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
+{
+	drm_lut_to_coeffs(lut_blob, coeffs,
+			  sector_tbl, ARRAY_SIZE(sector_tbl));
+}
+
+void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs)
+{
+	struct drm_color_ctm *ctm;
+	u32 i;
+
+	if (!ctm_blob)
+		return;
+
+	ctm = ctm_blob->data;
+
+	for (i = 0; i < KOMEDA_N_CTM_COEFFS; ++i) {
+		/* Convert from S31.32 to Q3.12. */
+		s64 v = ctm->matrix[i];
+
+		coeffs[i] = clamp_val(v, 1 - (1LL << 34), (1LL << 34) - 1) >> 20;
+	}
+}
+
+void komeda_color_duplicate_state(struct komeda_color_state *new,
+				  struct komeda_color_state *old)
+{
+	new->igamma = komeda_coeffs_get(old->igamma);
+	new->fgamma = komeda_coeffs_get(old->fgamma);
+}
+
+void komeda_color_cleanup_state(struct komeda_color_state *color_st)
+{
+	komeda_coeffs_put(color_st->igamma);
+	komeda_coeffs_put(color_st->fgamma);
+}
+
+int komeda_color_validate(struct komeda_color_manager *mgr,
+			  struct komeda_color_state *st,
+			  struct drm_property_blob *igamma_blob,
+			  struct drm_property_blob *fgamma_blob)
+{
+	u32 coeffs[KOMEDA_N_GAMMA_COEFFS];
+
+	komeda_color_cleanup_state(st);
+
+	if (igamma_blob) {
+		drm_lut_to_igamma_coeffs(igamma_blob, coeffs);
+		st->igamma = komeda_coeffs_request(mgr->igamma_mgr, coeffs);
+		if (!st->igamma) {
+			DRM_DEBUG_ATOMIC("request igamma table failed.\n");
+			return -EBUSY;
+		}
+	}
+
+	if (fgamma_blob) {
+		drm_lut_to_fgamma_coeffs(fgamma_blob, coeffs);
+		st->fgamma = komeda_coeffs_request(mgr->fgamma_mgr, coeffs);
+		if (!st->fgamma) {
+			DRM_DEBUG_ATOMIC("request fgamma table failed.\n");
+			return -EBUSY;
+		}
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
index a2df218f58e7..41a96b3b540f 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
@@ -4,14 +4,42 @@
  * Author: James.Qian.Wang <james.qian.wang@arm.com>
  *
  */
-
 #ifndef _KOMEDA_COLOR_MGMT_H_
 #define _KOMEDA_COLOR_MGMT_H_
 
 #include <drm/drm_color_mgmt.h>
+#include "komeda_coeffs.h"
 
 #define KOMEDA_N_YUV2RGB_COEFFS		12
+#define KOMEDA_N_RGB2YUV_COEFFS		12
+#define KOMEDA_COLOR_PRECISION		12
+#define KOMEDA_N_GAMMA_COEFFS		65
+#define KOMEDA_COLOR_LUT_SIZE		BIT(KOMEDA_COLOR_PRECISION)
+#define KOMEDA_N_CTM_COEFFS		9
+
+struct komeda_color_manager {
+	struct komeda_coeffs_manager *igamma_mgr;
+	struct komeda_coeffs_manager *fgamma_mgr;
+	bool has_ctm;
+};
+
+struct komeda_color_state {
+	struct komeda_coeffs_table *igamma;
+	struct komeda_coeffs_table *fgamma;
+};
+
+void komeda_color_duplicate_state(struct komeda_color_state *new,
+				  struct komeda_color_state *old);
+void komeda_color_cleanup_state(struct komeda_color_state *color_st);
+int komeda_color_validate(struct komeda_color_manager *mgr,
+			  struct komeda_color_state *st,
+			  struct drm_property_blob *igamma_blob,
+			  struct drm_property_blob *fgamma_blob);
+
+void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
+void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
+void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs);
 
 const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range);
 
-#endif
+#endif /*_KOMEDA_COLOR_MGMT_H_*/
-- 
2.20.1

Re: [PATCH v2 2/4] drm/komeda: Introduce komeda_color_manager/state

[Mihail Atanassov]

Hi James,

On Tuesday, 13 August 2019 05:56:07 BST james qian wang (Arm Technology China) wrote:
> Many komeda component support color management like layer and IPS, so
> komeda_color_manager/state are introduced to manager gamma, csc and degamma
> together for easily share it to multiple componpent.
> 
> And for komeda_color_manager which:
> - convert drm 3d gamma lut to komeda specific gamma coeffs
> - gamma table management and hide the HW difference for komeda-CORE
> 
> Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> ---
>  .../arm/display/komeda/komeda_color_mgmt.c    | 126 ++++++++++++++++++
>  .../arm/display/komeda/komeda_color_mgmt.h    |  32 ++++-
>  2 files changed, 156 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> index 9d14a92dbb17..bf2388d641b9 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> @@ -4,7 +4,9 @@
>   * Author: James.Qian.Wang <james.qian.wang@arm.com>
>   *
>   */
> +#include <drm/drm_print.h>
>  
> +#include "malidp_utils.h"
>  #include "komeda_color_mgmt.h"
>  
>  /* 10bit precision YUV2RGB matrix */
> @@ -65,3 +67,127 @@ const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range)
>  
>  	return coeffs;
>  }
> +
> +struct gamma_curve_sector {
> +	u32 boundary_start;
> +	u32 num_of_segments;
> +	u32 segment_width;
> +};
> +
> +struct gamma_curve_segment {
> +	u32 start;
> +	u32 end;
> +};
> +
> +static struct gamma_curve_sector sector_tbl[] = {
> +	{ 0,    4,  4   },
> +	{ 16,   4,  4   },
> +	{ 32,   4,  8   },
> +	{ 64,   4,  16  },
> +	{ 128,  4,  32  },
> +	{ 256,  4,  64  },
> +	{ 512,  16, 32  },
> +	{ 1024, 24, 128 },
> +};
> +
> +static struct gamma_curve_sector igamma_sector_tbl[] = {
> +	{0, 64, 64},
> +};
> +
> +static void
> +drm_lut_to_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs,
> +		  struct gamma_curve_sector *sector_tbl, u32 num_sectors)
> +{
> +	struct drm_color_lut *lut;
> +	u32 i, j, in, num = 0;
> +
> +	if (!lut_blob)
> +		return;
> +
> +	lut = lut_blob->data;
> +
> +	for (i = 0; i < num_sectors; i++) {
> +		for (j = 0; j < sector_tbl[i].num_of_segments; j++) {
> +			in = sector_tbl[i].boundary_start +
> +			     j * sector_tbl[i].segment_width;
> +
> +			coeffs[num++] = drm_color_lut_extract(lut[in].red,
> +						KOMEDA_COLOR_PRECISION);
> +		}
> +	}
> +
> +	coeffs[num] = BIT(KOMEDA_COLOR_PRECISION);
> +}
> +
> +void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
> +{
> +	drm_lut_to_coeffs(lut_blob, coeffs,
> +			  igamma_sector_tbl, ARRAY_SIZE(igamma_sector_tbl));
> +}
> +
> +void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
> +{
> +	drm_lut_to_coeffs(lut_blob, coeffs,
> +			  sector_tbl, ARRAY_SIZE(sector_tbl));
> +}
> +
> +void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs)
> +{
> +	struct drm_color_ctm *ctm;
> +	u32 i;
> +
> +	if (!ctm_blob)
> +		return;
> +
> +	ctm = ctm_blob->data;
> +
> +	for (i = 0; i < KOMEDA_N_CTM_COEFFS; ++i) {
> +		/* Convert from S31.32 to Q3.12. */
> +		s64 v = ctm->matrix[i];
> +
> +		coeffs[i] = clamp_val(v, 1 - (1LL << 34), (1LL << 34) - 1) >> 20;
CTM matrix values are S31.32, i.e. sign-magnitude, so clamp_val won't
give you the right result for negative coeffs. See
malidp_crtc_atomic_check_ctm for the sign-mag -> 2's complement
conversion.
> +	}
> +}
> +
> +void komeda_color_duplicate_state(struct komeda_color_state *new,
> +				  struct komeda_color_state *old)
[bikeshed] not really a _duplicate_state if all it does is refcounts.
kmemdup here and return a pointer (with ERR_PTR on fail), or memcpy if
you want to keep the signature?
> +{
> +	new->igamma = komeda_coeffs_get(old->igamma);
> +	new->fgamma = komeda_coeffs_get(old->fgamma);
> +}
> +
> +void komeda_color_cleanup_state(struct komeda_color_state *color_st)
> +{
> +	komeda_coeffs_put(color_st->igamma);
> +	komeda_coeffs_put(color_st->fgamma);
> +}
> +
> +int komeda_color_validate(struct komeda_color_manager *mgr,
> +			  struct komeda_color_state *st,
> +			  struct drm_property_blob *igamma_blob,
> +			  struct drm_property_blob *fgamma_blob)
> +{
> +	u32 coeffs[KOMEDA_N_GAMMA_COEFFS];
> +
> +	komeda_color_cleanup_state(st);
> +
> +	if (igamma_blob) {
> +		drm_lut_to_igamma_coeffs(igamma_blob, coeffs);
> +		st->igamma = komeda_coeffs_request(mgr->igamma_mgr, coeffs);
> +		if (!st->igamma) {
> +			DRM_DEBUG_ATOMIC("request igamma table failed.\n");
> +			return -EBUSY;
> +		}
> +	}
> +
> +	if (fgamma_blob) {
> +		drm_lut_to_fgamma_coeffs(fgamma_blob, coeffs);
> +		st->fgamma = komeda_coeffs_request(mgr->fgamma_mgr, coeffs);
> +		if (!st->fgamma) {
> +			DRM_DEBUG_ATOMIC("request fgamma table failed.\n");
> +			return -EBUSY;
> +		}
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> index a2df218f58e7..41a96b3b540f 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> @@ -4,14 +4,42 @@
>   * Author: James.Qian.Wang <james.qian.wang@arm.com>
>   *
>   */
> -
>  #ifndef _KOMEDA_COLOR_MGMT_H_
>  #define _KOMEDA_COLOR_MGMT_H_
>  
>  #include <drm/drm_color_mgmt.h>
> +#include "komeda_coeffs.h"
>  
>  #define KOMEDA_N_YUV2RGB_COEFFS		12
> +#define KOMEDA_N_RGB2YUV_COEFFS		12
> +#define KOMEDA_COLOR_PRECISION		12
> +#define KOMEDA_N_GAMMA_COEFFS		65
> +#define KOMEDA_COLOR_LUT_SIZE		BIT(KOMEDA_COLOR_PRECISION)
I don't see how the number of LUT entries has anything to do with the
bit-precision of each entry.
> +#define KOMEDA_N_CTM_COEFFS		9
> +
> +struct komeda_color_manager {
> +	struct komeda_coeffs_manager *igamma_mgr;
> +	struct komeda_coeffs_manager *fgamma_mgr;
> +	bool has_ctm;
> +};
> +
> +struct komeda_color_state {
> +	struct komeda_coeffs_table *igamma;
> +	struct komeda_coeffs_table *fgamma;
> +};
> +
> +void komeda_color_duplicate_state(struct komeda_color_state *new,
> +				  struct komeda_color_state *old);
> +void komeda_color_cleanup_state(struct komeda_color_state *color_st);
> +int komeda_color_validate(struct komeda_color_manager *mgr,
> +			  struct komeda_color_state *st,
> +			  struct drm_property_blob *igamma_blob,
> +			  struct drm_property_blob *fgamma_blob);
> +
> +void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
> +void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
> +void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs);
>  
>  const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range);
>  
> -#endif
> +#endif /*_KOMEDA_COLOR_MGMT_H_*/
> 

BR,
Mihail

Re: [PATCH v2 2/4] drm/komeda: Introduce komeda_color_manager/state

[james qian wang (Arm Technology China)]

On Tue, Aug 13, 2019 at 09:51:08AM +0000, Mihail Atanassov wrote:
> Hi James,
> 
> On Tuesday, 13 August 2019 05:56:07 BST james qian wang (Arm Technology China) wrote:
> > Many komeda component support color management like layer and IPS, so
> > komeda_color_manager/state are introduced to manager gamma, csc and degamma
> > together for easily share it to multiple componpent.
> > 
> > And for komeda_color_manager which:
> > - convert drm 3d gamma lut to komeda specific gamma coeffs
> > - gamma table management and hide the HW difference for komeda-CORE
> > 
> > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> > ---
> >  .../arm/display/komeda/komeda_color_mgmt.c    | 126 ++++++++++++++++++
> >  .../arm/display/komeda/komeda_color_mgmt.h    |  32 ++++-
> >  2 files changed, 156 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > index 9d14a92dbb17..bf2388d641b9 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > @@ -4,7 +4,9 @@
> >   * Author: James.Qian.Wang <james.qian.wang@arm.com>
> >   *
> >   */
> > +#include <drm/drm_print.h>
> >  
> > +#include "malidp_utils.h"
> >  #include "komeda_color_mgmt.h"
> >  
> >  /* 10bit precision YUV2RGB matrix */
> > @@ -65,3 +67,127 @@ const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range)
> >  
> >  	return coeffs;
> >  }
> > +
> > +struct gamma_curve_sector {
> > +	u32 boundary_start;
> > +	u32 num_of_segments;
> > +	u32 segment_width;
> > +};
> > +
> > +struct gamma_curve_segment {
> > +	u32 start;
> > +	u32 end;
> > +};
> > +
> > +static struct gamma_curve_sector sector_tbl[] = {
> > +	{ 0,    4,  4   },
> > +	{ 16,   4,  4   },
> > +	{ 32,   4,  8   },
> > +	{ 64,   4,  16  },
> > +	{ 128,  4,  32  },
> > +	{ 256,  4,  64  },
> > +	{ 512,  16, 32  },
> > +	{ 1024, 24, 128 },
> > +};
> > +
> > +static struct gamma_curve_sector igamma_sector_tbl[] = {
> > +	{0, 64, 64},
> > +};
> > +
> > +static void
> > +drm_lut_to_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs,
> > +		  struct gamma_curve_sector *sector_tbl, u32 num_sectors)
> > +{
> > +	struct drm_color_lut *lut;
> > +	u32 i, j, in, num = 0;
> > +
> > +	if (!lut_blob)
> > +		return;
> > +
> > +	lut = lut_blob->data;
> > +
> > +	for (i = 0; i < num_sectors; i++) {
> > +		for (j = 0; j < sector_tbl[i].num_of_segments; j++) {
> > +			in = sector_tbl[i].boundary_start +
> > +			     j * sector_tbl[i].segment_width;
> > +
> > +			coeffs[num++] = drm_color_lut_extract(lut[in].red,
> > +						KOMEDA_COLOR_PRECISION);
> > +		}
> > +	}
> > +
> > +	coeffs[num] = BIT(KOMEDA_COLOR_PRECISION);
> > +}
> > +
> > +void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
> > +{
> > +	drm_lut_to_coeffs(lut_blob, coeffs,
> > +			  igamma_sector_tbl, ARRAY_SIZE(igamma_sector_tbl));
> > +}
> > +
> > +void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
> > +{
> > +	drm_lut_to_coeffs(lut_blob, coeffs,
> > +			  sector_tbl, ARRAY_SIZE(sector_tbl));
> > +}
> > +
> > +void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs)
> > +{
> > +	struct drm_color_ctm *ctm;
> > +	u32 i;
> > +
> > +	if (!ctm_blob)
> > +		return;
> > +
> > +	ctm = ctm_blob->data;
> > +
> > +	for (i = 0; i < KOMEDA_N_CTM_COEFFS; ++i) {
> > +		/* Convert from S31.32 to Q3.12. */
> > +		s64 v = ctm->matrix[i];
> > +
> > +		coeffs[i] = clamp_val(v, 1 - (1LL << 34), (1LL << 34) - 1) >> 20;
> CTM matrix values are S31.32, i.e. sign-magnitude, so clamp_val won't
> give you the right result for negative coeffs. See
> malidp_crtc_atomic_check_ctm for the sign-mag -> 2's complement
> conversion.

Thank you Mihail for pointing this out.

No matter our user or kernel all assume this s31.32 as 2's complement. 
we need to correct them both.

> > +	}
> > +}
> > +
> > +void komeda_color_duplicate_state(struct komeda_color_state *new,
> > +				  struct komeda_color_state *old)
> [bikeshed] not really a _duplicate_state if all it does is refcounts.
> kmemdup here and return a pointer (with ERR_PTR on fail), or memcpy if
> you want to keep the signature?

Yes, the dup mostly should return a new ptr from a old, the dup name here
is not accurate.
the reason is the color_state is not a separated structure but always
embedded into layer_state, but I want to make all color_state operation
into a func.
Do you have any suggestion ?

> > +{
> > +	new->igamma = komeda_coeffs_get(old->igamma);
> > +	new->fgamma = komeda_coeffs_get(old->fgamma);
> > +}
> > +
> > +void komeda_color_cleanup_state(struct komeda_color_state *color_st)
> > +{
> > +	komeda_coeffs_put(color_st->igamma);
> > +	komeda_coeffs_put(color_st->fgamma);
> > +}
> > +
> > +int komeda_color_validate(struct komeda_color_manager *mgr,
> > +			  struct komeda_color_state *st,
> > +			  struct drm_property_blob *igamma_blob,
> > +			  struct drm_property_blob *fgamma_blob)
> > +{
> > +	u32 coeffs[KOMEDA_N_GAMMA_COEFFS];
> > +
> > +	komeda_color_cleanup_state(st);
> > +
> > +	if (igamma_blob) {
> > +		drm_lut_to_igamma_coeffs(igamma_blob, coeffs);
> > +		st->igamma = komeda_coeffs_request(mgr->igamma_mgr, coeffs);
> > +		if (!st->igamma) {
> > +			DRM_DEBUG_ATOMIC("request igamma table failed.\n");
> > +			return -EBUSY;
> > +		}
> > +	}
> > +
> > +	if (fgamma_blob) {
> > +		drm_lut_to_fgamma_coeffs(fgamma_blob, coeffs);
> > +		st->fgamma = komeda_coeffs_request(mgr->fgamma_mgr, coeffs);
> > +		if (!st->fgamma) {
> > +			DRM_DEBUG_ATOMIC("request fgamma table failed.\n");
> > +			return -EBUSY;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > index a2df218f58e7..41a96b3b540f 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > @@ -4,14 +4,42 @@
> >   * Author: James.Qian.Wang <james.qian.wang@arm.com>
> >   *
> >   */
> > -
> >  #ifndef _KOMEDA_COLOR_MGMT_H_
> >  #define _KOMEDA_COLOR_MGMT_H_
> >  
> >  #include <drm/drm_color_mgmt.h>
> > +#include "komeda_coeffs.h"
> >  
> >  #define KOMEDA_N_YUV2RGB_COEFFS		12
> > +#define KOMEDA_N_RGB2YUV_COEFFS		12
> > +#define KOMEDA_COLOR_PRECISION		12
> > +#define KOMEDA_N_GAMMA_COEFFS		65
> > +#define KOMEDA_COLOR_LUT_SIZE		BIT(KOMEDA_COLOR_PRECISION)

> I don't see how the number of LUT entries has anything to do with the
> bit-precision of each entry.

Because our komeda color is 12-bit precison, and for 1 vs 1 loopup
table, we need BIT(12) entries.

Thank you
James

> > +#define KOMEDA_N_CTM_COEFFS		9
> > +
> > +struct komeda_color_manager {
> > +	struct komeda_coeffs_manager *igamma_mgr;
> > +	struct komeda_coeffs_manager *fgamma_mgr;
> > +	bool has_ctm;
> > +};
> > +
> > +struct komeda_color_state {
> > +	struct komeda_coeffs_table *igamma;
> > +	struct komeda_coeffs_table *fgamma;
> > +};
> > +
> > +void komeda_color_duplicate_state(struct komeda_color_state *new,
> > +				  struct komeda_color_state *old);
> > +void komeda_color_cleanup_state(struct komeda_color_state *color_st);
> > +int komeda_color_validate(struct komeda_color_manager *mgr,
> > +			  struct komeda_color_state *st,
> > +			  struct drm_property_blob *igamma_blob,
> > +			  struct drm_property_blob *fgamma_blob);
> > +
> > +void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
> > +void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
> > +void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs);
> >  
> >  const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range);
> >  
> > -#endif
> > +#endif /*_KOMEDA_COLOR_MGMT_H_*/
> > 
> 
> BR,
> Mihail
> 
> 

Re: [PATCH v2 2/4] drm/komeda: Introduce komeda_color_manager/state

[Mihail Atanassov]

On Wednesday, 14 August 2019 08:52:18 BST james qian wang (Arm Technology China) wrote:
> On Tue, Aug 13, 2019 at 09:51:08AM +0000, Mihail Atanassov wrote:
> > Hi James,
> > 
> > On Tuesday, 13 August 2019 05:56:07 BST james qian wang (Arm Technology China) wrote:
> > > Many komeda component support color management like layer and IPS, so
> > > komeda_color_manager/state are introduced to manager gamma, csc and degamma
> > > together for easily share it to multiple componpent.
> > > 
> > > And for komeda_color_manager which:
> > > - convert drm 3d gamma lut to komeda specific gamma coeffs
> > > - gamma table management and hide the HW difference for komeda-CORE
> > > 
> > > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> > > ---
> > >  .../arm/display/komeda/komeda_color_mgmt.c    | 126 ++++++++++++++++++
> > >  .../arm/display/komeda/komeda_color_mgmt.h    |  32 ++++-
> > >  2 files changed, 156 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > > index 9d14a92dbb17..bf2388d641b9 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > > @@ -4,7 +4,9 @@
> > >   * Author: James.Qian.Wang <james.qian.wang@arm.com>
> > >   *
> > >   */
> > > +#include <drm/drm_print.h>
> > >  
> > > +#include "malidp_utils.h"
> > >  #include "komeda_color_mgmt.h"
> > >  
> > >  /* 10bit precision YUV2RGB matrix */
> > > @@ -65,3 +67,127 @@ const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range)
> > >  
> > >  	return coeffs;
> > >  }
> > > +
> > > +struct gamma_curve_sector {
> > > +	u32 boundary_start;
> > > +	u32 num_of_segments;
> > > +	u32 segment_width;
> > > +};
> > > +
> > > +struct gamma_curve_segment {
> > > +	u32 start;
> > > +	u32 end;
> > > +};
> > > +
> > > +static struct gamma_curve_sector sector_tbl[] = {
> > > +	{ 0,    4,  4   },
Max LUT precision (see full response below) is determined by your
smallest segment, which is 4.
> > > +	{ 16,   4,  4   },
> > > +	{ 32,   4,  8   },
> > > +	{ 64,   4,  16  },
> > > +	{ 128,  4,  32  },
> > > +	{ 256,  4,  64  },
> > > +	{ 512,  16, 32  },
> > > +	{ 1024, 24, 128 },
> > > +};
> > > +
> > > +static struct gamma_curve_sector igamma_sector_tbl[] = {
> > > +	{0, 64, 64},
> > > +};
> > > +
> > > +static void
> > > +drm_lut_to_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs,
> > > +		  struct gamma_curve_sector *sector_tbl, u32 num_sectors)
> > > +{
> > > +	struct drm_color_lut *lut;
> > > +	u32 i, j, in, num = 0;
> > > +
> > > +	if (!lut_blob)
> > > +		return;
> > > +
> > > +	lut = lut_blob->data;
> > > +
> > > +	for (i = 0; i < num_sectors; i++) {
> > > +		for (j = 0; j < sector_tbl[i].num_of_segments; j++) {
> > > +			in = sector_tbl[i].boundary_start +
> > > +			     j * sector_tbl[i].segment_width;
> > > +
> > > +			coeffs[num++] = drm_color_lut_extract(lut[in].red,
> > > +						KOMEDA_COLOR_PRECISION);
> > > +		}
> > > +	}
> > > +
> > > +	coeffs[num] = BIT(KOMEDA_COLOR_PRECISION);
> > > +}
> > > +
> > > +void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
> > > +{
> > > +	drm_lut_to_coeffs(lut_blob, coeffs,
> > > +			  igamma_sector_tbl, ARRAY_SIZE(igamma_sector_tbl));
> > > +}
> > > +
> > > +void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
> > > +{
> > > +	drm_lut_to_coeffs(lut_blob, coeffs,
> > > +			  sector_tbl, ARRAY_SIZE(sector_tbl));
> > > +}
> > > +
> > > +void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs)
> > > +{
> > > +	struct drm_color_ctm *ctm;
> > > +	u32 i;
> > > +
> > > +	if (!ctm_blob)
> > > +		return;
> > > +
> > > +	ctm = ctm_blob->data;
> > > +
> > > +	for (i = 0; i < KOMEDA_N_CTM_COEFFS; ++i) {
> > > +		/* Convert from S31.32 to Q3.12. */
> > > +		s64 v = ctm->matrix[i];
> > > +
> > > +		coeffs[i] = clamp_val(v, 1 - (1LL << 34), (1LL << 34) - 1) >> 20;
> > CTM matrix values are S31.32, i.e. sign-magnitude, so clamp_val won't
> > give you the right result for negative coeffs. See
> > malidp_crtc_atomic_check_ctm for the sign-mag -> 2's complement
> > conversion.
> 
> Thank you Mihail for pointing this out.
> 
> No matter our user or kernel all assume this s31.32 as 2's complement. 
> we need to correct them both.
> 
> > > +	}
> > > +}
> > > +
> > > +void komeda_color_duplicate_state(struct komeda_color_state *new,
> > > +				  struct komeda_color_state *old)
> > [bikeshed] not really a _duplicate_state if all it does is refcounts.
> > kmemdup here and return a pointer (with ERR_PTR on fail), or memcpy if
> > you want to keep the signature?
> 
> Yes, the dup mostly should return a new ptr from a old, the dup name here
> is not accurate.
> the reason is the color_state is not a separated structure but always
> embedded into layer_state, but I want to make all color_state operation
> into a func.
> Do you have any suggestion ?
> 
After looking at the follow-up patch, not really (at least not any
good ones). I did tag it with [bikeshed] after all, it's not that
big a deal.

> > > +{
> > > +	new->igamma = komeda_coeffs_get(old->igamma);
> > > +	new->fgamma = komeda_coeffs_get(old->fgamma);
> > > +}
> > > +
> > > +void komeda_color_cleanup_state(struct komeda_color_state *color_st)
> > > +{
> > > +	komeda_coeffs_put(color_st->igamma);
> > > +	komeda_coeffs_put(color_st->fgamma);
> > > +}
> > > +
> > > +int komeda_color_validate(struct komeda_color_manager *mgr,
> > > +			  struct komeda_color_state *st,
> > > +			  struct drm_property_blob *igamma_blob,
> > > +			  struct drm_property_blob *fgamma_blob)
> > > +{
> > > +	u32 coeffs[KOMEDA_N_GAMMA_COEFFS];
> > > +
> > > +	komeda_color_cleanup_state(st);
> > > +
> > > +	if (igamma_blob) {
> > > +		drm_lut_to_igamma_coeffs(igamma_blob, coeffs);
> > > +		st->igamma = komeda_coeffs_request(mgr->igamma_mgr, coeffs);
> > > +		if (!st->igamma) {
> > > +			DRM_DEBUG_ATOMIC("request igamma table failed.\n");
> > > +			return -EBUSY;
> > > +		}
> > > +	}
> > > +
> > > +	if (fgamma_blob) {
> > > +		drm_lut_to_fgamma_coeffs(fgamma_blob, coeffs);
> > > +		st->fgamma = komeda_coeffs_request(mgr->fgamma_mgr, coeffs);
> > > +		if (!st->fgamma) {
> > > +			DRM_DEBUG_ATOMIC("request fgamma table failed.\n");
> > > +			return -EBUSY;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > > index a2df218f58e7..41a96b3b540f 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > > @@ -4,14 +4,42 @@
> > >   * Author: James.Qian.Wang <james.qian.wang@arm.com>
> > >   *
> > >   */
> > > -
> > >  #ifndef _KOMEDA_COLOR_MGMT_H_
> > >  #define _KOMEDA_COLOR_MGMT_H_
> > >  
> > >  #include <drm/drm_color_mgmt.h>
> > > +#include "komeda_coeffs.h"
> > >  
> > >  #define KOMEDA_N_YUV2RGB_COEFFS		12
> > > +#define KOMEDA_N_RGB2YUV_COEFFS		12
> > > +#define KOMEDA_COLOR_PRECISION		12
> > > +#define KOMEDA_N_GAMMA_COEFFS		65
> > > +#define KOMEDA_COLOR_LUT_SIZE		BIT(KOMEDA_COLOR_PRECISION)
> 
> > I don't see how the number of LUT entries has anything to do with the
> > bit-precision of each entry.
> 
> Because our komeda color is 12-bit precison, and for 1 vs 1 loopup
> table, we need BIT(12) entries.
> 
> Thank you
> James
> 
But your maximum possible precision in HW is 4 times less. You only
really need one LUT entry per segment (its start) in order to
define it (and the slope, but you get the idea). I.e. at your current
4K-sized LUT table, the conversion to the internal representation only
_really_ needs to access offsets 0, 4, etc. and even less often as
it goes. If you make your table 1K entries instead, you save yourself
24KiB every time the (i)gamma changes.

TL;DR: you don't need 1:1 lookup, you need a lossless conversion from
the LUT to the HW format.

> > > +#define KOMEDA_N_CTM_COEFFS		9
> > > +
> > > +struct komeda_color_manager {
> > > +	struct komeda_coeffs_manager *igamma_mgr;
> > > +	struct komeda_coeffs_manager *fgamma_mgr;
> > > +	bool has_ctm;
> > > +};
> > > +
> > > +struct komeda_color_state {
> > > +	struct komeda_coeffs_table *igamma;
> > > +	struct komeda_coeffs_table *fgamma;
> > > +};
> > > +
> > > +void komeda_color_duplicate_state(struct komeda_color_state *new,
> > > +				  struct komeda_color_state *old);
> > > +void komeda_color_cleanup_state(struct komeda_color_state *color_st);
> > > +int komeda_color_validate(struct komeda_color_manager *mgr,
> > > +			  struct komeda_color_state *st,
> > > +			  struct drm_property_blob *igamma_blob,
> > > +			  struct drm_property_blob *fgamma_blob);
> > > +
> > > +void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
> > > +void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
> > > +void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs);
> > >  
> > >  const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range);
> > >  
> > > -#endif
> > > +#endif /*_KOMEDA_COLOR_MGMT_H_*/
> > > 
> > 
> > BR,
> > Mihail
> > 
> > 
> 

BR,
Mihail

Re: [PATCH v2 2/4] drm/komeda: Introduce komeda_color_manager/state

[james qian wang (Arm Technology China)]

On Wed, Aug 14, 2019 at 10:47:40AM +0000, Mihail Atanassov wrote:
> On Wednesday, 14 August 2019 08:52:18 BST james qian wang (Arm Technology China) wrote:
> > On Tue, Aug 13, 2019 at 09:51:08AM +0000, Mihail Atanassov wrote:
> > > Hi James,
> > > 
> > > On Tuesday, 13 August 2019 05:56:07 BST james qian wang (Arm Technology China) wrote:
> > > > Many komeda component support color management like layer and IPS, so
> > > > komeda_color_manager/state are introduced to manager gamma, csc and degamma
> > > > together for easily share it to multiple componpent.
> > > > 
> > > > And for komeda_color_manager which:
> > > > - convert drm 3d gamma lut to komeda specific gamma coeffs
> > > > - gamma table management and hide the HW difference for komeda-CORE
> > > > 
> > > > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> > > > ---
> > > >  .../arm/display/komeda/komeda_color_mgmt.c    | 126 ++++++++++++++++++
> > > >  .../arm/display/komeda/komeda_color_mgmt.h    |  32 ++++-
> > > >  2 files changed, 156 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > > > index 9d14a92dbb17..bf2388d641b9 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > > > @@ -4,7 +4,9 @@
> > > >   * Author: James.Qian.Wang <james.qian.wang@arm.com>
> > > >   *
> > > >   */
> > > > +#include <drm/drm_print.h>
> > > >  
> > > > +#include "malidp_utils.h"
> > > >  #include "komeda_color_mgmt.h"
> > > >  
> > > >  /* 10bit precision YUV2RGB matrix */
> > > > @@ -65,3 +67,127 @@ const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range)
> > > >  
> > > >  	return coeffs;
> > > >  }
> > > > +
> > > > +struct gamma_curve_sector {
> > > > +	u32 boundary_start;
> > > > +	u32 num_of_segments;
> > > > +	u32 segment_width;
> > > > +};
> > > > +
> > > > +struct gamma_curve_segment {
> > > > +	u32 start;
> > > > +	u32 end;
> > > > +};
> > > > +
> > > > +static struct gamma_curve_sector sector_tbl[] = {
> > > > +	{ 0,    4,  4   },
> Max LUT precision (see full response below) is determined by your
> smallest segment, which is 4.
> > > > +	{ 16,   4,  4   },
> > > > +	{ 32,   4,  8   },
> > > > +	{ 64,   4,  16  },
> > > > +	{ 128,  4,  32  },
> > > > +	{ 256,  4,  64  },
> > > > +	{ 512,  16, 32  },
> > > > +	{ 1024, 24, 128 },
> > > > +};
> > > > +
> > > > +static struct gamma_curve_sector igamma_sector_tbl[] = {
> > > > +	{0, 64, 64},
> > > > +};
> > > > +
> > > > +static void
> > > > +drm_lut_to_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs,
> > > > +		  struct gamma_curve_sector *sector_tbl, u32 num_sectors)
> > > > +{
> > > > +	struct drm_color_lut *lut;
> > > > +	u32 i, j, in, num = 0;
> > > > +
> > > > +	if (!lut_blob)
> > > > +		return;
> > > > +
> > > > +	lut = lut_blob->data;
> > > > +
> > > > +	for (i = 0; i < num_sectors; i++) {
> > > > +		for (j = 0; j < sector_tbl[i].num_of_segments; j++) {
> > > > +			in = sector_tbl[i].boundary_start +
> > > > +			     j * sector_tbl[i].segment_width;
> > > > +
> > > > +			coeffs[num++] = drm_color_lut_extract(lut[in].red,
> > > > +						KOMEDA_COLOR_PRECISION);
> > > > +		}
> > > > +	}
> > > > +
> > > > +	coeffs[num] = BIT(KOMEDA_COLOR_PRECISION);
> > > > +}
> > > > +
> > > > +void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
> > > > +{
> > > > +	drm_lut_to_coeffs(lut_blob, coeffs,
> > > > +			  igamma_sector_tbl, ARRAY_SIZE(igamma_sector_tbl));
> > > > +}
> > > > +
> > > > +void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
> > > > +{
> > > > +	drm_lut_to_coeffs(lut_blob, coeffs,
> > > > +			  sector_tbl, ARRAY_SIZE(sector_tbl));
> > > > +}
> > > > +
> > > > +void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs)
> > > > +{
> > > > +	struct drm_color_ctm *ctm;
> > > > +	u32 i;
> > > > +
> > > > +	if (!ctm_blob)
> > > > +		return;
> > > > +
> > > > +	ctm = ctm_blob->data;
> > > > +
> > > > +	for (i = 0; i < KOMEDA_N_CTM_COEFFS; ++i) {
> > > > +		/* Convert from S31.32 to Q3.12. */
> > > > +		s64 v = ctm->matrix[i];
> > > > +
> > > > +		coeffs[i] = clamp_val(v, 1 - (1LL << 34), (1LL << 34) - 1) >> 20;
> > > CTM matrix values are S31.32, i.e. sign-magnitude, so clamp_val won't
> > > give you the right result for negative coeffs. See
> > > malidp_crtc_atomic_check_ctm for the sign-mag -> 2's complement
> > > conversion.
> > 
> > Thank you Mihail for pointing this out.
> > 
> > No matter our user or kernel all assume this s31.32 as 2's complement. 
> > we need to correct them both.
> > 
> > > > +	}
> > > > +}
> > > > +
> > > > +void komeda_color_duplicate_state(struct komeda_color_state *new,
> > > > +				  struct komeda_color_state *old)
> > > [bikeshed] not really a _duplicate_state if all it does is refcounts.
> > > kmemdup here and return a pointer (with ERR_PTR on fail), or memcpy if
> > > you want to keep the signature?
> > 
> > Yes, the dup mostly should return a new ptr from a old, the dup name here
> > is not accurate.
> > the reason is the color_state is not a separated structure but always
> > embedded into layer_state, but I want to make all color_state operation
> > into a func.
> > Do you have any suggestion ?
> > 
> After looking at the follow-up patch, not really (at least not any
> good ones). I did tag it with [bikeshed] after all, it's not that
> big a deal.
> 
> > > > +{
> > > > +	new->igamma = komeda_coeffs_get(old->igamma);
> > > > +	new->fgamma = komeda_coeffs_get(old->fgamma);
> > > > +}
> > > > +
> > > > +void komeda_color_cleanup_state(struct komeda_color_state *color_st)
> > > > +{
> > > > +	komeda_coeffs_put(color_st->igamma);
> > > > +	komeda_coeffs_put(color_st->fgamma);
> > > > +}
> > > > +
> > > > +int komeda_color_validate(struct komeda_color_manager *mgr,
> > > > +			  struct komeda_color_state *st,
> > > > +			  struct drm_property_blob *igamma_blob,
> > > > +			  struct drm_property_blob *fgamma_blob)
> > > > +{
> > > > +	u32 coeffs[KOMEDA_N_GAMMA_COEFFS];
> > > > +
> > > > +	komeda_color_cleanup_state(st);
> > > > +
> > > > +	if (igamma_blob) {
> > > > +		drm_lut_to_igamma_coeffs(igamma_blob, coeffs);
> > > > +		st->igamma = komeda_coeffs_request(mgr->igamma_mgr, coeffs);
> > > > +		if (!st->igamma) {
> > > > +			DRM_DEBUG_ATOMIC("request igamma table failed.\n");
> > > > +			return -EBUSY;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (fgamma_blob) {
> > > > +		drm_lut_to_fgamma_coeffs(fgamma_blob, coeffs);
> > > > +		st->fgamma = komeda_coeffs_request(mgr->fgamma_mgr, coeffs);
> > > > +		if (!st->fgamma) {
> > > > +			DRM_DEBUG_ATOMIC("request fgamma table failed.\n");
> > > > +			return -EBUSY;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > > > index a2df218f58e7..41a96b3b540f 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > > > @@ -4,14 +4,42 @@
> > > >   * Author: James.Qian.Wang <james.qian.wang@arm.com>
> > > >   *
> > > >   */
> > > > -
> > > >  #ifndef _KOMEDA_COLOR_MGMT_H_
> > > >  #define _KOMEDA_COLOR_MGMT_H_
> > > >  
> > > >  #include <drm/drm_color_mgmt.h>
> > > > +#include "komeda_coeffs.h"
> > > >  
> > > >  #define KOMEDA_N_YUV2RGB_COEFFS		12
> > > > +#define KOMEDA_N_RGB2YUV_COEFFS		12
> > > > +#define KOMEDA_COLOR_PRECISION		12
> > > > +#define KOMEDA_N_GAMMA_COEFFS		65
> > > > +#define KOMEDA_COLOR_LUT_SIZE		BIT(KOMEDA_COLOR_PRECISION)
> > 
> > > I don't see how the number of LUT entries has anything to do with the
> > > bit-precision of each entry.
> > 
> > Because our komeda color is 12-bit precison, and for 1 vs 1 loopup
> > table, we need BIT(12) entries.
> > 
> > Thank you
> > James
> > 
> But your maximum possible precision in HW is 4 times less. You only
> really need one LUT entry per segment (its start) in order to
> define it (and the slope, but you get the idea). I.e. at your current
> 4K-sized LUT table, the conversion to the internal representation only
> _really_ needs to access offsets 0, 4, etc. and even less often as
> it goes. If you make your table 1K entries instead, you save yourself
> 24KiB every time the (i)gamma changes.
> 
> TL;DR: you don't need 1:1 lookup, you need a lossless conversion from
> the LUT to the HW format.

Hi Mihail:

Thank you for raising this topic.

I had consider this before, but I dropped it finally. because the
"compatibility".

Once we drop the 1vs1 lookup, but use a 1k table according the our D71
which made this lut d71 specific and leads:
- hard to compatable with third part user.
- hard to compatable with the future HW.

And all these color_mgmt properties are DRM standard, we also need to
follow DRM's way but not make it our HW only.

I don't see DRM directly say this table should be a 1 vs 1 lookup, but
we can got some hint from the doc of drm_crtc property "DEGAMMA_LUT”: 

"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])"

thanks
james.

> > > > +#define KOMEDA_N_CTM_COEFFS		9
> > > > +
> > > > +struct komeda_color_manager {
> > > > +	struct komeda_coeffs_manager *igamma_mgr;
> > > > +	struct komeda_coeffs_manager *fgamma_mgr;
> > > > +	bool has_ctm;
> > > > +};
> > > > +
> > > > +struct komeda_color_state {
> > > > +	struct komeda_coeffs_table *igamma;
> > > > +	struct komeda_coeffs_table *fgamma;
> > > > +};
> > > > +
> > > > +void komeda_color_duplicate_state(struct komeda_color_state *new,
> > > > +				  struct komeda_color_state *old);
> > > > +void komeda_color_cleanup_state(struct komeda_color_state *color_st);
> > > > +int komeda_color_validate(struct komeda_color_manager *mgr,
> > > > +			  struct komeda_color_state *st,
> > > > +			  struct drm_property_blob *igamma_blob,
> > > > +			  struct drm_property_blob *fgamma_blob);
> > > > +
> > > > +void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
> > > > +void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
> > > > +void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs);
> > > >  
> > > >  const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range);
> > > >  
> > > > -#endif
> > > > +#endif /*_KOMEDA_COLOR_MGMT_H_*/
> > > > 
> > > 
> > > BR,
> > > Mihail
> > > 
> > > 
> > 
> 
> BR,
> Mihail
> 
> 

Re: [PATCH v2 2/4] drm/komeda: Introduce komeda_color_manager/state

[Mihail Atanassov]

On Monday, 19 August 2019 09:50:55 BST james qian wang (Arm Technology China) wrote:
> On Wed, Aug 14, 2019 at 10:47:40AM +0000, Mihail Atanassov wrote:
> > On Wednesday, 14 August 2019 08:52:18 BST james qian wang (Arm Technology China) wrote:
> > > On Tue, Aug 13, 2019 at 09:51:08AM +0000, Mihail Atanassov wrote:
> > > > Hi James,
> > > > 
> > > > On Tuesday, 13 August 2019 05:56:07 BST james qian wang (Arm Technology China) wrote:
> > > > > Many komeda component support color management like layer and IPS, so
> > > > > komeda_color_manager/state are introduced to manager gamma, csc and degamma
> > > > > together for easily share it to multiple componpent.
> > > > > 
> > > > > And for komeda_color_manager which:
> > > > > - convert drm 3d gamma lut to komeda specific gamma coeffs
> > > > > - gamma table management and hide the HW difference for komeda-CORE
> > > > > 
> > > > > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> > > > > ---
> > > > >  .../arm/display/komeda/komeda_color_mgmt.c    | 126 ++++++++++++++++++
> > > > >  .../arm/display/komeda/komeda_color_mgmt.h    |  32 ++++-
> > > > >  2 files changed, 156 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > > > > index 9d14a92dbb17..bf2388d641b9 100644
> > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > > > > @@ -4,7 +4,9 @@
> > > > >   * Author: James.Qian.Wang <james.qian.wang@arm.com>
> > > > >   *
> > > > >   */
> > > > > +#include <drm/drm_print.h>
> > > > >  
> > > > > +#include "malidp_utils.h"
> > > > >  #include "komeda_color_mgmt.h"
> > > > >  
> > > > >  /* 10bit precision YUV2RGB matrix */
> > > > > @@ -65,3 +67,127 @@ const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range)
> > > > >  
> > > > >  	return coeffs;
> > > > >  }
> > > > > +
> > > > > +struct gamma_curve_sector {
> > > > > +	u32 boundary_start;
> > > > > +	u32 num_of_segments;
> > > > > +	u32 segment_width;
> > > > > +};
> > > > > +
> > > > > +struct gamma_curve_segment {
> > > > > +	u32 start;
> > > > > +	u32 end;
> > > > > +};
> > > > > +
> > > > > +static struct gamma_curve_sector sector_tbl[] = {
> > > > > +	{ 0,    4,  4   },
> > Max LUT precision (see full response below) is determined by your
> > smallest segment, which is 4.
> > > > > +	{ 16,   4,  4   },
> > > > > +	{ 32,   4,  8   },
> > > > > +	{ 64,   4,  16  },
> > > > > +	{ 128,  4,  32  },
> > > > > +	{ 256,  4,  64  },
> > > > > +	{ 512,  16, 32  },
> > > > > +	{ 1024, 24, 128 },
> > > > > +};
> > > > > +
> > > > > +static struct gamma_curve_sector igamma_sector_tbl[] = {
> > > > > +	{0, 64, 64},
> > > > > +};
> > > > > +
> > > > > +static void
> > > > > +drm_lut_to_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs,
> > > > > +		  struct gamma_curve_sector *sector_tbl, u32 num_sectors)
> > > > > +{
> > > > > +	struct drm_color_lut *lut;
> > > > > +	u32 i, j, in, num = 0;
> > > > > +
> > > > > +	if (!lut_blob)
> > > > > +		return;
You need to validate the LUTs size here, or you risk reading past its end below.
Consider drm_color_lut_size(const struct drm_property_blob*).
> > > > > +
> > > > > +	lut = lut_blob->data;
> > > > > +
> > > > > +	for (i = 0; i < num_sectors; i++) {
> > > > > +		for (j = 0; j < sector_tbl[i].num_of_segments; j++) {
> > > > > +			in = sector_tbl[i].boundary_start +
> > > > > +			     j * sector_tbl[i].segment_width;
> > > > > +
> > > > > +			coeffs[num++] = drm_color_lut_extract(lut[in].red,
> > > > > +						KOMEDA_COLOR_PRECISION);
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	coeffs[num] = BIT(KOMEDA_COLOR_PRECISION);
> > > > > +}
> > > > > +
> > > > > +void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
> > > > > +{
> > > > > +	drm_lut_to_coeffs(lut_blob, coeffs,
> > > > > +			  igamma_sector_tbl, ARRAY_SIZE(igamma_sector_tbl));
> > > > > +}
> > > > > +
> > > > > +void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
> > > > > +{
> > > > > +	drm_lut_to_coeffs(lut_blob, coeffs,
> > > > > +			  sector_tbl, ARRAY_SIZE(sector_tbl));
> > > > > +}
> > > > > +
> > > > > +void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs)
> > > > > +{
> > > > > +	struct drm_color_ctm *ctm;
> > > > > +	u32 i;
> > > > > +
> > > > > +	if (!ctm_blob)
> > > > > +		return;
> > > > > +
> > > > > +	ctm = ctm_blob->data;
> > > > > +
> > > > > +	for (i = 0; i < KOMEDA_N_CTM_COEFFS; ++i) {
> > > > > +		/* Convert from S31.32 to Q3.12. */
> > > > > +		s64 v = ctm->matrix[i];
> > > > > +
> > > > > +		coeffs[i] = clamp_val(v, 1 - (1LL << 34), (1LL << 34) - 1) >> 20;
> > > > CTM matrix values are S31.32, i.e. sign-magnitude, so clamp_val won't
> > > > give you the right result for negative coeffs. See
> > > > malidp_crtc_atomic_check_ctm for the sign-mag -> 2's complement
> > > > conversion.
> > > 
> > > Thank you Mihail for pointing this out.
> > > 
> > > No matter our user or kernel all assume this s31.32 as 2's complement. 
> > > we need to correct them both.
> > > 
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +void komeda_color_duplicate_state(struct komeda_color_state *new,
> > > > > +				  struct komeda_color_state *old)
> > > > [bikeshed] not really a _duplicate_state if all it does is refcounts.
> > > > kmemdup here and return a pointer (with ERR_PTR on fail), or memcpy if
> > > > you want to keep the signature?
> > > 
> > > Yes, the dup mostly should return a new ptr from a old, the dup name here
> > > is not accurate.
> > > the reason is the color_state is not a separated structure but always
> > > embedded into layer_state, but I want to make all color_state operation
> > > into a func.
> > > Do you have any suggestion ?
> > > 
> > After looking at the follow-up patch, not really (at least not any
> > good ones). I did tag it with [bikeshed] after all, it's not that
> > big a deal.
> > 
> > > > > +{
> > > > > +	new->igamma = komeda_coeffs_get(old->igamma);
> > > > > +	new->fgamma = komeda_coeffs_get(old->fgamma);
> > > > > +}
> > > > > +
> > > > > +void komeda_color_cleanup_state(struct komeda_color_state *color_st)
> > > > > +{
> > > > > +	komeda_coeffs_put(color_st->igamma);
> > > > > +	komeda_coeffs_put(color_st->fgamma);
> > > > > +}
> > > > > +
> > > > > +int komeda_color_validate(struct komeda_color_manager *mgr,
> > > > > +			  struct komeda_color_state *st,
> > > > > +			  struct drm_property_blob *igamma_blob,
> > > > > +			  struct drm_property_blob *fgamma_blob)
> > > > > +{
> > > > > +	u32 coeffs[KOMEDA_N_GAMMA_COEFFS];
> > > > > +
> > > > > +	komeda_color_cleanup_state(st);
> > > > > +
> > > > > +	if (igamma_blob) {
> > > > > +		drm_lut_to_igamma_coeffs(igamma_blob, coeffs);
> > > > > +		st->igamma = komeda_coeffs_request(mgr->igamma_mgr, coeffs);
> > > > > +		if (!st->igamma) {
> > > > > +			DRM_DEBUG_ATOMIC("request igamma table failed.\n");
> > > > > +			return -EBUSY;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	if (fgamma_blob) {
> > > > > +		drm_lut_to_fgamma_coeffs(fgamma_blob, coeffs);
> > > > > +		st->fgamma = komeda_coeffs_request(mgr->fgamma_mgr, coeffs);
> > > > > +		if (!st->fgamma) {
> > > > > +			DRM_DEBUG_ATOMIC("request fgamma table failed.\n");
> > > > > +			return -EBUSY;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > > > > index a2df218f58e7..41a96b3b540f 100644
> > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > > > > @@ -4,14 +4,42 @@
> > > > >   * Author: James.Qian.Wang <james.qian.wang@arm.com>
> > > > >   *
> > > > >   */
> > > > > -
> > > > >  #ifndef _KOMEDA_COLOR_MGMT_H_
> > > > >  #define _KOMEDA_COLOR_MGMT_H_
> > > > >  
> > > > >  #include <drm/drm_color_mgmt.h>
> > > > > +#include "komeda_coeffs.h"
> > > > >  
> > > > >  #define KOMEDA_N_YUV2RGB_COEFFS		12
> > > > > +#define KOMEDA_N_RGB2YUV_COEFFS		12
> > > > > +#define KOMEDA_COLOR_PRECISION		12
> > > > > +#define KOMEDA_N_GAMMA_COEFFS		65
> > > > > +#define KOMEDA_COLOR_LUT_SIZE		BIT(KOMEDA_COLOR_PRECISION)
> > > 
> > > > I don't see how the number of LUT entries has anything to do with the
> > > > bit-precision of each entry.
> > > 
> > > Because our komeda color is 12-bit precison, and for 1 vs 1 loopup
> > > table, we need BIT(12) entries.
> > > 
> > > Thank you
> > > James
> > > 
> > But your maximum possible precision in HW is 4 times less. You only
> > really need one LUT entry per segment (its start) in order to
> > define it (and the slope, but you get the idea). I.e. at your current
> > 4K-sized LUT table, the conversion to the internal representation only
> > _really_ needs to access offsets 0, 4, etc. and even less often as
> > it goes. If you make your table 1K entries instead, you save yourself
> > 24KiB every time the (i)gamma changes.
> > 
> > TL;DR: you don't need 1:1 lookup, you need a lossless conversion from
> > the LUT to the HW format.
> 
> Hi Mihail:
> 
> Thank you for raising this topic.
> 
> I had consider this before, but I dropped it finally. because the
> "compatibility".
> 
> Once we drop the 1vs1 lookup, but use a 1k table according the our D71
> which made this lut d71 specific and leads:
> - hard to compatable with third part user.
> - hard to compatable with the future HW.
> 
> And all these color_mgmt properties are DRM standard, we also need to
> follow DRM's way but not make it our HW only.
> 
> I don't see DRM directly say this table should be a 1 vs 1 lookup, but
> we can got some hint from the doc of drm_crtc property "DEGAMMA_LUT”: 
> 
> "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])"
> 
Won't help us if future hardware suddenly needs an 8K LUT ;).
Regardless, fixing all of that is easy with a u8 log2_lut_size in
komeda_dev when the time comes, which is why I raised this now.
There's no point planning for what we don't know how it'll turn out
given that the current solution may become inadequate either way.

Now, all that said, the GAMMA_LUT_SIZE prop has an interesting sentence
in its description:
"""
 *         If drivers support multiple LUT sizes then they should publish the
 *	largest size, and sub-sample smaller sized LUTs (e.g. for split-gamma
 *	modes) appropriately.
"""

I'll back down on this and say exposing 4K LUTs is fine, and if we
need to speed up redoing the LUTs, userland can send a smaller LUT
(or maybe even a pre-baked coeffs table if we're allowed).

Looking at i915's use of the props, the code accepts multiple different
sizes and interpretations of what is in the LUT (e.g. a 256 pallette
for C8) depending the HW, with >1 accepted sizes for a
particular HW. Given that there's precedent and we can enhance this
code later if needed (e.g. to save 24KiB per table as per my original
concern), please fix the LUT size check above and you have my:

Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>

> thanks
> james.
> 

> > > > > +#define KOMEDA_N_CTM_COEFFS		9
> > > > > +
> > > > > +struct komeda_color_manager {
> > > > > +	struct komeda_coeffs_manager *igamma_mgr;
> > > > > +	struct komeda_coeffs_manager *fgamma_mgr;
> > > > > +	bool has_ctm;
> > > > > +};
> > > > > +
> > > > > +struct komeda_color_state {
> > > > > +	struct komeda_coeffs_table *igamma;
> > > > > +	struct komeda_coeffs_table *fgamma;
> > > > > +};
> > > > > +
> > > > > +void komeda_color_duplicate_state(struct komeda_color_state *new,
> > > > > +				  struct komeda_color_state *old);
> > > > > +void komeda_color_cleanup_state(struct komeda_color_state *color_st);
> > > > > +int komeda_color_validate(struct komeda_color_manager *mgr,
> > > > > +			  struct komeda_color_state *st,
> > > > > +			  struct drm_property_blob *igamma_blob,
> > > > > +			  struct drm_property_blob *fgamma_blob);
> > > > > +
> > > > > +void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
> > > > > +void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
> > > > > +void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs);
> > > > >  
> > > > >  const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range);
> > > > >  
> > > > > -#endif
> > > > > +#endif /*_KOMEDA_COLOR_MGMT_H_*/
> > > > > 
> > > > 
> > > > BR,
> > > > Mihail
> > > > 
> > > > 
> > > 
> > 
> > BR,
> > Mihail
> > 
> > 
> 

BR,
Mihail

[PATCH v2 3/4] drm: Increase DRM_OBJECT_MAX_PROPERTY to 32

[james qian wang (Arm Technology China)]

DRM_OBJECT_MAX_PROPERTY number 24 is not enough for komeda usage, increase
it to 32 to fit komeda's requirement.

Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
---
 include/drm/drm_mode_object.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
index c34a3e8030e1..fd7666048197 100644
--- a/include/drm/drm_mode_object.h
+++ b/include/drm/drm_mode_object.h
@@ -60,7 +60,7 @@ struct drm_mode_object {
 	void (*free_cb)(struct kref *kref);
 };
 
-#define DRM_OBJECT_MAX_PROPERTY 24
+#define DRM_OBJECT_MAX_PROPERTY 32
 /**
  * struct drm_object_properties - property tracking for &drm_mode_object
  */
-- 
2.20.1

[PATCH v2 4/4] drm/komeda: Enable Layer color management for komeda

[james qian wang (Arm Technology China)]

- D71 has 3 global layer gamma table which can be used for all layers as
  gamma lookup table, no matter inverse or forward.
- Add komeda_color_manager/state to layer/layer_state for describe the
  color caps for the specific layer which will be initialized in
  d71_layer_init, and for D71 only layer with L_INFO_CM (color mgmt) bit
  can support forward gamma, and CSC.
- Update komeda-CORE code to validate and convert the plane color state to
  komeda_color_state.
- Enable plane color mgmt according to layer komeda_color_manager

This patch depends on:
- https://patchwork.freedesktop.org/series/30876/

Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
---
 .../arm/display/komeda/d71/d71_component.c    | 64 +++++++++++++++++++
 .../gpu/drm/arm/display/komeda/d71/d71_dev.c  |  5 +-
 .../gpu/drm/arm/display/komeda/d71/d71_dev.h  |  2 +
 .../drm/arm/display/komeda/komeda_pipeline.h  |  4 +-
 .../display/komeda/komeda_pipeline_state.c    | 53 ++++++++++++++-
 .../gpu/drm/arm/display/komeda/komeda_plane.c | 12 ++++
 .../arm/display/komeda/komeda_private_obj.c   |  4 ++
 7 files changed, 139 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
index 55a8cc94808a..c9c40a36e4d2 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
@@ -189,6 +189,46 @@ static void d71_layer_update_fb(struct komeda_component *c,
 	malidp_write32(reg, LAYER_FMT, kfb->format_caps->hw_id);
 }
 
+static u32 d71_layer_update_color(struct drm_plane_state *st,
+				  u32 __iomem *reg,
+				  struct komeda_color_state *color_st,
+				  u32 *mask)
+{
+	struct komeda_coeffs_table *igamma = color_st->igamma;
+	struct komeda_coeffs_table *fgamma = color_st->fgamma;
+	u32 ctrl = 0, v = 0;
+
+	if (!st->color_mgmt_changed)
+		return 0;
+
+	*mask |= L_IT | L_R2R | L_FT;
+
+	if (igamma) {
+		komeda_coeffs_update(igamma);
+		ctrl |= L_IT;
+		v = L_ITSEL(igamma->hw_id);
+	}
+
+	if (st->ctm) {
+		u32 ctm_coeffs[KOMEDA_N_CTM_COEFFS];
+
+		drm_ctm_to_coeffs(st->ctm, ctm_coeffs);
+		malidp_write_group(reg, LAYER_RGB_RGB_COEFF0,
+				   ARRAY_SIZE(ctm_coeffs),
+				   ctm_coeffs);
+		ctrl |= L_R2R; /* enable RGB2RGB conversion */
+	}
+
+	if (fgamma) {
+		komeda_coeffs_update(fgamma);
+		ctrl |= L_FT;
+		v |= L_FTSEL(fgamma->hw_id);
+	}
+
+	malidp_write32(reg, LAYER_LT_COEFFTAB, v);
+	return ctrl;
+}
+
 static void d71_layer_disable(struct komeda_component *c)
 {
 	malidp_write32_mask(c->reg, BLK_CONTROL, L_EN, 0);
@@ -261,6 +301,8 @@ static void d71_layer_update(struct komeda_component *c,
 
 	malidp_write32(reg, BLK_IN_SIZE, HV_SIZE(st->hsize, st->vsize));
 
+	ctrl |= d71_layer_update_color(plane_st, reg, &st->color_st, &ctrl_mask);
+
 	if (kfb->is_va)
 		ctrl |= L_TBU_EN;
 	malidp_write32_mask(reg, BLK_CONTROL, ctrl_mask, ctrl);
@@ -365,6 +407,12 @@ static int d71_layer_init(struct d71_dev *d71,
 	else
 		layer->layer_type = KOMEDA_FMT_SIMPLE_LAYER;
 
+	layer->color_mgr.igamma_mgr = d71->glb_lt_mgr;
+	if (layer_info & L_INFO_CM) {
+		layer->color_mgr.has_ctm = true;
+		layer->color_mgr.fgamma_mgr = d71->glb_lt_mgr;
+	}
+
 	set_range(&layer->hsize_in, 4, d71->max_line_size);
 	set_range(&layer->vsize_in, 4, d71->max_vsize);
 
@@ -1140,6 +1188,21 @@ static int d71_timing_ctrlr_init(struct d71_dev *d71,
 	return 0;
 }
 
+static void d71_gamma_update(struct komeda_coeffs_table *table)
+{
+	malidp_write_group(table->reg, GLB_LT_COEFF_DATA,
+			   GLB_LT_COEFF_NUM, table->coeffs);
+}
+
+static int d71_lt_coeff_init(struct d71_dev *d71,
+			     struct block_header *blk, u32 __iomem *reg)
+{
+	struct komeda_coeffs_manager *mgr = d71->glb_lt_mgr;
+	int hw_id = BLOCK_INFO_TYPE_ID(blk->block_info);
+
+	return komeda_coeffs_add(mgr, hw_id, reg, d71_gamma_update);
+}
+
 int d71_probe_block(struct d71_dev *d71,
 		    struct block_header *blk, u32 __iomem *reg)
 {
@@ -1202,6 +1265,7 @@ int d71_probe_block(struct d71_dev *d71,
 		break;
 
 	case D71_BLK_TYPE_GLB_LT_COEFF:
+		err = d71_lt_coeff_init(d71, blk, reg);
 		break;
 
 	case D71_BLK_TYPE_GLB_SCL_COEFF:
diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
index d567ab7ed314..edd5d7c7f2a2 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
@@ -341,7 +341,7 @@ static int d71_enum_resources(struct komeda_dev *mdev)
 	struct komeda_pipeline *pipe;
 	struct block_header blk;
 	u32 __iomem *blk_base;
-	u32 i, value, offset;
+	u32 i, value, offset, coeffs_size;
 	int err;
 
 	d71 = devm_kzalloc(mdev->dev, sizeof(*d71), GFP_KERNEL);
@@ -398,6 +398,9 @@ static int d71_enum_resources(struct komeda_dev *mdev)
 		d71->pipes[i] = to_d71_pipeline(pipe);
 	}
 
+	coeffs_size = GLB_LT_COEFF_NUM * sizeof(u32);
+	d71->glb_lt_mgr = komeda_coeffs_create_manager(coeffs_size);
+
 	/* loop the register blks and probe */
 	i = 2; /* exclude GCU and PERIPH */
 	offset = D71_BLOCK_SIZE; /* skip GCU */
diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.h
index 84f1878b647d..009c164a1584 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.h
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.h
@@ -39,6 +39,8 @@ struct d71_dev {
 	u32 __iomem	*periph_addr;
 
 	struct d71_pipeline *pipes[D71_MAX_PIPELINE];
+	 /* global layer transform coefficient table manager */
+	struct komeda_coeffs_manager *glb_lt_mgr;
 };
 
 #define to_d71_pipeline(x)	container_of(x, struct d71_pipeline, base)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
index a7a84e66549d..4104c81e5032 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
@@ -10,6 +10,7 @@
 #include <linux/types.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
+#include "komeda_color_mgmt.h"
 #include "malidp_utils.h"
 
 #define KOMEDA_MAX_PIPELINES		2
@@ -226,6 +227,7 @@ struct komeda_layer {
 	struct komeda_component base;
 	/* accepted h/v input range before rotation */
 	struct malidp_range hsize_in, vsize_in;
+	struct komeda_color_manager color_mgr;
 	u32 layer_type; /* RICH, SIMPLE or WB */
 	u32 supported_rots;
 	/* komeda supports layer split which splits a whole image to two parts
@@ -238,7 +240,7 @@ struct komeda_layer {
 
 struct komeda_layer_state {
 	struct komeda_component_state base;
-	/* layer specific configuration state */
+	struct komeda_color_state color_st;
 	u16 hsize, vsize;
 	u32 rot;
 	u16 afbc_crop_l;
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index ea26bc9c2d00..803715c1128e 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -95,6 +95,21 @@ komeda_pipeline_get_state_and_set_crtc(struct komeda_pipeline *pipe,
 	return st;
 }
 
+static bool komeda_component_is_new_active(struct komeda_component *c,
+					   struct drm_atomic_state *state)
+{
+	struct komeda_pipeline_state *ppl_old_st;
+
+	ppl_old_st = komeda_pipeline_get_old_state(c->pipeline, state);
+	if (IS_ERR(ppl_old_st))
+		return true;
+
+	if (has_bit(c->id, ppl_old_st->active_comps))
+		return false;
+
+	return true;
+}
+
 static struct komeda_component_state *
 komeda_component_get_state(struct komeda_component *c,
 			   struct drm_atomic_state *state)
@@ -279,6 +294,29 @@ komeda_rotate_data_flow(struct komeda_data_flow_cfg *dflow, u32 rot)
 	}
 }
 
+static int
+komeda_validate_plane_color(struct komeda_component *c,
+			    struct komeda_color_manager *color_mgr,
+			    struct komeda_color_state *color_st,
+			    struct drm_plane_state *plane_st)
+{
+	int err;
+
+	if (komeda_component_is_new_active(c, plane_st->state))
+		plane_st->color_mgmt_changed = true;
+
+	if (!plane_st->color_mgmt_changed)
+		return 0;
+
+	err = komeda_color_validate(color_mgr, color_st,
+				    plane_st->degamma_lut,
+				    plane_st->gamma_lut);
+	if (err)
+		DRM_DEBUG_ATOMIC("%s validate color failed.\n", c->name);
+
+	return err;
+}
+
 static int
 komeda_layer_check_cfg(struct komeda_layer *layer,
 		       struct komeda_fb *kfb,
@@ -362,6 +400,11 @@ komeda_layer_validate(struct komeda_layer *layer,
 		st->addr[i] = komeda_fb_get_pixel_addr(kfb, dflow->in_x,
 						       dflow->in_y, i);
 
+	err = komeda_validate_plane_color(&layer->base, &layer->color_mgr,
+					  &st->color_st, plane_st);
+	if (err)
+		return err;
+
 	err = komeda_component_validate_private(&layer->base, c_st);
 	if (err)
 		return err;
@@ -1177,7 +1220,7 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
 {
 	struct drm_atomic_state *drm_st = new->obj.state;
 	struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
-	struct komeda_component_state *c_st;
+	struct komeda_component_state *st;
 	struct komeda_component *c;
 	u32 disabling_comps, id;
 
@@ -1188,9 +1231,13 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
 	/* unbound all disabling component */
 	dp_for_each_set_bit(id, disabling_comps) {
 		c = komeda_pipeline_get_component(pipe, id);
-		c_st = komeda_component_get_state_and_set_user(c,
+		st = komeda_component_get_state_and_set_user(c,
 				drm_st, NULL, new->crtc);
-		WARN_ON(IS_ERR(c_st));
+
+		WARN_ON(IS_ERR(st));
+
+		if (has_bit(id, KOMEDA_PIPELINE_LAYERS))
+			komeda_color_cleanup_state(&to_layer_st(st)->color_st);
 	}
 }
 
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
index 98e915e325dd..69fa1dea41c9 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
@@ -11,6 +11,7 @@
 #include "komeda_dev.h"
 #include "komeda_kms.h"
 #include "komeda_framebuffer.h"
+#include "komeda_color_mgmt.h"
 
 static int
 komeda_plane_init_data_flow(struct drm_plane_state *st,
@@ -250,6 +251,7 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
 {
 	struct komeda_dev *mdev = kms->base.dev_private;
 	struct komeda_component *c = &layer->base;
+	struct komeda_color_manager *color_mgr;
 	struct komeda_plane *kplane;
 	struct drm_plane *plane;
 	u32 *formats, n_formats = 0;
@@ -306,6 +308,16 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
 	if (err)
 		goto cleanup;
 
+	err = drm_plane_color_create_prop(plane->dev, plane);
+	if (err)
+		goto cleanup;
+
+	color_mgr = &layer->color_mgr;
+	drm_plane_enable_color_mgmt(plane,
+			color_mgr->igamma_mgr ? KOMEDA_COLOR_LUT_SIZE : 0,
+			color_mgr->has_ctm,
+			color_mgr->fgamma_mgr ? KOMEDA_COLOR_LUT_SIZE : 0);
+
 	err = drm_plane_create_zpos_property(plane, layer->base.id, 0, 8);
 	if (err)
 		goto cleanup;
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c b/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c
index 914400c4af73..4c474663f554 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c
@@ -19,12 +19,15 @@ komeda_component_state_reset(struct komeda_component_state *st)
 static struct drm_private_state *
 komeda_layer_atomic_duplicate_state(struct drm_private_obj *obj)
 {
+	struct komeda_layer_state *old = to_layer_st(priv_to_comp_st(obj->state));
 	struct komeda_layer_state *st;
 
 	st = kmemdup(obj->state, sizeof(*st), GFP_KERNEL);
 	if (!st)
 		return NULL;
 
+	komeda_color_duplicate_state(&st->color_st, &old->color_st);
+
 	komeda_component_state_reset(&st->base);
 	__drm_atomic_helper_private_obj_duplicate_state(obj, &st->base.obj);
 
@@ -37,6 +40,7 @@ komeda_layer_atomic_destroy_state(struct drm_private_obj *obj,
 {
 	struct komeda_layer_state *st = to_layer_st(priv_to_comp_st(state));
 
+	komeda_color_cleanup_state(&st->color_st);
 	kfree(st);
 }
 
-- 
2.20.1

Re: [PATCH v2 4/4] drm/komeda: Enable Layer color management for komeda

[Mihail Atanassov]

Hi James,

On Tuesday, 13 August 2019 05:56:19 BST james qian wang (Arm Technology China) wrote:
> - D71 has 3 global layer gamma table which can be used for all layers as
>   gamma lookup table, no matter inverse or forward.
> - Add komeda_color_manager/state to layer/layer_state for describe the
>   color caps for the specific layer which will be initialized in
>   d71_layer_init, and for D71 only layer with L_INFO_CM (color mgmt) bit
>   can support forward gamma, and CSC.
> - Update komeda-CORE code to validate and convert the plane color state to
>   komeda_color_state.
> - Enable plane color mgmt according to layer komeda_color_manager
> 
> This patch depends on:
> - https://patchwork.freedesktop.org/series/30876/
> 
> Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> ---
>  .../arm/display/komeda/d71/d71_component.c    | 64 +++++++++++++++++++
>  .../gpu/drm/arm/display/komeda/d71/d71_dev.c  |  5 +-
>  .../gpu/drm/arm/display/komeda/d71/d71_dev.h  |  2 +
>  .../drm/arm/display/komeda/komeda_pipeline.h  |  4 +-
>  .../display/komeda/komeda_pipeline_state.c    | 53 ++++++++++++++-
>  .../gpu/drm/arm/display/komeda/komeda_plane.c | 12 ++++
>  .../arm/display/komeda/komeda_private_obj.c   |  4 ++
>  7 files changed, 139 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> index 55a8cc94808a..c9c40a36e4d2 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> @@ -189,6 +189,46 @@ static void d71_layer_update_fb(struct komeda_component *c,
>  	malidp_write32(reg, LAYER_FMT, kfb->format_caps->hw_id);
>  }
>  
> +static u32 d71_layer_update_color(struct drm_plane_state *st,
> +				  u32 __iomem *reg,
> +				  struct komeda_color_state *color_st,
> +				  u32 *mask)
> +{
> +	struct komeda_coeffs_table *igamma = color_st->igamma;
> +	struct komeda_coeffs_table *fgamma = color_st->fgamma;
> +	u32 ctrl = 0, v = 0;
> +
> +	if (!st->color_mgmt_changed)
> +		return 0;
> +
> +	*mask |= L_IT | L_R2R | L_FT;
> +
> +	if (igamma) {
> +		komeda_coeffs_update(igamma);
> +		ctrl |= L_IT;
> +		v = L_ITSEL(igamma->hw_id);
> +	}
> +
> +	if (st->ctm) {
> +		u32 ctm_coeffs[KOMEDA_N_CTM_COEFFS];
> +
> +		drm_ctm_to_coeffs(st->ctm, ctm_coeffs);
> +		malidp_write_group(reg, LAYER_RGB_RGB_COEFF0,
> +				   ARRAY_SIZE(ctm_coeffs),
> +				   ctm_coeffs);
> +		ctrl |= L_R2R; /* enable RGB2RGB conversion */
> +	}
> +
> +	if (fgamma) {
> +		komeda_coeffs_update(fgamma);
> +		ctrl |= L_FT;
> +		v |= L_FTSEL(fgamma->hw_id);
> +	}
> +
> +	malidp_write32(reg, LAYER_LT_COEFFTAB, v);
> +	return ctrl;
> +}
> +
>  static void d71_layer_disable(struct komeda_component *c)
>  {
>  	malidp_write32_mask(c->reg, BLK_CONTROL, L_EN, 0);
> @@ -261,6 +301,8 @@ static void d71_layer_update(struct komeda_component *c,
>  
>  	malidp_write32(reg, BLK_IN_SIZE, HV_SIZE(st->hsize, st->vsize));
>  
> +	ctrl |= d71_layer_update_color(plane_st, reg, &st->color_st, &ctrl_mask);
> +
>  	if (kfb->is_va)
>  		ctrl |= L_TBU_EN;
>  	malidp_write32_mask(reg, BLK_CONTROL, ctrl_mask, ctrl);
> @@ -365,6 +407,12 @@ static int d71_layer_init(struct d71_dev *d71,
>  	else
>  		layer->layer_type = KOMEDA_FMT_SIMPLE_LAYER;
>  
> +	layer->color_mgr.igamma_mgr = d71->glb_lt_mgr;
> +	if (layer_info & L_INFO_CM) {
> +		layer->color_mgr.has_ctm = true;
> +		layer->color_mgr.fgamma_mgr = d71->glb_lt_mgr;
> +	}
> +
>  	set_range(&layer->hsize_in, 4, d71->max_line_size);
>  	set_range(&layer->vsize_in, 4, d71->max_vsize);
>  
> @@ -1140,6 +1188,21 @@ static int d71_timing_ctrlr_init(struct d71_dev *d71,
>  	return 0;
>  }
>  
> +static void d71_gamma_update(struct komeda_coeffs_table *table)
> +{
> +	malidp_write_group(table->reg, GLB_LT_COEFF_DATA,
> +			   GLB_LT_COEFF_NUM, table->coeffs);
> +}
> +
> +static int d71_lt_coeff_init(struct d71_dev *d71,
> +			     struct block_header *blk, u32 __iomem *reg)
> +{
> +	struct komeda_coeffs_manager *mgr = d71->glb_lt_mgr;
> +	int hw_id = BLOCK_INFO_TYPE_ID(blk->block_info);
> +
> +	return komeda_coeffs_add(mgr, hw_id, reg, d71_gamma_update);
> +}
> +
>  int d71_probe_block(struct d71_dev *d71,
>  		    struct block_header *blk, u32 __iomem *reg)
>  {
> @@ -1202,6 +1265,7 @@ int d71_probe_block(struct d71_dev *d71,
>  		break;
>  
>  	case D71_BLK_TYPE_GLB_LT_COEFF:
> +		err = d71_lt_coeff_init(d71, blk, reg);
>  		break;
>  
>  	case D71_BLK_TYPE_GLB_SCL_COEFF:
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> index d567ab7ed314..edd5d7c7f2a2 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> @@ -341,7 +341,7 @@ static int d71_enum_resources(struct komeda_dev *mdev)
>  	struct komeda_pipeline *pipe;
>  	struct block_header blk;
>  	u32 __iomem *blk_base;
> -	u32 i, value, offset;
> +	u32 i, value, offset, coeffs_size;
>  	int err;
>  
>  	d71 = devm_kzalloc(mdev->dev, sizeof(*d71), GFP_KERNEL);
> @@ -398,6 +398,9 @@ static int d71_enum_resources(struct komeda_dev *mdev)
>  		d71->pipes[i] = to_d71_pipeline(pipe);
>  	}
>  
> +	coeffs_size = GLB_LT_COEFF_NUM * sizeof(u32);
> +	d71->glb_lt_mgr = komeda_coeffs_create_manager(coeffs_size);
> +
>  	/* loop the register blks and probe */
>  	i = 2; /* exclude GCU and PERIPH */
>  	offset = D71_BLOCK_SIZE; /* skip GCU */
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.h
> index 84f1878b647d..009c164a1584 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.h
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.h
> @@ -39,6 +39,8 @@ struct d71_dev {
>  	u32 __iomem	*periph_addr;
>  
>  	struct d71_pipeline *pipes[D71_MAX_PIPELINE];
> +	 /* global layer transform coefficient table manager */
> +	struct komeda_coeffs_manager *glb_lt_mgr;
>  };
>  
>  #define to_d71_pipeline(x)	container_of(x, struct d71_pipeline, base)
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> index a7a84e66549d..4104c81e5032 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> @@ -10,6 +10,7 @@
>  #include <linux/types.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> +#include "komeda_color_mgmt.h"
>  #include "malidp_utils.h"
>  
>  #define KOMEDA_MAX_PIPELINES		2
> @@ -226,6 +227,7 @@ struct komeda_layer {
>  	struct komeda_component base;
>  	/* accepted h/v input range before rotation */
>  	struct malidp_range hsize_in, vsize_in;
> +	struct komeda_color_manager color_mgr;
>  	u32 layer_type; /* RICH, SIMPLE or WB */
>  	u32 supported_rots;
>  	/* komeda supports layer split which splits a whole image to two parts
> @@ -238,7 +240,7 @@ struct komeda_layer {
>  
>  struct komeda_layer_state {
>  	struct komeda_component_state base;
> -	/* layer specific configuration state */
> +	struct komeda_color_state color_st;
>  	u16 hsize, vsize;
>  	u32 rot;
>  	u16 afbc_crop_l;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index ea26bc9c2d00..803715c1128e 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -95,6 +95,21 @@ komeda_pipeline_get_state_and_set_crtc(struct komeda_pipeline *pipe,
>  	return st;
>  }
>  
> +static bool komeda_component_is_new_active(struct komeda_component *c,
> +					   struct drm_atomic_state *state)
> +{
> +	struct komeda_pipeline_state *ppl_old_st;
> +
> +	ppl_old_st = komeda_pipeline_get_old_state(c->pipeline, state);
> +	if (IS_ERR(ppl_old_st))
> +		return true;
> +
> +	if (has_bit(c->id, ppl_old_st->active_comps))
> +		return false;
> +
> +	return true;
> +}
> +
>  static struct komeda_component_state *
>  komeda_component_get_state(struct komeda_component *c,
>  			   struct drm_atomic_state *state)
> @@ -279,6 +294,29 @@ komeda_rotate_data_flow(struct komeda_data_flow_cfg *dflow, u32 rot)
>  	}
>  }
>  
> +static int
> +komeda_validate_plane_color(struct komeda_component *c,
> +			    struct komeda_color_manager *color_mgr,
> +			    struct komeda_color_state *color_st,
> +			    struct drm_plane_state *plane_st)
> +{
> +	int err;
> +
> +	if (komeda_component_is_new_active(c, plane_st->state))
> +		plane_st->color_mgmt_changed = true;
> +
> +	if (!plane_st->color_mgmt_changed)
> +		return 0;
> +
> +	err = komeda_color_validate(color_mgr, color_st,
> +				    plane_st->degamma_lut,
> +				    plane_st->gamma_lut);
> +	if (err)
> +		DRM_DEBUG_ATOMIC("%s validate color failed.\n", c->name);
> +
> +	return err;
> +}
> +
>  static int
>  komeda_layer_check_cfg(struct komeda_layer *layer,
>  		       struct komeda_fb *kfb,
> @@ -362,6 +400,11 @@ komeda_layer_validate(struct komeda_layer *layer,
>  		st->addr[i] = komeda_fb_get_pixel_addr(kfb, dflow->in_x,
>  						       dflow->in_y, i);
>  
> +	err = komeda_validate_plane_color(&layer->base, &layer->color_mgr,
> +					  &st->color_st, plane_st);
> +	if (err)
> +		return err;
> +
>  	err = komeda_component_validate_private(&layer->base, c_st);
>  	if (err)
>  		return err;
> @@ -1177,7 +1220,7 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
>  {
>  	struct drm_atomic_state *drm_st = new->obj.state;
>  	struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
> -	struct komeda_component_state *c_st;
> +	struct komeda_component_state *st;
>  	struct komeda_component *c;
>  	u32 disabling_comps, id;
>  
> @@ -1188,9 +1231,13 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
>  	/* unbound all disabling component */
>  	dp_for_each_set_bit(id, disabling_comps) {
>  		c = komeda_pipeline_get_component(pipe, id);
> -		c_st = komeda_component_get_state_and_set_user(c,
> +		st = komeda_component_get_state_and_set_user(c,
>  				drm_st, NULL, new->crtc);
> -		WARN_ON(IS_ERR(c_st));
> +
> +		WARN_ON(IS_ERR(st));
I think this needs to be:
if (WARN_ON(IS_ERR(st)))
        continue;
...because...
> +
> +		if (has_bit(id, KOMEDA_PIPELINE_LAYERS))
> +			komeda_color_cleanup_state(&to_layer_st(st)->color_st);
... this may deref an invalid `st' otherwise.
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> index 98e915e325dd..69fa1dea41c9 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> @@ -11,6 +11,7 @@
>  #include "komeda_dev.h"
>  #include "komeda_kms.h"
>  #include "komeda_framebuffer.h"
> +#include "komeda_color_mgmt.h"
>  
>  static int
>  komeda_plane_init_data_flow(struct drm_plane_state *st,
> @@ -250,6 +251,7 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
>  {
>  	struct komeda_dev *mdev = kms->base.dev_private;
>  	struct komeda_component *c = &layer->base;
> +	struct komeda_color_manager *color_mgr;
>  	struct komeda_plane *kplane;
>  	struct drm_plane *plane;
>  	u32 *formats, n_formats = 0;
> @@ -306,6 +308,16 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
>  	if (err)
>  		goto cleanup;
>  
> +	err = drm_plane_color_create_prop(plane->dev, plane);
> +	if (err)
> +		goto cleanup;
> +
> +	color_mgr = &layer->color_mgr;
> +	drm_plane_enable_color_mgmt(plane,
> +			color_mgr->igamma_mgr ? KOMEDA_COLOR_LUT_SIZE : 0,
> +			color_mgr->has_ctm,
> +			color_mgr->fgamma_mgr ? KOMEDA_COLOR_LUT_SIZE : 0);
> +
>  	err = drm_plane_create_zpos_property(plane, layer->base.id, 0, 8);
>  	if (err)
>  		goto cleanup;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c b/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c
> index 914400c4af73..4c474663f554 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c
> @@ -19,12 +19,15 @@ komeda_component_state_reset(struct komeda_component_state *st)
>  static struct drm_private_state *
>  komeda_layer_atomic_duplicate_state(struct drm_private_obj *obj)
>  {
> +	struct komeda_layer_state *old = to_layer_st(priv_to_comp_st(obj->state));
>  	struct komeda_layer_state *st;
>  
>  	st = kmemdup(obj->state, sizeof(*st), GFP_KERNEL);
>  	if (!st)
>  		return NULL;
>  
> +	komeda_color_duplicate_state(&st->color_st, &old->color_st);
> +
>  	komeda_component_state_reset(&st->base);
>  	__drm_atomic_helper_private_obj_duplicate_state(obj, &st->base.obj);
>  
> @@ -37,6 +40,7 @@ komeda_layer_atomic_destroy_state(struct drm_private_obj *obj,
>  {
>  	struct komeda_layer_state *st = to_layer_st(priv_to_comp_st(state));
>  
> +	komeda_color_cleanup_state(&st->color_st);
>  	kfree(st);
>  }
>  
> 

BR,
Mihail

Re: [PATCH v2 4/4] drm/komeda: Enable Layer color management for komeda

[james qian wang (Arm Technology China)]

On Tue, Aug 13, 2019 at 10:07:36AM +0000, Mihail Atanassov wrote:
> Hi James,
> 
> On Tuesday, 13 August 2019 05:56:19 BST james qian wang (Arm Technology China) wrote:
> > - D71 has 3 global layer gamma table which can be used for all layers as
> >   gamma lookup table, no matter inverse or forward.
> > - Add komeda_color_manager/state to layer/layer_state for describe the
> >   color caps for the specific layer which will be initialized in
> >   d71_layer_init, and for D71 only layer with L_INFO_CM (color mgmt) bit
> >   can support forward gamma, and CSC.
> > - Update komeda-CORE code to validate and convert the plane color state to
> >   komeda_color_state.
> > - Enable plane color mgmt according to layer komeda_color_manager
> > 
> > This patch depends on:
> > - https://patchwork.freedesktop.org/series/30876/
> > 
> > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> > ---
> >  .../arm/display/komeda/d71/d71_component.c    | 64 +++++++++++++++++++
> >  .../gpu/drm/arm/display/komeda/d71/d71_dev.c  |  5 +-
> >  .../gpu/drm/arm/display/komeda/d71/d71_dev.h  |  2 +
> >  .../drm/arm/display/komeda/komeda_pipeline.h  |  4 +-
> >  .../display/komeda/komeda_pipeline_state.c    | 53 ++++++++++++++-
> >  .../gpu/drm/arm/display/komeda/komeda_plane.c | 12 ++++
> >  .../arm/display/komeda/komeda_private_obj.c   |  4 ++
> >  7 files changed, 139 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > index 55a8cc94808a..c9c40a36e4d2 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > @@ -189,6 +189,46 @@ static void d71_layer_update_fb(struct komeda_component *c,
> >  	malidp_write32(reg, LAYER_FMT, kfb->format_caps->hw_id);
> >  }
> >  
> > +static u32 d71_layer_update_color(struct drm_plane_state *st,
> > +				  u32 __iomem *reg,
> > +				  struct komeda_color_state *color_st,
> > +				  u32 *mask)
> > +{
> > +	struct komeda_coeffs_table *igamma = color_st->igamma;
> > +	struct komeda_coeffs_table *fgamma = color_st->fgamma;
> > +	u32 ctrl = 0, v = 0;
> > +
> > +	if (!st->color_mgmt_changed)
> > +		return 0;
> > +
> > +	*mask |= L_IT | L_R2R | L_FT;
> > +
> > +	if (igamma) {
> > +		komeda_coeffs_update(igamma);
> > +		ctrl |= L_IT;
> > +		v = L_ITSEL(igamma->hw_id);
> > +	}
> > +
> > +	if (st->ctm) {
> > +		u32 ctm_coeffs[KOMEDA_N_CTM_COEFFS];
> > +
> > +		drm_ctm_to_coeffs(st->ctm, ctm_coeffs);
> > +		malidp_write_group(reg, LAYER_RGB_RGB_COEFF0,
> > +				   ARRAY_SIZE(ctm_coeffs),
> > +				   ctm_coeffs);
> > +		ctrl |= L_R2R; /* enable RGB2RGB conversion */
> > +	}
> > +
> > +	if (fgamma) {
> > +		komeda_coeffs_update(fgamma);
> > +		ctrl |= L_FT;
> > +		v |= L_FTSEL(fgamma->hw_id);
> > +	}
> > +
> > +	malidp_write32(reg, LAYER_LT_COEFFTAB, v);
> > +	return ctrl;
> > +}
> > +
> >  static void d71_layer_disable(struct komeda_component *c)
> >  {
> >  	malidp_write32_mask(c->reg, BLK_CONTROL, L_EN, 0);
> > @@ -261,6 +301,8 @@ static void d71_layer_update(struct komeda_component *c,
> >  
> >  	malidp_write32(reg, BLK_IN_SIZE, HV_SIZE(st->hsize, st->vsize));
> >  
> > +	ctrl |= d71_layer_update_color(plane_st, reg, &st->color_st, &ctrl_mask);
> > +
> >  	if (kfb->is_va)
> >  		ctrl |= L_TBU_EN;
> >  	malidp_write32_mask(reg, BLK_CONTROL, ctrl_mask, ctrl);
> > @@ -365,6 +407,12 @@ static int d71_layer_init(struct d71_dev *d71,
> >  	else
> >  		layer->layer_type = KOMEDA_FMT_SIMPLE_LAYER;
> >  
> > +	layer->color_mgr.igamma_mgr = d71->glb_lt_mgr;
> > +	if (layer_info & L_INFO_CM) {
> > +		layer->color_mgr.has_ctm = true;
> > +		layer->color_mgr.fgamma_mgr = d71->glb_lt_mgr;
> > +	}
> > +
> >  	set_range(&layer->hsize_in, 4, d71->max_line_size);
> >  	set_range(&layer->vsize_in, 4, d71->max_vsize);
> >  
> > @@ -1140,6 +1188,21 @@ static int d71_timing_ctrlr_init(struct d71_dev *d71,
> >  	return 0;
> >  }
> >  
> > +static void d71_gamma_update(struct komeda_coeffs_table *table)
> > +{
> > +	malidp_write_group(table->reg, GLB_LT_COEFF_DATA,
> > +			   GLB_LT_COEFF_NUM, table->coeffs);
> > +}
> > +
> > +static int d71_lt_coeff_init(struct d71_dev *d71,
> > +			     struct block_header *blk, u32 __iomem *reg)
> > +{
> > +	struct komeda_coeffs_manager *mgr = d71->glb_lt_mgr;
> > +	int hw_id = BLOCK_INFO_TYPE_ID(blk->block_info);
> > +
> > +	return komeda_coeffs_add(mgr, hw_id, reg, d71_gamma_update);
> > +}
> > +
> >  int d71_probe_block(struct d71_dev *d71,
> >  		    struct block_header *blk, u32 __iomem *reg)
> >  {
> > @@ -1202,6 +1265,7 @@ int d71_probe_block(struct d71_dev *d71,
> >  		break;
> >  
> >  	case D71_BLK_TYPE_GLB_LT_COEFF:
> > +		err = d71_lt_coeff_init(d71, blk, reg);
> >  		break;
> >  
> >  	case D71_BLK_TYPE_GLB_SCL_COEFF:
> > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > index d567ab7ed314..edd5d7c7f2a2 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > @@ -341,7 +341,7 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> >  	struct komeda_pipeline *pipe;
> >  	struct block_header blk;
> >  	u32 __iomem *blk_base;
> > -	u32 i, value, offset;
> > +	u32 i, value, offset, coeffs_size;
> >  	int err;
> >  
> >  	d71 = devm_kzalloc(mdev->dev, sizeof(*d71), GFP_KERNEL);
> > @@ -398,6 +398,9 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> >  		d71->pipes[i] = to_d71_pipeline(pipe);
> >  	}
> >  
> > +	coeffs_size = GLB_LT_COEFF_NUM * sizeof(u32);
> > +	d71->glb_lt_mgr = komeda_coeffs_create_manager(coeffs_size);
> > +
> >  	/* loop the register blks and probe */
> >  	i = 2; /* exclude GCU and PERIPH */
> >  	offset = D71_BLOCK_SIZE; /* skip GCU */
> > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.h
> > index 84f1878b647d..009c164a1584 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.h
> > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.h
> > @@ -39,6 +39,8 @@ struct d71_dev {
> >  	u32 __iomem	*periph_addr;
> >  
> >  	struct d71_pipeline *pipes[D71_MAX_PIPELINE];
> > +	 /* global layer transform coefficient table manager */
> > +	struct komeda_coeffs_manager *glb_lt_mgr;
> >  };
> >  
> >  #define to_d71_pipeline(x)	container_of(x, struct d71_pipeline, base)
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > index a7a84e66549d..4104c81e5032 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > @@ -10,6 +10,7 @@
> >  #include <linux/types.h>
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> > +#include "komeda_color_mgmt.h"
> >  #include "malidp_utils.h"
> >  
> >  #define KOMEDA_MAX_PIPELINES		2
> > @@ -226,6 +227,7 @@ struct komeda_layer {
> >  	struct komeda_component base;
> >  	/* accepted h/v input range before rotation */
> >  	struct malidp_range hsize_in, vsize_in;
> > +	struct komeda_color_manager color_mgr;
> >  	u32 layer_type; /* RICH, SIMPLE or WB */
> >  	u32 supported_rots;
> >  	/* komeda supports layer split which splits a whole image to two parts
> > @@ -238,7 +240,7 @@ struct komeda_layer {
> >  
> >  struct komeda_layer_state {
> >  	struct komeda_component_state base;
> > -	/* layer specific configuration state */
> > +	struct komeda_color_state color_st;
> >  	u16 hsize, vsize;
> >  	u32 rot;
> >  	u16 afbc_crop_l;
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > index ea26bc9c2d00..803715c1128e 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > @@ -95,6 +95,21 @@ komeda_pipeline_get_state_and_set_crtc(struct komeda_pipeline *pipe,
> >  	return st;
> >  }
> >  
> > +static bool komeda_component_is_new_active(struct komeda_component *c,
> > +					   struct drm_atomic_state *state)
> > +{
> > +	struct komeda_pipeline_state *ppl_old_st;
> > +
> > +	ppl_old_st = komeda_pipeline_get_old_state(c->pipeline, state);
> > +	if (IS_ERR(ppl_old_st))
> > +		return true;
> > +
> > +	if (has_bit(c->id, ppl_old_st->active_comps))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  static struct komeda_component_state *
> >  komeda_component_get_state(struct komeda_component *c,
> >  			   struct drm_atomic_state *state)
> > @@ -279,6 +294,29 @@ komeda_rotate_data_flow(struct komeda_data_flow_cfg *dflow, u32 rot)
> >  	}
> >  }
> >  
> > +static int
> > +komeda_validate_plane_color(struct komeda_component *c,
> > +			    struct komeda_color_manager *color_mgr,
> > +			    struct komeda_color_state *color_st,
> > +			    struct drm_plane_state *plane_st)
> > +{
> > +	int err;
> > +
> > +	if (komeda_component_is_new_active(c, plane_st->state))
> > +		plane_st->color_mgmt_changed = true;
> > +
> > +	if (!plane_st->color_mgmt_changed)
> > +		return 0;
> > +
> > +	err = komeda_color_validate(color_mgr, color_st,
> > +				    plane_st->degamma_lut,
> > +				    plane_st->gamma_lut);
> > +	if (err)
> > +		DRM_DEBUG_ATOMIC("%s validate color failed.\n", c->name);
> > +
> > +	return err;
> > +}
> > +
> >  static int
> >  komeda_layer_check_cfg(struct komeda_layer *layer,
> >  		       struct komeda_fb *kfb,
> > @@ -362,6 +400,11 @@ komeda_layer_validate(struct komeda_layer *layer,
> >  		st->addr[i] = komeda_fb_get_pixel_addr(kfb, dflow->in_x,
> >  						       dflow->in_y, i);
> >  
> > +	err = komeda_validate_plane_color(&layer->base, &layer->color_mgr,
> > +					  &st->color_st, plane_st);
> > +	if (err)
> > +		return err;
> > +
> >  	err = komeda_component_validate_private(&layer->base, c_st);
> >  	if (err)
> >  		return err;
> > @@ -1177,7 +1220,7 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
> >  {
> >  	struct drm_atomic_state *drm_st = new->obj.state;
> >  	struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
> > -	struct komeda_component_state *c_st;
> > +	struct komeda_component_state *st;
> >  	struct komeda_component *c;
> >  	u32 disabling_comps, id;
> >  
> > @@ -1188,9 +1231,13 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
> >  	/* unbound all disabling component */
> >  	dp_for_each_set_bit(id, disabling_comps) {
> >  		c = komeda_pipeline_get_component(pipe, id);
> > -		c_st = komeda_component_get_state_and_set_user(c,
> > +		st = komeda_component_get_state_and_set_user(c,
> >  				drm_st, NULL, new->crtc);
> > -		WARN_ON(IS_ERR(c_st));
> > +
> > +		WARN_ON(IS_ERR(st));
> I think this needs to be:
> if (WARN_ON(IS_ERR(st)))
>         continue;
> ...because...
> > +
> > +		if (has_bit(id, KOMEDA_PIPELINE_LAYERS))
> > +			komeda_color_cleanup_state(&to_layer_st(st)->color_st);
> ... this may deref an invalid `st' otherwise.

Because I don't real think unbound will return a error, so just a lazy
WARN_ON for the error handling. :)

But you are right. here we check the error we need to handle it
correctly. will refine the logic as you suggested.

thanks
james

> >  	}
> >  }
> >  
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> > index 98e915e325dd..69fa1dea41c9 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> > @@ -11,6 +11,7 @@
> >  #include "komeda_dev.h"
> >  #include "komeda_kms.h"
> >  #include "komeda_framebuffer.h"
> > +#include "komeda_color_mgmt.h"
> >  
> >  static int
> >  komeda_plane_init_data_flow(struct drm_plane_state *st,
> > @@ -250,6 +251,7 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
> >  {
> >  	struct komeda_dev *mdev = kms->base.dev_private;
> >  	struct komeda_component *c = &layer->base;
> > +	struct komeda_color_manager *color_mgr;
> >  	struct komeda_plane *kplane;
> >  	struct drm_plane *plane;
> >  	u32 *formats, n_formats = 0;
> > @@ -306,6 +308,16 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
> >  	if (err)
> >  		goto cleanup;
> >  
> > +	err = drm_plane_color_create_prop(plane->dev, plane);
> > +	if (err)
> > +		goto cleanup;
> > +
> > +	color_mgr = &layer->color_mgr;
> > +	drm_plane_enable_color_mgmt(plane,
> > +			color_mgr->igamma_mgr ? KOMEDA_COLOR_LUT_SIZE : 0,
> > +			color_mgr->has_ctm,
> > +			color_mgr->fgamma_mgr ? KOMEDA_COLOR_LUT_SIZE : 0);
> > +
> >  	err = drm_plane_create_zpos_property(plane, layer->base.id, 0, 8);
> >  	if (err)
> >  		goto cleanup;
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c b/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c
> > index 914400c4af73..4c474663f554 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c
> > @@ -19,12 +19,15 @@ komeda_component_state_reset(struct komeda_component_state *st)
> >  static struct drm_private_state *
> >  komeda_layer_atomic_duplicate_state(struct drm_private_obj *obj)
> >  {
> > +	struct komeda_layer_state *old = to_layer_st(priv_to_comp_st(obj->state));
> >  	struct komeda_layer_state *st;
> >  
> >  	st = kmemdup(obj->state, sizeof(*st), GFP_KERNEL);
> >  	if (!st)
> >  		return NULL;
> >  
> > +	komeda_color_duplicate_state(&st->color_st, &old->color_st);
> > +
> >  	komeda_component_state_reset(&st->base);
> >  	__drm_atomic_helper_private_obj_duplicate_state(obj, &st->base.obj);
> >  
> > @@ -37,6 +40,7 @@ komeda_layer_atomic_destroy_state(struct drm_private_obj *obj,
> >  {
> >  	struct komeda_layer_state *st = to_layer_st(priv_to_comp_st(state));
> >  
> > +	komeda_color_cleanup_state(&st->color_st);
> >  	kfree(st);
> >  }
> >  
> > 
> 
> BR,
> Mihail
> 
>