[Intel-gfx] [v3 1/3] drm/i915/icl: Add icl pipe degamma and gamma support
Matt Roper
matthew.d.roper at intel.com
Thu Nov 29 23:07:32 UTC 2018
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 at intel.com>
> ---
> 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?
> }
>
> /* 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].
* 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.
* 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.
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
More information about the Intel-gfx
mailing list