[PATCH] drm/amd/display: assign fb_location only if bo is pinned

[S, Shirish]

On 10/25/2017 1:13 AM, Andrey Grodzovsky wrote:
>
>
> On 10/24/2017 12:06 PM, Michel Dänzer wrote:
>> On 24/10/17 06:00 PM, Andrey Grodzovsky wrote:
>>> On 10/24/2017 11:43 AM, Michel Dänzer wrote:
>>>> On 24/10/17 04:58 PM, Andrey Grodzovsky wrote:
>>>>> On 10/24/2017 10:36 AM, S, Shirish wrote:
>>>>>> On 10/24/2017 7:48 PM, Andrey Grodzovsky wrote:
>>>>>>> On 10/24/2017 09:51 AM, S, Shirish wrote:
>>>>>>>> From: Shirish S <shirish.s at amd.com>
>>>>>>>>
>>>>>>>> On some systems amdgpu_bo_gpu_offset seems to be
>>>>>>>> called before the BO is pinned, leading to page faults
>>>>>>>> as the GPU address is still subject to change.
>>>>>>>>
>>>>>>>> This patch fixes the above issue.
>>>>>>>>
>>>>>>>> Signed-off-by: Shirish S <shirish.s at amd.com>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 +++++--
>>>>>>>>     1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>> index 974ee97..b251b7e 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>> @@ -1786,8 +1786,11 @@ static int get_fb_info(const struct
>>>>>>>> amdgpu_framebuffer *amdgpu_fb,
>>>>>>>>             return r;
>>>>>>>>         }
>>>>>>>>     -    if (fb_location)
>>>>>>>> -        *fb_location = amdgpu_bo_gpu_offset(rbo);
>>>>>>>> +    r = amdgpu_bo_pin(rbo, AMDGPU_GEM_DOMAIN_VRAM, fb_location);
>>>>>>>> +    if (unlikely(r != 0)) {
>>>>>>>> +        amdgpu_bo_unreserve(rbo);
>>>>>>>> +        return -EINVAL;
>>>>>>>> +    }
>>>>>>> Where the unpin would happen then ? Today DAL calls pin from
>>>>>>> dm_plane_helper_prepare_fb while the unpin from
>>>>>>> dm_plane_helper_cleanup_fb.
>>>>>>>
>>>>>> Good point, am trying to address the issue Michel is facing, if this
>>>>>> patch works, we can think of unpinning the bo appropriately.
>>>>>> Ideally it should be at the end of amdgpu_dm_atomic_check(). Can you
>>>>>> think of any better place?
>>>>> Sounds right, you have to unpin within atomic_check since UMD 
>>>>> might call
>>>>> check without commit sometimes.
>>>> Not sure it's appropriate to pin BOs from atomic_check.
>>>>
>>>> In general, BOs should be pinned just before the display hardware 
>>>> starts
>>>> accessing them, and unpinned just after it stops accessing them.
>>> Not sure what other option (unless moving fill_plane_attributes_from_fb
>>> to commit)
>> If that is possible, it sounds like the proper solution.
>>
>>> since new fb parameters are needed in check to build the new plane 
>>> state.
>> Is the BO's MC address needed for the new plane state though?
>
> Yea, in general it seems tiling flags and address stuff can be 
> deferred to
> commit since they are really only needed for actual programming
> and by this skipping the need to call get_fb_info during check.
> The only problem is no sanity can be done on them in that case.
>
Andrey,
Till now the get_fb_info() never calculated fb_location(as addrReq was 
always false), but since now its required to program the high part of 
the address structure,
can you move it back into the commit scope and ensure that fb_location 
is calculated everytime.(I have limited knowledge behind why it was 
moved out)
And in case if you insist on adding pin & unpin in atomic_check() scope 
i can post a patch for it.
Regards,
Shirish S

> Thanks,
> Andrey
>
>>
>>
>>> Before we did upstream refactoring get_fb_info was always called called
>>> from atomic_commit so it was within pin/unpin scope. Moving it out into
>>> check we missed the problem Shirish just found.
>> Actually, I found it. :)
>>
>>> We can't do pin in beginning of check and unpin in the end of commit
>>> because as i said, sometimes check is called w\o commit, [...]
>> That also means pinning and unpinning within check isn't a good
>> solution, since pinning might involve moving the buffer, which could be
>> a waste of effort if the commit never follows.
>>
>>
>