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

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

On Thu, Mar 28, 2019 at 01:56:18PM -0700, Matt Roper wrote:
> On Thu, Mar 28, 2019 at 12:03:48PM +0530, Swati Sharma 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
> > -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.
> > -Optimizations and refractoring
> > 
> > Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> 
> I agree with Jani's feedback and have a couple other comments inline below.
> 
> Also, since I don't see it on the TODO list here, do you intend to also
> readout and compare degamma and CTM eventually?
> 
> > ---
> >  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);
> >  };
> >  
> >  #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)
> >  
> > +#define LGC_PALETTE_RED_MASK		(0xFF << 16)
> > +#define LGC_PALETTE_GREEN_MASK		(0xFF << 8)
> > +#define LGC_PALETTE_BLUE_MASK		(0xFF << 0)
> > +
> >  #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;
> > +}
> 
> Doesn't bdw (gen8) also use a 10 bit palette (PREC_PAL_DATA)?
> 
> I think we probably want to figure out the number of bits to compare
> based on gamma_mode rather than the platform we're running on.  E.g., a
> platform with 10-bit precision palettes might still be using legacy
> tables with 8 bits of precision in some cases.  And some platforms have
> even more potential gamma modes that we might enable in the future too.

We'll need to look at .gamma_mode, .gamma_enable (on pre-icl), and
.cgm_mode (on chv). Oh and .c8_planes too I suppose. Unfortunately
that's a bit of a problem since we have no real plane readout code
at the moment. Hmm, I guess we can ignore .c8_planes (and .gamma_enable
too I suppose) and just blindly read out the legacy LUT whenever
.gamma_mode says so, but we'll have to be prepared for the case where
the software state had no gamma LUT at all (since we program
.gamma_mode to 8bit in that case too).

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