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@xxxxxxxxx>
> > Sent: Friday, August 4, 2023 11:32 PM
> > To: Lin, Wayne <Wayne.Lin@xxxxxxx>
> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > lyude@xxxxxxxxxx; jani.nikula@xxxxxxxxx; ville.syrjala@xxxxxxxxxxxxxxx;
> > Wentland, Harry <Harry.Wentland@xxxxxxx>; Zuo, Jerry
> > <Jerry.Zuo@xxxxxxx>
> > 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.
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
