diff mbox series

[03/11] drm/i915: Disable HDCP signalling on transcoder disable

Message ID 20191203173638.94919-4-sean@poorly.run (mailing list archive)
State New, archived
Headers show
Series drm/i915: Add support for HDCP 1.4 over MST connectors | expand

Commit Message

Sean Paul Dec. 3, 2019, 5:36 p.m. UTC 
From: Sean Paul <seanpaul@chromium.org>

Currently we rely on intel_hdcp_disable() to disable HDCP signalling in
the DDI Function Control register. This patch adds a safety net by also
clearing the bit when we disable the transcoder.

Once we have HDCP over MST and disappearing connectors, we want to make
sure that the signalling is truly disabled even if HDCP teardown doesn't
go as planned.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Ville Syrjala Dec. 5, 2019, 7:33 p.m. UTC | #1 
On Tue, Dec 03, 2019 at 12:36:26PM -0500, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Currently we rely on intel_hdcp_disable() to disable HDCP signalling in
> the DDI Function Control register. This patch adds a safety net by also
> clearing the bit when we disable the transcoder.
> 
> Once we have HDCP over MST and disappearing connectors, we want to make
> sure that the signalling is truly disabled even if HDCP teardown doesn't
> go as planned.

Why wouldn't it go as planned?

> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index b51f244ad7a5..e8ac98a8ee7f 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1952,13 +1952,12 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state
>  	i915_reg_t reg = TRANS_DDI_FUNC_CTL(cpu_transcoder);
>  	u32 val = I915_READ(reg);
>  
> -	if (INTEL_GEN(dev_priv) >= 12) {
> -		val &= ~(TRANS_DDI_FUNC_ENABLE | TGL_TRANS_DDI_PORT_MASK |
> -			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
> -	} else {
> -		val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK |
> -			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
> -	}
> +	val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_DP_VC_PAYLOAD_ALLOC |
> +		 TRANS_DDI_HDCP_SIGNALLING);
> +	if (INTEL_GEN(dev_priv) >= 12)
> +		val &= ~TGL_TRANS_DDI_PORT_MASK;
> +	else
> +		val &= ~TRANS_DDI_PORT_MASK;
>  	I915_WRITE(reg, val);
>  
>  	if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME &&
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Sean Paul Dec. 6, 2019, 1:55 p.m. UTC | #2 
On Thu, Dec 05, 2019 at 09:33:19PM +0200, Ville Syrjälä wrote:
> On Tue, Dec 03, 2019 at 12:36:26PM -0500, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > Currently we rely on intel_hdcp_disable() to disable HDCP signalling in
> > the DDI Function Control register. This patch adds a safety net by also
> > clearing the bit when we disable the transcoder.
> > 
> > Once we have HDCP over MST and disappearing connectors, we want to make
> > sure that the signalling is truly disabled even if HDCP teardown doesn't
> > go as planned.
> 
> Why wouldn't it go as planned?
> 

Because things can fail in weird and wonderful ways on unplug :-)

It's a safety net. I saw this function and figured HDCP signalling should be
explicitly cleared here as well.

Sean

> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index b51f244ad7a5..e8ac98a8ee7f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -1952,13 +1952,12 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state
> >  	i915_reg_t reg = TRANS_DDI_FUNC_CTL(cpu_transcoder);
> >  	u32 val = I915_READ(reg);
> >  
> > -	if (INTEL_GEN(dev_priv) >= 12) {
> > -		val &= ~(TRANS_DDI_FUNC_ENABLE | TGL_TRANS_DDI_PORT_MASK |
> > -			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
> > -	} else {
> > -		val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK |
> > -			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
> > -	}
> > +	val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_DP_VC_PAYLOAD_ALLOC |
> > +		 TRANS_DDI_HDCP_SIGNALLING);
> > +	if (INTEL_GEN(dev_priv) >= 12)
> > +		val &= ~TGL_TRANS_DDI_PORT_MASK;
> > +	else
> > +		val &= ~TRANS_DDI_PORT_MASK;
> >  	I915_WRITE(reg, val);
> >  
> >  	if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME &&
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjala Dec. 9, 2019, 3:18 p.m. UTC | #3 
On Fri, Dec 06, 2019 at 08:55:09AM -0500, Sean Paul wrote:
> On Thu, Dec 05, 2019 at 09:33:19PM +0200, Ville Syrjälä wrote:
> > On Tue, Dec 03, 2019 at 12:36:26PM -0500, Sean Paul wrote:
> > > From: Sean Paul <seanpaul@chromium.org>
> > > 
> > > Currently we rely on intel_hdcp_disable() to disable HDCP signalling in
> > > the DDI Function Control register. This patch adds a safety net by also
> > > clearing the bit when we disable the transcoder.
> > > 
> > > Once we have HDCP over MST and disappearing connectors, we want to make
> > > sure that the signalling is truly disabled even if HDCP teardown doesn't
> > > go as planned.
> > 
> > Why wouldn't it go as planned?
> > 
> 
> Because things can fail in weird and wonderful ways on unplug :-)

Not really.

> 
> It's a safety net. I saw this function and figured HDCP signalling should be
> explicitly cleared here as well.

I call it dead and confusing code. If we get here with HDCP still
enabled we have a more serious bug somewhere else.
Sean Paul Dec. 9, 2019, 4:13 p.m. UTC | #4 
On Mon, Dec 9, 2019 at 10:18 AM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Fri, Dec 06, 2019 at 08:55:09AM -0500, Sean Paul wrote:
> > On Thu, Dec 05, 2019 at 09:33:19PM +0200, Ville Syrjälä wrote:
> > > On Tue, Dec 03, 2019 at 12:36:26PM -0500, Sean Paul wrote:
> > > > From: Sean Paul <seanpaul@chromium.org>
> > > >
> > > > Currently we rely on intel_hdcp_disable() to disable HDCP signalling in
> > > > the DDI Function Control register. This patch adds a safety net by also
> > > > clearing the bit when we disable the transcoder.
> > > >
> > > > Once we have HDCP over MST and disappearing connectors, we want to make
> > > > sure that the signalling is truly disabled even if HDCP teardown doesn't
> > > > go as planned.
> > >
> > > Why wouldn't it go as planned?
> > >
> >
> > Because things can fail in weird and wonderful ways on unplug :-)
>
> Not really.
>

That is a bold position to take, bugs happen, hardware flakes, etc.

> >
> > It's a safety net. I saw this function and figured HDCP signalling should be
> > explicitly cleared here as well.
>
> I call it dead and confusing code.

...adding a bit to an existing register clear is confusing? That might
be a touch hyperbolic.

> If we get here with HDCP still
> enabled we have a more serious bug somewhere else.
>

Ok, I suppose it's your call as to whether you take this patch, feel
free to drop.

Sean

> --
> Ville Syrjälä
> Intel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Dec. 10, 2019, 10:09 a.m. UTC | #5 
On Mon, Dec 09, 2019 at 11:13:36AM -0500, Sean Paul wrote:
> On Mon, Dec 9, 2019 at 10:18 AM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Fri, Dec 06, 2019 at 08:55:09AM -0500, Sean Paul wrote:
> > > On Thu, Dec 05, 2019 at 09:33:19PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Dec 03, 2019 at 12:36:26PM -0500, Sean Paul wrote:
> > > > > From: Sean Paul <seanpaul@chromium.org>
> > > > >
> > > > > Currently we rely on intel_hdcp_disable() to disable HDCP signalling in
> > > > > the DDI Function Control register. This patch adds a safety net by also
> > > > > clearing the bit when we disable the transcoder.
> > > > >
> > > > > Once we have HDCP over MST and disappearing connectors, we want to make
> > > > > sure that the signalling is truly disabled even if HDCP teardown doesn't
> > > > > go as planned.
> > > >
> > > > Why wouldn't it go as planned?
> > > >
> > >
> > > Because things can fail in weird and wonderful ways on unplug :-)
> >
> > Not really.
> >
> 
> That is a bold position to take, bugs happen, hardware flakes, etc.
> 
> > >
> > > It's a safety net. I saw this function and figured HDCP signalling should be
> > > explicitly cleared here as well.
> >
> > I call it dead and confusing code.
> 
> ...adding a bit to an existing register clear is confusing? That might
> be a touch hyperbolic.
> 
> > If we get here with HDCP still
> > enabled we have a more serious bug somewhere else.
> >
> 
> Ok, I suppose it's your call as to whether you take this patch, feel
> free to drop.

Maybe some expansion on this discussion here. We've had super-defensive
modeset code back 10 years ago when i915 started.

It was a mess, since all that "for safety" thing papered over real bugs,
except thanks to the safety net you mostly couldn't observe machines dying
for real. That's why we've gone pretty radical towards "you better know
what state your hw is in".

If you do want safatey, add a WARN_ON or similar that reads back hw state
and double checks it is what we think it should be. That's much better for
validating your driver than papering over all kinds of busg preemptively
and making the real ones very hard to track down.

tldr; WARN_ON hw/sw consistency safety checks good, "let me reclear/set
this" safety code bad. At least if it doesn't come with a huge WARN_ON
that things have gone terribly wrong so we can actually catch bugs in CI
and testing.
-Daniel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index b51f244ad7a5..e8ac98a8ee7f 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -1952,13 +1952,12 @@  void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state
 	i915_reg_t reg = TRANS_DDI_FUNC_CTL(cpu_transcoder);
 	u32 val = I915_READ(reg);
 
-	if (INTEL_GEN(dev_priv) >= 12) {
-		val &= ~(TRANS_DDI_FUNC_ENABLE | TGL_TRANS_DDI_PORT_MASK |
-			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
-	} else {
-		val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK |
-			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
-	}
+	val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_DP_VC_PAYLOAD_ALLOC |
+		 TRANS_DDI_HDCP_SIGNALLING);
+	if (INTEL_GEN(dev_priv) >= 12)
+		val &= ~TGL_TRANS_DDI_PORT_MASK;
+	else
+		val &= ~TRANS_DDI_PORT_MASK;
 	I915_WRITE(reg, val);
 
 	if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME &&