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

[Deng, Emily]

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

Best wishes
Emily Deng


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