> -----Original Message-----
> From: Deak, Imre <[email protected]>
> Sent: Friday, March 20, 2026 8:26 PM
> To: Shankar, Uma <[email protected]>
> Cc: [email protected]; [email protected]; Ville
> Syrjälä
> <[email protected]>; [email protected]
> Subject: Re: [PATCH] drm/i915/dp_tunnel: Fix error handling when clearing
> stream
> BW in atomic state
>
> On Fri, Mar 20, 2026 at 01:42:58PM +0200, Shankar, Uma wrote:
> >
> >
> > > -----Original Message-----
> > > From: Deak, Imre <[email protected]>
> > > Sent: Friday, March 20, 2026 2:59 PM
> > > To: [email protected]; [email protected]
> > > Cc: Shankar, Uma <[email protected]>; Ville Syrjälä
> > > <[email protected]>; [email protected]
> > > 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 <[email protected]>
> > > Cc: Ville Syrjälä <[email protected]>
> > > Cc: <[email protected]> # v6.9+
> > > Fixes: a4efae87ecb2 ("drm/i915/dp: Compute DP tunnel BW during
> > > encoder state
> > > computation")
> > > Signed-off-by: Imre Deak <[email protected]>
> > > ---
> > > 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.
Thanks for the explanation Imre, sounds good.
Reviewed-by: Uma Shankar <[email protected]>
> >
> > 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
> >
