[PATCH] drm/i915: Fix AVI/HDMI/SPD infoframes on HSW+

[PATCH] drm/i915: Fix AVI/HDMI/SPD infoframes on HSW+

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I broke AVI/HDMI/SPD infoframes on HSW+ with the register type
safety changes. We were supposed to check that the infoframe data
register is valid before writing the infoframe data, but the check
ended up inverted, and so in practice we never wrote or enabled
these infoframes.

We were still sending out the GCP infoframe when the sink was
deep-color capable. That and the fact that we use a single
bool to track our infoframe state meant that the state checker
only caught this when a HDMI sink that doesn't do deep-color was
used.

We really need to fix our infoframe state checking to be much
more anal. But in the meantime let's just fix the regression.
In fact let's just throw out the register validity check and
convert some of the "unknown info frame type" debug messages
into MISSING_CASE(). So far we support the same set of infoframe
types on all platforms, so the silent debug messages make no
sense.

Fixes: f0f59a00a1c9 ("drm/i915: Type safe register read/write")
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (irc)
Tested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (irc)
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 682554385631..7fae62626b71 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -78,7 +78,7 @@ static u32 g4x_infoframe_index(enum hdmi_infoframe_type type)
 	case HDMI_INFOFRAME_TYPE_VENDOR:
 		return VIDEO_DIP_SELECT_VENDOR;
 	default:
-		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
+		MISSING_CASE(type);
 		return 0;
 	}
 }
@@ -93,7 +93,7 @@ static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type)
 	case HDMI_INFOFRAME_TYPE_VENDOR:
 		return VIDEO_DIP_ENABLE_VENDOR;
 	default:
-		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
+		MISSING_CASE(type);
 		return 0;
 	}
 }
@@ -108,7 +108,7 @@ static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type)
 	case HDMI_INFOFRAME_TYPE_VENDOR:
 		return VIDEO_DIP_ENABLE_VS_HSW;
 	default:
-		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
+		MISSING_CASE(type);
 		return 0;
 	}
 }
@@ -127,7 +127,7 @@ hsw_dip_data_reg(struct drm_i915_private *dev_priv,
 	case HDMI_INFOFRAME_TYPE_VENDOR:
 		return HSW_TVIDEO_DIP_VS_DATA(cpu_transcoder, i);
 	default:
-		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
+		MISSING_CASE(type);
 		return INVALID_MMIO_REG;
 	}
 }
@@ -375,8 +375,6 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
 	u32 val = I915_READ(ctl_reg);
 
 	data_reg = hsw_dip_data_reg(dev_priv, cpu_transcoder, type, 0);
-	if (i915_mmio_reg_valid(data_reg))
-		return;
 
 	val &= ~hsw_infoframe_enable(type);
 	I915_WRITE(ctl_reg, val);
-- 
2.4.10

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix AVI/HDMI/SPD infoframes on HSW+

[Ville Syrjälä]

On Wed, Dec 16, 2015 at 06:10:00PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I broke AVI/HDMI/SPD infoframes on HSW+ with the register type
> safety changes. We were supposed to check that the infoframe data
> register is valid before writing the infoframe data, but the check
> ended up inverted, and so in practice we never wrote or enabled
> these infoframes.
> 
> We were still sending out the GCP infoframe when the sink was
> deep-color capable. That and the fact that we use a single
> bool to track our infoframe state meant that the state checker
> only caught this when a HDMI sink that doesn't do deep-color was
> used.
> 
> We really need to fix our infoframe state checking to be much
> more anal. But in the meantime let's just fix the regression.
> In fact let's just throw out the register validity check and
> convert some of the "unknown info frame type" debug messages
> into MISSING_CASE(). So far we support the same set of infoframe
> types on all platforms, so the silent debug messages make no
> sense.
> 
> Fixes: f0f59a00a1c9 ("drm/i915: Type safe register read/write")
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (irc)
> Tested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (irc)
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

And of course I forgot the bugzilla link
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93119

> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 682554385631..7fae62626b71 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -78,7 +78,7 @@ static u32 g4x_infoframe_index(enum hdmi_infoframe_type type)
>  	case HDMI_INFOFRAME_TYPE_VENDOR:
>  		return VIDEO_DIP_SELECT_VENDOR;
>  	default:
> -		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> +		MISSING_CASE(type);
>  		return 0;
>  	}
>  }
> @@ -93,7 +93,7 @@ static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type)
>  	case HDMI_INFOFRAME_TYPE_VENDOR:
>  		return VIDEO_DIP_ENABLE_VENDOR;
>  	default:
> -		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> +		MISSING_CASE(type);
>  		return 0;
>  	}
>  }
> @@ -108,7 +108,7 @@ static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type)
>  	case HDMI_INFOFRAME_TYPE_VENDOR:
>  		return VIDEO_DIP_ENABLE_VS_HSW;
>  	default:
> -		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> +		MISSING_CASE(type);
>  		return 0;
>  	}
>  }
> @@ -127,7 +127,7 @@ hsw_dip_data_reg(struct drm_i915_private *dev_priv,
>  	case HDMI_INFOFRAME_TYPE_VENDOR:
>  		return HSW_TVIDEO_DIP_VS_DATA(cpu_transcoder, i);
>  	default:
> -		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> +		MISSING_CASE(type);
>  		return INVALID_MMIO_REG;
>  	}
>  }
> @@ -375,8 +375,6 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
>  	u32 val = I915_READ(ctl_reg);
>  
>  	data_reg = hsw_dip_data_reg(dev_priv, cpu_transcoder, type, 0);
> -	if (i915_mmio_reg_valid(data_reg))
> -		return;
>  
>  	val &= ~hsw_infoframe_enable(type);
>  	I915_WRITE(ctl_reg, val);
> -- 
> 2.4.10

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix AVI/HDMI/SPD infoframes on HSW+

[Daniel Vetter]

On Wed, Dec 16, 2015 at 06:13:44PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 16, 2015 at 06:10:00PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > I broke AVI/HDMI/SPD infoframes on HSW+ with the register type
> > safety changes. We were supposed to check that the infoframe data
> > register is valid before writing the infoframe data, but the check
> > ended up inverted, and so in practice we never wrote or enabled
> > these infoframes.
> > 
> > We were still sending out the GCP infoframe when the sink was
> > deep-color capable. That and the fact that we use a single
> > bool to track our infoframe state meant that the state checker
> > only caught this when a HDMI sink that doesn't do deep-color was
> > used.
> > 
> > We really need to fix our infoframe state checking to be much
> > more anal. But in the meantime let's just fix the regression.
> > In fact let's just throw out the register validity check and
> > convert some of the "unknown info frame type" debug messages
> > into MISSING_CASE(). So far we support the same set of infoframe
> > types on all platforms, so the silent debug messages make no
> > sense.
> > 
> > Fixes: f0f59a00a1c9 ("drm/i915: Type safe register read/write")

I did recheck this comment for any other conversion to valid_reg that
might have gone wrong too. Didn't find anything.

> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (irc)
> > Tested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (irc)
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> And of course I forgot the bugzilla link
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93119

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

> 
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 682554385631..7fae62626b71 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -78,7 +78,7 @@ static u32 g4x_infoframe_index(enum hdmi_infoframe_type type)
> >  	case HDMI_INFOFRAME_TYPE_VENDOR:
> >  		return VIDEO_DIP_SELECT_VENDOR;
> >  	default:
> > -		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> > +		MISSING_CASE(type);
> >  		return 0;
> >  	}
> >  }
> > @@ -93,7 +93,7 @@ static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type)
> >  	case HDMI_INFOFRAME_TYPE_VENDOR:
> >  		return VIDEO_DIP_ENABLE_VENDOR;
> >  	default:
> > -		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> > +		MISSING_CASE(type);
> >  		return 0;
> >  	}
> >  }
> > @@ -108,7 +108,7 @@ static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type)
> >  	case HDMI_INFOFRAME_TYPE_VENDOR:
> >  		return VIDEO_DIP_ENABLE_VS_HSW;
> >  	default:
> > -		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> > +		MISSING_CASE(type);
> >  		return 0;
> >  	}
> >  }
> > @@ -127,7 +127,7 @@ hsw_dip_data_reg(struct drm_i915_private *dev_priv,
> >  	case HDMI_INFOFRAME_TYPE_VENDOR:
> >  		return HSW_TVIDEO_DIP_VS_DATA(cpu_transcoder, i);
> >  	default:
> > -		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> > +		MISSING_CASE(type);
> >  		return INVALID_MMIO_REG;
> >  	}
> >  }
> > @@ -375,8 +375,6 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
> >  	u32 val = I915_READ(ctl_reg);
> >  
> >  	data_reg = hsw_dip_data_reg(dev_priv, cpu_transcoder, type, 0);
> > -	if (i915_mmio_reg_valid(data_reg))
> > -		return;
> >  
> >  	val &= ~hsw_infoframe_enable(type);
> >  	I915_WRITE(ctl_reg, val);
> > -- 
> > 2.4.10
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix AVI/HDMI/SPD infoframes on HSW+

[Ville Syrjälä]

On Wed, Dec 16, 2015 at 05:15:18PM +0100, Daniel Vetter wrote:
> On Wed, Dec 16, 2015 at 06:13:44PM +0200, Ville Syrjälä wrote:
> > On Wed, Dec 16, 2015 at 06:10:00PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > I broke AVI/HDMI/SPD infoframes on HSW+ with the register type
> > > safety changes. We were supposed to check that the infoframe data
> > > register is valid before writing the infoframe data, but the check
> > > ended up inverted, and so in practice we never wrote or enabled
> > > these infoframes.
> > > 
> > > We were still sending out the GCP infoframe when the sink was
> > > deep-color capable. That and the fact that we use a single
> > > bool to track our infoframe state meant that the state checker
> > > only caught this when a HDMI sink that doesn't do deep-color was
> > > used.
> > > 
> > > We really need to fix our infoframe state checking to be much
> > > more anal. But in the meantime let's just fix the regression.
> > > In fact let's just throw out the register validity check and
> > > convert some of the "unknown info frame type" debug messages
> > > into MISSING_CASE(). So far we support the same set of infoframe
> > > types on all platforms, so the silent debug messages make no
> > > sense.
> > > 
> > > Fixes: f0f59a00a1c9 ("drm/i915: Type safe register read/write")
> 
> I did recheck this comment for any other conversion to valid_reg that
> might have gone wrong too. Didn't find anything.
> 
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (irc)
> > > Tested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (irc)
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > And of course I forgot the bugzilla link
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93119
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>

Thanks. Patch pushed to dinq with cc:fixes in an effort to ensure it
ends up in the same release as the regression it fixes.

> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index 682554385631..7fae62626b71 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -78,7 +78,7 @@ static u32 g4x_infoframe_index(enum hdmi_infoframe_type type)
> > >  	case HDMI_INFOFRAME_TYPE_VENDOR:
> > >  		return VIDEO_DIP_SELECT_VENDOR;
> > >  	default:
> > > -		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> > > +		MISSING_CASE(type);
> > >  		return 0;
> > >  	}
> > >  }
> > > @@ -93,7 +93,7 @@ static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type)
> > >  	case HDMI_INFOFRAME_TYPE_VENDOR:
> > >  		return VIDEO_DIP_ENABLE_VENDOR;
> > >  	default:
> > > -		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> > > +		MISSING_CASE(type);
> > >  		return 0;
> > >  	}
> > >  }
> > > @@ -108,7 +108,7 @@ static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type)
> > >  	case HDMI_INFOFRAME_TYPE_VENDOR:
> > >  		return VIDEO_DIP_ENABLE_VS_HSW;
> > >  	default:
> > > -		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> > > +		MISSING_CASE(type);
> > >  		return 0;
> > >  	}
> > >  }
> > > @@ -127,7 +127,7 @@ hsw_dip_data_reg(struct drm_i915_private *dev_priv,
> > >  	case HDMI_INFOFRAME_TYPE_VENDOR:
> > >  		return HSW_TVIDEO_DIP_VS_DATA(cpu_transcoder, i);
> > >  	default:
> > > -		DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> > > +		MISSING_CASE(type);
> > >  		return INVALID_MMIO_REG;
> > >  	}
> > >  }
> > > @@ -375,8 +375,6 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
> > >  	u32 val = I915_READ(ctl_reg);
> > >  
> > >  	data_reg = hsw_dip_data_reg(dev_priv, cpu_transcoder, type, 0);
> > > -	if (i915_mmio_reg_valid(data_reg))
> > > -		return;
> > >  
> > >  	val &= ~hsw_infoframe_enable(type);
> > >  	I915_WRITE(ctl_reg, val);
> > > -- 
> > > 2.4.10
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx