[PATCH] drm/xe: move the kernel lrc from hwe to execlist port

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Ilia Levi
Sent: Thursday, August 22, 2024 4:03 AM
To: intel-xe at lists.freedesktop.org
Cc: Levi, Ilia <ilia.levi at intel.com>; Vishwanathapura, Niranjana <niranjana.vishwanathapura at intel.com>; Brost, Matthew <matthew.brost at intel.com>; Elbaz, Koby <koby.elbaz at intel.com>; Avizrat, Yaron <yaron.avizrat at intel.com>
Subject: [PATCH] drm/xe: move the kernel lrc from hwe to execlist port
> 
> The kernel lrc is used solely by the execlist infra.
> Move it to the execlist port struct and initialize it only when
> execlists are used.
> 
> Signed-off-by: Ilia Levi <ilia.levi at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_execlist.c        | 10 ++++++++--
>  drivers/gpu/drm/xe/xe_execlist_types.h  |  2 ++
>  drivers/gpu/drm/xe/xe_hw_engine.c       | 15 ++-------------
>  drivers/gpu/drm/xe/xe_hw_engine_types.h |  2 --
>  4 files changed, 12 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_execlist.c b/drivers/gpu/drm/xe/xe_execlist.c
> index 7502e3486eaf..447c290cb02b 100644
> --- a/drivers/gpu/drm/xe/xe_execlist.c
> +++ b/drivers/gpu/drm/xe/xe_execlist.c
> @@ -123,8 +123,8 @@ static void __xe_execlist_port_idle(struct xe_execlist_port *port)
>  	if (!port->running_exl)
>  		return;
>  
> -	xe_lrc_write_ring(port->hwe->kernel_lrc, noop, sizeof(noop));
> -	__start_lrc(port->hwe, port->hwe->kernel_lrc, 0);
> +	xe_lrc_write_ring(port->lrc, noop, sizeof(noop));
> +	__start_lrc(port->hwe, port->lrc, 0);
>  	port->running_exl = NULL;
>  }
>  
> @@ -262,6 +262,10 @@ struct xe_execlist_port *xe_execlist_port_create(struct xe_device *xe,
>  
>  	port->hwe = hwe;
>  
> +	port->lrc = xe_lrc_create(hwe, NULL, SZ_16K);
> +	if (IS_ERR(port->lrc))
> +		return ERR_PTR(PTR_ERR(port->lrc));

Nit:
I get that just returning port->lrc isn't possible because the compiler would complain about
returning an incorrect data structure (xe_lrc vs xe_execlist_port), but surely there's a better
way of passing the error pointer forward?

Maybe it would look better if it was separated?

"""
	if (IS_ERR(port->lrc)) {
		int err = PTR_ERR(port->lrc);
		return ERR_PTR(err);
	}
"""

The only other alternatives I can think of would involve several large-scale changes to
how xe_lrc_create works, so what you're already doing is probably the best option.

Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt


> +
>  	spin_lock_init(&port->lock);
>  	for (i = 0; i < ARRAY_SIZE(port->active); i++)
>  		INIT_LIST_HEAD(&port->active[i]);
> @@ -287,6 +291,8 @@ void xe_execlist_port_destroy(struct xe_execlist_port *port)
>  	spin_lock_irq(&gt_to_xe(port->hwe->gt)->irq.lock);
>  	port->hwe->irq_handler = NULL;
>  	spin_unlock_irq(&gt_to_xe(port->hwe->gt)->irq.lock);
> +
> +	xe_lrc_put(port->lrc);
>  }
>  
>  static struct dma_fence *
> diff --git a/drivers/gpu/drm/xe/xe_execlist_types.h b/drivers/gpu/drm/xe/xe_execlist_types.h
> index f94bbf4c53e4..415140936f11 100644
> --- a/drivers/gpu/drm/xe/xe_execlist_types.h
> +++ b/drivers/gpu/drm/xe/xe_execlist_types.h
> @@ -27,6 +27,8 @@ struct xe_execlist_port {
>  	struct xe_execlist_exec_queue *running_exl;
>  
>  	struct timer_list irq_fail;
> +
> +	struct xe_lrc *lrc;
>  };
>  
>  struct xe_execlist_exec_queue {
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index aa9b2b16a6e0..35273192895c 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -273,7 +273,6 @@ static void hw_engine_fini(struct drm_device *drm, void *arg)
>  
>  	if (hwe->exl_port)
>  		xe_execlist_port_destroy(hwe->exl_port);
> -	xe_lrc_put(hwe->kernel_lrc);
>  
>  	hwe->gt = NULL;
>  }
> @@ -558,21 +557,13 @@ static int hw_engine_init(struct xe_gt *gt, struct xe_hw_engine *hwe,
>  		goto err_name;
>  	}
>  
> -	hwe->kernel_lrc = xe_lrc_create(hwe, NULL, SZ_16K);
> -	if (IS_ERR(hwe->kernel_lrc)) {
> -		err = PTR_ERR(hwe->kernel_lrc);
> -		goto err_hwsp;
> -	}
> -
>  	if (!xe_device_uc_enabled(xe)) {
>  		hwe->exl_port = xe_execlist_port_create(xe, hwe);
>  		if (IS_ERR(hwe->exl_port)) {
>  			err = PTR_ERR(hwe->exl_port);
> -			goto err_kernel_lrc;
> +			goto err_hwsp;
>  		}
> -	}
> -
> -	if (xe_device_uc_enabled(xe)) {
> +	} else {
>  		/* GSCCS has a special interrupt for reset */
>  		if (hwe->class == XE_ENGINE_CLASS_OTHER)
>  			hwe->irq_handler = xe_gsc_hwe_irq_handler;
> @@ -587,8 +578,6 @@ static int hw_engine_init(struct xe_gt *gt, struct xe_hw_engine *hwe,
>  
>  	return drmm_add_action_or_reset(&xe->drm, hw_engine_fini, hwe);
>  
> -err_kernel_lrc:
> -	xe_lrc_put(hwe->kernel_lrc);
>  err_hwsp:
>  	xe_bo_unpin_map_no_vm(hwe->hwsp);
>  err_name:
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine_types.h b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> index 39f24012d0f4..8be6d420ece4 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine_types.h
> +++ b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> @@ -136,8 +136,6 @@ struct xe_hw_engine {
>  	enum xe_force_wake_domains domain;
>  	/** @hwsp: hardware status page buffer object */
>  	struct xe_bo *hwsp;
> -	/** @kernel_lrc: Kernel LRC (should be replaced /w an xe_engine) */
> -	struct xe_lrc *kernel_lrc;
>  	/** @exl_port: execlists port */
>  	struct xe_execlist_port *exl_port;
>  	/** @fence_irq: fence IRQ to run when a hw engine IRQ is received */
> -- 
> 2.43.2
> 
>