Re: [PATCH v5 1/4] drm/xe/forcewake: synchronize engine-cycle access with unplug

["Bai, Zongyao" <zongyao.bai@intel.com>]

On 9/3/2026 9:20 AM, Rodrigo Vivi wrote:
> On Thu, Aug 13, 2026 at 12:06:51AM +0000, Zongyao Bai wrote:
>> Prepare engine-cycle queries for delayed forcewake release by
>> protecting their forcewake and MMIO section with drm_dev_enter(). This
>> lets unplug wait for admitted queries before flushing delayed-release
>> timers.
>>
>> Use drm_dev_unplug() on post-registration probe failure to provide the
>> same synchronization before teardown.
>>
>> Assisted-by: GitHub-Copilot:gpt-5.6-sol
>> Signed-off-by: Zongyao Bai <zongyao.bai@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_device.c |  2 +-
>>   drivers/gpu/drm/xe/xe_query.c  | 34 ++++++++++++++++++++++++++--------
>>   2 files changed, 27 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index 71111ad32465..81b31325e581 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -1166,7 +1166,7 @@ int xe_device_probe(struct xe_device *xe)
>>   
>>   err_unregister_display:
>>   	xe_display_unregister(xe);
>> -	drm_dev_unregister(&xe->drm);
>> +	drm_dev_unplug(&xe->drm);
>>   
>>   	return err;
>>   }
>> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
>> index dc975f595368..3c356bc48e2a 100644
>> --- a/drivers/gpu/drm/xe/xe_query.c
>> +++ b/drivers/gpu/drm/xe/xe_query.c
>> @@ -8,6 +8,7 @@
>>   #include <linux/nospec.h>
>>   #include <linux/sched/clock.h>
>>   
>> +#include <drm/drm_drv.h>
>>   #include <drm/ttm/ttm_placement.h>
>>   #include <generated/xe_wa_oob.h>
>>   #include <uapi/drm/xe_drm.h>
>> @@ -123,6 +124,7 @@ query_engine_cycles(struct xe_device *xe,
>>   	__ktime_func_t cpu_clock;
>>   	struct xe_hw_engine *hwe;
>>   	struct xe_gt *gt;
>> +	int dev_idx, err = 0;
> use ret instead of err, then...
>
>>   
>>   	if (IS_SRIOV_VF(xe))
>>   		return -EOPNOTSUPP;
>> @@ -143,28 +145,44 @@ query_engine_cycles(struct xe_device *xe,
>>   		return -EINVAL;
>>   
>>   	eci = &resp.eci;
>> +	if (!drm_dev_enter(&xe->drm, &dev_idx))
>> +		return -ENODEV;
>> +
>>   	gt = xe_device_get_gt(xe, eci->gt_id);
>> -	if (!gt)
>> -		return -EINVAL;
>> +	if (!gt) {
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>>   
>> -	if (eci->engine_class >= ARRAY_SIZE(user_to_xe_engine_class))
>> -		return -EINVAL;
>> +	if (eci->engine_class >= ARRAY_SIZE(user_to_xe_engine_class)) {
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>>   
>>   	hwe = xe_gt_hw_engine(gt, user_to_xe_engine_class[eci->engine_class],
>>   			      eci->engine_instance, true);
>> -	if (!hwe)
>> -		return -EINVAL;
>> +	if (!hwe) {
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>>   
>>   	fw_domain = xe_hw_engine_to_fw_domain(hwe);
>>   
>>   	xe_with_force_wake(fw_ref, gt_to_fw(gt), fw_domain) {
>> -		if (!xe_force_wake_ref_has_domain(fw_ref.domains, fw_domain))
>> -			return -EIO;
>> +		if (!xe_force_wake_ref_has_domain(fw_ref.domains, fw_domain)) {
>> +			err = -EIO;
>> +			goto out;
>> +		}
>>   
>>   		hwe_read_timestamp(hwe, &resp.engine_cycles, &resp.cpu_timestamp,
>>   				   &resp.cpu_delta, cpu_clock);
>>   	}
>>   
>> +out:
>> +	drm_dev_exit(dev_idx);
>> +	if (err)
> oh no!
> never to the if (err) after the goto! Sashiko is absolutely right!
>
> ... then you move this to the very end and simply:
>
> out:
> 	drm_dev_exit(dev_idx);
> 	return ret;

Thank you Vivi

Got it.

I will try to remove goto label to met both of the review requirements.

=====================================================

Re: [PATCH v5 4/4] drm/xe/forcewake: enable configurable delayed 
forcewake release
 > [Severity: Low]
 > Does this code mix goto-based error handling with scope-based cleanup 
macros?
 >
 > According to the cleanup subsystem guidelines, the expectation is 
that usage of
 > goto and cleanup helpers is never mixed in the same function.

Yes, I agree this is code style issue,
this function contains both a goto out and the 
xe_with_force_wake_delay() scope macro.
And this issue is introduced by path [PATCH v5 1/4], but not in this patch.

It may need split query_engine_cycles to two to avoid goto.
Since this appears to be more of a coding style preference,
I'd like to wait for consensus from other reviewers before updating it.

>
>> +		return err;
>> +
>>   	if (GRAPHICS_VER(xe) >= 20)
>>   		resp.width = 64;
>>   	else
>> -- 
>> 2.43.0
>>