> Subject: Re: [PATCH] drm/display/dp_mst: Add protection against 0 vcpi > > On Mon, Nov 17, 2025 at 07:09:38AM +0200, Suraj Kandpal wrote: > > > -----Original Message----- > > > From: Jani Nikula <[email protected]> > > > Sent: Thursday, November 13, 2025 9:55 PM > > > To: Deak, Imre <[email protected]>; Kandpal, Suraj > > > <[email protected]> > > > Cc: [email protected]; [email protected]; > > > intel- [email protected]; Nautiyal, Ankit K > > > <[email protected]>; Murthy, Arun R > > > <[email protected]> > > > Subject: Re: [PATCH] drm/display/dp_mst: Add protection against 0 > > > vcpi > > > > > > On Thu, 13 Nov 2025, Imre Deak <[email protected]> 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 <[email protected]> > > > >> --- > > > >> 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.
Hmm then in that case should we just return 0 early if vcpi turns out to be 0 here. Regards, Suraj Kandpal > > Regards > > Suraj Kandpal > > > > > BR, > > > Jani. > > > > > > > > > > > > > >> } > > > >> > > > >> return 0; > > > >> -- > > > >> 2.34.1 > > > >> > > > > > > -- > > > Jani Nikula, Intel
