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

[Michel Dänzer]

On 2018-12-21 7:23 p.m., Koenig, Christian wrote:
> Am 21.12.18 um 19:19 schrieb Alex Deucher:
>> On Fri, Dec 21, 2018 at 1:15 PM Christian König
>> <ckoenig.leichtzumerken at gmail.com> wrote:
>>> Am 21.12.18 um 10:09 schrieb 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.
>>> Opening the BO and doing something with it in OpenGL should result in
>>> proper fencing of the BO, so this sounds like a workaround for a bug
>>> somewhere else.
>>>
>>> As long as this isn't explained further this patch is certainly a NAK.
>>>
>>> Pinning for physical displays is allowed because they are limited in
>>> number, but this is not necessary the case with a virtual output.
>> Well practically, it's the same as real displays.  We limit the
>> virtual displays to 6 just like most hw.  It really should overate the
>> same.  We just don't need the pinning per se because there is no hw
>> actively scanning out of it.
> 
> Yeah, but do we limit the number of pending flips like we do for real 
> hardware as well?

Yeah, this is enforced in common code.

The issue this patch addressed was that it's also common code which
pinned the new BO and unpinned the old one on a page flip, but the
virtual display specific code otherwise didn't (un)pin BOs for
"scanout". This resulted in triggering the WARN I added recently for the
old BO on the first flip, because it wasn't pinned before, and in the
new BO of the last flip staying pinned indefinitely.

The result of this patch would be BOs getting (un)pinned for "scanout"
exactly the same way as for real HW scanout. Emily has now landed a
better patch which instead results in BOs never getting pinned for
virtual display "scanout".


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