Re: [Intel-gfx] [v5 4/6] drm/i915/icl: Add icl pipe degamma and gamma support
>-Original Message- >From: Roper, Matthew D >Sent: Saturday, January 12, 2019 3:49 AM >To: Shankar, Uma >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten >; Syrjala, Ville ; >Sharma, >Shashank >Subject: Re: [v5 4/6] drm/i915/icl: Add icl pipe degamma and gamma support > >On Tue, Jan 08, 2019 at 01:07:31PM +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. >> >> v4: Fixed Matt's review comments. >> >> v5: Corrected macro alignment as per Jani Nikula's comments. >> Addressed Ville and Matt's review comments. >> >> Signed-off-by: Uma Shankar >> --- >> drivers/gpu/drm/i915/i915_reg.h| 12 --- >> drivers/gpu/drm/i915/intel_color.c | 65 >> ++ >> 2 files changed, 72 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h >> b/drivers/gpu/drm/i915/i915_reg.h index 44958d9..f29eef7 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -7088,11 +7088,13 @@ enum { >> #define _GAMMA_MODE_A 0x4a480 >> #define _GAMMA_MODE_B 0x4ac80 >> #define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A, >_GAMMA_MODE_B) >> -#define GAMMA_MODE_MODE_MASK(3 << 0) >> -#define GAMMA_MODE_MODE_8BIT(0 << 0) >> -#define GAMMA_MODE_MODE_10BIT (1 << 0) >> -#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) >> +#define GAMMA_MODE_MODE_MASK (3 << 0) >> +#define GAMMA_MODE_MODE_8BIT (0 << 0) >> +#define GAMMA_MODE_MODE_10BIT (1 << 0) >> +#define GAMMA_MODE_MODE_12BIT (2 << 0) >> +#define GAMMA_MODE_MODE_SPLIT (3 << 0) >> >> /* DMC/CSR */ >> #define CSR_PROGRAM(i) _MMIO(0x8 + (i) * 4) >> diff --git a/drivers/gpu/drm/i915/intel_color.c >> b/drivers/gpu/drm/i915/intel_color.c >> index 9a72e64..9cd4646 100644 >> --- a/drivers/gpu/drm/i915/intel_color.c >> +++ b/drivers/gpu/drm/i915/intel_color.c >> @@ -520,6 +520,69 @@ static void glk_load_luts(struct intel_crtc_state >*crtc_state) >> POSTING_READ(GAMMA_MODE(pipe)); >> } >> >> +static void icl_load_degamma_lut(struct intel_crtc_state *crtc_state) > >As Ville noted, I think the degamma LUT works the same way on GLK-ICL; the >gamma part may be different (and there's extra stuff like the extra output CSC >on >ICL), but for degamma specifically I think the code you wrote below could just >be >used to replace the current body of glk_load_degamma_lut rather than adding it >as a separate function. Ok, will merge that to one function and extend GLK to handle this instead of the current pass through. >Since GLK-ICL only support equal r/g/b values, we also need to land the LUT >validation patches I wrote in December. Those are fully reviewed so I'll do >that as >soon as I get an ack from Dave/Daniel to merge the drm core patch through the >Intel tree. Yes, this would be needed. Thanks Matt for taking this up. Regards, Uma Shankar > >Matt > >> +{ >> +struct drm_device *dev = crtc_state->base.crtc->dev; >> +struct drm_i915_private *dev_priv = to_i915(dev); >> +enum pipe pipe = to_intel_crtc(crtc_state->base.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 (crtc_state->base.degamma_lut) { >> +struct drm_color_lut *lut = crtc_state->base.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. Since the precision is 16bit, the user value >> + * can be directly filled to register. >> + * ToDo: Extend to max 7.0. >> + */ >> +I915_WRITE(PRE_CSC_GAMC_DATA(pipe), lut[i].green); >> +} >> +} 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); >> +} >> +} >> + >> +/* Clamp values > 1.0. */ >> +while (i++ < 35) >>
Re: [Intel-gfx] [v5 4/6] drm/i915/icl: Add icl pipe degamma and gamma support
On Tue, Jan 08, 2019 at 01:07:31PM +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. > > v4: Fixed Matt's review comments. > > v5: Corrected macro alignment as per Jani Nikula's comments. > Addressed Ville and Matt's review comments. > > Signed-off-by: Uma Shankar > --- > drivers/gpu/drm/i915/i915_reg.h| 12 --- > drivers/gpu/drm/i915/intel_color.c | 65 > ++ > 2 files changed, 72 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 44958d9..f29eef7 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7088,11 +7088,13 @@ enum { > #define _GAMMA_MODE_A0x4a480 > #define _GAMMA_MODE_B0x4ac80 > #define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B) > -#define GAMMA_MODE_MODE_MASK (3 << 0) > -#define GAMMA_MODE_MODE_8BIT (0 << 0) > -#define GAMMA_MODE_MODE_10BIT(1 << 0) > -#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) > +#define GAMMA_MODE_MODE_MASK(3 << 0) > +#define GAMMA_MODE_MODE_8BIT(0 << 0) > +#define GAMMA_MODE_MODE_10BIT (1 << 0) > +#define GAMMA_MODE_MODE_12BIT (2 << 0) > +#define GAMMA_MODE_MODE_SPLIT (3 << 0) > > /* DMC/CSR */ > #define CSR_PROGRAM(i) _MMIO(0x8 + (i) * 4) > diff --git a/drivers/gpu/drm/i915/intel_color.c > b/drivers/gpu/drm/i915/intel_color.c > index 9a72e64..9cd4646 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -520,6 +520,69 @@ static void glk_load_luts(struct intel_crtc_state > *crtc_state) > POSTING_READ(GAMMA_MODE(pipe)); > } > > +static void icl_load_degamma_lut(struct intel_crtc_state *crtc_state) As Ville noted, I think the degamma LUT works the same way on GLK-ICL; the gamma part may be different (and there's extra stuff like the extra output CSC on ICL), but for degamma specifically I think the code you wrote below could just be used to replace the current body of glk_load_degamma_lut rather than adding it as a separate function. Since GLK-ICL only support equal r/g/b values, we also need to land the LUT validation patches I wrote in December. Those are fully reviewed so I'll do that as soon as I get an ack from Dave/Daniel to merge the drm core patch through the Intel tree. Matt > +{ > + struct drm_device *dev = crtc_state->base.crtc->dev; > + struct drm_i915_private *dev_priv = to_i915(dev); > + enum pipe pipe = to_intel_crtc(crtc_state->base.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 (crtc_state->base.degamma_lut) { > + struct drm_color_lut *lut = crtc_state->base.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. Since the precision is 16bit, the user value > + * can be directly filled to register. > + * ToDo: Extend to max 7.0. > + */ > + I915_WRITE(PRE_CSC_GAMC_DATA(pipe), lut[i].green); > + } > + } 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); > + } > + } > + > + /* Clamp values > 1.0. */ > + while (i++ < 35) > + I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16)); > +} > + > +static void icl_load_luts(struct intel_crtc_state *crtc_state) > +{ > + struct drm_crtc *crtc = crtc_state->base.crtc; > + struct drm_device *dev = crtc_state->base.crtc->dev; > + struct drm_i915_private *dev_priv = to_i915(dev); > + enum pipe pipe = to_intel_crtc(crtc)->pipe; > + > + if (crtc_state_is_legacy_gamma(crtc_state)) { > + haswell_load_luts(crtc_state); > + return; > + } > + > + icl_load_degamma_lut(crtc_stat