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

All of lore.kernel.org
 help / color / mirror / Atom feed

* [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values
@ 2019-08-30  8:31 Swati Sharma
  2019-08-30  8:31 ` [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision Swati Sharma
                   ` (14 more replies)
  0 siblings, 15 replies; 28+ messages in thread
From: Swati Sharma @ 2019-08-30  8:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, daniel.vetter, ankit.k.nautiyal

In this patch series, added state checker to validate gamma
(8BIT and 10BIT).This reads hardware state, and compares the originally
requested state(s/w) to the state read from the hardware.
This is done for legacy, i965, ilk, glk and their variant platforms. 

Intentionally, excluded bdw and ivb since they have spilt gamma mode;
for which degamma read outs are required (which I think shouldn't be
included in this patch series). Will include after degamma state checker
is completed.

v1: -Implementation done for legacy platforms
     (removed all the placeholders) (Jani)
v2: -Restructured code and created platform specific patch series for 
     gamma validation
v3: -Rebase
v4: -Minor changes-function name changes mainly
v5: -Added degamma validation (Ville)
v6: -Removed degamma changes, debugging was becoming difficult
    -Added function to assign bit_precision for gamma/degamma
     lut values /platform
    -Added debug info into intel_dump_pipe_config() (Jani)
v7: -Added platform specific functions to compute gamma bit precision
     on the basis of GAMMA_MODE (Ville)
    -Corrected checkpatch warnings
v8: -Restructured code
    -Removed bdw and ivb platform state checker
v9: -Obliged 80 character word limit [Uma]
    -Added state checker for icl
    -Added bit precision func for icl 

Swati Sharma (11):
  drm/i915/display: Add func to get gamma bit precision
  drm/i915/display: Add debug log for color parameters
  drm/i915/display: Add func to compare hw/sw gamma lut
  drm/i915/display: Add macro to compare gamma hw/sw lut
  drm/i915/display: Extract i9xx_read_luts()
  drm/i915/display: Extract i965_read_luts()
  drm/i915/display: Extract chv_read_luts()
  drm/i915/display: Extract ilk_read_luts()
  drm/i915/display: Extract glk_read_luts()
  drm/i915/display: Extract icl_read_luts()
  FOR_TESTING_ONLY: Print rgb values of hw and sw blobs

 drivers/gpu/drm/i915/display/intel_color.c   | 523 ++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_color.h   |   7 +
 drivers/gpu/drm/i915/display/intel_display.c |  34 ++
 drivers/gpu/drm/i915/i915_reg.h              |  21 ++
 4 files changed, 577 insertions(+), 8 deletions(-)

-- 
1.9.1

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
@ 2019-08-30  8:31 ` Swati Sharma
  2019-08-30 12:02   ` Jani Nikula
  2019-08-30  8:31 ` [v9][PATCH 02/11] drm/i915/display: Add debug log for color parameters Swati Sharma
                   ` (13 subsequent siblings)
  14 siblings, 1 reply; 28+ messages in thread
From: Swati Sharma @ 2019-08-30  8:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, daniel.vetter, ankit.k.nautiyal

Each platform supports different gamma modes and each gamma mode
has different bit precision. Here bit precision corresponds
to number of bits the hw LUT supports.

Add func per platform to return bit precision corresponding to gamma mode
which will be later used as a parameter in lut comparison function
intel_color_lut_equal().

This is done for legacy, i965, ilk, glk, icl and their variant platforms.

v6: -Added func intel_color_get_bit_precision() to get bit precision for
     gamma and degamma lut readout depending upon platform and
     corresponding to load_luts() [Ankit]
    -Made patch11 as patch3 [Jani]
v7: -Renamed func intel_color_get_bit_precision() to
     intel_color_get_gamma_bit_precision()
    -Added separate function/platform for gamma bit precision [Ville]
    -Corrected checkpatch warnings
v8: -Split patch 3 into 4 separate patches
v9: -Changed commit message, gave more info [Uma]
    -Added precision func for icl+ platform

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

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 71a0201..dcc65d7 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1371,6 +1371,105 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
+static int i9xx_gamma_precision(const struct intel_crtc_state *crtc_state)
+{
+	if (!crtc_state->gamma_enable)
+		return 0;
+
+	switch (crtc_state->gamma_mode) {
+	case GAMMA_MODE_MODE_8BIT:
+		return 8;
+	case GAMMA_MODE_MODE_10BIT:
+		return 16;
+	default:
+		MISSING_CASE(crtc_state->gamma_mode);
+		return 0;
+	}
+}
+
+static int ilk_gamma_precision(const struct intel_crtc_state *crtc_state)
+{
+	if (!crtc_state->gamma_enable)
+		return 0;
+
+	if ((crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) == 0)
+		return 0;
+
+	switch (crtc_state->gamma_mode) {
+	case GAMMA_MODE_MODE_8BIT:
+		return 8;
+	case GAMMA_MODE_MODE_10BIT:
+		return 10;
+	default:
+		MISSING_CASE(crtc_state->gamma_mode);
+		return 0;
+	}
+}
+
+static int chv_gamma_precision(const struct intel_crtc_state *crtc_state)
+{
+	if (crtc_state->cgm_mode & CGM_PIPE_MODE_GAMMA)
+		return 10;
+	else
+		return i9xx_gamma_precision(crtc_state);
+}
+
+static int glk_gamma_precision(const struct intel_crtc_state *crtc_state)
+{
+	if (!crtc_state->gamma_enable)
+		return 0;
+
+	switch (crtc_state->gamma_mode) {
+	case GAMMA_MODE_MODE_8BIT:
+		return 8;
+	case GAMMA_MODE_MODE_10BIT:
+		return 10;
+	default:
+		MISSING_CASE(crtc_state->gamma_mode);
+		return 0;
+	}
+}
+
+static int icl_gamma_precision(const struct intel_crtc_state *crtc_state)
+{
+	if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0)
+		return 0;
+
+	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
+	case GAMMA_MODE_MODE_8BIT:
+		return 8;
+	case GAMMA_MODE_MODE_10BIT:
+		return 10;
+	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
+		return 16;
+	default:
+		MISSING_CASE(crtc_state->gamma_mode);
+		return 0;
+	}
+}
+
+int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+
+	if (HAS_GMCH(dev_priv)) {
+		if (IS_CHERRYVIEW(dev_priv))
+			return chv_gamma_precision(crtc_state);
+		else
+			return i9xx_gamma_precision(crtc_state);
+	} else {
+		if (INTEL_GEN(dev_priv) >= 11)
+			return icl_gamma_precision(crtc_state);
+		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
+			return glk_gamma_precision(crtc_state);
+		else if (IS_IRONLAKE(dev_priv))
+			return ilk_gamma_precision(crtc_state);
+	}
+
+	return 0;
+}
+
 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/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h
index 057e8ac..0226d3a 100644
--- a/drivers/gpu/drm/i915/display/intel_color.h
+++ b/drivers/gpu/drm/i915/display/intel_color.h
@@ -14,5 +14,6 @@
 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);
+int intel_color_get_gamma_bit_precision(const 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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [v9][PATCH 02/11] drm/i915/display: Add debug log for color parameters
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
  2019-08-30  8:31 ` [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision Swati Sharma
@ 2019-08-30  8:31 ` Swati Sharma
  2019-08-30 12:06   ` Jani Nikula
  2019-08-30  8:31 ` [v9][PATCH 03/11] drm/i915/display: Add func to compare hw/sw gamma lut Swati Sharma
                   ` (12 subsequent siblings)
  14 siblings, 1 reply; 28+ messages in thread
From: Swati Sharma @ 2019-08-30  8:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, daniel.vetter, ankit.k.nautiyal

Add debug log for color related parameters like gamma_mode, gamma_enable,
csc_enable, etc inside intel_dump_pipe_config().

v6: -Added debug log for color para in intel_dump_pipe_config [Jani]
v7: -Split patch 3 into 4 patches
v8: -Corrected alignment [Uma]

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

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index ea2915d..f9c0842 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -12138,6 +12138,15 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
 
 	intel_dpll_dump_hw_state(dev_priv, &pipe_config->dpll_hw_state);
 
+	if (IS_CHERRYVIEW(dev_priv))
+		DRM_DEBUG_KMS("cgm_mode: 0x%x gamma_mode: 0x%x gamma_enable: %d csc_enable: %d\n",
+			      pipe_config->cgm_mode, pipe_config->gamma_mode,
+			      pipe_config->gamma_enable, pipe_config->csc_enable);
+	else
+		DRM_DEBUG_KMS("csc_mode: 0x%x gamma_mode: 0x%x gamma_enable: %d csc_enable: %d\n",
+			      pipe_config->csc_mode, pipe_config->gamma_mode,
+			      pipe_config->gamma_enable, pipe_config->csc_enable);
+
 dump_planes:
 	if (!state)
 		return;
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [v9][PATCH 03/11] drm/i915/display: Add func to compare hw/sw gamma lut
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
  2019-08-30  8:31 ` [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision Swati Sharma
  2019-08-30  8:31 ` [v9][PATCH 02/11] drm/i915/display: Add debug log for color parameters Swati Sharma
@ 2019-08-30  8:31 ` Swati Sharma
  2019-08-30 12:47   ` Jani Nikula
  2019-08-30  8:31 ` [v9][PATCH 04/11] drm/i915/display: Add macro to compare gamma hw/sw lut Swati Sharma
                   ` (11 subsequent siblings)
  14 siblings, 1 reply; 28+ messages in thread
From: Swati Sharma @ 2019-08-30  8:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, daniel.vetter, ankit.k.nautiyal

Add func intel_color_lut_equal() to compare hw/sw gamma
lut values. Since hw/sw gamma lut sizes and lut entries comparison
will be different for different gamma modes, add gamma mode dependent
checks.

v3: -Rebase
v4: -Renamed intel_compare_color_lut() to intel_color_lut_equal() [Jani]
    -Added the default label above the correct label [Jani]
    -Corrected smatch warn "variable dereferenced before check"
     [Dan Carpenter]
v5: -Added condition (!blob1 && !blob2) return true [Jani]
v6: -Made patch11 as patch3 [Jani]
v8: -Split patch 3 into 4 patches
    -Optimized blob check condition [Ville]
v9: -Exclude spilt gamma mode (bdw and ivb platforms)
     as there is exception in way gamma values are written in
     hardware [Ville]
    -Added exception made in commit [Uma]
    -Dropeed else, character limit and indentation [Uma]
    -Added multi segmented gama mode for icl+ platforms [Uma]

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

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index dcc65d7..141efb0 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1470,6 +1470,110 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat
 	return 0;
 }
 
+static inline bool 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);
+}
+
+static inline bool intel_color_lut_entry_equal(struct drm_color_lut *sw_lut,
+					       struct drm_color_lut *hw_lut,
+					       int hw_lut_size, u32 err)
+{
+	int i;
+
+	for (i = 0; i < hw_lut_size; i++) {
+		if (!err_check(&hw_lut[i], &sw_lut[i], err))
+			return false;
+	}
+
+	return true;
+}
+
+static inline bool intel_color_lut_entry_equal_multi(struct drm_color_lut *sw_lut,
+						     struct drm_color_lut *hw_lut,
+						     u32 err)
+{
+	int i;
+
+	for (i = 0; i < 9; i++) {
+		if (!err_check(&hw_lut[i], &sw_lut[i], err))
+			return false;
+	}
+
+	for (i = 1; i <  257; i++) {
+		if (!err_check(&hw_lut[8 + i], &sw_lut[i * 8], err))
+			return false;
+	}
+
+	for (i = 0; i < 256; i++) {
+		if (!err_check(&hw_lut[265 + i], &sw_lut[i * 8 * 128], err))
+			return false;
+	}
+
+	return true;
+}
+
+bool intel_color_lut_equal(struct drm_property_blob *blob1,
+			   struct drm_property_blob *blob2,
+			   u32 gamma_mode, u32 bit_precision)
+{
+	struct drm_color_lut *sw_lut, *hw_lut;
+	int sw_lut_size, hw_lut_size;
+	u32 err;
+
+	if (!blob1 != !blob2)
+		return false;
+
+	if (!blob1)
+		return true;
+
+	sw_lut_size = drm_color_lut_size(blob1);
+	hw_lut_size = drm_color_lut_size(blob2);
+
+	/* check sw and hw lut size */
+	switch (gamma_mode) {
+	case GAMMA_MODE_MODE_8BIT:
+	case GAMMA_MODE_MODE_10BIT:
+		if (sw_lut_size != hw_lut_size)
+			return false;
+		break;
+	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
+		if ((sw_lut_size != 262145) || (hw_lut_size != 521))
+			return false;
+		break;
+	default:
+		MISSING_CASE(gamma_mode);
+			return false;
+	}
+
+	sw_lut = blob1->data;
+	hw_lut = blob2->data;
+
+	err = 0xffff >> bit_precision;
+
+	/* check sw and hw lut entry to be equal */
+	switch (gamma_mode) {
+	case GAMMA_MODE_MODE_8BIT:
+	case GAMMA_MODE_MODE_10BIT:
+		if (!intel_color_lut_entry_equal(sw_lut, hw_lut,
+						 hw_lut_size, err))
+			return false;
+		break;
+	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
+		if (!intel_color_lut_entry_equal_multi(sw_lut, hw_lut, err))
+			return false;
+		break;
+	default:
+		MISSING_CASE(gamma_mode);
+			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/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h
index 0226d3a..173727a 100644
--- a/drivers/gpu/drm/i915/display/intel_color.h
+++ b/drivers/gpu/drm/i915/display/intel_color.h
@@ -6,8 +6,11 @@
 #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);
@@ -15,5 +18,8 @@
 void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
 void intel_color_get_config(struct intel_crtc_state *crtc_state);
 int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state);
+bool intel_color_lut_equal(struct drm_property_blob *blob1,
+			   struct drm_property_blob *blob2,
+			   u32 gamma_mode, u32 bit_precision);
 
 #endif /* __INTEL_COLOR_H__ */
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [v9][PATCH 04/11] drm/i915/display: Add macro to compare gamma hw/sw lut
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
                   ` (2 preceding siblings ...)
  2019-08-30  8:31 ` [v9][PATCH 03/11] drm/i915/display: Add func to compare hw/sw gamma lut Swati Sharma
@ 2019-08-30  8:31 ` Swati Sharma
  2019-08-30  8:31 ` [v9][PATCH 05/11] drm/i915/display: Extract i9xx_read_luts() Swati Sharma
                   ` (10 subsequent siblings)
  14 siblings, 0 replies; 28+ messages in thread
From: Swati Sharma @ 2019-08-30  8:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, daniel.vetter, ankit.k.nautiyal

Add macro to compare hw/sw gamma lut values. First need to
check whether hw/sw gamma mode matches or not. If not
no need to compare lut values, if matches then only compare
lut entries.

v5: -Called PIPE_CONF_CHECK_COLOR_LUT inside if (!adjust) [Jani]
    -Added #undef PIPE_CONF_CHECK_COLOR_LUT [Jani]
v8: -Added check for gamma mode before gamma lut entry comparison
     [Jani]
    -Split patch 3 into 4 patches

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
Reviewed-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index f9c0842..776b365 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -12529,6 +12529,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 {
 	struct drm_i915_private *dev_priv = to_i915(current_config->base.crtc->dev);
 	bool ret = true;
+	u32 bp_gamma = 0;
 	bool fixup_inherited = fastset &&
 		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
 		!(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED);
@@ -12680,6 +12681,24 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 	} \
 } while (0)
 
+#define PIPE_CONF_CHECK_COLOR_LUT(name1, name2, bit_precision) do { \
+	if (current_config->name1 != pipe_config->name1) { \
+		pipe_config_mismatch(fastset, __stringify(name1), \
+				"(expected %i, found %i, won't compare lut values)\n", \
+				current_config->name1, \
+				pipe_config->name1); \
+		ret = false;\
+	} else { \
+		if (!intel_color_lut_equal(current_config->name2, \
+					pipe_config->name2, pipe_config->name1, \
+					bit_precision)) { \
+			pipe_config_mismatch(fastset, __stringify(name2), \
+					"hw_state doesn't match sw_state\n"); \
+			ret = false; \
+		} \
+	} \
+} while (0)
+
 #define PIPE_CONF_QUIRK(quirk) \
 	((current_config->quirks | pipe_config->quirks) & (quirk))
 
@@ -12775,6 +12794,11 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 			PIPE_CONF_CHECK_X(csc_mode);
 		PIPE_CONF_CHECK_BOOL(gamma_enable);
 		PIPE_CONF_CHECK_BOOL(csc_enable);
+
+		bp_gamma = intel_color_get_gamma_bit_precision(pipe_config);
+		if (bp_gamma)
+			PIPE_CONF_CHECK_COLOR_LUT(gamma_mode, base.gamma_lut, bp_gamma);
+
 	}
 
 	PIPE_CONF_CHECK_BOOL(double_wide);
@@ -12837,6 +12861,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 #undef PIPE_CONF_CHECK_P
 #undef PIPE_CONF_CHECK_FLAGS
 #undef PIPE_CONF_CHECK_CLOCK_FUZZY
+#undef PIPE_CONF_CHECK_COLOR_LUT
 #undef PIPE_CONF_QUIRK
 
 	return ret;
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [v9][PATCH 05/11] drm/i915/display: Extract i9xx_read_luts()
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
                   ` (3 preceding siblings ...)
  2019-08-30  8:31 ` [v9][PATCH 04/11] drm/i915/display: Add macro to compare gamma hw/sw lut Swati Sharma
@ 2019-08-30  8:31 ` Swati Sharma
  2019-08-30 12:54   ` Jani Nikula
  2019-08-30  8:31 ` [v9][PATCH 06/11] drm/i915/display: Extract i965_read_luts() Swati Sharma
                   ` (9 subsequent siblings)
  14 siblings, 1 reply; 28+ messages in thread
From: Swati Sharma @ 2019-08-30  8:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, daniel.vetter, ankit.k.nautiyal

For the legacy(gen < 4) gamma, add hw read out to create hw blob of gamma
lut values. Also, add function intel_color_lut_pack to convert hw value
with given bit precision to lut property val.

v4: -No need to initialize *blob [Jani]
    -Removed right shifts [Jani]
    -Dropped dev local var [Jani]
v5: -Returned blob instead of assigning it internally within the
     function [Ville]
    -Renamed function i9xx_get_color_config() to i9xx_read_luts()
    -Renamed i9xx_get_config_internal() to i9xx_read_lut_8() [Ville]
v9: -Change in commit message [Jani, Uma]
    -Wrap commit within 75 characters [Uma]
    -Use macro for 256 [Uma]
    -Made read func para as const [Ville, Uma]

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

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 141efb0..107a03a 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1574,6 +1574,59 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1,
 	return true;
 }
 
+/* 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 struct drm_property_blob *
+i9xx_read_lut_8(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+	struct drm_property_blob *blob;
+	struct drm_color_lut *blob_data;
+	u32 i, val;
+
+	blob = drm_property_create_blob(&dev_priv->drm,
+					sizeof(struct drm_color_lut) * LEGACY_LUT_LENGTH,
+					NULL);
+	if (IS_ERR(blob))
+		return NULL;
+
+	blob_data = blob->data;
+
+	for (i = 0; i < LEGACY_LUT_LENGTH; 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), 8);
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(
+							  LGC_PALETTE_GREEN_MASK, val), 8);
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(
+							 LGC_PALETTE_BLUE_MASK, val), 8);
+	}
+
+	return blob;
+}
+
+void i9xx_read_luts(struct intel_crtc_state *crtc_state)
+{
+	crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1594,6 +1647,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.read_luts = i9xx_read_luts;
 		}
 	} else {
 		if (INTEL_GEN(dev_priv) >= 11)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 02e1ef1..09ea5b1 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7192,6 +7192,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 */
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [v9][PATCH 06/11] drm/i915/display: Extract i965_read_luts()
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
                   ` (4 preceding siblings ...)
  2019-08-30  8:31 ` [v9][PATCH 05/11] drm/i915/display: Extract i9xx_read_luts() Swati Sharma
@ 2019-08-30  8:31 ` Swati Sharma
  2019-08-30 13:08   ` Jani Nikula
  2019-08-30  8:31 ` [v9][PATCH 07/11] drm/i915/display: Extract chv_read_luts() Swati Sharma
                   ` (8 subsequent siblings)
  14 siblings, 1 reply; 28+ messages in thread
From: Swati Sharma @ 2019-08-30  8:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, daniel.vetter, ankit.k.nautiyal

For i965, add hw read out to create hw blob of gamma
lut values.

v4: -No need to initialize *blob [Jani]
    -Removed right shifts [Jani]
    -Dropped dev local var [Jani]
v5: -Returned blob instead of assigning it internally
     within the function [Ville]
    -Renamed i965_get_color_config() to i965_read_lut() [Ville]
    -Renamed i965_get_gamma_config_10p6() to i965_read_gamma_lut_10p6()
     [Ville]
v9: -Typo and 80 character limit [Uma]
    -Made read func para as const [Ville, Uma]
    -Renamed i965_read_gamma_lut_10p6() to i965_read_lut_10p6() [Ville, Uma]

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

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 107a03a..f87bb76 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1627,6 +1627,48 @@ void i9xx_read_luts(struct intel_crtc_state *crtc_state)
 	crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
 }
 
+static struct drm_property_blob *
+i965_read_lut_10p6(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
+	enum pipe pipe = crtc->pipe;
+	struct drm_property_blob *blob;
+	struct drm_color_lut *blob_data;
+	u32 i, val1, val2;
+
+	blob = drm_property_create_blob(&dev_priv->drm,
+					sizeof(struct drm_color_lut) * lut_size,
+					NULL);
+	if (IS_ERR(blob))
+		return NULL;
+
+	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) << 8 |
+						 REG_FIELD_GET(PALETTE_RED_MASK, val2);
+		blob_data[i].green = REG_FIELD_GET(PALETTE_GREEN_MASK, val1) << 8 |
+						   REG_FIELD_GET(PALETTE_GREEN_MASK, val2);
+		blob_data[i].blue = REG_FIELD_GET(PALETTE_BLUE_MASK, val1) << 8 |
+						  REG_FIELD_GET(PALETTE_BLUE_MASK, val2);
+	}
+
+	return blob;
+}
+
+static void i965_read_luts(struct intel_crtc_state *crtc_state)
+{
+	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
+		crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
+	else
+		crtc_state->base.gamma_lut = i965_read_lut_10p6(crtc_state);
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1642,6 +1684,7 @@ void intel_color_init(struct intel_crtc *crtc)
 		} else if (INTEL_GEN(dev_priv) >= 4) {
 			dev_priv->display.color_check = i9xx_color_check;
 			dev_priv->display.color_commit = i9xx_color_commit;
+			dev_priv->display.read_luts = i965_read_luts;
 			dev_priv->display.load_luts = i965_load_luts;
 		} else {
 			dev_priv->display.color_check = i9xx_color_check;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 09ea5b1..7e66673 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3558,6 +3558,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #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) + \
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [v9][PATCH 07/11] drm/i915/display: Extract chv_read_luts()
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
                   ` (5 preceding siblings ...)
  2019-08-30  8:31 ` [v9][PATCH 06/11] drm/i915/display: Extract i965_read_luts() Swati Sharma
@ 2019-08-30  8:31 ` Swati Sharma
  2019-08-30 13:15   ` Jani Nikula
  2019-08-30  8:31 ` [v9][PATCH 08/11] drm/i915/display: Extract ilk_read_luts() Swati Sharma
                   ` (7 subsequent siblings)
  14 siblings, 1 reply; 28+ messages in thread
From: Swati Sharma @ 2019-08-30  8:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, daniel.vetter, ankit.k.nautiyal

For cherryview, add hw read out to create hw blob of gamma
lut values.

v4: -No need to initialize *blob [Jani]
    -Removed right shifts [Jani]
    -Dropped dev local var [Jani]
v5: -Returned blob instead of assigning it internally within the
     function [Ville]
    -Renamed function cherryview_get_color_config() to chv_read_luts()
    -Renamed cherryview_get_gamma_config() to chv_read_cgm_gamma_lut()
     [Ville]
v9: -80 character limit [Uma]
    -Made read func para as const [Ville, Uma]
    -Renamed chv_read_cgm_gamma_lut() to chv_read_cgm_gamma_lut()
     [Ville, Uma]

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

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index f87bb76..b28b3b9 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1669,6 +1669,48 @@ static void i965_read_luts(struct intel_crtc_state *crtc_state)
 		crtc_state->base.gamma_lut = i965_read_lut_10p6(crtc_state);
 }
 
+static struct drm_property_blob *
+chv_read_cgm_lut(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
+	enum pipe pipe = crtc->pipe;
+	struct drm_property_blob *blob;
+	struct drm_color_lut *blob_data;
+	u32 i, val;
+
+	blob = drm_property_create_blob(&dev_priv->drm,
+					sizeof(struct drm_color_lut) * lut_size,
+					NULL);
+	if (IS_ERR(blob))
+		return NULL;
+
+	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), 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);
+	}
+
+	return blob;
+}
+
+static void chv_read_luts(struct intel_crtc_state *crtc_state)
+{
+	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
+		crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
+	else
+		crtc_state->base.gamma_lut = chv_read_cgm_lut(crtc_state);
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1681,6 +1723,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 = chv_load_luts;
+			dev_priv->display.read_luts = chv_read_luts;
 		} else if (INTEL_GEN(dev_priv) >= 4) {
 			dev_priv->display.color_check = i9xx_color_check;
 			dev_priv->display.color_commit = i9xx_color_commit;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7e66673..f5a2e73 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10311,6 +10311,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)
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [v9][PATCH 08/11] drm/i915/display: Extract ilk_read_luts()
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
                   ` (6 preceding siblings ...)
  2019-08-30  8:31 ` [v9][PATCH 07/11] drm/i915/display: Extract chv_read_luts() Swati Sharma
@ 2019-08-30  8:31 ` Swati Sharma
  2019-08-30 13:18   ` Jani Nikula
  2019-08-30  8:31 ` [v9][PATCH 09/11] drm/i915/display: Extract glk_read_luts() Swati Sharma
                   ` (6 subsequent siblings)
  14 siblings, 1 reply; 28+ messages in thread
From: Swati Sharma @ 2019-08-30  8:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, daniel.vetter, ankit.k.nautiyal

For ilk, add hw read out to create hw blob of gamma
lut values.

v4: -No need to initialize *blob [Jani]
    -Removed right shifts [Jani]
    -Dropped dev local var [Jani]
v5: -Returned blob instead of assigning it internally within the
     function [Ville]
    -Renamed ilk_get_color_config() to ilk_read_luts() [Ville]
v9: -80 character limit [Uma]
    -Made read func para as const [Ville, Uma]
    -Renamed ilk_read_gamma_lut() to ilk_read_lut_10() [Uma, Ville]

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

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index b28b3b9..33bcbda 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1711,6 +1711,47 @@ static void chv_read_luts(struct intel_crtc_state *crtc_state)
 		crtc_state->base.gamma_lut = chv_read_cgm_lut(crtc_state);
 }
 
