Re: [PATCH] drm/i915/dp_tunnel: Fix error handling when clearing stream BW in atomic state

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

On Fri, Mar 20, 2026 at 01:42:58PM +0200, Shankar, Uma wrote:
> 
> 
> > -----Original Message-----
> > From: Deak, Imre <imre.deak@intel.com>
> > Sent: Friday, March 20, 2026 2:59 PM
> > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > Cc: Shankar, Uma <uma.shankar@intel.com>; Ville Syrjälä
> > <ville.syrjala@linux.intel.com>; stable@vger.kernel.org
> > Subject: [PATCH] drm/i915/dp_tunnel: Fix error handling when clearing stream BW
> > in atomic state
> > 
> > Clearing the DP tunnel stream BW in the atomic state involves getting the tunnel
> > group state, which can fail. Handle the error accordingly.
> > 
> > This fixes at least one issue where drm_dp_tunnel_atomic_set_stream_bw()
> > failed to get the tunnel group state returning -EDEADLK, which wasn't handled.
> > This lead to the ctx->contended warn later in modeset_lock() while taking a WW
> > mutex for another object in the same atomic state, and thus within the same
> > already contended WW context.
> > 
> > Moving intel_crtc_state_alloc() later would avoid freeing saved_state on the error
> > path; this stable patch leaves that simplification for a follow-up.
> > 
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v6.9+
> > Fixes: a4efae87ecb2 ("drm/i915/dp: Compute DP tunnel BW during encoder state
> > computation")
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c  |  8 +++++++-
> >  .../gpu/drm/i915/display/intel_dp_tunnel.c    | 20 +++++++++++++------
> >  .../gpu/drm/i915/display/intel_dp_tunnel.h    | 11 ++++++----
> >  3 files changed, 28 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index ee501009a251f..882db77c0bbcd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4640,6 +4640,7 @@ intel_crtc_prepare_cleared_state(struct
> > intel_atomic_state *state,
> >  	struct intel_crtc_state *crtc_state =
> >  		intel_atomic_get_new_crtc_state(state, crtc);
> >  	struct intel_crtc_state *saved_state;
> > +	int err;
> > 
> >  	saved_state = intel_crtc_state_alloc(crtc);
> >  	if (!saved_state)
> > @@ -4648,7 +4649,12 @@ intel_crtc_prepare_cleared_state(struct
> > intel_atomic_state *state,
> >  	/* free the old crtc_state->hw members */
> >  	intel_crtc_free_hw_state(crtc_state);
> > 
> > -	intel_dp_tunnel_atomic_clear_stream_bw(state, crtc_state);
> > +	err = intel_dp_tunnel_atomic_clear_stream_bw(state, crtc_state);
> > +	if (err) {
> > +		kfree(saved_state);
> > +
> > +		return err;
> > +	}
> > 
> >  	/* FIXME: before the switch to atomic started, a new pipe_config was
> >  	 * kzalloc'd. Code that depends on any field being zero should be diff --git
> > a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> > b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> > index 1fd1ac8d556d8..7363c98172971 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> > @@ -659,19 +659,27 @@ int intel_dp_tunnel_atomic_compute_stream_bw(struct
> > intel_atomic_state *state,
> >   *
> >   * Clear any DP tunnel stream BW requirement set by
> >   * intel_dp_tunnel_atomic_compute_stream_bw().
> > + *
> > + * Returns 0 in case of success, a negative error code otherwise.
> >   */
> > -void intel_dp_tunnel_atomic_clear_stream_bw(struct intel_atomic_state *state,
> > -					    struct intel_crtc_state *crtc_state)
> > +int intel_dp_tunnel_atomic_clear_stream_bw(struct intel_atomic_state *state,
> > +					   struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	int err;
> > 
> >  	if (!crtc_state->dp_tunnel_ref.tunnel)
> > -		return;
> > +		return 0;
> > +
> > +	err = drm_dp_tunnel_atomic_set_stream_bw(&state->base,
> > +						 crtc_state->dp_tunnel_ref.tunnel,
> > +						 crtc->pipe, 0);
> > +	if (err)
> > +		return err;
> > 
> > -	drm_dp_tunnel_atomic_set_stream_bw(&state->base,
> > -					   crtc_state->dp_tunnel_ref.tunnel,
> > -					   crtc->pipe, 0);
> >  	drm_dp_tunnel_ref_put(&crtc_state->dp_tunnel_ref);
> 
> Hi Imre,
> Should we not drop reference even in case of failure, is this intentional ?

Yes, the early return in case of an error, preserving the tunnel reference
in the crtc state is intentional. The error here will make the whole
commit fail and the atomic state - within that the crtc state - being
freed. That crtc state freeing will drop this reference, see
intel_crtc_destroy_state().

Aside: it wouldn't cause a functional problem to drop the reference as
you suggest in case of the earlier error either - the related dropping
of the reference in intel_crtc_destroy_state() described above would be
skipped then. But I still think the usual early return - as done in the
patch - in case of an error is the logically correct way.

> 
> Regards,
> Uma Shankar
> 
> > +
> > +	return 0;
> >  }
> > 
> >  /**
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_tunnel.h
> > b/drivers/gpu/drm/i915/display/intel_dp_tunnel.h
> > index 7f0f720e8dcad..10ab9eebcef69 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_tunnel.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_tunnel.h
> > @@ -40,8 +40,8 @@ int intel_dp_tunnel_atomic_compute_stream_bw(struct
> > intel_atomic_state *state,
> >  					     struct intel_dp *intel_dp,
> >  					     const struct intel_connector
> > *connector,
> >  					     struct intel_crtc_state *crtc_state); -
> > void intel_dp_tunnel_atomic_clear_stream_bw(struct intel_atomic_state *state,
> > -					    struct intel_crtc_state *crtc_state);
> > +int intel_dp_tunnel_atomic_clear_stream_bw(struct intel_atomic_state *state,
> > +					   struct intel_crtc_state *crtc_state);
> > 
> >  int intel_dp_tunnel_atomic_add_state_for_crtc(struct intel_atomic_state *state,
> >  					      struct intel_crtc *crtc);
> > @@ -88,9 +88,12 @@ intel_dp_tunnel_atomic_compute_stream_bw(struct
> > intel_atomic_state *state,
> >  	return 0;
> >  }
> > 
> > -static inline void
> > +static inline int
> >  intel_dp_tunnel_atomic_clear_stream_bw(struct intel_atomic_state *state,
> > -				       struct intel_crtc_state *crtc_state) {}
> > +				       struct intel_crtc_state *crtc_state) {
> > +	return 0;
> > +}
> > 
> >  static inline int
> >  intel_dp_tunnel_atomic_add_state_for_crtc(struct intel_atomic_state *state,
> > --
> > 2.49.1
>