Possible fb ref count issue with drm_plane_force_disable()

[Tomi Valkeinen]

On 11/04/14 09:31, Archit Taneja wrote:
> Hi,
> 
> On Thursday 10 April 2014 05:17 PM, Tomi Valkeinen wrote:
>> Hi,
>>
>> I've been debugging omapdrm issues on top of the latest drm mainline
>> changes. Sometimes a drm_framebuffer ref count drops to -1 when aborting
>> a drm application, or unloading the modules.
>>
>> The setup is very basic, just a single crtc with the crtc's primary
>> plane.
>>
>> What seems to happen is:
>>
>> - App is started
>>
>> - fb is created, and taken into use by omapdrm. omapdrm takes a ref to
>> the fb.
>>
>> - the app is starts to shut down
>>
>> - drm_framebuffer_remove is called
> 
> Does drm_framebuffer_remove get called when we abort the application, or
> unload omapdrm, or both?

Both. When we abort an app, drm_framebuffer_remove gets called for the
fb that the app created. When we unload omapdrm, it gets called for the
main fb, used for fb console.

>> - fb->refcount.refcount > 1, so it goes to disable stuff
>>
>> - drm_plane_force_disable is called for the primary plane
> 
> Maybe we need to make sure that this func is called only when our driver
> has unreferenced it. In that case, we would probably need to flush our
> queue and disable interrupts(so that we don't queue more work).

Hmm, sorry, call which func, unreferenced what? =)

Do you mean we should call drm_framebuffer_remove() only if
fb->refcount.refcount == 1. That should be possible, and would probably
remove the issue, but it would just be going around the actual problem.

>> - drm_plane_force_disable does plane->disable_plane, which on omapdrm
>> puts stuff on a workqueue as plane cannot be disabled immediately
>>
>> - drm_plane_force_disable calls __drm_framebuffer_unreference()
>>
>> - at the end of drm_framebuffer_remove(), there's
>> drm_framebuffer_unreference, which causes ref count to go to zero, and
>> the fb to be destroyed
>>
>> - a bit later, the queued work is ran, which does
>> drm_framebuffer_unreference(), and ref count goes to -1. Here omapdrm is
>> removing the ref that had been taken in the beginning.
>>
>>
>> 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()?
> 
> 
> I can't get the corresponding reference for it either. But I can count 3
> of them when you run fbcon with drm's fb helper.
> 
> - One ref is taken in the drm_framebuffer_init called from
> omap_fbdev_create.
> 
> - fbcon will call fb_set_par, which calls drm_fb_helper_set_par, that
> seems to take a refernce to the fb in the end.

This one is not called for a normal app, as there's no fbdev framebuffer
for it. Note that the funcs I mention deal with drm framebuffer, which
is not the fbdev framebuffer (confusing =). fbdev fb is only used for
the "root" framebuffer. And, of course, fbdev fb uses the drm fb
functionality.

> - drm_crtc_helper_set_config() calls the omadrm specific mode_set
> drm_crtc_funcs, which eventually calls a drm_framebuffer_reference in
> update_pin().

Yep.

I forgot to mention that if I comment out the unref in
drm_plane_force_disable(), the ref counts match and all looks fine. And
also that I didn't see this issue with 3.14.

 Tomi


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140411/dab12e52/attachment-0001.sig>