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@xxxxxxxxxx
>
> > }
> >
> > io->phy = devm_phy_get(&pdev->dev, "dp");
--
Sean Paul, Software Engineer, Google / Chromium OS
