Possible fb ref count issue with drm_plane_force_disable()

[Rob Clark]

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.

BR,
-R


>
> All the other unrefs I can explain, except the drm_plane_force_disable() one.
>
>  Tomi
>
>