From: Matthew Brost <matthew.brost@intel.com>
To: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
Cc: <intel-xe@lists.freedesktop.org>
Subject: Re: [PATCH] drm/xe: Perform dma_map when moving system buffer objects to TT
Date: Thu, 2 May 2024 17:20:43 +0000 [thread overview]
Message-ID: <ZjPLa8H08IVmR6Fe@DUT025-TGLU.fm.intel.com> (raw)
In-Reply-To: <203d2774f5f901041bb9d20efd759001f968ffdf.camel@linux.intel.com>
On Thu, May 02, 2024 at 01:31:37PM +0200, Thomas Hellström wrote:
> On Tue, 2024-04-30 at 16:47 +0000, Matthew Brost wrote:
> > On Tue, Apr 30, 2024 at 02:02:14PM +0200, Thomas Hellström wrote:
> > > Currently we dma_map on ttm_tt population and dma_unmap when
> > > the pages are released in ttm_tt unpopulate.
> > >
> > > Strictly, the dma_map is not needed until the bo is moved to the
> > > XE_PL_TT placement, so perform the dma_mapping on such moves
> > > instead, and remove the dma_mappig when moving to XE_PL_SYSTEM.
> > >
> > > This is desired for the upcoming shrinker series where shrinking
> > > of a ttm_tt might fail. That would lead to an odd construct where
> > > we first dma_unmap, then shrink and if shrinking fails dma_map
> > > again. If dma_mapping instead is performed on move like this,
> > > shrinking does not need to care at all about dma mapping.
> > >
> > > Finally, where a ttm_tt is destroyed while bound to a different
> > > memory type than XE_PL_SYSTEM, we keep the dma_unmap in
> > > unpopulate().
> > >
> >
> > Makes sense.
> >
> > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > ---
> > > drivers/gpu/drm/xe/xe_bo.c | 45 ++++++++++++++++++++++++----------
> > > ----
> > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_bo.c
> > > b/drivers/gpu/drm/xe/xe_bo.c
> > > index bc1f794e3e61..4c1dd67a4588 100644
> > > --- a/drivers/gpu/drm/xe/xe_bo.c
> > > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > > @@ -302,6 +302,18 @@ static int xe_tt_map_sg(struct ttm_tt *tt)
> > > return 0;
> > > }
> > >
> > > +static void xe_tt_unmap_sg(struct ttm_tt *tt)
> > > +{
> > > + struct xe_ttm_tt *xe_tt = container_of(tt, struct
> > > xe_ttm_tt, ttm);
> > > +
> > > + if (xe_tt->sg) {
> > > + dma_unmap_sgtable(xe_tt->dev, xe_tt->sg,
> > > + DMA_BIDIRECTIONAL, 0);
> > > + sg_free_table(xe_tt->sg);
> > > + xe_tt->sg = NULL;
> > > + }
> > > +}
> > > +
> > > struct sg_table *xe_bo_sg(struct xe_bo *bo)
> > > {
> > > struct ttm_tt *tt = bo->ttm.ttm;
> > > @@ -377,27 +389,15 @@ static int xe_ttm_tt_populate(struct
> > > ttm_device *ttm_dev, struct ttm_tt *tt,
> > > if (err)
> > > return err;
> > >
> > > - /* A follow up may move this xe_bo_move when BO is moved
> > > to XE_PL_TT */
> > > - err = xe_tt_map_sg(tt);
> > > - if (err)
> > > - ttm_pool_free(&ttm_dev->pool, tt);
> > > -
> > > return err;
> > > }
> > >
> > > static void xe_ttm_tt_unpopulate(struct ttm_device *ttm_dev,
> > > struct ttm_tt *tt)
> > > {
> > > - struct xe_ttm_tt *xe_tt = container_of(tt, struct
> > > xe_ttm_tt, ttm);
> > > -
> > > if (tt->page_flags & TTM_TT_FLAG_EXTERNAL)
> > > return;
> > >
> > > - if (xe_tt->sg) {
> > > - dma_unmap_sgtable(xe_tt->dev, xe_tt->sg,
> > > - DMA_BIDIRECTIONAL, 0);
> > > - sg_free_table(xe_tt->sg);
> > > - xe_tt->sg = NULL;
> > > - }
> > > + xe_tt_unmap_sg(tt);
> > >
> > > return ttm_pool_free(&ttm_dev->pool, tt);
> > > }
> > > @@ -628,10 +628,14 @@ static int xe_bo_move(struct
> > > ttm_buffer_object *ttm_bo, bool evict,
> > > bool handle_system_ccs = (!IS_DGFX(xe) &&
> > > xe_bo_needs_ccs_pages(bo) &&
> > > ttm && ttm_tt_is_populated(ttm))
> > > ? true : false;
> > > int ret = 0;
> > > +
> > > /* Bo creation path, moving to system or TT. */
> > > if ((!old_mem && ttm) && !handle_system_ccs) {
> > > - ttm_bo_move_null(ttm_bo, new_mem);
> > > - return 0;
> > > + if (new_mem->mem_type == XE_PL_TT)
> > > + ret = xe_tt_map_sg(ttm);
> > > + if (!ret)
> > > + ttm_bo_move_null(ttm_bo, new_mem);
> >
> > Random ranting, ttm_bo_move_null is a terrible name. It is freeing
> > the
> > old memory and assigning a new one.
>
> I guess it stems from the move operation itself being a no-op, and at
> that time the resource assignment was only a metadata update...
>
>
> >
> > > + goto out;
> > > }
> > >
> > > if (ttm_bo->type == ttm_bo_type_sg) {
> > > @@ -650,6 +654,12 @@ static int xe_bo_move(struct ttm_buffer_object
> > > *ttm_bo, bool evict,
> > > needs_clear = (ttm && ttm->page_flags &
> > > TTM_TT_FLAG_ZERO_ALLOC) ||
> > > (!ttm && ttm_bo->type == ttm_bo_type_device);
> > >
> > > + if (new_mem->mem_type == XE_PL_TT) {
> > > + ret = xe_tt_map_sg(ttm);
> > > + if (ret)
> > > + goto out;
> > > + }
> > > +
> > > if ((move_lacks_source && !needs_clear)) {
> > > ttm_bo_move_null(ttm_bo, new_mem);
> > > goto out;
> > > @@ -786,8 +796,11 @@ static int xe_bo_move(struct ttm_buffer_object
> > > *ttm_bo, bool evict,
> > > xe_pm_runtime_put(xe);
> > >
> > > out:
> > > - return ret;
> > > + if ((!ttm_bo->resource || ttm_bo->resource->mem_type ==
> > > XE_PL_SYSTEM) &&
> > > + ttm_bo->ttm)
> >
> > So this is covering the case where we have moved to system and had
> > pages.
> >
> > What about the case where evict fails after the 2nd instance of
> > 'xe_tt_map_sg' in this function. I'm guessing xe_ttm_tt_unpopulate
> > covers that case?
>
> Yes, in that case we have a struct ttm_tt in the PL_TT placement. It
> can either get moved to system, or more likely get unpopulated and
> destroyed. In the latter case, the dma-umap happens in unpopulate.
>
Thanks for the explaination.
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> /Thomas
>
>
>
> >
> > Matt
> >
> > > + xe_tt_unmap_sg(ttm_bo->ttm);
> > >
> > > + return ret;
> > > }
> > >
> > > /**
> > > --
> > > 2.44.0
> > >
>
next prev parent reply other threads:[~2024-05-02 17:21 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 12:02 [PATCH] drm/xe: Perform dma_map when moving system buffer objects to TT Thomas Hellström
2024-04-30 12:07 ` ✓ CI.Patch_applied: success for " Patchwork
2024-04-30 12:07 ` ✓ CI.checkpatch: " Patchwork
2024-04-30 12:08 ` ✓ CI.KUnit: " Patchwork
2024-04-30 16:47 ` [PATCH] " Matthew Brost
2024-05-02 11:31 ` Thomas Hellström
2024-05-02 17:20 ` Matthew Brost [this message]
2024-05-02 12:06 ` ✓ CI.Patch_applied: success for drm/xe: Perform dma_map when moving system buffer objects to TT (rev2) Patchwork
2024-05-02 12:06 ` ✓ CI.checkpatch: " Patchwork
2024-05-02 12:07 ` ✓ CI.KUnit: " Patchwork
2024-05-02 12:34 ` ✓ CI.Build: " Patchwork
2024-05-02 12:37 ` ✓ CI.Hooks: " Patchwork
2024-05-02 12:39 ` ✓ CI.checksparse: " Patchwork
2024-05-02 13:39 ` ✗ CI.BAT: failure " Patchwork
2024-05-02 15:18 ` ✗ CI.FULL: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjPLa8H08IVmR6Fe@DUT025-TGLU.fm.intel.com \
--to=matthew.brost@intel.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=thomas.hellstrom@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).