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

Submitted by Deng, Emily on Dec. 21, 2018, 7:26 a.m.

Details

Message ID 1545377209-10718-1-git-send-email-Emily.Deng@amd.com
State New
Headers show
Series "drm/amdgpu/virtual_dce: Need to pin the fb's bo" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Deng, Emily Dec. 21, 2018, 7:26 a.m.
When the bo is used to set mode, the bo need to be pinned.

Signed-off-by: Emily Deng <Emily.Deng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/dce_virtual.c | 54 ++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_virtual.c b/drivers/gpu/drm/amd/amdgpu/dce_virtual.c
index 2d68181..77752e3 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_virtual.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_virtual.c
@@ -211,19 +211,6 @@  static void dce_virtual_crtc_disable(struct drm_crtc *crtc)
 	amdgpu_crtc->connector = NULL;
 }
 
-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)
-{
-	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
-
-	/* update the hw version fpr dpm */
-	amdgpu_crtc->hw_mode = *adjusted_mode;
-
-	return 0;
-}
-
 static bool dce_virtual_crtc_mode_fixup(struct drm_crtc *crtc,
 				     const struct drm_display_mode *mode,
 				     struct drm_display_mode *adjusted_mode)
@@ -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;
+}
+
+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)
+{
+	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
+
+	dce_virtual_crtc_set_base(crtc, x, y, old_fb);
+
+	/* update the hw version fpr dpm */
+	amdgpu_crtc->hw_mode = *adjusted_mode;
+
 	return 0;
 }
 

Comments

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 <Emily.Deng@amd.com>
> 
> [...]
>  
> @@ -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. :)
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.
>-----Original Message-----

>From: Michel Dänzer <michel@daenzer.net>

>Sent: Friday, December 21, 2018 4:38 PM

>To: Deng, Emily <Emily.Deng@amd.com>

>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
On 2018-12-21 9:45 a.m., Deng, Emily wrote:
>> -----Original Message-----
>> From: Michel Dänzer <michel@daenzer.net>
>> Sent: Friday, December 21, 2018 4:38 PM
>> To: Deng, Emily <Emily.Deng@amd.com>
>> 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?
>-----Original Message-----

>From: Michel Dänzer <michel@daenzer.net>

>Sent: Friday, December 21, 2018 4:52 PM

>To: Deng, Emily <Emily.Deng@amd.com>

>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 <michel@daenzer.net>

>>> Sent: Friday, December 21, 2018 4:38 PM

>>> To: Deng, Emily <Emily.Deng@amd.com>

>>> 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
>-----Original Message-----

>From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Deng,

>Emily

>Sent: Friday, December 21, 2018 5:10 PM

>To: Michel Dänzer <michel@daenzer.net>

>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 <michel@daenzer.net>

>>Sent: Friday, December 21, 2018 4:52 PM

>>To: Deng, Emily <Emily.Deng@amd.com>

>>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 <michel@daenzer.net>

>>>> Sent: Friday, December 21, 2018 4:38 PM

>>>> To: Deng, Emily <Emily.Deng@amd.com>

>>>> 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
On 2018-12-21 10:17 a.m., Deng, Emily wrote:
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Deng,
>> Emily
>>> From: Michel Dänzer <michel@daenzer.net>
>>> On 2018-12-21 9:45 a.m., Deng, Emily wrote:
>>>>> From: Michel Dänzer <michel@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.
>-----Original Message-----

>From: Michel Dänzer <michel@daenzer.net>

>Sent: Friday, December 21, 2018 5:28 PM

>To: Deng, Emily <Emily.Deng@amd.com>

>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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of

>>> Deng, Emily

>>>> From: Michel Dänzer <michel@daenzer.net> On 2018-12-21 9:45 a.m.,

>>>> Deng, Emily wrote:

>>>>>> From: Michel Dänzer <michel@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?

Best wishes
Emily Deng


>

>

>--

>Earthling Michel Dänzer               |               http://www.amd.com

>Libre software enthusiast             |             Mesa and X developer
On 2018-12-21 10:32 a.m., Deng, Emily wrote:
>> -----Original Message-----
>> From: Michel Dänzer <michel@daenzer.net>
>> Sent: Friday, December 21, 2018 5:28 PM
>> To: Deng, Emily <Emily.Deng@amd.com>
>> 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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>>> Deng, Emily
>>>>> From: Michel Dänzer <michel@daenzer.net> On 2018-12-21 9:45 a.m.,
>>>>> Deng, Emily wrote:
>>>>>>> From: Michel Dänzer <michel@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.
>-----Original Message-----

>From: Michel Dänzer <michel@daenzer.net>

>Sent: Friday, December 21, 2018 5:37 PM

>To: Deng, Emily <Emily.Deng@amd.com>

>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 <michel@daenzer.net>

>>> Sent: Friday, December 21, 2018 5:28 PM

>>> To: Deng, Emily <Emily.Deng@amd.com>

>>> 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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of

>>>>> Deng, Emily

>>>>>> From: Michel Dänzer <michel@daenzer.net> On 2018-12-21 9:45 a.m.,

>>>>>> Deng, Emily wrote:

>>>>>>>> From: Michel Dänzer <michel@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?
>

>

>--

>Earthling Michel Dänzer               |               http://www.amd.com

>Libre software enthusiast             |             Mesa and X developer
>-----Original Message-----

>From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Deng,

>Emily

>Sent: Friday, December 21, 2018 5:41 PM

>To: Michel Dänzer <michel@daenzer.net>

>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 <michel@daenzer.net>

>>Sent: Friday, December 21, 2018 5:37 PM

>>To: Deng, Emily <Emily.Deng@amd.com>

