Re: [PATCH v2 13/13] drm/msm: Implement HDCP 1.x using the new drm HDCP helpers

[Sean Paul <sean@poorly.run>]

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.org
> 
> >         }
> >
> >         io->phy = devm_phy_get(&pdev->dev, "dp");

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