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
