Re: [PATCH 1/2] RFC drm/xe: check pcode init status only on root gt of root tile

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

On Fri, Mar 08, 2024 at 02:25:16PM +0530, Riana Tauro wrote:
> The root tile indicates the pcode initialization is complete
> when all tiles have completed their initialization.
> So the mailbox can be polled only on the root tile.
> Check pcode init status only on root tile and move it to
> device probe early as root tile is initialized there.
> Also make similar changes in resume paths.
> 
> v2: Add lock/unlocked version of pcode_mailbox_rw
>     to allow pcode init to be called in device
>     early probe (Rodrigo)
> 
> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c | 21 +++++---
>  drivers/gpu/drm/xe/xe_pcode.c  | 93 ++++++++++++++++++++--------------
>  drivers/gpu/drm/xe/xe_pcode.h  |  3 +-
>  drivers/gpu/drm/xe/xe_pm.c     | 16 +++---
>  4 files changed, 78 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 919ad88f0495..83dd60f68566 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -413,8 +413,14 @@ static int xe_set_dma_info(struct xe_device *xe)
>  	return err;
>  }
>  
> -/*
> - * Initialize MMIO resources that don't require any knowledge about tile count.
> +/**
> + * xe_device_probe_early: Device early probe
> + * @xe: xe device instance
> + *
> + * Initialize MMIO resources that don't require any
> + * knowledge about tile count. Also initialize pcode
> + *
> + * Return: 0 on success, error code on failure
>   */
>  int xe_device_probe_early(struct xe_device *xe)
>  {
> @@ -428,6 +434,10 @@ int xe_device_probe_early(struct xe_device *xe)
>  	if (err)
>  		return err;
>  
> +	err = xe_pcode_ready(xe_root_mmio_gt(xe), false);

my first thought here was that we should also put a comment on why
it is okay to use only root tile. A simpler version of what you
told in the commit message above.

But then, looking further below, I started asking myself about
the names.

what about have a

xe_pcode_probe_early(xe)
that calls xe_pcode_ready(xe, false);

and inside the xe_pcode_ready you do the xe_root_mmio_gt(xe).

and you use the bool to grab (or not) the lock inside xe_pcode_ready.

then xe_pcode_probe continue to be only the lock initialization
although that could also be named xe_pcode_init.

And then on the resume paths you call xe_pcode_ready(xe, true).
There on resume paths we also only need the root tile check.

But only some extra thoughts that I had now seeing the final patch,
but since this already is a good movement towards our goal and is
correct, if you add the code comment about the root-tile being
enough, you can feel free to use:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +	if (err)
> +		return err;
> +
>  	return 0;
>  }
>  
> @@ -506,11 +516,8 @@ int xe_device_probe(struct xe_device *xe)
>  	if (err)
>  		return err;
>  
> -	for_each_gt(gt, xe, id) {
> -		err = xe_pcode_probe(gt);
> -		if (err)
> -			return err;
> -	}
> +	for_each_gt(gt, xe, id)
> +		xe_pcode_probe(gt);
>  
>  	err = xe_display_init_noirq(xe);
>  	if (err)
> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index b324dc2a5deb..6c0009fcd2fe 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -43,8 +43,6 @@ static int pcode_mailbox_status(struct xe_gt *gt)
>  		[PCODE_ERROR_MASK] = {-EPROTO, "Unknown"},
>  	};
>  
> -	lockdep_assert_held(&gt->pcode.lock);
> -
>  	err = xe_mmio_read32(gt, PCODE_MAILBOX) & PCODE_ERROR_MASK;
>  	if (err) {
>  		drm_err(&gt_to_xe(gt)->drm, "PCODE Mailbox failed: %d %s", err,
> @@ -55,17 +53,15 @@ static int pcode_mailbox_status(struct xe_gt *gt)
>  	return 0;
>  }
>  
> -static int pcode_mailbox_rw(struct xe_gt *gt, u32 mbox, u32 *data0, u32 *data1,
> -			    unsigned int timeout_ms, bool return_data,
> -			    bool atomic)
> +static int __pcode_mailbox_rw(struct xe_gt *gt, u32 mbox, u32 *data0, u32 *data1,
> +			      unsigned int timeout_ms, bool return_data,
> +			      bool atomic)
>  {
>  	int err;
>  
>  	if (gt_to_xe(gt)->info.skip_pcode)
>  		return 0;
>  
> -	lockdep_assert_held(&gt->pcode.lock);
> -
>  	if ((xe_mmio_read32(gt, PCODE_MAILBOX) & PCODE_READY) != 0)
>  		return -EAGAIN;
>  
> @@ -87,6 +83,18 @@ static int pcode_mailbox_rw(struct xe_gt *gt, u32 mbox, u32 *data0, u32 *data1,
>  	return pcode_mailbox_status(gt);
>  }
>  
> +static int pcode_mailbox_rw(struct xe_gt *gt, u32 mbox, u32 *data0, u32 *data1,
> +			    unsigned int timeout_ms, bool return_data,
> +			    bool atomic)
> +{
> +	if (gt_to_xe(gt)->info.skip_pcode)
> +		return 0;
> +
> +	lockdep_assert_held(&gt->pcode.lock);
> +
> +	return __pcode_mailbox_rw(gt, mbox, data0, data1, timeout_ms, return_data, atomic);
> +}
> +
>  int xe_pcode_write_timeout(struct xe_gt *gt, u32 mbox, u32 data, int timeout)
>  {
>  	int err;
> @@ -109,15 +117,19 @@ int xe_pcode_read(struct xe_gt *gt, u32 mbox, u32 *val, u32 *val1)
>  	return err;
>  }
>  
> -static int xe_pcode_try_request(struct xe_gt *gt, u32 mbox,
> -				u32 request, u32 reply_mask, u32 reply,
> -				u32 *status, bool atomic, int timeout_us)
> +static int pcode_try_request(struct xe_gt *gt, u32 mbox,
> +			     u32 request, u32 reply_mask, u32 reply,
> +			     u32 *status, bool atomic, int timeout_us, bool locked)
>  {
>  	int slept, wait = 10;
>  
>  	for (slept = 0; slept < timeout_us; slept += wait) {
> -		*status = pcode_mailbox_rw(gt, mbox, &request, NULL, 1, true,
> -					   atomic);
> +		if (locked)
> +			*status = pcode_mailbox_rw(gt, mbox, &request, NULL, 1, true,
> +						   atomic);
> +		else
> +			*status = __pcode_mailbox_rw(gt, mbox, &request, NULL, 1, true,
> +						     atomic);
>  		if ((*status == 0) && ((request & reply_mask) == reply))
>  			return 0;
>  
> @@ -158,8 +170,8 @@ int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
>  
>  	mutex_lock(&gt->pcode.lock);
>  
> -	ret = xe_pcode_try_request(gt, mbox, request, reply_mask, reply, &status,
> -				   false, timeout_base_ms * 1000);
> +	ret = pcode_try_request(gt, mbox, request, reply_mask, reply, &status,
> +				false, timeout_base_ms * 1000, true);
>  	if (!ret)
>  		goto out;
>  
> @@ -177,8 +189,8 @@ int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
>  		"PCODE timeout, retrying with preemption disabled\n");
>  	drm_WARN_ON_ONCE(&gt_to_xe(gt)->drm, timeout_base_ms > 1);
>  	preempt_disable();
> -	ret = xe_pcode_try_request(gt, mbox, request, reply_mask, reply, &status,
> -				   true, timeout_base_ms * 1000);
> +	ret = pcode_try_request(gt, mbox, request, reply_mask, reply, &status,
> +				true, timeout_base_ms * 1000, true);
>  	preempt_enable();
>  
>  out:
> @@ -238,15 +250,16 @@ int xe_pcode_init_min_freq_table(struct xe_gt *gt, u32 min_gt_freq,
>  }
>  
>  /**
> - * xe_pcode_init - Ensure PCODE is initialized
> + * xe_pcode_ready - Ensure PCODE is initialized
>   * @gt: gt instance
> + * @locked: true if lock held, false otherwise
>   *
> - * This function ensures that PCODE is properly initialized. To be called during
> - * probe and resume paths.
> + * This function ensures that PCODE is properly initialized. Can be called
> + * without locks only in early probe.
>   *
>   * It returns 0 on success, and -error number on failure.
>   */
> -int xe_pcode_init(struct xe_gt *gt)
> +int xe_pcode_ready(struct xe_gt *gt, bool locked)
>  {
>  	u32 status, request = DGFX_GET_INIT_STATUS;
>  	int timeout_us = 180000000; /* 3 min */
> @@ -258,12 +271,10 @@ int xe_pcode_init(struct xe_gt *gt)
>  	if (!IS_DGFX(gt_to_xe(gt)))
>  		return 0;
>  
> -	mutex_lock(&gt->pcode.lock);
> -	ret = xe_pcode_try_request(gt, DGFX_PCODE_STATUS, request,
> -				   DGFX_INIT_STATUS_COMPLETE,
> -				   DGFX_INIT_STATUS_COMPLETE,
> -				   &status, false, timeout_us);
> -	mutex_unlock(&gt->pcode.lock);
> +	ret = pcode_try_request(gt, DGFX_PCODE_STATUS, request,
> +				DGFX_INIT_STATUS_COMPLETE,
> +				DGFX_INIT_STATUS_COMPLETE,
> +				&status, false, timeout_us, locked);
>  
>  	if (ret)
>  		drm_err(&gt_to_xe(gt)->drm,
> @@ -273,24 +284,32 @@ int xe_pcode_init(struct xe_gt *gt)
>  }
>  
>  /**
> - * xe_pcode_probe - Prepare xe_pcode and also ensure PCODE is initialized.
> + * xe_pcode_init - initialize pcode
>   * @gt: gt instance
>   *
> - * This function initializes the xe_pcode component, and when needed, it ensures
> - * that PCODE has properly performed its initialization and it is really ready
> - * to go. To be called once only during probe.
> + * This function initializes pcode. Used in resume paths
>   *
>   * It returns 0 on success, and -error number on failure.
>   */
> -int xe_pcode_probe(struct xe_gt *gt)
> +int xe_pcode_init(struct xe_gt *gt)
>  {
> -	drmm_mutex_init(&gt_to_xe(gt)->drm, &gt->pcode.lock);
> +	int ret;
>  
> -	if (gt_to_xe(gt)->info.skip_pcode)
> -		return 0;
> +	mutex_lock(&gt->pcode.lock);
> +	ret = xe_pcode_ready(gt, true);
> +	mutex_unlock(&gt->pcode.lock);
>  
> -	if (!IS_DGFX(gt_to_xe(gt)))
> -		return 0;
> +	return ret;
> +}
>  
> -	return xe_pcode_init(gt);
> +/**
> + * xe_pcode_probe - Prepare pcode component
> + * @gt: gt instance
> + *
> + * This function initializes the xe_pcode component.
> + * To be called once only during probe.
> + */
> +void xe_pcode_probe(struct xe_gt *gt)
> +{
> +	drmm_mutex_init(&gt_to_xe(gt)->drm, &gt->pcode.lock);
>  }
> diff --git a/drivers/gpu/drm/xe/xe_pcode.h b/drivers/gpu/drm/xe/xe_pcode.h
> index 08cb1d047cba..e597dcc8ad9f 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.h
> +++ b/drivers/gpu/drm/xe/xe_pcode.h
> @@ -9,8 +9,9 @@
>  #include <linux/types.h>
>  struct xe_gt;
>  
> -int xe_pcode_probe(struct xe_gt *gt);
> +void xe_pcode_probe(struct xe_gt *gt);
>  int xe_pcode_init(struct xe_gt *gt);
> +int xe_pcode_ready(struct xe_gt *gt, bool locked);
>  int xe_pcode_init_min_freq_table(struct xe_gt *gt, u32 min_gt_freq,
>  				 u32 max_gt_freq);
>  int xe_pcode_read(struct xe_gt *gt, u32 mbox, u32 *val, u32 *val1);
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index 9fbb6f6c598a..83c316254e43 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -121,11 +121,9 @@ int xe_pm_resume(struct xe_device *xe)
>  	for_each_tile(tile, xe, id)
>  		xe_wa_apply_tile_workarounds(tile);
>  
> -	for_each_gt(gt, xe, id) {
> -		err = xe_pcode_init(gt);
> -		if (err)
> -			return err;
> -	}
> +	err = xe_pcode_init(xe_root_mmio_gt(xe));
> +	if (err)
> +		return err;
>  
>  	xe_display_pm_resume_early(xe);
>  
> @@ -374,11 +372,9 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>  	xe->d3cold.power_lost = xe_guc_in_reset(&gt->uc.guc);
>  
>  	if (xe->d3cold.allowed && xe->d3cold.power_lost) {
> -		for_each_gt(gt, xe, id) {
> -			err = xe_pcode_init(gt);
> -			if (err)
> -				goto out;
> -		}
> +		err = xe_pcode_init(xe_root_mmio_gt(xe));
> +		if (err)
> +			goto out;
>  
>  		/*
>  		 * This only restores pinned memory which is the memory
> -- 
> 2.40.0
>