Re: [PATCH v3 04/10] drm/msm/A6xx: Implement preemption for A7XX targets

[Connor Abbott]

On Mon, Sep 9, 2024 at 2:15 PM Antonino Maniscalco
<antomani103@xxxxxxxxx> wrote:
>
> On 9/6/24 9:54 PM, Akhil P Oommen wrote:
> > On Thu, Sep 05, 2024 at 04:51:22PM +0200, Antonino Maniscalco wrote:
> >> This patch implements preemption feature for A6xx targets, this allows
> >> the GPU to switch to a higher priority ringbuffer if one is ready. A6XX
> >> hardware as such supports multiple levels of preemption granularities,
> >> ranging from coarse grained(ringbuffer level) to a more fine grained
> >> such as draw-call level or a bin boundary level preemption. This patch
> >> enables the basic preemption level, with more fine grained preemption
> >> support to follow.
> >>
> >> Signed-off-by: Sharat Masetty <smasetty@xxxxxxxxxxxxxx>
> >> Signed-off-by: Antonino Maniscalco <antomani103@xxxxxxxxx>
> >> Tested-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx> # on SM8650-QRD
> >> ---
> >>   drivers/gpu/drm/msm/Makefile              |   1 +
> >>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c     | 293 +++++++++++++++++++++-
> >>   drivers/gpu/drm/msm/adreno/a6xx_gpu.h     | 161 ++++++++++++
> ...
> >
> > we can use the lighter smp variant here.
> >
> >> +
> >> +            if (a6xx_gpu->cur_ring == ring)
> >> +                    gpu_write(gpu, REG_A6XX_CP_RB_WPTR, wptr);
> >> +            else
> >> +                    ring->skip_inline_wptr = true;
> >> +    } else {
> >> +            ring->skip_inline_wptr = true;
> >> +    }
> >> +
> >> +    spin_unlock_irqrestore(&ring->preempt_lock, flags);
> >>   }
> >>
> >>   static void get_stats_counter(struct msm_ringbuffer *ring, u32 counter,
> >> @@ -138,12 +231,14 @@ static void a6xx_set_pagetable(struct a6xx_gpu *a6xx_gpu,
> >
> > set_pagetable checks "cur_ctx_seqno" to see if pt switch is needed or
> > not. This is currently not tracked separately for each ring. Can you
> > please check that?
>
> I totally missed that. Thanks for catching it!
>
> >
> > I wonder why that didn't cause any gpu errors in testing. Not sure if I
> > am missing something.
> >
>
> I think this is because, so long as a single context doesn't submit to
> two different rings with differenr priorities, we will only be incorrect
> in the sense that we emit more page table switches than necessary and
> never less. However untrusted userspace could create a context that
> submits to two different rings and that would lead to execution in the
> wrong context so we must fix this.

FWIW, in Mesa in the future we may want to expose multiple Vulkan
queues per device. Then this would definitely blow up.

Connor

>
> >>
> >>      /*
> >>       * Write the new TTBR0 to the memstore. This is good for debugging.
> >> +     * Needed for preemption
> >>       */
> >> -    OUT_PKT7(ring, CP_MEM_WRITE, 4);
> >> +    OUT_PKT7(ring, CP_MEM_WRITE, 5);
> >>      OUT_RING(ring, CP_MEM_WRITE_0_ADDR_LO(lower_32_bits(memptr)));
> >>      OUT_RING(ring, CP_MEM_WRITE_1_ADDR_HI(upper_32_bits(memptr)));
> >>      OUT_RING(ring, lower_32_bits(ttbr));
> >> -    OUT_RING(ring, (asid << 16) | upper_32_bits(ttbr));
> >> +    OUT_RING(ring, upper_32_bits(ttbr));
> >> +    OUT_RING(ring, ctx->seqno);
> >>
> >>      /*
> >>       * Sync both threads after switching pagetables and enable BR only
> >> @@ -268,6 +363,43 @@ static void a6xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> >>      a6xx_flush(gpu, ring);
> >>   }
> ...
> >> +    struct a6xx_preempt_record *record_ptr =
> >> +            a6xx_gpu->preempt[ring->id] + PREEMPT_OFFSET_PRIV_NON_SECURE;
> >> +    u64 ttbr0 = ring->memptrs->ttbr0;
> >> +    u32 context_idr = ring->memptrs->context_idr;
> >> +
> >> +    smmu_info_ptr->ttbr0 = ttbr0;
> >> +    smmu_info_ptr->context_idr = context_idr;
> >> +    record_ptr->wptr = get_wptr(ring);
> >> +
> >> +    /*
> >> +     * The GPU will write the wptr we set above when we preempt. Reset
> >> +     * skip_inline_wptr to make sure that we don't write WPTR to the same
> >> +     * thing twice. It's still possible subsequent submissions will update
> >> +     * wptr again, in which case they will set the flag to true. This has
> >> +     * to be protected by the lock for setting the flag and updating wptr
> >> +     * to be atomic.
> >> +     */
> >> +    ring->skip_inline_wptr = false;
> >> +
> >> +    spin_unlock_irqrestore(&ring->preempt_lock, flags);
> >> +
> >> +    gpu_write64(gpu,
> >> +            REG_A6XX_CP_CONTEXT_SWITCH_SMMU_INFO,
> >> +            a6xx_gpu->preempt_iova[ring->id] + PREEMPT_OFFSET_SMMU_INFO);
> >> +
> >> +    gpu_write64(gpu,
> >> +            REG_A6XX_CP_CONTEXT_SWITCH_PRIV_NON_SECURE_RESTORE_ADDR,
> >> +            a6xx_gpu->preempt_iova[ring->id] + PREEMPT_OFFSET_PRIV_NON_SECURE);
> >> +
> >> +    preempt_offset_priv_secure =
> >> +            PREEMPT_OFFSET_PRIV_SECURE(adreno_gpu->info->preempt_record_size);
> >> +    gpu_write64(gpu,
> >> +            REG_A6XX_CP_CONTEXT_SWITCH_PRIV_SECURE_RESTORE_ADDR,
> >> +            a6xx_gpu->preempt_iova[ring->id] + preempt_offset_priv_secure);
> >
> > Secure buffers are not supported currently, so we can skip this and the
> > context record allocation. Anyway this has to be a separate buffer
> > mapped in secure pagetable which don't currently have. We can skip the
> > same in pseudo register packet too.
> >
>
> Mmm it would appear that not setting it causes an hang very early. I'll
> see if I can find out more about what is going on.
>
> >> +
> >> +    a6xx_gpu->next_ring = ring;
> >> +
> ...
> >>
> >>   struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu *gpu, int id,
> >>
> >> --
> >> 2.46.0
> >>
>
> Best regards,
> --
> Antonino Maniscalco <antomani103@xxxxxxxxx>
>