[PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Jacob He]

SPM access the video memory according to SPM_VMID. It should be updated
with the job's vmid right before the job is scheduled. SPM_VMID is a
global resource

Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
Signed-off-by: Jacob He <jacob.he@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index c00696f3017e..c761d3a0b6e8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 	struct dma_fence *fence = NULL;
 	bool pasid_mapping_needed = false;
 	unsigned patch_offset = 0;
+	bool update_spm_vmid_needed = (job->vm && (job->vm->reserved_vmid[vmhub] != NULL));
 	int r;
 
+	if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
+		adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
+
 	if (amdgpu_vmid_had_gpu_reset(adev, id)) {
 		gds_switch_needed = true;
 		vm_flush_needed = true;
-- 
2.17.1

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

RE: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[He, Jacob]

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

[AMD Official Use Only - Internal Distribution Only]

Could anyone help to review the patch?

Thanks
Jacob

________________________________
From: He, Jacob <Jacob.He@amd.com>
Sent: Monday, March 2, 2020 1:35:29 PM
To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: He, Jacob <Jacob.He@amd.com>
Subject: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

SPM access the video memory according to SPM_VMID. It should be updated
with the job's vmid right before the job is scheduled. SPM_VMID is a
global resource

Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
Signed-off-by: Jacob He <jacob.he@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index c00696f3017e..c761d3a0b6e8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
         struct dma_fence *fence = NULL;
         bool pasid_mapping_needed = false;
         unsigned patch_offset = 0;
+       bool update_spm_vmid_needed = (job->vm && (job->vm->reserved_vmid[vmhub] != NULL));
         int r;

+       if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
+               adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
+
         if (amdgpu_vmid_had_gpu_reset(adev, id)) {
                 gds_switch_needed = true;
                 vm_flush_needed = true;
--
2.17.1


[-- Attachment #1.2: Type: text/html, Size: 4412 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] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Christian König]

Am 02.03.20 um 06:35 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He <jacob.he@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..c761d3a0b6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>   	struct dma_fence *fence = NULL;
>   	bool pasid_mapping_needed = false;
>   	unsigned patch_offset = 0;
> +	bool update_spm_vmid_needed = (job->vm && (job->vm->reserved_vmid[vmhub] != NULL));
>   	int r;
>   
> +	if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> +		adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +

It would be better if we could do that asynchronously with a register 
write on the ring.

The alternative is that we block for the VM to be idle in 
amdgpu_vm_ioctl() before unreserving the VMID.

In other words lock the reservation object of the root PD and call 
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().

Regards,
Christian.

>   	if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>   		gds_switch_needed = true;
>   		vm_flush_needed = true;

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

RE: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[He, Jacob]

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

[AMD Official Use Only - Internal Distribution Only]

It would be better if we could do that asynchronously with a register
write on the ring.

Sorry, I don’t get your point. Could you please elaborate more?

Thanks
Jacob

From: Christian König<mailto:ckoenig.leichtzumerken@gmail.com>
Sent: Tuesday, March 3, 2020 10:16 PM
To: He, Jacob<mailto:Jacob.He@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

Am 02.03.20 um 06:35 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He <jacob.he@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..c761d3a0b6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>        struct dma_fence *fence = NULL;
>        bool pasid_mapping_needed = false;
>        unsigned patch_offset = 0;
> +     bool update_spm_vmid_needed = (job->vm && (job->vm->reserved_vmid[vmhub] != NULL));
>        int r;
>
> +     if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> +             adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +

It would be better if we could do that asynchronously with a register
write on the ring.

The alternative is that we block for the VM to be idle in
amdgpu_vm_ioctl() before unreserving the VMID.

In other words lock the reservation object of the root PD and call
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().

Regards,
Christian.

>        if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>                gds_switch_needed = true;
>                vm_flush_needed = true;


[-- Attachment #1.2: Type: text/html, Size: 5236 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] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Christian König]

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

Am 03.03.20 um 15:34 schrieb He, Jacob:
>
> [AMD Official Use Only - Internal Distribution Only]
>
>
> /It would be better if we could do that asynchronously with a register
> write on the ring.
>
> /
>
> Sorry, I don’t get your point. Could you please elaborate more?
>

You pass the ring from amdgpu_vm_flush() to the *_update_spm_vmid() 
functions.

And then instead of using WREG32() you call amdgpu_ring_emit_wreg() to 
make the write asynchronously on the ring buffer using a CP command.

This way we avoid a bunch of trouble when one process drops the VMID 
reservation and another one grabs it.

Regards,
Christian.

> Thanks
>
> Jacob
>
> *From: *Christian König <mailto:ckoenig.leichtzumerken@gmail.com>
> *Sent: *Tuesday, March 3, 2020 10:16 PM
> *To: *He, Jacob <mailto:Jacob.He@amd.com>; 
> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
> *Subject: *Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid 
> when application reserves the vmid
>
> Am 02.03.20 um 06:35 schrieb Jacob He:
> > SPM access the video memory according to SPM_VMID. It should be updated
> > with the job's vmid right before the job is scheduled. SPM_VMID is a
> > global resource
> >
> > Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> > Signed-off-by: Jacob He <jacob.he@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index c00696f3017e..c761d3a0b6e8 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, 
> struct amdgpu_job *job,
> >        struct dma_fence *fence = NULL;
> >        bool pasid_mapping_needed = false;
> >        unsigned patch_offset = 0;
> > +     bool update_spm_vmid_needed = (job->vm && 
> (job->vm->reserved_vmid[vmhub] != NULL));
> >        int r;
> >
> > +     if (update_spm_vmid_needed && 
> adev->gfx.rlc.funcs->update_spm_vmid)
> > + adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> > +
>
> It would be better if we could do that asynchronously with a register
> write on the ring.
>
> The alternative is that we block for the VM to be idle in
> amdgpu_vm_ioctl() before unreserving the VMID.
>
> In other words lock the reservation object of the root PD and call
> amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().
>
> Regards,
> Christian.
>
> >        if (amdgpu_vmid_had_gpu_reset(adev, id)) {
> >                gds_switch_needed = true;
> >                vm_flush_needed = true;
>


[-- Attachment #1.2: Type: text/html, Size: 6997 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] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[He, Jacob]

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

[AMD Official Use Only - Internal Distribution Only]

Thanks!  Could you please take an example of trouble  “This way we avoid a bunch of trouble when one process drops the VMID reservation and another one grabs it.”?

Thanks
Jacob

From: Koenig, Christian<mailto:Christian.Koenig@amd.com>
Sent: Tuesday, March 3, 2020 11:03 PM
To: He, Jacob<mailto:Jacob.He@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

Am 03.03.20 um 15:34 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]

It would be better if we could do that asynchronously with a register
write on the ring.
Sorry, I don’t get your point. Could you please elaborate more?

You pass the ring from amdgpu_vm_flush() to the *_update_spm_vmid() functions.

And then instead of using WREG32() you call amdgpu_ring_emit_wreg() to make the write asynchronously on the ring buffer using a CP command.

This way we avoid a bunch of trouble when one process drops the VMID reservation and another one grabs it.

Regards,
Christian.



Thanks
Jacob

From: Christian König<mailto:ckoenig.leichtzumerken@gmail.com>
Sent: Tuesday, March 3, 2020 10:16 PM
To: He, Jacob<mailto:Jacob.He@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

Am 02.03.20 um 06:35 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He <jacob.he@amd.com><mailto:jacob.he@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..c761d3a0b6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>        struct dma_fence *fence = NULL;
>        bool pasid_mapping_needed = false;
>        unsigned patch_offset = 0;
> +     bool update_spm_vmid_needed = (job->vm && (job->vm->reserved_vmid[vmhub] != NULL));
>        int r;
>
> +     if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> +             adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +

It would be better if we could do that asynchronously with a register
write on the ring.

The alternative is that we block for the VM to be idle in
amdgpu_vm_ioctl() before unreserving the VMID.

In other words lock the reservation object of the root PD and call
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().

Regards,
Christian.

>        if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>                gds_switch_needed = true;
>                vm_flush_needed = true;




[-- Attachment #1.2: Type: text/html, Size: 8150 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] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Christian König]

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

See the SPM buffer address is set using CP commands as well, right? And 
those execute asynchronously.

When we now synchronously update the SPM VMID we risk that we switch 
from one process to another while the new process is not ready yet with 
its setup.

That could have quite a bunch of unforeseen consequences, including 
accidentally writing SPM data into the new process address space at 
whatever buffer address was used before.

This is something we at least should try to avoid.

Regards,
Christian.

Am 03.03.20 um 16:28 schrieb He, Jacob:
>
> [AMD Official Use Only - Internal Distribution Only]
>
>
> Thanks!  Could you please take an example of trouble  “This way we 
> avoid a bunch of trouble when one process drops the VMID reservation 
> and another one grabs it.”?
>
> Thanks
>
> Jacob
>
> *From: *Koenig, Christian <mailto:Christian.Koenig@amd.com>
> *Sent: *Tuesday, March 3, 2020 11:03 PM
> *To: *He, Jacob <mailto:Jacob.He@amd.com>; 
> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
> *Subject: *Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid 
> when application reserves the vmid
>
> Am 03.03.20 um 15:34 schrieb He, Jacob:
>
>     [AMD Official Use Only - Internal Distribution Only]
>
>     /It would be better if we could do that asynchronously with a
>     register
>     write on the ring./
>
>     Sorry, I don’t get your point. Could you please elaborate more?
>
>
> You pass the ring from amdgpu_vm_flush() to the *_update_spm_vmid() 
> functions.
>
> And then instead of using WREG32() you call amdgpu_ring_emit_wreg() to 
> make the write asynchronously on the ring buffer using a CP command.
>
> This way we avoid a bunch of trouble when one process drops the VMID 
> reservation and another one grabs it.
>
> Regards,
> Christian.
>
>
>     Thanks
>
>     Jacob
>
>     *From: *Christian König <mailto:ckoenig.leichtzumerken@gmail.com>
>     *Sent: *Tuesday, March 3, 2020 10:16 PM
>     *To: *He, Jacob <mailto:Jacob.He@amd.com>;
>     amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>     *Subject: *Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's
>     vmid when application reserves the vmid
>
>     Am 02.03.20 um 06:35 schrieb Jacob He:
>     > SPM access the video memory according to SPM_VMID. It should be
>     updated
>     > with the job's vmid right before the job is scheduled. SPM_VMID is a
>     > global resource
>     >
>     > Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
>     > Signed-off-by: Jacob He <jacob.he@amd.com> <mailto:jacob.he@amd.com>
>     > ---
>     >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>     >   1 file changed, 4 insertions(+)
>     >
>     > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>     b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>     > index c00696f3017e..c761d3a0b6e8 100644
>     > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>     > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>     > @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring
>     *ring, struct amdgpu_job *job,
>     >        struct dma_fence *fence = NULL;
>     >        bool pasid_mapping_needed = false;
>     >        unsigned patch_offset = 0;
>     > +     bool update_spm_vmid_needed = (job->vm &&
>     (job->vm->reserved_vmid[vmhub] != NULL));
>     >        int r;
>     >
>     > +     if (update_spm_vmid_needed &&
>     adev->gfx.rlc.funcs->update_spm_vmid)
>     > + adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
>     > +
>
>     It would be better if we could do that asynchronously with a register
>     write on the ring.
>
>     The alternative is that we block for the VM to be idle in
>     amdgpu_vm_ioctl() before unreserving the VMID.
>
>     In other words lock the reservation object of the root PD and call
>     amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().
>
>     Regards,
>     Christian.
>
>     >        if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>     >                gds_switch_needed = true;
>     >                vm_flush_needed = true;
>


[-- Attachment #1.2: Type: text/html, Size: 11040 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] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[He, Jacob]

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

[AMD Official Use Only - Internal Distribution Only]

Oh, you are right! If SPM_VMID is updated by other process while the SPM enabled commands is executing, that will cause VM fault.

Is the wait vm idle right before unreserve vmid still necessary if using asynchroneously setting SPM_VMID?

Thanks
Jacob

From: Koenig, Christian<mailto:Christian.Koenig@amd.com>
Sent: Tuesday, March 3, 2020 11:36 PM
To: He, Jacob<mailto:Jacob.He@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

See the SPM buffer address is set using CP commands as well, right? And those execute asynchronously.

When we now synchronously update the SPM VMID we risk that we switch from one process to another while the new process is not ready yet with its setup.

That could have quite a bunch of unforeseen consequences, including accidentally writing SPM data into the new process address space at whatever buffer address was used before.

This is something we at least should try to avoid.

Regards,
Christian.

Am 03.03.20 um 16:28 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]

Thanks!  Could you please take an example of trouble  “This way we avoid a bunch of trouble when one process drops the VMID reservation and another one grabs it.”?

Thanks
Jacob

From: Koenig, Christian<mailto:Christian.Koenig@amd.com>
Sent: Tuesday, March 3, 2020 11:03 PM
To: He, Jacob<mailto:Jacob.He@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

Am 03.03.20 um 15:34 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]

It would be better if we could do that asynchronously with a register
write on the ring.
Sorry, I don’t get your point. Could you please elaborate more?

You pass the ring from amdgpu_vm_flush() to the *_update_spm_vmid() functions.

And then instead of using WREG32() you call amdgpu_ring_emit_wreg() to make the write asynchronously on the ring buffer using a CP command.

This way we avoid a bunch of trouble when one process drops the VMID reservation and another one grabs it.

Regards,
Christian.




Thanks
Jacob

From: Christian König<mailto:ckoenig.leichtzumerken@gmail.com>
Sent: Tuesday, March 3, 2020 10:16 PM
To: He, Jacob<mailto:Jacob.He@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

Am 02.03.20 um 06:35 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He <jacob.he@amd.com><mailto:jacob.he@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..c761d3a0b6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>        struct dma_fence *fence = NULL;
>        bool pasid_mapping_needed = false;
>        unsigned patch_offset = 0;
> +     bool update_spm_vmid_needed = (job->vm && (job->vm->reserved_vmid[vmhub] != NULL));
>        int r;
>
> +     if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> +             adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +

It would be better if we could do that asynchronously with a register
write on the ring.

The alternative is that we block for the VM to be idle in
amdgpu_vm_ioctl() before unreserving the VMID.

In other words lock the reservation object of the root PD and call
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().

Regards,
Christian.

>        if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>                gds_switch_needed = true;
>                vm_flush_needed = true;






[-- Attachment #1.2: Type: text/html, Size: 10388 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] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Christian König]

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

Am 03.03.20 um 17:07 schrieb He, Jacob:
>
> [AMD Official Use Only - Internal Distribution Only]
>
>
> Oh, you are right! If SPM_VMID is updated by other process while the 
> SPM enabled commands is executing, that will cause VM fault.
>
> Is the wait vm idle right before unreserve vmid still necessary if 
> using asynchroneously setting SPM_VMID?
>

No, that are alternative approaches.

Updating the VMID asynchronously sounds a bit cleaner to me, but feel 
free to pick whatever is easier for you to implement.

Regards,
Christian.

> Thanks
>
> Jacob
>
> *From: *Koenig, Christian <mailto:Christian.Koenig@amd.com>
> *Sent: *Tuesday, March 3, 2020 11:36 PM
> *To: *He, Jacob <mailto:Jacob.He@amd.com>; 
> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
> *Subject: *Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid 
> when application reserves the vmid
>
> See the SPM buffer address is set using CP commands as well, right? 
> And those execute asynchronously.
>
> When we now synchronously update the SPM VMID we risk that we switch 
> from one process to another while the new process is not ready yet 
> with its setup.
>
> That could have quite a bunch of unforeseen consequences, including 
> accidentally writing SPM data into the new process address space at 
> whatever buffer address was used before.
>
> This is something we at least should try to avoid.
>
> Regards,
> Christian.
>
> Am 03.03.20 um 16:28 schrieb He, Jacob:
>
>     [AMD Official Use Only - Internal Distribution Only]
>
>     Thanks!  Could you please take an example of trouble  “This way we
>     avoid a bunch of trouble when one process drops the VMID
>     reservation and another one grabs it.”?
>
>     Thanks
>
>     Jacob
>
>     *From: *Koenig, Christian <mailto:Christian.Koenig@amd.com>
>     *Sent: *Tuesday, March 3, 2020 11:03 PM
>     *To: *He, Jacob <mailto:Jacob.He@amd.com>;
>     amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>     *Subject: *Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's
>     vmid when application reserves the vmid
>
>     Am 03.03.20 um 15:34 schrieb He, Jacob:
>
>         [AMD Official Use Only - Internal Distribution Only]
>
>         /It would be better if we could do that asynchronously with a
>         register
>         write on the ring./
>
>         Sorry, I don’t get your point. Could you please elaborate more?
>
>
>     You pass the ring from amdgpu_vm_flush() to the
>     *_update_spm_vmid() functions.
>
>     And then instead of using WREG32() you call
>     amdgpu_ring_emit_wreg() to make the write asynchronously on the
>     ring buffer using a CP command.
>
>     This way we avoid a bunch of trouble when one process drops the
>     VMID reservation and another one grabs it.
>
>     Regards,
>     Christian.
>
>
>
>         Thanks
>
>         Jacob
>
>         *From: *Christian König <mailto:ckoenig.leichtzumerken@gmail.com>
>         *Sent: *Tuesday, March 3, 2020 10:16 PM
>         *To: *He, Jacob <mailto:Jacob.He@amd.com>;
>         amd-gfx@lists.freedesktop.org
>         <mailto:amd-gfx@lists.freedesktop.org>
>         *Subject: *Re: [PATCH] drm/amdgpu: Update SPM_VMID with the
>         job's vmid when application reserves the vmid
>
>         Am 02.03.20 um 06:35 schrieb Jacob He:
>         > SPM access the video memory according to SPM_VMID. It should
>         be updated
>         > with the job's vmid right before the job is scheduled.
>         SPM_VMID is a
>         > global resource
>         >
>         > Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
>         > Signed-off-by: Jacob He <jacob.he@amd.com>
>         <mailto:jacob.he@amd.com>
>         > ---
>         >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>         >   1 file changed, 4 insertions(+)
>         >
>         > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>         b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>         > index c00696f3017e..c761d3a0b6e8 100644
>         > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>         > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>         > @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct
>         amdgpu_ring *ring, struct amdgpu_job *job,
>         >        struct dma_fence *fence = NULL;
>         >        bool pasid_mapping_needed = false;
>         >        unsigned patch_offset = 0;
>         > +     bool update_spm_vmid_needed = (job->vm &&
>         (job->vm->reserved_vmid[vmhub] != NULL));
>         >        int r;
>         >
>         > +     if (update_spm_vmid_needed &&
>         adev->gfx.rlc.funcs->update_spm_vmid)
>         > + adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
>         > +
>
>         It would be better if we could do that asynchronously with a
>         register
>         write on the ring.
>
>         The alternative is that we block for the VM to be idle in
>         amdgpu_vm_ioctl() before unreserving the VMID.
>
>         In other words lock the reservation object of the root PD and
>         call
>         amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().
>
>         Regards,
>         Christian.
>
>         >        if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>         >                gds_switch_needed = true;
>         >                vm_flush_needed = true;
>


[-- Attachment #1.2: Type: text/html, Size: 14375 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] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[He, Jacob]

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

[AMD Official Use Only - Internal Distribution Only]

Thanks million! I’ll change the patch.

-Jacob

________________________________
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Wednesday, March 4, 2020 12:09:36 AM
To: He, Jacob <Jacob.He@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

Am 03.03.20 um 17:07 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]


Oh, you are right! If SPM_VMID is updated by other process while the SPM enabled commands is executing, that will cause VM fault.



Is the wait vm idle right before unreserve vmid still necessary if using asynchroneously setting SPM_VMID?

No, that are alternative approaches.

Updating the VMID asynchronously sounds a bit cleaner to me, but feel free to pick whatever is easier for you to implement.

Regards,
Christian.




Thanks

Jacob



From: Koenig, Christian<mailto:Christian.Koenig@amd.com>
Sent: Tuesday, March 3, 2020 11:36 PM
To: He, Jacob<mailto:Jacob.He@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid



See the SPM buffer address is set using CP commands as well, right? And those execute asynchronously.

When we now synchronously update the SPM VMID we risk that we switch from one process to another while the new process is not ready yet with its setup.

That could have quite a bunch of unforeseen consequences, including accidentally writing SPM data into the new process address space at whatever buffer address was used before.

This is something we at least should try to avoid.

Regards,
Christian.

Am 03.03.20 um 16:28 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]



Thanks!  Could you please take an example of trouble  “This way we avoid a bunch of trouble when one process drops the VMID reservation and another one grabs it.”?



Thanks

Jacob



From: Koenig, Christian<mailto:Christian.Koenig@amd.com>
Sent: Tuesday, March 3, 2020 11:03 PM
To: He, Jacob<mailto:Jacob.He@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid



Am 03.03.20 um 15:34 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]



It would be better if we could do that asynchronously with a register
write on the ring.

Sorry, I don’t get your point. Could you please elaborate more?

You pass the ring from amdgpu_vm_flush() to the *_update_spm_vmid() functions.

And then instead of using WREG32() you call amdgpu_ring_emit_wreg() to make the write asynchronously on the ring buffer using a CP command.

This way we avoid a bunch of trouble when one process drops the VMID reservation and another one grabs it.

Regards,
Christian.






Thanks

Jacob



From: Christian König<mailto:ckoenig.leichtzumerken@gmail.com>
Sent: Tuesday, March 3, 2020 10:16 PM
To: He, Jacob<mailto:Jacob.He@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid



Am 02.03.20 um 06:35 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He <jacob.he@amd.com><mailto:jacob.he@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..c761d3a0b6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>        struct dma_fence *fence = NULL;
>        bool pasid_mapping_needed = false;
>        unsigned patch_offset = 0;
> +     bool update_spm_vmid_needed = (job->vm && (job->vm->reserved_vmid[vmhub] != NULL));
>        int r;
>
> +     if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> +             adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +

It would be better if we could do that asynchronously with a register
write on the ring.

The alternative is that we block for the VM to be idle in
amdgpu_vm_ioctl() before unreserving the VMID.

In other words lock the reservation object of the root PD and call
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().

Regards,
Christian.

>        if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>                gds_switch_needed = true;
>                vm_flush_needed = true;












[-- Attachment #1.2: Type: text/html, Size: 11765 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] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Christian König]

Am 05.03.20 um 13:08 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He <jacob.he@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..73398831196f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>   	struct dma_fence *fence = NULL;
>   	bool pasid_mapping_needed = false;
>   	unsigned patch_offset = 0;
> +	bool update_spm_vmid_needed = (job->vm && (job->vm->reserved_vmid[vmhub] != NULL));
>   	int r;
>   
> +	if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> +		adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +
>   	if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>   		gds_switch_needed = true;
>   		vm_flush_needed = true;
> @@ -3213,6 +3217,7 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   	union drm_amdgpu_vm *args = data;
>   	struct amdgpu_device *adev = dev->dev_private;
>   	struct amdgpu_fpriv *fpriv = filp->driver_priv;
> +	long timeout = msecs_to_jiffies(2000);
>   	int r;
>   
>   	switch (args->in.op) {
> @@ -3224,6 +3229,21 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   			return r;
>   		break;
>   	case AMDGPU_VM_OP_UNRESERVE_VMID:
> +		if (amdgpu_sriov_runtime(adev))
> +			timeout = 8 * timeout;
> +
> +		/* Wait vm idle to make sure the vmid set in SPM_VMID is
> +		 * not referenced anymore.
> +		 */
> +		r = amdgpu_bo_reserve(fpriv->vm.root.base.bo, true);
> +		if (r)
> +			return r;
> +
> +		r = amdgpu_vm_wait_idle(&fpriv->vm, timeout);
> +		if (r < 0)
> +			return r;
> +
> +		amdgpu_bo_unreserve(fpriv->vm.root.base.bo);
>   		amdgpu_vmid_free_reserved(adev, &fpriv->vm, AMDGPU_GFXHUB_0);
>   		break;
>   	default:

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

[PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Jacob He]

SPM access the video memory according to SPM_VMID. It should be updated
with the job's vmid right before the job is scheduled. SPM_VMID is a
global resource

Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
Signed-off-by: Jacob He <jacob.he@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index c00696f3017e..73398831196f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 	struct dma_fence *fence = NULL;
 	bool pasid_mapping_needed = false;
 	unsigned patch_offset = 0;
+	bool update_spm_vmid_needed = (job->vm && (job->vm->reserved_vmid[vmhub] != NULL));
 	int r;
 
+	if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
+		adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
+
 	if (amdgpu_vmid_had_gpu_reset(adev, id)) {
 		gds_switch_needed = true;
 		vm_flush_needed = true;
@@ -3213,6 +3217,7 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 	union drm_amdgpu_vm *args = data;
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_fpriv *fpriv = filp->driver_priv;
+	long timeout = msecs_to_jiffies(2000);
 	int r;
 
 	switch (args->in.op) {
@@ -3224,6 +3229,21 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 			return r;
 		break;
 	case AMDGPU_VM_OP_UNRESERVE_VMID:
+		if (amdgpu_sriov_runtime(adev))
+			timeout = 8 * timeout;
+
+		/* Wait vm idle to make sure the vmid set in SPM_VMID is
+		 * not referenced anymore.
+		 */
+		r = amdgpu_bo_reserve(fpriv->vm.root.base.bo, true);
+		if (r)
+			return r;
+
+		r = amdgpu_vm_wait_idle(&fpriv->vm, timeout);
+		if (r < 0)
+			return r;
+
+		amdgpu_bo_unreserve(fpriv->vm.root.base.bo);
 		amdgpu_vmid_free_reserved(adev, &fpriv->vm, AMDGPU_GFXHUB_0);
 		break;
 	default:
-- 
2.17.1

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

Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Christian König]

Am 04.03.20 um 05:06 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He <jacob.he@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..f08effb033a1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>   	struct dma_fence *fence = NULL;
>   	bool pasid_mapping_needed = false;
>   	unsigned patch_offset = 0;
> +	bool update_spm_vmid_needed = (job->vm && (job->vm->reserved_vmid[vmhub] != NULL));
>   	int r;
>   
> +	if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> +		adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +
>   	if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>   		gds_switch_needed = true;
>   		vm_flush_needed = true;
> @@ -3213,6 +3217,7 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   	union drm_amdgpu_vm *args = data;
>   	struct amdgpu_device *adev = dev->dev_private;
>   	struct amdgpu_fpriv *fpriv = filp->driver_priv;
> +	long timeout = msecs_to_jiffies(2000);
>   	int r;
>   
>   	switch (args->in.op) {
> @@ -3224,6 +3229,15 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   			return r;
>   		break;
>   	case AMDGPU_VM_OP_UNRESERVE_VMID:
> +		if (amdgpu_sriov_runtime(adev))
> +			timeout = 8 * timeout;
> +
> +		/* Wait vm idle to make sure the vmid set in SPM_VMID is
> +		 * not referenced anymore.
> +		 */
> +		r = amdgpu_vm_wait_idle(&fpriv->vm, timeout);
> +		if (r < 0)
> +			return r;

You also need to lock/unlock the whole VM around that. Otherwise 
amdgpu_vm_wait_idle() could crash.

E.g. call amdgpu_bo_reserve() on vm->root.base.bo and after you are done 
amdgpu_bo_unreserver().

Apart from that it looks good to me,
Christian.

>   		amdgpu_vmid_free_reserved(adev, &fpriv->vm, AMDGPU_GFXHUB_0);
>   		break;
>   	default:

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

[PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Jacob He]

SPM access the video memory according to SPM_VMID. It should be updated
with the job's vmid right before the job is scheduled. SPM_VMID is a
global resource

Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
Signed-off-by: Jacob He <jacob.he@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index c00696f3017e..f08effb033a1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 	struct dma_fence *fence = NULL;
 	bool pasid_mapping_needed = false;
 	unsigned patch_offset = 0;
+	bool update_spm_vmid_needed = (job->vm && (job->vm->reserved_vmid[vmhub] != NULL));
 	int r;
 
+	if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
+		adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
+
 	if (amdgpu_vmid_had_gpu_reset(adev, id)) {
 		gds_switch_needed = true;
 		vm_flush_needed = true;
@@ -3213,6 +3217,7 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 	union drm_amdgpu_vm *args = data;
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_fpriv *fpriv = filp->driver_priv;
+	long timeout = msecs_to_jiffies(2000);
 	int r;
 
 	switch (args->in.op) {
@@ -3224,6 +3229,15 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 			return r;
 		break;
 	case AMDGPU_VM_OP_UNRESERVE_VMID:
+		if (amdgpu_sriov_runtime(adev))
+			timeout = 8 * timeout;
+
+		/* Wait vm idle to make sure the vmid set in SPM_VMID is
+		 * not referenced anymore.
+		 */
+		r = amdgpu_vm_wait_idle(&fpriv->vm, timeout);
+		if (r < 0)
+			return r;
 		amdgpu_vmid_free_reserved(adev, &fpriv->vm, AMDGPU_GFXHUB_0);
 		break;
 	default:
-- 
2.17.1

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