[Intel-xe] [PATCH 1/6] drm/xe: Use a define to set initial seqno for fences

[Matthew Auld]

On Tue, 14 Mar 2023 at 13:03, Thomas Hellström
<thomas.hellstrom at linux.intel.com> wrote:
>
>
> On 3/14/23 12:54, Matthew Auld wrote:
> > On Mon, 13 Mar 2023 at 20:46, Thomas Hellström
> > <thomas.hellstrom at linux.intel.com> wrote:
> >> Also for HW fences, write the initial seqno - 1 to the HW completed
> >> seqno to initialize.
> >>
> >> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> >> ---
> >>   drivers/gpu/drm/xe/xe_engine.c   | 5 +++--
> >>   drivers/gpu/drm/xe/xe_hw_fence.c | 5 ++---
> >>   drivers/gpu/drm/xe/xe_hw_fence.h | 2 ++
> >>   drivers/gpu/drm/xe/xe_lrc.c      | 3 +++
> >>   4 files changed, 10 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_engine.c b/drivers/gpu/drm/xe/xe_engine.c
> >> index 3b6f8a25112a..7e6367a84f1f 100644
> >> --- a/drivers/gpu/drm/xe/xe_engine.c
> >> +++ b/drivers/gpu/drm/xe/xe_engine.c
> >> @@ -13,6 +13,7 @@
> >>
> >>   #include "xe_device.h"
> >>   #include "xe_gt.h"
> >> +#include "xe_hw_fence.h"
> >>   #include "xe_lrc.h"
> >>   #include "xe_macros.h"
> >>   #include "xe_migrate.h"
> >> @@ -57,11 +58,11 @@ static struct xe_engine *__xe_engine_create(struct xe_device *xe,
> >>
> >>          if (xe_engine_is_parallel(e)) {
> >>                  e->parallel.composite_fence_ctx = dma_fence_context_alloc(1);
> >> -               e->parallel.composite_fence_seqno = 1;
> >> +               e->parallel.composite_fence_seqno = XE_FENCE_INITIAL_SEQNO;
> >>          }
> >>          if (e->flags & ENGINE_FLAG_VM) {
> >>                  e->bind.fence_ctx = dma_fence_context_alloc(1);
> >> -               e->bind.fence_seqno = 1;
> >> +               e->bind.fence_seqno = XE_FENCE_INITIAL_SEQNO;
> >>          }
> >>
> >>          for (i = 0; i < width; ++i) {
> >> diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
> >> index e56ca2867545..ca8304650713 100644
> >> --- a/drivers/gpu/drm/xe/xe_hw_fence.c
> >> +++ b/drivers/gpu/drm/xe/xe_hw_fence.c
> >> @@ -129,7 +129,7 @@ void xe_hw_fence_ctx_init(struct xe_hw_fence_ctx *ctx, struct xe_gt *gt,
> >>          ctx->gt = gt;
> >>          ctx->irq = irq;
> >>          ctx->dma_fence_ctx = dma_fence_context_alloc(1);
> >> -       ctx->next_seqno = 1;
> >> +       ctx->next_seqno = XE_FENCE_INITIAL_SEQNO;
> >>          sprintf(ctx->name, "%s", name);
> >>   }
> >>
> >> @@ -164,8 +164,7 @@ static bool xe_hw_fence_signaled(struct dma_fence *dma_fence)
> >>          struct xe_device *xe = gt_to_xe(fence->ctx->gt);
> >>          u32 seqno = xe_map_rd(xe, &fence->seqno_map, 0, u32);
> >>
> >> -       return dma_fence->error ||
> >> -               (s32)fence->dma.seqno <= (s32)seqno;
> >> +       return dma_fence->error || (s32)(u32)fence->dma.seqno <= (s32)seqno;
> > Why do we need this change? Also the hw fence dma.seqno should always
> > be u32, right?
>
> Thanks for reviewing, Matt.
>
> Yes the value should always be u32, but the type is u64, and I read
> somewhere that casting an u64 to s32 could yield
> implementation-dependent results. I could double-check that, or actually
> even use __dma_fence_is_later(). That seems to be the best choice.

Ok, sounds good.

>
> /Thomas
>
>
>
> >
> >>   }
> >>
> >>   static bool xe_hw_fence_enable_signaling(struct dma_fence *dma_fence)
> >> diff --git a/drivers/gpu/drm/xe/xe_hw_fence.h b/drivers/gpu/drm/xe/xe_hw_fence.h
> >> index 07f202db6526..523c2611ef5d 100644
> >> --- a/drivers/gpu/drm/xe/xe_hw_fence.h
> >> +++ b/drivers/gpu/drm/xe/xe_hw_fence.h
> >> @@ -8,6 +8,8 @@
> >>
> >>   #include "xe_hw_fence_types.h"
> >>
> >> +#define XE_FENCE_INITIAL_SEQNO 1
> >> +
> >>   int xe_hw_fence_module_init(void);
> >>   void xe_hw_fence_module_exit(void);
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> >> index 9140b057a5ba..fb8c6f7d6528 100644
> >> --- a/drivers/gpu/drm/xe/xe_lrc.c
> >> +++ b/drivers/gpu/drm/xe/xe_lrc.c
> >> @@ -697,6 +697,9 @@ int xe_lrc_init(struct xe_lrc *lrc, struct xe_hw_engine *hwe,
> >>          arb_enable = MI_ARB_ON_OFF | MI_ARB_ENABLE;
> >>          xe_lrc_write_ring(lrc, &arb_enable, sizeof(arb_enable));
> >>
> >> +       map = __xe_lrc_seqno_map(lrc);
> >> +       xe_map_write32(lrc_to_xe(lrc), &map, lrc->fence_ctx.next_seqno - 1);
> >> +
> >>          return 0;
> >>
> >>   err_lrc_finish:
> >> --
> >> 2.39.2
> >>