Den 2025-11-26 kl. 22:17, skrev Ville Syrjälä:
> On Tue, Nov 04, 2025 at 09:36:28AM +0100, Maarten Lankhorst wrote:
>> finish_wait() may take a lock, which means that it can take any amount
>> of time. On PREEMPT-RT we should not be taking any lock after disabling
>> preemption, so ensure that the completion is done before disabling
>> interrupts.
>>
>> This also has the benefit of making vblank evasion more deterministic,
>> by performing the final vblank check after all locking is done.
>>
>> Signed-off-by: Maarten Lankhorst <[email protected]>
>> ---
>> drivers/gpu/drm/i915/display/intel_vblank.c | 35 ++++++++++-----------
>> 1 file changed, 17 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c
>> b/drivers/gpu/drm/i915/display/intel_vblank.c
>> index 2b106ffa3f5f5..3628d2a1b8f38 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
>> @@ -708,6 +708,13 @@ void intel_vblank_evade_init(const struct
>> intel_crtc_state *old_crtc_state,
>> evade->min -= vblank_delay;
>> }
>>
>> +static inline int vblank_evadable(struct intel_vblank_evade_ctx *evade, int
>> *scanline)
>
> The name is confusing. But having a function
> would be nice since we need two checks I guess.
> scanline_is_safe() or something?
>
> Also type should be bool, and inline looks pointless.
>
>> +{
>> + *scanline = intel_get_crtc_scanline(evade->crtc);
>> +
>> + return *scanline < evade->min || *scanline > evade->max;
>> +}
>> +
>> /* must be called with vblank interrupt already enabled! */
>> int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>> {
>> @@ -715,23 +722,22 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx
>> *evade)
>> struct intel_display *display = to_intel_display(crtc);
>> long timeout = msecs_to_jiffies_timeout(1);
>> wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
>> - DEFINE_WAIT(wait);
>> int scanline;
>>
>> if (evade->min <= 0 || evade->max <= 0)
>> return 0;
>>
>> - for (;;) {
>> - /*
>> - * prepare_to_wait() has a memory barrier, which guarantees
>> - * other CPUs can see the task state update by the time we
>> - * read the scanline.
>> - */
>> - prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>> + while (!vblank_evadable(evade, &scanline)) {
>> + local_irq_enable();
>>
>> - scanline = intel_get_crtc_scanline(crtc);
>> - if (scanline < evade->min || scanline > evade->max)
>> - break;
>> + DEFINE_WAIT(wait);
>> + while (!vblank_evadable(evade, &scanline) && timeout > 0) {
>> + prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>> + timeout = schedule_timeout(timeout);
>> + }
>> + finish_wait(wq, &wait);
>> +
>> + local_irq_disable();
>>
>> if (!timeout) {
>
> This looks to introduce the classic "didn't check the
> condition after timeout" race.
>
> I guess what you're going for is something like this (done
> through a somewhat less intrusive reordering of the current
> code):
>
> for (;;) {
> if (scanline_is_safe(evade, &scanline))
> break;
>
> if (!timeout) {
> drm_dbg_kms(display->drm,
> "Potential atomic update failure on pipe %c\n",
> pipe_name(crtc->pipe));
> break;
> }
>
> local_irq_enable();
>
> prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>
> if (!scanline_is_safe(evade, &scanline))
> timeout = schedule_timeout(timeout);
>
> finish_wait(wq, &wait);
>
> local_irq_disable();
> }
>
> And then maybe the whole prepare+wait+finish thing there
> could be a simple wait_event_timeout(). That would make
> that inner thing a loop though. We might not want that
> just because jiffies is so coarse and we don't really
> want to wait multiple times there.
This could be a wait_event_timeout(), except it takes the struct waitqueue, and
we
only have a pointer to the queue.
Wasn't sure if it wait_event_timeout(*queue, ...) was allowed, but it seems at
least
the msm driver and even the atomic helper does so, so should be ok to use.
>
>> drm_dbg_kms(display->drm,
>> @@ -740,15 +746,8 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx
>> *evade)
>> break;
>> }
>>
>> - local_irq_enable();
>> -
>> - timeout = schedule_timeout(timeout);
>> -
>> - local_irq_disable();
>> }
>>
>> - finish_wait(wq, &wait);
>> -
>> /*
>> * On VLV/CHV DSI the scanline counter would appear to
>> * increment approx. 1/3 of a scanline before start of vblank.
>> --
>> 2.51.0
>
