Re: [Intel-gfx] [PATCH v3 2/3] drm/i915/hdcp: read RxInfo once when reading RepeaterAuth_Send_ReceiverID_List

["Li, Juston" <juston.li@intel.com>]

On Wed, 2021-08-11 at 15:34 -0400, Rodrigo Vivi wrote:
> On Tue, Aug 10, 2021 at 04:52:11PM -0700, Juston Li wrote:
> > When reading RepeaterAuth_Send_ReceiverID_List, RxInfo is read by
> > itself
> > once to retrieve the DEVICE_COUNT to calculate the size of the
> > ReceiverID list then read a second time as a part of reading
> > ReceiverID
> > list.
> > 
> > On some MST docking stations, RxInfo can only be read after the
> > RxStatus
> > READY bit is set otherwise the read will return -EIO. The spec
> > states that
> > the READY bit should be cleared as soon as RxInfo has been read.
> > 
> > In this case, the first RxInfo read succeeds but after the READY
> > bit is
> > cleared, the second read fails.
> > 
> > Fix it by reading RxInfo once and storing it before reading the
> > rest of
> > RepeaterAuth_Send_ReceiverID_List once we know the size.
> > 
> > Modify get_receiver_id_list_size() to read and store RxInfo in the
> > message buffer and also parse DEVICE_COUNT so we know the size of
> > RepeaterAuth_Send_ReceiverID_List.
> > 
> > Afterwards, retrieve the rest of the message at the offset for
> > seq_num_V.
> > 
> > Changes in v4:
> > - rebase and edit commit message
> > 
> > Changes in v3:
> > - remove comment
> > 
> > Changes in v2:
> > - remove unnecessary moving of drm_i915_private from patch 1
> > 
> > Signed-off-by: Juston Li <juston.li@intel.com>
> > Acked-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 30 ++++++++++------
> > ----
> >  include/drm/drm_dp_helper.h                  |  2 +-
> >  2 files changed, 16 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > index 1d0096654776..526fd58b9b51 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c
> > @@ -496,11 +496,10 @@ get_rxinfo_hdcp_1_dev_downstream(struct
> > intel_digital_port *dig_port, bool *hdcp
> >  }
> >  
> >  static
> > -ssize_t get_receiver_id_list_size(struct intel_digital_port
> > *dig_port)
> > +ssize_t get_receiver_id_list_rx_info(struct intel_digital_port
> > *dig_port, u32 *dev_cnt, u8 *byte)
> >  {
> > -       u8 rx_info[HDCP_2_2_RXINFO_LEN];
> > -       u32 dev_cnt;
> >         ssize_t ret;
> > +       u8 *rx_info = byte;
> >  
> >         ret = drm_dp_dpcd_read(&dig_port->dp.aux,
> >                                DP_HDCP_2_2_REG_RXINFO_OFFSET,
> > @@ -508,15 +507,11 @@ ssize_t get_receiver_id_list_size(struct
> > intel_digital_port *dig_port)
> >         if (ret != HDCP_2_2_RXINFO_LEN)
> >                 return ret >= 0 ? -EIO : ret;
> >  
> > -       dev_cnt = (HDCP_2_2_DEV_COUNT_HI(rx_info[0]) << 4 |
> > +       *dev_cnt = (HDCP_2_2_DEV_COUNT_HI(rx_info[0]) << 4 |
> >                    HDCP_2_2_DEV_COUNT_LO(rx_info[1]));
> >  
> > -       if (dev_cnt > HDCP_2_2_MAX_DEVICE_COUNT)
> > -               dev_cnt = HDCP_2_2_MAX_DEVICE_COUNT;
> > -
> > -       ret = sizeof(struct hdcp2_rep_send_receiverid_list) -
> > -               HDCP_2_2_RECEIVER_IDS_MAX_LEN +
> > -               (dev_cnt * HDCP_2_2_RECEIVER_ID_LEN);
> > +       if (*dev_cnt > HDCP_2_2_MAX_DEVICE_COUNT)
> > +               *dev_cnt = HDCP_2_2_MAX_DEVICE_COUNT;
> >  
> >         return ret;
> >  }
> > @@ -534,6 +529,7 @@ int intel_dp_hdcp2_read_msg(struct
> > intel_digital_port *dig_port,
> >         const struct hdcp2_dp_msg_data *hdcp2_msg_data;
> >         ktime_t msg_end = ktime_set(0, 0);
> >         bool msg_expired;
> > +       u32 dev_cnt;
> >  
> >         hdcp2_msg_data = get_hdcp2_dp_msg_data(msg_id);
> >         if (!hdcp2_msg_data)
> > @@ -546,17 +542,21 @@ int intel_dp_hdcp2_read_msg(struct
> > intel_digital_port *dig_port,
> >  
> >         hdcp->cp_irq_count_cached = atomic_read(&hdcp-
> > >cp_irq_count);
> >  
> > +       /* DP adaptation msgs has no msg_id */
> > +       byte++;
> > +
> >         if (msg_id == HDCP_2_2_REP_SEND_RECVID_LIST) {
> > -               ret = get_receiver_id_list_size(dig_port);
> > +               ret = get_receiver_id_list_rx_info(dig_port,
> > &dev_cnt, byte);
> >                 if (ret < 0)
> >                         return ret;
> >  
> > -               size = ret;
> > +               byte += ret;
> > +               size = sizeof(struct
> > hdcp2_rep_send_receiverid_list) -
> > +               HDCP_2_2_RXINFO_LEN - HDCP_2_2_RECEIVER_IDS_MAX_LEN
> > +
> > +               (dev_cnt * HDCP_2_2_RECEIVER_ID_LEN);
> >         }
> > -       bytes_to_recv = size - 1;
> >  
> > -       /* DP adaptation msgs has no msg_id */
> > -       byte++;
> > +       bytes_to_recv = size - 1;
> >  
> >         while (bytes_to_recv) {
> >                 len = bytes_to_recv > DP_AUX_MAX_PAYLOAD_BYTES ?
> > diff --git a/include/drm/drm_dp_helper.h
> > b/include/drm/drm_dp_helper.h
> 
> I was about to merge to drm-intel-next but then I noticed this..
> 
> So we need to cc Dave and Daniel here to get an ack from them.
> 
> > index 3f2715eb965f..7476e7c6d0be 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1413,7 +1413,7 @@ enum drm_dp_phy {
> >  #define
> > DP_HDCP_2_2_LC_INIT_OFFSET             DP_HDCP_2_2_REG_RN_OFFSET
> >  #define
> > DP_HDCP_2_2_LC_SEND_LPRIME_OFFSET      DP_HDCP_2_2_REG_LPRIME_OFFSE
> > T
> >  #define
> > DP_HDCP_2_2_SKE_SEND_EKS_OFFSET                DP_HDCP_2_2_REG_EDKE
> > Y_KS_OFFSET
> > -#define
> > DP_HDCP_2_2_REP_SEND_RECVID_LIST_OFFSET        DP_HDCP_2_2_REG_RXIN
> > FO_OFFSET
> > +#define
> > DP_HDCP_2_2_REP_SEND_RECVID_LIST_OFFSET        DP_HDCP_2_2_REG_SEQ_
> > NUM_V_OFFSET
> 
> I'm not familiar with this so I'm not sure if this should be a
> separated patch
> or better explained in the commit message.
> 
> Dave/Daniel how do you prefer this?

Actually this is just incorrect, I should not be changing this define.

Instead, I'll change intel_dp_hdcp2_read_msg() to update the offset and
add HDCP_2_2_RXINFO_LEN to it after reading RxInfo.
eg. DP_HDCP_2_2_REP_SEND_RECVID_LIST_OFFSET + HDCP_2_2_RXINFO_LEN =
DP_HDCP_2_2_REG_SEQ_NUM_V_OFFSET which is the offset we need to
continue reading from.

Will send out new revision

Thanks for pointing this out Rodrigo!

> 
> >  #define
> > DP_HDCP_2_2_REP_SEND_ACK_OFFSET                DP_HDCP_2_2_REG_V_OF
> > FSET
> >  #define
> > DP_HDCP_2_2_REP_STREAM_MANAGE_OFFSET   DP_HDCP_2_2_REG_SEQ_NUM_M_OF
> > FSET
> >  #define
> > DP_HDCP_2_2_REP_STREAM_READY_OFFSET    DP_HDCP_2_2_REG_MPRIME_OFFSE
> > T
> > -- 
> > 2.31.1
> >