Re: [PATCH 1/3] drm/i915/icl: Add icl pipe degamma and gamma support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Oct 30, 2018 at 05:03:57PM -0700, Matt Roper wrote:
> On Wed, Oct 24, 2018 at 08:30:11PM +0530, Uma Shankar wrote:
> > Add support for icl pipe degamma and gamma.
> > 
> > v2: Removed a POSTING_READ and corrected the Bit
> > Definition as per Maarten's comments.
> > 
> > Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h    |  3 ++
> >  drivers/gpu/drm/i915/intel_color.c | 74 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8bd61f9..3adf689 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6965,6 +6965,9 @@ enum {
> >  #define GAMMA_MODE_MODE_12BIT	(2 << 0)
> >  #define GAMMA_MODE_MODE_SPLIT	(3 << 0)
> >  
> > +#define PRE_CSC_GAMMA_ENABLE	(1 << 31)
> > +#define POST_CSC_GAMMA_ENABLE	(1 << 30)
> > +
> >  /* DMC/CSR */
> >  #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
> >  #define CSR_SSP_BASE_ADDR_GEN9	0x00002FC0
> > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > index 5127da2..12c659f 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -424,6 +424,7 @@ static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
> >  	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> >  	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> >  	uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > +	uint32_t tmp;
> >  
> >  	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
> >  
> > @@ -464,6 +465,11 @@ static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
> >  		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16) - 1);
> >  		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16) - 1);
> >  	}
> > +
> > +	if (IS_ICELAKE(dev_priv)) {
> 
> We probably want to do "INTEL_GEN(dev_priv) >= 11" here to future-proof
> this against future gen's (or other platforms that might match gen11
> design without being classified as icelake).
> 
> > +		tmp = I915_READ(GAMMA_MODE(pipe));
> > +		I915_WRITE(GAMMA_MODE(pipe), tmp | POST_CSC_GAMMA_ENABLE);
> > +	}
> 
> We generally try to avoid read-modify-write approaches to updating
> registers; it's better to build the full register value from scratch to
> make sure we know exactly what's in it.
> 
> There are a few places in this patch you do a r-m-w cycle.  I'd suggest
> we add an intel_crtc_state->gamma_mode field and calculate all the
> appropriate bits during the atomic check phase.  Then we can just do a
> 
>     if (INTEL_GEN(dev_priv) >= 11)
>         I915_WRITE(GAMMA_MODE(pipe), intel_crtc_state->gamma_mode);
> 
> once somewhere more central any time crtc_state->color_mgmt_changed is
> true.
> 
> >  }
> >  
> >  /* Loads the palette/gamma unit for the CRTC on Broadwell+. */
> > @@ -523,6 +529,50 @@ static void glk_load_degamma_lut(struct drm_crtc_state *state)
> >  		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));
> >  }
> >  
> > +static void icl_load_degamma_lut(struct drm_crtc_state *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> > +	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> > +	const uint32_t lut_size = 33;
> 
> Isn't this the value from INTEL_INFO(dev)->color.degamma_lut_size?
> Actually, upon closer inspection it looks like LUT size, ranges, etc. is
> sort of complicated; more comments farther down.
> 
> > +	uint32_t tmp, i;
> > +
> > +	/*
> > +	 * When setting the auto-increment bit, the hardware seems to
> > +	 * ignore the index bits, so we need to reset it to index 0
> > +	 * separately.
> > +	 */
> 
> The final sentence of the bspec description says (emphasis mine):
> 
>         "While in auto increment mode, after performing reads or writes
>         to only part of the range, the auto increment bit must be
>         cleared *before* resetting the index value."
> 
> so I suspect it's technically the first write (which disables the
> auto-increment) that ignores the index bits.  Then your second write
> both sets the index bits (to 0) and turns auto-increment back on.
> 
> > +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), 0);
> > +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), PRE_CSC_GAMC_AUTO_INCREMENT);
> > +
> > +	if (state->degamma_lut) {
> > +		struct drm_color_lut *lut =
> > +			(struct drm_color_lut *) state->degamma_lut->data;
> > +		for (i = 0; i < lut_size; i++) {
> > +			/*
> > +			 * Currently Clamp input to 1.0.
> > +			 * ToDo: Extend to max 7.0.
> > +			 */
> 
> This comment is a little confusing.  I think what you mean is that this
> for() loop covers the first 33 entries, which represent the range of
> inputs from 0 to 1.0.  The remaining two entries are for extended range
> of inputs (representing gamma values at 3.0 and 7.0) and are not evenly
> spaced.
> 
> > +			uint32_t word =
> > +				drm_color_lut_extract(lut[i].red, 16);
> > +			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), word);
> > +		}
> 
> Following the loop is where we'd need to deal with the two extended
> range entries, right?  So if we leave dealing with extended range
> properly as a TODO for now, then I think we want to fill them in with
> linear values.  E.g.,
> 
>      I915_WRITE(PRE_CSC_GAMC_DATA(pipe), 0x30000);
>      I915_WRITE(PRE_CSC_GAMC_DATA(pipe), 0x70000);

Actually, it looks like you actually do set these at the bottom of the
function (and clamp to 1.0), so that's fine and you can disregard this
comment.


Matt

> 
> Also, I believe the 1.0 input can have a value >= 1.0, so we need to
> extract more bits of precision for that.
> 
> I'm not really an expert on how this color management stuff gets used by
> userspace in practice, but I suspect our current ABI probably needs to
> be augmented to deal more complicated gamma LUT's than we support today.
> E.g., different ranges with different input values, precision, etc.  Do
> we have a clear understanding of if/how userspace wants to deal with
> extended range?
> 
> > +	} else {
> > +		/* load a linear table. */
> > +		for (i = 0; i < lut_size; i++) {
> > +			uint32_t v = (i * (1 << 16)) / (lut_size - 1);
> > +
> > +			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
> > +		}
> 
> As above, I think we should still fill in the last two entries with
> linear values for now.
> 
> > +	}
> > +
> > +	tmp = I915_READ(GAMMA_MODE(pipe));
> > +	I915_WRITE(GAMMA_MODE(pipe), tmp | PRE_CSC_GAMMA_ENABLE);
> > +
> > +	/* Clamp values > 1.0. */
> > +	while (i++ < 35)
> > +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));
> > +}
> > +
> >  static void glk_load_luts(struct drm_crtc_state *state)
> >  {
> >  	struct drm_crtc *crtc = state->crtc;
> > @@ -606,6 +656,28 @@ static void cherryview_load_luts(struct drm_crtc_state *state)
> >  	i9xx_load_luts_internal(crtc, NULL, to_intel_crtc_state(state));
> >  }
> >  
> > +static void icl_load_luts(struct drm_crtc_state *state)
> > +{
> > +	struct drm_crtc *crtc = state->crtc;
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
> > +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > +
> > +	icl_load_degamma_lut(state);
> > +
> > +	if (crtc_state_is_legacy_gamma(state)) {
> > +		haswell_load_luts(state);
> > +		return;
> > +	}
> 
> This block will only run if there's no degamma LUT, so it might be
> slightly easier to read the code if we move this above the
> icl_load_degamma_lut() call above?
> 
> > +
> > +	bdw_load_gamma_lut(state, 0);
> > +	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
> 
> I think we also need to update PAL_EXT_GC_MAX with a FP3.16 for the 3.0
> input value and the PAL_EXT2_GC_MAX for the 7.0 input.  Assuming we want
> to leave the ABI details of this until later, I assume we'd handle this
> with linear values like I suggested for the degamma extended range
> entries?
> 
> 
> Matt
> 
> > +
> > +	I915_WRITE(GAMMA_MODE(pipe), I915_READ(GAMMA_MODE(pipe)) |
> > +			intel_state->gamma_mode);
> > +}
> > +
> >  void intel_color_load_luts(struct drm_crtc_state *crtc_state)
> >  {
> >  	struct drm_device *dev = crtc_state->crtc->dev;
> > @@ -662,6 +734,8 @@ void intel_color_init(struct drm_crtc *crtc)
> >  	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> >  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> >  		dev_priv->display.load_luts = glk_load_luts;
> > +	} else if (IS_ICELAKE(dev_priv)) {
> > +		dev_priv->display.load_luts = icl_load_luts;
> >  	} else {
> >  		dev_priv->display.load_luts = i9xx_load_luts;
> >  	}
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux