| Message ID | 20241209064632.1705578-1-xji@analogixsemi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/bridge:anx7625: Update HDCP status at atomic_disable() | expand |
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? 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@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. > } > > 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 >
Hi Dimitry, Thanks for the review. On Wed, Dec 11, 2024 at 5:44 PM Dmitry Baryshkov <dmitry.baryshkov@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@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. Maybe we can keep some of the checks in .atomic_check(), but I doubt if those logics actually make sense. > > > } > > > > 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
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@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@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
Hi Dmitry and Pin-yen Lin, thanks for the review. > -----Original Message----- > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Sent: Thursday, December 12, 2024 7:20 AM > To: Pin-yen Lin <treapking@chromium.org> > Cc: Xin Ji <xji@analogixsemi.com>; Andrzej Hajda <andrzej.hajda@intel.com>; > Neil Armstrong <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; > Laurent Pinchart <Laurent.pinchart@ideasonboard.com>; Jonas Karlman > <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; Maarten > Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard > <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David > Airlie <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Bernie Liang > <bliang@analogixsemi.com>; Qilin Wen <qwen@analogixsemi.com>; > treapking@google.com; dri-devel@lists.freedesktop.org; linux- > kernel@vger.kernel.org > 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@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@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. 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
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); } 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);
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. 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(). Signed-off-by: Xin Ji <xji@analogixsemi.com> --- drivers/gpu/drm/bridge/analogix/anx7625.c | 25 ++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)