PREEMPT_RT vs i915

[Ville Syrjälä]

On Thu, Jul 10, 2025 at 11:04:22AM -0700, Matthew Brost wrote:
> 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.

I don't want a silly open coded solution like that. The tracepoint
should just do the right thing on its own. But I haven't actually
looked how hard it would to add that into the existing macro
jungle that is tracepoints...

-- 
Ville Syrjälä
Intel