Hi all,
Debian currently provides non-default kernel packages with the
PREEMPT_RT patchset applied and enabled. However, for the Debian 14
"forky" release the plan is to use only the upstream RT support.
One result of this is that the i915 driver will not be included in our
RT kernel package on amd64 because the upstream version lacks the
patches to make it compatible with PREEMPT_RT. This was not a surprise
to us, but may disappoint some of our users (for example see <https://bugs.debian.org/1108324>).
I see that Sebastian submitted the i915 fixes upstream in October 2024.
If I understand the explanation in <https://lore.kernel.org/intel-gfx/[email protected]/> rightly,
much of these changes are unsafe because i915 has its own hard timing requirement for reprogramming multiple display controller registers
within a single frame. Is that still the sticking point?
It seems like the critical uncore lock is currently held in a lot of
places and potentially for a long time.
Would it be practical to split
this lock into:
1. raw spinlock protecting only state needed for the atomic (within-one- frame) update
2. regular spinlock protecting everything in uncore
or is almost all the uncore state potentially used during an atomic
update?
Would it help to offload the atomic updates to a kthread that runs with
RT priority but still with hard interrupts enabled?
Would it make things easier if setting CONFIG_PREEMPT_RT=y limited i915
to not run on some older hardware?
It seems like the critical uncore lock is currently held in a lot of
places and potentially for a long time.
It shouldn't be held for that long. I think it should just be
a raw spinlock.
On 2025-07-09 20:30:26 [+0300], Ville Syrj�l� wrote:
It seems like the critical uncore lock is currently held in a lot of places and potentially for a long time.
It shouldn't be held for that long. I think it should just be
a raw spinlock.
What about I resubmit the series and we look again? I don't think the
lock should be made raw just to be done with it.
And IIRC the other remaining problem with RT was the spinlocks used
inside tracepoints (which is uncore lock, and probably some vblank
locks). So that too needs some kind of solution because it's going to
very hard to debug the timing sensitive parts without the tracepoints.
A bit of a drive-by comment, but taking locks inside tracepoints seems
like a pretty horrible idea in general. We've managed to write an entire driver (Xe) from scratch and bring it up without doing this. I'd be very surprised if this is truly necessary in i915.
Matt
On Wed, Jul 09, 2025 at 09:44:43PM +0200, Sebastian Andrzej Siewior wrote:
On 2025-07-09 20:30:26 [+0300], Ville Syrjälä wrote:
It seems like the critical uncore lock is currently held in a lot of places and potentially for a long time.
It shouldn't be held for that long. I think it should just be
a raw spinlock.
What about I resubmit the series and we look again? I don't think the
lock should be made raw just to be done with it.
Until someone actually does the work to confirm the thing is working
reliably there's no point in posting anything.
And IIRC the other remaining problem with RT was the spinlocks used
inside tracepoints (which is uncore lock, and probably some vblank
locks). So that too needs some kind of solution because it's going to
very hard to debug the timing sensitive parts without the tracepoints.
When this was last discussed I suggested that there should be a
versions of the tracepoint macros that do the sampling outside
the lock, but that wasn't deemed acceptable for whatever reason.
I don't even know why the current macros are doing the
sampling while holding the lock...
On 2025-07-09 23:09:22 [+0300], Ville Syrj�l� wrote:
On Wed, Jul 09, 2025 at 09:44:43PM +0200, Sebastian Andrzej Siewior wrote:
On 2025-07-09 20:30:26 [+0300], Ville Syrj�l� wrote:
It seems like the critical uncore lock is currently held in a lot of places and potentially for a long time.
It shouldn't be held for that long. I think it should just be
a raw spinlock.
What about I resubmit the series and we look again? I don't think the lock should be made raw just to be done with it.
Until someone actually does the work to confirm the thing is working reliably there's no point in posting anything.
Well it works on my machine and this machine dose not pass the code
paths that I patch.
Every patch made was done because someone reported an error/ warning and confirmed afterwards that the patch fixes it for them and they can use
the machine and don't observe anything.
And IIRC the other remaining problem with RT was the spinlocks used
inside tracepoints (which is uncore lock, and probably some vblank
locks). So that too needs some kind of solution because it's going to
very hard to debug the timing sensitive parts without the tracepoints.
no, not just that. Making the lock raw led to latency spikes in simple
spikes and I just disabled trace points. It could be worked around by
taking the lock if the tracepoint is enabled and then invoking the
tracepoint unconditionally and not taking the lock anymore. Steven made
a suggestion a while ago how to put this in macro as far as I remember.
On Wed, Jul 09, 2025 at 11:09:22PM +0300, Ville Syrj�l� wrote:
On Wed, Jul 09, 2025 at 09:44:43PM +0200, Sebastian Andrzej Siewior wrote:
On 2025-07-09 20:30:26 [+0300], Ville Syrj�l� wrote:
It seems like the critical uncore lock is currently held in a lot of places and potentially for a long time.
It shouldn't be held for that long. I think it should just be
a raw spinlock.
What about I resubmit the series and we look again? I don't think the lock should be made raw just to be done with it.
Until someone actually does the work to confirm the thing is working reliably there's no point in posting anything.
And IIRC the other remaining problem with RT was the spinlocks used
inside tracepoints (which is uncore lock, and probably some vblank
locks). So that too needs some kind of solution because it's going to
very hard to debug the timing sensitive parts without the tracepoints.
A bit of a drive-by comment, but taking locks inside tracepoints seems
like a pretty horrible idea in general. We've managed to write an entire driver (Xe) from scratch and bring it up without doing this.
I'd be very
surprised if this is truly necessary in i915.
On Wed, 2025-07-09 at 23:09 +0300, Ville Syrj�l� wrote:
On Wed, Jul 09, 2025 at 09:44:43PM +0200, Sebastian Andrzej Siewior wrote:
On 2025-07-09 20:30:26 [+0300], Ville Syrj�l� wrote:
It seems like the critical uncore lock is currently held in a lot of places and potentially for a long time.
It shouldn't be held for that long. I think it should just be
a raw spinlock.
What about I resubmit the series and we look again? I don't think the lock should be made raw just to be done with it.
Until someone actually does the work to confirm the thing is working reliably there's no point in posting anything.
What does that entail?
On Thu, Jul 10, 2025 at 06:21:09PM +0300, Ville Syrjälä wrote:
On Wed, Jul 09, 2025 at 03:04:27PM -0700, Matthew Brost wrote:
On Wed, Jul 09, 2025 at 11:09:22PM +0300, Ville Syrjälä wrote:
On Wed, Jul 09, 2025 at 09:44:43PM +0200, Sebastian Andrzej Siewior wrote:
On 2025-07-09 20:30:26 [+0300], Ville Syrjälä wrote:
It seems like the critical uncore lock is currently held in a lot of
places and potentially for a long time.
It shouldn't be held for that long. I think it should just be
a raw spinlock.
What about I resubmit the series and we look again? I don't think the lock should be made raw just to be done with it.
Until someone actually does the work to confirm the thing is working reliably there's no point in posting anything.
And IIRC the other remaining problem with RT was the spinlocks used inside tracepoints (which is uncore lock, and probably some vblank locks). So that too needs some kind of solution because it's going to very hard to debug the timing sensitive parts without the tracepoints.
A bit of a drive-by comment, but taking locks inside tracepoints seems like a pretty horrible idea in general. We've managed to write an entire driver (Xe) from scratch and bring it up without doing this.
For xe gt stuff specifically the one reason for needing a lock
could be forcewake. Ie. if you read a register that needs
Yes, we don't forcewake on demand in Xe; instead, we expect the upper
layers to hold these.
forcewake from a tracepoint you might need some kind of protection
against concurrent access. But xe lacks any kind of forcewake
sanity checker, so no one would likely even know if you get that
We do have some asserts to catch missing forcewake—see xe_force_wake_assert_held—but this is incomplete compared to the i915 checker.
wrong. Unless they notice a bogus register value in the
trace that is. But maybe xe doesn't use such registers in its
tracepoints atm, who knows.
I just audited the Xe tracepoints. We mostly just assign values from
software state, or in some cases, read memory from the VRAM BAR. The
latter again doesn't require any locks—just asserts that the device is awake.
And speaking for hardware in general, indexed registers aren't
exactly an uncommon thing. So the tracing stuff really should
have a sane standard way to deal with them...
I'd be very
surprised if this is truly necessary in i915.
The most fundemental reason is the hardware issue present on
some platforms which can hang the machine if you access the
same cacheline from multiple threads simultaneously.
Ooo, yeah—yikes.
One novel solution here: move the MMIO reads out of the tracepoint code
and into the caller, passing the values as arguments to the tracepoint.
Then, just assign the values inside the tracepoint. I think that would
work and be functionally equivalent.
On 2025-07-10 18:04:42 [+0300], Ville Syrjälä wrote:
When this was last discussed I suggested that there should be a
versions of the tracepoint macros that do the sampling outside
the lock, but that wasn't deemed acceptable for whatever reason.
I don't even know why the current macros are doing the
sampling while holding the lock...
Any objections to me sending the batch and we figure out later how get
the tracepoints for i915 enabled again on RT?
It would be an improvement because you could take a vanilla kernel and
enable PREEMPT_RT and you would only miss the tracepoints while now you
can't enable i915 at all and XE either doesn't compile or spills
warnings at runtime due to the code it shares with i915.
Sebastian
From the xe point of view, I would say applying patch 1-3 is sufficient.Patch 3 because i915_utils.h is used by display still, unfortunately..
| Sysop: | Keyop |
|---|---|
| Location: | Huddersfield, West Yorkshire, UK |
| Users: | 715 |
| Nodes: | 16 (3 / 13) |
| Uptime: | 43:28:52 |
| Calls: | 12,111 |
| Calls today: | 2 |
| Files: | 15,008 |
| Messages: | 6,518,440 |