On 2020-08-11 at 13:21:38 -0400, Sean Paul wrote:
On Thu, Jul 9, 2020 at 9:09 AM Anshuman Gupta anshuman.gupta@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@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@chromium.org
Used to query whether an MST stream is encrypted or not.
Signed-off-by: Sean Paul seanpaul@chromium.org
Link: https://patchwork.freedesktop.org/patch/msgid/20200218220242.107265-14-sean@... #v4 Link: https://patchwork.freedesktop.org/patch/msgid/20200305201236.152307-15-sean@... #v5 Link: https://patchwork.freedesktop.org/patch/msgid/20200429195502.39919-15-sean@p... #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.
Yes that should be fine. Considering that all reply msg bits should parsed in reverse order, Reviewed-by: Anshuman Gupta anshuman.gupta@intel.com
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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx