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

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

[Luca Coelho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 5198 bytes --]

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: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.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;

 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"
 
 enum drm_scaling_filter;
 struct dpll;
diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index 2cec2abf9746..d9625db82681 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -265,6 +265,26 @@ 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.  Keep the decision-making centralized
+ * here.
+ */
+static inline void intel_vblank_section_enter(struct drm_i915_private *i915)
+{
+#ifdef I915
+	spin_lock(&i915->uncore.lock);
+#endif
+}
+
+static inline void intel_vblank_section_exit(struct drm_i915_private *i915)
+{
+#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 +322,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 +395,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 +434,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 +563,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 +572,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] [PATCH v4] drm/i915: handle uncore spinlock when not available

[Luca Coelho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 5198 bytes --]

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: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.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;

 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"
 
 enum drm_scaling_filter;
 struct dpll;
diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index 2cec2abf9746..d9625db82681 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -265,6 +265,26 @@ 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.  Keep the decision-making centralized
+ * here.
+ */
+static inline void intel_vblank_section_enter(struct drm_i915_private *i915)
+{
+#ifdef I915
+	spin_lock(&i915->uncore.lock);
+#endif
+}
+
+static inline void intel_vblank_section_exit(struct drm_i915_private *i915)
+{
+#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 +322,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 +395,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 +434,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 +563,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 +572,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

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

[Rodrigo Vivi]

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@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ville Syrj?l? <ville.syrjala@linux.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;
> 
>  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

>  
>  enum drm_scaling_filter;
>  struct dpll;
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 2cec2abf9746..d9625db82681 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -265,6 +265,26 @@ 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.  Keep the decision-making centralized
> + * here.

maybe we could add brief mention that it is only needed because old hardware
that is not supported by Xe.

> + */
> +static inline void intel_vblank_section_enter(struct drm_i915_private *i915)

let's avoid inline here.

> +{
> +#ifdef I915
> +	spin_lock(&i915->uncore.lock);
> +#endif
> +}
> +
> +static inline void intel_vblank_section_exit(struct drm_i915_private *i915)

and here

With these changes:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +{
> +#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 +322,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 +395,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 +434,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 +563,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 +572,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
> 

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

[Rodrigo Vivi]

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@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ville Syrj?l? <ville.syrjala@linux.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;
> 
>  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

>  
>  enum drm_scaling_filter;
>  struct dpll;
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 2cec2abf9746..d9625db82681 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -265,6 +265,26 @@ 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.  Keep the decision-making centralized
> + * here.

maybe we could add brief mention that it is only needed because old hardware
that is not supported by Xe.

> + */
> +static inline void intel_vblank_section_enter(struct drm_i915_private *i915)

let's avoid inline here.

> +{
> +#ifdef I915
> +	spin_lock(&i915->uncore.lock);
> +#endif
> +}
> +
> +static inline void intel_vblank_section_exit(struct drm_i915_private *i915)

and here

With these changes:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +{
> +#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 +322,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 +395,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 +434,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 +563,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 +572,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
> 

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

[Coelho, Luciano]

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@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Ville Syrj?l? <ville.syrjala@linux.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;
> > 
> >  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.


> >  enum drm_scaling_filter;
> >  struct dpll;
> > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> > index 2cec2abf9746..d9625db82681 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > @@ -265,6 +265,26 @@ 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.  Keep the decision-making centralized
> > + * here.
> 
> maybe we could add brief mention that it is only needed because old hardware
> that is not supported by Xe.

Good idea, I'll add it.

> 
> > + */
> > +static inline void intel_vblank_section_enter(struct drm_i915_private *i915)
> 
> let's avoid inline here.

Okay, I'll remove it.


> > +{
> > +#ifdef I915
> > +	spin_lock(&i915->uncore.lock);
> > +#endif
> > +}
> > +
> > +static inline void intel_vblank_section_exit(struct drm_i915_private *i915)
> 
> and here

Okay.


> With these changes:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks for the review, Rodrigo!

--
Cheers,
Luca.

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

[Coelho, Luciano]

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@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Ville Syrj?l? <ville.syrjala@linux.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;
> > 
> >  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.


> >  enum drm_scaling_filter;
> >  struct dpll;
> > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> > index 2cec2abf9746..d9625db82681 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > @@ -265,6 +265,26 @@ 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.  Keep the decision-making centralized
> > + * here.
> 
> maybe we could add brief mention that it is only needed because old hardware
> that is not supported by Xe.

Good idea, I'll add it.

> 
> > + */
> > +static inline void intel_vblank_section_enter(struct drm_i915_private *i915)
> 
> let's avoid inline here.

Okay, I'll remove it.


> > +{
> > +#ifdef I915
> > +	spin_lock(&i915->uncore.lock);
> > +#endif
> > +}
> > +
> > +static inline void intel_vblank_section_exit(struct drm_i915_private *i915)
> 
> and here

Okay.


> With these changes:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks for the review, Rodrigo!

--
Cheers,
Luca.

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

[Rodrigo Vivi]

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@intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Ville Syrj?l? <ville.syrjala@linux.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;
> > > 
> > >  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.

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.

> 
> 
> > >  enum drm_scaling_filter;
> > >  struct dpll;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> > > index 2cec2abf9746..d9625db82681 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > > @@ -265,6 +265,26 @@ 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.  Keep the decision-making centralized
> > > + * here.
> > 
> > maybe we could add brief mention that it is only needed because old hardware
> > that is not supported by Xe.
> 
> Good idea, I'll add it.
> 
> > 
> > > + */
> > > +static inline void intel_vblank_section_enter(struct drm_i915_private *i915)
> > 
> > let's avoid inline here.
> 
> Okay, I'll remove it.
> 
> 
> > > +{
> > > +#ifdef I915
> > > +	spin_lock(&i915->uncore.lock);
> > > +#endif
> > > +}
> > > +
> > > +static inline void intel_vblank_section_exit(struct drm_i915_private *i915)
> > 
> > and here
> 
> Okay.
> 
> 
> > With these changes:
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Thanks for the review, Rodrigo!
> 
> --
> Cheers,
> Luca.

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

[Rodrigo Vivi]

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@intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Ville Syrj?l? <ville.syrjala@linux.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;
> > > 
> > >  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.

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.

> 
> 
> > >  enum drm_scaling_filter;
> > >  struct dpll;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> > > index 2cec2abf9746..d9625db82681 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > > @@ -265,6 +265,26 @@ 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.  Keep the decision-making centralized
> > > + * here.
> > 
> > maybe we could add brief mention that it is only needed because old hardware
> > that is not supported by Xe.
> 
> Good idea, I'll add it.
> 
> > 
> > > + */
> > > +static inline void intel_vblank_section_enter(struct drm_i915_private *i915)
> > 
> > let's avoid inline here.
> 
> Okay, I'll remove it.
> 
> 
> > > +{
> > > +#ifdef I915
> > > +	spin_lock(&i915->uncore.lock);
> > > +#endif
> > > +}
> > > +
> > > +static inline void intel_vblank_section_exit(struct drm_i915_private *i915)
> > 
> > and here
> 
> Okay.
> 
> 
> > With these changes:
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Thanks for the review, Rodrigo!
> 
> --
> Cheers,
> Luca.

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

[Coelho, Luciano]

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@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Ville Syrj?l? <ville.syrjala@linux.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;
> > > > 
> > > >  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.

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

[Coelho, Luciano]

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@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Ville Syrj?l? <ville.syrjala@linux.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;
> > > > 
> > > >  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.