[v3] drm/bridge:anx7625: Update HDCP status at atomic_enable()

Commit Message

Xin Ji Dec. 12, 2024, 5:51 a.m. UTC

When user enabled HDCP feature, userspace 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
userspace will not update the HDCP content to
DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor disconnect.

So, anx7625 driver move hdcp content value checking from bridge
interface .atomic_check() to .atomic_enable(), then update hdcp content
according from currently HDCP status. And also disabled HDCP in bridge
interface .atomic_disable().

Signed-off-by: Xin Ji <[e-mail]>
---
 drivers/gpu/drm/bridge/analogix/anx7625.c | 74 ++++++++++++++---------
 1 file changed, 46 insertions(+), 28 deletions(-)

Comments

Chen-Yu Tsai Dec. 12, 2024, 7:29 a.m. UTC | #1

On Thu, Dec 12, 2024 at 1:51 PM Xin Ji <[e-mail]> wrote:
>
> When user enabled HDCP feature, userspace 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
> userspace will not update the HDCP content to
> DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor disconnect.
>
> So, anx7625 driver move hdcp content value checking from bridge
> interface .atomic_check() to .atomic_enable(), then update hdcp content
> according from currently HDCP status. And also disabled HDCP in bridge
> interface .atomic_disable().
>
> Signed-off-by: Xin Ji <[e-mail]>
> ---

No need to resend, but please provide a changelog under the "---" line
so reviewers know what you changed in this new version.

