Re: [PATCH v3 1/3] drm/xe: Split H2G and G2H into separate buffer objects

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Feb 18, 2026 at 04:12:16PM -0700, Summers, Stuart wrote:
> On Tue, 2026-02-17 at 20:33 -0800, Matthew Brost wrote:
> > H2G and G2H buffers have different access patterns (H2G is CPU-write,
> > GuC-read, while G2H is GPU-write, CPU-read). On dGPU, these patterns
> > benefit from different memory placements: H2G in VRAM and G2H in
> > system
> > memory. Split the CT buffer into two separate buffers—one for H2G and
> > one for G2H—and select the optimal placement for each.
> > 
> > This provides a significant performance improvement on the G2H read
> > path, reducing a single read from ~20 µs to under 1 µs on BMG.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > 
> > ---
> > v3:
> >  - Move BO to ctbs h2g or g2h structure (Michal)
> > ---
> >  drivers/gpu/drm/xe/xe_guc_ct.c       | 67 +++++++++++++++++++-------
> > --
> >  drivers/gpu/drm/xe/xe_guc_ct_types.h |  4 +-
> >  2 files changed, 47 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c
> > b/drivers/gpu/drm/xe/xe_guc_ct.c
> > index 8a45573f8812..ea07a27757d5 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > @@ -255,6 +255,7 @@ static bool g2h_fence_needs_alloc(struct
> > g2h_fence *g2h_fence)
> 
> We should update the documentation above then also to note that this
> isn't a single blob any more.
> 

Yes, will do.

> >  
> >  #define CTB_DESC_SIZE          ALIGN(sizeof(struct
> > guc_ct_buffer_desc), SZ_2K)
> >  #define CTB_H2G_BUFFER_OFFSET  (CTB_DESC_SIZE * 2)
> > +#define CTB_G2H_BUFFER_OFFSET  (CTB_DESC_SIZE * 2)
> >  #define CTB_H2G_BUFFER_SIZE    (SZ_4K)
> >  #define CTB_H2G_BUFFER_DWORDS  (CTB_H2G_BUFFER_SIZE / sizeof(u32))
> >  #define CTB_G2H_BUFFER_SIZE    (SZ_128K)
> > @@ -279,10 +280,14 @@ long xe_guc_ct_queue_proc_time_jiffies(struct
> > xe_guc_ct *ct)
> >         return (CTB_H2G_BUFFER_SIZE / SZ_4K) * HZ;
> >  }
> >  
> > -static size_t guc_ct_size(void)
> > +static size_t guc_h2g_size(void)
> >  {
> > -       return CTB_H2G_BUFFER_OFFSET + CTB_H2G_BUFFER_SIZE +
> > -               CTB_G2H_BUFFER_SIZE;
> > +       return CTB_H2G_BUFFER_OFFSET + CTB_H2G_BUFFER_SIZE;
> > +}
> > +
> > +static size_t guc_g2h_size(void)
> > +{
> > +       return CTB_G2H_BUFFER_OFFSET + CTB_G2H_BUFFER_SIZE;
> >  }
> >  
> >  static void guc_ct_fini(struct drm_device *drm, void *arg)
> > @@ -311,7 +316,8 @@ int xe_guc_ct_init_noalloc(struct xe_guc_ct *ct)
> >         struct xe_gt *gt = ct_to_gt(ct);
> >         int err;
> >  
> > -       xe_gt_assert(gt, !(guc_ct_size() % PAGE_SIZE));
> > +       xe_gt_assert(gt, !(guc_h2g_size() % PAGE_SIZE));
> 
> For the h2g since we might relocate to device mem, should we use SZ_4K?
> Maybe should append this to the discussion in that other series rev..
> 

This is clearly broken for PAGE_SIZE != 4K. I'd rather leave the assert
as is for now for so we know this is broken and eventually fix this. It
is probably easy as changing guc_h2g_size/guc_g2h_size to align to
PAGE_SIZE. Also iGPU we never reallocate so PAGE_SIZE is actually
correct there.

> > +       xe_gt_assert(gt, !(guc_g2h_size() % PAGE_SIZE));
> 
> It would be nice to have a comment here after that discussion in the
> previous rev around using PAGE_SIZE because of the system memory
> allocation vs SZ_4K. Otherwise it seems likely we'll just run into that
> again.
>

See above, IMO different/existing issue, thus should be addressed in a
different patch.

Matt

> Thanks,
> Stuart
> 
> >  
> >         err = drmm_mutex_init(&xe->drm, &ct->lock);
> >         if (err)
> > @@ -356,7 +362,17 @@ int xe_guc_ct_init(struct xe_guc_ct *ct)
> >         struct xe_tile *tile = gt_to_tile(gt);
> >         struct xe_bo *bo;
> >  
> > -       bo = xe_managed_bo_create_pin_map(xe, tile, guc_ct_size(),
> > +       bo = xe_managed_bo_create_pin_map(xe, tile, guc_h2g_size(),
> > +                                         XE_BO_FLAG_SYSTEM |
> > +                                         XE_BO_FLAG_GGTT |
> > +                                         XE_BO_FLAG_GGTT_INVALIDATE
> > |
> > +                                        
> > XE_BO_FLAG_PINNED_NORESTORE);
> > +       if (IS_ERR(bo))
> > +               return PTR_ERR(bo);
> > +
> > +       ct->ctbs.h2g.bo = bo;
> > +
> > +       bo = xe_managed_bo_create_pin_map(xe, tile, guc_g2h_size(),
> >                                           XE_BO_FLAG_SYSTEM |
> >                                           XE_BO_FLAG_GGTT |
> >                                           XE_BO_FLAG_GGTT_INVALIDATE
> > |
> > @@ -364,7 +380,7 @@ int xe_guc_ct_init(struct xe_guc_ct *ct)
> >         if (IS_ERR(bo))
> >                 return PTR_ERR(bo);
> >  
> > -       ct->bo = bo;
> > +       ct->ctbs.g2h.bo = bo;
> >  
> >         return devm_add_action_or_reset(xe->drm.dev,
> > guc_action_disable_ct, ct);
> >  }
> > @@ -389,7 +405,7 @@ int xe_guc_ct_init_post_hwconfig(struct xe_guc_ct
> > *ct)
> >         xe_assert(xe, !xe_guc_ct_enabled(ct));
> >  
> >         if (IS_DGFX(xe)) {
> > -               ret = xe_managed_bo_reinit_in_vram(xe, tile, &ct-
> > >bo);
> > +               ret = xe_managed_bo_reinit_in_vram(xe, tile, &ct-
> > >ctbs.h2g.bo);
> >                 if (ret)
> >                         return ret;
> >         }
> > @@ -439,8 +455,7 @@ static void guc_ct_ctb_g2h_init(struct xe_device
> > *xe, struct guc_ctb *g2h,
> >         g2h->desc = IOSYS_MAP_INIT_OFFSET(map, CTB_DESC_SIZE);
> >         xe_map_memset(xe, &g2h->desc, 0, 0, sizeof(struct
> > guc_ct_buffer_desc));
> >  
> > -       g2h->cmds = IOSYS_MAP_INIT_OFFSET(map, CTB_H2G_BUFFER_OFFSET
> > +
> > -                                           CTB_H2G_BUFFER_SIZE);
> > +       g2h->cmds = IOSYS_MAP_INIT_OFFSET(map,
> > CTB_G2H_BUFFER_OFFSET);
> >  }
> >  
> >  static int guc_ct_ctb_h2g_register(struct xe_guc_ct *ct)
> > @@ -449,8 +464,8 @@ static int guc_ct_ctb_h2g_register(struct
> > xe_guc_ct *ct)
> >         u32 desc_addr, ctb_addr, size;
> >         int err;
> >  
> > -       desc_addr = xe_bo_ggtt_addr(ct->bo);
> > -       ctb_addr = xe_bo_ggtt_addr(ct->bo) + CTB_H2G_BUFFER_OFFSET;
> > +       desc_addr = xe_bo_ggtt_addr(ct->ctbs.h2g.bo);
> > +       ctb_addr = xe_bo_ggtt_addr(ct->ctbs.h2g.bo) +
> > CTB_H2G_BUFFER_OFFSET;
> >         size = ct->ctbs.h2g.info.size * sizeof(u32);
> >  
> >         err = xe_guc_self_cfg64(guc,
> > @@ -476,9 +491,8 @@ static int guc_ct_ctb_g2h_register(struct
> > xe_guc_ct *ct)
> >         u32 desc_addr, ctb_addr, size;
> >         int err;
> >  
> > -       desc_addr = xe_bo_ggtt_addr(ct->bo) + CTB_DESC_SIZE;
> > -       ctb_addr = xe_bo_ggtt_addr(ct->bo) + CTB_H2G_BUFFER_OFFSET +
> > -               CTB_H2G_BUFFER_SIZE;
> > +       desc_addr = xe_bo_ggtt_addr(ct->ctbs.g2h.bo) + CTB_DESC_SIZE;
> > +       ctb_addr = xe_bo_ggtt_addr(ct->ctbs.g2h.bo) +
> > CTB_G2H_BUFFER_OFFSET;
> >         size = ct->ctbs.g2h.info.size * sizeof(u32);
> >  
> >         err = xe_guc_self_cfg64(guc,
> > @@ -605,9 +619,12 @@ static int __xe_guc_ct_start(struct xe_guc_ct
> > *ct, bool needs_register)
> >         xe_gt_assert(gt, !xe_guc_ct_enabled(ct));
> >  
> >         if (needs_register) {
> > -               xe_map_memset(xe, &ct->bo->vmap, 0, 0, xe_bo_size(ct-
> > >bo));
> > -               guc_ct_ctb_h2g_init(xe, &ct->ctbs.h2g, &ct->bo-
> > >vmap);
> > -               guc_ct_ctb_g2h_init(xe, &ct->ctbs.g2h, &ct->bo-
> > >vmap);
> > +               xe_map_memset(xe, &ct->ctbs.h2g.bo->vmap, 0, 0,
> > +                             xe_bo_size(ct->ctbs.h2g.bo));
> > +               xe_map_memset(xe, &ct->ctbs.g2h.bo->vmap, 0, 0,
> > +                             xe_bo_size(ct->ctbs.g2h.bo));
> > +               guc_ct_ctb_h2g_init(xe, &ct->ctbs.h2g, &ct-
> > >ctbs.h2g.bo->vmap);
> > +               guc_ct_ctb_g2h_init(xe, &ct->ctbs.g2h, &ct-
> > >ctbs.g2h.bo->vmap);
> >  
> >                 err = guc_ct_ctb_h2g_register(ct);
> >                 if (err)
> > @@ -624,7 +641,7 @@ static int __xe_guc_ct_start(struct xe_guc_ct
> > *ct, bool needs_register)
> >                 ct->ctbs.h2g.info.broken = false;
> >                 ct->ctbs.g2h.info.broken = false;
> >                 /* Skip everything in H2G buffer */
> > -               xe_map_memset(xe, &ct->bo->vmap,
> > CTB_H2G_BUFFER_OFFSET, 0,
> > +               xe_map_memset(xe, &ct->ctbs.h2g.bo->vmap,
> > CTB_H2G_BUFFER_OFFSET, 0,
> >                               CTB_H2G_BUFFER_SIZE);
> >         }
> >  
> > @@ -1963,8 +1980,9 @@ static struct xe_guc_ct_snapshot
> > *guc_ct_snapshot_alloc(struct xe_guc_ct *ct, bo
> >         if (!snapshot)
> >                 return NULL;
> >  
> > -       if (ct->bo && want_ctb) {
> > -               snapshot->ctb_size = xe_bo_size(ct->bo);
> > +       if (ct->ctbs.h2g.bo && ct->ctbs.g2h.bo && want_ctb) {
> > +               snapshot->ctb_size = xe_bo_size(ct->ctbs.h2g.bo) +
> > +                       xe_bo_size(ct->ctbs.g2h.bo);
> >                 snapshot->ctb = kmalloc(snapshot->ctb_size, atomic ?
> > GFP_ATOMIC : GFP_KERNEL);
> >         }
> >  
> > @@ -2012,8 +2030,13 @@ static struct xe_guc_ct_snapshot
> > *guc_ct_snapshot_capture(struct xe_guc_ct *ct,
> >                 guc_ctb_snapshot_capture(xe, &ct->ctbs.g2h,
> > &snapshot->g2h);
> >         }
> >  
> > -       if (ct->bo && snapshot->ctb)
> > -               xe_map_memcpy_from(xe, snapshot->ctb, &ct->bo->vmap,
> > 0, snapshot->ctb_size);
> > +       if (ct->ctbs.h2g.bo && ct->ctbs.g2h.bo && snapshot->ctb) {
> > +               xe_map_memcpy_from(xe, snapshot->ctb, &ct-
> > >ctbs.h2g.bo->vmap, 0,
> > +                                  xe_bo_size(ct->ctbs.h2g.bo));
> > +               xe_map_memcpy_from(xe, snapshot->ctb + xe_bo_size(ct-
> > >ctbs.h2g.bo),
> > +                                  &ct->ctbs.g2h.bo->vmap, 0,
> > +                                  xe_bo_size(ct->ctbs.g2h.bo));
> > +       }
> >  
> >         return snapshot;
> >  }
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ct_types.h
> > b/drivers/gpu/drm/xe/xe_guc_ct_types.h
> > index 09d7ff1ef42a..46ad1402347d 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ct_types.h
> > +++ b/drivers/gpu/drm/xe/xe_guc_ct_types.h
> > @@ -39,6 +39,8 @@ struct guc_ctb_info {
> >   * struct guc_ctb - GuC command transport buffer (CTB)
> >   */
> >  struct guc_ctb {
> > +       /** @bo: Xe BO for CTB */
> > +       struct xe_bo *bo;
> >         /** @desc: dma buffer map for CTB descriptor */
> >         struct iosys_map desc;
> >         /** @cmds: dma buffer map for CTB commands */
> > @@ -126,8 +128,6 @@ struct xe_fast_req_fence {
> >   * for the H2G and G2H requests sent and received through the
> > buffers.
> >   */
> >  struct xe_guc_ct {
> > -       /** @bo: Xe BO for CT */
> > -       struct xe_bo *bo;
> >         /** @lock: protects everything in CT layer */
> >         struct mutex lock;
> >         /** @fast_lock: protects G2H channel and credits */
>