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

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

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.

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