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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

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

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