| Message ID | 20170904190806.29345-1-david.weinehall@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting David Weinehall (2017-09-04 20:08:06) > Currently we're doing: > > 1. acquire lock > 2. write word to hardware > 3. release lock > 4. repeat from 1 > > to load the DMC firmware. Due to the cost of acquiring/releasing a lock, > and the size of the DMC firmware, this slows down DMC loading a lot. > > This patch simply acquires the lock, writes the entire firmware, > then releases the lock. Testing shows resume speedups > in the order of 10ms on platforms with DMC firmware (GEN9+). > > v2: Per feedback from Chris & Ville there's no need to do the whole > forcewake dance, so lose that bit (Chris, Ville) > > v3: Actually send the new version of the patch... > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_csr.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index 965988f79a55..28ea24932ef1 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -240,6 +240,7 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv) > { > u32 *payload = dev_priv->csr.dmc_payload; > uint32_t i, fw_size; > + unsigned long flags; > > if (!HAS_CSR(dev_priv)) { > DRM_ERROR("No CSR support available for this platform\n"); > @@ -252,8 +253,13 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv) > } > > fw_size = dev_priv->csr.dmc_fw_size; > + assert_rpm_wakelock_held(dev_priv); > + spin_lock_irqsave(&dev_priv->uncore.lock, flags); > + > for (i = 0; i < fw_size; i++) > - I915_WRITE(CSR_PROGRAM(i), payload[i]); > + I915_WRITE_FW(CSR_PROGRAM(i), payload[i]); > + > + spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); Still would like to see the version without the uncore.lock. Afaict, there isn't a requirement here -- unless you are serialising between multiple users (concurrent intel_csr_load_program?) of CSR_PROGRAM. -Chris
Quoting Chris Wilson (2017-09-04 20:14:32) > Quoting David Weinehall (2017-09-04 20:08:06) > > Currently we're doing: > > > > 1. acquire lock > > 2. write word to hardware > > 3. release lock > > 4. repeat from 1 > > > > to load the DMC firmware. Due to the cost of acquiring/releasing a lock, > > and the size of the DMC firmware, this slows down DMC loading a lot. > > > > This patch simply acquires the lock, writes the entire firmware, > > then releases the lock. Testing shows resume speedups > > in the order of 10ms on platforms with DMC firmware (GEN9+). > > > > v2: Per feedback from Chris & Ville there's no need to do the whole > > forcewake dance, so lose that bit (Chris, Ville) > > > > v3: Actually send the new version of the patch... > > > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_csr.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > > index 965988f79a55..28ea24932ef1 100644 > > --- a/drivers/gpu/drm/i915/intel_csr.c > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > @@ -240,6 +240,7 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv) > > { > > u32 *payload = dev_priv->csr.dmc_payload; > > uint32_t i, fw_size; > > + unsigned long flags; > > > > if (!HAS_CSR(dev_priv)) { > > DRM_ERROR("No CSR support available for this platform\n"); > > @@ -252,8 +253,13 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv) > > } > > > > fw_size = dev_priv->csr.dmc_fw_size; > > + assert_rpm_wakelock_held(dev_priv); > > + spin_lock_irqsave(&dev_priv->uncore.lock, flags); > > + > > for (i = 0; i < fw_size; i++) > > - I915_WRITE(CSR_PROGRAM(i), payload[i]); > > + I915_WRITE_FW(CSR_PROGRAM(i), payload[i]); > > + > > + spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); > > Still would like to see the version without the uncore.lock. Afaict, > there isn't a requirement here -- unless you are serialising between > multiple users (concurrent intel_csr_load_program?) of CSR_PROGRAM. You may also want to consider a preempt_disable around this block as well, the argument being that we want the writes tightly grouped. -Chris
On Mon, Sep 04, 2017 at 08:15:57PM +0100, Chris Wilson wrote: > Quoting Chris Wilson (2017-09-04 20:14:32) > > Quoting David Weinehall (2017-09-04 20:08:06) > > > Currently we're doing: > > > > > > 1. acquire lock > > > 2. write word to hardware > > > 3. release lock > > > 4. repeat from 1 > > > > > > to load the DMC firmware. Due to the cost of acquiring/releasing a lock, > > > and the size of the DMC firmware, this slows down DMC loading a lot. > > > > > > This patch simply acquires the lock, writes the entire firmware, > > > then releases the lock. Testing shows resume speedups > > > in the order of 10ms on platforms with DMC firmware (GEN9+). > > > > > > v2: Per feedback from Chris & Ville there's no need to do the whole > > > forcewake dance, so lose that bit (Chris, Ville) > > > > > > v3: Actually send the new version of the patch... > > > > > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_csr.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > > > index 965988f79a55..28ea24932ef1 100644 > > > --- a/drivers/gpu/drm/i915/intel_csr.c > > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > > @@ -240,6 +240,7 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv) > > > { > > > u32 *payload = dev_priv->csr.dmc_payload; > > > uint32_t i, fw_size; > > > + unsigned long flags; > > > > > > if (!HAS_CSR(dev_priv)) { > > > DRM_ERROR("No CSR support available for this platform\n"); > > > @@ -252,8 +253,13 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv) > > > } > > > > > > fw_size = dev_priv->csr.dmc_fw_size; > > > + assert_rpm_wakelock_held(dev_priv); > > > + spin_lock_irqsave(&dev_priv->uncore.lock, flags); > > > + > > > for (i = 0; i < fw_size; i++) > > > - I915_WRITE(CSR_PROGRAM(i), payload[i]); > > > + I915_WRITE_FW(CSR_PROGRAM(i), payload[i]); > > > + > > > + spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); > > > > Still would like to see the version without the uncore.lock. Afaict, > > there isn't a requirement here -- unless you are serialising between > > multiple users (concurrent intel_csr_load_program?) of CSR_PROGRAM. > > You may also want to consider a preempt_disable around this block as > well, the argument being that we want the writes tightly grouped. > -Chris OK, I'll spin a non-spinning v4 tomorrow :) Kind regards, David
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 965988f79a55..28ea24932ef1 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -240,6 +240,7 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv) { u32 *payload = dev_priv->csr.dmc_payload; uint32_t i, fw_size; + unsigned long flags; if (!HAS_CSR(dev_priv)) { DRM_ERROR("No CSR support available for this platform\n"); @@ -252,8 +253,13 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv) } fw_size = dev_priv->csr.dmc_fw_size; + assert_rpm_wakelock_held(dev_priv); + spin_lock_irqsave(&dev_priv->uncore.lock, flags); + for (i = 0; i < fw_size; i++) - I915_WRITE(CSR_PROGRAM(i), payload[i]); + I915_WRITE_FW(CSR_PROGRAM(i), payload[i]); + + spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); for (i = 0; i < dev_priv->csr.mmio_count; i++) { I915_WRITE(dev_priv->csr.mmioaddr[i],
Currently we're doing: 1. acquire lock 2. write word to hardware 3. release lock 4. repeat from 1 to load the DMC firmware. Due to the cost of acquiring/releasing a lock, and the size of the DMC firmware, this slows down DMC loading a lot. This patch simply acquires the lock, writes the entire firmware, then releases the lock. Testing shows resume speedups in the order of 10ms on platforms with DMC firmware (GEN9+). v2: Per feedback from Chris & Ville there's no need to do the whole forcewake dance, so lose that bit (Chris, Ville) v3: Actually send the new version of the patch... Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> --- drivers/gpu/drm/i915/intel_csr.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)