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

Commit Message

Lin, Wayne Aug. 4, 2023, 6:20 a.m. UTC

[Why]
Now in drm_dp_remove_payload_part2(), it utilizes the time slot number
of the payload in old state to represent the one in the payload table
at the moment.

It would be better to clarify the idea by using the latest allocated
time slot number for the port at the moment instead and which info is
already included in new mst_state. By this, we can also remove redundant
workaround for amdgpu driver.

[How]
Remove "old_payload" input of drm_dp_remove_payload_part2() and get the
latest number of allocated time slot for the port from new mst_state
instead.

Signed-off-by: Wayne Lin <[e-mail]>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 70 ++++---------------
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 32 ++++++---
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  7 +-
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  6 +-
 include/drm/display/drm_dp_mst_helper.h       |  9 ++-
 5 files changed, 40 insertions(+), 84 deletions(-)

Comments

Imre Deak Aug. 4, 2023, 3:31 p.m. UTC | #1

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

>  	/* 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;
>  }

Lin, Wayne Aug. 7, 2023, 2:43 a.m. UTC | #2

[AMD Official Use Only - General]

> -----Original Message-----
> From: Imre Deak <[e-mail]>
> Sent: Friday, August 4, 2023 11:32 PM
> To: Lin, Wayne <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Wentland, Harry <[e-mail]>; Zuo, Jerry
> <[e-mail]>
> 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. 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.

>
> >     /* 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

Imre Deak Aug. 7, 2023, 3:59 p.m. UTC | #3

On Mon, Aug 07, 2023 at 02:43:02AM +0000, Lin, Wayne wrote:
> [AMD Official Use Only - General]
> 
> > -----Original Message-----
> > From: Imre Deak <[e-mail]>
> > Sent: Friday, August 4, 2023 11:32 PM
> > To: Lin, Wayne <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > Wentland, Harry <[e-mail]>; Zuo, Jerry
> > <[e-mail]>
> > 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

Lin, Wayne Aug. 8, 2023, 3:47 a.m. UTC | #4

[AMD Official Use Only - General]

> -----Original Message-----
> From: Imre Deak <[e-mail]>
> Sent: Tuesday, August 8, 2023 12:00 AM
> To: Lin, Wayne <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Wentland, Harry <[e-mail]>; Zuo, Jerry
> <[e-mail]>
> 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 <[e-mail]>
> > > Sent: Friday, August 4, 2023 11:32 PM
> > > To: Lin, Wayne <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; Wentland, Harry
> > > <[e-mail]>; Zuo, Jerry <[e-mail]>
> > > 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.

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

Lyude Paul Aug. 17, 2023, 9:38 p.m. UTC | #5

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 <[e-mail]>
> > > Sent: Friday, August 4, 2023 11:32 PM
> > > To: Lin, Wayne <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > Wentland, Harry <[e-mail]>; Zuo, Jerry
> > > <[e-mail]>
> > > 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
>

Imre Deak Aug. 18, 2023, 5:46 p.m. UTC | #6

On Tue, Aug 08, 2023 at 03:47:47AM +0000, Lin, Wayne wrote:
> [AMD Official Use Only - General]
> 
> > -----Original Message-----
> > From: Imre Deak <[e-mail]>
> > Sent: Tuesday, August 8, 2023 12:00 AM
> > To: Lin, Wayne <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > Wentland, Harry <[e-mail]>; Zuo, Jerry
> > <[e-mail]>
> > 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 <[e-mail]>
> > > > Sent: Friday, August 4, 2023 11:32 PM
> > > > To: Lin, Wayne <[e-mail]>
> > > > Cc: [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; Wentland, Harry
> > > > <[e-mail]>; Zuo, Jerry <[e-mail]>
> > > > 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

Lin, Wayne Aug. 23, 2023, 3:16 a.m. UTC | #7

[AMD Official Use Only - General]

> -----Original Message-----
> From: Imre Deak <[e-mail]>
> Sent: Saturday, August 19, 2023 1:46 AM
> To: Lin, Wayne <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Wentland, Harry <[e-mail]>; Zuo, Jerry
> <[e-mail]>
> Subject: Re: [PATCH 3/3] drm/mst: adjust the function
> drm_dp_remove_payload_part2()
>
> On Tue, Aug 08, 2023 at 03:47:47AM +0000, Lin, Wayne wrote:
> > [AMD Official Use Only - General]
> >
> > > -----Original Message-----
> > > From: Imre Deak <[e-mail]>
> > > Sent: Tuesday, August 8, 2023 12:00 AM
> > > To: Lin, Wayne <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; Wentland, Harry
> > > <[e-mail]>; Zuo, Jerry <[e-mail]>
> > > 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 <[e-mail]>
> > > > > Sent: Friday, August 4, 2023 11:32 PM
> > > > > To: Lin, Wayne <[e-mail]>
> > > > > Cc: [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail]; Wentland,
> > > > > Harry <[e-mail]>; Zuo, Jerry <[e-mail]>
> > > > > 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.

Hi Imre,
I'm not saying that the time_slots carried in the old state is wrong within amd driver.
But just amd driver doesn't carry the drm state to the step when it's removing the
payload, since the info is already in its hardware and drm used to maintain the info
in the drm layer. So the patch is trying to get the behavior of this helper function
back to what it used to be.

And the main reason that I want to change the pass in parameter is like what I
mentioned previously. The commit manipulation could be a mix of allocations and
removements for the payload. And in the spec, it also introduces examples to reduce
or increase the payload allocation. Although we're not using reduction/increment
today, it implicitly imposes the limitation to use them before calling the removement
helper function with the old state as the passed in parameter. So I also want to remove
the dependency by this patch. Would like to know your thoughts about this.

Thanks, Imre!

>
> > 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
--
Regards,
Wayne

Imre Deak Aug. 25, 2023, 1:55 p.m. UTC | #8

On Wed, Aug 23, 2023 at 03:16:44AM +0000, Lin, Wayne wrote:
> [AMD Official Use Only - General]
> 
> > -----Original Message-----
> > From: Imre Deak <[e-mail]>
> > Sent: Saturday, August 19, 2023 1:46 AM
> > To: Lin, Wayne <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > Wentland, Harry <[e-mail]>; Zuo, Jerry
> > <[e-mail]>
> > Subject: Re: [PATCH 3/3] drm/mst: adjust the function
> > drm_dp_remove_payload_part2()
> >
> > On Tue, Aug 08, 2023 at 03:47:47AM +0000, Lin, Wayne wrote:
> > > [AMD Official Use Only - General]
> > >
> > > > -----Original Message-----
> > > > From: Imre Deak <[e-mail]>
> > > > Sent: Tuesday, August 8, 2023 12:00 AM
> > > > To: Lin, Wayne <[e-mail]>
> > > > Cc: [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; Wentland, Harry
> > > > <[e-mail]>; Zuo, Jerry <[e-mail]>
> > > > 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 <[e-mail]>
> > > > > > Sent: Friday, August 4, 2023 11:32 PM
> > > > > > To: Lin, Wayne <[e-mail]>
> > > > > > Cc: [e-mail];
> > > > > > [e-mail]; [e-mail];
> > > > > > [e-mail]; [e-mail]; Wentland,
> > > > > > Harry <[e-mail]>; Zuo, Jerry <[e-mail]>
> > > > > > 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.
> 
> Hi Imre,
> I'm not saying that the time_slots carried in the old state is wrong within amd driver.
> But just amd driver doesn't carry the drm state to the step when it's removing the
> payload, since the info is already in its hardware and drm used to maintain the info
> in the drm layer.

Hm, in 

dm_helpers_dp_mst_write_payload_allocation_table()

the

mst_state = to_drm_dp_mst_topology_state(mst_mgr->base.state);

used as the new state doesn't look ok to me. The above is the MST
object's current SW state after an atomic commit/swap, but this isn't
the new state a driver should program to the HW or pass to the drm
helper functions. The object's current state could be ahead of what the
driver should program to the HW, if the driver properly implements
commit pipelining (so at the point you're programming a state you'd have
already future commits computed and queued up, each queued-up commit
with its own state).

So instead of the above mst_state the driver should pass down the
drm_atomic_state to dm_helpers_dp_mst_write_payload_allocation_table()
which should use that instead to get the MST state it should program or
pass to the drm layer.

> So the patch is trying to get the behavior of this helper function
> back to what it used to be.

The behavior before was actually broken and one reason for that was a
confusion about what's stored in the new payload state. It's a mixture of
old/current state (vc_start_slot) and new state (time_slots). So I don't
think it's a good idea to add even more dependency on this semantic.

I think the right solution for what this patch is about - remove the
need for dm_helpers_construct_old_payload(), would be to pass down
drm_atomic_state to
dm_helpers_dp_mst_write_payload_allocation_table()
based on the above.

> And the main reason that I want to change the pass in parameter is
> like what I mentioned previously. The commit manipulation could be a
> mix of allocations and removements for the payload. And in the spec,
> it also introduces examples to reduce or increase the payload
> allocation. Although we're not using reduction/increment today, it
> implicitly imposes the limitation to use them before calling the
> removement helper function with the old state as the passed in
> parameter. So I also want to remove the dependency by this patch.
> Would like to know your thoughts about this.

It's not clear what would be the benefit of changing a payload without
removing it first. In any case, I think the right direction for the
driver would be to meet the requirement to use the proper atomic state
with the drm helper functions (not the object's current SW state) as
described above.

> Thanks, Imre!
> 
> >
> > > 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
> --
> Regards,
> Wayne

Lin, Wayne Sept. 7, 2023, 3:44 a.m. UTC | #9

[AMD Official Use Only - General]

> -----Original Message-----
> From: Imre Deak <[e-mail]>
> Sent: Friday, August 25, 2023 9:56 PM
> To: Lin, Wayne <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Wentland, Harry <[e-mail]>; Zuo, Jerry
> <[e-mail]>
> Subject: Re: [PATCH 3/3] drm/mst: adjust the function
> drm_dp_remove_payload_part2()
>
> On Wed, Aug 23, 2023 at 03:16:44AM +0000, Lin, Wayne wrote:
> > [AMD Official Use Only - General]
> >
> > > -----Original Message-----
> > > From: Imre Deak <[e-mail]>
> > > Sent: Saturday, August 19, 2023 1:46 AM
> > > To: Lin, Wayne <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; Wentland, Harry
> > > <[e-mail]>; Zuo, Jerry <[e-mail]>
> > > Subject: Re: [PATCH 3/3] drm/mst: adjust the function
> > > drm_dp_remove_payload_part2()
> > >
> > > On Tue, Aug 08, 2023 at 03:47:47AM +0000, Lin, Wayne wrote:
> > > > [AMD Official Use Only - General]
> > > >
> > > > > -----Original Message-----
> > > > > From: Imre Deak <[e-mail]>
> > > > > Sent: Tuesday, August 8, 2023 12:00 AM
> > > > > To: Lin, Wayne <[e-mail]>
> > > > > Cc: [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail]; Wentland,
> > > > > Harry <[e-mail]>; Zuo, Jerry <[e-mail]>
> > > > > 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 <[e-mail]>
> > > > > > > Sent: Friday, August 4, 2023 11:32 PM
> > > > > > > To: Lin, Wayne <[e-mail]>
> > > > > > > Cc: [e-mail];
> > > > > > > [e-mail]; [e-mail];
> > > > > > > [e-mail]; [e-mail];
> > > > > > > Wentland, Harry <[e-mail]>; Zuo, Jerry
> > > > > > > <[e-mail]>
> > > > > > > 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.
> >
> > Hi Imre,
> > I'm not saying that the time_slots carried in the old state is wrong within
> amd driver.
> > But just amd driver doesn't carry the drm state to the step when it's
> > removing the payload, since the info is already in its hardware and
> > drm used to maintain the info in the drm layer.
>
> Hm, in
>
> dm_helpers_dp_mst_write_payload_allocation_table()
>
> the
>
> mst_state = to_drm_dp_mst_topology_state(mst_mgr->base.state);
>
> used as the new state doesn't look ok to me. The above is the MST object's
> current SW state after an atomic commit/swap, but this isn't the new state a
> driver should program to the HW or pass to the drm helper functions. The
> object's current state could be ahead of what the driver should program to the
> HW, if the driver properly implements commit pipelining (so at the point
> you're programming a state you'd have already future commits computed and
> queued up, each queued-up commit with its own state).
>
> So instead of the above mst_state the driver should pass down the
> drm_atomic_state to dm_helpers_dp_mst_write_payload_allocation_table()
> which should use that instead to get the MST state it should program or pass
> to the drm layer.
>
> > So the patch is trying to get the behavior of this helper function
> > back to what it used to be.
>
> The behavior before was actually broken and one reason for that was a
> confusion about what's stored in the new payload state. It's a mixture of
> old/current state (vc_start_slot) and new state (time_slots). So I don't think
> it's a good idea to add even more dependency on this semantic.
>
> I think the right solution for what this patch is about - remove the need for
> dm_helpers_construct_old_payload(), would be to pass down
> drm_atomic_state to
> dm_helpers_dp_mst_write_payload_allocation_table()
> based on the above.
>
> > And the main reason that I want to change the pass in parameter is
> > like what I mentioned previously. The commit manipulation could be a
> > mix of allocations and removements for the payload. And in the spec,
> > it also introduces examples to reduce or increase the payload
> > allocation. Although we're not using reduction/increment today, it
> > implicitly imposes the limitation to use them before calling the
> > removement helper function with the old state as the passed in
> > parameter. So I also want to remove the dependency by this patch.
> > Would like to know your thoughts about this.
>
> It's not clear what would be the benefit of changing a payload without
> removing it first. In any case, I think the right direction for the driver would be
> to meet the requirement to use the proper atomic state with the drm helper
> functions (not the object's current SW state) as described above.

Hi Imre,

Thanks for pointing that out and that was also one of my plan to refactor! But I
would take that as another patch to follow up and would like to align with you
the idea for this helper function itself more.

My concern is referring to the old state to remove the time slot is just not generic
right for what the helper function shall accomplish. Even it doesn't bring benefit
as you see, having old state as the input imposes limitation to drivers using it which
is the downside that it brought. The limitation is like what I tried to explain before.
For an instance, if one driver has compressed streams allocated in the previous
commit and wants to disable these streams at the current commit. It handles and
accomplishes the commit into two steps that firstly to disable dsc engine only
(which increases the time slot), and next disable the streams. Under this design,
the old state can't represent the exact number of time slot that it want's to remove
at the moment. For this case, I don't think drm_dp_remove_payload_part2 should
block the driver to use it since the driver doesn't do things wrong.  Conversely, it's
due to the helper function constrains the driver to use an inappropriate input.

And with or without my patch, the current payload allocation table (i.e. vc_start_slot
In new state) and the time slot that this new state is going to set (i.e. time_slots in
new state) are already both in the new state. I think the patch doesn't add irrelevant
input for removing a payload as this helper function should do, because the time slot
it should remove is the exact one in the payload table now, not the one captured in
previous commit.  So in contrast, refer to old state time slot is a bit confusing to me
because it's not generic right. Would like to know your thought on this point.
Appreciate your time!

>
> > Thanks, Imre!
> >
> > >
> > > > 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
> > --
> > Regards,
> > Wayne
--
Regards,
Wayne

Imre Deak Sept. 8, 2023, 7:18 p.m. UTC | #10

On Thu, Sep 07, 2023 at 03:44:39AM +0000, Lin, Wayne wrote:
> [AMD Official Use Only - General]
> 
> > -----Original Message-----
> > From: Imre Deak <[e-mail]>
> > Sent: Friday, August 25, 2023 9:56 PM
> > To: Lin, Wayne <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > Wentland, Harry <[e-mail]>; Zuo, Jerry
> > <[e-mail]>
> > Subject: Re: [PATCH 3/3] drm/mst: adjust the function
> > drm_dp_remove_payload_part2()
> >
> > On Wed, Aug 23, 2023 at 03:16:44AM +0000, Lin, Wayne wrote:
> > > [AMD Official Use Only - General]
> > >
> > > > -----Original Message-----
> > > > From: Imre Deak <[e-mail]>
> > > > Sent: Saturday, August 19, 2023 1:46 AM
> > > > To: Lin, Wayne <[e-mail]>
> > > > Cc: [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; Wentland, Harry
> > > > <[e-mail]>; Zuo, Jerry <[e-mail]>
> > > > Subject: Re: [PATCH 3/3] drm/mst: adjust the function
> > > > drm_dp_remove_payload_part2()
> > > >
> > > > On Tue, Aug 08, 2023 at 03:47:47AM +0000, Lin, Wayne wrote:
> > > > > [AMD Official Use Only - General]
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Imre Deak <[e-mail]>
> > > > > > Sent: Tuesday, August 8, 2023 12:00 AM
> > > > > > To: Lin, Wayne <[e-mail]>
> > > > > > Cc: [e-mail];
> > > > > > [e-mail]; [e-mail];
> > > > > > [e-mail]; [e-mail]; Wentland,
> > > > > > Harry <[e-mail]>; Zuo, Jerry <[e-mail]>
> > > > > > 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 <[e-mail]>
> > > > > > > > Sent: Friday, August 4, 2023 11:32 PM
> > > > > > > > To: Lin, Wayne <[e-mail]>
> > > > > > > > Cc: [e-mail];
> > > > > > > > [e-mail]; [e-mail];
> > > > > > > > [e-mail]; [e-mail];
> > > > > > > > Wentland, Harry <[e-mail]>; Zuo, Jerry
> > > > > > > > <[e-mail]>
> > > > > > > > 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.
> > >
> > > Hi Imre,
> > > I'm not saying that the time_slots carried in the old state is wrong within
> > amd driver.
> > > But just amd driver doesn't carry the drm state to the step when it's
> > > removing the payload, since the info is already in its hardware and
> > > drm used to maintain the info in the drm layer.
> >
> > Hm, in
> >
> > dm_helpers_dp_mst_write_payload_allocation_table()
> >
> > the
> >
> > mst_state = to_drm_dp_mst_topology_state(mst_mgr->base.state);
> >
> > used as the new state doesn't look ok to me. The above is the MST object's
> > current SW state after an atomic commit/swap, but this isn't the new state a
> > driver should program to the HW or pass to the drm helper functions. The
> > object's current state could be ahead of what the driver should program to the
> > HW, if the driver properly implements commit pipelining (so at the point
> > you're programming a state you'd have already future commits computed and
> > queued up, each queued-up commit with its own state).
> >
> > So instead of the above mst_state the driver should pass down the
> > drm_atomic_state to dm_helpers_dp_mst_write_payload_allocation_table()
> > which should use that instead to get the MST state it should program or pass
> > to the drm layer.
> >
> > > So the patch is trying to get the behavior of this helper function
> > > back to what it used to be.
> >
> > The behavior before was actually broken and one reason for that was a
> > confusion about what's stored in the new payload state. It's a mixture of
> > old/current state (vc_start_slot) and new state (time_slots). So I don't think
> > it's a good idea to add even more dependency on this semantic.
> >
> > I think the right solution for what this patch is about - remove the need for
> > dm_helpers_construct_old_payload(), would be to pass down
> > drm_atomic_state to
> > dm_helpers_dp_mst_write_payload_allocation_table()
> > based on the above.
> >
> > > And the main reason that I want to change the pass in parameter is
> > > like what I mentioned previously. The commit manipulation could be a
> > > mix of allocations and removements for the payload. And in the spec,
> > > it also introduces examples to reduce or increase the payload
> > > allocation. Although we're not using reduction/increment today, it
> > > implicitly imposes the limitation to use them before calling the
> > > removement helper function with the old state as the passed in
> > > parameter. So I also want to remove the dependency by this patch.
> > > Would like to know your thoughts about this.
> >
> > It's not clear what would be the benefit of changing a payload without
> > removing it first. In any case, I think the right direction for the driver would be
> > to meet the requirement to use the proper atomic state with the drm helper
> > functions (not the object's current SW state) as described above.
> 
> Hi Imre,
> 
> Thanks for pointing that out and that was also one of my plan to refactor! But I
> would take that as another patch to follow up and would like to align with you
> the idea for this helper function itself more.
> 
> My concern is referring to the old state to remove the time slot is just not generic
> right for what the helper function shall accomplish. Even it doesn't bring benefit
> as you see, having old state as the input imposes limitation to drivers using it which
> is the downside that it brought. The limitation is like what I tried to explain before.
> For an instance, if one driver has compressed streams allocated in the previous
> commit and wants to disable these streams at the current commit. It handles and
> accomplishes the commit into two steps that firstly to disable dsc engine only
> (which increases the time slot), and next disable the streams. Under this design,
> the old state can't represent the exact number of time slot that it want's to remove
> at the moment. For this case, I don't think drm_dp_remove_payload_part2 should
> block the driver to use it since the driver doesn't do things wrong.  Conversely, it's
> due to the helper function constrains the driver to use an inappropriate input.
> 
> And with or without my patch, the current payload allocation table (i.e. vc_start_slot
> In new state) and the time slot that this new state is going to set (i.e. time_slots in
> new state) are already both in the new state. I think the patch doesn't add irrelevant
> input for removing a payload as this helper function should do, because the time slot
> it should remove is the exact one in the payload table now, not the one captured in
> previous commit.  So in contrast, refer to old state time slot is a bit confusing to me
> because it's not generic right. Would like to know your thought on this point.
> Appreciate your time!

I'd like to be sure that the payload is removed with the size it was
added with in the previous commit and as I wrote above not depend for
this on the new payload state with a mixture of old/current/new states.
Based on that I'd be ok for instance with a new

int drm_dp_remove_port_payload(mgr, mst_state, port)

function which looks up / removes the payload with the time_slots calculated
based on the payload table as in your patch and returns the calculated
time_slots.

The AMD driver could call the above function and the current
drm_dp_remove_payload(mgr, mst_state, old_payload) function would be

	time_slots = drm_dp_remove_port_payload(mgr, mst_state, old_payload->port);
	WARN_ON(time_slots != old_payload->time_slots);

--Imre

Lin, Wayne Sept. 12, 2023, 7:26 a.m. UTC | #11

[Public]

> -----Original Message-----
> From: Imre Deak <[e-mail]>
> Sent: Saturday, September 9, 2023 3:18 AM
> To: Lin, Wayne <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Wentland, Harry <[e-mail]>; Zuo, Jerry
> <[e-mail]>
> Subject: Re: [PATCH 3/3] drm/mst: adjust the function
> drm_dp_remove_payload_part2()
>
> On Thu, Sep 07, 2023 at 03:44:39AM +0000, Lin, Wayne wrote:
> > [AMD Official Use Only - General]
> >
> > > -----Original Message-----
> > > From: Imre Deak <[e-mail]>
> > > Sent: Friday, August 25, 2023 9:56 PM
> > > To: Lin, Wayne <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; Wentland, Harry
> > > <[e-mail]>; Zuo, Jerry <[e-mail]>
> > > Subject: Re: [PATCH 3/3] drm/mst: adjust the function
> > > drm_dp_remove_payload_part2()
> > >
> > > On Wed, Aug 23, 2023 at 03:16:44AM +0000, Lin, Wayne wrote:
> > > > [AMD Official Use Only - General]
> > > >
> > > > > -----Original Message-----
> > > > > From: Imre Deak <[e-mail]>
> > > > > Sent: Saturday, August 19, 2023 1:46 AM
> > > > > To: Lin, Wayne <[e-mail]>
> > > > > Cc: [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail]; Wentland,
> > > > > Harry <[e-mail]>; Zuo, Jerry <[e-mail]>
> > > > > Subject: Re: [PATCH 3/3] drm/mst: adjust the function
> > > > > drm_dp_remove_payload_part2()
> > > > >
> > > > > On Tue, Aug 08, 2023 at 03:47:47AM +0000, Lin, Wayne wrote:
> > > > > > [AMD Official Use Only - General]
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Imre Deak <[e-mail]>
> > > > > > > Sent: Tuesday, August 8, 2023 12:00 AM
> > > > > > > To: Lin, Wayne <[e-mail]>
> > > > > > > Cc: [e-mail];
> > > > > > > [e-mail]; [e-mail];
> > > > > > > [e-mail]; [e-mail];
> > > > > > > Wentland, Harry <[e-mail]>; Zuo, Jerry
> > > > > > > <[e-mail]>
> > > > > > > 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 <[e-mail]>
> > > > > > > > > Sent: Friday, August 4, 2023 11:32 PM
> > > > > > > > > To: Lin, Wayne <[e-mail]>
> > > > > > > > > Cc: [e-mail];
> > > > > > > > > [e-mail]; [e-mail];
> > > > > > > > > [e-mail]; [e-mail];
> > > > > > > > > Wentland, Harry <[e-mail]>; Zuo, Jerry
> > > > > > > > > <[e-mail]>
> > > > > > > > > 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.
> > > >
> > > > Hi Imre,
> > > > I'm not saying that the time_slots carried in the old state is
> > > > wrong within
> > > amd driver.
> > > > But just amd driver doesn't carry the drm state to the step when
> > > > it's removing the payload, since the info is already in its
> > > > hardware and drm used to maintain the info in the drm layer.
> > >
> > > Hm, in
> > >
> > > dm_helpers_dp_mst_write_payload_allocation_table()
> > >
> > > the
> > >
> > > mst_state = to_drm_dp_mst_topology_state(mst_mgr->base.state);
> > >
> > > used as the new state doesn't look ok to me. The above is the MST
> > > object's current SW state after an atomic commit/swap, but this
> > > isn't the new state a driver should program to the HW or pass to the
> > > drm helper functions. The object's current state could be ahead of
> > > what the driver should program to the HW, if the driver properly
> > > implements commit pipelining (so at the point you're programming a
> > > state you'd have already future commits computed and queued up, each
> queued-up commit with its own state).
> > >
> > > So instead of the above mst_state the driver should pass down the
> > > drm_atomic_state to
> > > dm_helpers_dp_mst_write_payload_allocation_table()
> > > which should use that instead to get the MST state it should program
> > > or pass to the drm layer.
> > >
> > > > So the patch is trying to get the behavior of this helper function
> > > > back to what it used to be.
> > >
> > > The behavior before was actually broken and one reason for that was
> > > a confusion about what's stored in the new payload state. It's a
> > > mixture of old/current state (vc_start_slot) and new state
> > > (time_slots). So I don't think it's a good idea to add even more dependency
> on this semantic.
> > >
> > > I think the right solution for what this patch is about - remove the
> > > need for dm_helpers_construct_old_payload(), would be to pass down
> > > drm_atomic_state to
> > > dm_helpers_dp_mst_write_payload_allocation_table()
> > > based on the above.
> > >
> > > > And the main reason that I want to change the pass in parameter is
> > > > like what I mentioned previously. The commit manipulation could be
> > > > a mix of allocations and removements for the payload. And in the
> > > > spec, it also introduces examples to reduce or increase the
> > > > payload allocation. Although we're not using reduction/increment
> > > > today, it implicitly imposes the limitation to use them before
> > > > calling the removement helper function with the old state as the
> > > > passed in parameter. So I also want to remove the dependency by this
> patch.
> > > > Would like to know your thoughts about this.
> > >
> > > It's not clear what would be the benefit of changing a payload
> > > without removing it first. In any case, I think the right direction
> > > for the driver would be to meet the requirement to use the proper
> > > atomic state with the drm helper functions (not the object's current SW
> state) as described above.
> >
> > Hi Imre,
> >
> > Thanks for pointing that out and that was also one of my plan to
> > refactor! But I would take that as another patch to follow up and
> > would like to align with you the idea for this helper function itself more.
> >
> > My concern is referring to the old state to remove the time slot is
> > just not generic right for what the helper function shall accomplish.
> > Even it doesn't bring benefit as you see, having old state as the
> > input imposes limitation to drivers using it which is the downside that it
> brought. The limitation is like what I tried to explain before.
> > For an instance, if one driver has compressed streams allocated in the
> > previous commit and wants to disable these streams at the current
> > commit. It handles and accomplishes the commit into two steps that
> > firstly to disable dsc engine only (which increases the time slot),
> > and next disable the streams. Under this design, the old state can't
> > represent the exact number of time slot that it want's to remove at
> > the moment. For this case, I don't think drm_dp_remove_payload_part2
> > should block the driver to use it since the driver doesn't do things wrong.
> Conversely, it's due to the helper function constrains the driver to use an
> inappropriate input.
> >
> > And with or without my patch, the current payload allocation table
> > (i.e. vc_start_slot In new state) and the time slot that this new
> > state is going to set (i.e. time_slots in new state) are already both
> > in the new state. I think the patch doesn't add irrelevant input for
> > removing a payload as this helper function should do, because the time
> > slot it should remove is the exact one in the payload table now, not
> > the one captured in previous commit.  So in contrast, refer to old state time
> slot is a bit confusing to me because it's not generic right. Would like to know
> your thought on this point.
> > Appreciate your time!
>
> I'd like to be sure that the payload is removed with the size it was added with
> in the previous commit and as I wrote above not depend for this on the new
> payload state with a mixture of old/current/new states.
> Based on that I'd be ok for instance with a new
>
> int drm_dp_remove_port_payload(mgr, mst_state, port)
>
> function which looks up / removes the payload with the time_slots calculated
> based on the payload table as in your patch and returns the calculated
> time_slots.
>
> The AMD driver could call the above function and the current
> drm_dp_remove_payload(mgr, mst_state, old_payload) function would be
>
>       time_slots = drm_dp_remove_port_payload(mgr, mst_state,
> old_payload->port);
>       WARN_ON(time_slots != old_payload->time_slots);
>
> --Imre

Sorry but I might not fully understand what you suggested here. Would like to know
if you agree on referring to the time slot number of the payload table at the moment
is better then referring old_payload->time_slots for drm_dp_remove_payload()? If
you agree on that, my patch actually is just replacing old_payload->time_slots with
the more appropriate one. Not adding mixture of old/current but replacing the old
with the current one. And like what I explained in previous example, when calling
drm_dp_remove_payload(), the time slot number to be removed shouldn't be
constrained to the one in previous commit. The number in the payload table when
we're about to remove the payload might be a better choice. Could you elaborate
more what's the mixture that this patch is adding on, please?

As for the changing suggestion, are you suggesting to create a new function
drm_dp_remove_port_payload() to wrap up the calculation in my patch? If so, I think
that's the consensus to use current time slot number to replace the one in old_payload.
Therefore, it doesn't have to pass old_payload to drm_dp_remove_port_payload(), and
"WARN_ON(time_slots != old_payload->time_slots);" is not appropriate as for the
example that I gave previously.

Thanks for helping me out here.
--
Regards,
Wayne

Imre Deak Sept. 12, 2023, 11:18 a.m. UTC | #12

On Tue, Sep 12, 2023 at 07:26:29AM +0000, Lin, Wayne wrote:
> [Public]
> [...]
> >
> > I'd like to be sure that the payload is removed with the size it was added with
> > in the previous commit and as I wrote above not depend for this on the new
> > payload state with a mixture of old/current/new states.
> > Based on that I'd be ok for instance with a new
> >
> > int drm_dp_remove_port_payload(mgr, mst_state, port)
> >
> > function which looks up / removes the payload with the time_slots calculated
> > based on the payload table as in your patch and returns the calculated
> > time_slots.
> >
> > The AMD driver could call the above function and the current
> > drm_dp_remove_payload(mgr, mst_state, old_payload) function would be
> >
> >       time_slots = drm_dp_remove_port_payload(mgr, mst_state,
> > old_payload->port);
> >       WARN_ON(time_slots != old_payload->time_slots);
> >
> > --Imre
> 
> Sorry but I might not fully understand what you suggested here. Would like to know
> if you agree on referring to the time slot number of the payload table at the moment
> is better then referring old_payload->time_slots for drm_dp_remove_payload()? If
> you agree on that, my patch actually is just replacing old_payload->time_slots with
> the more appropriate one. Not adding mixture of old/current but replacing the old
> with the current one. 

The new_payload state contains a mixture of old/current/new state at the
moment and this patch adds more dependency on that, recalculating the
old payload size from that state. For i915 this recalculation isn't
needed, the size is already available in the old payload state.

> And like what I explained in previous example, when calling
> drm_dp_remove_payload(), the time slot number to be removed shouldn't be
> constrained to the one in previous commit. The number in the payload table when
> we're about to remove the payload might be a better choice. Could you elaborate
> more what's the mixture that this patch is adding on, please?
>
> As for the changing suggestion, are you suggesting to create a new function
> drm_dp_remove_port_payload() to wrap up the calculation in my patch? If so, I think
> that's the consensus to use current time slot number to replace the one in old_payload.
> Therefore, it doesn't have to pass old_payload to drm_dp_remove_port_payload(), and
> "WARN_ON(time_slots != old_payload->time_slots);" is not appropriate as for the
> example that I gave previously.

I meant something like the following:

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index cbef4ff28cd8a..0555433d8050b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -343,7 +343,7 @@ bool dm_helpers_dp_mst_send_payload_allocation(
 	struct amdgpu_dm_connector *aconnector;
 	struct drm_dp_mst_topology_state *mst_state;
 	struct drm_dp_mst_topology_mgr *mst_mgr;
-	struct drm_dp_mst_atomic_payload *new_payload, *old_payload;
+	struct drm_dp_mst_atomic_payload *new_payload;
 	enum mst_progress_status set_flag = MST_ALLOCATE_NEW_PAYLOAD;
 	enum mst_progress_status clr_flag = MST_CLEAR_ALLOCATED_PAYLOAD;
 	int ret = 0;
@@ -366,9 +366,8 @@ bool dm_helpers_dp_mst_send_payload_allocation(
 	if (enable) {
 		ret = drm_dp_add_payload_part2(mst_mgr, mst_state->base.state, new_payload);
 	} else {
-		dm_helpers_construct_old_payload(stream->link, mst_state->pbn_div,
-						 new_payload, old_payload);
-		drm_dp_remove_payload_part2(mst_mgr, mst_state, old_payload, new_payload);
+		drm_dp_remove_current_payload_part2(mst_mgr, mst_state->base.state,
+						    aconnector->mst_output_port);
 	}
 
 	if (ret) {
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index e04f87ff755ac..4d25dba789e91 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3382,37 +3382,70 @@ 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
+ * @port: MST port
  *
  * 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
  * function whenever it removes a payload in its HW. It's independent to the result of payload
  * allocation/deallocation at branch devices along the virtual channel.
  */
-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)
+int drm_dp_remove_current_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
+					struct drm_atomic_state *state,
+					struct drm_dp_mst_port *port)
 {
 	struct drm_dp_mst_atomic_payload *pos;
+	struct drm_dp_mst_topology_state *mst_state =
+		drm_atomic_get_new_mst_topology_state(state, mgr);
+	struct drm_dp_mst_atomic_payload *new_payload =
+		drm_atomic_get_mst_payload_state(mst_state, port);
+	int 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 != new_payload &&
+		    pos->vc_start_slot > new_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 - new_payload->vc_start_slot;
 
 	/* 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;
+			pos->vc_start_slot -= time_slots_to_remove;
 	}
 	new_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);
 
 	new_payload->payload_allocation_status = DRM_DP_MST_PAYLOAD_ALLOCATION_NONE;
+
+	return time_slots_to_remove;
+}
+EXPORT_SYMBOL(drm_dp_remove_current_payload_part2);
+
+void drm_dp_remove_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
+				 struct drm_atomic_state *state,
+				 struct drm_dp_mst_port *port)
+{
+	struct drm_dp_mst_topology_state *old_mst_state =
+		drm_atomic_get_old_mst_topology_state(state, mgr);
+	struct drm_dp_mst_atomic_payload *old_payload =
+		drm_atomic_get_mst_payload_state(old_mst_state, port);
+	int time_slots;
+
+	time_slots = drm_dp_remove_current_payload_part2(mgr, state, port);
+
+	drm_WARN_ON(mgr->dev, time_slots != old_payload->time_slots);
 }
 EXPORT_SYMBOL(drm_dp_remove_payload_part2);
+
 /**
  * drm_dp_add_payload_part2() - Execute payload update part 2
  * @mgr: Manager to use.
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 1c7f0b6afe475..3ab491d9c8d27 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -576,14 +576,6 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state,
 	struct intel_dp *intel_dp = &dig_port->dp;
 	struct intel_connector *connector =
 		to_intel_connector(old_conn_state->connector);
-	struct drm_dp_mst_topology_state *old_mst_state =
-		drm_atomic_get_old_mst_topology_state(&state->base, &intel_dp->mst_mgr);
-	struct drm_dp_mst_topology_state *new_mst_state =
-		drm_atomic_get_new_mst_topology_state(&state->base, &intel_dp->mst_mgr);
-	const struct drm_dp_mst_atomic_payload *old_payload =
-		drm_atomic_get_mst_payload_state(old_mst_state, connector->port);
-	struct drm_dp_mst_atomic_payload *new_payload =
-		drm_atomic_get_mst_payload_state(new_mst_state, connector->port);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	bool last_mst_stream;
 
@@ -604,8 +596,7 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state,
 
 	wait_for_act_sent(encoder, old_crtc_state);
 
-	drm_dp_remove_payload_part2(&intel_dp->mst_mgr, new_mst_state,
-				    old_payload, new_payload);
+	drm_dp_remove_payload_part2(&intel_dp->mst_mgr, &state->base, connector->port);
 
 	intel_ddi_disable_transcoder_func(old_crtc_state);
 
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index bba01fa0780c9..1ed724fe11f96 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -889,17 +889,13 @@ nv50_msto_cleanup(struct drm_atomic_state *state,
 	struct nouveau_drm *drm = nouveau_drm(msto->encoder.dev);
 	struct drm_dp_mst_atomic_payload *new_payload =
 		drm_atomic_get_mst_payload_state(new_mst_state, msto->mstc->port);
-	struct drm_dp_mst_topology_state *old_mst_state =
-		drm_atomic_get_old_mst_topology_state(state, mgr);
-	const struct drm_dp_mst_atomic_payload *old_payload =
-		drm_atomic_get_mst_payload_state(old_mst_state, msto->mstc->port);
 
 	NV_ATOMIC(drm, "%s: msto cleanup\n", msto->encoder.name);
 
 	if (msto->disabled) {
 		msto->mstc = NULL;
 		msto->disabled = false;
-		drm_dp_remove_payload_part2(mgr, new_mst_state, old_payload, new_payload);
+		drm_dp_remove_payload_part2(mgr, state, msto->mstc->port);
 	} else if (msto->enabled) {
 		drm_dp_add_payload_part2(mgr, state, new_payload);
 		msto->enabled = false;
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 4429d3b1745b6..9288501ffe8d2 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -856,9 +856,11 @@ void drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
 				 struct drm_dp_mst_topology_state *mst_state,
 				 struct drm_dp_mst_atomic_payload *payload);
 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_atomic_state *state,
+				 struct drm_dp_mst_port *port);
+int drm_dp_remove_current_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
+					struct drm_atomic_state *state,
+					struct drm_dp_mst_port *port);
 
 int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
 
> Thanks for helping me out here.

Lin, Wayne Sept. 14, 2023, 3:29 a.m. UTC | #13

[Public]

> -----Original Message-----
> From: Imre Deak <[e-mail]>
> Sent: Tuesday, September 12, 2023 7:19 PM
> To: Lin, Wayne <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Wentland, Harry <[e-mail]>; Zuo, Jerry
> <[e-mail]>
> Subject: Re: [PATCH 3/3] drm/mst: adjust the function
> drm_dp_remove_payload_part2()
>
> On Tue, Sep 12, 2023 at 07:26:29AM +0000, Lin, Wayne wrote:
> > [Public]
> > [...]
> > >
> > > I'd like to be sure that the payload is removed with the size it was
> > > added with in the previous commit and as I wrote above not depend
> > > for this on the new payload state with a mixture of old/current/new states.
> > > Based on that I'd be ok for instance with a new
> > >
> > > int drm_dp_remove_port_payload(mgr, mst_state, port)
> > >
> > > function which looks up / removes the payload with the time_slots
> > > calculated based on the payload table as in your patch and returns
> > > the calculated time_slots.
> > >
> > > The AMD driver could call the above function and the current
> > > drm_dp_remove_payload(mgr, mst_state, old_payload) function would be
> > >
> > >       time_slots = drm_dp_remove_port_payload(mgr, mst_state,
> > > old_payload->port);
> > >       WARN_ON(time_slots != old_payload->time_slots);
> > >
> > > --Imre
> >
> > Sorry but I might not fully understand what you suggested here. Would
> > like to know if you agree on referring to the time slot number of the
> > payload table at the moment is better then referring
> > old_payload->time_slots for drm_dp_remove_payload()? If you agree on
> > that, my patch actually is just replacing old_payload->time_slots with
> > the more appropriate one. Not adding mixture of old/current but replacing
> the old with the current one.
>
> The new_payload state contains a mixture of old/current/new state at the
> moment and this patch adds more dependency on that, recalculating the old
> payload size from that state. For i915 this recalculation isn't needed, the size is
> already available in the old payload state.
>

I see. Thanks, Imre. So it's about the idea to have another patch to extract things
like vc_start_slot out of mst state.

> > And like what I explained in previous example, when calling
> > drm_dp_remove_payload(), the time slot number to be removed shouldn't
> > be constrained to the one in previous commit. The number in the
> > payload table when we're about to remove the payload might be a better
> > choice. Could you elaborate more what's the mixture that this patch is
> adding on, please?
> >
> > As for the changing suggestion, are you suggesting to create a new
> > function
> > drm_dp_remove_port_payload() to wrap up the calculation in my patch?
> > If so, I think that's the consensus to use current time slot number to replace
> the one in old_payload.
> > Therefore, it doesn't have to pass old_payload to
> > drm_dp_remove_port_payload(), and "WARN_ON(time_slots !=
> > old_payload->time_slots);" is not appropriate as for the example that I gave
> previously.
>
> I meant something like the following:

The change looks good to me. I'll update the patch. Thanks for the help.

>
> diff --git
> a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index cbef4ff28cd8a..0555433d8050b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -343,7 +343,7 @@ bool dm_helpers_dp_mst_send_payload_allocation(
>       struct amdgpu_dm_connector *aconnector;
>       struct drm_dp_mst_topology_state *mst_state;
>       struct drm_dp_mst_topology_mgr *mst_mgr;
> -     struct drm_dp_mst_atomic_payload *new_payload, *old_payload;
> +     struct drm_dp_mst_atomic_payload *new_payload;
>       enum mst_progress_status set_flag =
> MST_ALLOCATE_NEW_PAYLOAD;
>       enum mst_progress_status clr_flag =
> MST_CLEAR_ALLOCATED_PAYLOAD;
>       int ret = 0;
> @@ -366,9 +366,8 @@ bool dm_helpers_dp_mst_send_payload_allocation(
>       if (enable) {
>               ret = drm_dp_add_payload_part2(mst_mgr, mst_state-
> >base.state, new_payload);
>       } else {
> -             dm_helpers_construct_old_payload(stream->link, mst_state-
> >pbn_div,
> -                                              new_payload, old_payload);
> -             drm_dp_remove_payload_part2(mst_mgr, mst_state,
> old_payload, new_payload);
> +             drm_dp_remove_current_payload_part2(mst_mgr,
> mst_state->base.state,
> +                                                 aconnector-
> >mst_output_port);
>       }
>
>       if (ret) {
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index e04f87ff755ac..4d25dba789e91 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -3382,37 +3382,70 @@
> 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
> + * @port: MST port
>   *
>   * 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
>   * function whenever it removes a payload in its HW. It's independent to the
> result of payload
>   * allocation/deallocation at branch devices along the virtual channel.
>   */
> -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)
> +int drm_dp_remove_current_payload_part2(struct
> drm_dp_mst_topology_mgr *mgr,
> +                                     struct drm_atomic_state *state,
> +                                     struct drm_dp_mst_port *port)
>  {
>       struct drm_dp_mst_atomic_payload *pos;
> +     struct drm_dp_mst_topology_state *mst_state =
> +             drm_atomic_get_new_mst_topology_state(state, mgr);
> +     struct drm_dp_mst_atomic_payload *new_payload =
> +             drm_atomic_get_mst_payload_state(mst_state, port);
> +     int 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 != new_payload &&
> +                 pos->vc_start_slot > new_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 -
> +new_payload->vc_start_slot;
>
>       /* 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;
> +                     pos->vc_start_slot -= time_slots_to_remove;
>       }
>       new_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);
>
>       new_payload->payload_allocation_status =
> DRM_DP_MST_PAYLOAD_ALLOCATION_NONE;
> +
> +     return time_slots_to_remove;
> +}
> +EXPORT_SYMBOL(drm_dp_remove_current_payload_part2);
> +
> +void drm_dp_remove_payload_part2(struct drm_dp_mst_topology_mgr
> *mgr,
> +                              struct drm_atomic_state *state,
> +                              struct drm_dp_mst_port *port)
> +{
> +     struct drm_dp_mst_topology_state *old_mst_state =
> +             drm_atomic_get_old_mst_topology_state(state, mgr);
> +     struct drm_dp_mst_atomic_payload *old_payload =
> +             drm_atomic_get_mst_payload_state(old_mst_state, port);
> +     int time_slots;
> +
> +     time_slots = drm_dp_remove_current_payload_part2(mgr, state,
> port);
> +
> +     drm_WARN_ON(mgr->dev, time_slots != old_payload->time_slots);
>  }
>  EXPORT_SYMBOL(drm_dp_remove_payload_part2);
> +
>  /**
>   * drm_dp_add_payload_part2() - Execute payload update part 2
>   * @mgr: Manager to use.
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 1c7f0b6afe475..3ab491d9c8d27 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -576,14 +576,6 @@ static void intel_mst_post_disable_dp(struct
> intel_atomic_state *state,
>       struct intel_dp *intel_dp = &dig_port->dp;
>       struct intel_connector *connector =
>               to_intel_connector(old_conn_state->connector);
> -     struct drm_dp_mst_topology_state *old_mst_state =
> -             drm_atomic_get_old_mst_topology_state(&state->base,
> &intel_dp->mst_mgr);
> -     struct drm_dp_mst_topology_state *new_mst_state =
> -             drm_atomic_get_new_mst_topology_state(&state->base,
> &intel_dp->mst_mgr);
> -     const struct drm_dp_mst_atomic_payload *old_payload =
> -             drm_atomic_get_mst_payload_state(old_mst_state,
> connector->port);
> -     struct drm_dp_mst_atomic_payload *new_payload =
> -             drm_atomic_get_mst_payload_state(new_mst_state,
> connector->port);
>       struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>       bool last_mst_stream;
>
> @@ -604,8 +596,7 @@ static void intel_mst_post_disable_dp(struct
> intel_atomic_state *state,
>
>       wait_for_act_sent(encoder, old_crtc_state);
>
> -     drm_dp_remove_payload_part2(&intel_dp->mst_mgr,
> new_mst_state,
> -                                 old_payload, new_payload);
> +     drm_dp_remove_payload_part2(&intel_dp->mst_mgr, &state->base,
> +connector->port);
>
>       intel_ddi_disable_transcoder_func(old_crtc_state);
>
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index bba01fa0780c9..1ed724fe11f96 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -889,17 +889,13 @@ nv50_msto_cleanup(struct drm_atomic_state
> *state,
>       struct nouveau_drm *drm = nouveau_drm(msto->encoder.dev);
>       struct drm_dp_mst_atomic_payload *new_payload =
>               drm_atomic_get_mst_payload_state(new_mst_state, msto-
> >mstc->port);
> -     struct drm_dp_mst_topology_state *old_mst_state =
> -             drm_atomic_get_old_mst_topology_state(state, mgr);
> -     const struct drm_dp_mst_atomic_payload *old_payload =
> -             drm_atomic_get_mst_payload_state(old_mst_state, msto-
> >mstc->port);
>
>       NV_ATOMIC(drm, "%s: msto cleanup\n", msto->encoder.name);
>
>       if (msto->disabled) {
>               msto->mstc = NULL;
>               msto->disabled = false;
> -             drm_dp_remove_payload_part2(mgr, new_mst_state,
> old_payload, new_payload);
> +             drm_dp_remove_payload_part2(mgr, state, msto->mstc-
> >port);
>       } else if (msto->enabled) {
>               drm_dp_add_payload_part2(mgr, state, new_payload);
>               msto->enabled = false;
> diff --git a/include/drm/display/drm_dp_mst_helper.h
> b/include/drm/display/drm_dp_mst_helper.h
> index 4429d3b1745b6..9288501ffe8d2 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -856,9 +856,11 @@ void drm_dp_remove_payload_part1(struct
> drm_dp_mst_topology_mgr *mgr,
>                                struct drm_dp_mst_topology_state
> *mst_state,
>                                struct drm_dp_mst_atomic_payload
> *payload);  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_atomic_state *state,
> +                              struct drm_dp_mst_port *port);
> +int drm_dp_remove_current_payload_part2(struct
> drm_dp_mst_topology_mgr *mgr,
> +                                     struct drm_atomic_state *state,
> +                                     struct drm_dp_mst_port *port);
>
>  int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
>
> > Thanks for helping me out here.
--
Regards,
Wayne

Patch

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 9ad509279b0a..e852da686c26 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -203,40 +203,6 @@  void dm_helpers_dp_update_branch_info(
 	const struct dc_link *link)
 {}
 
-static void dm_helpers_construct_old_payload(
-			struct dc_link *link,
-			int pbn_per_slot,
-			struct drm_dp_mst_atomic_payload *new_payload,
-			struct drm_dp_mst_atomic_payload *old_payload)
-{
-	struct link_mst_stream_allocation_table current_link_table =
-									link->mst_stream_alloc_table;
-	struct link_mst_stream_allocation *dc_alloc;
-	int i;
-
-	*old_payload = *new_payload;
-
-	/* Set correct time_slots/PBN of old payload.
-	 * other fields (delete & dsc_enabled) in
-	 * struct drm_dp_mst_atomic_payload are don't care fields
-	 * while calling drm_dp_remove_payload_part2()
-	 */
-	for (i = 0; i < current_link_table.stream_count; i++) {
-		dc_alloc =
-			&current_link_table.stream_allocations[i];
-
-		if (dc_alloc->vcp_id == new_payload->vcpi) {
-			old_payload->time_slots = dc_alloc->slot_count;
-			old_payload->pbn = dc_alloc->slot_count * pbn_per_slot;
-			break;
-		}
-	}
-
-	/* make sure there is an old payload*/
-	ASSERT(i != current_link_table.stream_count);
-
-}
-
 /*
  * Writes payload allocation table in immediate downstream device.
  */
@@ -248,7 +214,7 @@  bool dm_helpers_dp_mst_write_payload_allocation_table(
 {
 	struct amdgpu_dm_connector *aconnector;
 	struct drm_dp_mst_topology_state *mst_state;
-	struct drm_dp_mst_atomic_payload *target_payload, *new_payload, old_payload;
+	struct drm_dp_mst_atomic_payload *payload;
 	struct drm_dp_mst_topology_mgr *mst_mgr;
 
 	aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
@@ -262,27 +228,20 @@  bool dm_helpers_dp_mst_write_payload_allocation_table(
 
 	mst_mgr = &aconnector->mst_root->mst_mgr;
 	mst_state = to_drm_dp_mst_topology_state(mst_mgr->base.state);
-	new_payload = drm_atomic_get_mst_payload_state(mst_state, aconnector->mst_output_port);
-
-	if (enable) {
-		target_payload = new_payload;
+	payload = drm_atomic_get_mst_payload_state(mst_state, aconnector->mst_output_port);
 
+	if (enable)
 		/* It's OK for this to fail */
-		drm_dp_add_payload_part1(mst_mgr, mst_state, new_payload);
-	} else {
-		/* construct old payload by VCPI*/
-		dm_helpers_construct_old_payload(stream->link, mst_state->pbn_div,
-						new_payload, &old_payload);
-		target_payload = &old_payload;
+		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
+	else
 
-		drm_dp_remove_payload_part1(mst_mgr, mst_state, new_payload);
-	}
+		drm_dp_remove_payload_part1(mst_mgr, mst_state, payload);
 
 	/* mst_mgr->->payloads are VC payload notify MST branch using DPCD or
 	 * AUX message. The sequence is slot 1-63 allocated sequence for each
 	 * stream. AMD ASIC stream slot allocation should follow the same
 	 * sequence. copy DRM MST allocation to dc */
-	fill_dc_mst_payload_table_from_drm(stream->link, enable, target_payload, proposed_table);
+	fill_dc_mst_payload_table_from_drm(stream->link, enable, payload, proposed_table);
 
 	return true;
 }
@@ -341,7 +300,7 @@  bool dm_helpers_dp_mst_send_payload_allocation(
 	struct amdgpu_dm_connector *aconnector;
 	struct drm_dp_mst_topology_state *mst_state;
 	struct drm_dp_mst_topology_mgr *mst_mgr;
-	struct drm_dp_mst_atomic_payload *new_payload, *old_payload;
+	struct drm_dp_mst_atomic_payload *payload;
 	enum mst_progress_status set_flag = MST_ALLOCATE_NEW_PAYLOAD;
 	enum mst_progress_status clr_flag = MST_CLEAR_ALLOCATED_PAYLOAD;
 	int ret = 0;
@@ -354,20 +313,17 @@  bool dm_helpers_dp_mst_send_payload_allocation(
 	mst_mgr = &aconnector->mst_root->mst_mgr;
 	mst_state = to_drm_dp_mst_topology_state(mst_mgr->base.state);
 
-	new_payload = drm_atomic_get_mst_payload_state(mst_state, aconnector->mst_output_port);
+	payload = drm_atomic_get_mst_payload_state(mst_state, aconnector->mst_output_port);
 
 	if (!enable) {
 		set_flag = MST_CLEAR_ALLOCATED_PAYLOAD;
 		clr_flag = MST_ALLOCATE_NEW_PAYLOAD;
 	}
 
-	if (enable) {
-		ret = drm_dp_add_payload_part2(mst_mgr, mst_state->base.state, new_payload);
-	} else {
-		dm_helpers_construct_old_payload(stream->link, mst_state->pbn_div,
-						 new_payload, old_payload);
-		drm_dp_remove_payload_part2(mst_mgr, mst_state, old_payload, new_payload);
-	}
+	if (enable)
+		ret = drm_dp_add_payload_part2(mst_mgr, mst_state->base.state, payload);
+	else
+		drm_dp_remove_payload_part2(mst_mgr, mst_state, payload);
 
 	if (ret) {
 		amdgpu_dm_set_mst_status(&aconnector->mst_status,
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;
 
 	/* 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;
 }
 EXPORT_SYMBOL(drm_dp_remove_payload_part2);
 /**
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 5f73cdabe7a1..91750c1dfc48 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -583,12 +583,8 @@  static void intel_mst_post_disable_dp(struct intel_atomic_state *state,
 	struct intel_dp *intel_dp = &dig_port->dp;
 	struct intel_connector *connector =
 		to_intel_connector(old_conn_state->connector);
-	struct drm_dp_mst_topology_state *old_mst_state =
-		drm_atomic_get_old_mst_topology_state(&state->base, &intel_dp->mst_mgr);
 	struct drm_dp_mst_topology_state *new_mst_state =
 		drm_atomic_get_new_mst_topology_state(&state->base, &intel_dp->mst_mgr);
-	const struct drm_dp_mst_atomic_payload *old_payload =
-		drm_atomic_get_mst_payload_state(old_mst_state, connector->port);
 	struct drm_dp_mst_atomic_payload *new_payload =
 		drm_atomic_get_mst_payload_state(new_mst_state, connector->port);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -611,8 +607,7 @@  static void intel_mst_post_disable_dp(struct intel_atomic_state *state,
 
 	wait_for_act_sent(encoder, old_crtc_state);
 
-	drm_dp_remove_payload_part2(&intel_dp->mst_mgr, new_mst_state,
-				    old_payload, new_payload);
+	drm_dp_remove_payload_part2(&intel_dp->mst_mgr, new_mst_state, new_payload);
 
 	intel_ddi_disable_transcoder_func(old_crtc_state);
 
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 6f1b7fcb98e6..63e2a286fffc 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -889,17 +889,13 @@  nv50_msto_cleanup(struct drm_atomic_state *state,
 	struct nouveau_drm *drm = nouveau_drm(msto->encoder.dev);
 	struct drm_dp_mst_atomic_payload *new_payload =
 		drm_atomic_get_mst_payload_state(new_mst_state, msto->mstc->port);
-	struct drm_dp_mst_topology_state *old_mst_state =
-		drm_atomic_get_old_mst_topology_state(state, mgr);
-	const struct drm_dp_mst_atomic_payload *old_payload =
-		drm_atomic_get_mst_payload_state(old_mst_state, msto->mstc->port);
 
 	NV_ATOMIC(drm, "%s: msto cleanup\n", msto->encoder.name);
 
 	if (msto->disabled) {
 		msto->mstc = NULL;
 		msto->disabled = false;
-		drm_dp_remove_payload_part2(mgr, new_mst_state, old_payload, new_payload);
+		drm_dp_remove_payload_part2(mgr, new_mst_state, new_payload);
 	} else if (msto->enabled) {
 		drm_dp_add_payload_part2(mgr, state, new_payload);
 		msto->enabled = false;
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 4429d3b1745b..3f8ad28c77b1 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -853,12 +853,11 @@  int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
 			     struct drm_atomic_state *state,
 			     struct drm_dp_mst_atomic_payload *payload);
 void drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
-				 struct drm_dp_mst_topology_state *mst_state,
-				 struct drm_dp_mst_atomic_payload *payload);
+				  struct drm_dp_mst_topology_state *mst_state,
+				  struct drm_dp_mst_atomic_payload *payload);
 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_topology_state *mst_state,
+				  struct drm_dp_mst_atomic_payload *payload);
 
 int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);