On Thu, 28 Mar 2019, Swati Sharma <swati2.sha...@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.sha...@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);
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
