[PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Michel Dänzer]

On 2018-12-21 10:55 a.m., Deng, Emily wrote:
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Deng,
>> Emily
>> Sent: Friday, December 21, 2018 5:41 PM
>> To: Michel Dänzer <michel at daenzer.net>
>> Cc: amd-gfx at lists.freedesktop.org
>> Subject: RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>
>>> -----Original Message-----
>>> From: Michel Dänzer <michel at daenzer.net>
>>> Sent: Friday, December 21, 2018 5:37 PM
>>> To: Deng, Emily <Emily.Deng at amd.com>
>>> Cc: amd-gfx at lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>>
>>> On 2018-12-21 10:32 a.m., Deng, Emily wrote:
>>>>> -----Original Message-----
>>>>> From: Michel Dänzer <michel at daenzer.net>
>>>>> Sent: Friday, December 21, 2018 5:28 PM
>>>>> To: Deng, Emily <Emily.Deng at amd.com>
>>>>> Cc: amd-gfx at lists.freedesktop.org
>>>>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>>>>
>>>>> On 2018-12-21 10:17 a.m., Deng, Emily wrote:
>>>>>>> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
>>>>>>> Deng, Emily
>>>>>>>> From: Michel Dänzer <michel at daenzer.net> On 2018-12-21 9:45 a.m.,
>>>>>>>> Deng, Emily wrote:
>>>>>>>>>> From: Michel Dänzer <michel at daenzer.net> On 2018-12-21 8:26
>>>>>>>>>> a.m., Emily Deng wrote:
>>>>>>>>>>> When the bo is used to set mode, the bo need to be pinned.
>>>>>>>>>>
>>>>>>>>>> On second thought, why does the BO need to be pinned? When
>>>>>>>>>> using the display hardware, the BO needs to be pinned to
>>>>>>>>>> prevent it from being moved while the hardware is scanning out
>>>>>>>>>> from it, but that shouldn't be
>>>>>>>> necessary here.
>>>>>>>>> The pin here is used for scan out the buffer by remote display app.
>>>>>>>>
>>>>>>>> I still don't understand why pinning is needed. What mechanism
>>>>>>>> does the remote display app use to access the BO contents?
>>>>>>> Sorry, I am not familiar with the remote display app. Maybe it
>>>>>>> will use drm ioctl function to get the current crtc's fb's
>>>>>>> information, and get the content in the fb's buffer object by mmap
>>>>>>> or translate the bo to an OpenGL texture for next rendering. Maybe
>>>>>>> don't need to pin the bo here, as the use has no different with other
>> normal bos.
>>>>>>> So please ignore the patch, and will send another patch to remove
>>>>>>> the unpin
>>>>> the fb's bo code.
>>>>>> It seems to be hard to remove all the pin for virtual_dce, as it
>>>>>> uses some
>>>>> common code in amdgpu_display.c.
>>>>>
>>>>> Because of amdgpu_display_unpin_work_func? That might be as simple
>>>>> as replacing
>>>>>
>>>>> 	schedule_work(&works->unpin_work);
>>>>>
>>>>> with
>>>>>
>>>>> 	kfree(works->shared);
>>>>> 	kfree(works);
>>>>>
>>>>> in dce_virtual_pageflip.
>>>> But the amdgpu_display_crtc_page_flip_target will pin the new_bo,
>>>> then we don't need to unpin it?
>>>
>>> Ah, right, but then dce_virtual_pageflip could just unpin it? Not
>>> ideal, but better than leaving it pinned unnecessarily.
>> Yes, it is not a good idea to leave it pinned. Then will need lots of "if
>> (amdgpu_virtual_display)", don't know whether it could be accept?

Should rather be if (adev->enable_virtual_display). If you want to never
pin, it's probably worth giving this a shot and seeing how bad it gets.
BTW, amdgpu_ttm_alloc_gart can probably also be skipped for virtual
display, maybe more.


> Another method is let the logical stay no change, just use if (!amdgpu_virtual_display)
> before WARN_ON_ONCE of amdgpu_bo_unpin to remove the virtual_dce's call trace.

That would be ugly IMHO.


But none of that is necessary if dce_virtual_pageflip simply unpins the
BO and skips unpin_work.


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