[PATCH 00/11] drm/i915: adding state checker for gamma lut values

[PATCH 00/11] drm/i915: adding state checker for gamma lut values

[Swati Sharma]

Thanks to Jani N, Matt and Ville for the review comments. Hopefully
I have addressed all the current review comments and ready to receive more :)

In this patch series, added state checker to validate gamma_lut values. This
reads hardware state, and compares the originally requested
state to the state read from hardware.

v1: -Implementation done for legacy platforms (removed all the placeholders) (Jani)
    -Added inverse function of drm_color_lut_extract to convert hardware
     read values back to user values (code written by Jani)
    -Renamed get_config() to color_config() (Jani)
    -Placed all platform specific shifts and masks in i915_reg.h (Jani)
    -Renamed i9xx_get_config to i9xx_color_config and all related
     functions (Jani)
    -Removed debug logs from compare function (Jani)
    -Renamed intel_compare_blob to intel_compare_lut and added platform specific
     bit precision of the readout into the function (Jani)
    -Renamed macro PIPE_CONF_CHECK_BLOB to PIPE_CONF_CHECK_COLOR_LUT (Jani)
    -Added check if blobs can be NULL (Jani)
    -Added function in intel_color.c that returns the bit precision (Jani),
     didn't add in device info since its gonna die soon (Ville)

v2: -Moved intel_compare_lut() from intel_display.c to intel_color.c (Jani)
    -Changed color_config() func names to get_color_config() and same
     for gamma func too (Jani)
    -Removed blank line in i915_reg.h and aligned MASK with their
     register name (Jani)
    -Mask definition code indented and defined using REG_GENMASK() and
     used using REG_FIELD_GET() (Jani)
    -Made bit_precision func inline (Jani/Matt)
    -Assigned bit_precision according to GAMMA_MODE (Matt/Ville)
    -Changed IS_ERR(blob) condition to (!blob) (Jani)
    -Rearranged blob check and lut_size conditions (Jani)
    -Used inline function for the comparison (Jani)
    -Separated the get config part from the state checker part to
     another patch (Jani)
    -Retained "internal" i9xx_internal_gamma_config for consistency
     (Matt)
    -Removed crtc_state_is_legacy_gamma check and replaced with
     GAMMA_MODE (Matt)
    -Created whole platform specific patch series as submitted by Ville for
     clean up intel_color_check()
    -Rebased on top of Ville's changes

v3: -Rebase

TODO:  -Matt suggested to rename get_gamma_config to get_gamma_lut or
        i9xx_read_out_gamma_lut, Ville also named functions like i9xx_read_lut_8(),
        i9xx_read_lut_10(). Should we follow this?
       -Answering Matt's query regarding intension to read and compare degamma/ctm".
        Degamma will be my first priority after this and later ctm.
        Should we have combined func or separate func for gamma/degamma val?
       -Add separate func to log errors. I am not sure, what needs to be printed here?
        Mismatched RGB value? 
       -Will make changes in next rev for changes made by Ville for cherryview_load_luts()


Swati Sharma (11):
  [v3] drm/i915: Introduce vfunc intel_get_color_config to create hw lut
  [v2] drm/i915: Extract i9xx_get_color_config()
  [v2] drm/i915: Extract cherryview_get_color_config()
  [v2] drm/i915: Extract i965_get_color_config()
  [v2] drm/i915: Extract icl_get_color_config()
  [v2] drm/i915: Extract glk_get_color_config()
  [v2] drm/i915: Extract bdw_get_color_config()
  [v2] drm/i915: Extract ivb_get_color_config()
  [v2] drm/i915: Extract ilk_get_color_config()
  [v2] drm/i915: Enable intel_get_color_config()
  [v3] drm/i915: Add intel_compare_color_lut() to compare hw and sw
    gamma lut values

 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/i915_reg.h      |  15 ++
 drivers/gpu/drm/i915/intel_color.c   | 347 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_color.h   |   7 +
 drivers/gpu/drm/i915/intel_display.c |  13 ++
 5 files changed, 378 insertions(+), 5 deletions(-)

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/11] [v3] drm/i915: Introduce vfunc intel_get_color_config to create hw lut

[Swati Sharma]

v3: Rebase

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    | 1 +
 drivers/gpu/drm/i915/intel_color.c | 7 +++++++
 drivers/gpu/drm/i915/intel_color.h | 1 +
 3 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 63eca30..69ed05c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -342,6 +342,7 @@ struct drm_i915_display_funcs {
 	 * involved with the same commit.
 	 */
 	void (*load_luts)(const struct intel_crtc_state *crtc_state);
+	void (*get_color_config)(struct intel_crtc_state *crtc_state);
 };
 
 #define CSR_VERSION(major, minor)	((major) << 16 | (minor))
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index ca341a9..321cf52 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -857,6 +857,13 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
 	return dev_priv->display.color_check(crtc_state);
 }
 
+void intel_get_color_config(struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+
+	dev_priv->display.get_color_config(crtc_state);
+}
+
 static bool need_plane_update(struct intel_plane *plane,
 			      const struct intel_crtc_state *crtc_state)
 {
diff --git a/drivers/gpu/drm/i915/intel_color.h b/drivers/gpu/drm/i915/intel_color.h
index b8a3ce6..7ca304f 100644
--- a/drivers/gpu/drm/i915/intel_color.h
+++ b/drivers/gpu/drm/i915/intel_color.h
@@ -13,5 +13,6 @@
 int intel_color_check(struct intel_crtc_state *crtc_state);
 void intel_color_commit(const struct intel_crtc_state *crtc_state);
 void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
+void intel_get_color_config(struct intel_crtc_state *crtc_state);
 
 #endif /* __INTEL_COLOR_H__ */
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/11] [v2] drm/i915: Extract i9xx_get_color_config()

[Swati Sharma]

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h    |  3 +++
 drivers/gpu/drm/i915/intel_color.c | 51 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9c206e8..8f2ae8a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7175,6 +7175,9 @@ enum {
 /* legacy palette */
 #define _LGC_PALETTE_A           0x4a000
 #define _LGC_PALETTE_B           0x4a800
+#define LGC_PALETTE_RED_MASK     REG_GENMASK(23, 16)
+#define LGC_PALETTE_GREEN_MASK   REG_GENMASK(15, 8)
+#define LGC_PALETTE_BLUE_MASK    REG_GENMASK(7, 0)
 #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) + (i) * 4)
 
 /* ilk/snb precision palette */
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 321cf52..f394402 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1228,6 +1228,56 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
+/* convert hw value with given bit_precision to lut property val */
+static u32 intel_color_lut_pack(u32 val, u32 bit_precision)
+{
+	u32 max = 0xffff >> (16 - bit_precision);
+
+	val = clamp_val(val, 0, max);
+
+	if (bit_precision < 16)
+		val <<= 16 - bit_precision;
+
+	return val;
+}
+
+static void i9xx_get_config_internal(struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+	struct drm_property_blob *blob = NULL;
+	struct drm_color_lut *blob_data;
+	u32 i, val;
+
+	blob = drm_property_create_blob(dev,
+					sizeof(struct drm_color_lut) * 256,
+					NULL);
+	if (IS_ERR(blob))
+		return;
+
+	blob_data = blob->data;
+
+	for (i = 0; i < 256; i++) {
+		if (HAS_GMCH(dev_priv))
+			val = I915_READ(PALETTE(pipe, i));
+		else
+			val = I915_READ(LGC_PALETTE(pipe, i));
+
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_RED_MASK, val) >> 16, 8);
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_GREEN_MASK, val) >> 8, 8);
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_BLUE_MASK, val), 8);
+	}
+
+	crtc_state->base.gamma_lut = blob;
+}
+
+static void i9xx_get_color_config(struct intel_crtc_state *crtc_state)
+{
+	i9xx_get_config_internal(crtc_state);
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1248,6 +1298,7 @@ void intel_color_init(struct intel_crtc *crtc)
 			dev_priv->display.color_check = i9xx_color_check;
 			dev_priv->display.color_commit = i9xx_color_commit;
 			dev_priv->display.load_luts = i9xx_load_luts;
+			dev_priv->display.get_color_config = i9xx_get_color_config;
 		}
 	} else {
 		if (INTEL_GEN(dev_priv) >= 11)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/11] [v2] drm/i915: Extract cherryview_get_color_config()

[Swati Sharma]

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h    |  3 +++
 drivers/gpu/drm/i915/intel_color.c | 39 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8f2ae8a..c9ae61e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10151,6 +10151,9 @@ enum skl_power_gate {
 #define   CGM_PIPE_MODE_GAMMA	(1 << 2)
 #define   CGM_PIPE_MODE_CSC	(1 << 1)
 #define   CGM_PIPE_MODE_DEGAMMA	(1 << 0)
+#define   CGM_PIPE_GAMMA_RED_MASK   REG_GENMASK(9, 0)
+#define   CGM_PIPE_GAMMA_GREEN_MASK REG_GENMASK(25, 16)
+#define   CGM_PIPE_GAMMA_BLUE_MASK  REG_GENMASK(9, 0)
 
 #define _CGM_PIPE_B_CSC_COEFF01	(VLV_DISPLAY_BASE + 0x69900)
 #define _CGM_PIPE_B_CSC_COEFF23	(VLV_DISPLAY_BASE + 0x69904)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index f394402..37d114e 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1278,6 +1278,44 @@ static void i9xx_get_color_config(struct intel_crtc_state *crtc_state)
 	i9xx_get_config_internal(crtc_state);
 }
 
+static void cherryview_get_gamma_config(struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 i, val, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
+	enum pipe pipe = crtc->pipe;
+	struct drm_property_blob *blob = NULL;
+	struct drm_color_lut *blob_data;
+
+	blob = drm_property_create_blob(dev,
+					sizeof(struct drm_color_lut) * 256,
+					NULL);
+	if (IS_ERR(blob))
+		return;
+
+	blob_data = blob->data;
+
+	for (i = 0; i < lut_size; i++) {
+		val = I915_READ(CGM_PIPE_GAMMA(pipe, i, 0));
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(CGM_PIPE_GAMMA_GREEN_MASK, val) >> 16, 10);
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(CGM_PIPE_GAMMA_BLUE_MASK, val), 10);
+
+		val = I915_READ(CGM_PIPE_GAMMA(pipe, i, 1));
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(CGM_PIPE_GAMMA_RED_MASK, val), 10);
+	}
+
+	crtc_state->base.gamma_lut = blob;
+}
+
+static void cherryview_get_color_config(struct intel_crtc_state *crtc_state)
+{
+	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
+		i9xx_get_color_config(crtc_state);
+	else
+		cherryview_get_gamma_config(crtc_state);
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1290,6 +1328,7 @@ void intel_color_init(struct intel_crtc *crtc)
 			dev_priv->display.color_check = chv_color_check;
 			dev_priv->display.color_commit = i9xx_color_commit;
 			dev_priv->display.load_luts = cherryview_load_luts;
+			dev_priv->display.get_color_config = cherryview_get_color_config;
 		} else if (INTEL_GEN(dev_priv) >= 4) {
 			dev_priv->display.color_check = i9xx_color_check;
 			dev_priv->display.color_commit = i9xx_color_commit;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/11] [v2] drm/i915: Extract i965_get_color_config()

[Swati Sharma]

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h    |  3 +++
 drivers/gpu/drm/i915/intel_color.c | 39 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c9ae61e..1d0575f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3582,6 +3582,9 @@ enum i915_power_well_id {
 #define _PALETTE_A		0xa000
 #define _PALETTE_B		0xa800
 #define _CHV_PALETTE_C		0xc000
+#define PALETTE_RED_MASK        REG_GENMASK(23, 16)
+#define PALETTE_GREEN_MASK      REG_GENMASK(15, 8)
+#define PALETTE_BLUE_MASK       REG_GENMASK(7, 0)
 #define PALETTE(pipe, i)	_MMIO(DISPLAY_MMIO_BASE(dev_priv) + \
 				      _PICK((pipe), _PALETTE_A,		\
 					    _PALETTE_B, _CHV_PALETTE_C) + \
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 37d114e..a9e5d73 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1316,6 +1316,44 @@ static void cherryview_get_color_config(struct intel_crtc_state *crtc_state)
 		cherryview_get_gamma_config(crtc_state);
 }
 
+static void i965_get_gamma_config_10p6(struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 i, val1, val2, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
+	enum pipe pipe = crtc->pipe;
+	struct drm_property_blob *blob = NULL;
+	struct drm_color_lut *blob_data;
+
+	blob = drm_property_create_blob(dev,
+					sizeof(struct drm_color_lut) * lut_size,
+					NULL);
+	if (IS_ERR(blob))
+		return;
+
+	blob_data = blob->data;
+
+	for (i = 0; i < lut_size - 1; i++) {
+		val1 = I915_READ(PALETTE(pipe, 2 * i + 0));
+		val2 = I915_READ(PALETTE(pipe, 2 * i + 1));
+
+		blob_data[i].red = (REG_FIELD_GET(PALETTE_RED_MASK, val1) >> 16) << 8 | (REG_FIELD_GET(PALETTE_RED_MASK, val2) >> 16);
+		blob_data[i].green = (REG_FIELD_GET(PALETTE_GREEN_MASK, val1) >> 8) << 8 | (REG_FIELD_GET(PALETTE_GREEN_MASK, val2) >> 8);
+		blob_data[i].blue = REG_FIELD_GET(PALETTE_BLUE_MASK, val1) << 8 | REG_FIELD_GET(PALETTE_BLUE_MASK, val2) ;
+	}
+
+	crtc_state->base.gamma_lut = blob;
+}
+
+static void i965_get_color_config(struct intel_crtc_state *crtc_state)
+{
+	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
+		i9xx_get_color_config(crtc_state);
+	else
+		i965_get_gamma_config_10p6(crtc_state);
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1333,6 +1371,7 @@ void intel_color_init(struct intel_crtc *crtc)
 			dev_priv->display.color_check = i9xx_color_check;
 			dev_priv->display.color_commit = i9xx_color_commit;
 			dev_priv->display.load_luts = i965_load_luts;
+			dev_priv->display.get_color_config = i965_get_color_config;
 		} else {
 			dev_priv->display.color_check = i9xx_color_check;
 			dev_priv->display.color_commit = i9xx_color_commit;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/11] [v2] drm/i915: Extract icl_get_color_config()

[Swati Sharma]

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h    |  3 +++
 drivers/gpu/drm/i915/intel_color.c | 49 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1d0575f..38d6684 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10115,6 +10115,9 @@ enum skl_power_gate {
 #define _PAL_PREC_DATA_A	0x4A404
 #define _PAL_PREC_DATA_B	0x4AC04
 #define _PAL_PREC_DATA_C	0x4B404
+#define   PREC_PAL_DATA_RED_MASK	REG_GENMASK(29, 20)
+#define   PREC_PAL_DATA_GREEN_MASK	REG_GENMASK(19, 10)
+#define   PREC_PAL_DATA_BLUE_MASK	REG_GENMASK(9, 0)
 #define _PAL_PREC_GC_MAX_A	0x4A410
 #define _PAL_PREC_GC_MAX_B	0x4AC10
 #define _PAL_PREC_GC_MAX_C	0x4B410
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index a9e5d73..49e5d8c 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1354,6 +1354,51 @@ static void i965_get_color_config(struct intel_crtc_state *crtc_state)
 		i965_get_gamma_config_10p6(crtc_state);
 }
 
+static void bdw_get_gamma_config(struct intel_crtc_state *crtc_state,
+			         u32 prec_index)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	int hw_lut_size = ivb_lut_10_size(prec_index);
+	enum pipe pipe = crtc->pipe;
+	struct drm_property_blob *blob = NULL;
+	struct drm_color_lut *blob_data;
+	u32 i, val;
+
+	I915_WRITE(PREC_PAL_INDEX(pipe), prec_index |
+		   PAL_PREC_AUTO_INCREMENT);
+
+	blob = drm_property_create_blob(dev,
+					sizeof(struct drm_color_lut) * hw_lut_size,
+					NULL);
+	if (IS_ERR(blob))
+		return;
+
+	blob_data = blob->data;
+
+	for (i = 0; i < hw_lut_size; i++) {
+		val = I915_READ(PREC_PAL_DATA(pipe));
+
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(PREC_PAL_DATA_RED_MASK, val) >> 20, 10);
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(PREC_PAL_DATA_GREEN_MASK, val) >> 10, 10);
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(PREC_PAL_DATA_BLUE_MASK, val), 10);
+	}
+
+	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
+
+	crtc_state->base.gamma_lut = blob;
+}
+
+static void icl_get_color_config(struct intel_crtc_state *crtc_state)
+{
+	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
+	    GAMMA_MODE_MODE_8BIT)
+		i9xx_get_color_config(crtc_state);
+	else
+		bdw_get_gamma_config(crtc_state, PAL_PREC_INDEX_VALUE(0));
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1395,8 +1440,10 @@ void intel_color_init(struct intel_crtc *crtc)
 		else
 			dev_priv->display.color_commit = ilk_color_commit;
 
-		if (INTEL_GEN(dev_priv) >= 11)
+		if (INTEL_GEN(dev_priv) >= 11) {
 			dev_priv->display.load_luts = icl_load_luts;
+			dev_priv->display.get_color_config = icl_get_color_config;
+		}
 		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
 			dev_priv->display.load_luts = glk_load_luts;
 		else if (INTEL_GEN(dev_priv) >= 8)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/11] [v2] drm/i915: Extract glk_get_color_config()

[Swati Sharma]

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 49e5d8c..6ccb76f 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1399,6 +1399,14 @@ static void icl_get_color_config(struct intel_crtc_state *crtc_state)
 		bdw_get_gamma_config(crtc_state, PAL_PREC_INDEX_VALUE(0));
 }
 
+static void glk_get_color_config(struct intel_crtc_state *crtc_state)
+{
+	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
+		i9xx_get_color_config(crtc_state);
+	else
+		bdw_get_gamma_config(crtc_state, PAL_PREC_INDEX_VALUE(0));
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1443,9 +1451,10 @@ void intel_color_init(struct intel_crtc *crtc)
 		if (INTEL_GEN(dev_priv) >= 11) {
 			dev_priv->display.load_luts = icl_load_luts;
 			dev_priv->display.get_color_config = icl_get_color_config;
-		}
-		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
+		} else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
 			dev_priv->display.load_luts = glk_load_luts;
+			dev_priv->display.get_color_config = glk_get_color_config;
+		}
 		else if (INTEL_GEN(dev_priv) >= 8)
 			dev_priv->display.load_luts = bdw_load_luts;
 		else if (INTEL_GEN(dev_priv) >= 7)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/11] [v2] drm/i915: Extract bdw_get_color_config()

[Swati Sharma]

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 6ccb76f..74f000e 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1407,6 +1407,17 @@ static void glk_get_color_config(struct intel_crtc_state *crtc_state)
 		bdw_get_gamma_config(crtc_state, PAL_PREC_INDEX_VALUE(0));
 }
 
+static void bdw_get_color_config(struct intel_crtc_state *crtc_state)
+{
+	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
+		i9xx_get_color_config(crtc_state);
+	else if (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)
+		bdw_get_gamma_config(crtc_state, PAL_PREC_SPLIT_MODE |
+				     PAL_PREC_INDEX_VALUE(512));
+	else
+		bdw_get_gamma_config(crtc_state, PAL_PREC_INDEX_VALUE(0));
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1454,9 +1465,10 @@ void intel_color_init(struct intel_crtc *crtc)
 		} else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
 			dev_priv->display.load_luts = glk_load_luts;
 			dev_priv->display.get_color_config = glk_get_color_config;
-		}
-		else if (INTEL_GEN(dev_priv) >= 8)
+		} else if (INTEL_GEN(dev_priv) >= 8) {
 			dev_priv->display.load_luts = bdw_load_luts;
+			dev_priv->display.get_color_config = bdw_get_color_config;
+		}
 		else if (INTEL_GEN(dev_priv) >= 7)
 			dev_priv->display.load_luts = ivb_load_luts;
 		else
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/11] [v2] drm/i915: Extract ivb_get_color_config()

[Swati Sharma]

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 50 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 74f000e..77b6f17 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1418,6 +1418,51 @@ static void bdw_get_color_config(struct intel_crtc_state *crtc_state)
 		bdw_get_gamma_config(crtc_state, PAL_PREC_INDEX_VALUE(0));
 }
 
+static void ivb_get_gamma_config(struct intel_crtc_state *crtc_state,
+			         u32 prec_index)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	int hw_lut_size = ivb_lut_10_size(prec_index);
+	enum pipe pipe = crtc->pipe;
+	struct drm_property_blob *blob = NULL;
+	struct drm_color_lut *blob_data;
+	u32 i, val;
+
+	blob = drm_property_create_blob(dev,
+					sizeof(struct drm_color_lut) * hw_lut_size,
+					NULL);
+	if (IS_ERR(blob))
+		return;
+
+	blob_data = blob->data;
+
+	for (i = 0; i < hw_lut_size; i++) {
+		I915_WRITE(PREC_PAL_INDEX(pipe), prec_index++);
+		val = I915_READ(PREC_PAL_DATA(pipe));
+
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(PREC_PAL_DATA_RED_MASK, val) >> 20, 10);
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(PREC_PAL_DATA_GREEN_MASK, val) >> 10, 10);
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(PREC_PAL_DATA_BLUE_MASK, val), 10);
+	}
+
+	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
+
+	crtc_state->base.gamma_lut = blob;
+}
+
+static void ivb_get_color_config(struct intel_crtc_state *crtc_state)
+{
+	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
+		i9xx_get_color_config(crtc_state);
+	else if (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)
+		ivb_get_gamma_config(crtc_state, PAL_PREC_SPLIT_MODE |
+				     PAL_PREC_INDEX_VALUE(512));
+	else
+		bdw_get_gamma_config(crtc_state, PAL_PREC_INDEX_VALUE(0));
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1468,9 +1513,10 @@ void intel_color_init(struct intel_crtc *crtc)
 		} else if (INTEL_GEN(dev_priv) >= 8) {
 			dev_priv->display.load_luts = bdw_load_luts;
 			dev_priv->display.get_color_config = bdw_get_color_config;
-		}
-		else if (INTEL_GEN(dev_priv) >= 7)
+		} else if (INTEL_GEN(dev_priv) >= 7) {
 			dev_priv->display.load_luts = ivb_load_luts;
+			dev_priv->display.get_color_config = ivb_get_color_config;
+		}
 		else
 			dev_priv->display.load_luts = ilk_load_luts;
 	}
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/11] [v2] drm/i915: Extract ilk_get_color_config()

[Swati Sharma]

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h    |  3 +++
 drivers/gpu/drm/i915/intel_color.c | 42 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 38d6684..f6a41f7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7186,6 +7186,9 @@ enum {
 /* ilk/snb precision palette */
 #define _PREC_PALETTE_A           0x4b000
 #define _PREC_PALETTE_B           0x4c000
+#define   PREC_PALETTE_RED_MASK   REG_GENMASK(29, 20)
+#define   PREC_PALETTE_GREEN_MASK REG_GENMASK(19, 10)
+#define   PREC_PALETTE_BLUE_MASK  REG_GENMASK(9, 0)
 #define PREC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _PREC_PALETTE_A, _PREC_PALETTE_B) + (i) * 4)
 
 #define  _PREC_PIPEAGCMAX              0x4d000
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 77b6f17..e2703f9 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1463,6 +1463,43 @@ static void ivb_get_color_config(struct intel_crtc_state *crtc_state)
 		bdw_get_gamma_config(crtc_state, PAL_PREC_INDEX_VALUE(0));
 }
 
