From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: "Chauhan, Madhav" <Madhav.Chauhan@amd.com>,
"amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 2/2] drm/amdgpu: stop allocating dummy GTT nodes
Date: Wed, 8 Jul 2020 11:47:29 +0200 [thread overview]
Message-ID: <442814ae-4f3f-2db4-1daf-b1d0f164b4a7@gmail.com> (raw)
In-Reply-To: <BL0PR12MB24334238C7953A2C8C34AE279C670@BL0PR12MB2433.namprd12.prod.outlook.com>
Am 08.07.20 um 09:35 schrieb Chauhan, Madhav:
> [AMD Public Use]
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian König
> Sent: Monday, July 6, 2020 11:18 PM
> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: [PATCH 2/2] drm/amdgpu: stop allocating dummy GTT nodes
>
> Now that TTM is fixed up we can finally stop that nonsense.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 104 ++++++--------------
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 18 +++-
> 2 files changed, 42 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> index 2c20d23d62d1..62cf4fbd803a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> @@ -150,60 +150,7 @@ static int amdgpu_gtt_mgr_fini(struct ttm_mem_type_manager *man)
> */
> bool amdgpu_gtt_mgr_has_gart_addr(struct ttm_mem_reg *mem) {
> - struct amdgpu_gtt_node *node = mem->mm_node;
> -
> - return (node->node.start != AMDGPU_BO_INVALID_OFFSET);
> -}
> -
> -/**
> - * amdgpu_gtt_mgr_alloc - allocate new ranges
> - *
> - * @man: TTM memory type manager
> - * @tbo: TTM BO we need this range for
> - * @place: placement flags and restrictions
> - * @mem: the resulting mem object
> - *
> - * Allocate the address space for a node.
> - */
> -static int amdgpu_gtt_mgr_alloc(struct ttm_mem_type_manager *man,
> - struct ttm_buffer_object *tbo,
> - const struct ttm_place *place,
> - struct ttm_mem_reg *mem)
> -{
> - struct amdgpu_device *adev = amdgpu_ttm_adev(man->bdev);
> - struct amdgpu_gtt_mgr *mgr = man->priv;
> - struct amdgpu_gtt_node *node = mem->mm_node;
> - enum drm_mm_insert_mode mode;
> - unsigned long fpfn, lpfn;
> - int r;
> -
> - if (amdgpu_gtt_mgr_has_gart_addr(mem))
> - return 0;
> -
> - if (place)
> - fpfn = place->fpfn;
> - else
> - fpfn = 0;
> -
> - if (place && place->lpfn)
> - lpfn = place->lpfn;
> - else
> - lpfn = adev->gart.num_cpu_pages;
> -
> - mode = DRM_MM_INSERT_BEST;
> - if (place && place->flags & TTM_PL_FLAG_TOPDOWN)
> - mode = DRM_MM_INSERT_HIGH;
> -
> - spin_lock(&mgr->lock);
> - r = drm_mm_insert_node_in_range(&mgr->mm, &node->node, mem->num_pages,
> - mem->page_alignment, 0, fpfn, lpfn,
> - mode);
> - spin_unlock(&mgr->lock);
> -
> - if (!r)
> - mem->start = node->node.start;
> -
> - return r;
> + return mem->mm_node != NULL;
> }
>
> /**
> @@ -234,29 +181,37 @@ static int amdgpu_gtt_mgr_new(struct ttm_mem_type_manager *man,
> atomic64_sub(mem->num_pages, &mgr->available);
> spin_unlock(&mgr->lock);
>
> + if (!place->lpfn) {
> + mem->mm_node = NULL;
> + mem->start = AMDGPU_BO_INVALID_OFFSET;
> + return 0;
> + }
> +
> node = kzalloc(sizeof(*node), GFP_KERNEL);
> if (!node) {
> r = -ENOMEM;
> goto err_out;
> }
>
> - node->node.start = AMDGPU_BO_INVALID_OFFSET;
> - node->node.size = mem->num_pages;
> node->tbo = tbo;
> - mem->mm_node = node;
>
> - if (place->fpfn || place->lpfn || place->flags & TTM_PL_FLAG_TOPDOWN) {
> - r = amdgpu_gtt_mgr_alloc(man, tbo, place, mem);
> - if (unlikely(r)) {
> - kfree(node);
> - mem->mm_node = NULL;
> - goto err_out;
> - }
> - } else {
> - mem->start = node->node.start;
> - }
> + spin_lock(&mgr->lock);
> + r = drm_mm_insert_node_in_range(&mgr->mm, &node->node, mem->num_pages,
> + mem->page_alignment, 0, place->fpfn,
> + place->lpfn, DRM_MM_INSERT_BEST);
> + spin_unlock(&mgr->lock);
> +
> + if (unlikely(r))
> + goto err_free;
> +
> + mem->mm_node = node;
> + mem->start = node->node.start;
>
> return 0;
> +
> +err_free:
> + kfree(node);
> +
> err_out:
> atomic64_add(mem->num_pages, &mgr->available);
>
> @@ -279,17 +234,14 @@ static void amdgpu_gtt_mgr_del(struct ttm_mem_type_manager *man,
> struct amdgpu_gtt_mgr *mgr = man->priv;
> struct amdgpu_gtt_node *node = mem->mm_node;
>
> - if (!node)
> - return;
> -
> - spin_lock(&mgr->lock);
> - if (node->node.start != AMDGPU_BO_INVALID_OFFSET)
> + if (node) {
> + spin_lock(&mgr->lock);
> drm_mm_remove_node(&node->node);
> - spin_unlock(&mgr->lock);
> - atomic64_add(mem->num_pages, &mgr->available);
> + spin_unlock(&mgr->lock);
> + kfree(node);
> + }
>
> - kfree(node);
> - mem->mm_node = NULL;
> + atomic64_add(mem->num_pages, &mgr->available);
> }
>
> Looks fine to me, nitpick: Should we update the documentation of amdgpu_gtt_mgr_new() which still says "Dummy"??
Yes, that's probably a good point.
Thanks,
Christian.
>
> Regards,
> Madhav
>
> /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 38d2a7fb5698..50503f860fce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -429,12 +429,22 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
> }
>
> src_offset = src->offset;
> - src_mm = amdgpu_find_mm_node(src->mem, &src_offset);
> - src_node_size = (src_mm->size << PAGE_SHIFT) - src_offset;
> + if (src->mem->mm_node) {
> + src_mm = amdgpu_find_mm_node(src->mem, &src_offset);
> + src_node_size = (src_mm->size << PAGE_SHIFT) - src_offset;
> + } else {
> + src_mm = NULL;
> + src_node_size = ULLONG_MAX;
> + }
>
> dst_offset = dst->offset;
> - dst_mm = amdgpu_find_mm_node(dst->mem, &dst_offset);
> - dst_node_size = (dst_mm->size << PAGE_SHIFT) - dst_offset;
> + if (dst->mem->mm_node) {
> + dst_mm = amdgpu_find_mm_node(dst->mem, &dst_offset);
> + dst_node_size = (dst_mm->size << PAGE_SHIFT) - dst_offset;
> + } else {
> + dst_mm = NULL;
> + dst_node_size = ULLONG_MAX;
> + }
>
> mutex_lock(&adev->mman.gtt_window_lock);
>
> --
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cmadhav.chauhan%40amd.com%7C9c9a919e6d10450ac9df08d821d4c5d1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637296545052666024&sdata=DSjWjB76AeiABFB06VesHBPUp6KhaDvaQOyR%2FgZSr98%3D&reserved=0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
WARNING: multiple messages have this Message-ID (diff)
From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: "Chauhan, Madhav" <Madhav.Chauhan@amd.com>,
"amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 2/2] drm/amdgpu: stop allocating dummy GTT nodes
Date: Wed, 8 Jul 2020 11:47:29 +0200 [thread overview]
Message-ID: <442814ae-4f3f-2db4-1daf-b1d0f164b4a7@gmail.com> (raw)
In-Reply-To: <BL0PR12MB24334238C7953A2C8C34AE279C670@BL0PR12MB2433.namprd12.prod.outlook.com>
Am 08.07.20 um 09:35 schrieb Chauhan, Madhav:
> [AMD Public Use]
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian König
> Sent: Monday, July 6, 2020 11:18 PM
> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: [PATCH 2/2] drm/amdgpu: stop allocating dummy GTT nodes
>
> Now that TTM is fixed up we can finally stop that nonsense.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 104 ++++++--------------
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 18 +++-
> 2 files changed, 42 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> index 2c20d23d62d1..62cf4fbd803a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> @@ -150,60 +150,7 @@ static int amdgpu_gtt_mgr_fini(struct ttm_mem_type_manager *man)
> */
> bool amdgpu_gtt_mgr_has_gart_addr(struct ttm_mem_reg *mem) {
> - struct amdgpu_gtt_node *node = mem->mm_node;
> -
> - return (node->node.start != AMDGPU_BO_INVALID_OFFSET);
> -}
> -
> -/**
> - * amdgpu_gtt_mgr_alloc - allocate new ranges
> - *
> - * @man: TTM memory type manager
> - * @tbo: TTM BO we need this range for
> - * @place: placement flags and restrictions
> - * @mem: the resulting mem object
> - *
> - * Allocate the address space for a node.
> - */
> -static int amdgpu_gtt_mgr_alloc(struct ttm_mem_type_manager *man,
> - struct ttm_buffer_object *tbo,
> - const struct ttm_place *place,
> - struct ttm_mem_reg *mem)
> -{
> - struct amdgpu_device *adev = amdgpu_ttm_adev(man->bdev);
> - struct amdgpu_gtt_mgr *mgr = man->priv;
> - struct amdgpu_gtt_node *node = mem->mm_node;
> - enum drm_mm_insert_mode mode;
> - unsigned long fpfn, lpfn;
> - int r;
> -
> - if (amdgpu_gtt_mgr_has_gart_addr(mem))
> - return 0;
> -
> - if (place)
> - fpfn = place->fpfn;
> - else
> - fpfn = 0;
> -
> - if (place && place->lpfn)
> - lpfn = place->lpfn;
> - else
> - lpfn = adev->gart.num_cpu_pages;
> -
> - mode = DRM_MM_INSERT_BEST;
> - if (place && place->flags & TTM_PL_FLAG_TOPDOWN)
> - mode = DRM_MM_INSERT_HIGH;
> -
> - spin_lock(&mgr->lock);
> - r = drm_mm_insert_node_in_range(&mgr->mm, &node->node, mem->num_pages,
> - mem->page_alignment, 0, fpfn, lpfn,
> - mode);
> - spin_unlock(&mgr->lock);
> -
> - if (!r)
> - mem->start = node->node.start;
> -
> - return r;
> + return mem->mm_node != NULL;
> }
>
> /**
> @@ -234,29 +181,37 @@ static int amdgpu_gtt_mgr_new(struct ttm_mem_type_manager *man,
> atomic64_sub(mem->num_pages, &mgr->available);
> spin_unlock(&mgr->lock);
>
> + if (!place->lpfn) {
> + mem->mm_node = NULL;
> + mem->start = AMDGPU_BO_INVALID_OFFSET;
> + return 0;
> + }
> +
> node = kzalloc(sizeof(*node), GFP_KERNEL);
> if (!node) {
> r = -ENOMEM;
> goto err_out;
> }
>
> - node->node.start = AMDGPU_BO_INVALID_OFFSET;
> - node->node.size = mem->num_pages;
> node->tbo = tbo;
> - mem->mm_node = node;
>
> - if (place->fpfn || place->lpfn || place->flags & TTM_PL_FLAG_TOPDOWN) {
> - r = amdgpu_gtt_mgr_alloc(man, tbo, place, mem);
> - if (unlikely(r)) {
> - kfree(node);
> - mem->mm_node = NULL;
> - goto err_out;
> - }
> - } else {
> - mem->start = node->node.start;
> - }
> + spin_lock(&mgr->lock);
> + r = drm_mm_insert_node_in_range(&mgr->mm, &node->node, mem->num_pages,
> + mem->page_alignment, 0, place->fpfn,
> + place->lpfn, DRM_MM_INSERT_BEST);
> + spin_unlock(&mgr->lock);
> +
> + if (unlikely(r))
> + goto err_free;
> +
> + mem->mm_node = node;
> + mem->start = node->node.start;
>
> return 0;
> +
> +err_free:
> + kfree(node);
> +
> err_out:
> atomic64_add(mem->num_pages, &mgr->available);
>
> @@ -279,17 +234,14 @@ static void amdgpu_gtt_mgr_del(struct ttm_mem_type_manager *man,
> struct amdgpu_gtt_mgr *mgr = man->priv;
> struct amdgpu_gtt_node *node = mem->mm_node;
>
> - if (!node)
> - return;
> -
> - spin_lock(&mgr->lock);
> - if (node->node.start != AMDGPU_BO_INVALID_OFFSET)
> + if (node) {
> + spin_lock(&mgr->lock);
> drm_mm_remove_node(&node->node);
> - spin_unlock(&mgr->lock);
> - atomic64_add(mem->num_pages, &mgr->available);
> + spin_unlock(&mgr->lock);
> + kfree(node);
> + }
>
> - kfree(node);
> - mem->mm_node = NULL;
> + atomic64_add(mem->num_pages, &mgr->available);
> }
>
> Looks fine to me, nitpick: Should we update the documentation of amdgpu_gtt_mgr_new() which still says "Dummy"??
Yes, that's probably a good point.
Thanks,
Christian.
>
> Regards,
> Madhav
>
> /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 38d2a7fb5698..50503f860fce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -429,12 +429,22 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
> }
>
> src_offset = src->offset;
> - src_mm = amdgpu_find_mm_node(src->mem, &src_offset);
> - src_node_size = (src_mm->size << PAGE_SHIFT) - src_offset;
> + if (src->mem->mm_node) {
> + src_mm = amdgpu_find_mm_node(src->mem, &src_offset);
> + src_node_size = (src_mm->size << PAGE_SHIFT) - src_offset;
> + } else {
> + src_mm = NULL;
> + src_node_size = ULLONG_MAX;
> + }
>
> dst_offset = dst->offset;
> - dst_mm = amdgpu_find_mm_node(dst->mem, &dst_offset);
> - dst_node_size = (dst_mm->size << PAGE_SHIFT) - dst_offset;
> + if (dst->mem->mm_node) {
> + dst_mm = amdgpu_find_mm_node(dst->mem, &dst_offset);
> + dst_node_size = (dst_mm->size << PAGE_SHIFT) - dst_offset;
> + } else {
> + dst_mm = NULL;
> + dst_node_size = ULLONG_MAX;
> + }
>
> mutex_lock(&adev->mman.gtt_window_lock);
>
> --
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cmadhav.chauhan%40amd.com%7C9c9a919e6d10450ac9df08d821d4c5d1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637296545052666024&sdata=DSjWjB76AeiABFB06VesHBPUp6KhaDvaQOyR%2FgZSr98%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2020-07-08 9:47 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-06 17:48 [PATCH 1/2] drm/ttm: further cleanup ttm_mem_reg handling Christian König
2020-07-06 17:48 ` Christian König
2020-07-06 17:48 ` [PATCH 2/2] drm/amdgpu: stop allocating dummy GTT nodes Christian König
2020-07-06 17:48 ` Christian König
2020-07-08 7:35 ` Chauhan, Madhav
2020-07-08 7:35 ` Chauhan, Madhav
2020-07-08 9:47 ` Christian König [this message]
2020-07-08 9:47 ` Christian König
2020-07-07 19:16 ` [PATCH 1/2] drm/ttm: further cleanup ttm_mem_reg handling Chauhan, Madhav
2020-07-07 19:16 ` Chauhan, Madhav
2020-07-08 9:43 ` Christian König
2020-07-08 9:43 ` Christian König
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=442814ae-4f3f-2db4-1daf-b1d0f164b4a7@gmail.com \
--to=ckoenig.leichtzumerken@gmail.com \
--cc=Madhav.Chauhan@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=christian.koenig@amd.com \
--cc=dri-devel@lists.freedesktop.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.