Make staging driver stable for SRIOV VF (1 v2)

Make staging driver stable for SRIOV VF (1 v2)

[Pixel Ding]

This is the first patch series to make latest staging driver
stable for SRIOV VF on both Tonga and Vega. Patches are merged
from SRIOV branches or reimplemented, including bug fixes and
small features requested by SRIOV users.  

v2: "drm/amdgpu: workaround for VM fault caused by SDMA" is dropped.

Please help reviewing, Thanks.

[PATCH 1/3] drm/amdgpu: always consider virualised device for
[PATCH 2/3] drm/amdgpu: report more amdgpu_fence_info v2
[PATCH 3/3] drm/amdgpu: busywait KIQ register accessing v2
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Pixel Ding]

From: pding <Pixel.Ding@amd.com>

The post checking on scratch registers isn't reliable for virtual
function.

Signed-off-by: pding <Pixel.Ding@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 683965b..ab8f0d6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -669,7 +669,7 @@ void amdgpu_gart_location(struct amdgpu_device *adev, struct amdgpu_mc *mc)
  * or post is needed if  hw reset is performed.
  * Returns true if need or false if not.
  */
-bool amdgpu_need_post(struct amdgpu_device *adev)
+static bool amdgpu_check_post(struct amdgpu_device *adev)
 {
 	uint32_t reg;
 
@@ -692,7 +692,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
 
 }
 
-static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
+bool amdgpu_need_post(struct amdgpu_device *adev)
 {
 	if (amdgpu_sriov_vf(adev))
 		return false;
@@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
 				return true;
 		}
 	}
-	return amdgpu_need_post(adev);
+	return amdgpu_check_post(adev);
 }
 
 /**
@@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	amdgpu_device_detect_sriov_bios(adev);
 
 	/* Post card if necessary */
-	if (amdgpu_vpost_needed(adev)) {
+	if (amdgpu_need_post(adev)) {
 		if (!adev->bios) {
 			dev_err(adev->dev, "no vBIOS found\n");
 			amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
-- 
2.9.5

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

RE: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Liu, Monk]

I don't understand how this patch works??? Looks like just rename vpost_needed to check_post

-----Original Message-----
From: Pixel Ding [mailto:Pixel.Ding@amd.com] 
Sent: 2017年10月17日 14:38
To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

From: pding <Pixel.Ding@amd.com>

The post checking on scratch registers isn't reliable for virtual function.

Signed-off-by: pding <Pixel.Ding@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 683965b..ab8f0d6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -669,7 +669,7 @@ void amdgpu_gart_location(struct amdgpu_device *adev, struct amdgpu_mc *mc)
  * or post is needed if  hw reset is performed.
  * Returns true if need or false if not.
  */
-bool amdgpu_need_post(struct amdgpu_device *adev)
+static bool amdgpu_check_post(struct amdgpu_device *adev)
 {
 	uint32_t reg;
 
@@ -692,7 +692,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
 
 }
 
-static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
+bool amdgpu_need_post(struct amdgpu_device *adev)
 {
 	if (amdgpu_sriov_vf(adev))
 		return false;
@@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
 				return true;
 		}
 	}
-	return amdgpu_need_post(adev);
+	return amdgpu_check_post(adev);
 }
 
 /**
@@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	amdgpu_device_detect_sriov_bios(adev);
 
 	/* Post card if necessary */
-	if (amdgpu_vpost_needed(adev)) {
+	if (amdgpu_need_post(adev)) {
 		if (!adev->bios) {
 			dev_err(adev->dev, "no vBIOS found\n");
 			amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
--
2.9.5

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

Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Ding, Pixel]

It fixes a issue hidden in:

95 static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
 96 {
 97	uint8_t __iomem *bios;
 98	resource_size_t vram_base;
 99	resource_size_t size = 256 * 1024; /* ??? */
100
101	if (!(adev->flags & AMD_IS_APU))
102		if (amdgpu_need_post(adev))
103		return false;


This makes bios reading fallback to SMC INDEX/DATA register case.

— 
Sincerely Yours,
Pixel








On 17/10/2017, 3:48 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:

>I don't understand how this patch works??? Looks like just rename vpost_needed to check_post
>
>-----Original Message-----
>From: Pixel Ding [mailto:Pixel.Ding@amd.com] 
>Sent: 2017年10月17日 14:38
>To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
>Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
>Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>
>From: pding <Pixel.Ding@amd.com>
>
>The post checking on scratch registers isn't reliable for virtual function.
>
>Signed-off-by: pding <Pixel.Ding@amd.com>
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>index 683965b..ab8f0d6 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>@@ -669,7 +669,7 @@ void amdgpu_gart_location(struct amdgpu_device *adev, struct amdgpu_mc *mc)
>  * or post is needed if  hw reset is performed.
>  * Returns true if need or false if not.
>  */
>-bool amdgpu_need_post(struct amdgpu_device *adev)
>+static bool amdgpu_check_post(struct amdgpu_device *adev)
> {
> 	uint32_t reg;
> 
>@@ -692,7 +692,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
> 
> }
> 
>-static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>+bool amdgpu_need_post(struct amdgpu_device *adev)
> {
> 	if (amdgpu_sriov_vf(adev))
> 		return false;
>@@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
> 				return true;
> 		}
> 	}
>-	return amdgpu_need_post(adev);
>+	return amdgpu_check_post(adev);
> }
> 
> /**
>@@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> 	amdgpu_device_detect_sriov_bios(adev);
> 
> 	/* Post card if necessary */
>-	if (amdgpu_vpost_needed(adev)) {
>+	if (amdgpu_need_post(adev)) {
> 		if (!adev->bios) {
> 			dev_err(adev->dev, "no vBIOS found\n");
> 			amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
>--
>2.9.5
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Liu, Monk]

From the patch itself I still couldn't tell the difference 

-----Original Message-----
From: Ding, Pixel 
Sent: 2017年10月17日 15:54
To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>
Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

It fixes a issue hidden in:

95 static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
 96 {
 97	uint8_t __iomem *bios;
 98	resource_size_t vram_base;
 99	resource_size_t size = 256 * 1024; /* ??? */
100
101	if (!(adev->flags & AMD_IS_APU))
102		if (amdgpu_need_post(adev))
103		return false;


This makes bios reading fallback to SMC INDEX/DATA register case.

— 
Sincerely Yours,
Pixel








On 17/10/2017, 3:48 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:

>I don't understand how this patch works??? Looks like just rename vpost_needed to check_post
>
>-----Original Message-----
>From: Pixel Ding [mailto:Pixel.Ding@amd.com] 
>Sent: 2017年10月17日 14:38
>To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
>Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
>Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>
>From: pding <Pixel.Ding@amd.com>
>
>The post checking on scratch registers isn't reliable for virtual function.
>
>Signed-off-by: pding <Pixel.Ding@amd.com>
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>index 683965b..ab8f0d6 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>@@ -669,7 +669,7 @@ void amdgpu_gart_location(struct amdgpu_device *adev, struct amdgpu_mc *mc)
>  * or post is needed if  hw reset is performed.
>  * Returns true if need or false if not.
>  */
>-bool amdgpu_need_post(struct amdgpu_device *adev)
>+static bool amdgpu_check_post(struct amdgpu_device *adev)
> {
> 	uint32_t reg;
> 
>@@ -692,7 +692,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
> 
> }
> 
>-static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>+bool amdgpu_need_post(struct amdgpu_device *adev)
> {
> 	if (amdgpu_sriov_vf(adev))
> 		return false;
>@@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
> 				return true;
> 		}
> 	}
>-	return amdgpu_need_post(adev);
>+	return amdgpu_check_post(adev);
> }
> 
> /**
>@@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> 	amdgpu_device_detect_sriov_bios(adev);
> 
> 	/* Post card if necessary */
>-	if (amdgpu_vpost_needed(adev)) {
>+	if (amdgpu_need_post(adev)) {
> 		if (!adev->bios) {
> 			dev_err(adev->dev, "no vBIOS found\n");
> 			amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
>--
>2.9.5
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Ding, Pixel]

you can see the difference of function amdgpu_need_post. Generally speaking, there were 2 functions to check VBIOS posting, one considers VF and passthru while the other doesn’t. In fact we should always consider VF and PT for checking, right? For example, this checking here believe VF needs a posting because SCRATCH registers are not the expected value. Is it clear?
— 
Sincerely Yours,
Pixel








On 17/10/2017, 5:00 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:

>From the patch itself I still couldn't tell the difference 
>
>-----Original Message-----
>From: Ding, Pixel 
>Sent: 2017年10月17日 15:54
>To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
>Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>
>Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>
>It fixes a issue hidden in:
>
>95 static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
> 96 {
> 97	uint8_t __iomem *bios;
> 98	resource_size_t vram_base;
> 99	resource_size_t size = 256 * 1024; /* ??? */
>100
>101	if (!(adev->flags & AMD_IS_APU))
>102		if (amdgpu_need_post(adev))
>103		return false;
>
>
>This makes bios reading fallback to SMC INDEX/DATA register case.
>
>— 
>Sincerely Yours,
>Pixel
>
>
>
>
>
>
>
>
>On 17/10/2017, 3:48 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>
>>I don't understand how this patch works??? Looks like just rename vpost_needed to check_post
>>
>>-----Original Message-----
>>From: Pixel Ding [mailto:Pixel.Ding@amd.com] 
>>Sent: 2017年10月17日 14:38
>>To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
>>Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
>>Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>>
>>From: pding <Pixel.Ding@amd.com>
>>
>>The post checking on scratch registers isn't reliable for virtual function.
>>
>>Signed-off-by: pding <Pixel.Ding@amd.com>
>>---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>index 683965b..ab8f0d6 100644
>>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>@@ -669,7 +669,7 @@ void amdgpu_gart_location(struct amdgpu_device *adev, struct amdgpu_mc *mc)
>>  * or post is needed if  hw reset is performed.
>>  * Returns true if need or false if not.
>>  */
>>-bool amdgpu_need_post(struct amdgpu_device *adev)
>>+static bool amdgpu_check_post(struct amdgpu_device *adev)
>> {
>> 	uint32_t reg;
>> 
>>@@ -692,7 +692,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
>> 
>> }
>> 
>>-static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>+bool amdgpu_need_post(struct amdgpu_device *adev)
>> {
>> 	if (amdgpu_sriov_vf(adev))
>> 		return false;
>>@@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>> 				return true;
>> 		}
>> 	}
>>-	return amdgpu_need_post(adev);
>>+	return amdgpu_check_post(adev);
>> }
>> 
>> /**
>>@@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>> 	amdgpu_device_detect_sriov_bios(adev);
>> 
>> 	/* Post card if necessary */
>>-	if (amdgpu_vpost_needed(adev)) {
>>+	if (amdgpu_need_post(adev)) {
>> 		if (!adev->bios) {
>> 			dev_err(adev->dev, "no vBIOS found\n");
>> 			amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
>>--
>>2.9.5
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Liu, Monk]

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

yeah,


RB-by: Monk Liu <monk.liu@amd.com>


________________________________
From: Ding, Pixel
Sent: Tuesday, October 17, 2017 4:06 PM
To: Liu, Monk; amd-gfx@lists.freedesktop.org; Koenig, Christian
Cc: Li, Bingley; Sun, Gary
Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

you can see the difference of function amdgpu_need_post. Generally speaking, there were 2 functions to check VBIOS posting, one considers VF and passthru while the other doesn’t. In fact we should always consider VF and PT for checking, right? For example, this checking here believe VF needs a posting because SCRATCH registers are not the expected value. Is it clear?
—
Sincerely Yours,
Pixel








On 17/10/2017, 5:00 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:

>From the patch itself I still couldn't tell the difference
>
>-----Original Message-----
>From: Ding, Pixel
>Sent: 2017年10月17日 15:54
>To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
>Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>
>Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>
>It fixes a issue hidden in:
>
>95 static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
> 96 {
> 97    uint8_t __iomem *bios;
> 98    resource_size_t vram_base;
> 99    resource_size_t size = 256 * 1024; /* ??? */
>100
>101    if (!(adev->flags & AMD_IS_APU))
>102            if (amdgpu_need_post(adev))
>103            return false;
>
>
>This makes bios reading fallback to SMC INDEX/DATA register case.
>
>—
>Sincerely Yours,
>Pixel
>
>
>
>
>
>
>
>
>On 17/10/2017, 3:48 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>
>>I don't understand how this patch works??? Looks like just rename vpost_needed to check_post
>>
>>-----Original Message-----
>>From: Pixel Ding [mailto:Pixel.Ding@amd.com]
>>Sent: 2017年10月17日 14:38
>>To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
>>Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
>>Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>>
>>From: pding <Pixel.Ding@amd.com>
>>
>>The post checking on scratch registers isn't reliable for virtual function.
>>
>>Signed-off-by: pding <Pixel.Ding@amd.com>
>>---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>index 683965b..ab8f0d6 100644
>>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>@@ -669,7 +669,7 @@ void amdgpu_gart_location(struct amdgpu_device *adev, struct amdgpu_mc *mc)
>>  * or post is needed if  hw reset is performed.
>>  * Returns true if need or false if not.
>>  */
>>-bool amdgpu_need_post(struct amdgpu_device *adev)
>>+static bool amdgpu_check_post(struct amdgpu_device *adev)
>> {
>>       uint32_t reg;
>>
>>@@ -692,7 +692,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
>>
>> }
>>
>>-static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>+bool amdgpu_need_post(struct amdgpu_device *adev)
>> {
>>       if (amdgpu_sriov_vf(adev))
>>               return false;
>>@@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>                               return true;
>>               }
>>       }
>>-     return amdgpu_need_post(adev);
>>+     return amdgpu_check_post(adev);
>> }
>>
>> /**
>>@@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>       amdgpu_device_detect_sriov_bios(adev);
>>
>>       /* Post card if necessary */
>>-     if (amdgpu_vpost_needed(adev)) {
>>+     if (amdgpu_need_post(adev)) {
>>               if (!adev->bios) {
>>                       dev_err(adev->dev, "no vBIOS found\n");
>>                       amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
>>--
>>2.9.5
>>

[-- Attachment #1.2: Type: text/html, Size: 7483 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Ding, Pixel]

Hi all,

Could someone review this patch? 

— 
Sincerely Yours,
Pixel







On 17/10/2017, 4:06 PM, "amd-gfx on behalf of Ding, Pixel" <amd-gfx-bounces@lists.freedesktop.org on behalf of Pixel.Ding@amd.com> wrote:

>you can see the difference of function amdgpu_need_post. Generally speaking, there were 2 functions to check VBIOS posting, one considers VF and passthru while the other doesn’t. In fact we should always consider VF and PT for checking, right? For example, this checking here believe VF needs a posting because SCRATCH registers are not the expected value. Is it clear?
>— 
>Sincerely Yours,
>Pixel
>
>
>
>
>
>
>
>
>On 17/10/2017, 5:00 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>
>>From the patch itself I still couldn't tell the difference 
>>
>>-----Original Message-----
>>From: Ding, Pixel 
>>Sent: 2017年10月17日 15:54
>>To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
>>Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>
>>Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>>
>>It fixes a issue hidden in:
>>
>>95 static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
>> 96 {
>> 97	uint8_t __iomem *bios;
>> 98	resource_size_t vram_base;
>> 99	resource_size_t size = 256 * 1024; /* ??? */
>>100
>>101	if (!(adev->flags & AMD_IS_APU))
>>102		if (amdgpu_need_post(adev))
>>103		return false;
>>
>>
>>This makes bios reading fallback to SMC INDEX/DATA register case.
>>
>>— 
>>Sincerely Yours,
>>Pixel
>>
>>
>>
>>
>>
>>
>>
>>
>>On 17/10/2017, 3:48 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>>
>>>I don't understand how this patch works??? Looks like just rename vpost_needed to check_post
>>>
>>>-----Original Message-----
>>>From: Pixel Ding [mailto:Pixel.Ding@amd.com] 
>>>Sent: 2017年10月17日 14:38
>>>To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
>>>Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
>>>Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>>>
>>>From: pding <Pixel.Ding@amd.com>
>>>
>>>The post checking on scratch registers isn't reliable for virtual function.
>>>
>>>Signed-off-by: pding <Pixel.Ding@amd.com>
>>>---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>index 683965b..ab8f0d6 100644
>>>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>@@ -669,7 +669,7 @@ void amdgpu_gart_location(struct amdgpu_device *adev, struct amdgpu_mc *mc)
>>>  * or post is needed if  hw reset is performed.
>>>  * Returns true if need or false if not.
>>>  */
>>>-bool amdgpu_need_post(struct amdgpu_device *adev)
>>>+static bool amdgpu_check_post(struct amdgpu_device *adev)
>>> {
>>> 	uint32_t reg;
>>> 
>>>@@ -692,7 +692,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
>>> 
>>> }
>>> 
>>>-static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>>+bool amdgpu_need_post(struct amdgpu_device *adev)
>>> {
>>> 	if (amdgpu_sriov_vf(adev))
>>> 		return false;
>>>@@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>> 				return true;
>>> 		}
>>> 	}
>>>-	return amdgpu_need_post(adev);
>>>+	return amdgpu_check_post(adev);
>>> }
>>> 
>>> /**
>>>@@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>> 	amdgpu_device_detect_sriov_bios(adev);
>>> 
>>> 	/* Post card if necessary */
>>>-	if (amdgpu_vpost_needed(adev)) {
>>>+	if (amdgpu_need_post(adev)) {
>>> 		if (!adev->bios) {
>>> 			dev_err(adev->dev, "no vBIOS found\n");
>>> 			amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
>>>--
>>>2.9.5
>>>
>_______________________________________________
>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

Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Ding, Pixel]

Hi Christian,

Would you please take a look at this change? It’s to unify the VBIOS post checking.
— 
Sincerely Yours,
Pixel








On 18/10/2017, 10:25 AM, "Ding, Pixel" <Pixel.Ding@amd.com> wrote:

>Hi all,
>
>Could someone review this patch? 
>
>— 
>Sincerely Yours,
>Pixel
>
>
>
>
>
>
>
>On 17/10/2017, 4:06 PM, "amd-gfx on behalf of Ding, Pixel" <amd-gfx-bounces@lists.freedesktop.org on behalf of Pixel.Ding@amd.com> wrote:
>
>>you can see the difference of function amdgpu_need_post. Generally speaking, there were 2 functions to check VBIOS posting, one considers VF and passthru while the other doesn’t. In fact we should always consider VF and PT for checking, right? For example, this checking here believe VF needs a posting because SCRATCH registers are not the expected value. Is it clear?
>>— 
>>Sincerely Yours,
>>Pixel
>>
>>
>>
>>
>>
>>
>>
>>
>>On 17/10/2017, 5:00 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>>
>>>From the patch itself I still couldn't tell the difference 
>>>
>>>-----Original Message-----
>>>From: Ding, Pixel 
>>>Sent: 2017年10月17日 15:54
>>>To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
>>>Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>
>>>Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>>>
>>>It fixes a issue hidden in:
>>>
>>>95 static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
>>> 96 {
>>> 97	uint8_t __iomem *bios;
>>> 98	resource_size_t vram_base;
>>> 99	resource_size_t size = 256 * 1024; /* ??? */
>>>100
>>>101	if (!(adev->flags & AMD_IS_APU))
>>>102		if (amdgpu_need_post(adev))
>>>103		return false;
>>>
>>>
>>>This makes bios reading fallback to SMC INDEX/DATA register case.
>>>
>>>— 
>>>Sincerely Yours,
>>>Pixel
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>On 17/10/2017, 3:48 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>>>
>>>>I don't understand how this patch works??? Looks like just rename vpost_needed to check_post
>>>>
>>>>-----Original Message-----
>>>>From: Pixel Ding [mailto:Pixel.Ding@amd.com] 
>>>>Sent: 2017年10月17日 14:38
>>>>To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
>>>>Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
>>>>Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>>>>
>>>>From: pding <Pixel.Ding@amd.com>
>>>>
>>>>The post checking on scratch registers isn't reliable for virtual function.
>>>>
>>>>Signed-off-by: pding <Pixel.Ding@amd.com>
>>>>---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>index 683965b..ab8f0d6 100644
>>>>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>@@ -669,7 +669,7 @@ void amdgpu_gart_location(struct amdgpu_device *adev, struct amdgpu_mc *mc)
>>>>  * or post is needed if  hw reset is performed.
>>>>  * Returns true if need or false if not.
>>>>  */
>>>>-bool amdgpu_need_post(struct amdgpu_device *adev)
>>>>+static bool amdgpu_check_post(struct amdgpu_device *adev)
>>>> {
>>>> 	uint32_t reg;
>>>> 
>>>>@@ -692,7 +692,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
>>>> 
>>>> }
>>>> 
>>>>-static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>>>+bool amdgpu_need_post(struct amdgpu_device *adev)
>>>> {
>>>> 	if (amdgpu_sriov_vf(adev))
>>>> 		return false;
>>>>@@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>>> 				return true;
>>>> 		}
>>>> 	}
>>>>-	return amdgpu_need_post(adev);
>>>>+	return amdgpu_check_post(adev);
>>>> }
>>>> 
>>>> /**
>>>>@@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>>> 	amdgpu_device_detect_sriov_bios(adev);
>>>> 
>>>> 	/* Post card if necessary */
>>>>-	if (amdgpu_vpost_needed(adev)) {
>>>>+	if (amdgpu_need_post(adev)) {
>>>> 		if (!adev->bios) {
>>>> 			dev_err(adev->dev, "no vBIOS found\n");
>>>> 			amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
>>>>--
>>>>2.9.5
>>>>
>>_______________________________________________
>>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

Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Christian König]

I've already did, but honestly have no idea what you do here.

ASIC post is not something I've every worked on.

It looks odd that you add/remove some static keyword here, but I can't 
judge the technical correctness.

Monk, Alex what do you think of this?

Sorry,
Christian.

Am 18.10.2017 um 09:19 schrieb Ding, Pixel:
> Hi Christian,
>
> Would you please take a look at this change? It’s to unify the VBIOS post checking.
> —
> Sincerely Yours,
> Pixel
>
>
>
>
>
>
>
>
> On 18/10/2017, 10:25 AM, "Ding, Pixel" <Pixel.Ding@amd.com> wrote:
>
>> Hi all,
>>
>> Could someone review this patch?
>>
>> —
>> Sincerely Yours,
>> Pixel
>>
>>
>>
>>
>>
>>
>>
>> On 17/10/2017, 4:06 PM, "amd-gfx on behalf of Ding, Pixel" <amd-gfx-bounces@lists.freedesktop.org on behalf of Pixel.Ding@amd.com> wrote:
>>
>>> you can see the difference of function amdgpu_need_post. Generally speaking, there were 2 functions to check VBIOS posting, one considers VF and passthru while the other doesn’t. In fact we should always consider VF and PT for checking, right? For example, this checking here believe VF needs a posting because SCRATCH registers are not the expected value. Is it clear?
>>> —
>>> Sincerely Yours,
>>> Pixel
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> On 17/10/2017, 5:00 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>>>
>>> >From the patch itself I still couldn't tell the difference 
>>>> -----Original Message-----
>>>> From: Ding, Pixel
>>>> Sent: 2017年10月17日 15:54
>>>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
>>>> Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>
>>>> Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>>>>
>>>> It fixes a issue hidden in:
>>>>
>>>> 95 static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
>>>> 96 {
>>>> 97	uint8_t __iomem *bios;
>>>> 98	resource_size_t vram_base;
>>>> 99	resource_size_t size = 256 * 1024; /* ??? */
>>>> 100
>>>> 101	if (!(adev->flags & AMD_IS_APU))
>>>> 102		if (amdgpu_need_post(adev))
>>>> 103		return false;
>>>>
>>>>
>>>> This makes bios reading fallback to SMC INDEX/DATA register case.
>>>>
>>>> —
>>>> Sincerely Yours,
>>>> Pixel
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On 17/10/2017, 3:48 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>>>>
>>>>> I don't understand how this patch works??? Looks like just rename vpost_needed to check_post
>>>>>
>>>>> -----Original Message-----
>>>>> From: Pixel Ding [mailto:Pixel.Ding@amd.com]
>>>>> Sent: 2017年10月17日 14:38
>>>>> To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
>>>>> Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
>>>>> Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>>>>>
>>>>> From: pding <Pixel.Ding@amd.com>
>>>>>
>>>>> The post checking on scratch registers isn't reliable for virtual function.
>>>>>
>>>>> Signed-off-by: pding <Pixel.Ding@amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> index 683965b..ab8f0d6 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> @@ -669,7 +669,7 @@ void amdgpu_gart_location(struct amdgpu_device *adev, struct amdgpu_mc *mc)
>>>>>   * or post is needed if  hw reset is performed.
>>>>>   * Returns true if need or false if not.
>>>>>   */
>>>>> -bool amdgpu_need_post(struct amdgpu_device *adev)
>>>>> +static bool amdgpu_check_post(struct amdgpu_device *adev)
>>>>> {
>>>>> 	uint32_t reg;
>>>>>
>>>>> @@ -692,7 +692,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
>>>>>
>>>>> }
>>>>>
>>>>> -static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>>>> +bool amdgpu_need_post(struct amdgpu_device *adev)
>>>>> {
>>>>> 	if (amdgpu_sriov_vf(adev))
>>>>> 		return false;
>>>>> @@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>>>> 				return true;
>>>>> 		}
>>>>> 	}
>>>>> -	return amdgpu_need_post(adev);
>>>>> +	return amdgpu_check_post(adev);
>>>>> }
>>>>>
>>>>> /**
>>>>> @@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>>>> 	amdgpu_device_detect_sriov_bios(adev);
>>>>>
>>>>> 	/* Post card if necessary */
>>>>> -	if (amdgpu_vpost_needed(adev)) {
>>>>> +	if (amdgpu_need_post(adev)) {
>>>>> 		if (!adev->bios) {
>>>>> 			dev_err(adev->dev, "no vBIOS found\n");
>>>>> 			amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
>>>>> --
>>>>> 2.9.5
>>>>>
>>> _______________________________________________
>>> 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

Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Liu, Monk]

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

yeah, this patch is correct

________________________________
From: Koenig, Christian
Sent: Wednesday, October 18, 2017 3:23:04 PM
To: Ding, Pixel; Liu, Monk; amd-gfx@lists.freedesktop.org
Cc: Sun, Gary; Li, Bingley
Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

I've already did, but honestly have no idea what you do here.

ASIC post is not something I've every worked on.

It looks odd that you add/remove some static keyword here, but I can't
judge the technical correctness.

Monk, Alex what do you think of this?

Sorry,
Christian.

Am 18.10.2017 um 09:19 schrieb Ding, Pixel:
> Hi Christian,
>
> Would you please take a look at this change? It’s to unify the VBIOS post checking.
> —
> Sincerely Yours,
> Pixel
>
>
>
>
>
>
>
>
> On 18/10/2017, 10:25 AM, "Ding, Pixel" <Pixel.Ding@amd.com> wrote:
>
>> Hi all,
>>
>> Could someone review this patch?
>>
>> —
>> Sincerely Yours,
>> Pixel
>>
>>
>>
>>
>>
>>
>>
>> On 17/10/2017, 4:06 PM, "amd-gfx on behalf of Ding, Pixel" <amd-gfx-bounces@lists.freedesktop.org on behalf of Pixel.Ding@amd.com> wrote:
>>
>>> you can see the difference of function amdgpu_need_post. Generally speaking, there were 2 functions to check VBIOS posting, one considers VF and passthru while the other doesn’t. In fact we should always consider VF and PT for checking, right? For example, this checking here believe VF needs a posting because SCRATCH registers are not the expected value. Is it clear?
>>> —
>>> Sincerely Yours,
>>> Pixel
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> On 17/10/2017, 5:00 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>>>
>>> >From the patch itself I still couldn't tell the difference
>>>> -----Original Message-----
>>>> From: Ding, Pixel
>>>> Sent: 2017年10月17日 15:54
>>>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
>>>> Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>
>>>> Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>>>>
>>>> It fixes a issue hidden in:
>>>>
>>>> 95 static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
>>>> 96 {
>>>> 97 uint8_t __iomem *bios;
>>>> 98 resource_size_t vram_base;
>>>> 99 resource_size_t size = 256 * 1024; /* ??? */
>>>> 100
>>>> 101        if (!(adev->flags & AMD_IS_APU))
>>>> 102                if (amdgpu_need_post(adev))
>>>> 103                return false;
>>>>
>>>>
>>>> This makes bios reading fallback to SMC INDEX/DATA register case.
>>>>
>>>> —
>>>> Sincerely Yours,
>>>> Pixel
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On 17/10/2017, 3:48 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>>>>
>>>>> I don't understand how this patch works??? Looks like just rename vpost_needed to check_post
>>>>>
>>>>> -----Original Message-----
>>>>> From: Pixel Ding [mailto:Pixel.Ding@amd.com]
>>>>> Sent: 2017年10月17日 14:38
>>>>> To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
>>>>> Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
>>>>> Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>>>>>
>>>>> From: pding <Pixel.Ding@amd.com>
>>>>>
>>>>> The post checking on scratch registers isn't reliable for virtual function.
>>>>>
>>>>> Signed-off-by: pding <Pixel.Ding@amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> index 683965b..ab8f0d6 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> @@ -669,7 +669,7 @@ void amdgpu_gart_location(struct amdgpu_device *adev, struct amdgpu_mc *mc)
>>>>>   * or post is needed if  hw reset is performed.
>>>>>   * Returns true if need or false if not.
>>>>>   */
>>>>> -bool amdgpu_need_post(struct amdgpu_device *adev)
>>>>> +static bool amdgpu_check_post(struct amdgpu_device *adev)
>>>>> {
>>>>>    uint32_t reg;
>>>>>
>>>>> @@ -692,7 +692,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
>>>>>
>>>>> }
>>>>>
>>>>> -static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>>>> +bool amdgpu_need_post(struct amdgpu_device *adev)
>>>>> {
>>>>>    if (amdgpu_sriov_vf(adev))
>>>>>            return false;
>>>>> @@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>>>>                            return true;
>>>>>            }
>>>>>    }
>>>>> - return amdgpu_need_post(adev);
>>>>> + return amdgpu_check_post(adev);
>>>>> }
>>>>>
>>>>> /**
>>>>> @@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>>>>    amdgpu_device_detect_sriov_bios(adev);
>>>>>
>>>>>    /* Post card if necessary */
>>>>> - if (amdgpu_vpost_needed(adev)) {
>>>>> + if (amdgpu_need_post(adev)) {
>>>>>            if (!adev->bios) {
>>>>>                    dev_err(adev->dev, "no vBIOS found\n");
>>>>>                    amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
>>>>> --
>>>>> 2.9.5
>>>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx



[-- Attachment #1.2: Type: text/html, Size: 10333 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Ding, Pixel]

Sorry for the confusion, while I still believe this is the right way to go.

amdgpu_need_post and amdgpu_vpost_needed both try to check the VBIOS posting, amdgpu_need_post doesn’t consider VF/PT devices. They were both in use.

This patch is to unify these 2 functions to single one and always consider the VF/PT devices.

So the amdgpu_vpost_needed is renamed to amdgpu_need_post, and the previous amdgpu_need_post becomes a static function.
— 
Sincerely Yours,
Pixel







On 18/10/2017, 3:23 PM, "amd-gfx on behalf of Christian König" <amd-gfx-bounces@lists.freedesktop.org on behalf of christian.koenig@amd.com> wrote:

>I've already did, but honestly have no idea what you do here.
>
>ASIC post is not something I've every worked on.
>
>It looks odd that you add/remove some static keyword here, but I can't 
>judge the technical correctness.
>
>Monk, Alex what do you think of this?
>
>Sorry,
>Christian.
>
>Am 18.10.2017 um 09:19 schrieb Ding, Pixel:
>> Hi Christian,
>>
>> Would you please take a look at this change? It’s to unify the VBIOS post checking.
>> —
>> Sincerely Yours,
>> Pixel
>>
>>
>>
>>
>>
>>
>>
>>
>> On 18/10/2017, 10:25 AM, "Ding, Pixel" <Pixel.Ding@amd.com> wrote:
>>
>>> Hi all,
>>>
>>> Could someone review this patch?
>>>
>>> —
>>> Sincerely Yours,
>>> Pixel
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> On 17/10/2017, 4:06 PM, "amd-gfx on behalf of Ding, Pixel" <amd-gfx-bounces@lists.freedesktop.org on behalf of Pixel.Ding@amd.com> wrote:
>>>
>>>> you can see the difference of function amdgpu_need_post. Generally speaking, there were 2 functions to check VBIOS posting, one considers VF and passthru while the other doesn’t. In fact we should always consider VF and PT for checking, right? For example, this checking here believe VF needs a posting because SCRATCH registers are not the expected value. Is it clear?
>>>> —
>>>> Sincerely Yours,
>>>> Pixel
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On 17/10/2017, 5:00 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>>>>
>>>> >From the patch itself I still couldn't tell the difference 
>>>>> -----Original Message-----
>>>>> From: Ding, Pixel
>>>>> Sent: 2017年10月17日 15:54
>>>>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
>>>>> Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>
>>>>> Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>>>>>
>>>>> It fixes a issue hidden in:
>>>>>
>>>>> 95 static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
>>>>> 96 {
>>>>> 97	uint8_t __iomem *bios;
>>>>> 98	resource_size_t vram_base;
>>>>> 99	resource_size_t size = 256 * 1024; /* ??? */
>>>>> 100
>>>>> 101	if (!(adev->flags & AMD_IS_APU))
>>>>> 102		if (amdgpu_need_post(adev))
>>>>> 103		return false;
>>>>>
>>>>>
>>>>> This makes bios reading fallback to SMC INDEX/DATA register case.
>>>>>
>>>>> —
>>>>> Sincerely Yours,
>>>>> Pixel
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 17/10/2017, 3:48 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>>>>>
>>>>>> I don't understand how this patch works??? Looks like just rename vpost_needed to check_post
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Pixel Ding [mailto:Pixel.Ding@amd.com]
>>>>>> Sent: 2017年10月17日 14:38
>>>>>> To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
>>>>>> Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
>>>>>> Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post
>>>>>>
>>>>>> From: pding <Pixel.Ding@amd.com>
>>>>>>
>>>>>> The post checking on scratch registers isn't reliable for virtual function.
>>>>>>
>>>>>> Signed-off-by: pding <Pixel.Ding@amd.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> index 683965b..ab8f0d6 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> @@ -669,7 +669,7 @@ void amdgpu_gart_location(struct amdgpu_device *adev, struct amdgpu_mc *mc)
>>>>>>   * or post is needed if  hw reset is performed.
>>>>>>   * Returns true if need or false if not.
>>>>>>   */
>>>>>> -bool amdgpu_need_post(struct amdgpu_device *adev)
>>>>>> +static bool amdgpu_check_post(struct amdgpu_device *adev)
>>>>>> {
>>>>>> 	uint32_t reg;
>>>>>>
>>>>>> @@ -692,7 +692,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
>>>>>>
>>>>>> }
>>>>>>
>>>>>> -static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>>>>> +bool amdgpu_need_post(struct amdgpu_device *adev)
>>>>>> {
>>>>>> 	if (amdgpu_sriov_vf(adev))
>>>>>> 		return false;
>>>>>> @@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>>>>> 				return true;
>>>>>> 		}
>>>>>> 	}
>>>>>> -	return amdgpu_need_post(adev);
>>>>>> +	return amdgpu_check_post(adev);
>>>>>> }
>>>>>>
>>>>>> /**
>>>>>> @@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>>>>> 	amdgpu_device_detect_sriov_bios(adev);
>>>>>>
>>>>>> 	/* Post card if necessary */
>>>>>> -	if (amdgpu_vpost_needed(adev)) {
>>>>>> +	if (amdgpu_need_post(adev)) {
>>>>>> 		if (!adev->bios) {
>>>>>> 			dev_err(adev->dev, "no vBIOS found\n");
>>>>>> 			amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
>>>>>> --
>>>>>> 2.9.5
>>>>>>
>>>> _______________________________________________
>>>> 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Christian König
> Sent: Wednesday, October 18, 2017 3:23 AM
> To: Ding, Pixel; Liu, Monk; amd-gfx@lists.freedesktop.org
> Cc: Sun, Gary; Li, Bingley
> Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for
> checking post
> 
> I've already did, but honestly have no idea what you do here.
> 
> ASIC post is not something I've every worked on.
> 
> It looks odd that you add/remove some static keyword here, but I can't
> judge the technical correctness.
> 
> Monk, Alex what do you think of this?

I think we should fix the issue in amdgpu_bios.c or sort out/merge amdgpu_need_post and amdgpu_vpost_needed.  This change is really unclear.

Alex

> 
> Sorry,
> Christian.
> 
> Am 18.10.2017 um 09:19 schrieb Ding, Pixel:
> > Hi Christian,
> >
> > Would you please take a look at this change? It’s to unify the VBIOS post
> checking.
> > —
> > Sincerely Yours,
> > Pixel
> >
> >
> >
> >
> >
> >
> >
> >
> > On 18/10/2017, 10:25 AM, "Ding, Pixel" <Pixel.Ding@amd.com> wrote:
> >
> >> Hi all,
> >>
> >> Could someone review this patch?
> >>
> >> —
> >> Sincerely Yours,
> >> Pixel
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >> On 17/10/2017, 4:06 PM, "amd-gfx on behalf of Ding, Pixel" <amd-gfx-
> bounces@lists.freedesktop.org on behalf of Pixel.Ding@amd.com> wrote:
> >>
> >>> you can see the difference of function amdgpu_need_post. Generally
> speaking, there were 2 functions to check VBIOS posting, one considers VF
> and passthru while the other doesn’t. In fact we should always consider VF
> and PT for checking, right? For example, this checking here believe VF needs
> a posting because SCRATCH registers are not the expected value. Is it clear?
> >>> —
> >>> Sincerely Yours,
> >>> Pixel
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> On 17/10/2017, 5:00 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
> >>>
> >>> >From the patch itself I still couldn't tell the difference
> >>>> -----Original Message-----
> >>>> From: Ding, Pixel
> >>>> Sent: 2017年10月17日 15:54
> >>>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org;
> Koenig, Christian <Christian.Koenig@amd.com>
> >>>> Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary
> <Gary.Sun@amd.com>
> >>>> Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised
> device for checking post
> >>>>
> >>>> It fixes a issue hidden in:
> >>>>
> >>>> 95 static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
> >>>> 96 {
> >>>> 97	uint8_t __iomem *bios;
> >>>> 98	resource_size_t vram_base;
> >>>> 99	resource_size_t size = 256 * 1024; /* ??? */
> >>>> 100
> >>>> 101	if (!(adev->flags & AMD_IS_APU))
> >>>> 102		if (amdgpu_need_post(adev))
> >>>> 103		return false;
> >>>>
> >>>>
> >>>> This makes bios reading fallback to SMC INDEX/DATA register case.
> >>>>
> >>>> —
> >>>> Sincerely Yours,
> >>>> Pixel
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> On 17/10/2017, 3:48 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
> >>>>
> >>>>> I don't understand how this patch works??? Looks like just rename
> vpost_needed to check_post
> >>>>>
> >>>>> -----Original Message-----
> >>>>> From: Pixel Ding [mailto:Pixel.Ding@amd.com]
> >>>>> Sent: 2017年10月17日 14:38
> >>>>> To: amd-gfx@lists.freedesktop.org; Liu, Monk
> <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
> >>>>> Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary
> <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
> >>>>> Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device
> for checking post
> >>>>>
> >>>>> From: pding <Pixel.Ding@amd.com>
> >>>>>
> >>>>> The post checking on scratch registers isn't reliable for virtual function.
> >>>>>
> >>>>> Signed-off-by: pding <Pixel.Ding@amd.com>
> >>>>> ---
> >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
> >>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>>>> index 683965b..ab8f0d6 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>>>> @@ -669,7 +669,7 @@ void amdgpu_gart_location(struct
> amdgpu_device *adev, struct amdgpu_mc *mc)
> >>>>>   * or post is needed if  hw reset is performed.
> >>>>>   * Returns true if need or false if not.
> >>>>>   */
> >>>>> -bool amdgpu_need_post(struct amdgpu_device *adev)
> >>>>> +static bool amdgpu_check_post(struct amdgpu_device *adev)
> >>>>> {
> >>>>> 	uint32_t reg;
> >>>>>
> >>>>> @@ -692,7 +692,7 @@ bool amdgpu_need_post(struct
> amdgpu_device *adev)
> >>>>>
> >>>>> }
> >>>>>
> >>>>> -static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
> >>>>> +bool amdgpu_need_post(struct amdgpu_device *adev)
> >>>>> {
> >>>>> 	if (amdgpu_sriov_vf(adev))
> >>>>> 		return false;
> >>>>> @@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct
> amdgpu_device *adev)
> >>>>> 				return true;
> >>>>> 		}
> >>>>> 	}
> >>>>> -	return amdgpu_need_post(adev);
> >>>>> +	return amdgpu_check_post(adev);
> >>>>> }
> >>>>>
> >>>>> /**
> >>>>> @@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct
> amdgpu_device *adev,
> >>>>> 	amdgpu_device_detect_sriov_bios(adev);
> >>>>>
> >>>>> 	/* Post card if necessary */
> >>>>> -	if (amdgpu_vpost_needed(adev)) {
> >>>>> +	if (amdgpu_need_post(adev)) {
> >>>>> 		if (!adev->bios) {
> >>>>> 			dev_err(adev->dev, "no vBIOS found\n");
> >>>>>
> 	amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
> >>>>> --
> >>>>> 2.9.5
> >>>>>
> >>> _______________________________________________
> >>> 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Ding, Pixel]

Hi Alex,

This change only affect virtualization and doesn’t change any code path of baremetal, and I have the virtualization test passed.

As we discussed in the v2 patch, merge 2 bios post checking function. Can I have your review for the v1? Or do you like to keep the amdgpu_vpost_needed() function name?

— 
Sincerely Yours,
Pixel







On 18/10/2017, 10:08 PM, "Deucher, Alexander" <Alexander.Deucher@amd.com> wrote:

>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of Christian König
>> Sent: Wednesday, October 18, 2017 3:23 AM
>> To: Ding, Pixel; Liu, Monk; amd-gfx@lists.freedesktop.org
>> Cc: Sun, Gary; Li, Bingley
>> Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for
>> checking post
>> 
>> I've already did, but honestly have no idea what you do here.
>> 
>> ASIC post is not something I've every worked on.
>> 
>> It looks odd that you add/remove some static keyword here, but I can't
>> judge the technical correctness.
>> 
>> Monk, Alex what do you think of this?
>
>I think we should fix the issue in amdgpu_bios.c or sort out/merge amdgpu_need_post and amdgpu_vpost_needed.  This change is really unclear.
>
>Alex
>
>> 
>> Sorry,
>> Christian.
>> 
>> Am 18.10.2017 um 09:19 schrieb Ding, Pixel:
>> > Hi Christian,
>> >
>> > Would you please take a look at this change? It’s to unify the VBIOS post
>> checking.
>> > —
>> > Sincerely Yours,
>> > Pixel
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> > On 18/10/2017, 10:25 AM, "Ding, Pixel" <Pixel.Ding@amd.com> wrote:
>> >
>> >> Hi all,
>> >>
>> >> Could someone review this patch?
>> >>
>> >> —
>> >> Sincerely Yours,
>> >> Pixel
>> >>
>> >>
>> >>
>> >>
>> >>
>> >>
>> >>
>> >> On 17/10/2017, 4:06 PM, "amd-gfx on behalf of Ding, Pixel" <amd-gfx-
>> bounces@lists.freedesktop.org on behalf of Pixel.Ding@amd.com> wrote:
>> >>
>> >>> you can see the difference of function amdgpu_need_post. Generally
>> speaking, there were 2 functions to check VBIOS posting, one considers VF
>> and passthru while the other doesn’t. In fact we should always consider VF
>> and PT for checking, right? For example, this checking here believe VF needs
>> a posting because SCRATCH registers are not the expected value. Is it clear?
>> >>> —
>> >>> Sincerely Yours,
>> >>> Pixel
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>
>> >>> On 17/10/2017, 5:00 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>> >>>
>> >>> >From the patch itself I still couldn't tell the difference
>> >>>> -----Original Message-----
>> >>>> From: Ding, Pixel
>> >>>> Sent: 2017年10月17日 15:54
>> >>>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org;
>> Koenig, Christian <Christian.Koenig@amd.com>
>> >>>> Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary
>> <Gary.Sun@amd.com>
>> >>>> Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised
>> device for checking post
>> >>>>
>> >>>> It fixes a issue hidden in:
>> >>>>
>> >>>> 95 static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
>> >>>> 96 {
>> >>>> 97	uint8_t __iomem *bios;
>> >>>> 98	resource_size_t vram_base;
>> >>>> 99	resource_size_t size = 256 * 1024; /* ??? */
>> >>>> 100
>> >>>> 101	if (!(adev->flags & AMD_IS_APU))
>> >>>> 102		if (amdgpu_need_post(adev))
>> >>>> 103		return false;
>> >>>>
>> >>>>
>> >>>> This makes bios reading fallback to SMC INDEX/DATA register case.
>> >>>>
>> >>>> —
>> >>>> Sincerely Yours,
>> >>>> Pixel
>> >>>>
>> >>>>
>> >>>>
>> >>>>
>> >>>>
>> >>>>
>> >>>>
>> >>>>
>> >>>> On 17/10/2017, 3:48 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
>> >>>>
>> >>>>> I don't understand how this patch works??? Looks like just rename
>> vpost_needed to check_post
>> >>>>>
>> >>>>> -----Original Message-----
>> >>>>> From: Pixel Ding [mailto:Pixel.Ding@amd.com]
>> >>>>> Sent: 2017年10月17日 14:38
>> >>>>> To: amd-gfx@lists.freedesktop.org; Liu, Monk
>> <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
>> >>>>> Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary
>> <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
>> >>>>> Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device
>> for checking post
>> >>>>>
>> >>>>> From: pding <Pixel.Ding@amd.com>
>> >>>>>
>> >>>>> The post checking on scratch registers isn't reliable for virtual function.
>> >>>>>
>> >>>>> Signed-off-by: pding <Pixel.Ding@amd.com>
>> >>>>> ---
>> >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
>> >>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>> >>>>>
>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> >>>>> index 683965b..ab8f0d6 100644
>> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> >>>>> @@ -669,7 +669,7 @@ void amdgpu_gart_location(struct
>> amdgpu_device *adev, struct amdgpu_mc *mc)
>> >>>>>   * or post is needed if  hw reset is performed.
>> >>>>>   * Returns true if need or false if not.
>> >>>>>   */
>> >>>>> -bool amdgpu_need_post(struct amdgpu_device *adev)
>> >>>>> +static bool amdgpu_check_post(struct amdgpu_device *adev)
>> >>>>> {
>> >>>>> 	uint32_t reg;
>> >>>>>
>> >>>>> @@ -692,7 +692,7 @@ bool amdgpu_need_post(struct
>> amdgpu_device *adev)
>> >>>>>
>> >>>>> }
>> >>>>>
>> >>>>> -static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>> >>>>> +bool amdgpu_need_post(struct amdgpu_device *adev)
>> >>>>> {
>> >>>>> 	if (amdgpu_sriov_vf(adev))
>> >>>>> 		return false;
>> >>>>> @@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct
>> amdgpu_device *adev)
>> >>>>> 				return true;
>> >>>>> 		}
>> >>>>> 	}
>> >>>>> -	return amdgpu_need_post(adev);
>> >>>>> +	return amdgpu_check_post(adev);
>> >>>>> }
>> >>>>>
>> >>>>> /**
>> >>>>> @@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct
>> amdgpu_device *adev,
>> >>>>> 	amdgpu_device_detect_sriov_bios(adev);
>> >>>>>
>> >>>>> 	/* Post card if necessary */
>> >>>>> -	if (amdgpu_vpost_needed(adev)) {
>> >>>>> +	if (amdgpu_need_post(adev)) {
>> >>>>> 		if (!adev->bios) {
>> >>>>> 			dev_err(adev->dev, "no vBIOS found\n");
>> >>>>>
>> 	amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
>> >>>>> --
>> >>>>> 2.9.5
>> >>>>>
>> >>> _______________________________________________
>> >>> 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/3] drm/amdgpu: always consider virualised device for checking post

[Deucher, Alexander]

> -----Original Message-----
> From: Ding, Pixel
> Sent: Wednesday, October 18, 2017 11:23 PM
> To: Deucher, Alexander; Koenig, Christian; Liu, Monk; amd-
> gfx@lists.freedesktop.org
> Cc: Sun, Gary; Li, Bingley
> Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device for
> checking post
> 
> Hi Alex,
> 
> This change only affect virtualization and doesn’t change any code path of
> baremetal, and I have the virtualization test passed.
> 
> As we discussed in the v2 patch, merge 2 bios post checking function. Can I
> have your review for the v1? Or do you like to keep the
> amdgpu_vpost_needed() function name?

I don't care what function name we use.  Just merge them into one if it's ok for virtualization.

Alex

> 
> —
> Sincerely Yours,
> Pixel
> 
> 
> 
> 
> 
> 
> 
> On 18/10/2017, 10:08 PM, "Deucher, Alexander"
> <Alexander.Deucher@amd.com> wrote:
> 
> >> -----Original Message-----
> >> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
> Behalf
> >> Of Christian König
> >> Sent: Wednesday, October 18, 2017 3:23 AM
> >> To: Ding, Pixel; Liu, Monk; amd-gfx@lists.freedesktop.org
> >> Cc: Sun, Gary; Li, Bingley
> >> Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised device
> for
> >> checking post
> >>
> >> I've already did, but honestly have no idea what you do here.
> >>
> >> ASIC post is not something I've every worked on.
> >>
> >> It looks odd that you add/remove some static keyword here, but I can't
> >> judge the technical correctness.
> >>
> >> Monk, Alex what do you think of this?
> >
> >I think we should fix the issue in amdgpu_bios.c or sort out/merge
> amdgpu_need_post and amdgpu_vpost_needed.  This change is really
> unclear.
> >
> >Alex
> >
> >>
> >> Sorry,
> >> Christian.
> >>
> >> Am 18.10.2017 um 09:19 schrieb Ding, Pixel:
> >> > Hi Christian,
> >> >
> >> > Would you please take a look at this change? It’s to unify the VBIOS post
> >> checking.
> >> > —
> >> > Sincerely Yours,
> >> > Pixel
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> > On 18/10/2017, 10:25 AM, "Ding, Pixel" <Pixel.Ding@amd.com> wrote:
> >> >
> >> >> Hi all,
> >> >>
> >> >> Could someone review this patch?
> >> >>
> >> >> —
> >> >> Sincerely Yours,
> >> >> Pixel
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
> >> >> On 17/10/2017, 4:06 PM, "amd-gfx on behalf of Ding, Pixel" <amd-gfx-
> >> bounces@lists.freedesktop.org on behalf of Pixel.Ding@amd.com>
> wrote:
> >> >>
> >> >>> you can see the difference of function amdgpu_need_post.
> Generally
> >> speaking, there were 2 functions to check VBIOS posting, one considers
> VF
> >> and passthru while the other doesn’t. In fact we should always consider
> VF
> >> and PT for checking, right? For example, this checking here believe VF
> needs
> >> a posting because SCRATCH registers are not the expected value. Is it
> clear?
> >> >>> —
> >> >>> Sincerely Yours,
> >> >>> Pixel
> >> >>>
> >> >>>
> >> >>>
> >> >>>
> >> >>>
> >> >>>
> >> >>>
> >> >>>
> >> >>> On 17/10/2017, 5:00 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
> >> >>>
> >> >>> >From the patch itself I still couldn't tell the difference
> >> >>>> -----Original Message-----
> >> >>>> From: Ding, Pixel
> >> >>>> Sent: 2017年10月17日 15:54
> >> >>>> To: Liu, Monk <Monk.Liu@amd.com>; amd-
> gfx@lists.freedesktop.org;
> >> Koenig, Christian <Christian.Koenig@amd.com>
> >> >>>> Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary
> >> <Gary.Sun@amd.com>
> >> >>>> Subject: Re: [PATCH 1/3] drm/amdgpu: always consider virualised
> >> device for checking post
> >> >>>>
> >> >>>> It fixes a issue hidden in:
> >> >>>>
> >> >>>> 95 static bool igp_read_bios_from_vram(struct amdgpu_device
> *adev)
> >> >>>> 96 {
> >> >>>> 97	uint8_t __iomem *bios;
> >> >>>> 98	resource_size_t vram_base;
> >> >>>> 99	resource_size_t size = 256 * 1024; /* ??? */
> >> >>>> 100
> >> >>>> 101	if (!(adev->flags & AMD_IS_APU))
> >> >>>> 102		if (amdgpu_need_post(adev))
> >> >>>> 103		return false;
> >> >>>>
> >> >>>>
> >> >>>> This makes bios reading fallback to SMC INDEX/DATA register case.
> >> >>>>
> >> >>>> —
> >> >>>> Sincerely Yours,
> >> >>>> Pixel
> >> >>>>
> >> >>>>
> >> >>>>
> >> >>>>
> >> >>>>
> >> >>>>
> >> >>>>
> >> >>>>
> >> >>>> On 17/10/2017, 3:48 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:
> >> >>>>
> >> >>>>> I don't understand how this patch works??? Looks like just rename
> >> vpost_needed to check_post
> >> >>>>>
> >> >>>>> -----Original Message-----
> >> >>>>> From: Pixel Ding [mailto:Pixel.Ding@amd.com]
> >> >>>>> Sent: 2017年10月17日 14:38
> >> >>>>> To: amd-gfx@lists.freedesktop.org; Liu, Monk
> >> <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
> >> >>>>> Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary
> >> <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
> >> >>>>> Subject: [PATCH 1/3] drm/amdgpu: always consider virualised
> device
> >> for checking post
> >> >>>>>
> >> >>>>> From: pding <Pixel.Ding@amd.com>
> >> >>>>>
> >> >>>>> The post checking on scratch registers isn't reliable for virtual
> function.
> >> >>>>>
> >> >>>>> Signed-off-by: pding <Pixel.Ding@amd.com>
> >> >>>>> ---
> >> >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++----
> >> >>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
> >> >>>>>
> >> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> >>>>> index 683965b..ab8f0d6 100644
> >> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> >>>>> @@ -669,7 +669,7 @@ void amdgpu_gart_location(struct
> >> amdgpu_device *adev, struct amdgpu_mc *mc)
> >> >>>>>   * or post is needed if  hw reset is performed.
> >> >>>>>   * Returns true if need or false if not.
> >> >>>>>   */
> >> >>>>> -bool amdgpu_need_post(struct amdgpu_device *adev)
> >> >>>>> +static bool amdgpu_check_post(struct amdgpu_device *adev)
> >> >>>>> {
> >> >>>>> 	uint32_t reg;
> >> >>>>>
> >> >>>>> @@ -692,7 +692,7 @@ bool amdgpu_need_post(struct
> >> amdgpu_device *adev)
> >> >>>>>
> >> >>>>> }
> >> >>>>>
> >> >>>>> -static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
> >> >>>>> +bool amdgpu_need_post(struct amdgpu_device *adev)
> >> >>>>> {
> >> >>>>> 	if (amdgpu_sriov_vf(adev))
> >> >>>>> 		return false;
> >> >>>>> @@ -716,7 +716,7 @@ static bool amdgpu_vpost_needed(struct
> >> amdgpu_device *adev)
> >> >>>>> 				return true;
> >> >>>>> 		}
> >> >>>>> 	}
> >> >>>>> -	return amdgpu_need_post(adev);
> >> >>>>> +	return amdgpu_check_post(adev);
> >> >>>>> }
> >> >>>>>
> >> >>>>> /**
> >> >>>>> @@ -2208,7 +2208,7 @@ int amdgpu_device_init(struct
> >> amdgpu_device *adev,
> >> >>>>> 	amdgpu_device_detect_sriov_bios(adev);
> >> >>>>>
> >> >>>>> 	/* Post card if necessary */
> >> >>>>> -	if (amdgpu_vpost_needed(adev)) {
> >> >>>>> +	if (amdgpu_need_post(adev)) {
> >> >>>>> 		if (!adev->bios) {
> >> >>>>> 			dev_err(adev->dev, "no vBIOS found\n");
> >> >>>>>
> >> 	amdgpu_vf_error_put(AMDGIM_ERROR_VF_NO_VBIOS, 0, 0);
> >> >>>>> --
> >> >>>>> 2.9.5
> >> >>>>>
> >> >>> _______________________________________________
> >> >>> 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/3] drm/amdgpu: report more amdgpu_fence_info v2

[Pixel Ding]

From: pding <Pixel.Ding@amd.com>

Only for GFX ring. This can help checking MCBP feature.

v2: report more fence offs.

The fence at the end of the frame will indicate the completion status.
If the frame completed normally, the fence is written to the address
given in the EVENT_WRITE_EOP packet. If preemption occurred in the
previous IB the address is adjusted by 2 DWs. If work submitted in the
frame was reset before completion, the fence address is adjusted by
four DWs. In the case that preemption occurred, and before preemption
completed a reset was initiated, the address will be adjusted with six
DWs

Signed-off-by: pding <Pixel.Ding@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 09d5a5c..688740e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -645,6 +645,19 @@ static int amdgpu_debugfs_fence_info(struct seq_file *m, void *data)
 			   atomic_read(&ring->fence_drv.last_seq));
 		seq_printf(m, "Last emitted        0x%08x\n",
 			   ring->fence_drv.sync_seq);
+
+		if (ring->funcs->type != AMDGPU_RING_TYPE_GFX)
+			continue;
+
+		/* set in CP_VMID_PREEMPT and preemption occurred */
+		seq_printf(m, "Last preempted      0x%08x\n",
+			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 2)));
+		/* set in CP_VMID_RESET and reset occurred */
+		seq_printf(m, "Last reset          0x%08x\n",
+			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 4)));
+		/* Both preemption and reset occurred */
+		seq_printf(m, "Last both           0x%08x\n",
+			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 6)));
 	}
 	return 0;
 }
-- 
2.9.5

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

Re: [PATCH 2/3] drm/amdgpu: report more amdgpu_fence_info v2

[Christian König]

Am 17.10.2017 um 08:37 schrieb Pixel Ding:
> From: pding <Pixel.Ding@amd.com>
>
> Only for GFX ring. This can help checking MCBP feature.
>
> v2: report more fence offs.
>
> The fence at the end of the frame will indicate the completion status.
> If the frame completed normally, the fence is written to the address
> given in the EVENT_WRITE_EOP packet. If preemption occurred in the
> previous IB the address is adjusted by 2 DWs. If work submitted in the
> frame was reset before completion, the fence address is adjusted by
> four DWs. In the case that preemption occurred, and before preemption
> completed a reset was initiated, the address will be adjusted with six
> DWs
>
> Signed-off-by: pding <Pixel.Ding@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 09d5a5c..688740e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> @@ -645,6 +645,19 @@ static int amdgpu_debugfs_fence_info(struct seq_file *m, void *data)
>   			   atomic_read(&ring->fence_drv.last_seq));
>   		seq_printf(m, "Last emitted        0x%08x\n",
>   			   ring->fence_drv.sync_seq);
> +
> +		if (ring->funcs->type != AMDGPU_RING_TYPE_GFX)
> +			continue;
> +
> +		/* set in CP_VMID_PREEMPT and preemption occurred */
> +		seq_printf(m, "Last preempted      0x%08x\n",
> +			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 2)));
> +		/* set in CP_VMID_RESET and reset occurred */
> +		seq_printf(m, "Last reset          0x%08x\n",
> +			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 4)));
> +		/* Both preemption and reset occurred */
> +		seq_printf(m, "Last both           0x%08x\n",
> +			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 6)));
>   	}
>   	return 0;
>   }


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

RE: [PATCH 2/3] drm/amdgpu: report more amdgpu_fence_info v2

[Liu, Monk]

Please use if (amdgpu_sriov_vf())
To protect your added part

-----Original Message-----
From: Pixel Ding [mailto:Pixel.Ding@amd.com] 
Sent: 2017年10月17日 14:38
To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
Subject: [PATCH 2/3] drm/amdgpu: report more amdgpu_fence_info v2

From: pding <Pixel.Ding@amd.com>

Only for GFX ring. This can help checking MCBP feature.

v2: report more fence offs.

The fence at the end of the frame will indicate the completion status.
If the frame completed normally, the fence is written to the address given in the EVENT_WRITE_EOP packet. If preemption occurred in the previous IB the address is adjusted by 2 DWs. If work submitted in the frame was reset before completion, the fence address is adjusted by four DWs. In the case that preemption occurred, and before preemption completed a reset was initiated, the address will be adjusted with six DWs

Signed-off-by: pding <Pixel.Ding@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 09d5a5c..688740e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -645,6 +645,19 @@ static int amdgpu_debugfs_fence_info(struct seq_file *m, void *data)
 			   atomic_read(&ring->fence_drv.last_seq));
 		seq_printf(m, "Last emitted        0x%08x\n",
 			   ring->fence_drv.sync_seq);
+
+		if (ring->funcs->type != AMDGPU_RING_TYPE_GFX)
+			continue;
+
+		/* set in CP_VMID_PREEMPT and preemption occurred */
+		seq_printf(m, "Last preempted      0x%08x\n",
+			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 2)));
+		/* set in CP_VMID_RESET and reset occurred */
+		seq_printf(m, "Last reset          0x%08x\n",
+			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 4)));
+		/* Both preemption and reset occurred */
+		seq_printf(m, "Last both           0x%08x\n",
+			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 6)));
 	}
 	return 0;
 }
--
2.9.5

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

Re: [PATCH 2/3] drm/amdgpu: report more amdgpu_fence_info v2

[Ding, Pixel]

Why? The fence WB is always 8 DW.
— 
Sincerely Yours,
Pixel







On 17/10/2017, 3:49 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:

>Please use if (amdgpu_sriov_vf())
>To protect your added part
>
>-----Original Message-----
>From: Pixel Ding [mailto:Pixel.Ding@amd.com] 
>Sent: 2017年10月17日 14:38
>To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
>Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
>Subject: [PATCH 2/3] drm/amdgpu: report more amdgpu_fence_info v2
>
>From: pding <Pixel.Ding@amd.com>
>
>Only for GFX ring. This can help checking MCBP feature.
>
>v2: report more fence offs.
>
>The fence at the end of the frame will indicate the completion status.
>If the frame completed normally, the fence is written to the address given in the EVENT_WRITE_EOP packet. If preemption occurred in the previous IB the address is adjusted by 2 DWs. If work submitted in the frame was reset before completion, the fence address is adjusted by four DWs. In the case that preemption occurred, and before preemption completed a reset was initiated, the address will be adjusted with six DWs
>
>Signed-off-by: pding <Pixel.Ding@amd.com>
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>index 09d5a5c..688740e 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>@@ -645,6 +645,19 @@ static int amdgpu_debugfs_fence_info(struct seq_file *m, void *data)
> 			   atomic_read(&ring->fence_drv.last_seq));
> 		seq_printf(m, "Last emitted        0x%08x\n",
> 			   ring->fence_drv.sync_seq);
>+
>+		if (ring->funcs->type != AMDGPU_RING_TYPE_GFX)
>+			continue;
>+
>+		/* set in CP_VMID_PREEMPT and preemption occurred */
>+		seq_printf(m, "Last preempted      0x%08x\n",
>+			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 2)));
>+		/* set in CP_VMID_RESET and reset occurred */
>+		seq_printf(m, "Last reset          0x%08x\n",
>+			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 4)));
>+		/* Both preemption and reset occurred */
>+		seq_printf(m, "Last both           0x%08x\n",
>+			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 6)));
> 	}
> 	return 0;
> }
>--
>2.9.5
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/3] drm/amdgpu: report more amdgpu_fence_info v2

[Liu, Monk]

Okay leave it, we may need OS preemption in future so it still can be used for that purpose

Reviewed-by: Monk Liu <monk.liu@amd.com>


-----Original Message-----
From: Ding, Pixel 
Sent: 2017年10月17日 15:56
To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>
Subject: Re: [PATCH 2/3] drm/amdgpu: report more amdgpu_fence_info v2

Why? The fence WB is always 8 DW.
— 
Sincerely Yours,
Pixel







On 17/10/2017, 3:49 PM, "Liu, Monk" <Monk.Liu@amd.com> wrote:

>Please use if (amdgpu_sriov_vf())
>To protect your added part
>
>-----Original Message-----
>From: Pixel Ding [mailto:Pixel.Ding@amd.com] 
>Sent: 2017年10月17日 14:38
>To: amd-gfx@lists.freedesktop.org; Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
>Cc: Li, Bingley <Bingley.Li@amd.com>; Sun, Gary <Gary.Sun@amd.com>; Ding, Pixel <Pixel.Ding@amd.com>
>Subject: [PATCH 2/3] drm/amdgpu: report more amdgpu_fence_info v2
>
>From: pding <Pixel.Ding@amd.com>
>
>Only for GFX ring. This can help checking MCBP feature.
>
>v2: report more fence offs.
>
>The fence at the end of the frame will indicate the completion status.
>If the frame completed normally, the fence is written to the address given in the EVENT_WRITE_EOP packet. If preemption occurred in the previous IB the address is adjusted by 2 DWs. If work submitted in the frame was reset before completion, the fence address is adjusted by four DWs. In the case that preemption occurred, and before preemption completed a reset was initiated, the address will be adjusted with six DWs
>
>Signed-off-by: pding <Pixel.Ding@amd.com>
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>index 09d5a5c..688740e 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>@@ -645,6 +645,19 @@ static int amdgpu_debugfs_fence_info(struct seq_file *m, void *data)
> 			   atomic_read(&ring->fence_drv.last_seq));
> 		seq_printf(m, "Last emitted        0x%08x\n",
> 			   ring->fence_drv.sync_seq);
>+
>+		if (ring->funcs->type != AMDGPU_RING_TYPE_GFX)
>+			continue;
>+
>+		/* set in CP_VMID_PREEMPT and preemption occurred */
>+		seq_printf(m, "Last preempted      0x%08x\n",
>+			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 2)));
>+		/* set in CP_VMID_RESET and reset occurred */
>+		seq_printf(m, "Last reset          0x%08x\n",
>+			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 4)));
>+		/* Both preemption and reset occurred */
>+		seq_printf(m, "Last both           0x%08x\n",
>+			   le32_to_cpu(*(ring->fence_drv.cpu_addr + 6)));
> 	}
> 	return 0;
> }
>--
>2.9.5
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/3] drm/amdgpu: busywait KIQ register accessing v2

[Pixel Ding]

From: pding <Pixel.Ding@amd.com>

Register accessing is performed when IRQ is disabled. Never sleep in
this function.

Known issue: dead sleep in many use cases of index/data registers.

v2: wrap polling fence functions. don't trigger IRQ for polling in
case of wrongly fence signal.

Signed-off-by: pding <Pixel.Ding@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h               |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c        |  8 +---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c         | 53 +++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c           |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h          |  4 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c          | 30 ++++++-------
 8 files changed, 78 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index ca212ef..cc06e62 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -885,7 +885,7 @@ struct amdgpu_mec {
 struct amdgpu_kiq {
 	u64			eop_gpu_addr;
 	struct amdgpu_bo	*eop_obj;
-	struct mutex		ring_mutex;
+	spinlock_t              ring_lock;
 	struct amdgpu_ring	ring;
 	struct amdgpu_irq_src	irq;
 };
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
index 9d9965d..6d83573 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
@@ -788,7 +788,7 @@ static int invalidate_tlbs_with_kiq(struct amdgpu_device *adev, uint16_t pasid)
 	struct dma_fence *f;
 	struct amdgpu_ring *ring = &adev->gfx.kiq.ring;
 
-	mutex_lock(&adev->gfx.kiq.ring_mutex);
+	spin_lock(&adev->gfx.kiq.ring_lock);
 	amdgpu_ring_alloc(ring, 12); /* fence + invalidate_tlbs package*/
 	amdgpu_ring_write(ring, PACKET3(PACKET3_INVALIDATE_TLBS, 0));
 	amdgpu_ring_write(ring,
@@ -796,7 +796,7 @@ static int invalidate_tlbs_with_kiq(struct amdgpu_device *adev, uint16_t pasid)
 			PACKET3_INVALIDATE_TLBS_PASID(pasid));
 	amdgpu_fence_emit(ring, &f);
 	amdgpu_ring_commit(ring);
-	mutex_unlock(&adev->gfx.kiq.ring_mutex);
+	spin_unlock(&adev->gfx.kiq.ring_lock);
 
 	r = dma_fence_wait(f, false);
 	if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
index edbae19..c92217f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
@@ -973,7 +973,7 @@ static int invalidate_tlbs_with_kiq(struct amdgpu_device *adev, uint16_t pasid)
 	struct dma_fence *f;
 	struct amdgpu_ring *ring = &adev->gfx.kiq.ring;
 
-	mutex_lock(&adev->gfx.kiq.ring_mutex);
+	spin_lock(&adev->gfx.kiq.ring_lock);
 	amdgpu_ring_alloc(ring, 12); /* fence + invalidate_tlbs package*/
 	amdgpu_ring_write(ring, PACKET3(PACKET3_INVALIDATE_TLBS, 0));
 	amdgpu_ring_write(ring,
@@ -983,7 +983,7 @@ static int invalidate_tlbs_with_kiq(struct amdgpu_device *adev, uint16_t pasid)
 			PACKET3_INVALIDATE_TLBS_FLUSH_TYPE(2));
 	amdgpu_fence_emit(ring, &f);
 	amdgpu_ring_commit(ring);
-	mutex_unlock(&adev->gfx.kiq.ring_mutex);
+	spin_unlock(&adev->gfx.kiq.ring_lock);
 
 	r = dma_fence_wait(f, false);
 	if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index ab8f0d6..1197274 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -109,10 +109,8 @@ uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
 {
 	uint32_t ret;
 
-	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev)) {
-		BUG_ON(in_interrupt());
+	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev))
 		return amdgpu_virt_kiq_rreg(adev, reg);
-	}
 
 	if ((reg * 4) < adev->rmmio_size && !(acc_flags & AMDGPU_REGS_IDX))
 		ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
@@ -137,10 +135,8 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
 		adev->last_mm_index = v;
 	}
 
-	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev)) {
-		BUG_ON(in_interrupt());
+	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev))
 		return amdgpu_virt_kiq_wreg(adev, reg, v);
-	}
 
 	if ((reg * 4) < adev->rmmio_size && !(acc_flags & AMDGPU_REGS_IDX))
 		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 688740e..68a5e90 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -169,6 +169,32 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f)
 }
 
 /**
+ * amdgpu_fence_emit_polling - emit a fence on the requeste ring
+ *
+ * @ring: ring the fence is associated with
+ * @s: resulting sequence number
+ *
+ * Emits a fence command on the requested ring (all asics).
+ * Used For polling fence.
+ * Returns 0 on success, -ENOMEM on failure.
+ */
+int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s)
+{
+	uint32_t seq;
+
+	if (!s)
+		return -EINVAL;
+
+	seq = ++ring->fence_drv.sync_seq;
+	amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr,
+			       seq, AMDGPU_FENCE_FLAG_INT);
+
+	*s = seq;
+
+	return 0;
+}
+
+/**
  * amdgpu_fence_schedule_fallback - schedule fallback check
  *
  * @ring: pointer to struct amdgpu_ring
@@ -281,6 +307,33 @@ int amdgpu_fence_wait_empty(struct amdgpu_ring *ring)
 	return r;
 }
 
+signed long amdgpu_fence_wait_polling(struct amdgpu_ring *ring,
+				      uint32_t wait_seq,
+				      signed long timeout)
+{
+	uint32_t seq, last_seq;
+	struct amdgpu_fence_driver *drv = &ring->fence_drv;
+
+	last_seq = atomic_read(&ring->fence_drv.last_seq);
+
+	do {
+		seq = amdgpu_fence_read(ring);
+
+		if (unlikely(seq == last_seq))
+			break;
+		if (seq >= wait_seq && wait_seq >= last_seq)
+			break;
+		if (seq <= last_seq &&
+		    (wait_seq <= seq || wait_seq >= last_seq))
+			break;
+		udelay(5);
+		timeout -= 5;
+	} while (timeout > 0);
+
+	atomic_cmpxchg(&drv->last_seq, last_seq, wait_seq);
+
+	return timeout;
+}
 /**
  * amdgpu_fence_count_emitted - get the count of emitted fences
  *
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index 4f6c68f..e5a9077 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -185,7 +185,7 @@ int amdgpu_gfx_kiq_init_ring(struct amdgpu_device *adev,
 	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
 	int r = 0;
 
-	mutex_init(&kiq->ring_mutex);
+	spin_lock_init(&kiq->ring_lock);
 
 	r = amdgpu_wb_get(adev, &adev->virt.reg_val_offs);
 	if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index af8e544..9de89ea 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -89,8 +89,12 @@ int amdgpu_fence_driver_start_ring(struct amdgpu_ring *ring,
 void amdgpu_fence_driver_suspend(struct amdgpu_device *adev);
 void amdgpu_fence_driver_resume(struct amdgpu_device *adev);
 int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **fence);
+int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s);
 void amdgpu_fence_process(struct amdgpu_ring *ring);
 int amdgpu_fence_wait_empty(struct amdgpu_ring *ring);
+signed long amdgpu_fence_wait_polling(struct amdgpu_ring *ring,
+				      uint32_t wait_seq,
+				      signed long timeout);
 unsigned amdgpu_fence_count_emitted(struct amdgpu_ring *ring);
 
 /*
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index ab05121..177fe10 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -22,7 +22,7 @@
  */
 
 #include "amdgpu.h"
-#define MAX_KIQ_REG_WAIT	100000
+#define MAX_KIQ_REG_WAIT	100000000 /* in usecs */
 
 int amdgpu_allocate_static_csa(struct amdgpu_device *adev)
 {
@@ -114,27 +114,24 @@ void amdgpu_virt_init_setting(struct amdgpu_device *adev)
 uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
 {
 	signed long r;
-	uint32_t val;
-	struct dma_fence *f;
+	uint32_t val, seq;
 	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
 	struct amdgpu_ring *ring = &kiq->ring;
 
 	BUG_ON(!ring->funcs->emit_rreg);
 
-	mutex_lock(&kiq->ring_mutex);
+	spin_lock(&kiq->ring_lock);
 	amdgpu_ring_alloc(ring, 32);
 	amdgpu_ring_emit_rreg(ring, reg);
-	amdgpu_fence_emit(ring, &f);
+	amdgpu_fence_emit_polling(ring, &seq);
 	amdgpu_ring_commit(ring);
-	mutex_unlock(&kiq->ring_mutex);
+	spin_unlock(&kiq->ring_lock);
 
-	r = dma_fence_wait_timeout(f, false, msecs_to_jiffies(MAX_KIQ_REG_WAIT));
-	dma_fence_put(f);
+	r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
 	if (r < 1) {
-		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
+		DRM_ERROR("wait for kiq fence error: %ld\n", r);
 		return ~0;
 	}
-
 	val = adev->wb.wb[adev->virt.reg_val_offs];
 
 	return val;
@@ -143,23 +140,22 @@ uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
 void amdgpu_virt_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v)
 {
 	signed long r;
-	struct dma_fence *f;
+	uint32_t seq;
 	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
 	struct amdgpu_ring *ring = &kiq->ring;
 
 	BUG_ON(!ring->funcs->emit_wreg);
 
-	mutex_lock(&kiq->ring_mutex);
+	spin_lock(&kiq->ring_lock);
 	amdgpu_ring_alloc(ring, 32);
 	amdgpu_ring_emit_wreg(ring, reg, v);
-	amdgpu_fence_emit(ring, &f);
+	amdgpu_fence_emit_polling(ring, &seq);
 	amdgpu_ring_commit(ring);
-	mutex_unlock(&kiq->ring_mutex);
+	spin_unlock(&kiq->ring_lock);
 
-	r = dma_fence_wait_timeout(f, false, msecs_to_jiffies(MAX_KIQ_REG_WAIT));
+	r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
 	if (r < 1)
-		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
-	dma_fence_put(f);
+		DRM_ERROR("wait for kiq fence error: %ld\n", r);
 }
 
 /**
-- 
2.9.5

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

Re: [PATCH 3/3] drm/amdgpu: busywait KIQ register accessing v2

[Christian König]

Am 17.10.2017 um 08:37 schrieb Pixel Ding:
> From: pding <Pixel.Ding@amd.com>
>
> Register accessing is performed when IRQ is disabled. Never sleep in
> this function.
>
> Known issue: dead sleep in many use cases of index/data registers.
>
> v2: wrap polling fence functions. don't trigger IRQ for polling in
> case of wrongly fence signal.
>
> Signed-off-by: pding <Pixel.Ding@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h               |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  4 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  4 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c        |  8 +---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c         | 53 +++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c           |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h          |  4 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c          | 30 ++++++-------
>   8 files changed, 78 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index ca212ef..cc06e62 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -885,7 +885,7 @@ struct amdgpu_mec {
>   struct amdgpu_kiq {
>   	u64			eop_gpu_addr;
>   	struct amdgpu_bo	*eop_obj;
> -	struct mutex		ring_mutex;
> +	spinlock_t              ring_lock;
>   	struct amdgpu_ring	ring;
>   	struct amdgpu_irq_src	irq;
>   };
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> index 9d9965d..6d83573 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> @@ -788,7 +788,7 @@ static int invalidate_tlbs_with_kiq(struct amdgpu_device *adev, uint16_t pasid)
>   	struct dma_fence *f;
>   	struct amdgpu_ring *ring = &adev->gfx.kiq.ring;
>   
> -	mutex_lock(&adev->gfx.kiq.ring_mutex);
> +	spin_lock(&adev->gfx.kiq.ring_lock);
>   	amdgpu_ring_alloc(ring, 12); /* fence + invalidate_tlbs package*/
>   	amdgpu_ring_write(ring, PACKET3(PACKET3_INVALIDATE_TLBS, 0));
>   	amdgpu_ring_write(ring,
> @@ -796,7 +796,7 @@ static int invalidate_tlbs_with_kiq(struct amdgpu_device *adev, uint16_t pasid)
>   			PACKET3_INVALIDATE_TLBS_PASID(pasid));
>   	amdgpu_fence_emit(ring, &f);
>   	amdgpu_ring_commit(ring);
> -	mutex_unlock(&adev->gfx.kiq.ring_mutex);
> +	spin_unlock(&adev->gfx.kiq.ring_lock);
>   
>   	r = dma_fence_wait(f, false);
>   	if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> index edbae19..c92217f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> @@ -973,7 +973,7 @@ static int invalidate_tlbs_with_kiq(struct amdgpu_device *adev, uint16_t pasid)
>   	struct dma_fence *f;
>   	struct amdgpu_ring *ring = &adev->gfx.kiq.ring;
>   
> -	mutex_lock(&adev->gfx.kiq.ring_mutex);
> +	spin_lock(&adev->gfx.kiq.ring_lock);
>   	amdgpu_ring_alloc(ring, 12); /* fence + invalidate_tlbs package*/
>   	amdgpu_ring_write(ring, PACKET3(PACKET3_INVALIDATE_TLBS, 0));
>   	amdgpu_ring_write(ring,
> @@ -983,7 +983,7 @@ static int invalidate_tlbs_with_kiq(struct amdgpu_device *adev, uint16_t pasid)
>   			PACKET3_INVALIDATE_TLBS_FLUSH_TYPE(2));
>   	amdgpu_fence_emit(ring, &f);
>   	amdgpu_ring_commit(ring);
> -	mutex_unlock(&adev->gfx.kiq.ring_mutex);
> +	spin_unlock(&adev->gfx.kiq.ring_lock);
>   
>   	r = dma_fence_wait(f, false);
>   	if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index ab8f0d6..1197274 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -109,10 +109,8 @@ uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
>   {
>   	uint32_t ret;
>   
> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev)) {
> -		BUG_ON(in_interrupt());
> +	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev))
>   		return amdgpu_virt_kiq_rreg(adev, reg);
> -	}
>   
>   	if ((reg * 4) < adev->rmmio_size && !(acc_flags & AMDGPU_REGS_IDX))
>   		ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
> @@ -137,10 +135,8 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
>   		adev->last_mm_index = v;
>   	}
>   
> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev)) {
> -		BUG_ON(in_interrupt());
> +	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev))
>   		return amdgpu_virt_kiq_wreg(adev, reg, v);
> -	}
>   
>   	if ((reg * 4) < adev->rmmio_size && !(acc_flags & AMDGPU_REGS_IDX))
>   		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 688740e..68a5e90 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> @@ -169,6 +169,32 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f)
>   }
>   
>   /**
> + * amdgpu_fence_emit_polling - emit a fence on the requeste ring
> + *
> + * @ring: ring the fence is associated with
> + * @s: resulting sequence number
> + *
> + * Emits a fence command on the requested ring (all asics).
> + * Used For polling fence.
> + * Returns 0 on success, -ENOMEM on failure.
> + */
> +int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s)
> +{
> +	uint32_t seq;
> +
> +	if (!s)
> +		return -EINVAL;
> +
> +	seq = ++ring->fence_drv.sync_seq;
> +	amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr,
> +			       seq, AMDGPU_FENCE_FLAG_INT);
> +
> +	*s = seq;
> +
> +	return 0;
> +}
> +
> +/**
>    * amdgpu_fence_schedule_fallback - schedule fallback check
>    *
>    * @ring: pointer to struct amdgpu_ring
> @@ -281,6 +307,33 @@ int amdgpu_fence_wait_empty(struct amdgpu_ring *ring)
>   	return r;
>   }
>   

Please add some documentation here that timeout is in usec.

> +signed long amdgpu_fence_wait_polling(struct amdgpu_ring *ring,
> +				      uint32_t wait_seq,
> +				      signed long timeout)
> +{
> +	uint32_t seq, last_seq;
> +	struct amdgpu_fence_driver *drv = &ring->fence_drv;
> +
> +	last_seq = atomic_read(&ring->fence_drv.last_seq);
> +
> +	do {
> +		seq = amdgpu_fence_read(ring);
> +
> +		if (unlikely(seq == last_seq))
> +			break;

That doesn't look correct to me, it will abort the busy wait as soon as 
we see the last value we have seen.

> +		if (seq >= wait_seq && wait_seq >= last_seq)
> +			break;
> +		if (seq <= last_seq &&
> +		    (wait_seq <= seq || wait_seq >= last_seq))
> +			break;

Why not just "(int32_t)(wait_seq - seq) > 0" ? IIRC that should be 
sufficient for a wrap around test.

Regards,
Christian.

> +		udelay(5);
> +		timeout -= 5;
> +	} while (timeout > 0);
> +
> +	atomic_cmpxchg(&drv->last_seq, last_seq, wait_seq);
> +
> +	return timeout;
> +}
>   /**
>    * amdgpu_fence_count_emitted - get the count of emitted fences
>    *
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 4f6c68f..e5a9077 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -185,7 +185,7 @@ int amdgpu_gfx_kiq_init_ring(struct amdgpu_device *adev,
>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
>   	int r = 0;
>   
> -	mutex_init(&kiq->ring_mutex);
> +	spin_lock_init(&kiq->ring_lock);
>   
>   	r = amdgpu_wb_get(adev, &adev->virt.reg_val_offs);
>   	if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index af8e544..9de89ea 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -89,8 +89,12 @@ int amdgpu_fence_driver_start_ring(struct amdgpu_ring *ring,
>   void amdgpu_fence_driver_suspend(struct amdgpu_device *adev);
>   void amdgpu_fence_driver_resume(struct amdgpu_device *adev);
>   int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **fence);
> +int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s);
>   void amdgpu_fence_process(struct amdgpu_ring *ring);
>   int amdgpu_fence_wait_empty(struct amdgpu_ring *ring);
> +signed long amdgpu_fence_wait_polling(struct amdgpu_ring *ring,
> +				      uint32_t wait_seq,
> +				      signed long timeout);
>   unsigned amdgpu_fence_count_emitted(struct amdgpu_ring *ring);
>   
>   /*
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index ab05121..177fe10 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -22,7 +22,7 @@
>    */
>   
>   #include "amdgpu.h"
> -#define MAX_KIQ_REG_WAIT	100000
> +#define MAX_KIQ_REG_WAIT	100000000 /* in usecs */
>   
>   int amdgpu_allocate_static_csa(struct amdgpu_device *adev)
>   {
> @@ -114,27 +114,24 @@ void amdgpu_virt_init_setting(struct amdgpu_device *adev)
>   uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
>   {
>   	signed long r;
> -	uint32_t val;
> -	struct dma_fence *f;
> +	uint32_t val, seq;
>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
>   	struct amdgpu_ring *ring = &kiq->ring;
>   
>   	BUG_ON(!ring->funcs->emit_rreg);
>   
> -	mutex_lock(&kiq->ring_mutex);
> +	spin_lock(&kiq->ring_lock);
>   	amdgpu_ring_alloc(ring, 32);
>   	amdgpu_ring_emit_rreg(ring, reg);
> -	amdgpu_fence_emit(ring, &f);
> +	amdgpu_fence_emit_polling(ring, &seq);
>   	amdgpu_ring_commit(ring);
> -	mutex_unlock(&kiq->ring_mutex);
> +	spin_unlock(&kiq->ring_lock);
>   
> -	r = dma_fence_wait_timeout(f, false, msecs_to_jiffies(MAX_KIQ_REG_WAIT));
> -	dma_fence_put(f);
> +	r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
>   	if (r < 1) {
> -		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
> +		DRM_ERROR("wait for kiq fence error: %ld\n", r);
>   		return ~0;
>   	}
> -
>   	val = adev->wb.wb[adev->virt.reg_val_offs];
>   
>   	return val;
> @@ -143,23 +140,22 @@ uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
>   void amdgpu_virt_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v)
>   {
>   	signed long r;
> -	struct dma_fence *f;
> +	uint32_t seq;
>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
>   	struct amdgpu_ring *ring = &kiq->ring;
>   
>   	BUG_ON(!ring->funcs->emit_wreg);
>   
> -	mutex_lock(&kiq->ring_mutex);
> +	spin_lock(&kiq->ring_lock);
>   	amdgpu_ring_alloc(ring, 32);
>   	amdgpu_ring_emit_wreg(ring, reg, v);
> -	amdgpu_fence_emit(ring, &f);
> +	amdgpu_fence_emit_polling(ring, &seq);
>   	amdgpu_ring_commit(ring);
> -	mutex_unlock(&kiq->ring_mutex);
> +	spin_unlock(&kiq->ring_lock);
>   
> -	r = dma_fence_wait_timeout(f, false, msecs_to_jiffies(MAX_KIQ_REG_WAIT));
> +	r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
>   	if (r < 1)
> -		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
> -	dma_fence_put(f);
> +		DRM_ERROR("wait for kiq fence error: %ld\n", r);
>   }
>   
>   /**


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

Re: [PATCH 3/3] drm/amdgpu: busywait KIQ register accessing v2

[Ding, Pixel]

On 17/10/2017, 4:20 PM, "Koenig, Christian" <Christian.Koenig@amd.com> wrote:

>Am 17.10.2017 um 08:37 schrieb Pixel Ding:
>> From: pding <Pixel.Ding@amd.com>
>>
>> Register accessing is performed when IRQ is disabled. Never sleep in
>> this function.
>>
>> Known issue: dead sleep in many use cases of index/data registers.
>>
>> v2: wrap polling fence functions. don't trigger IRQ for polling in
>> case of wrongly fence signal.
>>
>> Signed-off-by: pding <Pixel.Ding@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h               |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  4 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  4 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c        |  8 +---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c         | 53 +++++++++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c           |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h          |  4 ++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c          | 30 ++++++-------
>>   8 files changed, 78 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index ca212ef..cc06e62 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -885,7 +885,7 @@ struct amdgpu_mec {
>>   struct amdgpu_kiq {
>>   	u64			eop_gpu_addr;
>>   	struct amdgpu_bo	*eop_obj;
>> -	struct mutex		ring_mutex;
>> +	spinlock_t              ring_lock;
>>   	struct amdgpu_ring	ring;
>>   	struct amdgpu_irq_src	irq;
>>   };
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
>> index 9d9965d..6d83573 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
>> @@ -788,7 +788,7 @@ static int invalidate_tlbs_with_kiq(struct amdgpu_device *adev, uint16_t pasid)
>>   	struct dma_fence *f;
>>   	struct amdgpu_ring *ring = &adev->gfx.kiq.ring;
>>   
>> -	mutex_lock(&adev->gfx.kiq.ring_mutex);
>> +	spin_lock(&adev->gfx.kiq.ring_lock);
>>   	amdgpu_ring_alloc(ring, 12); /* fence + invalidate_tlbs package*/
>>   	amdgpu_ring_write(ring, PACKET3(PACKET3_INVALIDATE_TLBS, 0));
>>   	amdgpu_ring_write(ring,
>> @@ -796,7 +796,7 @@ static int invalidate_tlbs_with_kiq(struct amdgpu_device *adev, uint16_t pasid)
>>   			PACKET3_INVALIDATE_TLBS_PASID(pasid));
>>   	amdgpu_fence_emit(ring, &f);
>>   	amdgpu_ring_commit(ring);
>> -	mutex_unlock(&adev->gfx.kiq.ring_mutex);
>> +	spin_unlock(&adev->gfx.kiq.ring_lock);
>>   
>>   	r = dma_fence_wait(f, false);
>>   	if (r)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
>> index edbae19..c92217f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
>> @@ -973,7 +973,7 @@ static int invalidate_tlbs_with_kiq(struct amdgpu_device *adev, uint16_t pasid)
>>   	struct dma_fence *f;
>>   	struct amdgpu_ring *ring = &adev->gfx.kiq.ring;
>>   
>> -	mutex_lock(&adev->gfx.kiq.ring_mutex);
>> +	spin_lock(&adev->gfx.kiq.ring_lock);
>>   	amdgpu_ring_alloc(ring, 12); /* fence + invalidate_tlbs package*/
>>   	amdgpu_ring_write(ring, PACKET3(PACKET3_INVALIDATE_TLBS, 0));
>>   	amdgpu_ring_write(ring,
>> @@ -983,7 +983,7 @@ static int invalidate_tlbs_with_kiq(struct amdgpu_device *adev, uint16_t pasid)
>>   			PACKET3_INVALIDATE_TLBS_FLUSH_TYPE(2));
>>   	amdgpu_fence_emit(ring, &f);
>>   	amdgpu_ring_commit(ring);
>> -	mutex_unlock(&adev->gfx.kiq.ring_mutex);
>> +	spin_unlock(&adev->gfx.kiq.ring_lock);
>>   
>>   	r = dma_fence_wait(f, false);
>>   	if (r)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index ab8f0d6..1197274 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -109,10 +109,8 @@ uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
>>   {
>>   	uint32_t ret;
>>   
>> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev)) {
>> -		BUG_ON(in_interrupt());
>> +	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev))
>>   		return amdgpu_virt_kiq_rreg(adev, reg);
>> -	}
>>   
>>   	if ((reg * 4) < adev->rmmio_size && !(acc_flags & AMDGPU_REGS_IDX))
>>   		ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
>> @@ -137,10 +135,8 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
>>   		adev->last_mm_index = v;
>>   	}
>>   
>> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev)) {
>> -		BUG_ON(in_interrupt());
>> +	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev))
>>   		return amdgpu_virt_kiq_wreg(adev, reg, v);
>> -	}
>>   
>>   	if ((reg * 4) < adev->rmmio_size && !(acc_flags & AMDGPU_REGS_IDX))
>>   		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> index 688740e..68a5e90 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> @@ -169,6 +169,32 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f)
>>   }
>>   
>>   /**
>> + * amdgpu_fence_emit_polling - emit a fence on the requeste ring
>> + *
>> + * @ring: ring the fence is associated with
>> + * @s: resulting sequence number
>> + *
>> + * Emits a fence command on the requested ring (all asics).
>> + * Used For polling fence.
>> + * Returns 0 on success, -ENOMEM on failure.
>> + */
>> +int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s)
>> +{
>> +	uint32_t seq;
>> +
>> +	if (!s)
>> +		return -EINVAL;
>> +
>> +	seq = ++ring->fence_drv.sync_seq;
>> +	amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr,
>> +			       seq, AMDGPU_FENCE_FLAG_INT);
>> +
>> +	*s = seq;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>>    * amdgpu_fence_schedule_fallback - schedule fallback check
>>    *
>>    * @ring: pointer to struct amdgpu_ring
>> @@ -281,6 +307,33 @@ int amdgpu_fence_wait_empty(struct amdgpu_ring *ring)
>>   	return r;
>>   }
>>   
>
>Please add some documentation here that timeout is in usec.
[Pixel] sure.
>
>> +signed long amdgpu_fence_wait_polling(struct amdgpu_ring *ring,
>> +				      uint32_t wait_seq,
>> +				      signed long timeout)
>> +{
>> +	uint32_t seq, last_seq;
>> +	struct amdgpu_fence_driver *drv = &ring->fence_drv;
>> +
>> +	last_seq = atomic_read(&ring->fence_drv.last_seq);
>> +
>> +	do {
>> +		seq = amdgpu_fence_read(ring);
>> +
>> +		if (unlikely(seq == last_seq))
>> +			break;
>
>That doesn't look correct to me, it will abort the busy wait as soon as 
>we see the last value we have seen.
>[pixel]you’re right. My intention is to use the “seq" as latest submitted one(sync_seq), then it means all submitted seqs are notified. This condition could be removed.
>> +		if (seq >= wait_seq && wait_seq >= last_seq)
>> +			break;
>> +		if (seq <= last_seq &&
>> +		    (wait_seq <= seq || wait_seq >= last_seq))
>> +			break;
>
>Why not just "(int32_t)(wait_seq - seq) > 0" ? IIRC that should be 
>sufficient for a wrap around test.
>[pixel] seq could be larger (executed) or smaller (not yet) than wait_seq even without a wrap round, I think you mean "(int32_t)(last_seq - seq) > 0”
>it actually can know there’s a wrap around, but it still doesn't know if the waited seq is in the range between or not, we still need other conditions. Code here is to identify cases to break as:

===last_seq=====wait_seq====seq===
===wait_seq==seq========last_seq==
==seq==========last_seq==wait_seq=

Does it make sense?

Regards.
Pixel
>
>> +		udelay(5);
>> +		timeout -= 5;
>> +	} while (timeout > 0);
>> +
>> +	atomic_cmpxchg(&drv->last_seq, last_seq, wait_seq);
>> +
>> +	return timeout;
>> +}
>>   /**
>>    * amdgpu_fence_count_emitted - get the count of emitted fences
>>    *
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> index 4f6c68f..e5a9077 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> @@ -185,7 +185,7 @@ int amdgpu_gfx_kiq_init_ring(struct amdgpu_device *adev,
>>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
>>   	int r = 0;
>>   
>> -	mutex_init(&kiq->ring_mutex);
>> +	spin_lock_init(&kiq->ring_lock);
>>   
>>   	r = amdgpu_wb_get(adev, &adev->virt.reg_val_offs);
>>   	if (r)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> index af8e544..9de89ea 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> @@ -89,8 +89,12 @@ int amdgpu_fence_driver_start_ring(struct amdgpu_ring *ring,
>>   void amdgpu_fence_driver_suspend(struct amdgpu_device *adev);
>>   void amdgpu_fence_driver_resume(struct amdgpu_device *adev);
>>   int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **fence);
>> +int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s);
>>   void amdgpu_fence_process(struct amdgpu_ring *ring);
>>   int amdgpu_fence_wait_empty(struct amdgpu_ring *ring);
>> +signed long amdgpu_fence_wait_polling(struct amdgpu_ring *ring,
>> +				      uint32_t wait_seq,
>> +				      signed long timeout);
>>   unsigned amdgpu_fence_count_emitted(struct amdgpu_ring *ring);
>>   
>>   /*
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>> index ab05121..177fe10 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>> @@ -22,7 +22,7 @@
>>    */
>>   
>>   #include "amdgpu.h"
>> -#define MAX_KIQ_REG_WAIT	100000
>> +#define MAX_KIQ_REG_WAIT	100000000 /* in usecs */
>>   
>>   int amdgpu_allocate_static_csa(struct amdgpu_device *adev)
>>   {
>> @@ -114,27 +114,24 @@ void amdgpu_virt_init_setting(struct amdgpu_device *adev)
>>   uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
>>   {
>>   	signed long r;
>> -	uint32_t val;
>> -	struct dma_fence *f;
>> +	uint32_t val, seq;
>>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
>>   	struct amdgpu_ring *ring = &kiq->ring;
>>   
>>   	BUG_ON(!ring->funcs->emit_rreg);
>>   
>> -	mutex_lock(&kiq->ring_mutex);
>> +	spin_lock(&kiq->ring_lock);
>>   	amdgpu_ring_alloc(ring, 32);
>>   	amdgpu_ring_emit_rreg(ring, reg);
>> -	amdgpu_fence_emit(ring, &f);
>> +	amdgpu_fence_emit_polling(ring, &seq);
>>   	amdgpu_ring_commit(ring);
>> -	mutex_unlock(&kiq->ring_mutex);
>> +	spin_unlock(&kiq->ring_lock);
>>   
>> -	r = dma_fence_wait_timeout(f, false, msecs_to_jiffies(MAX_KIQ_REG_WAIT));
>> -	dma_fence_put(f);
>> +	r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
>>   	if (r < 1) {
>> -		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
>> +		DRM_ERROR("wait for kiq fence error: %ld\n", r);
>>   		return ~0;
>>   	}
>> -
>>   	val = adev->wb.wb[adev->virt.reg_val_offs];
>>   
>>   	return val;
>> @@ -143,23 +140,22 @@ uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
>>   void amdgpu_virt_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v)
>>   {
>>   	signed long r;
>> -	struct dma_fence *f;
>> +	uint32_t seq;
>>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
>>   	struct amdgpu_ring *ring = &kiq->ring;
>>   
>>   	BUG_ON(!ring->funcs->emit_wreg);
>>   
>> -	mutex_lock(&kiq->ring_mutex);
>> +	spin_lock(&kiq->ring_lock);
>>   	amdgpu_ring_alloc(ring, 32);
>>   	amdgpu_ring_emit_wreg(ring, reg, v);
>> -	amdgpu_fence_emit(ring, &f);
>> +	amdgpu_fence_emit_polling(ring, &seq);
>>   	amdgpu_ring_commit(ring);
>> -	mutex_unlock(&kiq->ring_mutex);
>> +	spin_unlock(&kiq->ring_lock);
>>   
>> -	r = dma_fence_wait_timeout(f, false, msecs_to_jiffies(MAX_KIQ_REG_WAIT));
>> +	r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
>>   	if (r < 1)
>> -		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
>> -	dma_fence_put(f);
>> +		DRM_ERROR("wait for kiq fence error: %ld\n", r);
>>   }
>>   
>>   /**
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amdgpu: busywait KIQ register accessing v2

[Christian König]

Am 17.10.2017 um 10:38 schrieb Ding, Pixel:
> [SNIP]
>>> +		if (seq >= wait_seq && wait_seq >= last_seq)
>>> +			break;
>>> +		if (seq <= last_seq &&
>>> +		    (wait_seq <= seq || wait_seq >= last_seq))
>>> +			break;
>> Why not just "(int32_t)(wait_seq - seq) > 0" ? IIRC that should be
>> sufficient for a wrap around test.
>> [pixel] seq could be larger (executed) or smaller (not yet) than wait_seq even without a wrap round, I think you mean "(int32_t)(last_seq - seq) > 0”
>> it actually can know there’s a wrap around, but it still doesn't know if the waited seq is in the range between or not, we still need other conditions. Code here is to identify cases to break as:
> ===last_seq=====wait_seq====seq===
> ===wait_seq==seq========last_seq==
> ==seq==========last_seq==wait_seq=
>
> Does it make sense?

No, let me explain a bit more. The full code I had in mind is this:

do {
     seq = amdgpu_fence_read(ring)
} while ((int32_t)(wait_seq - seq) > 0);

This should handle the following cases:
1. wait_seq is larger than seq, in this case we still need to wait.

(wait_seq - seq) is larger than zero and the loop continues.

2. wait_seq is smaller than seq, in this case the we can stop waiting.

(wait_seq -seq) is smaller or equal to zero and the loop stops.

3. wait_seq is much smaller than seq because of a wrap around, in this 
case we still need to wait.

(wait_seq - seq) is larger than zero because the substraction wraps 
around the numeric range as well and the look will continue.

4. wait_seq is much larger than seq because of a wrap around, in this 
case we can stop waiting.

(wait_seq - seq) is smaller than zero because the cast to a signed 
number makes it negative and the loop stops.

This is rather common code for wrap around testing and you don't need to 
fiddle with last_seq at all here.

If you want you can also use the __dma_fence_is_later() helper function 
which implements exactly that test as well.

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

Re: [PATCH 3/3] drm/amdgpu: busywait KIQ register accessing v2

[Ding, Pixel]

Thanks. In logically, it can’t handle the extreme case that the seq almost catches up with wait_seq in wrap round where we can’t use this trick, however I understand it can’t happen in reality.

I will test and modify.

— 
Sincerely Yours,
Pixel







On 17/10/2017, 4:59 PM, "Koenig, Christian" <Christian.Koenig@amd.com> wrote:

>Am 17.10.2017 um 10:38 schrieb Ding, Pixel:
>> [SNIP]
>>>> +		if (seq >= wait_seq && wait_seq >= last_seq)
>>>> +			break;
>>>> +		if (seq <= last_seq &&
>>>> +		    (wait_seq <= seq || wait_seq >= last_seq))
>>>> +			break;
>>> Why not just "(int32_t)(wait_seq - seq) > 0" ? IIRC that should be
>>> sufficient for a wrap around test.
>>> [pixel] seq could be larger (executed) or smaller (not yet) than wait_seq even without a wrap round, I think you mean "(int32_t)(last_seq - seq) > 0”
>>> it actually can know there’s a wrap around, but it still doesn't know if the waited seq is in the range between or not, we still need other conditions. Code here is to identify cases to break as:
>> ===last_seq=====wait_seq====seq===
>> ===wait_seq==seq========last_seq==
>> ==seq==========last_seq==wait_seq=
>>
>> Does it make sense?
>
>No, let me explain a bit more. The full code I had in mind is this:
>
>do {
>     seq = amdgpu_fence_read(ring)
>} while ((int32_t)(wait_seq - seq) > 0);
>
>This should handle the following cases:
>1. wait_seq is larger than seq, in this case we still need to wait.
>
>(wait_seq - seq) is larger than zero and the loop continues.
>
>2. wait_seq is smaller than seq, in this case the we can stop waiting.
>
>(wait_seq -seq) is smaller or equal to zero and the loop stops.
>
>3. wait_seq is much smaller than seq because of a wrap around, in this 
>case we still need to wait.
>
>(wait_seq - seq) is larger than zero because the substraction wraps 
>around the numeric range as well and the look will continue.
>
>4. wait_seq is much larger than seq because of a wrap around, in this 
>case we can stop waiting.
>
>(wait_seq - seq) is smaller than zero because the cast to a signed 
>number makes it negative and the loop stops.
>
>This is rather common code for wrap around testing and you don't need to 
>fiddle with last_seq at all here.
>
>If you want you can also use the __dma_fence_is_later() helper function 
>which implements exactly that test as well.
>
>Regards,
>Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx