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

[Sean Paul <sean@poorly.run>]

On Tue, Sep 21, 2021 at 07:25:41PM -0700, abhinavk@codeaurora.org wrote:
> On 2021-09-15 13:38, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > This patch adds HDCP 1.x support to msm DP connectors using the new HDCP
> > helpers.
> > 
> > Cc: Stephen Boyd <swboyd@chromium.org>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > Link:
> > https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-15-sean@poorly.run
> > #v1
> > 
> > Changes in v2:
> > -Squash [1] into this patch with the following changes (Stephen)
> >   -Update the sc7180 dtsi file
> >   -Remove resource names and just use index (Stephen)
> > 
> 
> 
> > [1]
> > https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-14-sean@poorly.run
> > ---

/snip

> > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> > index 904535eda0c4..98731fd262d6 100644
> > --- a/drivers/gpu/drm/msm/Makefile
> > +++ b/drivers/gpu/drm/msm/Makefile
> > @@ -109,6 +109,7 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
> >  	dp/dp_ctrl.o \
> >  	dp/dp_display.o \
> >  	dp/dp_drm.o \
> > +	dp/dp_hdcp.o \
> >  	dp/dp_hpd.o \
> >  	dp/dp_link.o \
> >  	dp/dp_panel.o \
> > 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

/snip

> > +static ssize_t dp_hdcp_key_write(struct file *file, const char __user
> > *ubuf,
> > +				 size_t len, loff_t *offp)
> > +{
> > +	char *input_buffer;
> > +	int ret = 0;
> > +	struct dp_debug_private *debug = file->private_data;
> > +	struct drm_device *dev;
> > +
> > +	dev = debug->drm_dev;
> > +
> > +	if (len != (DRM_HDCP_KSV_LEN + DP_HDCP_NUM_KEYS * DP_HDCP_KEY_LEN))
> > +		return -EINVAL;
> > +
> > +	if (!debug->hdcp)
> > +		return -ENOENT;
> > +
> > +	input_buffer = memdup_user_nul(ubuf, len);
> > +	if (IS_ERR(input_buffer))
> > +		return PTR_ERR(input_buffer);
> > +
> > +	ret = dp_hdcp_ingest_key(debug->hdcp, input_buffer, len);
> > +
> > +	kfree(input_buffer);
> > +	if (ret < 0) {
> > +		DRM_ERROR("Could not ingest HDCP key, ret=%d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	*offp += len;
> > +	return len;
> > +}
> 
> It seems like the HDCP keys written using debugfs, just for my
> understanding,
> are you storing this in some secure partition and the usermode reads from it
> and writes them here?
> 

We have not sorted out the userspace side of HDCP enablement yet, so it remains
to be seen whether the keys will be injected via debugfs/firmware file/property.

/snip

> > +static int dp_connector_atomic_check(struct drm_connector *connector,
> > +				     struct drm_atomic_state *state)
> > +{
> > +	struct drm_connector_state *conn_state;
> > +	struct dp_connector_state *dp_state;
> > +
> > +	conn_state = drm_atomic_get_new_connector_state(state, connector);
> > +	dp_state = to_dp_connector_state(conn_state);
> > +
> > +	dp_state->hdcp_transition = drm_hdcp_atomic_check(connector, state);
> 
> I have a general question related to the transition flag and overall tying
> the HDCP
> enable and authentication to the commit.
> So lets say there is a case where the driver needs to disable HDCP. It could
> be due
> to link integrity failure OR some other error condition which usermode is
> not aware of.
> In that case, we will set this hdcp_transition to true but in the next
> commit we will
> actually do the authentication. What if usermode doesnt issue a new frame?
> This question arises because currently the link intergrity check is done
> using SW polling
> in the previous patchset. But as I had commented there, this occurs in HW
> for us.
> I dont see that isr itself in this patchset. So wanted to understand if
> thats part of this
> approach to still tie it with commit.
> 
> So if we go with the HW polling based approach which is the preferred
> method, we need to
> untie this from the commit.
> 

In the case of error, the worker will detect it and try to re-authenticate. If
the re-authentication is successful, userspace will continue to be unaware and
everything will keep working. If re-authentication is unsuccessful, the worker
will update the property value and issue a uevent to userspace. So HDCP
enablement is only tied to commits when the property value is changing as a
result of userspace.

Regarding SW vs HW link checks, I don't think there's any difference in efficacy
between them. If HW can be relied on to issue an interrupt in failure cases, a
follow-up set allowing for this seems like a great idea.

> > +
> > +	return 0;
> > +}

/snip

> > +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);
> 
> I think all of these are TZ_*** registers. So the separation of write_hdcp()
> Vs write_hdcp_tz()
> is not very clear to me.
> Maybe change the write APIs to something like dp_hdcp_write_hdcp_tz() for
> the first address space
> and dp_hdcp_write_hdcp_tz_hlos() for the other one?
> 

Will do in v3, thank you for the suggestion.

> > +	wmb();
> > +
> > +	dp_hdcp_write_dp(hdcp, DP_HDCP_ENTROPY_CTRL0, get_random_u32());
> > +	dp_hdcp_write_dp(hdcp, DP_HDCP_ENTROPY_CTRL1, get_random_u32());
> > +	wmb();
> > +
> > +out:
> > +	mutex_unlock(&hdcp->key_lock);
> > +	return ret;
> > +}
> > +
> > +static int dp_hdcp_hdcp2_capable(struct drm_connector *connector,
> > bool *capable)
> > +{
> > +	*capable = false;
> > +	return 0;
> > +}
> > +
> > +static int dp_hdcp_hdcp1_read_an_aksv(struct drm_connector *connector,
> > +				      u32 *an, u32 *aksv)
> > +{
> > +	struct dp_hdcp *hdcp = dp_display_connector_to_hdcp(connector);
> > +	bool keys_valid;
> > +	int ret;
> > +	u32 val;
> > +
> > +	dp_hdcp_write_dp(hdcp, DP_HDCP_CTRL, 1);
> > +
> > +	ret = read_poll_timeout(dp_hdcp_are_keys_valid, keys_valid,
> > keys_valid,
> > +				20 * 1000, 10 * 1000, false, connector);
> > +	if (ret) {
> > +		drm_err(hdcp->dev, "HDCP keys invalid %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Clear AInfo */
> > +	dp_hdcp_write_dp(hdcp, DP_HDCP_RCVPORT_DATA4, 0);
> > +
> > +	aksv[0] = dp_hdcp_read_dp(hdcp, DP_HDCP_RCVPORT_DATA3);
> > +	aksv[1] = GENMASK(7, 0) & dp_hdcp_read_dp(hdcp,
> > DP_HDCP_RCVPORT_DATA4);
> > +
> > +	ret = read_poll_timeout(dp_hdcp_read_dp, val,
> > +				(val & DP_HDCP_AN_READY_MASK) == DP_HDCP_AN_READY_MASK,
> > +				100, 10 * 1000, false, hdcp, DP_HDCP_STATUS);
> > +	if (ret) {
> > +		drm_err(hdcp->dev, "AN failed to become ready %x/%d\n", val, ret);
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * Get An from hardware, for unknown reasons we need to read the reg
> > +	 * twice to get valid data.
> > +	 */
> > +	dp_hdcp_read_dp(hdcp, DP_HDCP_RCVPORT_DATA5);
> > +	an[0] = dp_hdcp_read_dp(hdcp, DP_HDCP_RCVPORT_DATA5);
> > +
> > +	dp_hdcp_read_dp(hdcp, DP_HDCP_RCVPORT_DATA6);
> > +	an[1] = dp_hdcp_read_dp(hdcp, DP_HDCP_RCVPORT_DATA6);
> 
> Yes its true, but we also have a 1 microsec delay between the first and
> second one.
> So I would certainly preserve that.

Will do in v3, thank you for the suggestion.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int dp_hdcp_hdcp1_store_receiver_info(struct drm_connector
> > *connector,
> > +					     u32 *ksv, u32 status, u8 bcaps,
> > +					     bool is_repeater)
> > +{
> > +	struct dp_hdcp *hdcp = dp_display_connector_to_hdcp(connector);
> > +	u32 val;
> > +
> > +	dp_hdcp_write_tz(hdcp, HDCP_SEC_DP_TZ_HV_HLOS_HDCP_RCVPORT_DATA0,
> > +			 ksv[0]);
> > +	dp_hdcp_write_tz(hdcp, HDCP_SEC_DP_TZ_HV_HLOS_HDCP_RCVPORT_DATA1,
> > +			 ksv[1]);
> > +
> > +	val = ((status & GENMASK(15, 0)) << 8) | (bcaps & GENMASK(7, 0));
> > +
> > +	dp_hdcp_write_tz(hdcp, HDCP_SEC_DP_TZ_HV_HLOS_HDCP_RCVPORT_DATA12,
> > val);
> > +
> 
> Cant this entire API be skipped for non-repeater cases from the hdcp lib
> layer?
> You can write the bcaps to this earlier and write the bstatus only if its a
> repeater.

Could you expand on the benefits of this?

> 
> > +	return 0;
> > +}

/snip

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