On Wed, Oct 18, 2017 at 11:09 PM, Ding, Pixel <Pixel.Ding at amd.com> wrote:
> Thanks Alex. Some comments inline.
> â??
> Sincerely Yours,
> Pixel
>
>
>
>
>
>
>
>
> On 19/10/2017, 10:34 AM, "Alex Deucher" <alexdeucher at gmail.com> wrote:
>
>>On Wed, Oct 18, 2017 at 9:44 PM, Pixel Ding <Pixel.Ding at amd.com> wrote:
>>> From: pding <Pixel.Ding at 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 at 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
