* [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
@ 2019-08-23 3:34 Monk Liu
[not found] ` <1566531249-1396-1-git-send-email-Monk.Liu-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Monk Liu @ 2019-08-23 3:34 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Monk Liu
for SOC15/vega10 the BACO reset would introduce vram lost in
the high end address range and current kmd's vram lost
checking cannot catch it since it only check visible frame buffer
TODO:
to confirm if mode1/2 reset would introduce vram lost
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++-----
drivers/gpu/drm/amd/amdgpu/cik.c | 2 +-
drivers/gpu/drm/amd/amdgpu/nv.c | 10 +++++++---
drivers/gpu/drm/amd/amdgpu/si.c | 2 +-
drivers/gpu/drm/amd/amdgpu/soc15.c | 4 +++-
drivers/gpu/drm/amd/amdgpu/vi.c | 2 +-
7 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index f6ae565..1fe3756 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -552,7 +552,7 @@ struct amdgpu_asic_funcs {
int (*read_register)(struct amdgpu_device *adev, u32 se_num,
u32 sh_num, u32 reg_offset, u32 *value);
void (*set_vga_state)(struct amdgpu_device *adev, bool state);
- int (*reset)(struct amdgpu_device *adev);
+ int (*reset)(struct amdgpu_device *adev, bool *lost);
enum amd_reset_method (*reset_method)(struct amdgpu_device *adev);
/* get the reference clock */
u32 (*get_xclk)(struct amdgpu_device *adev);
@@ -1136,7 +1136,7 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
* ASICs macro.
*/
#define amdgpu_asic_set_vga_state(adev, state) (adev)->asic_funcs->set_vga_state((adev), (state))
-#define amdgpu_asic_reset(adev) (adev)->asic_funcs->reset((adev))
+#define amdgpu_asic_reset(adev, lost) (adev)->asic_funcs->reset((adev), (lost))
#define amdgpu_asic_reset_method(adev) (adev)->asic_funcs->reset_method((adev))
#define amdgpu_asic_get_xclk(adev) (adev)->asic_funcs->get_xclk((adev))
#define amdgpu_asic_set_uvd_clocks(adev, v, d) (adev)->asic_funcs->set_uvd_clocks((adev), (v), (d))
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 02b3e7d..8668cb8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2546,7 +2546,7 @@ static void amdgpu_device_xgmi_reset_func(struct work_struct *__work)
struct amdgpu_device *adev =
container_of(__work, struct amdgpu_device, xgmi_reset_work);
- adev->asic_reset_res = amdgpu_asic_reset(adev);
+ adev->asic_reset_res = amdgpu_asic_reset(adev, NULL);
if (adev->asic_reset_res)
DRM_WARN("ASIC reset failed with error, %d for drm dev, %s",
adev->asic_reset_res, adev->ddev->unique);
@@ -2751,7 +2751,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
* E.g., driver was not cleanly unloaded previously, etc.
*/
if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
- r = amdgpu_asic_reset(adev);
+ r = amdgpu_asic_reset(adev, NULL);
if (r) {
dev_err(adev->dev, "asic reset on init failed\n");
goto failed;
@@ -3084,7 +3084,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
pci_disable_device(dev->pdev);
pci_set_power_state(dev->pdev, PCI_D3hot);
} else {
- r = amdgpu_asic_reset(adev);
+ r = amdgpu_asic_reset(adev, NULL);
if (r)
DRM_ERROR("amdgpu asic reset failed\n");
}
@@ -3604,7 +3604,7 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
if (!queue_work(system_highpri_wq, &tmp_adev->xgmi_reset_work))
r = -EALREADY;
} else
- r = amdgpu_asic_reset(tmp_adev);
+ r = amdgpu_asic_reset(tmp_adev, &vram_lost);
if (r) {
DRM_ERROR("ASIC reset failed with error, %d for drm dev, %s",
@@ -3645,7 +3645,9 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
if (r)
goto out;
- vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
+ if (!vram_lost)
+ vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
+
if (vram_lost) {
DRM_INFO("VRAM is lost due to GPU reset!\n");
atomic_inc(&tmp_adev->vram_lost_counter);
diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c
index 7b63d7a..0f25b82 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik.c
@@ -1277,7 +1277,7 @@ static int cik_gpu_pci_config_reset(struct amdgpu_device *adev)
* to reset them.
* Returns 0 for success.
*/
-static int cik_asic_reset(struct amdgpu_device *adev)
+static int cik_asic_reset(struct amdgpu_device *adev, bool *vramlost)
{
int r;
diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
index a3d99f2..53de7a6 100644
--- a/drivers/gpu/drm/amd/amdgpu/nv.c
+++ b/drivers/gpu/drm/amd/amdgpu/nv.c
@@ -301,7 +301,7 @@ nv_asic_reset_method(struct amdgpu_device *adev)
return AMD_RESET_METHOD_MODE1;
}
-static int nv_asic_reset(struct amdgpu_device *adev)
+static int nv_asic_reset(struct amdgpu_device *adev, bool *vramlost)
{
/* FIXME: it doesn't work since vega10 */
@@ -315,10 +315,14 @@ static int nv_asic_reset(struct amdgpu_device *adev)
int ret = 0;
struct smu_context *smu = &adev->smu;
- if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)
+ if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO) {
+ if (vramlost)
+ *vramlost = true;
ret = smu_baco_reset(smu);
- else
+ }
+ else {
ret = nv_asic_mode1_reset(adev);
+ }
return ret;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/si.c b/drivers/gpu/drm/amd/amdgpu/si.c
index 9043614..f324099 100644
--- a/drivers/gpu/drm/amd/amdgpu/si.c
+++ b/drivers/gpu/drm/amd/amdgpu/si.c
@@ -1180,7 +1180,7 @@ static bool si_read_bios_from_rom(struct amdgpu_device *adev,
}
//xxx: not implemented
-static int si_asic_reset(struct amdgpu_device *adev)
+static int si_asic_reset(struct amdgpu_device *adev, bool *vramlost)
{
return 0;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
index fe2212df..12b2966 100644
--- a/drivers/gpu/drm/amd/amdgpu/soc15.c
+++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
@@ -553,10 +553,12 @@ soc15_asic_reset_method(struct amdgpu_device *adev)
return AMD_RESET_METHOD_MODE1;
}
-static int soc15_asic_reset(struct amdgpu_device *adev)
+static int soc15_asic_reset(struct amdgpu_device *adev, bool *vramlost)
{
switch (soc15_asic_reset_method(adev)) {
case AMD_RESET_METHOD_BACO:
+ if (vramlost)
+ *vramlost = true;
return soc15_asic_baco_reset(adev);
case AMD_RESET_METHOD_MODE2:
return soc15_mode2_reset(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
index 56c882b..8eceb00 100644
--- a/drivers/gpu/drm/amd/amdgpu/vi.c
+++ b/drivers/gpu/drm/amd/amdgpu/vi.c
@@ -696,7 +696,7 @@ static int vi_gpu_pci_config_reset(struct amdgpu_device *adev)
* to reset them.
* Returns 0 for success.
*/
-static int vi_asic_reset(struct amdgpu_device *adev)
+static int vi_asic_reset(struct amdgpu_device *adev, bool *vramlost)
{
int r;
--
2.7.4
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
[not found] ` <1566531249-1396-1-git-send-email-Monk.Liu-5C7GfCeVMHo@public.gmane.org>
@ 2019-08-23 8:26 ` Christian König
[not found] ` <15d22497-cf4d-0d15-236c-5bc2b65eb656-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Christian König @ 2019-08-23 8:26 UTC (permalink / raw)
To: Monk Liu, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Am 23.08.19 um 05:34 schrieb Monk Liu:
> for SOC15/vega10 the BACO reset would introduce vram lost in
> the high end address range and current kmd's vram lost
> checking cannot catch it since it only check visible frame buffer
>
> TODO:
> to confirm if mode1/2 reset would introduce vram lost
Looks good in general, but I would make the value mandatory or maybe use
a special return code instead.
On the other hand wouldn't it be simpler to just increment
vram_lost_counter?
Christian.
>
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++--
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++-----
> drivers/gpu/drm/amd/amdgpu/cik.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/nv.c | 10 +++++++---
> drivers/gpu/drm/amd/amdgpu/si.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/soc15.c | 4 +++-
> drivers/gpu/drm/amd/amdgpu/vi.c | 2 +-
> 7 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index f6ae565..1fe3756 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -552,7 +552,7 @@ struct amdgpu_asic_funcs {
> int (*read_register)(struct amdgpu_device *adev, u32 se_num,
> u32 sh_num, u32 reg_offset, u32 *value);
> void (*set_vga_state)(struct amdgpu_device *adev, bool state);
> - int (*reset)(struct amdgpu_device *adev);
> + int (*reset)(struct amdgpu_device *adev, bool *lost);
> enum amd_reset_method (*reset_method)(struct amdgpu_device *adev);
> /* get the reference clock */
> u32 (*get_xclk)(struct amdgpu_device *adev);
> @@ -1136,7 +1136,7 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
> * ASICs macro.
> */
> #define amdgpu_asic_set_vga_state(adev, state) (adev)->asic_funcs->set_vga_state((adev), (state))
> -#define amdgpu_asic_reset(adev) (adev)->asic_funcs->reset((adev))
> +#define amdgpu_asic_reset(adev, lost) (adev)->asic_funcs->reset((adev), (lost))
> #define amdgpu_asic_reset_method(adev) (adev)->asic_funcs->reset_method((adev))
> #define amdgpu_asic_get_xclk(adev) (adev)->asic_funcs->get_xclk((adev))
> #define amdgpu_asic_set_uvd_clocks(adev, v, d) (adev)->asic_funcs->set_uvd_clocks((adev), (v), (d))
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 02b3e7d..8668cb8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2546,7 +2546,7 @@ static void amdgpu_device_xgmi_reset_func(struct work_struct *__work)
> struct amdgpu_device *adev =
> container_of(__work, struct amdgpu_device, xgmi_reset_work);
>
> - adev->asic_reset_res = amdgpu_asic_reset(adev);
> + adev->asic_reset_res = amdgpu_asic_reset(adev, NULL);
> if (adev->asic_reset_res)
> DRM_WARN("ASIC reset failed with error, %d for drm dev, %s",
> adev->asic_reset_res, adev->ddev->unique);
> @@ -2751,7 +2751,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> * E.g., driver was not cleanly unloaded previously, etc.
> */
> if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
> - r = amdgpu_asic_reset(adev);
> + r = amdgpu_asic_reset(adev, NULL);
> if (r) {
> dev_err(adev->dev, "asic reset on init failed\n");
> goto failed;
> @@ -3084,7 +3084,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
> pci_disable_device(dev->pdev);
> pci_set_power_state(dev->pdev, PCI_D3hot);
> } else {
> - r = amdgpu_asic_reset(adev);
> + r = amdgpu_asic_reset(adev, NULL);
> if (r)
> DRM_ERROR("amdgpu asic reset failed\n");
> }
> @@ -3604,7 +3604,7 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
> if (!queue_work(system_highpri_wq, &tmp_adev->xgmi_reset_work))
> r = -EALREADY;
> } else
> - r = amdgpu_asic_reset(tmp_adev);
> + r = amdgpu_asic_reset(tmp_adev, &vram_lost);
>
> if (r) {
> DRM_ERROR("ASIC reset failed with error, %d for drm dev, %s",
> @@ -3645,7 +3645,9 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
> if (r)
> goto out;
>
> - vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
> + if (!vram_lost)
> + vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
> +
> if (vram_lost) {
> DRM_INFO("VRAM is lost due to GPU reset!\n");
> atomic_inc(&tmp_adev->vram_lost_counter);
> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c
> index 7b63d7a..0f25b82 100644
> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
> @@ -1277,7 +1277,7 @@ static int cik_gpu_pci_config_reset(struct amdgpu_device *adev)
> * to reset them.
> * Returns 0 for success.
> */
> -static int cik_asic_reset(struct amdgpu_device *adev)
> +static int cik_asic_reset(struct amdgpu_device *adev, bool *vramlost)
> {
> int r;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
> index a3d99f2..53de7a6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
> @@ -301,7 +301,7 @@ nv_asic_reset_method(struct amdgpu_device *adev)
> return AMD_RESET_METHOD_MODE1;
> }
>
> -static int nv_asic_reset(struct amdgpu_device *adev)
> +static int nv_asic_reset(struct amdgpu_device *adev, bool *vramlost)
> {
>
> /* FIXME: it doesn't work since vega10 */
> @@ -315,10 +315,14 @@ static int nv_asic_reset(struct amdgpu_device *adev)
> int ret = 0;
> struct smu_context *smu = &adev->smu;
>
> - if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)
> + if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO) {
> + if (vramlost)
> + *vramlost = true;
> ret = smu_baco_reset(smu);
> - else
> + }
> + else {
> ret = nv_asic_mode1_reset(adev);
> + }
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/si.c b/drivers/gpu/drm/amd/amdgpu/si.c
> index 9043614..f324099 100644
> --- a/drivers/gpu/drm/amd/amdgpu/si.c
> +++ b/drivers/gpu/drm/amd/amdgpu/si.c
> @@ -1180,7 +1180,7 @@ static bool si_read_bios_from_rom(struct amdgpu_device *adev,
> }
>
> //xxx: not implemented
> -static int si_asic_reset(struct amdgpu_device *adev)
> +static int si_asic_reset(struct amdgpu_device *adev, bool *vramlost)
> {
> return 0;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
> index fe2212df..12b2966 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> @@ -553,10 +553,12 @@ soc15_asic_reset_method(struct amdgpu_device *adev)
> return AMD_RESET_METHOD_MODE1;
> }
>
> -static int soc15_asic_reset(struct amdgpu_device *adev)
> +static int soc15_asic_reset(struct amdgpu_device *adev, bool *vramlost)
> {
> switch (soc15_asic_reset_method(adev)) {
> case AMD_RESET_METHOD_BACO:
> + if (vramlost)
> + *vramlost = true;
> return soc15_asic_baco_reset(adev);
> case AMD_RESET_METHOD_MODE2:
> return soc15_mode2_reset(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
> index 56c882b..8eceb00 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> @@ -696,7 +696,7 @@ static int vi_gpu_pci_config_reset(struct amdgpu_device *adev)
> * to reset them.
> * Returns 0 for success.
> */
> -static int vi_asic_reset(struct amdgpu_device *adev)
> +static int vi_asic_reset(struct amdgpu_device *adev, bool *vramlost)
> {
> int r;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
[not found] ` <15d22497-cf4d-0d15-236c-5bc2b65eb656-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2019-08-23 8:32 ` Liu, Monk
[not found] ` <MN2PR12MB393366D270445A63B053DA3184A40-rweVpJHSKTq/67K4VYF1uAdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Liu, Monk @ 2019-08-23 8:32 UTC (permalink / raw)
To: Koenig, Christian,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
In where ? if you mean in amdgpu_device_recover routine I won't do that since the reset() can do any kind of reset like:
1) PF FLR
2) mode1/2 reset
3) magic reset through config space
4) BACO reset
PF FLR won't cause VRAM lost, mode_1/2 is not clear to me, only BACO reset is definitely a vram lost reset
If you increase the counter in general function that will be not accurate
_____________________________________
Monk Liu|GPU Virtualization Team |AMD
-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Friday, August 23, 2019 4:27 PM
To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
Am 23.08.19 um 05:34 schrieb Monk Liu:
> for SOC15/vega10 the BACO reset would introduce vram lost in the high
> end address range and current kmd's vram lost checking cannot catch it
> since it only check visible frame buffer
>
> TODO:
> to confirm if mode1/2 reset would introduce vram lost
Looks good in general, but I would make the value mandatory or maybe use a special return code instead.
On the other hand wouldn't it be simpler to just increment vram_lost_counter?
Christian.
>
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++--
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++-----
> drivers/gpu/drm/amd/amdgpu/cik.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/nv.c | 10 +++++++---
> drivers/gpu/drm/amd/amdgpu/si.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/soc15.c | 4 +++-
> drivers/gpu/drm/amd/amdgpu/vi.c | 2 +-
> 7 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index f6ae565..1fe3756 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -552,7 +552,7 @@ struct amdgpu_asic_funcs {
> int (*read_register)(struct amdgpu_device *adev, u32 se_num,
> u32 sh_num, u32 reg_offset, u32 *value);
> void (*set_vga_state)(struct amdgpu_device *adev, bool state);
> - int (*reset)(struct amdgpu_device *adev);
> + int (*reset)(struct amdgpu_device *adev, bool *lost);
> enum amd_reset_method (*reset_method)(struct amdgpu_device *adev);
> /* get the reference clock */
> u32 (*get_xclk)(struct amdgpu_device *adev); @@ -1136,7 +1136,7 @@
> int emu_soc_asic_init(struct amdgpu_device *adev);
> * ASICs macro.
> */
> #define amdgpu_asic_set_vga_state(adev, state)
> (adev)->asic_funcs->set_vga_state((adev), (state)) -#define
> amdgpu_asic_reset(adev) (adev)->asic_funcs->reset((adev))
> +#define amdgpu_asic_reset(adev, lost)
> +(adev)->asic_funcs->reset((adev), (lost))
> #define amdgpu_asic_reset_method(adev) (adev)->asic_funcs->reset_method((adev))
> #define amdgpu_asic_get_xclk(adev) (adev)->asic_funcs->get_xclk((adev))
> #define amdgpu_asic_set_uvd_clocks(adev, v, d)
> (adev)->asic_funcs->set_uvd_clocks((adev), (v), (d)) diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 02b3e7d..8668cb8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2546,7 +2546,7 @@ static void amdgpu_device_xgmi_reset_func(struct work_struct *__work)
> struct amdgpu_device *adev =
> container_of(__work, struct amdgpu_device, xgmi_reset_work);
>
> - adev->asic_reset_res = amdgpu_asic_reset(adev);
> + adev->asic_reset_res = amdgpu_asic_reset(adev, NULL);
> if (adev->asic_reset_res)
> DRM_WARN("ASIC reset failed with error, %d for drm dev, %s",
> adev->asic_reset_res, adev->ddev->unique); @@ -2751,7 +2751,7 @@
> int amdgpu_device_init(struct amdgpu_device *adev,
> * E.g., driver was not cleanly unloaded previously, etc.
> */
> if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
> - r = amdgpu_asic_reset(adev);
> + r = amdgpu_asic_reset(adev, NULL);
> if (r) {
> dev_err(adev->dev, "asic reset on init failed\n");
> goto failed;
> @@ -3084,7 +3084,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
> pci_disable_device(dev->pdev);
> pci_set_power_state(dev->pdev, PCI_D3hot);
> } else {
> - r = amdgpu_asic_reset(adev);
> + r = amdgpu_asic_reset(adev, NULL);
> if (r)
> DRM_ERROR("amdgpu asic reset failed\n");
> }
> @@ -3604,7 +3604,7 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
> if (!queue_work(system_highpri_wq, &tmp_adev->xgmi_reset_work))
> r = -EALREADY;
> } else
> - r = amdgpu_asic_reset(tmp_adev);
> + r = amdgpu_asic_reset(tmp_adev, &vram_lost);
>
> if (r) {
> DRM_ERROR("ASIC reset failed with error, %d for drm dev, %s", @@
> -3645,7 +3645,9 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
> if (r)
> goto out;
>
> - vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
> + if (!vram_lost)
> + vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
> +
> if (vram_lost) {
> DRM_INFO("VRAM is lost due to GPU reset!\n");
> atomic_inc(&tmp_adev->vram_lost_counter);
> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
> b/drivers/gpu/drm/amd/amdgpu/cik.c
> index 7b63d7a..0f25b82 100644
> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
> @@ -1277,7 +1277,7 @@ static int cik_gpu_pci_config_reset(struct amdgpu_device *adev)
> * to reset them.
> * Returns 0 for success.
> */
> -static int cik_asic_reset(struct amdgpu_device *adev)
> +static int cik_asic_reset(struct amdgpu_device *adev, bool *vramlost)
> {
> int r;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c
> b/drivers/gpu/drm/amd/amdgpu/nv.c index a3d99f2..53de7a6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
> @@ -301,7 +301,7 @@ nv_asic_reset_method(struct amdgpu_device *adev)
> return AMD_RESET_METHOD_MODE1;
> }
>
> -static int nv_asic_reset(struct amdgpu_device *adev)
> +static int nv_asic_reset(struct amdgpu_device *adev, bool *vramlost)
> {
>
> /* FIXME: it doesn't work since vega10 */ @@ -315,10 +315,14 @@
> static int nv_asic_reset(struct amdgpu_device *adev)
> int ret = 0;
> struct smu_context *smu = &adev->smu;
>
> - if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)
> + if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO) {
> + if (vramlost)
> + *vramlost = true;
> ret = smu_baco_reset(smu);
> - else
> + }
> + else {
> ret = nv_asic_mode1_reset(adev);
> + }
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/si.c
> b/drivers/gpu/drm/amd/amdgpu/si.c index 9043614..f324099 100644
> --- a/drivers/gpu/drm/amd/amdgpu/si.c
> +++ b/drivers/gpu/drm/amd/amdgpu/si.c
> @@ -1180,7 +1180,7 @@ static bool si_read_bios_from_rom(struct amdgpu_device *adev,
> }
>
> //xxx: not implemented
> -static int si_asic_reset(struct amdgpu_device *adev)
> +static int si_asic_reset(struct amdgpu_device *adev, bool *vramlost)
> {
> return 0;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
> b/drivers/gpu/drm/amd/amdgpu/soc15.c
> index fe2212df..12b2966 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> @@ -553,10 +553,12 @@ soc15_asic_reset_method(struct amdgpu_device *adev)
> return AMD_RESET_METHOD_MODE1;
> }
>
> -static int soc15_asic_reset(struct amdgpu_device *adev)
> +static int soc15_asic_reset(struct amdgpu_device *adev, bool
> +*vramlost)
> {
> switch (soc15_asic_reset_method(adev)) {
> case AMD_RESET_METHOD_BACO:
> + if (vramlost)
> + *vramlost = true;
> return soc15_asic_baco_reset(adev);
> case AMD_RESET_METHOD_MODE2:
> return soc15_mode2_reset(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
> b/drivers/gpu/drm/amd/amdgpu/vi.c index 56c882b..8eceb00 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> @@ -696,7 +696,7 @@ static int vi_gpu_pci_config_reset(struct amdgpu_device *adev)
> * to reset them.
> * Returns 0 for success.
> */
> -static int vi_asic_reset(struct amdgpu_device *adev)
> +static int vi_asic_reset(struct amdgpu_device *adev, bool *vramlost)
> {
> int r;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
[not found] ` <MN2PR12MB393366D270445A63B053DA3184A40-rweVpJHSKTq/67K4VYF1uAdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
@ 2019-08-23 8:34 ` Koenig, Christian
[not found] ` <82e98a1b-a624-1e54-aae7-1a6ff9dda7f2-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Koenig, Christian @ 2019-08-23 8:34 UTC (permalink / raw)
To: Liu, Monk,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
I thought in the BACO reset function.
The top level reset function doesn't do much more than increment the
vram_lost_counter either.
Christian.
Am 23.08.19 um 10:32 schrieb Liu, Monk:
>>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
> In where ? if you mean in amdgpu_device_recover routine I won't do that since the reset() can do any kind of reset like:
> 1) PF FLR
> 2) mode1/2 reset
> 3) magic reset through config space
> 4) BACO reset
>
> PF FLR won't cause VRAM lost, mode_1/2 is not clear to me, only BACO reset is definitely a vram lost reset
>
> If you increase the counter in general function that will be not accurate
> _____________________________________
> Monk Liu|GPU Virtualization Team |AMD
>
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Friday, August 23, 2019 4:27 PM
> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
>
> Am 23.08.19 um 05:34 schrieb Monk Liu:
>> for SOC15/vega10 the BACO reset would introduce vram lost in the high
>> end address range and current kmd's vram lost checking cannot catch it
>> since it only check visible frame buffer
>>
>> TODO:
>> to confirm if mode1/2 reset would introduce vram lost
> Looks good in general, but I would make the value mandatory or maybe use a special return code instead.
>
> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
>
> Christian.
>
>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++--
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++-----
>> drivers/gpu/drm/amd/amdgpu/cik.c | 2 +-
>> drivers/gpu/drm/amd/amdgpu/nv.c | 10 +++++++---
>> drivers/gpu/drm/amd/amdgpu/si.c | 2 +-
>> drivers/gpu/drm/amd/amdgpu/soc15.c | 4 +++-
>> drivers/gpu/drm/amd/amdgpu/vi.c | 2 +-
>> 7 files changed, 22 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index f6ae565..1fe3756 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -552,7 +552,7 @@ struct amdgpu_asic_funcs {
>> int (*read_register)(struct amdgpu_device *adev, u32 se_num,
>> u32 sh_num, u32 reg_offset, u32 *value);
>> void (*set_vga_state)(struct amdgpu_device *adev, bool state);
>> - int (*reset)(struct amdgpu_device *adev);
>> + int (*reset)(struct amdgpu_device *adev, bool *lost);
>> enum amd_reset_method (*reset_method)(struct amdgpu_device *adev);
>> /* get the reference clock */
>> u32 (*get_xclk)(struct amdgpu_device *adev); @@ -1136,7 +1136,7 @@
>> int emu_soc_asic_init(struct amdgpu_device *adev);
>> * ASICs macro.
>> */
>> #define amdgpu_asic_set_vga_state(adev, state)
>> (adev)->asic_funcs->set_vga_state((adev), (state)) -#define
>> amdgpu_asic_reset(adev) (adev)->asic_funcs->reset((adev))
>> +#define amdgpu_asic_reset(adev, lost)
>> +(adev)->asic_funcs->reset((adev), (lost))
>> #define amdgpu_asic_reset_method(adev) (adev)->asic_funcs->reset_method((adev))
>> #define amdgpu_asic_get_xclk(adev) (adev)->asic_funcs->get_xclk((adev))
>> #define amdgpu_asic_set_uvd_clocks(adev, v, d)
>> (adev)->asic_funcs->set_uvd_clocks((adev), (v), (d)) diff --git
>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 02b3e7d..8668cb8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -2546,7 +2546,7 @@ static void amdgpu_device_xgmi_reset_func(struct work_struct *__work)
>> struct amdgpu_device *adev =
>> container_of(__work, struct amdgpu_device, xgmi_reset_work);
>>
>> - adev->asic_reset_res = amdgpu_asic_reset(adev);
>> + adev->asic_reset_res = amdgpu_asic_reset(adev, NULL);
>> if (adev->asic_reset_res)
>> DRM_WARN("ASIC reset failed with error, %d for drm dev, %s",
>> adev->asic_reset_res, adev->ddev->unique); @@ -2751,7 +2751,7 @@
>> int amdgpu_device_init(struct amdgpu_device *adev,
>> * E.g., driver was not cleanly unloaded previously, etc.
>> */
>> if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
>> - r = amdgpu_asic_reset(adev);
>> + r = amdgpu_asic_reset(adev, NULL);
>> if (r) {
>> dev_err(adev->dev, "asic reset on init failed\n");
>> goto failed;
>> @@ -3084,7 +3084,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
>> pci_disable_device(dev->pdev);
>> pci_set_power_state(dev->pdev, PCI_D3hot);
>> } else {
>> - r = amdgpu_asic_reset(adev);
>> + r = amdgpu_asic_reset(adev, NULL);
>> if (r)
>> DRM_ERROR("amdgpu asic reset failed\n");
>> }
>> @@ -3604,7 +3604,7 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>> if (!queue_work(system_highpri_wq, &tmp_adev->xgmi_reset_work))
>> r = -EALREADY;
>> } else
>> - r = amdgpu_asic_reset(tmp_adev);
>> + r = amdgpu_asic_reset(tmp_adev, &vram_lost);
>>
>> if (r) {
>> DRM_ERROR("ASIC reset failed with error, %d for drm dev, %s", @@
>> -3645,7 +3645,9 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>> if (r)
>> goto out;
>>
>> - vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>> + if (!vram_lost)
>> + vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>> +
>> if (vram_lost) {
>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>> atomic_inc(&tmp_adev->vram_lost_counter);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
>> b/drivers/gpu/drm/amd/amdgpu/cik.c
>> index 7b63d7a..0f25b82 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
>> @@ -1277,7 +1277,7 @@ static int cik_gpu_pci_config_reset(struct amdgpu_device *adev)
>> * to reset them.
>> * Returns 0 for success.
>> */
>> -static int cik_asic_reset(struct amdgpu_device *adev)
>> +static int cik_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>> {
>> int r;
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c
>> b/drivers/gpu/drm/amd/amdgpu/nv.c index a3d99f2..53de7a6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
>> @@ -301,7 +301,7 @@ nv_asic_reset_method(struct amdgpu_device *adev)
>> return AMD_RESET_METHOD_MODE1;
>> }
>>
>> -static int nv_asic_reset(struct amdgpu_device *adev)
>> +static int nv_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>> {
>>
>> /* FIXME: it doesn't work since vega10 */ @@ -315,10 +315,14 @@
>> static int nv_asic_reset(struct amdgpu_device *adev)
>> int ret = 0;
>> struct smu_context *smu = &adev->smu;
>>
>> - if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)
>> + if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO) {
>> + if (vramlost)
>> + *vramlost = true;
>> ret = smu_baco_reset(smu);
>> - else
>> + }
>> + else {
>> ret = nv_asic_mode1_reset(adev);
>> + }
>>
>> return ret;
>> }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/si.c
>> b/drivers/gpu/drm/amd/amdgpu/si.c index 9043614..f324099 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/si.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/si.c
>> @@ -1180,7 +1180,7 @@ static bool si_read_bios_from_rom(struct amdgpu_device *adev,
>> }
>>
>> //xxx: not implemented
>> -static int si_asic_reset(struct amdgpu_device *adev)
>> +static int si_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>> {
>> return 0;
>> }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
>> b/drivers/gpu/drm/amd/amdgpu/soc15.c
>> index fe2212df..12b2966 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
>> @@ -553,10 +553,12 @@ soc15_asic_reset_method(struct amdgpu_device *adev)
>> return AMD_RESET_METHOD_MODE1;
>> }
>>
>> -static int soc15_asic_reset(struct amdgpu_device *adev)
>> +static int soc15_asic_reset(struct amdgpu_device *adev, bool
>> +*vramlost)
>> {
>> switch (soc15_asic_reset_method(adev)) {
>> case AMD_RESET_METHOD_BACO:
>> + if (vramlost)
>> + *vramlost = true;
>> return soc15_asic_baco_reset(adev);
>> case AMD_RESET_METHOD_MODE2:
>> return soc15_mode2_reset(adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
>> b/drivers/gpu/drm/amd/amdgpu/vi.c index 56c882b..8eceb00 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>> @@ -696,7 +696,7 @@ static int vi_gpu_pci_config_reset(struct amdgpu_device *adev)
>> * to reset them.
>> * Returns 0 for success.
>> */
>> -static int vi_asic_reset(struct amdgpu_device *adev)
>> +static int vi_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>> {
>> int r;
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
[not found] ` <82e98a1b-a624-1e54-aae7-1a6ff9dda7f2-5C7GfCeVMHo@public.gmane.org>
@ 2019-08-23 8:57 ` Liu, Monk
[not found] ` <MN2PR12MB393324F969E3F29F76A0DA8384A40-rweVpJHSKTq/67K4VYF1uAdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Liu, Monk @ 2019-08-23 8:57 UTC (permalink / raw)
To: Koenig, Christian,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>> if (vram_lost) {
>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>> atomic_inc(&tmp_adev->vram_lost_counter);
Above is the original logic, if we increment the counter in BACO reset routine, we would potentially
Have another counter increasement if by coincidence the "amdgpu_device_check_vram_lost" successfully detected the vram lost (although right now it didn't ..)
Do you mean we remove the amdgpu_device_check_vram_lost(tmp_adev) in device_recovery() routine ?
_____________________________________
Monk Liu|GPU Virtualization Team |AMD
-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Friday, August 23, 2019 4:34 PM
To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
I thought in the BACO reset function.
The top level reset function doesn't do much more than increment the vram_lost_counter either.
Christian.
Am 23.08.19 um 10:32 schrieb Liu, Monk:
>>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
> In where ? if you mean in amdgpu_device_recover routine I won't do that since the reset() can do any kind of reset like:
> 1) PF FLR
> 2) mode1/2 reset
> 3) magic reset through config space
> 4) BACO reset
>
> PF FLR won't cause VRAM lost, mode_1/2 is not clear to me, only BACO
> reset is definitely a vram lost reset
>
> If you increase the counter in general function that will be not
> accurate _____________________________________
> Monk Liu|GPU Virtualization Team |AMD
>
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Friday, August 23, 2019 4:27 PM
> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for
> reset function
>
> Am 23.08.19 um 05:34 schrieb Monk Liu:
>> for SOC15/vega10 the BACO reset would introduce vram lost in the high
>> end address range and current kmd's vram lost checking cannot catch
>> it since it only check visible frame buffer
>>
>> TODO:
>> to confirm if mode1/2 reset would introduce vram lost
> Looks good in general, but I would make the value mandatory or maybe use a special return code instead.
>
> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
>
> Christian.
>
>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++--
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++-----
>> drivers/gpu/drm/amd/amdgpu/cik.c | 2 +-
>> drivers/gpu/drm/amd/amdgpu/nv.c | 10 +++++++---
>> drivers/gpu/drm/amd/amdgpu/si.c | 2 +-
>> drivers/gpu/drm/amd/amdgpu/soc15.c | 4 +++-
>> drivers/gpu/drm/amd/amdgpu/vi.c | 2 +-
>> 7 files changed, 22 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index f6ae565..1fe3756 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -552,7 +552,7 @@ struct amdgpu_asic_funcs {
>> int (*read_register)(struct amdgpu_device *adev, u32 se_num,
>> u32 sh_num, u32 reg_offset, u32 *value);
>> void (*set_vga_state)(struct amdgpu_device *adev, bool state);
>> - int (*reset)(struct amdgpu_device *adev);
>> + int (*reset)(struct amdgpu_device *adev, bool *lost);
>> enum amd_reset_method (*reset_method)(struct amdgpu_device *adev);
>> /* get the reference clock */
>> u32 (*get_xclk)(struct amdgpu_device *adev); @@ -1136,7 +1136,7
>> @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>> * ASICs macro.
>> */
>> #define amdgpu_asic_set_vga_state(adev, state)
>> (adev)->asic_funcs->set_vga_state((adev), (state)) -#define
>> amdgpu_asic_reset(adev) (adev)->asic_funcs->reset((adev))
>> +#define amdgpu_asic_reset(adev, lost)
>> +(adev)->asic_funcs->reset((adev), (lost))
>> #define amdgpu_asic_reset_method(adev) (adev)->asic_funcs->reset_method((adev))
>> #define amdgpu_asic_get_xclk(adev) (adev)->asic_funcs->get_xclk((adev))
>> #define amdgpu_asic_set_uvd_clocks(adev, v, d)
>> (adev)->asic_funcs->set_uvd_clocks((adev), (v), (d)) diff --git
>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 02b3e7d..8668cb8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -2546,7 +2546,7 @@ static void amdgpu_device_xgmi_reset_func(struct work_struct *__work)
>> struct amdgpu_device *adev =
>> container_of(__work, struct amdgpu_device, xgmi_reset_work);
>>
>> - adev->asic_reset_res = amdgpu_asic_reset(adev);
>> + adev->asic_reset_res = amdgpu_asic_reset(adev, NULL);
>> if (adev->asic_reset_res)
>> DRM_WARN("ASIC reset failed with error, %d for drm dev, %s",
>> adev->asic_reset_res, adev->ddev->unique); @@ -2751,7 +2751,7
>> @@ int amdgpu_device_init(struct amdgpu_device *adev,
>> * E.g., driver was not cleanly unloaded previously, etc.
>> */
>> if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
>> - r = amdgpu_asic_reset(adev);
>> + r = amdgpu_asic_reset(adev, NULL);
>> if (r) {
>> dev_err(adev->dev, "asic reset on init failed\n");
>> goto failed;
>> @@ -3084,7 +3084,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
>> pci_disable_device(dev->pdev);
>> pci_set_power_state(dev->pdev, PCI_D3hot);
>> } else {
>> - r = amdgpu_asic_reset(adev);
>> + r = amdgpu_asic_reset(adev, NULL);
>> if (r)
>> DRM_ERROR("amdgpu asic reset failed\n");
>> }
>> @@ -3604,7 +3604,7 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>> if (!queue_work(system_highpri_wq, &tmp_adev->xgmi_reset_work))
>> r = -EALREADY;
>> } else
>> - r = amdgpu_asic_reset(tmp_adev);
>> + r = amdgpu_asic_reset(tmp_adev, &vram_lost);
>>
>> if (r) {
>> DRM_ERROR("ASIC reset failed with error, %d for drm dev, %s",
>> @@
>> -3645,7 +3645,9 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>> if (r)
>> goto out;
>>
>> - vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>> + if (!vram_lost)
>> + vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>> +
>> if (vram_lost) {
>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>> atomic_inc(&tmp_adev->vram_lost_counter);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
>> b/drivers/gpu/drm/amd/amdgpu/cik.c
>> index 7b63d7a..0f25b82 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
>> @@ -1277,7 +1277,7 @@ static int cik_gpu_pci_config_reset(struct amdgpu_device *adev)
>> * to reset them.
>> * Returns 0 for success.
>> */
>> -static int cik_asic_reset(struct amdgpu_device *adev)
>> +static int cik_asic_reset(struct amdgpu_device *adev, bool
>> +*vramlost)
>> {
>> int r;
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c
>> b/drivers/gpu/drm/amd/amdgpu/nv.c index a3d99f2..53de7a6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
>> @@ -301,7 +301,7 @@ nv_asic_reset_method(struct amdgpu_device *adev)
>> return AMD_RESET_METHOD_MODE1;
>> }
>>
>> -static int nv_asic_reset(struct amdgpu_device *adev)
>> +static int nv_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>> {
>>
>> /* FIXME: it doesn't work since vega10 */ @@ -315,10 +315,14 @@
>> static int nv_asic_reset(struct amdgpu_device *adev)
>> int ret = 0;
>> struct smu_context *smu = &adev->smu;
>>
>> - if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)
>> + if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO) {
>> + if (vramlost)
>> + *vramlost = true;
>> ret = smu_baco_reset(smu);
>> - else
>> + }
>> + else {
>> ret = nv_asic_mode1_reset(adev);
>> + }
>>
>> return ret;
>> }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/si.c
>> b/drivers/gpu/drm/amd/amdgpu/si.c index 9043614..f324099 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/si.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/si.c
>> @@ -1180,7 +1180,7 @@ static bool si_read_bios_from_rom(struct amdgpu_device *adev,
>> }
>>
>> //xxx: not implemented
>> -static int si_asic_reset(struct amdgpu_device *adev)
>> +static int si_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>> {
>> return 0;
>> }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
>> b/drivers/gpu/drm/amd/amdgpu/soc15.c
>> index fe2212df..12b2966 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
>> @@ -553,10 +553,12 @@ soc15_asic_reset_method(struct amdgpu_device *adev)
>> return AMD_RESET_METHOD_MODE1;
>> }
>>
>> -static int soc15_asic_reset(struct amdgpu_device *adev)
>> +static int soc15_asic_reset(struct amdgpu_device *adev, bool
>> +*vramlost)
>> {
>> switch (soc15_asic_reset_method(adev)) {
>> case AMD_RESET_METHOD_BACO:
>> + if (vramlost)
>> + *vramlost = true;
>> return soc15_asic_baco_reset(adev);
>> case AMD_RESET_METHOD_MODE2:
>> return soc15_mode2_reset(adev); diff --git
>> a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
>> index 56c882b..8eceb00 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>> @@ -696,7 +696,7 @@ static int vi_gpu_pci_config_reset(struct amdgpu_device *adev)
>> * to reset them.
>> * Returns 0 for success.
>> */
>> -static int vi_asic_reset(struct amdgpu_device *adev)
>> +static int vi_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>> {
>> int r;
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
[not found] ` <MN2PR12MB393324F969E3F29F76A0DA8384A40-rweVpJHSKTq/67K4VYF1uAdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
@ 2019-08-23 12:47 ` Koenig, Christian
[not found] ` <3920ef0e-857a-44ff-085e-e121c39e0420-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Koenig, Christian @ 2019-08-23 12:47 UTC (permalink / raw)
To: Liu, Monk,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Am 23.08.19 um 10:57 schrieb Liu, Monk:
>>> vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>> if (vram_lost) {
>>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>>> atomic_inc(&tmp_adev->vram_lost_counter);
> Above is the original logic, if we increment the counter in BACO reset routine, we would potentially
> Have another counter increasement if by coincidence the "amdgpu_device_check_vram_lost" successfully detected the vram lost (although right now it didn't ..)
Yeah, but would increment it twice be a problem? I don't think so.
> Do you mean we remove the amdgpu_device_check_vram_lost(tmp_adev) in device_recovery() routine ?
Please no, that thing certainly proved to be useful. Maybe we could
investigate why it failed to auto detect the lost VRAM.
Christian.
> _____________________________________
> Monk Liu|GPU Virtualization Team |AMD
>
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, August 23, 2019 4:34 PM
> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
>
> I thought in the BACO reset function.
>
> The top level reset function doesn't do much more than increment the vram_lost_counter either.
>
> Christian.
>
> Am 23.08.19 um 10:32 schrieb Liu, Monk:
>>>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
>> In where ? if you mean in amdgpu_device_recover routine I won't do that since the reset() can do any kind of reset like:
>> 1) PF FLR
>> 2) mode1/2 reset
>> 3) magic reset through config space
>> 4) BACO reset
>>
>> PF FLR won't cause VRAM lost, mode_1/2 is not clear to me, only BACO
>> reset is definitely a vram lost reset
>>
>> If you increase the counter in general function that will be not
>> accurate _____________________________________
>> Monk Liu|GPU Virtualization Team |AMD
>>
>>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Friday, August 23, 2019 4:27 PM
>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for
>> reset function
>>
>> Am 23.08.19 um 05:34 schrieb Monk Liu:
>>> for SOC15/vega10 the BACO reset would introduce vram lost in the high
>>> end address range and current kmd's vram lost checking cannot catch
>>> it since it only check visible frame buffer
>>>
>>> TODO:
>>> to confirm if mode1/2 reset would introduce vram lost
>> Looks good in general, but I would make the value mandatory or maybe use a special return code instead.
>>
>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
>>
>> Christian.
>>
>>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++--
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++-----
>>> drivers/gpu/drm/amd/amdgpu/cik.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/nv.c | 10 +++++++---
>>> drivers/gpu/drm/amd/amdgpu/si.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/soc15.c | 4 +++-
>>> drivers/gpu/drm/amd/amdgpu/vi.c | 2 +-
>>> 7 files changed, 22 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index f6ae565..1fe3756 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -552,7 +552,7 @@ struct amdgpu_asic_funcs {
>>> int (*read_register)(struct amdgpu_device *adev, u32 se_num,
>>> u32 sh_num, u32 reg_offset, u32 *value);
>>> void (*set_vga_state)(struct amdgpu_device *adev, bool state);
>>> - int (*reset)(struct amdgpu_device *adev);
>>> + int (*reset)(struct amdgpu_device *adev, bool *lost);
>>> enum amd_reset_method (*reset_method)(struct amdgpu_device *adev);
>>> /* get the reference clock */
>>> u32 (*get_xclk)(struct amdgpu_device *adev); @@ -1136,7 +1136,7
>>> @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>>> * ASICs macro.
>>> */
>>> #define amdgpu_asic_set_vga_state(adev, state)
>>> (adev)->asic_funcs->set_vga_state((adev), (state)) -#define
>>> amdgpu_asic_reset(adev) (adev)->asic_funcs->reset((adev))
>>> +#define amdgpu_asic_reset(adev, lost)
>>> +(adev)->asic_funcs->reset((adev), (lost))
>>> #define amdgpu_asic_reset_method(adev) (adev)->asic_funcs->reset_method((adev))
>>> #define amdgpu_asic_get_xclk(adev) (adev)->asic_funcs->get_xclk((adev))
>>> #define amdgpu_asic_set_uvd_clocks(adev, v, d)
>>> (adev)->asic_funcs->set_uvd_clocks((adev), (v), (d)) diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 02b3e7d..8668cb8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -2546,7 +2546,7 @@ static void amdgpu_device_xgmi_reset_func(struct work_struct *__work)
>>> struct amdgpu_device *adev =
>>> container_of(__work, struct amdgpu_device, xgmi_reset_work);
>>>
>>> - adev->asic_reset_res = amdgpu_asic_reset(adev);
>>> + adev->asic_reset_res = amdgpu_asic_reset(adev, NULL);
>>> if (adev->asic_reset_res)
>>> DRM_WARN("ASIC reset failed with error, %d for drm dev, %s",
>>> adev->asic_reset_res, adev->ddev->unique); @@ -2751,7 +2751,7
>>> @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>> * E.g., driver was not cleanly unloaded previously, etc.
>>> */
>>> if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
>>> - r = amdgpu_asic_reset(adev);
>>> + r = amdgpu_asic_reset(adev, NULL);
>>> if (r) {
>>> dev_err(adev->dev, "asic reset on init failed\n");
>>> goto failed;
>>> @@ -3084,7 +3084,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
>>> pci_disable_device(dev->pdev);
>>> pci_set_power_state(dev->pdev, PCI_D3hot);
>>> } else {
>>> - r = amdgpu_asic_reset(adev);
>>> + r = amdgpu_asic_reset(adev, NULL);
>>> if (r)
>>> DRM_ERROR("amdgpu asic reset failed\n");
>>> }
>>> @@ -3604,7 +3604,7 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>> if (!queue_work(system_highpri_wq, &tmp_adev->xgmi_reset_work))
>>> r = -EALREADY;
>>> } else
>>> - r = amdgpu_asic_reset(tmp_adev);
>>> + r = amdgpu_asic_reset(tmp_adev, &vram_lost);
>>>
>>> if (r) {
>>> DRM_ERROR("ASIC reset failed with error, %d for drm dev, %s",
>>> @@
>>> -3645,7 +3645,9 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>> if (r)
>>> goto out;
>>>
>>> - vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>> + if (!vram_lost)
>>> + vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>> +
>>> if (vram_lost) {
>>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>>> atomic_inc(&tmp_adev->vram_lost_counter);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
>>> b/drivers/gpu/drm/amd/amdgpu/cik.c
>>> index 7b63d7a..0f25b82 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
>>> @@ -1277,7 +1277,7 @@ static int cik_gpu_pci_config_reset(struct amdgpu_device *adev)
>>> * to reset them.
>>> * Returns 0 for success.
>>> */
>>> -static int cik_asic_reset(struct amdgpu_device *adev)
>>> +static int cik_asic_reset(struct amdgpu_device *adev, bool
>>> +*vramlost)
>>> {
>>> int r;
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c
>>> b/drivers/gpu/drm/amd/amdgpu/nv.c index a3d99f2..53de7a6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
>>> @@ -301,7 +301,7 @@ nv_asic_reset_method(struct amdgpu_device *adev)
>>> return AMD_RESET_METHOD_MODE1;
>>> }
>>>
>>> -static int nv_asic_reset(struct amdgpu_device *adev)
>>> +static int nv_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>>> {
>>>
>>> /* FIXME: it doesn't work since vega10 */ @@ -315,10 +315,14 @@
>>> static int nv_asic_reset(struct amdgpu_device *adev)
>>> int ret = 0;
>>> struct smu_context *smu = &adev->smu;
>>>
>>> - if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)
>>> + if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO) {
>>> + if (vramlost)
>>> + *vramlost = true;
>>> ret = smu_baco_reset(smu);
>>> - else
>>> + }
>>> + else {
>>> ret = nv_asic_mode1_reset(adev);
>>> + }
>>>
>>> return ret;
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/si.c
>>> b/drivers/gpu/drm/amd/amdgpu/si.c index 9043614..f324099 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/si.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/si.c
>>> @@ -1180,7 +1180,7 @@ static bool si_read_bios_from_rom(struct amdgpu_device *adev,
>>> }
>>>
>>> //xxx: not implemented
>>> -static int si_asic_reset(struct amdgpu_device *adev)
>>> +static int si_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>>> {
>>> return 0;
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> index fe2212df..12b2966 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> @@ -553,10 +553,12 @@ soc15_asic_reset_method(struct amdgpu_device *adev)
>>> return AMD_RESET_METHOD_MODE1;
>>> }
>>>
>>> -static int soc15_asic_reset(struct amdgpu_device *adev)
>>> +static int soc15_asic_reset(struct amdgpu_device *adev, bool
>>> +*vramlost)
>>> {
>>> switch (soc15_asic_reset_method(adev)) {
>>> case AMD_RESET_METHOD_BACO:
>>> + if (vramlost)
>>> + *vramlost = true;
>>> return soc15_asic_baco_reset(adev);
>>> case AMD_RESET_METHOD_MODE2:
>>> return soc15_mode2_reset(adev); diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
>>> index 56c882b..8eceb00 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>>> @@ -696,7 +696,7 @@ static int vi_gpu_pci_config_reset(struct amdgpu_device *adev)
>>> * to reset them.
>>> * Returns 0 for success.
>>> */
>>> -static int vi_asic_reset(struct amdgpu_device *adev)
>>> +static int vi_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>>> {
>>> int r;
>>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
[not found] ` <3920ef0e-857a-44ff-085e-e121c39e0420-5C7GfCeVMHo@public.gmane.org>
@ 2019-08-23 14:34 ` Deucher, Alexander
[not found] ` <BN6PR12MB1809D6CB333C3A473C71F2FCF7A40-/b2+HYfkarSEx6ez0IUAagdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2019-08-23 14:35 ` Liu, Monk
1 sibling, 1 reply; 10+ messages in thread
From: Deucher, Alexander @ 2019-08-23 14:34 UTC (permalink / raw)
To: Koenig, Christian, Liu, Monk,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
[-- Attachment #1.1: Type: text/plain, Size: 12478 bytes --]
for mode1 and BACO, I think we can assume vram is lost because the UMC gets reset in that case. Some of the data may still look valid, but it's not necessarily reliable. For mode2, vram should be fine because the UMC doesn't get reset.
Alex
________________________________
From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Koenig, Christian <Christian.Koenig-5C7GfCeVMHo@public.gmane.org>
Sent: Friday, August 23, 2019 8:47 AM
To: Liu, Monk <Monk.Liu-5C7GfCeVMHo@public.gmane.org>; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
Am 23.08.19 um 10:57 schrieb Liu, Monk:
>>> vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>> if (vram_lost) {
>>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>>> atomic_inc(&tmp_adev->vram_lost_counter);
> Above is the original logic, if we increment the counter in BACO reset routine, we would potentially
> Have another counter increasement if by coincidence the "amdgpu_device_check_vram_lost" successfully detected the vram lost (although right now it didn't ..)
Yeah, but would increment it twice be a problem? I don't think so.
> Do you mean we remove the amdgpu_device_check_vram_lost(tmp_adev) in device_recovery() routine ?
Please no, that thing certainly proved to be useful. Maybe we could
investigate why it failed to auto detect the lost VRAM.
Christian.
> _____________________________________
> Monk Liu|GPU Virtualization Team |AMD
>
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig-5C7GfCeVMHo@public.gmane.org>
> Sent: Friday, August 23, 2019 4:34 PM
> To: Liu, Monk <Monk.Liu-5C7GfCeVMHo@public.gmane.org>; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
>
> I thought in the BACO reset function.
>
> The top level reset function doesn't do much more than increment the vram_lost_counter either.
>
> Christian.
>
> Am 23.08.19 um 10:32 schrieb Liu, Monk:
>>>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
>> In where ? if you mean in amdgpu_device_recover routine I won't do that since the reset() can do any kind of reset like:
>> 1) PF FLR
>> 2) mode1/2 reset
>> 3) magic reset through config space
>> 4) BACO reset
>>
>> PF FLR won't cause VRAM lost, mode_1/2 is not clear to me, only BACO
>> reset is definitely a vram lost reset
>>
>> If you increase the counter in general function that will be not
>> accurate _____________________________________
>> Monk Liu|GPU Virtualization Team |AMD
>>
>>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Sent: Friday, August 23, 2019 4:27 PM
>> To: Liu, Monk <Monk.Liu-5C7GfCeVMHo@public.gmane.org>; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for
>> reset function
>>
>> Am 23.08.19 um 05:34 schrieb Monk Liu:
>>> for SOC15/vega10 the BACO reset would introduce vram lost in the high
>>> end address range and current kmd's vram lost checking cannot catch
>>> it since it only check visible frame buffer
>>>
>>> TODO:
>>> to confirm if mode1/2 reset would introduce vram lost
>> Looks good in general, but I would make the value mandatory or maybe use a special return code instead.
>>
>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
>>
>> Christian.
>>
>>> Signed-off-by: Monk Liu <Monk.Liu-5C7GfCeVMHo@public.gmane.org>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++--
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++-----
>>> drivers/gpu/drm/amd/amdgpu/cik.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/nv.c | 10 +++++++---
>>> drivers/gpu/drm/amd/amdgpu/si.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/soc15.c | 4 +++-
>>> drivers/gpu/drm/amd/amdgpu/vi.c | 2 +-
>>> 7 files changed, 22 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index f6ae565..1fe3756 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -552,7 +552,7 @@ struct amdgpu_asic_funcs {
>>> int (*read_register)(struct amdgpu_device *adev, u32 se_num,
>>> u32 sh_num, u32 reg_offset, u32 *value);
>>> void (*set_vga_state)(struct amdgpu_device *adev, bool state);
>>> - int (*reset)(struct amdgpu_device *adev);
>>> + int (*reset)(struct amdgpu_device *adev, bool *lost);
>>> enum amd_reset_method (*reset_method)(struct amdgpu_device *adev);
>>> /* get the reference clock */
>>> u32 (*get_xclk)(struct amdgpu_device *adev); @@ -1136,7 +1136,7
>>> @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>>> * ASICs macro.
>>> */
>>> #define amdgpu_asic_set_vga_state(adev, state)
>>> (adev)->asic_funcs->set_vga_state((adev), (state)) -#define
>>> amdgpu_asic_reset(adev) (adev)->asic_funcs->reset((adev))
>>> +#define amdgpu_asic_reset(adev, lost)
>>> +(adev)->asic_funcs->reset((adev), (lost))
>>> #define amdgpu_asic_reset_method(adev) (adev)->asic_funcs->reset_method((adev))
>>> #define amdgpu_asic_get_xclk(adev) (adev)->asic_funcs->get_xclk((adev))
>>> #define amdgpu_asic_set_uvd_clocks(adev, v, d)
>>> (adev)->asic_funcs->set_uvd_clocks((adev), (v), (d)) diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 02b3e7d..8668cb8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -2546,7 +2546,7 @@ static void amdgpu_device_xgmi_reset_func(struct work_struct *__work)
>>> struct amdgpu_device *adev =
>>> container_of(__work, struct amdgpu_device, xgmi_reset_work);
>>>
>>> - adev->asic_reset_res = amdgpu_asic_reset(adev);
>>> + adev->asic_reset_res = amdgpu_asic_reset(adev, NULL);
>>> if (adev->asic_reset_res)
>>> DRM_WARN("ASIC reset failed with error, %d for drm dev, %s",
>>> adev->asic_reset_res, adev->ddev->unique); @@ -2751,7 +2751,7
>>> @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>> * E.g., driver was not cleanly unloaded previously, etc.
>>> */
>>> if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
>>> - r = amdgpu_asic_reset(adev);
>>> + r = amdgpu_asic_reset(adev, NULL);
>>> if (r) {
>>> dev_err(adev->dev, "asic reset on init failed\n");
>>> goto failed;
>>> @@ -3084,7 +3084,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
>>> pci_disable_device(dev->pdev);
>>> pci_set_power_state(dev->pdev, PCI_D3hot);
>>> } else {
>>> - r = amdgpu_asic_reset(adev);
>>> + r = amdgpu_asic_reset(adev, NULL);
>>> if (r)
>>> DRM_ERROR("amdgpu asic reset failed\n");
>>> }
>>> @@ -3604,7 +3604,7 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>> if (!queue_work(system_highpri_wq, &tmp_adev->xgmi_reset_work))
>>> r = -EALREADY;
>>> } else
>>> - r = amdgpu_asic_reset(tmp_adev);
>>> + r = amdgpu_asic_reset(tmp_adev, &vram_lost);
>>>
>>> if (r) {
>>> DRM_ERROR("ASIC reset failed with error, %d for drm dev, %s",
>>> @@
>>> -3645,7 +3645,9 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>> if (r)
>>> goto out;
>>>
>>> - vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>> + if (!vram_lost)
>>> + vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>> +
>>> if (vram_lost) {
>>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>>> atomic_inc(&tmp_adev->vram_lost_counter);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
>>> b/drivers/gpu/drm/amd/amdgpu/cik.c
>>> index 7b63d7a..0f25b82 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
>>> @@ -1277,7 +1277,7 @@ static int cik_gpu_pci_config_reset(struct amdgpu_device *adev)
>>> * to reset them.
>>> * Returns 0 for success.
>>> */
>>> -static int cik_asic_reset(struct amdgpu_device *adev)
>>> +static int cik_asic_reset(struct amdgpu_device *adev, bool
>>> +*vramlost)
>>> {
>>> int r;
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c
>>> b/drivers/gpu/drm/amd/amdgpu/nv.c index a3d99f2..53de7a6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
>>> @@ -301,7 +301,7 @@ nv_asic_reset_method(struct amdgpu_device *adev)
>>> return AMD_RESET_METHOD_MODE1;
>>> }
>>>
>>> -static int nv_asic_reset(struct amdgpu_device *adev)
>>> +static int nv_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>>> {
>>>
>>> /* FIXME: it doesn't work since vega10 */ @@ -315,10 +315,14 @@
>>> static int nv_asic_reset(struct amdgpu_device *adev)
>>> int ret = 0;
>>> struct smu_context *smu = &adev->smu;
>>>
>>> - if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)
>>> + if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO) {
>>> + if (vramlost)
>>> + *vramlost = true;
>>> ret = smu_baco_reset(smu);
>>> - else
>>> + }
>>> + else {
>>> ret = nv_asic_mode1_reset(adev);
>>> + }
>>>
>>> return ret;
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/si.c
>>> b/drivers/gpu/drm/amd/amdgpu/si.c index 9043614..f324099 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/si.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/si.c
>>> @@ -1180,7 +1180,7 @@ static bool si_read_bios_from_rom(struct amdgpu_device *adev,
>>> }
>>>
>>> //xxx: not implemented
>>> -static int si_asic_reset(struct amdgpu_device *adev)
>>> +static int si_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>>> {
>>> return 0;
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> index fe2212df..12b2966 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> @@ -553,10 +553,12 @@ soc15_asic_reset_method(struct amdgpu_device *adev)
>>> return AMD_RESET_METHOD_MODE1;
>>> }
>>>
>>> -static int soc15_asic_reset(struct amdgpu_device *adev)
>>> +static int soc15_asic_reset(struct amdgpu_device *adev, bool
>>> +*vramlost)
>>> {
>>> switch (soc15_asic_reset_method(adev)) {
>>> case AMD_RESET_METHOD_BACO:
>>> + if (vramlost)
>>> + *vramlost = true;
>>> return soc15_asic_baco_reset(adev);
>>> case AMD_RESET_METHOD_MODE2:
>>> return soc15_mode2_reset(adev); diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
>>> index 56c882b..8eceb00 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>>> @@ -696,7 +696,7 @@ static int vi_gpu_pci_config_reset(struct amdgpu_device *adev)
>>> * to reset them.
>>> * Returns 0 for success.
>>> */
>>> -static int vi_asic_reset(struct amdgpu_device *adev)
>>> +static int vi_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>>> {
>>> int r;
>>>
_______________________________________________
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: 23871 bytes --]
[-- Attachment #2: Type: text/plain, Size: 153 bytes --]
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
[not found] ` <3920ef0e-857a-44ff-085e-e121c39e0420-5C7GfCeVMHo@public.gmane.org>
2019-08-23 14:34 ` Deucher, Alexander
@ 2019-08-23 14:35 ` Liu, Monk
[not found] ` <MN2PR12MB3933AC07ADDAEE39DD64608A84A40-rweVpJHSKTq/67K4VYF1uAdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
1 sibling, 1 reply; 10+ messages in thread
From: Liu, Monk @ 2019-08-23 14:35 UTC (permalink / raw)
To: Koenig, Christian,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> Please no, that thing certainly proved to be useful. Maybe we could investigate why it failed to auto detect the lost VRAM.
The reason is with BACO reset I found VRAM lost in high address e.g. 15~16 G (for 16 G vega10), amdgpu_device_check_vram_lost only checks the very ahead visible part
>> Yeah, but would increment it twice be a problem? I don't think so.
So your suggestion is we increase the counter in BACO reset , and no need to introduce the new "bool *" parameter, right ?
_____________________________________
Monk Liu|GPU Virtualization Team |AMD
-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Friday, August 23, 2019 8:47 PM
To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
Am 23.08.19 um 10:57 schrieb Liu, Monk:
>>> vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>> if (vram_lost) {
>>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>>> atomic_inc(&tmp_adev->vram_lost_counter);
> Above is the original logic, if we increment the counter in BACO reset
> routine, we would potentially Have another counter increasement if by
> coincidence the "amdgpu_device_check_vram_lost" successfully detected
> the vram lost (although right now it didn't ..)
Yeah, but would increment it twice be a problem? I don't think so.
> Do you mean we remove the amdgpu_device_check_vram_lost(tmp_adev) in device_recovery() routine ?
Please no, that thing certainly proved to be useful. Maybe we could investigate why it failed to auto detect the lost VRAM.
Christian.
> _____________________________________
> Monk Liu|GPU Virtualization Team |AMD
>
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, August 23, 2019 4:34 PM
> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for
> reset function
>
> I thought in the BACO reset function.
>
> The top level reset function doesn't do much more than increment the vram_lost_counter either.
>
> Christian.
>
> Am 23.08.19 um 10:32 schrieb Liu, Monk:
>>>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
>> In where ? if you mean in amdgpu_device_recover routine I won't do that since the reset() can do any kind of reset like:
>> 1) PF FLR
>> 2) mode1/2 reset
>> 3) magic reset through config space
>> 4) BACO reset
>>
>> PF FLR won't cause VRAM lost, mode_1/2 is not clear to me, only BACO
>> reset is definitely a vram lost reset
>>
>> If you increase the counter in general function that will be not
>> accurate _____________________________________
>> Monk Liu|GPU Virtualization Team |AMD
>>
>>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Friday, August 23, 2019 4:27 PM
>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for
>> reset function
>>
>> Am 23.08.19 um 05:34 schrieb Monk Liu:
>>> for SOC15/vega10 the BACO reset would introduce vram lost in the
>>> high end address range and current kmd's vram lost checking cannot
>>> catch it since it only check visible frame buffer
>>>
>>> TODO:
>>> to confirm if mode1/2 reset would introduce vram lost
>> Looks good in general, but I would make the value mandatory or maybe use a special return code instead.
>>
>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
>>
>> Christian.
>>
>>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++--
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++-----
>>> drivers/gpu/drm/amd/amdgpu/cik.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/nv.c | 10 +++++++---
>>> drivers/gpu/drm/amd/amdgpu/si.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/soc15.c | 4 +++-
>>> drivers/gpu/drm/amd/amdgpu/vi.c | 2 +-
>>> 7 files changed, 22 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index f6ae565..1fe3756 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -552,7 +552,7 @@ struct amdgpu_asic_funcs {
>>> int (*read_register)(struct amdgpu_device *adev, u32 se_num,
>>> u32 sh_num, u32 reg_offset, u32 *value);
>>> void (*set_vga_state)(struct amdgpu_device *adev, bool state);
>>> - int (*reset)(struct amdgpu_device *adev);
>>> + int (*reset)(struct amdgpu_device *adev, bool *lost);
>>> enum amd_reset_method (*reset_method)(struct amdgpu_device *adev);
>>> /* get the reference clock */
>>> u32 (*get_xclk)(struct amdgpu_device *adev); @@ -1136,7 +1136,7
>>> @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>>> * ASICs macro.
>>> */
>>> #define amdgpu_asic_set_vga_state(adev, state)
>>> (adev)->asic_funcs->set_vga_state((adev), (state)) -#define
>>> amdgpu_asic_reset(adev) (adev)->asic_funcs->reset((adev))
>>> +#define amdgpu_asic_reset(adev, lost)
>>> +(adev)->asic_funcs->reset((adev), (lost))
>>> #define amdgpu_asic_reset_method(adev) (adev)->asic_funcs->reset_method((adev))
>>> #define amdgpu_asic_get_xclk(adev) (adev)->asic_funcs->get_xclk((adev))
>>> #define amdgpu_asic_set_uvd_clocks(adev, v, d)
>>> (adev)->asic_funcs->set_uvd_clocks((adev), (v), (d)) diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 02b3e7d..8668cb8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -2546,7 +2546,7 @@ static void amdgpu_device_xgmi_reset_func(struct work_struct *__work)
>>> struct amdgpu_device *adev =
>>> container_of(__work, struct amdgpu_device, xgmi_reset_work);
>>>
>>> - adev->asic_reset_res = amdgpu_asic_reset(adev);
>>> + adev->asic_reset_res = amdgpu_asic_reset(adev, NULL);
>>> if (adev->asic_reset_res)
>>> DRM_WARN("ASIC reset failed with error, %d for drm dev, %s",
>>> adev->asic_reset_res, adev->ddev->unique); @@ -2751,7
>>> +2751,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>> * E.g., driver was not cleanly unloaded previously, etc.
>>> */
>>> if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
>>> - r = amdgpu_asic_reset(adev);
>>> + r = amdgpu_asic_reset(adev, NULL);
>>> if (r) {
>>> dev_err(adev->dev, "asic reset on init failed\n");
>>> goto failed;
>>> @@ -3084,7 +3084,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
>>> pci_disable_device(dev->pdev);
>>> pci_set_power_state(dev->pdev, PCI_D3hot);
>>> } else {
>>> - r = amdgpu_asic_reset(adev);
>>> + r = amdgpu_asic_reset(adev, NULL);
>>> if (r)
>>> DRM_ERROR("amdgpu asic reset failed\n");
>>> }
>>> @@ -3604,7 +3604,7 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>> if (!queue_work(system_highpri_wq, &tmp_adev->xgmi_reset_work))
>>> r = -EALREADY;
>>> } else
>>> - r = amdgpu_asic_reset(tmp_adev);
>>> + r = amdgpu_asic_reset(tmp_adev, &vram_lost);
>>>
>>> if (r) {
>>> DRM_ERROR("ASIC reset failed with error, %d for drm dev,
>>> %s", @@
>>> -3645,7 +3645,9 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>> if (r)
>>> goto out;
>>>
>>> - vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>> + if (!vram_lost)
>>> + vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>> +
>>> if (vram_lost) {
>>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>>> atomic_inc(&tmp_adev->vram_lost_counter);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
>>> b/drivers/gpu/drm/amd/amdgpu/cik.c
>>> index 7b63d7a..0f25b82 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
>>> @@ -1277,7 +1277,7 @@ static int cik_gpu_pci_config_reset(struct amdgpu_device *adev)
>>> * to reset them.
>>> * Returns 0 for success.
>>> */
>>> -static int cik_asic_reset(struct amdgpu_device *adev)
>>> +static int cik_asic_reset(struct amdgpu_device *adev, bool
>>> +*vramlost)
>>> {
>>> int r;
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c
>>> b/drivers/gpu/drm/amd/amdgpu/nv.c index a3d99f2..53de7a6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
>>> @@ -301,7 +301,7 @@ nv_asic_reset_method(struct amdgpu_device *adev)
>>> return AMD_RESET_METHOD_MODE1;
>>> }
>>>
>>> -static int nv_asic_reset(struct amdgpu_device *adev)
>>> +static int nv_asic_reset(struct amdgpu_device *adev, bool
>>> +*vramlost)
>>> {
>>>
>>> /* FIXME: it doesn't work since vega10 */ @@ -315,10 +315,14 @@
>>> static int nv_asic_reset(struct amdgpu_device *adev)
>>> int ret = 0;
>>> struct smu_context *smu = &adev->smu;
>>>
>>> - if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)
>>> + if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO) {
>>> + if (vramlost)
>>> + *vramlost = true;
>>> ret = smu_baco_reset(smu);
>>> - else
>>> + }
>>> + else {
>>> ret = nv_asic_mode1_reset(adev);
>>> + }
>>>
>>> return ret;
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/si.c
>>> b/drivers/gpu/drm/amd/amdgpu/si.c index 9043614..f324099 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/si.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/si.c
>>> @@ -1180,7 +1180,7 @@ static bool si_read_bios_from_rom(struct amdgpu_device *adev,
>>> }
>>>
>>> //xxx: not implemented
>>> -static int si_asic_reset(struct amdgpu_device *adev)
>>> +static int si_asic_reset(struct amdgpu_device *adev, bool
>>> +*vramlost)
>>> {
>>> return 0;
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> index fe2212df..12b2966 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> @@ -553,10 +553,12 @@ soc15_asic_reset_method(struct amdgpu_device *adev)
>>> return AMD_RESET_METHOD_MODE1;
>>> }
>>>
>>> -static int soc15_asic_reset(struct amdgpu_device *adev)
>>> +static int soc15_asic_reset(struct amdgpu_device *adev, bool
>>> +*vramlost)
>>> {
>>> switch (soc15_asic_reset_method(adev)) {
>>> case AMD_RESET_METHOD_BACO:
>>> + if (vramlost)
>>> + *vramlost = true;
>>> return soc15_asic_baco_reset(adev);
>>> case AMD_RESET_METHOD_MODE2:
>>> return soc15_mode2_reset(adev); diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
>>> index 56c882b..8eceb00 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>>> @@ -696,7 +696,7 @@ static int vi_gpu_pci_config_reset(struct amdgpu_device *adev)
>>> * to reset them.
>>> * Returns 0 for success.
>>> */
>>> -static int vi_asic_reset(struct amdgpu_device *adev)
>>> +static int vi_asic_reset(struct amdgpu_device *adev, bool
>>> +*vramlost)
>>> {
>>> int r;
>>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
[not found] ` <BN6PR12MB1809D6CB333C3A473C71F2FCF7A40-/b2+HYfkarSEx6ez0IUAagdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
@ 2019-08-23 14:36 ` Liu, Monk
0 siblings, 0 replies; 10+ messages in thread
From: Liu, Monk @ 2019-08-23 14:36 UTC (permalink / raw)
To: Deucher, Alexander, Koenig, Christian,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
[-- Attachment #1.1.1: Type: text/plain, Size: 13686 bytes --]
Thanks Alex
That sounds correct to me, mode1 rest once do clear the vram data on vega10
_____________________________________
Monk Liu|GPU Virtualization Team |AMD
[sig-cloud-gpu]
From: Deucher, Alexander <Alexander.Deucher-5C7GfCeVMHo@public.gmane.org>
Sent: Friday, August 23, 2019 10:34 PM
To: Koenig, Christian <Christian.Koenig-5C7GfCeVMHo@public.gmane.org>; Liu, Monk <Monk.Liu@amd.com>; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
for mode1 and BACO, I think we can assume vram is lost because the UMC gets reset in that case. Some of the data may still look valid, but it's not necessarily reliable. For mode2, vram should be fine because the UMC doesn't get reset.
Alex
________________________________
From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-bounces@lists.freedesktop.org>> on behalf of Koenig, Christian <Christian.Koenig@amd.com<mailto:Christian.Koenig-5C7GfCeVMHo@public.gmane.org>>
Sent: Friday, August 23, 2019 8:47 AM
To: Liu, Monk <Monk.Liu-5C7GfCeVMHo@public.gmane.org<mailto:Monk.Liu-5C7GfCeVMHo@public.gmane.org>>; amd-gfx-PD4FTy7X32k@public.gmane.orgeedesktop.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> <amd-gfx-PD4FTy7X32lzq7FAFJ/aeQ@public.gmane.orgtop.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>>
Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
Am 23.08.19 um 10:57 schrieb Liu, Monk:
>>> vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>> if (vram_lost) {
>>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>>> atomic_inc(&tmp_adev->vram_lost_counter);
> Above is the original logic, if we increment the counter in BACO reset routine, we would potentially
> Have another counter increasement if by coincidence the "amdgpu_device_check_vram_lost" successfully detected the vram lost (although right now it didn't ..)
Yeah, but would increment it twice be a problem? I don't think so.
> Do you mean we remove the amdgpu_device_check_vram_lost(tmp_adev) in device_recovery() routine ?
Please no, that thing certainly proved to be useful. Maybe we could
investigate why it failed to auto detect the lost VRAM.
Christian.
> _____________________________________
> Monk Liu|GPU Virtualization Team |AMD
>
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig-5C7GfCeVMHo@public.gmane.org<mailto:Christian.Koenig@amd.com>>
> Sent: Friday, August 23, 2019 4:34 PM
> To: Liu, Monk <Monk.Liu-5C7GfCeVMHo@public.gmane.org<mailto:Monk.Liu-5C7GfCeVMHo@public.gmane.org>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
>
> I thought in the BACO reset function.
>
> The top level reset function doesn't do much more than increment the vram_lost_counter either.
>
> Christian.
>
> Am 23.08.19 um 10:32 schrieb Liu, Monk:
>>>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
>> In where ? if you mean in amdgpu_device_recover routine I won't do that since the reset() can do any kind of reset like:
>> 1) PF FLR
>> 2) mode1/2 reset
>> 3) magic reset through config space
>> 4) BACO reset
>>
>> PF FLR won't cause VRAM lost, mode_1/2 is not clear to me, only BACO
>> reset is definitely a vram lost reset
>>
>> If you increase the counter in general function that will be not
>> accurate _____________________________________
>> Monk Liu|GPU Virtualization Team |AMD
>>
>>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org<mailto:ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>>
>> Sent: Friday, August 23, 2019 4:27 PM
>> To: Liu, Monk <Monk.Liu-5C7GfCeVMHo@public.gmane.org<mailto:Monk.Liu-5C7GfCeVMHo@public.gmane.org>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
>> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for
>> reset function
>>
>> Am 23.08.19 um 05:34 schrieb Monk Liu:
>>> for SOC15/vega10 the BACO reset would introduce vram lost in the high
>>> end address range and current kmd's vram lost checking cannot catch
>>> it since it only check visible frame buffer
>>>
>>> TODO:
>>> to confirm if mode1/2 reset would introduce vram lost
>> Looks good in general, but I would make the value mandatory or maybe use a special return code instead.
>>
>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
>>
>> Christian.
>>
>>> Signed-off-by: Monk Liu <Monk.Liu-5C7GfCeVMHo@public.gmane.org<mailto:Monk.Liu-5C7GfCeVMHo@public.gmane.org>>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++--
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++-----
>>> drivers/gpu/drm/amd/amdgpu/cik.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/nv.c | 10 +++++++---
>>> drivers/gpu/drm/amd/amdgpu/si.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/soc15.c | 4 +++-
>>> drivers/gpu/drm/amd/amdgpu/vi.c | 2 +-
>>> 7 files changed, 22 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index f6ae565..1fe3756 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -552,7 +552,7 @@ struct amdgpu_asic_funcs {
>>> int (*read_register)(struct amdgpu_device *adev, u32 se_num,
>>> u32 sh_num, u32 reg_offset, u32 *value);
>>> void (*set_vga_state)(struct amdgpu_device *adev, bool state);
>>> - int (*reset)(struct amdgpu_device *adev);
>>> + int (*reset)(struct amdgpu_device *adev, bool *lost);
>>> enum amd_reset_method (*reset_method)(struct amdgpu_device *adev);
>>> /* get the reference clock */
>>> u32 (*get_xclk)(struct amdgpu_device *adev); @@ -1136,7 +1136,7
>>> @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>>> * ASICs macro.
>>> */
>>> #define amdgpu_asic_set_vga_state(adev, state)
>>> (adev)->asic_funcs->set_vga_state((adev), (state)) -#define
>>> amdgpu_asic_reset(adev) (adev)->asic_funcs->reset((adev))
>>> +#define amdgpu_asic_reset(adev, lost)
>>> +(adev)->asic_funcs->reset((adev), (lost))
>>> #define amdgpu_asic_reset_method(adev) (adev)->asic_funcs->reset_method((adev))
>>> #define amdgpu_asic_get_xclk(adev) (adev)->asic_funcs->get_xclk((adev))
>>> #define amdgpu_asic_set_uvd_clocks(adev, v, d)
>>> (adev)->asic_funcs->set_uvd_clocks((adev), (v), (d)) diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 02b3e7d..8668cb8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -2546,7 +2546,7 @@ static void amdgpu_device_xgmi_reset_func(struct work_struct *__work)
>>> struct amdgpu_device *adev =
>>> container_of(__work, struct amdgpu_device, xgmi_reset_work);
>>>
>>> - adev->asic_reset_res = amdgpu_asic_reset(adev);
>>> + adev->asic_reset_res = amdgpu_asic_reset(adev, NULL);
>>> if (adev->asic_reset_res)
>>> DRM_WARN("ASIC reset failed with error, %d for drm dev, %s",
>>> adev->asic_reset_res, adev->ddev->unique); @@ -2751,7 +2751,7
>>> @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>> * E.g., driver was not cleanly unloaded previously, etc.
>>> */
>>> if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
>>> - r = amdgpu_asic_reset(adev);
>>> + r = amdgpu_asic_reset(adev, NULL);
>>> if (r) {
>>> dev_err(adev->dev, "asic reset on init failed\n");
>>> goto failed;
>>> @@ -3084,7 +3084,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
>>> pci_disable_device(dev->pdev);
>>> pci_set_power_state(dev->pdev, PCI_D3hot);
>>> } else {
>>> - r = amdgpu_asic_reset(adev);
>>> + r = amdgpu_asic_reset(adev, NULL);
>>> if (r)
>>> DRM_ERROR("amdgpu asic reset failed\n");
>>> }
>>> @@ -3604,7 +3604,7 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>> if (!queue_work(system_highpri_wq, &tmp_adev->xgmi_reset_work))
>>> r = -EALREADY;
>>> } else
>>> - r = amdgpu_asic_reset(tmp_adev);
>>> + r = amdgpu_asic_reset(tmp_adev, &vram_lost);
>>>
>>> if (r) {
>>> DRM_ERROR("ASIC reset failed with error, %d for drm dev, %s",
>>> @@
>>> -3645,7 +3645,9 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>> if (r)
>>> goto out;
>>>
>>> - vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>> + if (!vram_lost)
>>> + vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>> +
>>> if (vram_lost) {
>>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>>> atomic_inc(&tmp_adev->vram_lost_counter);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
>>> b/drivers/gpu/drm/amd/amdgpu/cik.c
>>> index 7b63d7a..0f25b82 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
>>> @@ -1277,7 +1277,7 @@ static int cik_gpu_pci_config_reset(struct amdgpu_device *adev)
>>> * to reset them.
>>> * Returns 0 for success.
>>> */
>>> -static int cik_asic_reset(struct amdgpu_device *adev)
>>> +static int cik_asic_reset(struct amdgpu_device *adev, bool
>>> +*vramlost)
>>> {
>>> int r;
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c
>>> b/drivers/gpu/drm/amd/amdgpu/nv.c index a3d99f2..53de7a6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
>>> @@ -301,7 +301,7 @@ nv_asic_reset_method(struct amdgpu_device *adev)
>>> return AMD_RESET_METHOD_MODE1;
>>> }
>>>
>>> -static int nv_asic_reset(struct amdgpu_device *adev)
>>> +static int nv_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>>> {
>>>
>>> /* FIXME: it doesn't work since vega10 */ @@ -315,10 +315,14 @@
>>> static int nv_asic_reset(struct amdgpu_device *adev)
>>> int ret = 0;
>>> struct smu_context *smu = &adev->smu;
>>>
>>> - if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)
>>> + if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO) {
>>> + if (vramlost)
>>> + *vramlost = true;
>>> ret = smu_baco_reset(smu);
>>> - else
>>> + }
>>> + else {
>>> ret = nv_asic_mode1_reset(adev);
>>> + }
>>>
>>> return ret;
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/si.c
>>> b/drivers/gpu/drm/amd/amdgpu/si.c index 9043614..f324099 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/si.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/si.c
>>> @@ -1180,7 +1180,7 @@ static bool si_read_bios_from_rom(struct amdgpu_device *adev,
>>> }
>>>
>>> //xxx: not implemented
>>> -static int si_asic_reset(struct amdgpu_device *adev)
>>> +static int si_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>>> {
>>> return 0;
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> index fe2212df..12b2966 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> @@ -553,10 +553,12 @@ soc15_asic_reset_method(struct amdgpu_device *adev)
>>> return AMD_RESET_METHOD_MODE1;
>>> }
>>>
>>> -static int soc15_asic_reset(struct amdgpu_device *adev)
>>> +static int soc15_asic_reset(struct amdgpu_device *adev, bool
>>> +*vramlost)
>>> {
>>> switch (soc15_asic_reset_method(adev)) {
>>> case AMD_RESET_METHOD_BACO:
>>> + if (vramlost)
>>> + *vramlost = true;
>>> return soc15_asic_baco_reset(adev);
>>> case AMD_RESET_METHOD_MODE2:
>>> return soc15_mode2_reset(adev); diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
>>> index 56c882b..8eceb00 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>>> @@ -696,7 +696,7 @@ static int vi_gpu_pci_config_reset(struct amdgpu_device *adev)
>>> * to reset them.
>>> * Returns 0 for success.
>>> */
>>> -static int vi_asic_reset(struct amdgpu_device *adev)
>>> +static int vi_asic_reset(struct amdgpu_device *adev, bool *vramlost)
>>> {
>>> int r;
>>>
_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[-- Attachment #1.1.2: Type: text/html, Size: 27783 bytes --]
[-- Attachment #1.2: image001.png --]
[-- Type: image/png, Size: 12243 bytes --]
[-- Attachment #2: Type: text/plain, Size: 153 bytes --]
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
[not found] ` <MN2PR12MB3933AC07ADDAEE39DD64608A84A40-rweVpJHSKTq/67K4VYF1uAdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
@ 2019-08-24 11:07 ` Christian König
0 siblings, 0 replies; 10+ messages in thread
From: Christian König @ 2019-08-24 11:07 UTC (permalink / raw)
To: Liu, Monk, Koenig, Christian,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> So your suggestion is we increase the counter in BACO reset , and no need to introduce the new "bool *" parameter, right ?
Correct, yes.
Cleanest way looks like adding some helper function like
amdgpu_device_vram_lost(adev) which is called by the BACO reset code.
Christian.
Am 23.08.19 um 16:35 schrieb Liu, Monk:
>>> Please no, that thing certainly proved to be useful. Maybe we could investigate why it failed to auto detect the lost VRAM.
> The reason is with BACO reset I found VRAM lost in high address e.g. 15~16 G (for 16 G vega10), amdgpu_device_check_vram_lost only checks the very ahead visible part
>
>>> Yeah, but would increment it twice be a problem? I don't think so.
> So your suggestion is we increase the counter in BACO reset , and no need to introduce the new "bool *" parameter, right ?
> _____________________________________
> Monk Liu|GPU Virtualization Team |AMD
>
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, August 23, 2019 8:47 PM
> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for reset function
>
> Am 23.08.19 um 10:57 schrieb Liu, Monk:
>>>> vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>>> if (vram_lost) {
>>>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>>>> atomic_inc(&tmp_adev->vram_lost_counter);
>> Above is the original logic, if we increment the counter in BACO reset
>> routine, we would potentially Have another counter increasement if by
>> coincidence the "amdgpu_device_check_vram_lost" successfully detected
>> the vram lost (although right now it didn't ..)
> Yeah, but would increment it twice be a problem? I don't think so.
>
>> Do you mean we remove the amdgpu_device_check_vram_lost(tmp_adev) in device_recovery() routine ?
> Please no, that thing certainly proved to be useful. Maybe we could investigate why it failed to auto detect the lost VRAM.
>
> Christian.
>
>> _____________________________________
>> Monk Liu|GPU Virtualization Team |AMD
>>
>>
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Friday, August 23, 2019 4:34 PM
>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for
>> reset function
>>
>> I thought in the BACO reset function.
>>
>> The top level reset function doesn't do much more than increment the vram_lost_counter either.
>>
>> Christian.
>>
>> Am 23.08.19 um 10:32 schrieb Liu, Monk:
>>>>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
>>> In where ? if you mean in amdgpu_device_recover routine I won't do that since the reset() can do any kind of reset like:
>>> 1) PF FLR
>>> 2) mode1/2 reset
>>> 3) magic reset through config space
>>> 4) BACO reset
>>>
>>> PF FLR won't cause VRAM lost, mode_1/2 is not clear to me, only BACO
>>> reset is definitely a vram lost reset
>>>
>>> If you increase the counter in general function that will be not
>>> accurate _____________________________________
>>> Monk Liu|GPU Virtualization Team |AMD
>>>
>>>
>>> -----Original Message-----
>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>> Sent: Friday, August 23, 2019 4:27 PM
>>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amdgpu: introduce vram lost paramter for
>>> reset function
>>>
>>> Am 23.08.19 um 05:34 schrieb Monk Liu:
>>>> for SOC15/vega10 the BACO reset would introduce vram lost in the
>>>> high end address range and current kmd's vram lost checking cannot
>>>> catch it since it only check visible frame buffer
>>>>
>>>> TODO:
>>>> to confirm if mode1/2 reset would introduce vram lost
>>> Looks good in general, but I would make the value mandatory or maybe use a special return code instead.
>>>
>>> On the other hand wouldn't it be simpler to just increment vram_lost_counter?
>>>
>>> Christian.
>>>
>>>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++--
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++-----
>>>> drivers/gpu/drm/amd/amdgpu/cik.c | 2 +-
>>>> drivers/gpu/drm/amd/amdgpu/nv.c | 10 +++++++---
>>>> drivers/gpu/drm/amd/amdgpu/si.c | 2 +-
>>>> drivers/gpu/drm/amd/amdgpu/soc15.c | 4 +++-
>>>> drivers/gpu/drm/amd/amdgpu/vi.c | 2 +-
>>>> 7 files changed, 22 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> index f6ae565..1fe3756 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> @@ -552,7 +552,7 @@ struct amdgpu_asic_funcs {
>>>> int (*read_register)(struct amdgpu_device *adev, u32 se_num,
>>>> u32 sh_num, u32 reg_offset, u32 *value);
>>>> void (*set_vga_state)(struct amdgpu_device *adev, bool state);
>>>> - int (*reset)(struct amdgpu_device *adev);
>>>> + int (*reset)(struct amdgpu_device *adev, bool *lost);
>>>> enum amd_reset_method (*reset_method)(struct amdgpu_device *adev);
>>>> /* get the reference clock */
>>>> u32 (*get_xclk)(struct amdgpu_device *adev); @@ -1136,7 +1136,7
>>>> @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>>>> * ASICs macro.
>>>> */
>>>> #define amdgpu_asic_set_vga_state(adev, state)
>>>> (adev)->asic_funcs->set_vga_state((adev), (state)) -#define
>>>> amdgpu_asic_reset(adev) (adev)->asic_funcs->reset((adev))
>>>> +#define amdgpu_asic_reset(adev, lost)
>>>> +(adev)->asic_funcs->reset((adev), (lost))
>>>> #define amdgpu_asic_reset_method(adev) (adev)->asic_funcs->reset_method((adev))
>>>> #define amdgpu_asic_get_xclk(adev) (adev)->asic_funcs->get_xclk((adev))
>>>> #define amdgpu_asic_set_uvd_clocks(adev, v, d)
>>>> (adev)->asic_funcs->set_uvd_clocks((adev), (v), (d)) diff --git
>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> index 02b3e7d..8668cb8 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> @@ -2546,7 +2546,7 @@ static void amdgpu_device_xgmi_reset_func(struct work_struct *__work)
>>>> struct amdgpu_device *adev =
>>>> container_of(__work, struct amdgpu_device, xgmi_reset_work);
>>>>
>>>> - adev->asic_reset_res = amdgpu_asic_reset(adev);
>>>> + adev->asic_reset_res = amdgpu_asic_reset(adev, NULL);
>>>> if (adev->asic_reset_res)
>>>> DRM_WARN("ASIC reset failed with error, %d for drm dev, %s",
>>>> adev->asic_reset_res, adev->ddev->unique); @@ -2751,7
>>>> +2751,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>>> * E.g., driver was not cleanly unloaded previously, etc.
>>>> */
>>>> if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
>>>> - r = amdgpu_asic_reset(adev);
>>>> + r = amdgpu_asic_reset(adev, NULL);
>>>> if (r) {
>>>> dev_err(adev->dev, "asic reset on init failed\n");
>>>> goto failed;
>>>> @@ -3084,7 +3084,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
>>>> pci_disable_device(dev->pdev);
>>>> pci_set_power_state(dev->pdev, PCI_D3hot);
>>>> } else {
>>>> - r = amdgpu_asic_reset(adev);
>>>> + r = amdgpu_asic_reset(adev, NULL);
>>>> if (r)
>>>> DRM_ERROR("amdgpu asic reset failed\n");
>>>> }
>>>> @@ -3604,7 +3604,7 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>>> if (!queue_work(system_highpri_wq, &tmp_adev->xgmi_reset_work))
>>>> r = -EALREADY;
>>>> } else
>>>> - r = amdgpu_asic_reset(tmp_adev);
>>>> + r = amdgpu_asic_reset(tmp_adev, &vram_lost);
>>>>
>>>> if (r) {
>>>> DRM_ERROR("ASIC reset failed with error, %d for drm dev,
>>>> %s", @@
>>>> -3645,7 +3645,9 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>>> if (r)
>>>> goto out;
>>>>
>>>> - vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>>> + if (!vram_lost)
>>>> + vram_lost = amdgpu_device_check_vram_lost(tmp_adev);
>>>> +
>>>> if (vram_lost) {
>>>> DRM_INFO("VRAM is lost due to GPU reset!\n");
>>>> atomic_inc(&tmp_adev->vram_lost_counter);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
>>>> b/drivers/gpu/drm/amd/amdgpu/cik.c
>>>> index 7b63d7a..0f25b82 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
>>>> @@ -1277,7 +1277,7 @@ static int cik_gpu_pci_config_reset(struct amdgpu_device *adev)
>>>> * to reset them.
>>>> * Returns 0 for success.
>>>> */
>>>> -static int cik_asic_reset(struct amdgpu_device *adev)
>>>> +static int cik_asic_reset(struct amdgpu_device *adev, bool
>>>> +*vramlost)
>>>> {
>>>> int r;
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c
>>>> b/drivers/gpu/drm/amd/amdgpu/nv.c index a3d99f2..53de7a6 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
>>>> @@ -301,7 +301,7 @@ nv_asic_reset_method(struct amdgpu_device *adev)
>>>> return AMD_RESET_METHOD_MODE1;
>>>> }
>>>>
>>>> -static int nv_asic_reset(struct amdgpu_device *adev)
>>>> +static int nv_asic_reset(struct amdgpu_device *adev, bool
>>>> +*vramlost)
>>>> {
>>>>
>>>> /* FIXME: it doesn't work since vega10 */ @@ -315,10 +315,14 @@
>>>> static int nv_asic_reset(struct amdgpu_device *adev)
>>>> int ret = 0;
>>>> struct smu_context *smu = &adev->smu;
>>>>
>>>> - if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)
>>>> + if (nv_asic_reset_method(adev) == AMD_RESET_METHOD_BACO) {
>>>> + if (vramlost)
>>>> + *vramlost = true;
>>>> ret = smu_baco_reset(smu);
>>>> - else
>>>> + }
>>>> + else {
>>>> ret = nv_asic_mode1_reset(adev);
>>>> + }
>>>>
>>>> return ret;
>>>> }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/si.c
>>>> b/drivers/gpu/drm/amd/amdgpu/si.c index 9043614..f324099 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/si.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/si.c
>>>> @@ -1180,7 +1180,7 @@ static bool si_read_bios_from_rom(struct amdgpu_device *adev,
>>>> }
>>>>
>>>> //xxx: not implemented
>>>> -static int si_asic_reset(struct amdgpu_device *adev)
>>>> +static int si_asic_reset(struct amdgpu_device *adev, bool
>>>> +*vramlost)
>>>> {
>>>> return 0;
>>>> }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>>> b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>>> index fe2212df..12b2966 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>>> @@ -553,10 +553,12 @@ soc15_asic_reset_method(struct amdgpu_device *adev)
>>>> return AMD_RESET_METHOD_MODE1;
>>>> }
>>>>
>>>> -static int soc15_asic_reset(struct amdgpu_device *adev)
>>>> +static int soc15_asic_reset(struct amdgpu_device *adev, bool
>>>> +*vramlost)
>>>> {
>>>> switch (soc15_asic_reset_method(adev)) {
>>>> case AMD_RESET_METHOD_BACO:
>>>> + if (vramlost)
>>>> + *vramlost = true;
>>>> return soc15_asic_baco_reset(adev);
>>>> case AMD_RESET_METHOD_MODE2:
>>>> return soc15_mode2_reset(adev); diff --git
>>>> a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
>>>> index 56c882b..8eceb00 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>>>> @@ -696,7 +696,7 @@ static int vi_gpu_pci_config_reset(struct amdgpu_device *adev)
>>>> * to reset them.
>>>> * Returns 0 for success.
>>>> */
>>>> -static int vi_asic_reset(struct amdgpu_device *adev)
>>>> +static int vi_asic_reset(struct amdgpu_device *adev, bool
>>>> +*vramlost)
>>>> {
>>>> int r;
>>>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-08-24 11:07 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-23 3:34 [PATCH] drm/amdgpu: introduce vram lost paramter for reset function Monk Liu
[not found] ` <1566531249-1396-1-git-send-email-Monk.Liu-5C7GfCeVMHo@public.gmane.org>
2019-08-23 8:26 ` Christian König
[not found] ` <15d22497-cf4d-0d15-236c-5bc2b65eb656-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2019-08-23 8:32 ` Liu, Monk
[not found] ` <MN2PR12MB393366D270445A63B053DA3184A40-rweVpJHSKTq/67K4VYF1uAdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2019-08-23 8:34 ` Koenig, Christian
[not found] ` <82e98a1b-a624-1e54-aae7-1a6ff9dda7f2-5C7GfCeVMHo@public.gmane.org>
2019-08-23 8:57 ` Liu, Monk
[not found] ` <MN2PR12MB393324F969E3F29F76A0DA8384A40-rweVpJHSKTq/67K4VYF1uAdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2019-08-23 12:47 ` Koenig, Christian
[not found] ` <3920ef0e-857a-44ff-085e-e121c39e0420-5C7GfCeVMHo@public.gmane.org>
2019-08-23 14:34 ` Deucher, Alexander
[not found] ` <BN6PR12MB1809D6CB333C3A473C71F2FCF7A40-/b2+HYfkarSEx6ez0IUAagdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2019-08-23 14:36 ` Liu, Monk
2019-08-23 14:35 ` Liu, Monk
[not found] ` <MN2PR12MB3933AC07ADDAEE39DD64608A84A40-rweVpJHSKTq/67K4VYF1uAdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2019-08-24 11:07 ` Christian König
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox