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

[Intel-xe] [PATCH v7] 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.

Cc: Tvrtko Ursulin <tvrto.ursulin@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---


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
+}
+
 static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
 				     bool in_vblank_irq,
 				     int *vpos, int *hpos,
@@ -302,11 +328,12 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
 	}
 
 	/*
-	 * Lock uncore.lock, as we will do multiple timing critical raw
-	 * register reads, potentially with preemption disabled, so the
-	 * following code must not block on uncore.lock.
+	 * Enter vblank critical section, as we will do multiple
+	 * timing critical raw register reads, potentially with
+	 * preemption disabled, so the following code must not block.
 	 */
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+	local_irq_save(irqflags);
+	intel_vblank_section_enter(dev_priv);
 
 	/* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */
 
@@ -374,7 +401,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_vblank_section_exit(dev_priv);
+	local_irq_restore(irqflags);
 
 	/*
 	 * While in vblank, position will be negative
@@ -412,9 +440,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_vblank_section_enter(dev_priv);
+
 	position = __intel_get_crtc_scanline(crtc);
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+
+	intel_vblank_section_exit(dev_priv);
+	local_irq_restore(irqflags);
 
 	return position;
 }
@@ -537,7 +569,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_vblank_section_enter(i915);
 
 	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
 
@@ -546,7 +578,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_vblank_section_exit(i915);
 	spin_unlock_irqrestore(&i915->drm.vblank_time_lock, irqflags);
 }
-- 
2.39.2

[Intel-xe] ✗ CI.Patch_applied: failure for drm/i915: handle uncore spinlock when not available (rev5)

[Patchwork]

== Series Details ==

Series: drm/i915: handle uncore spinlock when not available (rev5)
URL   : https://patchwork.freedesktop.org/series/125299/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-xe-next' with base: ===
Base commit: 450234a8c drm/xe/xe2: Add workaround 16020292621
=== git am output follows ===
error: patch failed: drivers/gpu/drm/i915/display/intel_vblank.c:302
error: drivers/gpu/drm/i915/display/intel_vblank.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Applying: drm/i915: handle uncore spinlock when not available
Patch failed at 0001 drm/i915: handle uncore spinlock when not available
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

[Hogander, Jouni]

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@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
> 
> 
> 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.


BR,

Jouni Högander

>  static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
>                                      bool in_vblank_irq,
>                                      int *vpos, int *hpos,
> @@ -302,11 +328,12 @@ static bool i915_get_crtc_scanoutpos(struct
> drm_crtc *_crtc,
>         }
>  
>         /*
> -        * Lock uncore.lock, as we will do multiple timing critical
> raw
> -        * register reads, potentially with preemption disabled, so
> the
> -        * following code must not block on uncore.lock.
> +        * Enter vblank critical section, as we will do multiple
> +        * timing critical raw register reads, potentially with
> +        * preemption disabled, so the following code must not block.
>          */
> -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +       local_irq_save(irqflags);
> +       intel_vblank_section_enter(dev_priv);
>  
>         /* preempt_disable_rt() should go right here in PREEMPT_RT
> patchset. */
>  
> @@ -374,7 +401,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_vblank_section_exit(dev_priv);
> +       local_irq_restore(irqflags);
>  
>         /*
>          * While in vblank, position will be negative
> @@ -412,9 +440,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_vblank_section_enter(dev_priv);
> +
>         position = __intel_get_crtc_scanline(crtc);
> -       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +
> +       intel_vblank_section_exit(dev_priv);
> +       local_irq_restore(irqflags);
>  
>         return position;
>  }
> @@ -537,7 +569,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_vblank_section_enter(i915);
>  
>         drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
>  
> @@ -546,7 +578,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_vblank_section_exit(i915);
>         spin_unlock_irqrestore(&i915->drm.vblank_time_lock,
> irqflags);
>  }

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

[Coelho, Luciano]

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@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > ---
> > 
> > 
> > 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.


--
Cheers,
Luca.

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

[Hogander, Jouni]

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@intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > > ---
> > > 
> > > 
> > > 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.

Ok, thank you for the clarification:

Reviewed-by: Jouni Högander <jouni.hogander@intel.com>

> 
> 
> --
> Cheers,
> Luca.

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

[Hogander, Jouni]

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@intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > > ---
> > > 
> > > 
> > > 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.

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

[Sebastian Andrzej Siewior]

On 2023-12-01 12:00:32 [+0200], Luca Coelho wrote:
> To handle this, split the spin_lock/unlock_irqsave/restore() into
> spin_lock/unlock() followed by a call to local_irq_save/restore() and

On PREEMPT_RT spinlock_t becomes a sleeping lock so this split is not
working. See Documentation/locking/locktypes.rst

What I don't understand: do you have to keep the interrupts disabled
for some reasons or is it just to avoid using _irqsave() twice?

I do have more i915 related patches in the PREEMPT_RT queue and I can
post them if you folks have time for it. For now, let me show what I did
here:

----------->8-------------

From: Mike Galbraith <umgwanakikbuti@gmail.com>
Date: Sat, 27 Feb 2016 08:09:11 +0100
Subject: [PATCH 03/10] drm/i915: Use preempt_disable/enable_rt() where
 recommended

Mario Kleiner suggest in commit
  ad3543ede630f ("drm/intel: Push get_scanout_position() timestamping into kms driver.")

a spots where preemption should be disabled on PREEMPT_RT. The
difference is that on PREEMPT_RT the intel_uncore::lock disables neither
preemption nor interrupts and so region remains preemptible.

The area covers only register reads and writes. The part that worries me
is:
- __intel_get_crtc_scanline() the worst case is 100us if no match is
  found.

- intel_crtc_scanlines_since_frame_timestamp() not sure how long this
  may take in the worst case.

It was in the RT queue for a while and nobody complained.
Disable preemption on PREEPMPT_RT during timestamping.

[bigeasy: patch description.]

Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
Signed-off-by: Mike Galbraith <umgwanakikbuti@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpu/drm/i915/display/intel_vblank.c |   38 ++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 10 deletions(-)

--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -275,6 +275,26 @@ int intel_crtc_scanline_to_hw(struct int
  * all register accesses to the same cacheline to be serialized,
  * otherwise they may hang.
  */
