Re: [Freedreno] [PATCH v2 04/13] drm/hdcp: Expand HDCP helper library for enable/disable/check
Hi Sean
On 2021-09-28 10:33, Sean Paul wrote:
On Tue, Sep 21, 2021 at 04:34:59PM -0700, abhin...@codeaurora.org
wrote:
On 2021-09-15 13:38, Sean Paul wrote:
> From: Sean Paul
>
> This patch expands upon the HDCP helper library to manage HDCP
> enable, disable, and check.
>
> Previous to this patch, the majority of the state management and sink
> interaction is tucked inside the Intel driver with the understanding
> that once a new platform supported HDCP we could make good decisions
> about what should be centralized. With the addition of HDCP support
> for Qualcomm, it's time to migrate the protocol-specific bits of HDCP
> authentication, key exchange, and link checks to the HDCP helper.
>
> In terms of functionality, this migration is 1:1 with the Intel driver,
> however things are laid out a bit differently than with intel_hdcp.c,
> which is why this is a separate patch from the i915 transition to the
> helper. On i915, the "shim" vtable is used to account for HDMI vs. DP
> vs. DP-MST differences whereas the helper library uses a LUT to
> account for the register offsets and a remote read function to route
> the messages. On i915, storing the sink information in the source is
> done inline whereas now we use the new drm_hdcp_helper_funcs vtable
> to store and fetch information to/from source hw. Finally, instead of
> calling enable/disable directly from the driver, we'll leave that
> decision to the helper and by calling drm_hdcp_helper_atomic_commit()
> from the driver. All told, this will centralize the protocol and state
> handling in the helper, ensuring we collect all of our bugs^Wlogic
> in one place.
>
> Signed-off-by: Sean Paul
> Link:
>
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-5-s...@poorly.run
> #v1
>
> Changes in v2:
> -Fixed set-but-unused variable identified by 0-day
> ---
> drivers/gpu/drm/drm_hdcp.c | 1103
> include/drm/drm_hdcp.h | 191 +++
> 2 files changed, 1294 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> index 742313ce8f6f..47c6e6923a76 100644
> --- a/drivers/gpu/drm/drm_hdcp.c
> +++ b/drivers/gpu/drm/drm_hdcp.c
/snip
> +static void drm_hdcp_helper_check_work(struct work_struct *work)
> +{
> + struct drm_hdcp_helper_data *data =
> container_of(to_delayed_work(work),
> + struct
drm_hdcp_helper_data,
> + check_work);
> + unsigned long check_link_interval;
> +
Does this SW polling for Ri' mismatch need to be done even if the HW
is
capable of doing it
on its own?
MSM HDCP 1x HW can periodically check Ri' mismatches and issue an
interrupt
if there is a mismatch.
In that case this SW polling is not needed. So maybe check if the HW
supports polling and if so
skip this SW polling?
One could certainly change this to be HW driven. There is also an
interrupt on
Intel for DP links which [re]schedules link check in the interrupt
handler,
something similar could be done for msm. Note that even on these Intel
links
which support the CP interrupt, the worker still runs on the normal
cadence. I
haven't considered relying solely on the interrupt since I want to be
sure we
didn't miss anything.
Sean
I think we should have the support for HW polling added. From our
experience,
it has been pretty reliable for us and has a pretty consistent cadence
in alignment
with the spec. I dont quite understand why we should have both in
chipsets capable
of HW polling and am bit surprised if SW polling will catch something
which HW
polling and the subsequent interrupt has missed.
> + mutex_lock(>mutex);
> + if (data->value != DRM_MODE_CONTENT_PROTECTION_ENABLED)
> + goto out_data_mutex;
> +
> + drm_hdcp_helper_driver_lock(data);
> +
> + if (data->enabled_type == DRM_MODE_HDCP_CONTENT_TYPE1) {
> + if (drm_hdcp_hdcp2_check_link(data))
> + goto out;
> + check_link_interval = DRM_HDCP2_CHECK_PERIOD_MS;
> + } else {
> + if (drm_hdcp_hdcp1_check_link(data))
> + goto out;
> + check_link_interval = DRM_HDCP_CHECK_PERIOD_MS;
> + }
> + schedule_delayed_work(>check_work, check_link_interval);
> +
> +out:
> + drm_hdcp_helper_driver_unlock(data);
> +out_data_mutex:
> + mutex_unlock(>mutex);
> +}
/snip
Re: [Freedreno] [PATCH v2 04/13] drm/hdcp: Expand HDCP helper library for enable/disable/check
On Tue, Sep 21, 2021 at 04:34:59PM -0700, abhin...@codeaurora.org wrote:
> On 2021-09-15 13:38, Sean Paul wrote:
> > From: Sean Paul
> >
> > This patch expands upon the HDCP helper library to manage HDCP
> > enable, disable, and check.
> >
> > Previous to this patch, the majority of the state management and sink
> > interaction is tucked inside the Intel driver with the understanding
> > that once a new platform supported HDCP we could make good decisions
> > about what should be centralized. With the addition of HDCP support
> > for Qualcomm, it's time to migrate the protocol-specific bits of HDCP
> > authentication, key exchange, and link checks to the HDCP helper.
> >
> > In terms of functionality, this migration is 1:1 with the Intel driver,
> > however things are laid out a bit differently than with intel_hdcp.c,
> > which is why this is a separate patch from the i915 transition to the
> > helper. On i915, the "shim" vtable is used to account for HDMI vs. DP
> > vs. DP-MST differences whereas the helper library uses a LUT to
> > account for the register offsets and a remote read function to route
> > the messages. On i915, storing the sink information in the source is
> > done inline whereas now we use the new drm_hdcp_helper_funcs vtable
> > to store and fetch information to/from source hw. Finally, instead of
> > calling enable/disable directly from the driver, we'll leave that
> > decision to the helper and by calling drm_hdcp_helper_atomic_commit()
> > from the driver. All told, this will centralize the protocol and state
> > handling in the helper, ensuring we collect all of our bugs^Wlogic
> > in one place.
> >
> > Signed-off-by: Sean Paul
> > Link:
> > https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-5-s...@poorly.run
> > #v1
> >
> > Changes in v2:
> > -Fixed set-but-unused variable identified by 0-day
> > ---
> > drivers/gpu/drm/drm_hdcp.c | 1103
> > include/drm/drm_hdcp.h | 191 +++
> > 2 files changed, 1294 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> > index 742313ce8f6f..47c6e6923a76 100644
> > --- a/drivers/gpu/drm/drm_hdcp.c
> > +++ b/drivers/gpu/drm/drm_hdcp.c
/snip
> > +static void drm_hdcp_helper_check_work(struct work_struct *work)
> > +{
> > + struct drm_hdcp_helper_data *data =
> > container_of(to_delayed_work(work),
> > +struct
> > drm_hdcp_helper_data,
> > +check_work);
> > + unsigned long check_link_interval;
> > +
>
> Does this SW polling for Ri' mismatch need to be done even if the HW is
> capable of doing it
> on its own?
> MSM HDCP 1x HW can periodically check Ri' mismatches and issue an interrupt
> if there is a mismatch.
> In that case this SW polling is not needed. So maybe check if the HW
> supports polling and if so
> skip this SW polling?
>
One could certainly change this to be HW driven. There is also an interrupt on
Intel for DP links which [re]schedules link check in the interrupt handler,
something similar could be done for msm. Note that even on these Intel links
which support the CP interrupt, the worker still runs on the normal cadence. I
haven't considered relying solely on the interrupt since I want to be sure we
didn't miss anything.
Sean
> > + mutex_lock(>mutex);
> > + if (data->value != DRM_MODE_CONTENT_PROTECTION_ENABLED)
> > + goto out_data_mutex;
> > +
> > + drm_hdcp_helper_driver_lock(data);
> > +
> > + if (data->enabled_type == DRM_MODE_HDCP_CONTENT_TYPE1) {
> > + if (drm_hdcp_hdcp2_check_link(data))
> > + goto out;
> > + check_link_interval = DRM_HDCP2_CHECK_PERIOD_MS;
> > + } else {
> > + if (drm_hdcp_hdcp1_check_link(data))
> > + goto out;
> > + check_link_interval = DRM_HDCP_CHECK_PERIOD_MS;
> > + }
> > + schedule_delayed_work(>check_work, check_link_interval);
> > +
> > +out:
> > + drm_hdcp_helper_driver_unlock(data);
> > +out_data_mutex:
> > + mutex_unlock(>mutex);
> > +}
/snip
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [Freedreno] [PATCH v2 04/13] drm/hdcp: Expand HDCP helper library for enable/disable/check
On 2021-09-15 13:38, Sean Paul wrote:
From: Sean Paul
This patch expands upon the HDCP helper library to manage HDCP
enable, disable, and check.
Previous to this patch, the majority of the state management and sink
interaction is tucked inside the Intel driver with the understanding
that once a new platform supported HDCP we could make good decisions
about what should be centralized. With the addition of HDCP support
for Qualcomm, it's time to migrate the protocol-specific bits of HDCP
authentication, key exchange, and link checks to the HDCP helper.
In terms of functionality, this migration is 1:1 with the Intel driver,
however things are laid out a bit differently than with intel_hdcp.c,
which is why this is a separate patch from the i915 transition to the
helper. On i915, the "shim" vtable is used to account for HDMI vs. DP
vs. DP-MST differences whereas the helper library uses a LUT to
account for the register offsets and a remote read function to route
the messages. On i915, storing the sink information in the source is
done inline whereas now we use the new drm_hdcp_helper_funcs vtable
to store and fetch information to/from source hw. Finally, instead of
calling enable/disable directly from the driver, we'll leave that
decision to the helper and by calling drm_hdcp_helper_atomic_commit()
from the driver. All told, this will centralize the protocol and state
handling in the helper, ensuring we collect all of our bugs^Wlogic
in one place.
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-5-s...@poorly.run
#v1
Changes in v2:
-Fixed set-but-unused variable identified by 0-day
---
drivers/gpu/drm/drm_hdcp.c | 1103
include/drm/drm_hdcp.h | 191 +++
2 files changed, 1294 insertions(+)
diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
index 742313ce8f6f..47c6e6923a76 100644
--- a/drivers/gpu/drm/drm_hdcp.c
+++ b/drivers/gpu/drm/drm_hdcp.c
@@ -6,15 +6,20 @@
* Ramalingam C
*/
+#include
#include
#include
#include
+#include
+#include
#include
#include
#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -513,3 +518,1101 @@ bool drm_hdcp_atomic_check(struct drm_connector
*connector,
return old_hdcp != new_hdcp;
}
EXPORT_SYMBOL(drm_hdcp_atomic_check);
+
+struct drm_hdcp_helper_data {
+ struct mutex mutex;
+ struct mutex *driver_mutex;
+
+ struct drm_connector *connector;
+ const struct drm_hdcp_helper_funcs *funcs;
+
+ u64 value;
+ unsigned int enabled_type;
+
+ struct delayed_work check_work;
+ struct work_struct prop_work;
+
+ struct drm_dp_aux *aux;
+ const struct drm_hdcp_hdcp1_receiver_reg_lut *hdcp1_lut;
+};
+
+struct drm_hdcp_hdcp1_receiver_reg_lut {
+ unsigned int bksv;
+ unsigned int ri;
+ unsigned int aksv;
+ unsigned int an;
+ unsigned int ainfo;
+ unsigned int v[5];
+ unsigned int bcaps;
+ unsigned int bcaps_mask_repeater_present;
+ unsigned int bstatus;
+};
+
+static const struct drm_hdcp_hdcp1_receiver_reg_lut
drm_hdcp_hdcp1_ddc_lut = {
+ .bksv = DRM_HDCP_DDC_BKSV,
+ .ri = DRM_HDCP_DDC_RI_PRIME,
+ .aksv = DRM_HDCP_DDC_AKSV,
+ .an = DRM_HDCP_DDC_AN,
+ .ainfo = DRM_HDCP_DDC_AINFO,
+ .v = { DRM_HDCP_DDC_V_PRIME(0), DRM_HDCP_DDC_V_PRIME(1),
+ DRM_HDCP_DDC_V_PRIME(2), DRM_HDCP_DDC_V_PRIME(3),
+ DRM_HDCP_DDC_V_PRIME(4) },
+ .bcaps = DRM_HDCP_DDC_BCAPS,
+ .bcaps_mask_repeater_present = DRM_HDCP_DDC_BCAPS_REPEATER_PRESENT,
+ .bstatus = DRM_HDCP_DDC_BSTATUS,
+};
+
+static const struct drm_hdcp_hdcp1_receiver_reg_lut
drm_hdcp_hdcp1_dpcd_lut = {
+ .bksv = DP_AUX_HDCP_BKSV,
+ .ri = DP_AUX_HDCP_RI_PRIME,
+ .aksv = DP_AUX_HDCP_AKSV,
+ .an = DP_AUX_HDCP_AN,
+ .ainfo = DP_AUX_HDCP_AINFO,
+ .v = { DP_AUX_HDCP_V_PRIME(0), DP_AUX_HDCP_V_PRIME(1),
+ DP_AUX_HDCP_V_PRIME(2), DP_AUX_HDCP_V_PRIME(3),
+ DP_AUX_HDCP_V_PRIME(4) },
+ .bcaps = DP_AUX_HDCP_BCAPS,
+ .bcaps_mask_repeater_present = DP_BCAPS_REPEATER_PRESENT,
+
+ /*
+* For some reason the HDMI and DP HDCP specs call this register
+ * definition by different names. In the HDMI spec, it's called
BSTATUS,
+* but in DP it's called BINFO.
+*/
+ .bstatus = DP_AUX_HDCP_BINFO,
+};
+
+static int drm_hdcp_remote_ddc_read(struct i2c_adapter *i2c,
+ unsigned int offset, u8 *value, size_t len)
+{
+ int ret;
+ u8 start = offset & 0xff;
+ struct i2c_msg msgs[] = {
+ {
+ .addr = DRM_HDCP_DDC_ADDR,
+ .flags = 0,
+ .len = 1,
+ .buf = ,
+ },
+ {
+ .addr = DRM_HDCP_DDC_ADDR,
+
[Freedreno] [PATCH v2 04/13] drm/hdcp: Expand HDCP helper library for enable/disable/check
From: Sean Paul
This patch expands upon the HDCP helper library to manage HDCP
enable, disable, and check.
Previous to this patch, the majority of the state management and sink
interaction is tucked inside the Intel driver with the understanding
that once a new platform supported HDCP we could make good decisions
about what should be centralized. With the addition of HDCP support
for Qualcomm, it's time to migrate the protocol-specific bits of HDCP
authentication, key exchange, and link checks to the HDCP helper.
In terms of functionality, this migration is 1:1 with the Intel driver,
however things are laid out a bit differently than with intel_hdcp.c,
which is why this is a separate patch from the i915 transition to the
helper. On i915, the "shim" vtable is used to account for HDMI vs. DP
vs. DP-MST differences whereas the helper library uses a LUT to
account for the register offsets and a remote read function to route
the messages. On i915, storing the sink information in the source is
done inline whereas now we use the new drm_hdcp_helper_funcs vtable
to store and fetch information to/from source hw. Finally, instead of
calling enable/disable directly from the driver, we'll leave that
decision to the helper and by calling drm_hdcp_helper_atomic_commit()
from the driver. All told, this will centralize the protocol and state
handling in the helper, ensuring we collect all of our bugs^Wlogic
in one place.
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-5-s...@poorly.run
#v1
Changes in v2:
-Fixed set-but-unused variable identified by 0-day
---
drivers/gpu/drm/drm_hdcp.c | 1103
include/drm/drm_hdcp.h | 191 +++
2 files changed, 1294 insertions(+)
diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
index 742313ce8f6f..47c6e6923a76 100644
--- a/drivers/gpu/drm/drm_hdcp.c
+++ b/drivers/gpu/drm/drm_hdcp.c
@@ -6,15 +6,20 @@
* Ramalingam C
*/
+#include
#include
#include
#include
+#include
+#include
#include
#include
#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -513,3 +518,1101 @@ bool drm_hdcp_atomic_check(struct drm_connector
*connector,
return old_hdcp != new_hdcp;
}
EXPORT_SYMBOL(drm_hdcp_atomic_check);
+
+struct drm_hdcp_helper_data {
+ struct mutex mutex;
+ struct mutex *driver_mutex;
+
+ struct drm_connector *connector;
+ const struct drm_hdcp_helper_funcs *funcs;
+
+ u64 value;
+ unsigned int enabled_type;
+
+ struct delayed_work check_work;
+ struct work_struct prop_work;
+
+ struct drm_dp_aux *aux;
+ const struct drm_hdcp_hdcp1_receiver_reg_lut *hdcp1_lut;
+};
+
+struct drm_hdcp_hdcp1_receiver_reg_lut {
+ unsigned int bksv;
+ unsigned int ri;
+ unsigned int aksv;
+ unsigned int an;
+ unsigned int ainfo;
+ unsigned int v[5];
+ unsigned int bcaps;
+ unsigned int bcaps_mask_repeater_present;
+ unsigned int bstatus;
+};
+
+static const struct drm_hdcp_hdcp1_receiver_reg_lut drm_hdcp_hdcp1_ddc_lut = {
+ .bksv = DRM_HDCP_DDC_BKSV,
+ .ri = DRM_HDCP_DDC_RI_PRIME,
+ .aksv = DRM_HDCP_DDC_AKSV,
+ .an = DRM_HDCP_DDC_AN,
+ .ainfo = DRM_HDCP_DDC_AINFO,
+ .v = { DRM_HDCP_DDC_V_PRIME(0), DRM_HDCP_DDC_V_PRIME(1),
+ DRM_HDCP_DDC_V_PRIME(2), DRM_HDCP_DDC_V_PRIME(3),
+ DRM_HDCP_DDC_V_PRIME(4) },
+ .bcaps = DRM_HDCP_DDC_BCAPS,
+ .bcaps_mask_repeater_present = DRM_HDCP_DDC_BCAPS_REPEATER_PRESENT,
+ .bstatus = DRM_HDCP_DDC_BSTATUS,
+};
+
+static const struct drm_hdcp_hdcp1_receiver_reg_lut drm_hdcp_hdcp1_dpcd_lut = {
+ .bksv = DP_AUX_HDCP_BKSV,
+ .ri = DP_AUX_HDCP_RI_PRIME,
+ .aksv = DP_AUX_HDCP_AKSV,
+ .an = DP_AUX_HDCP_AN,
+ .ainfo = DP_AUX_HDCP_AINFO,
+ .v = { DP_AUX_HDCP_V_PRIME(0), DP_AUX_HDCP_V_PRIME(1),
+ DP_AUX_HDCP_V_PRIME(2), DP_AUX_HDCP_V_PRIME(3),
+ DP_AUX_HDCP_V_PRIME(4) },
+ .bcaps = DP_AUX_HDCP_BCAPS,
+ .bcaps_mask_repeater_present = DP_BCAPS_REPEATER_PRESENT,
+
+ /*
+* For some reason the HDMI and DP HDCP specs call this register
+* definition by different names. In the HDMI spec, it's called BSTATUS,
+* but in DP it's called BINFO.
+*/
+ .bstatus = DP_AUX_HDCP_BINFO,
+};
+
+static int drm_hdcp_remote_ddc_read(struct i2c_adapter *i2c,
+ unsigned int offset, u8 *value, size_t len)
+{
+ int ret;
+ u8 start = offset & 0xff;
+ struct i2c_msg msgs[] = {
+ {
+ .addr = DRM_HDCP_DDC_ADDR,
+ .flags = 0,
+ .len = 1,
+ .buf = ,
+ },
+ {
+ .addr = DRM_HDCP_DDC_ADDR,
+ .flags = I2C_M_RD,
+
