On 6/17/26 21:27, Leo Li wrote:
> On 2026-06-17 04:56, Michel Dänzer wrote:
>> On 6/17/26 10:07, Michel Dänzer wrote:
>>> On 6/16/26 22:18, [email protected] wrote:
>>>>
>>>> * Add a flip_programmed completion. Arm it (reinit_completion) under
>>>> event_lock together with prepare_flip_isr(), and signal it
>>>> (complete_all) right after update_planes_and_stream_adapter() programs
>>>> the flip. It starts in the "completed" state at crtc init.
>>>
>>> Is the completion really necessary? Wouldn't moving the acrtc->pflip_status
>>> = AMDGPU_FLIP_SUBMITTED assignment after the flip programming suffice?
>
> [...]
> >> Or even just moving the unlocking of event_lock after the flip programming.
>
> I initially thought about doing so. But the possibility of
> update_planes_and_stream_adapter() sleeping made me think otherwise.
That is a problem.
If the flip programming could be done while holding event_lock, this could be
made 100% reliable, avoiding the issue below.
> I suppose the worst case scenario with arming acrtc->event/pflip_status after
> programming is we deliver the event a frame later than it needs to be (which
> is also the case with the current patch), thus stalling the next commit via
> flip_done, and making userspace think it missed the programming deadline.
Yep. Hopefully won't happen too often in practice?
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> index 00f7a3b445ebf..571198c46c0c2 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -4384,17 +4384,17 @@ static void amdgpu_dm_commit_planes(struct
>>>> drm_atomic_state *state,
>>>> * from 0 -> n planes we have to skip a hardware generated event
>>>> * and rely on sending it from software.
>>>> */
>>>> + spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
>>>> if (acrtc_attach->base.state->event &&
>>>> acrtc_state->active_planes > 0) {
>>>> drm_crtc_vblank_get(pcrtc);
>>>>
>>>> - spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
>>>> -
>>>> WARN_ON(acrtc_attach->pflip_status != AMDGPU_FLIP_NONE);
>>>> + /* Arm flip completion handling and event delivery */
>>>> +
>>>> reinit_completion(&acrtc_attach->dm_irq_params.flip_programmed);
>>>> prepare_flip_isr(acrtc_attach);
>>>> -
>>>> - spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
>>>> }
>>>> + spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
>>>>
>>>> if (acrtc_state->stream) {
>>>> if (acrtc_state->freesync_vrr_info_changed)
>>>
>>> Pulling event_lock out of the if block doesn't make any difference (other
>>> than locking it unnecessarily when the block isn't entered 🙂, does it?
>
> FWIU the crtc_state->event pointer itself should be guarded under event_lock,
> since it can be NULL'd concurrently.
Gotcha, I was fooled by the acrtc_attach->base.state->event disguise. :)
--
Earthling Michel Dänzer \ GNOME / Xwayland / Mesa developer
https://redhat.com \ Libre software enthusiast
