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

Thu Oct 19 03:23:23 UTC 2017

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 at amd.com> wrote:

>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf
>> Of Christian König
>> Sent: Wednesday, October 18, 2017 3:23 AM
>> To: Ding, Pixel; Liu, Monk; amd-gfx at 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 at 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 at lists.freedesktop.org on behalf of Pixel.Ding at 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 at 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 at amd.com>; amd-gfx at lists.freedesktop.org;
>> Koenig, Christian <Christian.Koenig at amd.com>
>> >>>> Cc: Li, Bingley <Bingley.Li at amd.com>; Sun, Gary
>> <Gary.Sun at 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 at 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 at amd.com]
>> >>>>> Sent: 2017年10月17日 14:38
>> >>>>> To: amd-gfx at lists.freedesktop.org; Liu, Monk
>> <Monk.Liu at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>
>> >>>>> Cc: Li, Bingley <Bingley.Li at amd.com>; Sun, Gary
>> <Gary.Sun at amd.com>; Ding, Pixel <Pixel.Ding at amd.com>
>> >>>>> Subject: [PATCH 1/3] drm/amdgpu: always consider virualised device
>> for checking post
>> >>>>>
>> >>>>> From: pding <Pixel.Ding at amd.com>
>> >>>>>
>> >>>>> The post checking on scratch registers isn't reliable for virtual function.
>> >>>>>
>> >>>>> Signed-off-by: pding <Pixel.Ding at 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 at lists.freedesktop.org
>> >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> 
>> 
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx