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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 24, 2018 at 06:12:27PM +0200, Daniel Vetter wrote:
> 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.

Not sure. Maybe just wait and see how crazy the compiler really is?
Apart from the dmesg noise no real harm should befall the user from
getting this wrong.

> 
> > +}
> > +
> >  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
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel