Re: [Intel-xe] [PATCH v2 7/7] drm/xe: enable idle msg and set hysteresis for GSCCS

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

On 8/17/2023 1:44 PM, Matt Roper wrote:
> On Thu, Aug 17, 2023 at 01:18:31PM -0700, Daniele Ceraolo Spurio wrote:
>> On MTL (and only on MTL) the GSCCS defaults with idle messaging
>> disabled. This means that, once awoken, the GSCCS will never signal its
>> idleness to the GT. To allow the GT to enter the proper low-power state,
>> we need therefore to turn idle messaging on. As part of this, we also
>> need to set a proper hysteresis value for the engine.
>>
>> Bspec: 71496
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>> ---
>>   drivers/gpu/drm/xe/regs/xe_engine_regs.h |  4 ++++
>>   drivers/gpu/drm/xe/xe_hw_engine.c        | 17 +++++++++++++++++
>>   2 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_engine_regs.h b/drivers/gpu/drm/xe/regs/xe_engine_regs.h
>> index 79873bf64e8d..f012a66cb3da 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_engine_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_engine_regs.h
>> @@ -22,9 +22,13 @@
>>   #define   RING_CTL_SIZE(size)			((size) - PAGE_SIZE) /* in bytes -> pages */
>>   
>>   #define RING_PSMI_CTL(base)			XE_REG((base) + 0x50, XE_REG_OPTION_MASKED)
>> +#define   IDLE_MSG_DISABLE			REG_BIT(0)
>>   #define   RC_SEMA_IDLE_MSG_DISABLE		REG_BIT(12)
>>   #define   WAIT_FOR_EVENT_POWER_DOWN_DISABLE	REG_BIT(7)
> Nipick:  might be best to put the new bit at the bottom since these
> usually descend.

ok

>
>>   
>> +#define RING_PWRCTX_MAXCNT(base)		XE_REG((base) + 0x54)
>> +#define   IDLE_WAIT_TIME			REG_GENMASK(19, 0)
>> +
>>   #define RING_ACTHD_UDW(base)			XE_REG((base) + 0x5c)
>>   #define RING_DMA_FADD_UDW(base)			XE_REG((base) + 0x60)
>>   #define RING_IPEIR(base)			XE_REG((base) + 0x64)
>> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
>> index 81281e9c02eb..cd0ac3ce5abf 100644
>> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
>> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
>> @@ -339,6 +339,23 @@ hw_engine_setup_default_state(struct xe_hw_engine *hwe)
>>   					   ring_cmd_cctl_val,
>>   					   XE_RTP_ACTION_FLAG(ENGINE_BASE)))
>>   		},
>> +		/*
>> +		 * To allow the GSC engine to go idle on MTL we need to enable
>> +		 * idle messaging and set the hysteresis value (we use 0xA=5us
>> +		 * as recommended in spec). On platform after MTL this is
>> +		 * enabled by default.
>> +		 */
>> +		{ XE_RTP_NAME("MTL GSCCS IDLE MSG enable"),
>> +		  XE_RTP_RULES(PLATFORM(METEORLAKE), ENGINE_CLASS(OTHER)),
> Although this change wont' be inherited by Xe2 IP and beyond, there's
> always the possibility that this Xe_LPM+ IP might get re-used in another
> non-MTL platform (e.g., some kind of refresh platform down the road).
> So it might be best to still use an IP version check here to cover that
> case.  I.e.,
>
>          MEDIA_VERSION(1300)

ok

>> +		  XE_RTP_ACTIONS(FIELD_SET(RING_PSMI_CTL(0),
>> +					   IDLE_MSG_DISABLE,
>> +					   0,
> Rather than FIELD_SET to 0, I think it would be more intuitive to do
>
>          CLR(RING_PSMI_CTL(0), IDLE_MSG_DISABLE)

I missed that we had a CLR macro. I agree this is cleaner, will send v2 
after CI confirms that nothing is broken.

Daniele

>
> here.
>
>
> Matt
>
>> +					   XE_RTP_ACTION_FLAG(ENGINE_BASE)),
>> +				 FIELD_SET(RING_PWRCTX_MAXCNT(0),
>> +					   IDLE_WAIT_TIME,
>> +					   0xA,
>> +					   XE_RTP_ACTION_FLAG(ENGINE_BASE)))
>> +		},
>>   		{}
>>   	};
>>   
>> -- 
>> 2.41.0
>>
>>