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 <dmitry.baryshkov@xxxxxxxxxx>
> Sent: Friday, December 13, 2024 6:47 PM
> To: Xin Ji <xji@xxxxxxxxxxxxxxxx>
> Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>; Neil Armstrong
> <neil.armstrong@xxxxxxxxxx>; Robert Foss <rfoss@xxxxxxxxxx>; Laurent Pinchart
> <Laurent.pinchart@xxxxxxxxxxxxxxxx>; Jonas Karlman <jonas@xxxxxxxxx>;
> Jernej Skrabec <jernej.skrabec@xxxxxxxxx>; Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard <mripard@xxxxxxxxxx>;
> Thomas Zimmermann <tzimmermann@xxxxxxx>; David Airlie
> <airlied@xxxxxxxxx>; Simona Vetter <simona@xxxxxxxx>; Bernie Liang
> <bliang@xxxxxxxxxxxxxxxx>; Qilin Wen <qwen@xxxxxxxxxxxxxxxx>;
> treapking@xxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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 <dmitry.baryshkov@xxxxxxxxxx>
> > > Sent: Thursday, December 12, 2024 5:18 PM
> > > To: Xin Ji <xji@xxxxxxxxxxxxxxxx>
> > > Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>; Neil Armstrong
> > > <neil.armstrong@xxxxxxxxxx>; Robert Foss <rfoss@xxxxxxxxxx>; Laurent
> > > Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>; Jonas Karlman
> > > <jonas@xxxxxxxxx>; Jernej Skrabec <jernej.skrabec@xxxxxxxxx>;
> > > Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard
> > > <mripard@xxxxxxxxxx>; Thomas Zimmermann <tzimmermann@xxxxxxx>;
> David
> > > Airlie <airlied@xxxxxxxxx>; Simona Vetter <simona@xxxxxxxx>; Bernie
> > > Liang <bliang@xxxxxxxxxxxxxxxx>; Qilin Wen <qwen@xxxxxxxxxxxxxxxx>;
> > > treapking@xxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-
> > > kernel@xxxxxxxxxxxxxxx
> > > 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 <xji@xxxxxxxxxxxxxxxx>
> > > > ---
> > > > 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