+static void ilk_get_gamma_config(struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 i, val, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
+	enum pipe pipe = crtc->pipe;
+	struct drm_property_blob *blob = NULL;
+	struct drm_color_lut *blob_data;
+
+	blob = drm_property_create_blob(dev,
+					sizeof(struct drm_color_lut) * lut_size,
+					NULL);
+	if (IS_ERR(blob))
+		return;
+
+	blob_data = blob->data;
+
+	for (i = 0; i < lut_size - 1; i++) {
+		val = I915_READ(PREC_PALETTE(pipe, i));
+
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(PREC_PALETTE_RED_MASK, val) >> 20, 10);
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(PREC_PALETTE_GREEN_MASK, val) >> 10, 10);
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(PREC_PALETTE_BLUE_MASK, val), 10);
+	}
+
+	crtc_state->base.gamma_lut = blob;
+}
+
+static void ilk_get_color_config(struct intel_crtc_state *crtc_state)
+{
+	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
+		i9xx_get_color_config(crtc_state);
+	else
+		ilk_get_gamma_config(crtc_state);
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1516,9 +1553,10 @@ void intel_color_init(struct intel_crtc *crtc)
 		} else if (INTEL_GEN(dev_priv) >= 7) {
 			dev_priv->display.load_luts = ivb_load_luts;
 			dev_priv->display.get_color_config = ivb_get_color_config;
-		}
-		else
+		} else {
 			dev_priv->display.load_luts = ilk_load_luts;
+			dev_priv->display.get_color_config = ilk_get_color_config;
+		}
 	}
 
 	drm_crtc_enable_color_mgmt(&crtc->base,
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/11] [v2] drm/i915: Enable intel_get_color_config()

[Swati Sharma]

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f29a348..0fc9dab 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8275,6 +8275,7 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 		pipe_config->cgm_mode = I915_READ(CGM_PIPE_MODE(crtc->pipe));
 
 	i9xx_get_pipe_color_config(pipe_config);
+	intel_get_color_config(pipe_config);
 
 	if (INTEL_GEN(dev_priv) < 4)
 		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
@@ -9348,6 +9349,7 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 	pipe_config->csc_mode = I915_READ(PIPE_CSC_MODE(crtc->pipe));
 
 	i9xx_get_pipe_color_config(pipe_config);
+	intel_get_color_config(pipe_config);
 
 	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
 		struct intel_shared_dpll *pll;
@@ -10011,6 +10013,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 			pipe_config->csc_enable = true;
 	} else {
 		i9xx_get_pipe_color_config(pipe_config);
+		intel_get_color_config(pipe_config);
 	}
 
 	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/11] [v3] drm/i915: Add intel_compare_color_lut() to compare hw and sw gamma lut values

[Swati Sharma]

v3: Rebase

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_color.c   | 49 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color.h   |  6 +++++
 drivers/gpu/drm/i915/intel_display.c | 10 ++++++++
 3 files changed, 65 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index e2703f9..867c1de 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1500,6 +1500,55 @@ static void ilk_get_color_config(struct intel_crtc_state *crtc_state)
 		ilk_get_gamma_config(crtc_state);
 }
 
+static inline bool gamma_err_check(struct drm_color_lut *sw_lut, struct drm_color_lut *hw_lut, u32 err)
+{
+	 return ((abs((long)hw_lut->red - sw_lut->red)) >= err) ||
+	        ((abs((long)hw_lut->blue - sw_lut->blue)) >= err) ||
+	        ((abs((long)hw_lut->green - sw_lut->green)) >= err);
+}
+
+bool intel_compare_color_lut(struct drm_property_blob *blob1,
+			     struct drm_property_blob *blob2,
+			     u32 gamma_mode)
+{
+	struct drm_color_lut *sw_lut = blob1->data;
+	struct drm_color_lut *hw_lut = blob2->data;
+	int sw_lut_size, hw_lut_size, i;
+	u32 bit_precision, err;
+
+	if (!blob1 || !blob2)
+		return false;
+
+	switch(gamma_mode) {
+	case GAMMA_MODE_MODE_8BIT:
+		bit_precision = 8;
+		break;
+	case GAMMA_MODE_MODE_10BIT:
+		bit_precision = 10;
+		break;
+	case GAMMA_MODE_MODE_12BIT:
+		bit_precision = 12;
+		break;
+	default:
+		bit_precision = 8;
+	}
+
+	err = 0xffff >> bit_precision;
+
+	sw_lut_size = drm_color_lut_size(blob1);
+	hw_lut_size = drm_color_lut_size(blob2);
+
+	if (sw_lut_size != hw_lut_size)
+		return false;
+
+	for (i = 0; i < sw_lut_size; i++) {
+		 if (!gamma_err_check(&hw_lut[i], &sw_lut[i], err))
+			return false;
+	}
+
+	return true;
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
diff --git a/drivers/gpu/drm/i915/intel_color.h b/drivers/gpu/drm/i915/intel_color.h
index 7ca304f..575003f 100644
--- a/drivers/gpu/drm/i915/intel_color.h
+++ b/drivers/gpu/drm/i915/intel_color.h
@@ -6,13 +6,19 @@
 #ifndef __INTEL_COLOR_H__
 #define __INTEL_COLOR_H__
 
+#include <linux/types.h>
+
 struct intel_crtc_state;
 struct intel_crtc;
+struct drm_property_blob;
 
 void intel_color_init(struct intel_crtc *crtc);
 int intel_color_check(struct intel_crtc_state *crtc_state);
 void intel_color_commit(const struct intel_crtc_state *crtc_state);
 void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
 void intel_get_color_config(struct intel_crtc_state *crtc_state);
+bool intel_compare_color_lut(struct drm_property_blob *blob1,
+			     struct drm_property_blob *blob2,
+			     u32 gamma_mode);
 
 #endif /* __INTEL_COLOR_H__ */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0fc9dab..b074f00 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12236,6 +12236,14 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 	} \
 } while (0)
 
+#define PIPE_CONF_CHECK_COLOR_LUT(name, gamma_mode) do { \
+	if (!intel_compare_color_lut(current_config->name, pipe_config->name, gamma_mode)) { \
+		pipe_config_err(adjust, __stringify(name), \
+				"hw_state doesn't match sw_state\n"); \
+		ret = false; \
+	} \
+} while (0)
+
 #define PIPE_CONF_QUIRK(quirk) \
 	((current_config->quirks | pipe_config->quirks) & (quirk))
 
@@ -12379,6 +12387,8 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 	PIPE_CONF_CHECK_INFOFRAME(spd);
 	PIPE_CONF_CHECK_INFOFRAME(hdmi);
 
+	PIPE_CONF_CHECK_COLOR_LUT(base.gamma_lut, pipe_config->gamma_mode);
+
 #undef PIPE_CONF_CHECK_X
 #undef PIPE_CONF_CHECK_I
 #undef PIPE_CONF_CHECK_BOOL
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/i915: adding state checker for gamma lut values (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915: adding state checker for gamma lut values (rev4)
URL   : https://patchwork.freedesktop.org/series/58039/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
37affd023b33 drm/i915: Introduce vfunc intel_get_color_config to create hw lut
2c1fbf49d4d5 drm/i915: Extract i9xx_get_color_config()
-:7: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

-:67: WARNING:LONG_LINE: line over 100 characters
#67: FILE: drivers/gpu/drm/i915/intel_color.c:1268:
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_RED_MASK, val) >> 16, 8);

-:68: WARNING:LONG_LINE: line over 100 characters
#68: FILE: drivers/gpu/drm/i915/intel_color.c:1269:
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_GREEN_MASK, val) >> 8, 8);

-:69: WARNING:LONG_LINE: line over 100 characters
#69: FILE: drivers/gpu/drm/i915/intel_color.c:1270:
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_BLUE_MASK, val), 8);

total: 0 errors, 4 warnings, 0 checks, 72 lines checked
ed916c9dbb95 drm/i915: Extract cherryview_get_color_config()
-:7: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

-:50: WARNING:LONG_LINE: line over 100 characters
#50: FILE: drivers/gpu/drm/i915/intel_color.c:1301:
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(CGM_PIPE_GAMMA_GREEN_MASK, val) >> 16, 10);

-:51: WARNING:LONG_LINE: line over 100 characters
#51: FILE: drivers/gpu/drm/i915/intel_color.c:1302:
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(CGM_PIPE_GAMMA_BLUE_MASK, val), 10);

-:54: WARNING:LONG_LINE: line over 100 characters
#54: FILE: drivers/gpu/drm/i915/intel_color.c:1305:
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(CGM_PIPE_GAMMA_RED_MASK, val), 10);

total: 0 errors, 4 warnings, 0 checks, 60 lines checked
4e1026ef5692 drm/i915: Extract i965_get_color_config()
-:7: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

-:52: WARNING:LONG_LINE: line over 100 characters
#52: FILE: drivers/gpu/drm/i915/intel_color.c:1341:
+		blob_data[i].red = (REG_FIELD_GET(PALETTE_RED_MASK, val1) >> 16) << 8 | (REG_FIELD_GET(PALETTE_RED_MASK, val2) >> 16);

-:53: WARNING:LONG_LINE: line over 100 characters
#53: FILE: drivers/gpu/drm/i915/intel_color.c:1342:
+		blob_data[i].green = (REG_FIELD_GET(PALETTE_GREEN_MASK, val1) >> 8) << 8 | (REG_FIELD_GET(PALETTE_GREEN_MASK, val2) >> 8);

-:54: WARNING:LONG_LINE: line over 100 characters
#54: FILE: drivers/gpu/drm/i915/intel_color.c:1343:
+		blob_data[i].blue = REG_FIELD_GET(PALETTE_BLUE_MASK, val1) << 8 | REG_FIELD_GET(PALETTE_BLUE_MASK, val2) ;

-:54: WARNING:SPACING: space prohibited before semicolon
#54: FILE: drivers/gpu/drm/i915/intel_color.c:1343:
+		blob_data[i].blue = REG_FIELD_GET(PALETTE_BLUE_MASK, val1) << 8 | REG_FIELD_GET(PALETTE_BLUE_MASK, val2) ;

total: 0 errors, 5 warnings, 0 checks, 60 lines checked
950b0424cebd drm/i915: Extract icl_get_color_config()
-:7: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

-:31: ERROR:CODE_INDENT: code indent should use tabs where possible
#31: FILE: drivers/gpu/drm/i915/intel_color.c:1358:
+^I^I^I         u32 prec_index)$

-:31: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#31: FILE: drivers/gpu/drm/i915/intel_color.c:1358:
+static void bdw_get_gamma_config(struct intel_crtc_state *crtc_state,
+			         u32 prec_index)

-:56: WARNING:LONG_LINE: line over 100 characters
#56: FILE: drivers/gpu/drm/i915/intel_color.c:1383:
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(PREC_PAL_DATA_RED_MASK, val) >> 20, 10);

-:57: WARNING:LONG_LINE: line over 100 characters
#57: FILE: drivers/gpu/drm/i915/intel_color.c:1384:
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(PREC_PAL_DATA_GREEN_MASK, val) >> 10, 10);

-:58: WARNING:LONG_LINE: line over 100 characters
#58: FILE: drivers/gpu/drm/i915/intel_color.c:1385:
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(PREC_PAL_DATA_BLUE_MASK, val), 10);

-:83: CHECK:BRACES: braces {} should be used on all arms of this statement
#83: FILE: drivers/gpu/drm/i915/intel_color.c:1443:
+		if (INTEL_GEN(dev_priv) >= 11) {
[...]
 		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
[...]

total: 1 errors, 4 warnings, 2 checks, 71 lines checked
f0350da652e9 drm/i915: Extract glk_get_color_config()
-:7: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

total: 0 errors, 1 warnings, 0 checks, 26 lines checked
6cd82ce2f4a1 drm/i915: Extract bdw_get_color_config()
-:7: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

total: 0 errors, 1 warnings, 0 checks, 29 lines checked
85fd57b4994b drm/i915: Extract ivb_get_color_config()
-:7: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

-:17: ERROR:CODE_INDENT: code indent should use tabs where possible
#17: FILE: drivers/gpu/drm/i915/intel_color.c:1422:
+^I^I^I         u32 prec_index)$

-:17: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#17: FILE: drivers/gpu/drm/i915/intel_color.c:1422:
+static void ivb_get_gamma_config(struct intel_crtc_state *crtc_state,
+			         u32 prec_index)

-:40: WARNING:LONG_LINE: line over 100 characters
#40: FILE: drivers/gpu/drm/i915/intel_color.c:1445:
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(PREC_PAL_DATA_RED_MASK, val) >> 20, 10);

-:41: WARNING:LONG_LINE: line over 100 characters
#41: FILE: drivers/gpu/drm/i915/intel_color.c:1446:
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(PREC_PAL_DATA_GREEN_MASK, val) >> 10, 10);

-:42: WARNING:LONG_LINE: line over 100 characters
#42: FILE: drivers/gpu/drm/i915/intel_color.c:1447:
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(PREC_PAL_DATA_BLUE_MASK, val), 10);

total: 1 errors, 4 warnings, 1 checks, 63 lines checked
c4b9331e9b26 drm/i915: Extract ilk_get_color_config()
-:7: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

-:51: WARNING:LONG_LINE: line over 100 characters
#51: FILE: drivers/gpu/drm/i915/intel_color.c:1487:
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(PREC_PALETTE_RED_MASK, val) >> 20, 10);

-:52: WARNING:LONG_LINE: line over 100 characters
#52: FILE: drivers/gpu/drm/i915/intel_color.c:1488:
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(PREC_PALETTE_GREEN_MASK, val) >> 10, 10);

-:53: WARNING:LONG_LINE: line over 100 characters
#53: FILE: drivers/gpu/drm/i915/intel_color.c:1489:
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(PREC_PALETTE_BLUE_MASK, val), 10);

total: 0 errors, 4 warnings, 0 checks, 64 lines checked
b4e5942038d9 drm/i915: Enable intel_get_color_config()
-:7: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

total: 0 errors, 1 warnings, 0 checks, 21 lines checked
4d127259db0f drm/i915: Add intel_compare_color_lut() to compare hw and sw gamma lut values
-:19: WARNING:LONG_LINE: line over 100 characters
#19: FILE: drivers/gpu/drm/i915/intel_color.c:1503:
+static inline bool gamma_err_check(struct drm_color_lut *sw_lut, struct drm_color_lut *hw_lut, u32 err)

-:21: WARNING:TABSTOP: Statements should start on a tabstop
#21: FILE: drivers/gpu/drm/i915/intel_color.c:1505:
+	 return ((abs((long)hw_lut->red - sw_lut->red)) >= err) ||

-:22: ERROR:CODE_INDENT: code indent should use tabs where possible
#22: FILE: drivers/gpu/drm/i915/intel_color.c:1506:
+^I        ((abs((long)hw_lut->blue - sw_lut->blue)) >= err) ||$

-:23: ERROR:CODE_INDENT: code indent should use tabs where possible
#23: FILE: drivers/gpu/drm/i915/intel_color.c:1507:
+^I        ((abs((long)hw_lut->green - sw_lut->green)) >= err);$

-:38: ERROR:SPACING: space required before the open parenthesis '('
#38: FILE: drivers/gpu/drm/i915/intel_color.c:1522:
+	switch(gamma_mode) {

-:60: WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (8, 17)
#60: FILE: drivers/gpu/drm/i915/intel_color.c:1544:
+	for (i = 0; i < sw_lut_size; i++) {
+		 if (!gamma_err_check(&hw_lut[i], &sw_lut[i], err))

-:61: WARNING:TABSTOP: Statements should start on a tabstop
#61: FILE: drivers/gpu/drm/i915/intel_color.c:1545:
+		 if (!gamma_err_check(&hw_lut[i], &sw_lut[i], err))

-:103: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'name' - possible side-effects?
#103: FILE: drivers/gpu/drm/i915/intel_display.c:12240:
+#define PIPE_CONF_CHECK_COLOR_LUT(name, gamma_mode) do { \
+	if (!intel_compare_color_lut(current_config->name, pipe_config->name, gamma_mode)) { \
+		pipe_config_err(adjust, __stringify(name), \
+				"hw_state doesn't match sw_state\n"); \
+		ret = false; \
+	} \
+} while (0)

-:103: CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'name' may be better as '(name)' to avoid precedence issues
#103: FILE: drivers/gpu/drm/i915/intel_display.c:12240:
+#define PIPE_CONF_CHECK_COLOR_LUT(name, gamma_mode) do { \
+	if (!intel_compare_color_lut(current_config->name, pipe_config->name, gamma_mode)) { \
+		pipe_config_err(adjust, __stringify(name), \
+				"hw_state doesn't match sw_state\n"); \
+		ret = false; \
+	} \
+} while (0)

total: 3 errors, 4 warnings, 2 checks, 96 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for drm/i915: adding state checker for gamma lut values (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915: adding state checker for gamma lut values (rev4)
URL   : https://patchwork.freedesktop.org/series/58039/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Introduce vfunc intel_get_color_config to create hw lut
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3616:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3617:16: warning: expression using sizeof(void)

Commit: drm/i915: Extract i9xx_get_color_config()
+drivers/gpu/drm/i915/intel_color.c:1236:15: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:1236:15: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:1236:15: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:1236:15: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:1236:15: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:1236:15: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:1236:15: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_color.c:1236:15: warning: expression using sizeof(void)

Commit: drm/i915: Extract cherryview_get_color_config()
Okay!

Commit: drm/i915: Extract i965_get_color_config()
Okay!

Commit: drm/i915: Extract icl_get_color_config()
Okay!

Commit: drm/i915: Extract glk_get_color_config()
Okay!

Commit: drm/i915: Extract bdw_get_color_config()
Okay!

Commit: drm/i915: Extract ivb_get_color_config()
Okay!

Commit: drm/i915: Extract ilk_get_color_config()
Okay!

Commit: drm/i915: Enable intel_get_color_config()
Okay!

Commit: drm/i915: Add intel_compare_color_lut() to compare hw and sw gamma lut values
Okay!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: adding state checker for gamma lut values (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915: adding state checker for gamma lut values (rev4)
URL   : https://patchwork.freedesktop.org/series/58039/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5933 -> Patchwork_12795
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12795 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12795, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58039/revisions/4/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12795:

### IGT changes ###

#### Possible regressions ####

  * igt@runner@aborted:
    - fi-ilk-650:         NOTRUN -> FAIL
    - fi-pnv-d510:        NOTRUN -> FAIL
    - fi-bdw-gvtdvm:      NOTRUN -> FAIL
    - fi-hsw-peppy:       NOTRUN -> FAIL
    - fi-gdg-551:         NOTRUN -> FAIL
    - fi-snb-2520m:       NOTRUN -> FAIL
    - fi-hsw-4770:        NOTRUN -> FAIL
    - fi-whl-u:           NOTRUN -> FAIL
    - fi-icl-u3:          NOTRUN -> FAIL
    - fi-ivb-3770:        NOTRUN -> FAIL
    - fi-byt-j1900:       NOTRUN -> FAIL
    - fi-icl-y:           NOTRUN -> FAIL
    - fi-bsw-n3050:       NOTRUN -> FAIL
    - fi-blb-e6850:       NOTRUN -> FAIL
    - fi-bsw-kefka:       NOTRUN -> FAIL
    - fi-hsw-4770r:       NOTRUN -> FAIL
    - fi-byt-clapper:     NOTRUN -> FAIL
    - fi-bdw-5557u:       NOTRUN -> FAIL
    - fi-bwr-2160:        NOTRUN -> FAIL
    - fi-byt-n2820:       NOTRUN -> FAIL
    - fi-elk-e7500:       NOTRUN -> FAIL

  
Known issues
------------

  Here are the changes found in Patchwork_12795 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@runner@aborted:
    - fi-cfl-8109u:       NOTRUN -> FAIL [k.org#202107] / [k.org#202109]
    - fi-glk-dsi:         NOTRUN -> FAIL [k.org#202321]
    - fi-bxt-j4205:       NOTRUN -> FAIL [fdo#109516]
    - fi-bxt-dsi:         NOTRUN -> FAIL [fdo#109516]
    - fi-skl-iommu:       NOTRUN -> FAIL [fdo#104108]
    - fi-cfl-guc:         NOTRUN -> FAIL [k.org#202107] / [k.org#202109]
    - fi-skl-guc:         NOTRUN -> FAIL [fdo#104108]
    - fi-skl-6700k2:      NOTRUN -> FAIL [fdo#104108]
    - fi-kbl-x1275:       NOTRUN -> FAIL [fdo#108903] / [fdo#108904] / [fdo#108905]
    - fi-cfl-8700k:       NOTRUN -> FAIL [k.org#202107] / [k.org#202109]
    - fi-skl-6600u:       NOTRUN -> FAIL [fdo#104108]
    - fi-skl-lmem:        NOTRUN -> FAIL [fdo#104108]
    - fi-apl-guc:         NOTRUN -> FAIL [fdo#109373]
    - fi-kbl-r:           NOTRUN -> FAIL [fdo#108903] / [fdo#108904] / [fdo#108905]
    - fi-skl-6770hq:      NOTRUN -> FAIL [fdo#104108]
    - fi-skl-gvtdvm:      NOTRUN -> FAIL [fdo#104108]
    - fi-snb-2600:        NOTRUN -> FAIL [fdo#108929]
    - fi-skl-6260u:       NOTRUN -> FAIL [fdo#104108]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#108903]: https://bugs.freedesktop.org/show_bug.cgi?id=108903
  [fdo#108904]: https://bugs.freedesktop.org/show_bug.cgi?id=108904
  [fdo#108905]: https://bugs.freedesktop.org/show_bug.cgi?id=108905
  [fdo#108929]: https://bugs.freedesktop.org/show_bug.cgi?id=108929
  [fdo#109373]: https://bugs.freedesktop.org/show_bug.cgi?id=109373
  [fdo#109516]: https://bugs.freedesktop.org/show_bug.cgi?id=109516
  [k.org#202107]: https://bugzilla.kernel.org/show_bug.cgi?id=202107
  [k.org#202109]: https://bugzilla.kernel.org/show_bug.cgi?id=202109
  [k.org#202321]: https://bugzilla.kernel.org/show_bug.cgi?id=202321
  [k.org#202973]: https://bugzilla.kernel.org/show_bug.cgi?id=202973


Participating hosts (47 -> 45)
------------------------------

  Additional (2): fi-icl-u2 fi-gdg-551 
  Missing    (4): fi-kbl-soraka fi-byt-squawks fi-bsw-cyan fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5933 -> Patchwork_12795

  CI_DRM_5933: 6ba1335cf9dff71bed39ca38565e23a4d008614c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4945: a52cc643cfe6733465cfc9ccb3d21cbdc4fd7506 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12795: 4d127259db0fd599f4845d02b70d454634a5ac20 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

4d127259db0f drm/i915: Add intel_compare_color_lut() to compare hw and sw gamma lut values
b4e5942038d9 drm/i915: Enable intel_get_color_config()
c4b9331e9b26 drm/i915: Extract ilk_get_color_config()
85fd57b4994b drm/i915: Extract ivb_get_color_config()
6cd82ce2f4a1 drm/i915: Extract bdw_get_color_config()
f0350da652e9 drm/i915: Extract glk_get_color_config()
950b0424cebd drm/i915: Extract icl_get_color_config()
4e1026ef5692 drm/i915: Extract i965_get_color_config()
ed916c9dbb95 drm/i915: Extract cherryview_get_color_config()
2c1fbf49d4d5 drm/i915: Extract i9xx_get_color_config()
37affd023b33 drm/i915: Introduce vfunc intel_get_color_config to create hw lut

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12795/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/11] [v3] drm/i915: Introduce vfunc intel_get_color_config to create hw lut

[Jani Nikula]

On Mon, 15 Apr 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> v3: Rebase
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    | 1 +
>  drivers/gpu/drm/i915/intel_color.c | 7 +++++++
>  drivers/gpu/drm/i915/intel_color.h | 1 +
>  3 files changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 63eca30..69ed05c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -342,6 +342,7 @@ struct drm_i915_display_funcs {
>  	 * involved with the same commit.
>  	 */
>  	void (*load_luts)(const struct intel_crtc_state *crtc_state);
> +	void (*get_color_config)(struct intel_crtc_state *crtc_state);
>  };
>  
>  #define CSR_VERSION(major, minor)	((major) << 16 | (minor))
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index ca341a9..321cf52 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -857,6 +857,13 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  	return dev_priv->display.color_check(crtc_state);
>  }
>  
> +void intel_get_color_config(struct intel_crtc_state *crtc_state)

Please call it intel_color_get_config.

Going forward, I'm going to push towards prefixing functions based on
the source filename. intel_foo.c -> intel_foo_bar().

You're not using this function anywhere... you only add that in patch
10/11. I'd rather see you add the user early on, and add the support
platform by platform incrementally.

> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> +
> +	dev_priv->display.get_color_config(crtc_state);

I.e. wrap this in if (dev_priv->display.get_color_config) and you'll be
able to call the function right away, regardless of the platform.

BR,
Jani.

> +}
> +
>  static bool need_plane_update(struct intel_plane *plane,
>  			      const struct intel_crtc_state *crtc_state)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_color.h b/drivers/gpu/drm/i915/intel_color.h
> index b8a3ce6..7ca304f 100644
> --- a/drivers/gpu/drm/i915/intel_color.h
> +++ b/drivers/gpu/drm/i915/intel_color.h
> @@ -13,5 +13,6 @@
>  int intel_color_check(struct intel_crtc_state *crtc_state);
>  void intel_color_commit(const struct intel_crtc_state *crtc_state);
>  void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
> +void intel_get_color_config(struct intel_crtc_state *crtc_state);
>  
>  #endif /* __INTEL_COLOR_H__ */

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/11] [v2] drm/i915: Enable intel_get_color_config()

[Jani Nikula]

On Mon, 15 Apr 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>

Please add a commit message, and make this patch #2 in the series after
the changes I suggested in patch #1.

BR,
Jani.

> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f29a348..0fc9dab 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8275,6 +8275,7 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  		pipe_config->cgm_mode = I915_READ(CGM_PIPE_MODE(crtc->pipe));
>  
>  	i9xx_get_pipe_color_config(pipe_config);
> +	intel_get_color_config(pipe_config);
>  
>  	if (INTEL_GEN(dev_priv) < 4)
>  		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
> @@ -9348,6 +9349,7 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  	pipe_config->csc_mode = I915_READ(PIPE_CSC_MODE(crtc->pipe));
>  
>  	i9xx_get_pipe_color_config(pipe_config);
> +	intel_get_color_config(pipe_config);
>  
>  	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
>  		struct intel_shared_dpll *pll;
> @@ -10011,6 +10013,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  			pipe_config->csc_enable = true;
>  	} else {
>  		i9xx_get_pipe_color_config(pipe_config);
> +		intel_get_color_config(pipe_config);
>  	}
>  
>  	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/11] [v3] drm/i915: Add intel_compare_color_lut() to compare hw and sw gamma lut values

[Jani Nikula]

On Mon, 15 Apr 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> v3: Rebase
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c   | 49 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color.h   |  6 +++++
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++
>  3 files changed, 65 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index e2703f9..867c1de 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -1500,6 +1500,55 @@ static void ilk_get_color_config(struct intel_crtc_state *crtc_state)
>  		ilk_get_gamma_config(crtc_state);
>  }
>  
> +static inline bool gamma_err_check(struct drm_color_lut *sw_lut, struct drm_color_lut *hw_lut, u32 err)
> +{
> +	 return ((abs((long)hw_lut->red - sw_lut->red)) >= err) ||
> +	        ((abs((long)hw_lut->blue - sw_lut->blue)) >= err) ||
> +	        ((abs((long)hw_lut->green - sw_lut->green)) >= err);
> +}
> +
> +bool intel_compare_color_lut(struct drm_property_blob *blob1,
> +			     struct drm_property_blob *blob2,
> +			     u32 gamma_mode)

Please prefix the function name with intel_color_ like I mentioned
earlier in the series.

I don't think it's obvious what a boolean function called "compare"
should return. I see intel_compare_link_m_n() and
intel_compare_infoframe() I think they're equally bad.

intel_color_lut_match() or intel_color_lut_equal() would be obvious.

The C strcmp() is the notorious example where 0 means equal.

> +{
> +	struct drm_color_lut *sw_lut = blob1->data;
> +	struct drm_color_lut *hw_lut = blob2->data;
> +	int sw_lut_size, hw_lut_size, i;
> +	u32 bit_precision, err;
> +
> +	if (!blob1 || !blob2)
> +		return false;
> +
> +	switch(gamma_mode) {
> +	case GAMMA_MODE_MODE_8BIT:
> +		bit_precision = 8;
> +		break;
> +	case GAMMA_MODE_MODE_10BIT:
> +		bit_precision = 10;
> +		break;
> +	case GAMMA_MODE_MODE_12BIT:
> +		bit_precision = 12;
> +		break;
> +	default:

It's customary to just add the default label above the correct label
above.

BR,
Jani.

> +		bit_precision = 8;
> +	}
> +
> +	err = 0xffff >> bit_precision;
> +
> +	sw_lut_size = drm_color_lut_size(blob1);
> +	hw_lut_size = drm_color_lut_size(blob2);
> +
> +	if (sw_lut_size != hw_lut_size)
> +		return false;
> +
> +	for (i = 0; i < sw_lut_size; i++) {
> +		 if (!gamma_err_check(&hw_lut[i], &sw_lut[i], err))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  void intel_color_init(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> diff --git a/drivers/gpu/drm/i915/intel_color.h b/drivers/gpu/drm/i915/intel_color.h
> index 7ca304f..575003f 100644
> --- a/drivers/gpu/drm/i915/intel_color.h
> +++ b/drivers/gpu/drm/i915/intel_color.h
> @@ -6,13 +6,19 @@
>  #ifndef __INTEL_COLOR_H__
>  #define __INTEL_COLOR_H__
>  
> +#include <linux/types.h>
> +
>  struct intel_crtc_state;
>  struct intel_crtc;
> +struct drm_property_blob;
>  
>  void intel_color_init(struct intel_crtc *crtc);
>  int intel_color_check(struct intel_crtc_state *crtc_state);
>  void intel_color_commit(const struct intel_crtc_state *crtc_state);
>  void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
>  void intel_get_color_config(struct intel_crtc_state *crtc_state);
> +bool intel_compare_color_lut(struct drm_property_blob *blob1,
> +			     struct drm_property_blob *blob2,
> +			     u32 gamma_mode);
>  
>  #endif /* __INTEL_COLOR_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0fc9dab..b074f00 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12236,6 +12236,14 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  	} \
>  } while (0)
>  
> +#define PIPE_CONF_CHECK_COLOR_LUT(name, gamma_mode) do { \
> +	if (!intel_compare_color_lut(current_config->name, pipe_config->name, gamma_mode)) { \
> +		pipe_config_err(adjust, __stringify(name), \
> +				"hw_state doesn't match sw_state\n"); \
> +		ret = false; \
> +	} \
> +} while (0)
> +
>  #define PIPE_CONF_QUIRK(quirk) \
>  	((current_config->quirks | pipe_config->quirks) & (quirk))
>  
> @@ -12379,6 +12387,8 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  	PIPE_CONF_CHECK_INFOFRAME(spd);
>  	PIPE_CONF_CHECK_INFOFRAME(hdmi);
>  
> +	PIPE_CONF_CHECK_COLOR_LUT(base.gamma_lut, pipe_config->gamma_mode);
> +
>  #undef PIPE_CONF_CHECK_X
>  #undef PIPE_CONF_CHECK_I
>  #undef PIPE_CONF_CHECK_BOOL

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/11] [v2] drm/i915: Extract i9xx_get_color_config()

[Jani Nikula]

On Mon, 15 Apr 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  3 +++
>  drivers/gpu/drm/i915/intel_color.c | 51 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9c206e8..8f2ae8a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7175,6 +7175,9 @@ enum {
>  /* legacy palette */
>  #define _LGC_PALETTE_A           0x4a000
>  #define _LGC_PALETTE_B           0x4a800
> +#define LGC_PALETTE_RED_MASK     REG_GENMASK(23, 16)
> +#define LGC_PALETTE_GREEN_MASK   REG_GENMASK(15, 8)
> +#define LGC_PALETTE_BLUE_MASK    REG_GENMASK(7, 0)
>  #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) + (i) * 4)
>  
>  /* ilk/snb precision palette */
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 321cf52..f394402 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -1228,6 +1228,56 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +/* convert hw value with given bit_precision to lut property val */
> +static u32 intel_color_lut_pack(u32 val, u32 bit_precision)
> +{
> +	u32 max = 0xffff >> (16 - bit_precision);
> +
> +	val = clamp_val(val, 0, max);
> +
> +	if (bit_precision < 16)
> +		val <<= 16 - bit_precision;
> +
> +	return val;
> +}
> +
> +static void i9xx_get_config_internal(struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	struct drm_property_blob *blob = NULL;

No need to initialize.

> +	struct drm_color_lut *blob_data;
> +	u32 i, val;
> +
> +	blob = drm_property_create_blob(dev,

You can drop the dev local var as it's simply &dev_priv->drm.

> +					sizeof(struct drm_color_lut) * 256,
> +					NULL);
> +	if (IS_ERR(blob))
> +		return;
> +
> +	blob_data = blob->data;
> +
> +	for (i = 0; i < 256; i++) {
> +		if (HAS_GMCH(dev_priv))
> +			val = I915_READ(PALETTE(pipe, i));
> +		else
> +			val = I915_READ(LGC_PALETTE(pipe, i));
> +
> +		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_RED_MASK, val) >> 16, 8);
> +		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_GREEN_MASK, val) >> 8, 8);
> +		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_BLUE_MASK, val), 8);

REG_FIELD_GET() return the fields already shifted down according to the
mask. So the right shifts here are not needed.

> +	}
> +
> +	crtc_state->base.gamma_lut = blob;
> +}
> +
> +static void i9xx_get_color_config(struct intel_crtc_state *crtc_state)
> +{
> +	i9xx_get_config_internal(crtc_state);
> +}
> +
>  void intel_color_init(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -1248,6 +1298,7 @@ void intel_color_init(struct intel_crtc *crtc)
>  			dev_priv->display.color_check = i9xx_color_check;
>  			dev_priv->display.color_commit = i9xx_color_commit;
>  			dev_priv->display.load_luts = i9xx_load_luts;
> +			dev_priv->display.get_color_config = i9xx_get_color_config;
>  		}
>  	} else {
>  		if (INTEL_GEN(dev_priv) >= 11)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/11] [v2] drm/i915: Extract i9xx_get_color_config()

[Jani Nikula]

On Tue, 16 Apr 2019, Jani Nikula <jani.nikula@intel.com> wrote:
> On Mon, 15 Apr 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h    |  3 +++
>>  drivers/gpu/drm/i915/intel_color.c | 51 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 9c206e8..8f2ae8a 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7175,6 +7175,9 @@ enum {
>>  /* legacy palette */
>>  #define _LGC_PALETTE_A           0x4a000
>>  #define _LGC_PALETTE_B           0x4a800
>> +#define LGC_PALETTE_RED_MASK     REG_GENMASK(23, 16)
>> +#define LGC_PALETTE_GREEN_MASK   REG_GENMASK(15, 8)
>> +#define LGC_PALETTE_BLUE_MASK    REG_GENMASK(7, 0)
>>  #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) + (i) * 4)
>>  
>>  /* ilk/snb precision palette */
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index 321cf52..f394402 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -1228,6 +1228,56 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>>  	return 0;
>>  }
>>  
>> +/* convert hw value with given bit_precision to lut property val */
>> +static u32 intel_color_lut_pack(u32 val, u32 bit_precision)
>> +{
>> +	u32 max = 0xffff >> (16 - bit_precision);
>> +
>> +	val = clamp_val(val, 0, max);
>> +
>> +	if (bit_precision < 16)
>> +		val <<= 16 - bit_precision;
>> +
>> +	return val;
>> +}
>> +
>> +static void i9xx_get_config_internal(struct intel_crtc_state *crtc_state)
>> +{
>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> +	struct drm_device *dev = crtc->base.dev;
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	enum pipe pipe = crtc->pipe;
>> +	struct drm_property_blob *blob = NULL;
>
> No need to initialize.
>
>> +	struct drm_color_lut *blob_data;
>> +	u32 i, val;
>> +
>> +	blob = drm_property_create_blob(dev,
>
> You can drop the dev local var as it's simply &dev_priv->drm.
>
>> +					sizeof(struct drm_color_lut) * 256,
>> +					NULL);
>> +	if (IS_ERR(blob))
>> +		return;
>> +
>> +	blob_data = blob->data;
>> +
>> +	for (i = 0; i < 256; i++) {
>> +		if (HAS_GMCH(dev_priv))
>> +			val = I915_READ(PALETTE(pipe, i));
>> +		else
>> +			val = I915_READ(LGC_PALETTE(pipe, i));
>> +
>> +		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_RED_MASK, val) >> 16, 8);
>> +		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_GREEN_MASK, val) >> 8, 8);
>> +		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_BLUE_MASK, val), 8);
>
> REG_FIELD_GET() return the fields already shifted down according to the
> mask. So the right shifts here are not needed.

So the same comments apply to the rest of the platform specific get
config patches in the series, I'm not going to repeat them individually.

BR,
Jani.


>
>> +	}
>> +
>> +	crtc_state->base.gamma_lut = blob;
>> +}
>> +
>> +static void i9xx_get_color_config(struct intel_crtc_state *crtc_state)
>> +{
>> +	i9xx_get_config_internal(crtc_state);
>> +}
>> +
>>  void intel_color_init(struct intel_crtc *crtc)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> @@ -1248,6 +1298,7 @@ void intel_color_init(struct intel_crtc *crtc)
>>  			dev_priv->display.color_check = i9xx_color_check;
>>  			dev_priv->display.color_commit = i9xx_color_commit;
>>  			dev_priv->display.load_luts = i9xx_load_luts;
>> +			dev_priv->display.get_color_config = i9xx_get_color_config;
>>  		}
>>  	} else {
>>  		if (INTEL_GEN(dev_priv) >= 11)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/11] [v3] drm/i915: Add intel_compare_color_lut() to compare hw and sw gamma lut values

[Dan Carpenter]

Hi Swati,

Thank you for the patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Swati-Sharma/drm-i915-adding-state-checker-for-gamma-lut-values/20190416-021708
base:   git://anongit.freedesktop.org/drm-intel for-linux-next

New smatch warnings:
drivers/gpu/drm/i915/intel_color.c:1519 intel_compare_color_lut() warn: variable dereferenced before check 'blob1' (see line 1514)
drivers/gpu/drm/i915/intel_color.c:1519 intel_compare_color_lut() warn: variable dereferenced before check 'blob2' (see line 1515)

# https://github.com/0day-ci/linux/commit/14a61c5a3c40291ad8779909b5fdc261aad53df3
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 14a61c5a3c40291ad8779909b5fdc261aad53df3
vim +/blob1 +1519 drivers/gpu/drm/i915/intel_color.c

14a61c5a Swati Sharma 2019-04-15  1510  bool intel_compare_color_lut(struct drm_property_blob *blob1,
14a61c5a Swati Sharma 2019-04-15  1511  			     struct drm_property_blob *blob2,
14a61c5a Swati Sharma 2019-04-15  1512  			     u32 gamma_mode)
14a61c5a Swati Sharma 2019-04-15  1513  {
14a61c5a Swati Sharma 2019-04-15 @1514  	struct drm_color_lut *sw_lut = blob1->data;
                                                                               ^^^^^^^^^^^
14a61c5a Swati Sharma 2019-04-15 @1515  	struct drm_color_lut *hw_lut = blob2->data;
                                                                               ^^^^^^^^^^^
14a61c5a Swati Sharma 2019-04-15  1516  	int sw_lut_size, hw_lut_size, i;
14a61c5a Swati Sharma 2019-04-15  1517  	u32 bit_precision, err;
14a61c5a Swati Sharma 2019-04-15  1518  
14a61c5a Swati Sharma 2019-04-15 @1519  	if (!blob1 || !blob2)
                                                     ^^^^^    ^^^^^^

14a61c5a Swati Sharma 2019-04-15  1520  		return false;
14a61c5a Swati Sharma 2019-04-15  1521  
14a61c5a Swati Sharma 2019-04-15  1522  	switch(gamma_mode) {
14a61c5a Swati Sharma 2019-04-15  1523  	case GAMMA_MODE_MODE_8BIT:
14a61c5a Swati Sharma 2019-04-15  1524  		bit_precision = 8;
14a61c5a Swati Sharma 2019-04-15  1525  		break;