[Intel-gfx] [PATCH 0/4] HDCP Misc series

[Intel-gfx] [PATCH 0/4] HDCP Misc series

[Anshuman Gupta]

Test-with: <20200120085158.9151-2-anshuman.gupta@intel.com>

This series have misc fixes for broken uapi and HDCP support
for Port_E, also there are few patches for DEBUG verbosity.

Anshuman Gupta (4):
  drm/i915/hdcp: Update CP as per the kernel internal state
  drm/i915: HDCP support on above PORT_E
  drm/i915: debugfs info print "HDCP shim isn't available"
  drm/i915: Add HDCP2.2 capable debug print

 drivers/gpu/drm/i915/display/intel_display.c |  4 +++
 drivers/gpu/drm/i915/display/intel_dp.c      |  2 ++
 drivers/gpu/drm/i915/display/intel_hdcp.c    | 29 +++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_hdcp.h    |  2 ++
 drivers/gpu/drm/i915/display/intel_hdmi.c    |  2 ++
 drivers/gpu/drm/i915/i915_debugfs.c          |  5 +++-
 6 files changed, 42 insertions(+), 2 deletions(-)

-- 
2.24.0

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

[Intel-gfx] [PATCH 1/4] drm/i915/hdcp: Update CP as per the kernel internal state

[Anshuman Gupta]

Content Protection property should be updated as per the kernel
internal state. Let's say if Content protection is disabled
by userspace, CP property should be set to UNDESIRED so that
reauthentication will not happen until userspace request it again,
but when kernel disables the HDCP due to any DDI disabling sequences
like modeset/DPMS operation, kernel should set the property to
DESIRED, so that when opportunity arises, kernel will start the
HDCP authentication on its own.

Somewhere in the line, state machine to set content protection to
DESIRED from kernel was broken and IGT coverage was missing for it.
This patch fixes it.
IGT patch to catch further regression on this features is being
worked upon.

CC: Ramalingam C <ramalingam.c@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c |  4 +++
 drivers/gpu/drm/i915/display/intel_hdcp.c    | 26 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_hdcp.h    |  2 ++
 3 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index da5266e76738..934cdf1f1858 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14595,6 +14595,10 @@ static int intel_atomic_check(struct drm_device *dev,
 		goto fail;
 
 	if (any_ms) {
+		/*
+		 * When there is modeset fix the hdcp uapi CP state.
+		 */
+		intel_hdcp_post_need_modeset_check(state);
 		ret = intel_modeset_checks(state);
 		if (ret)
 			goto fail;
diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
index 0fdbd39f6641..be083136eee2 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
@@ -2074,6 +2074,32 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
 	crtc_state->mode_changed = true;
 }
 
+/**
+ * intel_hdcp_post_need_modeset_check.
+ * @state: intel atomic state.
+ *
+ * This function fix the HDCP uapi state when hdcp disabling initiated from
+ * modeset DDI disabling sequence. It updates uapi CP state from ENABLED to
+ * DESIRED so that HDCP uapi state can be restored as per HDCP Auth state.
+ * This function should be called only in case of in case of modeset.
+ * FIXME: As per HDCP content protection property uapi doc, an uevent()
+ * need to be sent if there is transition from ENABLED->DESIRED.
+ */
+void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *old_state;
+	struct drm_connector_state *new_state;
+	int i;
+
+	for_each_oldnew_connector_in_state(&state->base, connector, old_state,
+					   new_state, i) {
+		if (old_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
+		    new_state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
+			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+	}
+}
+
 /* Handles the CP_IRQ raised from the DP HDCP sink */
 void intel_hdcp_handle_cp_irq(struct intel_connector *connector)
 {
diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h
index f3c3272e712a..7bf46bc3c348 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp.h
+++ b/drivers/gpu/drm/i915/display/intel_hdcp.h
@@ -13,6 +13,7 @@
 struct drm_connector;
 struct drm_connector_state;
 struct drm_i915_private;
+struct intel_atomic_state;
 struct intel_connector;
 struct intel_hdcp_shim;
 enum port;
@@ -21,6 +22,7 @@ enum transcoder;
 void intel_hdcp_atomic_check(struct drm_connector *connector,
 			     struct drm_connector_state *old_state,
 			     struct drm_connector_state *new_state);
+void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state);
 int intel_hdcp_init(struct intel_connector *connector,
 		    const struct intel_hdcp_shim *hdcp_shim);
 int intel_hdcp_enable(struct intel_connector *connector,
-- 
2.24.0

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

Re: [Intel-gfx] [PATCH 1/4] drm/i915/hdcp: Update CP as per the kernel internal state

[Jani Nikula]

On Tue, 28 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> Content Protection property should be updated as per the kernel
> internal state. Let's say if Content protection is disabled
> by userspace, CP property should be set to UNDESIRED so that
> reauthentication will not happen until userspace request it again,
> but when kernel disables the HDCP due to any DDI disabling sequences
> like modeset/DPMS operation, kernel should set the property to
> DESIRED, so that when opportunity arises, kernel will start the
> HDCP authentication on its own.
>
> Somewhere in the line, state machine to set content protection to
> DESIRED from kernel was broken and IGT coverage was missing for it.
> This patch fixes it.
> IGT patch to catch further regression on this features is being
> worked upon.
>
> CC: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  4 +++
>  drivers/gpu/drm/i915/display/intel_hdcp.c    | 26 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_hdcp.h    |  2 ++
>  3 files changed, 32 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index da5266e76738..934cdf1f1858 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14595,6 +14595,10 @@ static int intel_atomic_check(struct drm_device *dev,
>  		goto fail;
>  
>  	if (any_ms) {
> +		/*
> +		 * When there is modeset fix the hdcp uapi CP state.
> +		 */
> +		intel_hdcp_post_need_modeset_check(state);
>  		ret = intel_modeset_checks(state);
>  		if (ret)
>  			goto fail;
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index 0fdbd39f6641..be083136eee2 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> @@ -2074,6 +2074,32 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
>  	crtc_state->mode_changed = true;
>  }
>  
> +/**
> + * intel_hdcp_post_need_modeset_check.
> + * @state: intel atomic state.
> + *
> + * This function fix the HDCP uapi state when hdcp disabling initiated from
> + * modeset DDI disabling sequence. It updates uapi CP state from ENABLED to
> + * DESIRED so that HDCP uapi state can be restored as per HDCP Auth state.
> + * This function should be called only in case of in case of modeset.
> + * FIXME: As per HDCP content protection property uapi doc, an uevent()
> + * need to be sent if there is transition from ENABLED->DESIRED.
> + */
> +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *old_state;
> +	struct drm_connector_state *new_state;
> +	int i;
> +
> +	for_each_oldnew_connector_in_state(&state->base, connector, old_state,
> +					   new_state, i) {
> +		if (old_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> +		    new_state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> +			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> +	}
> +}
> +

Why does this feel like duplication of what you already have in
intel_hdcp_atomic_check()?

BR,
Jani.


>  /* Handles the CP_IRQ raised from the DP HDCP sink */
>  void intel_hdcp_handle_cp_irq(struct intel_connector *connector)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h
> index f3c3272e712a..7bf46bc3c348 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.h
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h
> @@ -13,6 +13,7 @@
>  struct drm_connector;
>  struct drm_connector_state;
>  struct drm_i915_private;
> +struct intel_atomic_state;
>  struct intel_connector;
>  struct intel_hdcp_shim;
>  enum port;
> @@ -21,6 +22,7 @@ enum transcoder;
>  void intel_hdcp_atomic_check(struct drm_connector *connector,
>  			     struct drm_connector_state *old_state,
>  			     struct drm_connector_state *new_state);
> +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state);
>  int intel_hdcp_init(struct intel_connector *connector,
>  		    const struct intel_hdcp_shim *hdcp_shim);
>  int intel_hdcp_enable(struct intel_connector *connector,

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/i915/hdcp: Update CP as per the kernel internal state

[Anshuman Gupta]

On 2020-01-28 at 16:19:31 +0200, Jani Nikula wrote:
> On Tue, 28 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> > Content Protection property should be updated as per the kernel
> > internal state. Let's say if Content protection is disabled
> > by userspace, CP property should be set to UNDESIRED so that
> > reauthentication will not happen until userspace request it again,
> > but when kernel disables the HDCP due to any DDI disabling sequences
> > like modeset/DPMS operation, kernel should set the property to
> > DESIRED, so that when opportunity arises, kernel will start the
> > HDCP authentication on its own.
> >
> > Somewhere in the line, state machine to set content protection to
> > DESIRED from kernel was broken and IGT coverage was missing for it.
> > This patch fixes it.
> > IGT patch to catch further regression on this features is being
> > worked upon.
> >
> > CC: Ramalingam C <ramalingam.c@intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c |  4 +++
> >  drivers/gpu/drm/i915/display/intel_hdcp.c    | 26 ++++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_hdcp.h    |  2 ++
> >  3 files changed, 32 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index da5266e76738..934cdf1f1858 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14595,6 +14595,10 @@ static int intel_atomic_check(struct drm_device *dev,
> >  		goto fail;
> >  
> >  	if (any_ms) {
> > +		/*
> > +		 * When there is modeset fix the hdcp uapi CP state.
> > +		 */
> > +		intel_hdcp_post_need_modeset_check(state);
> >  		ret = intel_modeset_checks(state);
> >  		if (ret)
> >  			goto fail;
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > index 0fdbd39f6641..be083136eee2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > @@ -2074,6 +2074,32 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> >  	crtc_state->mode_changed = true;
> >  }
> >  
> > +/**
> > + * intel_hdcp_post_need_modeset_check.
> > + * @state: intel atomic state.
> > + *
> > + * This function fix the HDCP uapi state when hdcp disabling initiated from
> > + * modeset DDI disabling sequence. It updates uapi CP state from ENABLED to
> > + * DESIRED so that HDCP uapi state can be restored as per HDCP Auth state.
> > + * This function should be called only in case of in case of modeset.
> > + * FIXME: As per HDCP content protection property uapi doc, an uevent()
> > + * need to be sent if there is transition from ENABLED->DESIRED.
> > + */
> > +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state)
> > +{
> > +	struct drm_connector *connector;
> > +	struct drm_connector_state *old_state;
> > +	struct drm_connector_state *new_state;
> > +	int i;
> > +
> > +	for_each_oldnew_connector_in_state(&state->base, connector, old_state,
> > +					   new_state, i) {
> > +		if (old_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> > +		    new_state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> > +			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > +	}
> > +}
> > +
> 
> Why does this feel like duplication of what you already have in
> intel_hdcp_atomic_check()?
intel_hdcp_atomic_check() have checks that for disconnected connector and it doesn't look for 
old state, that is not sufficient to fix the hdcp CP uapi state, it need to be fix only in case of 
modeset, Later on a fastset check can disable the modeset and we would endup calling intel_hdcp_enable
while hdcp is already enabled. That is the reason i think we would require a new API to
fix the uapi state.
Thanks ,
Anshuman Gupta.
> 
> BR,
> Jani.
> 
> 
> >  /* Handles the CP_IRQ raised from the DP HDCP sink */
> >  void intel_hdcp_handle_cp_irq(struct intel_connector *connector)
> >  {
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h
> > index f3c3272e712a..7bf46bc3c348 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.h
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h
> > @@ -13,6 +13,7 @@
> >  struct drm_connector;
> >  struct drm_connector_state;
> >  struct drm_i915_private;
> > +struct intel_atomic_state;
> >  struct intel_connector;
> >  struct intel_hdcp_shim;
> >  enum port;
> > @@ -21,6 +22,7 @@ enum transcoder;
> >  void intel_hdcp_atomic_check(struct drm_connector *connector,
> >  			     struct drm_connector_state *old_state,
> >  			     struct drm_connector_state *new_state);
> > +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state);
> >  int intel_hdcp_init(struct intel_connector *connector,
> >  		    const struct intel_hdcp_shim *hdcp_shim);
> >  int intel_hdcp_enable(struct intel_connector *connector,
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/i915/hdcp: Update CP as per the kernel internal state

[Anshuman Gupta]

On 2020-01-28 at 21:14:44 +0530, Anshuman Gupta wrote:
> On 2020-01-28 at 16:19:31 +0200, Jani Nikula wrote:
> > On Tue, 28 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> > > Content Protection property should be updated as per the kernel
> > > internal state. Let's say if Content protection is disabled
> > > by userspace, CP property should be set to UNDESIRED so that
> > > reauthentication will not happen until userspace request it again,
> > > but when kernel disables the HDCP due to any DDI disabling sequences
> > > like modeset/DPMS operation, kernel should set the property to
> > > DESIRED, so that when opportunity arises, kernel will start the
> > > HDCP authentication on its own.
> > >
> > > Somewhere in the line, state machine to set content protection to
> > > DESIRED from kernel was broken and IGT coverage was missing for it.
> > > This patch fixes it.
> > > IGT patch to catch further regression on this features is being
> > > worked upon.
> > >
> > > CC: Ramalingam C <ramalingam.c@intel.com>
> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c |  4 +++
> > >  drivers/gpu/drm/i915/display/intel_hdcp.c    | 26 ++++++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_hdcp.h    |  2 ++
> > >  3 files changed, 32 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index da5266e76738..934cdf1f1858 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14595,6 +14595,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > >  		goto fail;
> > >  
> > >  	if (any_ms) {
> > > +		/*
> > > +		 * When there is modeset fix the hdcp uapi CP state.
> > > +		 */
> > > +		intel_hdcp_post_need_modeset_check(state);
> > >  		ret = intel_modeset_checks(state);
> > >  		if (ret)
> > >  			goto fail;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > index 0fdbd39f6641..be083136eee2 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > @@ -2074,6 +2074,32 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> > >  	crtc_state->mode_changed = true;
> > >  }
> > >  
> > > +/**
> > > + * intel_hdcp_post_need_modeset_check.
> > > + * @state: intel atomic state.
> > > + *
> > > + * This function fix the HDCP uapi state when hdcp disabling initiated from
> > > + * modeset DDI disabling sequence. It updates uapi CP state from ENABLED to
> > > + * DESIRED so that HDCP uapi state can be restored as per HDCP Auth state.
> > > + * This function should be called only in case of in case of modeset.
> > > + * FIXME: As per HDCP content protection property uapi doc, an uevent()
> > > + * need to be sent if there is transition from ENABLED->DESIRED.
> > > + */
> > > +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state)
> > > +{
> > > +	struct drm_connector *connector;
> > > +	struct drm_connector_state *old_state;
> > > +	struct drm_connector_state *new_state;
> > > +	int i;
> > > +
> > > +	for_each_oldnew_connector_in_state(&state->base, connector, old_state,
> > > +					   new_state, i) {
> > > +		if (old_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> > > +		    new_state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> > > +			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > > +	}
> > > +}
> > > +
> > 
> > Why does this feel like duplication of what you already have in
> > intel_hdcp_atomic_check()?
> intel_hdcp_atomic_check() have checks that for disconnected connector and it doesn't look for 
typo here, "intel_hdcp_atomic_check() checks that for disconnected connector and it doesn't check for new state shouldn't be UNDESIRED" 
> old state, that is not sufficient to fix the hdcp CP uapi state, it need to be fix only in case of
> modeset, Later on a fastset check can disable the modeset and we would endup calling intel_hdcp_enable
> while hdcp is already enabled. That is the reason i think we would require a new API to
> fix the uapi state.
> Thanks ,
> Anshuman Gupta.
> > 
> > BR,
> > Jani.
> > 
> > 
> > >  /* Handles the CP_IRQ raised from the DP HDCP sink */
> > >  void intel_hdcp_handle_cp_irq(struct intel_connector *connector)
> > >  {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h
> > > index f3c3272e712a..7bf46bc3c348 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h
> > > @@ -13,6 +13,7 @@
> > >  struct drm_connector;
> > >  struct drm_connector_state;
> > >  struct drm_i915_private;
> > > +struct intel_atomic_state;
> > >  struct intel_connector;
> > >  struct intel_hdcp_shim;
> > >  enum port;
> > > @@ -21,6 +22,7 @@ enum transcoder;
> > >  void intel_hdcp_atomic_check(struct drm_connector *connector,
> > >  			     struct drm_connector_state *old_state,
> > >  			     struct drm_connector_state *new_state);
> > > +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state);
> > >  int intel_hdcp_init(struct intel_connector *connector,
> > >  		    const struct intel_hdcp_shim *hdcp_shim);
> > >  int intel_hdcp_enable(struct intel_connector *connector,
> > 
> > -- 
> > Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/i915/hdcp: Update CP as per the kernel internal state

[Anshuman Gupta]

On 2020-01-28 at 21:45:45 +0530, Anshuman Gupta wrote:
Hi Jani ,
As per my understanding intel_hdcp_atomic_check() is not sufficient to
fix the broken hdcp uapi state, as the state fixup required in case
of modeset.
If you do not have any concern, can we continue with the patch.
Thanks,
Anshuman Gupta.
> On 2020-01-28 at 21:14:44 +0530, Anshuman Gupta wrote:
> > On 2020-01-28 at 16:19:31 +0200, Jani Nikula wrote:
> > > On Tue, 28 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> > > > Content Protection property should be updated as per the kernel
> > > > internal state. Let's say if Content protection is disabled
> > > > by userspace, CP property should be set to UNDESIRED so that
> > > > reauthentication will not happen until userspace request it again,
> > > > but when kernel disables the HDCP due to any DDI disabling sequences
> > > > like modeset/DPMS operation, kernel should set the property to
> > > > DESIRED, so that when opportunity arises, kernel will start the
> > > > HDCP authentication on its own.
> > > >
> > > > Somewhere in the line, state machine to set content protection to
> > > > DESIRED from kernel was broken and IGT coverage was missing for it.
> > > > This patch fixes it.
> > > > IGT patch to catch further regression on this features is being
> > > > worked upon.
> > > >
> > > > CC: Ramalingam C <ramalingam.c@intel.com>
> > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c |  4 +++
> > > >  drivers/gpu/drm/i915/display/intel_hdcp.c    | 26 ++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/display/intel_hdcp.h    |  2 ++
> > > >  3 files changed, 32 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index da5266e76738..934cdf1f1858 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -14595,6 +14595,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > > >  		goto fail;
> > > >  
> > > >  	if (any_ms) {
> > > > +		/*
> > > > +		 * When there is modeset fix the hdcp uapi CP state.
> > > > +		 */
> > > > +		intel_hdcp_post_need_modeset_check(state);
> > > >  		ret = intel_modeset_checks(state);
> > > >  		if (ret)
> > > >  			goto fail;
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > > index 0fdbd39f6641..be083136eee2 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > > @@ -2074,6 +2074,32 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> > > >  	crtc_state->mode_changed = true;
> > > >  }
> > > >  
> > > > +/**
> > > > + * intel_hdcp_post_need_modeset_check.
> > > > + * @state: intel atomic state.
> > > > + *
> > > > + * This function fix the HDCP uapi state when hdcp disabling initiated from
> > > > + * modeset DDI disabling sequence. It updates uapi CP state from ENABLED to
> > > > + * DESIRED so that HDCP uapi state can be restored as per HDCP Auth state.
> > > > + * This function should be called only in case of in case of modeset.
> > > > + * FIXME: As per HDCP content protection property uapi doc, an uevent()
> > > > + * need to be sent if there is transition from ENABLED->DESIRED.
> > > > + */
> > > > +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state)
> > > > +{
> > > > +	struct drm_connector *connector;
> > > > +	struct drm_connector_state *old_state;
> > > > +	struct drm_connector_state *new_state;
> > > > +	int i;
> > > > +
> > > > +	for_each_oldnew_connector_in_state(&state->base, connector, old_state,
> > > > +					   new_state, i) {
> > > > +		if (old_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> > > > +		    new_state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> > > > +			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > > > +	}
> > > > +}
> > > > +
> > > 
> > > Why does this feel like duplication of what you already have in
> > > intel_hdcp_atomic_check()?
> > intel_hdcp_atomic_check() have checks that for disconnected connector and it doesn't look for 
> typo here, "intel_hdcp_atomic_check() checks that for disconnected connector and it doesn't check for new state shouldn't be UNDESIRED" 
> > old state, that is not sufficient to fix the hdcp CP uapi state, it need to be fix only in case of
> > modeset, Later on a fastset check can disable the modeset and we would endup calling intel_hdcp_enable
> > while hdcp is already enabled. That is the reason i think we would require a new API to
> > fix the uapi state.
> > Thanks ,
> > Anshuman Gupta.
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > >  /* Handles the CP_IRQ raised from the DP HDCP sink */
> > > >  void intel_hdcp_handle_cp_irq(struct intel_connector *connector)
> > > >  {
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h
> > > > index f3c3272e712a..7bf46bc3c348 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h
> > > > @@ -13,6 +13,7 @@
> > > >  struct drm_connector;
> > > >  struct drm_connector_state;
> > > >  struct drm_i915_private;
> > > > +struct intel_atomic_state;
> > > >  struct intel_connector;
> > > >  struct intel_hdcp_shim;
> > > >  enum port;
> > > > @@ -21,6 +22,7 @@ enum transcoder;
> > > >  void intel_hdcp_atomic_check(struct drm_connector *connector,
> > > >  			     struct drm_connector_state *old_state,
> > > >  			     struct drm_connector_state *new_state);
> > > > +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state);
> > > >  int intel_hdcp_init(struct intel_connector *connector,
> > > >  		    const struct intel_hdcp_shim *hdcp_shim);
> > > >  int intel_hdcp_enable(struct intel_connector *connector,
> > > 
> > > -- 
> > > Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/i915/hdcp: Update CP as per the kernel internal state

[Maarten Lankhorst]

Op 05-02-2020 om 06:07 schreef Anshuman Gupta:
> On 2020-01-28 at 21:45:45 +0530, Anshuman Gupta wrote:
> Hi Jani ,
> As per my understanding intel_hdcp_atomic_check() is not sufficient to
> fix the broken hdcp uapi state, as the state fixup required in case
> of modeset.
> If you do not have any concern, can we continue with the patch.
> Thanks,
> Anshuman Gupta.

Hey,

In case of a modeset, don't we always call atomic_check() on the connector, either before or after?

Should be fine to fixup there then?

>> On 2020-01-28 at 21:14:44 +0530, Anshuman Gupta wrote:
>>> On 2020-01-28 at 16:19:31 +0200, Jani Nikula wrote:
>>>> On Tue, 28 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
>>>>> Content Protection property should be updated as per the kernel
>>>>> internal state. Let's say if Content protection is disabled
>>>>> by userspace, CP property should be set to UNDESIRED so that
>>>>> reauthentication will not happen until userspace request it again,
>>>>> but when kernel disables the HDCP due to any DDI disabling sequences
>>>>> like modeset/DPMS operation, kernel should set the property to
>>>>> DESIRED, so that when opportunity arises, kernel will start the
>>>>> HDCP authentication on its own.
>>>>>
>>>>> Somewhere in the line, state machine to set content protection to
>>>>> DESIRED from kernel was broken and IGT coverage was missing for it.
>>>>> This patch fixes it.
>>>>> IGT patch to catch further regression on this features is being
>>>>> worked upon.
>>>>>
>>>>> CC: Ramalingam C <ramalingam.c@intel.com>
>>>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/display/intel_display.c |  4 +++
>>>>>  drivers/gpu/drm/i915/display/intel_hdcp.c    | 26 ++++++++++++++++++++
>>>>>  drivers/gpu/drm/i915/display/intel_hdcp.h    |  2 ++
>>>>>  3 files changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>>>> index da5266e76738..934cdf1f1858 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>>> @@ -14595,6 +14595,10 @@ static int intel_atomic_check(struct drm_device *dev,
>>>>>  		goto fail;
>>>>>  
>>>>>  	if (any_ms) {
>>>>> +		/*
>>>>> +		 * When there is modeset fix the hdcp uapi CP state.
>>>>> +		 */
>>>>> +		intel_hdcp_post_need_modeset_check(state);
>>>>>  		ret = intel_modeset_checks(state);
>>>>>  		if (ret)
>>>>>  			goto fail;
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
>>>>> index 0fdbd39f6641..be083136eee2 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
>>>>> @@ -2074,6 +2074,32 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
>>>>>  	crtc_state->mode_changed = true;
>>>>>  }
>>>>>  
>>>>> +/**
>>>>> + * intel_hdcp_post_need_modeset_check.
>>>>> + * @state: intel atomic state.
>>>>> + *
>>>>> + * This function fix the HDCP uapi state when hdcp disabling initiated from
>>>>> + * modeset DDI disabling sequence. It updates uapi CP state from ENABLED to
>>>>> + * DESIRED so that HDCP uapi state can be restored as per HDCP Auth state.
>>>>> + * This function should be called only in case of in case of modeset.
>>>>> + * FIXME: As per HDCP content protection property uapi doc, an uevent()
>>>>> + * need to be sent if there is transition from ENABLED->DESIRED.
>>>>> + */
>>>>> +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state)
>>>>> +{
>>>>> +	struct drm_connector *connector;
>>>>> +	struct drm_connector_state *old_state;
>>>>> +	struct drm_connector_state *new_state;
>>>>> +	int i;
>>>>> +
>>>>> +	for_each_oldnew_connector_in_state(&state->base, connector, old_state,
>>>>> +					   new_state, i) {
>>>>> +		if (old_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
>>>>> +		    new_state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
>>>>> +			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
>>>>> +	}
>>>>> +}
>>>>> +
>>>> Why does this feel like duplication of what you already have in
>>>> intel_hdcp_atomic_check()?
>>> intel_hdcp_atomic_check() have checks that for disconnected connector and it doesn't look for 
>> typo here, "intel_hdcp_atomic_check() checks that for disconnected connector and it doesn't check for new state shouldn't be UNDESIRED" 
>>> old state, that is not sufficient to fix the hdcp CP uapi state, it need to be fix only in case of
>>> modeset, Later on a fastset check can disable the modeset and we would endup calling intel_hdcp_enable
>>> while hdcp is already enabled. That is the reason i think we would require a new API to
>>> fix the uapi state.
>>> Thanks ,
>>> Anshuman Gupta.
>>>> BR,
>>>> Jani.
>>>>
>>>>
>>>>>  /* Handles the CP_IRQ raised from the DP HDCP sink */
>>>>>  void intel_hdcp_handle_cp_irq(struct intel_connector *connector)
>>>>>  {
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h
>>>>> index f3c3272e712a..7bf46bc3c348 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.h
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h
>>>>> @@ -13,6 +13,7 @@
>>>>>  struct drm_connector;
>>>>>  struct drm_connector_state;
>>>>>  struct drm_i915_private;
>>>>> +struct intel_atomic_state;
>>>>>  struct intel_connector;
>>>>>  struct intel_hdcp_shim;
>>>>>  enum port;
>>>>> @@ -21,6 +22,7 @@ enum transcoder;
>>>>>  void intel_hdcp_atomic_check(struct drm_connector *connector,
>>>>>  			     struct drm_connector_state *old_state,
>>>>>  			     struct drm_connector_state *new_state);
>>>>> +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state);
>>>>>  int intel_hdcp_init(struct intel_connector *connector,
>>>>>  		    const struct intel_hdcp_shim *hdcp_shim);
>>>>>  int intel_hdcp_enable(struct intel_connector *connector,
>>>> -- 
>>>> Jani Nikula, Intel Open Source Graphics Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

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

Re: [Intel-gfx] [PATCH 1/4] drm/i915/hdcp: Update CP as per the kernel internal state

[Anshuman Gupta]

On 2020-03-03 at 15:36:37 +0100, Maarten Lankhorst wrote:
> Op 05-02-2020 om 06:07 schreef Anshuman Gupta:
> > On 2020-01-28 at 21:45:45 +0530, Anshuman Gupta wrote:
> > Hi Jani ,
> > As per my understanding intel_hdcp_atomic_check() is not sufficient to
> > fix the broken hdcp uapi state, as the state fixup required in case
> > of modeset.
> > If you do not have any concern, can we continue with the patch.
> > Thanks,
> > Anshuman Gupta.
> 
> Hey,
>
Thanks martin for review. 
As full modeset DDI disable sequence  (encoder->disable()->intel_hdcp_disable()) can cause HDCP to 
disable without user space knowledge i.e. when Content Protetion state is not UNDESIRED, in those cases
we want to fix the HDCP Content Protection state.  
> In case of a modeset, don't we always call atomic_check() on the connector, either before or after?
yes it calls drm_atomic_helper_check_modeset()->intel_digital_connector_atomic_check()->intel_hdcp_atomic_check(),
but if we fix HDCP state in intel_hdcp_atomic_check(), there may be a case at later point that fastset 
check is true, which disable need_modeset and enable update_pipe due to which encoder->update_pipe()->intel_hdcp_update_pipe()
may endup enabling HDCP again when HDCP is already enabled, which is wrong.
> 
> Should be fine to fixup there then?
Therefore we want to fixup the HDCP state only when full modeset is required, when it is going
to disable DDI.

Thanks ,
Anshuman Gupta.
> 
> >> On 2020-01-28 at 21:14:44 +0530, Anshuman Gupta wrote:
> >>> On 2020-01-28 at 16:19:31 +0200, Jani Nikula wrote:
> >>>> On Tue, 28 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> >>>>> Content Protection property should be updated as per the kernel
> >>>>> internal state. Let's say if Content protection is disabled
> >>>>> by userspace, CP property should be set to UNDESIRED so that
> >>>>> reauthentication will not happen until userspace request it again,
> >>>>> but when kernel disables the HDCP due to any DDI disabling sequences
> >>>>> like modeset/DPMS operation, kernel should set the property to
> >>>>> DESIRED, so that when opportunity arises, kernel will start the
> >>>>> HDCP authentication on its own.
> >>>>>
> >>>>> Somewhere in the line, state machine to set content protection to
> >>>>> DESIRED from kernel was broken and IGT coverage was missing for it.
> >>>>> This patch fixes it.
> >>>>> IGT patch to catch further regression on this features is being
> >>>>> worked upon.
> >>>>>
> >>>>> CC: Ramalingam C <ramalingam.c@intel.com>
> >>>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/i915/display/intel_display.c |  4 +++
> >>>>>  drivers/gpu/drm/i915/display/intel_hdcp.c    | 26 ++++++++++++++++++++
> >>>>>  drivers/gpu/drm/i915/display/intel_hdcp.h    |  2 ++
> >>>>>  3 files changed, 32 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >>>>> index da5266e76738..934cdf1f1858 100644
> >>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >>>>> @@ -14595,6 +14595,10 @@ static int intel_atomic_check(struct drm_device *dev,
> >>>>>  		goto fail;
> >>>>>  
> >>>>>  	if (any_ms) {
> >>>>> +		/*
> >>>>> +		 * When there is modeset fix the hdcp uapi CP state.
> >>>>> +		 */
> >>>>> +		intel_hdcp_post_need_modeset_check(state);
> >>>>>  		ret = intel_modeset_checks(state);
> >>>>>  		if (ret)
> >>>>>  			goto fail;
> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> >>>>> index 0fdbd39f6641..be083136eee2 100644
> >>>>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> >>>>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> >>>>> @@ -2074,6 +2074,32 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> >>>>>  	crtc_state->mode_changed = true;
> >>>>>  }
> >>>>>  
> >>>>> +/**
> >>>>> + * intel_hdcp_post_need_modeset_check.
> >>>>> + * @state: intel atomic state.
> >>>>> + *
> >>>>> + * This function fix the HDCP uapi state when hdcp disabling initiated from
> >>>>> + * modeset DDI disabling sequence. It updates uapi CP state from ENABLED to
> >>>>> + * DESIRED so that HDCP uapi state can be restored as per HDCP Auth state.
> >>>>> + * This function should be called only in case of in case of modeset.
> >>>>> + * FIXME: As per HDCP content protection property uapi doc, an uevent()
> >>>>> + * need to be sent if there is transition from ENABLED->DESIRED.
> >>>>> + */
> >>>>> +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state)
> >>>>> +{
> >>>>> +	struct drm_connector *connector;
> >>>>> +	struct drm_connector_state *old_state;
> >>>>> +	struct drm_connector_state *new_state;
> >>>>> +	int i;
> >>>>> +
> >>>>> +	for_each_oldnew_connector_in_state(&state->base, connector, old_state,
> >>>>> +					   new_state, i) {
> >>>>> +		if (old_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> >>>>> +		    new_state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> >>>>> +			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> >>>>> +	}
> >>>>> +}
> >>>>> +
> >>>> Why does this feel like duplication of what you already have in
> >>>> intel_hdcp_atomic_check()?
> >>> intel_hdcp_atomic_check() have checks that for disconnected connector and it doesn't look for 
> >> typo here, "intel_hdcp_atomic_check() checks that for disconnected connector and it doesn't check for new state shouldn't be UNDESIRED" 
> >>> old state, that is not sufficient to fix the hdcp CP uapi state, it need to be fix only in case of
> >>> modeset, Later on a fastset check can disable the modeset and we would endup calling intel_hdcp_enable
> >>> while hdcp is already enabled. That is the reason i think we would require a new API to
> >>> fix the uapi state.
> >>> Thanks ,
> >>> Anshuman Gupta.
> >>>> BR,
> >>>> Jani.
> >>>>
> >>>>
> >>>>>  /* Handles the CP_IRQ raised from the DP HDCP sink */
> >>>>>  void intel_hdcp_handle_cp_irq(struct intel_connector *connector)
> >>>>>  {
> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h
> >>>>> index f3c3272e712a..7bf46bc3c348 100644
> >>>>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.h
> >>>>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h
> >>>>> @@ -13,6 +13,7 @@
> >>>>>  struct drm_connector;
> >>>>>  struct drm_connector_state;
> >>>>>  struct drm_i915_private;
> >>>>> +struct intel_atomic_state;
> >>>>>  struct intel_connector;
> >>>>>  struct intel_hdcp_shim;
> >>>>>  enum port;
> >>>>> @@ -21,6 +22,7 @@ enum transcoder;
> >>>>>  void intel_hdcp_atomic_check(struct drm_connector *connector,
> >>>>>  			     struct drm_connector_state *old_state,
> >>>>>  			     struct drm_connector_state *new_state);
> >>>>> +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state);
> >>>>>  int intel_hdcp_init(struct intel_connector *connector,
> >>>>>  		    const struct intel_hdcp_shim *hdcp_shim);
> >>>>>  int intel_hdcp_enable(struct intel_connector *connector,
> >>>> -- 
> >>>> Jani Nikula, Intel Open Source Graphics Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/i915/hdcp: Update CP as per the kernel internal state

[Maarten Lankhorst]

Op 03-03-2020 om 17:35 schreef Anshuman Gupta:
> On 2020-03-03 at 15:36:37 +0100, Maarten Lankhorst wrote:
>> Op 05-02-2020 om 06:07 schreef Anshuman Gupta:
>>> On 2020-01-28 at 21:45:45 +0530, Anshuman Gupta wrote:
>>> Hi Jani ,
>>> As per my understanding intel_hdcp_atomic_check() is not sufficient to
>>> fix the broken hdcp uapi state, as the state fixup required in case
>>> of modeset.
>>> If you do not have any concern, can we continue with the patch.
>>> Thanks,
>>> Anshuman Gupta.
>> Hey,
>>
> Thanks martin for review. 
> As full modeset DDI disable sequence  (encoder->disable()->intel_hdcp_disable()) can cause HDCP to 
> disable without user space knowledge i.e. when Content Protetion state is not UNDESIRED, in those cases
> we want to fix the HDCP Content Protection state.  
You can get to crtc_state from the connector_state->crtc, should be easy to fix up this case.
>> In case of a modeset, don't we always call atomic_check() on the connector, either before or after?
> yes it calls drm_atomic_helper_check_modeset()->intel_digital_connector_atomic_check()->intel_hdcp_atomic_check(),
> but if we fix HDCP state in intel_hdcp_atomic_check(), there may be a case at later point that fastset 
> check is true, which disable need_modeset and enable update_pipe due to which encoder->update_pipe()->intel_hdcp_update_pipe()
> may endup enabling HDCP again when HDCP is already enabled, which is wrong.

Seems that if you look at the crtc_state carefully, you can prevent that. :)


~Maarten

>> Should be fine to fixup there then?
> Therefore we want to fixup the HDCP state only when full modeset is required, when it is going
> to disable DDI.
>
> Thanks ,
> Anshuman Gupta.
>>>> On 2020-01-28 at 21:14:44 +0530, Anshuman Gupta wrote:
>>>>> On 2020-01-28 at 16:19:31 +0200, Jani Nikula wrote:
>>>>>> On Tue, 28 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
>>>>>>> Content Protection property should be updated as per the kernel
>>>>>>> internal state. Let's say if Content protection is disabled
>>>>>>> by userspace, CP property should be set to UNDESIRED so that
>>>>>>> reauthentication will not happen until userspace request it again,
>>>>>>> but when kernel disables the HDCP due to any DDI disabling sequences
>>>>>>> like modeset/DPMS operation, kernel should set the property to
>>>>>>> DESIRED, so that when opportunity arises, kernel will start the
>>>>>>> HDCP authentication on its own.
>>>>>>>
>>>>>>> Somewhere in the line, state machine to set content protection to
>>>>>>> DESIRED from kernel was broken and IGT coverage was missing for it.
>>>>>>> This patch fixes it.
>>>>>>> IGT patch to catch further regression on this features is being
>>>>>>> worked upon.
>>>>>>>
>>>>>>> CC: Ramalingam C <ramalingam.c@intel.com>
>>>>>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/i915/display/intel_display.c |  4 +++
>>>>>>>  drivers/gpu/drm/i915/display/intel_hdcp.c    | 26 ++++++++++++++++++++
>>>>>>>  drivers/gpu/drm/i915/display/intel_hdcp.h    |  2 ++
>>>>>>>  3 files changed, 32 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>>>>>> index da5266e76738..934cdf1f1858 100644
>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>>>>> @@ -14595,6 +14595,10 @@ static int intel_atomic_check(struct drm_device *dev,
>>>>>>>  		goto fail;
>>>>>>>  
>>>>>>>  	if (any_ms) {
>>>>>>> +		/*
>>>>>>> +		 * When there is modeset fix the hdcp uapi CP state.
>>>>>>> +		 */
>>>>>>> +		intel_hdcp_post_need_modeset_check(state);
>>>>>>>  		ret = intel_modeset_checks(state);
>>>>>>>  		if (ret)
>>>>>>>  			goto fail;
>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
>>>>>>> index 0fdbd39f6641..be083136eee2 100644
>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
>>>>>>> @@ -2074,6 +2074,32 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
>>>>>>>  	crtc_state->mode_changed = true;
>>>>>>>  }
>>>>>>>  
>>>>>>> +/**
>>>>>>> + * intel_hdcp_post_need_modeset_check.
>>>>>>> + * @state: intel atomic state.
>>>>>>> + *
>>>>>>> + * This function fix the HDCP uapi state when hdcp disabling initiated from
>>>>>>> + * modeset DDI disabling sequence. It updates uapi CP state from ENABLED to
>>>>>>> + * DESIRED so that HDCP uapi state can be restored as per HDCP Auth state.
>>>>>>> + * This function should be called only in case of in case of modeset.
>>>>>>> + * FIXME: As per HDCP content protection property uapi doc, an uevent()
>>>>>>> + * need to be sent if there is transition from ENABLED->DESIRED.
>>>>>>> + */
>>>>>>> +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state)
>>>>>>> +{
>>>>>>> +	struct drm_connector *connector;
>>>>>>> +	struct drm_connector_state *old_state;
>>>>>>> +	struct drm_connector_state *new_state;
>>>>>>> +	int i;
>>>>>>> +
>>>>>>> +	for_each_oldnew_connector_in_state(&state->base, connector, old_state,
>>>>>>> +					   new_state, i) {
>>>>>>> +		if (old_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
>>>>>>> +		    new_state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
>>>>>>> +			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
>>>>>>> +	}
>>>>>>> +}
>>>>>>> +
>>>>>> Why does this feel like duplication of what you already have in
>>>>>> intel_hdcp_atomic_check()?
>>>>> intel_hdcp_atomic_check() have checks that for disconnected connector and it doesn't look for 
>>>> typo here, "intel_hdcp_atomic_check() checks that for disconnected connector and it doesn't check for new state shouldn't be UNDESIRED" 
>>>>> old state, that is not sufficient to fix the hdcp CP uapi state, it need to be fix only in case of
>>>>> modeset, Later on a fastset check can disable the modeset and we would endup calling intel_hdcp_enable
>>>>> while hdcp is already enabled. That is the reason i think we would require a new API to
>>>>> fix the uapi state.
>>>>> Thanks ,
>>>>> Anshuman Gupta.
>>>>>> BR,
>>>>>> Jani.
>>>>>>
>>>>>>
>>>>>>>  /* Handles the CP_IRQ raised from the DP HDCP sink */
>>>>>>>  void intel_hdcp_handle_cp_irq(struct intel_connector *connector)
>>>>>>>  {
>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h
>>>>>>> index f3c3272e712a..7bf46bc3c348 100644
>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.h
>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h
>>>>>>> @@ -13,6 +13,7 @@
>>>>>>>  struct drm_connector;
>>>>>>>  struct drm_connector_state;
>>>>>>>  struct drm_i915_private;
>>>>>>> +struct intel_atomic_state;
>>>>>>>  struct intel_connector;
>>>>>>>  struct intel_hdcp_shim;
>>>>>>>  enum port;
>>>>>>> @@ -21,6 +22,7 @@ enum transcoder;
>>>>>>>  void intel_hdcp_atomic_check(struct drm_connector *connector,
>>>>>>>  			     struct drm_connector_state *old_state,
>>>>>>>  			     struct drm_connector_state *new_state);
>>>>>>> +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state);
>>>>>>>  int intel_hdcp_init(struct intel_connector *connector,
>>>>>>>  		    const struct intel_hdcp_shim *hdcp_shim);
>>>>>>>  int intel_hdcp_enable(struct intel_connector *connector,
>>>>>> -- 
>>>>>> Jani Nikula, Intel Open Source Graphics Center
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>

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

Re: [Intel-gfx] [PATCH 1/4] drm/i915/hdcp: Update CP as per the kernel internal state

[Anshuman Gupta]

On 2020-03-04 at 09:43:10 +0100, Maarten Lankhorst wrote:
> Op 03-03-2020 om 17:35 schreef Anshuman Gupta:
> > On 2020-03-03 at 15:36:37 +0100, Maarten Lankhorst wrote:
> >> Op 05-02-2020 om 06:07 schreef Anshuman Gupta:
> >>> On 2020-01-28 at 21:45:45 +0530, Anshuman Gupta wrote:
> >>> Hi Jani ,
> >>> As per my understanding intel_hdcp_atomic_check() is not sufficient to
> >>> fix the broken hdcp uapi state, as the state fixup required in case
> >>> of modeset.
> >>> If you do not have any concern, can we continue with the patch.
> >>> Thanks,
> >>> Anshuman Gupta.
> >> Hey,
> >>
Hi Maarten,
> > Thanks martin for review. 
My apology for typo here.
> > As full modeset DDI disable sequence  (encoder->disable()->intel_hdcp_disable()) can cause HDCP to 
> > disable without user space knowledge i.e. when Content Protetion state is not UNDESIRED, in those cases
> > we want to fix the HDCP Content Protection state.  
> You can get to crtc_state from the connector_state->crtc, should be easy to fix up this case.
> >> In case of a modeset, don't we always call atomic_check() on the connector, either before or after?
> > yes it calls drm_atomic_helper_check_modeset()->intel_digital_connector_atomic_check()->intel_hdcp_atomic_check(),
> > but if we fix HDCP state in intel_hdcp_atomic_check(), there may be a case at later point that fastset 
> > check is true, which disable need_modeset and enable update_pipe due to which encoder->update_pipe()->intel_hdcp_update_pipe()
> > may endup enabling HDCP again when HDCP is already enabled, which is wrong.
> 
> Seems that if you look at the crtc_state carefully, you can prevent that. :)
If i understand correctly your suggestion to use crtc_state->active state here,
or it is some other crtc state parameter to refer.
AFAIU crtc_state->active state can also be true for any modeset request,
Please correct me if i am wrong here.
Thanks,
Anshuman.
> 
> 
> ~Maarten
> 
> >> Should be fine to fixup there then?
> > Therefore we want to fixup the HDCP state only when full modeset is required, when it is going
> > to disable DDI.
> >
> > Thanks ,
> > Anshuman Gupta.
> >>>> On 2020-01-28 at 21:14:44 +0530, Anshuman Gupta wrote:
> >>>>> On 2020-01-28 at 16:19:31 +0200, Jani Nikula wrote:
> >>>>>> On Tue, 28 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> >>>>>>> Content Protection property should be updated as per the kernel
> >>>>>>> internal state. Let's say if Content protection is disabled
> >>>>>>> by userspace, CP property should be set to UNDESIRED so that
> >>>>>>> reauthentication will not happen until userspace request it again,
> >>>>>>> but when kernel disables the HDCP due to any DDI disabling sequences
> >>>>>>> like modeset/DPMS operation, kernel should set the property to
> >>>>>>> DESIRED, so that when opportunity arises, kernel will start the
> >>>>>>> HDCP authentication on its own.
> >>>>>>>
> >>>>>>> Somewhere in the line, state machine to set content protection to
> >>>>>>> DESIRED from kernel was broken and IGT coverage was missing for it.
> >>>>>>> This patch fixes it.
> >>>>>>> IGT patch to catch further regression on this features is being
> >>>>>>> worked upon.
> >>>>>>>
> >>>>>>> CC: Ramalingam C <ramalingam.c@intel.com>
> >>>>>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> >>>>>>> ---
> >>>>>>>  drivers/gpu/drm/i915/display/intel_display.c |  4 +++
> >>>>>>>  drivers/gpu/drm/i915/display/intel_hdcp.c    | 26 ++++++++++++++++++++
> >>>>>>>  drivers/gpu/drm/i915/display/intel_hdcp.h    |  2 ++
> >>>>>>>  3 files changed, 32 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >>>>>>> index da5266e76738..934cdf1f1858 100644
> >>>>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >>>>>>> @@ -14595,6 +14595,10 @@ static int intel_atomic_check(struct drm_device *dev,
> >>>>>>>  		goto fail;
> >>>>>>>  
> >>>>>>>  	if (any_ms) {
> >>>>>>> +		/*
> >>>>>>> +		 * When there is modeset fix the hdcp uapi CP state.
> >>>>>>> +		 */
> >>>>>>> +		intel_hdcp_post_need_modeset_check(state);
> >>>>>>>  		ret = intel_modeset_checks(state);
> >>>>>>>  		if (ret)
> >>>>>>>  			goto fail;
> >>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> >>>>>>> index 0fdbd39f6641..be083136eee2 100644
> >>>>>>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> >>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> >>>>>>> @@ -2074,6 +2074,32 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> >>>>>>>  	crtc_state->mode_changed = true;
> >>>>>>>  }
> >>>>>>>  
> >>>>>>> +/**
> >>>>>>> + * intel_hdcp_post_need_modeset_check.
> >>>>>>> + * @state: intel atomic state.
> >>>>>>> + *
> >>>>>>> + * This function fix the HDCP uapi state when hdcp disabling initiated from
> >>>>>>> + * modeset DDI disabling sequence. It updates uapi CP state from ENABLED to
> >>>>>>> + * DESIRED so that HDCP uapi state can be restored as per HDCP Auth state.
> >>>>>>> + * This function should be called only in case of in case of modeset.
> >>>>>>> + * FIXME: As per HDCP content protection property uapi doc, an uevent()
> >>>>>>> + * need to be sent if there is transition from ENABLED->DESIRED.
> >>>>>>> + */
> >>>>>>> +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state)
> >>>>>>> +{
> >>>>>>> +	struct drm_connector *connector;
> >>>>>>> +	struct drm_connector_state *old_state;
> >>>>>>> +	struct drm_connector_state *new_state;
> >>>>>>> +	int i;
> >>>>>>> +
> >>>>>>> +	for_each_oldnew_connector_in_state(&state->base, connector, old_state,
> >>>>>>> +					   new_state, i) {
> >>>>>>> +		if (old_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> >>>>>>> +		    new_state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> >>>>>>> +			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> >>>>>>> +	}
> >>>>>>> +}
> >>>>>>> +
> >>>>>> Why does this feel like duplication of what you already have in
> >>>>>> intel_hdcp_atomic_check()?
> >>>>> intel_hdcp_atomic_check() have checks that for disconnected connector and it doesn't look for 
> >>>> typo here, "intel_hdcp_atomic_check() checks that for disconnected connector and it doesn't check for new state shouldn't be UNDESIRED" 
> >>>>> old state, that is not sufficient to fix the hdcp CP uapi state, it need to be fix only in case of
> >>>>> modeset, Later on a fastset check can disable the modeset and we would endup calling intel_hdcp_enable
> >>>>> while hdcp is already enabled. That is the reason i think we would require a new API to
> >>>>> fix the uapi state.
> >>>>> Thanks ,
> >>>>> Anshuman Gupta.
> >>>>>> BR,
> >>>>>> Jani.
> >>>>>>
> >>>>>>
> >>>>>>>  /* Handles the CP_IRQ raised from the DP HDCP sink */
> >>>>>>>  void intel_hdcp_handle_cp_irq(struct intel_connector *connector)
> >>>>>>>  {
> >>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h
> >>>>>>> index f3c3272e712a..7bf46bc3c348 100644
> >>>>>>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.h
> >>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h
> >>>>>>> @@ -13,6 +13,7 @@
> >>>>>>>  struct drm_connector;
> >>>>>>>  struct drm_connector_state;
> >>>>>>>  struct drm_i915_private;
> >>>>>>> +struct intel_atomic_state;
> >>>>>>>  struct intel_connector;
> >>>>>>>  struct intel_hdcp_shim;
> >>>>>>>  enum port;
> >>>>>>> @@ -21,6 +22,7 @@ enum transcoder;
> >>>>>>>  void intel_hdcp_atomic_check(struct drm_connector *connector,
> >>>>>>>  			     struct drm_connector_state *old_state,
> >>>>>>>  			     struct drm_connector_state *new_state);
> >>>>>>> +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state);
> >>>>>>>  int intel_hdcp_init(struct intel_connector *connector,
> >>>>>>>  		    const struct intel_hdcp_shim *hdcp_shim);
> >>>>>>>  int intel_hdcp_enable(struct intel_connector *connector,
> >>>>>> -- 
> >>>>>> Jani Nikula, Intel Open Source Graphics Center
> >>> _______________________________________________
> >>> Intel-gfx mailing list
> >>> Intel-gfx@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/4] drm/i915: HDCP support on above PORT_E

[Anshuman Gupta]

As Gen12 onwards there are HDCP instances for each transcoder
instead of port, remove the (port >=PORT_E) hdcp support
limitation.

CC: Ramalingam C <ramalingam.c@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/display/intel_hdcp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
index be083136eee2..d3a1dd791ff9 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
@@ -922,7 +922,8 @@ static void intel_hdcp_prop_work(struct work_struct *work)
 bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
 {
 	/* PORT E doesn't have HDCP, and PORT F is disabled */
-	return INTEL_INFO(dev_priv)->display.has_hdcp && port < PORT_E;
+	return INTEL_INFO(dev_priv)->display.has_hdcp &&
+		((INTEL_GEN(dev_priv) >= 12) || port < PORT_E);
 }
 
 static int
-- 
2.24.0

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

Re: [Intel-gfx] [PATCH 2/4] drm/i915: HDCP support on above PORT_E

[Jani Nikula]

On Tue, 28 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> As Gen12 onwards there are HDCP instances for each transcoder
> instead of port, remove the (port >=PORT_E) hdcp support
> limitation.
>
> CC: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_hdcp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index be083136eee2..d3a1dd791ff9 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> @@ -922,7 +922,8 @@ static void intel_hdcp_prop_work(struct work_struct *work)
>  bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
>  {
>  	/* PORT E doesn't have HDCP, and PORT F is disabled */
> -	return INTEL_INFO(dev_priv)->display.has_hdcp && port < PORT_E;
> +	return INTEL_INFO(dev_priv)->display.has_hdcp &&
> +		((INTEL_GEN(dev_priv) >= 12) || port < PORT_E);

Superfluous braces around (INTEL_GEN(dev_priv) >= 12).

BR,
Jani.

>  }
>  
>  static int

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/4] drm/i915: HDCP support on above PORT_E

[Jani Nikula]

On Tue, 28 Jan 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 28 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
>> As Gen12 onwards there are HDCP instances for each transcoder
>> instead of port, remove the (port >=PORT_E) hdcp support
>> limitation.
>>
>> CC: Ramalingam C <ramalingam.c@intel.com>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_hdcp.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
>> index be083136eee2..d3a1dd791ff9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
>> @@ -922,7 +922,8 @@ static void intel_hdcp_prop_work(struct work_struct *work)
>>  bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
>>  {
>>  	/* PORT E doesn't have HDCP, and PORT F is disabled */

PS. It's probably best to nuke the comment.

>> -	return INTEL_INFO(dev_priv)->display.has_hdcp && port < PORT_E;
>> +	return INTEL_INFO(dev_priv)->display.has_hdcp &&
>> +		((INTEL_GEN(dev_priv) >= 12) || port < PORT_E);
>
> Superfluous braces around (INTEL_GEN(dev_priv) >= 12).
>
> BR,
> Jani.
>
>>  }
>>  
>>  static int

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 2/4] drm/i915: HDCP support on above PORT_E

[Anshuman Gupta]

As Gen12 onwards there are HDCP instances for each transcoder
instead of port, remove the (port < PORT_E) hdcp support
limitation for platform >= Gen12.

v2:
 - Nuke the comment and cosmetic changes. [Jani]

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ramalingam C <ramalingam.c@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/display/intel_hdcp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
index be083136eee2..231b9c12c0b6 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
@@ -921,8 +921,8 @@ static void intel_hdcp_prop_work(struct work_struct *work)
 
 bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
 {
-	/* PORT E doesn't have HDCP, and PORT F is disabled */
-	return INTEL_INFO(dev_priv)->display.has_hdcp && port < PORT_E;
+	return INTEL_INFO(dev_priv)->display.has_hdcp &&
+			(INTEL_GEN(dev_priv) >= 12 || port < PORT_E);
 }
 
 static int
-- 
2.24.0

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

Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: HDCP support on above PORT_E

[Ramalingam C]

On 2020-01-29 at 18:56:19 +0530, Anshuman Gupta wrote:
> As Gen12 onwards there are HDCP instances for each transcoder
> instead of port, remove the (port < PORT_E) hdcp support
> limitation for platform >= Gen12.
> 
> v2:
>  - Nuke the comment and cosmetic changes. [Jani]
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_hdcp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index be083136eee2..231b9c12c0b6 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> @@ -921,8 +921,8 @@ static void intel_hdcp_prop_work(struct work_struct *work)
>  
>  bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
>  {
> -	/* PORT E doesn't have HDCP, and PORT F is disabled */
> -	return INTEL_INFO(dev_priv)->display.has_hdcp && port < PORT_E;
> +	return INTEL_INFO(dev_priv)->display.has_hdcp &&
> +			(INTEL_GEN(dev_priv) >= 12 || port < PORT_E);
As we discussed offline, this <PORT_E check for even <gen12 doesn't make
sense as per Bspec. I would suggest we test HDCP1.4 and 2.2 on existing
setup of <gen12 too.

Or as we could remove this limitation and address the failure cases(not
expected any as per HW spec).

For >=gen12 transcoder based hdcp instances, this check dont make any sense.

Reviewed-by: Ramalingam C <ramalingam.c@intel.com>
>  }
>  
>  static int
> -- 
> 2.24.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/4] drm/i915: debugfs info print "HDCP shim isn't available"

[Anshuman Gupta]

If HDCP shim is not initialized, i915_display_info
connector info returns EINVAL without providing any debug
information. Adding a print for that will be useful for debugging.

CC: Ramalingam C <ramalingam.c@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0ac98e39eb75..6d913a71cbdb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2405,7 +2405,8 @@ static void intel_dp_info(struct seq_file *m,
 	if (intel_connector->hdcp.shim) {
 		seq_puts(m, "\tHDCP version: ");
 		intel_hdcp_info(m, intel_connector);
-	}
+	} else if (!intel_dp_is_edp(intel_dp))
+		seq_puts(m, "\tHDCP shim isn't available\n");
 }
 
 static void intel_dp_mst_info(struct seq_file *m,
@@ -2432,6 +2433,8 @@ static void intel_hdmi_info(struct seq_file *m,
 	if (intel_connector->hdcp.shim) {
 		seq_puts(m, "\tHDCP version: ");
 		intel_hdcp_info(m, intel_connector);
+	} else {
+		seq_puts(m, "\tHDCP shim isn't available\n");
 	}
 }
 
-- 
2.24.0

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

Re: [Intel-gfx] [PATCH 3/4] drm/i915: debugfs info print "HDCP shim isn't available"

[Ramalingam C]

On 2020-01-28 at 19:24:24 +0530, Anshuman Gupta wrote:
> If HDCP shim is not initialized, i915_display_info
> connector info returns EINVAL without providing any debug
> information. Adding a print for that will be useful for debugging.
> 
> CC: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0ac98e39eb75..6d913a71cbdb 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2405,7 +2405,8 @@ static void intel_dp_info(struct seq_file *m,
>  	if (intel_connector->hdcp.shim) {
>  		seq_puts(m, "\tHDCP version: ");
>  		intel_hdcp_info(m, intel_connector);
> -	}
> +	} else if (!intel_dp_is_edp(intel_dp))
> +		seq_puts(m, "\tHDCP shim isn't available\n");
I would prefer "HDCP shim is not initialized" for this purpose.

But when this can happen on DP/HDCP connector? Either has_hdcp is false
or when port is >= E. Those info might help better for debugging.

Infact as we discussed in other patch, we need to check whether we need\
port limitation for <gen12.

-Ram
>  }
>  
>  static void intel_dp_mst_info(struct seq_file *m,
> @@ -2432,6 +2433,8 @@ static void intel_hdmi_info(struct seq_file *m,
>  	if (intel_connector->hdcp.shim) {
>  		seq_puts(m, "\tHDCP version: ");
>  		intel_hdcp_info(m, intel_connector);
> +	} else {
> +		seq_puts(m, "\tHDCP shim isn't available\n");
>  	}
>  }
>  
> -- 
> 2.24.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/4] drm/i915: Add HDCP2.2 capable debug print

[Anshuman Gupta]

Few CI panel claims to support HDCP 2.2 but at CI
HDCP IGT test execution these panels are not detecting
as HDCP 2.2 supported panels. Adding HDCP 2.2 version
print will be useful in such cases.

CC: Ramalingam C <ramalingam.c@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c   | 2 ++
 drivers/gpu/drm/i915/display/intel_hdmi.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 991f343579ef..22a3c3e9ade2 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -6449,6 +6449,8 @@ int intel_dp_hdcp2_capable(struct intel_digital_port *intel_dig_port,
 	if (ret != HDCP_2_2_RXCAPS_LEN)
 		return ret >= 0 ? -EIO : ret;
 
+	DRM_DEBUG_KMS("HDCP 2.2 RxCaps VERSION 0x%x\n", rx_caps[0]);
+
 	if (rx_caps[0] == HDCP_2_2_RX_CAPS_VERSION_VAL &&
 	    HDCP_2_2_DP_HDCP_CAPABLE(rx_caps[2]))
 		*capable = true;
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 685589064d10..a7af0be83397 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -1701,6 +1701,8 @@ int intel_hdmi_hdcp2_capable(struct intel_digital_port *intel_dig_port,
 	*capable = false;
 	ret = intel_hdmi_hdcp_read(intel_dig_port, HDCP_2_2_HDMI_REG_VER_OFFSET,
 				   &hdcp2_version, sizeof(hdcp2_version));
+	DRM_DEBUG_KMS("HDCP2Version 0%x\n", hdcp2_version);
+
 	if (!ret && hdcp2_version & HDCP_2_2_HDMI_SUPPORT_MASK)
 		*capable = true;
 
-- 
2.24.0

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

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Add HDCP2.2 capable debug print

[Jani Nikula]

On Tue, 28 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> Few CI panel claims to support HDCP 2.2 but at CI
> HDCP IGT test execution these panels are not detecting
> as HDCP 2.2 supported panels. Adding HDCP 2.2 version
> print will be useful in such cases.
>
> CC: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c   | 2 ++
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 991f343579ef..22a3c3e9ade2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -6449,6 +6449,8 @@ int intel_dp_hdcp2_capable(struct intel_digital_port *intel_dig_port,
>  	if (ret != HDCP_2_2_RXCAPS_LEN)
>  		return ret >= 0 ? -EIO : ret;
>  
> +	DRM_DEBUG_KMS("HDCP 2.2 RxCaps VERSION 0x%x\n", rx_caps[0]);

Please use drm_dbg_kms().

> +
>  	if (rx_caps[0] == HDCP_2_2_RX_CAPS_VERSION_VAL &&
>  	    HDCP_2_2_DP_HDCP_CAPABLE(rx_caps[2]))
>  		*capable = true;
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 685589064d10..a7af0be83397 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -1701,6 +1701,8 @@ int intel_hdmi_hdcp2_capable(struct intel_digital_port *intel_dig_port,
>  	*capable = false;
>  	ret = intel_hdmi_hdcp_read(intel_dig_port, HDCP_2_2_HDMI_REG_VER_OFFSET,
>  				   &hdcp2_version, sizeof(hdcp2_version));
> +	DRM_DEBUG_KMS("HDCP2Version 0%x\n", hdcp2_version);

Please use drm_dbg_kms(). Why is the previous debug "HDCP 2.2" and this
one "HDCP2Version"? You're probably missing an x in 0%x.

BR,
Jani.


> +
>  	if (!ret && hdcp2_version & HDCP_2_2_HDMI_SUPPORT_MASK)
>  		*capable = true;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Add HDCP2.2 capable debug print

[Ramalingam C]

On 2020-01-28 at 19:24:25 +0530, Anshuman Gupta wrote:
> Few CI panel claims to support HDCP 2.2 but at CI
> HDCP IGT test execution these panels are not detecting
> as HDCP 2.2 supported panels. Adding HDCP 2.2 version
> print will be useful in such cases.
> 
> CC: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c   | 2 ++
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 991f343579ef..22a3c3e9ade2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -6449,6 +6449,8 @@ int intel_dp_hdcp2_capable(struct intel_digital_port *intel_dig_port,
>  	if (ret != HDCP_2_2_RXCAPS_LEN)
>  		return ret >= 0 ? -EIO : ret;
>  
> +	DRM_DEBUG_KMS("HDCP 2.2 RxCaps VERSION 0x%x\n", rx_caps[0]);
I am not able to convince myself this is needed. After getting this
value we will manually do the operations implemented in the next lines.
So this is not adding any value.
> +
>  	if (rx_caps[0] == HDCP_2_2_RX_CAPS_VERSION_VAL &&
>  	    HDCP_2_2_DP_HDCP_CAPABLE(rx_caps[2]))
>  		*capable = true;
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 685589064d10..a7af0be83397 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -1701,6 +1701,8 @@ int intel_hdmi_hdcp2_capable(struct intel_digital_port *intel_dig_port,
>  	*capable = false;
>  	ret = intel_hdmi_hdcp_read(intel_dig_port, HDCP_2_2_HDMI_REG_VER_OFFSET,
>  				   &hdcp2_version, sizeof(hdcp2_version));
> +	DRM_DEBUG_KMS("HDCP2Version 0%x\n", hdcp2_version);
Same here too.

Ram.
> +
>  	if (!ret && hdcp2_version & HDCP_2_2_HDMI_SUPPORT_MASK)
>  		*capable = true;
>  
> -- 
> 2.24.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx