On Thu, 2023-12-07 at 09:30 +0000, Coelho, Luciano wrote:
> 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.
Thank you Luca for the patch. This is now pushed to drm-intel-next.
BR,
Jouni Högander
>
>
> --
> Cheers,
> Luca.
