>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, November 30, 2018 4:38 AM
>To: Shankar, Uma <uma.shankar@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Lankhorst, Maarten
><maarten.lankhorst@xxxxxxxxx>; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Sharma,
>Shashank <shashank.sharma@xxxxxxxxx>
>Subject: Re: [v3 1/3] drm/i915/icl: Add icl pipe degamma and gamma support
>
>On Thu, Nov 29, 2018 at 08:21:41PM +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.
>>
>> v3: Addressed Matt's review comments. Removed rmw patterns as
>> suggested by Matt.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 3 ++
>> drivers/gpu/drm/i915/intel_color.c | 73
>> ++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 76 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index 47baf2fe..b0147bf 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7058,6 +7058,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..7c8c996 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -422,6 +422,7 @@ static void bdw_load_degamma_lut(struct
>> drm_crtc_state *state) static void bdw_load_gamma_lut(struct
>> drm_crtc_state *state, u32 offset) {
>> struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
>> + struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
>> enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
>> uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>>
>> @@ -464,6 +465,9 @@ 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 (INTEL_GEN(dev_priv) >= 11)
>> + intel_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
>
>icl_load_luts updates this field as soon as this function returns, so it might be
>easier to make this update there, at the same point you set MODE_10BIT.
>
>However the overall use and handling of gamma_mode by the driver seems
>strange. Generally we try to calculate important state variables during atomic
>check and then use those derived state values during the commit programming
>phase, but we're not following that pattern with this field.
>Can we just move the logic to build this field into the atomic check phase so that
>we're not trying to update state during the commit?
>
>Actually, looking closer, I'm not sure if we even need this field. For most
>platforms we have a fixed set of bits that we write (e.g.,
>PRE_CSC_GAMMA_ENABLE | POST_CSC_GAMMA_ENABLE |
>GAMMA_MODE_10_BIT for ICL), so we can just put those directly into the
>I915_WRITE statement.
>Unless I'm overlooking something, the only place where this variable is used for
>something else is in haswell_load_luts() where we have to disable IPS before
>accessing the LUT's if we're in split gamma mode.
>But the driver itself only ever sets 8BIT for Haswell as far as I can see. BIOS might
>have put us in split mode before the driver loaded, but that might be easier to
>sanitize during hardware state readout?
Yeah I agree, we can avoid this state variable altogether. I will try to clean it up
and resend the series.
>
>> }
>>
>> /* Loads the palette/gamma unit for the CRTC on Broadwell+. */ @@
>> -523,6 +527,53 @@ 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);
>> + struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
>> + enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
>> + const uint32_t lut_size = INTEL_INFO(dev_priv)-
>>color.degamma_lut_size;
>> + uint32_t 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.
>> + */
>> + 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++) {
>> + /*
>> + * First 33 entries represent range from 0 to 1.0
>> + * 34th and 35th entry will represent extended range
>> + * inputs 3.0 and 7.0 respectively, currently clamped
>> + * at 1.0.
>> + * ToDo: Extend to max 7.0.
>> + */
>> + uint32_t word =
>> + drm_color_lut_extract(lut[i].red, 16);
>
>A few minor things I notice; not sure if you want to bother addressing them at the
>moment or not, but I figured I'd point them out now:
>
> * Technically we could just set word directly to lut[i].red since we're
> extracting the full 16-bits (so no rounding) and our u16 input can't
> hold any value outside our clamping range of [0,0.99998].
Yes, it's practically NOOP here. Will update this.
> * It's a minor deviation, but the scaling is slightly off here such
> that the 33rd entry written by this loop (which represents a 1.0
> input) will be clamped to 0xFFFF (0.99998) rather than 1.0. So
> userspace won't be able to exactly match the linear table we'd
> otherwise program if no userspace LUT was provided.
Yes, with the current data structure definition of lut (16bit entries), its
not possible for userspace to send exact 1.0 (which will require 17bits).
We need to use extended structure in order to get over this corner case
and also to handle extended range 3.0 and 7.0 etc. I tried to introduce that
as part of "drm: Add Enhanced Gamma LUT precision structure" patch in series
https://patchwork.freedesktop.org/series/30875/
We need to use above extension to get over this corner case.
> * GLK and gen11 deviate from our other platforms in that they just use
> a single value for red, green, and blue rather than accepting
> separate values for each. GLK punts on this and doesn't even expose
> degamma to userspace (just loads a linear table), whereas your patch
> silently ignores green/blue values, which might be a surprise to
> userspace. Maybe we should switch GLK code to do what you do here,
> but also add some validation of new table blobs during atomic-check
> such that we reject any table with non-equal values for r/g/b. While
> we're at it, we could also check to ensure that LUT entries are never
> decreasing, since that's also a hardware requirement documented in
> the bspec.
Ok sure, I will add that check to make sure userspace requests for non-equal
R, G and B components is not allowed and user is aware that hardware expects
equal LUT values for each component.
Thanks Matt for your useful comments and inputs.
Regards,
Uma Shankar
>
>Matt
>
>> + I915_WRITE(PRE_CSC_GAMC_DATA(pipe), word);
>> + }
>> + } 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);
>> + }
>> + }
>> +
>> + intel_state->gamma_mode |= 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 +657,26 @@ 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;
>> +
>> + if (crtc_state_is_legacy_gamma(state)) {
>> + haswell_load_luts(state);
>> + return;
>> + }
>> +
>> + icl_load_degamma_lut(state);
>> + bdw_load_gamma_lut(state, 0);
>> +
>> + intel_state->gamma_mode |= GAMMA_MODE_MODE_10BIT;
>> + I915_WRITE(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 +733,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
>>
>
>--
>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
