On Mon, 23 Oct 2023, "Coelho, Luciano" <luciano.coelho@xxxxxxxxx> wrote: > On Mon, 2023-10-23 at 12:11 +0300, Jani Nikula wrote: >> On Mon, 23 Oct 2023, Luca Coelho <luciano.coelho@xxxxxxxxx> 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. >> > >> > Signed-off-by: Luca Coelho <luciano.coelho@xxxxxxxxx> >> > --- >> > >> > Note: this patch was accidentally sent only to intel-xe[1], but should >> > have been sent to intel-gfx. Thus, this is v2. >> > >> > In v2: >> > >> > * Renamed uncore_spin_*() to intel_spin_*() >> > * Corrected the order: save, lock, unlock, restore >> > >> > [1] https://patchwork.freedesktop.org/patch/563288/ >> > >> > >> > drivers/gpu/drm/i915/display/intel_display.h | 22 +++++++++++++++++++- >> > drivers/gpu/drm/i915/display/intel_vblank.c | 19 ++++++++++------- >> > 2 files changed, 33 insertions(+), 8 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h >> > index 0e5dffe8f018..099476906f4c 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" >> >> In general, please avoid including headers from headers. In particular, >> please don't include i915_drv.h from headers. The header >> interdependencies are pretty bad already, and we need to clean it up. > > Right, I thought twice about this, but this seems far from clean, as > you say, so I thought it would be fine. Adding that single include bumps the total recursive includes of this file from 2 to 97... i915_drv.h is pretty bad. BR, Jani. > > There's not much point, though, since we have a declaration for > drm_i915_private in this file anyway, so the dependency is still > there... > > In any case, I'll unspin this change and go back to passing the actual > lock instead of passing drm_i915_private. > > -- > Cheers, > Luca. -- Jani Nikula, Intel
