[PATCH] drm/exynos: fix plane-framebuffer linkage

[PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Drake]

Pageflipping currently causes some inconsistencies that lead to
crashes. Just run an app that causes a CRTC pageflip in a raw X session
and check that it exits cleanly and can be restarted - you'll see
crashes like:
 Unable to handle kernel NULL pointer dereference at virtual address 00000334
 PC is at exynos_drm_crtc_plane_commit+0x20/0x40
 LR is at exynos_drm_crtc_plane_commit+0x20/0x40
 [<c03749b4>] (exynos_drm_crtc_plane_commit) from [<c03741bc>] (exynos_drm_crtc_commit+0x44/0x70)
 [<c03741bc>] (exynos_drm_crtc_commit) from [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4)
 [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2) from [<c03744f4>] (exynos_drm_crtc_page_flip+0x140/0x1a8)
 [<c03744f4>] (exynos_drm_crtc_page_flip) from [<c036b20c>] (drm_mode_page_flip_ioctl+0x224/0x2dc)
 [<c036b20c>] (drm_mode_page_flip_ioctl) from [<c035c324>] (drm_ioctl+0x338/0x4fc)

These crashes happen because drm_plane_force_disable has previously set
plane->crtc to NULL.

When drm_mode_page_flip_ioctl() is used to flip another framebuffer
onto the primary plane, crtc->primary->fb is correctly updated (this is
a virtual plane created by plane_helper), but plane->fb is not (this
plane is the real one, created by exynos_drm_crtc_create).

We then come to handle rmfb of the backbuffer, which the "real" primary
plane is incorrectly pointing at. So drm_framebuffer_remove() decides that
the buffer is actually active on a plane and force-disables the plane.

Ensuring that plane->fb is kept up-to-date solves that issue, but
exposes a reference counting problem. Now we see crashes when rmfb is
called on the front-buffer, because the rmfb code expects to drop 3
references here, and there are only 2.

That can be fixed by adopting the reference management found in omapdrm:
Framebuffer references are not taken directly in crtc mode_set context,
but rather in the context of updating the plane, which also covers
flips. Like omapdrm we also unreference the old framebuffer here.

Signed-off-by: Daniel Drake <drake@endlessm.com>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 12 ++----------
 drivers/gpu/drm/exynos/exynos_drm_plane.c |  8 ++++++++
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index b68e58f..7aa9dee 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -140,16 +140,8 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
 	if (manager->ops->mode_set)
 		manager->ops->mode_set(manager, &crtc->mode);
 
-	ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
-				    x, y, crtc_w, crtc_h);
-	if (ret)
-		return ret;
-
-	plane->crtc = crtc;
-	plane->fb = crtc->primary->fb;
-	drm_framebuffer_reference(plane->fb);
-
-	return 0;
+	return exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0,
+				     crtc_w, crtc_h, x, y, crtc_w, crtc_h);
 }
 
 static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index 8371cbd..df27e35 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -139,6 +139,14 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
 			overlay->crtc_x, overlay->crtc_y,
 			overlay->crtc_width, overlay->crtc_height);
 
+	if (plane->fb)
+		drm_framebuffer_unreference(plane->fb);
+
+	drm_framebuffer_reference(fb);
+
+	plane->fb = fb;
+	plane->crtc = crtc;
+
 	exynos_drm_crtc_plane_mode_set(crtc, overlay);
 
 	return 0;
-- 
1.9.1

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Vetter]

On Mon, Sep 15, 2014 at 12:52:17PM -0600, Daniel Drake wrote:
> Pageflipping currently causes some inconsistencies that lead to
> crashes. Just run an app that causes a CRTC pageflip in a raw X session
> and check that it exits cleanly and can be restarted - you'll see
> crashes like:
>  Unable to handle kernel NULL pointer dereference at virtual address 00000334
>  PC is at exynos_drm_crtc_plane_commit+0x20/0x40
>  LR is at exynos_drm_crtc_plane_commit+0x20/0x40
>  [<c03749b4>] (exynos_drm_crtc_plane_commit) from [<c03741bc>] (exynos_drm_crtc_commit+0x44/0x70)
>  [<c03741bc>] (exynos_drm_crtc_commit) from [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4)
>  [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2) from [<c03744f4>] (exynos_drm_crtc_page_flip+0x140/0x1a8)
>  [<c03744f4>] (exynos_drm_crtc_page_flip) from [<c036b20c>] (drm_mode_page_flip_ioctl+0x224/0x2dc)
>  [<c036b20c>] (drm_mode_page_flip_ioctl) from [<c035c324>] (drm_ioctl+0x338/0x4fc)
> 
> These crashes happen because drm_plane_force_disable has previously set
> plane->crtc to NULL.
> 
> When drm_mode_page_flip_ioctl() is used to flip another framebuffer
> onto the primary plane, crtc->primary->fb is correctly updated (this is
> a virtual plane created by plane_helper), but plane->fb is not (this
> plane is the real one, created by exynos_drm_crtc_create).
> 
> We then come to handle rmfb of the backbuffer, which the "real" primary
> plane is incorrectly pointing at. So drm_framebuffer_remove() decides that
> the buffer is actually active on a plane and force-disables the plane.
> 
> Ensuring that plane->fb is kept up-to-date solves that issue, but
> exposes a reference counting problem. Now we see crashes when rmfb is
> called on the front-buffer, because the rmfb code expects to drop 3
> references here, and there are only 2.
> 
> That can be fixed by adopting the reference management found in omapdrm:
> Framebuffer references are not taken directly in crtc mode_set context,
> but rather in the context of updating the plane, which also covers
> flips. Like omapdrm we also unreference the old framebuffer here.
> 
> Signed-off-by: Daniel Drake <drake@endlessm.com>

This sounds very much like exynos should switch to universal planes so
that the fake primary plane created by the helpers doesn't get in the way.
And for chips which already use planes for everything internally this
shouldn't be a lot more than a few lines.
-Daniel

> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 12 ++----------
>  drivers/gpu/drm/exynos/exynos_drm_plane.c |  8 ++++++++
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index b68e58f..7aa9dee 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -140,16 +140,8 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>  	if (manager->ops->mode_set)
>  		manager->ops->mode_set(manager, &crtc->mode);
>  
> -	ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
> -				    x, y, crtc_w, crtc_h);
> -	if (ret)
> -		return ret;
> -
> -	plane->crtc = crtc;
> -	plane->fb = crtc->primary->fb;
> -	drm_framebuffer_reference(plane->fb);
> -
> -	return 0;
> +	return exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0,
> +				     crtc_w, crtc_h, x, y, crtc_w, crtc_h);
>  }
>  
>  static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> index 8371cbd..df27e35 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> @@ -139,6 +139,14 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
>  			overlay->crtc_x, overlay->crtc_y,
>  			overlay->crtc_width, overlay->crtc_height);
>  
> +	if (plane->fb)
> +		drm_framebuffer_unreference(plane->fb);
> +
> +	drm_framebuffer_reference(fb);
> +
> +	plane->fb = fb;
> +	plane->crtc = crtc;
> +
>  	exynos_drm_crtc_plane_mode_set(crtc, overlay);
>  
>  	return 0;
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Andrzej Hajda]

Hi,

On 09/16/2014 08:35 AM, Daniel Vetter wrote:
> On Mon, Sep 15, 2014 at 12:52:17PM -0600, Daniel Drake wrote:
>> Pageflipping currently causes some inconsistencies that lead to
>> crashes. Just run an app that causes a CRTC pageflip in a raw X session
>> and check that it exits cleanly and can be restarted - you'll see
>> crashes like:
>>  Unable to handle kernel NULL pointer dereference at virtual address 00000334
>>  PC is at exynos_drm_crtc_plane_commit+0x20/0x40
>>  LR is at exynos_drm_crtc_plane_commit+0x20/0x40
>>  [<c03749b4>] (exynos_drm_crtc_plane_commit) from [<c03741bc>] (exynos_drm_crtc_commit+0x44/0x70)
>>  [<c03741bc>] (exynos_drm_crtc_commit) from [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4)
>>  [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2) from [<c03744f4>] (exynos_drm_crtc_page_flip+0x140/0x1a8)
>>  [<c03744f4>] (exynos_drm_crtc_page_flip) from [<c036b20c>] (drm_mode_page_flip_ioctl+0x224/0x2dc)
>>  [<c036b20c>] (drm_mode_page_flip_ioctl) from [<c035c324>] (drm_ioctl+0x338/0x4fc)
>>
>> These crashes happen because drm_plane_force_disable has previously set
>> plane->crtc to NULL.
>>
>> When drm_mode_page_flip_ioctl() is used to flip another framebuffer
>> onto the primary plane, crtc->primary->fb is correctly updated (this is
>> a virtual plane created by plane_helper), but plane->fb is not (this
>> plane is the real one, created by exynos_drm_crtc_create).
>>
>> We then come to handle rmfb of the backbuffer, which the "real" primary
>> plane is incorrectly pointing at. So drm_framebuffer_remove() decides that
>> the buffer is actually active on a plane and force-disables the plane.
>>
>> Ensuring that plane->fb is kept up-to-date solves that issue, but
>> exposes a reference counting problem. Now we see crashes when rmfb is
>> called on the front-buffer, because the rmfb code expects to drop 3
>> references here, and there are only 2.
>>
>> That can be fixed by adopting the reference management found in omapdrm:
>> Framebuffer references are not taken directly in crtc mode_set context,
>> but rather in the context of updating the plane, which also covers
>> flips. Like omapdrm we also unreference the old framebuffer here.
>>
>> Signed-off-by: Daniel Drake <drake@endlessm.com>
> This sounds very much like exynos should switch to universal planes so
> that the fake primary plane created by the helpers doesn't get in the way.
> And for chips which already use planes for everything internally this
> shouldn't be a lot more than a few lines.
> -Daniel

The patch proposed here of course supersedes my patch fixing fb refcounting.
But the best solution is to get rid of virtual plane as Daniel Vetter
stated.
Daniel (Drake of course :) ) do you want to prepare patch switching to
universal planes?
Maybe other volunteers? If not I can try to do it, as it seems quite
straightforward.

Regards
Andrzej

>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 12 ++----------
>>  drivers/gpu/drm/exynos/exynos_drm_plane.c |  8 ++++++++
>>  2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> index b68e58f..7aa9dee 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> @@ -140,16 +140,8 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>  	if (manager->ops->mode_set)
>>  		manager->ops->mode_set(manager, &crtc->mode);
>>  
>> -	ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
>> -				    x, y, crtc_w, crtc_h);
>> -	if (ret)
>> -		return ret;
>> -
>> -	plane->crtc = crtc;
>> -	plane->fb = crtc->primary->fb;
>> -	drm_framebuffer_reference(plane->fb);
>> -
>> -	return 0;
>> +	return exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0,
>> +				     crtc_w, crtc_h, x, y, crtc_w, crtc_h);
>>  }
>>  
>>  static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> index 8371cbd..df27e35 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> @@ -139,6 +139,14 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
>>  			overlay->crtc_x, overlay->crtc_y,
>>  			overlay->crtc_width, overlay->crtc_height);
>>  
>> +	if (plane->fb)
>> +		drm_framebuffer_unreference(plane->fb);
>> +
>> +	drm_framebuffer_reference(fb);
>> +
>> +	plane->fb = fb;
>> +	plane->crtc = crtc;
>> +
>>  	exynos_drm_crtc_plane_mode_set(crtc, overlay);
>>  
>>  	return 0;
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Inki Dae]

On 2014년 09월 17일 15:35, Andrzej Hajda wrote:
> Hi,
> 
> On 09/16/2014 08:35 AM, Daniel Vetter wrote:
>> On Mon, Sep 15, 2014 at 12:52:17PM -0600, Daniel Drake wrote:
>>> Pageflipping currently causes some inconsistencies that lead to
>>> crashes. Just run an app that causes a CRTC pageflip in a raw X session
>>> and check that it exits cleanly and can be restarted - you'll see
>>> crashes like:
>>>  Unable to handle kernel NULL pointer dereference at virtual address 00000334
>>>  PC is at exynos_drm_crtc_plane_commit+0x20/0x40
>>>  LR is at exynos_drm_crtc_plane_commit+0x20/0x40
>>>  [<c03749b4>] (exynos_drm_crtc_plane_commit) from [<c03741bc>] (exynos_drm_crtc_commit+0x44/0x70)
>>>  [<c03741bc>] (exynos_drm_crtc_commit) from [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4)
>>>  [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2) from [<c03744f4>] (exynos_drm_crtc_page_flip+0x140/0x1a8)
>>>  [<c03744f4>] (exynos_drm_crtc_page_flip) from [<c036b20c>] (drm_mode_page_flip_ioctl+0x224/0x2dc)
>>>  [<c036b20c>] (drm_mode_page_flip_ioctl) from [<c035c324>] (drm_ioctl+0x338/0x4fc)
>>>
>>> These crashes happen because drm_plane_force_disable has previously set
>>> plane->crtc to NULL.
>>>
>>> When drm_mode_page_flip_ioctl() is used to flip another framebuffer
>>> onto the primary plane, crtc->primary->fb is correctly updated (this is
>>> a virtual plane created by plane_helper), but plane->fb is not (this
>>> plane is the real one, created by exynos_drm_crtc_create).
>>>
>>> We then come to handle rmfb of the backbuffer, which the "real" primary
>>> plane is incorrectly pointing at. So drm_framebuffer_remove() decides that
>>> the buffer is actually active on a plane and force-disables the plane.
>>>
>>> Ensuring that plane->fb is kept up-to-date solves that issue, but
>>> exposes a reference counting problem. Now we see crashes when rmfb is
>>> called on the front-buffer, because the rmfb code expects to drop 3
>>> references here, and there are only 2.
>>>
>>> That can be fixed by adopting the reference management found in omapdrm:
>>> Framebuffer references are not taken directly in crtc mode_set context,
>>> but rather in the context of updating the plane, which also covers
>>> flips. Like omapdrm we also unreference the old framebuffer here.
>>>
>>> Signed-off-by: Daniel Drake <drake@endlessm.com>
>> This sounds very much like exynos should switch to universal planes so
>> that the fake primary plane created by the helpers doesn't get in the way.
>> And for chips which already use planes for everything internally this
>> shouldn't be a lot more than a few lines.
>> -Daniel
> 
> The patch proposed here of course supersedes my patch fixing fb refcounting.
> But the best solution is to get rid of virtual plane as Daniel Vetter
> stated.
> Daniel (Drake of course :) ) do you want to prepare patch switching to
> universal planes?
> Maybe other volunteers? If not I can try to do it, as it seems quite
> straightforward.

I think you can do it and you would be a right person to do it.

Thanks,
Inki Dae

> 
> Regards
> Andrzej
> 
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 12 ++----------
>>>  drivers/gpu/drm/exynos/exynos_drm_plane.c |  8 ++++++++
>>>  2 files changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> index b68e58f..7aa9dee 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> @@ -140,16 +140,8 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>  	if (manager->ops->mode_set)
>>>  		manager->ops->mode_set(manager, &crtc->mode);
>>>  
>>> -	ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
>>> -				    x, y, crtc_w, crtc_h);
>>> -	if (ret)
>>> -		return ret;
>>> -
>>> -	plane->crtc = crtc;
>>> -	plane->fb = crtc->primary->fb;
>>> -	drm_framebuffer_reference(plane->fb);
>>> -
>>> -	return 0;
>>> +	return exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0,
>>> +				     crtc_w, crtc_h, x, y, crtc_w, crtc_h);
>>>  }
>>>  
>>>  static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> index 8371cbd..df27e35 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> @@ -139,6 +139,14 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
>>>  			overlay->crtc_x, overlay->crtc_y,
>>>  			overlay->crtc_width, overlay->crtc_height);
>>>  
>>> +	if (plane->fb)
>>> +		drm_framebuffer_unreference(plane->fb);
>>> +
>>> +	drm_framebuffer_reference(fb);
>>> +
>>> +	plane->fb = fb;
>>> +	plane->crtc = crtc;
>>> +
>>>  	exynos_drm_crtc_plane_mode_set(crtc, overlay);
>>>  
>>>  	return 0;
>>> -- 
>>> 1.9.1
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Joonyoung Shim]

Hi,

On 09/17/2014 03:49 PM, Inki Dae wrote:
> On 2014년 09월 17일 15:35, Andrzej Hajda wrote:
>> Hi,
>>
>> On 09/16/2014 08:35 AM, Daniel Vetter wrote:
>>> On Mon, Sep 15, 2014 at 12:52:17PM -0600, Daniel Drake wrote:
>>>> Pageflipping currently causes some inconsistencies that lead to
>>>> crashes. Just run an app that causes a CRTC pageflip in a raw X session
>>>> and check that it exits cleanly and can be restarted - you'll see
>>>> crashes like:
>>>>  Unable to handle kernel NULL pointer dereference at virtual address 00000334
>>>>  PC is at exynos_drm_crtc_plane_commit+0x20/0x40
>>>>  LR is at exynos_drm_crtc_plane_commit+0x20/0x40
>>>>  [<c03749b4>] (exynos_drm_crtc_plane_commit) from [<c03741bc>] (exynos_drm_crtc_commit+0x44/0x70)
>>>>  [<c03741bc>] (exynos_drm_crtc_commit) from [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4)
>>>>  [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2) from [<c03744f4>] (exynos_drm_crtc_page_flip+0x140/0x1a8)
>>>>  [<c03744f4>] (exynos_drm_crtc_page_flip) from [<c036b20c>] (drm_mode_page_flip_ioctl+0x224/0x2dc)
>>>>  [<c036b20c>] (drm_mode_page_flip_ioctl) from [<c035c324>] (drm_ioctl+0x338/0x4fc)
>>>>
>>>> These crashes happen because drm_plane_force_disable has previously set
>>>> plane->crtc to NULL.
>>>>
>>>> When drm_mode_page_flip_ioctl() is used to flip another framebuffer
>>>> onto the primary plane, crtc->primary->fb is correctly updated (this is
>>>> a virtual plane created by plane_helper), but plane->fb is not (this
>>>> plane is the real one, created by exynos_drm_crtc_create).
>>>>
>>>> We then come to handle rmfb of the backbuffer, which the "real" primary
>>>> plane is incorrectly pointing at. So drm_framebuffer_remove() decides that
>>>> the buffer is actually active on a plane and force-disables the plane.
>>>>
>>>> Ensuring that plane->fb is kept up-to-date solves that issue, but
>>>> exposes a reference counting problem. Now we see crashes when rmfb is
>>>> called on the front-buffer, because the rmfb code expects to drop 3
>>>> references here, and there are only 2.
>>>>
>>>> That can be fixed by adopting the reference management found in omapdrm:
>>>> Framebuffer references are not taken directly in crtc mode_set context,
>>>> but rather in the context of updating the plane, which also covers
>>>> flips. Like omapdrm we also unreference the old framebuffer here.
>>>>
>>>> Signed-off-by: Daniel Drake <drake@endlessm.com>
>>> This sounds very much like exynos should switch to universal planes so
>>> that the fake primary plane created by the helpers doesn't get in the way.
>>> And for chips which already use planes for everything internally this
>>> shouldn't be a lot more than a few lines.
>>> -Daniel
>>
>> The patch proposed here of course supersedes my patch fixing fb refcounting.
>> But the best solution is to get rid of virtual plane as Daniel Vetter
>> stated.
>> Daniel (Drake of course :) ) do you want to prepare patch switching to
>> universal planes?
>> Maybe other volunteers? If not I can try to do it, as it seems quite
>> straightforward.
> 
> I think you can do it and you would be a right person to do it.
> 
> Thanks,
> Inki Dae
> 
>>
>> Regards
>> Andrzej
>>
>>>> ---
>>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 12 ++----------
>>>>  drivers/gpu/drm/exynos/exynos_drm_plane.c |  8 ++++++++
>>>>  2 files changed, 10 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>> index b68e58f..7aa9dee 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>> @@ -140,16 +140,8 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>>  	if (manager->ops->mode_set)
>>>>  		manager->ops->mode_set(manager, &crtc->mode);
>>>>  
>>>> -	ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
>>>> -				    x, y, crtc_w, crtc_h);
>>>> -	if (ret)
>>>> -		return ret;
>>>> -
>>>> -	plane->crtc = crtc;
>>>> -	plane->fb = crtc->primary->fb;
>>>> -	drm_framebuffer_reference(plane->fb);

It's problem to add this from commit 25c8b5c3048cb6c98d402ca8d4735ccf910f727c.
Chip specific drm driver internally doesn't have to care fb reference count if
there is no special case. We should have switched to universal plane at that
time.

Thanks.

>>>> -
>>>> -	return 0;
>>>> +	return exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0,
>>>> +				     crtc_w, crtc_h, x, y, crtc_w, crtc_h);
>>>>  }
>>>>  
>>>>  static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>>> index 8371cbd..df27e35 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>>> @@ -139,6 +139,14 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
>>>>  			overlay->crtc_x, overlay->crtc_y,
>>>>  			overlay->crtc_width, overlay->crtc_height);
>>>>  
>>>> +	if (plane->fb)
>>>> +		drm_framebuffer_unreference(plane->fb);
>>>> +
>>>> +	drm_framebuffer_reference(fb);
>>>> +
>>>> +	plane->fb = fb;
>>>> +	plane->crtc = crtc;
>>>> +
>>>>  	exynos_drm_crtc_plane_mode_set(crtc, overlay);
>>>>  
>>>>  	return 0;
>>>> -- 
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> 

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Drake]

On Wed, Sep 17, 2014 at 1:44 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> It's problem to add this from commit 25c8b5c3048cb6c98d402ca8d4735ccf910f727c.

My patch moves that drm_framebuffer_reference() call to the plane
function which is called from crtc_mode_set context (and also called
in crtc pageflip path), so there should be no problem here.

> Chip specific drm driver internally doesn't have to care fb reference count if
> there is no special case. We should have switched to universal plane at that
> time.

To me it seems like the chip-specific DRM drivers do need to add a
reference in the crtc_mode_set and crtc page flip paths otherwise
framebuffer removal crashes (expecting to remove 3 references), as
noted by my testing and also in commit 25c8b5c304.

However, I'll be happy if universal planes means the driver does not
have to care about this any more. Andrej, please go ahead if you are
interested, I'll be happy to test your results.

Thanks
Daniel

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Vetter]

On Wed, Sep 17, 2014 at 2:19 PM, Daniel Drake <drake@endlessm.com> wrote:
>> Chip specific drm driver internally doesn't have to care fb reference count if
>> there is no special case. We should have switched to universal plane at that
>> time.
>
> To me it seems like the chip-specific DRM drivers do need to add a
> reference in the crtc_mode_set and crtc page flip paths otherwise
> framebuffer removal crashes (expecting to remove 3 references), as
> noted by my testing and also in commit 25c8b5c304.

I think fb refcounting in exynos is just plain busted. If you look at
other drivers the only place the refcount framebuffers or backing
storage objects is for pageflips to make sure the memory doesn't go
away while the hw is still scanning out the old framebuffer. If you
refcount anywhere else you either do something really crazy or your
driver is broken.

> However, I'll be happy if universal planes means the driver does not
> have to care about this any more. Andrej, please go ahead if you are
> interested, I'll be happy to test your results.

universal planes will fix up the mess with 2 drm plane objects
(primary plane + exonys internal primary). So should help to untangle
this not, but it will not magically fix the refcounting bugs itself.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Drake]

On Wed, Sep 17, 2014 at 7:45 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> I think fb refcounting in exynos is just plain busted. If you look at
> other drivers the only place the refcount framebuffers or backing
> storage objects is for pageflips to make sure the memory doesn't go
> away while the hw is still scanning out the old framebuffer. If you
> refcount anywhere else you either do something really crazy or your
> driver is broken.

With my patch actually the behaviour is much more similar to omapdrm,
which also doesn't quite match your description of "other drivers".
See omap_plane.c.

There is a fb reference taken for "pinning" in update_pin() which
presumably is what you describe - avoid destroying the fb while it is
being scanned out. (Maybe exynos should have something equivalent too,
but thats a separate issue)

However there is *another* fb reference taken in
omap_plane_mode_set(). And my patch is modelled to do the same in
exynos-drm.

I believe this is necessary under the current model. At least, when
drm_mode_rmfb() is running for the last user of the active
framebuffer, it expects to drop 3 references from the framebuffer
before dropping the 4th causes the object to be destroyed, as follows:

1. drm_mode_rmfb explicitly drops a reference - it calls
__drm_framebuffer_unregister which then calls
__drm_framebuffer_unreference
    /* Mark fb as reaped, we still have a ref from fpriv->fbs. */
    __drm_framebuffer_unregister(dev, fb);

2. drm_mode_rmfb then calls drm_framebuffer_remove, which calls
drm_mode_set_config_internal() in order to turn off the CRTC, dropping
another reference in the process.
        if (tmp->old_fb)
            drm_framebuffer_unreference(tmp->old_fb);

