Hey Uma,
Den 2025-11-26 kl. 21:03, skrev Shankar, Uma:
>
>
>> -----Original Message-----
>> From: Intel-xe <[email protected]> On Behalf Of Maarten
>> Lankhorst
>> Sent: Tuesday, November 4, 2025 2:06 PM
>> To: [email protected]; [email protected]
>> Cc: [email protected]; Maarten Lankhorst <[email protected]>;
>> Mario
>> Kleiner <[email protected]>; Mike Galbraith
>> <[email protected]>; Thomas Gleixner <[email protected]>; Sebastian
>> Andrzej Siewior <[email protected]>; Clark Williams
>> <[email protected]>; Steven Rostedt <[email protected]>
>> Subject: [PATCH v2 4/7] drm/i915/display: Remove locking from
>> intel_vblank_evade critical section
>>
>> 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) {
>> + *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) {
>
> Not sure if doing the scanline check with interrupts on is ok. The scanlines
> can move
> if we get interrupted or what happens if we get a vblank interrupt. Looks
> vulnerable to race.
>
> I will try to check this further and get back.
There is a double check here to make it safe:
while (!vblank_evade()) {
drop locks();
while (!vblank_evadable())
wait();
re-acquire locks();
}
Even if the middle vblank evadable is unsafe, and it has to be to be able to
wait, we re-check after acquiring locks.
We must do so there anyway since the locking acquire can add any amount of
latency as well.
Kind regards,
Maarten Lankhorst
