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
