[Intel-gfx] [PATCH 31/49] drm/i915: Reduce locking in execlist command submission
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Mar 27 04:40:12 PDT 2015
Hi,
On 03/27/2015 11:02 AM, Chris Wilson wrote:
> This eliminates six needless spin lock/unlock pairs when writing out
> ELSP.
>
> v2: Respin with my preferred colour.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk> [v2]
> ---
> drivers/gpu/drm/i915/i915_drv.h | 18 ++++++++
> drivers/gpu/drm/i915/intel_lrc.c | 14 +++---
> drivers/gpu/drm/i915/intel_uncore.c | 86 ++++++++++++++++++++++++++-----------
> 3 files changed, 86 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0c6e4356fa06..4b51169c37ea 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2540,6 +2540,13 @@ void intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
> enum forcewake_domains domains);
> void intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
> enum forcewake_domains domains);
> +/* Like above but take and hold the uncore lock for the duration.
> + * Must be used with I915_READ_FW and friends.
> + */
> +void intel_uncore_forcewake_irqlock(struct drm_i915_private *dev_priv,
> + enum forcewake_domains domains);
> +void intel_uncore_forcewake_irqunlock(struct drm_i915_private *dev_priv,
> + enum forcewake_domains domains);
Oh well I don't like your colour. :)
I would make the comment clearer in saying the function itself will take
the lock and not release it since "take and hold the uncore lock for the
duration" to me reads ambiguous.
Also, not sure about the _irqlock suffix. It is well established in
spinlocks and the functions even does the opposite from that!
Maybe _get_and_lock / _put_and_unlock, or other way round?
> void assert_forcewakes_inactive(struct drm_i915_private *dev_priv);
> static inline bool intel_vgpu_active(struct drm_device *dev)
> {
> @@ -3232,6 +3239,17 @@ int intel_freq_opcode(struct drm_i915_private *dev_priv, int val);
> #define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg)
> #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg)
>
> +/* These are untraced mmio-accessors that are only valid to be used inside
> + * criticial sections inside IRQ handlers where forcewake is explicitly
> + * controlled.
> + * Think twice, and think again, before using these.
> + * Note: Should only be used between intel_uncore_forcewake_irqlock() and
> + * intel_uncore_forcewake_irqunlock().
> + */
> +#define I915_READ_FW(reg__) readl(dev_priv->regs + (reg__))
> +#define I915_WRITE_FW(reg__, val__) writel(val__, dev_priv->regs + (reg__))
> +#define POSTING_READ_FW(reg__) (void)I915_READ_FW(reg__)
> +
> /* "Broadcast RGB" property */
> #define INTEL_BROADCAST_RGB_AUTO 0
> #define INTEL_BROADCAST_RGB_FULL 1
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 5e51ed5232e8..454bb7df27fe 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -278,17 +278,17 @@ static void execlists_submit_pair(struct intel_engine_cs *ring)
> desc[3] = ring->execlist_port[0]->seqno;
>
> /* Note: You must always write both descriptors in the order below. */
> - intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> - I915_WRITE(RING_ELSP(ring), desc[1]);
> - I915_WRITE(RING_ELSP(ring), desc[0]);
> - I915_WRITE(RING_ELSP(ring), desc[3]);
> + intel_uncore_forcewake_irqlock(dev_priv, FORCEWAKE_ALL);
> + I915_WRITE_FW(RING_ELSP(ring), desc[1]);
> + I915_WRITE_FW(RING_ELSP(ring), desc[0]);
> + I915_WRITE_FW(RING_ELSP(ring), desc[3]);
>
> /* The context is automatically loaded after the following */
> - I915_WRITE(RING_ELSP(ring), desc[2]);
> + I915_WRITE_FW(RING_ELSP(ring), desc[2]);
>
> /* ELSP is a wo register, use another nearby reg for posting instead */
> - POSTING_READ(RING_EXECLIST_STATUS(ring));
> - intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> + POSTING_READ_FW(RING_EXECLIST_STATUS(ring));
> + intel_uncore_forcewake_irqunlock(dev_priv, FORCEWAKE_ALL);
> }
>
> static void execlists_context_unqueue(struct intel_engine_cs *ring)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 0e32bbbcada8..a063f7d9f31b 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -399,6 +399,26 @@ void intel_uncore_sanitize(struct drm_device *dev)
> intel_disable_gt_powersave(dev);
> }
>
> +static void __intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
> + enum forcewake_domains fw_domains)
> +{
> + struct intel_uncore_forcewake_domain *domain;
> + enum forcewake_domain_id id;
> +
> + if (!dev_priv->uncore.funcs.force_wake_get)
> + return;
> +
> + fw_domains &= dev_priv->uncore.fw_domains;
> +
> + for_each_fw_domain_mask(domain, fw_domains, dev_priv, id) {
> + if (domain->wake_count++)
> + fw_domains &= ~(1 << id);
> + }
> +
> + if (fw_domains)
> + dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
> +}
> +
> /**
> * intel_uncore_forcewake_get - grab forcewake domain references
> * @dev_priv: i915 device instance
> @@ -416,41 +436,30 @@ void intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
> enum forcewake_domains fw_domains)
> {
> unsigned long irqflags;
> - struct intel_uncore_forcewake_domain *domain;
> - enum forcewake_domain_id id;
>
> if (!dev_priv->uncore.funcs.force_wake_get)
> return;
>
> WARN_ON(dev_priv->pm.suspended);
>
> - fw_domains &= dev_priv->uncore.fw_domains;
> -
> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> + __intel_uncore_forcewake_get(dev_priv, fw_domains);
> + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +}
>
> - for_each_fw_domain_mask(domain, fw_domains, dev_priv, id) {
> - if (domain->wake_count++)
> - fw_domains &= ~(1 << id);
> - }
> -
> - if (fw_domains)
> - dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
> +void intel_uncore_forcewake_irqlock(struct drm_i915_private *dev_priv,
> + enum forcewake_domains fw_domains)
> +{
And kerneldoc probably if we are finalising this.
> + if (!dev_priv->uncore.funcs.force_wake_get)
> + return;
>
> - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> + spin_lock(&dev_priv->uncore.lock);
> + __intel_uncore_forcewake_get(dev_priv, fw_domains);
> }
So this, why plain spin_lock? It can only be called from irq context now
but the comment does not say that and there aren't any assert (if they
are even possible nowadays).
>
> -/**
> - * intel_uncore_forcewake_put - release a forcewake domain reference
> - * @dev_priv: i915 device instance
> - * @fw_domains: forcewake domains to put references
> - *
> - * This function drops the device-level forcewakes for specified
> - * domains obtained by intel_uncore_forcewake_get().
> - */
> -void intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
> - enum forcewake_domains fw_domains)
> +static void __intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
> + enum forcewake_domains fw_domains)
> {
> - unsigned long irqflags;
> struct intel_uncore_forcewake_domain *domain;
> enum forcewake_domain_id id;
>
> @@ -459,8 +468,6 @@ void intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
>
> fw_domains &= dev_priv->uncore.fw_domains;
>
> - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -
> for_each_fw_domain_mask(domain, fw_domains, dev_priv, id) {
> if (WARN_ON(domain->wake_count == 0))
> continue;
> @@ -471,10 +478,39 @@ void intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
> domain->wake_count++;
> fw_domain_arm_timer(domain);
> }
> +}
> +
> +/**
> + * intel_uncore_forcewake_put - release a forcewake domain reference
> + * @dev_priv: i915 device instance
> + * @fw_domains: forcewake domains to put references
> + *
> + * This function drops the device-level forcewakes for specified
> + * domains obtained by intel_uncore_forcewake_get().
> + */
> +void intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
> + enum forcewake_domains fw_domains)
> +{
> + unsigned long irqflags;
> +
> + if (!dev_priv->uncore.funcs.force_wake_put)
> + return;
>
> + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> + __intel_uncore_forcewake_put(dev_priv, fw_domains);
> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> }
>
> +void intel_uncore_forcewake_irqunlock(struct drm_i915_private *dev_priv,
> + enum forcewake_domains fw_domains)
> +{
> + if (!dev_priv->uncore.funcs.force_wake_put)
> + return;
> +
> + __intel_uncore_forcewake_put(dev_priv, fw_domains);
> + spin_unlock(&dev_priv->uncore.lock);
> +}
> +
> void assert_forcewakes_inactive(struct drm_i915_private *dev_priv)
> {
> struct intel_uncore_forcewake_domain *domain;
>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list