>>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 <michel@daenzer.net>

>>>> Sent: Friday, December 21, 2018 5:28 PM

>>>> To: Deng, Emily <Emily.Deng@amd.com>

>>>> 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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of

>>>>>> Deng, Emily

>>>>>>> From: Michel Dänzer <michel@daenzer.net> On 2018-12-21 9:45 a.m.,

>>>>>>> Deng, Emily wrote:

>>>>>>>>> From: Michel Dänzer <michel@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?

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
On 2018-12-21 10:55 a.m., Deng, Emily wrote:
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Deng,
>> Emily
>> Sent: Friday, December 21, 2018 5:41 PM
>> To: Michel Dänzer <michel@daenzer.net>
>> 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 <michel@daenzer.net>
>>> Sent: Friday, December 21, 2018 5:37 PM
>>> To: Deng, Emily <Emily.Deng@amd.com>
>>> 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 <michel@daenzer.net>
>>>>> Sent: Friday, December 21, 2018 5:28 PM
>>>>> To: Deng, Emily <Emily.Deng@amd.com>
>>>>> 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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>>>>>> Deng, Emily
>>>>>>>> From: Michel Dänzer <michel@daenzer.net> On 2018-12-21 9:45 a.m.,
>>>>>>>> Deng, Emily wrote:
>>>>>>>>>> From: Michel Dänzer <michel@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.
>-----Original Message-----

>From: Michel Dänzer <michel@daenzer.net>

>Sent: Friday, December 21, 2018 6:08 PM

>To: Deng, Emily <Emily.Deng@amd.com>

>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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of

>>> Deng, Emily

>>> Sent: Friday, December 21, 2018 5:41 PM

>>> To: Michel Dänzer <michel@daenzer.net>

>>> 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 <michel@daenzer.net>

>>>> Sent: Friday, December 21, 2018 5:37 PM

>>>> To: Deng, Emily <Emily.Deng@amd.com>

>>>> 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 <michel@daenzer.net>

>>>>>> Sent: Friday, December 21, 2018 5:28 PM

>>>>>> To: Deng, Emily <Emily.Deng@amd.com>

>>>>>> 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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf

>>>>>>>> Of Deng, Emily

>>>>>>>>> From: Michel Dänzer <michel@daenzer.net> On 2018-12-21 9:45

>>>>>>>>> a.m., Deng, Emily wrote:

>>>>>>>>>>> From: Michel Dänzer <michel@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
On 2018-12-21 11:15 a.m., Deng, Emily wrote:
>> -----Original Message-----
>> From: Michel Dänzer <michel@daenzer.net>
>> Sent: Friday, December 21, 2018 6:08 PM
>> To: Deng, Emily <Emily.Deng@amd.com>
>> 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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>>> Deng, Emily
>>>> Sent: Friday, December 21, 2018 5:41 PM
>>>> To: Michel Dänzer <michel@daenzer.net>
>>>> 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 <michel@daenzer.net>
>>>>> Sent: Friday, December 21, 2018 5:37 PM
>>>>> To: Deng, Emily <Emily.Deng@amd.com>
>>>>> 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 <michel@daenzer.net>
>>>>>>> Sent: Friday, December 21, 2018 5:28 PM
>>>>>>> To: Deng, Emily <Emily.Deng@amd.com>
>>>>>>> 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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf
>>>>>>>>> Of Deng, Emily
>>>>>>>>>> From: Michel Dänzer <michel@daenzer.net> On 2018-12-21 9:45
>>>>>>>>>> a.m., Deng, Emily wrote:
>>>>>>>>>>>> From: Michel Dänzer <michel@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.
Am 21.12.18 um 10:09 schrieb Deng, Emily:
>> -----Original Message-----
>> From: Michel Dänzer <michel@daenzer.net>
>> Sent: Friday, December 21, 2018 4:52 PM
>> To: Deng, Emily <Emily.Deng@amd.com>
>> 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 <michel@daenzer.net>
>>>> Sent: Friday, December 21, 2018 4:38 PM
>>>> To: Deng, Emily <Emily.Deng@amd.com>
>>>> 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
On Fri, Dec 21, 2018 at 1:15 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 21.12.18 um 10:09 schrieb Deng, Emily:
> >> -----Original Message-----
> >> From: Michel Dänzer <michel@daenzer.net>
> >> Sent: Friday, December 21, 2018 4:52 PM
> >> To: Deng, Emily <Emily.Deng@amd.com>
> >> 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 <michel@daenzer.net>
> >>>> Sent: Friday, December 21, 2018 4:38 PM
> >>>> To: Deng, Emily <Emily.Deng@amd.com>
> >>>> 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
Am 21.12.18 um 19:19 schrieb Alex Deucher:
> On Fri, Dec 21, 2018 at 1:15 PM Christian König

> <ckoenig.leichtzumerken@gmail.com> wrote:

>> Am 21.12.18 um 10:09 schrieb Deng, Emily:

>>>> -----Original Message-----

>>>> From: Michel Dänzer <michel@daenzer.net>

>>>> Sent: Friday, December 21, 2018 4:52 PM

>>>> To: Deng, Emily <Emily.Deng@amd.com>

>>>> 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 <michel@daenzer.net>

>>>>>> Sent: Friday, December 21, 2018 4:38 PM

>>>>>> To: Deng, Emily <Emily.Deng@amd.com>

>>>>>> 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
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@gmail.com> wrote:
>>> Am 21.12.18 um 10:09 schrieb Deng, Emily:
>>>>> From: Michel Dänzer <michel@daenzer.net>
>>>>> On 2018-12-21 9:45 a.m., Deng, Emily wrote:
>>>>>>> From: Michel Dänzer <michel@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".