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

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 28 Mar 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> Added state checker to validate gamma_lut values. This
> reads hardware state, and compares the originally requested
> state to the state read from hardware.
>
> v1: -Implementation done for legacy platforms (removed all the placeholders) (Jani)
>     -Added inverse function of drm_color_lut_extract to convert hardware
>      read values back to user values (code written by Jani)
>     -Renamed get_config() to color_config() (Jani)
>     -Placed all platform specific shifts and masks in i915_reg.h (Jani)
>     -Renamed i9xx_get_config to i9xx_color_config and all related
>      functions (Jani)
>     -Removed debug logs from compare function (Jani)
>     -Renamed intel_compare_blob to intel_compare_lut and added platform specific
>      bit precision of the readout into the function (Jani)
>     -Renamed macro PIPE_CONF_CHECK_BLOB to PIPE_CONF_CHECK_COLOR_LUT (Jani)
>     -Added check if blobs can be NULL (Jani)
>     -Added function in intel_color.c that returns the bit precision (Jani),
>      didn't add in device info since its gonna die soon (Ville)
>
> TODO:
> -Add a separate function to log errors at the higher level

Should be a separate follow-up patch.

> -Haven't moved intel_compare_lut() from intel_display.c to intel_color.c
>  Since all the comparison functions are placed in intel_display, isn't
>  it the right place (or) we want to move to consolidate color related functions
>  together? Opinion? Please correct me if I am wrong.

Consolidate color to intel_color.c, as all the info about the blob and
its use is there.

> -Optimizations and refractoring
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   1 +
>  drivers/gpu/drm/i915/i915_reg.h      |  12 +++
>  drivers/gpu/drm/i915/intel_color.c   | 186 +++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_display.c |  48 +++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  5 files changed, 243 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c4ffe19..b422ea6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -334,6 +334,7 @@ struct drm_i915_display_funcs {
>  	 * involved with the same commit.
>  	 */
>  	void (*load_luts)(const struct intel_crtc_state *crtc_state);
> +	void (*color_config)(struct intel_crtc_state *crtc_state);

Please call this *get* config. Same for the platform specific
functions. It doesn't configure, it reads the configuration.

>  };
>  
>  #define CSR_VERSION(major, minor)	((major) << 16 | (minor))
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c0cd7a8..2813033 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7156,6 +7156,10 @@ enum {
>  #define _LGC_PALETTE_B           0x4a800
>  #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) + (i) * 4)
>  

No blank line here. Ditto for the other groups.

> +#define LGC_PALETTE_RED_MASK		(0xFF << 16)
> +#define LGC_PALETTE_GREEN_MASK		(0xFF << 8)
> +#define LGC_PALETTE_BLUE_MASK		(0xFF << 0)

Please indent according to the comment at the top of the file. Please
define these using the new REG_GENMASK() and use the REG_FIELD_PREP()
and REG_FIELD_GET() macros in code. Ditto for the other groups.

> +
>  #define _GAMMA_MODE_A		0x4a480
>  #define _GAMMA_MODE_B		0x4ac80
>  #define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
> @@ -10102,6 +10106,10 @@ enum skl_power_gate {
>  #define PRE_CSC_GAMC_INDEX(pipe)	_MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
>  #define PRE_CSC_GAMC_DATA(pipe)		_MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
>  
> +#define PREC_PAL_DATA_RED_MASK		(0x3FF << 20)
> +#define PREC_PAL_DATA_GREEN_MASK	(0x3FF << 10)
> +#define PREC_PAL_DATA_BLUE_MASK		(0x3FF << 0)
> +
>  /* pipe CSC & degamma/gamma LUTs on CHV */
>  #define _CGM_PIPE_A_CSC_COEFF01	(VLV_DISPLAY_BASE + 0x67900)
>  #define _CGM_PIPE_A_CSC_COEFF23	(VLV_DISPLAY_BASE + 0x67904)
> @@ -10133,6 +10141,10 @@ enum skl_power_gate {
>  #define CGM_PIPE_GAMMA(pipe, i, w)	_MMIO(_PIPE(pipe, _CGM_PIPE_A_GAMMA, _CGM_PIPE_B_GAMMA) + (i) * 8 + (w) * 4)
>  #define CGM_PIPE_MODE(pipe)		_MMIO_PIPE(pipe, _CGM_PIPE_A_MODE, _CGM_PIPE_B_MODE)
>  
> +#define CGM_PIPE_GAMMA_RED_MASK		(0x3FF << 0)
> +#define CGM_PIPE_GAMMA_GREEN_MASK	(0x3FF << 16)
> +#define CGM_PIPE_GAMMA_BLUE_MASK	(0x3FF << 0)
> +
>  /* MIPI DSI registers */
>  
>  #define _MIPI_PORT(port, a, c)	(((port) == PORT_A) ? a : c)	/* ports A and C only */
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index da7a07d..bd4f1b1 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -679,6 +679,172 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
>  	dev_priv->display.load_luts(crtc_state);
>  }
>  
> +u32 intel_color_bit_precision(struct drm_i915_private *dev_priv)
> +{
> +	if (INTEL_GEN(dev_priv) >= 9)
> +		return 10;
> +	else
> +		return 8;
> +}

Can be made static if the comparison function is moved here.

> +
> +/* convert hw value with given bit_precision to lut property val */
> +static u32 intel_color_lut_pack(u32 val, u32 bit_precision)
> +{
> +	u32 max = 0xFFFF >> (16 - bit_precision);
> +
> +	val = clamp_val(val, 0, max);
> +
> +	if (bit_precision < 16)
> +		val <<= 16 - bit_precision;
> +
> +	return val;
> +}
> +
> +static void i9xx_internal_gamma_config(struct intel_crtc_state *crtc_state)

"get" missing in name, same throughout.

> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 i, tmp;
> +	enum pipe pipe = crtc->pipe;
> +	struct drm_property_blob *blob = NULL;
> +	struct drm_color_lut *blob_data;
> +
> +	blob = drm_property_create_blob(dev,
> +					sizeof(struct drm_color_lut) * 256,
> +					NULL);
> +	if (IS_ERR(blob))
> +		return;
> +
> +	blob_data = blob->data;
> +
> +	for (i = 0; i < 256; i++) {
> +		if (HAS_GMCH(dev_priv))
> +			tmp = I915_READ(PALETTE(pipe, i));
> +		else
> +			tmp = I915_READ(LGC_PALETTE(pipe, i));
> +		blob_data[i].red = intel_color_lut_pack((tmp & LGC_PALETTE_RED_MASK) >> 16, 8);
> +		blob_data[i].green = intel_color_lut_pack((tmp & LGC_PALETTE_GREEN_MASK) >> 8, 8);
> +		blob_data[i].blue = intel_color_lut_pack((tmp & LGC_PALETTE_BLUE_MASK), 8);

Please use REG_FIELD_GET().

Same throughout.

> +	}
> +
> +	crtc_state->base.gamma_lut = blob;
> +}
> +
> +static void i9xx_color_config(struct intel_crtc_state *crtc_state)
> +{
> +	i9xx_internal_gamma_config(crtc_state);
> +}
> +
> +static void cherryview_gamma_config(struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 i, tmp, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> +	enum pipe pipe = crtc->pipe;
> +	struct drm_property_blob *blob = NULL;
> +	struct drm_color_lut *blob_data;
> +
> +	blob = drm_property_create_blob(dev,
> +					sizeof(struct drm_color_lut) * 256,
> +					NULL);
> +	if (IS_ERR(blob))
> +		return;
> +
> +	blob_data = blob->data;
> +
> +	for (i = 0; i < lut_size; i++) {
> +		tmp = I915_READ(CGM_PIPE_GAMMA(pipe, i, 0));
> +		blob_data[i].green = intel_color_lut_pack((tmp & CGM_PIPE_GAMMA_GREEN_MASK) >> 16, 10);
> +		blob_data[i].blue = intel_color_lut_pack((tmp & CGM_PIPE_GAMMA_BLUE_MASK), 10);
> +		tmp = I915_READ(CGM_PIPE_GAMMA(pipe, i, 1));
> +		blob_data[i].red = intel_color_lut_pack((tmp & CGM_PIPE_GAMMA_RED_MASK), 10);
> +	}
> +
> +	crtc_state->base.gamma_lut = blob;
> +}
> +
> +static void bdw_gamma_config(struct intel_crtc_state *crtc_state, u32 offset)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 i, tmp, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> +	enum pipe pipe = crtc->pipe;
> +	struct drm_property_blob *blob = NULL;
> +	struct drm_color_lut *blob_data;
> +
> +	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
> +
> +	I915_WRITE(PREC_PAL_INDEX(pipe),
> +		  (offset ? PAL_PREC_SPLIT_MODE : 0) |
> +		   PAL_PREC_AUTO_INCREMENT |
> +		   offset);
> +
> +	blob = drm_property_create_blob(dev,
> +					sizeof(struct drm_color_lut) * lut_size,
> +					NULL);
> +	if (IS_ERR(blob))
> +		return;
> +
> +	blob_data = blob->data;
> +
> +	for (i = 0; i < lut_size; i++) {
> +		tmp = I915_READ(PREC_PAL_DATA(pipe));
> +		blob_data[i].red = intel_color_lut_pack((tmp & PREC_PAL_DATA_RED_MASK) >> 20, 10);
> +		blob_data[i].green = intel_color_lut_pack((tmp & PREC_PAL_DATA_GREEN_MASK) >> 10, 10);
> +		blob_data[i].blue = intel_color_lut_pack((tmp & PREC_PAL_DATA_BLUE_MASK), 10);
> +	}
> +
> +	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
> +
> +	crtc_state->base.gamma_lut = blob;
> +}
> +
> +static void cherryview_color_config(struct intel_crtc_state *crtc_state)
> +{
> +	if (crtc_state_is_legacy_gamma(crtc_state))
> +		i9xx_color_config(crtc_state);
> +	else
> +		cherryview_gamma_config(crtc_state);
> +}
> +
> +static void broadwell_color_config(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 (crtc_state_is_legacy_gamma(crtc_state))
> +		i9xx_color_config(crtc_state);
> +	else
> +		bdw_gamma_config(crtc_state,
> +			         INTEL_INFO(dev_priv)->color.degamma_lut_size);
> +}
> +
> +static void glk_color_config(struct intel_crtc_state *crtc_state)
> +{
> +	if (crtc_state_is_legacy_gamma(crtc_state))
> +		i9xx_color_config(crtc_state);
> +	else
> +		bdw_gamma_config(crtc_state, 0);
> +}
> +
> +static void icl_color_config(struct intel_crtc_state *crtc_state)
> +{
> +	if (crtc_state_is_legacy_gamma(crtc_state))
> +		i9xx_color_config(crtc_state);
> +	else
> +		bdw_gamma_config(crtc_state, 0);
> +}
> +
> +void intel_color_config(struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> +
> +	dev_priv->display.color_config(crtc_state);
> +}
> +
>  void intel_color_commit(const struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> @@ -834,21 +1000,29 @@ void intel_color_init(struct intel_crtc *crtc)
>  	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
>  
>  	if (HAS_GMCH(dev_priv)) {
> -		if (IS_CHERRYVIEW(dev_priv))
> +		if (IS_CHERRYVIEW(dev_priv)) {
>  			dev_priv->display.load_luts = cherryview_load_luts;
> -		else
> +			dev_priv->display.color_config = cherryview_color_config;
> +		} else {
>  			dev_priv->display.load_luts = i9xx_load_luts;
> +			dev_priv->display.color_config = i9xx_color_config;
> +		}
>  
>  		dev_priv->display.color_commit = i9xx_color_commit;
>  	} else {
> -		if (IS_ICELAKE(dev_priv))
> +		if (IS_ICELAKE(dev_priv)) {
>  			dev_priv->display.load_luts = icl_load_luts;
> -		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> +			dev_priv->display.color_config = icl_color_config;
> +		} else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
>  			dev_priv->display.load_luts = glk_load_luts;
> -		else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> +			dev_priv->display.color_config = glk_color_config;
> +		} else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) {
>  			dev_priv->display.load_luts = broadwell_load_luts;
> -		else
> +			dev_priv->display.color_config = broadwell_color_config;
> +		} else {
>  			dev_priv->display.load_luts = i9xx_load_luts;
> +			dev_priv->display.color_config = i9xx_color_config;
> +		}
>  
>  		if (INTEL_GEN(dev_priv) >= 9)
>  			dev_priv->display.color_commit = skl_color_commit;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 963b4bd..31bb652 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8212,6 +8212,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  
>  	i9xx_get_pipe_color_config(pipe_config);
>  
> +	intel_color_config(pipe_config);
> +
>  	if (INTEL_GEN(dev_priv) < 4)
>  		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
>  
> @@ -9284,6 +9286,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  
>  	i9xx_get_pipe_color_config(pipe_config);
>  
> +	intel_color_config(pipe_config);
> +
>  	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
>  		struct intel_shared_dpll *pll;
>  		enum intel_dpll_id pll_id;
> @@ -9932,6 +9936,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  		i9xx_get_pipe_color_config(pipe_config);
>  	}
>  
> +	intel_color_config(pipe_config);
> +
>  	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>  	if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
>  		WARN_ON(power_domain_mask & BIT_ULL(power_domain));
> @@ -11990,6 +11996,36 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  	return false;
>  }
>  
> +static bool intel_compare_color_lut(struct drm_property_blob *blob1,
> +				    struct drm_property_blob *blob2,
> +			            u32 bit_precision)
> +{
> +	struct drm_color_lut *sw_lut = blob1->data;
> +	struct drm_color_lut *hw_lut = blob2->data;
> +	u32 err = 0xFFFF >> bit_precision;
> +	int sw_lut_size, hw_lut_size;
> +	int i;
> +
> +	sw_lut_size = drm_color_lut_size(blob1);
> +	hw_lut_size = drm_color_lut_size(blob2);
> +
> +	if (IS_ERR(blob1) || IS_ERR(blob2))
> +		return false;

Mmmh, if this condition is true, we've oopsed on drm_color_lut_size()
already.

> +
> +	if (sw_lut_size != hw_lut_size)
> +		return false;
> +
> +	for (i = 0; i < sw_lut_size; i++) {
> +		if (((abs((long)hw_lut[i].red - sw_lut[i].red)) >= err) ||
> +		   ((abs((long)hw_lut[i].blue - sw_lut[i].blue)) >= err) ||
> +		   ((abs((long)hw_lut[i].green - sw_lut[i].green)) >= err)) {

I think using an inline function for the comparison with err slack would
be nice.

> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}

I think separate the get config part from the state checker part to
another patch.

I think move this function to intel_color.c, as intel_display.c is
already overcrowded. Pass dev_priv to it so it can handle bit precision
internally instead, and you can make intel_color_bit_precision()
static. (Or just inline that in the function.)

> +
>  static bool
>  intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  			  struct intel_crtc_state *current_config,
> @@ -11997,6 +12033,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  			  bool adjust)
>  {
>  	bool ret = true;
> +	u32 bit_precision;
>  	bool fixup_inherited = adjust &&
>  		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
>  		!(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED);
> @@ -12148,6 +12185,14 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  	} \
>  } while (0)
>  
> +#define PIPE_CONF_CHECK_COLOR_LUT(name, bit_precision) do { \
> +	if (!intel_compare_color_lut(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))
>  
> @@ -12287,6 +12332,9 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  	PIPE_CONF_CHECK_INFOFRAME(spd);
>  	PIPE_CONF_CHECK_INFOFRAME(hdmi);
>  
> +	bit_precision = intel_color_bit_precision(dev_priv);
> +	PIPE_CONF_CHECK_COLOR_LUT(base.gamma_lut, bit_precision);
> +
>  #undef PIPE_CONF_CHECK_X
>  #undef PIPE_CONF_CHECK_I
>  #undef PIPE_CONF_CHECK_BOOL
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 40ebc94..6a89d2d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2512,6 +2512,8 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>  int intel_color_check(struct intel_crtc_state *crtc_state);
>  void intel_color_commit(const struct intel_crtc_state *crtc_state);
>  void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
> +void intel_color_config(struct intel_crtc_state *crtc_state);
> +u32 intel_color_bit_precision(struct drm_i915_private *dev_priv);
>  
>  /* intel_lspcon.c */
>  bool lspcon_init(struct intel_digital_port *intel_dig_port);

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