Re: [Intel-gfx] [PATCH 4/9] drm/i915/dp_mst: Fix payload removal during output disabling

[Imre Deak <imre.deak@intel.com>]

On Thu, Jan 26, 2023 at 08:38:16PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 25, 2023 at 01:48:47PM +0200, Imre Deak wrote:
> > Use the correct old/new topology and payload states in
> > intel_mst_disable_dp(). So far drm_atomic_get_mst_topology_state() it
> > used returned either the old state, in case the state was added already
> > earlier during the atomic check phase or otherwise the new state (but
> > the latter could fail, which can't be handled in the enable/disable
> > hooks). After the first patch in the patchset, the state should always
> > get added already during the check phase, so here we can get the
> > old/new states without a failure.
> > 
> > drm_dp_remove_payload() should use time_slots from the old payload state
> > and vc_start_slot in the new one. It should update the new payload
> > states to reflect the sink's current payload table after the payload is
> > removed. Pass the new topology state and the old and new payload states
> > accordingly.
> > 
> > This also fixes a problem where the payload allocations for multiple MST
> > streams on the same link got inconsistent after a few commits, as
> > during payload removal the old instead of the new payload state got
> > updated, so the subsequent enabling sequence and commits used a stale
> > payload state.
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: stable@vger.kernel.org # 6.1
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 5f7bcb5c14847..800fa12a61d93 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -524,10 +524,14 @@ static void intel_mst_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 *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_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);
> > +	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);
> 
> old states could be const no?

Yes, will change this.

> >  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> >  
> >  	drm_dbg_kms(&i915->drm, "active links %d\n",
> > @@ -535,8 +539,8 @@ 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,
> > -			      payload, payload);
> > +	drm_dp_remove_payload(&intel_dp->mst_mgr, new_mst_state,
> 
> Right that one needs to be 'new' to update the start_slots
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +			      old_payload, new_payload);
> >  
> >  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
> >  }
> > -- 
> > 2.37.1
> 
> -- 
> Ville Syrjälä
> Intel