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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 23 Oct 2023, "Coelho, Luciano" <luciano.coelho@intel.com> wrote:
> On Mon, 2023-10-23 at 12:11 +0300, Jani Nikula wrote:
>> On Mon, 23 Oct 2023, Luca Coelho <luciano.coelho@intel.com> 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@intel.com>
>> > ---
>> > 
>> > 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