[Intel-gfx] [PATCH] drm/i915: Add intel_pcode_probe

[Intel-gfx] [PATCH] drm/i915: Add intel_pcode_probe

[Sujaritha Sundaresan]

Added intel_pcode_probe, promoted wait for lmem init and
intel_pcode_init prior to mmio_probe during load,
so that GT registers can be accessed only after this, else MCA
is observed.

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c  | 37 ++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_uncore.c | 12 ----------
 2 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index f8dbee7a5af7..92cafceaf447 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -93,6 +93,7 @@
 #include "i915_memcpy.h"
 #include "i915_perf.h"
 #include "i915_query.h"
+#include "i915_reg.h"
 #include "i915_suspend.h"
 #include "i915_switcheroo.h"
 #include "i915_sysfs.h"
@@ -436,6 +437,32 @@ static int i915_pcode_init(struct drm_i915_private *i915)
 	return 0;
 }
 
+static int intel_pcode_probe(struct drm_i915_private *i915)
+{
+	struct intel_uncore *uncore;
+	int ret;
+
+	/*
+	 * The boot firmware initializes local memory and assesses its health.
+	 * If memory training fails, the punit will have been instructed to
+	 * keep the GT powered down; we won't be able to communicate with it
+	 * and we should not continue with driver initialization.
+	 */
+	if (IS_DGFX(i915) &&
+		!(__raw_uncore_read32(uncore, GU_CNTL) & LMEM_INIT)) {
+		drm_err(&i915->drm, "LMEM not initialized by firmware\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Driver handshakes with pcode via mailbox command to know that SoC
+	 * initialization is complete before proceeding further
+	 */
+	ret = i915_pcode_init(i915);
+
+	return ret;
+}
+
 /**
  * i915_driver_hw_probe - setup state requiring device access
  * @dev_priv: device private
@@ -547,10 +574,6 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
 
 	intel_opregion_setup(dev_priv);
 
-	ret = i915_pcode_init(dev_priv);
-	if (ret)
-		goto err_opregion;
-
 	/*
 	 * Fill the dram structure to get the system dram info. This will be
 	 * used for memory latency calculation.
@@ -561,8 +584,6 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
 
 	return 0;
 
-err_opregion:
-	intel_opregion_cleanup(dev_priv);
 err_msi:
 	if (pdev->msi_enabled)
 		pci_disable_msi(pdev);
@@ -778,6 +799,10 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret < 0)
 		goto out_runtime_pm_put;
 
+	ret = intel_pcode_probe(i915);
+	if (ret)
+		goto out_tiles_cleanup;
+
 	ret = i915_driver_mmio_probe(i915);
 	if (ret < 0)
 		goto out_tiles_cleanup;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index dfefad5a5fec..4a353d4adf86 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -2658,18 +2658,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 	if (ret)
 		return ret;
 
-	/*
-	 * The boot firmware initializes local memory and assesses its health.
-	 * If memory training fails, the punit will have been instructed to
-	 * keep the GT powered down; we won't be able to communicate with it
-	 * and we should not continue with driver initialization.
-	 */
-	if (IS_DGFX(i915) &&
-	    !(__raw_uncore_read32(uncore, GU_CNTL) & LMEM_INIT)) {
-		drm_err(&i915->drm, "LMEM not initialized by firmware\n");
-		return -ENODEV;
-	}
-
 	if (GRAPHICS_VER(i915) > 5 && !intel_vgpu_active(i915))
 		uncore->flags |= UNCORE_HAS_FORCEWAKE;
 
-- 
2.41.0

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add intel_pcode_probe

[Patchwork]

== Series Details ==

Series: drm/i915: Add intel_pcode_probe
URL   : https://patchwork.freedesktop.org/series/122610/
State : warning

== Summary ==

Error: dim checkpatch failed
/home/kbuild2/linux/maintainer-tools/dim: line 50: /home/kbuild2/.dimrc: No such file or directory

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: Add intel_pcode_probe

[Patchwork]

== Series Details ==

Series: drm/i915: Add intel_pcode_probe
URL   : https://patchwork.freedesktop.org/series/122610/
State : warning

== Summary ==

Error: dim sparse failed
/home/kbuild/linux/maintainer-tools/dim: line 50: /home/kbuild/.dimrc: No such file or directory

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Add intel_pcode_probe

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 5401 bytes --]

== Series Details ==

Series: drm/i915: Add intel_pcode_probe
URL   : https://patchwork.freedesktop.org/series/122610/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13534 -> Patchwork_122610v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_122610v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_122610v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122610v1/index.html

Participating hosts (39 -> 38)
------------------------------

  Additional (1): bat-dg2-8 
  Missing    (2): fi-cfl-8700k fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_122610v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - bat-dg1-5:          [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13534/bat-dg1-5/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122610v1/bat-dg1-5/igt@i915_module_load@load.html
    - bat-dg2-11:         [PASS][3] -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13534/bat-dg2-11/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122610v1/bat-dg2-11/igt@i915_module_load@load.html
    - bat-dg2-9:          [PASS][5] -> [ABORT][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13534/bat-dg2-9/igt@i915_module_load@load.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122610v1/bat-dg2-9/igt@i915_module_load@load.html
    - bat-dg2-8:          NOTRUN -> [ABORT][7]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122610v1/bat-dg2-8/igt@i915_module_load@load.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_module_load@load:
    - {bat-dg2-13}:       [DMESG-WARN][8] ([Intel XE#486] / [i915#8879]) -> [ABORT][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13534/bat-dg2-13/igt@i915_module_load@load.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122610v1/bat-dg2-13/igt@i915_module_load@load.html
    - {bat-dg2-14}:       [DMESG-WARN][10] ([Intel XE#486] / [i915#8879]) -> [ABORT][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13534/bat-dg2-14/igt@i915_module_load@load.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122610v1/bat-dg2-14/igt@i915_module_load@load.html

  
Known issues
------------

  Here are the changes found in Patchwork_122610v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@mman:
    - bat-rpls-2:         [PASS][12] -> [TIMEOUT][13] ([i915#6794] / [i915#7392])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13534/bat-rpls-2/igt@i915_selftest@live@mman.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122610v1/bat-rpls-2/igt@i915_selftest@live@mman.html

  * igt@i915_suspend@basic-s2idle-without-i915:
    - bat-rpls-2:         [PASS][14] -> [WARN][15] ([i915#8747])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13534/bat-rpls-2/igt@i915_suspend@basic-s2idle-without-i915.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122610v1/bat-rpls-2/igt@i915_suspend@basic-s2idle-without-i915.html

  
#### Warnings ####

  * igt@kms_psr@sprite_plane_onoff:
    - bat-rplp-1:         [SKIP][16] ([i915#1072]) -> [ABORT][17] ([i915#8442] / [i915#8668] / [i915#8712])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13534/bat-rplp-1/igt@kms_psr@sprite_plane_onoff.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122610v1/bat-rplp-1/igt@kms_psr@sprite_plane_onoff.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [Intel XE#486]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/486
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#6794]: https://gitlab.freedesktop.org/drm/intel/issues/6794
  [i915#7392]: https://gitlab.freedesktop.org/drm/intel/issues/7392
  [i915#8442]: https://gitlab.freedesktop.org/drm/intel/issues/8442
  [i915#8668]: https://gitlab.freedesktop.org/drm/intel/issues/8668
  [i915#8712]: https://gitlab.freedesktop.org/drm/intel/issues/8712
  [i915#8747]: https://gitlab.freedesktop.org/drm/intel/issues/8747
  [i915#8879]: https://gitlab.freedesktop.org/drm/intel/issues/8879


Build changes
-------------

  * Linux: CI_DRM_13534 -> Patchwork_122610v1

  CI-20190529: 20190529
  CI_DRM_13534: 2c5542cb377b2e9e15c1f84a6e78a3d6fca47ea0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7443: 953448dbf2e63918a8eced9707f65fc0a19a9c85 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_122610v1: 2c5542cb377b2e9e15c1f84a6e78a3d6fca47ea0 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

77b0510ba212 drm/i915: Add intel_pcode_probe

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_122610v1/index.html

[-- Attachment #2: Type: text/html, Size: 6355 bytes --]

Re: [Intel-gfx] [PATCH] drm/i915: Add intel_pcode_probe

[Gupta, Anshuman]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Sujaritha Sundaresan
> Sent: Friday, August 18, 2023 8:16 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH] drm/i915: Add intel_pcode_probe
> 
> Added intel_pcode_probe, promoted wait for lmem init and intel_pcode_init
> prior to mmio_probe during load, so that GT registers can be accessed only
> after this, else MCA is observed.
> 
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Both DG1 and DG2 crashed during i915_pci_probe.
BAT is failing.
Thanks,
Anshuman Gupta. 
> ---
>  drivers/gpu/drm/i915/i915_driver.c  | 37 ++++++++++++++++++++++++-----
> drivers/gpu/drm/i915/intel_uncore.c | 12 ----------
>  2 files changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index f8dbee7a5af7..92cafceaf447 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -93,6 +93,7 @@
>  #include "i915_memcpy.h"
>  #include "i915_perf.h"
>  #include "i915_query.h"
> +#include "i915_reg.h"
>  #include "i915_suspend.h"
>  #include "i915_switcheroo.h"
>  #include "i915_sysfs.h"
> @@ -436,6 +437,32 @@ static int i915_pcode_init(struct drm_i915_private
> *i915)
>  	return 0;
>  }
> 
> +static int intel_pcode_probe(struct drm_i915_private *i915) {
> +	struct intel_uncore *uncore;
> +	int ret;
> +
> +	/*
> +	 * The boot firmware initializes local memory and assesses its health.
> +	 * If memory training fails, the punit will have been instructed to
> +	 * keep the GT powered down; we won't be able to communicate
> with it
> +	 * and we should not continue with driver initialization.
> +	 */
> +	if (IS_DGFX(i915) &&
> +		!(__raw_uncore_read32(uncore, GU_CNTL) & LMEM_INIT))
> {
> +		drm_err(&i915->drm, "LMEM not initialized by firmware\n");
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Driver handshakes with pcode via mailbox command to know that
> SoC
> +	 * initialization is complete before proceeding further
> +	 */
> +	ret = i915_pcode_init(i915);
> +
> +	return ret;
> +}
> +
>  /**
>   * i915_driver_hw_probe - setup state requiring device access
>   * @dev_priv: device private
> @@ -547,10 +574,6 @@ static int i915_driver_hw_probe(struct
> drm_i915_private *dev_priv)
> 
>  	intel_opregion_setup(dev_priv);
> 
> -	ret = i915_pcode_init(dev_priv);
> -	if (ret)
> -		goto err_opregion;
> -
>  	/*
>  	 * Fill the dram structure to get the system dram info. This will be
>  	 * used for memory latency calculation.
> @@ -561,8 +584,6 @@ static int i915_driver_hw_probe(struct
> drm_i915_private *dev_priv)
> 
>  	return 0;
> 
> -err_opregion:
> -	intel_opregion_cleanup(dev_priv);
>  err_msi:
>  	if (pdev->msi_enabled)
>  		pci_disable_msi(pdev);
> @@ -778,6 +799,10 @@ int i915_driver_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>  	if (ret < 0)
>  		goto out_runtime_pm_put;
> 
> +	ret = intel_pcode_probe(i915);
> +	if (ret)
> +		goto out_tiles_cleanup;
> +
>  	ret = i915_driver_mmio_probe(i915);
>  	if (ret < 0)
>  		goto out_tiles_cleanup;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> b/drivers/gpu/drm/i915/intel_uncore.c
> index dfefad5a5fec..4a353d4adf86 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -2658,18 +2658,6 @@ int intel_uncore_init_mmio(struct intel_uncore
> *uncore)
>  	if (ret)
>  		return ret;
> 
> -	/*
> -	 * The boot firmware initializes local memory and assesses its health.
> -	 * If memory training fails, the punit will have been instructed to
> -	 * keep the GT powered down; we won't be able to communicate
> with it
> -	 * and we should not continue with driver initialization.
> -	 */
> -	if (IS_DGFX(i915) &&
> -	    !(__raw_uncore_read32(uncore, GU_CNTL) & LMEM_INIT)) {
> -		drm_err(&i915->drm, "LMEM not initialized by firmware\n");
> -		return -ENODEV;
> -	}
> -
>  	if (GRAPHICS_VER(i915) > 5 && !intel_vgpu_active(i915))
>  		uncore->flags |= UNCORE_HAS_FORCEWAKE;
> 
> --
> 2.41.0

Re: [Intel-gfx] [PATCH] drm/i915: Add intel_pcode_probe

[Sundaresan, Sujaritha]

On 8/18/2023 11:30 AM, Gupta, Anshuman wrote:
>
>> -----Original Message-----
>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Sujaritha Sundaresan
>> Sent: Friday, August 18, 2023 8:16 AM
>> To: intel-gfx@lists.freedesktop.org
>> Subject: [Intel-gfx] [PATCH] drm/i915: Add intel_pcode_probe
>>
>> Added intel_pcode_probe, promoted wait for lmem init and intel_pcode_init
>> prior to mmio_probe during load, so that GT registers can be accessed only
>> after this, else MCA is observed.
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Both DG1 and DG2 crashed during i915_pci_probe.
> BAT is failing.
> Thanks,
> Anshuman Gupta.

Hi Anshuman,

Yes I'm currently looking into it.

Thanks,

Suja

>> ---
>>   drivers/gpu/drm/i915/i915_driver.c  | 37 ++++++++++++++++++++++++-----
>> drivers/gpu/drm/i915/intel_uncore.c | 12 ----------
>>   2 files changed, 31 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_driver.c
>> b/drivers/gpu/drm/i915/i915_driver.c
>> index f8dbee7a5af7..92cafceaf447 100644
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -93,6 +93,7 @@
>>   #include "i915_memcpy.h"
>>   #include "i915_perf.h"
>>   #include "i915_query.h"
>> +#include "i915_reg.h"
>>   #include "i915_suspend.h"
>>   #include "i915_switcheroo.h"
>>   #include "i915_sysfs.h"
>> @@ -436,6 +437,32 @@ static int i915_pcode_init(struct drm_i915_private
>> *i915)
>>   	return 0;
>>   }
>>
>> +static int intel_pcode_probe(struct drm_i915_private *i915) {
>> +	struct intel_uncore *uncore;
>> +	int ret;
>> +
>> +	/*
>> +	 * The boot firmware initializes local memory and assesses its health.
>> +	 * If memory training fails, the punit will have been instructed to
>> +	 * keep the GT powered down; we won't be able to communicate
>> with it
>> +	 * and we should not continue with driver initialization.
>> +	 */
>> +	if (IS_DGFX(i915) &&
>> +		!(__raw_uncore_read32(uncore, GU_CNTL) & LMEM_INIT))
>> {
>> +		drm_err(&i915->drm, "LMEM not initialized by firmware\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	/*
>> +	 * Driver handshakes with pcode via mailbox command to know that
>> SoC
>> +	 * initialization is complete before proceeding further
>> +	 */
>> +	ret = i915_pcode_init(i915);
>> +
>> +	return ret;
>> +}
>> +
>>   /**
>>    * i915_driver_hw_probe - setup state requiring device access
>>    * @dev_priv: device private
>> @@ -547,10 +574,6 @@ static int i915_driver_hw_probe(struct
>> drm_i915_private *dev_priv)
>>
>>   	intel_opregion_setup(dev_priv);
>>
>> -	ret = i915_pcode_init(dev_priv);
>> -	if (ret)
>> -		goto err_opregion;
>> -
>>   	/*
>>   	 * Fill the dram structure to get the system dram info. This will be
>>   	 * used for memory latency calculation.
>> @@ -561,8 +584,6 @@ static int i915_driver_hw_probe(struct
>> drm_i915_private *dev_priv)
>>
>>   	return 0;
>>
>> -err_opregion:
>> -	intel_opregion_cleanup(dev_priv);
>>   err_msi:
>>   	if (pdev->msi_enabled)
>>   		pci_disable_msi(pdev);
>> @@ -778,6 +799,10 @@ int i915_driver_probe(struct pci_dev *pdev, const
>> struct pci_device_id *ent)
>>   	if (ret < 0)
>>   		goto out_runtime_pm_put;
>>
>> +	ret = intel_pcode_probe(i915);
>> +	if (ret)
>> +		goto out_tiles_cleanup;
>> +
>>   	ret = i915_driver_mmio_probe(i915);
>>   	if (ret < 0)
>>   		goto out_tiles_cleanup;
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
>> b/drivers/gpu/drm/i915/intel_uncore.c
>> index dfefad5a5fec..4a353d4adf86 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -2658,18 +2658,6 @@ int intel_uncore_init_mmio(struct intel_uncore
>> *uncore)
>>   	if (ret)
>>   		return ret;
>>
>> -	/*
>> -	 * The boot firmware initializes local memory and assesses its health.
>> -	 * If memory training fails, the punit will have been instructed to
>> -	 * keep the GT powered down; we won't be able to communicate
>> with it
>> -	 * and we should not continue with driver initialization.
>> -	 */
>> -	if (IS_DGFX(i915) &&
>> -	    !(__raw_uncore_read32(uncore, GU_CNTL) & LMEM_INIT)) {
>> -		drm_err(&i915->drm, "LMEM not initialized by firmware\n");
>> -		return -ENODEV;
>> -	}
>> -
>>   	if (GRAPHICS_VER(i915) > 5 && !intel_vgpu_active(i915))
>>   		uncore->flags |= UNCORE_HAS_FORCEWAKE;
>>
>> --
>> 2.41.0

Re: [Intel-gfx] [PATCH] drm/i915: Add intel_pcode_probe

[Rodrigo Vivi]

On Fri, Aug 18, 2023 at 11:32:27AM +0530, Sundaresan, Sujaritha wrote:
> 
> On 8/18/2023 11:30 AM, Gupta, Anshuman wrote:
> > 
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > > Sujaritha Sundaresan
> > > Sent: Friday, August 18, 2023 8:16 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Subject: [Intel-gfx] [PATCH] drm/i915: Add intel_pcode_probe
> > > 
> > > Added intel_pcode_probe, promoted wait for lmem init and intel_pcode_init
> > > prior to mmio_probe during load, so that GT registers can be accessed only
> > > after this, else MCA is observed.
> > > 
> > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> > Both DG1 and DG2 crashed during i915_pci_probe.
> > BAT is failing.
> > Thanks,
> > Anshuman Gupta.
> 
> Hi Anshuman,
> 
> Yes I'm currently looking into it.
> 
> Thanks,
> 
> Suja
> 
> > > ---
> > >   drivers/gpu/drm/i915/i915_driver.c  | 37 ++++++++++++++++++++++++-----
> > > drivers/gpu/drm/i915/intel_uncore.c | 12 ----------
> > >   2 files changed, 31 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> > > b/drivers/gpu/drm/i915/i915_driver.c
> > > index f8dbee7a5af7..92cafceaf447 100644
> > > --- a/drivers/gpu/drm/i915/i915_driver.c
> > > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > > @@ -93,6 +93,7 @@
> > >   #include "i915_memcpy.h"
> > >   #include "i915_perf.h"
> > >   #include "i915_query.h"
> > > +#include "i915_reg.h"
> > >   #include "i915_suspend.h"
> > >   #include "i915_switcheroo.h"
> > >   #include "i915_sysfs.h"
> > > @@ -436,6 +437,32 @@ static int i915_pcode_init(struct drm_i915_private
> > > *i915)
> > >   	return 0;
> > >   }
> > > 
> > > +static int intel_pcode_probe(struct drm_i915_private *i915) {
> > > +	struct intel_uncore *uncore;
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * The boot firmware initializes local memory and assesses its health.
> > > +	 * If memory training fails, the punit will have been instructed to
> > > +	 * keep the GT powered down; we won't be able to communicate
> > > with it
> > > +	 * and we should not continue with driver initialization.
> > > +	 */
> > > +	if (IS_DGFX(i915) &&
> > > +		!(__raw_uncore_read32(uncore, GU_CNTL) & LMEM_INIT))
> > > {
> > > +		drm_err(&i915->drm, "LMEM not initialized by firmware\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Driver handshakes with pcode via mailbox command to know that
> > > SoC
> > > +	 * initialization is complete before proceeding further
> > > +	 */
> > > +	ret = i915_pcode_init(i915);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >   /**
> > >    * i915_driver_hw_probe - setup state requiring device access
> > >    * @dev_priv: device private
> > > @@ -547,10 +574,6 @@ static int i915_driver_hw_probe(struct
> > > drm_i915_private *dev_priv)
> > > 
> > >   	intel_opregion_setup(dev_priv);
> > > 
> > > -	ret = i915_pcode_init(dev_priv);
> > > -	if (ret)
> > > -		goto err_opregion;
> > > -
> > >   	/*
> > >   	 * Fill the dram structure to get the system dram info. This will be
> > >   	 * used for memory latency calculation.
> > > @@ -561,8 +584,6 @@ static int i915_driver_hw_probe(struct
> > > drm_i915_private *dev_priv)
> > > 
> > >   	return 0;
> > > 
> > > -err_opregion:
> > > -	intel_opregion_cleanup(dev_priv);
> > >   err_msi:
> > >   	if (pdev->msi_enabled)
> > >   		pci_disable_msi(pdev);
> > > @@ -778,6 +799,10 @@ int i915_driver_probe(struct pci_dev *pdev, const
> > > struct pci_device_id *ent)
> > >   	if (ret < 0)
> > >   		goto out_runtime_pm_put;
> > > 
> > > +	ret = intel_pcode_probe(i915);
> > > +	if (ret)
> > > +		goto out_tiles_cleanup;
> > > +
> > >   	ret = i915_driver_mmio_probe(i915);

chicken-egg problem here?!

I don't believe this could ever work. You need the MMIO space to be able
to communicate with PCODE mailbox and check the lmem init, no?!

I believe the bug is that PCODE check should come before the LMEM_INIT
check.

LMEM won't be ready before pcode state that everything was ready for
the lmem access. And on your code pcode ready check is still after
the lmem.

Cc: Aravind Iddamsetty <aravind.iddamsetty@intel.com>

who was recently raising that we had an order problem there.

> > >   	if (ret < 0)
> > >   		goto out_tiles_cleanup;
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> > > b/drivers/gpu/drm/i915/intel_uncore.c
> > > index dfefad5a5fec..4a353d4adf86 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -2658,18 +2658,6 @@ int intel_uncore_init_mmio(struct intel_uncore
> > > *uncore)
> > >   	if (ret)
> > >   		return ret;
> > > 
> > > -	/*
> > > -	 * The boot firmware initializes local memory and assesses its health.
> > > -	 * If memory training fails, the punit will have been instructed to
> > > -	 * keep the GT powered down; we won't be able to communicate
> > > with it
> > > -	 * and we should not continue with driver initialization.
> > > -	 */
> > > -	if (IS_DGFX(i915) &&
> > > -	    !(__raw_uncore_read32(uncore, GU_CNTL) & LMEM_INIT)) {
> > > -		drm_err(&i915->drm, "LMEM not initialized by firmware\n");
> > > -		return -ENODEV;
> > > -	}
> > > -
> > >   	if (GRAPHICS_VER(i915) > 5 && !intel_vgpu_active(i915))
> > >   		uncore->flags |= UNCORE_HAS_FORCEWAKE;
> > > 
> > > --
> > > 2.41.0

Re: [Intel-gfx] [PATCH] drm/i915: Add intel_pcode_probe

[Sundaresan, Sujaritha]

On 8/18/2023 6:29 PM, Rodrigo Vivi wrote:
> On Fri, Aug 18, 2023 at 11:32:27AM +0530, Sundaresan, Sujaritha wrote:
>> On 8/18/2023 11:30 AM, Gupta, Anshuman wrote:
>>>> -----Original Message-----
>>>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>>> Sujaritha Sundaresan
>>>> Sent: Friday, August 18, 2023 8:16 AM
>>>> To: intel-gfx@lists.freedesktop.org
>>>> Subject: [Intel-gfx] [PATCH] drm/i915: Add intel_pcode_probe
>>>>
>>>> Added intel_pcode_probe, promoted wait for lmem init and intel_pcode_init
>>>> prior to mmio_probe during load, so that GT registers can be accessed only
>>>> after this, else MCA is observed.
>>>>
>>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>> Both DG1 and DG2 crashed during i915_pci_probe.
>>> BAT is failing.
>>> Thanks,
>>> Anshuman Gupta.
>> Hi Anshuman,
>>
>> Yes I'm currently looking into it.
>>
>> Thanks,
>>
>> Suja
>>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_driver.c  | 37 ++++++++++++++++++++++++-----
>>>> drivers/gpu/drm/i915/intel_uncore.c | 12 ----------
>>>>    2 files changed, 31 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_driver.c
>>>> b/drivers/gpu/drm/i915/i915_driver.c
>>>> index f8dbee7a5af7..92cafceaf447 100644
>>>> --- a/drivers/gpu/drm/i915/i915_driver.c
>>>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>>>> @@ -93,6 +93,7 @@
>>>>    #include "i915_memcpy.h"
>>>>    #include "i915_perf.h"
>>>>    #include "i915_query.h"
>>>> +#include "i915_reg.h"
>>>>    #include "i915_suspend.h"
>>>>    #include "i915_switcheroo.h"
>>>>    #include "i915_sysfs.h"
>>>> @@ -436,6 +437,32 @@ static int i915_pcode_init(struct drm_i915_private
>>>> *i915)
>>>>    	return 0;
>>>>    }
>>>>
>>>> +static int intel_pcode_probe(struct drm_i915_private *i915) {
>>>> +	struct intel_uncore *uncore;
>>>> +	int ret;
>>>> +
>>>> +	/*
>>>> +	 * The boot firmware initializes local memory and assesses its health.
>>>> +	 * If memory training fails, the punit will have been instructed to
>>>> +	 * keep the GT powered down; we won't be able to communicate
>>>> with it
>>>> +	 * and we should not continue with driver initialization.
>>>> +	 */
>>>> +	if (IS_DGFX(i915) &&
>>>> +		!(__raw_uncore_read32(uncore, GU_CNTL) & LMEM_INIT))
>>>> {
>>>> +		drm_err(&i915->drm, "LMEM not initialized by firmware\n");
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * Driver handshakes with pcode via mailbox command to know that
>>>> SoC
>>>> +	 * initialization is complete before proceeding further
>>>> +	 */
>>>> +	ret = i915_pcode_init(i915);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>>    /**
>>>>     * i915_driver_hw_probe - setup state requiring device access
>>>>     * @dev_priv: device private
>>>> @@ -547,10 +574,6 @@ static int i915_driver_hw_probe(struct
>>>> drm_i915_private *dev_priv)
>>>>
>>>>    	intel_opregion_setup(dev_priv);
>>>>
>>>> -	ret = i915_pcode_init(dev_priv);
>>>> -	if (ret)
>>>> -		goto err_opregion;
>>>> -
>>>>    	/*
>>>>    	 * Fill the dram structure to get the system dram info. This will be
>>>>    	 * used for memory latency calculation.
>>>> @@ -561,8 +584,6 @@ static int i915_driver_hw_probe(struct
>>>> drm_i915_private *dev_priv)
>>>>
>>>>    	return 0;
>>>>
>>>> -err_opregion:
>>>> -	intel_opregion_cleanup(dev_priv);
>>>>    err_msi:
>>>>    	if (pdev->msi_enabled)
>>>>    		pci_disable_msi(pdev);
>>>> @@ -778,6 +799,10 @@ int i915_driver_probe(struct pci_dev *pdev, const
>>>> struct pci_device_id *ent)
>>>>    	if (ret < 0)
>>>>    		goto out_runtime_pm_put;
>>>>
>>>> +	ret = intel_pcode_probe(i915);
>>>> +	if (ret)
>>>> +		goto out_tiles_cleanup;
>>>> +
>>>>    	ret = i915_driver_mmio_probe(i915);
> chicken-egg problem here?!
>
> I don't believe this could ever work. You need the MMIO space to be able
> to communicate with PCODE mailbox and check the lmem init, no?!
>
> I believe the bug is that PCODE check should come before the LMEM_INIT
> check.
>
> LMEM won't be ready before pcode state that everything was ready for
> the lmem access. And on your code pcode ready check is still after
> the lmem.
>
> Cc: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
>
> who was recently raising that we had an order problem there.

Yes looks like there is definitely an ordering issue.

Will look into this more.

>
>>>>    	if (ret < 0)
>>>>    		goto out_tiles_cleanup;
>>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
>>>> b/drivers/gpu/drm/i915/intel_uncore.c
>>>> index dfefad5a5fec..4a353d4adf86 100644
>>>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>>>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>>>> @@ -2658,18 +2658,6 @@ int intel_uncore_init_mmio(struct intel_uncore
>>>> *uncore)
>>>>    	if (ret)
>>>>    		return ret;
>>>>
>>>> -	/*
>>>> -	 * The boot firmware initializes local memory and assesses its health.
>>>> -	 * If memory training fails, the punit will have been instructed to
>>>> -	 * keep the GT powered down; we won't be able to communicate
>>>> with it
>>>> -	 * and we should not continue with driver initialization.
>>>> -	 */
>>>> -	if (IS_DGFX(i915) &&
>>>> -	    !(__raw_uncore_read32(uncore, GU_CNTL) & LMEM_INIT)) {
>>>> -		drm_err(&i915->drm, "LMEM not initialized by firmware\n");
>>>> -		return -ENODEV;
>>>> -	}
>>>> -
>>>>    	if (GRAPHICS_VER(i915) > 5 && !intel_vgpu_active(i915))
>>>>    		uncore->flags |= UNCORE_HAS_FORCEWAKE;
>>>>
>>>> --
>>>> 2.41.0