Possible fb ref count issue with drm_plane_force_disable()

[Andrzej Hajda]

On 04/15/2014 12:10 PM, Rob Clark wrote:
> On Tue, Apr 15, 2014 at 5:16 AM, Tomi Valkeinen <tomi.valkeinen at ti.com> wrote:
>> On 14/04/14 11:43, Tomi Valkeinen wrote:
>>> On 11/04/14 14:50, Ville Syrjälä wrote:
>>>
>>>>> So the explicit unref done by drm_plane_force_disable() seems a bit out
>>>>> of place. I can't figure out which drm_framebuffer_reference() would be
>>>>> the matching one for the unref done by drm_plane_force_disable().
>>>>>
>>>>> Any ideas what ref is that? Or is the __drm_framebuffer_unreference()
>>>>> extra in drm_plane_force_disable()?
>>>>
>>>> That's the reference that was taken by the drm_mode_setplane() when it
>>>> succesfully called the .update_plane() hook.
>>>
>>> But drm_mode_setplane() is called via DRM_IOCTL_MODE_SETPLANE, which is
>>> only used for "proper" planes, not for crtc primary planes, right?
>>>
>>> At least I don't see drm_mode_setplane() in my stack traces, and git
>>> grep doesn't show it called via any other means than ioctl.
>>>
>>> I am not using any planes from my app, just the crtc and (indirectly)
>>> its primary plane.
>>
>> So here's a summary of the fb refs and unrefs. It still seems to me that the
>> drm_plane_force_disable does an extra unref. Either that, or omapdrm is missing
>> something that takes the matching reference.
>>
>> A line like this in the summary below:
>>
>> 2) ref36/3      drm_mode_setcrtc / drm_framebuffer_lookup
>>
>> Means that "2" is the number I assigned to that ref, and there's a matching
>> unref with 2) prefix later. ref36/2 means that FB ID 36 was referenced, and ref
>> count is 3. And the rest of the line shows rough idea of the stack trace where
>> the ref/unref happens.
>>
>>
>> # DRM_IOCTL_MODE_ADDFB 2
>> 0)              kref_init
>> 1) ref36/2      drm_mode_addfb2
>>
>> # DRM_IOCTL_MODE_SETCRTC
>> 2) ref36/3      drm_mode_setcrtc / drm_framebuffer_lookup
>> 3) ref36/4      drm_mode_setcrtc / drm_mode_set_config_internal
>> 2) unref36/3    drm_mode_setcrtc
>>
>> # pin new fb
>> 4) ref36/4      apply_worker / omap_plane_pre_apply
>>
>>
>> # BREAK
>>
>>
>> # RELEASE
>> 1) unref36/3    drm_release / drm_fb_release / __drm_framebuffer_unregister
>> 3) unref36/2    drm_release / drm_fb_release / drm_framebuffer_remove / drm_mode_set_config_internal
>> ?) unref36/1    drm_release / drm_fb_release / drm_framebuffer_remove / drm_plane_force_disable
>> 0) unref36/0    drm_release / drm_fb_release / drm_framebuffer_remove
>>
>> # unpin old fb
>> 4) unref36/-1   flip_worker / unpin_worker / drm_framebuffer_unreference
>>
> 
> Ok, sorry to take so long to comment on the thread, it took me a while
> before I had an idea ;-)
> 
> so, what triggers this is that previously drm_framebuffer_remove() did
> not process private planes.  But now the fb shows up attached to a
> plane as well, the crtc's primary plane.
> 
> I suspect there is some way in omap_crtc that I must have assumed the
> ref the crtc held to the fb was sufficient for the plane to hold the
> same ref.  Which is no longer the case.  So somewhere between
> omap_crtc and it's primary plane, there now needs to be an extra level
> of ref/unref.  So ref should have gone up to 5.

I have experienced similar problem with exynos_drm. I have found that
in exynos_drm_crtc_mode_set there is line:

	plane->fb = crtc->primary->fb;

without drm_framebuffer_reference.
In result drm_framebuffer_remove dereferences it twice:
- because of crtc->primary->fb == fb,
- because of fb being on dev->mode_config.plane_list

I am not sure how it should be solved properly, but adding
drm_framebuffer_reference in exynos_drm_crtc_mode_set helps.

In omap_plane_mode_set there is also assignment:

	plane->fb = fb

without drm_framebuffer_reference so maybe it can be solved the same way.

Regards
Andrzej

> 
> BR,
> -R
> 
> 
>>
>> All the other unrefs I can explain, except the drm_plane_force_disable() one.
>>
>>  Tomi
>>
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>