Re: [Intel-gfx] [PATCH v5 05/22] drm/i915/dg1: Wait for pcode/uncore handshake at startup

[Lucas De Marchi <lucas.demarchi@intel.com>]

On Mon, Aug 03, 2020 at 04:24:17PM -0700, Jose Souza wrote:
>On Fri, 2020-07-24 at 14:39 -0700, Lucas De Marchi wrote:
>> From: Matt Roper <
>> matthew.d.roper@intel.com
>> >
>>
>> DG1 does some additional pcode/uncore handshaking at
>> boot time; this handshaking must complete before various other pcode
>> commands are effective and before general work is submitted to the GPU.
>> We need to poll a new pcode mailbox during startup until it reports that
>> this handshaking is complete.
>>
>> The bspec doesn't give guidance on how long we may need to wait for this
>> handshaking to complete.  For now, let's just set a really long timeout;
>> if we still don't get a completion status by the end of that timeout,
>> we'll just continue on and hope for the best.
>>
>> Bspec: 52065
>> Cc: Clinton Taylor <
>> Clinton.A.Taylor@intel.com
>> >
>> Cc: Ville Syrjälä <
>> ville.syrjala@linux.intel.com
>> >
>> Cc: Radhakrishna Sripada <
>> radhakrishna.sripada@intel.com
>> >
>> Signed-off-by: Matt Roper <
>> matthew.d.roper@intel.com
>> >
>> Signed-off-by: Lucas De Marchi <
>> lucas.demarchi@intel.com
>> >
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c       |  3 +++
>>  drivers/gpu/drm/i915/i915_reg.h       |  3 +++
>>  drivers/gpu/drm/i915/intel_sideband.c | 15 +++++++++++++++
>>  drivers/gpu/drm/i915/intel_sideband.h |  2 ++
>>  4 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 5fd5af4bc855..5473bfe9126c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -85,6 +85,7 @@
>>  #include "intel_gvt.h"
>>  #include "intel_memory_region.h"
>>  #include "intel_pm.h"
>> +#include "intel_sideband.h"
>>  #include "vlv_suspend.h"
>>
>>  static struct drm_driver driver;
>> @@ -737,6 +738,8 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
>>  	 */
>>  	intel_dram_detect(dev_priv);
>>
>> +	intel_pcode_init(dev_priv);
>> +
>>  	intel_bw_init_hw(dev_priv);
>>
>>  	return 0;
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index a0d31f3bf634..3767b32127da 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -9245,6 +9245,9 @@ enum {
>>  #define     GEN9_SAGV_DISABLE			0x0
>>  #define     GEN9_SAGV_IS_DISABLED		0x1
>>  #define     GEN9_SAGV_ENABLE			0x3
>> +#define   DG1_PCODE_STATUS			0x7E
>> +#define     DG1_CHECK_UNCORE_INIT_STATUS	0x0
>> +#define     DG1_UNCORE_INIT_COMPLETE		0x1
>
>With s/DG1_CHECK_UNCORE_INIT_STATUS/DG1_CHECK_UNCORE_INIT_STATUS_COMPLETE or something similar that makes easy to understand that 0x1 is the response
>of the DG1_CHECK_UNCORE_INIT_STATUS sub-command.

checking all the other users of skl_pcode_request() I don't see a
pattern there. Examples:

ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL,
                         SKL_CDCLK_PREPARE_FOR_CHANGE,     
                         SKL_CDCLK_READY_FOR_CHANGE,       
                         SKL_CDCLK_READY_FOR_CHANGE, 3);   

ret = skl_pcode_request(dev_priv, GEN9_PCODE_SAGV_CONTROL,            
                         GEN9_SAGV_DISABLE,                            
                         GEN9_SAGV_IS_DISABLED, GEN9_SAGV_IS_DISABLED, 
                         1);                                           

Giveng the current uses, I'd rather rename like:

+#define   DG1_PCODE_STATUS			0x7E
+#define     DG1_UNCORE_GET_INIT_STATUS		0x0
+#define     DG1_UNCORE_INIT_STATUS_COMPLETE	0x1


>Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

does that still stands with the rename above?

thanks
Lucas De Marchi

>
>
>>  #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
>>  #define GEN6_PCODE_DATA				_MMIO(0x138128)
>>  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
>> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
>> index 916ccd1c0e96..8b093525240d 100644
>> --- a/drivers/gpu/drm/i915/intel_sideband.c
>> +++ b/drivers/gpu/drm/i915/intel_sideband.c
>> @@ -543,3 +543,18 @@ int skl_pcode_request(struct drm_i915_private *i915, u32 mbox, u32 request,
>>  	return ret ? ret : status;
>>  #undef COND
>>  }
>> +
>> +void intel_pcode_init(struct drm_i915_private *i915)
>> +{
>> +	int ret;
>> +
>> +	if (!IS_DGFX(i915))
>> +		return;
>> +
>> +	ret = skl_pcode_request(i915, DG1_PCODE_STATUS,
>> +				DG1_CHECK_UNCORE_INIT_STATUS,
>> +				DG1_UNCORE_INIT_COMPLETE,
>> +				DG1_UNCORE_INIT_COMPLETE, 50);
>> +	if (ret)
>> +		drm_err(&i915->drm, "Pcode did not report uncore initialization completion!\n");
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_sideband.h b/drivers/gpu/drm/i915/intel_sideband.h
>> index 7fb95745a444..094c7b19c5d4 100644
>> --- a/drivers/gpu/drm/i915/intel_sideband.h
>> +++ b/drivers/gpu/drm/i915/intel_sideband.h
>> @@ -138,4 +138,6 @@ int sandybridge_pcode_write_timeout(struct drm_i915_private *i915, u32 mbox,
>>  int skl_pcode_request(struct drm_i915_private *i915, u32 mbox, u32 request,
>>  		      u32 reply_mask, u32 reply, int timeout_base_ms);
>>
>> +void intel_pcode_init(struct drm_i915_private *i915);
>> +
>>  #endif /* _INTEL_SIDEBAND_H */
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx