[Intel-gfx] [PATCH 2/2] drm/i915/skl: corrected csr mutex lock declaration.

[Daniel Vetter]

On Wed, May 13, 2015 at 09:10:41PM +0530, Animesh Manna wrote:
> 
> 
> On 5/12/2015 1:59 PM, Daniel Vetter wrote:
> >On Tue, May 12, 2015 at 01:02:08PM +0530, Animesh Manna wrote:
> >>Specifically csr mutex lock is to protect csr-related data structures
> >>so declaration moved intel_csr structure.
> >>
> >>Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> >>Signed-off-by: A.Sunil Kamath <sunil.kamath at intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_dma.c  |  2 +-
> >>  drivers/gpu/drm/i915/i915_drv.h  |  6 +++---
> >>  drivers/gpu/drm/i915/intel_csr.c | 12 ++++++------
> >>  3 files changed, 10 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> >>index a238889..78e6ae8 100644
> >>--- a/drivers/gpu/drm/i915/i915_dma.c
> >>+++ b/drivers/gpu/drm/i915/i915_dma.c
> >>@@ -816,7 +816,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >>  	spin_lock_init(&dev_priv->mmio_flip_lock);
> >>  	mutex_init(&dev_priv->dpio_lock);
> >>  	mutex_init(&dev_priv->modeset_restore_lock);
> >>-	mutex_init(&dev_priv->csr_lock);
> >>+	mutex_init(&dev_priv->csr.csr_lock);
> >>  	intel_pm_setup(dev);
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index 136d42a..43011d7 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -676,6 +676,9 @@ enum csr_state {
> >>  };
> >>  struct intel_csr {
> >>+	/* CSR protection, used to protect firmware loading status: csr_state */
> >>+	struct mutex csr_lock;
> >csr_ prefix is redundant. But the real trouble is that essentially your
> >open-coding a completion using this mutex and csr->state. And that's just
> >confusing compared to using completions directly. Can you please replace
> >the usage of csr->state with a completion and remove the mutex entirely?
> >-Daniel
> 
> I will remove csr_ prefix. During initialization or suspend-resume scenario
> firmware data is written to CSR mmio registers and the completion success/failure
> need to capture which later is used to enable dc5/dc6. Not sure how to replace
> csr->state with completion (understood as returning true/false from a function).
> Agree as display initialization and suspend-resume is mutually exclusive mutex
> is not needed.

Ok instead of just looking at the mutex code I looked at the design
overall and we need to replace it. The following

	/* TODO: wait for a completion event or
	 * similar here instead of busy
	 * waiting using wait_for function.
	 */
	wait_for((state = intel_csr_load_status_get(dev_priv)) !=
			FW_UNINITIALIZED, 1000);

from skl_set_power_well really is not how things should work. This would
be the place that instead of the busys-loop would call
wait_for_completion(). No need to for dev_priv->csr.state at all afaics.

But digging deeper event the completion isn't necessary. The usual design
in i915 for delaying runtime pm until everything is ready is to grab an
additional reference for that power well until everything is set up. That
way there's no need for checks, no need for completions and depencies
become a lot simpler. I.e.
1) In the driver load code when you schedule the firmware loading work you
also grab an appropropriate display power well (the one that will prevent
going into dc5/6).
2) In the firmware load code you drop that power well reference _after_
everything has been set up. No locking needed at all, the power well code
has it's own locks (power_domains->lock).
3) csr->state & csr_lock and all related code can be completely removed.

Cheers, Daniel

> 
> Regards,
> Animesh
> 
> >
> >>+
> >>  	const char *fw_path;
> >>  	__be32 *dmc_payload;
> >>  	uint32_t dmc_fw_size;
> >>@@ -1592,9 +1595,6 @@ struct drm_i915_private {
> >>  	struct intel_csr csr;
> >>-	/* Display CSR-related protection */
> >>-	struct mutex csr_lock;
> >>-
> >>  	struct intel_gmbus gmbus[GMBUS_NUM_PINS];
> >>  	/** gmbus_mutex protects against concurrent usage of the single hw gmbus
> >>diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> >>index 174106f..b03b9b3 100644
> >>--- a/drivers/gpu/drm/i915/intel_csr.c
> >>+++ b/drivers/gpu/drm/i915/intel_csr.c
> >>@@ -195,9 +195,9 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv)
> >>  {
> >>  	enum csr_state state;
> >>-	mutex_lock(&dev_priv->csr_lock);
> >>+	mutex_lock(&dev_priv->csr.csr_lock);
> >>  	state = dev_priv->csr.state;
> >>-	mutex_unlock(&dev_priv->csr_lock);
> >>+	mutex_unlock(&dev_priv->csr.csr_lock);
> >>  	return state;
> >>  }
> >>@@ -212,9 +212,9 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv)
> >>  void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
> >>  			enum csr_state state)
> >>  {
> >>-	mutex_lock(&dev_priv->csr_lock);
> >>+	mutex_lock(&dev_priv->csr.csr_lock);
> >>  	dev_priv->csr.state = state;
> >>-	mutex_unlock(&dev_priv->csr_lock);
> >>+	mutex_unlock(&dev_priv->csr.csr_lock);
> >>  }
> >>  /**
> >>@@ -236,7 +236,7 @@ void intel_csr_load_program(struct drm_device *dev)
> >>  		return;
> >>  	}
> >>-	mutex_lock(&dev_priv->csr_lock);
> >>+	mutex_lock(&dev_priv->csr.csr_lock);
> >>  	fw_size = dev_priv->csr.dmc_fw_size;
> >>  	for (i = 0; i < fw_size; i++)
> >>  		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
> >>@@ -248,7 +248,7 @@ void intel_csr_load_program(struct drm_device *dev)
> >>  	}
> >>  	dev_priv->csr.state = FW_LOADED;
> >>-	mutex_unlock(&dev_priv->csr_lock);
> >>+	mutex_unlock(&dev_priv->csr.csr_lock);
> >>  }
> >>  static void finish_csr_load(const struct firmware *fw, void *context)
> >>-- 
> >>2.0.2
> >>
> >>_______________________________________________
> >>Intel-gfx mailing list
> >>Intel-gfx at lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch