[PATCH 1/2] drm/i915/skl: Documentation for CSR firmware.

[PATCH 1/2] drm/i915/skl: Documentation for CSR firmware.

[Animesh Manna]

Added docbook info regarding context save and restore (CSR)
firmware support added from gen9 onwards to drive newly added
DMC (Display microcontroller) in display engine.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>
---
 Documentation/DocBook/drm.tmpl   | 22 +++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_csr.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 7c68ecc..217b4fc 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -4153,6 +4153,27 @@ int num_ioctls;</synopsis>
 	  </tgroup>
 	</table>
       </sect2>
+
+      <sect2>
+       <title>CSR firmware support for DMC</title>
+       <para>
+     Display Context Save and Restore (CSR) firmware support added from gen9
+     onwards to drive newly added DMC (Display microcontroller) in display
+     engine to save and restore the state of display engine when it enter into
+     low-power state and comes back to nomral. The above API are used to manage
+     the firmware loading process.
+       </para>
+       <para>
+     Firmware loading status will be one of the below states: FW_UNINITIALIZED,
+     FW_LOADED, FW_FAILED.
+       </para>
+       <para>
+     Once the firmware is written into the registers status will be moved from
+     FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
+     be moved to FW_FAILED.
+       </para>
+!Idrivers/gpu/drm/i915/intel_csr.c
+      </sect2>
     </sect1>
 
     <sect1>
@@ -4204,7 +4225,6 @@ int num_ioctls;</synopsis>
 !Idrivers/gpu/drm/i915/i915_gem_shrinker.c
       </sect2>
     </sect1>
-
     <sect1>
       <title> Tracing </title>
       <para>
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 9311cdd..174106f 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -183,6 +183,14 @@ static char intel_get_substepping(struct drm_device *dev)
 		return -ENODATA;
 }
 
+/**
+ * intel_csr_load_status_get() - to get firmware loading status.
+ * @dev_priv: i915 device.
+ *
+ * This function helps to get the firmware loading status.
+ *
+ * Return: Firmware loading status.
+ */
 enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv)
 {
 	enum csr_state state;
@@ -194,6 +202,13 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv)
 	return state;
 }
 
+/**
+ * intel_csr_load_status_set() - help to set firmware loading status.
+ * @dev_priv: i915 device.
+ * @state: enumeration of firmware loading status.
+ *
+ * Set the firmware loading status.
+ */
 void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
 			enum csr_state state)
 {
@@ -202,6 +217,14 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
 	mutex_unlock(&dev_priv->csr_lock);
 }
 
+/**
+ * intel_csr_load_program() - write the firmware from memory to register.
+ * @dev: drm device.
+ *
+ * CSR firmware is read from a .bin file and kept in internal memory one time.
+ * Everytime display comes back from low power state this function is called to
+ * copy the firmware from internal memory to registers.
+ */
 void intel_csr_load_program(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -359,6 +382,13 @@ out:
 	release_firmware(fw);
 }
 
+/**
+ * intel_csr_ucode_init() - initialize the firmware loading.
+ * @dev: drm device.
+ *
+ * This function is called at the time of loading the display driver to read
+ * firmware from a .bin file and copied into a internal memory.
+ */
 void intel_csr_ucode_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -393,6 +423,13 @@ void intel_csr_ucode_init(struct drm_device *dev)
 	}
 }
 
+/**
+ * intel_csr_ucode_fini() - unload the CSR firmware.
+ * @dev: drm device.
+ *
+ * Firmmware unloading includes freeing the internal momory and reset the
+ * firmware loading status.
+ */
 void intel_csr_ucode_fini(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
2.0.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Animesh Manna]

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

Re: [PATCH 2/2] drm/i915/skl: corrected csr mutex lock declaration.

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/skl: Documentation for CSR firmware.

[Daniel Vetter]

On Tue, May 12, 2015 at 01:02:07PM +0530, Animesh Manna wrote:
> Added docbook info regarding context save and restore (CSR)
> firmware support added from gen9 onwards to drive newly added
> DMC (Display microcontroller) in display engine.
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>
> ---
>  Documentation/DocBook/drm.tmpl   | 22 +++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_csr.c | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 7c68ecc..217b4fc 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -4153,6 +4153,27 @@ int num_ioctls;</synopsis>
>  	  </tgroup>
>  	</table>
>        </sect2>
> +
> +      <sect2>
> +       <title>CSR firmware support for DMC</title>
> +       <para>
> +     Display Context Save and Restore (CSR) firmware support added from gen9
> +     onwards to drive newly added DMC (Display microcontroller) in display
> +     engine to save and restore the state of display engine when it enter into
> +     low-power state and comes back to nomral. The above API are used to manage
> +     the firmware loading process.
> +       </para>
> +       <para>
> +     Firmware loading status will be one of the below states: FW_UNINITIALIZED,
> +     FW_LOADED, FW_FAILED.
> +       </para>
> +       <para>
> +     Once the firmware is written into the registers status will be moved from
> +     FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
> +     be moved to FW_FAILED.

These kind of overview comments are best put into a DOC: section in the
sourcefiles and then pulled in like the function docs. There's lots of
examples for this in i915 already.
-Daniel

> +       </para>
> +!Idrivers/gpu/drm/i915/intel_csr.c
> +      </sect2>
>      </sect1>
>  
>      <sect1>
> @@ -4204,7 +4225,6 @@ int num_ioctls;</synopsis>
>  !Idrivers/gpu/drm/i915/i915_gem_shrinker.c
>        </sect2>
>      </sect1>
> -
>      <sect1>
>        <title> Tracing </title>
>        <para>
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 9311cdd..174106f 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -183,6 +183,14 @@ static char intel_get_substepping(struct drm_device *dev)
>  		return -ENODATA;
>  }
>  
> +/**
> + * intel_csr_load_status_get() - to get firmware loading status.
> + * @dev_priv: i915 device.
> + *
> + * This function helps to get the firmware loading status.
> + *
> + * Return: Firmware loading status.
> + */
>  enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv)
>  {
>  	enum csr_state state;
> @@ -194,6 +202,13 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv)
>  	return state;
>  }
>  
> +/**
> + * intel_csr_load_status_set() - help to set firmware loading status.
> + * @dev_priv: i915 device.
> + * @state: enumeration of firmware loading status.
> + *
> + * Set the firmware loading status.
> + */
>  void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
>  			enum csr_state state)
>  {
> @@ -202,6 +217,14 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
>  	mutex_unlock(&dev_priv->csr_lock);
>  }
>  
> +/**
> + * intel_csr_load_program() - write the firmware from memory to register.
> + * @dev: drm device.
> + *
> + * CSR firmware is read from a .bin file and kept in internal memory one time.
> + * Everytime display comes back from low power state this function is called to
> + * copy the firmware from internal memory to registers.
> + */
>  void intel_csr_load_program(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -359,6 +382,13 @@ out:
>  	release_firmware(fw);
>  }
>  
> +/**
> + * intel_csr_ucode_init() - initialize the firmware loading.
> + * @dev: drm device.
> + *
> + * This function is called at the time of loading the display driver to read
> + * firmware from a .bin file and copied into a internal memory.
> + */
>  void intel_csr_ucode_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -393,6 +423,13 @@ void intel_csr_ucode_init(struct drm_device *dev)
>  	}
>  }
>  
> +/**
> + * intel_csr_ucode_fini() - unload the CSR firmware.
> + * @dev: drm device.
> + *
> + * Firmmware unloading includes freeing the internal momory and reset the
> + * firmware loading status.
> + */
>  void intel_csr_ucode_fini(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/skl: corrected csr mutex lock declaration.

[Animesh Manna]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/2] drm/i915/skl: Documentation for CSR firmware

[Animesh Manna]

Added docbook info regarding context save and restore (CSR)
firmware support added from gen9 onwards to drive newly added
DMC (Display microcontroller) in display engine.

v1: Initial version as RFC.

v2: Used "DOC:" tag for csr description based on review comment from Daniel.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>
---
 Documentation/DocBook/drm.tmpl   |  7 +++++-
 drivers/gpu/drm/i915/intel_csr.c | 53 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 7c68ecc..b6fc354 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -4153,6 +4153,12 @@ int num_ioctls;</synopsis>
 	  </tgroup>
 	</table>
       </sect2>
+
+      <sect2>
+       <title>CSR firmware support for DMC</title>
+!Pdrivers/gpu/drm/i915/intel_csr.c csr support for dmc
+!Idrivers/gpu/drm/i915/intel_csr.c
+      </sect2>
     </sect1>
 
     <sect1>
@@ -4204,7 +4210,6 @@ int num_ioctls;</synopsis>
 !Idrivers/gpu/drm/i915/i915_gem_shrinker.c
       </sect2>
     </sect1>
-
     <sect1>
       <title> Tracing </title>
       <para>
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 9311cdd..5cb8cc1 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -25,6 +25,22 @@
 #include "i915_drv.h"
 #include "i915_reg.h"
 
+/**
+ * DOC: csr support for dmc
+ *
+ * Display Context Save and Restore (CSR) firmware support added from gen9
+ * onwards to drive newly added DMC (Display microcontroller) in display
+ * engine to save and restore the state of display engine when it enter into
+ * low-power state and comes back to normal.
+ *
+ * Firmware loading status will be one of the below states: FW_UNINITIALIZED,
+ * FW_LOADED, FW_FAILED.
+ *
+ * Once the firmware is written into the registers status will be moved from
+ * FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
+ * be moved to FW_FAILED.
+ */
+
 #define I915_CSR_SKL "i915/skl_dmc_ver4.bin"
 
 MODULE_FIRMWARE(I915_CSR_SKL);
@@ -183,6 +199,14 @@ static char intel_get_substepping(struct drm_device *dev)
 		return -ENODATA;
 }
 
+/**
+ * intel_csr_load_status_get() - to get firmware loading status.
+ * @dev_priv: i915 device.
+ *
+ * This function helps to get the firmware loading status.
+ *
+ * Return: Firmware loading status.
+ */
 enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv)
 {
 	enum csr_state state;
@@ -194,6 +218,13 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv)
 	return state;
 }
 
+/**
+ * intel_csr_load_status_set() - help to set firmware loading status.
+ * @dev_priv: i915 device.
+ * @state: enumeration of firmware loading status.
+ *
+ * Set the firmware loading status.
+ */
 void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
 			enum csr_state state)
 {
@@ -202,6 +233,14 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
 	mutex_unlock(&dev_priv->csr_lock);
 }
 
+/**
+ * intel_csr_load_program() - write the firmware from memory to register.
+ * @dev: drm device.
+ *
+ * CSR firmware is read from a .bin file and kept in internal memory one time.
+ * Everytime display comes back from low power state this function is called to
+ * copy the firmware from internal memory to registers.
+ */
 void intel_csr_load_program(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -359,6 +398,13 @@ out:
 	release_firmware(fw);
 }
 
+/**
+ * intel_csr_ucode_init() - initialize the firmware loading.
+ * @dev: drm device.
+ *
+ * This function is called at the time of loading the display driver to read
+ * firmware from a .bin file and copied into a internal memory.
+ */
 void intel_csr_ucode_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -393,6 +439,13 @@ void intel_csr_ucode_init(struct drm_device *dev)
 	}
 }
 
+/**
+ * intel_csr_ucode_fini() - unload the CSR firmware.
+ * @dev: drm device.
+ *
+ * Firmmware unloading includes freeing the internal momory and reset the
+ * firmware loading status.
+ */
 void intel_csr_ucode_fini(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
2.0.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/skl: corrected csr mutex lock declaration.

[shuang.he]

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 '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915/skl: Documentation for CSR firmware

[Daniel Vetter]

On Wed, May 13, 2015 at 10:13:29PM +0530, Animesh Manna wrote:
> Added docbook info regarding context save and restore (CSR)
> firmware support added from gen9 onwards to drive newly added
> DMC (Display microcontroller) in display engine.
> 
> v1: Initial version as RFC.
> 
> v2: Used "DOC:" tag for csr description based on review comment from Daniel.
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  Documentation/DocBook/drm.tmpl   |  7 +++++-
>  drivers/gpu/drm/i915/intel_csr.c | 53 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 7c68ecc..b6fc354 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -4153,6 +4153,12 @@ int num_ioctls;</synopsis>
>  	  </tgroup>
>  	</table>
>        </sect2>
> +
> +      <sect2>
> +       <title>CSR firmware support for DMC</title>
> +!Pdrivers/gpu/drm/i915/intel_csr.c csr support for dmc
> +!Idrivers/gpu/drm/i915/intel_csr.c
> +      </sect2>
>      </sect1>
>  
>      <sect1>
> @@ -4204,7 +4210,6 @@ int num_ioctls;</synopsis>
>  !Idrivers/gpu/drm/i915/i915_gem_shrinker.c
>        </sect2>
>      </sect1>
> -
>      <sect1>
>        <title> Tracing </title>
>        <para>
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 9311cdd..5cb8cc1 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -25,6 +25,22 @@
>  #include "i915_drv.h"
>  #include "i915_reg.h"
>  
> +/**
> + * DOC: csr support for dmc
> + *
> + * Display Context Save and Restore (CSR) firmware support added from gen9
> + * onwards to drive newly added DMC (Display microcontroller) in display
> + * engine to save and restore the state of display engine when it enter into
> + * low-power state and comes back to normal.
> + *
> + * Firmware loading status will be one of the below states: FW_UNINITIALIZED,
> + * FW_LOADED, FW_FAILED.
> + *
> + * Once the firmware is written into the registers status will be moved from
> + * FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
> + * be moved to FW_FAILED.
> + */
> +
>  #define I915_CSR_SKL "i915/skl_dmc_ver4.bin"
>  
>  MODULE_FIRMWARE(I915_CSR_SKL);
> @@ -183,6 +199,14 @@ static char intel_get_substepping(struct drm_device *dev)
>  		return -ENODATA;
>  }
>  
> +/**
> + * intel_csr_load_status_get() - to get firmware loading status.
> + * @dev_priv: i915 device.
> + *
> + * This function helps to get the firmware loading status.
> + *
> + * Return: Firmware loading status.
> + */
>  enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv)
>  {
>  	enum csr_state state;
> @@ -194,6 +218,13 @@ enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv)
>  	return state;
>  }
>  
> +/**
> + * intel_csr_load_status_set() - help to set firmware loading status.
> + * @dev_priv: i915 device.
> + * @state: enumeration of firmware loading status.
> + *
> + * Set the firmware loading status.
> + */
>  void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
>  			enum csr_state state)
>  {
> @@ -202,6 +233,14 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
>  	mutex_unlock(&dev_priv->csr_lock);
>  }
>  
> +/**
> + * intel_csr_load_program() - write the firmware from memory to register.
> + * @dev: drm device.
> + *
> + * CSR firmware is read from a .bin file and kept in internal memory one time.
> + * Everytime display comes back from low power state this function is called to
> + * copy the firmware from internal memory to registers.
> + */
>  void intel_csr_load_program(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -359,6 +398,13 @@ out:
>  	release_firmware(fw);
>  }
>  
> +/**
> + * intel_csr_ucode_init() - initialize the firmware loading.
> + * @dev: drm device.
> + *
> + * This function is called at the time of loading the display driver to read
> + * firmware from a .bin file and copied into a internal memory.
> + */
>  void intel_csr_ucode_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -393,6 +439,13 @@ void intel_csr_ucode_init(struct drm_device *dev)
>  	}
>  }
>  
> +/**
> + * intel_csr_ucode_fini() - unload the CSR firmware.
> + * @dev: drm device.
> + *
> + * Firmmware unloading includes freeing the internal momory and reset the
> + * firmware loading status.
> + */
>  void intel_csr_ucode_fini(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx