RE: [PATCH v4] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[Public]

> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxx>
> Sent: Thursday, June 8, 2023 8:26 PM
> To: Lin, Wayne <Wayne.Lin@xxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: lyude@xxxxxxxxxx; ville.syrjala@xxxxxxxxxxxxxxx; imre.deak@xxxxxxxxx;
> Wentland, Harry <Harry.Wentland@xxxxxxx>; Zuo, Jerry
> <Jerry.Zuo@xxxxxxx>; Lin, Wayne <Wayne.Lin@xxxxxxx>;
> stable@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4] drm/dp_mst: Clear MSG_RDY flag before sending new
> message
>
> On Thu, 08 Jun 2023, Wayne Lin <Wayne.Lin@xxxxxxx> wrote:
> > [Why]
> > The sequence for collecting down_reply from source perspective should
> > be:
> >
> > Request_n->repeat (get partial reply of Request_n->clear message ready
> > flag to ack DPRX that the message is received) till all partial
> > replies for Request_n are received->new Request_n+1.
> >
> > Now there is chance that drm_dp_mst_hpd_irq() will fire new down
> > request in the tx queue when the down reply is incomplete. Source is
> > restricted to generate interveleaved message transactions so we should
> > avoid it.
> >
> > Also, while assembling partial reply packets, reading out DPCD
> > DOWN_REP Sideband MSG buffer + clearing DOWN_REP_MSG_RDY flag
> should
> > be wrapped up as a complete operation for reading out a reply packet.
> > Kicking off a new request before clearing DOWN_REP_MSG_RDY flag might
> > be risky. e.g. If the reply of the new request has overwritten the
> > DPRX DOWN_REP Sideband MSG buffer before source writing one to clear
> > DOWN_REP_MSG_RDY flag, source then unintentionally flushes the reply
> > for the new request. Should handle the up request in the same way.
> >
> > [How]
> > Separete drm_dp_mst_hpd_irq() into 2 steps. After acking the MST IRQ
> > event, driver calls drm_dp_mst_hpd_irq_send_new_request() and might
> > trigger drm_dp_mst_kick_tx() only when there is no on going message
> > transaction.
> >
> > Changes since v1:
> > * Reworked on review comments received
> > -> Adjust the fix to let driver explicitly kick off new down request
> > when mst irq event is handled and acked
> > -> Adjust the commit message
> >
> > Changes since v2:
> > * Adjust the commit message
> > * Adjust the naming of the divided 2 functions and add a new input
> >   parameter "ack".
> > * Adjust code flow as per review comments.
> >
> > Changes since v3:
> > * Update the function description of drm_dp_mst_hpd_irq_handle_event
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@xxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 ++++++------
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 54
> ++++++++++++++++---
> >  drivers/gpu/drm/i915/display/intel_dp.c       |  7 +--
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       | 12 +++--
> >  include/drm/display/drm_dp_mst_helper.h       |  7 ++-
> >  5 files changed, 82 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index d5cec03eaa8d..597c3368bcfb 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3236,6 +3236,7 @@ static void dm_handle_mst_sideband_msg(struct
> > amdgpu_dm_connector *aconnector)  {
> >     u8 esi[DP_PSR_ERROR_STATUS - DP_SINK_COUNT_ESI] = { 0 };
> >     u8 dret;
> > +   u8 ack;
> >     bool new_irq_handled = false;
> >     int dpcd_addr;
> >     int dpcd_bytes_to_read;
> > @@ -3265,34 +3266,36 @@ static void
> dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
> >             process_count < max_process_count) {
> >             u8 retry;
> >             dret = 0;
> > +           ack = 0;
> >
> >             process_count++;
> >
> >             DRM_DEBUG_DRIVER("ESI %02x %02x %02x\n", esi[0],
> esi[1], esi[2]);
> >             /* handle HPD short pulse irq */
> >             if (aconnector->mst_mgr.mst_state)
> > -                   drm_dp_mst_hpd_irq(
> > -                           &aconnector->mst_mgr,
> > -                           esi,
> > -                           &new_irq_handled);
> > +                   drm_dp_mst_hpd_irq_handle_event(&aconnector-
> >mst_mgr,
> > +                                                   esi,
> > +                                                   &ack,
> > +                                                   &new_irq_handled);
> >
> >             if (new_irq_handled) {
> >                     /* ACK at DPCD to notify down stream */
> > -                   const int ack_dpcd_bytes_to_write =
> > -                           dpcd_bytes_to_read - 1;
> > -
> >                     for (retry = 0; retry < 3; retry++) {
> > -                           u8 wret;
> > -
> > -                           wret = drm_dp_dpcd_write(
> > -                                   &aconnector->dm_dp_aux.aux,
> > -                                   dpcd_addr + 1,
> > -                                   &esi[1],
> > -                                   ack_dpcd_bytes_to_write);
> > -                           if (wret == ack_dpcd_bytes_to_write)
> > +                           ssize_t wret;
> > +
> > +                           wret = drm_dp_dpcd_writeb(&aconnector-
> >dm_dp_aux.aux,
> > +                                                     dpcd_addr + 1,
> > +                                                     ack);
> > +                           if (wret == 1)
> >                                     break;
> >                     }
> >
> > +                   if (retry == 3) {
> > +                           DRM_ERROR("Failed to ack MST event.\n");
> > +                           return;
> > +                   }
> > +
> > +
>       drm_dp_mst_hpd_irq_send_new_request(&aconnector->mst_mgr);
> >                     /* check if there is new irq to be handled */
> >                     dret = drm_dp_dpcd_read(
> >                             &aconnector->dm_dp_aux.aux,
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 38dab76ae69e..3a018f5b604c 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4053,17 +4053,28 @@ static int drm_dp_mst_handle_up_req(struct
> > drm_dp_mst_topology_mgr *mgr)  }
> >
> >  /**
> > - * drm_dp_mst_hpd_irq() - MST hotplug IRQ notify
> > + * drm_dp_mst_hpd_irq_handle_event() - MST hotplug IRQ handle MST
> > + event
> >   * @mgr: manager to notify irq for.
> >   * @esi: 4 bytes from SINK_COUNT_ESI
> > + * @ack: flags of events to ack
> >   * @handled: whether the hpd interrupt was consumed or not
> >   *
> > - * This should be called from the driver when it detects a short IRQ,
> > + * This should be called from the driver when it detects a HPD IRQ,
> >   * along with the value of the DEVICE_SERVICE_IRQ_VECTOR_ESI0. The
> > - * topology manager will process the sideband messages received as a
> > result
> > - * of this.
> > + * topology manager will process the sideband messages received
> > + * as indicated in the DEVICE_SERVICE_IRQ_VECTOR_ESI0 and set the
> > + * corresponding flags that Driver has to ack the DP receiver later.
> > + *
> > + * Note that driver shall also call
> > + * drm_dp_mst_hpd_irq_send_new_request() if the 'handled' is set
> > + * after calling this function, to try to kick off a new request in
> > + * the queue if the previous message transaction is completed.
> > + *
> > + * See also:
> > + * drm_dp_mst_hpd_irq_send_new_request()
> >   */
> > -int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi,
> > bool *handled)
> > +int drm_dp_mst_hpd_irq_handle_event(struct drm_dp_mst_topology_mgr
> *mgr, const u8 *esi,
> > +                               u8 *ack, bool *handled)
> >  {
> >     int ret = 0;
> >     int sc;
> > @@ -4078,18 +4089,47 @@ int drm_dp_mst_hpd_irq(struct
> drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
> >     if (esi[1] & DP_DOWN_REP_MSG_RDY) {
> >             ret = drm_dp_mst_handle_down_rep(mgr);
> >             *handled = true;
> > +           *ack |= DP_DOWN_REP_MSG_RDY;
>
> My idea was that esi and ack would be the same size buffers, so the caller
> wouldn't have to worry where exactly to point ack to.
>
> I think the asymmetry here is misleading, with ack and esi having to point at
> different locations.
>
Thanks, Jani.

But Event status Indicator Files (DPCD 0x2000h ~ 0x21FFH) are not all designed
to be ack clear, e.g. esi[0] here. My thought is to be precise about what is handled
and what is going to be ack clear. Otherwise, write ack[0] to DPCD 0x2002h is
not reasonable.

Regards,
Wayne
> BR,
> Jani.
>
>
>
> >     }
> >
> >     if (esi[1] & DP_UP_REQ_MSG_RDY) {
> >             ret |= drm_dp_mst_handle_up_req(mgr);
> >             *handled = true;
> > +           *ack |= DP_UP_REQ_MSG_RDY;
> >     }
> >
> > -   drm_dp_mst_kick_tx(mgr);
> >     return ret;
> >  }
> > -EXPORT_SYMBOL(drm_dp_mst_hpd_irq);
> > +EXPORT_SYMBOL(drm_dp_mst_hpd_irq_handle_event);
> >
> > +/**
> > + * drm_dp_mst_hpd_irq_send_new_request() - MST hotplug IRQ kick off
> > +new request
> > + * @mgr: manager to notify irq for.
> > + *
> > + * This should be called from the driver when mst irq event is
> > +handled
> > + * and acked. Note that new down request should only be sent when
> > + * previous message transaction is completed. Source is not supposed
> > +to generate
> > + * interleaved message transactions.
> > + */
> > +void drm_dp_mst_hpd_irq_send_new_request(struct
> > +drm_dp_mst_topology_mgr *mgr) {
> > +   struct drm_dp_sideband_msg_tx *txmsg;
> > +   bool kick = true;
> > +
> > +   mutex_lock(&mgr->qlock);
> > +   txmsg = list_first_entry_or_null(&mgr->tx_msg_downq,
> > +                                    struct drm_dp_sideband_msg_tx,
> next);
> > +   /* If last transaction is not completed yet*/
> > +   if (!txmsg ||
> > +       txmsg->state == DRM_DP_SIDEBAND_TX_START_SEND ||
> > +       txmsg->state == DRM_DP_SIDEBAND_TX_SENT)
> > +           kick = false;
> > +   mutex_unlock(&mgr->qlock);
> > +
> > +   if (kick)
> > +           drm_dp_mst_kick_tx(mgr);
> > +}
> > +EXPORT_SYMBOL(drm_dp_mst_hpd_irq_send_new_request);
> >  /**
> >   * drm_dp_mst_detect_port() - get connection status for an MST port
> >   * @connector: DRM connector for this port diff --git
> > a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 4bec8cd7979f..f24602887015 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4062,9 +4062,7 @@ intel_dp_mst_hpd_irq(struct intel_dp *intel_dp,
> > u8 *esi, u8 *ack)  {
> >     bool handled = false;
> >
> > -   drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> > -   if (handled)
> > -           ack[1] |= esi[1] & (DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> > +   drm_dp_mst_hpd_irq_handle_event(&intel_dp->mst_mgr, esi,
> &ack[1],
> > +&handled);
> >
> >     if (esi[1] & DP_CP_IRQ) {
> >             intel_hdcp_handle_cp_irq(intel_dp->attached_connector);
> > @@ -4139,6 +4137,9 @@ intel_dp_check_mst_status(struct intel_dp
> > *intel_dp)
> >
> >             if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
> >                     drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
> > +
> > +           if (ack[1] & (DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY))
> > +                   drm_dp_mst_hpd_irq_send_new_request(&intel_dp-
> >mst_mgr);
> >     }
> >
> >     return link_ok;
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index 9b6824f6b9e4..b2d9978e88a8 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -1357,6 +1357,7 @@ nv50_mstm_service(struct nouveau_drm *drm,
> >     bool handled = true, ret = true;
> >     int rc;
> >     u8 esi[8] = {};
> > +   u8 ack;
> >
> >     while (handled) {
> >             rc = drm_dp_dpcd_read(aux, DP_SINK_COUNT_ESI, esi, 8);
> @@ -1365,16
> > +1366,19 @@ nv50_mstm_service(struct nouveau_drm *drm,
> >                     break;
> >             }
> >
> > -           drm_dp_mst_hpd_irq(&mstm->mgr, esi, &handled);
> > +           ack = 0;
> > +           drm_dp_mst_hpd_irq_handle_event(&mstm->mgr, esi, &ack,
> &handled);
> >             if (!handled)
> >                     break;
> >
> > -           rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
> &esi[1],
> > -                                  3);
> > -           if (rc != 3) {
> > +           rc = drm_dp_dpcd_writeb(aux, DP_SINK_COUNT_ESI + 1,
> ack);
> > +
> > +           if (rc != 1) {
> >                     ret = false;
> >                     break;
> >             }
> > +
> > +           drm_dp_mst_hpd_irq_send_new_request(&mstm->mgr);
> >     }
> >
> >     if (!ret)
> > diff --git a/include/drm/display/drm_dp_mst_helper.h
> > b/include/drm/display/drm_dp_mst_helper.h
> > index 32c764fb9cb5..40e855c8407c 100644
> > --- a/include/drm/display/drm_dp_mst_helper.h
> > +++ b/include/drm/display/drm_dp_mst_helper.h
> > @@ -815,8 +815,11 @@ void drm_dp_mst_topology_mgr_destroy(struct
> > drm_dp_mst_topology_mgr *mgr);  bool drm_dp_read_mst_cap(struct
> > drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]);  int
> > drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr
> *mgr,
> > bool mst_state);
> >
> > -int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi,
> > bool *handled);
> > -
> > +int drm_dp_mst_hpd_irq_handle_event(struct drm_dp_mst_topology_mgr
> *mgr,
> > +                               const u8 *esi,
> > +                               u8 *ack,
> > +                               bool *handled);
> > +void drm_dp_mst_hpd_irq_send_new_request(struct
> > +drm_dp_mst_topology_mgr *mgr);
> >
> >  int
> >  drm_dp_mst_detect_port(struct drm_connector *connector,
>
> --
> Jani Nikula, Intel Open Source Graphics Center




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux