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@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.
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
