[RFC] drm/i915: adding state checker for gamma lut values

[swati2.sharma@intel.com]

From: Swati Sharma <swati2.sharma@intel.com>

Added state checker to validate gamma_lut values. This
reads hardware state, and compares the originally requested
state to the state read from hardware.

This implementation can be used for Gen9+ platforms,
I haven't implemented it for legacy platforms. Just want to get
feedback if this is the right approach to follow.

Also, inverse function of drm_color_lut_extract is missing
to convert hardware read values back to user values.
Thinking for that. I have added all "TODOs" and "Placeholders".

Another approach:
Storing "word" to be written into hardware in dev_priv and
instead of comparing blob, comparing "word"? In dev_priv,
only pointer will be there (something like *gamma_word).

For this too, I will send a patch to make it more clear.

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_color.c   | 127 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_display.c |  50 ++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |   1 +
 4 files changed, 173 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c4ffe19..b41bfaa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -334,6 +334,7 @@ struct drm_i915_display_funcs {
 	 * involved with the same commit.
 	 */
 	void (*load_luts)(const struct intel_crtc_state *crtc_state);
+	void (*get_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 da7a07d..a515e9f 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -74,6 +74,11 @@
 #define ILK_CSC_COEFF_1_0		\
 	((7 << 12) | ILK_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
 
+/* Mask to extract RGB values from registers */
+#define COLOR_BLUE_MASK         0x000003FF              /* 9:0 */
+#define COLOR_GREEN_MASK        0x000FFC00              /* 19:10 */
+#define COLOR_RED_MASK          0x3FF00000              /* 29:20 */
+
 static bool lut_is_legacy(const struct drm_property_blob *lut)
 {
 	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
@@ -672,6 +677,97 @@ static void cherryview_load_luts(const struct intel_crtc_state *crtc_state)
 	i9xx_load_luts_internal(crtc_state, NULL);
 }
 
+static void bdw_get_gamma_config(struct intel_crtc_state *crtc_state, u32 offset)
+{
+	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, tmp, 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;
+
+	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
+
+	I915_WRITE(PREC_PAL_INDEX(pipe),
+			(offset ? PAL_PREC_SPLIT_MODE : 0) |
+			PAL_PREC_AUTO_INCREMENT |
+			offset);
+
+	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; i++) {
+		tmp = I915_READ(PREC_PAL_DATA(pipe));
+		/*
+		 * TODO: convert RGB value read from register into corresponding user value using
+		 * some wrapper like drm_color_lut_put() (or) intel_color_lut_put() so that it
+		 * can be compared later.
+		 */
+		blob_data[i].red = (tmp & COLOR_RED_MASK) >> 20;
+		blob_data[i].green = (tmp & COLOR_GREEN_MASK) >> 10;
+		blob_data[i].blue = tmp & COLOR_BLUE_MASK;
+	}
+
+	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
+
+	crtc_state->base.gamma_lut = blob;
+}
+
+static void i9xx_get_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, tmp;
+	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 < 256; i++) {
+		if (HAS_GMCH(dev_priv))
+			tmp = I915_READ(PALETTE(pipe, i));
+		else
+			tmp = I915_READ(LGC_PALETTE(pipe, i));
+		/*
+		 * TODO: convert RGB value read from register into corresponding user value using
+		 * some wrapper like drm_color_lut_put() (or) intel_color_lut_put() so that it
+		 * can be compared later.
+		 */
+		blob_data[i].red = (tmp & COLOR_RED_MASK) >> 20;
+		blob_data[i].green = (tmp & COLOR_GREEN_MASK) >> 10;
+		blob_data[i].blue = (tmp & COLOR_BLUE_MASK);
+	}
+
+	crtc_state->base.gamma_lut = blob;
+}
+
+static void icl_get_config(struct intel_crtc_state *crtc_state)
+{
+	/*
+	 * TODO: placeholder for degamma validation
+	 * glk_get_degamma_config(crtc_state, 0);
+	 */
+
+	if (crtc_state_is_legacy_gamma(crtc_state))
+		i9xx_get_config(crtc_state);
+	else
+		bdw_get_gamma_config(crtc_state, 0);
+}
+
 void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
@@ -679,6 +775,13 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
 	dev_priv->display.load_luts(crtc_state);
 }
 
+void intel_color_get_config(struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+
+	dev_priv->display.get_config(crtc_state);
+}
+
 void intel_color_commit(const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
@@ -833,22 +936,34 @@ void intel_color_init(struct intel_crtc *crtc)
 
 	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
 
+	/*
+	 * TODO: In the following if ladder, kept placeholders
+	 * for extending lut validation for legacy platforms.
+	 */
 	if (HAS_GMCH(dev_priv)) {
-		if (IS_CHERRYVIEW(dev_priv))
+		if (IS_CHERRYVIEW(dev_priv)) {
 			dev_priv->display.load_luts = cherryview_load_luts;
-		else
+			 /* dev_priv->display.get_config = cherryview_get_config; */
+		} else {
 			dev_priv->display.load_luts = i9xx_load_luts;
+			dev_priv->display.get_config = i9xx_get_config;
+		}
 
 		dev_priv->display.color_commit = i9xx_color_commit;
 	} else {
-		if (IS_ICELAKE(dev_priv))
+		if (IS_ICELAKE(dev_priv)) {
 			dev_priv->display.load_luts = icl_load_luts;
-		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
+			dev_priv->display.get_config = icl_get_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) >= 9 || IS_BROADWELL(dev_priv))
+			/* dev_priv->display.get_config = glk_get_config; */
+		} else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) {
 			dev_priv->display.load_luts = broadwell_load_luts;
-		else
+			/* dev_priv->display.get_config = broadwell_get_config; */
+		} else {
 			dev_priv->display.load_luts = i9xx_load_luts;
+			dev_priv->display.get_config = i9xx_get_config;
+		}
 
 		if (INTEL_GEN(dev_priv) >= 9)
 			dev_priv->display.color_commit = skl_color_commit;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 963b4bd..165d797 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8212,6 +8212,11 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 
 	i9xx_get_pipe_color_config(pipe_config);
 
+	/*
+	 * TODO: Yet to implement for legacy platforms
+	 * intel_color_get_config(pipe_config);
+	 */
+
 	if (INTEL_GEN(dev_priv) < 4)
 		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
 
@@ -9284,6 +9289,11 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 
 	i9xx_get_pipe_color_config(pipe_config);
 
+	/*
+	 * TODO: Yet to implement for legacy platforms
+	 * intel_color_get_config(pipe_config);
+	 */
+
 	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
 		struct intel_shared_dpll *pll;
 		enum intel_dpll_id pll_id;
@@ -9932,6 +9942,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 		i9xx_get_pipe_color_config(pipe_config);
 	}
 
+	intel_color_get_config(pipe_config);
+
 	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
 	if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
 		WARN_ON(power_domain_mask & BIT_ULL(power_domain));
@@ -11990,6 +12002,34 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 	return false;
 }
 
+static bool intel_compare_blob(struct drm_property_blob *blob1,
+			       struct drm_property_blob *blob2)
+{
+	struct drm_color_lut *sw_lut = blob1->data;
+	struct drm_color_lut *hw_lut = blob2->data;
+	int sw_lut_size, hw_lut_size;
+	int i;
+
+	sw_lut_size = drm_color_lut_size(blob1);
+	hw_lut_size = drm_color_lut_size(blob2);
+
+	if (sw_lut_size != hw_lut_size) {
+		DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n", hw_lut_size, sw_lut_size);
+		return false;
+	}
+
+	for (i = 0; i < sw_lut_size; i++) {
+		if (hw_lut[i].red != sw_lut[i].red ||
+		    hw_lut[i].blue != sw_lut[i].blue ||
+		    hw_lut[i].green != sw_lut[i].green) {
+			DRM_DEBUG_KMS("Invalid LUT value; sw_lut value doesn't match hw_lut value\n");
+			return false;
+		}
+	}
+
+	return true;
+}
+
 static bool
 intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 			  struct intel_crtc_state *current_config,
@@ -12148,6 +12188,14 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 	} \
 } while (0)
 
+#define PIPE_CONF_CHECK_BLOB(name) do { \
+	if (!intel_compare_blob(current_config->name, pipe_config->name)) { \
+		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))
 
@@ -12287,6 +12335,8 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 	PIPE_CONF_CHECK_INFOFRAME(spd);
 	PIPE_CONF_CHECK_INFOFRAME(hdmi);
 
+	PIPE_CONF_CHECK_BLOB(base.gamma_lut);
+
 #undef PIPE_CONF_CHECK_X
 #undef PIPE_CONF_CHECK_I
 #undef PIPE_CONF_CHECK_BOOL
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 40ebc94..a6acd9b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2512,6 +2512,7 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_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_color_get_config(struct intel_crtc_state *crtc_state);
 
 /* intel_lspcon.c */
 bool lspcon_init(struct intel_digital_port *intel_dig_port);
-- 
1.9.1

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