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

From: Jani Nikula <jani.nikula@intel.com>
To: Swati Sharma <swati2.sharma@intel.com>, intel-gfx@lists.freedesktop.org
Cc: daniel.vetter@ffwll.ch, ankit.k.nautiyal@intel.com
Subject: Re: [v9][PATCH 03/11] drm/i915/display: Add func to compare hw/sw gamma lut
Date: Fri, 30 Aug 2019 15:47:46 +0300	[thread overview]
Message-ID: <87o9066cr1.fsf@intel.com> (raw)
In-Reply-To: <1567153913-20166-4-git-send-email-swati2.sharma@intel.com>

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.

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