+static void intel_vblank_section_enter_irqsave(struct drm_i915_private *i915, unsigned long *flags)
+	__acquires(i915->uncore.lock)
+{
+#ifdef I915
+	spin_lock_irqsave(&i915->uncore.lock, *flags);
+#else
+	*flags = NULL;
+#endif
+}
+
+static void intel_vblank_section_exit_irqrestore(struct drm_i915_private *i915, unsigned long flags)
+	__releases(i915->uncore.lock)
+{
+#ifdef I915
+	spin_unlock_irqrestore(&i915->uncore.lock, flags);
+#else
+	if (flags)
+		;
+#endif
+}
 static void intel_vblank_section_enter(struct drm_i915_private *i915)
 	__acquires(i915->uncore.lock)
 {
@@ -332,10 +352,10 @@ static bool i915_get_crtc_scanoutpos(str
 	 * timing critical raw register reads, potentially with
 	 * preemption disabled, so the following code must not block.
 	 */
-	local_irq_save(irqflags);
-	intel_vblank_section_enter(dev_priv);
+	intel_vblank_section_enter_irqsave(dev_priv, &irqflags);
 
-	/* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */
+	if (IS_ENABLED(CONFIG_PREEMPT_RT))
+		preempt_disable();
 
 	/* Get optional system timestamp before query. */
 	if (stime)
@@ -399,10 +419,10 @@ static bool i915_get_crtc_scanoutpos(str
 	if (etime)
 		*etime = ktime_get();
 
-	/* preempt_enable_rt() should go right here in PREEMPT_RT patchset. */
+	if (IS_ENABLED(CONFIG_PREEMPT_RT))
+		preempt_enable();
 
-	intel_vblank_section_exit(dev_priv);
-	local_irq_restore(irqflags);
+	intel_vblank_section_exit_irqrestore(dev_priv, irqflags);
 
 	/*
 	 * While in vblank, position will be negative
@@ -440,13 +460,11 @@ int intel_get_crtc_scanline(struct intel
 	unsigned long irqflags;
 	int position;
 
-	local_irq_save(irqflags);
-	intel_vblank_section_enter(dev_priv);
+	intel_vblank_section_enter_irqsave(dev_priv, &irqflags);
 
 	position = __intel_get_crtc_scanline(crtc);
 
-	intel_vblank_section_exit(dev_priv);
-	local_irq_restore(irqflags);
+	intel_vblank_section_exit_irqrestore(dev_priv, irqflags);
 
 	return position;
 }