From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 42097C433EF for ; Thu, 28 Oct 2021 11:20:37 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 055D560230 for ; Thu, 28 Oct 2021 11:20:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 055D560230 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 48F946E952; Thu, 28 Oct 2021 11:20:30 +0000 (UTC) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by gabe.freedesktop.org (Postfix) with ESMTPS id DD24F6E42D; Thu, 28 Oct 2021 11:20:28 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10150"; a="217295681" X-IronPort-AV: E=Sophos;i="5.87,189,1631602800"; d="scan'208";a="217295681" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Oct 2021 04:20:08 -0700 X-IronPort-AV: E=Sophos;i="5.87,189,1631602800"; d="scan'208";a="529981725" Received: from jxu13-mobl.amr.corp.intel.com (HELO [10.249.254.218]) ([10.249.254.218]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Oct 2021 04:20:06 -0700 Message-ID: <5e0bb734-9e4b-9d17-2202-4eba95703d6f@linux.intel.com> Date: Thu, 28 Oct 2021 13:20:04 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0 Content-Language: en-US To: Matthew Auld , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Cc: maarten.lankhorst@linux.intel.com References: <20211027105211.485125-1-thomas.hellstrom@linux.intel.com> <20211027105211.485125-2-thomas.hellstrom@linux.intel.com> <0f548cca-214f-26c2-1628-35e6fa0d7c95@intel.com> <1d2f46030369b17405550b5ea42d0326199ad4bf.camel@linux.intel.com> <467c16ed-5efd-ed33-1e4b-18b0f447af9e@linux.intel.com> <81061bce-c651-70f0-3f3a-6d307754cb92@intel.com> From: =?UTF-8?Q?Thomas_Hellstr=c3=b6m?= In-Reply-To: <81061bce-c651-70f0-3f3a-6d307754cb92@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Intel-gfx] [PATCH v2 1/3] drm/i915: Introduce refcounted sg-tables X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On 10/28/21 11:58, Matthew Auld wrote: > On 28/10/2021 10:35, Thomas Hellström wrote: >> >> On 10/28/21 10:47, Matthew Auld wrote: >>> On 28/10/2021 08:04, Thomas Hellström wrote: >>>> On Wed, 2021-10-27 at 19:03 +0100, Matthew Auld wrote: >>>>> On 27/10/2021 11:52, Thomas Hellström wrote: >>>>>> As we start to introduce asynchronous failsafe object migration, >>>>>> where we update the object state and then submit asynchronous >>>>>> commands we need to record what memory resources are actually used >>>>>> by various part of the command stream. Initially for three >>>>>> purposes: >>>>>> >>>>>> 1) Error capture. >>>>>> 2) Asynchronous migration error recovery. >>>>>> 3) Asynchronous vma bind. >>>>>> >>>>>> At the time where these happens, the object state may have been >>>>>> updated >>>>>> to be several migrations ahead and object sg-tables discarded. >>>>>> >>>>>> In order to make it possible to keep sg-tables with memory resource >>>>>> information for these operations, introduce refcounted sg-tables >>>>>> that >>>>>> aren't freed until the last user is done with them. >>>>>> >>>>>> The alternative would be to reference information sitting on the >>>>>> corresponding ttm_resources which typically have the same lifetime >>>>>> as >>>>>> these refcountes sg_tables, but that leads to other awkward >>>>>> constructs: >>>>>> Due to the design direction chosen for ttm resource managers that >>>>>> would >>>>>> lead to diamond-style inheritance, the LMEM resources may sometimes >>>>>> be >>>>>> prematurely freed, and finally the subclassed struct ttm_resource >>>>>> would >>>>>> have to bleed into the asynchronous vma bind code. >>>>>> >>>>>> Signed-off-by: Thomas Hellström >>>>>> --- >>>>>>    drivers/gpu/drm/i915/gem/i915_gem_object.h    |   4 +- >>>>>>    .../gpu/drm/i915/gem/i915_gem_object_types.h  |   3 +- >>>>>>    drivers/gpu/drm/i915/gem/i915_gem_shmem.c     |  16 +- >>>>>>    drivers/gpu/drm/i915/gem/i915_gem_ttm.c       | 188 +++++++++++-- >>>>>> ----- >>>>>>    drivers/gpu/drm/i915/i915_scatterlist.c       |  62 ++++-- >>>>>>    drivers/gpu/drm/i915/i915_scatterlist.h       |  76 ++++++- >>>>>>    drivers/gpu/drm/i915/intel_region_ttm.c       |  15 +- >>>>>>    drivers/gpu/drm/i915/intel_region_ttm.h       |   5 +- >>>>>>    drivers/gpu/drm/i915/selftests/mock_region.c  |  12 +- >>>>>>    9 files changed, 262 insertions(+), 119 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>>>> b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>>>> index a5479ac7a4ad..c5ab364d4311 100644 >>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>>>> @@ -624,8 +624,8 @@ struct sg_table *shmem_alloc_st(struct >>>>>> drm_i915_private *i915, >>>>>>                                  size_t size, struct >>>>>> intel_memory_region *mr, >>>>>>                                  struct address_space *mapping, >>>>>>                                  unsigned int max_segment); >>>>>> -void shmem_free_st(struct sg_table *st, struct address_space >>>>>> *mapping, >>>>>> -                  bool dirty, bool backup); >>>>>> +void shmem_free_st_table(struct sg_table *st, struct address_space >>>>>> *mapping, >>>>>> +                        bool dirty, bool backup); >>>>> >>>>> s/st_table/sg_table/? >>>>> >>>>> I thought st was shorthand for sg_table? Maybe shmem_sg_free_table? >>>> >>>> Yes the naming is indeed a bit unfortunate here. To be consistent with >>>> the core's sg_free_table(), I changed to >>>> shmem_sg_free_table() / shmem_sg_alloc_table. >>>> >>>>> >>>>> >>>>>>    void __shmem_writeback(size_t size, struct address_space >>>>>> *mapping); >>>>>>       #ifdef CONFIG_MMU_NOTIFIER >>>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >>>>>> b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >>>>>> index a4b69a43b898..604ed5ad77f5 100644 >>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >>>>>> @@ -544,6 +544,7 @@ struct drm_i915_gem_object { >>>>>>                   */ >>>>>>                  struct list_head region_link; >>>>>>    +               struct i915_refct_sgt *rsgt; >>>>>>                  struct sg_table *pages; >>>>>>                  void *mapping; >>>>>>    @@ -597,7 +598,7 @@ struct drm_i915_gem_object { >>>>>>          } mm; >>>>>>             struct { >>>>>> -               struct sg_table *cached_io_st; >>>>>> +               struct i915_refct_sgt *cached_io_rsgt; >>>>>>                  struct i915_gem_object_page_iter get_io_page; >>>>>>                  struct drm_i915_gem_object *backup; >>>>>>                  bool created:1; >>>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >>>>>> b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >>>>>> index 01f332d8dbde..67c6bee695c7 100644 >>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >>>>>> @@ -25,8 +25,8 @@ static void check_release_pagevec(struct pagevec >>>>>> *pvec) >>>>>>          cond_resched(); >>>>>>    } >>>>>>    -void shmem_free_st(struct sg_table *st, struct address_space >>>>>> *mapping, >>>>>> -                  bool dirty, bool backup) >>>>>> +void shmem_free_st_table(struct sg_table *st, struct address_space >>>>>> *mapping, >>>>>> +                        bool dirty, bool backup) >>>>>>    { >>>>>>          struct sgt_iter sgt_iter; >>>>>>          struct pagevec pvec; >>>>>> @@ -49,7 +49,6 @@ void shmem_free_st(struct sg_table *st, struct >>>>>> address_space *mapping, >>>>>>                  check_release_pagevec(&pvec); >>>>>>             sg_free_table(st); >>>>>> -       kfree(st); >>>>>>    } >>>>>>       struct sg_table *shmem_alloc_st(struct drm_i915_private *i915, >>>>>> @@ -171,7 +170,8 @@ struct sg_table *shmem_alloc_st(struct >>>>>> drm_i915_private *i915, >>>>>>    err_sg: >>>>>>          sg_mark_end(sg); >>>>>>          if (sg != st->sgl) { >>>>>> -               shmem_free_st(st, mapping, false, false); >>>>>> +               shmem_free_st_table(st, mapping, false, false); >>>>>> +               kfree(st); >>>>>>          } else { >>>>>>                  mapping_clear_unevictable(mapping); >>>>>>                  sg_free_table(st); >>>>>> @@ -254,7 +254,8 @@ static int shmem_get_pages(struct >>>>>> drm_i915_gem_object *obj) >>>>>>          return 0; >>>>>>       err_pages: >>>>>> -       shmem_free_st(st, mapping, false, false); >>>>>> +       shmem_free_st_table(st, mapping, false, false); >>>>>> +       kfree(st); >>>>>>          /* >>>>>>           * shmemfs first checks if there is enough memory to >>>>>> allocate the page >>>>>>           * and reports ENOSPC should there be insufficient, along >>>>>> with the usual >>>>>> @@ -374,8 +375,9 @@ void i915_gem_object_put_pages_shmem(struct >>>>>> drm_i915_gem_object *obj, struct sg_ >>>>>>          if (i915_gem_object_needs_bit17_swizzle(obj)) >>>>>>                  i915_gem_object_save_bit_17_swizzle(obj, pages); >>>>>>    -       shmem_free_st(pages, >>>>>> file_inode(obj->base.filp)->i_mapping, >>>>>> -                     obj->mm.dirty, obj->mm.madv == >>>>>> I915_MADV_WILLNEED); >>>>>> +       shmem_free_st_table(pages, file_inode(obj->base.filp)- >>>>>>> i_mapping, >>>>>> +                           obj->mm.dirty, obj->mm.madv == >>>>>> I915_MADV_WILLNEED); >>>>>> +       kfree(pages); >>>>>>          obj->mm.dirty = false; >>>>>>    } >>>>>>    diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >>>>>> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >>>>>> index 4fd2edb20dd9..6826e3859e18 100644 >>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >>>>>> @@ -34,7 +34,7 @@ >>>>>>     * struct i915_ttm_tt - TTM page vector with additional private >>>>>> information >>>>>>     * @ttm: The base TTM page vector. >>>>>>     * @dev: The struct device used for dma mapping and unmapping. >>>>>> - * @cached_st: The cached scatter-gather table. >>>>>> + * @cached_rsgt: The cached scatter-gather table. >>>>>>     * @is_shmem: Set if using shmem. >>>>>>     * @filp: The shmem file, if using shmem backend. >>>>>>     * >>>>>> @@ -47,7 +47,7 @@ >>>>>>    struct i915_ttm_tt { >>>>>>          struct ttm_tt ttm; >>>>>>          struct device *dev; >>>>>> -       struct sg_table *cached_st; >>>>>> +       struct i915_refct_sgt cached_rsgt; >>>>>>             bool is_shmem; >>>>>>          struct file *filp; >>>>>> @@ -221,14 +221,10 @@ static int i915_ttm_tt_shmem_populate(struct >>>>>> ttm_device *bdev, >>>>>>          if (IS_ERR(st)) >>>>>>                  return PTR_ERR(st); >>>>>>    -       err = dma_map_sg_attrs(i915_tt->dev, >>>>>> -                              st->sgl, st->nents, >>>>>> -                              DMA_BIDIRECTIONAL, >>>>>> -                              DMA_ATTR_SKIP_CPU_SYNC); >>>>>> -       if (err <= 0) { >>>>>> -               err = -EINVAL; >>>>>> +       err = dma_map_sgtable(i915_tt->dev, st, DMA_BIDIRECTIONAL, >>>>>> +                             DMA_ATTR_SKIP_CPU_SYNC); >>>>>> +       if (err) >>>>>>                  goto err_free_st; >>>>>> -       } >>>>>>             i = 0; >>>>>>          for_each_sgt_page(page, sgt_iter, st) >>>>>> @@ -237,11 +233,15 @@ static int i915_ttm_tt_shmem_populate(struct >>>>>> ttm_device *bdev, >>>>>>          if (ttm->page_flags & TTM_TT_FLAG_SWAPPED) >>>>>>                  ttm->page_flags &= ~TTM_TT_FLAG_SWAPPED; >>>>>>    -       i915_tt->cached_st = st; >>>>>> +       i915_tt->cached_rsgt.table = *st; >>>>>> +       kfree(st); >>>>> >>>>> Will it work if the above just operates on the rsgt.table? >>>> >>>> I'll change the shmem utility here to not allocate the struct sg_table >>>> and then we can operate on it directly. >>>> >>>>> >>>>>> + >>>>>>          return 0; >>>>>>       err_free_st: >>>>>> -       shmem_free_st(st, filp->f_mapping, false, false); >>>>>> +       shmem_free_st_table(st, filp->f_mapping, false, false); >>>>>> +       kfree(st); >>>>>> + >>>>>>          return err; >>>>>>    } >>>>>>    @@ -249,16 +249,29 @@ static void >>>>>> i915_ttm_tt_shmem_unpopulate(struct ttm_tt *ttm) >>>>>>    { >>>>>>          struct i915_ttm_tt *i915_tt = container_of(ttm, >>>>>> typeof(*i915_tt), ttm); >>>>>>          bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED; >>>>>> +       struct sg_table *st = &i915_tt->cached_rsgt.table; >>>>>>    -       dma_unmap_sg(i915_tt->dev, i915_tt->cached_st->sgl, >>>>>> -                    i915_tt->cached_st->nents, >>>>>> -                    DMA_BIDIRECTIONAL); >>>>>> +       if (st->sgl) >>>>> >>>>> Can we ever hit this? I would have presumed not? Below also. >>>> >>>> Yes, here's where we typically free the scatterlist. >>> >>> What I meant to say: can the above ever not be true? i.e sgl == NULL >> >> Hm, Yes I think it can. If we're populating a non-shmem ttm tt, that >> sg-list is, IIRC allocated lazily on first use. Although I haven't >> analyzed in detail whether it can actually be populated and then not >> lazily allocated under the same lock. > > Oh right. I guess we could maybe drop the check in the shmem part? Ah yes, that should be safe. Will do that. Thanks. /Thomas > >> >>