[PATCH 2/9] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[PATCH 2/9] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Imre Deak]

Atm, drm_dp_remove_payload() uses the same payload state to both get the
vc_start_slot required for the payload removal DPCD message and to
deduct time_slots from vc_start_slot of all payloads after the one being
removed.

The above isn't always correct, as vc_start_slot must be the up-to-date
version contained in the new payload state, but time_slots must be the
one used when the payload was previously added, contained in the old
payload state. The new payload's time_slots can change vs. the old one
if the current atomic commit changes the corresponding mode.

This patch let's drivers pass the old and new payload states to
drm_dp_remove_payload(), but keeps these the same for now in all drivers
not to change the behavior. A follow-up i915 patch will pass in that
driver the correct old and new states to the function.

Cc: Lyude Paul <lyude@redhat.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Karol Herbst <kherbst@redhat.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Wayne Lin <Wayne.Lin@amd.com>
Cc: stable@vger.kernel.org # 6.1
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 ++++++++++---------
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  4 +++-
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 +-
 include/drm/display/drm_dp_mst_helper.h       |  3 ++-
 5 files changed, 19 insertions(+), 14 deletions(-)

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 6994c9a1ed858..fed4ce6821161 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
@@ -179,7 +179,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
 	if (enable)
 		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
 	else
-		drm_dp_remove_payload(mst_mgr, mst_state, payload);
+		drm_dp_remove_payload(mst_mgr, mst_state, payload, 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
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 5861b0a6247bc..ebf6e31e156e0 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
  * drm_dp_remove_payload() - Remove an MST payload
  * @mgr: Manager to use.
  * @mst_state: The MST atomic state
- * @payload: The payload to write
+ * @old_payload: The payload with its old state
+ * @new_payload: The payload to write
  *
  * Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
  * the starting time slots of all other payloads which would have been shifted towards the start of
@@ -3350,33 +3351,34 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
  */
 void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
 			   struct drm_dp_mst_topology_state *mst_state,
-			   struct drm_dp_mst_atomic_payload *payload)
+			   const struct drm_dp_mst_atomic_payload *old_payload,
+			   struct drm_dp_mst_atomic_payload *new_payload)
 {
 	struct drm_dp_mst_atomic_payload *pos;
 	bool send_remove = false;
 
 	/* We failed to make the payload, so nothing to do */
-	if (payload->vc_start_slot == -1)
+	if (new_payload->vc_start_slot == -1)
 		return;
 
 	mutex_lock(&mgr->lock);
-	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
+	send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary);
 	mutex_unlock(&mgr->lock);
 
 	if (send_remove)
-		drm_dp_destroy_payload_step1(mgr, mst_state, payload);
+		drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
 	else
 		drm_dbg_kms(mgr->dev, "Payload for VCPI %d not in topology, not sending remove\n",
-			    payload->vcpi);
+			    new_payload->vcpi);
 
 	list_for_each_entry(pos, &mst_state->payloads, next) {
-		if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)
-			pos->vc_start_slot -= payload->time_slots;
+		if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
+			pos->vc_start_slot -= old_payload->time_slots;
 	}
-	payload->vc_start_slot = -1;
+	new_payload->vc_start_slot = -1;
 
 	mgr->payload_count--;
-	mgr->next_start_slot -= payload->time_slots;
+	mgr->next_start_slot -= old_payload->time_slots;
 }
 EXPORT_SYMBOL(drm_dp_remove_payload);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index ba29c294b7c1b..5f7bcb5c14847 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
 		to_intel_connector(old_conn_state->connector);
 	struct drm_dp_mst_topology_state *mst_state =
 		drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
+	struct drm_dp_mst_atomic_payload *payload =
+		drm_atomic_get_mst_payload_state(mst_state, connector->port);
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
 
 	drm_dbg_kms(&i915->drm, "active links %d\n",
@@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
 	intel_hdcp_disable(intel_mst->connector);
 
 	drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
-			      drm_atomic_get_mst_payload_state(mst_state, connector->port));
+			      payload, payload);
 
 	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
 }
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index edcb2529b4025..ed9d374147b8d 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state,
 
 	// TODO: Figure out if we want to do a better job of handling VCPI allocation failures here?
 	if (msto->disabled) {
-		drm_dp_remove_payload(mgr, mst_state, payload);
+		drm_dp_remove_payload(mgr, mst_state, payload, payload);
 
 		nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0);
 	} else {
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 41fd8352ab656..f5eb9aa152b14 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
 			     struct drm_dp_mst_atomic_payload *payload);
 void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
 			   struct drm_dp_mst_topology_state *mst_state,
-			   struct drm_dp_mst_atomic_payload *payload);
+			   const struct drm_dp_mst_atomic_payload *old_payload,
+			   struct drm_dp_mst_atomic_payload *new_payload);
 
 int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
 
-- 
2.37.1

Re: [PATCH 2/9] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Ville Syrjälä]

On Wed, Jan 25, 2023 at 01:48:45PM +0200, Imre Deak wrote:
> Atm, drm_dp_remove_payload() uses the same payload state to both get the
> vc_start_slot required for the payload removal DPCD message and to
> deduct time_slots from vc_start_slot of all payloads after the one being
> removed.
> 
> The above isn't always correct, as vc_start_slot must be the up-to-date
> version contained in the new payload state,

Why is that? In fact couldn't we just clear both start_slot and
pbn to 0 here?

> but time_slots must be the
> one used when the payload was previously added, contained in the old
> payload state. The new payload's time_slots can change vs. the old one
> if the current atomic commit changes the corresponding mode.
> 
> This patch let's drivers pass the old and new payload states to
> drm_dp_remove_payload(), but keeps these the same for now in all drivers
> not to change the behavior. A follow-up i915 patch will pass in that
> driver the correct old and new states to the function.
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Karol Herbst <kherbst@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Wayne Lin <Wayne.Lin@amd.com>
> Cc: stable@vger.kernel.org # 6.1
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 ++++++++++---------
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  4 +++-
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 +-
>  include/drm/display/drm_dp_mst_helper.h       |  3 ++-
>  5 files changed, 19 insertions(+), 14 deletions(-)
> 
> 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 6994c9a1ed858..fed4ce6821161 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
> @@ -179,7 +179,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>  	if (enable)
>  		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
>  	else
> -		drm_dp_remove_payload(mst_mgr, mst_state, payload);
> +		drm_dp_remove_payload(mst_mgr, mst_state, payload, 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
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 5861b0a6247bc..ebf6e31e156e0 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
>   * drm_dp_remove_payload() - Remove an MST payload
>   * @mgr: Manager to use.
>   * @mst_state: The MST atomic state
> - * @payload: The payload to write
> + * @old_payload: The payload with its old state
> + * @new_payload: The payload to write
>   *
>   * Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
>   * the starting time slots of all other payloads which would have been shifted towards the start of
> @@ -3350,33 +3351,34 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
>   */
>  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
>  			   struct drm_dp_mst_topology_state *mst_state,
> -			   struct drm_dp_mst_atomic_payload *payload)
> +			   const struct drm_dp_mst_atomic_payload *old_payload,
> +			   struct drm_dp_mst_atomic_payload *new_payload)
>  {
>  	struct drm_dp_mst_atomic_payload *pos;
>  	bool send_remove = false;
>  
>  	/* We failed to make the payload, so nothing to do */
> -	if (payload->vc_start_slot == -1)
> +	if (new_payload->vc_start_slot == -1)
>  		return;

So I take it the only reason we even have that is the copy being done in
drm_dp_mst_atomic_wait_for_dependencies()? I don't really understand
why any of that is being done tbh. If the new payload hasn't been
allocated yet then why can't its vc_start_slots just stay at -1
until that time?

This whole thing feels a bit weird since the payload table really isn't
your normal atomic state that is computed ahead of time. Instead it just
gets built up on as we go during the actual commit. So not really sure
why we're even tracking it in atomic state...

>  
>  	mutex_lock(&mgr->lock);
> -	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
> +	send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary);
>  	mutex_unlock(&mgr->lock);
>  
>  	if (send_remove)
> -		drm_dp_destroy_payload_step1(mgr, mst_state, payload);
> +		drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
>  	else
>  		drm_dbg_kms(mgr->dev, "Payload for VCPI %d not in topology, not sending remove\n",
> -			    payload->vcpi);
> +			    new_payload->vcpi);
>  
>  	list_for_each_entry(pos, &mst_state->payloads, next) {
> -		if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)
> -			pos->vc_start_slot -= payload->time_slots;
> +		if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
> +			pos->vc_start_slot -= old_payload->time_slots;
>  	}
> -	payload->vc_start_slot = -1;
> +	new_payload->vc_start_slot = -1;
>  
>  	mgr->payload_count--;
> -	mgr->next_start_slot -= payload->time_slots;
> +	mgr->next_start_slot -= old_payload->time_slots;
>  }
>  EXPORT_SYMBOL(drm_dp_remove_payload);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index ba29c294b7c1b..5f7bcb5c14847 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
>  		to_intel_connector(old_conn_state->connector);
>  	struct drm_dp_mst_topology_state *mst_state =
>  		drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> +	struct drm_dp_mst_atomic_payload *payload =
> +		drm_atomic_get_mst_payload_state(mst_state, connector->port);
>  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>  
>  	drm_dbg_kms(&i915->drm, "active links %d\n",
> @@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
>  	intel_hdcp_disable(intel_mst->connector);
>  
>  	drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
> -			      drm_atomic_get_mst_payload_state(mst_state, connector->port));
> +			      payload, payload);
>  
>  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
>  }
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index edcb2529b4025..ed9d374147b8d 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state,
>  
>  	// TODO: Figure out if we want to do a better job of handling VCPI allocation failures here?
>  	if (msto->disabled) {
> -		drm_dp_remove_payload(mgr, mst_state, payload);
> +		drm_dp_remove_payload(mgr, mst_state, payload, payload);
>  
>  		nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0);
>  	} else {
> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> index 41fd8352ab656..f5eb9aa152b14 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
>  			     struct drm_dp_mst_atomic_payload *payload);
>  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
>  			   struct drm_dp_mst_topology_state *mst_state,
> -			   struct drm_dp_mst_atomic_payload *payload);
> +			   const struct drm_dp_mst_atomic_payload *old_payload,
> +			   struct drm_dp_mst_atomic_payload *new_payload);
>  
>  int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
>  
> -- 
> 2.37.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/9] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Ville Syrjälä]

On Thu, Jan 26, 2023 at 07:37:04PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 25, 2023 at 01:48:45PM +0200, Imre Deak wrote:
> > Atm, drm_dp_remove_payload() uses the same payload state to both get the
> > vc_start_slot required for the payload removal DPCD message and to
> > deduct time_slots from vc_start_slot of all payloads after the one being
> > removed.
> > 
> > The above isn't always correct, as vc_start_slot must be the up-to-date
> > version contained in the new payload state,
> 
> Why is that? In fact couldn't we just clear both start_slot and
> pbn to 0 here?

OK, so it has to be the "current" start slot. Which in this case
means new_payload since that's what's housed in the new topolpogy state
which is the one getting mutated when streams are being removed/added.

Confusing, but seems correct
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> > but time_slots must be the
> > one used when the payload was previously added, contained in the old
> > payload state. The new payload's time_slots can change vs. the old one
> > if the current atomic commit changes the corresponding mode.
> > 
> > This patch let's drivers pass the old and new payload states to
> > drm_dp_remove_payload(), but keeps these the same for now in all drivers
> > not to change the behavior. A follow-up i915 patch will pass in that
> > driver the correct old and new states to the function.
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Ben Skeggs <bskeggs@redhat.com>
> > Cc: Karol Herbst <kherbst@redhat.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Wayne Lin <Wayne.Lin@amd.com>
> > Cc: stable@vger.kernel.org # 6.1
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 ++++++++++---------
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  4 +++-
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 +-
> >  include/drm/display/drm_dp_mst_helper.h       |  3 ++-
> >  5 files changed, 19 insertions(+), 14 deletions(-)
> > 
> > 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 6994c9a1ed858..fed4ce6821161 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
> > @@ -179,7 +179,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
> >  	if (enable)
> >  		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
> >  	else
> > -		drm_dp_remove_payload(mst_mgr, mst_state, payload);
> > +		drm_dp_remove_payload(mst_mgr, mst_state, payload, 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
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 5861b0a6247bc..ebf6e31e156e0 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> >   * drm_dp_remove_payload() - Remove an MST payload
> >   * @mgr: Manager to use.
> >   * @mst_state: The MST atomic state
> > - * @payload: The payload to write
> > + * @old_payload: The payload with its old state
> > + * @new_payload: The payload to write
> >   *
> >   * Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
> >   * the starting time slots of all other payloads which would have been shifted towards the start of
> > @@ -3350,33 +3351,34 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> >   */
> >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> >  			   struct drm_dp_mst_topology_state *mst_state,
> > -			   struct drm_dp_mst_atomic_payload *payload)
> > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > +			   struct drm_dp_mst_atomic_payload *new_payload)
> >  {
> >  	struct drm_dp_mst_atomic_payload *pos;
> >  	bool send_remove = false;
> >  
> >  	/* We failed to make the payload, so nothing to do */
> > -	if (payload->vc_start_slot == -1)
> > +	if (new_payload->vc_start_slot == -1)
> >  		return;
> 
> So I take it the only reason we even have that is the copy being done in
> drm_dp_mst_atomic_wait_for_dependencies()? I don't really understand
> why any of that is being done tbh. If the new payload hasn't been
> allocated yet then why can't its vc_start_slots just stay at -1
> until that time?
> 
> This whole thing feels a bit weird since the payload table really isn't
> your normal atomic state that is computed ahead of time. Instead it just
> gets built up on as we go during the actual commit. So not really sure
> why we're even tracking it in atomic state...
> 
> >  
> >  	mutex_lock(&mgr->lock);
> > -	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
> > +	send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary);
> >  	mutex_unlock(&mgr->lock);
> >  
> >  	if (send_remove)
> > -		drm_dp_destroy_payload_step1(mgr, mst_state, payload);
> > +		drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
> >  	else
> >  		drm_dbg_kms(mgr->dev, "Payload for VCPI %d not in topology, not sending remove\n",
> > -			    payload->vcpi);
> > +			    new_payload->vcpi);
> >  
> >  	list_for_each_entry(pos, &mst_state->payloads, next) {
> > -		if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)
> > -			pos->vc_start_slot -= payload->time_slots;
> > +		if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
> > +			pos->vc_start_slot -= old_payload->time_slots;
> >  	}
> > -	payload->vc_start_slot = -1;
> > +	new_payload->vc_start_slot = -1;
> >  
> >  	mgr->payload_count--;
> > -	mgr->next_start_slot -= payload->time_slots;
> > +	mgr->next_start_slot -= old_payload->time_slots;
> >  }
> >  EXPORT_SYMBOL(drm_dp_remove_payload);
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index ba29c294b7c1b..5f7bcb5c14847 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> >  		to_intel_connector(old_conn_state->connector);
> >  	struct drm_dp_mst_topology_state *mst_state =
> >  		drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> > +	struct drm_dp_mst_atomic_payload *payload =
> > +		drm_atomic_get_mst_payload_state(mst_state, connector->port);
> >  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> >  
> >  	drm_dbg_kms(&i915->drm, "active links %d\n",
> > @@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> >  	intel_hdcp_disable(intel_mst->connector);
> >  
> >  	drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
> > -			      drm_atomic_get_mst_payload_state(mst_state, connector->port));
> > +			      payload, payload);
> >  
> >  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
> >  }
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index edcb2529b4025..ed9d374147b8d 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state,
> >  
> >  	// TODO: Figure out if we want to do a better job of handling VCPI allocation failures here?
> >  	if (msto->disabled) {
> > -		drm_dp_remove_payload(mgr, mst_state, payload);
> > +		drm_dp_remove_payload(mgr, mst_state, payload, payload);
> >  
> >  		nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0);
> >  	} else {
> > diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> > index 41fd8352ab656..f5eb9aa152b14 100644
> > --- a/include/drm/display/drm_dp_mst_helper.h
> > +++ b/include/drm/display/drm_dp_mst_helper.h
> > @@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
> >  			     struct drm_dp_mst_atomic_payload *payload);
> >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> >  			   struct drm_dp_mst_topology_state *mst_state,
> > -			   struct drm_dp_mst_atomic_payload *payload);
> > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > +			   struct drm_dp_mst_atomic_payload *new_payload);
> >  
> >  int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
> >  
> > -- 
> > 2.37.1
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/9] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Imre Deak]

On Thu, Jan 26, 2023 at 08:33:42PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 26, 2023 at 07:37:04PM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 25, 2023 at 01:48:45PM +0200, Imre Deak wrote:
> > > Atm, drm_dp_remove_payload() uses the same payload state to both get the
> > > vc_start_slot required for the payload removal DPCD message and to
> > > deduct time_slots from vc_start_slot of all payloads after the one being
> > > removed.
> > > 
> > > The above isn't always correct, as vc_start_slot must be the up-to-date
> > > version contained in the new payload state,
> > 
> > Why is that? In fact couldn't we just clear both start_slot and
> > pbn to 0 here?

The DP spec requires sending the actual start slot, even though the hubs
I checked ignored it and deleted all the allocation of the given VCPI
whatever start slot was used. Imo we should still not depend on this.

> OK, so it has to be the "current" start slot. Which in this case
> means new_payload since that's what's housed in the new topolpogy state
> which is the one getting mutated when streams are being removed/added.

Yes.

> Confusing, but seems correct
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > 
> > > but time_slots must be the
> > > one used when the payload was previously added, contained in the old
> > > payload state. The new payload's time_slots can change vs. the old one
> > > if the current atomic commit changes the corresponding mode.
> > > 
> > > This patch let's drivers pass the old and new payload states to
> > > drm_dp_remove_payload(), but keeps these the same for now in all drivers
> > > not to change the behavior. A follow-up i915 patch will pass in that
> > > driver the correct old and new states to the function.
> > > 
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Ben Skeggs <bskeggs@redhat.com>
> > > Cc: Karol Herbst <kherbst@redhat.com>
> > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: Wayne Lin <Wayne.Lin@amd.com>
> > > Cc: stable@vger.kernel.org # 6.1
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
> > >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 ++++++++++---------
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  4 +++-
> > >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 +-
> > >  include/drm/display/drm_dp_mst_helper.h       |  3 ++-
> > >  5 files changed, 19 insertions(+), 14 deletions(-)
> > > 
> > > 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 6994c9a1ed858..fed4ce6821161 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
> > > @@ -179,7 +179,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
> > >  	if (enable)
> > >  		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
> > >  	else
> > > -		drm_dp_remove_payload(mst_mgr, mst_state, payload);
> > > +		drm_dp_remove_payload(mst_mgr, mst_state, payload, 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
> > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > index 5861b0a6247bc..ebf6e31e156e0 100644
> > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > @@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> > >   * drm_dp_remove_payload() - Remove an MST payload
> > >   * @mgr: Manager to use.
> > >   * @mst_state: The MST atomic state
> > > - * @payload: The payload to write
> > > + * @old_payload: The payload with its old state
> > > + * @new_payload: The payload to write
> > >   *
> > >   * Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
> > >   * the starting time slots of all other payloads which would have been shifted towards the start of
> > > @@ -3350,33 +3351,34 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> > >   */
> > >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> > >  			   struct drm_dp_mst_topology_state *mst_state,
> > > -			   struct drm_dp_mst_atomic_payload *payload)
> > > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > > +			   struct drm_dp_mst_atomic_payload *new_payload)
> > >  {
> > >  	struct drm_dp_mst_atomic_payload *pos;
> > >  	bool send_remove = false;
> > >  
> > >  	/* We failed to make the payload, so nothing to do */
> > > -	if (payload->vc_start_slot == -1)
> > > +	if (new_payload->vc_start_slot == -1)
> > >  		return;
> > 
> > So I take it the only reason we even have that is the copy being done in
> > drm_dp_mst_atomic_wait_for_dependencies()? I don't really understand
> > why any of that is being done tbh. If the new payload hasn't been
> > allocated yet then why can't its vc_start_slots just stay at -1
> > until that time?
> > 
> > This whole thing feels a bit weird since the payload table really isn't
> > your normal atomic state that is computed ahead of time. Instead it just
> > gets built up on as we go during the actual commit. So not really sure
> > why we're even tracking it in atomic state...
> > 
> > >  
> > >  	mutex_lock(&mgr->lock);
> > > -	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
> > > +	send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary);
> > >  	mutex_unlock(&mgr->lock);
> > >  
> > >  	if (send_remove)
> > > -		drm_dp_destroy_payload_step1(mgr, mst_state, payload);
> > > +		drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
> > >  	else
> > >  		drm_dbg_kms(mgr->dev, "Payload for VCPI %d not in topology, not sending remove\n",
> > > -			    payload->vcpi);
> > > +			    new_payload->vcpi);
> > >  
> > >  	list_for_each_entry(pos, &mst_state->payloads, next) {
> > > -		if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)
> > > -			pos->vc_start_slot -= payload->time_slots;
> > > +		if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
> > > +			pos->vc_start_slot -= old_payload->time_slots;
> > >  	}
> > > -	payload->vc_start_slot = -1;
> > > +	new_payload->vc_start_slot = -1;
> > >  
> > >  	mgr->payload_count--;
> > > -	mgr->next_start_slot -= payload->time_slots;
> > > +	mgr->next_start_slot -= old_payload->time_slots;
> > >  }
> > >  EXPORT_SYMBOL(drm_dp_remove_payload);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > index ba29c294b7c1b..5f7bcb5c14847 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> > >  		to_intel_connector(old_conn_state->connector);
> > >  	struct drm_dp_mst_topology_state *mst_state =
> > >  		drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> > > +	struct drm_dp_mst_atomic_payload *payload =
> > > +		drm_atomic_get_mst_payload_state(mst_state, connector->port);
> > >  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > >  
> > >  	drm_dbg_kms(&i915->drm, "active links %d\n",
> > > @@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> > >  	intel_hdcp_disable(intel_mst->connector);
> > >  
> > >  	drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
> > > -			      drm_atomic_get_mst_payload_state(mst_state, connector->port));
> > > +			      payload, payload);
> > >  
> > >  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
> > >  }
> > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > index edcb2529b4025..ed9d374147b8d 100644
> > > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > @@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state,
> > >  
> > >  	// TODO: Figure out if we want to do a better job of handling VCPI allocation failures here?
> > >  	if (msto->disabled) {
> > > -		drm_dp_remove_payload(mgr, mst_state, payload);
> > > +		drm_dp_remove_payload(mgr, mst_state, payload, payload);
> > >  
> > >  		nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0);
> > >  	} else {
> > > diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> > > index 41fd8352ab656..f5eb9aa152b14 100644
> > > --- a/include/drm/display/drm_dp_mst_helper.h
> > > +++ b/include/drm/display/drm_dp_mst_helper.h
> > > @@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
> > >  			     struct drm_dp_mst_atomic_payload *payload);
> > >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> > >  			   struct drm_dp_mst_topology_state *mst_state,
> > > -			   struct drm_dp_mst_atomic_payload *payload);
> > > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > > +			   struct drm_dp_mst_atomic_payload *new_payload);
> > >  
> > >  int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
> > >  
> > > -- 
> > > 2.37.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Ville Syrjälä
> Intel

[PATCH 3/9] drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()

[Imre Deak]

Add a function to get the old MST topology state, required by a
follow-up i915 patch.

While at it clarify the code comment of
drm_atomic_get_new_mst_topology_state().

Cc: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org # 6.1
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 29 +++++++++++++++++--
 include/drm/display/drm_dp_mst_helper.h       |  3 ++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index ebf6e31e156e0..81cc0c3b1e000 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -5362,18 +5362,43 @@ struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_a
 }
 EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
 
+/**
+ * drm_atomic_get_old_mst_topology_state: get old MST topology state in atomic state, if any
+ * @state: global atomic state
+ * @mgr: MST topology manager, also the private object in this case
+ *
+ * This function wraps drm_atomic_get_old_private_obj_state() passing in the MST atomic
+ * state vtable so that the private object state returned is that of a MST
+ * topology object.
+ *
+ * Returns:
+ *
+ * The old MST topology state, or NULL if there's no topology state for this MST mgr
+ * in the global atomic state
+ */
+struct drm_dp_mst_topology_state *
+drm_atomic_get_old_mst_topology_state(struct drm_atomic_state *state,
+				      struct drm_dp_mst_topology_mgr *mgr)
+{
+	struct drm_private_state *priv_state =
+		drm_atomic_get_old_private_obj_state(state, &mgr->base);
+
+	return priv_state ? to_dp_mst_topology_state(priv_state) : NULL;
+}
+EXPORT_SYMBOL(drm_atomic_get_old_mst_topology_state);
+
 /**
  * drm_atomic_get_new_mst_topology_state: get new MST topology state in atomic state, if any
  * @state: global atomic state
  * @mgr: MST topology manager, also the private object in this case
  *
- * This function wraps drm_atomic_get_priv_obj_state() passing in the MST atomic
+ * This function wraps drm_atomic_get_new_private_obj_state() passing in the MST atomic
  * state vtable so that the private object state returned is that of a MST
  * topology object.
  *
  * Returns:
  *
- * The MST topology state, or NULL if there's no topology state for this MST mgr
+ * The new MST topology state, or NULL if there's no topology state for this MST mgr
  * in the global atomic state
  */
 struct drm_dp_mst_topology_state *
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index f5eb9aa152b14..32c764fb9cb56 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -868,6 +868,9 @@ struct drm_dp_mst_topology_state *
 drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
 				  struct drm_dp_mst_topology_mgr *mgr);
 struct drm_dp_mst_topology_state *
+drm_atomic_get_old_mst_topology_state(struct drm_atomic_state *state,
+				      struct drm_dp_mst_topology_mgr *mgr);
+struct drm_dp_mst_topology_state *
 drm_atomic_get_new_mst_topology_state(struct drm_atomic_state *state,
 				      struct drm_dp_mst_topology_mgr *mgr);
 struct drm_dp_mst_atomic_payload *
-- 
2.37.1

Re: [PATCH 3/9] drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()

[Ville Syrjälä]

On Wed, Jan 25, 2023 at 01:48:46PM +0200, Imre Deak wrote:
> Add a function to get the old MST topology state, required by a
> follow-up i915 patch.
> 
> While at it clarify the code comment of
> drm_atomic_get_new_mst_topology_state().
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org # 6.1
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 29 +++++++++++++++++--
>  include/drm/display/drm_dp_mst_helper.h       |  3 ++
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index ebf6e31e156e0..81cc0c3b1e000 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -5362,18 +5362,43 @@ struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_a
>  }
>  EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
>  
> +/**
> + * drm_atomic_get_old_mst_topology_state: get old MST topology state in atomic state, if any
> + * @state: global atomic state
> + * @mgr: MST topology manager, also the private object in this case
> + *
> + * This function wraps drm_atomic_get_old_private_obj_state() passing in the MST atomic
> + * state vtable so that the private object state returned is that of a MST
> + * topology object.
> + *
> + * Returns:
> + *
> + * The old MST topology state, or NULL if there's no topology state for this MST mgr
> + * in the global atomic state
> + */
> +struct drm_dp_mst_topology_state *
> +drm_atomic_get_old_mst_topology_state(struct drm_atomic_state *state,
> +				      struct drm_dp_mst_topology_mgr *mgr)
> +{
> +	struct drm_private_state *priv_state =

I would include 'old_' in the variable name to remind the reader what it
is.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		drm_atomic_get_old_private_obj_state(state, &mgr->base);
> +
> +	return priv_state ? to_dp_mst_topology_state(priv_state) : NULL;
> +}
> +EXPORT_SYMBOL(drm_atomic_get_old_mst_topology_state);
> +
>  /**
>   * drm_atomic_get_new_mst_topology_state: get new MST topology state in atomic state, if any
>   * @state: global atomic state
>   * @mgr: MST topology manager, also the private object in this case
>   *
> - * This function wraps drm_atomic_get_priv_obj_state() passing in the MST atomic
> + * This function wraps drm_atomic_get_new_private_obj_state() passing in the MST atomic
>   * state vtable so that the private object state returned is that of a MST
>   * topology object.
>   *
>   * Returns:
>   *
> - * The MST topology state, or NULL if there's no topology state for this MST mgr
> + * The new MST topology state, or NULL if there's no topology state for this MST mgr
>   * in the global atomic state
>   */
>  struct drm_dp_mst_topology_state *
> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> index f5eb9aa152b14..32c764fb9cb56 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -868,6 +868,9 @@ struct drm_dp_mst_topology_state *
>  drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
>  				  struct drm_dp_mst_topology_mgr *mgr);
>  struct drm_dp_mst_topology_state *
> +drm_atomic_get_old_mst_topology_state(struct drm_atomic_state *state,
> +				      struct drm_dp_mst_topology_mgr *mgr);
> +struct drm_dp_mst_topology_state *
>  drm_atomic_get_new_mst_topology_state(struct drm_atomic_state *state,
>  				      struct drm_dp_mst_topology_mgr *mgr);
>  struct drm_dp_mst_atomic_payload *
> -- 
> 2.37.1

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/9] drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()

[Imre Deak]

On Thu, Jan 26, 2023 at 08:36:20PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 25, 2023 at 01:48:46PM +0200, Imre Deak wrote:
> > Add a function to get the old MST topology state, required by a
> > follow-up i915 patch.
> > 
> > While at it clarify the code comment of
> > drm_atomic_get_new_mst_topology_state().
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: stable@vger.kernel.org # 6.1
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 29 +++++++++++++++++--
> >  include/drm/display/drm_dp_mst_helper.h       |  3 ++
> >  2 files changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index ebf6e31e156e0..81cc0c3b1e000 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -5362,18 +5362,43 @@ struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_a
> >  }
> >  EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
> >  
> > +/**
> > + * drm_atomic_get_old_mst_topology_state: get old MST topology state in atomic state, if any
> > + * @state: global atomic state
> > + * @mgr: MST topology manager, also the private object in this case
> > + *
> > + * This function wraps drm_atomic_get_old_private_obj_state() passing in the MST atomic
> > + * state vtable so that the private object state returned is that of a MST
> > + * topology object.
> > + *
> > + * Returns:
> > + *
> > + * The old MST topology state, or NULL if there's no topology state for this MST mgr
> > + * in the global atomic state
> > + */
> > +struct drm_dp_mst_topology_state *
> > +drm_atomic_get_old_mst_topology_state(struct drm_atomic_state *state,
> > +				      struct drm_dp_mst_topology_mgr *mgr)
> > +{
> > +	struct drm_private_state *priv_state =
> 
> I would include 'old_' in the variable name to remind the reader what it
> is.

Ok, will change it.

> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +		drm_atomic_get_old_private_obj_state(state, &mgr->base);
> > +
> > +	return priv_state ? to_dp_mst_topology_state(priv_state) : NULL;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_get_old_mst_topology_state);
> > +
> >  /**
> >   * drm_atomic_get_new_mst_topology_state: get new MST topology state in atomic state, if any
> >   * @state: global atomic state
> >   * @mgr: MST topology manager, also the private object in this case
> >   *
> > - * This function wraps drm_atomic_get_priv_obj_state() passing in the MST atomic
> > + * This function wraps drm_atomic_get_new_private_obj_state() passing in the MST atomic
> >   * state vtable so that the private object state returned is that of a MST
> >   * topology object.
> >   *
> >   * Returns:
> >   *
> > - * The MST topology state, or NULL if there's no topology state for this MST mgr
> > + * The new MST topology state, or NULL if there's no topology state for this MST mgr
> >   * in the global atomic state
> >   */
> >  struct drm_dp_mst_topology_state *
> > diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> > index f5eb9aa152b14..32c764fb9cb56 100644
> > --- a/include/drm/display/drm_dp_mst_helper.h
> > +++ b/include/drm/display/drm_dp_mst_helper.h
> > @@ -868,6 +868,9 @@ struct drm_dp_mst_topology_state *
> >  drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> >  				  struct drm_dp_mst_topology_mgr *mgr);
> >  struct drm_dp_mst_topology_state *
> > +drm_atomic_get_old_mst_topology_state(struct drm_atomic_state *state,
> > +				      struct drm_dp_mst_topology_mgr *mgr);
> > +struct drm_dp_mst_topology_state *
> >  drm_atomic_get_new_mst_topology_state(struct drm_atomic_state *state,
> >  				      struct drm_dp_mst_topology_mgr *mgr);
> >  struct drm_dp_mst_atomic_payload *
> > -- 
> > 2.37.1
> 
> -- 
> Ville Syrjälä
> Intel

[PATCH 5/9] drm/display/dp_mst: Fix the payload VCPI check in drm_dp_mst_dump_topology()

[Imre Deak]

Fix an off-by-one error in the VCPI check in drm_dp_mst_dump_topology().

Cc: Lyude Paul <lyude@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 81cc0c3b1e000..619f616d69e20 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4770,7 +4770,7 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
 		list_for_each_entry(payload, &state->payloads, next) {
 			char name[14];
 
-			if (payload->vcpi != i || payload->delete)
+			if (payload->vcpi != i + 1 || payload->delete)
 				continue;
 
 			fetch_monitor_name(mgr, payload->port, name, sizeof(name));
-- 
2.37.1

Re: [PATCH 5/9] drm/display/dp_mst: Fix the payload VCPI check in drm_dp_mst_dump_topology()

[Ville Syrjälä]

On Wed, Jan 25, 2023 at 01:48:48PM +0200, Imre Deak wrote:
> Fix an off-by-one error in the VCPI check in drm_dp_mst_dump_topology().
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 81cc0c3b1e000..619f616d69e20 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4770,7 +4770,7 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
>  		list_for_each_entry(payload, &state->payloads, next) {
>  			char name[14];
>  
> -			if (payload->vcpi != i || payload->delete)
> +			if (payload->vcpi != i + 1 || payload->delete)

Why does this code even do that funny nested double loop?

>  				continue;
>  
>  			fetch_monitor_name(mgr, payload->port, name, sizeof(name));
> -- 
> 2.37.1

-- 
Ville Syrjälä
Intel

Re: [PATCH 5/9] drm/display/dp_mst: Fix the payload VCPI check in drm_dp_mst_dump_topology()

[Imre Deak]

On Fri, Jan 27, 2023 at 09:42:39PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 25, 2023 at 01:48:48PM +0200, Imre Deak wrote:
> > Fix an off-by-one error in the VCPI check in drm_dp_mst_dump_topology().
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 81cc0c3b1e000..619f616d69e20 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4770,7 +4770,7 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
> >  		list_for_each_entry(payload, &state->payloads, next) {
> >  			char name[14];
> >  
> > -			if (payload->vcpi != i || payload->delete)
> > +			if (payload->vcpi != i + 1 || payload->delete)
> 
> Why does this code even do that funny nested double loop?

The payload list is not ordered by VCPIs I think, but the printout wants
to list them in VCPI order.

> 
> >  				continue;
> >  
> >  			fetch_monitor_name(mgr, payload->port, name, sizeof(name));
> > -- 
> > 2.37.1
> 
> -- 
> Ville Syrjälä
> Intel

[PATCH 8/9] drm/display/dp_mst: Add a helper to verify the MST payload state

[Imre Deak]

Add a function drivers can use to verify the MST payload state tracking
and compare this to the sink's payload table.

Cc: Lyude Paul <lyude@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 169 ++++++++++++++++++
 include/drm/display/drm_dp.h                  |   3 +
 include/drm/display/drm_dp_mst_helper.h       |   3 +
 3 files changed, 175 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 619f616d69e20..7597d27db4fa6 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4834,6 +4834,175 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
 }
 EXPORT_SYMBOL(drm_dp_mst_dump_topology);
 
+static bool verify_mst_payload_state(struct drm_dp_mst_topology_state *mst_state)
+{
+	struct drm_dp_mst_topology_mgr *mgr = mst_state->mgr;
+	struct drm_dp_mst_atomic_payload *payload;
+	int payload_count = 0;
+	u64 time_slot_mask = 0;
+	u32 vcpi_mask = 0;
+	int last_set;
+
+	if (BITS_PER_TYPE(time_slot_mask) < mst_state->total_avail_slots)
+		return false;
+
+	list_for_each_entry(payload, &mst_state->payloads, next) {
+		u64 mask;
+
+		if (payload->vc_start_slot == -1)
+			continue;
+
+		if (!payload->time_slots)
+			return false;
+
+		if (payload->vc_start_slot < mst_state->start_slot)
+			return false;
+
+		if (payload->vc_start_slot + payload->time_slots - mst_state->start_slot >
+		    mst_state->total_avail_slots)
+			return false;
+
+		mask = GENMASK_ULL(payload->vc_start_slot + payload->time_slots - 1,
+				   payload->vc_start_slot);
+
+		if (time_slot_mask & mask)
+			return false;
+
+		time_slot_mask |= mask;
+
+		if (payload->vcpi < 1 ||
+		    payload->vcpi & ~DP_PAYLOAD_ID_MASK ||
+		    payload->vcpi > BITS_PER_TYPE(vcpi_mask))
+			return false;
+		if (BIT(payload->vcpi - 1) & vcpi_mask)
+			return false;
+		vcpi_mask |= BIT(payload->vcpi - 1);
+
+		payload_count++;
+	}
+
+	if (payload_count != mgr->payload_count)
+		return false;
+
+	last_set = fls64(time_slot_mask);
+
+	if (last_set &&
+	    GENMASK_ULL(last_set - 1, mst_state->start_slot) != time_slot_mask)
+		return false;
+
+	if (max(mst_state->start_slot, mgr->next_start_slot) !=
+	    max_t(int, mst_state->start_slot, last_set))
+		return false;
+
+	return true;
+}
+
+static int get_payload_table_vcpi(const u8 *table, int slot)
+{
+	if (slot == 0)
+		return FIELD_GET(DP_PAYLOAD_ID_SLOT0_5_0_MASK, table[0]) |
+		       (FIELD_GET(DP_PAYLOAD_ID_SLOT0_6, table[1]) << 6);
+	else
+		return FIELD_GET(DP_PAYLOAD_ID_MASK, table[slot]);
+}
+
+static bool verify_mst_payload_table(struct drm_dp_mst_topology_state *mst_state,
+				     const u8 *payload_table)
+{
+	struct drm_dp_mst_topology_mgr *mgr = mst_state->mgr;
+	struct drm_dp_mst_atomic_payload *payload;
+	int i;
+
+	list_for_each_entry(payload, &mst_state->payloads, next) {
+		if (payload->vc_start_slot == -1)
+			continue;
+
+		if (payload->vc_start_slot + payload->time_slots > DP_PAYLOAD_TABLE_SIZE)
+			return false;
+
+		for (i = 0; i < payload->time_slots; i++)
+			if (get_payload_table_vcpi(payload_table,
+						   payload->vc_start_slot + i) != payload->vcpi)
+				return false;
+	}
+
+	for (i = max(mgr->next_start_slot, mst_state->start_slot);
+	     i < DP_PAYLOAD_TABLE_SIZE;
+	     i++) {
+		if (get_payload_table_vcpi(payload_table, i) != 0)
+			return false;
+	}
+
+	return true;
+}
+
+static void print_mst_payload_state(struct drm_dp_mst_topology_mgr *mgr,
+				    struct drm_dp_mst_topology_state *mst_state,
+				    const u8 *payload_table)
+{
+	struct drm_dp_mst_atomic_payload *payload;
+	int i = 0;
+
+	drm_dbg(mgr->dev,
+		"Payload state: start_slot %d total_avail_slots %d next_start_slot %d payload_count %d\n",
+		mst_state->start_slot, mst_state->total_avail_slots,
+		mgr->next_start_slot, mgr->payload_count);
+
+	list_for_each_entry(payload, &mst_state->payloads, next) {
+		drm_dbg(mgr->dev,
+			"  Payload#%d: port %p VCPI %d delete %d vc_start_slot %d time_slots %d\n",
+			i, payload->port, payload->vcpi,
+			payload->delete, payload->vc_start_slot, payload->time_slots);
+		i++;
+	}
+
+	if (!payload_table)
+		return;
+
+	drm_dbg(mgr->dev, "Payload table:\n");
+	print_hex_dump(KERN_DEBUG, "  Ptbl ",
+		       DUMP_PREFIX_OFFSET, 16, 1,
+		       payload_table, DP_PAYLOAD_TABLE_SIZE, false);
+}
+
+/**
+ * drm_dp_mst_verify_payload_state - Verify the atomic state for payloads and the related sink payload table
+ * @state: atomic state
+ * @mgr: manager to verify the state for
+ * @verify_sink: %true if the sink payload table needs to be verified as well
+ *
+ * Verify @mgr's atomic state tracking all its payloads and optionally the
+ * related sink payload table.
+ */
+void drm_dp_mst_verify_payload_state(struct drm_atomic_state *state,
+				     struct drm_dp_mst_topology_mgr *mgr,
+				     bool verify_sink)
+{
+	struct drm_dp_mst_topology_state *mst_state;
+	u8 payload_table[DP_PAYLOAD_TABLE_SIZE];
+
+	mst_state = drm_atomic_get_new_mst_topology_state(state, mgr);
+	if (drm_WARN_ON(mgr->dev, !mst_state))
+		return;
+
+	if (drm_WARN_ON(mgr->dev, !verify_mst_payload_state(mst_state))) {
+		print_mst_payload_state(mgr, mst_state, NULL);
+		return;
+	}
+
+	if (!verify_sink)
+		return;
+
+	if (!dump_dp_payload_table(mgr, payload_table))
+		return;
+
+	if (!verify_mst_payload_table(mst_state, payload_table)) {
+		drm_err(mgr->dev, "MST payload state mismatches payload table\n");
+		print_mst_payload_state(mgr, mst_state, payload_table);
+	}
+}
+EXPORT_SYMBOL(drm_dp_mst_verify_payload_state);
+
 static void drm_dp_tx_work(struct work_struct *work)
 {
 	struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, tx_work);
diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
index 632376c291db6..bcc5183188a68 100644
--- a/include/drm/display/drm_dp.h
+++ b/include/drm/display/drm_dp.h
@@ -925,9 +925,12 @@
 #define DP_PAYLOAD_TABLE_UPDATE_STATUS      0x2c0   /* 1.2 MST */
 # define DP_PAYLOAD_TABLE_UPDATED           (1 << 0)
 # define DP_PAYLOAD_ACT_HANDLED             (1 << 1)
+# define DP_PAYLOAD_ID_SLOT0_5_0_MASK	    (0x3f << 2)
 
 #define DP_VC_PAYLOAD_ID_SLOT_1             0x2c1   /* 1.2 MST */
 /* up to ID_SLOT_63 at 0x2ff */
+# define DP_PAYLOAD_ID_SLOT0_6		    (1 << 7)
+# define DP_PAYLOAD_ID_MASK		    0x7f
 
 /* Source Device-specific */
 #define DP_SOURCE_OUI			    0x300
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 32c764fb9cb56..44c6710ebf315 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -848,6 +848,9 @@ int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
 
 void drm_dp_mst_dump_topology(struct seq_file *m,
 			      struct drm_dp_mst_topology_mgr *mgr);
+void drm_dp_mst_verify_payload_state(struct drm_atomic_state *state,
+				     struct drm_dp_mst_topology_mgr *mgr,
+				     bool verify_sink);
 
 void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
 int __must_check
-- 
2.37.1