[PATCH v2 2/5] drm/xe: Split lrc seqno fence creation up
Matthew Brost
matthew.brost at intel.com
Thu May 23 17:58:04 UTC 2024
On Thu, May 23, 2024 at 05:29:08PM +0200, Thomas Hellström wrote:
> Since sometimes a lock is required to initialize a seqno fence,
> and it might be desirable not to hold that lock while performing
> memory allocations, split the lrc seqno fence creation up into an
> allocation phase and an initialization phase.
>
> Since lrc seqno fences under the hood are hw_fences, do the same
> for these and remove the xe_hw_fence_create() function since it
> is not used anymore.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
I think in general this a patch it actually a really good patch to
perhaps share with the team as a learning. We (mainly me) have conflated
create / init functions frequently in Xe and I think this the 3rd or 4th
time we have had to split a function. Probably best practices going
forward would be always split these upfront.
Anyways, patch LGTM:
Reviewed-by: Matthew Brost <matthew.brost at intel.com>
> ---
> drivers/gpu/drm/xe/xe_hw_fence.c | 59 +++++++++++++++++++++++++-------
> drivers/gpu/drm/xe/xe_hw_fence.h | 7 ++--
> drivers/gpu/drm/xe/xe_lrc.c | 48 ++++++++++++++++++++++++--
> drivers/gpu/drm/xe/xe_lrc.h | 3 ++
> 4 files changed, 101 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
> index f872ef103127..35c0063a831a 100644
> --- a/drivers/gpu/drm/xe/xe_hw_fence.c
> +++ b/drivers/gpu/drm/xe/xe_hw_fence.c
> @@ -208,23 +208,58 @@ static struct xe_hw_fence *to_xe_hw_fence(struct dma_fence *fence)
> return container_of(fence, struct xe_hw_fence, dma);
> }
>
> -struct xe_hw_fence *xe_hw_fence_create(struct xe_hw_fence_ctx *ctx,
> - struct iosys_map seqno_map)
> +/**
> + * xe_hw_fence_alloc() - Allocate an hw fence.
> + *
> + * Allocate but don't initialize an hw fence.
> + *
> + * Return: Pointer to the allocated fence or
> + * negative error pointer on error.
> + */
> +struct dma_fence *xe_hw_fence_alloc(void)
> {
> - struct xe_hw_fence *fence;
> + struct xe_hw_fence *hw_fence = fence_alloc();
>
> - fence = fence_alloc();
> - if (!fence)
> + if (!hw_fence)
> return ERR_PTR(-ENOMEM);
>
> - fence->ctx = ctx;
> - fence->seqno_map = seqno_map;
> - INIT_LIST_HEAD(&fence->irq_link);
> + return &hw_fence->dma;
> +}
>
> - dma_fence_init(&fence->dma, &xe_hw_fence_ops, &ctx->irq->lock,
> - ctx->dma_fence_ctx, ctx->next_seqno++);
> +/**
> + * xe_hw_fence_free() - Free an hw fence.
> + * @fence: Pointer to the fence to free.
> + *
> + * Frees an hw fence that hasn't yet been
> + * initialized.
> + */
> +void xe_hw_fence_free(struct dma_fence *fence)
> +{
> + fence_free(&fence->rcu);
> +}
>
> - trace_xe_hw_fence_create(fence);
> +/**
> + * xe_hw_fence_init() - Initialize an hw fence.
> + * @fence: Pointer to the fence to initialize.
> + * @ctx: Pointer to the struct xe_hw_fence_ctx fence context.
> + * @seqno_map: Pointer to the map into where the seqno is blitted.
> + *
> + * Initializes a pre-allocated hw fence.
> + * After initialization, the fence is subject to normal
> + * dma-fence refcounting.
> + */
> +void xe_hw_fence_init(struct dma_fence *fence, struct xe_hw_fence_ctx *ctx,
> + struct iosys_map seqno_map)
> +{
> + struct xe_hw_fence *hw_fence =
> + container_of(fence, typeof(*hw_fence), dma);
> +
> + hw_fence->ctx = ctx;
> + hw_fence->seqno_map = seqno_map;
> + INIT_LIST_HEAD(&hw_fence->irq_link);
> +
> + dma_fence_init(fence, &xe_hw_fence_ops, &ctx->irq->lock,
> + ctx->dma_fence_ctx, ctx->next_seqno++);
>
> - return fence;
> + trace_xe_hw_fence_create(hw_fence);
> }
> diff --git a/drivers/gpu/drm/xe/xe_hw_fence.h b/drivers/gpu/drm/xe/xe_hw_fence.h
> index cfe5fd603787..f13a1c4982c7 100644
> --- a/drivers/gpu/drm/xe/xe_hw_fence.h
> +++ b/drivers/gpu/drm/xe/xe_hw_fence.h
> @@ -24,7 +24,10 @@ void xe_hw_fence_ctx_init(struct xe_hw_fence_ctx *ctx, struct xe_gt *gt,
> struct xe_hw_fence_irq *irq, const char *name);
> void xe_hw_fence_ctx_finish(struct xe_hw_fence_ctx *ctx);
>
> -struct xe_hw_fence *xe_hw_fence_create(struct xe_hw_fence_ctx *ctx,
> - struct iosys_map seqno_map);
> +struct dma_fence *xe_hw_fence_alloc(void);
>
> +void xe_hw_fence_free(struct dma_fence *fence);
> +
> +void xe_hw_fence_init(struct dma_fence *fence, struct xe_hw_fence_ctx *ctx,
> + struct iosys_map seqno_map);
> #endif
> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> index 9b0a4078add3..7441230ad627 100644
> --- a/drivers/gpu/drm/xe/xe_lrc.c
> +++ b/drivers/gpu/drm/xe/xe_lrc.c
> @@ -1030,10 +1030,54 @@ u32 xe_lrc_seqno_ggtt_addr(struct xe_lrc *lrc)
> return __xe_lrc_seqno_ggtt_addr(lrc);
> }
>
> +/**
> + * xe_lrc_alloc_seqno_fence() - Allocate an lrc seqno fence.
> + *
> + * Allocate but don't initialize an lrc seqno fence.
> + *
> + * Return: Pointer to the allocated fence or
> + * negative error pointer on error.
> + */
> +struct dma_fence *xe_lrc_alloc_seqno_fence(void)
> +{
> + return xe_hw_fence_alloc();
> +}
> +
> +/**
> + * xe_lrc_free_seqno_fence() - Free an lrc seqno fence.
> + * @fence: Pointer to the fence to free.
> + *
> + * Frees an lrc seqno fence that hasn't yet been
> + * initialized.
> + */
> +void xe_lrc_free_seqno_fence(struct dma_fence *fence)
> +{
> + xe_hw_fence_free(fence);
> +}
> +
> +/**
> + * xe_lrc_init_seqno_fence() - Initialize an lrc seqno fence.
> + * @lrc: Pointer to the lrc.
> + * @fence: Pointer to the fence to initialize.
> + *
> + * Initializes a pre-allocated lrc seqno fence.
> + * After initialization, the fence is subject to normal
> + * dma-fence refcounting.
> + */
> +void xe_lrc_init_seqno_fence(struct xe_lrc *lrc, struct dma_fence *fence)
> +{
> + xe_hw_fence_init(fence, &lrc->fence_ctx, __xe_lrc_seqno_map(lrc));
> +}
> +
> struct dma_fence *xe_lrc_create_seqno_fence(struct xe_lrc *lrc)
> {
> - return &xe_hw_fence_create(&lrc->fence_ctx,
> - __xe_lrc_seqno_map(lrc))->dma;
> + struct dma_fence *fence = xe_lrc_alloc_seqno_fence();
> +
> + if (IS_ERR(fence))
> + return fence;
> +
> + xe_lrc_init_seqno_fence(lrc, fence);
> + return fence;
> }
>
> s32 xe_lrc_seqno(struct xe_lrc *lrc)
> diff --git a/drivers/gpu/drm/xe/xe_lrc.h b/drivers/gpu/drm/xe/xe_lrc.h
> index e0e841963c23..84e4f4ef7f68 100644
> --- a/drivers/gpu/drm/xe/xe_lrc.h
> +++ b/drivers/gpu/drm/xe/xe_lrc.h
> @@ -44,6 +44,9 @@ void xe_lrc_write_ctx_reg(struct xe_lrc *lrc, int reg_nr, u32 val);
> u64 xe_lrc_descriptor(struct xe_lrc *lrc);
>
> u32 xe_lrc_seqno_ggtt_addr(struct xe_lrc *lrc);
> +struct dma_fence *xe_lrc_alloc_seqno_fence(void);
> +void xe_lrc_free_seqno_fence(struct dma_fence *fence);
> +void xe_lrc_init_seqno_fence(struct xe_lrc *lrc, struct dma_fence *fence);
> struct dma_fence *xe_lrc_create_seqno_fence(struct xe_lrc *lrc);
> s32 xe_lrc_seqno(struct xe_lrc *lrc);
>
> --
> 2.44.0
>
More information about the Intel-xe
mailing list