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

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

[Luca Coelho]

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"
 
 enum drm_scaling_filter;
 struct dpll;
@@ -41,7 +42,6 @@ struct drm_file;
 struct drm_format_info;
 struct drm_framebuffer;
 struct drm_i915_gem_object;
-struct drm_i915_private;
 struct drm_mode_fb_cmd2;
 struct drm_modeset_acquire_ctx;
 struct drm_plane;
@@ -559,4 +559,24 @@ bool assert_port_valid(struct drm_i915_private *i915, enum port port);
 
 bool intel_scanout_needs_vtd_wa(struct drm_i915_private *i915);
 
+/*
+ * 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.  Keep the decision-making centralized
+ * here.
+ */
+static inline void intel_spin_lock(struct drm_i915_private *i915)
+{
+#ifdef I915
+	spin_lock(&i915->uncore.lock);
+#endif
+}
+
+static inline void intel_spin_unlock(struct drm_i915_private *i915)
+{
+#ifdef I915
+	spin_unlock(&i915->uncore.lock);
+#endif
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index 2cec2abf9746..7c624ea7e902 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -306,7 +306,8 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
 	 * register reads, potentially with preemption disabled, so the
 	 * following code must not block on uncore.lock.
 	 */
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+	local_irq_save(irqflags);
+	intel_spin_lock(dev_priv);
 
 	/* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */
 
@@ -374,7 +375,8 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
 
 	/* preempt_enable_rt() should go right here in PREEMPT_RT patchset. */
 
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+	intel_spin_unlock(dev_priv);
+	local_irq_restore(irqflags);
 
 	/*
 	 * While in vblank, position will be negative
@@ -412,9 +414,13 @@ int intel_get_crtc_scanline(struct intel_crtc *crtc)
 	unsigned long irqflags;
 	int position;
 
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+	local_irq_save(irqflags);
+	intel_spin_lock(dev_priv);
+
 	position = __intel_get_crtc_scanline(crtc);
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+
+	intel_spin_unlock(dev_priv);
+	local_irq_restore(irqflags);
 
 	return position;
 }
@@ -537,7 +543,7 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state,
 	 * Need to audit everything to make sure it's safe.
 	 */
 	spin_lock_irqsave(&i915->drm.vblank_time_lock, irqflags);
-	spin_lock(&i915->uncore.lock);
+	intel_spin_lock(i915);
 
 	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
 
@@ -546,7 +552,6 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state,
 	crtc->mode_flags = mode_flags;
 
 	crtc->scanline_offset = intel_crtc_scanline_offset(crtc_state);
-
-	spin_unlock(&i915->uncore.lock);
+	intel_spin_unlock(i915);
 	spin_unlock_irqrestore(&i915->drm.vblank_time_lock, irqflags);
 }
-- 
2.39.2

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

[Jani Nikula]

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.

BR,
Jani.



>  
>  enum drm_scaling_filter;
>  struct dpll;
> @@ -41,7 +42,6 @@ struct drm_file;
>  struct drm_format_info;
>  struct drm_framebuffer;
>  struct drm_i915_gem_object;
> -struct drm_i915_private;
>  struct drm_mode_fb_cmd2;
>  struct drm_modeset_acquire_ctx;
>  struct drm_plane;
> @@ -559,4 +559,24 @@ bool assert_port_valid(struct drm_i915_private *i915, enum port port);
>  
>  bool intel_scanout_needs_vtd_wa(struct drm_i915_private *i915);
>  
> +/*
> + * 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.  Keep the decision-making centralized
> + * here.
> + */
> +static inline void intel_spin_lock(struct drm_i915_private *i915)
> +{
> +#ifdef I915
> +	spin_lock(&i915->uncore.lock);
> +#endif
> +}
> +
> +static inline void intel_spin_unlock(struct drm_i915_private *i915)
> +{
> +#ifdef I915
> +	spin_unlock(&i915->uncore.lock);
> +#endif
> +}
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 2cec2abf9746..7c624ea7e902 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -306,7 +306,8 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
>  	 * register reads, potentially with preemption disabled, so the
>  	 * following code must not block on uncore.lock.
>  	 */
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +	local_irq_save(irqflags);
> +	intel_spin_lock(dev_priv);
>  
>  	/* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */
>  
> @@ -374,7 +375,8 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
>  
>  	/* preempt_enable_rt() should go right here in PREEMPT_RT patchset. */
>  
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +	intel_spin_unlock(dev_priv);
> +	local_irq_restore(irqflags);
>  
>  	/*
>  	 * While in vblank, position will be negative
> @@ -412,9 +414,13 @@ int intel_get_crtc_scanline(struct intel_crtc *crtc)
>  	unsigned long irqflags;
>  	int position;
>  
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +	local_irq_save(irqflags);
> +	intel_spin_lock(dev_priv);
> +
>  	position = __intel_get_crtc_scanline(crtc);
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +
> +	intel_spin_unlock(dev_priv);
> +	local_irq_restore(irqflags);
>  
>  	return position;
>  }
> @@ -537,7 +543,7 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state,
>  	 * Need to audit everything to make sure it's safe.
>  	 */
>  	spin_lock_irqsave(&i915->drm.vblank_time_lock, irqflags);
> -	spin_lock(&i915->uncore.lock);
> +	intel_spin_lock(i915);
>  
>  	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
>  
> @@ -546,7 +552,6 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state,
>  	crtc->mode_flags = mode_flags;
>  
>  	crtc->scanline_offset = intel_crtc_scanline_offset(crtc_state);
> -
> -	spin_unlock(&i915->uncore.lock);
> +	intel_spin_unlock(i915);
>  	spin_unlock_irqrestore(&i915->drm.vblank_time_lock, irqflags);
>  }

-- 
Jani Nikula, Intel

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

[Coelho, Luciano]

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.

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.

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

[Jani Nikula]

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

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

[Coelho, Luciano]

On Mon, 2023-10-23 at 13:21 +0300, Jani Nikula wrote:
> 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.

Argh.  I'm sending a v3 asap! :)

--
Luca.