[PATCH 1/2] RFC drm/xe: check pcode init status only on root gt of root tile
Rodrigo Vivi
rodrigo.vivi at intel.com
Fri Mar 8 14:37:32 UTC 2024
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 at 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 at 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(>->pcode.lock);
> -
> err = xe_mmio_read32(gt, PCODE_MAILBOX) & PCODE_ERROR_MASK;
> if (err) {
> drm_err(>_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(>->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(>->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(>->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(>_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(>->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(>->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(>_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(>_to_xe(gt)->drm, >->pcode.lock);
> + int ret;
>
> - if (gt_to_xe(gt)->info.skip_pcode)
> - return 0;
> + mutex_lock(>->pcode.lock);
> + ret = xe_pcode_ready(gt, true);
> + mutex_unlock(>->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(>_to_xe(gt)->drm, >->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(>->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
>
More information about the Intel-xe
mailing list