>  drivers/gpu/drm/bridge/analogix/anx7625.c | 74 ++++++++++++++---------
>  1 file changed, 46 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index a2675b121fe4..f96ce5665e8d 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;
> @@ -2149,34 +2165,6 @@ static int anx7625_connector_atomic_check(struct anx7625_data *ctx,
>         if (cp == ctx->hdcp_cp)
>                 return 0;
>
> -       if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> -               if (ctx->dp_en) {
> -                       dev_dbg(dev, "enable HDCP\n");
> -                       anx7625_hdcp_enable(ctx);
> -
> -                       queue_delayed_work(ctx->hdcp_workqueue,
> -                                          &ctx->hdcp_work,
> -                                          msecs_to_jiffies(2000));
> -               }
> -       }
> -
> -       if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> -               if (ctx->hdcp_cp != DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> -                       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");
> -       }
> -
> -       if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> -               dev_err(dev, "Userspace illegal set to PROTECTION ENABLE\n");
> -               return -EINVAL;
> -       }
> -
>         return 0;
>  }
>
> @@ -2425,6 +2413,8 @@ static void anx7625_bridge_atomic_enable(struct drm_bridge *bridge,
>         struct anx7625_data *ctx = bridge_to_anx7625(bridge);
>         struct device *dev = ctx->dev;
>         struct drm_connector *connector;
> +       struct drm_connector_state *conn_state;
> +       int cp;
>
>         dev_dbg(dev, "drm atomic enable\n");
>
> @@ -2439,6 +2429,32 @@ static void anx7625_bridge_atomic_enable(struct drm_bridge *bridge,
>         _anx7625_hpd_polling(ctx, 5000 * 100);
>
>         anx7625_dp_start(ctx);
> +
> +       conn_state = drm_atomic_get_new_connector_state(state->base.state, connector);
> +
> +       if (WARN_ON(!conn_state))
> +               return;
> +
> +       cp = conn_state->content_protection;
> +       if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> +               if (ctx->dp_en) {
> +                       dev_dbg(dev, "enable HDCP\n");
> +                       anx7625_hdcp_enable(ctx);
> +
> +                       queue_delayed_work(ctx->hdcp_workqueue,
> +                                          &ctx->hdcp_work,
> +                                          msecs_to_jiffies(2000));
> +               }
> +       }
> +
> +       if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> +               if (ctx->hdcp_cp != DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> +                       dev_err(dev, "current CP is not ENABLED\n");
> +                       return;
> +               }
> +
> +               anx7625_hdcp_disable_and_update_cp(ctx);
> +       }
>  }
>
>  static void anx7625_bridge_atomic_disable(struct drm_bridge *bridge,
> @@ -2449,6 +2465,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
>

Dmitry Baryshkov Dec. 12, 2024, 9:18 a.m. UTC | #2

On Thu, Dec 12, 2024 at 01:51:10PM +0800, Xin Ji wrote:
> When user enabled HDCP feature, userspace 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
> userspace will not update the HDCP content to
> DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor disconnect.

It seems you've ingored a part of the previous review comment. It's the
userspace who triggers the ENABLED -> UNDESIRED transition, not the
kernel side. The change to move HDCP handling to atomic_enable() looks
fine, the change to disable HDCP is not (unless I misunderstand
something).

> 
> So, anx7625 driver move hdcp content value checking from bridge
> interface .atomic_check() to .atomic_enable(), then update hdcp content
> according from currently HDCP status. And also disabled HDCP in bridge
> interface .atomic_disable().
> 
> Signed-off-by: Xin Ji <[e-mail]>
> ---
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 74 ++++++++++++++---------
>  1 file changed, 46 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index a2675b121fe4..f96ce5665e8d 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;
> @@ -2149,34 +2165,6 @@ static int anx7625_connector_atomic_check(struct anx7625_data *ctx,
>  	if (cp == ctx->hdcp_cp)
>  		return 0;
>  
> -	if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> -		if (ctx->dp_en) {
> -			dev_dbg(dev, "enable HDCP\n");
> -			anx7625_hdcp_enable(ctx);
> -
> -			queue_delayed_work(ctx->hdcp_workqueue,
> -					   &ctx->hdcp_work,
> -					   msecs_to_jiffies(2000));
> -		}
> -	}
> -
> -	if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> -		if (ctx->hdcp_cp != DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> -			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");
> -	}
> -
> -	if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> -		dev_err(dev, "Userspace illegal set to PROTECTION ENABLE\n");
> -		return -EINVAL;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -2425,6 +2413,8 @@ static void anx7625_bridge_atomic_enable(struct drm_bridge *bridge,
>  	struct anx7625_data *ctx = bridge_to_anx7625(bridge);
>  	struct device *dev = ctx->dev;
>  	struct drm_connector *connector;
> +	struct drm_connector_state *conn_state;
> +	int cp;
>  
>  	dev_dbg(dev, "drm atomic enable\n");
>  
> @@ -2439,6 +2429,32 @@ static void anx7625_bridge_atomic_enable(struct drm_bridge *bridge,
>  	_anx7625_hpd_polling(ctx, 5000 * 100);
>  
>  	anx7625_dp_start(ctx);
> +
> +	conn_state = drm_atomic_get_new_connector_state(state->base.state, connector);
> +
> +	if (WARN_ON(!conn_state))
> +		return;
> +
> +	cp = conn_state->content_protection;
> +	if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> +		if (ctx->dp_en) {
> +			dev_dbg(dev, "enable HDCP\n");
> +			anx7625_hdcp_enable(ctx);
> +
> +			queue_delayed_work(ctx->hdcp_workqueue,
> +					   &ctx->hdcp_work,
> +					   msecs_to_jiffies(2000));
> +		}
> +	}
> +
> +	if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> +		if (ctx->hdcp_cp != DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> +			dev_err(dev, "current CP is not ENABLED\n");
> +			return;
> +		}
> +
> +		anx7625_hdcp_disable_and_update_cp(ctx);
> +	}
>  }
>  
>  static void anx7625_bridge_atomic_disable(struct drm_bridge *bridge,
> @@ -2449,6 +2465,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
>

Xin Ji Dec. 13, 2024, 10:06 a.m. UTC | #3

Hi Dmitry, thanks for the review, I made some changes which change ENABLE to DESIRE
in .atomic_disable(), I'll upstream it after testing. Thanks!

> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: Thursday, December 12, 2024 5:18 PM
> To: Xin Ji <[e-mail]>
> Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> <[e-mail]>; Robert Foss <[e-mail]>; Laurent Pinchart
> <[e-mail]>; Jonas Karlman <[e-mail]>;
> Jernej Skrabec <[e-mail]>; Maarten Lankhorst
> <[e-mail]>; Maxime Ripard <[e-mail]>;
> Thomas Zimmermann <[e-mail]>; David Airlie
> <[e-mail]>; Simona Vetter <[e-mail]>; Bernie Liang
> <[e-mail]>; Qilin Wen <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> atomic_enable()
> 
> CAUTION: This email originated from outside of the organization. Please do not
> click links or open attachments unless you recognize the sender, and know the
> content is safe.
> 
> 
> On Thu, Dec 12, 2024 at 01:51:10PM +0800, Xin Ji wrote:
> > When user enabled HDCP feature, userspace 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
> > userspace will not update the HDCP content to
> > DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor disconnect.
> 
> It seems you've ingored a part of the previous review comment. It's the
> userspace who triggers the ENABLED -> UNDESIRED transition, not the kernel
> side. The change to move HDCP handling to atomic_enable() looks fine, the
> change to disable HDCP is not (unless I misunderstand something).
> 
> >
> > So, anx7625 driver move hdcp content value checking from bridge
> > interface .atomic_check() to .atomic_enable(), then update hdcp
> > content according from currently HDCP status. And also disabled HDCP
> > in bridge interface .atomic_disable().
> >
> > Signed-off-by: Xin Ji <[e-mail]>
> > ---
> >  drivers/gpu/drm/bridge/analogix/anx7625.c | 74
> > ++++++++++++++---------
> >  1 file changed, 46 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index a2675b121fe4..f96ce5665e8d 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;
> > @@ -2149,34 +2165,6 @@ static int anx7625_connector_atomic_check(struct
> anx7625_data *ctx,
> >       if (cp == ctx->hdcp_cp)
> >               return 0;
> >
> > -     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > -             if (ctx->dp_en) {
> > -                     dev_dbg(dev, "enable HDCP\n");
> > -                     anx7625_hdcp_enable(ctx);
> > -
> > -                     queue_delayed_work(ctx->hdcp_workqueue,
> > -                                        &ctx->hdcp_work,
> > -                                        msecs_to_jiffies(2000));
> > -             }
> > -     }
> > -
> > -     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > -             if (ctx->hdcp_cp != DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > -                     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");
> > -     }
> > -
> > -     if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > -             dev_err(dev, "Userspace illegal set to PROTECTION ENABLE\n");
> > -             return -EINVAL;
> > -     }
> > -
> >       return 0;
> >  }
> >
> > @@ -2425,6 +2413,8 @@ static void anx7625_bridge_atomic_enable(struct
> drm_bridge *bridge,
> >       struct anx7625_data *ctx = bridge_to_anx7625(bridge);
> >       struct device *dev = ctx->dev;
> >       struct drm_connector *connector;
> > +     struct drm_connector_state *conn_state;
> > +     int cp;
> >
> >       dev_dbg(dev, "drm atomic enable\n");
> >
> > @@ -2439,6 +2429,32 @@ static void anx7625_bridge_atomic_enable(struct
> drm_bridge *bridge,
> >       _anx7625_hpd_polling(ctx, 5000 * 100);
> >
> >       anx7625_dp_start(ctx);
> > +
> > +     conn_state =
> > + drm_atomic_get_new_connector_state(state->base.state, connector);
> > +
> > +     if (WARN_ON(!conn_state))
> > +             return;
> > +
> > +     cp = conn_state->content_protection;
> > +     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > +             if (ctx->dp_en) {
> > +                     dev_dbg(dev, "enable HDCP\n");
> > +                     anx7625_hdcp_enable(ctx);
> > +
> > +                     queue_delayed_work(ctx->hdcp_workqueue,
> > +                                        &ctx->hdcp_work,
> > +                                        msecs_to_jiffies(2000));
> > +             }
> > +     }
> > +
> > +     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > +             if (ctx->hdcp_cp != DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > +                     dev_err(dev, "current CP is not ENABLED\n");
> > +                     return;
> > +             }
> > +
> > +             anx7625_hdcp_disable_and_update_cp(ctx);
> > +     }
> >  }
> >
> >  static void anx7625_bridge_atomic_disable(struct drm_bridge *bridge,
> > @@ -2449,6 +2465,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

Dmitry Baryshkov Dec. 13, 2024, 10:46 a.m. UTC | #4

On Fri, Dec 13, 2024 at 10:06:36AM +0000, Xin Ji wrote:
> Hi Dmitry, thanks for the review, I made some changes which change ENABLE to DESIRE
> in .atomic_disable(), I'll upstream it after testing. Thanks!

- Please don't top-post.

- You still didn't explain, why do you want to do this change of HDCP
  status. Could you please provide an explanation before sending the
  next iteration?

> 
> > -----Original Message-----
> > From: Dmitry Baryshkov <[e-mail]>
> > Sent: Thursday, December 12, 2024 5:18 PM
> > To: Xin Ji <[e-mail]>
> > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > <[e-mail]>; Robert Foss <[e-mail]>; Laurent Pinchart
> > <[e-mail]>; Jonas Karlman <[e-mail]>;
> > Jernej Skrabec <[e-mail]>; Maarten Lankhorst
> > <[e-mail]>; Maxime Ripard <[e-mail]>;
> > Thomas Zimmermann <[e-mail]>; David Airlie
> > <[e-mail]>; Simona Vetter <[e-mail]>; Bernie Liang
> > <[e-mail]>; Qilin Wen <[e-mail]>;
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > atomic_enable()
> > 
> > CAUTION: This email originated from outside of the organization. Please do not
> > click links or open attachments unless you recognize the sender, and know the
> > content is safe.
> > 
> > 
> > On Thu, Dec 12, 2024 at 01:51:10PM +0800, Xin Ji wrote:
> > > When user enabled HDCP feature, userspace 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
> > > userspace will not update the HDCP content to
> > > DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor disconnect.
> > 
> > It seems you've ingored a part of the previous review comment. It's the
> > userspace who triggers the ENABLED -> UNDESIRED transition, not the kernel
> > side. The change to move HDCP handling to atomic_enable() looks fine, the
> > change to disable HDCP is not (unless I misunderstand something).
> > 
> > >
> > > So, anx7625 driver move hdcp content value checking from bridge
> > > interface .atomic_check() to .atomic_enable(), then update hdcp
> > > content according from currently HDCP status. And also disabled HDCP
> > > in bridge interface .atomic_disable().
> > >
> > > Signed-off-by: Xin Ji <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/bridge/analogix/anx7625.c | 74
> > > ++++++++++++++---------
> > >  1 file changed, 46 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > index a2675b121fe4..f96ce5665e8d 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;
> > > @@ -2149,34 +2165,6 @@ static int anx7625_connector_atomic_check(struct
> > anx7625_data *ctx,
> > >       if (cp == ctx->hdcp_cp)
> > >               return 0;
> > >
> > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > -             if (ctx->dp_en) {
> > > -                     dev_dbg(dev, "enable HDCP\n");
> > > -                     anx7625_hdcp_enable(ctx);
> > > -
> > > -                     queue_delayed_work(ctx->hdcp_workqueue,
> > > -                                        &ctx->hdcp_work,
> > > -                                        msecs_to_jiffies(2000));
> > > -             }
> > > -     }
> > > -
> > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > -             if (ctx->hdcp_cp != DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > -                     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");
> > > -     }
> > > -
> > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > -             dev_err(dev, "Userspace illegal set to PROTECTION ENABLE\n");
> > > -             return -EINVAL;
> > > -     }
> > > -
> > >       return 0;
> > >  }
> > >
> > > @@ -2425,6 +2413,8 @@ static void anx7625_bridge_atomic_enable(struct
> > drm_bridge *bridge,
> > >       struct anx7625_data *ctx = bridge_to_anx7625(bridge);
> > >       struct device *dev = ctx->dev;
> > >       struct drm_connector *connector;
> > > +     struct drm_connector_state *conn_state;
> > > +     int cp;
> > >
> > >       dev_dbg(dev, "drm atomic enable\n");
> > >
> > > @@ -2439,6 +2429,32 @@ static void anx7625_bridge_atomic_enable(struct
> > drm_bridge *bridge,
> > >       _anx7625_hpd_polling(ctx, 5000 * 100);
> > >
> > >       anx7625_dp_start(ctx);
> > > +
> > > +     conn_state =
> > > + drm_atomic_get_new_connector_state(state->base.state, connector);
> > > +
> > > +     if (WARN_ON(!conn_state))
> > > +             return;
> > > +
> > > +     cp = conn_state->content_protection;
> > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > +             if (ctx->dp_en) {
> > > +                     dev_dbg(dev, "enable HDCP\n");
> > > +                     anx7625_hdcp_enable(ctx);
> > > +
> > > +                     queue_delayed_work(ctx->hdcp_workqueue,
> > > +                                        &ctx->hdcp_work,
> > > +                                        msecs_to_jiffies(2000));
> > > +             }
> > > +     }
> > > +
> > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > +             if (ctx->hdcp_cp != DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > +                     dev_err(dev, "current CP is not ENABLED\n");
> > > +                     return;
> > > +             }
> > > +
> > > +             anx7625_hdcp_disable_and_update_cp(ctx);
> > > +     }
> > >  }
> > >
> > >  static void anx7625_bridge_atomic_disable(struct drm_bridge *bridge,
> > > @@ -2449,6 +2465,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

Xin Ji Dec. 13, 2024, 11 a.m. UTC | #5

Hi Dmitry, sorry, I didn't clear describe the reason.

Anx7625 implement DSI to DP convert behind USB Type-C port, when user plug into USB Type-C
Dongle with DP monitor, the user space will enable HDCP feature, then kernel do HDCP and
output display and set HDCP content to ENABLE, but the issue happened if user manually
change the monitor's resolution later.

Each time user change the resolution, kernel will call bridge interface .atomic_disable() and
.atomic_enable(), the old driver will keep HDCP state to ENABLE, this is a BUG, when user
change the resolution, kernel must change HDCP content too (mustn't keep to ENABLE),
as DRM doc said, kernel cannot change from ENABLE to UNDESIRE, so next patch,
I'll change it to DESIRE in .atomic_disable().

Thanks!
Xin

> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: Friday, December 13, 2024 6:47 PM
> To: Xin Ji <[e-mail]>
> Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> <[e-mail]>; Robert Foss <[e-mail]>; Laurent Pinchart
> <[e-mail]>; Jonas Karlman <[e-mail]>;
> Jernej Skrabec <[e-mail]>; Maarten Lankhorst
> <[e-mail]>; Maxime Ripard <[e-mail]>;
> Thomas Zimmermann <[e-mail]>; David Airlie
> <[e-mail]>; Simona Vetter <[e-mail]>; Bernie Liang
> <[e-mail]>; Qilin Wen <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> atomic_enable()
> 
> CAUTION: This email originated from outside of the organization. Please do not
> click links or open attachments unless you recognize the sender, and know the
> content is safe.
> 
> 
> On Fri, Dec 13, 2024 at 10:06:36AM +0000, Xin Ji wrote:
> > Hi Dmitry, thanks for the review, I made some changes which change
> > ENABLE to DESIRE in .atomic_disable(), I'll upstream it after testing. Thanks!
> 
> - Please don't top-post.
> 
> - You still didn't explain, why do you want to do this change of HDCP
>   status. Could you please provide an explanation before sending the
>   next iteration?
> 
> >
> > > -----Original Message-----
> > > From: Dmitry Baryshkov <[e-mail]>
> > > Sent: Thursday, December 12, 2024 5:18 PM
> > > To: Xin Ji <[e-mail]>
> > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > <[e-mail]>; Robert Foss <[e-mail]>; Laurent
> > > Pinchart <[e-mail]>; Jonas Karlman
> > > <[e-mail]>; Jernej Skrabec <[e-mail]>;
> > > Maarten Lankhorst <[e-mail]>; Maxime Ripard
> > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> David
> > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>; Bernie
> > > Liang <[e-mail]>; Qilin Wen <[e-mail]>;
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]
> > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > > atomic_enable()
> > >
> > > CAUTION: This email originated from outside of the organization.
> > > Please do not click links or open attachments unless you recognize
> > > the sender, and know the content is safe.
> > >
> > >
> > > On Thu, Dec 12, 2024 at 01:51:10PM +0800, Xin Ji wrote:
> > > > When user enabled HDCP feature, userspace 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
> > > > userspace will not update the HDCP content to
> > > > DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor
> disconnect.
> > >
> > > It seems you've ingored a part of the previous review comment. It's
> > > the userspace who triggers the ENABLED -> UNDESIRED transition, not
> > > the kernel side. The change to move HDCP handling to atomic_enable()
> > > looks fine, the change to disable HDCP is not (unless I misunderstand
> something).
> > >
> > > >
> > > > So, anx7625 driver move hdcp content value checking from bridge
> > > > interface .atomic_check() to .atomic_enable(), then update hdcp
> > > > content according from currently HDCP status. And also disabled
> > > > HDCP in bridge interface .atomic_disable().
> > > >
> > > > Signed-off-by: Xin Ji <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/bridge/analogix/anx7625.c | 74
> > > > ++++++++++++++---------
> > > >  1 file changed, 46 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > index a2675b121fe4..f96ce5665e8d 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;
> > > > @@ -2149,34 +2165,6 @@ static int
> > > > anx7625_connector_atomic_check(struct
> > > anx7625_data *ctx,
> > > >       if (cp == ctx->hdcp_cp)
> > > >               return 0;
> > > >
> > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > > -             if (ctx->dp_en) {
> > > > -                     dev_dbg(dev, "enable HDCP\n");
> > > > -                     anx7625_hdcp_enable(ctx);
> > > > -
> > > > -                     queue_delayed_work(ctx->hdcp_workqueue,
> > > > -                                        &ctx->hdcp_work,
> > > > -                                        msecs_to_jiffies(2000));
> > > > -             }
> > > > -     }
> > > > -
> > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > -             if (ctx->hdcp_cp !=
> DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > -                     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");
> > > > -     }
> > > > -
> > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > -             dev_err(dev, "Userspace illegal set to PROTECTION ENABLE\n");
> > > > -             return -EINVAL;
> > > > -     }
> > > > -
> > > >       return 0;
> > > >  }
> > > >
> > > > @@ -2425,6 +2413,8 @@ static void
> > > > anx7625_bridge_atomic_enable(struct
> > > drm_bridge *bridge,
> > > >       struct anx7625_data *ctx = bridge_to_anx7625(bridge);
> > > >       struct device *dev = ctx->dev;
> > > >       struct drm_connector *connector;
> > > > +     struct drm_connector_state *conn_state;
> > > > +     int cp;
> > > >
> > > >       dev_dbg(dev, "drm atomic enable\n");
> > > >
> > > > @@ -2439,6 +2429,32 @@ static void
> > > > anx7625_bridge_atomic_enable(struct
> > > drm_bridge *bridge,
> > > >       _anx7625_hpd_polling(ctx, 5000 * 100);
> > > >
> > > >       anx7625_dp_start(ctx);
> > > > +
> > > > +     conn_state =
> > > > + drm_atomic_get_new_connector_state(state->base.state,
> > > > + connector);
> > > > +
> > > > +     if (WARN_ON(!conn_state))
> > > > +             return;
> > > > +
> > > > +     cp = conn_state->content_protection;
> > > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > > +             if (ctx->dp_en) {
> > > > +                     dev_dbg(dev, "enable HDCP\n");
> > > > +                     anx7625_hdcp_enable(ctx);
> > > > +
> > > > +                     queue_delayed_work(ctx->hdcp_workqueue,
> > > > +                                        &ctx->hdcp_work,
> > > > +                                        msecs_to_jiffies(2000));
> > > > +             }
> > > > +     }
> > > > +
> > > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > +             if (ctx->hdcp_cp !=
> DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > +                     dev_err(dev, "current CP is not ENABLED\n");
> > > > +                     return;
> > > > +             }
> > > > +
> > > > +             anx7625_hdcp_disable_and_update_cp(ctx);
> > > > +     }
> > > >  }
> > > >
> > > >  static void anx7625_bridge_atomic_disable(struct drm_bridge
> > > > *bridge, @@ -2449,6 +2465,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
> 
> --
> With best wishes
> Dmitry

Dmitry Baryshkov Dec. 13, 2024, 1:16 p.m. UTC | #6

On Fri, 13 Dec 2024 at 13:00, Xin Ji <[e-mail]> wrote:
>
> Hi Dmitry, sorry, I didn't clear describe the reason.

Please. Do not top-post. Please paste your answer under the question,
not somewhere at the top of the email. This allows us to have a more
constructive dialog. Additional bonus if you can fix your email client
to insert sensible quoting information instead of dumping the headers
of the original email.

>
> Anx7625 implement DSI to DP convert behind USB Type-C port, when user plug into USB Type-C
> Dongle with DP monitor, the user space will enable HDCP feature, then kernel do HDCP and
> output display and set HDCP content to ENABLE, but the issue happened if user manually
> change the monitor's resolution later.
>
> Each time user change the resolution, kernel will call bridge interface .atomic_disable() and
> .atomic_enable(), the old driver will keep HDCP state to ENABLE, this is a BUG, when user
> change the resolution, kernel must change HDCP content too (mustn't keep to ENABLE),

Why? Could you please point me to the corresponding documentation or a
code path in the other driver? Preferably i915, AMD or Nouveau.

> as DRM doc said, kernel cannot change from ENABLE to UNDESIRE, so next patch,
> I'll change it to DESIRE in .atomic_disable().
>
> Thanks!
> Xin
>
> > -----Original Message-----
> > From: Dmitry Baryshkov <[e-mail]>
> > Sent: Friday, December 13, 2024 6:47 PM
> > To: Xin Ji <[e-mail]>
> > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > <[e-mail]>; Robert Foss <[e-mail]>; Laurent Pinchart
> > <[e-mail]>; Jonas Karlman <[e-mail]>;
> > Jernej Skrabec <[e-mail]>; Maarten Lankhorst
> > <[e-mail]>; Maxime Ripard <[e-mail]>;
> > Thomas Zimmermann <[e-mail]>; David Airlie
> > <[e-mail]>; Simona Vetter <[e-mail]>; Bernie Liang
> > <[e-mail]>; Qilin Wen <[e-mail]>;
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > atomic_enable()
> >
> > CAUTION: This email originated from outside of the organization. Please do not
> > click links or open attachments unless you recognize the sender, and know the
> > content is safe.
> >
> >
> > On Fri, Dec 13, 2024 at 10:06:36AM +0000, Xin Ji wrote:
> > > Hi Dmitry, thanks for the review, I made some changes which change
> > > ENABLE to DESIRE in .atomic_disable(), I'll upstream it after testing. Thanks!
> >
> > - Please don't top-post.
> >
> > - You still didn't explain, why do you want to do this change of HDCP
> >   status. Could you please provide an explanation before sending the
> >   next iteration?
> >
> > >
> > > > -----Original Message-----
> > > > From: Dmitry Baryshkov <[e-mail]>
> > > > Sent: Thursday, December 12, 2024 5:18 PM
> > > > To: Xin Ji <[e-mail]>
> > > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > > <[e-mail]>; Robert Foss <[e-mail]>; Laurent
> > > > Pinchart <[e-mail]>; Jonas Karlman
> > > > <[e-mail]>; Jernej Skrabec <[e-mail]>;
> > > > Maarten Lankhorst <[e-mail]>; Maxime Ripard
> > > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> > David
> > > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>; Bernie
> > > > Liang <[e-mail]>; Qilin Wen <[e-mail]>;
> > > > [e-mail]; [e-mail]; linux-
> > > > [e-mail]
> > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > > > atomic_enable()
> > > >
> > > > CAUTION: This email originated from outside of the organization.
> > > > Please do not click links or open attachments unless you recognize
> > > > the sender, and know the content is safe.
> > > >
> > > >
> > > > On Thu, Dec 12, 2024 at 01:51:10PM +0800, Xin Ji wrote:
> > > > > When user enabled HDCP feature, userspace 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
> > > > > userspace will not update the HDCP content to
> > > > > DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor
> > disconnect.
> > > >
> > > > It seems you've ingored a part of the previous review comment. It's
> > > > the userspace who triggers the ENABLED -> UNDESIRED transition, not
> > > > the kernel side. The change to move HDCP handling to atomic_enable()
> > > > looks fine, the change to disable HDCP is not (unless I misunderstand
> > something).
> > > >
> > > > >
> > > > > So, anx7625 driver move hdcp content value checking from bridge
> > > > > interface .atomic_check() to .atomic_enable(), then update hdcp
> > > > > content according from currently HDCP status. And also disabled
> > > > > HDCP in bridge interface .atomic_disable().
> > > > >
> > > > > Signed-off-by: Xin Ji <[e-mail]>
> > > > > ---
> > > > >  drivers/gpu/drm/bridge/analogix/anx7625.c | 74
> > > > > ++++++++++++++---------
> > > > >  1 file changed, 46 insertions(+), 28 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > index a2675b121fe4..f96ce5665e8d 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;
> > > > > @@ -2149,34 +2165,6 @@ static int
> > > > > anx7625_connector_atomic_check(struct
> > > > anx7625_data *ctx,
> > > > >       if (cp == ctx->hdcp_cp)
> > > > >               return 0;
> > > > >
> > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > > > -             if (ctx->dp_en) {
> > > > > -                     dev_dbg(dev, "enable HDCP\n");
> > > > > -                     anx7625_hdcp_enable(ctx);
> > > > > -
> > > > > -                     queue_delayed_work(ctx->hdcp_workqueue,
> > > > > -                                        &ctx->hdcp_work,
> > > > > -                                        msecs_to_jiffies(2000));
> > > > > -             }
> > > > > -     }
> > > > > -
> > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > > -             if (ctx->hdcp_cp !=
> > DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > -                     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");
> > > > > -     }
> > > > > -
> > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > -             dev_err(dev, "Userspace illegal set to PROTECTION ENABLE\n");
> > > > > -             return -EINVAL;
> > > > > -     }
> > > > > -
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > @@ -2425,6 +2413,8 @@ static void
> > > > > anx7625_bridge_atomic_enable(struct
> > > > drm_bridge *bridge,
> > > > >       struct anx7625_data *ctx = bridge_to_anx7625(bridge);
> > > > >       struct device *dev = ctx->dev;
> > > > >       struct drm_connector *connector;
> > > > > +     struct drm_connector_state *conn_state;
> > > > > +     int cp;
> > > > >
> > > > >       dev_dbg(dev, "drm atomic enable\n");
> > > > >
> > > > > @@ -2439,6 +2429,32 @@ static void
> > > > > anx7625_bridge_atomic_enable(struct
> > > > drm_bridge *bridge,
> > > > >       _anx7625_hpd_polling(ctx, 5000 * 100);
> > > > >
> > > > >       anx7625_dp_start(ctx);
> > > > > +
> > > > > +     conn_state =
> > > > > + drm_atomic_get_new_connector_state(state->base.state,
> > > > > + connector);
> > > > > +
> > > > > +     if (WARN_ON(!conn_state))
> > > > > +             return;
> > > > > +
> > > > > +     cp = conn_state->content_protection;
> > > > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > > > +             if (ctx->dp_en) {
> > > > > +                     dev_dbg(dev, "enable HDCP\n");
> > > > > +                     anx7625_hdcp_enable(ctx);
> > > > > +
> > > > > +                     queue_delayed_work(ctx->hdcp_workqueue,
> > > > > +                                        &ctx->hdcp_work,
> > > > > +                                        msecs_to_jiffies(2000));
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > > +             if (ctx->hdcp_cp !=
> > DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > +                     dev_err(dev, "current CP is not ENABLED\n");
> > > > > +                     return;
> > > > > +             }
> > > > > +
> > > > > +             anx7625_hdcp_disable_and_update_cp(ctx);
> > > > > +     }
> > > > >  }
> > > > >
> > > > >  static void anx7625_bridge_atomic_disable(struct drm_bridge
> > > > > *bridge, @@ -2449,6 +2465,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
> >
> > --
> > With best wishes
> > Dmitry

Xin Ji Dec. 16, 2024, 8:33 a.m. UTC | #7

> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: Friday, December 13, 2024 9:17 PM
> To: Xin Ji <[e-mail]>
> Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> <[e-mail]>; Robert Foss <[e-mail]>; Laurent Pinchart
> <[e-mail]>; Jonas Karlman <[e-mail]>;
> Jernej Skrabec <[e-mail]>; Maarten Lankhorst
> <[e-mail]>; Maxime Ripard <[e-mail]>;
> Thomas Zimmermann <[e-mail]>; David Airlie
> <[e-mail]>; Simona Vetter <[e-mail]>; Bernie Liang
> <[e-mail]>; Qilin Wen <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> atomic_enable()
> 
> CAUTION: This email originated from outside of the organization. Please do not
> click links or open attachments unless you recognize the sender, and know the
> content is safe.
> 
> 
> On Fri, 13 Dec 2024 at 13:00, Xin Ji <[e-mail]> wrote:
> >
> > Hi Dmitry, sorry, I didn't clear describe the reason.
> 
> Please. Do not top-post. Please paste your answer under the question, not
> somewhere at the top of the email. This allows us to have a more constructive
> dialog. Additional bonus if you can fix your email client to insert sensible quoting
> information instead of dumping the headers of the original email.
Hi Dmitry, OK, sorry about it. Currently, we have problem to fetch email from
Microsoft on Ubuntu. I'll try to fix it later.
> 
> >
> > Anx7625 implement DSI to DP convert behind USB Type-C port, when user
> > plug into USB Type-C Dongle with DP monitor, the user space will
> > enable HDCP feature, then kernel do HDCP and output display and set
> > HDCP content to ENABLE, but the issue happened if user manually change the
> monitor's resolution later.
> >
> > Each time user change the resolution, kernel will call bridge
> > interface .atomic_disable() and .atomic_enable(), the old driver will
> > keep HDCP state to ENABLE, this is a BUG, when user change the
> > resolution, kernel must change HDCP content too (mustn't keep to
> > ENABLE),
> 
> Why? Could you please point me to the corresponding documentation or a code
> path in the other driver? Preferably i915, AMD or Nouveau.
As https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_connector.c#L1423: 
        - ENABLED -> DESIRED (termination of authentication)
As there is no other interface to tell anx7625 bridge driver, so the I think best place to handle
ENABLE -> DESIRED in .atomic_disable().

> 
> > as DRM doc said, kernel cannot change from ENABLE to UNDESIRE, so next
> > patch, I'll change it to DESIRE in .atomic_disable().
> >
> > Thanks!
> > Xin
> >
> > > -----Original Message-----
> > > From: Dmitry Baryshkov <[e-mail]>
> > > Sent: Friday, December 13, 2024 6:47 PM
> > > To: Xin Ji <[e-mail]>
> > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > <[e-mail]>; Robert Foss <[e-mail]>; Laurent
> > > Pinchart <[e-mail]>; Jonas Karlman
> > > <[e-mail]>; Jernej Skrabec <[e-mail]>;
> > > Maarten Lankhorst <[e-mail]>; Maxime Ripard
> > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> David
> > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>; Bernie
> > > Liang <[e-mail]>; Qilin Wen <[e-mail]>;
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]
> > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > > atomic_enable()
> > >
> > > CAUTION: This email originated from outside of the organization.
> > > Please do not click links or open attachments unless you recognize
> > > the sender, and know the content is safe.
> > >
> > >
> > > On Fri, Dec 13, 2024 at 10:06:36AM +0000, Xin Ji wrote:
> > > > Hi Dmitry, thanks for the review, I made some changes which change
> > > > ENABLE to DESIRE in .atomic_disable(), I'll upstream it after testing. Thanks!
> > >
> > > - Please don't top-post.
> > >
> > > - You still didn't explain, why do you want to do this change of HDCP
> > >   status. Could you please provide an explanation before sending the
> > >   next iteration?
> > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Dmitry Baryshkov <[e-mail]>
> > > > > Sent: Thursday, December 12, 2024 5:18 PM
> > > > > To: Xin Ji <[e-mail]>
> > > > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > > > <[e-mail]>; Robert Foss <[e-mail]>;
> > > > > Laurent Pinchart <[e-mail]>; Jonas
> > > > > Karlman <[e-mail]>; Jernej Skrabec
> > > > > <[e-mail]>; Maarten Lankhorst
> > > > > <[e-mail]>; Maxime Ripard
> > > > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> > > David
> > > > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>;
> > > > > Bernie Liang <[e-mail]>; Qilin Wen
> > > > > <[e-mail]>; [e-mail];
> > > > > [e-mail]; linux- [e-mail]
> > > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status
> > > > > at
> > > > > atomic_enable()
> > > > >
> > > > > CAUTION: This email originated from outside of the organization.
> > > > > Please do not click links or open attachments unless you
> > > > > recognize the sender, and know the content is safe.
> > > > >
> > > > >
> > > > > On Thu, Dec 12, 2024 at 01:51:10PM +0800, Xin Ji wrote:
> > > > > > When user enabled HDCP feature, userspace 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
> > > > > > userspace will not update the HDCP content to
> > > > > > DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor
> > > disconnect.
> > > > >
> > > > > It seems you've ingored a part of the previous review comment.
> > > > > It's the userspace who triggers the ENABLED -> UNDESIRED
> > > > > transition, not the kernel side. The change to move HDCP
> > > > > handling to atomic_enable() looks fine, the change to disable
> > > > > HDCP is not (unless I misunderstand
> > > something).
> > > > >
> > > > > >
> > > > > > So, anx7625 driver move hdcp content value checking from
> > > > > > bridge interface .atomic_check() to .atomic_enable(), then
> > > > > > update hdcp content according from currently HDCP status. And
> > > > > > also disabled HDCP in bridge interface .atomic_disable().
> > > > > >
> > > > > > Signed-off-by: Xin Ji <[e-mail]>
> > > > > > ---
> > > > > >  drivers/gpu/drm/bridge/analogix/anx7625.c | 74
> > > > > > ++++++++++++++---------
> > > > > >  1 file changed, 46 insertions(+), 28 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > > index a2675b121fe4..f96ce5665e8d 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;
> > > > > > @@ -2149,34 +2165,6 @@ static int
> > > > > > anx7625_connector_atomic_check(struct
> > > > > anx7625_data *ctx,
> > > > > >       if (cp == ctx->hdcp_cp)
> > > > > >               return 0;
> > > > > >
> > > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > > > > -             if (ctx->dp_en) {
> > > > > > -                     dev_dbg(dev, "enable HDCP\n");
> > > > > > -                     anx7625_hdcp_enable(ctx);
> > > > > > -
> > > > > > -                     queue_delayed_work(ctx->hdcp_workqueue,
> > > > > > -                                        &ctx->hdcp_work,
> > > > > > -                                        msecs_to_jiffies(2000));
> > > > > > -             }
> > > > > > -     }
> > > > > > -
> > > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > > > -             if (ctx->hdcp_cp !=
> > > DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > > -                     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");
> > > > > > -     }
> > > > > > -
> > > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > > -             dev_err(dev, "Userspace illegal set to PROTECTION ENABLE\n");
> > > > > > -             return -EINVAL;
> > > > > > -     }
> > > > > > -
> > > > > >       return 0;
> > > > > >  }
> > > > > >
> > > > > > @@ -2425,6 +2413,8 @@ static void
> > > > > > anx7625_bridge_atomic_enable(struct
> > > > > drm_bridge *bridge,
> > > > > >       struct anx7625_data *ctx = bridge_to_anx7625(bridge);
> > > > > >       struct device *dev = ctx->dev;
> > > > > >       struct drm_connector *connector;
> > > > > > +     struct drm_connector_state *conn_state;
> > > > > > +     int cp;
> > > > > >
> > > > > >       dev_dbg(dev, "drm atomic enable\n");
> > > > > >
> > > > > > @@ -2439,6 +2429,32 @@ static void
> > > > > > anx7625_bridge_atomic_enable(struct
> > > > > drm_bridge *bridge,
> > > > > >       _anx7625_hpd_polling(ctx, 5000 * 100);
> > > > > >
> > > > > >       anx7625_dp_start(ctx);
> > > > > > +
> > > > > > +     conn_state =
> > > > > > + drm_atomic_get_new_connector_state(state->base.state,
> > > > > > + connector);
> > > > > > +
> > > > > > +     if (WARN_ON(!conn_state))
> > > > > > +             return;
> > > > > > +
> > > > > > +     cp = conn_state->content_protection;
> > > > > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > > > > +             if (ctx->dp_en) {
> > > > > > +                     dev_dbg(dev, "enable HDCP\n");
> > > > > > +                     anx7625_hdcp_enable(ctx);
> > > > > > +
> > > > > > +                     queue_delayed_work(ctx->hdcp_workqueue,
> > > > > > +                                        &ctx->hdcp_work,
> > > > > > +                                        msecs_to_jiffies(2000));
> > > > > > +             }
> > > > > > +     }
> > > > > > +
> > > > > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > > > +             if (ctx->hdcp_cp !=
> > > DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > > +                     dev_err(dev, "current CP is not ENABLED\n");
> > > > > > +                     return;
> > > > > > +             }
> > > > > > +
> > > > > > +             anx7625_hdcp_disable_and_update_cp(ctx);
> > > > > > +     }
> > > > > >  }
> > > > > >
> > > > > >  static void anx7625_bridge_atomic_disable(struct drm_bridge
> > > > > > *bridge, @@ -2449,6 +2465,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
> > >
> > > --
> > > With best wishes
> > > Dmitry
> 
> 
> 
> --
> With best wishes
> Dmitry

Dmitry Baryshkov Dec. 16, 2024, 9:43 a.m. UTC | #8

On Mon, Dec 16, 2024 at 08:33:23AM +0000, Xin Ji wrote:
> > -----Original Message-----
> > From: Dmitry Baryshkov <[e-mail]>
> > Sent: Friday, December 13, 2024 9:17 PM
> > To: Xin Ji <[e-mail]>
> > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > <[e-mail]>; Robert Foss <[e-mail]>; Laurent Pinchart
> > <[e-mail]>; Jonas Karlman <[e-mail]>;
> > Jernej Skrabec <[e-mail]>; Maarten Lankhorst
> > <[e-mail]>; Maxime Ripard <[e-mail]>;
> > Thomas Zimmermann <[e-mail]>; David Airlie
> > <[e-mail]>; Simona Vetter <[e-mail]>; Bernie Liang
> > <[e-mail]>; Qilin Wen <[e-mail]>;
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > atomic_enable()
> > 
> > CAUTION: This email originated from outside of the organization. Please do not
> > click links or open attachments unless you recognize the sender, and know the
> > content is safe.
> > 
> > 
> > On Fri, 13 Dec 2024 at 13:00, Xin Ji <[e-mail]> wrote:
> > >
> > > Hi Dmitry, sorry, I didn't clear describe the reason.
> > 
> > Please. Do not top-post. Please paste your answer under the question, not
> > somewhere at the top of the email. This allows us to have a more constructive
> > dialog. Additional bonus if you can fix your email client to insert sensible quoting
> > information instead of dumping the headers of the original email.
> Hi Dmitry, OK, sorry about it. Currently, we have problem to fetch email from
> Microsoft on Ubuntu. I'll try to fix it later.
> > 
> > >
> > > Anx7625 implement DSI to DP convert behind USB Type-C port, when user
> > > plug into USB Type-C Dongle with DP monitor, the user space will
> > > enable HDCP feature, then kernel do HDCP and output display and set
> > > HDCP content to ENABLE, but the issue happened if user manually change the
> > monitor's resolution later.
> > >
> > > Each time user change the resolution, kernel will call bridge
> > > interface .atomic_disable() and .atomic_enable(), the old driver will
> > > keep HDCP state to ENABLE, this is a BUG, when user change the
> > > resolution, kernel must change HDCP content too (mustn't keep to
> > > ENABLE),
> > 
> > Why? Could you please point me to the corresponding documentation or a code
> > path in the other driver? Preferably i915, AMD or Nouveau.
> As https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_connector.c#L1423: 
>         - ENABLED -> DESIRED (termination of authentication)
> As there is no other interface to tell anx7625 bridge driver, so the I think best place to handle
> ENABLE -> DESIRED in .atomic_disable().

I was looking for something like cdns_mhdp_connector_atomic_check(),
which switches to UNDESIRED if there is no new CRTC. Likewise i915
driver performs this in intel_hdcp_atomic_check() if there is a need for
modeset.

For the "termination of authentication" case see
cdns_mhdp_hdcp_check_link(), which detects if the HDCP got disabled by
HW and then updates the status accordingly.

> 
> > 
> > > as DRM doc said, kernel cannot change from ENABLE to UNDESIRE, so next
> > > patch, I'll change it to DESIRE in .atomic_disable().

This e.g. will result in HDCP being restarted for all modesets. Is this
an expected behaviour?

> > >
> > > Thanks!
> > > Xin
> > >
> > > > -----Original Message-----
> > > > From: Dmitry Baryshkov <[e-mail]>
> > > > Sent: Friday, December 13, 2024 6:47 PM
> > > > To: Xin Ji <[e-mail]>
> > > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > > <[e-mail]>; Robert Foss <[e-mail]>; Laurent
> > > > Pinchart <[e-mail]>; Jonas Karlman
> > > > <[e-mail]>; Jernej Skrabec <[e-mail]>;
> > > > Maarten Lankhorst <[e-mail]>; Maxime Ripard
> > > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> > David
> > > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>; Bernie
> > > > Liang <[e-mail]>; Qilin Wen <[e-mail]>;
> > > > [e-mail]; [e-mail]; linux-
> > > > [e-mail]
> > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > > > atomic_enable()
> > > >
> > > > CAUTION: This email originated from outside of the organization.
> > > > Please do not click links or open attachments unless you recognize
> > > > the sender, and know the content is safe.
> > > >
> > > >
> > > > On Fri, Dec 13, 2024 at 10:06:36AM +0000, Xin Ji wrote:
> > > > > Hi Dmitry, thanks for the review, I made some changes which change
> > > > > ENABLE to DESIRE in .atomic_disable(), I'll upstream it after testing. Thanks!
> > > >
> > > > - Please don't top-post.
> > > >
> > > > - You still didn't explain, why do you want to do this change of HDCP
> > > >   status. Could you please provide an explanation before sending the
> > > >   next iteration?
> > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Dmitry Baryshkov <[e-mail]>
> > > > > > Sent: Thursday, December 12, 2024 5:18 PM
> > > > > > To: Xin Ji <[e-mail]>
> > > > > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > > > > <[e-mail]>; Robert Foss <[e-mail]>;
> > > > > > Laurent Pinchart <[e-mail]>; Jonas
> > > > > > Karlman <[e-mail]>; Jernej Skrabec
> > > > > > <[e-mail]>; Maarten Lankhorst
> > > > > > <[e-mail]>; Maxime Ripard
> > > > > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> > > > David
> > > > > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>;
> > > > > > Bernie Liang <[e-mail]>; Qilin Wen
> > > > > > <[e-mail]>; [e-mail];
> > > > > > [e-mail]; linux- [e-mail]
> > > > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status
> > > > > > at
> > > > > > atomic_enable()
> > > > > >
> > > > > > CAUTION: This email originated from outside of the organization.
> > > > > > Please do not click links or open attachments unless you
> > > > > > recognize the sender, and know the content is safe.
> > > > > >
> > > > > >
> > > > > > On Thu, Dec 12, 2024 at 01:51:10PM +0800, Xin Ji wrote:
> > > > > > > When user enabled HDCP feature, userspace 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
> > > > > > > userspace will not update the HDCP content to
> > > > > > > DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor
> > > > disconnect.
> > > > > >
> > > > > > It seems you've ingored a part of the previous review comment.
> > > > > > It's the userspace who triggers the ENABLED -> UNDESIRED
> > > > > > transition, not the kernel side. The change to move HDCP
> > > > > > handling to atomic_enable() looks fine, the change to disable
> > > > > > HDCP is not (unless I misunderstand
> > > > something).
> > > > > >
> > > > > > >
> > > > > > > So, anx7625 driver move hdcp content value checking from
> > > > > > > bridge interface .atomic_check() to .atomic_enable(), then
> > > > > > > update hdcp content according from currently HDCP status. And
> > > > > > > also disabled HDCP in bridge interface .atomic_disable().
> > > > > > >
> > > > > > > Signed-off-by: Xin Ji <[e-mail]>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/bridge/analogix/anx7625.c | 74
> > > > > > > ++++++++++++++---------
> > > > > > >  1 file changed, 46 insertions(+), 28 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > > > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > > > index a2675b121fe4..f96ce5665e8d 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;
> > > > > > > @@ -2149,34 +2165,6 @@ static int
> > > > > > > anx7625_connector_atomic_check(struct
> > > > > > anx7625_data *ctx,
> > > > > > >       if (cp == ctx->hdcp_cp)
> > > > > > >               return 0;
> > > > > > >
> > > > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > > > > > -             if (ctx->dp_en) {
> > > > > > > -                     dev_dbg(dev, "enable HDCP\n");
> > > > > > > -                     anx7625_hdcp_enable(ctx);
> > > > > > > -
> > > > > > > -                     queue_delayed_work(ctx->hdcp_workqueue,
> > > > > > > -                                        &ctx->hdcp_work,
> > > > > > > -                                        msecs_to_jiffies(2000));
> > > > > > > -             }
> > > > > > > -     }
> > > > > > > -
> > > > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > > > > -             if (ctx->hdcp_cp !=
> > > > DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > > > -                     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");
> > > > > > > -     }
> > > > > > > -
> > > > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > > > -             dev_err(dev, "Userspace illegal set to PROTECTION ENABLE\n");
> > > > > > > -             return -EINVAL;
> > > > > > > -     }
> > > > > > > -
> > > > > > >       return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > @@ -2425,6 +2413,8 @@ static void
> > > > > > > anx7625_bridge_atomic_enable(struct
> > > > > > drm_bridge *bridge,
> > > > > > >       struct anx7625_data *ctx = bridge_to_anx7625(bridge);
> > > > > > >       struct device *dev = ctx->dev;
> > > > > > >       struct drm_connector *connector;
> > > > > > > +     struct drm_connector_state *conn_state;
> > > > > > > +     int cp;
> > > > > > >
> > > > > > >       dev_dbg(dev, "drm atomic enable\n");
> > > > > > >
> > > > > > > @@ -2439,6 +2429,32 @@ static void
> > > > > > > anx7625_bridge_atomic_enable(struct
> > > > > > drm_bridge *bridge,
> > > > > > >       _anx7625_hpd_polling(ctx, 5000 * 100);
> > > > > > >
> > > > > > >       anx7625_dp_start(ctx);
> > > > > > > +
> > > > > > > +     conn_state =
> > > > > > > + drm_atomic_get_new_connector_state(state->base.state,
> > > > > > > + connector);
> > > > > > > +
> > > > > > > +     if (WARN_ON(!conn_state))
> > > > > > > +             return;
> > > > > > > +
> > > > > > > +     cp = conn_state->content_protection;
> > > > > > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > > > > > +             if (ctx->dp_en) {
> > > > > > > +                     dev_dbg(dev, "enable HDCP\n");
> > > > > > > +                     anx7625_hdcp_enable(ctx);
> > > > > > > +
> > > > > > > +                     queue_delayed_work(ctx->hdcp_workqueue,
> > > > > > > +                                        &ctx->hdcp_work,
> > > > > > > +                                        msecs_to_jiffies(2000));
> > > > > > > +             }
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > > > > +             if (ctx->hdcp_cp !=
> > > > DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > > > +                     dev_err(dev, "current CP is not ENABLED\n");
> > > > > > > +                     return;
> > > > > > > +             }
> > > > > > > +
> > > > > > > +             anx7625_hdcp_disable_and_update_cp(ctx);
> > > > > > > +     }
> > > > > > >  }
> > > > > > >
> > > > > > >  static void anx7625_bridge_atomic_disable(struct drm_bridge
> > > > > > > *bridge, @@ -2449,6 +2465,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
> > > >
> > > > --
> > > > With best wishes
> > > > Dmitry
> > 
> > 
> > 
> > --
> > With best wishes
> > Dmitry

Pin-yen Lin Dec. 16, 2024, 12:05 p.m. UTC | #9

Hi Dmitry,

On Mon, Dec 16, 2024 at 7:59 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> On Mon, Dec 16, 2024 at 08:33:23AM +0000, Xin Ji wrote:
> > > -----Original Message-----
> > > From: Dmitry Baryshkov <[e-mail]>
> > > Sent: Friday, December 13, 2024 9:17 PM
> > > To: Xin Ji <[e-mail]>
> > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > <[e-mail]>; Robert Foss <[e-mail]>; Laurent Pinchart
> > > <[e-mail]>; Jonas Karlman <[e-mail]>;
> > > Jernej Skrabec <[e-mail]>; Maarten Lankhorst
> > > <[e-mail]>; Maxime Ripard <[e-mail]>;
> > > Thomas Zimmermann <[e-mail]>; David Airlie
> > > <[e-mail]>; Simona Vetter <[e-mail]>; Bernie Liang
> > > <[e-mail]>; Qilin Wen <[e-mail]>;
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]
> > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > > atomic_enable()
> > >
> > > CAUTION: This email originated from outside of the organization. Please do not
> > > click links or open attachments unless you recognize the sender, and know the
> > > content is safe.
> > >
> > >
> > > On Fri, 13 Dec 2024 at 13:00, Xin Ji <[e-mail]> wrote:
> > > >
> > > > Hi Dmitry, sorry, I didn't clear describe the reason.
> > >
> > > Please. Do not top-post. Please paste your answer under the question, not
> > > somewhere at the top of the email. This allows us to have a more constructive
> > > dialog. Additional bonus if you can fix your email client to insert sensible quoting
> > > information instead of dumping the headers of the original email.
> > Hi Dmitry, OK, sorry about it. Currently, we have problem to fetch email from
> > Microsoft on Ubuntu. I'll try to fix it later.
> > >
> > > >
> > > > Anx7625 implement DSI to DP convert behind USB Type-C port, when user
> > > > plug into USB Type-C Dongle with DP monitor, the user space will
> > > > enable HDCP feature, then kernel do HDCP and output display and set
> > > > HDCP content to ENABLE, but the issue happened if user manually change the
> > > monitor's resolution later.
> > > >
> > > > Each time user change the resolution, kernel will call bridge
> > > > interface .atomic_disable() and .atomic_enable(), the old driver will
> > > > keep HDCP state to ENABLE, this is a BUG, when user change the
> > > > resolution, kernel must change HDCP content too (mustn't keep to
> > > > ENABLE),
> > >
> > > Why? Could you please point me to the corresponding documentation or a code
> > > path in the other driver? Preferably i915, AMD or Nouveau.
> > As https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_connector.c#L1423:
> >         - ENABLED -> DESIRED (termination of authentication)
> > As there is no other interface to tell anx7625 bridge driver, so the I think best place to handle
> > ENABLE -> DESIRED in .atomic_disable().
>
> I was looking for something like cdns_mhdp_connector_atomic_check(),
> which switches to UNDESIRED if there is no new CRTC. Likewise i915
> driver performs this in intel_hdcp_atomic_check() if there is a need for
> modeset.

I believe you mean "DESIRED" here.
>
> For the "termination of authentication" case see
> cdns_mhdp_hdcp_check_link(), which detects if the HDCP got disabled by
> HW and then updates the status accordingly.
>
> >
> > >
> > > > as DRM doc said, kernel cannot change from ENABLE to UNDESIRE, so next
> > > > patch, I'll change it to DESIRE in .atomic_disable().
>
> This e.g. will result in HDCP being restarted for all modesets. Is this
> an expected behaviour?

The bridge could be powered off between .atomic_disable() and
.atomic_enable(), though I'm not sure if this would be a concern for
anx7625. I'll let Xin to comment on this.
>
> > > >
> > > > Thanks!
> > > > Xin
> > > >
> > > > > -----Original Message-----
> > > > > From: Dmitry Baryshkov <[e-mail]>
> > > > > Sent: Friday, December 13, 2024 6:47 PM
> > > > > To: Xin Ji <[e-mail]>
> > > > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > > > <[e-mail]>; Robert Foss <[e-mail]>; Laurent
> > > > > Pinchart <[e-mail]>; Jonas Karlman
> > > > > <[e-mail]>; Jernej Skrabec <[e-mail]>;
> > > > > Maarten Lankhorst <[e-mail]>; Maxime Ripard
> > > > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> > > David
> > > > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>; Bernie
> > > > > Liang <[e-mail]>; Qilin Wen <[e-mail]>;
> > > > > [e-mail]; [e-mail]; linux-
> > > > > [e-mail]
> > > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > > > > atomic_enable()
> > > > >
> > > > > CAUTION: This email originated from outside of the organization.
> > > > > Please do not click links or open attachments unless you recognize
> > > > > the sender, and know the content is safe.
> > > > >
> > > > >
> > > > > On Fri, Dec 13, 2024 at 10:06:36AM +0000, Xin Ji wrote:
> > > > > > Hi Dmitry, thanks for the review, I made some changes which change
> > > > > > ENABLE to DESIRE in .atomic_disable(), I'll upstream it after testing. Thanks!
> > > > >
> > > > > - Please don't top-post.
> > > > >
> > > > > - You still didn't explain, why do you want to do this change of HDCP
> > > > >   status. Could you please provide an explanation before sending the
> > > > >   next iteration?
> > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Dmitry Baryshkov <[e-mail]>
> > > > > > > Sent: Thursday, December 12, 2024 5:18 PM
> > > > > > > To: Xin Ji <[e-mail]>
> > > > > > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > > > > > <[e-mail]>; Robert Foss <[e-mail]>;
> > > > > > > Laurent Pinchart <[e-mail]>; Jonas
> > > > > > > Karlman <[e-mail]>; Jernej Skrabec
> > > > > > > <[e-mail]>; Maarten Lankhorst
> > > > > > > <[e-mail]>; Maxime Ripard
> > > > > > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> > > > > David
> > > > > > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>;
> > > > > > > Bernie Liang <[e-mail]>; Qilin Wen
> > > > > > > <[e-mail]>; [e-mail];
> > > > > > > [e-mail]; linux- [e-mail]
> > > > > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status
> > > > > > > at
> > > > > > > atomic_enable()
> > > > > > >
> > > > > > > CAUTION: This email originated from outside of the organization.
> > > > > > > Please do not click links or open attachments unless you
> > > > > > > recognize the sender, and know the content is safe.
> > > > > > >
> > > > > > >
> > > > > > > On Thu, Dec 12, 2024 at 01:51:10PM +0800, Xin Ji wrote:
> > > > > > > > When user enabled HDCP feature, userspace 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
> > > > > > > > userspace will not update the HDCP content to
> > > > > > > > DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor
> > > > > disconnect.
> > > > > > >
> > > > > > > It seems you've ingored a part of the previous review comment.
> > > > > > > It's the userspace who triggers the ENABLED -> UNDESIRED
> > > > > > > transition, not the kernel side. The change to move HDCP
> > > > > > > handling to atomic_enable() looks fine, the change to disable
> > > > > > > HDCP is not (unless I misunderstand
> > > > > something).
> > > > > > >
> > > > > > > >
> > > > > > > > So, anx7625 driver move hdcp content value checking from
> > > > > > > > bridge interface .atomic_check() to .atomic_enable(), then
> > > > > > > > update hdcp content according from currently HDCP status. And
> > > > > > > > also disabled HDCP in bridge interface .atomic_disable().
> > > > > > > >
> > > > > > > > Signed-off-by: Xin Ji <[e-mail]>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/bridge/analogix/anx7625.c | 74
> > > > > > > > ++++++++++++++---------
> > > > > > > >  1 file changed, 46 insertions(+), 28 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > > > > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > > > > index a2675b121fe4..f96ce5665e8d 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;
> > > > > > > > @@ -2149,34 +2165,6 @@ static int
> > > > > > > > anx7625_connector_atomic_check(struct
> > > > > > > anx7625_data *ctx,
> > > > > > > >       if (cp == ctx->hdcp_cp)
> > > > > > > >               return 0;
> > > > > > > >
> > > > > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > > > > > > -             if (ctx->dp_en) {
> > > > > > > > -                     dev_dbg(dev, "enable HDCP\n");
> > > > > > > > -                     anx7625_hdcp_enable(ctx);
> > > > > > > > -
> > > > > > > > -                     queue_delayed_work(ctx->hdcp_workqueue,
> > > > > > > > -                                        &ctx->hdcp_work,
> > > > > > > > -                                        msecs_to_jiffies(2000));
> > > > > > > > -             }
> > > > > > > > -     }
> > > > > > > > -
> > > > > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > > > > > -             if (ctx->hdcp_cp !=
> > > > > DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > > > > -                     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");
> > > > > > > > -     }
> > > > > > > > -
> > > > > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > > > > -             dev_err(dev, "Userspace illegal set to PROTECTION ENABLE\n");
> > > > > > > > -             return -EINVAL;
> > > > > > > > -     }
> > > > > > > > -
> > > > > > > >       return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > @@ -2425,6 +2413,8 @@ static void
> > > > > > > > anx7625_bridge_atomic_enable(struct
> > > > > > > drm_bridge *bridge,
> > > > > > > >       struct anx7625_data *ctx = bridge_to_anx7625(bridge);
> > > > > > > >       struct device *dev = ctx->dev;
> > > > > > > >       struct drm_connector *connector;
> > > > > > > > +     struct drm_connector_state *conn_state;
> > > > > > > > +     int cp;
> > > > > > > >
> > > > > > > >       dev_dbg(dev, "drm atomic enable\n");
> > > > > > > >
> > > > > > > > @@ -2439,6 +2429,32 @@ static void
> > > > > > > > anx7625_bridge_atomic_enable(struct
> > > > > > > drm_bridge *bridge,
> > > > > > > >       _anx7625_hpd_polling(ctx, 5000 * 100);
> > > > > > > >
> > > > > > > >       anx7625_dp_start(ctx);
> > > > > > > > +
> > > > > > > > +     conn_state =
> > > > > > > > + drm_atomic_get_new_connector_state(state->base.state,
> > > > > > > > + connector);
> > > > > > > > +
> > > > > > > > +     if (WARN_ON(!conn_state))
> > > > > > > > +             return;
> > > > > > > > +
> > > > > > > > +     cp = conn_state->content_protection;
> > > > > > > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > > > > > > +             if (ctx->dp_en) {
> > > > > > > > +                     dev_dbg(dev, "enable HDCP\n");
> > > > > > > > +                     anx7625_hdcp_enable(ctx);
> > > > > > > > +
> > > > > > > > +                     queue_delayed_work(ctx->hdcp_workqueue,
> > > > > > > > +                                        &ctx->hdcp_work,
> > > > > > > > +                                        msecs_to_jiffies(2000));
> > > > > > > > +             }
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > > > > > +             if (ctx->hdcp_cp !=
> > > > > DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > > > > +                     dev_err(dev, "current CP is not ENABLED\n");
> > > > > > > > +                     return;
> > > > > > > > +             }
> > > > > > > > +
> > > > > > > > +             anx7625_hdcp_disable_and_update_cp(ctx);
> > > > > > > > +     }
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static void anx7625_bridge_atomic_disable(struct drm_bridge
> > > > > > > > *bridge, @@ -2449,6 +2465,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
> > > > >
> > > > > --
> > > > > With best wishes
> > > > > Dmitry
> > >
> > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
>
> --
> With best wishes
> Dmitry

Regards,
Pin-yen

Xin Ji Dec. 17, 2024, 1:50 a.m. UTC | #10

> -----Original Message-----
> From: Pin-yen Lin <[e-mail]>
> Sent: Monday, December 16, 2024 8:05 PM
> To: Dmitry Baryshkov <[e-mail]>
> Cc: Xin Ji <[e-mail]>; Andrzej Hajda <[e-mail]>;
> Neil Armstrong <[e-mail]>; Robert Foss <[e-mail]>;
> Laurent Pinchart <[e-mail]>; Jonas Karlman
> <[e-mail]>; Jernej Skrabec <[e-mail]>; Maarten
> Lankhorst <[e-mail]>; Maxime Ripard
> <[e-mail]>; Thomas Zimmermann <[e-mail]>; David
> Airlie <[e-mail]>; Simona Vetter <[e-mail]>; Bernie Liang
> <[e-mail]>; Qilin Wen <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> atomic_enable()
> 
> 
> Hi Dmitry,
> 
> On Mon, Dec 16, 2024 at 7:59 PM Dmitry Baryshkov
> <[e-mail]> wrote:
> >
> > On Mon, Dec 16, 2024 at 08:33:23AM +0000, Xin Ji wrote:
> > > > -----Original Message-----
> > > > From: Dmitry Baryshkov <[e-mail]>
> > > > Sent: Friday, December 13, 2024 9:17 PM
> > > > To: Xin Ji <[e-mail]>
> > > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > > <[e-mail]>; Robert Foss <[e-mail]>;
> > > > Laurent Pinchart <[e-mail]>; Jonas
> > > > Karlman <[e-mail]>; Jernej Skrabec
> > > > <[e-mail]>; Maarten Lankhorst
> > > > <[e-mail]>; Maxime Ripard
> > > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> > > > David Airlie <[e-mail]>; Simona Vetter <[e-mail]>;
> > > > Bernie Liang <[e-mail]>; Qilin Wen
> > > > <[e-mail]>; [e-mail];
> > > > [e-mail]; linux- [e-mail]
> > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > > > atomic_enable()
> > > >
> > > > CAUTION: This email originated from outside of the organization.
> > > > Please do not click links or open attachments unless you recognize
> > > > the sender, and know the content is safe.
> > > >
> > > >
> > > > On Fri, 13 Dec 2024 at 13:00, Xin Ji <[e-mail]> wrote:
> > > > >
> > > > > Hi Dmitry, sorry, I didn't clear describe the reason.
> > > >
> > > > Please. Do not top-post. Please paste your answer under the
> > > > question, not somewhere at the top of the email. This allows us to
> > > > have a more constructive dialog. Additional bonus if you can fix
> > > > your email client to insert sensible quoting information instead of dumping
> the headers of the original email.
> > > Hi Dmitry, OK, sorry about it. Currently, we have problem to fetch
> > > email from Microsoft on Ubuntu. I'll try to fix it later.
> > > >
> > > > >
> > > > > Anx7625 implement DSI to DP convert behind USB Type-C port, when
> > > > > user plug into USB Type-C Dongle with DP monitor, the user space
> > > > > will enable HDCP feature, then kernel do HDCP and output display
> > > > > and set HDCP content to ENABLE, but the issue happened if user
> > > > > manually change the
> > > > monitor's resolution later.
> > > > >
> > > > > Each time user change the resolution, kernel will call bridge
> > > > > interface .atomic_disable() and .atomic_enable(), the old driver
> > > > > will keep HDCP state to ENABLE, this is a BUG, when user change
> > > > > the resolution, kernel must change HDCP content too (mustn't
> > > > > keep to ENABLE),
> > > >
> > > > Why? Could you please point me to the corresponding documentation
> > > > or a code path in the other driver? Preferably i915, AMD or Nouveau.
> > > As
> https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_connecto
> r.c#L1423:
> > >         - ENABLED -> DESIRED (termination of authentication) As
> > > there is no other interface to tell anx7625 bridge driver, so the I
> > > think best place to handle ENABLE -> DESIRED in .atomic_disable().
> >
> > I was looking for something like cdns_mhdp_connector_atomic_check(),
> > which switches to UNDESIRED if there is no new CRTC. Likewise i915
> > driver performs this in intel_hdcp_atomic_check() if there is a need
> > for modeset.
Hi Dmitry, the bridge driver is different with i915, anx7625 bridge driver
only implement some passive callback interface. There is no HDCP status
checking while do resolution switch.
> 
> I believe you mean "DESIRED" here.
> >
> > For the "termination of authentication" case see
> > cdns_mhdp_hdcp_check_link(), which detects if the HDCP got disabled by
> > HW and then updates the status accordingly.
> >
> > >
> > > >
> > > > > as DRM doc said, kernel cannot change from ENABLE to UNDESIRE,
> > > > > so next patch, I'll change it to DESIRE in .atomic_disable().
> >
> > This e.g. will result in HDCP being restarted for all modesets. Is
> > this an expected behaviour?
> 
> The bridge could be powered off between .atomic_disable()
> and .atomic_enable(), though I'm not sure if this would be a concern for
> anx7625. I'll let Xin to comment on this.
The flow of resolution switch is:
    -> .atomic_disable().
        -> .atomic_enable().
            -> HDCP status check and enable.
As resolution switch is triggered by user space, at this moment, the HDCP
works fine(cannot get disabled event from HW). So, I think, .atomic_disable() is
the best place to do termination of authentication (turn HDCP status to DESIRED).
This is the only entrance which can do termination of authentication, I cannot
find others.

> >
> > > > >
> > > > > Thanks!
> > > > > Xin
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Dmitry Baryshkov <[e-mail]>
> > > > > > Sent: Friday, December 13, 2024 6:47 PM
> > > > > > To: Xin Ji <[e-mail]>
> > > > > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > > > > <[e-mail]>; Robert Foss <[e-mail]>;
> > > > > > Laurent Pinchart <[e-mail]>; Jonas
> > > > > > Karlman <[e-mail]>; Jernej Skrabec
> > > > > > <[e-mail]>; Maarten Lankhorst
> > > > > > <[e-mail]>; Maxime Ripard
> > > > > > <[e-mail]>; Thomas Zimmermann
> <[e-mail]>;
> > > > David
> > > > > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>;
> > > > > > Bernie Liang <[e-mail]>; Qilin Wen
> > > > > > <[e-mail]>; [e-mail];
> > > > > > [e-mail]; linux- [e-mail]
> > > > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status
> > > > > > at
> > > > > > atomic_enable()
> > > > > >
> > > > > > CAUTION: This email originated from outside of the organization.
> > > > > > Please do not click links or open attachments unless you
> > > > > > recognize the sender, and know the content is safe.
> > > > > >
> > > > > >
> > > > > > On Fri, Dec 13, 2024 at 10:06:36AM +0000, Xin Ji wrote:
> > > > > > > Hi Dmitry, thanks for the review, I made some changes which
> > > > > > > change ENABLE to DESIRE in .atomic_disable(), I'll upstream it after
> testing. Thanks!
> > > > > >
> > > > > > - Please don't top-post.
> > > > > >
> > > > > > - You still didn't explain, why do you want to do this change of HDCP
> > > > > >   status. Could you please provide an explanation before sending the
> > > > > >   next iteration?
> > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Dmitry Baryshkov <[e-mail]>
> > > > > > > > Sent: Thursday, December 12, 2024 5:18 PM
> > > > > > > > To: Xin Ji <[e-mail]>
> > > > > > > > Cc: Andrzej Hajda <[e-mail]>; Neil
> > > > > > > > Armstrong <[e-mail]>; Robert Foss
> > > > > > > > <[e-mail]>; Laurent Pinchart
> > > > > > > > <[e-mail]>; Jonas Karlman
> > > > > > > > <[e-mail]>; Jernej Skrabec
> > > > > > > > <[e-mail]>; Maarten Lankhorst
> > > > > > > > <[e-mail]>; Maxime Ripard
> > > > > > > > <[e-mail]>; Thomas Zimmermann
> > > > > > > > <[e-mail]>;
> > > > > > David
> > > > > > > > Airlie <[e-mail]>; Simona Vetter
> > > > > > > > <[e-mail]>; Bernie Liang <[e-mail]>;
> > > > > > > > Qilin Wen <[e-mail]>; [e-mail];
> > > > > > > > [e-mail]; linux-
> > > > > > > > [e-mail]
> > > > > > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP
> > > > > > > > status at
> > > > > > > > atomic_enable()
> > > > > > > >
> > > > > > > > CAUTION: This email originated from outside of the organization.
> > > > > > > > Please do not click links or open attachments unless you
> > > > > > > > recognize the sender, and know the content is safe.
> > > > > > > >
> > > > > > > >
> > > > > > > > On Thu, Dec 12, 2024 at 01:51:10PM +0800, Xin Ji wrote:
> > > > > > > > > When user enabled HDCP feature, userspace 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
> > > > > > > > > userspace will not update the HDCP content to
> > > > > > > > > DRM_MODE_CONTENT_PROTECTION_UNDESIRED until monitor
> > > > > > disconnect.
> > > > > > > >
> > > > > > > > It seems you've ingored a part of the previous review comment.
> > > > > > > > It's the userspace who triggers the ENABLED -> UNDESIRED
> > > > > > > > transition, not the kernel side. The change to move HDCP
> > > > > > > > handling to atomic_enable() looks fine, the change to
> > > > > > > > disable HDCP is not (unless I misunderstand
> > > > > > something).
> > > > > > > >
> > > > > > > > >
> > > > > > > > > So, anx7625 driver move hdcp content value checking from
> > > > > > > > > bridge interface .atomic_check() to .atomic_enable(),
> > > > > > > > > then update hdcp content according from currently HDCP
> > > > > > > > > status. And also disabled HDCP in bridge
> interface .atomic_disable().
> > > > > > > > >
> > > > > > > > > Signed-off-by: Xin Ji <[e-mail]>
> > > > > > > > > ---
> > > > > > > > >  drivers/gpu/drm/bridge/analogix/anx7625.c | 74
> > > > > > > > > ++++++++++++++---------
> > > > > > > > >  1 file changed, 46 insertions(+), 28 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > > > > > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > > > > > index a2675b121fe4..f96ce5665e8d 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;
> > > > > > > > > @@ -2149,34 +2165,6 @@ static int
> > > > > > > > > anx7625_connector_atomic_check(struct
> > > > > > > > anx7625_data *ctx,
> > > > > > > > >       if (cp == ctx->hdcp_cp)
> > > > > > > > >               return 0;
> > > > > > > > >
> > > > > > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > > > > > > > -             if (ctx->dp_en) {
> > > > > > > > > -                     dev_dbg(dev, "enable HDCP\n");
> > > > > > > > > -                     anx7625_hdcp_enable(ctx);
> > > > > > > > > -
> > > > > > > > > -                     queue_delayed_work(ctx->hdcp_workqueue,
> > > > > > > > > -                                        &ctx->hdcp_work,
> > > > > > > > > -                                        msecs_to_jiffies(2000));
> > > > > > > > > -             }
> > > > > > > > > -     }
> > > > > > > > > -
> > > > > > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > > > > > > -             if (ctx->hdcp_cp !=
> > > > > > DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > > > > > -                     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");
> > > > > > > > > -     }
> > > > > > > > > -
> > > > > > > > > -     if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > > > > > -             dev_err(dev, "Userspace illegal set to PROTECTION
> ENABLE\n");
> > > > > > > > > -             return -EINVAL;
> > > > > > > > > -     }
> > > > > > > > > -
> > > > > > > > >       return 0;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > @@ -2425,6 +2413,8 @@ static void
> > > > > > > > > anx7625_bridge_atomic_enable(struct
> > > > > > > > drm_bridge *bridge,
> > > > > > > > >       struct anx7625_data *ctx = bridge_to_anx7625(bridge);
> > > > > > > > >       struct device *dev = ctx->dev;
> > > > > > > > >       struct drm_connector *connector;
> > > > > > > > > +     struct drm_connector_state *conn_state;
> > > > > > > > > +     int cp;
> > > > > > > > >
> > > > > > > > >       dev_dbg(dev, "drm atomic enable\n");
> > > > > > > > >
> > > > > > > > > @@ -2439,6 +2429,32 @@ static void
> > > > > > > > > anx7625_bridge_atomic_enable(struct
> > > > > > > > drm_bridge *bridge,
> > > > > > > > >       _anx7625_hpd_polling(ctx, 5000 * 100);
> > > > > > > > >
> > > > > > > > >       anx7625_dp_start(ctx);
> > > > > > > > > +
> > > > > > > > > +     conn_state =
> > > > > > > > > + drm_atomic_get_new_connector_state(state->base.state,
> > > > > > > > > + connector);
> > > > > > > > > +
> > > > > > > > > +     if (WARN_ON(!conn_state))
> > > > > > > > > +             return;
> > > > > > > > > +
> > > > > > > > > +     cp = conn_state->content_protection;
> > > > > > > > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > > > > > > > > +             if (ctx->dp_en) {
> > > > > > > > > +                     dev_dbg(dev, "enable HDCP\n");
> > > > > > > > > +                     anx7625_hdcp_enable(ctx);
> > > > > > > > > +
> > > > > > > > > +                     queue_delayed_work(ctx->hdcp_workqueue,
> > > > > > > > > +                                        &ctx->hdcp_work,
> > > > > > > > > +                                        msecs_to_jiffies(2000));
> > > > > > > > > +             }
> > > > > > > > > +     }
> > > > > > > > > +
> > > > > > > > > +     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > > > > > > +             if (ctx->hdcp_cp !=
> > > > > > DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > > > > > > > > +                     dev_err(dev, "current CP is not ENABLED\n");
> > > > > > > > > +                     return;
> > > > > > > > > +             }
> > > > > > > > > +
> > > > > > > > > +             anx7625_hdcp_disable_and_update_cp(ctx);
> > > > > > > > > +     }
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  static void anx7625_bridge_atomic_disable(struct
> > > > > > > > > drm_bridge *bridge, @@ -2449,6 +2465,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
> > > > > >
> > > > > > --
> > > > > > With best wishes
> > > > > > Dmitry
> > > >
> > > >
> > > >
> > > > --
> > > > With best wishes
> > > > Dmitry
> >
> > --
> > With best wishes
> > Dmitry
> 
> Regards,
> Pin-yen

Dmitry Baryshkov Dec. 17, 2024, 12:30 p.m. UTC | #11

On Tue, Dec 17, 2024 at 01:50:00AM +0000, Xin Ji wrote:
> > -----Original Message-----
> > From: Pin-yen Lin <[e-mail]>
> > Sent: Monday, December 16, 2024 8:05 PM
> > To: Dmitry Baryshkov <[e-mail]>
> > Cc: Xin Ji <[e-mail]>; Andrzej Hajda <[e-mail]>;
> > Neil Armstrong <[e-mail]>; Robert Foss <[e-mail]>;
> > Laurent Pinchart <[e-mail]>; Jonas Karlman
> > <[e-mail]>; Jernej Skrabec <[e-mail]>; Maarten
> > Lankhorst <[e-mail]>; Maxime Ripard
> > <[e-mail]>; Thomas Zimmermann <[e-mail]>; David
> > Airlie <[e-mail]>; Simona Vetter <[e-mail]>; Bernie Liang
> > <[e-mail]>; Qilin Wen <[e-mail]>;
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > atomic_enable()
> > 
> > 
> > Hi Dmitry,
> > 
> > On Mon, Dec 16, 2024 at 7:59 PM Dmitry Baryshkov
> > <[e-mail]> wrote:
> > >
> > > On Mon, Dec 16, 2024 at 08:33:23AM +0000, Xin Ji wrote:
> > > > > -----Original Message-----
> > > > > From: Dmitry Baryshkov <[e-mail]>
> > > > > Sent: Friday, December 13, 2024 9:17 PM
> > > > > To: Xin Ji <[e-mail]>
> > > > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > > > <[e-mail]>; Robert Foss <[e-mail]>;
> > > > > Laurent Pinchart <[e-mail]>; Jonas
> > > > > Karlman <[e-mail]>; Jernej Skrabec
> > > > > <[e-mail]>; Maarten Lankhorst
> > > > > <[e-mail]>; Maxime Ripard
> > > > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> > > > > David Airlie <[e-mail]>; Simona Vetter <[e-mail]>;
> > > > > Bernie Liang <[e-mail]>; Qilin Wen
> > > > > <[e-mail]>; [e-mail];
> > > > > [e-mail]; linux- [e-mail]
> > > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > > > > atomic_enable()
> > > > >
> > > > > CAUTION: This email originated from outside of the organization.
> > > > > Please do not click links or open attachments unless you recognize
> > > > > the sender, and know the content is safe.
> > > > >
> > > > >
> > > > > On Fri, 13 Dec 2024 at 13:00, Xin Ji <[e-mail]> wrote:
> > > > > >
> > > > > > Hi Dmitry, sorry, I didn't clear describe the reason.
> > > > >
> > > > > Please. Do not top-post. Please paste your answer under the
> > > > > question, not somewhere at the top of the email. This allows us to
> > > > > have a more constructive dialog. Additional bonus if you can fix
> > > > > your email client to insert sensible quoting information instead of dumping
> > the headers of the original email.
> > > > Hi Dmitry, OK, sorry about it. Currently, we have problem to fetch
> > > > email from Microsoft on Ubuntu. I'll try to fix it later.
> > > > >
> > > > > >
> > > > > > Anx7625 implement DSI to DP convert behind USB Type-C port, when
> > > > > > user plug into USB Type-C Dongle with DP monitor, the user space
> > > > > > will enable HDCP feature, then kernel do HDCP and output display
> > > > > > and set HDCP content to ENABLE, but the issue happened if user
> > > > > > manually change the
> > > > > monitor's resolution later.
> > > > > >
> > > > > > Each time user change the resolution, kernel will call bridge
> > > > > > interface .atomic_disable() and .atomic_enable(), the old driver
> > > > > > will keep HDCP state to ENABLE, this is a BUG, when user change
> > > > > > the resolution, kernel must change HDCP content too (mustn't
> > > > > > keep to ENABLE),
> > > > >
> > > > > Why? Could you please point me to the corresponding documentation
> > > > > or a code path in the other driver? Preferably i915, AMD or Nouveau.
> > > > As
> > https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_connecto
> > r.c#L1423:
> > > >         - ENABLED -> DESIRED (termination of authentication) As
> > > > there is no other interface to tell anx7625 bridge driver, so the I
> > > > think best place to handle ENABLE -> DESIRED in .atomic_disable().
> > >
> > > I was looking for something like cdns_mhdp_connector_atomic_check(),
> > > which switches to UNDESIRED if there is no new CRTC. Likewise i915
> > > driver performs this in intel_hdcp_atomic_check() if there is a need
> > > for modeset.
> Hi Dmitry, the bridge driver is different with i915, anx7625 bridge driver
> only implement some passive callback interface. There is no HDCP status
> checking while do resolution switch.

Does that mean that you _have_ to reestablish HDCP after mode switching
in order for it to work?

> > 
> > I believe you mean "DESIRED" here.
> > >
> > > For the "termination of authentication" case see
> > > cdns_mhdp_hdcp_check_link(), which detects if the HDCP got disabled by
> > > HW and then updates the status accordingly.
> > >
> > > >
> > > > >
> > > > > > as DRM doc said, kernel cannot change from ENABLE to UNDESIRE,
> > > > > > so next patch, I'll change it to DESIRE in .atomic_disable().
> > >
> > > This e.g. will result in HDCP being restarted for all modesets. Is
> > > this an expected behaviour?
> > 
> > The bridge could be powered off between .atomic_disable()
> > and .atomic_enable(), though I'm not sure if this would be a concern for
> > anx7625. I'll let Xin to comment on this.
> The flow of resolution switch is:
>     -> .atomic_disable().
>         -> .atomic_enable().
>             -> HDCP status check and enable.
> As resolution switch is triggered by user space, at this moment, the HDCP
> works fine(cannot get disabled event from HW).
> So, I think, .atomic_disable() is
> the best place to do termination of authentication (turn HDCP status to DESIRED).
> This is the only entrance which can do termination of authentication, I cannot
> find others.

I'm probably still missing a reference to the standard (please excuse
me, I don't know it by heart). Is it a standard requirement to
reauthenticate on the mode switch?

BTW, while reading the anx7625 code, I noticed that there is a possible
race between .atomic_enable() / .atomic_disable() which set
ctx->connector and the hdcp_check_work_func() which reads ctx->connector
without any protection.

Xin Ji Dec. 18, 2024, 8:46 a.m. UTC | #12

> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: Tuesday, December 17, 2024 8:30 PM
> To: Xin Ji <[e-mail]>
> Cc: Pin-yen Lin <[e-mail]>; Andrzej Hajda
> <[e-mail]>; Neil Armstrong <[e-mail]>;
> Robert Foss <[e-mail]>; Laurent Pinchart
> <[e-mail]>; Jonas Karlman <[e-mail]>;
> Jernej Skrabec <[e-mail]>; Maarten Lankhorst
> <[e-mail]>; Maxime Ripard <[e-mail]>;
> Thomas Zimmermann <[e-mail]>; David Airlie
> <[e-mail]>; Simona Vetter <[e-mail]>; Bernie Liang
> <[e-mail]>; Qilin Wen <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> atomic_enable()
> 
> 
> On Tue, Dec 17, 2024 at 01:50:00AM +0000, Xin Ji wrote:
> > > -----Original Message-----
> > > From: Pin-yen Lin <[e-mail]>
> > > Sent: Monday, December 16, 2024 8:05 PM
> > > To: Dmitry Baryshkov <[e-mail]>
> > > Cc: Xin Ji <[e-mail]>; Andrzej Hajda
> > > <[e-mail]>; Neil Armstrong
> > > <[e-mail]>; Robert Foss <[e-mail]>; Laurent
> > > Pinchart <[e-mail]>; Jonas Karlman
> > > <[e-mail]>; Jernej Skrabec <[e-mail]>;
> > > Maarten Lankhorst <[e-mail]>; Maxime Ripard
> > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> David
> > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>; Bernie
> > > Liang <[e-mail]>; Qilin Wen <[e-mail]>;
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]
> > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > > atomic_enable()
> > >
> > >
> > > Hi Dmitry,
> > >
> > > On Mon, Dec 16, 2024 at 7:59 PM Dmitry Baryshkov
> > > <[e-mail]> wrote:
> > > >
> > > > On Mon, Dec 16, 2024 at 08:33:23AM +0000, Xin Ji wrote:
> > > > > > -----Original Message-----
> > > > > > From: Dmitry Baryshkov <[e-mail]>
> > > > > > Sent: Friday, December 13, 2024 9:17 PM
> > > > > > To: Xin Ji <[e-mail]>
> > > > > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > > > > <[e-mail]>; Robert Foss <[e-mail]>;
> > > > > > Laurent Pinchart <[e-mail]>; Jonas
> > > > > > Karlman <[e-mail]>; Jernej Skrabec
> > > > > > <[e-mail]>; Maarten Lankhorst
> > > > > > <[e-mail]>; Maxime Ripard
> > > > > > <[e-mail]>; Thomas Zimmermann
> <[e-mail]>;
> > > > > > David Airlie <[e-mail]>; Simona Vetter
> > > > > > <[e-mail]>; Bernie Liang <[e-mail]>;
> > > > > > Qilin Wen <[e-mail]>; [e-mail];
> > > > > > [e-mail]; linux- [e-mail]
> > > > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status
> > > > > > at
> > > > > > atomic_enable()
> > > > > >
> > > > > > CAUTION: This email originated from outside of the organization.
> > > > > > Please do not click links or open attachments unless you
> > > > > > recognize the sender, and know the content is safe.
> > > > > >
> > > > > >
> > > > > > On Fri, 13 Dec 2024 at 13:00, Xin Ji <[e-mail]> wrote:
> > > > > > >
> > > > > > > Hi Dmitry, sorry, I didn't clear describe the reason.
> > > > > >
> > > > > > Please. Do not top-post. Please paste your answer under the
> > > > > > question, not somewhere at the top of the email. This allows
> > > > > > us to have a more constructive dialog. Additional bonus if you
> > > > > > can fix your email client to insert sensible quoting
> > > > > > information instead of dumping
> > > the headers of the original email.
> > > > > Hi Dmitry, OK, sorry about it. Currently, we have problem to
> > > > > fetch email from Microsoft on Ubuntu. I'll try to fix it later.
> > > > > >
> > > > > > >
> > > > > > > Anx7625 implement DSI to DP convert behind USB Type-C port,
> > > > > > > when user plug into USB Type-C Dongle with DP monitor, the
> > > > > > > user space will enable HDCP feature, then kernel do HDCP and
> > > > > > > output display and set HDCP content to ENABLE, but the issue
> > > > > > > happened if user manually change the
> > > > > > monitor's resolution later.
> > > > > > >
> > > > > > > Each time user change the resolution, kernel will call
> > > > > > > bridge interface .atomic_disable() and .atomic_enable(), the
> > > > > > > old driver will keep HDCP state to ENABLE, this is a BUG,
> > > > > > > when user change the resolution, kernel must change HDCP
> > > > > > > content too (mustn't keep to ENABLE),
> > > > > >
> > > > > > Why? Could you please point me to the corresponding
> > > > > > documentation or a code path in the other driver? Preferably i915,
> AMD or Nouveau.
> > > > > As
> > > https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_
> > > connecto
> > > r.c#L1423:
> > > > >         - ENABLED -> DESIRED (termination of authentication) As
> > > > > there is no other interface to tell anx7625 bridge driver, so
> > > > > the I think best place to handle ENABLE -> DESIRED in .atomic_disable().
> > > >
> > > > I was looking for something like
> > > > cdns_mhdp_connector_atomic_check(),
> > > > which switches to UNDESIRED if there is no new CRTC. Likewise i915
> > > > driver performs this in intel_hdcp_atomic_check() if there is a
> > > > need for modeset.
> > Hi Dmitry, the bridge driver is different with i915, anx7625 bridge
> > driver only implement some passive callback interface. There is no
> > HDCP status checking while do resolution switch.
> 
> Does that mean that you _have_ to reestablish HDCP after mode switching in
> order for it to work?
Hi Dmitry, anx7625 bridge IC will be power down after call .atomic_disable(),
and all HDCP status will be lost on chip. So, we should reestablish HDCP again
in .atomic_enable().
> 
> > >
> > > I believe you mean "DESIRED" here.
> > > >
> > > > For the "termination of authentication" case see
> > > > cdns_mhdp_hdcp_check_link(), which detects if the HDCP got
> > > > disabled by HW and then updates the status accordingly.
> > > >
> > > > >
> > > > > >
> > > > > > > as DRM doc said, kernel cannot change from ENABLE to
> > > > > > > UNDESIRE, so next patch, I'll change it to DESIRE in .atomic_disable().
> > > >
> > > > This e.g. will result in HDCP being restarted for all modesets. Is
> > > > this an expected behaviour?
> > >
> > > The bridge could be powered off between .atomic_disable() and
> > > .atomic_enable(), though I'm not sure if this would be a concern for
> > > anx7625. I'll let Xin to comment on this.
> > The flow of resolution switch is:
> >     -> .atomic_disable().
> >         -> .atomic_enable().
> >             -> HDCP status check and enable.
> > As resolution switch is triggered by user space, at this moment, the
> > HDCP works fine(cannot get disabled event from HW).
> > So, I think, .atomic_disable() is
> > the best place to do termination of authentication (turn HDCP status to
> DESIRED).
> > This is the only entrance which can do termination of authentication,
> > I cannot find others.
> 
> I'm probably still missing a reference to the standard (please excuse me, I don't
> know it by heart). Is it a standard requirement to reauthenticate on the mode
> switch?
I'm not sure, but anx7625 have power cycle on the mode switch. All HDCP status
Will be lost.
> 
> BTW, while reading the anx7625 code, I noticed that there is a possible race
> between .atomic_enable() / .atomic_disable() which set
> ctx->connector and the hdcp_check_work_func() which reads ctx->connector
> without any protection.
OK, I'll upstream another patch.
> 
> --
> With best wishes
> Dmitry

Dmitry Baryshkov Dec. 18, 2024, 10:31 a.m. UTC | #13

On Wed, 18 Dec 2024 at 10:46, Xin Ji <[e-mail]> wrote:
>
> > -----Original Message-----
> > From: Dmitry Baryshkov <[e-mail]>
> > Sent: Tuesday, December 17, 2024 8:30 PM
> > To: Xin Ji <[e-mail]>
> > Cc: Pin-yen Lin <[e-mail]>; Andrzej Hajda
> > <[e-mail]>; Neil Armstrong <[e-mail]>;
> > Robert Foss <[e-mail]>; Laurent Pinchart
> > <[e-mail]>; Jonas Karlman <[e-mail]>;
> > Jernej Skrabec <[e-mail]>; Maarten Lankhorst
> > <[e-mail]>; Maxime Ripard <[e-mail]>;
> > Thomas Zimmermann <[e-mail]>; David Airlie
> > <[e-mail]>; Simona Vetter <[e-mail]>; Bernie Liang
> > <[e-mail]>; Qilin Wen <[e-mail]>;
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > atomic_enable()
> >
> >
> > On Tue, Dec 17, 2024 at 01:50:00AM +0000, Xin Ji wrote:
> > > > -----Original Message-----
> > > > From: Pin-yen Lin <[e-mail]>
> > > > Sent: Monday, December 16, 2024 8:05 PM
> > > > To: Dmitry Baryshkov <[e-mail]>
> > > > Cc: Xin Ji <[e-mail]>; Andrzej Hajda
> > > > <[e-mail]>; Neil Armstrong
> > > > <[e-mail]>; Robert Foss <[e-mail]>; Laurent
> > > > Pinchart <[e-mail]>; Jonas Karlman
> > > > <[e-mail]>; Jernej Skrabec <[e-mail]>;
> > > > Maarten Lankhorst <[e-mail]>; Maxime Ripard
> > > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> > David
> > > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>; Bernie
> > > > Liang <[e-mail]>; Qilin Wen <[e-mail]>;
> > > > [e-mail]; [e-mail]; linux-
> > > > [e-mail]
> > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > > > atomic_enable()
> > > >
> > > >
> > > > Hi Dmitry,
> > > >
> > > > On Mon, Dec 16, 2024 at 7:59 PM Dmitry Baryshkov
> > > > <[e-mail]> wrote:
> > > > >
> > > > > On Mon, Dec 16, 2024 at 08:33:23AM +0000, Xin Ji wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Dmitry Baryshkov <[e-mail]>
> > > > > > > Sent: Friday, December 13, 2024 9:17 PM
> > > > > > > To: Xin Ji <[e-mail]>
> > > > > > > Cc: Andrzej Hajda <[e-mail]>; Neil Armstrong
> > > > > > > <[e-mail]>; Robert Foss <[e-mail]>;
> > > > > > > Laurent Pinchart <[e-mail]>; Jonas
> > > > > > > Karlman <[e-mail]>; Jernej Skrabec
> > > > > > > <[e-mail]>; Maarten Lankhorst
> > > > > > > <[e-mail]>; Maxime Ripard
> > > > > > > <[e-mail]>; Thomas Zimmermann
> > <[e-mail]>;
> > > > > > > David Airlie <[e-mail]>; Simona Vetter
> > > > > > > <[e-mail]>; Bernie Liang <[e-mail]>;
> > > > > > > Qilin Wen <[e-mail]>; [e-mail];
> > > > > > > [e-mail]; linux- [e-mail]
> > > > > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status
> > > > > > > at
> > > > > > > atomic_enable()
> > > > > > >
> > > > > > > CAUTION: This email originated from outside of the organization.
> > > > > > > Please do not click links or open attachments unless you
> > > > > > > recognize the sender, and know the content is safe.
> > > > > > >
> > > > > > >
> > > > > > > On Fri, 13 Dec 2024 at 13:00, Xin Ji <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > Hi Dmitry, sorry, I didn't clear describe the reason.
> > > > > > >
> > > > > > > Please. Do not top-post. Please paste your answer under the
> > > > > > > question, not somewhere at the top of the email. This allows
> > > > > > > us to have a more constructive dialog. Additional bonus if you
> > > > > > > can fix your email client to insert sensible quoting
> > > > > > > information instead of dumping
> > > > the headers of the original email.
> > > > > > Hi Dmitry, OK, sorry about it. Currently, we have problem to
> > > > > > fetch email from Microsoft on Ubuntu. I'll try to fix it later.
> > > > > > >
> > > > > > > >
> > > > > > > > Anx7625 implement DSI to DP convert behind USB Type-C port,
> > > > > > > > when user plug into USB Type-C Dongle with DP monitor, the
> > > > > > > > user space will enable HDCP feature, then kernel do HDCP and
> > > > > > > > output display and set HDCP content to ENABLE, but the issue
> > > > > > > > happened if user manually change the
> > > > > > > monitor's resolution later.
> > > > > > > >
> > > > > > > > Each time user change the resolution, kernel will call
> > > > > > > > bridge interface .atomic_disable() and .atomic_enable(), the
> > > > > > > > old driver will keep HDCP state to ENABLE, this is a BUG,
> > > > > > > > when user change the resolution, kernel must change HDCP
> > > > > > > > content too (mustn't keep to ENABLE),
> > > > > > >
> > > > > > > Why? Could you please point me to the corresponding
> > > > > > > documentation or a code path in the other driver? Preferably i915,
> > AMD or Nouveau.
> > > > > > As
> > > > https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_
> > > > connecto
> > > > r.c#L1423:
> > > > > >         - ENABLED -> DESIRED (termination of authentication) As
> > > > > > there is no other interface to tell anx7625 bridge driver, so
> > > > > > the I think best place to handle ENABLE -> DESIRED in .atomic_disable().
> > > > >
> > > > > I was looking for something like
> > > > > cdns_mhdp_connector_atomic_check(),
> > > > > which switches to UNDESIRED if there is no new CRTC. Likewise i915
> > > > > driver performs this in intel_hdcp_atomic_check() if there is a
> > > > > need for modeset.
> > > Hi Dmitry, the bridge driver is different with i915, anx7625 bridge
> > > driver only implement some passive callback interface. There is no
> > > HDCP status checking while do resolution switch.
> >
> > Does that mean that you _have_ to reestablish HDCP after mode switching in
> > order for it to work?
> Hi Dmitry, anx7625 bridge IC will be power down after call .atomic_disable(),
> and all HDCP status will be lost on chip. So, we should reestablish HDCP again
> in .atomic_enable().

Finally! This phrase should be in the commit message.

> >
> > > >
> > > > I believe you mean "DESIRED" here.
> > > > >
> > > > > For the "termination of authentication" case see
> > > > > cdns_mhdp_hdcp_check_link(), which detects if the HDCP got
> > > > > disabled by HW and then updates the status accordingly.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > as DRM doc said, kernel cannot change from ENABLE to
> > > > > > > > UNDESIRE, so next patch, I'll change it to DESIRE in .atomic_disable().
> > > > >
> > > > > This e.g. will result in HDCP being restarted for all modesets. Is
> > > > > this an expected behaviour?
> > > >
> > > > The bridge could be powered off between .atomic_disable() and
> > > > .atomic_enable(), though I'm not sure if this would be a concern for
> > > > anx7625. I'll let Xin to comment on this.
> > > The flow of resolution switch is:
> > >     -> .atomic_disable().
> > >         -> .atomic_enable().
> > >             -> HDCP status check and enable.
> > > As resolution switch is triggered by user space, at this moment, the
> > > HDCP works fine(cannot get disabled event from HW).
> > > So, I think, .atomic_disable() is
> > > the best place to do termination of authentication (turn HDCP status to
> > DESIRED).
> > > This is the only entrance which can do termination of authentication,
> > > I cannot find others.
> >
> > I'm probably still missing a reference to the standard (please excuse me, I don't
> > know it by heart). Is it a standard requirement to reauthenticate on the mode
> > switch?
> I'm not sure, but anx7625 have power cycle on the mode switch. All HDCP status
> Will be lost.
> >
> > BTW, while reading the anx7625 code, I noticed that there is a possible race
> > between .atomic_enable() / .atomic_disable() which set
> > ctx->connector and the hdcp_check_work_func() which reads ctx->connector
> > without any protection.
> OK, I'll upstream another patch.
> >
> > --
> > With best wishes
> > Dmitry

Xin Ji Dec. 19, 2024, 2:33 a.m. UTC | #14

> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: Wednesday, December 18, 2024 6:32 PM
> To: Xin Ji <[e-mail]>
> Cc: Pin-yen Lin <[e-mail]>; Andrzej Hajda
> <[e-mail]>; Neil Armstrong <[e-mail]>;
> Robert Foss <[e-mail]>; Laurent Pinchart
> <[e-mail]>; Jonas Karlman <[e-mail]>;
> Jernej Skrabec <[e-mail]>; Maarten Lankhorst
> <[e-mail]>; Maxime Ripard <[e-mail]>;
> Thomas Zimmermann <[e-mail]>; David Airlie
> <[e-mail]>; Simona Vetter <[e-mail]>; Bernie Liang
> <[e-mail]>; Qilin Wen <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> atomic_enable()
> 
> 
> On Wed, 18 Dec 2024 at 10:46, Xin Ji <[e-mail]> wrote:
> >
> > > -----Original Message-----
> > > From: Dmitry Baryshkov <[e-mail]>
> > > Sent: Tuesday, December 17, 2024 8:30 PM
> > > To: Xin Ji <[e-mail]>
> > > Cc: Pin-yen Lin <[e-mail]>; Andrzej Hajda
> > > <[e-mail]>; Neil Armstrong
> > > <[e-mail]>; Robert Foss <[e-mail]>; Laurent
> > > Pinchart <[e-mail]>; Jonas Karlman
> > > <[e-mail]>; Jernej Skrabec <[e-mail]>;
> > > Maarten Lankhorst <[e-mail]>; Maxime Ripard
> > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> David
> > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>; Bernie
> > > Liang <[e-mail]>; Qilin Wen <[e-mail]>;
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]
> > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status at
> > > atomic_enable()
> > >
> > >
> > > On Tue, Dec 17, 2024 at 01:50:00AM +0000, Xin Ji wrote:
> > > > > -----Original Message-----
> > > > > From: Pin-yen Lin <[e-mail]>
> > > > > Sent: Monday, December 16, 2024 8:05 PM
> > > > > To: Dmitry Baryshkov <[e-mail]>
> > > > > Cc: Xin Ji <[e-mail]>; Andrzej Hajda
> > > > > <[e-mail]>; Neil Armstrong
> > > > > <[e-mail]>; Robert Foss <[e-mail]>;
> > > > > Laurent Pinchart <[e-mail]>; Jonas
> > > > > Karlman <[e-mail]>; Jernej Skrabec
> > > > > <[e-mail]>; Maarten Lankhorst
> > > > > <[e-mail]>; Maxime Ripard
> > > > > <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> > > David
> > > > > Airlie <[e-mail]>; Simona Vetter <[e-mail]>;
> > > > > Bernie Liang <[e-mail]>; Qilin Wen
> > > > > <[e-mail]>; [e-mail];
> > > > > [e-mail]; linux- [e-mail]
> > > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP status
> > > > > at
> > > > > atomic_enable()
> > > > >
> > > > >
> > > > > Hi Dmitry,
> > > > >
> > > > > On Mon, Dec 16, 2024 at 7:59 PM Dmitry Baryshkov
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > On Mon, Dec 16, 2024 at 08:33:23AM +0000, Xin Ji wrote:
> > > > > > > > -----Original Message-----
> > > > > > > > From: Dmitry Baryshkov <[e-mail]>
> > > > > > > > Sent: Friday, December 13, 2024 9:17 PM
> > > > > > > > To: Xin Ji <[e-mail]>
> > > > > > > > Cc: Andrzej Hajda <[e-mail]>; Neil
> > > > > > > > Armstrong <[e-mail]>; Robert Foss
> > > > > > > > <[e-mail]>; Laurent Pinchart
> > > > > > > > <[e-mail]>; Jonas Karlman
> > > > > > > > <[e-mail]>; Jernej Skrabec
> > > > > > > > <[e-mail]>; Maarten Lankhorst
> > > > > > > > <[e-mail]>; Maxime Ripard
> > > > > > > > <[e-mail]>; Thomas Zimmermann
> > > <[e-mail]>;
> > > > > > > > David Airlie <[e-mail]>; Simona Vetter
> > > > > > > > <[e-mail]>; Bernie Liang <[e-mail]>;
> > > > > > > > Qilin Wen <[e-mail]>; [e-mail];
> > > > > > > > [e-mail]; linux-
> > > > > > > > [e-mail]
> > > > > > > > Subject: Re: [PATCH v3] drm/bridge:anx7625: Update HDCP
> > > > > > > > status at
> > > > > > > > atomic_enable()
> > > > > > > >
> > > > > > > > CAUTION: This email originated from outside of the organization.
> > > > > > > > Please do not click links or open attachments unless you
> > > > > > > > recognize the sender, and know the content is safe.
> > > > > > > >
> > > > > > > >
> > > > > > > > On Fri, 13 Dec 2024 at 13:00, Xin Ji <[e-mail]> wrote:
> > > > > > > > >
> > > > > > > > > Hi Dmitry, sorry, I didn't clear describe the reason.
> > > > > > > >
> > > > > > > > Please. Do not top-post. Please paste your answer under
> > > > > > > > the question, not somewhere at the top of the email. This
> > > > > > > > allows us to have a more constructive dialog. Additional
> > > > > > > > bonus if you can fix your email client to insert sensible
> > > > > > > > quoting information instead of dumping
> > > > > the headers of the original email.
> > > > > > > Hi Dmitry, OK, sorry about it. Currently, we have problem to
> > > > > > > fetch email from Microsoft on Ubuntu. I'll try to fix it later.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Anx7625 implement DSI to DP convert behind USB Type-C
> > > > > > > > > port, when user plug into USB Type-C Dongle with DP
> > > > > > > > > monitor, the user space will enable HDCP feature, then
> > > > > > > > > kernel do HDCP and output display and set HDCP content
> > > > > > > > > to ENABLE, but the issue happened if user manually
> > > > > > > > > change the
> > > > > > > > monitor's resolution later.
> > > > > > > > >
> > > > > > > > > Each time user change the resolution, kernel will call
> > > > > > > > > bridge interface .atomic_disable() and .atomic_enable(),
> > > > > > > > > the old driver will keep HDCP state to ENABLE, this is a
> > > > > > > > > BUG, when user change the resolution, kernel must change
> > > > > > > > > HDCP content too (mustn't keep to ENABLE),
> > > > > > > >
> > > > > > > > Why? Could you please point me to the corresponding
> > > > > > > > documentation or a code path in the other driver?
> > > > > > > > Preferably i915,
> > > AMD or Nouveau.
> > > > > > > As
> > > > > https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/
> > > > > drm_
> > > > > connecto
> > > > > r.c#L1423:
> > > > > > >         - ENABLED -> DESIRED (termination of authentication)
> > > > > > > As there is no other interface to tell anx7625 bridge
> > > > > > > driver, so the I think best place to handle ENABLE -> DESIRED
> in .atomic_disable().
> > > > > >
> > > > > > I was looking for something like
> > > > > > cdns_mhdp_connector_atomic_check(),
> > > > > > which switches to UNDESIRED if there is no new CRTC. Likewise
> > > > > > i915 driver performs this in intel_hdcp_atomic_check() if
> > > > > > there is a need for modeset.
> > > > Hi Dmitry, the bridge driver is different with i915, anx7625
> > > > bridge driver only implement some passive callback interface.
> > > > There is no HDCP status checking while do resolution switch.
> > >
> > > Does that mean that you _have_ to reestablish HDCP after mode
> > > switching in order for it to work?
> > Hi Dmitry, anx7625 bridge IC will be power down after call
> > .atomic_disable(), and all HDCP status will be lost on chip. So, we
> > should reestablish HDCP again in .atomic_enable().
> 
> Finally! This phrase should be in the commit message.
Hi Dmitry, OK, I'll add it in the commit message and send new patch.
Thanks,
Xin

> 
> > >
> > > > >
> > > > > I believe you mean "DESIRED" here.
> > > > > >
> > > > > > For the "termination of authentication" case see
> > > > > > cdns_mhdp_hdcp_check_link(), which detects if the HDCP got
> > > > > > disabled by HW and then updates the status accordingly.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > > as DRM doc said, kernel cannot change from ENABLE to
> > > > > > > > > UNDESIRE, so next patch, I'll change it to DESIRE
> in .atomic_disable().
> > > > > >
> > > > > > This e.g. will result in HDCP being restarted for all
> > > > > > modesets. Is this an expected behaviour?
> > > > >
> > > > > The bridge could be powered off between .atomic_disable() and
> > > > > .atomic_enable(), though I'm not sure if this would be a concern
> > > > > for anx7625. I'll let Xin to comment on this.
> > > > The flow of resolution switch is:
> > > >     -> .atomic_disable().
> > > >         -> .atomic_enable().
> > > >             -> HDCP status check and enable.
> > > > As resolution switch is triggered by user space, at this moment,
> > > > the HDCP works fine(cannot get disabled event from HW).
> > > > So, I think, .atomic_disable() is
> > > > the best place to do termination of authentication (turn HDCP
> > > > status to
> > > DESIRED).
> > > > This is the only entrance which can do termination of
> > > > authentication, I cannot find others.
> > >
> > > I'm probably still missing a reference to the standard (please
> > > excuse me, I don't know it by heart). Is it a standard requirement
> > > to reauthenticate on the mode switch?
> > I'm not sure, but anx7625 have power cycle on the mode switch. All
> > HDCP status Will be lost.
> > >
> > > BTW, while reading the anx7625 code, I noticed that there is a
> > > possible race between .atomic_enable() / .atomic_disable() which set
> > > ctx->connector and the hdcp_check_work_func() which reads
> > > ctx->ctx->connector
> > > without any protection.
> > OK, I'll upstream another patch.
> > >
> > > --
> > > With best wishes
> > > Dmitry
> 
> 
> 
> --
> With best wishes
> Dmitry

Patch

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index a2675b121fe4..f96ce5665e8d 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;
@@ -2149,34 +2165,6 @@  static int anx7625_connector_atomic_check(struct anx7625_data *ctx,
 	if (cp == ctx->hdcp_cp)
 		return 0;
 
-	if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
-		if (ctx->dp_en) {
-			dev_dbg(dev, "enable HDCP\n");
-			anx7625_hdcp_enable(ctx);
-
-			queue_delayed_work(ctx->hdcp_workqueue,
-					   &ctx->hdcp_work,
-					   msecs_to_jiffies(2000));
-		}
-	}
-
-	if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
-		if (ctx->hdcp_cp != DRM_MODE_CONTENT_PROTECTION_ENABLED) {
-			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");
-	}
-
-	if (cp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
-		dev_err(dev, "Userspace illegal set to PROTECTION ENABLE\n");
-		return -EINVAL;
-	}
-
 	return 0;
 }
 
@@ -2425,6 +2413,8 @@  static void anx7625_bridge_atomic_enable(struct drm_bridge *bridge,
 	struct anx7625_data *ctx = bridge_to_anx7625(bridge);
 	struct device *dev = ctx->dev;
 	struct drm_connector *connector;
+	struct drm_connector_state *conn_state;
+	int cp;
 
 	dev_dbg(dev, "drm atomic enable\n");
 
@@ -2439,6 +2429,32 @@  static void anx7625_bridge_atomic_enable(struct drm_bridge *bridge,
 	_anx7625_hpd_polling(ctx, 5000 * 100);
 
 	anx7625_dp_start(ctx);
+
+	conn_state = drm_atomic_get_new_connector_state(state->base.state, connector);
+
+	if (WARN_ON(!conn_state))
+		return;
+
+	cp = conn_state->content_protection;
+	if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
+		if (ctx->dp_en) {
+			dev_dbg(dev, "enable HDCP\n");
+			anx7625_hdcp_enable(ctx);
+
+			queue_delayed_work(ctx->hdcp_workqueue,
+					   &ctx->hdcp_work,
+					   msecs_to_jiffies(2000));
+		}
+	}
+
+	if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
+		if (ctx->hdcp_cp != DRM_MODE_CONTENT_PROTECTION_ENABLED) {
+			dev_err(dev, "current CP is not ENABLED\n");
+			return;
+		}
+
+		anx7625_hdcp_disable_and_update_cp(ctx);
+	}
 }
 
 static void anx7625_bridge_atomic_disable(struct drm_bridge *bridge,
@@ -2449,6 +2465,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);