[2/2] drm/i915/skl: corrected csr mutex lock declaration.
diff mbox

Message ID 1431415928-21270-2-git-send-email-animesh.manna@intel.com
State New
Headers show

Commit Message

Animesh Manna May 12, 2015, 7:32 a.m. UTC
Specifically csr mutex lock is to protect csr-related data structures
so declaration moved intel_csr structure.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: A.Sunil Kamath <sunil.kamath@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(-)

Comments

Daniel Vetter May 12, 2015, 8:29 a.m. UTC | #1
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@intel.com>
> Signed-off-by: A.Sunil Kamath <sunil.kamath@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

> +
>  	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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Animesh Manna May 13, 2015, 3:40 p.m. UTC | #2
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@intel.com>
>> Signed-off-by: A.Sunil Kamath <sunil.kamath@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.

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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Shuang He May 15, 2015, 8:06 a.m. UTC | #3
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6385
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                 -1              314/314              313/314
IVB                                  338/338              338/338
BYT                                  286/286              286/286
BDW                 -1              320/320              319/320
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      DMESG_WARN(13)PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
*BDW  igt@gem_gtt_hog      PASS(4)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_display.c:#assert_plane[i915]()@WARNING:.* at .* assert_plane
assertion_failure@assertion failure
WARNING:at_drivers/gpu/drm/drm_irq.c:#drm_wait_one_vblank[drm]()@WARNING:.* at .* drm_wait_one_vblank+0x
Note: You need to pay more attention to line start with '*'
Daniel Vetter May 18, 2015, 7:12 a.m. UTC | #4
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@intel.com>
> >>Signed-off-by: A.Sunil Kamath <sunil.kamath@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@lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Patch
diff mbox

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;
+
 	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)