[PATCH 1/2] drm//amdgpu: Add job fence to resv conditionally

[PATCH 1/2] drm//amdgpu: Add job fence to resv conditionally

[xinhui pan]

If a job need sync the bo resv, it is likely that bo need the job fence
to sync with others.

Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Felix Kuehling <Felix.Kuehling@amd.com>
Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: xinhui pan <xinhui.pan@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      | 5 +++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 9 +++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index b5705fcfc935..ca6021b4200b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -226,6 +226,11 @@ struct amdgpu_vm_update_params {
 	 * @num_dw_left: number of dw left for the IB
 	 */
 	unsigned int num_dw_left;
+
+	/**
+	 * @resv: sync the resv and add job fence to it conditionally.
+	 */
+	struct dma_resv *resv;
 };
 
 struct amdgpu_vm_update_funcs {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
index 4cc7881f438c..0cfac59bff36 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
@@ -70,6 +70,8 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
 
 	p->num_dw_left = ndw;
 
+	p->resv = resv;
+
 	if (!resv)
 		return 0;
 
@@ -111,6 +113,13 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
 		swap(p->vm->last_delayed, tmp);
 	dma_fence_put(tmp);
 
+	/* add job fence to resv.
+	 * MM notifier path is an exception as we can not grab the
+	 * resv lock.
+	 */
+	if (!p->direct && p->resv)
+		dma_resv_add_shared_fence(p->resv, f);
+
 	if (fence && !p->direct)
 		swap(*fence, f);
 	dma_fence_put(f);
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdgpu: unref the bo after job submit

[xinhui pan]

Otherwise we might free BOs before job has completed.
We add the fence to BO after commit, so free BOs after that.

Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Felix Kuehling <Felix.Kuehling@amd.com>
Signed-off-by: xinhui pan <xinhui.pan@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 29 +++++++++++++++++---------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 73398831196f..cdfc26535c7d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -942,13 +942,17 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
  *
  * @entry: PDE to free
  */
-static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry)
+static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry,
+					struct list_head *head)
 {
 	if (entry->base.bo) {
 		entry->base.bo->vm_bo = NULL;
 		list_del(&entry->base.vm_status);
-		amdgpu_bo_unref(&entry->base.bo->shadow);
-		amdgpu_bo_unref(&entry->base.bo);
+		if (!head) {
+			amdgpu_bo_unref(&entry->base.bo->shadow);
+			amdgpu_bo_unref(&entry->base.bo);
+		} else
+			list_add(&entry->base.vm_status, head);
 	}
 	kvfree(entry->entries);
 	entry->entries = NULL;
@@ -965,7 +969,8 @@ static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry)
  */
 static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
 			       struct amdgpu_vm *vm,
-			       struct amdgpu_vm_pt_cursor *start)
+			       struct amdgpu_vm_pt_cursor *start,
+				   struct list_head *head)
 {
 	struct amdgpu_vm_pt_cursor cursor;
 	struct amdgpu_vm_pt *entry;
@@ -973,10 +978,10 @@ static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
 	vm->bulk_moveable = false;
 
 	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, start, cursor, entry)
-		amdgpu_vm_free_table(entry);
+		amdgpu_vm_free_table(entry, head);
 
 	if (start)
-		amdgpu_vm_free_table(start->entry);
+		amdgpu_vm_free_table(start->entry, head);
 }
 
 /**
@@ -1428,7 +1433,8 @@ static void amdgpu_vm_fragment(struct amdgpu_vm_update_params *params,
  */
 static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 				 uint64_t start, uint64_t end,
