Re: [Intel-xe] [Intel-gfx] [PATCH v3] drm/i915: handle uncore spinlock when not available

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

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