[Intel-gfx] [PATCH v7 15/17] drm/mst: Add support for QUERY_STREAM_ENCRYPTION_STATUS MST sideband message
Sean Paul
sean at poorly.run
Tue Aug 11 17:21:38 UTC 2020
On Thu, Jul 9, 2020 at 9:09 AM Anshuman Gupta <anshuman.gupta at intel.com> wrote:
>
> On 2020-07-02 at 20:07:36 +0530, Anshuman Gupta wrote:
> > On 2020-06-30 at 12:48:34 -0400, Sean Paul wrote:
> > > On Tue, Jun 30, 2020 at 10:21 AM Anshuman Gupta
> > > <anshuman.gupta at intel.com> wrote:
> > > >
> > > > On 2020-06-23 at 21:29:05 +0530, Sean Paul wrote:
> > > > Hi Sean,
> > > > I am new to DP MST stuff, I am looking to DP MST spec DP v1.2a.
> > > > I have looked the entire series, i will take up this opportunity to review
> > > > the series from HDCP over DP MST POV.
> > > > I think theoretically this series should work or Gen12 as well, as DP MST streams
> > > > are getting encrypted by QUERY_STREAM_ENCRYPTION_STATUS reply tranaction msg
> > > > (generating Stream State Signature L’).
> > > > I will test this on Gen12 H/W with DP MST support and will provide my inputs.
> > > >
> > > > Meanwhile while going through DP MST v1.2a specs(Page 262) came to know about
> > > > a DP irq vector LINK_SERVICE_IRQ_VECTOR_ESI0 (02005h),
> > > > Bit 2 : STREAM_STATUS_CHANGED.
> > > > When this bit set to ‘1’ indicates the source must re-check the Stream Status
> > > > with the QUERY_STREAM_ENCRYPTION_STATUS message.
> > > > Currently i feel this irq support is missing, do we require to support
> > > > above IRQ vector for DP MST stream encryption.
> > > >
> > >
> > > Hi Anshuman,
> > > Thank you for your comments.
> > >
> > > QUERY_STREAM_ENCRYPTION_STATUS is not necessary for HDCP 1.x, I added
> > > this as a safety check to ensure that the streams were being
> > > encrypted. As such, the existing integrity checks in place for DP are
> > > sufficient to satisfy spec. When HDCP 2.2 support is added for MST,
> > > handling QSES will need to be addressed to meet spec. Note also that
> > > we're not validating the QSES signature for the same reason.
> > Thanks sean for the explanation,
> > overall patch looks good to me but i have couple of doubt see below.
> > >
> > > Sean
> > >
> > >
> > > > Thanks,
> > > > Anshuman Gupta.
> > > >
> > > > > From: Sean Paul <seanpaul at chromium.org>
> > > > >
> > > > > Used to query whether an MST stream is encrypted or not.
> > > > >
> > > > > Signed-off-by: Sean Paul <seanpaul at chromium.org>
> > > > >
> > > > > Link: https://patchwork.freedesktop.org/patch/msgid/20200218220242.107265-14-sean@poorly.run #v4
> > > > > Link: https://patchwork.freedesktop.org/patch/msgid/20200305201236.152307-15-sean@poorly.run #v5
> > > > > Link: https://patchwork.freedesktop.org/patch/msgid/20200429195502.39919-15-sean@poorly.run #v6
> > > > >
> > > > > Changes in v4:
> > > > > -Added to the set
> > > > > Changes in v5:
> > > > > -None
> > > > > Changes in v6:
> > > > > -Use FIELD_PREP to generate request buffer bitfields (Lyude)
> > > > > -Add mst selftest and dump/decode_sideband_req for QSES (Lyude)
> > > > > Changes in v7:
> > > > > -None
> > > > > ---
> > > > > drivers/gpu/drm/drm_dp_mst_topology.c | 142 ++++++++++++++++++
> > > > > .../drm/selftests/test-drm_dp_mst_helper.c | 17 +++
> > > > > include/drm/drm_dp_helper.h | 3 +
> > > > > include/drm/drm_dp_mst_helper.h | 44 ++++++
> > > > > 4 files changed, 206 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > index b2f5a84b4cfb..fc68478eaeb4 100644
> > > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > @@ -20,11 +20,13 @@
> > > > > * OF THIS SOFTWARE.
> > > > > */
> > > > >
> > > > > +#include <linux/bitfield.h>
> > > > > #include <linux/delay.h>
> > > > > #include <linux/errno.h>
> > > > > #include <linux/i2c.h>
> > > > > #include <linux/init.h>
> > > > > #include <linux/kernel.h>
> > > > > +#include <linux/random.h>
> > > > > #include <linux/sched.h>
> > > > > #include <linux/seq_file.h>
> > > > > #include <linux/iopoll.h>
> > > > > @@ -419,6 +421,22 @@ drm_dp_encode_sideband_req(const struct drm_dp_sideband_msg_req_body *req,
> > > > > memcpy(&buf[idx], req->u.i2c_write.bytes, req->u.i2c_write.num_bytes);
> > > > > idx += req->u.i2c_write.num_bytes;
> > > > > break;
> > > > > + case DP_QUERY_STREAM_ENC_STATUS: {
> > > > > + const struct drm_dp_query_stream_enc_status *msg;
> > > > > +
> > > > > + msg = &req->u.enc_status;
> > > > > + buf[idx] = msg->stream_id;
> > > > > + idx++;
> > > > > + memcpy(&buf[idx], msg->client_id, sizeof(msg->client_id));
> > > > > + idx += sizeof(msg->client_id);
> > > > > + buf[idx] = 0;
> > > > > + buf[idx] |= FIELD_PREP(GENMASK(1, 0), msg->stream_event);
> > > > > + buf[idx] |= msg->valid_stream_event ? BIT(2) : 0;
> > > > > + buf[idx] |= FIELD_PREP(GENMASK(4, 3), msg->stream_behavior);
> > > > > + buf[idx] |= msg->valid_stream_behavior ? BIT(5) : 0;
> > > > > + idx++;
> > > > > + }
> > > > > + break;
> > > > > }
> > > > > raw->cur_len = idx;
> > > > > }
> > > > > @@ -547,6 +565,20 @@ drm_dp_decode_sideband_req(const struct drm_dp_sideband_msg_tx *raw,
> > > > > return -ENOMEM;
> > > > > }
> > > > > break;
> > > > > + case DP_QUERY_STREAM_ENC_STATUS:
> > > > > + req->u.enc_status.stream_id = buf[idx++];
> > > > > + for (i = 0; i < sizeof(req->u.enc_status.client_id); i++)
> > > > > + req->u.enc_status.client_id[i] = buf[idx++];
> > > > > +
> > > > > + req->u.enc_status.stream_event = FIELD_GET(GENMASK(1, 0),
> > > > > + buf[idx]);
> > > > > + req->u.enc_status.valid_stream_event = FIELD_GET(BIT(2),
> > > > > + buf[idx]);
> > > > > + req->u.enc_status.stream_behavior = FIELD_GET(GENMASK(4, 3),
> > > > > + buf[idx]);
> > > > > + req->u.enc_status.valid_stream_behavior = FIELD_GET(BIT(5),
> > > > > + buf[idx]);
> > > > > + break;
> > > > > }
> > > > >
> > > > > return 0;
> > > > > @@ -625,6 +657,16 @@ drm_dp_dump_sideband_msg_req_body(const struct drm_dp_sideband_msg_req_body *req
> > > > > req->u.i2c_write.num_bytes, req->u.i2c_write.num_bytes,
> > > > > req->u.i2c_write.bytes);
> > > > > break;
> > > > > + case DP_QUERY_STREAM_ENC_STATUS:
> > > > > + P("stream_id=%u client_id=%*ph stream_event=%x "
> > > > > + "valid_event=%d stream_behavior=%x valid_behavior=%d",
> > > > > + req->u.enc_status.stream_id,
> > > > > + (int)ARRAY_SIZE(req->u.enc_status.client_id),
> > > > > + req->u.enc_status.client_id, req->u.enc_status.stream_event,
> > > > > + req->u.enc_status.valid_stream_event,
> > > > > + req->u.enc_status.stream_behavior,
> > > > > + req->u.enc_status.valid_stream_behavior);
> > > > > + break;
> > > > > default:
> > > > > P("???\n");
> > > > > break;
> > > > > @@ -925,6 +967,34 @@ static bool drm_dp_sideband_parse_power_updown_phy_ack(struct drm_dp_sideband_ms
> > > > > return true;
> > > > > }
> > > > >
> > > > > +static bool
> > > > > +drm_dp_sideband_parse_query_stream_enc_status(
> > > > > + struct drm_dp_sideband_msg_rx *raw,
> > > > > + struct drm_dp_sideband_msg_reply_body *repmsg)
> > > > > +{
> > > > > + struct drm_dp_query_stream_enc_status_ack_reply *reply;
> > > > > +
> > > > > + reply = &repmsg->u.enc_status;
> > > > > +
> > > > > + reply->stream_id = raw->msg[3];
> > It seems msg[0] is not part of reply data,
> > could you help me with pointers, where msg is formatted.
> > > > > +
> > > > > + reply->reply_signed = raw->msg[2] & BIT(0);
> This whole patch looks good to me i could have given RB but i am having
> some concerns regarding parsing of bits here.
> reply_signed is 15th bit of reply message i.e MSB of msg[2] here.
> it seems bit parsing is in reverse order in all places.
> > > > > +
> > > > > + reply->hdcp_1x_device_present = raw->msg[2] & BIT(3);
> > > > > + reply->hdcp_2x_device_present = raw->msg[2] & BIT(4);
> But these two bits are not parse as reverse order, these are parse
> as similar in DP specs, hdcp_1x 11th bit (bit 3 of msg[2]),
> hdcp_2x 12th bit (bit 4 of msg[2]).
> Please correct me if i am wrong here.
I'm not really sure what to make of this field since when I connect a
display which only supports HDCP 1.x (no HDCP 2.x), I get:
msg[2][4:3] = 01b
I've reproduced this with multiple hubs. I don't have an HDCP 2.x
display, so I can't reproduce to see if the other bit lights up under
those conditions.
We're not using these fields at the moment, so I suppose I can switch
them around and leave a comment in case someone notices weirdness.
Sean
> Thanks,
> Anshuman Gupta.
> > > > > +
> > > > > + reply->query_capable_device_present = raw->msg[2] & BIT(5);
> > > > > + reply->legacy_device_present = raw->msg[2] & BIT(6);
> > > > > + reply->unauthorizable_device_present = raw->msg[2] & BIT(7);
> > > > > +
> > > > > + reply->auth_completed = !!(raw->msg[1] & BIT(3));
>
> > > > > + reply->encryption_enabled = !!(raw->msg[1] & BIT(4));
> > > > > + reply->repeater_present = !!(raw->msg[1] & BIT(5));
> > > > > + reply->state = (raw->msg[1] & GENMASK(7, 6)) >> 6;
> > > > > +
> > > > > + return true;
> > > > > +}
> > > > > +
> > > > > static bool drm_dp_sideband_parse_reply(struct drm_dp_sideband_msg_rx *raw,
> > > > > struct drm_dp_sideband_msg_reply_body *msg)
> > > > > {
> > > > > @@ -959,6 +1029,8 @@ static bool drm_dp_sideband_parse_reply(struct drm_dp_sideband_msg_rx *raw,
> > > > > return drm_dp_sideband_parse_power_updown_phy_ack(raw, msg);
> > > > > case DP_CLEAR_PAYLOAD_ID_TABLE:
> > > > > return true; /* since there's nothing to parse */
> > > > > + case DP_QUERY_STREAM_ENC_STATUS:
> > > > > + return drm_dp_sideband_parse_query_stream_enc_status(raw, msg);
> > > > > default:
> > > > > DRM_ERROR("Got unknown reply 0x%02x (%s)\n", msg->req_type,
> > > > > drm_dp_mst_req_type_str(msg->req_type));
> > > > > @@ -1109,6 +1181,25 @@ static void build_power_updown_phy(struct drm_dp_sideband_msg_tx *msg,
> > > > > msg->path_msg = true;
> > > > > }
> > > > >
> > > > > +static int
> > > > > +build_query_stream_enc_status(struct drm_dp_sideband_msg_tx *msg, u8 stream_id,
> > > > > + u8 *q_id)
> > > > > +{
> > > > > + struct drm_dp_sideband_msg_req_body req;
> > > > > +
> > > > > + req.req_type = DP_QUERY_STREAM_ENC_STATUS;
> > > > > + req.u.enc_status.stream_id = stream_id;
> > > > > + memcpy(req.u.enc_status.client_id, q_id,
> > > > > + sizeof(req.u.enc_status.client_id));
> > > > > + req.u.enc_status.stream_event = 0;
> > > > > + req.u.enc_status.valid_stream_event = false;
> > > > > + req.u.enc_status.stream_behavior = 0;
> > > > > + req.u.enc_status.valid_stream_behavior = false;
> > > > > +
> > > > > + drm_dp_encode_sideband_req(&req, msg);
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > static int drm_dp_mst_assign_payload_id(struct drm_dp_mst_topology_mgr *mgr,
> > > > > struct drm_dp_vcpi *vcpi)
> > > > > {
> > > > > @@ -3137,6 +3228,57 @@ int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr,
> > > > > }
> > > > > EXPORT_SYMBOL(drm_dp_send_power_updown_phy);
> > > > >
> > > > > +int drm_dp_send_query_stream_enc_status(struct drm_dp_mst_topology_mgr *mgr,
> > > > > + struct drm_dp_mst_port *port,
> > > > > + struct drm_dp_query_stream_enc_status_ack_reply *status)
> > > > > +{
> > > > > + struct drm_dp_sideband_msg_tx *txmsg;
> > > > > + u8 nonce[7];
> > > > > + int len, ret;
> > > > > +
> > > > > + txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
> > > > > + if (!txmsg)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + port = drm_dp_mst_topology_get_port_validated(mgr, port);
> > > > > + if (!port) {
> > > > > + ret = -EINVAL;
> > > > > + goto out_get_port;
> > > > > + }
> > > > > +
> > > > > + get_random_bytes(nonce, sizeof(nonce));
> > > > > +
> > > > > + /*
> > > > > + * "Source device targets the QUERY_STREAM_ENCRYPTION_STATUS message
> > > > > + * transaction at the MST Branch device directly connected to the
> > > > > + * Source"
> > > > > + */
> > > > > + txmsg->dst = mgr->mst_primary;
> > > > > +
> > > > > + len = build_query_stream_enc_status(txmsg, port->vcpi.vcpi, nonce);
> > > > > +
> > > > > + drm_dp_queue_down_tx(mgr, txmsg);
> > > > > +
> > > > > + ret = drm_dp_mst_wait_tx_reply(mgr->mst_primary, txmsg);
> > > > > + if (ret < 0) {
> > > > > + goto out;
> > > > > + } else if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) {
> > > > > + DRM_DEBUG_KMS("query encryption status nak received\n");
> > > > > + ret = -ENXIO;
> > > > > + goto out;
> > > > > + }
> > > > > +
> > > > > + ret = 0;
> > > > > + memcpy(status, &txmsg->reply.u.enc_status, sizeof(*status));
> > > > > +
> > > > > +out:
> > > > > + drm_dp_mst_topology_put_port(port);
> > > > > +out_get_port:
> > > > > + kfree(txmsg);
> > > > > + return ret;
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_dp_send_query_stream_enc_status);
> > > > > +
> > > > > static int drm_dp_create_payload_step1(struct drm_dp_mst_topology_mgr *mgr,
> > > > > int id,
> > > > > struct drm_dp_payload *payload)
> > > > > diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> > > > > index bd990d178765..1d696ec001cf 100644
> > > > > --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> > > > > +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> > > > > @@ -5,6 +5,8 @@
> > > > >
> > > > > #define PREFIX_STR "[drm_dp_mst_helper]"
> > > > >
> > > > > +#include <linux/random.h>
> > > > > +
> > > > > #include <drm/drm_dp_mst_helper.h>
> > > > > #include <drm/drm_print.h>
> > > > >
> > > > > @@ -237,6 +239,21 @@ int igt_dp_mst_sideband_msg_req_decode(void *unused)
> > > > > in.u.i2c_write.bytes = data;
> > > > > DO_TEST();
> > > > >
> > > > > + in.req_type = DP_QUERY_STREAM_ENC_STATUS;
> > > > > + in.u.enc_status.stream_id = 1;
> > > > > + DO_TEST();
> > > > > + get_random_bytes(in.u.enc_status.client_id,
> > > > > + sizeof(in.u.enc_status.client_id));
> > > > > + DO_TEST();
> > > > > + in.u.enc_status.stream_event = 3;
> > > > > + DO_TEST();
> > > > > + in.u.enc_status.valid_stream_event = 0;
> > > > > + DO_TEST();
> > > > > + in.u.enc_status.stream_behavior = 3;
> > > > > + DO_TEST();
> > > > > + in.u.enc_status.valid_stream_behavior = 1;
> > > > > + DO_TEST();
> > > > > +
> > > > > #undef DO_TEST
> > > > > return 0;
> > > > > }
> > > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > > > > index e47dc22ebf50..e2d2df5e869e 100644
> > > > > --- a/include/drm/drm_dp_helper.h
> > > > > +++ b/include/drm/drm_dp_helper.h
> > > > > @@ -1108,6 +1108,9 @@
> > > > > #define DP_POWER_DOWN_PHY 0x25
> > > > > #define DP_SINK_EVENT_NOTIFY 0x30
> > > > > #define DP_QUERY_STREAM_ENC_STATUS 0x38
> > > > > +#define DP_QUERY_STREAM_ENC_STATUS_STATE_NO_EXIST 0
> > > > > +#define DP_QUERY_STREAM_ENC_STATUS_STATE_INACTIVE 1
> > > > > +#define DP_QUERY_STREAM_ENC_STATUS_STATE_ACTIVE 2
> > > > >
> > > > > /* DP 1.2 MST sideband reply types */
> > > > > #define DP_SIDEBAND_REPLY_ACK 0x00
> > > > > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> > > > > index 8b9eb4db3381..371eef8798ad 100644
> > > > > --- a/include/drm/drm_dp_mst_helper.h
> > > > > +++ b/include/drm/drm_dp_mst_helper.h
> > > > > @@ -313,6 +313,34 @@ struct drm_dp_remote_i2c_write_ack_reply {
> > > > > u8 port_number;
> > > > > };
> > > > >
> > > > > +struct drm_dp_query_stream_enc_status_ack_reply {
> > > > > + /* Bit[23:16]- Stream Id */
> > > > > + u8 stream_id;
> > > > > +
> > > > > + /* Bit[15]- Signed */
> > > > > + bool reply_signed;
> > > > > +
> > > > > + /* Bit[10:8]- Stream Output Sink Type */
> > > > > + bool unauthorizable_device_present;
> > > > > + bool legacy_device_present;
> > > > > + bool query_capable_device_present;
> > > > > +
> > > > > + /* Bit[12:11]- Stream Output CP Type */
> > > > > + bool hdcp_1x_device_present;
> > > > > + bool hdcp_2x_device_present;
> > > > > +
> > > > > + /* Bit[4]- Stream Authentication */
> > > > > + bool auth_completed;
> > > > > +
> > > > > + /* Bit[3]- Stream Encryption */
> > > > > + bool encryption_enabled;
> > > > > +
> > > > > + /* Bit[2]- Stream Repeater Function Present */
> > > > > + bool repeater_present;
> > > > > +
> > > > > + /* Bit[1:0]- Stream State */
> > > > > + u8 state;
> > reply msg also has 20 byte of signature L' (HDCP 1.4),
> > AFAIU it has lefted out for HDCP 2.2 implementation, which is of 32 bytes for HDCP 2.2.
> > Please correct me if i am wrong here.
> > Thanks,
> > Anshuman Gupta.
> > > > > +};
> > > > >
> > > > > #define DRM_DP_MAX_SDP_STREAMS 16
> > > > > struct drm_dp_allocate_payload {
> > > > > @@ -374,6 +402,15 @@ struct drm_dp_remote_i2c_write {
> > > > > u8 *bytes;
> > > > > };
> > > > >
> > > > > +struct drm_dp_query_stream_enc_status {
> > > > > + u8 stream_id;
> > > > > + u8 client_id[7]; /* 56-bit nonce */
> > > > > + u8 stream_event;
> > > > > + bool valid_stream_event;
> > > > > + u8 stream_behavior;
> > > > > + u8 valid_stream_behavior;
> > > > > +};
> > > > > +
> > > > > /* this covers ENUM_RESOURCES, POWER_DOWN_PHY, POWER_UP_PHY */
> > > > > struct drm_dp_port_number_req {
> > > > > u8 port_number;
> > > > > @@ -422,6 +459,8 @@ struct drm_dp_sideband_msg_req_body {
> > > > >
> > > > > struct drm_dp_remote_i2c_read i2c_read;
> > > > > struct drm_dp_remote_i2c_write i2c_write;
> > > > > +
> > > > > + struct drm_dp_query_stream_enc_status enc_status;
> > > > > } u;
> > > > > };
> > > > >
> > > > > @@ -444,6 +483,8 @@ struct drm_dp_sideband_msg_reply_body {
> > > > > struct drm_dp_remote_i2c_read_ack_reply remote_i2c_read_ack;
> > > > > struct drm_dp_remote_i2c_read_nak_reply remote_i2c_read_nack;
> > > > > struct drm_dp_remote_i2c_write_ack_reply remote_i2c_write_ack;
> > > > > +
> > > > > + struct drm_dp_query_stream_enc_status_ack_reply enc_status;
> > > > > } u;
> > > > > };
> > > > >
> > > > > @@ -808,6 +849,9 @@ drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
> > > > > struct drm_dp_mst_port *port);
> > > > > int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr,
> > > > > struct drm_dp_mst_port *port, bool power_up);
> > > > > +int drm_dp_send_query_stream_enc_status(struct drm_dp_mst_topology_mgr *mgr,
> > > > > + struct drm_dp_mst_port *port,
> > > > > + struct drm_dp_query_stream_enc_status_ack_reply *status);
> > > > > int __must_check drm_dp_mst_atomic_check(struct drm_atomic_state *state);
> > > > >
> > > > > void drm_dp_mst_get_port_malloc(struct drm_dp_mst_port *port);
> > > > > --
> > > > > Sean Paul, Software Engineer, Google / Chromium OS
> > > > >
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx at lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list