Hi Michel, thanks for the review!
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?
I think this would create a window between HW programming and arming
acrtc->event and pflip_status, where HW latch (VUPDATE_NO_LOCK) can fire and
run the handler:
Thread A: Thread B:
PROGRAM(flip_n)
LATCH(flip_n)
vupdate_no_lock_handler(flip_n) # no event armed; skip
sending
ARM(flip_n)
Ah, but the flip_programmed completion has the same issue...
Thread A: Thread B:
INIT_(flip_programmed)
ARM(flip_n)
PROGRAM(flip_n)
LATCH(flip_n)
vupdate_no_lock_handler(flip_n) # flip_programmed
not complete; skip sending
COMPLETE(flip_programmed)
>
> 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.
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. That
is, if the exact scenario above happens.
If it sounds good to you, I'll roll that into v2 as well.
>
>>> 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.
- Leo
