On Wed, 20 Mar 2019, "Sharma, Swati2" <swati2.sha...@intel.com> wrote: > On 15-Mar-19 3:17 PM, Nikula, Jani wrote: >> On Fri, 15 Mar 2019, swati2.sha...@intel.com wrote: >>> From: Swati Sharma <swati2.sha...@intel.com> >>> >>> Added state checker to validate gamma_lut values. This >>> reads hardware state, and compares the originally requested >>> state to the state read from hardware. >>> >>> This implementation can be used for Gen9+ platforms, >>> I haven't implemented it for legacy platforms. Just want to get >>> feedback if this is the right approach to follow. >>> >>> Also, inverse function of drm_color_lut_extract is missing >>> to convert hardware read values back to user values. >>> Thinking for that. I have added all "TODOs" and "Placeholders". >>> >>> Another approach: >>> Storing "word" to be written into hardware in dev_priv and >>> instead of comparing blob, comparing "word"? In dev_priv, >>> only pointer will be there (something like *gamma_word). >> You can't store it in dev_priv because it's crtc state specific >> data. Even if stored in crtc state, the approach doesn't help the >> initial hw state readout and takeover. >> >> Please use intel-gfx mailing list for i915 patches. >> >>> For this too, I will send a patch to make it more clear. >>> >>> Signed-off-by: Swati Sharma <swati2.sha...@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_drv.h | 1 + >>> drivers/gpu/drm/i915/intel_color.c | 127 >>> +++++++++++++++++++++++++++++++++-- >>> drivers/gpu/drm/i915/intel_display.c | 50 ++++++++++++++ >>> drivers/gpu/drm/i915/intel_drv.h | 1 + >>> 4 files changed, 173 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>> b/drivers/gpu/drm/i915/i915_drv.h >>> index c4ffe19..b41bfaa 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 (*get_config)(struct intel_crtc_state *crtc_state); >> The name is too generic. >> >>> }; >>> >>> #define CSR_VERSION(major, minor) ((major) << 16 | (minor)) >>> diff --git a/drivers/gpu/drm/i915/intel_color.c >>> b/drivers/gpu/drm/i915/intel_color.c >>> index da7a07d..a515e9f 100644 >>> --- a/drivers/gpu/drm/i915/intel_color.c >>> +++ b/drivers/gpu/drm/i915/intel_color.c >>> @@ -74,6 +74,11 @@ >>> #define ILK_CSC_COEFF_1_0 \ >>> ((7 << 12) | ILK_CSC_COEFF_FP(CTM_COEFF_1_0, 8)) >>> >>> +/* Mask to extract RGB values from registers */ >>> +#define COLOR_BLUE_MASK 0x000003FF /* 9:0 */ >>> +#define COLOR_GREEN_MASK 0x000FFC00 /* 19:10 */ >>> +#define COLOR_RED_MASK 0x3FF00000 /* 29:20 */ >> These belong in i915_reg.h, and you need platform specific shifts and >> masks. The code that writes the registers seems to use magic numbers... >> >>> + >>> static bool lut_is_legacy(const struct drm_property_blob *lut) >>> { >>> return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH; >>> @@ -672,6 +677,97 @@ static void cherryview_load_luts(const struct >>> intel_crtc_state *crtc_state) >>> i9xx_load_luts_internal(crtc_state, NULL); >>> } >>> >>> +static void bdw_get_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)); >>> + /* >>> + * TODO: convert RGB value read from register into >>> corresponding user value using >>> + * some wrapper like drm_color_lut_put() (or) >>> intel_color_lut_put() so that it >>> + * can be compared later. >>> + */ >> Yeah, you'll need this. > Can you please help in this?
Something like this: /* convert hw value with given bit_precision to lut property val */ u32 drm_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; } /* compare two lut property values with given bit_precision */ bool drm_color_lut_match(u32 a, u32 b, u32 bit_precision) { u32 err = 0xffff >> bit_precision; return abs((long)a - b) <= err; } I didn't double check the math, but it's probably close enough for starters. ;) BR, Jani. >> >>> + blob_data[i].red = (tmp & COLOR_RED_MASK) >> 20; >>> + blob_data[i].green = (tmp & COLOR_GREEN_MASK) >> 10; >>> + blob_data[i].blue = tmp & COLOR_BLUE_MASK; >>> + } >>> + >>> + I915_WRITE(PREC_PAL_INDEX(pipe), 0); >>> + >>> + crtc_state->base.gamma_lut = blob; >>> +} >>> + >>> +static void i9xx_get_config(struct intel_crtc_state *crtc_state) >> Please include (de)gamma or color in the function names. >> >>> +{ >>> + 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)); >>> + /* >>> + * TODO: convert RGB value read from register into >>> corresponding user value using >>> + * some wrapper like drm_color_lut_put() (or) >>> intel_color_lut_put() so that it >>> + * can be compared later. >>> + */ >>> + blob_data[i].red = (tmp & COLOR_RED_MASK) >> 20; >>> + blob_data[i].green = (tmp & COLOR_GREEN_MASK) >> 10; >>> + blob_data[i].blue = (tmp & COLOR_BLUE_MASK); >>> + } >>> + >>> + crtc_state->base.gamma_lut = blob; >>> +} >>> + >>> +static void icl_get_config(struct intel_crtc_state *crtc_state) >>> +{ >>> + /* >>> + * TODO: placeholder for degamma validation >>> + * glk_get_degamma_config(crtc_state, 0); >>> + */ >>> + >>> + if (crtc_state_is_legacy_gamma(crtc_state)) >>> + i9xx_get_config(crtc_state); >>> + else >>> + bdw_get_gamma_config(crtc_state, 0); >>> +} >>> + >>> void intel_color_load_luts(const struct intel_crtc_state *crtc_state) >>> { >>> struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); >>> @@ -679,6 +775,13 @@ void intel_color_load_luts(const struct >>> intel_crtc_state *crtc_state) >>> dev_priv->display.load_luts(crtc_state); >>> } >>> >>> +void intel_color_get_config(struct intel_crtc_state *crtc_state) >>> +{ >>> + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); >>> + >>> + dev_priv->display.get_config(crtc_state); >> This will oops on platforms where you haven't implemented it yet. >> >>> +} >>> + >>> void intel_color_commit(const struct intel_crtc_state *crtc_state) >>> { >>> struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); >>> @@ -833,22 +936,34 @@ void intel_color_init(struct intel_crtc *crtc) >>> >>> drm_mode_crtc_set_gamma_size(&crtc->base, 256); >>> >>> + /* >>> + * TODO: In the following if ladder, kept placeholders >>> + * for extending lut validation for legacy platforms. >>> + */ >> I think adding the placeholder comments is too messy. >> >>> 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.get_config = >>> cherryview_get_config; */ >>> + } else { >>> dev_priv->display.load_luts = i9xx_load_luts; >>> + dev_priv->display.get_config = i9xx_get_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.get_config = icl_get_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.get_config = glk_get_config; */ >>> + } else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) { >>> dev_priv->display.load_luts = broadwell_load_luts; >>> - else >>> + /* dev_priv->display.get_config = broadwell_get_config; >>> */ >>> + } else { >>> dev_priv->display.load_luts = i9xx_load_luts; >>> + dev_priv->display.get_config = i9xx_get_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..165d797 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -8212,6 +8212,11 @@ static bool i9xx_get_pipe_config(struct intel_crtc >>> *crtc, >>> >>> i9xx_get_pipe_color_config(pipe_config); >>> >>> + /* >>> + * TODO: Yet to implement for legacy platforms >>> + * intel_color_get_config(pipe_config); >>> + */ >>> + >> I'm not sure about adding these comments either. >> >>> if (INTEL_GEN(dev_priv) < 4) >>> pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE; >>> >>> @@ -9284,6 +9289,11 @@ static bool ironlake_get_pipe_config(struct >>> intel_crtc *crtc, >>> >>> i9xx_get_pipe_color_config(pipe_config); >>> >>> + /* >>> + * TODO: Yet to implement for legacy platforms >>> + * intel_color_get_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 +9942,8 @@ static bool haswell_get_pipe_config(struct intel_crtc >>> *crtc, >>> i9xx_get_pipe_color_config(pipe_config); >>> } >>> >>> + intel_color_get_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 +12002,34 @@ static bool fastboot_enabled(struct >>> drm_i915_private *dev_priv) >>> return false; >>> } >>> >>> +static bool intel_compare_blob(struct drm_property_blob *blob1, >>> + struct drm_property_blob *blob2) >>> +{ >>> + struct drm_color_lut *sw_lut = blob1->data; >>> + struct drm_color_lut *hw_lut = blob2->data; >>> + 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 (sw_lut_size != hw_lut_size) { >>> + DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n", >>> hw_lut_size, sw_lut_size); >> Please don't add these debug logs here. Instead, add a separate function >> to log errors at the higher level. Contrast PIPE_CONF_CHECK_INFOFRAME() >> and pipe_config_infoframe_err(). That can come as a follow-up >> improvement, but the debug logs here are unnecessary. >> >>> + return false; >>> + } >>> + >>> + for (i = 0; i < sw_lut_size; i++) { >>> + if (hw_lut[i].red != sw_lut[i].red || >>> + hw_lut[i].blue != sw_lut[i].blue || >>> + hw_lut[i].green != sw_lut[i].green) { >>> + DRM_DEBUG_KMS("Invalid LUT value; sw_lut value doesn't >>> match hw_lut value\n"); >>> + return false; >>> + } >>> + } >>> + >>> + return true; >>> +} >>> + >> I'd put this in intel_color.c and name it properly. You can't use it to >> compare random blobs, and the comparison needs to take platform specific >> bit precision of the readout into account. >> >> The blobs may be NULL. >> >>> static bool >>> intel_pipe_config_compare(struct drm_i915_private *dev_priv, >>> struct intel_crtc_state *current_config, >>> @@ -12148,6 +12188,14 @@ static bool fastboot_enabled(struct >>> drm_i915_private *dev_priv) >>> } \ >>> } while (0) >>> >>> +#define PIPE_CONF_CHECK_BLOB(name) do { \ >> CHECK_LUT or CHECK_COLOR_LUT or something, not just CHECK_BLOB. >> >>> + if (!intel_compare_blob(current_config->name, pipe_config->name)) { \ >>> + 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 +12335,8 @@ static bool fastboot_enabled(struct >>> drm_i915_private *dev_priv) >>> PIPE_CONF_CHECK_INFOFRAME(spd); >>> PIPE_CONF_CHECK_INFOFRAME(hdmi); >>> >>> + PIPE_CONF_CHECK_BLOB(base.gamma_lut); >>> + >>> #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..a6acd9b 100644 >>> --- a/drivers/gpu/drm/i915/intel_drv.h >>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>> @@ -2512,6 +2512,7 @@ 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_get_config(struct intel_crtc_state *crtc_state); >>> >>> /* intel_lspcon.c */ >>> bool lspcon_init(struct intel_digital_port *intel_dig_port); -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel