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
- fb->refcount.refcount > 1, so it goes to disable stuff
- drm_plane_force_disable is called for the primary plane
- 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()?
Tomi
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?
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).
- 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.
- drm_crtc_helper_set_config() calls the omadrm specific mode_set drm_crtc_funcs, which eventually calls a drm_framebuffer_reference in update_pin().
Archit
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
On Friday 11 April 2014 12:10 PM, Tomi Valkeinen wrote:
<snip>
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.
Yes, I meant our driver should call drm_framebuffer_remove() only when we are know that the fb reference omapdrm had taken(the one in update_pin), has a corresponding fb unref called(in unpin_worker).
Isn't that something omapdrm should take care of?
<snip>
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
Yes. In the case of a normal app, we would call the ADD_FB2 ioctl to get a drm_framebuffer, that will internally take a fb reference. So the count won't seem to go beyond 2.
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.
That's strange. I guess there must be an extra unref happening somewhere in the newer commits. Did you try a diff of drm code between the 2 kernels? :)
Archit
On 11/04/14 09:57, Archit Taneja wrote:
Yes, I meant our driver should call drm_framebuffer_remove() only when we are know that the fb reference omapdrm had taken(the one in update_pin), has a corresponding fb unref called(in unpin_worker).
Isn't that something omapdrm should take care of?
Yes, we could do it, and it's probably something we should do. I haven't looked at it, so I don't know how easy or difficult it is to make sure the fb won't be used after we think it has been disabled.
But even so, the drm_framebuffer_remove() seems to be designed to handle the case where the fb is still in use, but it's failing.
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.
That's strange. I guess there must be an extra unref happening somewhere in the newer commits. Did you try a diff of drm code between the 2 kernels? :)
Not yet. I'm guessing that it's about the plane changes. There was already one issue with omapdrm, where the crtc calls plane->destroy on the crtc's primary plane. With the latest changes, the all the planes, including the primary planes, are destroyed automatically, so the crtc's destroy call is not needed (and causes a crash).
Tomi
On Thu, Apr 10, 2014 at 02:47:52PM +0300, 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
fb->refcount.refcount > 1, so it goes to disable stuff
drm_plane_force_disable is called for the primary plane
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()?
plane->fb is holding a reference which should have been acquired in drm_mode_setplane. It's not really explicit since drm_framebuffer_lookup implicitly grabs a ref for you.
Hence when we disable the plane and set plane->fb = NULL we must drop that ref again. So that's not the bug, but clearly something goes wrong somewhere. -Daniel
On Thu, Apr 10, 2014 at 02:47:52PM +0300, 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
fb->refcount.refcount > 1, so it goes to disable stuff
drm_plane_force_disable is called for the primary plane
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()?
That's the reference that was taken by the drm_mode_setplane() when it succesfully called the .update_plane() hook.
__drm_framebuffer_unregister() drops the "idr reference" taken in drm_framebuffer_init().
And the last ref dropped by drm_framebuffer_remove() is the initial ref from kref_init() which I suppose is what the 'fpriv->fbs reference' comments are referring to.
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.
I've attached a slightly cleaned up log when running my test app. I have dump_stack()s in the drm_framebuffer_(un)reference functions, and a printk just before the dump_stack which prints something like "REF FB ID: 36: 2", where 36 is the fb id, and 2 is the ref count after the ref/unref.
In the middle of the log there are multiple blank lines, that's where I ctrl-c the app and it starts shutting down. FB ID 35 is probably the fbconsole fb, and 36 is the app's fb. And at some point the FB ID 36 changes to 0, as the drm framework clears it.
__drm_framebuffer_unregister() drops the "idr reference" taken in drm_framebuffer_init().
What's "idr"?
Tomi
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
All the other unrefs I can explain, except the drm_plane_force_disable() one.
Tomi
On Tue, Apr 15, 2014 at 5:16 AM, Tomi Valkeinen tomi.valkeinen@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:
- 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
- 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
- unref36/3 drm_release / drm_fb_release / __drm_framebuffer_unregister
- 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
On 04/15/2014 12:10 PM, Rob Clark wrote:
On Tue, Apr 15, 2014 at 5:16 AM, Tomi Valkeinen tomi.valkeinen@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:
- 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
- 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
- unref36/3 drm_release / drm_fb_release / __drm_framebuffer_unregister
- 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 15/04/14 13:29, Andrzej Hajda wrote:
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.
The omap_plane_mode_set() is called also when using non-primary planes. For those the refcounting goes right at the moment (I think), so adding drm_framebuffer_reference() at that func would break it.
I guess I could check if the plane is primary, and add ref only then. Or add the ref in omap_crtc, before it calls omap_plane_mode_set(). Both feel a bit hacky...
Tomi
On Tue, Apr 15, 2014 at 6:44 AM, Tomi Valkeinen tomi.valkeinen@ti.com wrote:
On 15/04/14 13:29, Andrzej Hajda wrote:
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 = fbwithout drm_framebuffer_reference so maybe it can be solved the same way.
The omap_plane_mode_set() is called also when using non-primary planes. For those the refcounting goes right at the moment (I think), so adding drm_framebuffer_reference() at that func would break it.
I guess I could check if the plane is primary, and add ref only then. Or add the ref in omap_crtc, before it calls omap_plane_mode_set(). Both feel a bit hacky...
probably split out omap_plane_mode_set_internal(), call that directly from update_plane() for plane operations. And then do the refcnt dance in the new omap_plane_mode_set() which calls _internal()..
BR, -R
Tomi
On 15/04/14 15:24, Rob Clark wrote:
probably split out omap_plane_mode_set_internal(), call that directly from update_plane() for plane operations. And then do the refcnt dance in the new omap_plane_mode_set() which calls _internal()..
We don't actually need that. This did the trick:
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index df1725247cca..3cf31ee59aac 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -225,6 +225,11 @@ int omap_plane_mode_set(struct drm_plane *plane, omap_plane->apply_done_cb.arg = arg; }
+ if (plane->fb) + drm_framebuffer_unreference(plane->fb); + + drm_framebuffer_reference(fb); + plane->fb = fb; plane->crtc = crtc;
@@ -241,11 +246,6 @@ static int omap_plane_update(struct drm_plane *plane, struct omap_plane *omap_plane = to_omap_plane(plane); omap_plane->enabled = true;
- if (plane->fb) - drm_framebuffer_unreference(plane->fb); - - drm_framebuffer_reference(fb); - /* omap_plane_mode_set() takes adjusted src */ switch (omap_plane->win.rotation & 0xf) { case BIT(DRM_ROTATE_90):
With quick tests, works fine so far.
Still I have to say that the ref counting doesn't feel quite right (or I'm missing something).
As far as I understand, the drm_mode_setplane() gets a ref to the fb, and stores it in plane->fb. Why do we take a new ref in omap_plane_update (or with the patch above, in omap_plane_mode_set), and also store it in plane->fb there? Feels like the same task is done in two places.
It looks to me like drm_mode_setplane() doesn't really presume that the update_plane would set plane->fb or plane->crtc, as it does that itself (and only if update_plane has succeeded).
Tomi
On Tue, Apr 15, 2014 at 3:30 PM, Tomi Valkeinen tomi.valkeinen@ti.com wrote:
It looks to me like drm_mode_setplane() doesn't really presume that the update_plane would set plane->fb or plane->crtc, as it does that itself (and only if update_plane has succeeded).
Yeah that's a bit of historical hilarity. ->set_config should update crtc->primary->fb, but when calling ->update_plane the drm core will do this for you. In both cases the plane->fb is the old fb when your driver hook gets called.
I guess now that we have primary planes we should unify these semantics a bit since they're fairly pointless. -Daniel
On 15/04/14 13:10, Rob Clark wrote:
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 still quite lost here... Looking at the non-primary plane's fb ref counting, some drivers add and remove refs to fb in plane->plane_update(). But not all.
For omapdrm, update_plane takes a ref, but I'm not quite sure who frees that ref. Nothing in omap_plane.c seems to do that.
Maybe the ref taken in omapdrm's update_plane is the "same" one that should be taken for the primary plane also. But I'm not sure where that should be taken, and if I do take the ref, then I guess it's freed somewhere else than in omapdrm.
Taking and releasing refs in totally different places is really bad practice =).
Tomi
On Tue, Apr 15, 2014 at 12:10 PM, Rob Clark robdclark@gmail.com wrote:
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.
That shouldn're really matter - if we have the fb assigned as the primary plane then the set_config_internal should clear crtc->primary->fb. Which means in the next loop (where we go over all planes) we won't look at it again. Or at least this is how it should work.
So dropping a ref twice due to that would indeed be a bug, but one in drm_crtc.c. I have to admit that I don't see it really. -Daniel
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Archit Taneja -
Daniel Vetter -
Rob Clark -
Tomi Valkeinen -
Ville Syrjälä