On Fri, 2023-11-03 at 09:47 +0000, Tvrtko Ursulin wrote: > > On 03/11/2023 08:58, Coelho, Luciano wrote: > > On Fri, 2023-11-03 at 03:31 +0000, Vivi, Rodrigo wrote: > > > > > > > > > > Any other suggestions? > > > > > > > > I think it will boil down to the reason uncore lock is held > > > > over > > > > the > > > > respective sections ie. the comment in > > > > i915_get_crtc_scanoutpos. > > > > > > > > If it is timing sensitive to the extent irq off was needed it > > > > may > > > > apply > > > > to Xe as well. Likewise the need to use mmio helpers which rely > > > > on > > > > the > > > > uncore lock already held. Question of whether there is > > > > conceptual > > > > commonality, will probably drive the best name, or the best > > > > approach > > > > in > > > > general. > > > > > > yeap, this is how I'm seeing this. If i915-display needs this > > > global > > > lock around mmio operations, then we wound need to add it to the > > > xe_mmio as well and then solve the name, etc. > > > > > > However, I don't believe that other users of the mmio would need > > > this lock. So I believe the right thing to do is to create a > > > i915- > > > display only spin_lock, around the intel_de_mmio calls and here. > > > > > > With this we entirely kill the dependency on someone-else's lock > > > and have something that is entirely inside display code so it > > > doesn't need to be ported to one or another driver core > > > components. > > > > Right, I agree too. > > > > My patch was just trying to address the quick hack made for Xe, not > > the > > actual implementation in Xe's side. But it makes sense to > > implement > > this new lock internally in the display so there are no > > dependencies or > > wrappers needed. > > > > I'll respin. > > You could also make sure it needs to be a lock and not just say a > preempt off or irq section? indeed a good question. maybe we don't need the lock at all there... > > Regards, > > Tvrtko