3. drm_framebuffer_remove calls drm_plane_force_disable() which drops
another reference:
    /* disconnect the plane from the fb and crtc: */
    __drm_framebuffer_unreference(old_fb);

4. drm_framebuffer drops the final reference itself, to cause freeing
of the object:
    drm_framebuffer_unreference(fb);


So ordinarily, after a fb is created by drm core (with refcnt at 1),
there would have to be 3 references added to it by the time it is the
primary fb so that when we do rmfb, it has a refcnt of 4, and gets
freed correctly.
(The second bug I was seeing with pageflips was that refcnt was 3,
which means that the final reference was dropped in (3) above, but
__drm_framebuffer_unreference doesn't like that at all - it calls
drm_framebuffer_free_bug)

Not being overly familiar with DRM internals I tried to go backwards
to find out where these 3 references would be created during normal
operation. 2 are clear:

1. drm_framebuffer_init() explicitly grabs one:
    /* Grab the idr reference. */
    drm_framebuffer_reference(fb)

2. drm_mode_set_config_internal() takes one:
        if (tmp->primary->fb)
            drm_framebuffer_reference(tmp->primary->fb);

Where should the 3rd one be created? I don't know, but looking at
previous exynos commit 25c8b5c304 and omapdrm, I assumed that the drm
driver should take one, both on crtc mode set and crtc page flip.

>> However, I'll be happy if universal planes means the driver does not
>> have to care about this any more. Andrej, please go ahead if you are
>> interested, I'll be happy to test your results.
>
> universal planes will fix up the mess with 2 drm plane objects
> (primary plane + exonys internal primary). So should help to untangle
> this not, but it will not magically fix the refcounting bugs itself.

So even when we move to universal planes (fixing 1 of the issues), its
good that we're having this refcount discussion (which we need to
understand to confidently solve the 2nd issue). Thanks for your input!

Daniel

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Joonyoung Shim]

Hi,

On 09/18/2014 01:41 AM, Daniel Drake wrote:
> On Wed, Sep 17, 2014 at 7:45 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> I think fb refcounting in exynos is just plain busted. If you look at
>> other drivers the only place the refcount framebuffers or backing
>> storage objects is for pageflips to make sure the memory doesn't go
>> away while the hw is still scanning out the old framebuffer. If you
>> refcount anywhere else you either do something really crazy or your
>> driver is broken.
> 
> With my patch actually the behaviour is much more similar to omapdrm,

Your patch will occur fb reference count problem when setplane.

> which also doesn't quite match your description of "other drivers".
> See omap_plane.c.
> 
> There is a fb reference taken for "pinning" in update_pin() which
> presumably is what you describe - avoid destroying the fb while it is
> being scanned out. (Maybe exynos should have something equivalent too,
> but thats a separate issue)
> 
> However there is *another* fb reference taken in
> omap_plane_mode_set(). And my patch is modelled to do the same in
> exynos-drm.
> 
> I believe this is necessary under the current model. At least, when
> drm_mode_rmfb() is running for the last user of the active
> framebuffer, it expects to drop 3 references from the framebuffer
> before dropping the 4th causes the object to be destroyed, as follows:
> 
> 1. drm_mode_rmfb explicitly drops a reference - it calls
> __drm_framebuffer_unregister which then calls
> __drm_framebuffer_unreference
>     /* Mark fb as reaped, we still have a ref from fpriv->fbs. */
>     __drm_framebuffer_unregister(dev, fb);
> 
> 2. drm_mode_rmfb then calls drm_framebuffer_remove, which calls
> drm_mode_set_config_internal() in order to turn off the CRTC, dropping
> another reference in the process.
>         if (tmp->old_fb)
>             drm_framebuffer_unreference(tmp->old_fb);
> 
> 3. drm_framebuffer_remove calls drm_plane_force_disable() which drops
> another reference:
>     /* disconnect the plane from the fb and crtc: */
>     __drm_framebuffer_unreference(old_fb);

This call is new path, before universal planes merged, private plane of
exynos crtc wasn't included in dev->mode_config.plane_list because
private plane wasn't exposed to userspace so this path wasn't called.

> 
> 4. drm_framebuffer drops the final reference itself, to cause freeing
> of the object:
>     drm_framebuffer_unreference(fb);
> 
> 
> So ordinarily, after a fb is created by drm core (with refcnt at 1),
> there would have to be 3 references added to it by the time it is the
> primary fb so that when we do rmfb, it has a refcnt of 4, and gets
> freed correctly.
> (The second bug I was seeing with pageflips was that refcnt was 3,
> which means that the final reference was dropped in (3) above, but
> __drm_framebuffer_unreference doesn't like that at all - it calls
> drm_framebuffer_free_bug)
> 
> Not being overly familiar with DRM internals I tried to go backwards
> to find out where these 3 references would be created during normal
> operation. 2 are clear:
> 
> 1. drm_framebuffer_init() explicitly grabs one:
>     /* Grab the idr reference. */
>     drm_framebuffer_reference(fb)
> 
> 2. drm_mode_set_config_internal() takes one:
>         if (tmp->primary->fb)
>             drm_framebuffer_reference(tmp->primary->fb);
> 
> Where should the 3rd one be created? I don't know, but looking at
> previous exynos commit 25c8b5c304 and omapdrm, I assumed that the drm
> driver should take one, both on crtc mode set and crtc page flip.

So Andrzej added fb reference count increasing in crtc modeset path, but
i think we can take away this workaround if remove private plane for
exynos crtc.

Thanks.

> 
>>> However, I'll be happy if universal planes means the driver does not
>>> have to care about this any more. Andrej, please go ahead if you are
>>> interested, I'll be happy to test your results.
>>
>> universal planes will fix up the mess with 2 drm plane objects
>> (primary plane + exonys internal primary). So should help to untangle
>> this not, but it will not magically fix the refcounting bugs itself.
> 
> So even when we move to universal planes (fixing 1 of the issues), its
> good that we're having this refcount discussion (which we need to
> understand to confidently solve the 2nd issue). Thanks for your input!
> 
> Daniel
> 

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Vetter]

On Wed, Sep 17, 2014 at 6:41 PM, Daniel Drake <drake@endlessm.com> wrote:
> However there is *another* fb reference taken in
> omap_plane_mode_set(). And my patch is modelled to do the same in
> exynos-drm.

This is because omapdrm does _everything_ asynchrously, even plain
modesets. Unfortunately that async modeset support is broken, so the
latest omapdrm patches insert a synchronization point.

So picking omap's mode_set logic as a reference because it also does
fb refcounting is not a good idea - that code does something crazy and
gets it wrong. And really, if you do modeset synchronously the drm
core will take care of your refcounting needs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Vetter]

On Wed, Sep 17, 2014 at 6:41 PM, Daniel Drake <drake@endlessm.com> wrote:
> 2. drm_mode_rmfb then calls drm_framebuffer_remove, which calls
> drm_mode_set_config_internal() in order to turn off the CRTC, dropping
> another reference in the process.
>         if (tmp->old_fb)
>             drm_framebuffer_unreference(tmp->old_fb);
>
> 3. drm_framebuffer_remove calls drm_plane_force_disable() which drops
> another reference:
>     /* disconnect the plane from the fb and crtc: */
>     __drm_framebuffer_unreference(old_fb);

If 3. here is about the primary plane then this won't happen, since
the primary plane pointer&reference has already been cleared in step
2.

And even if their would be a bug in here, you _certainly_ should not
try to paper over this in your driver, but instead fix up the
refcounting done in the drm core.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Drake]

On Thu, Sep 18, 2014 at 12:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 17, 2014 at 6:41 PM, Daniel Drake <drake@endlessm.com> wrote:
>> 2. drm_mode_rmfb then calls drm_framebuffer_remove, which calls
>> drm_mode_set_config_internal() in order to turn off the CRTC, dropping
>> another reference in the process.
>>         if (tmp->old_fb)
>>             drm_framebuffer_unreference(tmp->old_fb);
>>
>> 3. drm_framebuffer_remove calls drm_plane_force_disable() which drops
>> another reference:
>>     /* disconnect the plane from the fb and crtc: */
>>     __drm_framebuffer_unreference(old_fb);
>
> If 3. here is about the primary plane then this won't happen, since
> the primary plane pointer&reference has already been cleared in step
> 2.

I just checked - as Joonyoung suspects, the plane being force disabled
in step 3 is the private exynos-drm plane. So thats an issue - but at
least now I have a complete understanding of the problem.

Sounds like that will also be fixed by moving to universal planes.
I'll wait for Andrzej's patch.

Thanks!
Daniel