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

[Imre Deak]

On Tue, Aug 08, 2023 at 03:47:47AM +0000, Lin, Wayne wrote:
> [AMD Official Use Only - General]
> 
> > -----Original Message-----
> > From: Imre Deak <imre.deak at intel.com>
> > Sent: Tuesday, August 8, 2023 12:00 AM
> > 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 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).
> >
> Appreciate your time, Imre!
> 
> Yes I understand, so I'm not using the number of the time slot in the new state.
> I'm referring to the start slot instead which is updated during every allocation
> and removement at current commit.
> 
> Like what you said, current commit manipulation could be a mix of allocations
> and removements for the payload. My thought is, conceptually, looking up the
> latest number of time slot is a better choice rather than the one in old state.
> It's relatively intuitive to me since we are removing payload from current
> payload table and which changes since last preceding commit till the moment
> we're deleting the payload. Although it's unreasonable that these values are
> different.
> 
> > > 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.
> >
> 
> So the reason I chose to pass the new state is like what I mentioned above. I
> would prefer to carry the latest updated payload table instead which is in the new
> state. And I agree with the explanation for the vc_start_slot and that's also my
> thought at the beginning. It could be a refactor later, but no matter the start slot
> is put into payload state or the topology manager I would prefer to refer to the
> latest payload table rather than the number of time slot in the old state.
> 
> > 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);
> >
> > ?
> 
> AMD doesn't pass the drm old state to the stage while HW is deleting
> the payload.  The reason is that HW payload table is known during HW
> programming procedure, so the payload removement is based on the table
> at the moment.
>
> AMD expected the current number of time slot is also
> already maintained in drm layer.

Yes, both of the above are maintained by the drm layer, but it also
means it doesn't really need to recalculate time_slots_to_remove as done
in this patch, since that info is already available in the old payload
state.

Afaics the AMD driver calls properly 

drm_atomic_helper_commit() -> drm_atomic_helper_swap_state()

after a commit, so that all the payloads it added should be tracked
now as the old payload state.

So could you confirm what is the old_payload->time_slots value (which
you get with the above functions) at the point of removing this payload
and if it's not the time_slots value this same payload was actually
added with previously, why this is so (via some example sequence)?

Thanks.

> Again, thanks for your feedback Imre!
> 
> >
> > > > >     /* 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
> 
> --
> Regards,
> Wayne