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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
