diff mbox series

[v7,03/12] drm/i915: Add func to compare hw/sw gamma lut

Message ID 1559123462-7343-4-git-send-email-swati2.sharma@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: adding state checker for gamma lut values | expand

Commit Message

Sharma, Swati2 May 29, 2019, 9:50 a.m. UTC
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]
    -Called PIPE_CONF_CHECK_COLOR_LUT inside if (!adjust) [Jani]
    -Added #undef PIPE_CONF_CHECK_COLOR_LUT [Jani]
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]
    -Added debug log for color para in intel_dump_pipe_config [Jani]
    -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

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

Comments

Ville Syrjala May 31, 2019, 3:28 p.m. UTC | #1
On Wed, May 29, 2019 at 03:20:53PM +0530, Swati Sharma wrote:
> 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]
>     -Called PIPE_CONF_CHECK_COLOR_LUT inside if (!adjust) [Jani]
>     -Added #undef PIPE_CONF_CHECK_COLOR_LUT [Jani]
> 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]
>     -Added debug log for color para in intel_dump_pipe_config [Jani]
>     -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
> 
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c   | 166 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color.h   |   7 ++
>  drivers/gpu/drm/i915/intel_display.c |  24 +++++
>  3 files changed, 197 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 50b98ee..b20a2c6 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -1251,6 +1251,172 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +static int i9xx_gamma_precision(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 chv_gamma_precision(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 ilk_gamma_precision(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 ivb_gamma_precision(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_SPLIT:
> +	case GAMMA_MODE_MODE_10BIT:
> +		return 10;
> +	default:
> +		MISSING_CASE(crtc_state->gamma_mode);
> +		return 0;
> +	}
> +}
> +
> +static int glk_gamma_precision(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;

glk doens't have that bit.

> +
> +	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(struct intel_crtc_state *crtc_state)
> +{
> +	if ((crtc_state->gamma_mode & PRE_CSC_GAMMA_ENABLE) == 0)
> +		return 0;

POST_CSC_GAMMA

> +
> +	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
> +	case GAMMA_MODE_MODE_8BIT:
> +		return 8;
> +	case GAMMA_MODE_MODE_10BIT:
> +		return 10;
> +	default:
> +		MISSING_CASE(crtc_state->gamma_mode);
> +		return 0;
> +	}
> +}
> +
> +int intel_color_get_gamma_bit_precision(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 (INTEL_GEN(dev_priv) >= 7)
> +			return ivb_gamma_precision(crtc_state);
> +		else
> +			return ilk_gamma_precision(crtc_state);
> +	}
> +
> +	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);
> +}
> +
> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
> +			   struct drm_property_blob *blob2,
> +			   u32 bit_precision)
> +{
> +	struct drm_color_lut *sw_lut, *hw_lut;
> +	int sw_lut_size, hw_lut_size, i;
> +	u32 err;
> +
> +	if (!blob1 && !blob2)
> +		return true;
> +
> +	if (!blob1)
> +		return true;
> +
> +	if (!blob2)
> +		return false;

What is this logic trying to do?

Something like?

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);
> +
> +	if (sw_lut_size != hw_lut_size)
> +		return false;

This is going to trigger with the split gamma mode. We'll need to be
more clever when comparing the two.

> +
> +	sw_lut = blob1->data;
> +	hw_lut = blob2->data;
> +
> +	err = 0xffff >> bit_precision;
> +
> +	for (i = 0; i < sw_lut_size; i++) {
> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  void intel_color_init(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> diff --git a/drivers/gpu/drm/i915/intel_color.h b/drivers/gpu/drm/i915/intel_color.h
> index 057e8ac..02ea1bc 100644
> --- a/drivers/gpu/drm/i915/intel_color.h
> +++ b/drivers/gpu/drm/i915/intel_color.h
> @@ -6,13 +6,20 @@
>  #ifndef __INTEL_COLOR_H__
>  #define __INTEL_COLOR_H__
>  
> +#include <linux/types.h>
> +
>  struct intel_crtc_state;
>  struct intel_crtc;
> +struct drm_property_blob;
>  
>  void intel_color_init(struct intel_crtc *crtc);
>  int intel_color_check(struct intel_crtc_state *crtc_state);
>  void intel_color_commit(const struct intel_crtc_state *crtc_state);
>  void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
>  void intel_color_get_config(struct intel_crtc_state *crtc_state);
> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
> +			   struct drm_property_blob *blob2,
> +			   u32 bit_precision);
> +int intel_color_get_gamma_bit_precision(struct intel_crtc_state *crtc_state);
>  
>  #endif /* __INTEL_COLOR_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3e01028..b8ff3f4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11570,6 +11570,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  				      drm_rect_width(&state->base.dst),
>  				      drm_rect_height(&state->base.dst));
>  	}
> +
> +	if (IS_CHERRYVIEW(dev_priv))
> +		DRM_DEBUG_KMS("cgm_mode:%d gamma_mode:%d 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:%d gamma_mode:%d gamma_enable:%d csc_enable:%d\n",
> +			       pipe_config->csc_mode, pipe_config->gamma_mode,
> +			       pipe_config->gamma_enable, pipe_config->csc_enable);
>  }
>  
>  static bool check_digital_port_conflicts(struct drm_atomic_state *state)
> @@ -11947,6 +11956,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  			  bool adjust)
>  {
>  	bool ret = true;
> +	u32 bp_gamma = 0;
>  	bool fixup_inherited = adjust &&
>  		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
>  		!(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED);
> @@ -12098,6 +12108,15 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  	} \
>  } while (0)
>  
> +#define PIPE_CONF_CHECK_COLOR_LUT(name, bit_precision) do { \
> +	if (!intel_color_lut_equal(current_config->name, \
> +				   pipe_config->name, bit_precision)) { \
> +		pipe_config_err(adjust, __stringify(name), \
> +				"hw_state doesn't match sw_state\n"); \
> +		ret = false; \
> +	} \
> +} while (0)
> +
>  #define PIPE_CONF_QUIRK(quirk) \
>  	((current_config->quirks | pipe_config->quirks) & (quirk))
>  
> @@ -12193,6 +12212,10 @@ 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(base.gamma_lut, bp_gamma);

Sometimes the sw gamma gets programmed in the hw degamma, so we do need
degamma readout, and we'll need to somehow figure out which hw thing
is which sw thing.

I'm thinking the following sort of logic should work since so
far it should be just the the sw.gamma that can end up in 
hw.degamma:

if (sw.gamma && !hw.gamma) {
	CHECK(sw.gamma, hw.degamma);
	CHECK(sw.degamma, hw.gamma);
} else {
	CHECK(sw.gamma, hw.gamma);
	CHECK(sw.degamma, hw.degamma);
}

>  	}
>  
>  	PIPE_CONF_CHECK_BOOL(double_wide);
> @@ -12255,6 +12278,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  #undef PIPE_CONF_CHECK_FLAGS
>  #undef PIPE_CONF_CHECK_CLOCK_FUZZY
>  #undef PIPE_CONF_QUIRK
> +#undef PIPE_CONF_CHECK_COLOR_LUT
>  
>  	return ret;
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula June 5, 2019, 10:07 a.m. UTC | #2
On Wed, 29 May 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> 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]
>     -Called PIPE_CONF_CHECK_COLOR_LUT inside if (!adjust) [Jani]
>     -Added #undef PIPE_CONF_CHECK_COLOR_LUT [Jani]
> 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]
>     -Added debug log for color para in intel_dump_pipe_config [Jani]
>     -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

The patch revisions are great, but you do need to add an actual commit
message first as well.

In general, please don't reference "this patch" because on the one hand
it's kind of obvious and on the other hand it ceases to be a patch when
it gets applied and becomes a commit.

Please also use the imperative style in the commit message like you have
in the title/subject line.

So instead of "In this patch, foo is added." use "Add foo."

BR,
Jani.


>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c   | 166 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color.h   |   7 ++
>  drivers/gpu/drm/i915/intel_display.c |  24 +++++
>  3 files changed, 197 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 50b98ee..b20a2c6 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -1251,6 +1251,172 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +static int i9xx_gamma_precision(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 chv_gamma_precision(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 ilk_gamma_precision(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 ivb_gamma_precision(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_SPLIT:
> +	case GAMMA_MODE_MODE_10BIT:
> +		return 10;
> +	default:
> +		MISSING_CASE(crtc_state->gamma_mode);
> +		return 0;
> +	}
> +}
> +
> +static int glk_gamma_precision(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 icl_gamma_precision(struct intel_crtc_state *crtc_state)
> +{
> +	if ((crtc_state->gamma_mode & PRE_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;
> +	default:
> +		MISSING_CASE(crtc_state->gamma_mode);
> +		return 0;
> +	}
> +}
> +
> +int intel_color_get_gamma_bit_precision(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 (INTEL_GEN(dev_priv) >= 7)
> +			return ivb_gamma_precision(crtc_state);
> +		else
> +			return ilk_gamma_precision(crtc_state);
> +	}
> +
> +	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);
> +}
> +
> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
> +			   struct drm_property_blob *blob2,
> +			   u32 bit_precision)
> +{
> +	struct drm_color_lut *sw_lut, *hw_lut;
> +	int sw_lut_size, hw_lut_size, i;
> +	u32 err;
> +
> +	if (!blob1 && !blob2)
> +		return true;
> +
> +	if (!blob1)
> +		return true;
> +
> +	if (!blob2)
> +		return false;
> +
> +	sw_lut_size = drm_color_lut_size(blob1);
> +	hw_lut_size = drm_color_lut_size(blob2);
> +
> +	if (sw_lut_size != hw_lut_size)
> +		return false;
> +
> +	sw_lut = blob1->data;
> +	hw_lut = blob2->data;
> +
> +	err = 0xffff >> bit_precision;
> +
> +	for (i = 0; i < sw_lut_size; i++) {
> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  void intel_color_init(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> diff --git a/drivers/gpu/drm/i915/intel_color.h b/drivers/gpu/drm/i915/intel_color.h
> index 057e8ac..02ea1bc 100644
> --- a/drivers/gpu/drm/i915/intel_color.h
> +++ b/drivers/gpu/drm/i915/intel_color.h
> @@ -6,13 +6,20 @@
>  #ifndef __INTEL_COLOR_H__
>  #define __INTEL_COLOR_H__
>  
> +#include <linux/types.h>
> +
>  struct intel_crtc_state;
>  struct intel_crtc;
> +struct drm_property_blob;
>  
>  void intel_color_init(struct intel_crtc *crtc);
>  int intel_color_check(struct intel_crtc_state *crtc_state);
>  void intel_color_commit(const struct intel_crtc_state *crtc_state);
>  void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
>  void intel_color_get_config(struct intel_crtc_state *crtc_state);
> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
> +			   struct drm_property_blob *blob2,
> +			   u32 bit_precision);
> +int intel_color_get_gamma_bit_precision(struct intel_crtc_state *crtc_state);
>  
>  #endif /* __INTEL_COLOR_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3e01028..b8ff3f4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11570,6 +11570,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  				      drm_rect_width(&state->base.dst),
>  				      drm_rect_height(&state->base.dst));
>  	}
> +
> +	if (IS_CHERRYVIEW(dev_priv))
> +		DRM_DEBUG_KMS("cgm_mode:%d gamma_mode:%d 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:%d gamma_mode:%d gamma_enable:%d csc_enable:%d\n",
> +			       pipe_config->csc_mode, pipe_config->gamma_mode,
> +			       pipe_config->gamma_enable, pipe_config->csc_enable);
>  }
>  
>  static bool check_digital_port_conflicts(struct drm_atomic_state *state)
> @@ -11947,6 +11956,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  			  bool adjust)
>  {
>  	bool ret = true;
> +	u32 bp_gamma = 0;
>  	bool fixup_inherited = adjust &&
>  		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
>  		!(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED);
> @@ -12098,6 +12108,15 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  	} \
>  } while (0)
>  
> +#define PIPE_CONF_CHECK_COLOR_LUT(name, bit_precision) do { \
> +	if (!intel_color_lut_equal(current_config->name, \
> +				   pipe_config->name, bit_precision)) { \
> +		pipe_config_err(adjust, __stringify(name), \
> +				"hw_state doesn't match sw_state\n"); \
> +		ret = false; \
> +	} \
> +} while (0)
> +
>  #define PIPE_CONF_QUIRK(quirk) \
>  	((current_config->quirks | pipe_config->quirks) & (quirk))
>  
> @@ -12193,6 +12212,10 @@ 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(base.gamma_lut, bp_gamma);
>  	}
>  
>  	PIPE_CONF_CHECK_BOOL(double_wide);
> @@ -12255,6 +12278,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  #undef PIPE_CONF_CHECK_FLAGS
>  #undef PIPE_CONF_CHECK_CLOCK_FUZZY
>  #undef PIPE_CONF_QUIRK
> +#undef PIPE_CONF_CHECK_COLOR_LUT
>  
>  	return ret;
>  }
Sharma, Swati2 June 10, 2019, 10:54 a.m. UTC | #3
On 31-May-19 8:58 PM, Ville Syrjälä wrote:

> On Wed, May 29, 2019 at 03:20:53PM +0530, Swati Sharma wrote:
>> 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]
>>      -Called PIPE_CONF_CHECK_COLOR_LUT inside if (!adjust) [Jani]
>>      -Added #undef PIPE_CONF_CHECK_COLOR_LUT [Jani]
>> 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]
>>      -Added debug log for color para in intel_dump_pipe_config [Jani]
>>      -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
>>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_color.c   | 166 +++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color.h   |   7 ++
>>   drivers/gpu/drm/i915/intel_display.c |  24 +++++
>>   3 files changed, 197 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index 50b98ee..b20a2c6 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -1251,6 +1251,172 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>>   	return 0;
>>   }
>>   
>> +static int i9xx_gamma_precision(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 chv_gamma_precision(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 ilk_gamma_precision(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 ivb_gamma_precision(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_SPLIT:
>> +	case GAMMA_MODE_MODE_10BIT:
>> +		return 10;
>> +	default:
>> +		MISSING_CASE(crtc_state->gamma_mode);
>> +		return 0;
>> +	}
>> +}
>> +
>> +static int glk_gamma_precision(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;
> glk doens't have that bit.
>
>> +
>> +	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(struct intel_crtc_state *crtc_state)
>> +{
>> +	if ((crtc_state->gamma_mode & PRE_CSC_GAMMA_ENABLE) == 0)
>> +		return 0;
> POST_CSC_GAMMA
>
>> +
>> +	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
>> +	case GAMMA_MODE_MODE_8BIT:
>> +		return 8;
>> +	case GAMMA_MODE_MODE_10BIT:
>> +		return 10;
>> +	default:
>> +		MISSING_CASE(crtc_state->gamma_mode);
>> +		return 0;
>> +	}
>> +}
>> +
>> +int intel_color_get_gamma_bit_precision(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 (INTEL_GEN(dev_priv) >= 7)
>> +			return ivb_gamma_precision(crtc_state);
>> +		else
>> +			return ilk_gamma_precision(crtc_state);
>> +	}
>> +
>> +	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);
>> +}
>> +
>> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
>> +			   struct drm_property_blob *blob2,
>> +			   u32 bit_precision)
>> +{
>> +	struct drm_color_lut *sw_lut, *hw_lut;
>> +	int sw_lut_size, hw_lut_size, i;
>> +	u32 err;
>> +
>> +	if (!blob1 && !blob2)
>> +		return true;
>> +
>> +	if (!blob1)
>> +		return true;
>> +
>> +	if (!blob2)
>> +		return false;
> What is this logic trying to do?
>
> Something like?
>
> 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);
>> +
>> +	if (sw_lut_size != hw_lut_size)
>> +		return false;
> This is going to trigger with the split gamma mode. We'll need to be
> more clever when comparing the two.

