>-----Original Message-----
>From: Roper, Matthew D
>Sent: Thursday, November 1, 2018 5:10 AM
>To: Shankar, Uma <uma.shankar@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>;
>Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx>
>Subject: Re: [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@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).
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@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
