Re: [PATCH v2 05/10] drm/xe: Bail from adjust_idledly() if no applicable workaround

[Gustavo Sousa <gustavo.sousa@intel.com>]

"Vivekanandan, Balasubramani" <balasubramani.vivekanandan@intel.com>
writes:

> On 30.10.2026 19:57, Gustavo Sousa wrote:
>> Re-organize adjust_idledly() so that we return and avoid touching the
>> hardware if no applicable workaround was found.
>> 
>> Reviewed-by: Matt Atwood <matthew.s.atwood@intel.com>
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_hw_engine.c | 32 +++++++++++++++++++-------------
>>  1 file changed, 19 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
>> index 01a7baf061eb..32d33c70e7f4 100644
>> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
>> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
>> @@ -605,7 +605,7 @@ static void adjust_idledly(struct xe_hw_engine *hwe)
>>  	u64 idledly_ps, idledly_hw_ps;
>>  	u64 min_idledly_ps = 0;
>>  	u64 max_idledly_ps = -1;
>> -	bool inhibit_switch = false;
>> +	bool clear_inhibit_switch = false;
>>  	bool wa_applied = false;
>>  
>>  	if (IS_SRIOV_VF(gt_to_xe(gt)))
>> @@ -616,12 +616,6 @@ static void adjust_idledly(struct xe_hw_engine *hwe)
>>  		if (!idledly_units_ps)
>>  			return;
>>  
>> -		idledly_reg_val = xe_mmio_read32(&gt->mmio, RING_IDLEDLY(hwe->mmio_base));
>> -		inhibit_switch = idledly_reg_val & INHIBIT_SWITCH_UNTIL_PREEMPTED;
>> -		idledly = REG_FIELD_GET(IDLE_DELAY, idledly_reg_val);
>> -		idledly_ps = idledly * idledly_units_ps;
>> -		idledly_hw_ps = idledly_ps;
>> -
>>  		if (XE_GT_WA(gt, 14025941587))
>>  			min_idledly_ps = max(min_idledly_ps, 5000000);
>>  
>> @@ -633,13 +627,24 @@ static void adjust_idledly(struct xe_hw_engine *hwe)
>>  					       max_idledly_ps,
>>  					       REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt) * maxcnt_units_ps);
>>  
>> -			/* Clear the inhibit switch without disturbing a valid delay. */
>> -			if (inhibit_switch) {
>> -				idledly_reg_val &= ~INHIBIT_SWITCH_UNTIL_PREEMPTED;
>> -				wa_applied = true;
>> -			}
>> +			clear_inhibit_switch = true;
>>  		}
>>  
>> +		/* No applicable workaround found. */
>> +		if (min_idledly_ps == 0 && max_idledly_ps == -1 && !clear_inhibit_switch)
>> +			return;
>
> We can skip the check for "max_idledly_ps == -1".

You say that because of the "!clear_inhibit_switch" check?

If so, I would prefer that we keep the check so that we do not tie the
maximum idle delay with the clear_inhibit_switch flag.  It is true that
today the only workaround that defines a maximum idle delay also sets
clear_inhibit_switch, but I think we should make the logic treat those
two things orthogonally, like it is already being done below.

>
> Reviewed-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>

Thanks! :-)

Let me know if I can go ahead with you r-b on the patch as-is; or if you
prefer to further discuss this.

--
Gustavo Sousa

>
> Regards,
> Bala
>
>> +
>> +		idledly_reg_val = xe_mmio_read32(&gt->mmio, RING_IDLEDLY(hwe->mmio_base));
>> +
>> +		if (clear_inhibit_switch && idledly_reg_val & INHIBIT_SWITCH_UNTIL_PREEMPTED) {
>> +			idledly_reg_val &= ~INHIBIT_SWITCH_UNTIL_PREEMPTED;
>> +			wa_applied = true;
>> +		}
>> +
>> +		idledly = REG_FIELD_GET(IDLE_DELAY, idledly_reg_val);
>> +		idledly_ps = idledly * idledly_units_ps;
>> +		idledly_hw_ps = idledly_ps;
>> +
>>  		/*
>>  		 * min_idledly_ps is applied before max_idledly_ps, which takes
>>  		 * priority if the two ever conflict (not expected in practice).
>> @@ -671,7 +676,8 @@ static void adjust_idledly(struct xe_hw_engine *hwe)
>>  
>>  			xe_gt_dbg(gt, "idledly %s: set %llups min=%llups max=%llups inh=%u ts=%ups\n",
>>  				  hwe->name, idledly_ps, min_idledly_ps, max_idledly_ps,
>> -				  !!inhibit_switch, gt->info.timestamp_base);
>> +				  !!(idledly_reg_val & INHIBIT_SWITCH_UNTIL_PREEMPTED),
>> +				  gt->info.timestamp_base);
>>  			xe_mmio_write32(&gt->mmio,
>>  					RING_IDLEDLY(hwe->mmio_base),
>>  					idledly_reg_val);
>> 
>> -- 
>> 2.56.0
>>