Re: [PATCH] drm/display/dp_mst: Add protection against 0 vcpi

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

On Mon, Nov 17, 2025 at 12:19:18PM +0200, Jani Nikula wrote:
> On Mon, 17 Nov 2025, Imre Deak <imre.deak@intel.com> wrote:
> > On Mon, Nov 17, 2025 at 07:09:38AM +0200, Suraj Kandpal wrote:
> >> > -----Original Message-----
> >> > From: Jani Nikula <jani.nikula@linux.intel.com>
> >> > Sent: Thursday, November 13, 2025 9:55 PM
> >> > To: Deak, Imre <imre.deak@intel.com>; Kandpal, Suraj
> >> > <suraj.kandpal@intel.com>
> >> > Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> >> > gfx@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> >> > Murthy, Arun R <arun.r.murthy@intel.com>
> >> > Subject: Re: [PATCH] drm/display/dp_mst: Add protection against 0 vcpi
> >> > 
> >> > On Thu, 13 Nov 2025, Imre Deak <imre.deak@intel.com> wrote:
> >> > > On Thu, Nov 13, 2025 at 10:09:19AM +0530, Suraj Kandpal wrote:
> >> > >> When releasing a timeslot there is a slight chance we may end up with
> >> > >> the wrong payload mask due to overflow if the delayed_destroy_work
> >> > >> ends up coming into play after a DP 2.1 monitor gets disconnected
> >> > >> which causes vcpi to become 0 then we try to make the payload =
> >> > >> ~BIT(vcpi - 1) which is a negative shift.
> >> > >>
> >> > >> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >> > >> ---
> >> > >>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 4 +++-
> >> > >>  1 file changed, 3 insertions(+), 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 64e5c176d5cc..3cf1eafcfcb5 100644
> >> > >> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> >> > >> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> >> > >> @@ -4531,6 +4531,7 @@ int drm_dp_atomic_release_time_slots(struct
> >> > drm_atomic_state *state,
> >> > >>  	struct drm_dp_mst_atomic_payload *payload;
> >> > >>  	struct drm_connector_state *old_conn_state, *new_conn_state;
> >> > >>  	bool update_payload = true;
> >> > >> +	int bit;
> >> > >>
> >> > >>  	old_conn_state = drm_atomic_get_old_connector_state(state, port-
> >> > >connector);
> >> > >>  	if (!old_conn_state->crtc)
> >> > >> @@ -4572,7 +4573,8 @@ int drm_dp_atomic_release_time_slots(struct
> >> > drm_atomic_state *state,
> >> > >>  	if (!payload->delete) {
> >> > >>  		payload->pbn = 0;
> >> > >>  		payload->delete = true;
> >> > >> -		topology_state->payload_mask &= ~BIT(payload->vcpi - 1);
> >> > >> +		bit = payload->vcpi ? payload->vcpi - 1 : 0;
> >> > >> +		topology_state->payload_mask &= ~BIT(bit);
> >> > >
> >> > > This looks wrong, clearing the bit for an unrelated payload.
> >> > 
> >> > Agreed.
> >> > 
> >> > The logs have, among other things,
> >> > 
> >> > <7> [515.138211] xe 0000:03:00.0: [drm:intel_dp_sink_set_dsc_decompression
> >> > [xe]] Failed to enable sink decompression state
> >> > 
> >> > <7> [515.193484] xe 0000:03:00.0: [drm:drm_dp_add_payload_part1
> >> > [drm_display_helper]] VCPI 0 for port ffff888126ce9000 not in topology, not
> >> > creating a payload to remote
> >> > 
> >> > <7> [515.194671] xe 0000:03:00.0: [drm:drm_dp_add_payload_part2
> >> > [drm_display_helper]] Part 1 of payload creation for DP-5 failed, skipping part 2
> >> > 
> >> > <7> [515.347331] xe 0000:03:00.0: [drm:drm_dp_remove_payload_part1
> >> > [drm_display_helper]] Payload for VCPI 0 not in topology, not sending remove
> >> > 
> >> > So it's no wonder the port's not in topology and everything fails. We obviously
> >> > need to skip payload_mask updates when the VCPI is 0, but that's just a
> >> > symptom of other stuff going wrong first. Perhaps we could do with some
> >> > earlier error handling too?
> >> 
> >> Yes I agree the question is how high will the error handling needs to be added.
> >> A lot of weird things going on here.
> >>
> >> 1st one is how is it finding a payload which we do not create while we
> >> call destroy function
> >>
> >> 2nd how is VCPI with id 0 possible from what I see VCPI are 1 at least
> >> that's what I gather from drm_dp_mst_atomic_check_payload_alloc_limits.So what
> >> are we missing when we create a payload?
> >>
> >> Imre, Jani any idea still new to how payload creation work so am I
> >> missing something.
> >
> > A VCPI ID will be assigned to a payload during an atomic commit only if
> > the corresponding MST connector is still connected. If the MST connector
> > gets disconnected by the time of the atomic commit - as in the above
> > case - no VCPI ID will assigned and the allocation table in the branch
> > device cannot be updated either for the payload, as indicated by the
> > above payload creation/removal failed messages.
> >
> > I think the fix should be not to clear the VCPI ID if it's 0. Valid VCPI
> > IDs start from 1.
> 
> Agreed. As I said above, "We obviously need to skip payload_mask updates
> when the VCPI is 0".
> 
> But there are *also* a bunch of other things going wrong before that,
> but we plunge on. Should we do something about that?

Creating or removing a payload, which need to update the payload table
in the branch device can fail expectedly if the corresponding MST
connector is disconnected by the time of the atomic commit. This is
indicated by the above

VCPI 0 for port ffff888126ce9000 not in topology, not creating a payload to remote
Part 1 of payload creation for DP-5 failed, skipping part 2
Payload for VCPI 0 not in topology, not sending remove

debug messages. The commit must still continue and complete since the
user should be able to disable each MST connector in a topology one
MST connector at a time.

> BR,
> Jani.
> 
> 
> 
> 
> >
> >> Regards
> >> Suraj Kandpal 
> >> 
> >> > BR,
> >> > Jani.
> >> > 
> >> > 
> >> > >
> >> > >>  	}
> >> > >>
> >> > >>  	return 0;
> >> > >> --
> >> > >> 2.34.1
> >> > >>
> >> > 
> >> > --
> >> > Jani Nikula, Intel
> 
> -- 
> Jani Nikula, Intel