On Thu, Jul 9, 2020 at 9:09 AM Anshuman Gupta <anshuman.gupta@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxx> > > > > > > > > > > Used to query whether an MST stream is encrypted or not. > > > > > > > > > > Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx> > > > > > > > > > > Link: https://patchwork.freedesktop.org/patch/msgid/20200218220242.107265-14-sean@xxxxxxxxxx #v4 > > > > > Link: https://patchwork.freedesktop.org/patch/msgid/20200305201236.152307-15-sean@xxxxxxxxxx #v5 > > > > > Link: https://patchwork.freedesktop.org/patch/msgid/20200429195502.39919-15-sean@xxxxxxxxxx #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@xxxxxxxxxxxxxxxxxxxxx > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
