Re: [v9][PATCH 03/11] drm/i915/display: Add func to compare hw/sw gamma lut

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

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

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

BR,
Jani.





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

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