Re: [PATCH 17/18] drm/i915: Check infoframe state in intel_pipe_config_compare()

Daniel Vetter
2018-09-24T16:12:31Z

On Thu, Sep 20, 2018 at 09:51:44PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Check the infoframes and infoframe enable state when comparing two
> crtc states.
>
> We'll use the infoframe logging functions from video/hdmi.c to
> show the infoframes as part of the state dump.
>
> TODO: Try to better integrate the infoframe dumps with
> drm state dumps
>
> v2: drm_printk() is no more
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---

Might need adapting to PIPE_CONFIG_QUIRK_INFOFRAME, but aside from that

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> drivers/gpu/drm/i915/intel_display.c | 49 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fbcc56caffb6..3dce49e36a05 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11380,6 +11380,37 @@ intel_compare_link_m_n(const struct intel_link_m_n *m_n,
> return false;
> }
>
> +static bool
> +intel_compare_infoframe(const union hdmi_infoframe *a,
> + const union hdmi_infoframe *b)
> +{
> + return memcmp(a, b, sizeof(*a)) == 0;
> +}
> +
> +static void
> +pipe_config_infoframe_err(struct drm_i915_private *dev_priv,
> + bool adjust, const char *name,
> + const union hdmi_infoframe *a,
> + const union hdmi_infoframe *b)
> +{
> + if (adjust) {
> + if ((drm_debug & DRM_UT_KMS) == 0)
> + return;
> +
> + drm_dbg(DRM_UT_KMS, "mismatch in %s infoframe", name);
> + drm_dbg(DRM_UT_KMS, "expected:");
> + hdmi_infoframe_log(KERN_DEBUG, dev_priv->drm.dev, a);
> + drm_dbg(DRM_UT_KMS, "found");
> + hdmi_infoframe_log(KERN_DEBUG, dev_priv->drm.dev, b);
> + } else {
> + drm_err("mismatch in %s infoframe", name);
> + drm_err("expected:");
> + hdmi_infoframe_log(KERN_ERR, dev_priv->drm.dev, a);
> + drm_err("found");
> + hdmi_infoframe_log(KERN_ERR, dev_priv->drm.dev, b);
> + }

Mildly concerned about padding fields (since these are the not-compatified
structs). Maybe dump the mismatching byte too, plus byte offset? Or maybe
I'm just too paranoid.

> +}
> +
> static void __printf(3, 4)
> pipe_config_err(bool adjust, const char *name, const char *format, ...)
> {
> @@ -11541,7 +11572,17 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> } \
> } while (0)
>
> -#define PIPE_CONF_QUIRK(quirk) \
> +#define PIPE_CONF_CHECK_INFOFRAME(name) do { \
> + if (!intel_compare_infoframe(&current_config->infoframes.name, \
> + &pipe_config->infoframes.name)) { \
> + pipe_config_infoframe_err(dev_priv, adjust, __stringify(name), \
> + &current_config->infoframes.name, \
> + &pipe_config->infoframes.name); \
> + ret = false; \
> + } \
> +} while (0)
> +
> +#define PIPE_CONF_QUIRK(quirk) \
> ((current_config->quirks | pipe_config->quirks) & (quirk))
>
> PIPE_CONF_CHECK_I(cpu_transcoder);
> @@ -11670,6 +11711,12 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>
> PIPE_CONF_CHECK_I(min_voltage_level);
>
> + PIPE_CONF_CHECK_X(infoframes.enable);
> + PIPE_CONF_CHECK_X(infoframes.gcp);
> + PIPE_CONF_CHECK_INFOFRAME(avi);
> + PIPE_CONF_CHECK_INFOFRAME(spd);
> + PIPE_CONF_CHECK_INFOFRAME(hdmi);
> +
> #undef PIPE_CONF_CHECK_X
> #undef PIPE_CONF_CHECK_I
> #undef PIPE_CONF_CHECK_BOOL
> --
> 2.16.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation