[PATCH 0/7] static CSA for SRIOV serials

[PATCH 0/7] static CSA for SRIOV serials

[Monk Liu]

per device CSA is needed by world switch preemption for SRIOV virtualization
we create CSA per device, and map it on each VM in vm_init, and update CSA BO
bo_va in each submission.

*** BLURB HERE ***

Monk Liu (7):
  drm/amdgpu:add new file for SRIOV
  drm/amdgpu:new field members for SRIOV
  drm/amdgpu:impl static CSA alloc/dealloc
  drm/amdgpu:impl static CSA map/unmap routines
  drm/amdgpu:set static csa function
  drm/amdgpu:alloc/dealloc csa accordingly
  drm/amdgpu:map/unmap static csa accordingly

 drivers/gpu/drm/amd/amdgpu/Makefile        |   2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     |  14 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |   7 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c   | 142 +++++++++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h   |  21 +++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     |  11 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |   2 +
 drivers/gpu/drm/amd/amdgpu/vi.c            |   3 +
 8 files changed, 201 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c

-- 
2.7.4

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

[PATCH 1/7] drm/amdgpu:add new file for SRIOV

[Monk Liu]

Change-Id: I7e21c52e5b0c7ef39a706a3191293f3d7b5a7e08
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/Makefile      |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
index 4185b03..0b8e470 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -30,7 +30,7 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \
 	atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
 	amdgpu_prime.o amdgpu_vm.o amdgpu_ib.o amdgpu_pll.o \
 	amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
-	amdgpu_gtt_mgr.o amdgpu_vram_mgr.o
+	amdgpu_gtt_mgr.o amdgpu_vram_mgr.o amdgpu_virt.o
 
 # add asic specific block
 amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o kv_smc.o kv_dpm.o \
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
new file mode 100644
index 0000000..0bfec66
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -0,0 +1,23 @@
+/*
+ * Copyright 2016 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+#include "amdgpu.h"
\ No newline at end of file
-- 
2.7.4

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

Re: [PATCH 1/7] drm/amdgpu:add new file for SRIOV

[Christian König]

Am 09.01.2017 um 09:02 schrieb Monk Liu:
> Change-Id: I7e21c52e5b0c7ef39a706a3191293f3d7b5a7e08
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>

Just adding the file without any functionality doesn't make much sense, 
please squash into the other commits.

Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/Makefile      |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 23 +++++++++++++++++++++++
>   2 files changed, 24 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 4185b03..0b8e470 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -30,7 +30,7 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \
>   	atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
>   	amdgpu_prime.o amdgpu_vm.o amdgpu_ib.o amdgpu_pll.o \
>   	amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
> -	amdgpu_gtt_mgr.o amdgpu_vram_mgr.o
> +	amdgpu_gtt_mgr.o amdgpu_vram_mgr.o amdgpu_virt.o
>   
>   # add asic specific block
>   amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o kv_smc.o kv_dpm.o \
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> new file mode 100644
> index 0000000..0bfec66
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -0,0 +1,23 @@
> +/*
> + * Copyright 2016 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +#include "amdgpu.h"
> \ No newline at end of file


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

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

[Monk Liu]

Change-Id: Ife0eff7b13b8b5946f005a39f6ecb8db1cb72c38
Signed-off-by: Monk Liu <Monk.Liu@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);
+};
+
+struct amdgpu_vm_virt {
+	/* each VM will map on CSA */
+	struct ttm_validate_buffer csa_tv;
+	struct amdgpu_bo_va *csa_bo_va;
+	/* virtual MC address of CSA & GDS for each VM */
+	uint64_t vm_csa_addr;
+	uint64_t vm_gds_addr;
 };
 
 #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 {
-- 
2.7.4

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

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

[Christian König]

Am 09.01.2017 um 09:02 schrieb Monk Liu:
> Change-Id: Ife0eff7b13b8b5946f005a39f6ecb8db1cb72c38
> Signed-off-by: Monk Liu <Monk.Liu@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Liu, Monk]

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

> +     /* 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 ...

________________________________
发件人: Christian König <deathsimple@vodafone.de>
发送时间: 2017年1月9日 18:57:06
收件人: Liu, Monk; amd-gfx@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@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 {



[-- Attachment #1.2: Type: text/html, Size: 5819 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 2/7] drm/amdgpu:new field members for SRIOV

[Christian König]

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

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-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
> *发送时间:* 2017年1月9日 18:57:06
> *收件人:* Liu, Monk; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.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-5C7GfCeVMHo@public.gmane.org>
> > ---
> >   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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx



[-- Attachment #1.2: Type: text/html, Size: 8245 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 2/7] drm/amdgpu:new field members for SRIOV

[Liu, Monk]

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

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@lists.freedesktop.org> 代表 Christian König <deathsimple@vodafone.de>
发送时间: 2017年1月10日 17:51:46
收件人: Liu, Monk; amd-gfx@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@vodafone.de><mailto:deathsimple@vodafone.de>
发送时间: 2017年1月9日 18:57:06
收件人: Liu, Monk; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@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@amd.com><mailto:Monk.Liu@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@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx



[-- Attachment #1.2: Type: text/html, Size: 8571 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 2/7] drm/amdgpu:new field members for SRIOV

[Christian König]

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

> 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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> 代表 Christian 
> König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
> *发送时间:* 2017年1月10日 17:51:46
> *收件人:* Liu, Monk; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.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-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
>> *发送时间:* 2017年1月9日 18:57:06
>> *收件人:* Liu, Monk; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.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-5C7GfCeVMHo@public.gmane.org>
>> > ---
>> >   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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>


[-- Attachment #1.2: Type: text/html, Size: 12280 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 2/7] drm/amdgpu:new field members for SRIOV

[Liu, Monk]

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


> +};
> +
> +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@vodafone.de>
发送时间: 2017年1月9日 18:57:06
收件人: Liu, Monk; amd-gfx@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@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 {



[-- Attachment #1.2: Type: text/html, Size: 5960 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 2/7] drm/amdgpu:new field members for SRIOV

[Christian König]

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

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-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
> *发送时间:* 2017年1月9日 18:57:06
> *收件人:* Liu, Monk; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.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-5C7GfCeVMHo@public.gmane.org>
> > ---
> >   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 {
>
>


[-- Attachment #1.2: Type: text/html, Size: 7669 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 3/7] drm/amdgpu:impl static CSA alloc/dealloc

[Monk Liu]

for SRIOV usage, CSA is only used per device and each
VM will map on it.

Change-Id: I0736b6e51329d643343e26968a47a81c3a27035f
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index 0bfec66..a6bce36 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -20,4 +20,25 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  *
  */
-#include "amdgpu.h"
\ No newline at end of file
+#include "amdgpu.h"
+static int amdgpu_allocate_static_csa(struct amdgpu_device *adev)
+{
+	int r;
+	void *ptr;
+
+	r = amdgpu_bo_create_kernel(adev, adev->virt.csa_size, PAGE_SIZE,
+				AMDGPU_GEM_DOMAIN_VRAM, &adev->virt.csa_obj,
+				&adev->virt.csa_vmid0_addr, &ptr);
+	if (r)
+		return r;
+
+	/* gds part take the bottom half buffer */
+	adev->virt.gds_vmid0_addr = adev->virt.csa_vmid0_addr + PAGE_SIZE;
+	memset(ptr, 0, adev->virt.csa_size);
+	return 0;
+}
+
+static void amdgpu_deallocate_static_csa(struct amdgpu_device *adev)
+{
+	amdgpu_bo_free_kernel(&adev->virt.csa_obj, &adev->virt.csa_vmid0_addr, NULL);
+}
-- 
2.7.4

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

Re: [PATCH 3/7] drm/amdgpu:impl static CSA alloc/dealloc

[Christian König]

Am 09.01.2017 um 09:02 schrieb Monk Liu:
> for SRIOV usage, CSA is only used per device and each
> VM will map on it.
>
> Change-Id: I0736b6e51329d643343e26968a47a81c3a27035f
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index 0bfec66..a6bce36 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -20,4 +20,25 @@
>    * OTHER DEALINGS IN THE SOFTWARE.
>    *
>    */
> -#include "amdgpu.h"
> \ No newline at end of file
> +#include "amdgpu.h"

Please add at least a newline between the includes and the start of the 
code.

> +static int amdgpu_allocate_static_csa(struct amdgpu_device *adev)
> +{
> +	int r;
> +	void *ptr;
> +
> +	r = amdgpu_bo_create_kernel(adev, adev->virt.csa_size, PAGE_SIZE,
> +				AMDGPU_GEM_DOMAIN_VRAM, &adev->virt.csa_obj,
> +				&adev->virt.csa_vmid0_addr, &ptr);
> +	if (r)
> +		return r;
> +
> +	/* gds part take the bottom half buffer */
> +	adev->virt.gds_vmid0_addr = adev->virt.csa_vmid0_addr + PAGE_SIZE;
> +	memset(ptr, 0, adev->virt.csa_size);
> +	return 0;
> +}
> +
> +static void amdgpu_deallocate_static_csa(struct amdgpu_device *adev)
> +{
> +	amdgpu_bo_free_kernel(&adev->virt.csa_obj, &adev->virt.csa_vmid0_addr, NULL);
> +}

Just static function which aren't called will raise compiler warning, 
please squash that into the functionality where it is used.

Christian.

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

[PATCH 4/7] drm/amdgpu:impl static CSA map/unmap routines

[Monk Liu]

Change-Id: I76ddeb3212c96d87a2d15a608ae8c0771e2d94ed
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 90 ++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index a6bce36..5b15483 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -42,3 +42,93 @@ static void amdgpu_deallocate_static_csa(struct amdgpu_device *adev)
 {
 	amdgpu_bo_free_kernel(&adev->virt.csa_obj, &adev->virt.csa_vmid0_addr, NULL);
 }
+
+/*
+ * amdgpu_map_static_csa should be called during amdgpu_vm_init
+ * it maps virtual address "AMDGPU_VA_RESERVED_SIZE - adev->virt.csa_size"
+ * to this VM, and each command submission of GFX should use this virtual
+ * address within META_DATA init package to support SRIOV gfx preemption.
+ */
+
+static int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm)
+{
+	int r;
+	uint64_t vaddr;
+	struct amdgpu_bo_va *bo_va;
+	struct ww_acquire_ctx ticket;
+	struct list_head list, duplicates;
+	struct amdgpu_bo_list_entry pd;
+
+	INIT_LIST_HEAD(&list);
+	INIT_LIST_HEAD(&duplicates);
+	INIT_LIST_HEAD(&vm->vm_virt.csa_tv.head);
+	vm->vm_virt.csa_tv.bo = &adev->virt.csa_obj->tbo;
+	vm->vm_virt.csa_tv.shared = true;
+
+	list_add(&vm->vm_virt.csa_tv.head, &list);
+	amdgpu_vm_get_pd_bo(vm, &list, &pd);
+
+	vaddr = AMDGPU_VA_RESERVED_SIZE - adev->virt.csa_size;
+	r = ttm_eu_reserve_buffers(&ticket, &list, true, &duplicates);
+	if (r) {
+		DRM_ERROR("failed to reserve CSA,PD BOs: err=%d\n", r);
+		return r;
+	}
+
+	bo_va = amdgpu_vm_bo_add(adev, vm, adev->virt.csa_obj);
+	if (!bo_va) {
+		ttm_eu_backoff_reservation(&ticket, &list);
+		DRM_ERROR("failed to create bo_va for static CSA\n");
+		return -ENOMEM;
+	}
+
+	r = amdgpu_vm_bo_map(adev, bo_va, vaddr, 0, adev->virt.csa_size,
+						AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
+						AMDGPU_PTE_EXECUTABLE);
+
+	if (r) {
+		DRM_ERROR("failed to do bo_map on static CSA, err=%d\n", r);
+		amdgpu_vm_bo_rmv(adev, bo_va);
+		ttm_eu_backoff_reservation(&ticket, &list);
+		kfree(bo_va);
+		return r;
+	}
+
+	ttm_eu_backoff_reservation(&ticket, &list);
+
+	vm->vm_virt.csa_bo_va = bo_va;
+	vm->vm_virt.vm_csa_addr = vaddr;
+	vm->vm_virt.vm_gds_addr = vaddr + PAGE_SIZE;
+	return 0;
+}
+
+static void amdgpu_unmap_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm)
+{
+	int r;
+	struct ww_acquire_ctx ticket;
+	struct list_head list, duplicate;
+	struct amdgpu_bo_list_entry pd;
+
+	if (vm->vm_virt.csa_bo_va) {
+		INIT_LIST_HEAD(&list);
+		INIT_LIST_HEAD(&duplicate);
+		list_add(&vm->vm_virt.csa_tv.head, &list);
+		amdgpu_vm_get_pd_bo(vm, &list, &pd);
+
+		r = ttm_eu_reserve_buffers(&ticket, &list, true, &duplicate);
+		if (r) {
+			DRM_ERROR("failed to reserve static CSA buffer: err=%d\n", r);
+			return;
+		}
+
+		amdgpu_vm_bo_rmv(adev, vm->vm_virt.csa_bo_va);
+		/* maybe no need to do real clearing cuz this vm will die soon */
+		r = amdgpu_vm_clear_freed(adev, vm);
+		if (r)
+			DRM_ERROR("failed to clear static CSA bo: err=%d", r);
+
+		ttm_eu_backoff_reservation(&ticket, &list);
+		vm->vm_virt.csa_bo_va = NULL;
+		vm->vm_virt.vm_csa_addr = vm->vm_virt.vm_gds_addr = 0;
+	}
+}
-- 
2.7.4

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

Re: [PATCH 4/7] drm/amdgpu:impl static CSA map/unmap routines

[Christian König]

Am 09.01.2017 um 09:02 schrieb Monk Liu:
> Change-Id: I76ddeb3212c96d87a2d15a608ae8c0771e2d94ed
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>

As discussed before that approach won't work.

Just add the call I suggest to amdgpu_bo_vm_update_pte() in amdgpu_cs.c.

Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 90 ++++++++++++++++++++++++++++++++
>   1 file changed, 90 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index a6bce36..5b15483 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -42,3 +42,93 @@ static void amdgpu_deallocate_static_csa(struct amdgpu_device *adev)
>   {
>   	amdgpu_bo_free_kernel(&adev->virt.csa_obj, &adev->virt.csa_vmid0_addr, NULL);
>   }
> +
> +/*
> + * amdgpu_map_static_csa should be called during amdgpu_vm_init
> + * it maps virtual address "AMDGPU_VA_RESERVED_SIZE - adev->virt.csa_size"
> + * to this VM, and each command submission of GFX should use this virtual
> + * address within META_DATA init package to support SRIOV gfx preemption.
> + */
> +
> +static int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> +{
> +	int r;
> +	uint64_t vaddr;
> +	struct amdgpu_bo_va *bo_va;
> +	struct ww_acquire_ctx ticket;
> +	struct list_head list, duplicates;
> +	struct amdgpu_bo_list_entry pd;
> +
> +	INIT_LIST_HEAD(&list);
> +	INIT_LIST_HEAD(&duplicates);
> +	INIT_LIST_HEAD(&vm->vm_virt.csa_tv.head);
> +	vm->vm_virt.csa_tv.bo = &adev->virt.csa_obj->tbo;
> +	vm->vm_virt.csa_tv.shared = true;
> +
> +	list_add(&vm->vm_virt.csa_tv.head, &list);
> +	amdgpu_vm_get_pd_bo(vm, &list, &pd);
> +
> +	vaddr = AMDGPU_VA_RESERVED_SIZE - adev->virt.csa_size;
> +	r = ttm_eu_reserve_buffers(&ticket, &list, true, &duplicates);
> +	if (r) {
> +		DRM_ERROR("failed to reserve CSA,PD BOs: err=%d\n", r);
> +		return r;
> +	}
> +
> +	bo_va = amdgpu_vm_bo_add(adev, vm, adev->virt.csa_obj);
> +	if (!bo_va) {
> +		ttm_eu_backoff_reservation(&ticket, &list);
> +		DRM_ERROR("failed to create bo_va for static CSA\n");
> +		return -ENOMEM;
> +	}
> +
> +	r = amdgpu_vm_bo_map(adev, bo_va, vaddr, 0, adev->virt.csa_size,
> +						AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
> +						AMDGPU_PTE_EXECUTABLE);
> +
> +	if (r) {
> +		DRM_ERROR("failed to do bo_map on static CSA, err=%d\n", r);
> +		amdgpu_vm_bo_rmv(adev, bo_va);
> +		ttm_eu_backoff_reservation(&ticket, &list);
> +		kfree(bo_va);
> +		return r;
> +	}
> +
> +	ttm_eu_backoff_reservation(&ticket, &list);
> +
> +	vm->vm_virt.csa_bo_va = bo_va;
> +	vm->vm_virt.vm_csa_addr = vaddr;
> +	vm->vm_virt.vm_gds_addr = vaddr + PAGE_SIZE;
> +	return 0;
> +}
> +
> +static void amdgpu_unmap_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> +{
> +	int r;
> +	struct ww_acquire_ctx ticket;
> +	struct list_head list, duplicate;
> +	struct amdgpu_bo_list_entry pd;
> +
> +	if (vm->vm_virt.csa_bo_va) {
> +		INIT_LIST_HEAD(&list);
> +		INIT_LIST_HEAD(&duplicate);
> +		list_add(&vm->vm_virt.csa_tv.head, &list);
> +		amdgpu_vm_get_pd_bo(vm, &list, &pd);
> +
> +		r = ttm_eu_reserve_buffers(&ticket, &list, true, &duplicate);
> +		if (r) {
> +			DRM_ERROR("failed to reserve static CSA buffer: err=%d\n", r);
> +			return;
> +		}
> +
> +		amdgpu_vm_bo_rmv(adev, vm->vm_virt.csa_bo_va);
> +		/* maybe no need to do real clearing cuz this vm will die soon */
> +		r = amdgpu_vm_clear_freed(adev, vm);
> +		if (r)
> +			DRM_ERROR("failed to clear static CSA bo: err=%d", r);
> +
> +		ttm_eu_backoff_reservation(&ticket, &list);
> +		vm->vm_virt.csa_bo_va = NULL;
> +		vm->vm_virt.vm_csa_addr = vm->vm_virt.vm_gds_addr = 0;
> +	}
> +}


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

[PATCH 5/7] drm/amdgpu:set static csa function

[Monk Liu]

Change-Id: Ib59c9e7f37a10ef1d6335d400ab41fe3cc9e2fb6
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 8 ++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h | 2 ++
 drivers/gpu/drm/amd/amdgpu/vi.c          | 3 +++
 3 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index 5b15483..8f9dff5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -132,3 +132,11 @@ static void amdgpu_unmap_static_csa(struct amdgpu_device *adev, struct amdgpu_vm
 		vm->vm_virt.vm_csa_addr = vm->vm_virt.vm_gds_addr = 0;
 	}
 }
+
+void amdgpu_virt_set_csa_funcs(struct amdgpu_device *adev) {
+	adev->virt.allocate_csa = amdgpu_allocate_static_csa;
+	adev->virt.deallocate_csa = amdgpu_deallocate_static_csa;
+	adev->virt.map_csa = amdgpu_map_static_csa;
+	adev->virt.unmap_csa = amdgpu_unmap_static_csa;
+	adev->virt.csa_size = 8 * 1024; /* two page now for VI and AI */
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
index 5aa7f0c..f036d3a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
@@ -73,4 +73,6 @@ static inline bool is_virtual_machine(void)
 #endif
 }
 
+void amdgpu_virt_set_csa_funcs(struct amdgpu_device *adev);
+
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
index 7350a8f..42e96cb 100644
--- a/drivers/gpu/drm/amd/amdgpu/vi.c
+++ b/drivers/gpu/drm/amd/amdgpu/vi.c
@@ -455,6 +455,9 @@ static void vi_detect_hw_virtualization(struct amdgpu_device *adev)
 		if (is_virtual_machine()) /* passthrough mode exclus sr-iov mode */
 			adev->virt.caps |= AMDGPU_PASSTHROUGH_MODE;
 	}
+
+	if (amdgpu_sriov_vf(adev))
+		amdgpu_virt_set_csa_funcs(adev);
 }
 
 static const struct amdgpu_allowed_register_entry tonga_allowed_read_registers[] = {
-- 
2.7.4

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

Re: [PATCH 5/7] drm/amdgpu:set static csa function

[Christian König]

Am 09.01.2017 um 09:03 schrieb Monk Liu:
> Change-Id: Ib59c9e7f37a10ef1d6335d400ab41fe3cc9e2fb6
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 8 ++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h | 2 ++
>   drivers/gpu/drm/amd/amdgpu/vi.c          | 3 +++
>   3 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index 5b15483..8f9dff5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -132,3 +132,11 @@ static void amdgpu_unmap_static_csa(struct amdgpu_device *adev, struct amdgpu_vm
>   		vm->vm_virt.vm_csa_addr = vm->vm_virt.vm_gds_addr = 0;
>   	}
>   }
> +
> +void amdgpu_virt_set_csa_funcs(struct amdgpu_device *adev) {
> +	adev->virt.allocate_csa = amdgpu_allocate_static_csa;
> +	adev->virt.deallocate_csa = amdgpu_deallocate_static_csa;
> +	adev->virt.map_csa = amdgpu_map_static_csa;
> +	adev->virt.unmap_csa = amdgpu_unmap_static_csa;
> +	adev->virt.csa_size = 8 * 1024; /* two page now for VI and AI */
> +}

You don't abstract anything by using indirect calls, so please avoid 
them and call the appropriate functions directly.

Christian.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> index 5aa7f0c..f036d3a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> @@ -73,4 +73,6 @@ static inline bool is_virtual_machine(void)
>   #endif
>   }
>   
> +void amdgpu_virt_set_csa_funcs(struct amdgpu_device *adev);
> +
>   #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
> index 7350a8f..42e96cb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> @@ -455,6 +455,9 @@ static void vi_detect_hw_virtualization(struct amdgpu_device *adev)
>   		if (is_virtual_machine()) /* passthrough mode exclus sr-iov mode */
>   			adev->virt.caps |= AMDGPU_PASSTHROUGH_MODE;
>   	}
> +
> +	if (amdgpu_sriov_vf(adev))
> +		amdgpu_virt_set_csa_funcs(adev);
>   }
>   
>   static const struct amdgpu_allowed_register_entry tonga_allowed_read_registers[] = {


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

[PATCH 6/7] drm/amdgpu:alloc/dealloc csa accordingly

[Monk Liu]

Change-Id: I110af93c4f17ab18d1be199c6ebe9ee965483a66
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index f82919d..5253aa2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1384,6 +1384,10 @@ static int amdgpu_init(struct amdgpu_device *adev)
 				return r;
 			}
 			adev->ip_blocks[i].status.hw = true;
+
+			/* right after GMC hw init, we create CSA */
+			if (amdgpu_sriov_vf(adev))
+				adev->virt.allocate_csa(adev);
 		}
 	}
 
@@ -1517,6 +1521,9 @@ static int amdgpu_fini(struct amdgpu_device *adev)
 		adev->ip_blocks[i].status.late_initialized = false;
 	}
 
+	if (amdgpu_sriov_vf(adev))
+		adev->virt.deallocate_csa(adev);
+
 	return 0;
 }
 
-- 
2.7.4

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

Re: [PATCH 6/7] drm/amdgpu:alloc/dealloc csa accordingly

[Nils Wallménius]

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

Hi Monk, a comment below.


Den 9 jan. 2017 9:07 fm skrev "Monk Liu" <Monk.Liu-5C7GfCeVMHo@public.gmane.org>:

Change-Id: I110af93c4f17ab18d1be199c6ebe9ee965483a66
Signed-off-by: Monk Liu <Monk.Liu-5C7GfCeVMHo@public.gmane.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index f82919d..5253aa2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1384,6 +1384,10 @@ static int amdgpu_init(struct amdgpu_device *adev)
                                return r;
                        }
                        adev->ip_blocks[i].status.hw = true;
+
+                       /* right after GMC hw init, we create CSA */
+                       if (amdgpu_sriov_vf(adev))
+                               adev->virt.allocate_csa(adev);


The amdgpu_allocate_static_csa function returns a value, presumably an
error on failure. Shouldn't the return value be checked here and errors
handled?

Best regards,
Nils

                }
        }

@@ -1517,6 +1521,9 @@ static int amdgpu_fini(struct amdgpu_device *adev)
                adev->ip_blocks[i].status.late_initialized = false;
        }

+       if (amdgpu_sriov_vf(adev))
+               adev->virt.deallocate_csa(adev);
+
        return 0;
 }

--
2.7.4

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

[-- Attachment #1.2: Type: text/html, Size: 3226 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 6/7] drm/amdgpu:alloc/dealloc csa accordingly

[Liu, Monk]

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

Yeah, make sense

 thanks


BR Monk

________________________________
发件人: Nils Wallménius <nils.wallmenius@gmail.com>
发送时间: 2017年1月10日 15:56:36
收件人: Liu, Monk
抄送: amd-gfx@lists.freedesktop.org
主题: Re: [PATCH 6/7] drm/amdgpu:alloc/dealloc csa accordingly

Hi Monk, a comment below.


Den 9 jan. 2017 9:07 fm skrev "Monk Liu" <Monk.Liu@amd.com<mailto:Monk.Liu@amd.com>>:
Change-Id: I110af93c4f17ab18d1be199c6ebe9ee965483a66
Signed-off-by: Monk Liu <Monk.Liu@amd.com<mailto:Monk.Liu@amd.com>>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index f82919d..5253aa2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1384,6 +1384,10 @@ static int amdgpu_init(struct amdgpu_device *adev)
                                return r;
                        }
                        adev->ip_blocks[i].status.hw = true;
+
+                       /* right after GMC hw init, we create CSA */
+                       if (amdgpu_sriov_vf(adev))
+                               adev->virt.allocate_csa(adev);

The amdgpu_allocate_static_csa function returns a value, presumably an error on failure. Shouldn't the return value be checked here and errors handled?

Best regards,
Nils

                }
        }

@@ -1517,6 +1521,9 @@ static int amdgpu_fini(struct amdgpu_device *adev)
                adev->ip_blocks[i].status.late_initialized = false;
        }

+       if (amdgpu_sriov_vf(adev))
+               adev->virt.deallocate_csa(adev);
+
        return 0;
 }

--
2.7.4

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


[-- Attachment #1.2: Type: text/html, Size: 4812 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 7/7] drm/amdgpu:map/unmap static csa accordingly

[Monk Liu]

and update CSA bo_va in each submit

Change-Id: I5ed73e1b7f89743d90298bc814a42a91e166be3b
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 14 ++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 11 +++++++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 6159afc..083ab73 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -771,6 +771,20 @@ static int amdgpu_bo_vm_update_pte(struct amdgpu_cs_parser *p,
 	if (r)
 		return r;
 
+	if (amdgpu_sriov_vf(adev)) {
+		struct fence *f;
+		bo_va = vm->vm_virt.csa_bo_va;
+		BUG_ON(!bo_va);
+		r = amdgpu_vm_bo_update(adev, bo_va, false);
+		if (r)
+			return r;
+
+		f = bo_va->last_pt_update;
+		r = amdgpu_sync_fence(adev, &p->job->sync, f);
+		if (r)
+			return r;
+	}
+
 	if (p->bo_list) {
 		for (i = 0; i < p->bo_list->num_entries; i++) {
 			struct fence *f;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index d05546e..b9cd686 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1576,6 +1576,14 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	vm->last_eviction_counter = atomic64_read(&adev->num_evictions);
 	amdgpu_bo_unreserve(vm->page_directory);
 
+	if (amdgpu_sriov_vf(adev)) {
+		BUG_ON(!adev->virt.map_csa);
+		BUG_ON(!adev->virt.unmap_csa);
+		r = adev->virt.map_csa(adev, vm);
+		if (r)
+			goto error_free_page_directory;
+	}
+
 	return 0;
 
 error_free_page_directory:
@@ -1606,6 +1614,9 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	struct amdgpu_bo_va_mapping *mapping, *tmp;
 	int i;
 
+	if (amdgpu_sriov_vf(adev))
+		adev->virt.unmap_csa(adev, vm);
+
 	amd_sched_entity_fini(vm->entity.sched, &vm->entity);
 
 	if (!RB_EMPTY_ROOT(&vm->va)) {
-- 
2.7.4

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

Re: [PATCH 7/7] drm/amdgpu:map/unmap static csa accordingly

[Christian König]

Am 09.01.2017 um 09:03 schrieb Monk Liu:
> and update CSA bo_va in each submit
>
> Change-Id: I5ed73e1b7f89743d90298bc814a42a91e166be3b
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 14 ++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 11 +++++++++++
>   2 files changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 6159afc..083ab73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -771,6 +771,20 @@ static int amdgpu_bo_vm_update_pte(struct amdgpu_cs_parser *p,
>   	if (r)
>   		return r;
>   
> +	if (amdgpu_sriov_vf(adev)) {
> +		struct fence *f;

A new line is needed between deceleration and code.

> +		bo_va = vm->vm_virt.csa_bo_va;
> +		BUG_ON(!bo_va);
> +		r = amdgpu_vm_bo_update(adev, bo_va, false);
> +		if (r)
> +			return r;
> +
> +		f = bo_va->last_pt_update;
> +		r = amdgpu_sync_fence(adev, &p->job->sync, f);
> +		if (r)
> +			return r;
> +	}
> +
>   	if (p->bo_list) {
>   		for (i = 0; i < p->bo_list->num_entries; i++) {
>   			struct fence *f;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index d05546e..b9cd686 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1576,6 +1576,14 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   	vm->last_eviction_counter = atomic64_read(&adev->num_evictions);
>   	amdgpu_bo_unreserve(vm->page_directory);
>   
> +	if (amdgpu_sriov_vf(adev)) {
> +		BUG_ON(!adev->virt.map_csa);
> +		BUG_ON(!adev->virt.unmap_csa);
> +		r = adev->virt.map_csa(adev, vm);
> +		if (r)
> +			goto error_free_page_directory;
> +	}
> +

Just completely drop that. Updating the VM on the first command 
submission should be sufficient.

Christian.

>   	return 0;
>   
>   error_free_page_directory:
> @@ -1606,6 +1614,9 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   	struct amdgpu_bo_va_mapping *mapping, *tmp;
>   	int i;
>   
> +	if (amdgpu_sriov_vf(adev))
> +		adev->virt.unmap_csa(adev, vm);
> +
>   	amd_sched_entity_fini(vm->entity.sched, &vm->entity);
>   
>   	if (!RB_EMPTY_ROOT(&vm->va)) {


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

答复: [PATCH 7/7] drm/amdgpu:map/unmap static csa accordingly

[Liu, Monk]

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

why you want to drop that "adev->virt.map_csa()" calling ?  without that calling you don't have CSA's initialized bo_va and mappings as well, and without bo_va and mappings how you can make "amdgpu_vm_bo_update()" invoke work ?


Monk


________________________________
发件人: Christian König <deathsimple@vodafone.de>
发送时间: 2017年1月9日 19:04:44
收件人: Liu, Monk; amd-gfx@lists.freedesktop.org
主题: Re: [PATCH 7/7] drm/amdgpu:map/unmap static csa accordingly

Am 09.01.2017 um 09:03 schrieb Monk Liu:
> and update CSA bo_va in each submit
>
> Change-Id: I5ed73e1b7f89743d90298bc814a42a91e166be3b
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 14 ++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 11 +++++++++++
>   2 files changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 6159afc..083ab73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -771,6 +771,20 @@ static int amdgpu_bo_vm_update_pte(struct amdgpu_cs_parser *p,
>        if (r)
>                return r;
>
> +     if (amdgpu_sriov_vf(adev)) {
> +             struct fence *f;

A new line is needed between deceleration and code.

> +             bo_va = vm->vm_virt.csa_bo_va;
> +             BUG_ON(!bo_va);
> +             r = amdgpu_vm_bo_update(adev, bo_va, false);
> +             if (r)
> +                     return r;
> +
> +             f = bo_va->last_pt_update;
> +             r = amdgpu_sync_fence(adev, &p->job->sync, f);
> +             if (r)
> +                     return r;
> +     }
> +
>        if (p->bo_list) {
>                for (i = 0; i < p->bo_list->num_entries; i++) {
>                        struct fence *f;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index d05546e..b9cd686 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1576,6 +1576,14 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>        vm->last_eviction_counter = atomic64_read(&adev->num_evictions);
>        amdgpu_bo_unreserve(vm->page_directory);
>
> +     if (amdgpu_sriov_vf(adev)) {
> +             BUG_ON(!adev->virt.map_csa);
> +             BUG_ON(!adev->virt.unmap_csa);
> +             r = adev->virt.map_csa(adev, vm);
> +             if (r)
> +                     goto error_free_page_directory;
> +     }
> +

Just completely drop that. Updating the VM on the first command
submission should be sufficient.

Christian.

>        return 0;
>
>   error_free_page_directory:
> @@ -1606,6 +1614,9 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>        struct amdgpu_bo_va_mapping *mapping, *tmp;
>        int i;
>
> +     if (amdgpu_sriov_vf(adev))
> +             adev->virt.unmap_csa(adev, vm);
> +
>        amd_sched_entity_fini(vm->entity.sched, &vm->entity);
>
>        if (!RB_EMPTY_ROOT(&vm->va)) {



[-- Attachment #1.2: Type: text/html, Size: 6841 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 7/7] drm/amdgpu:map/unmap static csa accordingly

[Christian König]

Adding the BO and it's mapping is VM specific code, we should initialize 
that directly in amdgpu_vm_init() and not call any helper to delegate 
the work.

Especially don't call from the VM code into the virt code and back into 
the VM code.

If you really want to keep that in amdgpu_virt. an alternative would be 
to put the call into amdgpu_driver_open_kms() directly after calling 
amdgpu_vm_init().

Regards,
Christian.

Am 10.01.2017 um 03:40 schrieb Liu, Monk:
>
> why you want to drop that "adev->virt.map_csa()" calling ?  without 
> that calling you don't have CSA's initialized bo_va and mappings as 
> well, and without bo_va and mappings how you can make 
> "amdgpu_vm_bo_update()" invoke work ?
>
>
> Monk
>
> ------------------------------------------------------------------------
> *发件人:* Christian König <deathsimple@vodafone.de>
> *发送时间:* 2017年1月9日 19:04:44
> *收件人:* Liu, Monk; amd-gfx@lists.freedesktop.org
> *主题:* Re: [PATCH 7/7] drm/amdgpu:map/unmap static csa accordingly
> Am 09.01.2017 um 09:03 schrieb Monk Liu:
> > and update CSA bo_va in each submit
> >
> > Change-Id: I5ed73e1b7f89743d90298bc814a42a91e166be3b
> > Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 14 ++++++++++++++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 11 +++++++++++
> >   2 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > index 6159afc..083ab73 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > @@ -771,6 +771,20 @@ static int amdgpu_bo_vm_update_pte(struct 
> amdgpu_cs_parser *p,
> >        if (r)
> >                return r;
> >
> > +     if (amdgpu_sriov_vf(adev)) {
> > +             struct fence *f;
>
> A new line is needed between deceleration and code.
>
> > +             bo_va = vm->vm_virt.csa_bo_va;
> > +             BUG_ON(!bo_va);
> > +             r = amdgpu_vm_bo_update(adev, bo_va, false);
> > +             if (r)
> > +                     return r;
> > +
> > +             f = bo_va->last_pt_update;
> > +             r = amdgpu_sync_fence(adev, &p->job->sync, f);
> > +             if (r)
> > +                     return r;
> > +     }
> > +
> >        if (p->bo_list) {
> >                for (i = 0; i < p->bo_list->num_entries; i++) {
> >                        struct fence *f;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index d05546e..b9cd686 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -1576,6 +1576,14 @@ int amdgpu_vm_init(struct amdgpu_device 
> *adev, struct amdgpu_vm *vm)
> >        vm->last_eviction_counter = atomic64_read(&adev->num_evictions);
> >        amdgpu_bo_unreserve(vm->page_directory);
> >
> > +     if (amdgpu_sriov_vf(adev)) {
> > +             BUG_ON(!adev->virt.map_csa);
> > +             BUG_ON(!adev->virt.unmap_csa);
> > +             r = adev->virt.map_csa(adev, vm);
> > +             if (r)
> > +                     goto error_free_page_directory;
> > +     }
> > +
>
> Just completely drop that. Updating the VM on the first command
> submission should be sufficient.
>
> Christian.
>
> >        return 0;
> >
> >   error_free_page_directory:
> > @@ -1606,6 +1614,9 @@ void amdgpu_vm_fini(struct amdgpu_device 
> *adev, struct amdgpu_vm *vm)
> >        struct amdgpu_bo_va_mapping *mapping, *tmp;
> >        int i;
> >
> > +     if (amdgpu_sriov_vf(adev))
> > +             adev->virt.unmap_csa(adev, vm);
> > +
> >        amd_sched_entity_fini(vm->entity.sched, &vm->entity);
> >
> >        if (!RB_EMPTY_ROOT(&vm->va)) {
>
>

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

答复: 答复: [PATCH 7/7] drm/amdgpu:map/unmap static csa accordingly

[Liu, Monk]

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

then what about "amdgpu_vm_bo_map" ? we need call it first before call "amdgpu_vm_bo_update" in "amdgpu_bo_vm_update_pte" , correct ? otherwise we even not create PT bo for the CSA ...


Do you suggest that rip out of amdgpu_map_csa() routine totally, and manually call "amdgpu_vm_bo_map" as well as "amdgpu_vm_bo_update" in sequence in "amdgpu_vm_bo_update" ?


BR Monk

________________________________
发件人: Christian König <deathsimple@vodafone.de>
发送时间: 2017年1月10日 17:44:21
收件人: Liu, Monk; amd-gfx@lists.freedesktop.org
主题: Re: 答复: [PATCH 7/7] drm/amdgpu:map/unmap static csa accordingly

Adding the BO and it's mapping is VM specific code, we should initialize
that directly in amdgpu_vm_init() and not call any helper to delegate
the work.

Especially don't call from the VM code into the virt code and back into
the VM code.

If you really want to keep that in amdgpu_virt. an alternative would be
to put the call into amdgpu_driver_open_kms() directly after calling
amdgpu_vm_init().

Regards,
Christian.

Am 10.01.2017 um 03:40 schrieb Liu, Monk:
>
> why you want to drop that "adev->virt.map_csa()" calling ?  without
> that calling you don't have CSA's initialized bo_va and mappings as
> well, and without bo_va and mappings how you can make
> "amdgpu_vm_bo_update()" invoke work ?
>
>
> Monk
>
> ------------------------------------------------------------------------
> *发件人:* Christian König <deathsimple@vodafone.de>
> *发送时间:* 2017年1月9日 19:04:44
> *收件人:* Liu, Monk; amd-gfx@lists.freedesktop.org
> *主题:* Re: [PATCH 7/7] drm/amdgpu:map/unmap static csa accordingly
> Am 09.01.2017 um 09:03 schrieb Monk Liu:
> > and update CSA bo_va in each submit
> >
> > Change-Id: I5ed73e1b7f89743d90298bc814a42a91e166be3b
> > Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 14 ++++++++++++++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 11 +++++++++++
> >   2 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > index 6159afc..083ab73 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > @@ -771,6 +771,20 @@ static int amdgpu_bo_vm_update_pte(struct
> amdgpu_cs_parser *p,
> >        if (r)
> >                return r;
> >
> > +     if (amdgpu_sriov_vf(adev)) {
> > +             struct fence *f;
>
> A new line is needed between deceleration and code.
>
> > +             bo_va = vm->vm_virt.csa_bo_va;
> > +             BUG_ON(!bo_va);
> > +             r = amdgpu_vm_bo_update(adev, bo_va, false);
> > +             if (r)
> > +                     return r;
> > +
> > +             f = bo_va->last_pt_update;
> > +             r = amdgpu_sync_fence(adev, &p->job->sync, f);
> > +             if (r)
> > +                     return r;
> > +     }
> > +
> >        if (p->bo_list) {
> >                for (i = 0; i < p->bo_list->num_entries; i++) {
> >                        struct fence *f;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index d05546e..b9cd686 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -1576,6 +1576,14 @@ int amdgpu_vm_init(struct amdgpu_device
> *adev, struct amdgpu_vm *vm)
> >        vm->last_eviction_counter = atomic64_read(&adev->num_evictions);
> >        amdgpu_bo_unreserve(vm->page_directory);
> >
> > +     if (amdgpu_sriov_vf(adev)) {
> > +             BUG_ON(!adev->virt.map_csa);
> > +             BUG_ON(!adev->virt.unmap_csa);
> > +             r = adev->virt.map_csa(adev, vm);
> > +             if (r)
> > +                     goto error_free_page_directory;
> > +     }
> > +
>
> Just completely drop that. Updating the VM on the first command
> submission should be sufficient.
>
> Christian.
>
> >        return 0;
> >
> >   error_free_page_directory:
> > @@ -1606,6 +1614,9 @@ void amdgpu_vm_fini(struct amdgpu_device
> *adev, struct amdgpu_vm *vm)
> >        struct amdgpu_bo_va_mapping *mapping, *tmp;
> >        int i;
> >
> > +     if (amdgpu_sriov_vf(adev))
> > +             adev->virt.unmap_csa(adev, vm);
> > +
> >        amd_sched_entity_fini(vm->entity.sched, &vm->entity);
> >
> >        if (!RB_EMPTY_ROOT(&vm->va)) {
>
>


[-- Attachment #1.2: Type: text/html, Size: 8691 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 7/7] drm/amdgpu:map/unmap static csa accordingly

[Christian König]

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

> Do you suggest that rip out of amdgpu_map_csa() routine totally,
>
Yes, mapping the CSA is not something specific to virtualization, so we 
shouldn't put that into amdgpu_virt.c.

> and manually call "amdgpu_vm_bo_map" as well as "amdgpu_vm_bo_update" 
> in sequence in "amdgpu_vm_bo_update" ?
>
No, during command submission it is to late to allocate the PT. Better 
put this a layer up into amdgpu_driver_open_kms() after the VM is 
initialized.

Regards,
Christian.

Am 10.01.2017 um 10:53 schrieb Liu, Monk:
>
> then what about "amdgpu_vm_bo_map" ? we need call it first before call 
> "amdgpu_vm_bo_update" in "amdgpu_bo_vm_update_pte" , correct ? 
> otherwise we even not create PT bo for the CSA ...
>
>
> Do you suggest that rip out of amdgpu_map_csa() routine totally, and 
> manually call "amdgpu_vm_bo_map" as well as "amdgpu_vm_bo_update" in 
> sequence in "amdgpu_vm_bo_update" ?
>
>
> BR Monk
>
> ------------------------------------------------------------------------
> *发件人:* Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
> *发送时间:* 2017年1月10日 17:44:21
> *收件人:* Liu, Monk; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> *主题:* Re: 答复: [PATCH 7/7] drm/amdgpu:map/unmap static csa accordingly
> Adding the BO and it's mapping is VM specific code, we should initialize
> that directly in amdgpu_vm_init() and not call any helper to delegate
> the work.
>
> Especially don't call from the VM code into the virt code and back into
> the VM code.
>
> If you really want to keep that in amdgpu_virt. an alternative would be
> to put the call into amdgpu_driver_open_kms() directly after calling
> amdgpu_vm_init().
>
> Regards,
> Christian.
>
> Am 10.01.2017 um 03:40 schrieb Liu, Monk:
> >
> > why you want to drop that "adev->virt.map_csa()" calling ?  without
> > that calling you don't have CSA's initialized bo_va and mappings as
> > well, and without bo_va and mappings how you can make
> > "amdgpu_vm_bo_update()" invoke work ?
> >
> >
> > Monk
> >
> > ------------------------------------------------------------------------
> > *发件人:* Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
> > *发送时间:* 2017年1月9日 19:04:44
> > *收件人:* Liu, Monk; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> > *主题:* Re: [PATCH 7/7] drm/amdgpu:map/unmap static csa accordingly
> > Am 09.01.2017 um 09:03 schrieb Monk Liu:
> > > and update CSA bo_va in each submit
> > >
> > > Change-Id: I5ed73e1b7f89743d90298bc814a42a91e166be3b
> > > Signed-off-by: Monk Liu <Monk.Liu-5C7GfCeVMHo@public.gmane.org>
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 14 ++++++++++++++
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 11 +++++++++++
> > >   2 files changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > index 6159afc..083ab73 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > @@ -771,6 +771,20 @@ static int amdgpu_bo_vm_update_pte(struct
> > amdgpu_cs_parser *p,
> > >        if (r)
> > >                return r;
> > >
> > > +     if (amdgpu_sriov_vf(adev)) {
> > > +             struct fence *f;
> >
> > A new line is needed between deceleration and code.
> >
> > > +             bo_va = vm->vm_virt.csa_bo_va;
> > > +             BUG_ON(!bo_va);
> > > +             r = amdgpu_vm_bo_update(adev, bo_va, false);
> > > +             if (r)
> > > +                     return r;
> > > +
> > > +             f = bo_va->last_pt_update;
> > > +             r = amdgpu_sync_fence(adev, &p->job->sync, f);
> > > +             if (r)
> > > +                     return r;
> > > +     }
> > > +
> > >        if (p->bo_list) {
> > >                for (i = 0; i < p->bo_list->num_entries; i++) {
> > >                        struct fence *f;
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > index d05546e..b9cd686 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > @@ -1576,6 +1576,14 @@ int amdgpu_vm_init(struct amdgpu_device
> > *adev, struct amdgpu_vm *vm)
> > >        vm->last_eviction_counter = 
> atomic64_read(&adev->num_evictions);
> > >        amdgpu_bo_unreserve(vm->page_directory);
> > >
> > > +     if (amdgpu_sriov_vf(adev)) {
> > > +             BUG_ON(!adev->virt.map_csa);
> > > +             BUG_ON(!adev->virt.unmap_csa);
> > > +             r = adev->virt.map_csa(adev, vm);
> > > +             if (r)
> > > +                     goto error_free_page_directory;
> > > +     }
> > > +
> >
> > Just completely drop that. Updating the VM on the first command
> > submission should be sufficient.
> >
> > Christian.
> >
> > >        return 0;
> > >
> > >   error_free_page_directory:
> > > @@ -1606,6 +1614,9 @@ void amdgpu_vm_fini(struct amdgpu_device
> > *adev, struct amdgpu_vm *vm)
> > >        struct amdgpu_bo_va_mapping *mapping, *tmp;
> > >        int i;
> > >
> > > +     if (amdgpu_sriov_vf(adev))
> > > +             adev->virt.unmap_csa(adev, vm);
> > > +
> > >        amd_sched_entity_fini(vm->entity.sched, &vm->entity);
> > >
> > >        if (!RB_EMPTY_ROOT(&vm->va)) {
> >
> >
>


[-- Attachment #1.2: Type: text/html, Size: 10555 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