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

[Christian König]

> NAH, for AI/VI the gds_addr is the bottom half of CSA area (8kb), but 
> we don't know for later chips ...
>
In this case we shouldn't store the offset anywhere, but rather just 
calculate it in the chip specific code from the CSA address when it is 
needed.

> I prefer keep a member field in the struct for flexibility ...
>
We can still change it if we find that we need more flexibility here. 
Usually we try to avoid flexibility for the cost of complexity if we 
don't have a good reason for it.

It just complicates the code and over the long term creates maintenance 
problems because you end of with a lot of only halve way implemented 
features.

Regards,
Christian.

Am 10.01.2017 um 10:57 schrieb Liu, Monk:
>
> NAH, for AI/VI the gds_addr is the bottom half of CSA area (8kb), but 
> we don't know for later chips ...
>
> I prefer keep a member field in the struct for flexibility ...
>
>
> regards with put those member into amdgpu_vm structure, if you really 
> prefer that way  I'm fine by moving them, not too important anyway.
>
>
> BR Monk
>
>
> ------------------------------------------------------------------------
> *发件人:* amd-gfx <amd-gfx-bounces at lists.freedesktop.org> 代表 Christian 
> König <deathsimple at vodafone.de>
> *发送时间:* 2017年1月10日 17:51:46
> *收件人:* Liu, Monk; amd-gfx at lists.freedesktop.org
> *主题:* Re: 答复: [PATCH 2/7] drm/amdgpu:new field members for SRIOV
> Am 10.01.2017 um 04:03 schrieb Liu, Monk:
>>
>> > +/* 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?
>>
>>
>>
>> [ML] do you mean "const uint64_t vm_csa_addr" ? of cause not if 
>> that's your point ... compiling error on that
>>
>> and strictly speaking that's not constant, the address is calculated ...
>>
>
> No that wasn't what I meant. The address is constant for all VM's in 
> the system, isn't it?
>
> Just add a define similar to AMDGPU_VA_RESERVED_SIZE and stop 
> calculating it all together (BTW: AMDGPU_VA_RESERVED_SIZE should 
> probably be moved to amdgpu_vm.h).
>
> Regards,
> Christian.
>
>> ------------------------------------------------------------------------
>> *发件人:* 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 {
>>
>>
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>

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