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

[Dmitry Baryshkov]

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

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

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

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

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

-- 
With best wishes
Dmitry