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

[Michel Dänzer]

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?


> 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.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer