[v2] drm/bridge:anx7625: Update HDCP status at atomic_disable()

[PATCH v2] drm/bridge:anx7625: Update HDCP status at atomic_disable()

Posted by Xin Ji 1 year ago

When user enabled HDCP feature, upper layer will set HDCP content
to DRM_MODE_CONTENT_PROTECTION_DESIRED. Next, anx7625 will update
HDCP content to DRM_MODE_CONTENT_PROTECTION_ENABLED if down stream
support HDCP feature.

However once HDCP content turn to  DRM_MODE_CONTENT_PROTECTION_ENABLED
upper layer will not update the HDCP content to
DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor disconnect.

So when user dynamic change the display resolution, anx7625 driver must
call drm_hdcp_update_content_protection() to update HDCP content to
DRM_MODE_CONTENT_PROTECTION_UNDESIRED in bridge interface
.atomic_disable().

Signed-off-by: Xin Ji <xji@analogixsemi.com>
---
 drivers/gpu/drm/bridge/analogix/anx7625.c | 25 ++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index a2675b121fe4..a75f519ddcb8 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -861,6 +861,22 @@ static int anx7625_hdcp_disable(struct anx7625_data *ctx)
 				 TX_HDCP_CTRL0, ~HARD_AUTH_EN & 0xFF);
 }
 
+static void anx7625_hdcp_disable_and_update_cp(struct anx7625_data *ctx)
+{
+	struct device *dev = ctx->dev;
+
+	if (!ctx->connector)
+		return;
+
+	anx7625_hdcp_disable(ctx);
+
+	ctx->hdcp_cp = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
+	drm_hdcp_update_content_protection(ctx->connector,
+					   ctx->hdcp_cp);
+
+	dev_dbg(dev, "update CP to UNDESIRE\n");
+}
+
 static int anx7625_hdcp_enable(struct anx7625_data *ctx)
 {
 	u8 bcap;
@@ -2165,11 +2181,8 @@ static int anx7625_connector_atomic_check(struct anx7625_data *ctx,
 			dev_err(dev, "current CP is not ENABLED\n");
 			return -EINVAL;
 		}
-		anx7625_hdcp_disable(ctx);
-		ctx->hdcp_cp = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
-		drm_hdcp_update_content_protection(ctx->connector,
-						   ctx->hdcp_cp);
-		dev_dbg(dev, "update CP to UNDESIRE\n");
+
+		anx7625_hdcp_disable_and_update_cp(ctx);
 	}
 
 	if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
@@ -2449,6 +2462,8 @@ static void anx7625_bridge_atomic_disable(struct drm_bridge *bridge,
 
 	dev_dbg(dev, "drm atomic disable\n");
 
+	anx7625_hdcp_disable_and_update_cp(ctx);
+
 	ctx->connector = NULL;
 	anx7625_dp_stop(ctx);
 
-- 
2.25.1

Re: [PATCH v2] drm/bridge:anx7625: Update HDCP status at atomic_disable()

Posted by Dmitry Baryshkov 1 year ago

On Mon, Dec 09, 2024 at 02:46:32PM +0800, Xin Ji wrote:
> When user enabled HDCP feature, upper layer will set HDCP content
> to DRM_MODE_CONTENT_PROTECTION_DESIRED. Next, anx7625 will update
> HDCP content to DRM_MODE_CONTENT_PROTECTION_ENABLED if down stream
> support HDCP feature.
> 
> However once HDCP content turn to  DRM_MODE_CONTENT_PROTECTION_ENABLED
> upper layer will not update the HDCP content to
> DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor disconnect.

What is "upper layer"? Is it a kernel or a userspace?

From drm_hdcp_update_content_protection() documentation:

No uevent for DESIRED->UNDESIRED or ENABLED->UNDESIRED,
as userspace is triggering such state change and kernel performs it without
fail.This function update the new state of the property into the connector's
state and generate an uevent to notify the userspace.


> 
> So when user dynamic change the display resolution, anx7625 driver must
> call drm_hdcp_update_content_protection() to update HDCP content to
> DRM_MODE_CONTENT_PROTECTION_UNDESIRED in bridge interface
> .atomic_disable().

Why?

> 
> Signed-off-by: Xin Ji <xji@analogixsemi.com>
> ---
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 25 ++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index a2675b121fe4..a75f519ddcb8 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -861,6 +861,22 @@ static int anx7625_hdcp_disable(struct anx7625_data *ctx)
>  				 TX_HDCP_CTRL0, ~HARD_AUTH_EN & 0xFF);
>  }
>  
> +static void anx7625_hdcp_disable_and_update_cp(struct anx7625_data *ctx)
> +{
> +	struct device *dev = ctx->dev;
> +
> +	if (!ctx->connector)
> +		return;
> +
> +	anx7625_hdcp_disable(ctx);
> +
> +	ctx->hdcp_cp = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
> +	drm_hdcp_update_content_protection(ctx->connector,
> +					   ctx->hdcp_cp);
> +
> +	dev_dbg(dev, "update CP to UNDESIRE\n");
> +}
> +
>  static int anx7625_hdcp_enable(struct anx7625_data *ctx)
>  {
>  	u8 bcap;
> @@ -2165,11 +2181,8 @@ static int anx7625_connector_atomic_check(struct anx7625_data *ctx,
>  			dev_err(dev, "current CP is not ENABLED\n");
>  			return -EINVAL;
>  		}
> -		anx7625_hdcp_disable(ctx);
> -		ctx->hdcp_cp = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
> -		drm_hdcp_update_content_protection(ctx->connector,
> -						   ctx->hdcp_cp);
> -		dev_dbg(dev, "update CP to UNDESIRE\n");
> +
> +		anx7625_hdcp_disable_and_update_cp(ctx);

No. atomic_check() MAY NOT perform any changes to the hardware. It might
be just a probe from userspace to check if the mode or a particular
option can be set in a particular way. There is no guarantee that
userspace will even try to commit it.

>  	}
>  
>  	if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> @@ -2449,6 +2462,8 @@ static void anx7625_bridge_atomic_disable(struct drm_bridge *bridge,
>  
>  	dev_dbg(dev, "drm atomic disable\n");
>  
> +	anx7625_hdcp_disable_and_update_cp(ctx);
> +
>  	ctx->connector = NULL;
>  	anx7625_dp_stop(ctx);
>  
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/bridge:anx7625: Update HDCP status at atomic_disable()

Posted by Pin-yen Lin 1 year ago

Hi Dimitry,

Thanks for the review.

On Wed, Dec 11, 2024 at 5:44 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Mon, Dec 09, 2024 at 02:46:32PM +0800, Xin Ji wrote:
> > When user enabled HDCP feature, upper layer will set HDCP content
> > to DRM_MODE_CONTENT_PROTECTION_DESIRED. Next, anx7625 will update
> > HDCP content to DRM_MODE_CONTENT_PROTECTION_ENABLED if down stream
> > support HDCP feature.
> >
> > However once HDCP content turn to  DRM_MODE_CONTENT_PROTECTION_ENABLED
> > upper layer will not update the HDCP content to
> > DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor disconnect.
>
> What is "upper layer"? Is it a kernel or a userspace?

I think Xin meant userspace, but sounds like there are some
misunderstanding around the HDCP status.
>
> >From drm_hdcp_update_content_protection() documentation:
>
> No uevent for DESIRED->UNDESIRED or ENABLED->UNDESIRED,
> as userspace is triggering such state change and kernel performs it without
> fail.This function update the new state of the property into the connector's
> state and generate an uevent to notify the userspace.
>
>
> >
> > So when user dynamic change the display resolution, anx7625 driver must
> > call drm_hdcp_update_content_protection() to update HDCP content to
> > DRM_MODE_CONTENT_PROTECTION_UNDESIRED in bridge interface
> > .atomic_disable().
>
> Why?
>
> >
> > Signed-off-by: Xin Ji <xji@analogixsemi.com>
> > ---
> >  drivers/gpu/drm/bridge/analogix/anx7625.c | 25 ++++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index a2675b121fe4..a75f519ddcb8 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -861,6 +861,22 @@ static int anx7625_hdcp_disable(struct anx7625_data *ctx)
> >                                TX_HDCP_CTRL0, ~HARD_AUTH_EN & 0xFF);
> >  }
> >
> > +static void anx7625_hdcp_disable_and_update_cp(struct anx7625_data *ctx)
> > +{
> > +     struct device *dev = ctx->dev;
> > +
> > +     if (!ctx->connector)
> > +             return;
> > +
> > +     anx7625_hdcp_disable(ctx);
> > +
> > +     ctx->hdcp_cp = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
> > +     drm_hdcp_update_content_protection(ctx->connector,
> > +                                        ctx->hdcp_cp);
> > +
> > +     dev_dbg(dev, "update CP to UNDESIRE\n");
> > +}
> > +
> >  static int anx7625_hdcp_enable(struct anx7625_data *ctx)
> >  {
> >       u8 bcap;
> > @@ -2165,11 +2181,8 @@ static int anx7625_connector_atomic_check(struct anx7625_data *ctx,
> >                       dev_err(dev, "current CP is not ENABLED\n");
> >                       return -EINVAL;
> >               }
> > -             anx7625_hdcp_disable(ctx);
> > -             ctx->hdcp_cp = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
> > -             drm_hdcp_update_content_protection(ctx->connector,
> > -                                                ctx->hdcp_cp);
> > -             dev_dbg(dev, "update CP to UNDESIRE\n");
> > +
> > +             anx7625_hdcp_disable_and_update_cp(ctx);
>
> No. atomic_check() MAY NOT perform any changes to the hardware. It might
> be just a probe from userspace to check if the mode or a particular
> option can be set in a particular way. There is no guarantee that
> userspace will even try to commit it.

So, we should move the hdcp status update from .atomic_check() to
.atomic_enable() and .atomic_disable(), right? That is, enable HDCP
for the chip at .atomic_enable() if it is DESIRED and disable it at
.atomic_disable() if we enabled it previously.

Maybe we can keep some of the checks in .atomic_check(), but I doubt
if those logics actually make sense.
>
> >       }
> >
> >       if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > @@ -2449,6 +2462,8 @@ static void anx7625_bridge_atomic_disable(struct drm_bridge *bridge,
> >
> >       dev_dbg(dev, "drm atomic disable\n");
> >
> > +     anx7625_hdcp_disable_and_update_cp(ctx);
> > +
> >       ctx->connector = NULL;
> >       anx7625_dp_stop(ctx);
> >
> > --
> > 2.25.1
> >
>
> --
> With best wishes
> Dmitry

Regards,
Pin-yen

Re: [PATCH v2] drm/bridge:anx7625: Update HDCP status at atomic_disable()

Posted by Dmitry Baryshkov 1 year ago

On Wed, Dec 11, 2024 at 11:54:54PM +0800, Pin-yen Lin wrote:
> Hi Dimitry,
> 
> Thanks for the review.
> 
> On Wed, Dec 11, 2024 at 5:44 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Mon, Dec 09, 2024 at 02:46:32PM +0800, Xin Ji wrote:
> > > When user enabled HDCP feature, upper layer will set HDCP content
> > > to DRM_MODE_CONTENT_PROTECTION_DESIRED. Next, anx7625 will update
> > > HDCP content to DRM_MODE_CONTENT_PROTECTION_ENABLED if down stream
> > > support HDCP feature.
> > >
> > > However once HDCP content turn to  DRM_MODE_CONTENT_PROTECTION_ENABLED
> > > upper layer will not update the HDCP content to
> > > DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor disconnect.
> >
> > What is "upper layer"? Is it a kernel or a userspace?
> 
> I think Xin meant userspace, but sounds like there are some
> misunderstanding around the HDCP status.
> >
> > >From drm_hdcp_update_content_protection() documentation:
> >
> > No uevent for DESIRED->UNDESIRED or ENABLED->UNDESIRED,
> > as userspace is triggering such state change and kernel performs it without
> > fail.This function update the new state of the property into the connector's
> > state and generate an uevent to notify the userspace.
> >
> >
> > >
> > > So when user dynamic change the display resolution, anx7625 driver must
> > > call drm_hdcp_update_content_protection() to update HDCP content to
> > > DRM_MODE_CONTENT_PROTECTION_UNDESIRED in bridge interface
> > > .atomic_disable().
> >
> > Why?
> >
> > >
> > > Signed-off-by: Xin Ji <xji@analogixsemi.com>
> > > ---
> > >  drivers/gpu/drm/bridge/analogix/anx7625.c | 25 ++++++++++++++++++-----
> > >  1 file changed, 20 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > index a2675b121fe4..a75f519ddcb8 100644
> > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > @@ -861,6 +861,22 @@ static int anx7625_hdcp_disable(struct anx7625_data *ctx)
> > >                                TX_HDCP_CTRL0, ~HARD_AUTH_EN & 0xFF);
> > >  }
> > >
> > > +static void anx7625_hdcp_disable_and_update_cp(struct anx7625_data *ctx)
> > > +{
> > > +     struct device *dev = ctx->dev;
> > > +
> > > +     if (!ctx->connector)
> > > +             return;
> > > +
> > > +     anx7625_hdcp_disable(ctx);
> > > +
> > > +     ctx->hdcp_cp = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
> > > +     drm_hdcp_update_content_protection(ctx->connector,
> > > +                                        ctx->hdcp_cp);
> > > +
> > > +     dev_dbg(dev, "update CP to UNDESIRE\n");
> > > +}
> > > +
> > >  static int anx7625_hdcp_enable(struct anx7625_data *ctx)
> > >  {
> > >       u8 bcap;
> > > @@ -2165,11 +2181,8 @@ static int anx7625_connector_atomic_check(struct anx7625_data *ctx,
> > >                       dev_err(dev, "current CP is not ENABLED\n");
> > >                       return -EINVAL;
> > >               }
> > > -             anx7625_hdcp_disable(ctx);
> > > -             ctx->hdcp_cp = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
> > > -             drm_hdcp_update_content_protection(ctx->connector,
> > > -                                                ctx->hdcp_cp);
> > > -             dev_dbg(dev, "update CP to UNDESIRE\n");
> > > +
> > > +             anx7625_hdcp_disable_and_update_cp(ctx);
> >
> > No. atomic_check() MAY NOT perform any changes to the hardware. It might
> > be just a probe from userspace to check if the mode or a particular
> > option can be set in a particular way. There is no guarantee that
> > userspace will even try to commit it.
> 
> So, we should move the hdcp status update from .atomic_check() to
> .atomic_enable() and .atomic_disable(), right? That is, enable HDCP
> for the chip at .atomic_enable() if it is DESIRED and disable it at
> .atomic_disable() if we enabled it previously.

This is one of the options (e.g. used by cdns-mhdp8546). Another option
(i915, amd) is to enable and disable HDCP in atomic_enable() following
selected HDCP state.

> 
> Maybe we can keep some of the checks in .atomic_check(), but I doubt
> if those logics actually make sense.

I think these checks are okay, just move the
anx7625_hdcp_disable_and_update_cp() to a proper place.

> >
> > >       }
> > >
> > >       if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > @@ -2449,6 +2462,8 @@ static void anx7625_bridge_atomic_disable(struct drm_bridge *bridge,
> > >
> > >       dev_dbg(dev, "drm atomic disable\n");
> > >
> > > +     anx7625_hdcp_disable_and_update_cp(ctx);
> > > +
> > >       ctx->connector = NULL;
> > >       anx7625_dp_stop(ctx);
> > >
> > > --
> > > 2.25.1
> > >
> >
> > --
> > With best wishes
> > Dmitry
> 
> Regards,
> Pin-yen

-- 
With best wishes
Dmitry