RE: [PATCH 1/2] drm: Update MST First Link Slot Information Based on Encoding Format

["Zuo, Jerry" <Jerry.Zuo@amd.com>]

[AMD Official Use Only]

> -----Original Message-----
> From: Lyude Paul <lyude@redhat.com>
> Sent: August 30, 2021 4:00 PM
> To: Zuo, Jerry <Jerry.Zuo@amd.com>; dri-devel@lists.freedesktop.org
> Cc: Wentland, Harry <Harry.Wentland@amd.com>; Kazlauskas, Nicholas
> <Nicholas.Kazlauskas@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>
> Subject: Re: [PATCH 1/2] drm: Update MST First Link Slot Information Based
> on Encoding Format
>
> On Fri, 2021-08-27 at 19:43 -0400, Fangzhi Zuo wrote:
> > 8b/10b encoding format requires to reserve the first slot for
> > recording metadata. Real data transmission starts from the second
> > slot, with a total of available 63 slots available.
> >
> > In 128b/132b encoding format, metadata is transmitted separately in
> > LLCP packet before MTP. Real data transmission starts from the first
> > slot, with a total of 64 slots available.
> >
> > Signed-off-by: Fangzhi Zuo <Jerry.Zuo@amd.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 27
> > ++++++++++++++++++++-------
> >  include/drm/drm_dp_mst_helper.h       |  9 +++++++++
> >  2 files changed, 29 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 86d13d6bc463..30544801d2b8 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -3370,7 +3370,7 @@ int drm_dp_update_payload_part1(struct
> > drm_dp_mst_topology_mgr *mgr)
> >         struct drm_dp_payload req_payload;
> >         struct drm_dp_mst_port *port;
> >         int i, j;
> > -       int cur_slots = 1;
> > +       int cur_slots = mgr->start_slot;
> >         bool skip;
> >
> >         mutex_lock(&mgr->payload_lock); @@ -4323,7 +4323,7 @@ int
> > drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
> >         num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> >
> >         /* max. time slots - one slot for MTP header */
> > -       if (num_slots > 63)
> > +       if (num_slots > mgr->total_avail_slots)
> >                 return -ENOSPC;
> >         return num_slots;
> >  }
> > @@ -4335,7 +4335,7 @@ static int drm_dp_init_vcpi(struct
> > drm_dp_mst_topology_mgr *mgr,
> >         int ret;
> >
> >         /* max. time slots - one slot for MTP header */
> > -       if (slots > 63)
> > +       if (slots > mgr->total_avail_slots)
> >                 return -ENOSPC;
> >
> >         vcpi->pbn = pbn;
> > @@ -4509,6 +4509,17 @@ int drm_dp_atomic_release_vcpi_slots(struct
> > drm_atomic_state *state,
> >  }
> >  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
> >
> > +void drm_dp_mst_update_encoding_cap(struct
> drm_dp_mst_topology_mgr
> > +*mgr,
> > uint8_t link_encoding_cap)
> > +{
> > +       if (link_encoding_cap == DP_CAP_ANSI_128B132B) {
> > +               mgr->total_avail_slots = 64;
> > +               mgr->start_slot = 0;
> > +       }
> > +       DRM_DEBUG_KMS("%s encoding format determined\n",
> > +                     (link_encoding_cap == DP_CAP_ANSI_128B132B) ?
> > "128b/132b" : "8b/10b");
> > +}
> > +EXPORT_SYMBOL(drm_dp_mst_update_encoding_cap);
> > +
>
> This seems to be missing kdocs, can you fix that?
>
> Also - I'm not convinced this is all of the work we have to do, as there's no
> locking taking place here in this function. If we're changing the number of
> available VCPI slots that we have, we need to be able to factor that into the
> atomic check logic which means that we can't rely on mgr->* for any kind of
> data that isn't guaranteed to remain consistent throughout the lifetime of the
> driver or topology. (Note that some of the old MST code didn't follow this
> logic, so I wouldn't be surprised if there's still exceptions to this we need to
> clean up).
>
> Note that I still expect we'll have to keep some sort of track of the current
> total slot count in the topology mgr, but that should be reflecting the
> currently programmed state and not be relied on from our atomic check.
>

Thanks Lyude for your comments.

Seems I should keep existing code to keep track of current slot status in mgr.
That information is getting updated each time when topology change detected.
That slot information saved in mgr is a sort of static, and could only be used
for debug purpose to track what is the current encoding format.

> IMHO - the correct way we should go about adding support for this is to add
> something into drm_dp_mst_topology_state and integrate this into the
> atomic check helpers.

The slot information should also be added into drm_dp_mst_topology_state to
reflect the real-time slot status.

I'd like to confirm the best place to get slot count info. updated.
Should the update be done within &drm_mode_config_funcs. atomic_check(),
before new stream is created, OR
should be updated within drm_dp_mst_atomic_check() ?

The updated slot count will be used in drm_dp_mst_atomic_check() to check slot limit,
and in drm_dp_update_payload_part1() as initial cur_slots.

>
> >  /**
> >   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
> >   * @mgr: manager for this port
> > @@ -4540,8 +4551,8 @@ bool drm_dp_mst_allocate_vcpi(struct
> > drm_dp_mst_topology_mgr *mgr,
> >
> >         ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
> >         if (ret) {
> > -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d
> > max=63 ret=%d\n",
> > -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
> > +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d
> > +max=%d
> > ret=%d\n",
> > +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-
> > >total_avail_slots, ret);
> >                 drm_dp_mst_topology_put_port(port);
> >                 goto out;
> >         }
> > @@ -5228,7 +5239,7 @@
> drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
> > drm_dp_mst_topology_mgr *mgr,
> >                                          struct
> > drm_dp_mst_topology_state
> > *mst_state)
> >  {
> >         struct drm_dp_vcpi_allocation *vcpi;
> > -       int avail_slots = 63, payload_count = 0;
> > +       int avail_slots = mgr->total_avail_slots, payload_count = 0;
> >
> >         list_for_each_entry(vcpi, &mst_state->vcpis, next) {
> >                 /* Releasing VCPI is always OK-even if the port is
> > gone */ @@ -5257,7 +5268,7 @@
> > drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
> > drm_dp_mst_topology_mgr *mgr,
> >                 }
> >         }
> >         drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI
> > avail=%d used=%d\n",
> > -                      mgr, mst_state, avail_slots, 63 - avail_slots);
> > +                      mgr, mst_state, avail_slots,
> > +mgr->total_avail_slots -
> > avail_slots);
> >
> >         return 0;
> >  }
> > @@ -5529,6 +5540,8 @@ int drm_dp_mst_topology_mgr_init(struct
> > drm_dp_mst_topology_mgr *mgr,
> >         if (!mgr->proposed_vcpis)
> >                 return -ENOMEM;
> >         set_bit(0, &mgr->payload_mask);
> > +       mgr->total_avail_slots = 63;
> > +       mgr->start_slot = 1;
> >
> >         mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
> >         if (mst_state == NULL)
> > diff --git a/include/drm/drm_dp_mst_helper.h
> > b/include/drm/drm_dp_mst_helper.h index ddb9231d0309..eac5ce48f214
> > 100644
> > --- a/include/drm/drm_dp_mst_helper.h
> > +++ b/include/drm/drm_dp_mst_helper.h
> > @@ -661,6 +661,15 @@ struct drm_dp_mst_topology_mgr {
> >          */
> >         int pbn_div;
> >
> > +       /**
> > +        * @total_avail_slots: available slots for data transmission
> > +        */
> > +       u8 total_avail_slots;
> > +       /**
> > +        * @start_slot: first slot index for data transmission
> > +        */
> > +       u8 start_slot;
> > +
> >         /**
> >          * @funcs: Atomic helper callbacks
> >          */
>
> --
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat