| Message ID | 20191009065542.27415-4-swati2.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix broken state checker and enable state checker for icl+ | expand |
On Wed, Oct 09, 2019 at 12:25:41PM +0530, Swati Sharma wrote: > For icl+, have hw read out to create hw blob of gamma > lut values. icl+ platforms supports multi segmented gamma > mode by default, add hw lut creation for this mode. > > This will be used to validate gamma programming using dsb > (display state buffer) which is a tgl specific feature. > > Major change done-removal of readouts of coarse and fine segments > because PAL_PREC_DATA register isn't giving propoer values. > State checker limited only to "fine segment" > > v2: -readout code for multisegmented gamma has to come > up with some intermediate entries that aren't preserved > in hardware (Jani N) > -linear interpolation (Ville) > -moved common code to check gamma_enable to specific funcs, > since icl doesn't support that > v3: -use u16 instead of __u16 [Jani N] > -used single lut [Jani N] > -improved and more readable for loops [Jani N] > -read values directly to actual locations and then fill gaps [Jani N] > -moved cleaning to patch 1 [Jani N] > -renamed icl_read_lut_multi_seg() to icl_read_lut_multi_segment to > make it similar to icl_load_luts() > -renamed icl_compute_interpolated_gamma_blob() to > icl_compute_interpolated_gamma_lut_values() more sensible, I guess > v4: -removed interpolated func for creating gamma lut values > -removed readouts of fine and coarse segments, failure to read PAL_PREC_DATA > correctly > v5: -added gamma_enable check inside read_luts() > > Signed-off-by: Swati Sharma <swati2.sharma@intel.com> > --- > drivers/gpu/drm/i915/display/intel_color.c | 114 ++++++++++++++++++--- > drivers/gpu/drm/i915/i915_reg.h | 6 ++ > 2 files changed, 108 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > index fa44eb73d088..614e0ad386ca 100644 > --- a/drivers/gpu/drm/i915/display/intel_color.c > +++ b/drivers/gpu/drm/i915/display/intel_color.c > @@ -1477,6 +1477,25 @@ static int glk_gamma_precision(const struct intel_crtc_state *crtc_state) > } > } > > +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state) > +{ > + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0) > + return 0; > + > + switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) { > + case GAMMA_MODE_MODE_8BIT: > + return 8; > + case GAMMA_MODE_MODE_10BIT: > + return 10; > + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: > + return 16; > + default: > + MISSING_CASE(crtc_state->gamma_mode); > + return 0; > + } > + > +} > + > int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state) > { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > @@ -1488,7 +1507,9 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat > else > return i9xx_gamma_precision(crtc_state); > } else { > - if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) > + if (INTEL_GEN(dev_priv) >= 11) > + return icl_gamma_precision(crtc_state); > + else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) > return glk_gamma_precision(crtc_state); > else if (IS_IRONLAKE(dev_priv)) > return ilk_gamma_precision(crtc_state); > @@ -1519,6 +1540,20 @@ static bool intel_color_lut_entry_equal(struct drm_color_lut *lut1, > return true; > } > > +static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1, > + struct drm_color_lut *lut2, > + int lut_size, u32 err) > +{ > + int i; > + > + for (i = 0; i < 9; i++) { > + if (!err_check(&lut1[i], &lut2[i], err)) > + return false; > + } > + > + return true; > +} > + > bool intel_color_lut_equal(struct drm_property_blob *blob1, > struct drm_property_blob *blob2, > u32 gamma_mode, u32 bit_precision) > @@ -1537,16 +1572,8 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, > lut_size2 = drm_color_lut_size(blob2); > > /* check sw and hw lut size */ > - switch (gamma_mode) { > - case GAMMA_MODE_MODE_8BIT: > - case GAMMA_MODE_MODE_10BIT: > - if (lut_size1 != lut_size2) > - return false; > - break; > - default: > - MISSING_CASE(gamma_mode); > - return false; > - } > + if (lut_size1 != lut_size2) > + return false; > > lut1 = blob1->data; > lut2 = blob2->data; > @@ -1554,13 +1581,18 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, > err = 0xffff >> bit_precision; > > /* check sw and hw lut entry to be equal */ > - switch (gamma_mode) { > + switch (gamma_mode & GAMMA_MODE_MODE_MASK) { > case GAMMA_MODE_MODE_8BIT: > case GAMMA_MODE_MODE_10BIT: > if (!intel_color_lut_entry_equal(lut1, lut2, > lut_size2, err)) > return false; > break; > + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: > + if (!intel_color_lut_entry_multi_equal(lut1, lut2, > + lut_size2, err)) I don't think you need a new function for that. Just pass 9 as the size to intel_color_lut_entry_equal() ? Hmm, should probably rename that to just intel_color_lut_equal() since it checks the entire LUT (or at least the specified subset) and not just a single entry... > + return false; > + break; > default: > MISSING_CASE(gamma_mode); > return false; > @@ -1815,6 +1847,63 @@ static void glk_read_luts(struct intel_crtc_state *crtc_state) > crtc_state->base.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0)); > } > > +static struct drm_property_blob * > +icl_read_lut_multi_segment(const struct intel_crtc_state *crtc_state) > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + int lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size; > + enum pipe pipe = crtc->pipe; > + struct drm_property_blob *blob; > + struct drm_color_lut *blob_data; > + u32 i, val1, val2; > + > + blob = drm_property_create_blob(&dev_priv->drm, > + sizeof(struct drm_color_lut) * lut_size, > + NULL); > + if (IS_ERR(blob)) > + return NULL; > + > + blob_data = blob->data; > + > + I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); > + > + for (i = 0; i < 9; i++) { > + val1 = I915_READ(PREC_PAL_MULTI_SEG_DATA(pipe)); > + val2 = I915_READ(PREC_PAL_MULTI_SEG_DATA(pipe)); > + > + blob_data[i].red = REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_UDW_MASK, val2) << 6 | > + REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_LDW_MASK, val1); > + blob_data[i].green = REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_UDW_MASK, val2) << 6 | > + REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_LDW_MASK, val1); > + blob_data[i].blue = REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_UDW_MASK, val2) << 6 | > + REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_LDW_MASK, val1); > + } > + > + /* > + * FIXME readouts from PAL_PREC_DATA register aren't giving correct values > + * in the case of fine and coarse segments. Restricting readouts only for > + * super fine segment as of now. > + */ > + > + return blob; > +} > + > +static void icl_read_luts(struct intel_crtc_state *crtc_state) > +{ > + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0) > + return; > + > + if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) == > + GAMMA_MODE_MODE_8BIT) Please make this a switch statement. I pushed patches 1-2 to dinq. Thanks. > + crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state); > + else if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) == > + GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED) > + crtc_state->base.gamma_lut = icl_read_lut_multi_segment(crtc_state); > + else > + crtc_state->base.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0)); > +} > + > void intel_color_init(struct intel_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > @@ -1858,6 +1947,7 @@ void intel_color_init(struct intel_crtc *crtc) > > if (INTEL_GEN(dev_priv) >= 11) { > dev_priv->display.load_luts = icl_load_luts; > + dev_priv->display.read_luts = icl_read_luts; > } else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) { > dev_priv->display.load_luts = glk_load_luts; > dev_priv->display.read_luts = glk_read_luts; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 1dc067fc57ab..06b6398b7d62 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -10584,6 +10584,12 @@ enum skl_power_gate { > > #define _PAL_PREC_MULTI_SEG_DATA_A 0x4A40C > #define _PAL_PREC_MULTI_SEG_DATA_B 0x4AC0C > +#define PAL_PREC_MULTI_SEG_RED_LDW_MASK REG_GENMASK(29, 24) > +#define PAL_PREC_MULTI_SEG_RED_UDW_MASK REG_GENMASK(29, 20) > +#define PAL_PREC_MULTI_SEG_GREEN_LDW_MASK REG_GENMASK(19, 14) > +#define PAL_PREC_MULTI_SEG_GREEN_UDW_MASK REG_GENMASK(19, 10) > +#define PAL_PREC_MULTI_SEG_BLUE_LDW_MASK REG_GENMASK(9, 4) > +#define PAL_PREC_MULTI_SEG_BLUE_UDW_MASK REG_GENMASK(9, 0) > > #define PREC_PAL_MULTI_SEG_INDEX(pipe) _MMIO_PIPE(pipe, \ > _PAL_PREC_MULTI_SEG_INDEX_A, \ > -- > 2.23.0
On 09-Oct-19 7:46 PM, Ville Syrjälä wrote: > On Wed, Oct 09, 2019 at 12:25:41PM +0530, Swati Sharma wrote: >> For icl+, have hw read out to create hw blob of gamma >> lut values. icl+ platforms supports multi segmented gamma >> mode by default, add hw lut creation for this mode. >> >> This will be used to validate gamma programming using dsb >> (display state buffer) which is a tgl specific feature. >> >> Major change done-removal of readouts of coarse and fine segments >> because PAL_PREC_DATA register isn't giving propoer values. >> State checker limited only to "fine segment" >> >> v2: -readout code for multisegmented gamma has to come >> up with some intermediate entries that aren't preserved >> in hardware (Jani N) >> -linear interpolation (Ville) >> -moved common code to check gamma_enable to specific funcs, >> since icl doesn't support that >> v3: -use u16 instead of __u16 [Jani N] >> -used single lut [Jani N] >> -improved and more readable for loops [Jani N] >> -read values directly to actual locations and then fill gaps [Jani N] >> -moved cleaning to patch 1 [Jani N] >> -renamed icl_read_lut_multi_seg() to icl_read_lut_multi_segment to >> make it similar to icl_load_luts() >> -renamed icl_compute_interpolated_gamma_blob() to >> icl_compute_interpolated_gamma_lut_values() more sensible, I guess >> v4: -removed interpolated func for creating gamma lut values >> -removed readouts of fine and coarse segments, failure to read PAL_PREC_DATA >> correctly >> v5: -added gamma_enable check inside read_luts() >> >> Signed-off-by: Swati Sharma <swati2.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_color.c | 114 ++++++++++++++++++--- >> drivers/gpu/drm/i915/i915_reg.h | 6 ++ >> 2 files changed, 108 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c >> index fa44eb73d088..614e0ad386ca 100644 >> --- a/drivers/gpu/drm/i915/display/intel_color.c >> +++ b/drivers/gpu/drm/i915/display/intel_color.c >> @@ -1477,6 +1477,25 @@ static int glk_gamma_precision(const struct intel_crtc_state *crtc_state) >> } >> } >> >> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state) >> +{ >> + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0) >> + return 0; >> + >> + switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) { >> + case GAMMA_MODE_MODE_8BIT: >> + return 8; >> + case GAMMA_MODE_MODE_10BIT: >> + return 10; >> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: >> + return 16; >> + default: >> + MISSING_CASE(crtc_state->gamma_mode); >> + return 0; >> + } >> + >> +} >> + >> int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state) >> { >> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); >> @@ -1488,7 +1507,9 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat >> else >> return i9xx_gamma_precision(crtc_state); >> } else { >> - if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) >> + if (INTEL_GEN(dev_priv) >= 11) >> + return icl_gamma_precision(crtc_state); >> + else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) >> return glk_gamma_precision(crtc_state); >> else if (IS_IRONLAKE(dev_priv)) >> return ilk_gamma_precision(crtc_state); >> @@ -1519,6 +1540,20 @@ static bool intel_color_lut_entry_equal(struct drm_color_lut *lut1, >> return true; >> } >> >> +static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1, >> + struct drm_color_lut *lut2, >> + int lut_size, u32 err) >> +{ >> + int i; >> + >> + for (i = 0; i < 9; i++) { >> + if (!err_check(&lut1[i], &lut2[i], err)) >> + return false; >> + } >> + >> + return true; >> +} >> + >> bool intel_color_lut_equal(struct drm_property_blob *blob1, >> struct drm_property_blob *blob2, >> u32 gamma_mode, u32 bit_precision) >> @@ -1537,16 +1572,8 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, >> lut_size2 = drm_color_lut_size(blob2); >> >> /* check sw and hw lut size */ >> - switch (gamma_mode) { >> - case GAMMA_MODE_MODE_8BIT: >> - case GAMMA_MODE_MODE_10BIT: >> - if (lut_size1 != lut_size2) >> - return false; >> - break; >> - default: >> - MISSING_CASE(gamma_mode); >> - return false; >> - } >> + if (lut_size1 != lut_size2) >> + return false; >> >> lut1 = blob1->data; >> lut2 = blob2->data; >> @@ -1554,13 +1581,18 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, >> err = 0xffff >> bit_precision; >> >> /* check sw and hw lut entry to be equal */ >> - switch (gamma_mode) { >> + switch (gamma_mode & GAMMA_MODE_MODE_MASK) { >> case GAMMA_MODE_MODE_8BIT: >> case GAMMA_MODE_MODE_10BIT: >> if (!intel_color_lut_entry_equal(lut1, lut2, >> lut_size2, err)) >> return false; >> break; >> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: >> + if (!intel_color_lut_entry_multi_equal(lut1, lut2, >> + lut_size2, err)) > > I don't think you need a new function for that. Just pass 9 as the size > to intel_color_lut_entry_equal() ? I had made a separate function for multi-segmented gamma since there will be 3 loops for comparing superfine, fine and course segments which wont go with intel_lut_entry_equal() structure. Right now we are limiting to superfine segment only but in future we will add for other segments too (once we get fix from h/w) Func() should look like this. Actually there is no need to passing lut_size only in this function if we continue with this function only. +static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1, + struct drm_color_lut *lut2, + int lut_size, u32 err) +{ + int i; + + for (i = 0; i < 9; i++) { + if (!err_check(&lut1[i], &lut2[i], err)) + return false; + } + + for (i = 1; i < 257; i++) { + if (!err_check(&lut1[i * 8], &lut2[i * 8], err)) + return false; + } + + for (i = 0; i < 256; i++) { + if (!err_check(&lut1[i * 8 * 128], &lut2[i * 8 * 128], err)) + return false; + } + + return true; +} + Please suggest. > > Hmm, should probably rename that to just intel_color_lut_equal() since > it checks the entire LUT (or at least the specified subset) and not > just a single entry... This will be fine for this segment but for other two segments it won't work. Right? > >> + return false; >> + break; >> default: >> MISSING_CASE(gamma_mode); >> return false; >> @@ -1815,6 +1847,63 @@ static void glk_read_luts(struct intel_crtc_state *crtc_state) >> crtc_state->base.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0)); >> } >> >> +static struct drm_property_blob * >> +icl_read_lut_multi_segment(const struct intel_crtc_state *crtc_state) >> +{ >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); >> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> + int lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size; >> + enum pipe pipe = crtc->pipe; >> + struct drm_property_blob *blob; >> + struct drm_color_lut *blob_data; >> + u32 i, val1, val2; >> + >> + blob = drm_property_create_blob(&dev_priv->drm, >> + sizeof(struct drm_color_lut) * lut_size, >> + NULL); >> + if (IS_ERR(blob)) >> + return NULL; >> + >> + blob_data = blob->data; >> + >> + I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >> + >> + for (i = 0; i < 9; i++) { >> + val1 = I915_READ(PREC_PAL_MULTI_SEG_DATA(pipe)); >> + val2 = I915_READ(PREC_PAL_MULTI_SEG_DATA(pipe)); >> + >> + blob_data[i].red = REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_UDW_MASK, val2) << 6 | >> + REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_LDW_MASK, val1); >> + blob_data[i].green = REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_UDW_MASK, val2) << 6 | >> + REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_LDW_MASK, val1); >> + blob_data[i].blue = REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_UDW_MASK, val2) << 6 | >> + REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_LDW_MASK, val1); >> + } >> + >> + /* >> + * FIXME readouts from PAL_PREC_DATA register aren't giving correct values >> + * in the case of fine and coarse segments. Restricting readouts only for >> + * super fine segment as of now. >> + */ >> + >> + return blob; >> +} >> + >> +static void icl_read_luts(struct intel_crtc_state *crtc_state) >> +{ >> + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0) >> + return; >> + >> + if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) == >> + GAMMA_MODE_MODE_8BIT) > > Please make this a switch statement. > > I pushed patches 1-2 to dinq. Thanks. > >> + crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state); >> + else if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) == >> + GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED) >> + crtc_state->base.gamma_lut = icl_read_lut_multi_segment(crtc_state); >> + else >> + crtc_state->base.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0)); >> +} >> + >> void intel_color_init(struct intel_crtc *crtc) >> { >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> @@ -1858,6 +1947,7 @@ void intel_color_init(struct intel_crtc *crtc) >> >> if (INTEL_GEN(dev_priv) >= 11) { >> dev_priv->display.load_luts = icl_load_luts; >> + dev_priv->display.read_luts = icl_read_luts; >> } else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) { >> dev_priv->display.load_luts = glk_load_luts; >> dev_priv->display.read_luts = glk_read_luts; >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 1dc067fc57ab..06b6398b7d62 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -10584,6 +10584,12 @@ enum skl_power_gate { >> >> #define _PAL_PREC_MULTI_SEG_DATA_A 0x4A40C >> #define _PAL_PREC_MULTI_SEG_DATA_B 0x4AC0C >> +#define PAL_PREC_MULTI_SEG_RED_LDW_MASK REG_GENMASK(29, 24) >> +#define PAL_PREC_MULTI_SEG_RED_UDW_MASK REG_GENMASK(29, 20) >> +#define PAL_PREC_MULTI_SEG_GREEN_LDW_MASK REG_GENMASK(19, 14) >> +#define PAL_PREC_MULTI_SEG_GREEN_UDW_MASK REG_GENMASK(19, 10) >> +#define PAL_PREC_MULTI_SEG_BLUE_LDW_MASK REG_GENMASK(9, 4) >> +#define PAL_PREC_MULTI_SEG_BLUE_UDW_MASK REG_GENMASK(9, 0) >> >> #define PREC_PAL_MULTI_SEG_INDEX(pipe) _MMIO_PIPE(pipe, \ >> _PAL_PREC_MULTI_SEG_INDEX_A, \ >> -- >> 2.23.0 >
On Wed, 09 Oct 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Oct 09, 2019 at 12:25:41PM +0530, Swati Sharma wrote: >> For icl+, have hw read out to create hw blob of gamma >> lut values. icl+ platforms supports multi segmented gamma >> mode by default, add hw lut creation for this mode. >> >> This will be used to validate gamma programming using dsb >> (display state buffer) which is a tgl specific feature. >> >> Major change done-removal of readouts of coarse and fine segments >> because PAL_PREC_DATA register isn't giving propoer values. >> State checker limited only to "fine segment" >> >> v2: -readout code for multisegmented gamma has to come >> up with some intermediate entries that aren't preserved >> in hardware (Jani N) >> -linear interpolation (Ville) >> -moved common code to check gamma_enable to specific funcs, >> since icl doesn't support that >> v3: -use u16 instead of __u16 [Jani N] >> -used single lut [Jani N] >> -improved and more readable for loops [Jani N] >> -read values directly to actual locations and then fill gaps [Jani N] >> -moved cleaning to patch 1 [Jani N] >> -renamed icl_read_lut_multi_seg() to icl_read_lut_multi_segment to >> make it similar to icl_load_luts() >> -renamed icl_compute_interpolated_gamma_blob() to >> icl_compute_interpolated_gamma_lut_values() more sensible, I guess >> v4: -removed interpolated func for creating gamma lut values >> -removed readouts of fine and coarse segments, failure to read PAL_PREC_DATA >> correctly >> v5: -added gamma_enable check inside read_luts() >> >> Signed-off-by: Swati Sharma <swati2.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_color.c | 114 ++++++++++++++++++--- >> drivers/gpu/drm/i915/i915_reg.h | 6 ++ >> 2 files changed, 108 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c >> index fa44eb73d088..614e0ad386ca 100644 >> --- a/drivers/gpu/drm/i915/display/intel_color.c >> +++ b/drivers/gpu/drm/i915/display/intel_color.c >> @@ -1477,6 +1477,25 @@ static int glk_gamma_precision(const struct intel_crtc_state *crtc_state) >> } >> } >> >> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state) >> +{ >> + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0) >> + return 0; >> + >> + switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) { >> + case GAMMA_MODE_MODE_8BIT: >> + return 8; >> + case GAMMA_MODE_MODE_10BIT: >> + return 10; >> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: >> + return 16; >> + default: >> + MISSING_CASE(crtc_state->gamma_mode); >> + return 0; >> + } >> + >> +} >> + >> int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state) >> { >> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); >> @@ -1488,7 +1507,9 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat >> else >> return i9xx_gamma_precision(crtc_state); >> } else { >> - if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) >> + if (INTEL_GEN(dev_priv) >= 11) >> + return icl_gamma_precision(crtc_state); >> + else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) >> return glk_gamma_precision(crtc_state); >> else if (IS_IRONLAKE(dev_priv)) >> return ilk_gamma_precision(crtc_state); >> @@ -1519,6 +1540,20 @@ static bool intel_color_lut_entry_equal(struct drm_color_lut *lut1, >> return true; >> } >> >> +static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1, >> + struct drm_color_lut *lut2, >> + int lut_size, u32 err) >> +{ >> + int i; >> + >> + for (i = 0; i < 9; i++) { >> + if (!err_check(&lut1[i], &lut2[i], err)) >> + return false; >> + } >> + >> + return true; >> +} >> + >> bool intel_color_lut_equal(struct drm_property_blob *blob1, >> struct drm_property_blob *blob2, >> u32 gamma_mode, u32 bit_precision) >> @@ -1537,16 +1572,8 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, >> lut_size2 = drm_color_lut_size(blob2); >> >> /* check sw and hw lut size */ >> - switch (gamma_mode) { >> - case GAMMA_MODE_MODE_8BIT: >> - case GAMMA_MODE_MODE_10BIT: >> - if (lut_size1 != lut_size2) >> - return false; >> - break; >> - default: >> - MISSING_CASE(gamma_mode); >> - return false; >> - } >> + if (lut_size1 != lut_size2) >> + return false; >> >> lut1 = blob1->data; >> lut2 = blob2->data; >> @@ -1554,13 +1581,18 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, >> err = 0xffff >> bit_precision; >> >> /* check sw and hw lut entry to be equal */ >> - switch (gamma_mode) { >> + switch (gamma_mode & GAMMA_MODE_MODE_MASK) { >> case GAMMA_MODE_MODE_8BIT: >> case GAMMA_MODE_MODE_10BIT: >> if (!intel_color_lut_entry_equal(lut1, lut2, >> lut_size2, err)) >> return false; >> break; >> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: >> + if (!intel_color_lut_entry_multi_equal(lut1, lut2, >> + lut_size2, err)) > > I don't think you need a new function for that. Just pass 9 as the size > to intel_color_lut_entry_equal() ? I think the idea is to prepare for if/when we can actually check the multi-segmented case where we have items that need to be ignored with different spacing. BR, Jani. > > Hmm, should probably rename that to just intel_color_lut_equal() since > it checks the entire LUT (or at least the specified subset) and not > just a single entry... > >> + return false; >> + break; >> default: >> MISSING_CASE(gamma_mode); >> return false; >> @@ -1815,6 +1847,63 @@ static void glk_read_luts(struct intel_crtc_state *crtc_state) >> crtc_state->base.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0)); >> } >> >> +static struct drm_property_blob * >> +icl_read_lut_multi_segment(const struct intel_crtc_state *crtc_state) >> +{ >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); >> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> + int lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size; >> + enum pipe pipe = crtc->pipe; >> + struct drm_property_blob *blob; >> + struct drm_color_lut *blob_data; >> + u32 i, val1, val2; >> + >> + blob = drm_property_create_blob(&dev_priv->drm, >> + sizeof(struct drm_color_lut) * lut_size, >> + NULL); >> + if (IS_ERR(blob)) >> + return NULL; >> + >> + blob_data = blob->data; >> + >> + I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); >> + >> + for (i = 0; i < 9; i++) { >> + val1 = I915_READ(PREC_PAL_MULTI_SEG_DATA(pipe)); >> + val2 = I915_READ(PREC_PAL_MULTI_SEG_DATA(pipe)); >> + >> + blob_data[i].red = REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_UDW_MASK, val2) << 6 | >> + REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_LDW_MASK, val1); >> + blob_data[i].green = REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_UDW_MASK, val2) << 6 | >> + REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_LDW_MASK, val1); >> + blob_data[i].blue = REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_UDW_MASK, val2) << 6 | >> + REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_LDW_MASK, val1); >> + } >> + >> + /* >> + * FIXME readouts from PAL_PREC_DATA register aren't giving correct values >> + * in the case of fine and coarse segments. Restricting readouts only for >> + * super fine segment as of now. >> + */ >> + >> + return blob; >> +} >> + >> +static void icl_read_luts(struct intel_crtc_state *crtc_state) >> +{ >> + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0) >> + return; >> + >> + if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) == >> + GAMMA_MODE_MODE_8BIT) > > Please make this a switch statement. > > I pushed patches 1-2 to dinq. Thanks. > >> + crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state); >> + else if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) == >> + GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED) >> + crtc_state->base.gamma_lut = icl_read_lut_multi_segment(crtc_state); >> + else >> + crtc_state->base.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0)); >> +} >> + >> void intel_color_init(struct intel_crtc *crtc) >> { >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> @@ -1858,6 +1947,7 @@ void intel_color_init(struct intel_crtc *crtc) >> >> if (INTEL_GEN(dev_priv) >= 11) { >> dev_priv->display.load_luts = icl_load_luts; >> + dev_priv->display.read_luts = icl_read_luts; >> } else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) { >> dev_priv->display.load_luts = glk_load_luts; >> dev_priv->display.read_luts = glk_read_luts; >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 1dc067fc57ab..06b6398b7d62 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -10584,6 +10584,12 @@ enum skl_power_gate { >> >> #define _PAL_PREC_MULTI_SEG_DATA_A 0x4A40C >> #define _PAL_PREC_MULTI_SEG_DATA_B 0x4AC0C >> +#define PAL_PREC_MULTI_SEG_RED_LDW_MASK REG_GENMASK(29, 24) >> +#define PAL_PREC_MULTI_SEG_RED_UDW_MASK REG_GENMASK(29, 20) >> +#define PAL_PREC_MULTI_SEG_GREEN_LDW_MASK REG_GENMASK(19, 14) >> +#define PAL_PREC_MULTI_SEG_GREEN_UDW_MASK REG_GENMASK(19, 10) >> +#define PAL_PREC_MULTI_SEG_BLUE_LDW_MASK REG_GENMASK(9, 4) >> +#define PAL_PREC_MULTI_SEG_BLUE_UDW_MASK REG_GENMASK(9, 0) >> >> #define PREC_PAL_MULTI_SEG_INDEX(pipe) _MMIO_PIPE(pipe, \ >> _PAL_PREC_MULTI_SEG_INDEX_A, \ >> -- >> 2.23.0
On Thu, Oct 10, 2019 at 04:20:04PM +0530, Sharma, Swati2 wrote: > On 09-Oct-19 7:46 PM, Ville Syrjälä wrote: > > On Wed, Oct 09, 2019 at 12:25:41PM +0530, Swati Sharma wrote: > >> For icl+, have hw read out to create hw blob of gamma > >> lut values. icl+ platforms supports multi segmented gamma > >> mode by default, add hw lut creation for this mode. > >> > >> This will be used to validate gamma programming using dsb > >> (display state buffer) which is a tgl specific feature. > >> > >> Major change done-removal of readouts of coarse and fine segments > >> because PAL_PREC_DATA register isn't giving propoer values. > >> State checker limited only to "fine segment" > >> > >> v2: -readout code for multisegmented gamma has to come > >> up with some intermediate entries that aren't preserved > >> in hardware (Jani N) > >> -linear interpolation (Ville) > >> -moved common code to check gamma_enable to specific funcs, > >> since icl doesn't support that > >> v3: -use u16 instead of __u16 [Jani N] > >> -used single lut [Jani N] > >> -improved and more readable for loops [Jani N] > >> -read values directly to actual locations and then fill gaps [Jani N] > >> -moved cleaning to patch 1 [Jani N] > >> -renamed icl_read_lut_multi_seg() to icl_read_lut_multi_segment to > >> make it similar to icl_load_luts() > >> -renamed icl_compute_interpolated_gamma_blob() to > >> icl_compute_interpolated_gamma_lut_values() more sensible, I guess > >> v4: -removed interpolated func for creating gamma lut values > >> -removed readouts of fine and coarse segments, failure to read PAL_PREC_DATA > >> correctly > >> v5: -added gamma_enable check inside read_luts() > >> > >> Signed-off-by: Swati Sharma <swati2.sharma@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_color.c | 114 ++++++++++++++++++--- > >> drivers/gpu/drm/i915/i915_reg.h | 6 ++ > >> 2 files changed, 108 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > >> index fa44eb73d088..614e0ad386ca 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_color.c > >> +++ b/drivers/gpu/drm/i915/display/intel_color.c > >> @@ -1477,6 +1477,25 @@ static int glk_gamma_precision(const struct intel_crtc_state *crtc_state) > >> } > >> } > >> > >> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state) > >> +{ > >> + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0) > >> + return 0; > >> + > >> + switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) { > >> + case GAMMA_MODE_MODE_8BIT: > >> + return 8; > >> + case GAMMA_MODE_MODE_10BIT: > >> + return 10; > >> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: > >> + return 16; > >> + default: > >> + MISSING_CASE(crtc_state->gamma_mode); > >> + return 0; > >> + } > >> + > >> +} > >> + > >> int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state) > >> { > >> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > >> @@ -1488,7 +1507,9 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat > >> else > >> return i9xx_gamma_precision(crtc_state); > >> } else { > >> - if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) > >> + if (INTEL_GEN(dev_priv) >= 11) > >> + return icl_gamma_precision(crtc_state); > >> + else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) > >> return glk_gamma_precision(crtc_state); > >> else if (IS_IRONLAKE(dev_priv)) > >> return ilk_gamma_precision(crtc_state); > >> @@ -1519,6 +1540,20 @@ static bool intel_color_lut_entry_equal(struct drm_color_lut *lut1, > >> return true; > >> } > >> > >> +static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1, > >> + struct drm_color_lut *lut2, > >> + int lut_size, u32 err) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < 9; i++) { > >> + if (!err_check(&lut1[i], &lut2[i], err)) > >> + return false; > >> + } > >> + > >> + return true; > >> +} > >> + > >> bool intel_color_lut_equal(struct drm_property_blob *blob1, > >> struct drm_property_blob *blob2, > >> u32 gamma_mode, u32 bit_precision) > >> @@ -1537,16 +1572,8 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, > >> lut_size2 = drm_color_lut_size(blob2); > >> > >> /* check sw and hw lut size */ > >> - switch (gamma_mode) { > >> - case GAMMA_MODE_MODE_8BIT: > >> - case GAMMA_MODE_MODE_10BIT: > >> - if (lut_size1 != lut_size2) > >> - return false; > >> - break; > >> - default: > >> - MISSING_CASE(gamma_mode); > >> - return false; > >> - } > >> + if (lut_size1 != lut_size2) > >> + return false; > >> > >> lut1 = blob1->data; > >> lut2 = blob2->data; > >> @@ -1554,13 +1581,18 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, > >> err = 0xffff >> bit_precision; > >> > >> /* check sw and hw lut entry to be equal */ > >> - switch (gamma_mode) { > >> + switch (gamma_mode & GAMMA_MODE_MODE_MASK) { > >> case GAMMA_MODE_MODE_8BIT: > >> case GAMMA_MODE_MODE_10BIT: > >> if (!intel_color_lut_entry_equal(lut1, lut2, > >> lut_size2, err)) > >> return false; > >> break; > >> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: > >> + if (!intel_color_lut_entry_multi_equal(lut1, lut2, > >> + lut_size2, err)) > > > > I don't think you need a new function for that. Just pass 9 as the size > > to intel_color_lut_entry_equal() ? > > I had made a separate function for multi-segmented gamma since there > will be 3 loops for comparing superfine, fine and course segments which > wont go with intel_lut_entry_equal() structure. > > Right now we are limiting to superfine segment only but in future we > will add for other segments too (once we get fix from h/w) > > Func() should look like this. Actually there is no need to passing > lut_size only in this function if we continue with this function only. > > +static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1, > + struct drm_color_lut *lut2, > + int lut_size, u32 err) > +{ > + int i; > + > + for (i = 0; i < 9; i++) { > + if (!err_check(&lut1[i], &lut2[i], err)) > + return false; > + } > + > + for (i = 1; i < 257; i++) { > + if (!err_check(&lut1[i * 8], &lut2[i * 8], err)) > + return false; > + } > + > + for (i = 0; i < 256; i++) { > + if (!err_check(&lut1[i * 8 * 128], &lut2[i * 8 * 128], err)) > + return false; > + } > + > + return true; > +} > + > Please suggest. There's not much point in duplicating code until it's proven to be required. Who knows when the hw gets fixed, maybe never. > > > > > Hmm, should probably rename that to just intel_color_lut_equal() since > > it checks the entire LUT (or at least the specified subset) and not > > just a single entry... > > This will be fine for this segment but for other two segments it won't > work. Right? We could generalize it to take start+end+stride. Dunno if that would be particularly beneficial though.
On 15-Oct-19 6:04 PM, Ville Syrjälä wrote: > On Thu, Oct 10, 2019 at 04:20:04PM +0530, Sharma, Swati2 wrote: >> On 09-Oct-19 7:46 PM, Ville Syrjälä wrote: >>> On Wed, Oct 09, 2019 at 12:25:41PM +0530, Swati Sharma wrote: >>>> For icl+, have hw read out to create hw blob of gamma >>>> lut values. icl+ platforms supports multi segmented gamma >>>> mode by default, add hw lut creation for this mode. >>>> >>>> This will be used to validate gamma programming using dsb >>>> (display state buffer) which is a tgl specific feature. >>>> >>>> Major change done-removal of readouts of coarse and fine segments >>>> because PAL_PREC_DATA register isn't giving propoer values. >>>> State checker limited only to "fine segment" >>>> >>>> v2: -readout code for multisegmented gamma has to come >>>> up with some intermediate entries that aren't preserved >>>> in hardware (Jani N) >>>> -linear interpolation (Ville) >>>> -moved common code to check gamma_enable to specific funcs, >>>> since icl doesn't support that >>>> v3: -use u16 instead of __u16 [Jani N] >>>> -used single lut [Jani N] >>>> -improved and more readable for loops [Jani N] >>>> -read values directly to actual locations and then fill gaps [Jani N] >>>> -moved cleaning to patch 1 [Jani N] >>>> -renamed icl_read_lut_multi_seg() to icl_read_lut_multi_segment to >>>> make it similar to icl_load_luts() >>>> -renamed icl_compute_interpolated_gamma_blob() to >>>> icl_compute_interpolated_gamma_lut_values() more sensible, I guess >>>> v4: -removed interpolated func for creating gamma lut values >>>> -removed readouts of fine and coarse segments, failure to read PAL_PREC_DATA >>>> correctly >>>> v5: -added gamma_enable check inside read_luts() >>>> >>>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/display/intel_color.c | 114 ++++++++++++++++++--- >>>> drivers/gpu/drm/i915/i915_reg.h | 6 ++ >>>> 2 files changed, 108 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c >>>> index fa44eb73d088..614e0ad386ca 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_color.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c >>>> @@ -1477,6 +1477,25 @@ static int glk_gamma_precision(const struct intel_crtc_state *crtc_state) >>>> } >>>> } >>>> >>>> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state) >>>> +{ >>>> + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0) >>>> + return 0; >>>> + >>>> + switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) { >>>> + case GAMMA_MODE_MODE_8BIT: >>>> + return 8; >>>> + case GAMMA_MODE_MODE_10BIT: >>>> + return 10; >>>> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: >>>> + return 16; >>>> + default: >>>> + MISSING_CASE(crtc_state->gamma_mode); >>>> + return 0; >>>> + } >>>> + >>>> +} >>>> + >>>> int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state) >>>> { >>>> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); >>>> @@ -1488,7 +1507,9 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat >>>> else >>>> return i9xx_gamma_precision(crtc_state); >>>> } else { >>>> - if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) >>>> + if (INTEL_GEN(dev_priv) >= 11) >>>> + return icl_gamma_precision(crtc_state); >>>> + else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) >>>> return glk_gamma_precision(crtc_state); >>>> else if (IS_IRONLAKE(dev_priv)) >>>> return ilk_gamma_precision(crtc_state); >>>> @@ -1519,6 +1540,20 @@ static bool intel_color_lut_entry_equal(struct drm_color_lut *lut1, >>>> return true; >>>> } >>>> >>>> +static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1, >>>> + struct drm_color_lut *lut2, >>>> + int lut_size, u32 err) >>>> +{ >>>> + int i; >>>> + >>>> + for (i = 0; i < 9; i++) { >>>> + if (!err_check(&lut1[i], &lut2[i], err)) >>>> + return false; >>>> + } >>>> + >>>> + return true; >>>> +} >>>> + >>>> bool intel_color_lut_equal(struct drm_property_blob *blob1, >>>> struct drm_property_blob *blob2, >>>> u32 gamma_mode, u32 bit_precision) >>>> @@ -1537,16 +1572,8 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, >>>> lut_size2 = drm_color_lut_size(blob2); >>>> >>>> /* check sw and hw lut size */ >>>> - switch (gamma_mode) { >>>> - case GAMMA_MODE_MODE_8BIT: >>>> - case GAMMA_MODE_MODE_10BIT: >>>> - if (lut_size1 != lut_size2) >>>> - return false; >>>> - break; >>>> - default: >>>> - MISSING_CASE(gamma_mode); >>>> - return false; >>>> - } >>>> + if (lut_size1 != lut_size2) >>>> + return false; >>>> >>>> lut1 = blob1->data; >>>> lut2 = blob2->data; >>>> @@ -1554,13 +1581,18 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, >>>> err = 0xffff >> bit_precision; >>>> >>>> /* check sw and hw lut entry to be equal */ >>>> - switch (gamma_mode) { >>>> + switch (gamma_mode & GAMMA_MODE_MODE_MASK) { >>>> case GAMMA_MODE_MODE_8BIT: >>>> case GAMMA_MODE_MODE_10BIT: >>>> if (!intel_color_lut_entry_equal(lut1, lut2, >>>> lut_size2, err)) >>>> return false; >>>> break; >>>> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: >>>> + if (!intel_color_lut_entry_multi_equal(lut1, lut2, >>>> + lut_size2, err)) >>> >>> I don't think you need a new function for that. Just pass 9 as the size >>> to intel_color_lut_entry_equal() ? >> >> I had made a separate function for multi-segmented gamma since there >> will be 3 loops for comparing superfine, fine and course segments which >> wont go with intel_lut_entry_equal() structure. >> >> Right now we are limiting to superfine segment only but in future we >> will add for other segments too (once we get fix from h/w) >> >> Func() should look like this. Actually there is no need to passing >> lut_size only in this function if we continue with this function only. >> >> +static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1, >> + struct drm_color_lut *lut2, >> + int lut_size, u32 err) >> +{ >> + int i; >> + >> + for (i = 0; i < 9; i++) { >> + if (!err_check(&lut1[i], &lut2[i], err)) >> + return false; >> + } >> + >> + for (i = 1; i < 257; i++) { >> + if (!err_check(&lut1[i * 8], &lut2[i * 8], err)) >> + return false; >> + } >> + >> + for (i = 0; i < 256; i++) { >> + if (!err_check(&lut1[i * 8 * 128], &lut2[i * 8 * 128], err)) >> + return false; >> + } >> + >> + return true; >> +} >> + >> Please suggest. > > There's not much point in duplicating code until it's proven to > be required. Who knows when the hw gets fixed, maybe never. Now, got your point. Thanks! > >> >>> >>> Hmm, should probably rename that to just intel_color_lut_equal() since >>> it checks the entire LUT (or at least the specified subset) and not >>> just a single entry... Already intel_color_lut_equal() function exits, should i rename that to intel_color_blob_equal() or intel_color_gamma_blob()? and make this func intel_color_lut_entry_equal() to intel_color_lut_equal() as suggested by you? Please comment! >> >> This will be fine for this segment but for other two segments it won't >> work. Right? > > We could generalize it to take start+end+stride. Dunno if that would be > particularly beneficial though. >
On Tue, Oct 15, 2019 at 07:34:00PM +0530, Sharma, Swati2 wrote: > On 15-Oct-19 6:04 PM, Ville Syrjälä wrote: > > On Thu, Oct 10, 2019 at 04:20:04PM +0530, Sharma, Swati2 wrote: > >> On 09-Oct-19 7:46 PM, Ville Syrjälä wrote: > >>> On Wed, Oct 09, 2019 at 12:25:41PM +0530, Swati Sharma wrote: > >>>> For icl+, have hw read out to create hw blob of gamma > >>>> lut values. icl+ platforms supports multi segmented gamma > >>>> mode by default, add hw lut creation for this mode. > >>>> > >>>> This will be used to validate gamma programming using dsb > >>>> (display state buffer) which is a tgl specific feature. > >>>> > >>>> Major change done-removal of readouts of coarse and fine segments > >>>> because PAL_PREC_DATA register isn't giving propoer values. > >>>> State checker limited only to "fine segment" > >>>> > >>>> v2: -readout code for multisegmented gamma has to come > >>>> up with some intermediate entries that aren't preserved > >>>> in hardware (Jani N) > >>>> -linear interpolation (Ville) > >>>> -moved common code to check gamma_enable to specific funcs, > >>>> since icl doesn't support that > >>>> v3: -use u16 instead of __u16 [Jani N] > >>>> -used single lut [Jani N] > >>>> -improved and more readable for loops [Jani N] > >>>> -read values directly to actual locations and then fill gaps [Jani N] > >>>> -moved cleaning to patch 1 [Jani N] > >>>> -renamed icl_read_lut_multi_seg() to icl_read_lut_multi_segment to > >>>> make it similar to icl_load_luts() > >>>> -renamed icl_compute_interpolated_gamma_blob() to > >>>> icl_compute_interpolated_gamma_lut_values() more sensible, I guess > >>>> v4: -removed interpolated func for creating gamma lut values > >>>> -removed readouts of fine and coarse segments, failure to read PAL_PREC_DATA > >>>> correctly > >>>> v5: -added gamma_enable check inside read_luts() > >>>> > >>>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com> > >>>> --- > >>>> drivers/gpu/drm/i915/display/intel_color.c | 114 ++++++++++++++++++--- > >>>> drivers/gpu/drm/i915/i915_reg.h | 6 ++ > >>>> 2 files changed, 108 insertions(+), 12 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > >>>> index fa44eb73d088..614e0ad386ca 100644 > >>>> --- a/drivers/gpu/drm/i915/display/intel_color.c > >>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c > >>>> @@ -1477,6 +1477,25 @@ static int glk_gamma_precision(const struct intel_crtc_state *crtc_state) > >>>> } > >>>> } > >>>> > >>>> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state) > >>>> +{ > >>>> + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0) > >>>> + return 0; > >>>> + > >>>> + switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) { > >>>> + case GAMMA_MODE_MODE_8BIT: > >>>> + return 8; > >>>> + case GAMMA_MODE_MODE_10BIT: > >>>> + return 10; > >>>> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: > >>>> + return 16; > >>>> + default: > >>>> + MISSING_CASE(crtc_state->gamma_mode); > >>>> + return 0; > >>>> + } > >>>> + > >>>> +} > >>>> + > >>>> int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state) > >>>> { > >>>> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > >>>> @@ -1488,7 +1507,9 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat > >>>> else > >>>> return i9xx_gamma_precision(crtc_state); > >>>> } else { > >>>> - if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) > >>>> + if (INTEL_GEN(dev_priv) >= 11) > >>>> + return icl_gamma_precision(crtc_state); > >>>> + else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) > >>>> return glk_gamma_precision(crtc_state); > >>>> else if (IS_IRONLAKE(dev_priv)) > >>>> return ilk_gamma_precision(crtc_state); > >>>> @@ -1519,6 +1540,20 @@ static bool intel_color_lut_entry_equal(struct drm_color_lut *lut1, > >>>> return true; > >>>> } > >>>> > >>>> +static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1, > >>>> + struct drm_color_lut *lut2, > >>>> + int lut_size, u32 err) > >>>> +{ > >>>> + int i; > >>>> + > >>>> + for (i = 0; i < 9; i++) { > >>>> + if (!err_check(&lut1[i], &lut2[i], err)) > >>>> + return false; > >>>> + } > >>>> + > >>>> + return true; > >>>> +} > >>>> + > >>>> bool intel_color_lut_equal(struct drm_property_blob *blob1, > >>>> struct drm_property_blob *blob2, > >>>> u32 gamma_mode, u32 bit_precision) > >>>> @@ -1537,16 +1572,8 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, > >>>> lut_size2 = drm_color_lut_size(blob2); > >>>> > >>>> /* check sw and hw lut size */ > >>>> - switch (gamma_mode) { > >>>> - case GAMMA_MODE_MODE_8BIT: > >>>> - case GAMMA_MODE_MODE_10BIT: > >>>> - if (lut_size1 != lut_size2) > >>>> - return false; > >>>> - break; > >>>> - default: > >>>> - MISSING_CASE(gamma_mode); > >>>> - return false; > >>>> - } > >>>> + if (lut_size1 != lut_size2) > >>>> + return false; > >>>> > >>>> lut1 = blob1->data; > >>>> lut2 = blob2->data; > >>>> @@ -1554,13 +1581,18 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, > >>>> err = 0xffff >> bit_precision; > >>>> > >>>> /* check sw and hw lut entry to be equal */ > >>>> - switch (gamma_mode) { > >>>> + switch (gamma_mode & GAMMA_MODE_MODE_MASK) { > >>>> case GAMMA_MODE_MODE_8BIT: > >>>> case GAMMA_MODE_MODE_10BIT: > >>>> if (!intel_color_lut_entry_equal(lut1, lut2, > >>>> lut_size2, err)) > >>>> return false; > >>>> break; > >>>> + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: > >>>> + if (!intel_color_lut_entry_multi_equal(lut1, lut2, > >>>> + lut_size2, err)) > >>> > >>> I don't think you need a new function for that. Just pass 9 as the size > >>> to intel_color_lut_entry_equal() ? > >> > >> I had made a separate function for multi-segmented gamma since there > >> will be 3 loops for comparing superfine, fine and course segments which > >> wont go with intel_lut_entry_equal() structure. > >> > >> Right now we are limiting to superfine segment only but in future we > >> will add for other segments too (once we get fix from h/w) > >> > >> Func() should look like this. Actually there is no need to passing > >> lut_size only in this function if we continue with this function only. > >> > >> +static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1, > >> + struct drm_color_lut *lut2, > >> + int lut_size, u32 err) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < 9; i++) { > >> + if (!err_check(&lut1[i], &lut2[i], err)) > >> + return false; > >> + } > >> + > >> + for (i = 1; i < 257; i++) { > >> + if (!err_check(&lut1[i * 8], &lut2[i * 8], err)) > >> + return false; > >> + } > >> + > >> + for (i = 0; i < 256; i++) { > >> + if (!err_check(&lut1[i * 8 * 128], &lut2[i * 8 * 128], err)) > >> + return false; > >> + } > >> + > >> + return true; > >> +} > >> + > >> Please suggest. > > > > There's not much point in duplicating code until it's proven to > > be required. Who knows when the hw gets fixed, maybe never. > Now, got your point. Thanks! > > > >> > >>> > >>> Hmm, should probably rename that to just intel_color_lut_equal() since > >>> it checks the entire LUT (or at least the specified subset) and not > >>> just a single entry... > Already intel_color_lut_equal() function exits, should i rename that to > intel_color_blob_equal() or intel_color_gamma_blob()? and make this func > intel_color_lut_entry_equal() to intel_color_lut_equal() as suggested by > you? Hmm. In that case I think just s/entry/entries/ would make sense.
diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index fa44eb73d088..614e0ad386ca 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -1477,6 +1477,25 @@ static int glk_gamma_precision(const struct intel_crtc_state *crtc_state) } } +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state) +{ + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0) + return 0; + + switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) { + case GAMMA_MODE_MODE_8BIT: + return 8; + case GAMMA_MODE_MODE_10BIT: + return 10; + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: + return 16; + default: + MISSING_CASE(crtc_state->gamma_mode); + return 0; + } + +} + int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state) { struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); @@ -1488,7 +1507,9 @@ int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_stat else return i9xx_gamma_precision(crtc_state); } else { - if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) + if (INTEL_GEN(dev_priv) >= 11) + return icl_gamma_precision(crtc_state); + else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) return glk_gamma_precision(crtc_state); else if (IS_IRONLAKE(dev_priv)) return ilk_gamma_precision(crtc_state); @@ -1519,6 +1540,20 @@ static bool intel_color_lut_entry_equal(struct drm_color_lut *lut1, return true; } +static bool intel_color_lut_entry_multi_equal(struct drm_color_lut *lut1, + struct drm_color_lut *lut2, + int lut_size, u32 err) +{ + int i; + + for (i = 0; i < 9; i++) { + if (!err_check(&lut1[i], &lut2[i], err)) + return false; + } + + return true; +} + bool intel_color_lut_equal(struct drm_property_blob *blob1, struct drm_property_blob *blob2, u32 gamma_mode, u32 bit_precision) @@ -1537,16 +1572,8 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, lut_size2 = drm_color_lut_size(blob2); /* check sw and hw lut size */ - switch (gamma_mode) { - case GAMMA_MODE_MODE_8BIT: - case GAMMA_MODE_MODE_10BIT: - if (lut_size1 != lut_size2) - return false; - break; - default: - MISSING_CASE(gamma_mode); - return false; - } + if (lut_size1 != lut_size2) + return false; lut1 = blob1->data; lut2 = blob2->data; @@ -1554,13 +1581,18 @@ bool intel_color_lut_equal(struct drm_property_blob *blob1, err = 0xffff >> bit_precision; /* check sw and hw lut entry to be equal */ - switch (gamma_mode) { + switch (gamma_mode & GAMMA_MODE_MODE_MASK) { case GAMMA_MODE_MODE_8BIT: case GAMMA_MODE_MODE_10BIT: if (!intel_color_lut_entry_equal(lut1, lut2, lut_size2, err)) return false; break; + case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED: + if (!intel_color_lut_entry_multi_equal(lut1, lut2, + lut_size2, err)) + return false; + break; default: MISSING_CASE(gamma_mode); return false; @@ -1815,6 +1847,63 @@ static void glk_read_luts(struct intel_crtc_state *crtc_state) crtc_state->base.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0)); } +static struct drm_property_blob * +icl_read_lut_multi_segment(const struct intel_crtc_state *crtc_state) +{ + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + int lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size; + enum pipe pipe = crtc->pipe; + struct drm_property_blob *blob; + struct drm_color_lut *blob_data; + u32 i, val1, val2; + + blob = drm_property_create_blob(&dev_priv->drm, + sizeof(struct drm_color_lut) * lut_size, + NULL); + if (IS_ERR(blob)) + return NULL; + + blob_data = blob->data; + + I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT); + + for (i = 0; i < 9; i++) { + val1 = I915_READ(PREC_PAL_MULTI_SEG_DATA(pipe)); + val2 = I915_READ(PREC_PAL_MULTI_SEG_DATA(pipe)); + + blob_data[i].red = REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_UDW_MASK, val2) << 6 | + REG_FIELD_GET(PAL_PREC_MULTI_SEG_RED_LDW_MASK, val1); + blob_data[i].green = REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_UDW_MASK, val2) << 6 | + REG_FIELD_GET(PAL_PREC_MULTI_SEG_GREEN_LDW_MASK, val1); + blob_data[i].blue = REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_UDW_MASK, val2) << 6 | + REG_FIELD_GET(PAL_PREC_MULTI_SEG_BLUE_LDW_MASK, val1); + } + + /* + * FIXME readouts from PAL_PREC_DATA register aren't giving correct values + * in the case of fine and coarse segments. Restricting readouts only for + * super fine segment as of now. + */ + + return blob; +} + +static void icl_read_luts(struct intel_crtc_state *crtc_state) +{ + if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0) + return; + + if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) == + GAMMA_MODE_MODE_8BIT) + crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state); + else if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) == + GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED) + crtc_state->base.gamma_lut = icl_read_lut_multi_segment(crtc_state); + else + crtc_state->base.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0)); +} + void intel_color_init(struct intel_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); @@ -1858,6 +1947,7 @@ void intel_color_init(struct intel_crtc *crtc) if (INTEL_GEN(dev_priv) >= 11) { dev_priv->display.load_luts = icl_load_luts; + dev_priv->display.read_luts = icl_read_luts; } else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) { dev_priv->display.load_luts = glk_load_luts; dev_priv->display.read_luts = glk_read_luts; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 1dc067fc57ab..06b6398b7d62 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -10584,6 +10584,12 @@ enum skl_power_gate { #define _PAL_PREC_MULTI_SEG_DATA_A 0x4A40C #define _PAL_PREC_MULTI_SEG_DATA_B 0x4AC0C +#define PAL_PREC_MULTI_SEG_RED_LDW_MASK REG_GENMASK(29, 24) +#define PAL_PREC_MULTI_SEG_RED_UDW_MASK REG_GENMASK(29, 20) +#define PAL_PREC_MULTI_SEG_GREEN_LDW_MASK REG_GENMASK(19, 14) +#define PAL_PREC_MULTI_SEG_GREEN_UDW_MASK REG_GENMASK(19, 10) +#define PAL_PREC_MULTI_SEG_BLUE_LDW_MASK REG_GENMASK(9, 4) +#define PAL_PREC_MULTI_SEG_BLUE_UDW_MASK REG_GENMASK(9, 0) #define PREC_PAL_MULTI_SEG_INDEX(pipe) _MMIO_PIPE(pipe, \ _PAL_PREC_MULTI_SEG_INDEX_A, \
For icl+, have hw read out to create hw blob of gamma lut values. icl+ platforms supports multi segmented gamma mode by default, add hw lut creation for this mode. This will be used to validate gamma programming using dsb (display state buffer) which is a tgl specific feature. Major change done-removal of readouts of coarse and fine segments because PAL_PREC_DATA register isn't giving propoer values. State checker limited only to "fine segment" v2: -readout code for multisegmented gamma has to come up with some intermediate entries that aren't preserved in hardware (Jani N) -linear interpolation (Ville) -moved common code to check gamma_enable to specific funcs, since icl doesn't support that v3: -use u16 instead of __u16 [Jani N] -used single lut [Jani N] -improved and more readable for loops [Jani N] -read values directly to actual locations and then fill gaps [Jani N] -moved cleaning to patch 1 [Jani N] -renamed icl_read_lut_multi_seg() to icl_read_lut_multi_segment to make it similar to icl_load_luts() -renamed icl_compute_interpolated_gamma_blob() to icl_compute_interpolated_gamma_lut_values() more sensible, I guess v4: -removed interpolated func for creating gamma lut values -removed readouts of fine and coarse segments, failure to read PAL_PREC_DATA correctly v5: -added gamma_enable check inside read_luts() Signed-off-by: Swati Sharma <swati2.sharma@intel.com> --- drivers/gpu/drm/i915/display/intel_color.c | 114 ++++++++++++++++++--- drivers/gpu/drm/i915/i915_reg.h | 6 ++ 2 files changed, 108 insertions(+), 12 deletions(-)