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

2018-10-02 Thread Daniel Vetter 
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ä 
> > > 
> > > 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ä 
> > > ---
> > 
> > Might need adapting to PIPE_CONFIG_QUIRK_INFOFRAME, but aside from that
> > 
> > Reviewed-by: Daniel Vetter 
> > 
> > >  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(_config->infoframes.name, \
> > > +  _config->infoframes.name)) { \
> > > + pipe_config_infoframe_err(dev_priv, adjust, __stringify(name), \
> > > +   _config->infoframes.name, \
> > > +   _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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org

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

2018-10-01 Thread Ville Syrjälä 
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ä 
> > 
> > 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ä 
> > ---
> 
> Might need adapting to PIPE_CONFIG_QUIRK_INFOFRAME, but aside from that
> 
> Reviewed-by: Daniel Vetter 
> 
> >  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(_config->infoframes.name, \
> > +_config->infoframes.name)) { \
> > +   pipe_config_infoframe_err(dev_priv, adjust, __stringify(name), \
> > + _config->infoframes.name, \
> > + _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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2018-09-24 Thread Daniel Vetter 
On Thu, Sep 20, 2018 at 09:51:44PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> 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ä 
> ---

Might need adapting to PIPE_CONFIG_QUIRK_INFOFRAME, but aside from that

Reviewed-by: Daniel Vetter 

>  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(_config->infoframes.name, \
> +  _config->infoframes.name)) { \
> + pipe_config_infoframe_err(dev_priv, adjust, __stringify(name), \
> +   _config->infoframes.name, \
> +   _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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2018-09-20 Thread Ville Syrjala 
From: Ville Syrjälä 

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ä 
---
 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);
+   }
+}
+
 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(_config->infoframes.name, \
+_config->infoframes.name)) { \
+   pipe_config_infoframe_err(dev_priv, adjust, __stringify(name), \
+ _config->infoframes.name, \
+ _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