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

[Xin Ji]

Hi Dmitry and Pin-yen Lin, thanks for the review.

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

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