+static struct drm_property_blob *
+ilk_read_lut_10(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
+	enum pipe pipe = crtc->pipe;
+	struct drm_property_blob *blob;
+	struct drm_color_lut *blob_data;
+	u32 i, val;
+
+	blob = drm_property_create_blob(&dev_priv->drm,
+					sizeof(struct drm_color_lut) * lut_size,
+					NULL);
+	if (IS_ERR(blob))
+		return NULL;
+
+	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), 10);
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(
+							  PREC_PALETTE_GREEN_MASK, val), 10);
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(
+							 PREC_PALETTE_BLUE_MASK, val), 10);
+	}
+
+	return blob;
+}
+
+static void ilk_read_luts(struct intel_crtc_state *crtc_state)
+{
+	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
+		crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
+	else
+		crtc_state->base.gamma_lut = ilk_read_lut_10(crtc_state);
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1760,8 +1801,10 @@ void intel_color_init(struct intel_crtc *crtc)
 			dev_priv->display.load_luts = bdw_load_luts;
 		else if (INTEL_GEN(dev_priv) >= 7)
 			dev_priv->display.load_luts = ivb_load_luts;
-		else
+		else {
 			dev_priv->display.load_luts = ilk_load_luts;
+			dev_priv->display.read_luts = ilk_read_luts;
+		}
 	}
 
 	drm_crtc_enable_color_mgmt(&crtc->base,
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f5a2e73..e457739 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7203,6 +7203,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
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [v9][PATCH 09/11] drm/i915/display: Extract glk_read_luts()
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
                   ` (7 preceding siblings ...)
  2019-08-30  8:31 ` [v9][PATCH 08/11] drm/i915/display: Extract ilk_read_luts() Swati Sharma
@ 2019-08-30  8:31 ` Swati Sharma
  2019-08-30  8:31 ` [v9][PATCH 10/11] drm/i915/display: Extract icl_read_luts() Swati Sharma
                   ` (5 subsequent siblings)
  14 siblings, 0 replies; 28+ messages in thread
From: Swati Sharma @ 2019-08-30  8:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, daniel.vetter, ankit.k.nautiyal

For glk, add hw read out to create hw blob of gamma
lut values.

v4: -No need to initialize *blob [Jani]
    -Removed right shifts [Jani]
    -Dropped dev local var [Jani]
v5: -Returned blob instead of assigning it internally within the
     function [Ville]
    -Renamed glk_get_color_config() to glk_read_luts() [Ville]
    -Added degamma validation [Ville]
v9: -80 character limit [Uma]
    -Made read func para as const [Ville, Uma]

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

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 33bcbda..621f540 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1752,6 +1752,52 @@ static void ilk_read_luts(struct intel_crtc_state *crtc_state)
 		crtc_state->base.gamma_lut = ilk_read_lut_10(crtc_state);
 }
 
+static struct drm_property_blob *
+glk_read_lut_10(const struct intel_crtc_state *crtc_state, u32 prec_index)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	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;
+	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_priv->drm,
+					sizeof(struct drm_color_lut) * hw_lut_size,
+					NULL);
+	if (IS_ERR(blob))
+		return NULL;
+
+	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), 10);
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(
+							PREC_PAL_DATA_GREEN_MASK, val), 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);
+
+	return blob;
+}
+
+static void glk_read_luts(struct intel_crtc_state *crtc_state)
+{
+	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
+		crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
+	else
+		crtc_state->base.gamma_lut = glk_read_lut_10(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);
@@ -1795,9 +1841,10 @@ void intel_color_init(struct intel_crtc *crtc)
 
 		if (INTEL_GEN(dev_priv) >= 11)
 			dev_priv->display.load_luts = icl_load_luts;
-		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;
-		else if (INTEL_GEN(dev_priv) >= 8)
+			dev_priv->display.read_luts = glk_read_luts;
+		} else if (INTEL_GEN(dev_priv) >= 8)
 			dev_priv->display.load_luts = bdw_load_luts;
 		else if (INTEL_GEN(dev_priv) >= 7)
 			dev_priv->display.load_luts = ivb_load_luts;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e457739..6b8bbfd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10262,6 +10262,9 @@ enum skl_power_gate {
 #define _PAL_PREC_GC_MAX_A	0x4A410
 #define _PAL_PREC_GC_MAX_B	0x4AC10
 #define _PAL_PREC_GC_MAX_C	0x4B410
+#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_EXT_GC_MAX_A	0x4A420
 #define _PAL_PREC_EXT_GC_MAX_B	0x4AC20
 #define _PAL_PREC_EXT_GC_MAX_C	0x4B420
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [v9][PATCH 10/11] drm/i915/display: Extract icl_read_luts()
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
                   ` (8 preceding siblings ...)
  2019-08-30  8:31 ` [v9][PATCH 09/11] drm/i915/display: Extract glk_read_luts() Swati Sharma
@ 2019-08-30  8:31 ` Swati Sharma
  2019-08-30  8:31 ` [v9][PATCH 11/11] FOR_TESTING_ONLY: Print rgb values of hw and sw blobs Swati Sharma
                   ` (4 subsequent siblings)
  14 siblings, 0 replies; 28+ messages in thread
From: Swati Sharma @ 2019-08-30  8:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, daniel.vetter, ankit.k.nautiyal

For icl+, add hw read out to create hw blob of gamma
lut values. icl+ platforms supports multi segmented gamma
mode, add hw lut creation for this mode.

This will be used to validate gamma programming using dsb
(display state buffer) which is a tgl feature.

v4: -No need to initialize *blob [Jani]
    -Removed right shifts [Jani]
    -Dropped dev local var [Jani]
v5: -Returned blob instead of assigning it internally within the
     function [Ville]
    -Renamed icl_get_color_config() to icl_read_luts() [Ville]
    -Renamed bdw_get_gamma_config() to bdw_read_lut_10() [Ville]
v9: -Added hw lut creation for multi-seg gamma mode

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

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 621f540..8d6f3c4 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -837,7 +837,6 @@ static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
 	u32 i;
 
 	/*
-	 *
 	 * Program Fine segment (let's call it seg2)...
 	 *
 	 * Fine segment's step is 1/(128 * 256) ie 1/(128 * 256),  2/(128*256)
@@ -890,12 +889,10 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 	case GAMMA_MODE_MODE_8BIT:
 		i9xx_load_luts(crtc_state);
 		break;
-
 	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
 		icl_program_gamma_superfine_segment(crtc_state);
 		icl_program_gamma_multi_segment(crtc_state);
 		break;
-
 	default:
 		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
 		ivb_load_lut_ext_max(crtc);
@@ -1798,6 +1795,80 @@ static void glk_read_luts(struct intel_crtc_state *crtc_state)
 		crtc_state->base.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0));
 }
 
+static struct drm_property_blob *
+icl_read_lut_super_multi_seg(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	int hw_lut_size = 521;
+	enum pipe pipe = crtc->pipe;
+	struct drm_property_blob *blob;
+	struct drm_color_lut *blob_data;
+	u32 i, val1, val2;
+
+	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
+
+	blob = drm_property_create_blob(&dev_priv->drm,
+					sizeof(struct drm_color_lut) * hw_lut_size,
+					NULL);
+	if (IS_ERR(blob))
+		return NULL;
+
+	blob_data = blob->data;
+
+	for (i = 0; i < 9; i++) {
+		val1 = I915_READ(PREC_PAL_MULTI_SEG_DATA(pipe));
+		val2 = I915_READ(PREC_PAL_MULTI_SEG_DATA(pipe));
+
+		blob_data[i].red = REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_LDW_MASK, val1) << 10 |
+				   REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_UDW_MASK, val2);
+		blob_data[i].green = REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_LDW_MASK, val1) << 10 |
+				     REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_UDW_MASK, val2);
+		blob_data[i].blue = REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_LDW_MASK, val1) << 10 |
+				    REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_UDW_MASK, val2);
+	}
+
+	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
+
+	for (i = 1; i < 257; i++) {
+		val1 = I915_READ(PREC_PAL_DATA(pipe));
+		val2 = I915_READ(PREC_PAL_DATA(pipe));
+
+		blob_data[i + 8].red = REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_LDW_MASK, val1) << 10 |
+				       REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_UDW_MASK, val2);
+		blob_data[i + 8].green = REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_LDW_MASK, val1) << 10 |
+					 REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_UDW_MASK, val2);
+		blob_data[i + 8].blue = REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_LDW_MASK, val1) << 10 |
+					REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_UDW_MASK, val2);
+	}
+
+	for (i = 0; i < 256; i++) {
+		val1 = I915_READ(PREC_PAL_DATA(pipe));
+		val2 = I915_READ(PREC_PAL_DATA(pipe));
+
+		blob_data[i + 265].red = REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_LDW_MASK, val1) << 10 |
+					 REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_UDW_MASK, val2);
+		blob_data[i + 265].green = REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_LDW_MASK, val1) << 10 |
+					   REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_UDW_MASK, val2);
+		blob_data[i + 265].blue = REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_LDW_MASK, val1) << 10 |
+					  REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_UDW_MASK, val2);
+	}
+
+	return blob;
+}
+
+static void icl_read_luts(struct intel_crtc_state *crtc_state)
+{
+	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
+	    GAMMA_MODE_MODE_8BIT)
+		crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
+	else if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
+		 GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED)
+		crtc_state->base.gamma_lut = icl_read_lut_super_multi_seg(crtc_state);
+	else
+		crtc_state->base.gamma_lut = glk_read_lut_10(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);
@@ -1839,16 +1910,17 @@ 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;
-		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
+			dev_priv->display.read_luts = icl_read_luts;
+		} else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
 			dev_priv->display.load_luts = glk_load_luts;
 			dev_priv->display.read_luts = glk_read_luts;
-		} else if (INTEL_GEN(dev_priv) >= 8)
+		} else if (INTEL_GEN(dev_priv) >= 8) {
 			dev_priv->display.load_luts = bdw_load_luts;
-		else if (INTEL_GEN(dev_priv) >= 7)
+		} else if (INTEL_GEN(dev_priv) >= 7) {
 			dev_priv->display.load_luts = ivb_load_luts;
-		else {
+		} else {
 			dev_priv->display.load_luts = ilk_load_luts;
 			dev_priv->display.read_luts = ilk_read_luts;
 		}
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6b8bbfd..553b71d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10297,6 +10297,12 @@ enum skl_power_gate {
 
 #define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
 #define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C
+#define  PAL_PREC_MULTI_SEG_RED_LDW_MASK   REG_GENMASK(29, 24)
+#define  PAL_PREC_MULTI_SEG_RED_UDW_MASK   REG_GENMASK(29, 20)
+#define  PAL_PREC_MULTI_SEG_GREEN_LDW_MASK REG_GENMASK(19, 14)
+#define  PAL_PREC_MULTI_SEG_GREEN_UDW_MASK REG_GENMASK(19, 10)
+#define  PAL_PREC_MULTI_SEG_BLUE_LDW_MASK  REG_GENMASK(9, 4)
+#define  PAL_PREC_MULTI_SEG_BLUE_UDW_MASK  REG_GENMASK(9, 0)
 
 #define PREC_PAL_MULTI_SEG_INDEX(pipe)	_MMIO_PIPE(pipe, \
 					_PAL_PREC_MULTI_SEG_INDEX_A, \
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [v9][PATCH 11/11] FOR_TESTING_ONLY: Print rgb values of hw and sw blobs
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
                   ` (9 preceding siblings ...)
  2019-08-30  8:31 ` [v9][PATCH 10/11] drm/i915/display: Extract icl_read_luts() Swati Sharma
@ 2019-08-30  8:31 ` Swati Sharma
  2019-08-30  9:39 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: adding state checker for gamma lut values (rev13) Patchwork
                   ` (3 subsequent siblings)
  14 siblings, 0 replies; 28+ messages in thread
From: Swati Sharma @ 2019-08-30  8:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, daniel.vetter, ankit.k.nautiyal

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

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 8d6f3c4..c560c91 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1470,6 +1470,8 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat
 static inline bool err_check(struct drm_color_lut *sw_lut,
 			     struct drm_color_lut *hw_lut, u32 err)
 {
+	DRM_DEBUG_KMS("hw_lut->red=0x%x sw_lut->red=0x%x hw_lut->blue=0x%x sw_lut->blue=0x%x hw_lut->green=0x%x sw_lut->green=0x%x", hw_lut->red, sw_lut->red, hw_lut->blue, sw_lut->blue, hw_lut->green, sw_lut->green);
+
 	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);
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: adding state checker for gamma lut values (rev13)
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
                   ` (10 preceding siblings ...)
  2019-08-30  8:31 ` [v9][PATCH 11/11] FOR_TESTING_ONLY: Print rgb values of hw and sw blobs Swati Sharma
@ 2019-08-30  9:39 ` Patchwork
  2019-08-30  9:44 ` ✗ Fi.CI.SPARSE: " Patchwork
                   ` (2 subsequent siblings)
  14 siblings, 0 replies; 28+ messages in thread
From: Patchwork @ 2019-08-30  9:39 UTC (permalink / raw)
  To: Swati Sharma; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

$ dim checkpatch origin/drm-tip
329aa0177067 drm/i915/display: Add func to get gamma bit precision
b39c8ce28cc7 drm/i915/display: Add debug log for color parameters
a66c08206a9b drm/i915/display: Add func to compare hw/sw gamma lut
-:108: CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'sw_lut_size != 262145'
#108: FILE: drivers/gpu/drm/i915/display/intel_color.c:1544:
+		if ((sw_lut_size != 262145) || (hw_lut_size != 521))

-:108: CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'hw_lut_size != 521'
#108: FILE: drivers/gpu/drm/i915/display/intel_color.c:1544:
+		if ((sw_lut_size != 262145) || (hw_lut_size != 521))

total: 0 errors, 0 warnings, 2 checks, 129 lines checked
b82a8dde760e drm/i915/display: Add macro to compare gamma hw/sw lut
-:36: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'name1' - possible side-effects?
#36: FILE: drivers/gpu/drm/i915/display/intel_display.c:12684:
+#define PIPE_CONF_CHECK_COLOR_LUT(name1, name2, bit_precision) do { \
+	if (current_config->name1 != pipe_config->name1) { \
+		pipe_config_mismatch(fastset, __stringify(name1), \
+				"(expected %i, found %i, won't compare lut values)\n", \
+				current_config->name1, \
+				pipe_config->name1); \
+		ret = false;\
+	} else { \
+		if (!intel_color_lut_equal(current_config->name2, \
+					pipe_config->name2, pipe_config->name1, \
+					bit_precision)) { \
+			pipe_config_mismatch(fastset, __stringify(name2), \
+					"hw_state doesn't match sw_state\n"); \
+			ret = false; \
+		} \
+	} \
+} while (0)

-:36: CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'name1' may be better as '(name1)' to avoid precedence issues
#36: FILE: drivers/gpu/drm/i915/display/intel_display.c:12684:
+#define PIPE_CONF_CHECK_COLOR_LUT(name1, name2, bit_precision) do { \
+	if (current_config->name1 != pipe_config->name1) { \
+		pipe_config_mismatch(fastset, __stringify(name1), \
+				"(expected %i, found %i, won't compare lut values)\n", \
+				current_config->name1, \
+				pipe_config->name1); \
+		ret = false;\
+	} else { \
+		if (!intel_color_lut_equal(current_config->name2, \
+					pipe_config->name2, pipe_config->name1, \
+					bit_precision)) { \
+			pipe_config_mismatch(fastset, __stringify(name2), \
+					"hw_state doesn't match sw_state\n"); \
+			ret = false; \
+		} \
+	} \
+} while (0)

-:36: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'name2' - possible side-effects?
#36: FILE: drivers/gpu/drm/i915/display/intel_display.c:12684:
+#define PIPE_CONF_CHECK_COLOR_LUT(name1, name2, bit_precision) do { \
+	if (current_config->name1 != pipe_config->name1) { \
+		pipe_config_mismatch(fastset, __stringify(name1), \
+				"(expected %i, found %i, won't compare lut values)\n", \
+				current_config->name1, \
+				pipe_config->name1); \
+		ret = false;\
+	} else { \
+		if (!intel_color_lut_equal(current_config->name2, \
+					pipe_config->name2, pipe_config->name1, \
+					bit_precision)) { \
+			pipe_config_mismatch(fastset, __stringify(name2), \
+					"hw_state doesn't match sw_state\n"); \
+			ret = false; \
+		} \
+	} \
+} while (0)

-:36: CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'name2' may be better as '(name2)' to avoid precedence issues
#36: FILE: drivers/gpu/drm/i915/display/intel_display.c:12684:
+#define PIPE_CONF_CHECK_COLOR_LUT(name1, name2, bit_precision) do { \
+	if (current_config->name1 != pipe_config->name1) { \
+		pipe_config_mismatch(fastset, __stringify(name1), \
+				"(expected %i, found %i, won't compare lut values)\n", \
+				current_config->name1, \
+				pipe_config->name1); \
+		ret = false;\
+	} else { \
+		if (!intel_color_lut_equal(current_config->name2, \
+					pipe_config->name2, pipe_config->name1, \
+					bit_precision)) { \
+			pipe_config_mismatch(fastset, __stringify(name2), \
+					"hw_state doesn't match sw_state\n"); \
+			ret = false; \
+		} \
+	} \
+} while (0)

total: 0 errors, 0 warnings, 4 checks, 49 lines checked
6821749514aa drm/i915/display: Extract i9xx_read_luts()
-:69: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#69: FILE: drivers/gpu/drm/i915/display/intel_color.c:1614:
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(

-:71: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#71: FILE: drivers/gpu/drm/i915/display/intel_color.c:1616:
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(

-:73: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#73: FILE: drivers/gpu/drm/i915/display/intel_color.c:1618:
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(

total: 0 errors, 0 warnings, 3 checks, 75 lines checked
0e30173fca80 drm/i915/display: Extract i965_read_luts()
-:19: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#19: 
    -Renamed i965_read_gamma_lut_10p6() to i965_read_lut_10p6() [Ville, Uma]

total: 0 errors, 1 warnings, 0 checks, 64 lines checked
392f6c41e8aa drm/i915/display: Extract chv_read_luts()
-:53: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#53: FILE: drivers/gpu/drm/i915/display/intel_color.c:1693:
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(

-:55: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#55: FILE: drivers/gpu/drm/i915/display/intel_color.c:1695:
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(

-:59: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#59: FILE: drivers/gpu/drm/i915/display/intel_color.c:1699:
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(

total: 0 errors, 0 warnings, 3 checks, 64 lines checked
bc148699e793 drm/i915/display: Extract ilk_read_luts()
-:51: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#51: FILE: drivers/gpu/drm/i915/display/intel_color.c:1736:
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(

-:53: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#53: FILE: drivers/gpu/drm/i915/display/intel_color.c:1738:
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(

-:55: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#55: FILE: drivers/gpu/drm/i915/display/intel_color.c:1740:
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(

-:78: CHECK:BRACES: Unbalanced braces around else statement
#78: FILE: drivers/gpu/drm/i915/display/intel_color.c:1804:
+		else {

total: 0 errors, 0 warnings, 4 checks, 67 lines checked
f1edfeae6b32 drm/i915/display: Extract glk_read_luts()
-:54: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#54: FILE: drivers/gpu/drm/i915/display/intel_color.c:1780:
+		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(

-:56: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#56: FILE: drivers/gpu/drm/i915/display/intel_color.c:1782:
+		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(

-:58: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#58: FILE: drivers/gpu/drm/i915/display/intel_color.c:1784:
+		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(

total: 0 errors, 0 warnings, 3 checks, 73 lines checked
ccb2fd7e595f drm/i915/display: Extract icl_read_luts()
-:94: WARNING:LONG_LINE: line over 100 characters
#94: FILE: drivers/gpu/drm/i915/display/intel_color.c:1839:
+		blob_data[i + 8].green = REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_LDW_MASK, val1) << 10 |

-:96: WARNING:LONG_LINE: line over 100 characters
#96: FILE: drivers/gpu/drm/i915/display/intel_color.c:1841:
+		blob_data[i + 8].blue = REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_LDW_MASK, val1) << 10 |

-:104: WARNING:LONG_LINE: line over 100 characters
#104: FILE: drivers/gpu/drm/i915/display/intel_color.c:1849:
+		blob_data[i + 265].red = REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_LDW_MASK, val1) << 10 |

-:106: WARNING:LONG_LINE: line over 100 characters
#106: FILE: drivers/gpu/drm/i915/display/intel_color.c:1851:
+		blob_data[i + 265].green = REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_LDW_MASK, val1) << 10 |

-:108: WARNING:LONG_LINE: line over 100 characters
#108: FILE: drivers/gpu/drm/i915/display/intel_color.c:1853:
+		blob_data[i + 265].blue = REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_LDW_MASK, val1) << 10 |

total: 0 errors, 5 warnings, 0 checks, 133 lines checked
324b6c5f95e6 FOR_TESTING_ONLY: Print rgb values of hw and sw blobs
-:7: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

-:16: WARNING:LONG_LINE: line over 100 characters
#16: FILE: drivers/gpu/drm/i915/display/intel_color.c:1473:
+	DRM_DEBUG_KMS("hw_lut->red=0x%x sw_lut->red=0x%x hw_lut->blue=0x%x sw_lut->blue=0x%x hw_lut->green=0x%x sw_lut->green=0x%x", hw_lut->red, sw_lut->red, hw_lut->blue, sw_lut->blue, hw_lut->green, sw_lut->green);

total: 0 errors, 2 warnings, 0 checks, 8 lines checked

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* ✗ Fi.CI.SPARSE: warning for drm/i915: adding state checker for gamma lut values (rev13)
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
                   ` (11 preceding siblings ...)
  2019-08-30  9:39 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: adding state checker for gamma lut values (rev13) Patchwork
@ 2019-08-30  9:44 ` Patchwork
  2019-08-30 10:01 ` ✓ Fi.CI.BAT: success " Patchwork
  2019-08-31  3:01 ` ✓ Fi.CI.IGT: " Patchwork
  14 siblings, 0 replies; 28+ messages in thread
From: Patchwork @ 2019-08-30  9:44 UTC (permalink / raw)
  To: Swati Sharma; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.6.0
Commit: drm/i915/display: Add func to get gamma bit precision
Okay!

Commit: drm/i915/display: Add debug log for color parameters
Okay!

Commit: drm/i915/display: Add func to compare hw/sw gamma lut
Okay!

Commit: drm/i915/display: Add macro to compare gamma hw/sw lut
Okay!

Commit: drm/i915/display: Extract i9xx_read_luts()
+drivers/gpu/drm/i915/display/intel_color.c:1625:6: warning: symbol 'i9xx_read_luts' was not declared. Should it be static?

Commit: drm/i915/display: Extract i965_read_luts()
Okay!

Commit: drm/i915/display: Extract chv_read_luts()
Okay!

Commit: drm/i915/display: Extract ilk_read_luts()
Okay!

Commit: drm/i915/display: Extract glk_read_luts()
Okay!

Commit: drm/i915/display: Extract icl_read_luts()
Okay!

Commit: FOR_TESTING_ONLY: Print rgb values of hw and sw blobs
Okay!

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: adding state checker for gamma lut values (rev13)
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
                   ` (12 preceding siblings ...)
  2019-08-30  9:44 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2019-08-30 10:01 ` Patchwork
  2019-08-31  3:01 ` ✓ Fi.CI.IGT: " Patchwork
  14 siblings, 0 replies; 28+ messages in thread
From: Patchwork @ 2019-08-30 10:01 UTC (permalink / raw)
  To: Swati Sharma; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_6807 -> Patchwork_14233
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-snb-2600:        [PASS][1] -> [DMESG-WARN][2] ([fdo#110684] / [fdo#111115])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/fi-snb-2600/igt@gem_exec_suspend@basic-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/fi-snb-2600/igt@gem_exec_suspend@basic-s3.html

  
#### Possible fixes ####

  * igt@i915_selftest@live_execlists:
    - fi-skl-gvtdvm:      [DMESG-FAIL][3] ([fdo#111108]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/fi-skl-gvtdvm/igt@i915_selftest@live_execlists.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/fi-skl-gvtdvm/igt@i915_selftest@live_execlists.html

  * igt@i915_selftest@live_workarounds:
    - fi-bsw-kefka:       [DMESG-WARN][5] ([fdo#111373]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/fi-bsw-kefka/igt@i915_selftest@live_workarounds.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/fi-bsw-kefka/igt@i915_selftest@live_workarounds.html

  * igt@kms_flip@basic-flip-vs-wf_vblank:
    - {fi-kbl-soraka}:    [FAIL][7] ([fdo#100368]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/fi-kbl-soraka/igt@kms_flip@basic-flip-vs-wf_vblank.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/fi-kbl-soraka/igt@kms_flip@basic-flip-vs-wf_vblank.html

  
#### Warnings ####

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-kbl-guc:         [FAIL][9] ([fdo#107707]) -> [SKIP][10] ([fdo#109271])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/fi-kbl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/fi-kbl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][11] ([fdo#111407]) -> [FAIL][12] ([fdo#111096])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

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

  [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
  [fdo#107707]: https://bugs.freedesktop.org/show_bug.cgi?id=107707
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#110684]: https://bugs.freedesktop.org/show_bug.cgi?id=110684
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
  [fdo#111108]: https://bugs.freedesktop.org/show_bug.cgi?id=111108
  [fdo#111115]: https://bugs.freedesktop.org/show_bug.cgi?id=111115
  [fdo#111373]: https://bugs.freedesktop.org/show_bug.cgi?id=111373
  [fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407


Participating hosts (51 -> 45)
------------------------------

  Additional (1): fi-hsw-peppy 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6807 -> Patchwork_14233

  CI-20190529: 20190529
  CI_DRM_6807: 7dfe53944a301f8b7946e69edb570661798dd0b9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5159: c06ee2989c012fcc43cd361e75e9ee5a3a9292df @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14233: 324b6c5f95e64bb0c2bee0e4c9faee606d9b12d5 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

324b6c5f95e6 FOR_TESTING_ONLY: Print rgb values of hw and sw blobs
ccb2fd7e595f drm/i915/display: Extract icl_read_luts()
f1edfeae6b32 drm/i915/display: Extract glk_read_luts()
bc148699e793 drm/i915/display: Extract ilk_read_luts()
392f6c41e8aa drm/i915/display: Extract chv_read_luts()
0e30173fca80 drm/i915/display: Extract i965_read_luts()
6821749514aa drm/i915/display: Extract i9xx_read_luts()
b82a8dde760e drm/i915/display: Add macro to compare gamma hw/sw lut
a66c08206a9b drm/i915/display: Add func to compare hw/sw gamma lut
b39c8ce28cc7 drm/i915/display: Add debug log for color parameters
329aa0177067 drm/i915/display: Add func to get gamma bit precision

== Logs ==

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision
  2019-08-30  8:31 ` [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision Swati Sharma
@ 2019-08-30 12:02   ` Jani Nikula
  2019-08-30 12:04     ` Jani Nikula
  0 siblings, 1 reply; 28+ messages in thread
From: Jani Nikula @ 2019-08-30 12:02 UTC (permalink / raw)
  To: Swati Sharma, intel-gfx; +Cc: daniel.vetter, ankit.k.nautiyal

On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> Each platform supports different gamma modes and each gamma mode
> has different bit precision. Here bit precision corresponds
> to number of bits the hw LUT supports.
>
> Add func per platform to return bit precision corresponding to gamma mode
> which will be later used as a parameter in lut comparison function
> intel_color_lut_equal().
>
> This is done for legacy, i965, ilk, glk, icl and their variant platforms.
>
> v6: -Added func intel_color_get_bit_precision() to get bit precision for
>      gamma and degamma lut readout depending upon platform and
>      corresponding to load_luts() [Ankit]
>     -Made patch11 as patch3 [Jani]
> v7: -Renamed func intel_color_get_bit_precision() to
>      intel_color_get_gamma_bit_precision()
>     -Added separate function/platform for gamma bit precision [Ville]
>     -Corrected checkpatch warnings
> v8: -Split patch 3 into 4 separate patches
> v9: -Changed commit message, gave more info [Uma]
>     -Added precision func for icl+ platform
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 99 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_color.h |  1 +
>  2 files changed, 100 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 71a0201..dcc65d7 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1371,6 +1371,105 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +static int i9xx_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
> +	if (!crtc_state->gamma_enable)
> +		return 0;
> +
> +	switch (crtc_state->gamma_mode) {
> +	case GAMMA_MODE_MODE_8BIT:
> +		return 8;
> +	case GAMMA_MODE_MODE_10BIT:
> +		return 16;
> +	default:
> +		MISSING_CASE(crtc_state->gamma_mode);
> +		return 0;
> +	}
> +}
> +
> +static int ilk_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
> +	if (!crtc_state->gamma_enable)
> +		return 0;
> +
> +	if ((crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) == 0)
> +		return 0;
> +
> +	switch (crtc_state->gamma_mode) {
> +	case GAMMA_MODE_MODE_8BIT:
> +		return 8;
> +	case GAMMA_MODE_MODE_10BIT:
> +		return 10;
> +	default:
> +		MISSING_CASE(crtc_state->gamma_mode);
> +		return 0;
> +	}
> +}
> +
> +static int chv_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
> +	if (crtc_state->cgm_mode & CGM_PIPE_MODE_GAMMA)
> +		return 10;
> +	else
> +		return i9xx_gamma_precision(crtc_state);

Why does one branch check for ->gamma_enable and the other not? See below.

> +}
> +
> +static int glk_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
> +	if (!crtc_state->gamma_enable)
> +		return 0;
> +
> +	switch (crtc_state->gamma_mode) {
> +	case GAMMA_MODE_MODE_8BIT:
> +		return 8;
> +	case GAMMA_MODE_MODE_10BIT:
> +		return 10;
> +	default:
> +		MISSING_CASE(crtc_state->gamma_mode);
> +		return 0;
> +	}
> +}
> +
> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{

Why does this function not check for ->gamma_enable but the others do?
See below.

> +	if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0)
> +		return 0;
> +
> +	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
> +	case GAMMA_MODE_MODE_8BIT:
> +		return 8;
> +	case GAMMA_MODE_MODE_10BIT:
> +		return 10;
> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> +		return 16;
> +	default:
> +		MISSING_CASE(crtc_state->gamma_mode);
> +		return 0;
> +	}
> +}
> +
> +int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +

Should the ->gamma_enable check be here once, instead?

With that fixed,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


BR,
Jani.


> +	if (HAS_GMCH(dev_priv)) {
> +		if (IS_CHERRYVIEW(dev_priv))
> +			return chv_gamma_precision(crtc_state);
> +		else
> +			return i9xx_gamma_precision(crtc_state);
> +	} else {
> +		if (INTEL_GEN(dev_priv) >= 11)
> +			return icl_gamma_precision(crtc_state);
> +		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> +			return glk_gamma_precision(crtc_state);
> +		else if (IS_IRONLAKE(dev_priv))
> +			return ilk_gamma_precision(crtc_state);
> +	}
> +
> +	return 0;
> +}
> +
>  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/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h
> index 057e8ac..0226d3a 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.h
> +++ b/drivers/gpu/drm/i915/display/intel_color.h
> @@ -14,5 +14,6 @@
>  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);
> +int intel_color_get_gamma_bit_precision(const 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision
  2019-08-30 12:02   ` Jani Nikula
@ 2019-08-30 12:04     ` Jani Nikula
  2019-08-30 12:05       ` Jani Nikula
  0 siblings, 1 reply; 28+ messages in thread
From: Jani Nikula @ 2019-08-30 12:04 UTC (permalink / raw)
  To: Swati Sharma, intel-gfx; +Cc: daniel.vetter, ankit.k.nautiyal

On Fri, 30 Aug 2019, Jani Nikula <jani.nikula@intel.com> wrote:
> On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
>> Each platform supports different gamma modes and each gamma mode
>> has different bit precision. Here bit precision corresponds
>> to number of bits the hw LUT supports.
>>
>> Add func per platform to return bit precision corresponding to gamma mode
>> which will be later used as a parameter in lut comparison function
>> intel_color_lut_equal().
>>
>> This is done for legacy, i965, ilk, glk, icl and their variant platforms.
>>
>> v6: -Added func intel_color_get_bit_precision() to get bit precision for
>>      gamma and degamma lut readout depending upon platform and
>>      corresponding to load_luts() [Ankit]
>>     -Made patch11 as patch3 [Jani]
>> v7: -Renamed func intel_color_get_bit_precision() to
>>      intel_color_get_gamma_bit_precision()
>>     -Added separate function/platform for gamma bit precision [Ville]
>>     -Corrected checkpatch warnings
>> v8: -Split patch 3 into 4 separate patches
>> v9: -Changed commit message, gave more info [Uma]
>>     -Added precision func for icl+ platform
>>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>> ---
>>  drivers/gpu/drm/i915/display/intel_color.c | 99 ++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/display/intel_color.h |  1 +
>>  2 files changed, 100 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>> index 71a0201..dcc65d7 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> @@ -1371,6 +1371,105 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>>  	return 0;
>>  }
>>  
>> +static int i9xx_gamma_precision(const struct intel_crtc_state *crtc_state)
>> +{
>> +	if (!crtc_state->gamma_enable)
>> +		return 0;
>> +
>> +	switch (crtc_state->gamma_mode) {
>> +	case GAMMA_MODE_MODE_8BIT:
>> +		return 8;
>> +	case GAMMA_MODE_MODE_10BIT:
>> +		return 16;
>> +	default:
>> +		MISSING_CASE(crtc_state->gamma_mode);
>> +		return 0;
>> +	}
>> +}
>> +
>> +static int ilk_gamma_precision(const struct intel_crtc_state *crtc_state)
>> +{
>> +	if (!crtc_state->gamma_enable)
>> +		return 0;
>> +
>> +	if ((crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) == 0)
>> +		return 0;
>> +
>> +	switch (crtc_state->gamma_mode) {
>> +	case GAMMA_MODE_MODE_8BIT:
>> +		return 8;
>> +	case GAMMA_MODE_MODE_10BIT:
>> +		return 10;
>> +	default:
>> +		MISSING_CASE(crtc_state->gamma_mode);
>> +		return 0;
>> +	}
>> +}
>> +
>> +static int chv_gamma_precision(const struct intel_crtc_state *crtc_state)
>> +{
>> +	if (crtc_state->cgm_mode & CGM_PIPE_MODE_GAMMA)
>> +		return 10;
>> +	else
>> +		return i9xx_gamma_precision(crtc_state);
>
> Why does one branch check for ->gamma_enable and the other not? See below.
>
>> +}
>> +
>> +static int glk_gamma_precision(const struct intel_crtc_state *crtc_state)
>> +{
>> +	if (!crtc_state->gamma_enable)
>> +		return 0;
>> +
>> +	switch (crtc_state->gamma_mode) {
>> +	case GAMMA_MODE_MODE_8BIT:
>> +		return 8;
>> +	case GAMMA_MODE_MODE_10BIT:
>> +		return 10;
>> +	default:
>> +		MISSING_CASE(crtc_state->gamma_mode);
>> +		return 0;
>> +	}
>> +}
>> +
>> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state)
>> +{
>
> Why does this function not check for ->gamma_enable but the others do?
> See below.
>
>> +	if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0)
>> +		return 0;
>> +
>> +	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
>> +	case GAMMA_MODE_MODE_8BIT:
>> +		return 8;
>> +	case GAMMA_MODE_MODE_10BIT:
>> +		return 10;
>> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>> +		return 16;
>> +	default:
>> +		MISSING_CASE(crtc_state->gamma_mode);
>> +		return 0;
>> +	}
>> +}
>> +
>> +int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state)
>> +{
>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +
>
> Should the ->gamma_enable check be here once, instead?
>
> With that fixed,
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
>
> BR,
> Jani.
>
>
>> +	if (HAS_GMCH(dev_priv)) {
>> +		if (IS_CHERRYVIEW(dev_priv))
>> +			return chv_gamma_precision(crtc_state);
>> +		else
>> +			return i9xx_gamma_precision(crtc_state);
>> +	} else {
>> +		if (INTEL_GEN(dev_priv) >= 11)
>> +			return icl_gamma_precision(crtc_state);
>> +		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>> +			return glk_gamma_precision(crtc_state);
>> +		else if (IS_IRONLAKE(dev_priv))
>> +			return ilk_gamma_precision(crtc_state);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  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/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h
>> index 057e8ac..0226d3a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.h
>> +++ b/drivers/gpu/drm/i915/display/intel_color.h
>> @@ -14,5 +14,6 @@
>>  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);
>> +int intel_color_get_gamma_bit_precision(const 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision
  2019-08-30 12:04     ` Jani Nikula
@ 2019-08-30 12:05       ` Jani Nikula
  0 siblings, 0 replies; 28+ messages in thread
From: Jani Nikula @ 2019-08-30 12:05 UTC (permalink / raw)
  To: Swati Sharma, intel-gfx; +Cc: daniel.vetter, ankit.k.nautiyal

On Fri, 30 Aug 2019, Jani Nikula <jani.nikula@intel.com> wrote:
> On Fri, 30 Aug 2019, Jani Nikula <jani.nikula@intel.com> wrote:
>> On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
>>> Each platform supports different gamma modes and each gamma mode
>>> has different bit precision. Here bit precision corresponds
>>> to number of bits the hw LUT supports.
>>>
>>> Add func per platform to return bit precision corresponding to gamma mode
>>> which will be later used as a parameter in lut comparison function
>>> intel_color_lut_equal().
>>>
>>> This is done for legacy, i965, ilk, glk, icl and their variant platforms.
>>>
>>> v6: -Added func intel_color_get_bit_precision() to get bit precision for
>>>      gamma and degamma lut readout depending upon platform and
>>>      corresponding to load_luts() [Ankit]
>>>     -Made patch11 as patch3 [Jani]
>>> v7: -Renamed func intel_color_get_bit_precision() to
>>>      intel_color_get_gamma_bit_precision()
>>>     -Added separate function/platform for gamma bit precision [Ville]
>>>     -Corrected checkpatch warnings
>>> v8: -Split patch 3 into 4 separate patches
>>> v9: -Changed commit message, gave more info [Uma]
>>>     -Added precision func for icl+ platform
>>>
>>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Ooops, finger slipped this to the wrong patch. Was supposed to be for
patch 2...

>
>>> ---
>>>  drivers/gpu/drm/i915/display/intel_color.c | 99 ++++++++++++++++++++++++++++++
>>>  drivers/gpu/drm/i915/display/intel_color.h |  1 +
>>>  2 files changed, 100 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>>> index 71a0201..dcc65d7 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>>> @@ -1371,6 +1371,105 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>>>  	return 0;
>>>  }
>>>  
>>> +static int i9xx_gamma_precision(const struct intel_crtc_state *crtc_state)
>>> +{
>>> +	if (!crtc_state->gamma_enable)
>>> +		return 0;
>>> +
>>> +	switch (crtc_state->gamma_mode) {
>>> +	case GAMMA_MODE_MODE_8BIT:
>>> +		return 8;
>>> +	case GAMMA_MODE_MODE_10BIT:
>>> +		return 16;
>>> +	default:
>>> +		MISSING_CASE(crtc_state->gamma_mode);
>>> +		return 0;
>>> +	}
>>> +}
>>> +
>>> +static int ilk_gamma_precision(const struct intel_crtc_state *crtc_state)
>>> +{
>>> +	if (!crtc_state->gamma_enable)
>>> +		return 0;
>>> +
>>> +	if ((crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) == 0)
>>> +		return 0;
>>> +
>>> +	switch (crtc_state->gamma_mode) {
>>> +	case GAMMA_MODE_MODE_8BIT:
>>> +		return 8;
>>> +	case GAMMA_MODE_MODE_10BIT:
>>> +		return 10;
>>> +	default:
>>> +		MISSING_CASE(crtc_state->gamma_mode);
>>> +		return 0;
>>> +	}
>>> +}
>>> +
>>> +static int chv_gamma_precision(const struct intel_crtc_state *crtc_state)
>>> +{
>>> +	if (crtc_state->cgm_mode & CGM_PIPE_MODE_GAMMA)
>>> +		return 10;
>>> +	else
>>> +		return i9xx_gamma_precision(crtc_state);
>>
>> Why does one branch check for ->gamma_enable and the other not? See below.
>>
>>> +}
>>> +
>>> +static int glk_gamma_precision(const struct intel_crtc_state *crtc_state)
>>> +{
>>> +	if (!crtc_state->gamma_enable)
>>> +		return 0;
>>> +
>>> +	switch (crtc_state->gamma_mode) {
>>> +	case GAMMA_MODE_MODE_8BIT:
>>> +		return 8;
>>> +	case GAMMA_MODE_MODE_10BIT:
>>> +		return 10;
>>> +	default:
>>> +		MISSING_CASE(crtc_state->gamma_mode);
>>> +		return 0;
>>> +	}
>>> +}
>>> +
>>> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state)
>>> +{
>>
>> Why does this function not check for ->gamma_enable but the others do?
>> See below.
>>
>>> +	if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0)
>>> +		return 0;
>>> +
>>> +	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
>>> +	case GAMMA_MODE_MODE_8BIT:
>>> +		return 8;
>>> +	case GAMMA_MODE_MODE_10BIT:
>>> +		return 10;
>>> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>>> +		return 16;
>>> +	default:
>>> +		MISSING_CASE(crtc_state->gamma_mode);
>>> +		return 0;
>>> +	}
>>> +}
>>> +
>>> +int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state)
>>> +{
>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> +
>>
>> Should the ->gamma_enable check be here once, instead?
>>
>> With that fixed,
>>
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>>
>>
>> BR,
>> Jani.
>>
>>
>>> +	if (HAS_GMCH(dev_priv)) {
>>> +		if (IS_CHERRYVIEW(dev_priv))
>>> +			return chv_gamma_precision(crtc_state);
>>> +		else
>>> +			return i9xx_gamma_precision(crtc_state);
>>> +	} else {
>>> +		if (INTEL_GEN(dev_priv) >= 11)
>>> +			return icl_gamma_precision(crtc_state);
>>> +		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>>> +			return glk_gamma_precision(crtc_state);
>>> +		else if (IS_IRONLAKE(dev_priv))
>>> +			return ilk_gamma_precision(crtc_state);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  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/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h
>>> index 057e8ac..0226d3a 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_color.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_color.h
>>> @@ -14,5 +14,6 @@
>>>  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);
>>> +int intel_color_get_gamma_bit_precision(const 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v9][PATCH 02/11] drm/i915/display: Add debug log for color parameters
  2019-08-30  8:31 ` [v9][PATCH 02/11] drm/i915/display: Add debug log for color parameters Swati Sharma
@ 2019-08-30 12:06   ` Jani Nikula
  0 siblings, 0 replies; 28+ messages in thread
From: Jani Nikula @ 2019-08-30 12:06 UTC (permalink / raw)
  To: Swati Sharma, intel-gfx; +Cc: daniel.vetter, ankit.k.nautiyal

On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> Add debug log for color related parameters like gamma_mode, gamma_enable,
> csc_enable, etc inside intel_dump_pipe_config().
>
> v6: -Added debug log for color para in intel_dump_pipe_config [Jani]
> v7: -Split patch 3 into 4 patches
> v8: -Corrected alignment [Uma]
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index ea2915d..f9c0842 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12138,6 +12138,15 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
>  
>  	intel_dpll_dump_hw_state(dev_priv, &pipe_config->dpll_hw_state);
>  
> +	if (IS_CHERRYVIEW(dev_priv))
> +		DRM_DEBUG_KMS("cgm_mode: 0x%x gamma_mode: 0x%x gamma_enable: %d csc_enable: %d\n",
> +			      pipe_config->cgm_mode, pipe_config->gamma_mode,
> +			      pipe_config->gamma_enable, pipe_config->csc_enable);
> +	else
> +		DRM_DEBUG_KMS("csc_mode: 0x%x gamma_mode: 0x%x gamma_enable: %d csc_enable: %d\n",
> +			      pipe_config->csc_mode, pipe_config->gamma_mode,
> +			      pipe_config->gamma_enable, pipe_config->csc_enable);
> +
>  dump_planes:
>  	if (!state)
>  		return;

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v9][PATCH 03/11] drm/i915/display: Add func to compare hw/sw gamma lut
  2019-08-30  8:31 ` [v9][PATCH 03/11] drm/i915/display: Add func to compare hw/sw gamma lut Swati Sharma
@ 2019-08-30 12:47   ` Jani Nikula
  2019-09-09 13:53     ` Sharma, Swati2
  0 siblings, 1 reply; 28+ messages in thread
From: Jani Nikula @ 2019-08-30 12:47 UTC (permalink / raw)
  To: Swati Sharma, intel-gfx; +Cc: daniel.vetter, ankit.k.nautiyal

On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> Add func intel_color_lut_equal() to compare hw/sw gamma
> lut values. Since hw/sw gamma lut sizes and lut entries comparison
> will be different for different gamma modes, add gamma mode dependent
> checks.
>
> v3: -Rebase
> v4: -Renamed intel_compare_color_lut() to intel_color_lut_equal() [Jani]
>     -Added the default label above the correct label [Jani]
>     -Corrected smatch warn "variable dereferenced before check"
>      [Dan Carpenter]
> v5: -Added condition (!blob1 && !blob2) return true [Jani]
> v6: -Made patch11 as patch3 [Jani]
> v8: -Split patch 3 into 4 patches
>     -Optimized blob check condition [Ville]
> v9: -Exclude spilt gamma mode (bdw and ivb platforms)
>      as there is exception in way gamma values are written in
>      hardware [Ville]
>     -Added exception made in commit [Uma]
>     -Dropeed else, character limit and indentation [Uma]
>     -Added multi segmented gama mode for icl+ platforms [Uma]
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 104 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_color.h |   6 ++
>  2 files changed, 110 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index dcc65d7..141efb0 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1470,6 +1470,110 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat
>  	return 0;
>  }
>  
> +static inline bool err_check(struct drm_color_lut *sw_lut,

Please drop the inline throughout in .c files. For static functions the
compiler will make the best call what to do here.

> +			     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);
> +}
> +
> +static inline bool intel_color_lut_entry_equal(struct drm_color_lut *sw_lut,
> +					       struct drm_color_lut *hw_lut,
> +					       int hw_lut_size, u32 err)
> +{
> +	int i;
> +
> +	for (i = 0; i < hw_lut_size; i++) {
> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +static inline bool intel_color_lut_entry_equal_multi(struct drm_color_lut *sw_lut,
> +						     struct drm_color_lut *hw_lut,
> +						     u32 err)
> +{
> +	int i;
> +
> +	for (i = 0; i < 9; i++) {
> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
> +			return false;
> +	}
> +
> +	for (i = 1; i <  257; i++) {
> +		if (!err_check(&hw_lut[8 + i], &sw_lut[i * 8], err))
> +			return false;
> +	}
> +
> +	for (i = 0; i < 256; i++) {
> +		if (!err_check(&hw_lut[265 + i], &sw_lut[i * 8 * 128], err))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
> +			   struct drm_property_blob *blob2,
> +			   u32 gamma_mode, u32 bit_precision)
> +{
> +	struct drm_color_lut *sw_lut, *hw_lut;
> +	int sw_lut_size, hw_lut_size;
> +	u32 err;
> +
> +	if (!blob1 != !blob2)
> +		return false;
> +
> +	if (!blob1)
> +		return true;
> +
> +	sw_lut_size = drm_color_lut_size(blob1);
> +	hw_lut_size = drm_color_lut_size(blob2);

Basically the code here shouldn't assume one is hw state and the other
is sw state...

> +
> +	/* check sw and hw lut size */
> +	switch (gamma_mode) {
> +	case GAMMA_MODE_MODE_8BIT:
> +	case GAMMA_MODE_MODE_10BIT:
> +		if (sw_lut_size != hw_lut_size)
> +			return false;
> +		break;
> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> +		if ((sw_lut_size != 262145) || (hw_lut_size != 521))
> +			return false;

The readout code should result in a blob similar to the hardware
state. Assuming distinct hw and sw, you'll bypass fastset on icl
whenever multisegmented gamma is enabled! See
intel_crtc_check_fastset().

Ville also pointed out that on fastboot, the state read out from the
hardware is presented to the userspace, resulting in a bogus 521 lut
size.

So the readout code for multisegmented gamma has to come up with some
intermediate entries that aren't preserved in hardware. Not unlike the
precision is lost in hardware. Those may be read out by the userspace
after fastboot. The compare code has to ignore those interpolated
values, and only look at the values that have been read from the
hardware.

This means intel_color_lut_entry_equal_multi() as-is does not fly.

---

More importantly, this also means the patch can't be merged, and what
could have been straightforward stuff for earlier gens and legacy gamma
keeps being blocked by the more complicated stuff. So despite what I
said in private, I'm afraid the best course of action is indeed to
refactor the series to not include multi-segmented gamma, save it for a
follow-up series.

For example, do the absolute minimal series to add GMCH platform gamma
checks. Could even be without CHV for starters. Add the infrastructure,
get it working, get it off our hands. After that, focus on the next.

BR,
Jani.


> +		break;
> +	default:
> +		MISSING_CASE(gamma_mode);
> +			return false;
> +	}
> +
> +	sw_lut = blob1->data;
> +	hw_lut = blob2->data;
> +
> +	err = 0xffff >> bit_precision;
> +
> +	/* check sw and hw lut entry to be equal */
> +	switch (gamma_mode) {
> +	case GAMMA_MODE_MODE_8BIT:
> +	case GAMMA_MODE_MODE_10BIT:
> +		if (!intel_color_lut_entry_equal(sw_lut, hw_lut,
> +						 hw_lut_size, err))
> +			return false;
> +		break;
> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> +		if (!intel_color_lut_entry_equal_multi(sw_lut, hw_lut, err))
> +			return false;
> +		break;
> +	default:
> +		MISSING_CASE(gamma_mode);
> +			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/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h
> index 0226d3a..173727a 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.h
> +++ b/drivers/gpu/drm/i915/display/intel_color.h
> @@ -6,8 +6,11 @@
>  #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);
> @@ -15,5 +18,8 @@
>  void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
>  void intel_color_get_config(struct intel_crtc_state *crtc_state);
>  int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state);
> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
> +			   struct drm_property_blob *blob2,
> +			   u32 gamma_mode, u32 bit_precision);
>  
>  #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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v9][PATCH 05/11] drm/i915/display: Extract i9xx_read_luts()
  2019-08-30  8:31 ` [v9][PATCH 05/11] drm/i915/display: Extract i9xx_read_luts() Swati Sharma
@ 2019-08-30 12:54   ` Jani Nikula
  0 siblings, 0 replies; 28+ messages in thread
From: Jani Nikula @ 2019-08-30 12:54 UTC (permalink / raw)
  To: Swati Sharma, intel-gfx; +Cc: daniel.vetter, ankit.k.nautiyal

On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> For the legacy(gen < 4) gamma, add hw read out to create hw blob of gamma
> lut values. Also, add function intel_color_lut_pack to convert hw value
> with given bit precision to lut property val.
>
> v4: -No need to initialize *blob [Jani]
>     -Removed right shifts [Jani]
>     -Dropped dev local var [Jani]
> v5: -Returned blob instead of assigning it internally within the
>      function [Ville]
>     -Renamed function i9xx_get_color_config() to i9xx_read_luts()
>     -Renamed i9xx_get_config_internal() to i9xx_read_lut_8() [Ville]
> v9: -Change in commit message [Jani, Uma]
>     -Wrap commit within 75 characters [Uma]
>     -Use macro for 256 [Uma]
>     -Made read func para as const [Ville, Uma]
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 54 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h            |  3 ++
>  2 files changed, 57 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 141efb0..107a03a 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1574,6 +1574,59 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1,
>  	return true;
>  }
>  
> +/* 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 struct drm_property_blob *
> +i9xx_read_lut_8(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	struct drm_property_blob *blob;
> +	struct drm_color_lut *blob_data;
> +	u32 i, val;
> +
> +	blob = drm_property_create_blob(&dev_priv->drm,
> +					sizeof(struct drm_color_lut) * LEGACY_LUT_LENGTH,
> +					NULL);
> +	if (IS_ERR(blob))
> +		return NULL;
> +
> +	blob_data = blob->data;
> +
> +	for (i = 0; i < LEGACY_LUT_LENGTH; 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), 8);
> +		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(
> +							  LGC_PALETTE_GREEN_MASK, val), 8);
> +		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(
> +							 LGC_PALETTE_BLUE_MASK, val), 8);
> +	}
> +
> +	return blob;
> +}
> +
> +void i9xx_read_luts(struct intel_crtc_state *crtc_state)

Should be static, as warned by sparse.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +{
> +	crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
> +}
> +
>  void intel_color_init(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -1594,6 +1647,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.read_luts = i9xx_read_luts;
>  		}
>  	} else {
>  		if (INTEL_GEN(dev_priv) >= 11)
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 02e1ef1..09ea5b1 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7192,6 +7192,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 */

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v9][PATCH 06/11] drm/i915/display: Extract i965_read_luts()
  2019-08-30  8:31 ` [v9][PATCH 06/11] drm/i915/display: Extract i965_read_luts() Swati Sharma
@ 2019-08-30 13:08   ` Jani Nikula
  0 siblings, 0 replies; 28+ messages in thread
From: Jani Nikula @ 2019-08-30 13:08 UTC (permalink / raw)
  To: Swati Sharma, intel-gfx; +Cc: daniel.vetter, ankit.k.nautiyal

On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> For i965, add hw read out to create hw blob of gamma
> lut values.
>
> v4: -No need to initialize *blob [Jani]
>     -Removed right shifts [Jani]
>     -Dropped dev local var [Jani]
> v5: -Returned blob instead of assigning it internally
>      within the function [Ville]
>     -Renamed i965_get_color_config() to i965_read_lut() [Ville]
>     -Renamed i965_get_gamma_config_10p6() to i965_read_gamma_lut_10p6()
>      [Ville]
> v9: -Typo and 80 character limit [Uma]
>     -Made read func para as const [Ville, Uma]
>     -Renamed i965_read_gamma_lut_10p6() to i965_read_lut_10p6() [Ville, Uma]
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 43 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h            |  3 +++
>  2 files changed, 46 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 107a03a..f87bb76 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1627,6 +1627,48 @@ void i9xx_read_luts(struct intel_crtc_state *crtc_state)
>  	crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
>  }
>  
> +static struct drm_property_blob *
> +i965_read_lut_10p6(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> +	enum pipe pipe = crtc->pipe;
> +	struct drm_property_blob *blob;
> +	struct drm_color_lut *blob_data;
> +	u32 i, val1, val2;
> +
> +	blob = drm_property_create_blob(&dev_priv->drm,
> +					sizeof(struct drm_color_lut) * lut_size,
> +					NULL);
> +	if (IS_ERR(blob))
> +		return NULL;
> +
> +	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) << 8 |
> +						 REG_FIELD_GET(PALETTE_RED_MASK, val2);
> +		blob_data[i].green = REG_FIELD_GET(PALETTE_GREEN_MASK, val1) << 8 |
> +						   REG_FIELD_GET(PALETTE_GREEN_MASK, val2);
> +		blob_data[i].blue = REG_FIELD_GET(PALETTE_BLUE_MASK, val1) << 8 |
> +						  REG_FIELD_GET(PALETTE_BLUE_MASK, val2);

Please double check the property -> i965_load_lut_10p6 ->
i965_read_lut_10p6 -> property round trip. Does this not swap the hi and
lo bytes?

And why doesn't CI catch this...?

BR,
Jani.


> +	}
> +
> +	return blob;
> +}
> +
> +static void i965_read_luts(struct intel_crtc_state *crtc_state)
> +{
> +	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
> +		crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
> +	else
> +		crtc_state->base.gamma_lut = i965_read_lut_10p6(crtc_state);
> +}
> +
>  void intel_color_init(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -1642,6 +1684,7 @@ void intel_color_init(struct intel_crtc *crtc)
>  		} else if (INTEL_GEN(dev_priv) >= 4) {
>  			dev_priv->display.color_check = i9xx_color_check;
>  			dev_priv->display.color_commit = i9xx_color_commit;
> +			dev_priv->display.read_luts = i965_read_luts;
>  			dev_priv->display.load_luts = i965_load_luts;
>  		} else {
>  			dev_priv->display.color_check = i9xx_color_check;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 09ea5b1..7e66673 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3558,6 +3558,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #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) + \

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v9][PATCH 07/11] drm/i915/display: Extract chv_read_luts()
  2019-08-30  8:31 ` [v9][PATCH 07/11] drm/i915/display: Extract chv_read_luts() Swati Sharma
@ 2019-08-30 13:15   ` Jani Nikula
  0 siblings, 0 replies; 28+ messages in thread
From: Jani Nikula @ 2019-08-30 13:15 UTC (permalink / raw)
  To: Swati Sharma, intel-gfx; +Cc: daniel.vetter, ankit.k.nautiyal

On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> For cherryview, add hw read out to create hw blob of gamma
> lut values.
>
> v4: -No need to initialize *blob [Jani]
>     -Removed right shifts [Jani]
>     -Dropped dev local var [Jani]
> v5: -Returned blob instead of assigning it internally within the
>      function [Ville]
>     -Renamed function cherryview_get_color_config() to chv_read_luts()
>     -Renamed cherryview_get_gamma_config() to chv_read_cgm_gamma_lut()
>      [Ville]
> v9: -80 character limit [Uma]
>     -Made read func para as const [Ville, Uma]
>     -Renamed chv_read_cgm_gamma_lut() to chv_read_cgm_gamma_lut()
>      [Ville, Uma]
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 43 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h            |  3 +++
>  2 files changed, 46 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index f87bb76..b28b3b9 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1669,6 +1669,48 @@ static void i965_read_luts(struct intel_crtc_state *crtc_state)
>  		crtc_state->base.gamma_lut = i965_read_lut_10p6(crtc_state);
>  }
>  
> +static struct drm_property_blob *
> +chv_read_cgm_lut(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> +	enum pipe pipe = crtc->pipe;
> +	struct drm_property_blob *blob;
> +	struct drm_color_lut *blob_data;
> +	u32 i, val;
> +
> +	blob = drm_property_create_blob(&dev_priv->drm,
> +					sizeof(struct drm_color_lut) * lut_size,
> +					NULL);
> +	if (IS_ERR(blob))
> +		return NULL;
> +
> +	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), 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);
> +	}
> +
> +	return blob;
> +}
> +
> +static void chv_read_luts(struct intel_crtc_state *crtc_state)
> +{
> +	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
> +		crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
> +	else
> +		crtc_state->base.gamma_lut = chv_read_cgm_lut(crtc_state);
> +}
> +
>  void intel_color_init(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -1681,6 +1723,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 = chv_load_luts;
> +			dev_priv->display.read_luts = chv_read_luts;
>  		} else if (INTEL_GEN(dev_priv) >= 4) {
>  			dev_priv->display.color_check = i9xx_color_check;
>  			dev_priv->display.color_commit = i9xx_color_commit;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7e66673..f5a2e73 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -10311,6 +10311,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)

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v9][PATCH 08/11] drm/i915/display: Extract ilk_read_luts()
  2019-08-30  8:31 ` [v9][PATCH 08/11] drm/i915/display: Extract ilk_read_luts() Swati Sharma
@ 2019-08-30 13:18   ` Jani Nikula
  0 siblings, 0 replies; 28+ messages in thread
From: Jani Nikula @ 2019-08-30 13:18 UTC (permalink / raw)
  To: Swati Sharma, intel-gfx; +Cc: daniel.vetter, ankit.k.nautiyal

On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> For ilk, add hw read out to create hw blob of gamma
> lut values.
>
> v4: -No need to initialize *blob [Jani]
>     -Removed right shifts [Jani]
>     -Dropped dev local var [Jani]
> v5: -Returned blob instead of assigning it internally within the
>      function [Ville]
>     -Renamed ilk_get_color_config() to ilk_read_luts() [Ville]
> v9: -80 character limit [Uma]
>     -Made read func para as const [Ville, Uma]
>     -Renamed ilk_read_gamma_lut() to ilk_read_lut_10() [Uma, Ville]
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 45 +++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h            |  3 ++
>  2 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index b28b3b9..33bcbda 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1711,6 +1711,47 @@ static void chv_read_luts(struct intel_crtc_state *crtc_state)
>  		crtc_state->base.gamma_lut = chv_read_cgm_lut(crtc_state);
>  }
>  
> +static struct drm_property_blob *
> +ilk_read_lut_10(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> +	enum pipe pipe = crtc->pipe;
> +	struct drm_property_blob *blob;
> +	struct drm_color_lut *blob_data;
> +	u32 i, val;
> +
> +	blob = drm_property_create_blob(&dev_priv->drm,
> +					sizeof(struct drm_color_lut) * lut_size,
> +					NULL);
> +	if (IS_ERR(blob))
> +		return NULL;
> +
> +	blob_data = blob->data;
> +
> +	for (i = 0; i < lut_size - 1; i++) {

ilk_load_lut_10 has lut_size, not (lut_size - 1).

With that fixed,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +		val = I915_READ(PREC_PALETTE(pipe, i));
> +
> +		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(
> +							PREC_PALETTE_RED_MASK, val), 10);
> +		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(
> +							  PREC_PALETTE_GREEN_MASK, val), 10);
> +		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(
> +							 PREC_PALETTE_BLUE_MASK, val), 10);
> +	}
> +
> +	return blob;
> +}
> +
> +static void ilk_read_luts(struct intel_crtc_state *crtc_state)
> +{
> +	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
> +		crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state);
> +	else
> +		crtc_state->base.gamma_lut = ilk_read_lut_10(crtc_state);
> +}
> +
>  void intel_color_init(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -1760,8 +1801,10 @@ void intel_color_init(struct intel_crtc *crtc)
>  			dev_priv->display.load_luts = bdw_load_luts;
>  		else if (INTEL_GEN(dev_priv) >= 7)
>  			dev_priv->display.load_luts = ivb_load_luts;
> -		else
> +		else {
>  			dev_priv->display.load_luts = ilk_load_luts;
> +			dev_priv->display.read_luts = ilk_read_luts;
> +		}
>  	}
>  
>  	drm_crtc_enable_color_mgmt(&crtc->base,
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f5a2e73..e457739 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7203,6 +7203,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

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915: adding state checker for gamma lut values (rev13)
  2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
                   ` (13 preceding siblings ...)
  2019-08-30 10:01 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2019-08-31  3:01 ` Patchwork
  14 siblings, 0 replies; 28+ messages in thread
From: Patchwork @ 2019-08-31  3:01 UTC (permalink / raw)
  To: Swati Sharma; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_6807_full -> Patchwork_14233_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_parallel@rcs0-contexts:
    - shard-hsw:          [PASS][1] -> [TIMEOUT][2] ([fdo#111469])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/shard-hsw4/igt@gem_exec_parallel@rcs0-contexts.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/shard-hsw4/igt@gem_exec_parallel@rcs0-contexts.html

  * igt@gem_exec_reloc@basic-write-wc-active:
    - shard-skl:          [PASS][3] -> [DMESG-WARN][4] ([fdo#106107])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/shard-skl2/igt@gem_exec_reloc@basic-write-wc-active.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/shard-skl6/igt@gem_exec_reloc@basic-write-wc-active.html

  * igt@gem_exec_suspend@basic-s3:
    - shard-snb:          [PASS][5] -> [DMESG-WARN][6] ([fdo#110684] / [fdo#111115])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/shard-snb2/igt@gem_exec_suspend@basic-s3.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/shard-snb2/igt@gem_exec_suspend@basic-s3.html

  * igt@i915_pm_rpm@modeset-stress-extra-wait:
    - shard-apl:          [PASS][7] -> [INCOMPLETE][8] ([fdo#103927]) +2 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/shard-apl3/igt@i915_pm_rpm@modeset-stress-extra-wait.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/shard-apl7/igt@i915_pm_rpm@modeset-stress-extra-wait.html

  * igt@i915_suspend@sysfs-reader:
    - shard-apl:          [PASS][9] -> [DMESG-WARN][10] ([fdo#108566]) +2 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/shard-apl1/igt@i915_suspend@sysfs-reader.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/shard-apl7/igt@i915_suspend@sysfs-reader.html

  * igt@kms_cursor_crc@pipe-b-cursor-suspend:
    - shard-skl:          [PASS][11] -> [INCOMPLETE][12] ([fdo#110741])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/shard-skl5/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/shard-skl9/igt@kms_cursor_crc@pipe-b-cursor-suspend.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([fdo#102887])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/shard-skl2/igt@kms_flip@flip-vs-expired-vblank.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/shard-skl7/igt@kms_flip@flip-vs-expired-vblank.html

  
#### Possible fixes ####

  * igt@i915_pm_rpm@i2c:
    - shard-hsw:          [FAIL][15] ([fdo#104097]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/shard-hsw2/igt@i915_pm_rpm@i2c.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/shard-hsw8/igt@i915_pm_rpm@i2c.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-skl:          [INCOMPLETE][17] ([fdo#110741]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/shard-skl6/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/shard-skl5/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [FAIL][19] ([fdo#105363]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/shard-skl2/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/shard-skl3/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-hsw:          [INCOMPLETE][21] ([fdo#103540]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/shard-hsw5/igt@kms_flip@flip-vs-suspend-interruptible.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/shard-hsw2/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-apl:          [DMESG-WARN][23] ([fdo#108566]) -> [PASS][24] +4 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6807/shard-apl4/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14233/shard-apl4/igt@kms_frontbuffer_tracking@fbc-suspend.html

  
  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104097]: https://bugs.freedesktop.org/show_bug.cgi?id=104097
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#106107]: https://bugs.freedesktop.org/show_bug.cgi?id=106107
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#110684]: https://bugs.freedesktop.org/show_bug.cgi?id=110684
  [fdo#110741]: https://bugs.freedesktop.org/show_bug.cgi?id=110741
  [fdo#111115]: https://bugs.freedesktop.org/show_bug.cgi?id=111115
  [fdo#111469]: https://bugs.freedesktop.org/show_bug.cgi?id=111469


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6807 -> Patchwork_14233

  CI-20190529: 20190529
  CI_DRM_6807: 7dfe53944a301f8b7946e69edb570661798dd0b9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5159: c06ee2989c012fcc43cd361e75e9ee5a3a9292df @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14233: 324b6c5f95e64bb0c2bee0e4c9faee606d9b12d5 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v9][PATCH 03/11] drm/i915/display: Add func to compare hw/sw gamma lut
  2019-08-30 12:47   ` Jani Nikula
@ 2019-09-09 13:53     ` Sharma, Swati2
  2019-09-09 14:14       ` Jani Nikula
  0 siblings, 1 reply; 28+ messages in thread
From: Sharma, Swati2 @ 2019-09-09 13:53 UTC (permalink / raw)
  To: Jani Nikula, intel-gfx; +Cc: daniel.vetter, ankit.k.nautiyal

On 30-Aug-19 6:17 PM, Jani Nikula wrote:
> On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
>> Add func intel_color_lut_equal() to compare hw/sw gamma
>> lut values. Since hw/sw gamma lut sizes and lut entries comparison
>> will be different for different gamma modes, add gamma mode dependent
>> checks.
>>
>> v3: -Rebase
>> v4: -Renamed intel_compare_color_lut() to intel_color_lut_equal() [Jani]
>>      -Added the default label above the correct label [Jani]
>>      -Corrected smatch warn "variable dereferenced before check"
>>       [Dan Carpenter]
>> v5: -Added condition (!blob1 && !blob2) return true [Jani]
>> v6: -Made patch11 as patch3 [Jani]
>> v8: -Split patch 3 into 4 patches
>>      -Optimized blob check condition [Ville]
>> v9: -Exclude spilt gamma mode (bdw and ivb platforms)
>>       as there is exception in way gamma values are written in
>>       hardware [Ville]
>>      -Added exception made in commit [Uma]
>>      -Dropeed else, character limit and indentation [Uma]
>>      -Added multi segmented gama mode for icl+ platforms [Uma]
>>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_color.c | 104 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_color.h |   6 ++
>>   2 files changed, 110 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>> index dcc65d7..141efb0 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> @@ -1470,6 +1470,110 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat
>>   	return 0;
>>   }
>>   
>> +static inline bool err_check(struct drm_color_lut *sw_lut,
> 
> Please drop the inline throughout in .c files. For static functions the
> compiler will make the best call what to do here.
> 
>> +			     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);
>> +}
>> +
>> +static inline bool intel_color_lut_entry_equal(struct drm_color_lut *sw_lut,
>> +					       struct drm_color_lut *hw_lut,
>> +					       int hw_lut_size, u32 err)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < hw_lut_size; i++) {
>> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +static inline bool intel_color_lut_entry_equal_multi(struct drm_color_lut *sw_lut,
>> +						     struct drm_color_lut *hw_lut,
>> +						     u32 err)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < 9; i++) {
>> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
>> +			return false;
>> +	}
>> +
>> +	for (i = 1; i <  257; i++) {
>> +		if (!err_check(&hw_lut[8 + i], &sw_lut[i * 8], err))
>> +			return false;
>> +	}
>> +
>> +	for (i = 0; i < 256; i++) {
>> +		if (!err_check(&hw_lut[265 + i], &sw_lut[i * 8 * 128], err))
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
>> +			   struct drm_property_blob *blob2,
>> +			   u32 gamma_mode, u32 bit_precision)
>> +{
>> +	struct drm_color_lut *sw_lut, *hw_lut;
>> +	int sw_lut_size, hw_lut_size;
>> +	u32 err;
>> +
>> +	if (!blob1 != !blob2)
>> +		return false;
>> +
>> +	if (!blob1)
>> +		return true;
>> +
>> +	sw_lut_size = drm_color_lut_size(blob1);
>> +	hw_lut_size = drm_color_lut_size(blob2);
> 
> Basically the code here shouldn't assume one is hw state and the other
> is sw state...
> 
>> +
>> +	/* check sw and hw lut size */
>> +	switch (gamma_mode) {
>> +	case GAMMA_MODE_MODE_8BIT:
>> +	case GAMMA_MODE_MODE_10BIT:
>> +		if (sw_lut_size != hw_lut_size)
>> +			return false;
>> +		break;
>> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>> +		if ((sw_lut_size != 262145) || (hw_lut_size != 521))
>> +			return false;
> 
> The readout code should result in a blob similar to the hardware
> state. Assuming distinct hw and sw, you'll bypass fastset on icl
> whenever multisegmented gamma is enabled! See
> intel_crtc_check_fastset().
> 
> Ville also pointed out that on fastboot, the state read out from the
> hardware is presented to the userspace, resulting in a bogus 521 lut
> size.
> 
> So the readout code for multisegmented gamma has to come up with some
> intermediate entries that aren't preserved in hardware. Not unlike the
> precision is lost in hardware. Those may be read out by the userspace
> after fastboot. The compare code has to ignore those interpolated
> values, and only look at the values that have been read from the
> hardware.
> 
Hi Jani,
As you stated readout code should result in a blob similar to hardware
state and readout code for multi-seg gamma has to come up with 
"intermediate values". Do these intermediate values need to be some
logical values or any junk values while creating a hw blob?

> This means intel_color_lut_entry_equal_multi() as-is does not fly.
> 
> ---
> 
> More importantly, this also means the patch can't be merged, and what
> could have been straightforward stuff for earlier gens and legacy gamma
> keeps being blocked by the more complicated stuff. So despite what I
> said in private, I'm afraid the best course of action is indeed to
> refactor the series to not include multi-segmented gamma, save it for a
> follow-up series.
> 
> For example, do the absolute minimal series to add GMCH platform gamma
> checks. Could even be without CHV for starters. Add the infrastructure,
> get it working, get it off our hands. After that, focus on the next.
> 
> BR,
> Jani.
> 
> 
>> +		break;
>> +	default:
>> +		MISSING_CASE(gamma_mode);
>> +			return false;
>> +	}
>> +
>> +	sw_lut = blob1->data;
>> +	hw_lut = blob2->data;
>> +
>> +	err = 0xffff >> bit_precision;
>> +
>> +	/* check sw and hw lut entry to be equal */
>> +	switch (gamma_mode) {
>> +	case GAMMA_MODE_MODE_8BIT:
>> +	case GAMMA_MODE_MODE_10BIT:
>> +		if (!intel_color_lut_entry_equal(sw_lut, hw_lut,
>> +						 hw_lut_size, err))
>> +			return false;
>> +		break;
>> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>> +		if (!intel_color_lut_entry_equal_multi(sw_lut, hw_lut, err))
>> +			return false;
>> +		break;
>> +	default:
>> +		MISSING_CASE(gamma_mode);
>> +			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/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h
>> index 0226d3a..173727a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.h
>> +++ b/drivers/gpu/drm/i915/display/intel_color.h
>> @@ -6,8 +6,11 @@
>>   #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);
>> @@ -15,5 +18,8 @@
>>   void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
>>   void intel_color_get_config(struct intel_crtc_state *crtc_state);
>>   int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state);
>> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
>> +			   struct drm_property_blob *blob2,
>> +			   u32 gamma_mode, u32 bit_precision);
>>   
>>   #endif /* __INTEL_COLOR_H__ */
> 


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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v9][PATCH 03/11] drm/i915/display: Add func to compare hw/sw gamma lut
  2019-09-09 13:53     ` Sharma, Swati2
@ 2019-09-09 14:14       ` Jani Nikula
  2019-09-09 14:19         ` Ville Syrjälä
  0 siblings, 1 reply; 28+ messages in thread
From: Jani Nikula @ 2019-09-09 14:14 UTC (permalink / raw)
  To: Sharma, Swati2, intel-gfx; +Cc: daniel.vetter, ankit.k.nautiyal

On Mon, 09 Sep 2019, "Sharma, Swati2" <swati2.sharma@intel.com> wrote:
> On 30-Aug-19 6:17 PM, Jani Nikula wrote:
>> On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
>>> Add func intel_color_lut_equal() to compare hw/sw gamma
>>> lut values. Since hw/sw gamma lut sizes and lut entries comparison
>>> will be different for different gamma modes, add gamma mode dependent
>>> checks.
>>>
>>> v3: -Rebase
>>> v4: -Renamed intel_compare_color_lut() to intel_color_lut_equal() [Jani]
>>>      -Added the default label above the correct label [Jani]
>>>      -Corrected smatch warn "variable dereferenced before check"
>>>       [Dan Carpenter]
>>> v5: -Added condition (!blob1 && !blob2) return true [Jani]
>>> v6: -Made patch11 as patch3 [Jani]
>>> v8: -Split patch 3 into 4 patches
>>>      -Optimized blob check condition [Ville]
>>> v9: -Exclude spilt gamma mode (bdw and ivb platforms)
>>>       as there is exception in way gamma values are written in
>>>       hardware [Ville]
>>>      -Added exception made in commit [Uma]
>>>      -Dropeed else, character limit and indentation [Uma]
>>>      -Added multi segmented gama mode for icl+ platforms [Uma]
>>>
>>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/display/intel_color.c | 104 +++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/display/intel_color.h |   6 ++
>>>   2 files changed, 110 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>>> index dcc65d7..141efb0 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>>> @@ -1470,6 +1470,110 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat
>>>   	return 0;
>>>   }
>>>   
>>> +static inline bool err_check(struct drm_color_lut *sw_lut,
>> 
>> Please drop the inline throughout in .c files. For static functions the
>> compiler will make the best call what to do here.
>> 
>>> +			     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);
>>> +}
>>> +
>>> +static inline bool intel_color_lut_entry_equal(struct drm_color_lut *sw_lut,
>>> +					       struct drm_color_lut *hw_lut,
>>> +					       int hw_lut_size, u32 err)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i < hw_lut_size; i++) {
>>> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
>>> +			return false;
>>> +	}
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +static inline bool intel_color_lut_entry_equal_multi(struct drm_color_lut *sw_lut,
>>> +						     struct drm_color_lut *hw_lut,
>>> +						     u32 err)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i < 9; i++) {
>>> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
>>> +			return false;
>>> +	}
>>> +
>>> +	for (i = 1; i <  257; i++) {
>>> +		if (!err_check(&hw_lut[8 + i], &sw_lut[i * 8], err))
>>> +			return false;
>>> +	}
>>> +
>>> +	for (i = 0; i < 256; i++) {
>>> +		if (!err_check(&hw_lut[265 + i], &sw_lut[i * 8 * 128], err))
>>> +			return false;
>>> +	}
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
>>> +			   struct drm_property_blob *blob2,
>>> +			   u32 gamma_mode, u32 bit_precision)
>>> +{
>>> +	struct drm_color_lut *sw_lut, *hw_lut;
>>> +	int sw_lut_size, hw_lut_size;
>>> +	u32 err;
>>> +
>>> +	if (!blob1 != !blob2)
>>> +		return false;
>>> +
>>> +	if (!blob1)
>>> +		return true;
>>> +
>>> +	sw_lut_size = drm_color_lut_size(blob1);
>>> +	hw_lut_size = drm_color_lut_size(blob2);
>> 
>> Basically the code here shouldn't assume one is hw state and the other
>> is sw state...
>> 
>>> +
>>> +	/* check sw and hw lut size */
>>> +	switch (gamma_mode) {
>>> +	case GAMMA_MODE_MODE_8BIT:
>>> +	case GAMMA_MODE_MODE_10BIT:
>>> +		if (sw_lut_size != hw_lut_size)
>>> +			return false;
>>> +		break;
>>> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>>> +		if ((sw_lut_size != 262145) || (hw_lut_size != 521))
>>> +			return false;
>> 
>> The readout code should result in a blob similar to the hardware
>> state. Assuming distinct hw and sw, you'll bypass fastset on icl
>> whenever multisegmented gamma is enabled! See
>> intel_crtc_check_fastset().
>> 
>> Ville also pointed out that on fastboot, the state read out from the
>> hardware is presented to the userspace, resulting in a bogus 521 lut
>> size.
>> 
>> So the readout code for multisegmented gamma has to come up with some
>> intermediate entries that aren't preserved in hardware. Not unlike the
>> precision is lost in hardware. Those may be read out by the userspace
>> after fastboot. The compare code has to ignore those interpolated
>> values, and only look at the values that have been read from the
>> hardware.
>> 
> Hi Jani,
> As you stated readout code should result in a blob similar to hardware
> state and readout code for multi-seg gamma has to come up with 
> "intermediate values". Do these intermediate values need to be some
> logical values or any junk values while creating a hw blob?

Preferrably sensible values, as they may be read out by the userspace
after fastboot. But it can be the simplest interpolation I think.

BR,
Jani.





>
>> This means intel_color_lut_entry_equal_multi() as-is does not fly.
>> 
>> ---
>> 
>> More importantly, this also means the patch can't be merged, and what
>> could have been straightforward stuff for earlier gens and legacy gamma
>> keeps being blocked by the more complicated stuff. So despite what I
>> said in private, I'm afraid the best course of action is indeed to
>> refactor the series to not include multi-segmented gamma, save it for a
>> follow-up series.
>> 
>> For example, do the absolute minimal series to add GMCH platform gamma
>> checks. Could even be without CHV for starters. Add the infrastructure,
>> get it working, get it off our hands. After that, focus on the next.
>> 
>> BR,
>> Jani.
>> 
>> 
>>> +		break;
>>> +	default:
>>> +		MISSING_CASE(gamma_mode);
>>> +			return false;
>>> +	}
>>> +
>>> +	sw_lut = blob1->data;
>>> +	hw_lut = blob2->data;
>>> +
>>> +	err = 0xffff >> bit_precision;
>>> +
>>> +	/* check sw and hw lut entry to be equal */
>>> +	switch (gamma_mode) {
>>> +	case GAMMA_MODE_MODE_8BIT:
>>> +	case GAMMA_MODE_MODE_10BIT:
>>> +		if (!intel_color_lut_entry_equal(sw_lut, hw_lut,
>>> +						 hw_lut_size, err))
>>> +			return false;
>>> +		break;
>>> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>>> +		if (!intel_color_lut_entry_equal_multi(sw_lut, hw_lut, err))
>>> +			return false;
>>> +		break;
>>> +	default:
>>> +		MISSING_CASE(gamma_mode);
>>> +			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/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h
>>> index 0226d3a..173727a 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_color.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_color.h
>>> @@ -6,8 +6,11 @@
>>>   #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);
>>> @@ -15,5 +18,8 @@
>>>   void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
>>>   void intel_color_get_config(struct intel_crtc_state *crtc_state);
>>>   int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state);
>>> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
>>> +			   struct drm_property_blob *blob2,
>>> +			   u32 gamma_mode, u32 bit_precision);
>>>   
>>>   #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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v9][PATCH 03/11] drm/i915/display: Add func to compare hw/sw gamma lut
  2019-09-09 14:14       ` Jani Nikula
@ 2019-09-09 14:19         ` Ville Syrjälä
  0 siblings, 0 replies; 28+ messages in thread
From: Ville Syrjälä @ 2019-09-09 14:19 UTC (permalink / raw)
  To: Jani Nikula; +Cc: daniel.vetter, intel-gfx, ankit.k.nautiyal

On Mon, Sep 09, 2019 at 05:14:05PM +0300, Jani Nikula wrote:
> On Mon, 09 Sep 2019, "Sharma, Swati2" <swati2.sharma@intel.com> wrote:
> > On 30-Aug-19 6:17 PM, Jani Nikula wrote:
> >> On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> >>> Add func intel_color_lut_equal() to compare hw/sw gamma
> >>> lut values. Since hw/sw gamma lut sizes and lut entries comparison
> >>> will be different for different gamma modes, add gamma mode dependent
> >>> checks.
> >>>
> >>> v3: -Rebase
> >>> v4: -Renamed intel_compare_color_lut() to intel_color_lut_equal() [Jani]
> >>>      -Added the default label above the correct label [Jani]
> >>>      -Corrected smatch warn "variable dereferenced before check"
> >>>       [Dan Carpenter]
> >>> v5: -Added condition (!blob1 && !blob2) return true [Jani]
> >>> v6: -Made patch11 as patch3 [Jani]
> >>> v8: -Split patch 3 into 4 patches
> >>>      -Optimized blob check condition [Ville]
> >>> v9: -Exclude spilt gamma mode (bdw and ivb platforms)
> >>>       as there is exception in way gamma values are written in
> >>>       hardware [Ville]
> >>>      -Added exception made in commit [Uma]
> >>>      -Dropeed else, character limit and indentation [Uma]
> >>>      -Added multi segmented gama mode for icl+ platforms [Uma]
> >>>
> >>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/display/intel_color.c | 104 +++++++++++++++++++++++++++++
> >>>   drivers/gpu/drm/i915/display/intel_color.h |   6 ++
> >>>   2 files changed, 110 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> >>> index dcc65d7..141efb0 100644
> >>> --- a/drivers/gpu/drm/i915/display/intel_color.c
> >>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> >>> @@ -1470,6 +1470,110 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat
> >>>   	return 0;
> >>>   }
> >>>   
> >>> +static inline bool err_check(struct drm_color_lut *sw_lut,
> >> 
> >> Please drop the inline throughout in .c files. For static functions the
> >> compiler will make the best call what to do here.
> >> 
> >>> +			     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);
> >>> +}
> >>> +
> >>> +static inline bool intel_color_lut_entry_equal(struct drm_color_lut *sw_lut,
> >>> +					       struct drm_color_lut *hw_lut,
> >>> +					       int hw_lut_size, u32 err)
> >>> +{
> >>> +	int i;
> >>> +
> >>> +	for (i = 0; i < hw_lut_size; i++) {
> >>> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
> >>> +			return false;
> >>> +	}
> >>> +
> >>> +	return true;
> >>> +}
> >>> +
> >>> +static inline bool intel_color_lut_entry_equal_multi(struct drm_color_lut *sw_lut,
> >>> +						     struct drm_color_lut *hw_lut,
> >>> +						     u32 err)
> >>> +{
> >>> +	int i;
> >>> +
> >>> +	for (i = 0; i < 9; i++) {
> >>> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
> >>> +			return false;
> >>> +	}
> >>> +
> >>> +	for (i = 1; i <  257; i++) {
> >>> +		if (!err_check(&hw_lut[8 + i], &sw_lut[i * 8], err))
> >>> +			return false;
> >>> +	}
> >>> +
> >>> +	for (i = 0; i < 256; i++) {
> >>> +		if (!err_check(&hw_lut[265 + i], &sw_lut[i * 8 * 128], err))
> >>> +			return false;
> >>> +	}
> >>> +
> >>> +	return true;
> >>> +}
> >>> +
> >>> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
> >>> +			   struct drm_property_blob *blob2,
> >>> +			   u32 gamma_mode, u32 bit_precision)
> >>> +{
> >>> +	struct drm_color_lut *sw_lut, *hw_lut;
> >>> +	int sw_lut_size, hw_lut_size;
> >>> +	u32 err;
> >>> +
> >>> +	if (!blob1 != !blob2)
> >>> +		return false;
> >>> +
> >>> +	if (!blob1)
> >>> +		return true;
> >>> +
> >>> +	sw_lut_size = drm_color_lut_size(blob1);
> >>> +	hw_lut_size = drm_color_lut_size(blob2);
> >> 
> >> Basically the code here shouldn't assume one is hw state and the other
> >> is sw state...
> >> 
> >>> +
> >>> +	/* check sw and hw lut size */
> >>> +	switch (gamma_mode) {
> >>> +	case GAMMA_MODE_MODE_8BIT:
> >>> +	case GAMMA_MODE_MODE_10BIT:
> >>> +		if (sw_lut_size != hw_lut_size)
> >>> +			return false;
> >>> +		break;
> >>> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> >>> +		if ((sw_lut_size != 262145) || (hw_lut_size != 521))
> >>> +			return false;
> >> 
> >> The readout code should result in a blob similar to the hardware
> >> state. Assuming distinct hw and sw, you'll bypass fastset on icl
> >> whenever multisegmented gamma is enabled! See
> >> intel_crtc_check_fastset().
> >> 
> >> Ville also pointed out that on fastboot, the state read out from the
> >> hardware is presented to the userspace, resulting in a bogus 521 lut
> >> size.
> >> 
> >> So the readout code for multisegmented gamma has to come up with some
> >> intermediate entries that aren't preserved in hardware. Not unlike the
> >> precision is lost in hardware. Those may be read out by the userspace
> >> after fastboot. The compare code has to ignore those interpolated
> >> values, and only look at the values that have been read from the
> >> hardware.
> >> 
> > Hi Jani,
> > As you stated readout code should result in a blob similar to hardware
> > state and readout code for multi-seg gamma has to come up with 
> > "intermediate values". Do these intermediate values need to be some
> > logical values or any junk values while creating a hw blob?
> 
> Preferrably sensible values, as they may be read out by the userspace
> after fastboot. But it can be the simplest interpolation I think.

The hardware uses linear interpolation anyway, so no point in doing
anything fancier in software either.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2019-09-09 14:19 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-30  8:31 [v9][PATCH 00/11] drm/i915: adding state checker for gamma lut values Swati Sharma
2019-08-30  8:31 ` [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision Swati Sharma
2019-08-30 12:02   ` Jani Nikula
2019-08-30 12:04     ` Jani Nikula
2019-08-30 12:05       ` Jani Nikula
2019-08-30  8:31 ` [v9][PATCH 02/11] drm/i915/display: Add debug log for color parameters Swati Sharma
2019-08-30 12:06   ` Jani Nikula
2019-08-30  8:31 ` [v9][PATCH 03/11] drm/i915/display: Add func to compare hw/sw gamma lut Swati Sharma
2019-08-30 12:47   ` Jani Nikula
2019-09-09 13:53     ` Sharma, Swati2
2019-09-09 14:14       ` Jani Nikula
2019-09-09 14:19         ` Ville Syrjälä
2019-08-30  8:31 ` [v9][PATCH 04/11] drm/i915/display: Add macro to compare gamma hw/sw lut Swati Sharma
2019-08-30  8:31 ` [v9][PATCH 05/11] drm/i915/display: Extract i9xx_read_luts() Swati Sharma
2019-08-30 12:54   ` Jani Nikula
2019-08-30  8:31 ` [v9][PATCH 06/11] drm/i915/display: Extract i965_read_luts() Swati Sharma
2019-08-30 13:08   ` Jani Nikula
2019-08-30  8:31 ` [v9][PATCH 07/11] drm/i915/display: Extract chv_read_luts() Swati Sharma
2019-08-30 13:15   ` Jani Nikula
2019-08-30  8:31 ` [v9][PATCH 08/11] drm/i915/display: Extract ilk_read_luts() Swati Sharma
2019-08-30 13:18   ` Jani Nikula
2019-08-30  8:31 ` [v9][PATCH 09/11] drm/i915/display: Extract glk_read_luts() Swati Sharma
2019-08-30  8:31 ` [v9][PATCH 10/11] drm/i915/display: Extract icl_read_luts() Swati Sharma
2019-08-30  8:31 ` [v9][PATCH 11/11] FOR_TESTING_ONLY: Print rgb values of hw and sw blobs Swati Sharma
2019-08-30  9:39 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: adding state checker for gamma lut values (rev13) Patchwork
2019-08-30  9:44 ` ✗ Fi.CI.SPARSE: " Patchwork
2019-08-30 10:01 ` ✓ Fi.CI.BAT: success " Patchwork
2019-08-31  3:01 ` ✓ Fi.CI.IGT: " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.