| Message ID | 1540393213-11907-2-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Gen 11 pipe color features | expand |
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@intel.com> > --- > 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); 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > --- > > 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@lists.freedesktop.org > > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>-----Original Message----- >From: Roper, Matthew D >Sent: Thursday, November 1, 2018 5:10 AM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; >Lankhorst, Maarten <maarten.lankhorst@intel.com> >Subject: Re: [Intel-gfx] [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@intel.com> >> > --- >> > 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@lists.freedesktop.org >> > 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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Matt Roper >Graphics Software Engineer >IoTG Platform Enabling & Development >Intel Corporation >(916) 356-2795
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)) { + tmp = I915_READ(GAMMA_MODE(pipe)); + I915_WRITE(GAMMA_MODE(pipe), tmp | POST_CSC_GAMMA_ENABLE); + } } /* 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; + 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. + */ + 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. + */ + uint32_t word = + drm_color_lut_extract(lut[i].red, 16); + 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); + } + } + + 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; + } + + bdw_load_gamma_lut(state, 0); + intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT; + + 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; }
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@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 3 ++ drivers/gpu/drm/i915/intel_color.c | 74 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+)