On Thu, Sep 16, 2021 at 11:00:25PM -0700, Stephen Boyd wrote:

Quoting Sean Paul (2021-09-15 13:38:32)

/snip

quoted

diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c b/drivers/gpu/drm/msm/dp/dp_debug.c
index 2f6247e80e9d..de16fca8782a 100644
--- a/drivers/gpu/drm/msm/dp/dp_debug.c
+++ b/drivers/gpu/drm/msm/dp/dp_debug.c

@@ -8,6 +8,7 @@
 #include <linux/debugfs.h>
 #include <drm/drm_connector.h>
 #include <drm/drm_file.h>
+#include <drm/drm_hdcp.h>

 #include "dp_parser.h"
 #include "dp_catalog.h"

@@ -15,6 +16,7 @@
 #include "dp_ctrl.h"
 #include "dp_debug.h"
 #include "dp_display.h"
+#include "dp_hdcp.h"

 #define DEBUG_NAME "msm_dp"

@@ -24,6 +26,7 @@ struct dp_debug_private {
        struct dp_usbpd *usbpd;
        struct dp_link *link;
        struct dp_panel *panel;
+       struct dp_hdcp *hdcp;
        struct drm_connector **connector;
        struct device *dev;
        struct drm_device *drm_dev;

@@ -349,6 +352,38 @@ static int dp_test_active_open(struct inode *inode,
                        inode->i_private);
 }

+static ssize_t dp_hdcp_key_write(struct file *file, const char __user *ubuf,

Is this the API that userspace is going to use to set the key? Or a
simple debug interface that's used to test this code out? I hope it's a
debugging aid and not the normal flow given that it's through debugfs.

At the moment, generic UAPI is not useful beyond msm-based CrOS devices, which
is not really a burden upstream should be carrying. On other platforms
(including qc-based Android devices), the key injection is done in HW. As such,
I'm tempted to kick key injection UAPI down the road.

Once I finish the userspace client in CrOS, I can upload the UAPI for folks to
comment on.

/snip

quoted

diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 8b47cdabb67e..421268e47f30 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h

quoted

+static int dp_hdcp_load_keys(struct drm_connector *connector)
+{
+       struct dp_hdcp *hdcp = dp_display_connector_to_hdcp(connector);
+       struct dp_hdcp_key *key;
+       int i, ret = 0;
+
+       mutex_lock(&hdcp->key_lock);
+
+       key = hdcp->key;
+
+       if (!key->valid) {
+               ret = -ENOENT;
+               goto out;
+       }
+
+       dp_hdcp_write_dp(hdcp, DP_HDCP_SW_LOWER_AKSV, key->ksv.words[0]);
+       dp_hdcp_write_dp(hdcp, DP_HDCP_SW_UPPER_AKSV, key->ksv.words[1]);
+
+       for (i = 0; i < DP_HDCP_NUM_KEYS; i++) {
+               dp_hdcp_write_hdcp(hdcp, DP_HDCP_KEY_LSB(i),
+                                  key->keys[i].words[0]);
+               dp_hdcp_write_hdcp(hdcp, DP_HDCP_KEY_MSB(i),
+                                  key->keys[i].words[1]);
+       }
+
+       dp_hdcp_write_hdcp(hdcp, DP_HDCP_KEY_VALID, DP_HDCP_SW_KEY_VALID);
+       wmb();

What are the wmb()s for? Can you add a comment indicating what we're
trying to fix by having them?

I think these were left over from testing (when things weren't working for me).
Will remove in the next version, thanks for catching!

/snip

quoted

diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index 0519dd3ac3c3..75a163b0b5af 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c

/snip

quoted

@@ -55,6 +55,8 @@ static void dp_parser_unmap_io_resources(struct dp_parser *parser)
 {
        struct dp_io *io = &parser->io;

+       msm_dss_iounmap(&io->hdcp_tz);
+       msm_dss_iounmap(&io->hdcp_key);
        msm_dss_iounmap(&io->dp_controller);
 }

@@ -64,10 +66,20 @@ static int dp_parser_ctrl_res(struct dp_parser *parser)
        struct platform_device *pdev = parser->pdev;
        struct dp_io *io = &parser->io;

-       rc = msm_dss_ioremap(pdev, &io->dp_controller);
-       if (rc) {
-               DRM_ERROR("unable to remap dp io resources, rc=%d\n", rc);
+       rc = msm_dss_ioremap(pdev, &io->dp_controller, 0);
+       if (rc)
                goto err;
+
+       rc = msm_dss_ioremap(pdev, &io->hdcp_key, 1);
+       if (rc) {
+               io->hdcp_key.base = NULL;
+               io->hdcp_key.len = 0;
+       }
+
+       rc = msm_dss_ioremap(pdev, &io->hdcp_tz, 2);
+       if (rc) {
+               io->hdcp_tz.base = NULL;
+               io->hdcp_tz.len = 0;

Bjorn is trying to split the single io region apart into 4 different
regions[1]. This would add two more io regions. Maybe this should come
after those patches and be indexed later? I worry about needing to add
more register properties later on though. Maybe a better approach would
be to make them mandatory for certain compatible strings instead.

Thanks for the heads up, I'll look into adding a compatible string.

All your other comments will be addressed in v3.

Sean

[1] https://lore.kernel.org/r/20210825222557.1499104-6-bjorn.andersson@linaro.org (local)

quoted

        }

        io->phy = devm_phy_get(&pdev->dev, "dp");

-- 
Sean Paul, Software Engineer, Google / Chromium OS