Re: [Intel-gfx] [PATCH v7 15/17] drm/mst: Add support for QUERY_STREAM_ENCRYPTION_STATUS MST sideband message
On 2020-08-11 at 13:21:38 -0400, Sean Paul wrote:
> On Thu, Jul 9, 2020 at 9:09 AM Anshuman Gupta
> 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
> > > > 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
> > > > > >
> > > > > > Used to query whether an MST stream is encrypted or not.
> > > > > >
> > > > > > Signed-off-by: Sean Paul
> > > > > >
> > > > > > Link:
> > > > > > https://patchwork.freedesktop.org/patch/msgid/20200218220242.107265-14-s...@poorly.run
> > > > > > #v4
> > > > > > Link:
> > > > > > https://patchwork.freedesktop.org/patch/msgid/20200305201236.152307-15-s...@poorly.run
> > > > > > #v5
> > > > > > Link:
> > > > > > https://patchwork.freedesktop.org/patch/msgid/20200429195502.39919-15-s...@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
> > > > > > #include
> > > > > > #include
> > > > > > #include
> > > > > > #include
> > > > > > #include
> > > > > > +#include
> > > > > > #include
> > > > > > #include
> > > > > > #include
> > > > > > @@ -419,6 +421,22 @@ drm_dp_encode_sideband_req(const struct
> > > > > > drm_dp_sideband_msg_req_body *req,
> > > > > > memcpy([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 = >u.enc_status;
> > > > > > + buf[idx] = msg->stream_id;
> > > > > > + idx++;
> > > > > > + memcpy([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);
> > > > > >
Re: [Intel-gfx] [PATCH v7 15/17] drm/mst: Add support for QUERY_STREAM_ENCRYPTION_STATUS MST sideband message
On Thu, Jul 9, 2020 at 9:09 AM Anshuman Gupta 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
> > > 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
> > > > >
> > > > > Used to query whether an MST stream is encrypted or not.
> > > > >
> > > > > Signed-off-by: Sean Paul
> > > > >
> > > > > Link:
> > > > > https://patchwork.freedesktop.org/patch/msgid/20200218220242.107265-14-s...@poorly.run
> > > > > #v4
> > > > > Link:
> > > > > https://patchwork.freedesktop.org/patch/msgid/20200305201236.152307-15-s...@poorly.run
> > > > > #v5
> > > > > Link:
> > > > > https://patchwork.freedesktop.org/patch/msgid/20200429195502.39919-15-s...@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
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > +#include
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > @@ -419,6 +421,22 @@ drm_dp_encode_sideband_req(const struct
> > > > > drm_dp_sideband_msg_req_body *req,
> > > > > memcpy([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 = >u.enc_status;
> > > > > + buf[idx] = msg->stream_id;
> > > > > + idx++;
> > > > > + memcpy([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++;
> > > > > + }
>
Re: [Intel-gfx] [PATCH v7 15/17] drm/mst: Add support for QUERY_STREAM_ENCRYPTION_STATUS MST sideband message
On Thu, Jul 2, 2020 at 10:49 AM 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
> > wrote:
> > >
\snip
> > > > +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 = >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.
msg[0] is the reply type, see drm_dp_sideband_parse_reply()
> > > > +
> > > > + reply->reply_signed = raw->msg[2] & BIT(0);
> > > > +
> > > > + reply->hdcp_1x_device_present = raw->msg[2] & BIT(3);
> > > > + reply->hdcp_2x_device_present = raw->msg[2] & BIT(4);
> > > > +
> > > > + 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;
> > > > +}
> > > > +
/snip
> > > >
> > > > +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),
Yeah, I haven't done signature parsing because we simply can't. To
compute the signature we need Link_s, which maps to M0 in HDCP 1.4
terms. The Intel HDCP bspec states "M0 cannot be exposed to
software. It is kept internal to hardware". So it's impossible to
compute/verify on the host side.
Sean
> 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,
> > > > +
Re: [Intel-gfx] [PATCH v7 15/17] drm/mst: Add support for QUERY_STREAM_ENCRYPTION_STATUS MST sideband message
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
> > 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
> > > >
> > > > Used to query whether an MST stream is encrypted or not.
> > > >
> > > > Signed-off-by: Sean Paul
> > > >
> > > > Link:
> > > > https://patchwork.freedesktop.org/patch/msgid/20200218220242.107265-14-s...@poorly.run
> > > > #v4
> > > > Link:
> > > > https://patchwork.freedesktop.org/patch/msgid/20200305201236.152307-15-s...@poorly.run
> > > > #v5
> > > > Link:
> > > > https://patchwork.freedesktop.org/patch/msgid/20200429195502.39919-15-s...@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
> > > > #include
> > > > #include
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -419,6 +421,22 @@ drm_dp_encode_sideband_req(const struct
> > > > drm_dp_sideband_msg_req_body *req,
> > > > memcpy([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 = >u.enc_status;
> > > > + buf[idx] = msg->stream_id;
> > > > + idx++;
> > > > + memcpy([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
Re: [Intel-gfx] [PATCH v7 15/17] drm/mst: Add support for QUERY_STREAM_ENCRYPTION_STATUS MST sideband message
On 2020-06-30 at 12:48:34 -0400, Sean Paul wrote:
> On Tue, Jun 30, 2020 at 10:21 AM Anshuman Gupta
> 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
> > >
> > > Used to query whether an MST stream is encrypted or not.
> > >
> > > Signed-off-by: Sean Paul
> > >
> > > Link:
> > > https://patchwork.freedesktop.org/patch/msgid/20200218220242.107265-14-s...@poorly.run
> > > #v4
> > > Link:
> > > https://patchwork.freedesktop.org/patch/msgid/20200305201236.152307-15-s...@poorly.run
> > > #v5
> > > Link:
> > > https://patchwork.freedesktop.org/patch/msgid/20200429195502.39919-15-s...@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
> > > #include
> > > #include
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -419,6 +421,22 @@ drm_dp_encode_sideband_req(const struct
> > > drm_dp_sideband_msg_req_body *req,
> > > memcpy([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 = >u.enc_status;
> > > + buf[idx] = msg->stream_id;
> > > + idx++;
> > > + memcpy([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),
> > > +
Re: [Intel-gfx] [PATCH v7 15/17] drm/mst: Add support for QUERY_STREAM_ENCRYPTION_STATUS MST sideband message
On Tue, Jun 30, 2020 at 10:21 AM Anshuman Gupta
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.
Sean
> Thanks,
> Anshuman Gupta.
>
> > From: Sean Paul
> >
> > Used to query whether an MST stream is encrypted or not.
> >
> > Signed-off-by: Sean Paul
> >
> > Link:
> > https://patchwork.freedesktop.org/patch/msgid/20200218220242.107265-14-s...@poorly.run
> > #v4
> > Link:
> > https://patchwork.freedesktop.org/patch/msgid/20200305201236.152307-15-s...@poorly.run
> > #v5
> > Link:
> > https://patchwork.freedesktop.org/patch/msgid/20200429195502.39919-15-s...@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
> > #include
> > #include
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -419,6 +421,22 @@ drm_dp_encode_sideband_req(const struct
> > drm_dp_sideband_msg_req_body *req,
> > memcpy([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 = >u.enc_status;
> > + buf[idx] = msg->stream_id;
> > + idx++;
> > + memcpy([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]);
> > +
Re: [Intel-gfx] [PATCH v7 15/17] drm/mst: Add support for QUERY_STREAM_ENCRYPTION_STATUS MST sideband message
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.
Thanks,
Anshuman Gupta.
> From: Sean Paul
>
> Used to query whether an MST stream is encrypted or not.
>
> Signed-off-by: Sean Paul
>
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20200218220242.107265-14-s...@poorly.run
> #v4
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20200305201236.152307-15-s...@poorly.run
> #v5
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20200429195502.39919-15-s...@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
> #include
> #include
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -419,6 +421,22 @@ drm_dp_encode_sideband_req(const struct
> drm_dp_sideband_msg_req_body *req,
> memcpy([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 = >u.enc_status;
> + buf[idx] = msg->stream_id;
> + idx++;
> + memcpy([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),
> +
