[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 at 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 at 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 at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
>