On Wed, 2023-11-29 at 15:34 -0500, Rodrigo Vivi wrote:
> On Wed, Nov 29, 2023 at 03:24:33PM -0500, Coelho, Luciano wrote:
> > On Wed, 2023-11-29 at 13:01 -0500, Rodrigo Vivi wrote:
> > > On Wed, Nov 29, 2023 at 11:17:28AM +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: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > > Cc: Ville Syrj?l? <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > 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;
> > > >
> > > > drivers/gpu/drm/i915/display/intel_display.h | 1 +
> > > > drivers/gpu/drm/i915/display/intel_vblank.c | 45 +++++++++++++++-----
> > > > 2 files changed, 36 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> > > > index 8548f49e3972..5ff299bc4b87 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > > @@ -29,6 +29,7 @@
> > > >
> > > > #include "i915_reg_defs.h"
> > > > #include "intel_display_limits.h"
> > > > +#include "i915_drv.h"
> > >
> > > please move this include to intel_vblank.c
> >
> > Oops, this is a leftover of some tests I was making to see just how
> > much worse things would get by adding this here.
> >
> > Actually, why don't we move the drm_i915_private structure (and maybe
> > others?) to a lighter header file than i915_drv.h? IMHO it's really
> > annoying to have the forward declarations for it in many places just
> > because we don't want to include the actual header. When I want to
> > find its global definition, cscope always returns tens of results
> > because of the forward declarations... This is obviously orthogonal to
> > the current patch.
>
> yeah, I know. It is really inconvenient sometimes. I got used to run
> cscope and then search for "struct something {" to find the right place.
That's what I do as well. Actually, just searching for " {" in the
results usually suffices. But anyway inconvenient.
> But this inconvenience is actually smaller when compared to the compilation
> time whenever a header gets modified and included by other headers. If I
> remember correctly Jani did the initial assessment of compilation times
> and started to move headers including out of other headers. He might
> have more details/data on his findings.
Okay, I'll talk to him. But it's always nice to rant a bit in public,
so my blurt stands. :D
--
Cheers,
Luca.
