Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
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 >> wrote: >>> Am 21.12.18 um 10:09 schrieb Deng, Emily: > From: Michel Dänzer > On 2018-12-21 9:45 a.m., Deng, Emily wrote: >>> From: Michel Dänzer >>> 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 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
Am 21.12.18 um 19:19 schrieb Alex Deucher: > On Fri, Dec 21, 2018 at 1:15 PM Christian König > wrote: >> Am 21.12.18 um 10:09 schrieb Deng, Emily: >>>> -Original Message- >>>> From: Michel Dänzer >>>> Sent: Friday, December 21, 2018 4:52 PM >>>> To: Deng, Emily >>>> Cc: amd-gfx@lists.freedesktop.org >>>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo >>>> >>>> On 2018-12-21 9:45 a.m., Deng, Emily wrote: >>>>>> -Original Message- >>>>>> From: Michel Dänzer >>>>>> Sent: Friday, December 21, 2018 4:38 PM >>>>>> To: Deng, Emily >>>>>> Cc: amd-gfx@lists.freedesktop.org >>>>>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo >>>>>> >>>>>> 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? For real hard we have at maximum two BOs pinned, the current one and the next one. For the virtual scanout I'm not sure about that. Christian. > > Alex ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
On Fri, Dec 21, 2018 at 1:15 PM Christian König wrote: > > Am 21.12.18 um 10:09 schrieb Deng, Emily: > >> -Original Message- > >> From: Michel Dänzer > >> Sent: Friday, December 21, 2018 4:52 PM > >> To: Deng, Emily > >> Cc: amd-gfx@lists.freedesktop.org > >> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo > >> > >> On 2018-12-21 9:45 a.m., Deng, Emily wrote: > >>>> -Original Message- > >>>> From: Michel Dänzer > >>>> Sent: Friday, December 21, 2018 4:38 PM > >>>> To: Deng, Emily > >>>> Cc: amd-gfx@lists.freedesktop.org > >>>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo > >>>> > >>>> 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. Alex > > Regards, > Christian. > > >> > >> -- > >> Earthling Michel Dänzer | http://www.amd.com > >> Libre software enthusiast | Mesa and X developer > > ___ > > amd-gfx mailing list > > amd-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > ___ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
Am 21.12.18 um 10:09 schrieb Deng, Emily: -Original Message- From: Michel Dänzer Sent: Friday, December 21, 2018 4:52 PM To: Deng, Emily Cc: amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo On 2018-12-21 9:45 a.m., Deng, Emily wrote: -Original Message- From: Michel Dänzer Sent: Friday, December 21, 2018 4:38 PM To: Deng, Emily Cc: amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo 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. Regards, Christian. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
On 2018-12-21 11:15 a.m., Deng, Emily wrote: >> -Original Message- >> From: Michel Dänzer >> Sent: Friday, December 21, 2018 6:08 PM >> To: Deng, Emily >> Cc: amd-gfx@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 On Behalf Of >>>> Deng, Emily >>>> Sent: Friday, December 21, 2018 5:41 PM >>>> To: Michel Dänzer >>>> Cc: amd-gfx@lists.freedesktop.org >>>> Subject: RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo >>>> >>>>> -Original Message----- >>>>> From: Michel Dänzer >>>>> Sent: Friday, December 21, 2018 5:37 PM >>>>> To: Deng, Emily >>>>> Cc: amd-gfx@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 >>>>>>> Sent: Friday, December 21, 2018 5:28 PM >>>>>>> To: Deng, Emily >>>>>>> Cc: amd-gfx@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 On Behalf >>>>>>>>> Of Deng, Emily >>>>>>>>>> From: Michel Dänzer On 2018-12-21 9:45 >>>>>>>>>> a.m., Deng, Emily wrote: >>>>>>>>>>>> From: Michel Dänzer 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 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>-Original Message- >From: Michel Dänzer >Sent: Friday, December 21, 2018 6:08 PM >To: Deng, Emily >Cc: amd-gfx@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 On Behalf Of >>> Deng, Emily >>> Sent: Friday, December 21, 2018 5:41 PM >>> To: Michel Dänzer >>> Cc: amd-gfx@lists.freedesktop.org >>> Subject: RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo >>> >>>> -Original Message- >>>> From: Michel Dänzer >>>> Sent: Friday, December 21, 2018 5:37 PM >>>> To: Deng, Emily >>>> Cc: amd-gfx@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 >>>>>> Sent: Friday, December 21, 2018 5:28 PM >>>>>> To: Deng, Emily >>>>>> Cc: amd-gfx@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 On Behalf >>>>>>>> Of Deng, Emily >>>>>>>>> From: Michel Dänzer On 2018-12-21 9:45 >>>>>>>>> a.m., Deng, Emily wrote: >>>>>>>>>>> From: Michel Dänzer 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 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
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 On Behalf Of Deng, >> Emily >> Sent: Friday, December 21, 2018 5:41 PM >> To: Michel Dänzer >> Cc: amd-gfx@lists.freedesktop.org >> Subject: RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo >> >>> -Original Message- >>> From: Michel Dänzer >>> Sent: Friday, December 21, 2018 5:37 PM >>> To: Deng, Emily >>> Cc: amd-gfx@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 >>>>> Sent: Friday, December 21, 2018 5:28 PM >>>>> To: Deng, Emily >>>>> Cc: amd-gfx@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 On Behalf Of >>>>>>> Deng, Emily >>>>>>>> From: Michel Dänzer On 2018-12-21 9:45 a.m., >>>>>>>> Deng, Emily wrote: >>>>>>>>>> From: Michel Dänzer 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 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>-Original Message- >From: amd-gfx On Behalf Of Deng, >Emily >Sent: Friday, December 21, 2018 5:41 PM >To: Michel Dänzer >Cc: amd-gfx@lists.freedesktop.org >Subject: RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo > >>-Original Message- >>From: Michel Dänzer >>Sent: Friday, December 21, 2018 5:37 PM >>To: Deng, Emily >>Cc: amd-gfx@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 >>>> Sent: Friday, December 21, 2018 5:28 PM >>>> To: Deng, Emily >>>> Cc: amd-gfx@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 On Behalf Of >>>>>> Deng, Emily >>>>>>> From: Michel Dänzer On 2018-12-21 9:45 a.m., >>>>>>> Deng, Emily wrote: >>>>>>>>> From: Michel Dänzer 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? 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. Which method do you think is better? >> >> >>-- >>Earthling Michel Dänzer | http://www.amd.com >>Libre software enthusiast | Mesa and X developer >___ >amd-gfx mailing list >amd-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>-Original Message- >From: Michel Dänzer >Sent: Friday, December 21, 2018 5:37 PM >To: Deng, Emily >Cc: amd-gfx@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 >>> Sent: Friday, December 21, 2018 5:28 PM >>> To: Deng, Emily >>> Cc: amd-gfx@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 On Behalf Of >>>>> Deng, Emily >>>>>> From: Michel Dänzer On 2018-12-21 9:45 a.m., >>>>>> Deng, Emily wrote: >>>>>>>> From: Michel Dänzer 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? > > >-- >Earthling Michel Dänzer | http://www.amd.com >Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
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 >> Sent: Friday, December 21, 2018 5:28 PM >> To: Deng, Emily >> Cc: amd-gfx@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 On Behalf Of >>>> Deng, Emily >>>>> From: Michel Dänzer On 2018-12-21 9:45 a.m., >>>>> Deng, Emily wrote: >>>>>>> From: Michel Dänzer 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. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>-Original Message- >From: Michel Dänzer >Sent: Friday, December 21, 2018 5:28 PM >To: Deng, Emily >Cc: amd-gfx@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 On Behalf Of >>> Deng, Emily >>>> From: Michel Dänzer On 2018-12-21 9:45 a.m., >>>> Deng, Emily wrote: >>>>>> From: Michel Dänzer 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? Best wishes Emily Deng > > >-- >Earthling Michel Dänzer | http://www.amd.com >Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
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 On Behalf Of Deng, >> Emily >>> From: Michel Dänzer >>> On 2018-12-21 9:45 a.m., Deng, Emily wrote: > From: Michel Dänzer > 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. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>-Original Message- >From: amd-gfx On Behalf Of Deng, >Emily >Sent: Friday, December 21, 2018 5:10 PM >To: Michel Dänzer >Cc: amd-gfx@lists.freedesktop.org >Subject: RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo > >>-Original Message- >>From: Michel Dänzer >>Sent: Friday, December 21, 2018 4:52 PM >>To: Deng, Emily >>Cc: amd-gfx@lists.freedesktop.org >>Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo >> >>On 2018-12-21 9:45 a.m., Deng, Emily wrote: >>>> -Original Message- >>>> From: Michel Dänzer >>>> Sent: Friday, December 21, 2018 4:38 PM >>>> To: Deng, Emily >>>> Cc: amd-gfx@lists.freedesktop.org >>>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo >>>> >>>> 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. So for code consistency, maybe still need to add the pin here. Best wishes Emily Deng >> >> >>-- >>Earthling Michel Dänzer | http://www.amd.com >>Libre software enthusiast | Mesa and X developer >___ >amd-gfx mailing list >amd-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>-Original Message- >From: Michel Dänzer >Sent: Friday, December 21, 2018 4:52 PM >To: Deng, Emily >Cc: amd-gfx@lists.freedesktop.org >Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo > >On 2018-12-21 9:45 a.m., Deng, Emily wrote: >>> -Original Message- >>> From: Michel Dänzer >>> Sent: Friday, December 21, 2018 4:38 PM >>> To: Deng, Emily >>> Cc: amd-gfx@lists.freedesktop.org >>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo >>> >>> 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. > > >-- >Earthling Michel Dänzer | http://www.amd.com >Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
On 2018-12-21 9:45 a.m., Deng, Emily wrote: >> -Original Message- >> From: Michel Dänzer >> Sent: Friday, December 21, 2018 4:38 PM >> To: Deng, Emily >> Cc: amd-gfx@lists.freedesktop.org >> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo >> >> 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? -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>-Original Message- >From: Michel Dänzer >Sent: Friday, December 21, 2018 4:38 PM >To: Deng, Emily >Cc: amd-gfx@lists.freedesktop.org >Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo > >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. Best wishes Emily Deng > >-- >Earthling Michel Dänzer | http://www.amd.com >Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
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. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
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.
>
> Signed-off-by: Emily Deng
>
> [...]
>
> @@ -235,6 +222,47 @@ static bool dce_virtual_crtc_mode_fixup(struct drm_crtc
> *crtc,
> static int dce_virtual_crtc_set_base(struct drm_crtc *crtc, int x, int y,
> struct drm_framebuffer *old_fb)
> {
> + struct drm_framebuffer *target_fb;
> + struct drm_gem_object *obj;
> + struct amdgpu_bo *abo;
> + int r;
> +
> + /* no fb bound */
> + if (!crtc->primary->fb) {
> + DRM_DEBUG_KMS("No FB bound\n");
> + return 0;
> + }
> +
> + target_fb = crtc->primary->fb;
> +
> + obj = kcl_drm_fb_get_gem_obj(target_fb, 0);
> + abo = gem_to_amdgpu_bo(obj);
> + r = amdgpu_bo_reserve(abo, false);
> + if (unlikely(r != 0))
> + return r;
> +
> + r = amdgpu_bo_pin(abo, AMDGPU_GEM_DOMAIN_VRAM);
> + if (unlikely(r != 0)) {
> + amdgpu_bo_unreserve(abo);
> + return -EINVAL;
> + }
> +
> + amdgpu_bo_unreserve(abo);
> + return 0;
> +}
Good catch, but old_fb also needs to be unpinned, otherwise the pinning
is unbalanced and a BO will always be pinned after it's scanned out once.
> +static int dce_virtual_crtc_mode_set(struct drm_crtc *crtc,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode,
> + int x, int y, struct drm_framebuffer *old_fb)
Indentation of the continuation lines here looks wrong (needs 3 more
spaces if I'm counting correctly). Would be good to fix that up while
you're moving the function. :)
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
