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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org
Subject: Re: [Intel-gfx] [PATCH 4/9] drm/i915/dp_mst: Fix payload removal during output disabling
Date: Thu, 26 Jan 2023 20:38:16 +0200	[thread overview]
Message-ID: <Y9LImOVP9e4tkq9j@intel.com> (raw)
In-Reply-To: <20230125114852.748337-5-imre.deak@intel.com>

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?

>  	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