[PATCH v3] drm/bridge:anx7625: Update HDCP status at atomic_enable()
Xin Ji
xji at analogixsemi.com
Fri Dec 13 11:00:39 UTC 2024
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 at linaro.org>
> Sent: Friday, December 13, 2024 6:47 PM
> To: Xin Ji <xji at analogixsemi.com>
> Cc: Andrzej Hajda <andrzej.hajda at intel.com>; Neil Armstrong
> <neil.armstrong at linaro.org>; Robert Foss <rfoss at kernel.org>; Laurent Pinchart
> <Laurent.pinchart at ideasonboard.com>; Jonas Karlman <jonas at kwiboo.se>;
> Jernej Skrabec <jernej.skrabec at gmail.com>; Maarten Lankhorst
> <maarten.lankhorst at linux.intel.com>; Maxime Ripard <mripard at kernel.org>;
> Thomas Zimmermann <tzimmermann at suse.de>; David Airlie
> <airlied at gmail.com>; Simona Vetter <simona at ffwll.ch>; Bernie Liang
> <bliang at analogixsemi.com>; Qilin Wen <qwen at analogixsemi.com>;
> treapking at google.com; dri-devel at lists.freedesktop.org; linux-
> kernel at vger.kernel.org
> 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 at linaro.org>
> > > Sent: Thursday, December 12, 2024 5:18 PM
> > > To: Xin Ji <xji at analogixsemi.com>
> > > Cc: Andrzej Hajda <andrzej.hajda at intel.com>; Neil Armstrong
> > > <neil.armstrong at linaro.org>; Robert Foss <rfoss at kernel.org>; Laurent
> > > Pinchart <Laurent.pinchart at ideasonboard.com>; Jonas Karlman
> > > <jonas at kwiboo.se>; Jernej Skrabec <jernej.skrabec at gmail.com>;
> > > Maarten Lankhorst <maarten.lankhorst at linux.intel.com>; Maxime Ripard
> > > <mripard at kernel.org>; Thomas Zimmermann <tzimmermann at suse.de>;
> David
> > > Airlie <airlied at gmail.com>; Simona Vetter <simona at ffwll.ch>; Bernie
> > > Liang <bliang at analogixsemi.com>; Qilin Wen <qwen at analogixsemi.com>;
> > > treapking at google.com; dri-devel at lists.freedesktop.org; linux-
> > > kernel at vger.kernel.org
> > > 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 at analogixsemi.com>
> > > > ---
> > > > 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
More information about the dri-devel
mailing list