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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 01, 2018 at 11:35:05PM +0300, Ville Syrjälä wrote:
> 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.

The scenario I have in mind is padding fields + someone didn't use
kzalloc. Dumping the mismatching byte would help in debugging this.
-Daniel

> 
> > 
> > > +}
> > > +
> > >  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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel