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

["Shankar, Uma" <uma.shankar@intel.com>]

>-----Original Message-----
>From: Roper, Matthew D
>Sent: Thursday, November 1, 2018 5:10 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>;
>Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [PATCH 1/3] drm/i915/icl: Add icl pipe degamma and
>gamma support
>
>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@intel.com>
>> > ---
>> >  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).

Ok, will modify this.

>> > +		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.

Sure, will do it in similar way.

>> >  }
>> >
>> >  /* 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?

Yes, this hardcoding can be dropped from here.

>> Actually, upon closer inspection it looks like LUT size, ranges, etc.
>> is sort of complicated; more comments farther down.

Yeah, with the introduction of multi segmented gamma this is really
not straight forward. Current implementation just handle things without
multi segmented gamma. I will add support of multi segmented gamma
and introduce some kind of tuples (set of 3 or 4 struct elements to define
a segment). Ville has given some ideas on the same.

>>
>> > +	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.

Yes, this is my understanding as well. So, when we need to reprogram the
LUT, take index to 0 and set auto increment. This is working and all the
LUT values get programmed correctly.

>> > +	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.

Yes, you are right. I will add bit more explanation here though, to avoid
ambiguity and give more clarity.

>> > +			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.

Yes, the 34th and 35the entry is programmed to clamp at 1.0.

>
>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?

I believe current userspace just goes for 1.0. Yes, we can extend and add and
extended mode as well to deal with these 7.0 ranges. I will try to create some
kind of design and send a RFC for input to handle multi segment as well as such
extended range inputs. Will try to catch Ville as well for his inputs :)

>> > +	} 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.

This is ok and taken care as mentioned above.

>> > +	}
>> > +
>> > +	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?

Sure, I will do that.

>> > +
>> > +	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?

As mentioned above, will try to create some RFC for review to handle extended ranges.

Thanks Matt for your review and valuable comments.

Regards,
Uma Shankar
>>
>> 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@lists.freedesktop.org
>> > 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@lists.freedesktop.org
>> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx