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

[Michel Dänzer]

On 2018-12-21 11:15 a.m., Deng, Emily wrote:
>> -----Original Message-----
>> From: Michel Dänzer <michel at daenzer.net>
>> Sent: Friday, December 21, 2018 6:08 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: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.
> Ok, then I will try, but the code may be ugly.
>
> [...]
>
>> But none of that is necessary if dce_virtual_pageflip simply unpins the BO and
>> skips unpin_work.
> Yes, but it will still have the issue that it won't unpin the bo which is pinned by amdgpu_display_crtc_page_flip_target.

I mean dce_virtual_pageflip unpinning precisely the BO pinned by
amdgpu_display_crtc_page_flip_target.


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