diff mbox series

[04/11] drm/i915: Don't WARN on HDCP toggle if get_hw_state returns false

Message ID 20191203173638.94919-5-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>

Now that we can rely on transcoder disable to toggle signalling off,
it's less of a catastrophe if get_hw_state() returns false.

Once we enable MST, this will be a valid exit path and we want to make
sure we're not spamming the logs needlessly.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ville Syrjala Dec. 5, 2019, 7:39 p.m. UTC | #1
On Tue, Dec 03, 2019 at 12:36:27PM -0500, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Now that we can rely on transcoder disable to toggle signalling off,
> it's less of a catastrophe if get_hw_state() returns false.
> 
> Once we enable MST, this will be a valid exit path and we want to make
> sure we're not spamming the logs needlessly.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index e8ac98a8ee7f..ca28913a4c9f 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1983,7 +1983,7 @@ int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder,
>  	if (WARN_ON(!wakeref))
>  		return -ENXIO;
>  
> -	if (WARN_ON(!intel_encoder->get_hw_state(intel_encoder, &pipe))) {
> +	if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) {

How can this get called when the encoder is not enabled?
Feels like this whole thing is trying to paper over some
bigger bug in the hdcp code.

>  		ret = -EIO;
>  		goto out;
>  	}
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Sean Paul Dec. 6, 2019, 1:52 p.m. UTC | #2
On Thu, Dec 05, 2019 at 09:39:35PM +0200, Ville Syrjälä wrote:
> On Tue, Dec 03, 2019 at 12:36:27PM -0500, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > Now that we can rely on transcoder disable to toggle signalling off,
> > it's less of a catastrophe if get_hw_state() returns false.
> > 
> > Once we enable MST, this will be a valid exit path and we want to make
> > sure we're not spamming the logs needlessly.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index e8ac98a8ee7f..ca28913a4c9f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -1983,7 +1983,7 @@ int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder,
> >  	if (WARN_ON(!wakeref))
> >  		return -ENXIO;
> >  
> > -	if (WARN_ON(!intel_encoder->get_hw_state(intel_encoder, &pipe))) {
> > +	if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) {
> 
> How can this get called when the encoder is not enabled?
> Feels like this whole thing is trying to paper over some
> bigger bug in the hdcp code.

In the MST patch, I've added a call to intel_hdcp_disable() in the connector
destroy path. Usually toggling will be disabled as part of the check_link call
that is initiated on unplug, so in the destroy path it's non-essential to do
this again.

Sean

> 
> >  		ret = -EIO;
> >  		goto out;
> >  	}
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjala Dec. 9, 2019, 3:21 p.m. UTC | #3
On Fri, Dec 06, 2019 at 08:52:33AM -0500, Sean Paul wrote:
> On Thu, Dec 05, 2019 at 09:39:35PM +0200, Ville Syrjälä wrote:
> > On Tue, Dec 03, 2019 at 12:36:27PM -0500, Sean Paul wrote:
> > > From: Sean Paul <seanpaul@chromium.org>
> > > 
> > > Now that we can rely on transcoder disable to toggle signalling off,
> > > it's less of a catastrophe if get_hw_state() returns false.
> > > 
> > > Once we enable MST, this will be a valid exit path and we want to make
> > > sure we're not spamming the logs needlessly.
> > > 
> > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index e8ac98a8ee7f..ca28913a4c9f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -1983,7 +1983,7 @@ int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder,
> > >  	if (WARN_ON(!wakeref))
> > >  		return -ENXIO;
> > >  
> > > -	if (WARN_ON(!intel_encoder->get_hw_state(intel_encoder, &pipe))) {
> > > +	if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) {
> > 
> > How can this get called when the encoder is not enabled?
> > Feels like this whole thing is trying to paper over some
> > bigger bug in the hdcp code.
> 
> In the MST patch, I've added a call to intel_hdcp_disable() in the connector
> destroy path. Usually toggling will be disabled as part of the check_link call
> that is initiated on unplug, so in the destroy path it's non-essential to do
> this again.

Can't we just leave things be until userspace disables the thing?
If not, then we should know whether hdcp is still enabled. And if
hdcp is enabled so is the encoder, thus we don't need such silly
checks.
Sean Paul Dec. 9, 2019, 4:16 p.m. UTC | #4
On Mon, Dec 9, 2019 at 10:21 AM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Fri, Dec 06, 2019 at 08:52:33AM -0500, Sean Paul wrote:
> > On Thu, Dec 05, 2019 at 09:39:35PM +0200, Ville Syrjälä wrote:
> > > On Tue, Dec 03, 2019 at 12:36:27PM -0500, Sean Paul wrote:
> > > > From: Sean Paul <seanpaul@chromium.org>
> > > >
> > > > Now that we can rely on transcoder disable to toggle signalling off,
> > > > it's less of a catastrophe if get_hw_state() returns false.
> > > >
> > > > Once we enable MST, this will be a valid exit path and we want to make
> > > > sure we're not spamming the logs needlessly.
> > > >
> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_ddi.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > index e8ac98a8ee7f..ca28913a4c9f 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > @@ -1983,7 +1983,7 @@ int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder,
> > > >   if (WARN_ON(!wakeref))
> > > >           return -ENXIO;
> > > >
> > > > - if (WARN_ON(!intel_encoder->get_hw_state(intel_encoder, &pipe))) {
> > > > + if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) {
> > >
> > > How can this get called when the encoder is not enabled?
> > > Feels like this whole thing is trying to paper over some
> > > bigger bug in the hdcp code.
> >
> > In the MST patch, I've added a call to intel_hdcp_disable() in the connector
> > destroy path. Usually toggling will be disabled as part of the check_link call
> > that is initiated on unplug, so in the destroy path it's non-essential to do
> > this again.
>
> Can't we just leave things be until userspace disables the thing?

The connector disappears, so userspace won't be able to disable it.

> If not, then we should know whether hdcp is still enabled. And if
> hdcp is enabled so is the encoder, thus we don't need such silly
> checks.

Alright, I'll look at taking this angle.

Sean

>
> --
> Ville Syrjälä
> Intel
Ville Syrjala Dec. 9, 2019, 5:22 p.m. UTC | #5
On Mon, Dec 09, 2019 at 11:16:27AM -0500, Sean Paul wrote:
> On Mon, Dec 9, 2019 at 10:21 AM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Fri, Dec 06, 2019 at 08:52:33AM -0500, Sean Paul wrote:
> > > On Thu, Dec 05, 2019 at 09:39:35PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Dec 03, 2019 at 12:36:27PM -0500, Sean Paul wrote:
> > > > > From: Sean Paul <seanpaul@chromium.org>
> > > > >
> > > > > Now that we can rely on transcoder disable to toggle signalling off,
> > > > > it's less of a catastrophe if get_hw_state() returns false.
> > > > >
> > > > > Once we enable MST, this will be a valid exit path and we want to make
> > > > > sure we're not spamming the logs needlessly.
> > > > >
> > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_ddi.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > index e8ac98a8ee7f..ca28913a4c9f 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > @@ -1983,7 +1983,7 @@ int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder,
> > > > >   if (WARN_ON(!wakeref))
> > > > >           return -ENXIO;
> > > > >
> > > > > - if (WARN_ON(!intel_encoder->get_hw_state(intel_encoder, &pipe))) {
> > > > > + if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) {
> > > >
> > > > How can this get called when the encoder is not enabled?
> > > > Feels like this whole thing is trying to paper over some
> > > > bigger bug in the hdcp code.
> > >
> > > In the MST patch, I've added a call to intel_hdcp_disable() in the connector
> > > destroy path. Usually toggling will be disabled as part of the check_link call
> > > that is initiated on unplug, so in the destroy path it's non-essential to do
> > > this again.
> >
> > Can't we just leave things be until userspace disables the thing?
> 
> The connector disappears, so userspace won't be able to disable it.

That would make everything broken. The connector should hang around
as a zombie until the last user disappears.

> 
> > If not, then we should know whether hdcp is still enabled. And if
> > hdcp is enabled so is the encoder, thus we don't need such silly
> > checks.
> 
> Alright, I'll look at taking this angle.
> 
> Sean
> 
> >
> > --
> > Ville Syrjälä
> > Intel
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 e8ac98a8ee7f..ca28913a4c9f 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -1983,7 +1983,7 @@  int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder,
 	if (WARN_ON(!wakeref))
 		return -ENXIO;
 
-	if (WARN_ON(!intel_encoder->get_hw_state(intel_encoder, &pipe))) {
+	if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) {
 		ret = -EIO;
 		goto out;
 	}