So, we need check for gamma_mode here as-well?

>
>> +
>> +	sw_lut = blob1->data;
>> +	hw_lut = blob2->data;
>> +
>> +	err = 0xffff >> bit_precision;
>> +
>> +	for (i = 0; i < sw_lut_size; i++) {
>> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>   void intel_color_init(struct intel_crtc *crtc)
>>   {
>>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> diff --git a/drivers/gpu/drm/i915/intel_color.h b/drivers/gpu/drm/i915/intel_color.h
>> index 057e8ac..02ea1bc 100644
>> --- a/drivers/gpu/drm/i915/intel_color.h
>> +++ b/drivers/gpu/drm/i915/intel_color.h
>> @@ -6,13 +6,20 @@
>>   #ifndef __INTEL_COLOR_H__
>>   #define __INTEL_COLOR_H__
>>   
>> +#include <linux/types.h>
>> +
>>   struct intel_crtc_state;
>>   struct intel_crtc;
>> +struct drm_property_blob;
>>   
>>   void intel_color_init(struct intel_crtc *crtc);
>>   int intel_color_check(struct intel_crtc_state *crtc_state);
>>   void intel_color_commit(const struct intel_crtc_state *crtc_state);
>>   void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
>>   void intel_color_get_config(struct intel_crtc_state *crtc_state);
>> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
>> +			   struct drm_property_blob *blob2,
>> +			   u32 bit_precision);
>> +int intel_color_get_gamma_bit_precision(struct intel_crtc_state *crtc_state);
>>   
>>   #endif /* __INTEL_COLOR_H__ */
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 3e01028..b8ff3f4 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11570,6 +11570,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>>   				      drm_rect_width(&state->base.dst),
>>   				      drm_rect_height(&state->base.dst));
>>   	}
>> +
>> +	if (IS_CHERRYVIEW(dev_priv))
>> +		DRM_DEBUG_KMS("cgm_mode:%d gamma_mode:%d 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:%d gamma_mode:%d gamma_enable:%d csc_enable:%d\n",
>> +			       pipe_config->csc_mode, pipe_config->gamma_mode,
>> +			       pipe_config->gamma_enable, pipe_config->csc_enable);
>>   }
>>   
>>   static bool check_digital_port_conflicts(struct drm_atomic_state *state)
>> @@ -11947,6 +11956,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>>   			  bool adjust)
>>   {
>>   	bool ret = true;
>> +	u32 bp_gamma = 0;
>>   	bool fixup_inherited = adjust &&
>>   		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
>>   		!(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED);
>> @@ -12098,6 +12108,15 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>>   	} \
>>   } while (0)
>>   
>> +#define PIPE_CONF_CHECK_COLOR_LUT(name, bit_precision) do { \
>> +	if (!intel_color_lut_equal(current_config->name, \
>> +				   pipe_config->name, bit_precision)) { \
>> +		pipe_config_err(adjust, __stringify(name), \
>> +				"hw_state doesn't match sw_state\n"); \
>> +		ret = false; \
>> +	} \
>> +} while (0)
>> +
>>   #define PIPE_CONF_QUIRK(quirk) \
>>   	((current_config->quirks | pipe_config->quirks) & (quirk))
>>   
>> @@ -12193,6 +12212,10 @@ 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(base.gamma_lut, bp_gamma);
> Sometimes the sw gamma gets programmed in the hw degamma, so we do need
> degamma readout, and we'll need to somehow figure out which hw thing
> is which sw thing.

Can you please little bit elaborate on this? What's the usecase?

>
> I'm thinking the following sort of logic should work since so
> far it should be just the the sw.gamma that can end up in
> hw.degamma:
>
> if (sw.gamma && !hw.gamma) {
> 	CHECK(sw.gamma, hw.degamma);
> 	CHECK(sw.degamma, hw.gamma);
> } else {
> 	CHECK(sw.gamma, hw.gamma);
> 	CHECK(sw.degamma, hw.degamma);
> }
>
>>   	}
>>   
>>   	PIPE_CONF_CHECK_BOOL(double_wide);
>> @@ -12255,6 +12278,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>>   #undef PIPE_CONF_CHECK_FLAGS
>>   #undef PIPE_CONF_CHECK_CLOCK_FUZZY
>>   #undef PIPE_CONF_QUIRK
>> +#undef PIPE_CONF_CHECK_COLOR_LUT
>>   
>>   	return ret;
>>   }
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjala June 10, 2019, 5:03 p.m. UTC | #4
On Mon, Jun 10, 2019 at 04:24:54PM +0530, Sharma, Swati2 wrote:
> On 31-May-19 8:58 PM, Ville Syrjälä wrote:
> 
> > On Wed, May 29, 2019 at 03:20:53PM +0530, Swati Sharma wrote:
> >> 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]
> >>      -Called PIPE_CONF_CHECK_COLOR_LUT inside if (!adjust) [Jani]
> >>      -Added #undef PIPE_CONF_CHECK_COLOR_LUT [Jani]
> >> 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]
> >>      -Added debug log for color para in intel_dump_pipe_config [Jani]
> >>      -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
> >>
> >> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_color.c   | 166 +++++++++++++++++++++++++++++++++++
> >>   drivers/gpu/drm/i915/intel_color.h   |   7 ++
> >>   drivers/gpu/drm/i915/intel_display.c |  24 +++++
> >>   3 files changed, 197 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> >> index 50b98ee..b20a2c6 100644
> >> --- a/drivers/gpu/drm/i915/intel_color.c
> >> +++ b/drivers/gpu/drm/i915/intel_color.c
> >> @@ -1251,6 +1251,172 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
> >>   	return 0;
> >>   }
> >>   
> >> +static int i9xx_gamma_precision(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 chv_gamma_precision(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 ilk_gamma_precision(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 ivb_gamma_precision(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_SPLIT:
> >> +	case GAMMA_MODE_MODE_10BIT:
> >> +		return 10;
> >> +	default:
> >> +		MISSING_CASE(crtc_state->gamma_mode);
> >> +		return 0;
> >> +	}
> >> +}
> >> +
> >> +static int glk_gamma_precision(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;
> > glk doens't have that bit.
> >
> >> +
> >> +	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(struct intel_crtc_state *crtc_state)
> >> +{
> >> +	if ((crtc_state->gamma_mode & PRE_CSC_GAMMA_ENABLE) == 0)
> >> +		return 0;
> > POST_CSC_GAMMA
> >
> >> +
> >> +	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
> >> +	case GAMMA_MODE_MODE_8BIT:
> >> +		return 8;
> >> +	case GAMMA_MODE_MODE_10BIT:
> >> +		return 10;
> >> +	default:
> >> +		MISSING_CASE(crtc_state->gamma_mode);
> >> +		return 0;
> >> +	}
> >> +}
> >> +
> >> +int intel_color_get_gamma_bit_precision(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 (INTEL_GEN(dev_priv) >= 7)
> >> +			return ivb_gamma_precision(crtc_state);
> >> +		else
> >> +			return ilk_gamma_precision(crtc_state);
> >> +	}
> >> +
> >> +	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);
> >> +}
> >> +
> >> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
> >> +			   struct drm_property_blob *blob2,
> >> +			   u32 bit_precision)
> >> +{
> >> +	struct drm_color_lut *sw_lut, *hw_lut;
> >> +	int sw_lut_size, hw_lut_size, i;
> >> +	u32 err;
> >> +
> >> +	if (!blob1 && !blob2)
> >> +		return true;
> >> +
> >> +	if (!blob1)
> >> +		return true;
> >> +
> >> +	if (!blob2)
> >> +		return false;
> > What is this logic trying to do?
> >
> > Something like?
> >
> > 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);
> >> +
> >> +	if (sw_lut_size != hw_lut_size)
> >> +		return false;
> > This is going to trigger with the split gamma mode. We'll need to be
> > more clever when comparing the two.
> 
> So, we need check for gamma_mode here as-well?

Maybe. We're going to have to make the exception somewhere,
and then we need to pick the LUT entries to compare similarly
as how the relevanty load_lut() function picks them.


> 
> >
> >> +
> >> +	sw_lut = blob1->data;
> >> +	hw_lut = blob2->data;
> >> +
> >> +	err = 0xffff >> bit_precision;
> >> +
> >> +	for (i = 0; i < sw_lut_size; i++) {
> >> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
> >> +			return false;
> >> +	}
> >> +
> >> +	return true;
> >> +}
> >> +
> >>   void intel_color_init(struct intel_crtc *crtc)
> >>   {
> >>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> diff --git a/drivers/gpu/drm/i915/intel_color.h b/drivers/gpu/drm/i915/intel_color.h
> >> index 057e8ac..02ea1bc 100644
> >> --- a/drivers/gpu/drm/i915/intel_color.h
> >> +++ b/drivers/gpu/drm/i915/intel_color.h
> >> @@ -6,13 +6,20 @@
> >>   #ifndef __INTEL_COLOR_H__
> >>   #define __INTEL_COLOR_H__
> >>   
> >> +#include <linux/types.h>
> >> +
> >>   struct intel_crtc_state;
> >>   struct intel_crtc;
> >> +struct drm_property_blob;
> >>   
> >>   void intel_color_init(struct intel_crtc *crtc);
> >>   int intel_color_check(struct intel_crtc_state *crtc_state);
> >>   void intel_color_commit(const struct intel_crtc_state *crtc_state);
> >>   void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
> >>   void intel_color_get_config(struct intel_crtc_state *crtc_state);
> >> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
> >> +			   struct drm_property_blob *blob2,
> >> +			   u32 bit_precision);
> >> +int intel_color_get_gamma_bit_precision(struct intel_crtc_state *crtc_state);
> >>   
> >>   #endif /* __INTEL_COLOR_H__ */
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 3e01028..b8ff3f4 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -11570,6 +11570,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> >>   				      drm_rect_width(&state->base.dst),
> >>   				      drm_rect_height(&state->base.dst));
> >>   	}
> >> +
> >> +	if (IS_CHERRYVIEW(dev_priv))
> >> +		DRM_DEBUG_KMS("cgm_mode:%d gamma_mode:%d 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:%d gamma_mode:%d gamma_enable:%d csc_enable:%d\n",
> >> +			       pipe_config->csc_mode, pipe_config->gamma_mode,
> >> +			       pipe_config->gamma_enable, pipe_config->csc_enable);
> >>   }
> >>   
> >>   static bool check_digital_port_conflicts(struct drm_atomic_state *state)
> >> @@ -11947,6 +11956,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
> >>   			  bool adjust)
> >>   {
> >>   	bool ret = true;
> >> +	u32 bp_gamma = 0;
> >>   	bool fixup_inherited = adjust &&
> >>   		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
> >>   		!(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED);
> >> @@ -12098,6 +12108,15 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
> >>   	} \
> >>   } while (0)
> >>   
> >> +#define PIPE_CONF_CHECK_COLOR_LUT(name, bit_precision) do { \
> >> +	if (!intel_color_lut_equal(current_config->name, \
> >> +				   pipe_config->name, bit_precision)) { \
> >> +		pipe_config_err(adjust, __stringify(name), \
> >> +				"hw_state doesn't match sw_state\n"); \
> >> +		ret = false; \
> >> +	} \
> >> +} while (0)
> >> +
> >>   #define PIPE_CONF_QUIRK(quirk) \
> >>   	((current_config->quirks | pipe_config->quirks) & (quirk))
> >>   
> >> @@ -12193,6 +12212,10 @@ 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(base.gamma_lut, bp_gamma);
> > Sometimes the sw gamma gets programmed in the hw degamma, so we do need
> > degamma readout, and we'll need to somehow figure out which hw thing
> > is which sw thing.
> 
> Can you please little bit elaborate on this? What's the usecase?

Any time gamma+ycbcr/limited range rgb output is used.
See ivb_csc_mode().


> 
> >
> > I'm thinking the following sort of logic should work since so
> > far it should be just the the sw.gamma that can end up in
> > hw.degamma:
> >
> > if (sw.gamma && !hw.gamma) {
> > 	CHECK(sw.gamma, hw.degamma);
> > 	CHECK(sw.degamma, hw.gamma);
> > } else {
> > 	CHECK(sw.gamma, hw.gamma);
> > 	CHECK(sw.degamma, hw.degamma);
> > }
> >
> >>   	}
> >>   
> >>   	PIPE_CONF_CHECK_BOOL(double_wide);
> >> @@ -12255,6 +12278,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
> >>   #undef PIPE_CONF_CHECK_FLAGS
> >>   #undef PIPE_CONF_CHECK_CLOCK_FUZZY
> >>   #undef PIPE_CONF_QUIRK
> >> +#undef PIPE_CONF_CHECK_COLOR_LUT
> >>   
> >>   	return ret;
> >>   }
> >> -- 
> >> 1.9.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> -- 
> ~Swati Sharma
>
Sharma, Swati2 June 12, 2019, 11:38 a.m. UTC | #5
On 05-Jun-19 3:37 PM, Jani Nikula wrote:
> On Wed, 29 May 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
>> 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]
>>      -Called PIPE_CONF_CHECK_COLOR_LUT inside if (!adjust) [Jani]
>>      -Added #undef PIPE_CONF_CHECK_COLOR_LUT [Jani]
>> 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]
>>      -Added debug log for color para in intel_dump_pipe_config [Jani]
>>      -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
> The patch revisions are great, but you do need to add an actual commit
> message first as well.
>
> In general, please don't reference "this patch" because on the one hand
> it's kind of obvious and on the other hand it ceases to be a patch when
> it gets applied and becomes a commit.
>
> Please also use the imperative style in the commit message like you have
> in the title/subject line.
>
> So instead of "In this patch, foo is added." use "Add foo."
>
> BR,
> Jani.

Sure, thanks Jani. Will correct other patches.

>
>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_color.c   | 166 +++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color.h   |   7 ++
>>   drivers/gpu/drm/i915/intel_display.c |  24 +++++
>>   3 files changed, 197 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index 50b98ee..b20a2c6 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -1251,6 +1251,172 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>>   	return 0;
>>   }
>>   
>> +static int i9xx_gamma_precision(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 chv_gamma_precision(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 ilk_gamma_precision(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 ivb_gamma_precision(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_SPLIT:
>> +	case GAMMA_MODE_MODE_10BIT:
>> +		return 10;
>> +	default:
>> +		MISSING_CASE(crtc_state->gamma_mode);
>> +		return 0;
>> +	}
>> +}
>> +
>> +static int glk_gamma_precision(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 icl_gamma_precision(struct intel_crtc_state *crtc_state)
>> +{
>> +	if ((crtc_state->gamma_mode & PRE_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;
>> +	default:
>> +		MISSING_CASE(crtc_state->gamma_mode);
>> +		return 0;
>> +	}
>> +}
>> +
>> +int intel_color_get_gamma_bit_precision(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 (INTEL_GEN(dev_priv) >= 7)
>> +			return ivb_gamma_precision(crtc_state);
>> +		else
>> +			return ilk_gamma_precision(crtc_state);
>> +	}
>> +
>> +	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);
>> +}
>> +
>> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
>> +			   struct drm_property_blob *blob2,
>> +			   u32 bit_precision)
>> +{
>> +	struct drm_color_lut *sw_lut, *hw_lut;
>> +	int sw_lut_size, hw_lut_size, i;
>> +	u32 err;
>> +
>> +	if (!blob1 && !blob2)
>> +		return true;
>> +
>> +	if (!blob1)
>> +		return true;
>> +
>> +	if (!blob2)
>> +		return false;
>> +
>> +	sw_lut_size = drm_color_lut_size(blob1);
>> +	hw_lut_size = drm_color_lut_size(blob2);
>> +
>> +	if (sw_lut_size != hw_lut_size)
>> +		return false;
>> +
>> +	sw_lut = blob1->data;
>> +	hw_lut = blob2->data;
>> +
>> +	err = 0xffff >> bit_precision;
>> +
>> +	for (i = 0; i < sw_lut_size; i++) {
>> +		if (!err_check(&hw_lut[i], &sw_lut[i], err))
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>   void intel_color_init(struct intel_crtc *crtc)
>>   {
>>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> diff --git a/drivers/gpu/drm/i915/intel_color.h b/drivers/gpu/drm/i915/intel_color.h
>> index 057e8ac..02ea1bc 100644
>> --- a/drivers/gpu/drm/i915/intel_color.h
>> +++ b/drivers/gpu/drm/i915/intel_color.h
>> @@ -6,13 +6,20 @@
>>   #ifndef __INTEL_COLOR_H__
>>   #define __INTEL_COLOR_H__
>>   
>> +#include <linux/types.h>
>> +
>>   struct intel_crtc_state;
>>   struct intel_crtc;
>> +struct drm_property_blob;
>>   
>>   void intel_color_init(struct intel_crtc *crtc);
>>   int intel_color_check(struct intel_crtc_state *crtc_state);
>>   void intel_color_commit(const struct intel_crtc_state *crtc_state);
>>   void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
>>   void intel_color_get_config(struct intel_crtc_state *crtc_state);
>> +bool intel_color_lut_equal(struct drm_property_blob *blob1,
>> +			   struct drm_property_blob *blob2,
>> +			   u32 bit_precision);
>> +int intel_color_get_gamma_bit_precision(struct intel_crtc_state *crtc_state);
>>   
>>   #endif /* __INTEL_COLOR_H__ */
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 3e01028..b8ff3f4 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11570,6 +11570,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>>   				      drm_rect_width(&state->base.dst),
>>   				      drm_rect_height(&state->base.dst));
>>   	}
>> +
>> +	if (IS_CHERRYVIEW(dev_priv))
>> +		DRM_DEBUG_KMS("cgm_mode:%d gamma_mode:%d 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:%d gamma_mode:%d gamma_enable:%d csc_enable:%d\n",
>> +			       pipe_config->csc_mode, pipe_config->gamma_mode,
>> +			       pipe_config->gamma_enable, pipe_config->csc_enable);
>>   }
>>   
>>   static bool check_digital_port_conflicts(struct drm_atomic_state *state)
>> @@ -11947,6 +11956,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>>   			  bool adjust)
>>   {
>>   	bool ret = true;
>> +	u32 bp_gamma = 0;
>>   	bool fixup_inherited = adjust &&
>>   		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
>>   		!(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED);
>> @@ -12098,6 +12108,15 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>>   	} \
>>   } while (0)
>>   
>> +#define PIPE_CONF_CHECK_COLOR_LUT(name, bit_precision) do { \
>> +	if (!intel_color_lut_equal(current_config->name, \
>> +				   pipe_config->name, bit_precision)) { \
>> +		pipe_config_err(adjust, __stringify(name), \
>> +				"hw_state doesn't match sw_state\n"); \
>> +		ret = false; \
>> +	} \
>> +} while (0)
>> +
>>   #define PIPE_CONF_QUIRK(quirk) \
>>   	((current_config->quirks | pipe_config->quirks) & (quirk))
>>   
>> @@ -12193,6 +12212,10 @@ 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(base.gamma_lut, bp_gamma);
>>   	}
>>   
>>   	PIPE_CONF_CHECK_BOOL(double_wide);
>> @@ -12255,6 +12278,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>>   #undef PIPE_CONF_CHECK_FLAGS
>>   #undef PIPE_CONF_CHECK_CLOCK_FUZZY
>>   #undef PIPE_CONF_QUIRK
>> +#undef PIPE_CONF_CHECK_COLOR_LUT
>>   
>>   	return ret;
>>   }
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 50b98ee..b20a2c6 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1251,6 +1251,172 @@  static int icl_color_check(struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
+static int i9xx_gamma_precision(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 chv_gamma_precision(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 ilk_gamma_precision(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 ivb_gamma_precision(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_SPLIT:
+	case GAMMA_MODE_MODE_10BIT:
+		return 10;
+	default:
+		MISSING_CASE(crtc_state->gamma_mode);
+		return 0;
+	}
+}
+
+static int glk_gamma_precision(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 icl_gamma_precision(struct intel_crtc_state *crtc_state)
+{
+	if ((crtc_state->gamma_mode & PRE_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;
+	default:
+		MISSING_CASE(crtc_state->gamma_mode);
+		return 0;
+	}
+}
+
+int intel_color_get_gamma_bit_precision(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 (INTEL_GEN(dev_priv) >= 7)
+			return ivb_gamma_precision(crtc_state);
+		else
+			return ilk_gamma_precision(crtc_state);
+	}
+
+	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);
+}
+
+bool intel_color_lut_equal(struct drm_property_blob *blob1,
+			   struct drm_property_blob *blob2,
+			   u32 bit_precision)
+{
+	struct drm_color_lut *sw_lut, *hw_lut;
+	int sw_lut_size, hw_lut_size, i;
+	u32 err;
+
+	if (!blob1 && !blob2)
+		return true;
+
+	if (!blob1)
+		return true;
+
+	if (!blob2)
+		return false;
+
+	sw_lut_size = drm_color_lut_size(blob1);
+	hw_lut_size = drm_color_lut_size(blob2);
+
+	if (sw_lut_size != hw_lut_size)
+		return false;
+
+	sw_lut = blob1->data;
+	hw_lut = blob2->data;
+
+	err = 0xffff >> bit_precision;
+
+	for (i = 0; i < sw_lut_size; i++) {
+		if (!err_check(&hw_lut[i], &sw_lut[i], err))
+			return false;
+	}
+
+	return true;
+}
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
diff --git a/drivers/gpu/drm/i915/intel_color.h b/drivers/gpu/drm/i915/intel_color.h
index 057e8ac..02ea1bc 100644
--- a/drivers/gpu/drm/i915/intel_color.h
+++ b/drivers/gpu/drm/i915/intel_color.h
@@ -6,13 +6,20 @@ 
 #ifndef __INTEL_COLOR_H__
 #define __INTEL_COLOR_H__
 
+#include <linux/types.h>
+
 struct intel_crtc_state;
 struct intel_crtc;
+struct drm_property_blob;
 
 void intel_color_init(struct intel_crtc *crtc);
 int intel_color_check(struct intel_crtc_state *crtc_state);
 void intel_color_commit(const struct intel_crtc_state *crtc_state);
 void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
 void intel_color_get_config(struct intel_crtc_state *crtc_state);
+bool intel_color_lut_equal(struct drm_property_blob *blob1,
+			   struct drm_property_blob *blob2,
+			   u32 bit_precision);
+int intel_color_get_gamma_bit_precision(struct intel_crtc_state *crtc_state);
 
 #endif /* __INTEL_COLOR_H__ */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3e01028..b8ff3f4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11570,6 +11570,15 @@  static void intel_dump_pipe_config(struct intel_crtc *crtc,
 				      drm_rect_width(&state->base.dst),
 				      drm_rect_height(&state->base.dst));
 	}
+
+	if (IS_CHERRYVIEW(dev_priv))
+		DRM_DEBUG_KMS("cgm_mode:%d gamma_mode:%d 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:%d gamma_mode:%d gamma_enable:%d csc_enable:%d\n",
+			       pipe_config->csc_mode, pipe_config->gamma_mode,
+			       pipe_config->gamma_enable, pipe_config->csc_enable);
 }
 
 static bool check_digital_port_conflicts(struct drm_atomic_state *state)
@@ -11947,6 +11956,7 @@  static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 			  bool adjust)
 {
 	bool ret = true;
+	u32 bp_gamma = 0;
 	bool fixup_inherited = adjust &&
 		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
 		!(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED);
@@ -12098,6 +12108,15 @@  static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 	} \
 } while (0)
 
+#define PIPE_CONF_CHECK_COLOR_LUT(name, bit_precision) do { \
+	if (!intel_color_lut_equal(current_config->name, \
+				   pipe_config->name, bit_precision)) { \
+		pipe_config_err(adjust, __stringify(name), \
+				"hw_state doesn't match sw_state\n"); \
+		ret = false; \
+	} \
+} while (0)
+
 #define PIPE_CONF_QUIRK(quirk) \
 	((current_config->quirks | pipe_config->quirks) & (quirk))
 
@@ -12193,6 +12212,10 @@  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(base.gamma_lut, bp_gamma);
 	}
 
 	PIPE_CONF_CHECK_BOOL(double_wide);
@@ -12255,6 +12278,7 @@  static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 #undef PIPE_CONF_CHECK_FLAGS
 #undef PIPE_CONF_CHECK_CLOCK_FUZZY
 #undef PIPE_CONF_QUIRK
+#undef PIPE_CONF_CHECK_COLOR_LUT
 
 	return ret;
 }