-				 uint64_t dst, uint64_t flags)
+				 uint64_t dst, uint64_t flags,
+				 struct list_head *head)
 {
 	struct amdgpu_device *adev = params->adev;
 	struct amdgpu_vm_pt_cursor cursor;
@@ -1539,7 +1545,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 			 * completely covered by the range and so potentially still in use.
 			 */
 			while (cursor.pfn < frag_start) {
-				amdgpu_vm_free_pts(adev, params->vm, &cursor);
+				amdgpu_vm_free_pts(adev, params->vm, &cursor, head);
 				amdgpu_vm_pt_next(adev, &cursor);
 			}
 
@@ -1582,6 +1588,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	struct amdgpu_vm_update_params params;
 	enum amdgpu_sync_mode sync_mode;
 	int r;
+	struct list_head head;
 
 	memset(&params, 0, sizeof(params));
 	params.adev = adev;
@@ -1589,6 +1596,8 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	params.direct = direct;
 	params.pages_addr = pages_addr;
 
+	INIT_LIST_HEAD(&head);
+
 	/* Implicitly sync to command submissions in the same VM before
 	 * unmapping. Sync to moving fences before mapping.
 	 */
@@ -1617,7 +1626,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	if (r)
 		goto error_unlock;
 
-	r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
+	r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags, &head);
 	if (r)
 		goto error_unlock;
 
@@ -3118,7 +3127,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 		amdgpu_vm_free_mapping(adev, vm, mapping, NULL);
 	}
 
-	amdgpu_vm_free_pts(adev, vm, NULL);
+	amdgpu_vm_free_pts(adev, vm, NULL, NULL);
 	amdgpu_bo_unreserve(root);
 	amdgpu_bo_unref(&root);
 	WARN_ON(vm->root.base.bo);
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm//amdgpu: Add job fence to resv conditionally

[Pan, Xinhui]

wait, this causes test case in Sl+ state. need take a look.
 
