| 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 |
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
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
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.
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
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 --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; }