On Thu, 2023-12-07 at 08:24 +0000, Hogander, Jouni wrote:
> On Fri, 2023-12-01 at 12:00 +0200, Luca Coelho wrote:
> > The uncore code may not always be available (e.g. when we build the
> > display code with Xe), so we can't always rely on having the uncore's
> > spinlock.
> >
> > To handle this, split the spin_lock/unlock_irqsave/restore() into
> > spin_lock/unlock() followed by a call to local_irq_save/restore() and
> > create wrapper functions for locking and unlocking the uncore's
> > spinlock. In these functions, we have a condition check and only
> > actually try to lock/unlock the spinlock when I915 is defined, and
> > thus uncore is available.
> >
> > This keeps the ifdefs contained in these new functions and all such
> > logic inside the display code.
> >
> > Cc: Tvrtko Ursulin <tvrto.ursulin@xxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Signed-off-by: Luca Coelho <luciano.coelho@xxxxxxxxx>
> > ---
> >
> >
> > In v2:
> >
> > * Renamed uncore_spin_*() to intel_spin_*()
> > * Corrected the order: save, lock, unlock, restore
> >
> > In v3:
> >
> > * Undid the change to pass drm_i915_private instead of the lock
> > itself, since we would have to include i915_drv.h and that pulls
> > in a truckload of other includes.
> >
> > In v4:
> >
> > * After a brief attempt to replace this with a different patch,
> > we're back to this one;
> > * Pass drm_i195_private again, and move the functions to
> > intel_vblank.c, so we don't need to include i915_drv.h in a
> > header file and it's already included in intel_vblank.c;
> >
> > In v5:
> >
> > * Remove stray include in intel_display.h;
> > * Remove unnecessary inline modifiers in the new functions.
> >
> > In v6:
> >
> > * Just removed the umlauts from Ville's name, because patchwork
> > didn't catch my patch and I suspect it was some UTF-8 confusion.
> >
> > In v7:
> >
> > * Add __acquires()/__releases() annotation to resolve sparse
> > warnings.
> >
> > drivers/gpu/drm/i915/display/intel_vblank.c | 51 +++++++++++++++++--
> > --
> > 1 file changed, 41 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c
> > b/drivers/gpu/drm/i915/display/intel_vblank.c
> > index 2cec2abf9746..fe256bf7b485 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > @@ -265,6 +265,32 @@ int intel_crtc_scanline_to_hw(struct intel_crtc
> > *crtc, int scanline)
> > return (scanline + vtotal - crtc->scanline_offset) % vtotal;
> > }
> >
> > +/*
> > + * The uncore version of the spin lock functions is used to decide
> > + * whether we need to lock the uncore lock or not. This is only
> > + * needed in i915, not in Xe.
> > + *
> > + * This lock in i915 is needed because some old platforms (at least
> > + * IVB and possibly HSW as well), which are not supported in Xe,
> > need
> > + * all register accesses to the same cacheline to be serialized,
> > + * otherwise they may hang.
> > + */
> > +static void intel_vblank_section_enter(struct drm_i915_private
> > *i915)
> > + __acquires(i915->uncore.lock)
> > +{
> > +#ifdef I915
> > + spin_lock(&i915->uncore.lock);
> > +#endif
> > +}
> > +
> > +static void intel_vblank_section_exit(struct drm_i915_private *i915)
> > + __releases(i915->uncore.lock)
> > +{
> > +#ifdef I915
> > + spin_unlock(&i915->uncore.lock);
> > +#endif
> > +}
> > +
>
> Why don't you move these into gpu/drm/i915/intel_uncore.c/h? Then you
> could have empty defines/functions for these in gpu/drm/xe/compat-i915-
> headers/intel_uncore.h. That way you don't need these ifdefs. If you
> move them as I proposed you should rename them as well.
We already went forth and back with this for some time. In the end we
agreed that this is not related to uncore directly, so we decided to
keep it here.
We also agreed that I'll make a follow-up patch where it won't be only
the lock that will be handled by this, but also enabling/disabling
interrupts, which doesn't have anything to do with uncore, thus the
name of the function.
--
Cheers,
Luca.