> 2020年3月13日 19:53，Pan, Xinhui <Xinhui.Pan@amd.com> 写道：
> 
> If a job need sync the bo resv, it is likely that bo need the job fence
> to sync with others.
> 
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      | 5 +++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 9 +++++++++
> 2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index b5705fcfc935..ca6021b4200b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -226,6 +226,11 @@ struct amdgpu_vm_update_params {
> 	 * @num_dw_left: number of dw left for the IB
> 	 */
> 	unsigned int num_dw_left;
> +
> +	/**
> +	 * @resv: sync the resv and add job fence to it conditionally.
> +	 */
> +	struct dma_resv *resv;
> };
> 
> struct amdgpu_vm_update_funcs {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 4cc7881f438c..0cfac59bff36 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -70,6 +70,8 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
> 
> 	p->num_dw_left = ndw;
> 
> +	p->resv = resv;
> +
> 	if (!resv)
> 		return 0;
> 
> @@ -111,6 +113,13 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
> 		swap(p->vm->last_delayed, tmp);
> 	dma_fence_put(tmp);
> 
> +	/* add job fence to resv.
> +	 * MM notifier path is an exception as we can not grab the
> +	 * resv lock.
> +	 */
> +	if (!p->direct && p->resv)
> +		dma_resv_add_shared_fence(p->resv, f);
> +
> 	if (fence && !p->direct)
> 		swap(*fence, f);
> 	dma_fence_put(f);
> -- 
> 2.17.1
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm//amdgpu: Add job fence to resv conditionally

[Christian König]

Am 13.03.20 um 12:53 schrieb xinhui pan:
> If a job need sync the bo resv, it is likely that bo need the job fence
> to sync with others.

That won't work because this is the wrong resv object :)

You added the fence to the mapped BO and not the page table.

No wonder that this doesn't work,
Christian.

>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      | 5 +++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 9 +++++++++
>   2 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index b5705fcfc935..ca6021b4200b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -226,6 +226,11 @@ struct amdgpu_vm_update_params {
>   	 * @num_dw_left: number of dw left for the IB
>   	 */
>   	unsigned int num_dw_left;
> +
> +	/**
> +	 * @resv: sync the resv and add job fence to it conditionally.
> +	 */
> +	struct dma_resv *resv;
>   };
>   
>   struct amdgpu_vm_update_funcs {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 4cc7881f438c..0cfac59bff36 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -70,6 +70,8 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
>   
>   	p->num_dw_left = ndw;
>   
> +	p->resv = resv;
> +
>   	if (!resv)
>   		return 0;
>   
> @@ -111,6 +113,13 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>   		swap(p->vm->last_delayed, tmp);
>   	dma_fence_put(tmp);
>   
> +	/* add job fence to resv.
> +	 * MM notifier path is an exception as we can not grab the
> +	 * resv lock.
> +	 */
> +	if (!p->direct && p->resv)
> +		dma_resv_add_shared_fence(p->resv, f);
> +
>   	if (fence && !p->direct)
>   		swap(*fence, f);
>   	dma_fence_put(f);

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm//amdgpu: Add job fence to resv conditionally

[Pan, Xinhui]

[-- Attachment #1.1: Type: text/plain, Size: 2922 bytes --]

[AMD Official Use Only - Internal Distribution Only]

page table BOs share same resv.It should be ok using any of them, root bo resv or bo resv.
I forgot to unref bos which cause problems. not good at rebasing...


________________________________
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Friday, March 13, 2020 9:34:42 PM
To: Pan, Xinhui <Xinhui.Pan@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>
Subject: Re: [PATCH 1/2] drm//amdgpu: Add job fence to resv conditionally

Am 13.03.20 um 12:53 schrieb xinhui pan:
> If a job need sync the bo resv, it is likely that bo need the job fence
> to sync with others.

That won't work because this is the wrong resv object :)

You added the fence to the mapped BO and not the page table.

No wonder that this doesn't work,
Christian.

>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      | 5 +++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 9 +++++++++
>   2 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index b5705fcfc935..ca6021b4200b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -226,6 +226,11 @@ struct amdgpu_vm_update_params {
>         * @num_dw_left: number of dw left for the IB
>         */
>        unsigned int num_dw_left;
> +
> +     /**
> +      * @resv: sync the resv and add job fence to it conditionally.
> +      */
> +     struct dma_resv *resv;
>   };
>
>   struct amdgpu_vm_update_funcs {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 4cc7881f438c..0cfac59bff36 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -70,6 +70,8 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
>
>        p->num_dw_left = ndw;
>
> +     p->resv = resv;
> +
>        if (!resv)
>                return 0;
>
> @@ -111,6 +113,13 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>                swap(p->vm->last_delayed, tmp);
>        dma_fence_put(tmp);
>
> +     /* add job fence to resv.
> +      * MM notifier path is an exception as we can not grab the
> +      * resv lock.
> +      */
> +     if (!p->direct && p->resv)
> +             dma_resv_add_shared_fence(p->resv, f);
> +
>        if (fence && !p->direct)
>                swap(*fence, f);
>        dma_fence_put(f);


[-- Attachment #1.2: Type: text/html, Size: 5754 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm//amdgpu: Add job fence to resv conditionally

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 3563 bytes --]

Yeah, but this is still the wrong resv object :)

See the object passed to amdgpu_vm_sdma_prepare() is the one of the BO 
which is mapped into the page tables and NOT the one of the page tables.

You need to use p->vm->root.base.bo->tbo.base.resv here.

Regards,
Christian.

Am 13.03.20 um 14:43 schrieb Pan, Xinhui:
>
> [AMD Official Use Only - Internal Distribution Only]
>
>
> page table BOs share same resv.It should be ok using any of them, root 
> bo resv or bo resv.
> I forgot to unref bos which cause problems. not good at rebasing...
>
>
> ------------------------------------------------------------------------
> *From:* Koenig, Christian <Christian.Koenig@amd.com>
> *Sent:* Friday, March 13, 2020 9:34:42 PM
> *To:* Pan, Xinhui <Xinhui.Pan@amd.com>; amd-gfx@lists.freedesktop.org 
> <amd-gfx@lists.freedesktop.org>
> *Cc:* Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix 
> <Felix.Kuehling@amd.com>
> *Subject:* Re: [PATCH 1/2] drm//amdgpu: Add job fence to resv 
> conditionally
> Am 13.03.20 um 12:53 schrieb xinhui pan:
> > If a job need sync the bo resv, it is likely that bo need the job fence
> > to sync with others.
>
> That won't work because this is the wrong resv object :)
>
> You added the fence to the mapped BO and not the page table.
>
> No wonder that this doesn't work,
> Christian.
>
> >
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> > Suggested-by: Christian König <christian.koenig@amd.com>
> > Signed-off-by: xinhui pan <xinhui.pan@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      | 5 +++++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 9 +++++++++
> >   2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > index b5705fcfc935..ca6021b4200b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > @@ -226,6 +226,11 @@ struct amdgpu_vm_update_params {
> >         * @num_dw_left: number of dw left for the IB
> >         */
> >        unsigned int num_dw_left;
> > +
> > +     /**
> > +      * @resv: sync the resv and add job fence to it conditionally.
> > +      */
> > +     struct dma_resv *resv;
> >   };
> >
> >   struct amdgpu_vm_update_funcs {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> > index 4cc7881f438c..0cfac59bff36 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> > @@ -70,6 +70,8 @@ static int amdgpu_vm_sdma_prepare(struct 
> amdgpu_vm_update_params *p,
> >
> >        p->num_dw_left = ndw;
> >
> > +     p->resv = resv;
> > +
> >        if (!resv)
> >                return 0;
> >
> > @@ -111,6 +113,13 @@ static int amdgpu_vm_sdma_commit(struct 
> amdgpu_vm_update_params *p,
> >                swap(p->vm->last_delayed, tmp);
> >        dma_fence_put(tmp);
> >
> > +     /* add job fence to resv.
> > +      * MM notifier path is an exception as we can not grab the
> > +      * resv lock.
> > +      */
> > +     if (!p->direct && p->resv)
> > +             dma_resv_add_shared_fence(p->resv, f);
> > +
> >        if (fence && !p->direct)
> >                swap(*fence, f);
> >        dma_fence_put(f);
>


[-- Attachment #1.2: Type: text/html, Size: 8936 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm//amdgpu: Add job fence to resv conditionally

[Pan, Xinhui]

[-- Attachment #1.1: Type: text/plain, Size: 4044 bytes --]

Yep, will send v3.

Thanks
xinhui

发件人: "Koenig, Christian" <Christian.Koenig@amd.com>
日期: 2020年3月13日 星期五 21:46
收件人: "Pan, Xinhui" <Xinhui.Pan@amd.com>, "amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org>
抄送: "Deucher, Alexander" <Alexander.Deucher@amd.com>, "Kuehling, Felix" <Felix.Kuehling@amd.com>
主题: Re: [PATCH 1/2] drm//amdgpu: Add job fence to resv conditionally

Yeah, but this is still the wrong resv object :)

See the object passed to amdgpu_vm_sdma_prepare() is the one of the BO which is mapped into the page tables and NOT the one of the page tables.

You need to use p->vm->root.base.bo->tbo.base.resv here.

Regards,
Christian.

Am 13.03.20 um 14:43 schrieb Pan, Xinhui:

[AMD Official Use Only - Internal Distribution Only]

page table BOs share same resv.It should be ok using any of them, root bo resv or bo resv.
I forgot to unref bos which cause problems. not good at rebasing...


________________________________
From: Koenig, Christian <Christian.Koenig@amd.com><mailto:Christian.Koenig@amd.com>
Sent: Friday, March 13, 2020 9:34:42 PM
To: Pan, Xinhui <Xinhui.Pan@amd.com><mailto:Xinhui.Pan@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com><mailto:Alexander.Deucher@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com><mailto:Felix.Kuehling@amd.com>
Subject: Re: [PATCH 1/2] drm//amdgpu: Add job fence to resv conditionally

Am 13.03.20 um 12:53 schrieb xinhui pan:
> If a job need sync the bo resv, it is likely that bo need the job fence
> to sync with others.

That won't work because this is the wrong resv object :)

You added the fence to the mapped BO and not the page table.

No wonder that this doesn't work,
Christian.

>
> Cc: Christian König <christian.koenig@amd.com><mailto:christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com><mailto:alexander.deucher@amd.com>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com><mailto:Felix.Kuehling@amd.com>
> Suggested-by: Christian König <christian.koenig@amd.com><mailto:christian.koenig@amd.com>
> Signed-off-by: xinhui pan <xinhui.pan@amd.com><mailto:xinhui.pan@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      | 5 +++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 9 +++++++++
>   2 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index b5705fcfc935..ca6021b4200b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -226,6 +226,11 @@ struct amdgpu_vm_update_params {
>         * @num_dw_left: number of dw left for the IB
>         */
>        unsigned int num_dw_left;
> +
> +     /**
> +      * @resv: sync the resv and add job fence to it conditionally.
> +      */
> +     struct dma_resv *resv;
>   };
>
>   struct amdgpu_vm_update_funcs {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 4cc7881f438c..0cfac59bff36 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -70,6 +70,8 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
>
>        p->num_dw_left = ndw;
>
> +     p->resv = resv;
> +
>        if (!resv)
>                return 0;
>
> @@ -111,6 +113,13 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>                swap(p->vm->last_delayed, tmp);
>        dma_fence_put(tmp);
>
> +     /* add job fence to resv.
> +      * MM notifier path is an exception as we can not grab the
> +      * resv lock.
> +      */
> +     if (!p->direct && p->resv)
> +             dma_resv_add_shared_fence(p->resv, f);
> +
>        if (fence && !p->direct)
>                swap(*fence, f);
>        dma_fence_put(f);



[-- Attachment #1.2: Type: text/html, Size: 12269 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm//amdgpu: Add job fence to resv conditionally

[Pan, Xinhui]

[-- Attachment #1.1: Type: text/plain, Size: 2936 bytes --]

[AMD Official Use Only - Internal Distribution Only]

page table BOs share same resv.It should be ok using any of them, root bo resv or bo resv.
I forgot to unref bos which cause problems. not good at rebasing...

thanks
xinhui
________________________________
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Friday, March 13, 2020 9:34:42 PM
To: Pan, Xinhui <Xinhui.Pan@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>
Subject: Re: [PATCH 1/2] drm//amdgpu: Add job fence to resv conditionally

Am 13.03.20 um 12:53 schrieb xinhui pan:
> If a job need sync the bo resv, it is likely that bo need the job fence
> to sync with others.

That won't work because this is the wrong resv object :)

You added the fence to the mapped BO and not the page table.

No wonder that this doesn't work,
Christian.

>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      | 5 +++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 9 +++++++++
>   2 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index b5705fcfc935..ca6021b4200b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -226,6 +226,11 @@ struct amdgpu_vm_update_params {
>         * @num_dw_left: number of dw left for the IB
>         */
>        unsigned int num_dw_left;
> +
> +     /**
> +      * @resv: sync the resv and add job fence to it conditionally.
> +      */
> +     struct dma_resv *resv;
>   };
>
>   struct amdgpu_vm_update_funcs {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 4cc7881f438c..0cfac59bff36 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -70,6 +70,8 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
>
>        p->num_dw_left = ndw;
>
> +     p->resv = resv;
> +
>        if (!resv)
>                return 0;
>
> @@ -111,6 +113,13 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>                swap(p->vm->last_delayed, tmp);
>        dma_fence_put(tmp);
>
> +     /* add job fence to resv.
> +      * MM notifier path is an exception as we can not grab the
> +      * resv lock.
> +      */
> +     if (!p->direct && p->resv)
> +             dma_resv_add_shared_fence(p->resv, f);
> +
>        if (fence && !p->direct)
>                swap(*fence, f);
>        dma_fence_put(f);


[-- Attachment #1.2: Type: text/html, Size: 5674 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 0/2] fix gmc page fault on navi1X

[xinhui pan]

We hit gmc page fault on navi1X.
UMR tells that the physical address of pte is bad.
Two issues:
1) we did not sync job schedule fence while update mapping.
we sync resv, but the last fence is not there. So any wait on the resv
finished before job completed. say, bo has been released while job
running.
2) we might unref page table bo during update ptes, at the same time, there
is job pending on bo. and submit a job in commit after free bo.
We need free the bo after adding all fence to bo.

xinhui pan (2):
  drm//amdgpu: Always sync fence before unlock eviction_lock
  drm/amdgpu: unref the bo after job submit

 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 46 +++++++++++++++++++-------
 1 file changed, 34 insertions(+), 12 deletions(-)

-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdgpu: unref the bo after job submit

[xinhui pan]

Otherwise we might free BOs before job has completed.
We add the fence to BO after commit, so free BOs after that.

Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Felix Kuehling <Felix.Kuehling@amd.com>
Signed-off-by: xinhui pan <xinhui.pan@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37 +++++++++++++++++++-------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index f424b5969930..513f3b77d3da 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -942,13 +942,17 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
  *
  * @entry: PDE to free
  */
-static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry)
+static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry,
+					struct list_head *head)
 {
 	if (entry->base.bo) {
 		entry->base.bo->vm_bo = NULL;
 		list_del(&entry->base.vm_status);
-		amdgpu_bo_unref(&entry->base.bo->shadow);
-		amdgpu_bo_unref(&entry->base.bo);
+		if (!head) {
+			amdgpu_bo_unref(&entry->base.bo->shadow);
+			amdgpu_bo_unref(&entry->base.bo);
+		} else
+			list_add(&entry->base.vm_status, head);
 	}
 	kvfree(entry->entries);
 	entry->entries = NULL;
@@ -965,7 +969,8 @@ static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry)
  */
 static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
 			       struct amdgpu_vm *vm,
-			       struct amdgpu_vm_pt_cursor *start)
+			       struct amdgpu_vm_pt_cursor *start,
+				   struct list_head *head)
 {
 	struct amdgpu_vm_pt_cursor cursor;
 	struct amdgpu_vm_pt *entry;
@@ -973,10 +978,10 @@ static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
 	vm->bulk_moveable = false;
 
 	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, start, cursor, entry)
-		amdgpu_vm_free_table(entry);
+		amdgpu_vm_free_table(entry, head);
 
 	if (start)
-		amdgpu_vm_free_table(start->entry);
+		amdgpu_vm_free_table(start->entry, head);
 }
 
 /**
@@ -1428,7 +1433,8 @@ static void amdgpu_vm_fragment(struct amdgpu_vm_update_params *params,
  */
 static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 				 uint64_t start, uint64_t end,
-				 uint64_t dst, uint64_t flags)
+				 uint64_t dst, uint64_t flags,
+				 struct list_head *head)
 {
 	struct amdgpu_device *adev = params->adev;
 	struct amdgpu_vm_pt_cursor cursor;
@@ -1539,7 +1545,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 			 * completely covered by the range and so potentially still in use.
 			 */
 			while (cursor.pfn < frag_start) {
-				amdgpu_vm_free_pts(adev, params->vm, &cursor);
+				amdgpu_vm_free_pts(adev, params->vm, &cursor, head);
 				amdgpu_vm_pt_next(adev, &cursor);
 			}
 
@@ -1583,6 +1589,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	enum amdgpu_sync_mode sync_mode;
 	int r;
 	struct amdgpu_bo *root = vm->root.base.bo;
+	struct list_head head;
 
 	memset(&params, 0, sizeof(params));
 	params.adev = adev;
@@ -1590,6 +1597,8 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	params.direct = direct;
 	params.pages_addr = pages_addr;
 
+	INIT_LIST_HEAD(&head);
+
 	/* Implicitly sync to command submissions in the same VM before
 	 * unmapping. Sync to moving fences before mapping.
 	 */
@@ -1616,7 +1625,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	if (r)
 		goto error_unlock;
 
-	r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
+	r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags, &head);
 	if (r)
 		goto error_unlock;
 
@@ -1629,6 +1638,14 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 		amdgpu_bo_fence(root, vm->last_delayed, true);
 
 error_unlock:
+	while (!list_empty(&head)) {
+		struct amdgpu_vm_pt *entry = list_first_entry(&head,
+				struct amdgpu_vm_pt, base.vm_status);
+		list_del(&entry->base.vm_status);
+
+		amdgpu_bo_unref(&entry->base.bo->shadow);
+		amdgpu_bo_unref(&entry->base.bo);
+	}
 	amdgpu_vm_eviction_unlock(vm);
 	return r;
 }
@@ -3123,7 +3140,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 		amdgpu_vm_free_mapping(adev, vm, mapping, NULL);
 	}
 
-	amdgpu_vm_free_pts(adev, vm, NULL);
+	amdgpu_vm_free_pts(adev, vm, NULL, NULL);
 	amdgpu_bo_unreserve(root);
 	amdgpu_bo_unref(&root);
 	WARN_ON(vm->root.base.bo);
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx