[PATCH] drm/amdgpu: drm/amdgpu: always consider virualised device for checking post (v2)

[PATCH] drm/amdgpu: drm/amdgpu: always consider virualised device for checking post (v2)

[Pixel Ding]

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

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

v2: only change in IGP reading bios.

Signed-off-by: pding <Pixel.Ding@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c   | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 0280ae5..caabc5b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1840,6 +1840,7 @@ amdgpu_get_sdma_instance(struct amdgpu_ring *ring)
 /* Common functions */
 int amdgpu_gpu_reset(struct amdgpu_device *adev);
 bool amdgpu_need_backup(struct amdgpu_device *adev);
+bool amdgpu_vpost_needed(struct amdgpu_device *adev);
 void amdgpu_pci_config_reset(struct amdgpu_device *adev);
 bool amdgpu_need_post(struct amdgpu_device *adev);
 void amdgpu_update_display_priority(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
index c21adf6..25f43eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
@@ -99,7 +99,7 @@ static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
 	resource_size_t size = 256 * 1024; /* ??? */
 
 	if (!(adev->flags & AMD_IS_APU))
-		if (amdgpu_need_post(adev))
+		if (amdgpu_vpost_needed(adev))
 			return false;
 
 	adev->bios = NULL;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index a601d87..098cd44 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -764,7 +764,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
 
 }
 
-static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
+bool amdgpu_vpost_needed(struct amdgpu_device *adev)
 {
 	if (amdgpu_sriov_vf(adev))
 		return false;
-- 
2.9.5

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

Re: [PATCH] drm/amdgpu: drm/amdgpu: always consider virualised device for checking post (v2)

[Ding, Pixel]

Hi Alex,

This is the v2 patch as you suggested. Only fix the issue found in amdgpu_bios.c.

Please review, thanks.

— 
Sincerely Yours,
Pixel








On 19/10/2017, 9:44 AM, "amd-gfx on behalf of Pixel Ding" <amd-gfx-bounces@lists.freedesktop.org on behalf of Pixel.Ding@amd.com> wrote:

>From: pding <Pixel.Ding@amd.com>
>
>The post checking on scratch registers isn't reliable for virtual function.
>
>v2: only change in IGP reading bios.
>
>Signed-off-by: pding <Pixel.Ding@amd.com>
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c   | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> 3 files changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>index 0280ae5..caabc5b 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>@@ -1840,6 +1840,7 @@ amdgpu_get_sdma_instance(struct amdgpu_ring *ring)
> /* Common functions */
> int amdgpu_gpu_reset(struct amdgpu_device *adev);
> bool amdgpu_need_backup(struct amdgpu_device *adev);
>+bool amdgpu_vpost_needed(struct amdgpu_device *adev);
> void amdgpu_pci_config_reset(struct amdgpu_device *adev);
> bool amdgpu_need_post(struct amdgpu_device *adev);
> void amdgpu_update_display_priority(struct amdgpu_device *adev);
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
>index c21adf6..25f43eb 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
>@@ -99,7 +99,7 @@ static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
> 	resource_size_t size = 256 * 1024; /* ??? */
> 
> 	if (!(adev->flags & AMD_IS_APU))
>-		if (amdgpu_need_post(adev))
>+		if (amdgpu_vpost_needed(adev))
> 			return false;
> 
> 	adev->bios = NULL;
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>index a601d87..098cd44 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>@@ -764,7 +764,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
> 
> }
> 
>-static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>+bool amdgpu_vpost_needed(struct amdgpu_device *adev)
> {
> 	if (amdgpu_sriov_vf(adev))
> 		return false;
>-- 
>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] drm/amdgpu: drm/amdgpu: always consider virualised device for checking post (v2)

[Alex Deucher]

On Wed, Oct 18, 2017 at 9:44 PM, Pixel Ding <Pixel.Ding@amd.com> wrote:
> From: pding <Pixel.Ding@amd.com>
>
> The post checking on scratch registers isn't reliable for virtual function.
>
> v2: only change in IGP reading bios.


Subject has "drm/amdgpu: drm/amdgpu: " drop one of them.


>
> Signed-off-by: pding <Pixel.Ding@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c   | 2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
>  3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 0280ae5..caabc5b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1840,6 +1840,7 @@ amdgpu_get_sdma_instance(struct amdgpu_ring *ring)
>  /* Common functions */
>  int amdgpu_gpu_reset(struct amdgpu_device *adev);
>  bool amdgpu_need_backup(struct amdgpu_device *adev);
> +bool amdgpu_vpost_needed(struct amdgpu_device *adev);
>  void amdgpu_pci_config_reset(struct amdgpu_device *adev);
>  bool amdgpu_need_post(struct amdgpu_device *adev);

amdgpu_need_post can be dropped now.

>  void amdgpu_update_display_priority(struct amdgpu_device *adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> index c21adf6..25f43eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> @@ -99,7 +99,7 @@ static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
>         resource_size_t size = 256 * 1024; /* ??? */
>
>         if (!(adev->flags & AMD_IS_APU))
> -               if (amdgpu_need_post(adev))
> +               if (amdgpu_vpost_needed(adev))
>                         return false;
>
>         adev->bios = NULL;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index a601d87..098cd44 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -764,7 +764,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
>
>  }
>
> -static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
> +bool amdgpu_vpost_needed(struct amdgpu_device *adev)

Please also make amdgpu_need_post() static now.

Is there a way we can just merge amdgpu_need_post() into
amdgpu_vpost_needed()?  For bare metal it should be fine.  Might need
some logic adjustments for sr-iov amdgpu_device_resume().  That can be
a follow on patch if you want.

Alex


>  {
>         if (amdgpu_sriov_vf(adev))
>                 return false;
> --
> 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] drm/amdgpu: drm/amdgpu: always consider virualised device for checking post (v2)

[Ding, Pixel]

Thanks Alex. Some comments inline.
— 
Sincerely Yours,
Pixel








On 19/10/2017, 10:34 AM, "Alex Deucher" <alexdeucher@gmail.com> wrote:

>On Wed, Oct 18, 2017 at 9:44 PM, Pixel Ding <Pixel.Ding@amd.com> wrote:
>> From: pding <Pixel.Ding@amd.com>
>>
>> The post checking on scratch registers isn't reliable for virtual function.
>>
>> v2: only change in IGP reading bios.
>
>
>Subject has "drm/amdgpu: drm/amdgpu: " drop one of them.
>[Pixel] get it.
>
>
>>
>> Signed-off-by: pding <Pixel.Ding@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 1 +
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c   | 2 +-
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
>>  3 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 0280ae5..caabc5b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1840,6 +1840,7 @@ amdgpu_get_sdma_instance(struct amdgpu_ring *ring)
>>  /* Common functions */
>>  int amdgpu_gpu_reset(struct amdgpu_device *adev);
>>  bool amdgpu_need_backup(struct amdgpu_device *adev);
>> +bool amdgpu_vpost_needed(struct amdgpu_device *adev);
>>  void amdgpu_pci_config_reset(struct amdgpu_device *adev);
>>  bool amdgpu_need_post(struct amdgpu_device *adev);
>
>amdgpu_need_post can be dropped now.
[Pixel] some other functions still invoke the amdgpu_need_post(). Only if we change all to use amdgpu_vpost_needed(). that’s what the v1 patch did, however it’s confused. the only difference is that original patch uses amdgpu_need_post() function name instead of amdgpu_vpost_needed(), I thought it’s more generic.
>
>>  void amdgpu_update_display_priority(struct amdgpu_device *adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
>> index c21adf6..25f43eb 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
>> @@ -99,7 +99,7 @@ static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
>>         resource_size_t size = 256 * 1024; /* ??? */
>>
>>         if (!(adev->flags & AMD_IS_APU))
>> -               if (amdgpu_need_post(adev))
>> +               if (amdgpu_vpost_needed(adev))
>>                         return false;
>>
>>         adev->bios = NULL;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a601d87..098cd44 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -764,7 +764,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
>>
>>  }
>>
>> -static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>> +bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>
>Please also make amdgpu_need_post() static now.
>
>Is there a way we can just merge amdgpu_need_post() into
>amdgpu_vpost_needed()?  For bare metal it should be fine.  Might need
>some logic adjustments for sr-iov amdgpu_device_resume().  That can be
>a follow on patch if you want.
[Pixel] the original patch replaces all amdgpu_need_post() with amdgpu_vpost_needed(), then rename the amdgpu_vpost_needed() to amdgpu_need_post(), and amdgpu_need_post() to static amdgpu_check_post(). After these transform it looks unclear. By now I think we can go back to that one.

>
>Alex
>
>
>>  {
>>         if (amdgpu_sriov_vf(adev))
>>                 return false;
>> --
>> 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] drm/amdgpu: drm/amdgpu: always consider virualised device for checking post (v2)

[Alex Deucher]

On Wed, Oct 18, 2017 at 11:09 PM, Ding, Pixel <Pixel.Ding@amd.com> wrote:
> Thanks Alex. Some comments inline.
> —
> Sincerely Yours,
> Pixel
>
>
>
>
>
>
>
>
> On 19/10/2017, 10:34 AM, "Alex Deucher" <alexdeucher@gmail.com> wrote:
>
>>On Wed, Oct 18, 2017 at 9:44 PM, Pixel Ding <Pixel.Ding@amd.com> wrote:
>>> From: pding <Pixel.Ding@amd.com>
>>>
>>> The post checking on scratch registers isn't reliable for virtual function.
>>>
>>> v2: only change in IGP reading bios.
>>
>>
>>Subject has "drm/amdgpu: drm/amdgpu: " drop one of them.
>>[Pixel] get it.
>>
>>
>>>
>>> Signed-off-by: pding <Pixel.Ding@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 1 +
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c   | 2 +-
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
>>>  3 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 0280ae5..caabc5b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -1840,6 +1840,7 @@ amdgpu_get_sdma_instance(struct amdgpu_ring *ring)
>>>  /* Common functions */
>>>  int amdgpu_gpu_reset(struct amdgpu_device *adev);
>>>  bool amdgpu_need_backup(struct amdgpu_device *adev);
>>> +bool amdgpu_vpost_needed(struct amdgpu_device *adev);
>>>  void amdgpu_pci_config_reset(struct amdgpu_device *adev);
>>>  bool amdgpu_need_post(struct amdgpu_device *adev);
>>
>>amdgpu_need_post can be dropped now.
> [Pixel] some other functions still invoke the amdgpu_need_post(). Only if we change all to use amdgpu_vpost_needed(). that’s what the v1 patch did, however it’s confused. the only difference is that original patch uses amdgpu_need_post() function name instead of amdgpu_vpost_needed(), I thought it’s more generic.
>>
>>>  void amdgpu_update_display_priority(struct amdgpu_device *adev);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
>>> index c21adf6..25f43eb 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
>>> @@ -99,7 +99,7 @@ static bool igp_read_bios_from_vram(struct amdgpu_device *adev)
>>>         resource_size_t size = 256 * 1024; /* ??? */
>>>
>>>         if (!(adev->flags & AMD_IS_APU))
>>> -               if (amdgpu_need_post(adev))
>>> +               if (amdgpu_vpost_needed(adev))
>>>                         return false;
>>>
>>>         adev->bios = NULL;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index a601d87..098cd44 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -764,7 +764,7 @@ bool amdgpu_need_post(struct amdgpu_device *adev)
>>>
>>>  }
>>>
>>> -static bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>> +bool amdgpu_vpost_needed(struct amdgpu_device *adev)
>>
>>Please also make amdgpu_need_post() static now.
>>
>>Is there a way we can just merge amdgpu_need_post() into
>>amdgpu_vpost_needed()?  For bare metal it should be fine.  Might need
>>some logic adjustments for sr-iov amdgpu_device_resume().  That can be
>>a follow on patch if you want.
> [Pixel] the original patch replaces all amdgpu_need_post() with amdgpu_vpost_needed(), then rename the amdgpu_vpost_needed() to amdgpu_need_post(), and amdgpu_need_post() to static amdgpu_check_post(). After these transform it looks unclear. By now I think we can go back to that one.

I'm fine with that as long as it doesn't break anything.  The only
difference between the two functions is for virtualization.  I'm not
sure why we use one in some places and other in other places.  I'd
prefer to just merge them to avoid any confusion about when to use
which one.

Alex

>
>>
>>Alex
>>
>>
>>>  {
>>>         if (amdgpu_sriov_vf(adev))
>>>                 return false;
>>> --
>>> 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