On 2/19/26 14:08, Michele Palazzi wrote:
> On 2/19/26 12:09, Michel Dänzer wrote:
>> On 2/18/26 11:09, Michele Palazzi wrote:
>>> On 2/18/26 10:41, Michel Dänzer wrote:
>>>> On 2/18/26 01:31, Michele Palazzi wrote:
>>>>>
>>>>> 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 a8a59126b2d2..35987ce80c71 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> @@ -10168,8 +10168,7 @@ static void amdgpu_dm_commit_planes(struct
>>>>> drm_atomic_state *state,
>>>>> } else if (cursor_update && acrtc_state->active_planes > 0) {
>>>>> spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
>>>>> if (acrtc_attach->base.state->event) {
>>>>> - drm_crtc_vblank_get(pcrtc);
>>>>> - acrtc_attach->event = acrtc_attach->base.state->event;
>>>>> + drm_crtc_send_vblank_event(pcrtc,
>>>>> acrtc_attach->base.state->event);
>>>>
>>>> Can this code run before start of vblank? If yes, the event would have the
>>>> wrong sequence number and timestamp.
>>>
>>> Yes, but the original code path had the same problem.
>>
>> Are you sure?
>>
>> I'd expect the original code to send the event only when an interrupt fires
>> during vblank, at which point the values are correct.
>
> Actually you are indeed right, this approach potentially produces slightly
> anticipated cursor events, not noticeable but wrong nonetheless.
"not noticeable" by what? It might be noticeable e.g. for mutter's KMS thread
deadline timer.
>>> Would drm_crtc_arm_vblank_event() be more appropriate here? The concern is
>>> that it reintroduces the starvation race if the vblank off-delay fires
>>> before the interrupt.
>>
>> Not sure that could happen, some of the issues discussed in the comment
>> above drm_crtc_arm_vblank_event might apply though.
>
> If i understand correctly, using drm_crtc_arm_vblank_event() we would still
> be having incorrect sequence, although delayed this time,
Not if used correctly, though per the comment above the function, there are
various potential races as well.
> We could maybe add a dedicated flag to amdgpu_dm_crtc_handle_vblank()
> instead, but it's something i already tried before submitting this and it
> produced the second race condition, so that alone is not enough.
>
> If there is a suggested approach i am willing to explore it
Can't the issue be solved by fixing the pflip_status handling in the vblank
handler?
I guess that might also hit the second race condition:
> - If vblank is disabled by the off-delay timer before the handler
> runs, the PENDING cursor event is never delivered and the commit hangs.
That sounds like the drm_crtc_vblank_get/put handling might be incorrect in
amdgpu_dm.
In a nutshell, the vblank interrupt is kept enabled as long as there have been
more
drm_crtc_vblank_get calls than _put ones for the CRTC. I.e. amdgpu_dm needs to
call the former under circumstances where it needs the interrupt to be on, and
the latter only once it's no longer needed for those circumstances (in this
case when sending the event).
--
Earthling Michel Dänzer \ GNOME / Xwayland / Mesa developer
https://redhat.com \ Libre software enthusiast
