Re: why we need to do infinite RLC_SPM register setting during VM flush

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

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

I would also prefer to update the SPM VMID register using PM4 packets 
instead of the current handling.

Regards,
Christian.

Am 20.04.20 um 09:50 schrieb Liu, Monk:
>
> I just try to explain what I want to do here, no real patch formalized 
> yet
>
> _____________________________________
>
> Monk Liu|GPU Virtualization Team |AMD
>
> sig-cloud-gpu
>
> *From:* He, Jacob <Jacob.He@amd.com>
> *Sent:* Monday, April 20, 2020 3:45 PM
> *To:* Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian 
> <Christian.Koenig@amd.com>
> *Cc:* amd-gfx@lists.freedesktop.org
> *Subject:* Re: why we need to do infinite RLC_SPM register setting 
> during VM flush
>
> [AMD Official Use Only - Internal Distribution Only]
>
> Do you miss a file which adds spm_updatedto vm structure?
>
> ------------------------------------------------------------------------
>
> *From:*Liu, Monk <Monk.Liu@amd.com <mailto:Monk.Liu@amd.com>>
> *Sent:* Monday, April 20, 2020 3:32 PM
> *To:* He, Jacob <Jacob.He@amd.com <mailto:Jacob.He@amd.com>>; Koenig, 
> Christian <Christian.Koenig@amd.com <mailto:Christian.Koenig@amd.com>>
> *Cc:* amd-gfx@lists.freedesktop.org 
> <mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org 
> <mailto:amd-gfx@lists.freedesktop.org>>
> *Subject:* why we need to do infinite RLC_SPM register setting during 
> VM flush
>
> Hi Jaco & Christian
>
> As titled , check below patch:
>
> commit 10790a09ea584cc832353a5c2a481012e5e31a13
>
> Author: Jacob He <jacob.he@amd.com <mailto:jacob.he@amd.com>>
>
> Date:   Fri Feb 28 20:24:41 2020 +0800
>
>     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 <mailto:jacob.he@amd.com>>
>
>     Reviewed-by: Christian König <christian.koenig@amd.com 
> <mailto:christian.koenig@amd.com>>
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>
> index 6e6fc8c..ba2236a 100644
>
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>
> @@ -1056,8 +1056,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;
>
> this update_spm_vmid() looks an completely overkill to me, we only 
> need to do it once for its VM …
>
> in SRIOV the register reading/writing for update_spm_vmid() is now 
> carried by KIQ thus there is too much burden on KIQ for such 
> unnecessary jobs …
>
> I want to change it to only do it once per VM, like:
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>
> index 6e6fc8c..ba2236a 100644
>
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>
> @@ -1056,8 +1056,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 &&  !vm->spm_updated) {
>
> + adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
>
> +               vm->spm_updated = true;
>
> +       }
>
>         if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>
>                 gds_switch_needed = true;
>
>                 vm_flush_needed = true;
>
> what do you think ?
>
> P.S.: the best way is to let GFX ring itself to do the 
> update_spm_vmid() instead of let CPU doing it, e.g.: we put more PM4 
> command in VM-FLUSH packets ….
>
> But I prefer the simple way first like I demonstrated above
>
> _____________________________________
>
> Monk Liu|GPU Virtualization Team |AMD
>
> sig-cloud-gpu
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2.1: Type: text/html, Size: 15742 bytes --]

[-- Attachment #1.2.2: image001.png --]
[-- Type: image/png, Size: 12243 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