[PATCH 3/3] drm/mst: adjust the function drm_dp_remove_payload_part2()

[Lyude Paul]

On Mon, 2023-08-07 at 18:59 +0300, Imre Deak wrote:
> On Mon, Aug 07, 2023 at 02:43:02AM +0000, Lin, Wayne wrote:
> > [AMD Official Use Only - General]
> > 
> > > -----Original Message-----
> > > From: Imre Deak <imre.deak at intel.com>
> > > Sent: Friday, August 4, 2023 11:32 PM
> > > To: Lin, Wayne <Wayne.Lin at amd.com>
> > > Cc: dri-devel at lists.freedesktop.org; amd-gfx at lists.freedesktop.org;
> > > lyude at redhat.com; jani.nikula at intel.com; ville.syrjala at linux.intel.com;
> > > Wentland, Harry <Harry.Wentland at amd.com>; Zuo, Jerry
> > > <Jerry.Zuo at amd.com>
> > > Subject: Re: [PATCH 3/3] drm/mst: adjust the function
> > > drm_dp_remove_payload_part2()
> > > 
> > > On Fri, Aug 04, 2023 at 02:20:29PM +0800, Wayne Lin wrote:
> > > > [...]
> > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > index e04f87ff755a..4270178f95f6 100644
> > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > @@ -3382,8 +3382,7 @@
> > > EXPORT_SYMBOL(drm_dp_remove_payload_part1);
> > > >   * drm_dp_remove_payload_part2() - Remove an MST payload locally
> > > >   * @mgr: Manager to use.
> > > >   * @mst_state: The MST atomic state
> > > > - * @old_payload: The payload with its old state
> > > > - * @new_payload: The payload with its latest state
> > > > + * @payload: The payload with its latest state
> > > >   *
> > > >   * Updates the starting time slots of all other payloads which would have
> > > been shifted towards
> > > >   * the start of the payload ID table as a result of removing a
> > > > payload. Driver should call this @@ -3392,25 +3391,36 @@
> > > EXPORT_SYMBOL(drm_dp_remove_payload_part1);
> > > >   */
> > > >  void drm_dp_remove_payload_part2(struct drm_dp_mst_topology_mgr
> > > *mgr,
> > > >                              struct drm_dp_mst_topology_state
> > > *mst_state,
> > > > -                            const struct drm_dp_mst_atomic_payload
> > > *old_payload,
> > > > -                            struct drm_dp_mst_atomic_payload
> > > *new_payload)
> > > > +                            struct drm_dp_mst_atomic_payload
> > > *payload)
> > > >  {
> > > >     struct drm_dp_mst_atomic_payload *pos;
> > > > +   u8 time_slots_to_remove;
> > > > +   u8 next_payload_vc_start = mgr->next_start_slot;
> > > > +
> > > > +   /* Find the current allocated time slot number of the payload */
> > > > +   list_for_each_entry(pos, &mst_state->payloads, next) {
> > > > +           if (pos != payload &&
> > > > +               pos->vc_start_slot > payload->vc_start_slot &&
> > > > +               pos->vc_start_slot < next_payload_vc_start)
> > > > +                   next_payload_vc_start = pos->vc_start_slot;
> > > > +   }
> > > > +
> > > > +   time_slots_to_remove = next_payload_vc_start -
> > > > +payload->vc_start_slot;
> > > 
> > > Imo, the intuitive way would be to pass the old payload state to this function -
> > > which already contains the required time_slots param - and refactor things
> > > instead moving vc_start_slot from the payload state to mgr suggested by Ville
> > > earlier.
> > > 
> > > --Imre
> > 
> > Hi Imre,
> > Thanks for your feedback!
> > 
> > I understand it's functionally correct. But IMHO, it's still a bit
> > conceptually different between the time slot in old state and the time
> > slot in current payload table. My thought is the time slot at the
> > moment when we are removing the payload would be a better choice.
> 
> Yes, they are different. The old state contains the time slot the
> payload was added with in a preceding commit and so the time slot value
> which should be used when removing the same payload in the current
> commit.
> 
> The new state contains a time slot value with which the payload will be
> added in the current commit and can be different than the one in the old
> state if the current commit has changed the payload size (meaning that
> the same atomic commit will first remove the payload using the time slot
> value in the old state and afterwards will add back the same payload
> using the time slot value in the new state).
> 
> > And with this, we can also simplify some codes. Especially remove
> > workaround in amd driver. In fact, DRM mst code maintains the payload
> > table and all the time slot info is in it already. We don't really
> > have to pass a new parameter.
> 
> I agree that drm_dp_remove_payload() could be simplified, but this
> should be done so that the drivers can pass the old payload state to it
> (without having to pass the new state). This would be possible if
> vc_start_slot was not tracked in the payload state (which is really not
> an atomic state that can be precomputed as all other atomic state),
> rather it would be tracked in struct drm_dp_mst_topology_mgr.

JFYI too  - I think I'm fine with us moving vc_start_slot elsewhere at this
point ;)

> 
> It looks like AMD has to reconstruct the old state in
> dm_helpers_construct_old_payload(). Could you explain why it couldn't
> instead just pass old_payload acquired by
> 
> old_mst_state = drm_atomic_get_old_mst_topology_state();
> old_payload = drm_atomic_get_mst_payload_state(old_mst_state);
> 
> ?
> 
> > > >     /* Remove local payload allocation */
> > > >     list_for_each_entry(pos, &mst_state->payloads, next) {
> > > > -           if (pos != new_payload && pos->vc_start_slot > new_payload-
> > > > vc_start_slot)
> > > > -                   pos->vc_start_slot -= old_payload->time_slots;
> > > > +           if (pos != payload && pos->vc_start_slot > payload-
> > > > vc_start_slot)
> > > > +                   pos->vc_start_slot -= time_slots_to_remove;
> > > >     }
> > > > -   new_payload->vc_start_slot = -1;
> > > > +   payload->vc_start_slot = -1;
> > > > 
> > > >     mgr->payload_count--;
> > > > -   mgr->next_start_slot -= old_payload->time_slots;
> > > > +   mgr->next_start_slot -= time_slots_to_remove;
> > > > 
> > > > -   if (new_payload->delete)
> > > > -           drm_dp_mst_put_port_malloc(new_payload->port);
> > > > +   if (payload->delete)
> > > > +           drm_dp_mst_put_port_malloc(payload->port);
> > > > 
> > > > -   new_payload->payload_allocation_status =
> > > DRM_DP_MST_PAYLOAD_ALLOCATION_NONE;
> > > > +   payload->payload_allocation_status =
> > > > +DRM_DP_MST_PAYLOAD_ALLOCATION_NONE;
> > > >  }
> > 
> > --
> > Regards,
> > Wayne
> 

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