[PATCH v2 2/2] drm/xe: upon page fault, use generic uc fw instead of guc
Matthew Brost
matthew.brost at intel.com
Mon Apr 28 21:03:49 UTC 2025
On Tue, Feb 18, 2025 at 01:27:02PM +0200, Dafna Hirschfeld wrote:
> Future asic will have different firmware to handle page faults.
> Change the code in xe_gt_pagefault.c to use xe_uc_fw instead of
> xe_guc and add a callback to struct xe_uc_fw to send the page
> fault replay to the fw.
>
I think this is on the right track - the current code is clearly layered
incorrectly.
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld at intel.com>
> ---
> drivers/gpu/drm/xe/xe_gt_pagefault.c | 24 ++++--------------------
> drivers/gpu/drm/xe/xe_gt_pagefault.h | 6 +++---
> drivers/gpu/drm/xe/xe_gt_types.h | 2 ++
> drivers/gpu/drm/xe/xe_guc.c | 15 +++++++++++++++
> drivers/gpu/drm/xe/xe_guc_ct.c | 8 ++++----
> drivers/gpu/drm/xe/xe_uc_fw_types.h | 5 +++++
> 6 files changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> index 51c4bcc769d2..c1e076e14219 100644
> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> @@ -11,13 +11,10 @@
> #include <drm/drm_exec.h>
> #include <drm/drm_managed.h>
>
> -#include "abi/guc_actions_abi.h"
> #include "xe_bo.h"
> #include "xe_gt.h"
> #include "xe_gt_stats.h"
> #include "xe_gt_tlb_invalidation.h"
> -#include "xe_guc.h"
> -#include "xe_guc_ct.h"
Yes, removing all GuC specific code in xe_gt_pagefaul.c is a good idea.
> #include "xe_migrate.h"
> #include "xe_trace_bo.h"
> #include "xe_vm.h"
> @@ -246,18 +243,6 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
> return err;
> }
>
> -static int send_pagefault_reply(struct xe_guc *guc,
> - struct xe_uc_pagefault_reply *reply)
> -{
> - u32 action[] = {
> - XE_GUC_ACTION_PAGE_FAULT_RES_DESC,
> - reply->dw0,
> - reply->dw1,
> - };
> -
> - return xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action), 0, 0);
> -}
> -
> static void print_pagefault(struct xe_device *xe, struct pagefault *pf)
> {
> drm_dbg(&xe->drm, "\n\tASID: %d\n"
> @@ -322,9 +307,8 @@ static bool pf_queue_full(struct pf_queue *pf_queue)
> PF_MSG_LEN_DW;
> }
>
> -int xe_guc_pagefault_handler(struct xe_guc *guc, u32 *msg, u32 len)
> +int xe_pagefault_handler(struct xe_gt *gt, struct xe_uc_fw *fw, u32 *msg, u32 len)
s/xe_pagefault_handler/xe_gt_pagefault_handler/
I think this needs a interface refactor.
This is still assuming the 'msg' field will be the same format across
firmwares which I don't think is a assumption we can safely make.
> {
> - struct xe_gt *gt = guc_to_gt(guc);
> struct xe_device *xe = gt_to_xe(gt);
> struct pf_queue *pf_queue;
> unsigned long flags;
> @@ -336,6 +320,7 @@ int xe_guc_pagefault_handler(struct xe_guc *guc, u32 *msg, u32 len)
>
> asid = FIELD_GET(PFD_ASID, msg[1]);
> pf_queue = gt->usm.pf_queue + (asid % NUM_PF_QUEUE);
> + pf_queue->fw = fw;
>
This doesn't work as if you have multiple FWs queuing up page faults
this could be pointing to the wrong firmware by the time the fault is
processed.
> /*
> * The below logic doesn't work unless PF_QUEUE_NUM_DW % PF_MSG_LEN_DW == 0
> @@ -390,7 +375,7 @@ static void pf_queue_work_func(struct work_struct *w)
> FIELD_PREP(PFR_ENG_CLASS, pf.engine_class) |
> FIELD_PREP(PFR_PDATA, pf.pdata);
>
> - send_pagefault_reply(>->uc.guc, &reply);
I also don't think it is the job GT page fault layer to encode the
response to the firmware as that could be different between firmwares
too.
So roughly I think we something along these lines...
- The firmwares parse the fields which GT page fault handler needs to
understand (page_addr, asid, access_type) and passing these in as
arguments to xe_gt_pagefault_handler
- The original FW message is also passed in, we define max length for
this, this is just opaque data to GT page fault layer
- The firmware passes in vfunc for send_pagefault_reply, this accepts
original FW message + pass / fail value so the firmware can encode the
reply
- All the above is stored in a pf_queue entry
- Internally in GT page fault layer we drop looking at firmware specifc
'struct pagefault' rather the new format which encodes page_addr,
asid, access_type, the reply vfunc, and opaque firmware data.
This will be more data in the pf_queue but it makes the GT page fault
layer truly opaque to any firmware interfaces.
Does this make sense?
Matt
> + pf_queue->fw->send_pagefault_reply(gt, pf_queue->fw, &reply);
>
> if (time_after(jiffies, threshold) &&
> pf_queue->tail != pf_queue->head) {
> @@ -664,9 +649,8 @@ static bool acc_queue_full(struct acc_queue *acc_queue)
> ACC_MSG_LEN_DW;
> }
>
> -int xe_guc_access_counter_notify_handler(struct xe_guc *guc, u32 *msg, u32 len)
> +int xe_access_counter_notify_handler(struct xe_gt *gt, struct xe_uc_fw *fw, u32 *msg, u32 len)
> {
> - struct xe_gt *gt = guc_to_gt(guc);
> struct acc_queue *acc_queue;
> u32 asid;
> bool full;
> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.h b/drivers/gpu/drm/xe/xe_gt_pagefault.h
> index 839c065a5e4c..0c01af3b7124 100644
> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.h
> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.h
> @@ -9,11 +9,11 @@
> #include <linux/types.h>
>
> struct xe_gt;
> -struct xe_guc;
> +struct xe_uc_fw;
>
> int xe_gt_pagefault_init(struct xe_gt *gt);
> void xe_gt_pagefault_reset(struct xe_gt *gt);
> -int xe_guc_pagefault_handler(struct xe_guc *guc, u32 *msg, u32 len);
> -int xe_guc_access_counter_notify_handler(struct xe_guc *guc, u32 *msg, u32 len);
> +int xe_pagefault_handler(struct xe_gt *gt, struct xe_uc_fw *fw, u32 *msg, u32 len);
> +int xe_access_counter_notify_handler(struct xe_gt *gt, struct xe_uc_fw *fw, u32 *msg, u32 len);
>
> #endif /* _XE_GT_PAGEFAULT_ */
> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> index 6e66bf0e8b3f..73f7bd044828 100644
> --- a/drivers/gpu/drm/xe/xe_gt_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> @@ -249,6 +249,8 @@ struct xe_gt {
> struct pf_queue {
> /** @usm.pf_queue.gt: back pointer to GT */
> struct xe_gt *gt;
> + /** @usm.pf_queue.fw the fw that handles the pf */
> + struct xe_uc_fw *fw;
> /** @usm.pf_queue.data: data in the page fault queue */
> u32 *data;
> /**
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index 1619c0a52db9..1ea3678f1463 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -5,6 +5,7 @@
>
> #include "xe_guc.h"
>
> +#include "xe_uc_fw_types.h"
> #include <drm/drm_managed.h>
>
> #include <generated/xe_wa_oob.h>
> @@ -642,6 +643,19 @@ static int vf_guc_init(struct xe_guc *guc)
> return 0;
> }
>
> +static int send_pagefault_reply(struct xe_gt *gt, struct xe_uc_fw *fw,
> + struct xe_uc_pagefault_reply *reply)
> +{
> + struct xe_guc *guc = container_of(fw, struct xe_guc, fw);
> + u32 action[] = {
> + XE_GUC_ACTION_PAGE_FAULT_RES_DESC,
> + reply->dw0,
> + reply->dw1,
> + };
> +
> + return xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action), 0, 0);
> +}
> +
> int xe_guc_init(struct xe_guc *guc)
> {
> struct xe_device *xe = guc_to_xe(guc);
> @@ -652,6 +666,7 @@ int xe_guc_init(struct xe_guc *guc)
> ret = xe_uc_fw_init(&guc->fw);
> if (ret)
> goto out;
> + guc->fw.send_pagefault_reply = &send_pagefault_reply;
>
> if (!xe_uc_fw_is_enabled(&guc->fw))
> return 0;
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index 72ad576fc18e..14ff5f255d45 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> @@ -1293,15 +1293,15 @@ static int process_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
> adj_len);
> break;
> case XE_GUC_ACTION_REPORT_PAGE_FAULT_REQ_DESC:
> - ret = xe_guc_pagefault_handler(guc, payload, adj_len);
> + ret = xe_pagefault_handler(gt, &guc->fw, payload, adj_len);
> break;
> case XE_GUC_ACTION_TLB_INVALIDATION_DONE:
> ret = xe_guc_tlb_invalidation_done_handler(guc, payload,
> adj_len);
> break;
> case XE_GUC_ACTION_ACCESS_COUNTER_NOTIFY:
> - ret = xe_guc_access_counter_notify_handler(guc, payload,
> - adj_len);
> + ret = xe_access_counter_notify_handler(gt, &guc->fw, payload,
> + adj_len);
> break;
> case XE_GUC_ACTION_GUC2PF_RELAY_FROM_VF:
> ret = xe_guc_relay_process_guc2pf(&guc->relay, hxg, hxg_len);
> @@ -1494,7 +1494,7 @@ static void g2h_fast_path(struct xe_guc_ct *ct, u32 *msg, u32 len)
>
> switch (action) {
> case XE_GUC_ACTION_REPORT_PAGE_FAULT_REQ_DESC:
> - ret = xe_guc_pagefault_handler(guc, payload, adj_len);
> + ret = xe_pagefault_handler(gt, &guc->fw, payload, adj_len);
> break;
> case XE_GUC_ACTION_TLB_INVALIDATION_DONE:
> __g2h_release_space(ct, len);
> diff --git a/drivers/gpu/drm/xe/xe_uc_fw_types.h b/drivers/gpu/drm/xe/xe_uc_fw_types.h
> index d8714ccb3f78..4d82430e3274 100644
> --- a/drivers/gpu/drm/xe/xe_uc_fw_types.h
> +++ b/drivers/gpu/drm/xe/xe_uc_fw_types.h
> @@ -9,6 +9,7 @@
> #include <linux/types.h>
>
> struct xe_bo;
> +struct xe_gt;
>
> /*
> * +------------+---------------------------------------------------+
> @@ -228,6 +229,10 @@ struct xe_uc_fw {
>
> /** @private_data_size: size of private data found in uC css header */
> u32 private_data_size;
> +
> + /** a callback to send the page fault replay to the fw */
> + int (*send_pagefault_reply)(struct xe_gt *gt, struct xe_uc_fw *xe_fw,
> + struct xe_uc_pagefault_reply *reply);
> };
>
> #endif
> --
> 2.34.1
>
More information about the Intel-xe
mailing list