Re: 答复: [PATCH 2/7] drm/amdgpu:new field members for SRIOV

[Christian König]

We don't need the validate buffer and even when you need it putting it 
into the VM structure is illegal, because it can be used by more than 
one thread at the same time.

This leaves only the csa_bo_va in the amdgpu_vm_virt structure and 
having a structure with only one field is rather pointless.

Additional to that I really don't see CSA handling as something related 
to virtualization.

Regards,
Christian.

Am 10.01.2017 um 04:05 schrieb Liu, Monk:
>
>
> > +};
> > +
> > +struct amdgpu_vm_virt {
> > +     /* each VM will map on CSA */
> > +     struct ttm_validate_buffer csa_tv;
> > +     struct amdgpu_bo_va *csa_bo_va;
>
> Please put that directly into the amdgpu_vm structure.
>
>
>
> [ML] they are not a tiny used by bare-metal case so I don't think they 
> should be put into common structure at all.
>
> see we have "struct amdgpu_virt" in "struct amdgpu_device", the same 
> style
>
>
> BR Monk
>
> ------------------------------------------------------------------------
> *发件人:* Christian König <deathsimple at vodafone.de>
> *发送时间:* 2017年1月9日 18:57:06
> *收件人:* Liu, Monk; amd-gfx at lists.freedesktop.org
> *主题:* Re: [PATCH 2/7] drm/amdgpu:new field members for SRIOV
> Am 09.01.2017 um 09:02 schrieb Monk Liu:
> > Change-Id: Ife0eff7b13b8b5946f005a39f6ecb8db1cb72c38
> > Signed-off-by: Monk Liu <Monk.Liu at amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h | 19 +++++++++++++++++++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h   |  2 ++
> >   2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> > index 0d821d9..5aa7f0c 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> > @@ -28,9 +28,28 @@
> >   #define AMDGPU_SRIOV_CAPS_ENABLE_IOV   (1 << 1) /* sr-iov is 
> enabled on this GPU */
> >   #define AMDGPU_SRIOV_CAPS_IS_VF        (1 << 2) /* this GPU is a 
> virtual function */
> >   #define AMDGPU_PASSTHROUGH_MODE        (1 << 3) /* thw whole GPU 
> is pass through for VM */
> > +
> > +struct amdgpu_vm;
> >   /* GPU virtualization */
> >   struct amdgpu_virt {
> >        uint32_t caps;
> > +     uint32_t csa_size;
> > +     struct amdgpu_bo *csa_obj;
> > +     uint64_t csa_vmid0_addr;
> > +     uint64_t gds_vmid0_addr;
> > +     int (*allocate_csa)(struct amdgpu_device *adev);
> > +     void (*deallocate_csa)(struct amdgpu_device *adev);
> > +     int (*map_csa)(struct amdgpu_device *adev, struct amdgpu_vm *vm);
> > +     void (*unmap_csa)(struct amdgpu_device *adev, struct amdgpu_vm 
> *vm);
>
> Why callbacks for this?
>
> > +};
> > +
> > +struct amdgpu_vm_virt {
> > +     /* each VM will map on CSA */
> > +     struct ttm_validate_buffer csa_tv;
> > +     struct amdgpu_bo_va *csa_bo_va;
>
> Please put that directly into the amdgpu_vm structure.
>
> > +     /* virtual MC address of CSA & GDS for each VM */
> > +     uint64_t vm_csa_addr;
> > +     uint64_t vm_gds_addr;
>
> That should be constant, shouldn't it?
>
> Regards,
> Christian.
>
> >   };
> >
> >   #define amdgpu_sriov_enabled(adev) \
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > index 42a629b..27cbcbc 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > @@ -29,6 +29,7 @@
> >   #include "gpu_scheduler.h"
> >   #include "amdgpu_sync.h"
> >   #include "amdgpu_ring.h"
> > +#include "amdgpu_virt.h"
> >
> >   struct amdgpu_bo_va;
> >   struct amdgpu_job;
> > @@ -111,6 +112,7 @@ struct amdgpu_vm {
> >
> >        /* client id */
> >        u64                     client_id;
> > +     struct amdgpu_vm_virt   vm_virt;
> >   };
> >
> >   struct amdgpu_vm_id {
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170110/3d07d14a/attachment-0001.html>