Re: [PATCH] drm: Update MST First Link Slot Information Based on Encoding Format

[Lyude Paul <lyude@redhat.com>]

First off, sidenote: I wonder if we even need total_avail_slots and
could just use start_slot. Anyway, more productive comments below:

On Mon, 2021-10-18 at 15:47 -0400, Bhawanpreet Lakha wrote:
> I understand the mst_state argument its just that most of the mst
> functions are using mst_mgr/port structs and there is no easy way to
> extract the mst_state using mgr/port in these places
> (drm_dp_update_payload_part1, drm_dp_mst_allocate_vcpi, drm_dp_init_vcpi
> etc) where we need the slot info.
>
Ahh, I see why this might be confusing. I think surprisingly though,
this actually should probably be OK. Mostly, two of these functions
don't actually need the slot count and one I think I have a workaround
for:

* drm_dp_init_vcpi() - This function does use the total slot count here:

    if (slots > 63)
            return -ENOSPC;

  However, drm_dp_init_vcpi() assigns the current payload which means
  it's called by the driver at commit time, not atomic check time. Since
  atomic commits are only allowed to happen after we've successfully
  tested the state in question, we aren't allowed to fail them in the
  middle of a commit - which is definitely what we're doing in
  drm_dp_init_vcpi(). So, I'd actually say we should either totally
  ignore this bit of drm_dp_init_vcpi() or preferably, just entirely
  drop it in a prerequisite patch.
  (If you aren't familiar with atomic modesetting, the reason we "can't"
  just fail in the middle of committing a new atomic state is because we
  may very well have already committed that state to hardware partially.
  So, there's no nice way of aborting at that point without seriously
  complicating things - hence the need for having an atomic check before
  commits)

* drm_dp_mst_allocate_vcpi() - this seems to only be an issue because of
  drm_dp_init_vcpi(), and we additionally print the maximum number of
  slots in a drm_dbg_kms() message:

    drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n",
                …

  Since we should have already decided
  all of the new payloads by the time we're in drm_dp_mst_allocate_vcpi
  (again, since we're in atomic commit), that's probably not the best
  place for us to print that anyway. So, I think we'd be fine just
  dropping the "max=63" from that string.

* drm_dp_update_payload_part1() - This one does need the current slot
  count, you're right. I would normally say we should just fix this
  function and move the payload info over to the atomic state, which is
  the eventual plan, but doing that would require dealing with the
  radeon.ko MST mess which is probably too much to ask for with
  something as simple as this patch. I think I know a workaround though:
  Let's keep this new slot info (e.g. num_slots, and possibly
  total_avail_slots if we keep that) in the MST atomic state, _but_ as a
  temporary solution we can instead add a start_slot argument to
  drm_dp_update_payload_part1(). That way we have an easy solution for
  making sure radeon still compiles, and atomic drivers can just extract
  the start_slot info themselves from the topology state and pass it
  into drm_dp_update_payload_part1(). Then I can get rid of that
  start_slots argument at a later date when I've started moving all of
  the payload info for MST into the atomic state.
  If we do this solution though, we should definitely document in the
  patch and in the kdocs for drm_dp_update_payload_part1() that passing
  the start slot is a temporary workaround for non-atomic drivers, and
  will be removed when non-atomic portions of the MST helpers have been
  moved out of helpers and into atomic state.

Does this sound good? There's a lot of moving pieces here so hopefully I
didn't miss anything

> So either we need to keep a copy of the slots in the mgr because that's
> what most of the code is using right now or pass around the atomic state
> to get the mgr->state mapping. (I don't have much experience with the
> mst code so maybe I am missing some key detail here?)

Worst case scenario, if there was something I missed that implies we DO
need access to a mgr->state mapping, I might be OK with us using that in
the interim for these patches. I don't think we need that quite yet as
far as I can tell though.

>
>
> Thanks,
>
> Bhawan
>
>
> On 2021-10-15 4:41 p.m., Lyude Paul wrote:
> > [more snip]
> >
> > On Fri, 2021-10-15 at 15:43 -0400, Bhawanpreet Lakha wrote:
> > > Thanks for the response,
> > >
> > > That function is per port so not sure how that will work. Also we only
> > > need to check this inside drm_dp_mst_atomic_check_vcpi_alloc_limit(),
> > > which doesn't have a state.
> > >
> > > We could add the slots(or some DP version indicator) inside the
> > > drm_connector, and add a parameter to
> > > drm_dp_mst_atomic_check_vcpi_alloc_limit(int slots)? and call it with
> > > this info via drm_dp_mst_atomic_check() and then update the mgr->slot in
> > > commit.
> > TBH - I think we can actually just get away with having all of this info
> > in
> > drm_dp_mst_topology_state
> >
> > >
> > > Bhawan
> > >
> > > > >                   ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-
> > > > > > mst_primary,
> > > > >                                                              
> > > > > mst_state);
> > > > >                   mutex_unlock(&mgr->lock);
> > > > > @@ -5527,11 +5543,16 @@ 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)
> > > > >                   return -ENOMEM;
> > > > >   
> > > > > +       mst_state->total_avail_slots = 63;
> > > > > +       mst_state->start_slot = 1;
> > > > > +
> > > > >           mst_state->mgr = mgr;
> > > > >           INIT_LIST_HEAD(&mst_state->vcpis);
> > > > >   
> > > > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > > > b/include/drm/drm_dp_mst_helper.h
> > > > > index ddb9231d0309..f8152dfb34ed 100644
> > > > > --- a/include/drm/drm_dp_mst_helper.h
> > > > > +++ b/include/drm/drm_dp_mst_helper.h
> > > > > @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
> > > > >           struct drm_private_state base;
> > > > >           struct list_head vcpis;
> > > > >           struct drm_dp_mst_topology_mgr *mgr;
> > > > > +       u8 total_avail_slots;
> > > > > +       u8 start_slot;
> > > > >    };
> > > > >   
> > > > >    #define to_dp_mst_topology_mgr(x) container_of(x, struct
> > > > > drm_dp_mst_topology_mgr, base)
> > > > > @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
> > > > >            */
> > > > >           int pbn_div;
> > > > >   
> > > > > +       /**
> > > > > +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
> > > > > +        */
> > > > > +       u8 total_avail_slots;
> > > > > +
> > > > > +       /**
> > > > > +        * @start_slot: 1 for 8b/10b, 0 for 128/132b
> > > > > +        */
> > > > > +       u8 start_slot;
> > > > > +
> > > > >           /**
> > > > >            * @funcs: Atomic helper callbacks
> > > > >            */
> > > > > @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct
> > > > > drm_dp_mst_topology_mgr *mgr, struct drm_dp
> > > > >   
> > > > >    void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr
> > > > > *mgr,
> > > > > struct drm_dp_mst_port *port);
> > > > >   
> > > > > +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
> > > > > *mst_state, uint8_t link_coding_cap);
> > > > >   
> > > > >    void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr
> > > > > *mgr,
> > > > >                                   struct drm_dp_mst_port *port);
>

--
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat