| Message ID | 1553754828-14696-1-git-send-email-swati2.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: adding state checker for gamma lut values | expand |
On Thu, 28 Mar 2019, Swati Sharma <swati2.sharma@intel.com> wrote: > Added state checker to validate gamma_lut values. This > reads hardware state, and compares the originally requested > state to the state read from hardware. > > v1: -Implementation done for legacy platforms (removed all the placeholders) (Jani) > -Added inverse function of drm_color_lut_extract to convert hardware > read values back to user values (code written by Jani) > -Renamed get_config() to color_config() (Jani) > -Placed all platform specific shifts and masks in i915_reg.h (Jani) > -Renamed i9xx_get_config to i9xx_color_config and all related > functions (Jani) > -Removed debug logs from compare function (Jani) > -Renamed intel_compare_blob to intel_compare_lut and added platform specific > bit precision of the readout into the function (Jani) > -Renamed macro PIPE_CONF_CHECK_BLOB to PIPE_CONF_CHECK_COLOR_LUT (Jani) > -Added check if blobs can be NULL (Jani) > -Added function in intel_color.c that returns the bit precision (Jani), > didn't add in device info since its gonna die soon (Ville) > > TODO: > -Add a separate function to log errors at the higher level Should be a separate follow-up patch. > -Haven't moved intel_compare_lut() from intel_display.c to intel_color.c > Since all the comparison functions are placed in intel_display, isn't > it the right place (or) we want to move to consolidate color related functions > together? Opinion? Please correct me if I am wrong. Consolidate color to intel_color.c, as all the info about the blob and its use is there. > -Optimizations and refractoring > > Signed-off-by: Swati Sharma <swati2.sharma@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 12 +++ > drivers/gpu/drm/i915/intel_color.c | 186 +++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_display.c | 48 +++++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 + > 5 files changed, 243 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c4ffe19..b422ea6 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -334,6 +334,7 @@ struct drm_i915_display_funcs { > * involved with the same commit. > */ > void (*load_luts)(const struct intel_crtc_state *crtc_state); > + void (*color_config)(struct intel_crtc_state *crtc_state); Please call this *get* config. Same for the platform specific functions. It doesn't configure, it reads the configuration. > }; > > #define CSR_VERSION(major, minor) ((major) << 16 | (minor)) > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index c0cd7a8..2813033 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7156,6 +7156,10 @@ enum { > #define _LGC_PALETTE_B 0x4a800 > #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) + (i) * 4) > No blank line here. Ditto for the other groups. > +#define LGC_PALETTE_RED_MASK (0xFF << 16) > +#define LGC_PALETTE_GREEN_MASK (0xFF << 8) > +#define LGC_PALETTE_BLUE_MASK (0xFF << 0) Please indent according to the comment at the top of the file. Please define these using the new REG_GENMASK() and use the REG_FIELD_PREP() and REG_FIELD_GET() macros in code. Ditto for the other groups. > + > #define _GAMMA_MODE_A 0x4a480 > #define _GAMMA_MODE_B 0x4ac80 > #define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B) > @@ -10102,6 +10106,10 @@ enum skl_power_gate { > #define PRE_CSC_GAMC_INDEX(pipe) _MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B) > #define PRE_CSC_GAMC_DATA(pipe) _MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B) > > +#define PREC_PAL_DATA_RED_MASK (0x3FF << 20) > +#define PREC_PAL_DATA_GREEN_MASK (0x3FF << 10) > +#define PREC_PAL_DATA_BLUE_MASK (0x3FF << 0) > + > /* pipe CSC & degamma/gamma LUTs on CHV */ > #define _CGM_PIPE_A_CSC_COEFF01 (VLV_DISPLAY_BASE + 0x67900) > #define _CGM_PIPE_A_CSC_COEFF23 (VLV_DISPLAY_BASE + 0x67904) > @@ -10133,6 +10141,10 @@ enum skl_power_gate { > #define CGM_PIPE_GAMMA(pipe, i, w) _MMIO(_PIPE(pipe, _CGM_PIPE_A_GAMMA, _CGM_PIPE_B_GAMMA) + (i) * 8 + (w) * 4) > #define CGM_PIPE_MODE(pipe) _MMIO_PIPE(pipe, _CGM_PIPE_A_MODE, _CGM_PIPE_B_MODE) > > +#define CGM_PIPE_GAMMA_RED_MASK (0x3FF << 0) > +#define CGM_PIPE_GAMMA_GREEN_MASK (0x3FF << 16) > +#define CGM_PIPE_GAMMA_BLUE_MASK (0x3FF << 0) > + > /* MIPI DSI registers */ > > #define _MIPI_PORT(port, a, c) (((port) == PORT_A) ? a : c) /* ports A and C only */ > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > index da7a07d..bd4f1b1 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -679,6 +679,172 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state) > dev_priv->display.load_luts(crtc_state); > } > > +u32 intel_color_bit_precision(struct drm_i915_private *dev_priv) > +{ > + if (INTEL_GEN(dev_priv) >= 9) > + return 10; > + else > + return 8; > +} Can be made static if the comparison function is moved here. > + > +/* convert hw value with given bit_precision to lut property val */ > +static u32 intel_color_lut_pack(u32 val, u32 bit_precision) > +{ > + u32 max = 0xFFFF >> (16 - bit_precision); > + > + val = clamp_val(val, 0, max); > + > + if (bit_precision < 16) > + val <<= 16 - bit_precision; > + > + return val; > +} > + > +static void i9xx_internal_gamma_config(struct intel_crtc_state *crtc_state) "get" missing in name, same throughout. > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > + struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + u32 i, tmp; > + enum pipe pipe = crtc->pipe; > + struct drm_property_blob *blob = NULL; > + struct drm_color_lut *blob_data; > + > + blob = drm_property_create_blob(dev, > + sizeof(struct drm_color_lut) * 256, > + NULL); > + if (IS_ERR(blob)) > + return; > + > + blob_data = blob->data; > + > + for (i = 0; i < 256; i++) { > + if (HAS_GMCH(dev_priv)) > + tmp = I915_READ(PALETTE(pipe, i)); > + else > + tmp = I915_READ(LGC_PALETTE(pipe, i)); > + blob_data[i].red = intel_color_lut_pack((tmp & LGC_PALETTE_RED_MASK) >> 16, 8); > + blob_data[i].green = intel_color_lut_pack((tmp & LGC_PALETTE_GREEN_MASK) >> 8, 8); > + blob_data[i].blue = intel_color_lut_pack((tmp & LGC_PALETTE_BLUE_MASK), 8); Please use REG_FIELD_GET(). Same throughout. > + } > + > + crtc_state->base.gamma_lut = blob; > +} > + > +static void i9xx_color_config(struct intel_crtc_state *crtc_state) > +{ > + i9xx_internal_gamma_config(crtc_state); > +} > + > +static void cherryview_gamma_config(struct intel_crtc_state *crtc_state) > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > + struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + u32 i, tmp, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size; > + enum pipe pipe = crtc->pipe; > + struct drm_property_blob *blob = NULL; > + struct drm_color_lut *blob_data; > + > + blob = drm_property_create_blob(dev, > + sizeof(struct drm_color_lut) * 256, > + NULL); > + if (IS_ERR(blob)) > + return; > + > + blob_data = blob->data; > + > + for (i = 0; i < lut_size; i++) { > + tmp = I915_READ(CGM_PIPE_GAMMA(pipe, i, 0)); > + blob_data[i].green = intel_color_lut_pack((tmp & CGM_PIPE_GAMMA_GREEN_MASK) >> 16, 10); > + blob_data[i].blue = intel_color_lut_pack((tmp & CGM_PIPE_GAMMA_BLUE_MASK), 10); > + tmp = I915_READ(CGM_PIPE_GAMMA(pipe, i, 1)); > + blob_data[i].red = intel_color_lut_pack((tmp & CGM_PIPE_GAMMA_RED_MASK), 10); > + } > + > + crtc_state->base.gamma_lut = blob; > +} > + > +static void bdw_gamma_config(struct intel_crtc_state *crtc_state, u32 offset) > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > + struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + u32 i, tmp, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size; > + enum pipe pipe = crtc->pipe; > + struct drm_property_blob *blob = NULL; > + struct drm_color_lut *blob_data; > + > + WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK); > + > + I915_WRITE(PREC_PAL_INDEX(pipe), > + (offset ? PAL_PREC_SPLIT_MODE : 0) | > + PAL_PREC_AUTO_INCREMENT | > + offset); > + > + blob = drm_property_create_blob(dev, > + sizeof(struct drm_color_lut) * lut_size, > + NULL); > + if (IS_ERR(blob)) > + return; > + > + blob_data = blob->data; > + > + for (i = 0; i < lut_size; i++) { > + tmp = I915_READ(PREC_PAL_DATA(pipe)); > + blob_data[i].red = intel_color_lut_pack((tmp & PREC_PAL_DATA_RED_MASK) >> 20, 10); > + blob_data[i].green = intel_color_lut_pack((tmp & PREC_PAL_DATA_GREEN_MASK) >> 10, 10); > + blob_data[i].blue = intel_color_lut_pack((tmp & PREC_PAL_DATA_BLUE_MASK), 10); > + } > + > + I915_WRITE(PREC_PAL_INDEX(pipe), 0); > + > + crtc_state->base.gamma_lut = blob; > +} > + > +static void cherryview_color_config(struct intel_crtc_state *crtc_state) > +{ > + if (crtc_state_is_legacy_gamma(crtc_state)) > + i9xx_color_config(crtc_state); > + else > + cherryview_gamma_config(crtc_state); > +} > + > +static void broadwell_color_config(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); > + > + if (crtc_state_is_legacy_gamma(crtc_state)) > + i9xx_color_config(crtc_state); > + else > + bdw_gamma_config(crtc_state, > + INTEL_INFO(dev_priv)->color.degamma_lut_size); > +} > + > +static void glk_color_config(struct intel_crtc_state *crtc_state) > +{ > + if (crtc_state_is_legacy_gamma(crtc_state)) > + i9xx_color_config(crtc_state); > + else > + bdw_gamma_config(crtc_state, 0); > +} > + > +static void icl_color_config(struct intel_crtc_state *crtc_state) > +{ > + if (crtc_state_is_legacy_gamma(crtc_state)) > + i9xx_color_config(crtc_state); > + else > + bdw_gamma_config(crtc_state, 0); > +} > + > +void intel_color_config(struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > + > + dev_priv->display.color_config(crtc_state); > +} > + > void intel_color_commit(const struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > @@ -834,21 +1000,29 @@ void intel_color_init(struct intel_crtc *crtc) > drm_mode_crtc_set_gamma_size(&crtc->base, 256); > > if (HAS_GMCH(dev_priv)) { > - if (IS_CHERRYVIEW(dev_priv)) > + if (IS_CHERRYVIEW(dev_priv)) { > dev_priv->display.load_luts = cherryview_load_luts; > - else > + dev_priv->display.color_config = cherryview_color_config; > + } else { > dev_priv->display.load_luts = i9xx_load_luts; > + dev_priv->display.color_config = i9xx_color_config; > + } > > dev_priv->display.color_commit = i9xx_color_commit; > } else { > - if (IS_ICELAKE(dev_priv)) > + if (IS_ICELAKE(dev_priv)) { > dev_priv->display.load_luts = icl_load_luts; > - else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) > + dev_priv->display.color_config = icl_color_config; > + } else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) { > dev_priv->display.load_luts = glk_load_luts; > - else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) > + dev_priv->display.color_config = glk_color_config; > + } else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) { > dev_priv->display.load_luts = broadwell_load_luts; > - else > + dev_priv->display.color_config = broadwell_color_config; > + } else { > dev_priv->display.load_luts = i9xx_load_luts; > + dev_priv->display.color_config = i9xx_color_config; > + } > > if (INTEL_GEN(dev_priv) >= 9) > dev_priv->display.color_commit = skl_color_commit; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 963b4bd..31bb652 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8212,6 +8212,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, > > i9xx_get_pipe_color_config(pipe_config); > > + intel_color_config(pipe_config); > + > if (INTEL_GEN(dev_priv) < 4) > pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE; > > @@ -9284,6 +9286,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc, > > i9xx_get_pipe_color_config(pipe_config); > > + intel_color_config(pipe_config); > + > if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) { > struct intel_shared_dpll *pll; > enum intel_dpll_id pll_id; > @@ -9932,6 +9936,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > i9xx_get_pipe_color_config(pipe_config); > } > > + intel_color_config(pipe_config); > + > power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe); > if (intel_display_power_get_if_enabled(dev_priv, power_domain)) { > WARN_ON(power_domain_mask & BIT_ULL(power_domain)); > @@ -11990,6 +11996,36 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv) > return false; > } > > +static bool intel_compare_color_lut(struct drm_property_blob *blob1, > + struct drm_property_blob *blob2, > + u32 bit_precision) > +{ > + struct drm_color_lut *sw_lut = blob1->data; > + struct drm_color_lut *hw_lut = blob2->data; > + u32 err = 0xFFFF >> bit_precision; > + int sw_lut_size, hw_lut_size; > + int i; > + > + sw_lut_size = drm_color_lut_size(blob1); > + hw_lut_size = drm_color_lut_size(blob2); > + > + if (IS_ERR(blob1) || IS_ERR(blob2)) > + return false; Mmmh, if this condition is true, we've oopsed on drm_color_lut_size() already. > + > + if (sw_lut_size != hw_lut_size) > + return false; > + > + for (i = 0; i < sw_lut_size; i++) { > + if (((abs((long)hw_lut[i].red - sw_lut[i].red)) >= err) || > + ((abs((long)hw_lut[i].blue - sw_lut[i].blue)) >= err) || > + ((abs((long)hw_lut[i].green - sw_lut[i].green)) >= err)) { I think using an inline function for the comparison with err slack would be nice. > + return false; > + } > + } > + > + return true; > +} I think separate the get config part from the state checker part to another patch. I think move this function to intel_color.c, as intel_display.c is already overcrowded. Pass dev_priv to it so it can handle bit precision internally instead, and you can make intel_color_bit_precision() static. (Or just inline that in the function.) > + > static bool > intel_pipe_config_compare(struct drm_i915_private *dev_priv, > struct intel_crtc_state *current_config, > @@ -11997,6 +12033,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv) > bool adjust) > { > bool ret = true; > + u32 bit_precision; > bool fixup_inherited = adjust && > (current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) && > !(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED); > @@ -12148,6 +12185,14 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv) > } \ > } while (0) > > +#define PIPE_CONF_CHECK_COLOR_LUT(name, bit_precision) do { \ > + if (!intel_compare_color_lut(current_config->name, pipe_config->name, bit_precision)) { \ > + pipe_config_err(adjust, __stringify(name), \ > + "hw_state doesn't match sw_state\n"); \ > + ret = false; \ > + } \ > +} while (0) > + > #define PIPE_CONF_QUIRK(quirk) \ > ((current_config->quirks | pipe_config->quirks) & (quirk)) > > @@ -12287,6 +12332,9 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv) > PIPE_CONF_CHECK_INFOFRAME(spd); > PIPE_CONF_CHECK_INFOFRAME(hdmi); > > + bit_precision = intel_color_bit_precision(dev_priv); > + PIPE_CONF_CHECK_COLOR_LUT(base.gamma_lut, bit_precision); > + > #undef PIPE_CONF_CHECK_X > #undef PIPE_CONF_CHECK_I > #undef PIPE_CONF_CHECK_BOOL > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 40ebc94..6a89d2d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -2512,6 +2512,8 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > int intel_color_check(struct intel_crtc_state *crtc_state); > void intel_color_commit(const struct intel_crtc_state *crtc_state); > void intel_color_load_luts(const struct intel_crtc_state *crtc_state); > +void intel_color_config(struct intel_crtc_state *crtc_state); > +u32 intel_color_bit_precision(struct drm_i915_private *dev_priv); > > /* intel_lspcon.c */ > bool lspcon_init(struct intel_digital_port *intel_dig_port);
On Thu, Mar 28, 2019 at 12:03:48PM +0530, Swati Sharma wrote: > Added state checker to validate gamma_lut values. This > reads hardware state, and compares the originally requested > state to the state read from hardware. > > v1: -Implementation done for legacy platforms (removed all the placeholders) (Jani) > -Added inverse function of drm_color_lut_extract to convert hardware > read values back to user values (code written by Jani) > -Renamed get_config() to color_config() (Jani) > -Placed all platform specific shifts and masks in i915_reg.h (Jani) > -Renamed i9xx_get_config to i9xx_color_config and all related > functions (Jani) > -Removed debug logs from compare function (Jani) > -Renamed intel_compare_blob to intel_compare_lut and added platform specific > bit precision of the readout into the function (Jani) > -Renamed macro PIPE_CONF_CHECK_BLOB to PIPE_CONF_CHECK_COLOR_LUT (Jani) > -Added check if blobs can be NULL (Jani) > -Added function in intel_color.c that returns the bit precision (Jani), > didn't add in device info since its gonna die soon (Ville) > > TODO: > -Add a separate function to log errors at the higher level > -Haven't moved intel_compare_lut() from intel_display.c to intel_color.c > Since all the comparison functions are placed in intel_display, isn't > it the right place (or) we want to move to consolidate color related functions > together? Opinion? Please correct me if I am wrong. > -Optimizations and refractoring > > Signed-off-by: Swati Sharma <swati2.sharma@intel.com> I agree with Jani's feedback and have a couple other comments inline below. Also, since I don't see it on the TODO list here, do you intend to also readout and compare degamma and CTM eventually? > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 12 +++ > drivers/gpu/drm/i915/intel_color.c | 186 +++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_display.c | 48 +++++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 + > 5 files changed, 243 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c4ffe19..b422ea6 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -334,6 +334,7 @@ struct drm_i915_display_funcs { > * involved with the same commit. > */ > void (*load_luts)(const struct intel_crtc_state *crtc_state); > + void (*color_config)(struct intel_crtc_state *crtc_state); > }; > > #define CSR_VERSION(major, minor) ((major) << 16 | (minor)) > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index c0cd7a8..2813033 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7156,6 +7156,10 @@ enum { > #define _LGC_PALETTE_B 0x4a800 > #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) + (i) * 4) > > +#define LGC_PALETTE_RED_MASK (0xFF << 16) > +#define LGC_PALETTE_GREEN_MASK (0xFF << 8) > +#define LGC_PALETTE_BLUE_MASK (0xFF << 0) > + > #define _GAMMA_MODE_A 0x4a480 > #define _GAMMA_MODE_B 0x4ac80 > #define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B) > @@ -10102,6 +10106,10 @@ enum skl_power_gate { > #define PRE_CSC_GAMC_INDEX(pipe) _MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B) > #define PRE_CSC_GAMC_DATA(pipe) _MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B) > > +#define PREC_PAL_DATA_RED_MASK (0x3FF << 20) > +#define PREC_PAL_DATA_GREEN_MASK (0x3FF << 10) > +#define PREC_PAL_DATA_BLUE_MASK (0x3FF << 0) > + > /* pipe CSC & degamma/gamma LUTs on CHV */ > #define _CGM_PIPE_A_CSC_COEFF01 (VLV_DISPLAY_BASE + 0x67900) > #define _CGM_PIPE_A_CSC_COEFF23 (VLV_DISPLAY_BASE + 0x67904) > @@ -10133,6 +10141,10 @@ enum skl_power_gate { > #define CGM_PIPE_GAMMA(pipe, i, w) _MMIO(_PIPE(pipe, _CGM_PIPE_A_GAMMA, _CGM_PIPE_B_GAMMA) + (i) * 8 + (w) * 4) > #define CGM_PIPE_MODE(pipe) _MMIO_PIPE(pipe, _CGM_PIPE_A_MODE, _CGM_PIPE_B_MODE) > > +#define CGM_PIPE_GAMMA_RED_MASK (0x3FF << 0) > +#define CGM_PIPE_GAMMA_GREEN_MASK (0x3FF << 16) > +#define CGM_PIPE_GAMMA_BLUE_MASK (0x3FF << 0) > + > /* MIPI DSI registers */ > > #define _MIPI_PORT(port, a, c) (((port) == PORT_A) ? a : c) /* ports A and C only */ > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > index da7a07d..bd4f1b1 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -679,6 +679,172 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state) > dev_priv->display.load_luts(crtc_state); > } > > +u32 intel_color_bit_precision(struct drm_i915_private *dev_priv) > +{ > + if (INTEL_GEN(dev_priv) >= 9) > + return 10; > + else > + return 8; > +} Doesn't bdw (gen8) also use a 10 bit palette (PREC_PAL_DATA)? I think we probably want to figure out the number of bits to compare based on gamma_mode rather than the platform we're running on. E.g., a platform with 10-bit precision palettes might still be using legacy tables with 8 bits of precision in some cases. And some platforms have even more potential gamma modes that we might enable in the future too. As Jani noted, it might be easiest to just inline this logic in the comparison function where it gets used rather than adding this as a separate function. > + > +/* convert hw value with given bit_precision to lut property val */ > +static u32 intel_color_lut_pack(u32 val, u32 bit_precision) > +{ > + u32 max = 0xFFFF >> (16 - bit_precision); > + > + val = clamp_val(val, 0, max); > + > + if (bit_precision < 16) > + val <<= 16 - bit_precision; > + > + return val; > +} > + > +static void i9xx_internal_gamma_config(struct intel_crtc_state *crtc_state) What does the "internal" in this name refer to? I think just something like i9xx_get_gamma_config() would be sufficient Actually the term "gamma_config" on these functions makes me think we're going to be reading out the gamma mode register as well, although that's actually done in foo_get_pipe_config(). Maybe just calling this something like "i9xx_get_gamma_lut()" or "i9xx_readout_gamma_lut()" would be more clear? > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > + struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + u32 i, tmp; > + enum pipe pipe = crtc->pipe; > + struct drm_property_blob *blob = NULL; > + struct drm_color_lut *blob_data; > + > + blob = drm_property_create_blob(dev, > + sizeof(struct drm_color_lut) * 256, > + NULL); > + if (IS_ERR(blob)) > + return; > + > + blob_data = blob->data; > + > + for (i = 0; i < 256; i++) { > + if (HAS_GMCH(dev_priv)) > + tmp = I915_READ(PALETTE(pipe, i)); > + else > + tmp = I915_READ(LGC_PALETTE(pipe, i)); > + blob_data[i].red = intel_color_lut_pack((tmp & LGC_PALETTE_RED_MASK) >> 16, 8); > + blob_data[i].green = intel_color_lut_pack((tmp & LGC_PALETTE_GREEN_MASK) >> 8, 8); > + blob_data[i].blue = intel_color_lut_pack((tmp & LGC_PALETTE_BLUE_MASK), 8); > + } > + > + crtc_state->base.gamma_lut = blob; > +} > + > +static void i9xx_color_config(struct intel_crtc_state *crtc_state) > +{ > + i9xx_internal_gamma_config(crtc_state); > +} > + > +static void cherryview_gamma_config(struct intel_crtc_state *crtc_state) > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > + struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + u32 i, tmp, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size; > + enum pipe pipe = crtc->pipe; > + struct drm_property_blob *blob = NULL; > + struct drm_color_lut *blob_data; > + > + blob = drm_property_create_blob(dev, > + sizeof(struct drm_color_lut) * 256, > + NULL); > + if (IS_ERR(blob)) > + return; > + > + blob_data = blob->data; > + > + for (i = 0; i < lut_size; i++) { > + tmp = I915_READ(CGM_PIPE_GAMMA(pipe, i, 0)); > + blob_data[i].green = intel_color_lut_pack((tmp & CGM_PIPE_GAMMA_GREEN_MASK) >> 16, 10); > + blob_data[i].blue = intel_color_lut_pack((tmp & CGM_PIPE_GAMMA_BLUE_MASK), 10); > + tmp = I915_READ(CGM_PIPE_GAMMA(pipe, i, 1)); > + blob_data[i].red = intel_color_lut_pack((tmp & CGM_PIPE_GAMMA_RED_MASK), 10); > + } > + > + crtc_state->base.gamma_lut = blob; > +} > + > +static void bdw_gamma_config(struct intel_crtc_state *crtc_state, u32 offset) > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > + struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + u32 i, tmp, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size; > + enum pipe pipe = crtc->pipe; > + struct drm_property_blob *blob = NULL; > + struct drm_color_lut *blob_data; > + > + WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK); > + > + I915_WRITE(PREC_PAL_INDEX(pipe), > + (offset ? PAL_PREC_SPLIT_MODE : 0) | > + PAL_PREC_AUTO_INCREMENT | > + offset); > + > + blob = drm_property_create_blob(dev, > + sizeof(struct drm_color_lut) * lut_size, > + NULL); > + if (IS_ERR(blob)) > + return; > + > + blob_data = blob->data; > + > + for (i = 0; i < lut_size; i++) { > + tmp = I915_READ(PREC_PAL_DATA(pipe)); > + blob_data[i].red = intel_color_lut_pack((tmp & PREC_PAL_DATA_RED_MASK) >> 20, 10); > + blob_data[i].green = intel_color_lut_pack((tmp & PREC_PAL_DATA_GREEN_MASK) >> 10, 10); > + blob_data[i].blue = intel_color_lut_pack((tmp & PREC_PAL_DATA_BLUE_MASK), 10); > + } > + > + I915_WRITE(PREC_PAL_INDEX(pipe), 0); > + > + crtc_state->base.gamma_lut = blob; > +} > + > +static void cherryview_color_config(struct intel_crtc_state *crtc_state) > +{ > + if (crtc_state_is_legacy_gamma(crtc_state)) crtc_state is the state that we're trying to read out of the hardware, right? In that case, crtc_state->gamma will always be NULL when you get to this point (since you haven't actually reconstructed the LUT's yet) and thus this will always return false. Like above, I think you want to look at the contents of the gamma mode register (which we read out into crtc_state->gamma_mode) to figure out whether you need to read out a legacy gamma table (from LGC_PALETTE), a precision gamma table (from PREC_PAL_DATA), etc. Also, if the gamma is disabled, then I believe you'd want to leave the table as NULL regardless of what the rest of the register settings are. (Same comment for the rest of the platforms below too) > + i9xx_color_config(crtc_state); It seems like for consistency you'd want to call i9xx_internal_gamma_config() here rather than i9xx_color_config(). My assumption is that the foo_[get_]color_config() functions will eventually be extended to also readout degamma and CTM as well? > + else > + cherryview_gamma_config(crtc_state); > +} > + > +static void broadwell_color_config(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); > + > + if (crtc_state_is_legacy_gamma(crtc_state)) > + i9xx_color_config(crtc_state); > + else > + bdw_gamma_config(crtc_state, > + INTEL_INFO(dev_priv)->color.degamma_lut_size); > +} > + > +static void glk_color_config(struct intel_crtc_state *crtc_state) > +{ > + if (crtc_state_is_legacy_gamma(crtc_state)) > + i9xx_color_config(crtc_state); > + else > + bdw_gamma_config(crtc_state, 0); > +} > + > +static void icl_color_config(struct intel_crtc_state *crtc_state) > +{ > + if (crtc_state_is_legacy_gamma(crtc_state)) > + i9xx_color_config(crtc_state); > + else > + bdw_gamma_config(crtc_state, 0); > +} > + > +void intel_color_config(struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > + > + dev_priv->display.color_config(crtc_state); > +} > + > void intel_color_commit(const struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > @@ -834,21 +1000,29 @@ void intel_color_init(struct intel_crtc *crtc) > drm_mode_crtc_set_gamma_size(&crtc->base, 256); > > if (HAS_GMCH(dev_priv)) { > - if (IS_CHERRYVIEW(dev_priv)) > + if (IS_CHERRYVIEW(dev_priv)) { > dev_priv->display.load_luts = cherryview_load_luts; > - else > + dev_priv->display.color_config = cherryview_color_config; > + } else { > dev_priv->display.load_luts = i9xx_load_luts; > + dev_priv->display.color_config = i9xx_color_config; > + } > > dev_priv->display.color_commit = i9xx_color_commit; > } else { > - if (IS_ICELAKE(dev_priv)) > + if (IS_ICELAKE(dev_priv)) { > dev_priv->display.load_luts = icl_load_luts; > - else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) > + dev_priv->display.color_config = icl_color_config; > + } else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) { > dev_priv->display.load_luts = glk_load_luts; > - else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) > + dev_priv->display.color_config = glk_color_config; > + } else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) { > dev_priv->display.load_luts = broadwell_load_luts; > - else > + dev_priv->display.color_config = broadwell_color_config; > + } else { > dev_priv->display.load_luts = i9xx_load_luts; > + dev_priv->display.color_config = i9xx_color_config; > + } > > if (INTEL_GEN(dev_priv) >= 9) > dev_priv->display.color_commit = skl_color_commit; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 963b4bd..31bb652 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8212,6 +8212,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, > > i9xx_get_pipe_color_config(pipe_config); > > + intel_color_config(pipe_config); > + > if (INTEL_GEN(dev_priv) < 4) > pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE; > > @@ -9284,6 +9286,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc, > > i9xx_get_pipe_color_config(pipe_config); > > + intel_color_config(pipe_config); > + > if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) { > struct intel_shared_dpll *pll; > enum intel_dpll_id pll_id; > @@ -9932,6 +9936,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > i9xx_get_pipe_color_config(pipe_config); > } > > + intel_color_config(pipe_config); > + > power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe); > if (intel_display_power_get_if_enabled(dev_priv, power_domain)) { > WARN_ON(power_domain_mask & BIT_ULL(power_domain)); > @@ -11990,6 +11996,36 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv) > return false; > } > > +static bool intel_compare_color_lut(struct drm_property_blob *blob1, > + struct drm_property_blob *blob2, > + u32 bit_precision) > +{ > + struct drm_color_lut *sw_lut = blob1->data; > + struct drm_color_lut *hw_lut = blob2->data; > + u32 err = 0xFFFF >> bit_precision; > + int sw_lut_size, hw_lut_size; > + int i; > + > + sw_lut_size = drm_color_lut_size(blob1); > + hw_lut_size = drm_color_lut_size(blob2); > + > + if (IS_ERR(blob1) || IS_ERR(blob2)) > + return false; As Jani noted, we'd want a test like this before we try to use the blobs in drm_color_lut_size(). But is this even possible? If you failed to allocate the readout blob, you just return immediately from foo_gamma_config and never try to assign it to crtc_state->gamma_lut. Likewise I don't see a way for the current software config to hold a PTR_ERR value either; if it does then we've got a bug somewhere else in the driver and we're probably going to wind up crashing somewhere else anyway. I think the case you do need to handle in this function is if a blob is NULL. Matt > + > + if (sw_lut_size != hw_lut_size) > + return false; > + > + for (i = 0; i < sw_lut_size; i++) { > + if (((abs((long)hw_lut[i].red - sw_lut[i].red)) >= err) || > + ((abs((long)hw_lut[i].blue - sw_lut[i].blue)) >= err) || > + ((abs((long)hw_lut[i].green - sw_lut[i].green)) >= err)) { > + return false; > + } > + } > + > + return true; > +} > + > static bool > intel_pipe_config_compare(struct drm_i915_private *dev_priv, > struct intel_crtc_state *current_config, > @@ -11997,6 +12033,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv) > bool adjust) > { > bool ret = true; > + u32 bit_precision; > bool fixup_inherited = adjust && > (current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) && > !(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED); > @@ -12148,6 +12185,14 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv) > } \ > } while (0) > > +#define PIPE_CONF_CHECK_COLOR_LUT(name, bit_precision) do { \ > + if (!intel_compare_color_lut(current_config->name, pipe_config->name, bit_precision)) { \ > + pipe_config_err(adjust, __stringify(name), \ > + "hw_state doesn't match sw_state\n"); \ > + ret = false; \ > + } \ > +} while (0) > + > #define PIPE_CONF_QUIRK(quirk) \ > ((current_config->quirks | pipe_config->quirks) & (quirk)) > > @@ -12287,6 +12332,9 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv) > PIPE_CONF_CHECK_INFOFRAME(spd); > PIPE_CONF_CHECK_INFOFRAME(hdmi); > > + bit_precision = intel_color_bit_precision(dev_priv); > + PIPE_CONF_CHECK_COLOR_LUT(base.gamma_lut, bit_precision); > + > #undef PIPE_CONF_CHECK_X > #undef PIPE_CONF_CHECK_I > #undef PIPE_CONF_CHECK_BOOL > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 40ebc94..6a89d2d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -2512,6 +2512,8 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ > int intel_color_check(struct intel_crtc_state *crtc_state); > void intel_color_commit(const struct intel_crtc_state *crtc_state); > void intel_color_load_luts(const struct intel_crtc_state *crtc_state); > +void intel_color_config(struct intel_crtc_state *crtc_state); > +u32 intel_color_bit_precision(struct drm_i915_private *dev_priv); > > /* intel_lspcon.c */ > bool lspcon_init(struct intel_digital_port *intel_dig_port); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Mar 28, 2019 at 01:56:18PM -0700, Matt Roper wrote: > On Thu, Mar 28, 2019 at 12:03:48PM +0530, Swati Sharma wrote: > > Added state checker to validate gamma_lut values. This > > reads hardware state, and compares the originally requested > > state to the state read from hardware. > > > > v1: -Implementation done for legacy platforms (removed all the placeholders) (Jani) > > -Added inverse function of drm_color_lut_extract to convert hardware > > read values back to user values (code written by Jani) > > -Renamed get_config() to color_config() (Jani) > > -Placed all platform specific shifts and masks in i915_reg.h (Jani) > > -Renamed i9xx_get_config to i9xx_color_config and all related > > functions (Jani) > > -Removed debug logs from compare function (Jani) > > -Renamed intel_compare_blob to intel_compare_lut and added platform specific > > bit precision of the readout into the function (Jani) > > -Renamed macro PIPE_CONF_CHECK_BLOB to PIPE_CONF_CHECK_COLOR_LUT (Jani) > > -Added check if blobs can be NULL (Jani) > > -Added function in intel_color.c that returns the bit precision (Jani), > > didn't add in device info since its gonna die soon (Ville) > > > > TODO: > > -Add a separate function to log errors at the higher level > > -Haven't moved intel_compare_lut() from intel_display.c to intel_color.c > > Since all the comparison functions are placed in intel_display, isn't > > it the right place (or) we want to move to consolidate color related functions > > together? Opinion? Please correct me if I am wrong. > > -Optimizations and refractoring > > > > Signed-off-by: Swati Sharma <swati2.sharma@intel.com> > > I agree with Jani's feedback and have a couple other comments inline below. > > Also, since I don't see it on the TODO list here, do you intend to also > readout and compare degamma and CTM eventually? > > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/i915_reg.h | 12 +++ > > drivers/gpu/drm/i915/intel_color.c | 186 +++++++++++++++++++++++++++++++++-- > > drivers/gpu/drm/i915/intel_display.c | 48 +++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 2 + > > 5 files changed, 243 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index c4ffe19..b422ea6 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -334,6 +334,7 @@ struct drm_i915_display_funcs { > > * involved with the same commit. > > */ > > void (*load_luts)(const struct intel_crtc_state *crtc_state); > > + void (*color_config)(struct intel_crtc_state *crtc_state); > > }; > > > > #define CSR_VERSION(major, minor) ((major) << 16 | (minor)) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index c0cd7a8..2813033 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -7156,6 +7156,10 @@ enum { > > #define _LGC_PALETTE_B 0x4a800 > > #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) + (i) * 4) > > > > +#define LGC_PALETTE_RED_MASK (0xFF << 16) > > +#define LGC_PALETTE_GREEN_MASK (0xFF << 8) > > +#define LGC_PALETTE_BLUE_MASK (0xFF << 0) > > + > > #define _GAMMA_MODE_A 0x4a480 > > #define _GAMMA_MODE_B 0x4ac80 > > #define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B) > > @@ -10102,6 +10106,10 @@ enum skl_power_gate { > > #define PRE_CSC_GAMC_INDEX(pipe) _MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B) > > #define PRE_CSC_GAMC_DATA(pipe) _MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B) > > > > +#define PREC_PAL_DATA_RED_MASK (0x3FF << 20) > > +#define PREC_PAL_DATA_GREEN_MASK (0x3FF << 10) > > +#define PREC_PAL_DATA_BLUE_MASK (0x3FF << 0) > > + > > /* pipe CSC & degamma/gamma LUTs on CHV */ > > #define _CGM_PIPE_A_CSC_COEFF01 (VLV_DISPLAY_BASE + 0x67900) > > #define _CGM_PIPE_A_CSC_COEFF23 (VLV_DISPLAY_BASE + 0x67904) > > @@ -10133,6 +10141,10 @@ enum skl_power_gate { > > #define CGM_PIPE_GAMMA(pipe, i, w) _MMIO(_PIPE(pipe, _CGM_PIPE_A_GAMMA, _CGM_PIPE_B_GAMMA) + (i) * 8 + (w) * 4) > > #define CGM_PIPE_MODE(pipe) _MMIO_PIPE(pipe, _CGM_PIPE_A_MODE, _CGM_PIPE_B_MODE) > > > > +#define CGM_PIPE_GAMMA_RED_MASK (0x3FF << 0) > > +#define CGM_PIPE_GAMMA_GREEN_MASK (0x3FF << 16) > > +#define CGM_PIPE_GAMMA_BLUE_MASK (0x3FF << 0) > > + > > /* MIPI DSI registers */ > > > > #define _MIPI_PORT(port, a, c) (((port) == PORT_A) ? a : c) /* ports A and C only */ > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > > index da7a07d..bd4f1b1 100644 > > --- a/drivers/gpu/drm/i915/intel_color.c > > +++ b/drivers/gpu/drm/i915/intel_color.c > > @@ -679,6 +679,172 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state) > > dev_priv->display.load_luts(crtc_state); > > } > > > > +u32 intel_color_bit_precision(struct drm_i915_private *dev_priv) > > +{ > > + if (INTEL_GEN(dev_priv) >= 9) > > + return 10; > > + else > > + return 8; > > +} > > Doesn't bdw (gen8) also use a 10 bit palette (PREC_PAL_DATA)? > > I think we probably want to figure out the number of bits to compare > based on gamma_mode rather than the platform we're running on. E.g., a > platform with 10-bit precision palettes might still be using legacy > tables with 8 bits of precision in some cases. And some platforms have > even more potential gamma modes that we might enable in the future too. We'll need to look at .gamma_mode, .gamma_enable (on pre-icl), and .cgm_mode (on chv). Oh and .c8_planes too I suppose. Unfortunately that's a bit of a problem since we have no real plane readout code at the moment. Hmm, I guess we can ignore .c8_planes (and .gamma_enable too I suppose) and just blindly read out the legacy LUT whenever .gamma_mode says so, but we'll have to be prepared for the case where the software state had no gamma LUT at all (since we program .gamma_mode to 8bit in that case too).
On Thu, 28 Mar 2019, Matt Roper <matthew.d.roper@intel.com> wrote: > I agree with Jani's feedback and have a couple other comments inline below. Thanks Matt, good stuff here. One naming note below. > What does the "internal" in this name refer to? I think just something > like i9xx_get_gamma_config() would be sufficient > > Actually the term "gamma_config" on these functions makes me think we're > going to be reading out the gamma mode register as well, although that's > actually done in foo_get_pipe_config(). Maybe just calling this > something like "i9xx_get_gamma_lut()" or "i9xx_readout_gamma_lut()" > would be more clear? I'd like to retain "get" and "config" in the names, just to mentally map to what's going on. Having "gamma lut" in there is good too to distinguish from *_get_pipe_config(). BR, Jani.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c4ffe19..b422ea6 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -334,6 +334,7 @@ struct drm_i915_display_funcs { * involved with the same commit. */ void (*load_luts)(const struct intel_crtc_state *crtc_state); + void (*color_config)(struct intel_crtc_state *crtc_state); }; #define CSR_VERSION(major, minor) ((major) << 16 | (minor)) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index c0cd7a8..2813033 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7156,6 +7156,10 @@ enum { #define _LGC_PALETTE_B 0x4a800 #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) + (i) * 4) +#define LGC_PALETTE_RED_MASK (0xFF << 16) +#define LGC_PALETTE_GREEN_MASK (0xFF << 8) +#define LGC_PALETTE_BLUE_MASK (0xFF << 0) + #define _GAMMA_MODE_A 0x4a480 #define _GAMMA_MODE_B 0x4ac80 #define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B) @@ -10102,6 +10106,10 @@ enum skl_power_gate { #define PRE_CSC_GAMC_INDEX(pipe) _MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B) #define PRE_CSC_GAMC_DATA(pipe) _MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B) +#define PREC_PAL_DATA_RED_MASK (0x3FF << 20) +#define PREC_PAL_DATA_GREEN_MASK (0x3FF << 10) +#define PREC_PAL_DATA_BLUE_MASK (0x3FF << 0) + /* pipe CSC & degamma/gamma LUTs on CHV */ #define _CGM_PIPE_A_CSC_COEFF01 (VLV_DISPLAY_BASE + 0x67900) #define _CGM_PIPE_A_CSC_COEFF23 (VLV_DISPLAY_BASE + 0x67904) @@ -10133,6 +10141,10 @@ enum skl_power_gate { #define CGM_PIPE_GAMMA(pipe, i, w) _MMIO(_PIPE(pipe, _CGM_PIPE_A_GAMMA, _CGM_PIPE_B_GAMMA) + (i) * 8 + (w) * 4) #define CGM_PIPE_MODE(pipe) _MMIO_PIPE(pipe, _CGM_PIPE_A_MODE, _CGM_PIPE_B_MODE) +#define CGM_PIPE_GAMMA_RED_MASK (0x3FF << 0) +#define CGM_PIPE_GAMMA_GREEN_MASK (0x3FF << 16) +#define CGM_PIPE_GAMMA_BLUE_MASK (0x3FF << 0) + /* MIPI DSI registers */ #define _MIPI_PORT(port, a, c) (((port) == PORT_A) ? a : c) /* ports A and C only */ diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index da7a07d..bd4f1b1 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -679,6 +679,172 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state) dev_priv->display.load_luts(crtc_state); } +u32 intel_color_bit_precision(struct drm_i915_private *dev_priv) +{ + if (INTEL_GEN(dev_priv) >= 9) + return 10; + else + return 8; +} + +/* convert hw value with given bit_precision to lut property val */ +static u32 intel_color_lut_pack(u32 val, u32 bit_precision) +{ + u32 max = 0xFFFF >> (16 - bit_precision); + + val = clamp_val(val, 0, max); + + if (bit_precision < 16) + val <<= 16 - bit_precision; + + return val; +} + +static void i9xx_internal_gamma_config(struct intel_crtc_state *crtc_state) +{ + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); + struct drm_device *dev = crtc->base.dev; + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + u32 i, tmp; + enum pipe pipe = crtc->pipe; + struct drm_property_blob *blob = NULL; + struct drm_color_lut *blob_data; + + blob = drm_property_create_blob(dev, + sizeof(struct drm_color_lut) * 256, + NULL); + if (IS_ERR(blob)) + return; + + blob_data = blob->data; + + for (i = 0; i < 256; i++) { + if (HAS_GMCH(dev_priv)) + tmp = I915_READ(PALETTE(pipe, i)); + else + tmp = I915_READ(LGC_PALETTE(pipe, i)); + blob_data[i].red = intel_color_lut_pack((tmp & LGC_PALETTE_RED_MASK) >> 16, 8); + blob_data[i].green = intel_color_lut_pack((tmp & LGC_PALETTE_GREEN_MASK) >> 8, 8); + blob_data[i].blue = intel_color_lut_pack((tmp & LGC_PALETTE_BLUE_MASK), 8); + } + + crtc_state->base.gamma_lut = blob; +} + +static void i9xx_color_config(struct intel_crtc_state *crtc_state) +{ + i9xx_internal_gamma_config(crtc_state); +} + +static void cherryview_gamma_config(struct intel_crtc_state *crtc_state) +{ + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); + struct drm_device *dev = crtc->base.dev; + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + u32 i, tmp, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size; + enum pipe pipe = crtc->pipe; + struct drm_property_blob *blob = NULL; + struct drm_color_lut *blob_data; + + blob = drm_property_create_blob(dev, + sizeof(struct drm_color_lut) * 256, + NULL); + if (IS_ERR(blob)) + return; + + blob_data = blob->data; + + for (i = 0; i < lut_size; i++) { + tmp = I915_READ(CGM_PIPE_GAMMA(pipe, i, 0)); + blob_data[i].green = intel_color_lut_pack((tmp & CGM_PIPE_GAMMA_GREEN_MASK) >> 16, 10); + blob_data[i].blue = intel_color_lut_pack((tmp & CGM_PIPE_GAMMA_BLUE_MASK), 10); + tmp = I915_READ(CGM_PIPE_GAMMA(pipe, i, 1)); + blob_data[i].red = intel_color_lut_pack((tmp & CGM_PIPE_GAMMA_RED_MASK), 10); + } + + crtc_state->base.gamma_lut = blob; +} + +static void bdw_gamma_config(struct intel_crtc_state *crtc_state, u32 offset) +{ + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); + struct drm_device *dev = crtc->base.dev; + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + u32 i, tmp, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size; + enum pipe pipe = crtc->pipe; + struct drm_property_blob *blob = NULL; + struct drm_color_lut *blob_data; + + WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK); + + I915_WRITE(PREC_PAL_INDEX(pipe), + (offset ? PAL_PREC_SPLIT_MODE : 0) | + PAL_PREC_AUTO_INCREMENT | + offset); + + blob = drm_property_create_blob(dev, + sizeof(struct drm_color_lut) * lut_size, + NULL); + if (IS_ERR(blob)) + return; + + blob_data = blob->data; + + for (i = 0; i < lut_size; i++) { + tmp = I915_READ(PREC_PAL_DATA(pipe)); + blob_data[i].red = intel_color_lut_pack((tmp & PREC_PAL_DATA_RED_MASK) >> 20, 10); + blob_data[i].green = intel_color_lut_pack((tmp & PREC_PAL_DATA_GREEN_MASK) >> 10, 10); + blob_data[i].blue = intel_color_lut_pack((tmp & PREC_PAL_DATA_BLUE_MASK), 10); + } + + I915_WRITE(PREC_PAL_INDEX(pipe), 0); + + crtc_state->base.gamma_lut = blob; +} + +static void cherryview_color_config(struct intel_crtc_state *crtc_state) +{ + if (crtc_state_is_legacy_gamma(crtc_state)) + i9xx_color_config(crtc_state); + else + cherryview_gamma_config(crtc_state); +} + +static void broadwell_color_config(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); + + if (crtc_state_is_legacy_gamma(crtc_state)) + i9xx_color_config(crtc_state); + else + bdw_gamma_config(crtc_state, + INTEL_INFO(dev_priv)->color.degamma_lut_size); +} + +static void glk_color_config(struct intel_crtc_state *crtc_state) +{ + if (crtc_state_is_legacy_gamma(crtc_state)) + i9xx_color_config(crtc_state); + else + bdw_gamma_config(crtc_state, 0); +} + +static void icl_color_config(struct intel_crtc_state *crtc_state) +{ + if (crtc_state_is_legacy_gamma(crtc_state)) + i9xx_color_config(crtc_state); + else + bdw_gamma_config(crtc_state, 0); +} + +void intel_color_config(struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); + + dev_priv->display.color_config(crtc_state); +} + void intel_color_commit(const struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); @@ -834,21 +1000,29 @@ void intel_color_init(struct intel_crtc *crtc) drm_mode_crtc_set_gamma_size(&crtc->base, 256); if (HAS_GMCH(dev_priv)) { - if (IS_CHERRYVIEW(dev_priv)) + if (IS_CHERRYVIEW(dev_priv)) { dev_priv->display.load_luts = cherryview_load_luts; - else + dev_priv->display.color_config = cherryview_color_config; + } else { dev_priv->display.load_luts = i9xx_load_luts; + dev_priv->display.color_config = i9xx_color_config; + } dev_priv->display.color_commit = i9xx_color_commit; } else { - if (IS_ICELAKE(dev_priv)) + if (IS_ICELAKE(dev_priv)) { dev_priv->display.load_luts = icl_load_luts; - else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) + dev_priv->display.color_config = icl_color_config; + } else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) { dev_priv->display.load_luts = glk_load_luts; - else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) + dev_priv->display.color_config = glk_color_config; + } else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) { dev_priv->display.load_luts = broadwell_load_luts; - else + dev_priv->display.color_config = broadwell_color_config; + } else { dev_priv->display.load_luts = i9xx_load_luts; + dev_priv->display.color_config = i9xx_color_config; + } if (INTEL_GEN(dev_priv) >= 9) dev_priv->display.color_commit = skl_color_commit; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 963b4bd..31bb652 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8212,6 +8212,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, i9xx_get_pipe_color_config(pipe_config); + intel_color_config(pipe_config); + if (INTEL_GEN(dev_priv) < 4) pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE; @@ -9284,6 +9286,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc, i9xx_get_pipe_color_config(pipe_config); + intel_color_config(pipe_config); + if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) { struct intel_shared_dpll *pll; enum intel_dpll_id pll_id; @@ -9932,6 +9936,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, i9xx_get_pipe_color_config(pipe_config); } + intel_color_config(pipe_config); + power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe); if (intel_display_power_get_if_enabled(dev_priv, power_domain)) { WARN_ON(power_domain_mask & BIT_ULL(power_domain)); @@ -11990,6 +11996,36 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv) return false; } +static bool intel_compare_color_lut(struct drm_property_blob *blob1, + struct drm_property_blob *blob2, + u32 bit_precision) +{ + struct drm_color_lut *sw_lut = blob1->data; + struct drm_color_lut *hw_lut = blob2->data; + u32 err = 0xFFFF >> bit_precision; + int sw_lut_size, hw_lut_size; + int i; + + sw_lut_size = drm_color_lut_size(blob1); + hw_lut_size = drm_color_lut_size(blob2); + + if (IS_ERR(blob1) || IS_ERR(blob2)) + return false; + + if (sw_lut_size != hw_lut_size) + return false; + + for (i = 0; i < sw_lut_size; i++) { + if (((abs((long)hw_lut[i].red - sw_lut[i].red)) >= err) || + ((abs((long)hw_lut[i].blue - sw_lut[i].blue)) >= err) || + ((abs((long)hw_lut[i].green - sw_lut[i].green)) >= err)) { + return false; + } + } + + return true; +} + static bool intel_pipe_config_compare(struct drm_i915_private *dev_priv, struct intel_crtc_state *current_config, @@ -11997,6 +12033,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv) bool adjust) { bool ret = true; + u32 bit_precision; bool fixup_inherited = adjust && (current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) && !(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED); @@ -12148,6 +12185,14 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv) } \ } while (0) +#define PIPE_CONF_CHECK_COLOR_LUT(name, bit_precision) do { \ + if (!intel_compare_color_lut(current_config->name, pipe_config->name, bit_precision)) { \ + pipe_config_err(adjust, __stringify(name), \ + "hw_state doesn't match sw_state\n"); \ + ret = false; \ + } \ +} while (0) + #define PIPE_CONF_QUIRK(quirk) \ ((current_config->quirks | pipe_config->quirks) & (quirk)) @@ -12287,6 +12332,9 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv) PIPE_CONF_CHECK_INFOFRAME(spd); PIPE_CONF_CHECK_INFOFRAME(hdmi); + bit_precision = intel_color_bit_precision(dev_priv); + PIPE_CONF_CHECK_COLOR_LUT(base.gamma_lut, bit_precision); + #undef PIPE_CONF_CHECK_X #undef PIPE_CONF_CHECK_I #undef PIPE_CONF_CHECK_BOOL diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 40ebc94..6a89d2d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -2512,6 +2512,8 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ int intel_color_check(struct intel_crtc_state *crtc_state); void intel_color_commit(const struct intel_crtc_state *crtc_state); void intel_color_load_luts(const struct intel_crtc_state *crtc_state); +void intel_color_config(struct intel_crtc_state *crtc_state); +u32 intel_color_bit_precision(struct drm_i915_private *dev_priv); /* intel_lspcon.c */ bool lspcon_init(struct intel_digital_port *intel_dig_port);
Added state checker to validate gamma_lut values. This reads hardware state, and compares the originally requested state to the state read from hardware. v1: -Implementation done for legacy platforms (removed all the placeholders) (Jani) -Added inverse function of drm_color_lut_extract to convert hardware read values back to user values (code written by Jani) -Renamed get_config() to color_config() (Jani) -Placed all platform specific shifts and masks in i915_reg.h (Jani) -Renamed i9xx_get_config to i9xx_color_config and all related functions (Jani) -Removed debug logs from compare function (Jani) -Renamed intel_compare_blob to intel_compare_lut and added platform specific bit precision of the readout into the function (Jani) -Renamed macro PIPE_CONF_CHECK_BLOB to PIPE_CONF_CHECK_COLOR_LUT (Jani) -Added check if blobs can be NULL (Jani) -Added function in intel_color.c that returns the bit precision (Jani), didn't add in device info since its gonna die soon (Ville) TODO: -Add a separate function to log errors at the higher level -Haven't moved intel_compare_lut() from intel_display.c to intel_color.c Since all the comparison functions are placed in intel_display, isn't it the right place (or) we want to move to consolidate color related functions together? Opinion? Please correct me if I am wrong. -Optimizations and refractoring Signed-off-by: Swati Sharma <swati2.sharma@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_reg.h | 12 +++ drivers/gpu/drm/i915/intel_color.c | 186 +++++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_display.c | 48 +++++++++ drivers/gpu/drm/i915/intel_drv.h | 2 + 5 files changed, 243 insertions(+), 6 deletions(-)