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

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

On Mon, 25 Mar 2019, "Sharma, Swati2" <swati2.sharma@intel.com> wrote:
> On 20-Mar-19 6:11 PM, Jani Nikula wrote:
>> On Wed, 20 Mar 2019, "Sharma, Swati2" <swati2.sharma@intel.com> wrote:
>>> On 15-Mar-19 3:17 PM, Nikula, Jani wrote:
>>>> On Fri, 15 Mar 2019, swati2.sharma@intel.com wrote:
>>>>> From: Swati Sharma <swati2.sharma@intel.com>
>>>>>
>>>>> Added state checker to validate gamma_lut values. This
>>>>> reads hardware state, and compares the originally requested
>>>>> state to the state read from hardware.
>>>>>
>>>>> This implementation can be used for Gen9+ platforms,
>>>>> I haven't implemented it for legacy platforms. Just want to get
>>>>> feedback if this is the right approach to follow.
>>>>>
>>>>> Also, inverse function of drm_color_lut_extract is missing
>>>>> to convert hardware read values back to user values.
>>>>> Thinking for that. I have added all "TODOs" and "Placeholders".
>>>>>
>>>>> Another approach:
>>>>> Storing "word" to be written into hardware in dev_priv and
>>>>> instead of comparing blob, comparing "word"? In dev_priv,
>>>>> only pointer will be there (something like *gamma_word).
>>>> You can't store it in dev_priv because it's crtc state specific
>>>> data. Even if stored in crtc state, the approach doesn't help the
>>>> initial hw state readout and takeover.
>>>>
>>>> Please use intel-gfx mailing list for i915 patches.
>>>>
>>>>> For this too, I will send a patch to make it more clear.
>>>>>
>>>>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/i915_drv.h      |   1 +
>>>>>    drivers/gpu/drm/i915/intel_color.c   | 127 +++++++++++++++++++++++++++++++++--
>>>>>    drivers/gpu/drm/i915/intel_display.c |  50 ++++++++++++++
>>>>>    drivers/gpu/drm/i915/intel_drv.h     |   1 +
>>>>>    4 files changed, 173 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>>> index c4ffe19..b41bfaa 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -334,6 +334,7 @@ struct drm_i915_display_funcs {
>>>>>    	 * involved with the same commit.
>>>>>    	 */
>>>>>    	void (*load_luts)(const struct intel_crtc_state *crtc_state);
>>>>> +	void (*get_config)(struct intel_crtc_state *crtc_state);
>>>> The name is too generic.
>>>>
>>>>>    };
>>>>>    
>>>>>    #define CSR_VERSION(major, minor)	((major) << 16 | (minor))
>>>>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>>>>> index da7a07d..a515e9f 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_color.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_color.c
>>>>> @@ -74,6 +74,11 @@
>>>>>    #define ILK_CSC_COEFF_1_0		\
>>>>>    	((7 << 12) | ILK_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
>>>>>    
>>>>> +/* Mask to extract RGB values from registers */
>>>>> +#define COLOR_BLUE_MASK         0x000003FF              /* 9:0 */
>>>>> +#define COLOR_GREEN_MASK        0x000FFC00              /* 19:10 */
>>>>> +#define COLOR_RED_MASK          0x3FF00000              /* 29:20 */
>>>> These belong in i915_reg.h, and you need platform specific shifts and
>>>> masks. The code that writes the registers seems to use magic numbers...
>>>>
>>>>> +
>>>>>    static bool lut_is_legacy(const struct drm_property_blob *lut)
>>>>>    {
>>>>>    	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
>>>>> @@ -672,6 +677,97 @@ static void cherryview_load_luts(const struct intel_crtc_state *crtc_state)
>>>>>    	i9xx_load_luts_internal(crtc_state, NULL);
>>>>>    }
>>>>>    
>>>>> +static void bdw_get_gamma_config(struct intel_crtc_state *crtc_state, u32 offset)
>>>>> +{
>>>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>>>> +	struct drm_device *dev = crtc->base.dev;
>>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>>> +	u32 i, tmp, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>>>>> +	enum pipe pipe = crtc->pipe;
>>>>> +	struct drm_property_blob *blob = NULL;
>>>>> +	struct drm_color_lut *blob_data;
>>>>> +
>>>>> +	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
>>>>> +
>>>>> +	I915_WRITE(PREC_PAL_INDEX(pipe),
>>>>> +			(offset ? PAL_PREC_SPLIT_MODE : 0) |
>>>>> +			PAL_PREC_AUTO_INCREMENT |
>>>>> +			offset);
>>>>> +
>>>>> +	blob = drm_property_create_blob(dev,
>>>>> +			sizeof(struct drm_color_lut) * lut_size,
>>>>> +			NULL);
>>>>> +	if (IS_ERR(blob))
>>>>> +		return;
>>>>> +
>>>>> +	blob_data = blob->data;
>>>>> +
>>>>> +	for (i = 0; i < lut_size; i++) {
>>>>> +		tmp = I915_READ(PREC_PAL_DATA(pipe));
>>>>> +		/*
>>>>> +		 * TODO: convert RGB value read from register into corresponding user value using
>>>>> +		 * some wrapper like drm_color_lut_put() (or) intel_color_lut_put() so that it
>>>>> +		 * can be compared later.
>>>>> +		 */
>>>> Yeah, you'll need this.
>>> Can you please help in this?
>> Something like this:
>>
>> /* convert hw value with given bit_precision to lut property val */
>> u32 drm_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;
>> }
>>
>> /* compare two lut property values with given bit_precision */
>> bool drm_color_lut_match(u32 a, u32 b, u32 bit_precision)
>> {
>> 	u32 err = 0xffff >> bit_precision;
>>
>> 	return abs((long)a - b) <= err;
>> }
> Also, how to pass "bit_precision" since in platform specific load luts,
> its hard coded. Do we store platform specific bit precision?

You could add a function in intel_color.c that returns the bit
precision.

> Also, how to handle "else" case of load luts where we are creating the word
> w/o rounding off.

In drm_color_lut_pack() we don't actually need the if there at all, it's
just for clarity and similarity with the extract. bit_precision == 16
leads to 0 shift.

And in drm_color_lut_match() err becomes 0 for 16-bit precision.

BR,
Jani.

>>
>> I didn't double check the math, but it's probably close enough for
>> starters. ;)
>>
>>
>>
>> BR,
>> Jani.
>>
>>
>>
>>
>>>>> +		blob_data[i].red = (tmp & COLOR_RED_MASK) >> 20;
>>>>> +		blob_data[i].green = (tmp & COLOR_GREEN_MASK) >> 10;
>>>>> +		blob_data[i].blue = tmp & COLOR_BLUE_MASK;
>>>>> +	}
>>>>> +
>>>>> +	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
>>>>> +
>>>>> +	crtc_state->base.gamma_lut = blob;
>>>>> +}
>>>>> +
>>>>> +static void i9xx_get_config(struct intel_crtc_state *crtc_state)
>>>> Please include (de)gamma or color in the function names.
>>>>
>>>>> +{
>>>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>>>> +	struct drm_device *dev = crtc->base.dev;
>>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>>> +	u32 i, tmp;
>>>>> +	enum pipe pipe = crtc->pipe;
>>>>> +	struct drm_property_blob *blob = NULL;
>>>>> +	struct drm_color_lut *blob_data;
>>>>> +
>>>>> +	blob = drm_property_create_blob(dev,
>>>>> +					sizeof(struct drm_color_lut) * 256,
>>>>> +					NULL);
>>>>> +	if (IS_ERR(blob))
>>>>> +		return;
>>>>> +
>>>>> +	blob_data = blob->data;
>>>>> +
>>>>> +	for (i = 0; i < 256; i++) {
>>>>> +		if (HAS_GMCH(dev_priv))
>>>>> +			tmp = I915_READ(PALETTE(pipe, i));
>>>>> +		else
>>>>> +			tmp = I915_READ(LGC_PALETTE(pipe, i));
>>>>> +		/*
>>>>> +		 * TODO: convert RGB value read from register into corresponding user value using
>>>>> +		 * some wrapper like drm_color_lut_put() (or) intel_color_lut_put() so that it
>>>>> +		 * can be compared later.
>>>>> +		 */
>>>>> +		blob_data[i].red = (tmp & COLOR_RED_MASK) >> 20;
>>>>> +		blob_data[i].green = (tmp & COLOR_GREEN_MASK) >> 10;
>>>>> +		blob_data[i].blue = (tmp & COLOR_BLUE_MASK);
>>>>> +	}
>>>>> +
>>>>> +	crtc_state->base.gamma_lut = blob;
>>>>> +}
>>>>> +
>>>>> +static void icl_get_config(struct intel_crtc_state *crtc_state)
>>>>> +{
>>>>> +	/*
>>>>> +	 * TODO: placeholder for degamma validation
>>>>> +	 * glk_get_degamma_config(crtc_state, 0);
>>>>> +	 */
>>>>> +
>>>>> +	if (crtc_state_is_legacy_gamma(crtc_state))
>>>>> +		i9xx_get_config(crtc_state);
>>>>> +	else
>>>>> +		bdw_get_gamma_config(crtc_state, 0);
>>>>> +}
>>>>> +
>>>>>    void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
>>>>>    {
>>>>>    	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>>>>> @@ -679,6 +775,13 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
>>>>>    	dev_priv->display.load_luts(crtc_state);
>>>>>    }
>>>>>    
>>>>> +void intel_color_get_config(struct intel_crtc_state *crtc_state)
>>>>> +{
>>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>>>>> +
>>>>> +	dev_priv->display.get_config(crtc_state);
>>>> This will oops on platforms where you haven't implemented it yet.
>>>>
>>>>> +}
>>>>> +
>>>>>    void intel_color_commit(const struct intel_crtc_state *crtc_state)
>>>>>    {
>>>>>    	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>>>>> @@ -833,22 +936,34 @@ void intel_color_init(struct intel_crtc *crtc)
>>>>>    
>>>>>    	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
>>>>>    
>>>>> +	/*
>>>>> +	 * TODO: In the following if ladder, kept placeholders
>>>>> +	 * for extending lut validation for legacy platforms.
>>>>> +	 */
>>>> I think adding the placeholder comments is too messy.
>>>>
>>>>>    	if (HAS_GMCH(dev_priv)) {
>>>>> -		if (IS_CHERRYVIEW(dev_priv))
>>>>> +		if (IS_CHERRYVIEW(dev_priv)) {
>>>>>    			dev_priv->display.load_luts = cherryview_load_luts;
>>>>> -		else
>>>>> +			 /* dev_priv->display.get_config = cherryview_get_config; */
>>>>> +		} else {
>>>>>    			dev_priv->display.load_luts = i9xx_load_luts;
>>>>> +			dev_priv->display.get_config = i9xx_get_config;
>>>>> +		}
>>>>>    
>>>>>    		dev_priv->display.color_commit = i9xx_color_commit;
>>>>>    	} else {
>>>>> -		if (IS_ICELAKE(dev_priv))
>>>>> +		if (IS_ICELAKE(dev_priv)) {
>>>>>    			dev_priv->display.load_luts = icl_load_luts;
>>>>> -		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
>>>>> +			dev_priv->display.get_config = icl_get_config;
>>>>> +		} else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
>>>>>    			dev_priv->display.load_luts = glk_load_luts;
>>>>> -		else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
>>>>> +			/* dev_priv->display.get_config = glk_get_config; */
>>>>> +		} else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) {
>>>>>    			dev_priv->display.load_luts = broadwell_load_luts;
>>>>> -		else
>>>>> +			/* dev_priv->display.get_config = broadwell_get_config; */
>>>>> +		} else {
>>>>>    			dev_priv->display.load_luts = i9xx_load_luts;
>>>>> +			dev_priv->display.get_config = i9xx_get_config;
>>>>> +		}
>>>>>    
>>>>>    		if (INTEL_GEN(dev_priv) >= 9)
>>>>>    			dev_priv->display.color_commit = skl_color_commit;
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>> index 963b4bd..165d797 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -8212,6 +8212,11 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>>>>>    
>>>>>    	i9xx_get_pipe_color_config(pipe_config);
>>>>>    
>>>>> +	/*
>>>>> +	 * TODO: Yet to implement for legacy platforms
>>>>> +	 * intel_color_get_config(pipe_config);
>>>>> +	 */
>>>>> +
>>>> I'm not sure about adding these comments either.
>>>>
>>>>>    	if (INTEL_GEN(dev_priv) < 4)
>>>>>    		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
>>>>>    
>>>>> @@ -9284,6 +9289,11 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>>>>>    
>>>>>    	i9xx_get_pipe_color_config(pipe_config);
>>>>>    
>>>>> +	/*
>>>>> +	 * TODO: Yet to implement for legacy platforms
>>>>> +	 * intel_color_get_config(pipe_config);
>>>>> +	 */
>>>>> +
>>>>>    	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
>>>>>    		struct intel_shared_dpll *pll;
>>>>>    		enum intel_dpll_id pll_id;
>>>>> @@ -9932,6 +9942,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>>>>    		i9xx_get_pipe_color_config(pipe_config);
>>>>>    	}
>>>>>    
>>>>> +	intel_color_get_config(pipe_config);
>>>>> +
>>>>>    	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>>>>>    	if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
>>>>>    		WARN_ON(power_domain_mask & BIT_ULL(power_domain));
>>>>> @@ -11990,6 +12002,34 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>>>>>    	return false;
>>>>>    }
>>>>>    
>>>>> +static bool intel_compare_blob(struct drm_property_blob *blob1,
>>>>> +			       struct drm_property_blob *blob2)
>>>>> +{
>>>>> +	struct drm_color_lut *sw_lut = blob1->data;
>>>>> +	struct drm_color_lut *hw_lut = blob2->data;
>>>>> +	int sw_lut_size, hw_lut_size;
>>>>> +	int i;
>>>>> +
>>>>> +	sw_lut_size = drm_color_lut_size(blob1);
>>>>> +	hw_lut_size = drm_color_lut_size(blob2);
>>>>> +
>>>>> +	if (sw_lut_size != hw_lut_size) {
>>>>> +		DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n", hw_lut_size, sw_lut_size);
>>>> Please don't add these debug logs here. Instead, add a separate function
>>>> to log errors at the higher level. Contrast PIPE_CONF_CHECK_INFOFRAME()
>>>> and pipe_config_infoframe_err(). That can come as a follow-up
>>>> improvement, but the debug logs here are unnecessary.
>>>>
>>>>> +		return false;
>>>>> +	}
>>>>> +
>>>>> +	for (i = 0; i < sw_lut_size; i++) {
>>>>> +		if (hw_lut[i].red != sw_lut[i].red ||
>>>>> +		    hw_lut[i].blue != sw_lut[i].blue ||
>>>>> +		    hw_lut[i].green != sw_lut[i].green) {
>>>>> +			DRM_DEBUG_KMS("Invalid LUT value; sw_lut value doesn't match hw_lut value\n");
>>>>> +			return false;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	return true;
>>>>> +}
>>>>> +
>>>> I'd put this in intel_color.c and name it properly. You can't use it to
>>>> compare random blobs, and the comparison needs to take platform specific
>>>> bit precision of the readout into account.
>>>>
>>>> The blobs may be NULL.
>>>>
>>>>>    static bool
>>>>>    intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>>>>>    			  struct intel_crtc_state *current_config,
>>>>> @@ -12148,6 +12188,14 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>>>>>    	} \
>>>>>    } while (0)
>>>>>    
>>>>> +#define PIPE_CONF_CHECK_BLOB(name) do { \
>>>> CHECK_LUT or CHECK_COLOR_LUT or something, not just CHECK_BLOB.
>>>>
>>>>> +	if (!intel_compare_blob(current_config->name, pipe_config->name)) { \
>>>>> +		pipe_config_err(adjust, __stringify(name), \
>>>>> +				"hw_state doesn't match sw_state\n"); \
>>>>> +		ret = false; \
>>>>> +	} \
>>>>> +} while (0)
>>>>> +
>>>>>    #define PIPE_CONF_QUIRK(quirk) \
>>>>>    	((current_config->quirks | pipe_config->quirks) & (quirk))
>>>>>    
>>>>> @@ -12287,6 +12335,8 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>>>>>    	PIPE_CONF_CHECK_INFOFRAME(spd);
>>>>>    	PIPE_CONF_CHECK_INFOFRAME(hdmi);
>>>>>    
>>>>> +	PIPE_CONF_CHECK_BLOB(base.gamma_lut);
>>>>> +
>>>>>    #undef PIPE_CONF_CHECK_X
>>>>>    #undef PIPE_CONF_CHECK_I
>>>>>    #undef PIPE_CONF_CHECK_BOOL
>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>>> index 40ebc94..a6acd9b 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>>> @@ -2512,6 +2512,7 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>>>>>    int intel_color_check(struct intel_crtc_state *crtc_state);
>>>>>    void intel_color_commit(const struct intel_crtc_state *crtc_state);
>>>>>    void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
>>>>> +void intel_color_get_config(struct intel_crtc_state *crtc_state);
>>>>>    
>>>>>    /* intel_lspcon.c */
>>>>>    bool lspcon_init(struct intel_digital_port *intel_dig_port);

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