On Thu, Sep 16, 2021 at 11:00:25PM -0700, Stephen Boyd wrote:
Quoting Sean Paul (2021-09-15 13:38:32)
/snip
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
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
+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
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
@@ -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.or...
} io->phy = devm_phy_get(&pdev->dev, "dp");