On 17/11/2023 12:21, Coelho, Luciano wrote:
Adding Tvrtko, for some reason he didn't get CCed before.
On Fri, 2023-11-17 at 11:26 +0200, Ville Syrjälä wrote:
On Fri, Nov 17, 2023 at 10:41:43AM +0200, Ville Syrjälä wrote:
On Fri, Nov 17, 2023 at 08:05:21AM +0000, Coelho, Luciano wrote:
Thanks for your comments, Ville!
On Fri, 2023-11-17 at 09:19 +0200, Ville Syrjälä wrote:
On Thu, Nov 16, 2023 at 01:27:00PM +0200, Luca Coelho wrote:
Since we're abstracting the display code from the underlying driver
(i.e. i915 vs xe), we can't use the uncore's spinlock to protect
critical sections of our code.
After further inspection, it seems that the spinlock is not needed at
all and this can be handled by disabling preemption and interrupts
instead.
uncore.lock has multiple purposes:
1. serialize all register accesses to the same cacheline as on
certain platforms that can hang the machine
Okay, do you remember which platforms?
HSW is the one I remember for sure being affected.
Althoguh I don't recall if I ever managed to hang it
using display registers specifically. intel_gpu_top
certainly was very good at reproducing the problem.
I couldn't find any reference to
this reason.
If all else fails git log is your friend.
It seems to be documented in intel_uncore.h. Though that one
mentions IVB instead of HSW for some reason. I don't recall
seeing it on IVB myself, but I suppose it might have been an
issue there as well. How long the problem remained after HSW
I have no idea.
Oh, somehow I missed that. Thanks.
So, it seems that the locking is indeed needed. I think there are two
options, then:
1. Go back to my previous version of the patch, where I had the wrapper
that didn't lock anything on Xe and implement the display part when we
get a similar implementation of the uncore.lock on Xe (if ever needed).
2. Implement a display-local lock for this, as suggested at some point,
including the other intel_de*() accesses. But can we be sure that it's
enough to protect only the registers accessed by the display? I.e.
won't accessing display registers non-serially in relation to non-
display registers?
Preferences?
AFAIR my initial complaint was the naming which was along the lines of
intel_spin_lock(uncore). How to come up with a clean and logical name is
the question...
On Xe you don't need a lock since HSW/IVB/cacheline angle does not exist
but you need a name which does not clash with either.
Assuming you still need the preempt off (or irq off) on Xe for better
timings, maybe along the lines of intel_vblank_section_enter/exit(i915)?
Although I am not up to speed with what object instead of i915 would you
be passing in from Xe ie. how exactly polymorphism is implemented in
shared code.
Regards,
Tvrtko
