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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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;

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