Multiple issues with omapdrm

Multiple issues with omapdrm

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 6102 bytes --]

Hi,

I did some testing with omapdrm on v3.10-rc1. Here are some issues I encountered.
For most of them I don't have any idea where to even start looking for a problem,
so I hope that you may have some ideas.


dispc_runtime_get/put used in irq context
=========================================

dispc_runtime_get/put are used in irq context in omap_irq.c. I can hack
around that with if (!in_atomic()), but I have no idea yet how to fix
it properly.

drm_crtc warning
================

I got this once when unloading the modules, but haven't happened since:

drm_crtc.c line 3869

	WARN_ON(!list_empty(&dev->mode_config.fb_list));

As it only happened once, maybe some kind of race.


omap_gem warning
================

This happens on module unload:

[   30.167480] ------------[ cut here ]------------
[   30.167724] WARNING: at drivers/gpu/drm/omapdrm/omap_gem.c:1318 omap_gem_free_object+0x24c/0x25c [omapdrm]()
[   30.182952] Modules linked in: omapdrm(-) drm_kms_helper drm panel_taal panel_tfp410 panel_generic_dpi omapdss
[   30.183166] CPU: 0 PID: 6 Comm: kworker/u4:0 Not tainted 3.10.0-rc1-00004-g8ed4760 #110
[   30.183166] Workqueue: omapdrm apply_worker [omapdrm]
[   30.207641] Backtrace: 
[   30.210296] [<c00189ac>] (dump_backtrace+0x0/0x10c) from [<c0018b48>] (show_stack+0x18/0x1c)
[   30.219299]  r6:bf0e5988 r5:00000009 r4:00000000 r3:eb872080
[   30.225433] [<c0018b30>] (show_stack+0x0/0x1c) from [<c0520c04>] (dump_stack+0x20/0x28)
[   30.234039] [<c0520be4>] (dump_stack+0x0/0x28) from [<c0041fb8>] (warn_slowpath_common+0x54/0x74)
[   30.243530] [<c0041f64>] (warn_slowpath_common+0x0/0x74) from [<c0041ffc>] (warn_slowpath_null+0x24/0x2c)
[   30.243774]  r8:ebfab99c r7:ebb41800 r6:ebb41830 r5:ebefce40 r4:ebefce40
r3:00000009
[   30.262176] [<c0041fd8>] (warn_slowpath_null+0x0/0x2c) from [<bf0e5988>] (omap_gem_free_object+0x24c/0x25c [omapdrm])
[   30.273498] [<bf0e573c>] (omap_gem_free_object+0x0/0x25c [omapdrm]) from [<bf080c0c>] (drm_gem_object_free+0x30/0x38 [drm])
[   30.285675] [<bf080bdc>] (drm_gem_object_free+0x0/0x38 [drm]) from [<bf0e196c>] (omap_plane_post_apply+0x108/0x10c [omapdrm])
[   30.285675] [<bf0e1864>] (omap_plane_post_apply+0x0/0x10c [omapdrm]) from [<bf0e100c>] (apply_worker+0x68/0x188 [omapdrm])
[   30.309509]  r6:ebae5c6c r5:ebae5c5c r4:ebae5c5c
[   30.312042] [<bf0e0fa4>] (apply_worker+0x0/0x188 [omapdrm]) from [<c005ef10>] (process_one_work+0x1b8/0x4e8)
[   30.325012] [<c005ed58>] (process_one_work+0x0/0x4e8) from [<c005f654>] (worker_thread+0x138/0x388)
[   30.325378] [<c005f51c>] (worker_thread+0x0/0x388) from [<c0066070>] (kthread+0xac/0xb8)
[   30.343292] [<c0065fc4>] (kthread+0x0/0xb8) from [<c00146e8>] (ret_from_fork+0x14/0x2c)
[   30.351837]  r7:00000000 r6:00000000 r5:c0065fc4 r4:eb843d54
[   30.352111] ---[ end trace 78317663d3b4807c ]---


Deadlock at module unload
=========================
When removing the modules, there's a dealock. This is the last line printed:

[   30.399200] [drm] Module unloaded

Then, when the deadlock detection kicks in:

[  240.962432] INFO: task rmmod:894 blocked for more than 120 seconds.
[  240.962432] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  240.977508] rmmod           D c0524964     0   894    884 0x00000000
[  240.981719] Backtrace: 
[  240.987274] [<c05245ec>] (__schedule+0x0/0x7e8) from [<c0524eb0>] (schedule+0x38/0x78)
[  240.995819] [<c0524e78>] (schedule+0x0/0x78) from [<c05251ac>] (schedule_preempt_disabled+0x28/0x38)
[  241.005706] [<c0525184>] (schedule_preempt_disabled+0x0/0x38) from [<c05238c4>] (mutex_lock_nested+0x1b8/0x3a8)
[  241.016693]  r4:c07f5bf8 r3:eb978180
[  241.020812] [<c052370c>] (mutex_lock_nested+0x0/0x3a8) from [<c033c9d0>] (driver_detach+0x4c/0xc0)
[  241.021026] [<c033c984>] (driver_detach+0x0/0xc0) from [<c033bd24>] (bus_remove_driver+0x94/0xfc)
[  241.040100]  r6:c07f5b38 r5:bf0ec410 r4:00000000 r3:00000000
[  241.042633] [<c033bc90>] (bus_remove_driver+0x0/0xfc) from [<c033d054>] (driver_unregister+0x58/0x78)
[  241.056060]  r6:c07c28a4 r5:bf0ec410 r4:00000000 r3:eb978180
[  241.062164] [<c033cffc>] (driver_unregister+0x0/0x78) from [<c033ddc0>] (platform_driver_unregister+0x14/0x18)
[  241.072875]  r5:bf0ebc18 r4:c07c2860
[  241.075439] [<c033ddac>] (platform_driver_unregister+0x0/0x18) from [<bf0df2c8>] (pdev_remove+0x48/0x54 [omapdrm])
[  241.087921] [<bf0df280>] (pdev_remove+0x0/0x54 [omapdrm]) from [<c033dc38>] (platform_drv_remove+0x20/0x24)
[  241.098327]  r4:c07c2870 r3:bf0df280
[  241.100860] [<c033dc18>] (platform_drv_remove+0x0/0x24) from [<c033bfc4>] (__device_release_driver+0x78/0xd4)
[  241.112792] [<c033bf4c>] (__device_release_driver+0x0/0xd4) from [<c033ca40>] (driver_detach+0xbc/0xc0)
[  241.113159]  r5:bf0ebc18 r4:c07c2870
[  241.122833] [<c033c984>] (driver_detach+0x0/0xc0) from [<c033bd24>] (bus_remove_driver+0x94/0xfc)
[  241.136108]  r6:c07f5b38 r5:bf0ebc18 r4:00000000 r3:00000000
[  241.142211] [<c033bc90>] (bus_remove_driver+0x0/0xfc) from [<c033d054>] (driver_unregister+0x58/0x78)
[  241.152069]  r6:00000880 r5:bf0ebc18 r4:00000000 r3:00000000
[  241.153472] [<c033cffc>] (driver_unregister+0x0/0x78) from [<c033ddc0>] (platform_driver_unregister+0x14/0x18)
[  241.168823]  r5:bf0ec4a0 r4:00000000
[  241.171356] [<c033ddac>] (platform_driver_unregister+0x0/0x18) from [<bf0e9bb8>] (omap_drm_fini+0x28/0x3c [omapdrm])
[  241.183990] [<bf0e9b90>] (omap_drm_fini+0x0/0x3c [omapdrm]) from [<c009ef10>] (SyS_delete_module+0x154/0x288)
[  241.194610] [<c009edbc>] (SyS_delete_module+0x0/0x288) from [<c0014620>] (ret_fast_syscall+0x0/0x48)
[  241.204345]  r7:00000081 r6:006d7264 r5:70616d6f r4:0001aac4
[  241.206726] 3 locks held by rmmod/894:
[  241.214416]  #0:  (&__lockdep_no_validate__){......}, at: [<c033c9d0>] driver_detach+0x4c/0xc0
[  241.223693]  #1:  (&__lockdep_no_validate__){......}, at: [<c033c9dc>] driver_detach+0x58/0xc0
[  241.232940]  #2:  (&__lockdep_no_validate__){......}, at: [<c033c9d0>] driver_detach+0x4c/0xc0

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Multiple issues with omapdrm

[Rob Clark]

On Wed, Jun 5, 2013 at 4:59 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi,
>
> I did some testing with omapdrm on v3.10-rc1. Here are some issues I encountered.
> For most of them I don't have any idea where to even start looking for a problem,
> so I hope that you may have some ideas.
>
>
> dispc_runtime_get/put used in irq context
> =========================================
>
> dispc_runtime_get/put are used in irq context in omap_irq.c. I can hack
> around that with if (!in_atomic()), but I have no idea yet how to fix
> it properly.

Probably should have a flag/refcnt that is set when in the irq hander,
since we know that things are on in irq.

>
> drm_crtc warning
> ================
>
> I got this once when unloading the modules, but haven't happened since:
>
> drm_crtc.c line 3869
>
>         WARN_ON(!list_empty(&dev->mode_config.fb_list));
>
> As it only happened once, maybe some kind of race.
>

Hmm, this is new..  it is certain that there can be a fb with a
reference still around, waiting for a vblank/framedone.  But I think
it should already be removed from userspace perspective and no longer
on fb_list.  I think I need to go back and have a closer look at
2b677e8c0 where Daniel introduced that WARN_ON().

> omap_gem warning
> ================
>
> This happens on module unload:

oh, that is easy.. it is because the omap_fbdev.c does an unbalanced
omap_gem_get_paddr() to keep the fbcon fb from getting unmapped from
tiler (so if we have to restore fbcon mode in an opps, we don't have
to worry about grabbing mutexes or anything like that).  Maybe
somewhere in the cleanup it should do a put_paddr().  Otoh, I have
some skepticism about whether module unloading is really going to be
that reliable in practice, so meh..


BR,
-R

>
> [   30.167480] ------------[ cut here ]------------
> [   30.167724] WARNING: at drivers/gpu/drm/omapdrm/omap_gem.c:1318 omap_gem_free_object+0x24c/0x25c [omapdrm]()
> [   30.182952] Modules linked in: omapdrm(-) drm_kms_helper drm panel_taal panel_tfp410 panel_generic_dpi omapdss
> [   30.183166] CPU: 0 PID: 6 Comm: kworker/u4:0 Not tainted 3.10.0-rc1-00004-g8ed4760 #110
> [   30.183166] Workqueue: omapdrm apply_worker [omapdrm]
> [   30.207641] Backtrace:
> [   30.210296] [<c00189ac>] (dump_backtrace+0x0/0x10c) from [<c0018b48>] (show_stack+0x18/0x1c)
> [   30.219299]  r6:bf0e5988 r5:00000009 r4:00000000 r3:eb872080
> [   30.225433] [<c0018b30>] (show_stack+0x0/0x1c) from [<c0520c04>] (dump_stack+0x20/0x28)
> [   30.234039] [<c0520be4>] (dump_stack+0x0/0x28) from [<c0041fb8>] (warn_slowpath_common+0x54/0x74)
> [   30.243530] [<c0041f64>] (warn_slowpath_common+0x0/0x74) from [<c0041ffc>] (warn_slowpath_null+0x24/0x2c)
> [   30.243774]  r8:ebfab99c r7:ebb41800 r6:ebb41830 r5:ebefce40 r4:ebefce40
> r3:00000009
> [   30.262176] [<c0041fd8>] (warn_slowpath_null+0x0/0x2c) from [<bf0e5988>] (omap_gem_free_object+0x24c/0x25c [omapdrm])
> [   30.273498] [<bf0e573c>] (omap_gem_free_object+0x0/0x25c [omapdrm]) from [<bf080c0c>] (drm_gem_object_free+0x30/0x38 [drm])
> [   30.285675] [<bf080bdc>] (drm_gem_object_free+0x0/0x38 [drm]) from [<bf0e196c>] (omap_plane_post_apply+0x108/0x10c [omapdrm])
> [   30.285675] [<bf0e1864>] (omap_plane_post_apply+0x0/0x10c [omapdrm]) from [<bf0e100c>] (apply_worker+0x68/0x188 [omapdrm])
> [   30.309509]  r6:ebae5c6c r5:ebae5c5c r4:ebae5c5c
> [   30.312042] [<bf0e0fa4>] (apply_worker+0x0/0x188 [omapdrm]) from [<c005ef10>] (process_one_work+0x1b8/0x4e8)
> [   30.325012] [<c005ed58>] (process_one_work+0x0/0x4e8) from [<c005f654>] (worker_thread+0x138/0x388)
> [   30.325378] [<c005f51c>] (worker_thread+0x0/0x388) from [<c0066070>] (kthread+0xac/0xb8)
> [   30.343292] [<c0065fc4>] (kthread+0x0/0xb8) from [<c00146e8>] (ret_from_fork+0x14/0x2c)
> [   30.351837]  r7:00000000 r6:00000000 r5:c0065fc4 r4:eb843d54
> [   30.352111] ---[ end trace 78317663d3b4807c ]---
>
>
> Deadlock at module unload
> =========================
> When removing the modules, there's a dealock. This is the last line printed:
>
> [   30.399200] [drm] Module unloaded
>
> Then, when the deadlock detection kicks in:
>
> [  240.962432] INFO: task rmmod:894 blocked for more than 120 seconds.
> [  240.962432] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> [  240.977508] rmmod           D c0524964     0   894    884 0x00000000
> [  240.981719] Backtrace:
> [  240.987274] [<c05245ec>] (__schedule+0x0/0x7e8) from [<c0524eb0>] (schedule+0x38/0x78)
> [  240.995819] [<c0524e78>] (schedule+0x0/0x78) from [<c05251ac>] (schedule_preempt_disabled+0x28/0x38)
> [  241.005706] [<c0525184>] (schedule_preempt_disabled+0x0/0x38) from [<c05238c4>] (mutex_lock_nested+0x1b8/0x3a8)
> [  241.016693]  r4:c07f5bf8 r3:eb978180
> [  241.020812] [<c052370c>] (mutex_lock_nested+0x0/0x3a8) from [<c033c9d0>] (driver_detach+0x4c/0xc0)
> [  241.021026] [<c033c984>] (driver_detach+0x0/0xc0) from [<c033bd24>] (bus_remove_driver+0x94/0xfc)
> [  241.040100]  r6:c07f5b38 r5:bf0ec410 r4:00000000 r3:00000000
> [  241.042633] [<c033bc90>] (bus_remove_driver+0x0/0xfc) from [<c033d054>] (driver_unregister+0x58/0x78)
> [  241.056060]  r6:c07c28a4 r5:bf0ec410 r4:00000000 r3:eb978180
> [  241.062164] [<c033cffc>] (driver_unregister+0x0/0x78) from [<c033ddc0>] (platform_driver_unregister+0x14/0x18)
> [  241.072875]  r5:bf0ebc18 r4:c07c2860
> [  241.075439] [<c033ddac>] (platform_driver_unregister+0x0/0x18) from [<bf0df2c8>] (pdev_remove+0x48/0x54 [omapdrm])
> [  241.087921] [<bf0df280>] (pdev_remove+0x0/0x54 [omapdrm]) from [<c033dc38>] (platform_drv_remove+0x20/0x24)
> [  241.098327]  r4:c07c2870 r3:bf0df280
> [  241.100860] [<c033dc18>] (platform_drv_remove+0x0/0x24) from [<c033bfc4>] (__device_release_driver+0x78/0xd4)
> [  241.112792] [<c033bf4c>] (__device_release_driver+0x0/0xd4) from [<c033ca40>] (driver_detach+0xbc/0xc0)
> [  241.113159]  r5:bf0ebc18 r4:c07c2870
> [  241.122833] [<c033c984>] (driver_detach+0x0/0xc0) from [<c033bd24>] (bus_remove_driver+0x94/0xfc)
> [  241.136108]  r6:c07f5b38 r5:bf0ebc18 r4:00000000 r3:00000000
> [  241.142211] [<c033bc90>] (bus_remove_driver+0x0/0xfc) from [<c033d054>] (driver_unregister+0x58/0x78)
> [  241.152069]  r6:00000880 r5:bf0ebc18 r4:00000000 r3:00000000
> [  241.153472] [<c033cffc>] (driver_unregister+0x0/0x78) from [<c033ddc0>] (platform_driver_unregister+0x14/0x18)
> [  241.168823]  r5:bf0ec4a0 r4:00000000
> [  241.171356] [<c033ddac>] (platform_driver_unregister+0x0/0x18) from [<bf0e9bb8>] (omap_drm_fini+0x28/0x3c [omapdrm])
> [  241.183990] [<bf0e9b90>] (omap_drm_fini+0x0/0x3c [omapdrm]) from [<c009ef10>] (SyS_delete_module+0x154/0x288)
> [  241.194610] [<c009edbc>] (SyS_delete_module+0x0/0x288) from [<c0014620>] (ret_fast_syscall+0x0/0x48)
> [  241.204345]  r7:00000081 r6:006d7264 r5:70616d6f r4:0001aac4
> [  241.206726] 3 locks held by rmmod/894:
> [  241.214416]  #0:  (&__lockdep_no_validate__){......}, at: [<c033c9d0>] driver_detach+0x4c/0xc0
> [  241.223693]  #1:  (&__lockdep_no_validate__){......}, at: [<c033c9dc>] driver_detach+0x58/0xc0
> [  241.232940]  #2:  (&__lockdep_no_validate__){......}, at: [<c033c9d0>] driver_detach+0x4c/0xc0
>
>  Tomi
>

Re: Multiple issues with omapdrm

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1713 bytes --]

On 05/06/13 13:43, Rob Clark wrote:
> On Wed, Jun 5, 2013 at 4:59 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> Hi,
>>
>> I did some testing with omapdrm on v3.10-rc1. Here are some issues I encountered.
>> For most of them I don't have any idea where to even start looking for a problem,
>> so I hope that you may have some ideas.
>>
>>
>> dispc_runtime_get/put used in irq context
>> =========================================
>>
>> dispc_runtime_get/put are used in irq context in omap_irq.c. I can hack
>> around that with if (!in_atomic()), but I have no idea yet how to fix
>> it properly.
> 
> Probably should have a flag/refcnt that is set when in the irq hander,
> since we know that things are on in irq.

Maybe.

However, I'd like more an approach where the dispc registers are only
programmed when a display is enabled. If the display in question is
disabled, no registers would be written.

I think that would also support losing DISPC register context. The DISPC
driver currently stores all its registers when the pm_runtime refcount
goes to 0, and restores them on pm_runtime_get. That's a bit heavy, and
uses the OMAP specific ctx-loss-count support, which should be removed
(and if that's removed, the register save/restore becomes even heavier).

Things would be simpler if omapdrm (and omapfb) knows how to program all
the relevant registers when a display is enabled, and keeps the
necessary state stored in memory (including irq settings).

> somewhere in the cleanup it should do a put_paddr().  Otoh, I have
> some skepticism about whether module unloading is really going to be
> that reliable in practice, so meh..

Why is that?

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Multiple issues with omapdrm

[Rob Clark]

On Wed, Jun 5, 2013 at 6:43 AM, Rob Clark <robdclark@gmail.com> wrote:
> On Wed, Jun 5, 2013 at 4:59 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> Hi,
>>
>> I did some testing with omapdrm on v3.10-rc1. Here are some issues I encountered.
>> For most of them I don't have any idea where to even start looking for a problem,
>> so I hope that you may have some ideas.
>>
>>
>> dispc_runtime_get/put used in irq context
>> =========================================
>>
>> dispc_runtime_get/put are used in irq context in omap_irq.c. I can hack
>> around that with if (!in_atomic()), but I have no idea yet how to fix
>> it properly.
>
> Probably should have a flag/refcnt that is set when in the irq hander,
> since we know that things are on in irq.
>
>>
>> drm_crtc warning
>> ================
>>
>> I got this once when unloading the modules, but haven't happened since:
>>
>> drm_crtc.c line 3869
>>
>>         WARN_ON(!list_empty(&dev->mode_config.fb_list));
>>
>> As it only happened once, maybe some kind of race.
>>
>
> Hmm, this is new..  it is certain that there can be a fb with a
> reference still around, waiting for a vblank/framedone.  But I think
> it should already be removed from userspace perspective and no longer
> on fb_list.  I think I need to go back and have a closer look at
> 2b677e8c0 where Daniel introduced that WARN_ON().
>
>> omap_gem warning
>> ================
>>
>> This happens on module unload:
>
> oh, that is easy.. it is because the omap_fbdev.c does an unbalanced
> omap_gem_get_paddr() to keep the fbcon fb from getting unmapped from
> tiler (so if we have to restore fbcon mode in an opps, we don't have
> to worry about grabbing mutexes or anything like that).  Maybe
> somewhere in the cleanup it should do a put_paddr().  Otoh, I have
> some skepticism about whether module unloading is really going to be
> that reliable in practice, so meh..
>

by which I mean: module unloading is a nice-to-have for development,
but not a real use case.. it is more important to fix the issues we
have with module loading:

1) drmOpen("omap") will try to modprobe "omap", not "omapdrm" so we
need to rename the .ko
2) sorting out the modprobe of panel drivers..  although with the
current structure of omapdrm+omapdss I can't think of any clean way to
handle this.  I suppose we could do a hack with a bunch of
request_module()s

these are issues that actually cause problems for distros which want
to build a unified kernel with all the drm drivers as modules

BR,
-R

>
> BR,
> -R
>
>>
>> [   30.167480] ------------[ cut here ]------------
>> [   30.167724] WARNING: at drivers/gpu/drm/omapdrm/omap_gem.c:1318 omap_gem_free_object+0x24c/0x25c [omapdrm]()
>> [   30.182952] Modules linked in: omapdrm(-) drm_kms_helper drm panel_taal panel_tfp410 panel_generic_dpi omapdss
>> [   30.183166] CPU: 0 PID: 6 Comm: kworker/u4:0 Not tainted 3.10.0-rc1-00004-g8ed4760 #110
>> [   30.183166] Workqueue: omapdrm apply_worker [omapdrm]
>> [   30.207641] Backtrace:
>> [   30.210296] [<c00189ac>] (dump_backtrace+0x0/0x10c) from [<c0018b48>] (show_stack+0x18/0x1c)
>> [   30.219299]  r6:bf0e5988 r5:00000009 r4:00000000 r3:eb872080
>> [   30.225433] [<c0018b30>] (show_stack+0x0/0x1c) from [<c0520c04>] (dump_stack+0x20/0x28)
>> [   30.234039] [<c0520be4>] (dump_stack+0x0/0x28) from [<c0041fb8>] (warn_slowpath_common+0x54/0x74)
>> [   30.243530] [<c0041f64>] (warn_slowpath_common+0x0/0x74) from [<c0041ffc>] (warn_slowpath_null+0x24/0x2c)
>> [   30.243774]  r8:ebfab99c r7:ebb41800 r6:ebb41830 r5:ebefce40 r4:ebefce40
>> r3:00000009
>> [   30.262176] [<c0041fd8>] (warn_slowpath_null+0x0/0x2c) from [<bf0e5988>] (omap_gem_free_object+0x24c/0x25c [omapdrm])
>> [   30.273498] [<bf0e573c>] (omap_gem_free_object+0x0/0x25c [omapdrm]) from [<bf080c0c>] (drm_gem_object_free+0x30/0x38 [drm])
>> [   30.285675] [<bf080bdc>] (drm_gem_object_free+0x0/0x38 [drm]) from [<bf0e196c>] (omap_plane_post_apply+0x108/0x10c [omapdrm])
>> [   30.285675] [<bf0e1864>] (omap_plane_post_apply+0x0/0x10c [omapdrm]) from [<bf0e100c>] (apply_worker+0x68/0x188 [omapdrm])
>> [   30.309509]  r6:ebae5c6c r5:ebae5c5c r4:ebae5c5c
>> [   30.312042] [<bf0e0fa4>] (apply_worker+0x0/0x188 [omapdrm]) from [<c005ef10>] (process_one_work+0x1b8/0x4e8)
>> [   30.325012] [<c005ed58>] (process_one_work+0x0/0x4e8) from [<c005f654>] (worker_thread+0x138/0x388)
>> [   30.325378] [<c005f51c>] (worker_thread+0x0/0x388) from [<c0066070>] (kthread+0xac/0xb8)
>> [   30.343292] [<c0065fc4>] (kthread+0x0/0xb8) from [<c00146e8>] (ret_from_fork+0x14/0x2c)
>> [   30.351837]  r7:00000000 r6:00000000 r5:c0065fc4 r4:eb843d54
>> [   30.352111] ---[ end trace 78317663d3b4807c ]---
>>
>>
>> Deadlock at module unload
>> =========================
>> When removing the modules, there's a dealock. This is the last line printed:
>>
>> [   30.399200] [drm] Module unloaded
>>
>> Then, when the deadlock detection kicks in:
>>
>> [  240.962432] INFO: task rmmod:894 blocked for more than 120 seconds.
>> [  240.962432] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>> [  240.977508] rmmod           D c0524964     0   894    884 0x00000000
>> [  240.981719] Backtrace:
>> [  240.987274] [<c05245ec>] (__schedule+0x0/0x7e8) from [<c0524eb0>] (schedule+0x38/0x78)
>> [  240.995819] [<c0524e78>] (schedule+0x0/0x78) from [<c05251ac>] (schedule_preempt_disabled+0x28/0x38)
>> [  241.005706] [<c0525184>] (schedule_preempt_disabled+0x0/0x38) from [<c05238c4>] (mutex_lock_nested+0x1b8/0x3a8)
>> [  241.016693]  r4:c07f5bf8 r3:eb978180
>> [  241.020812] [<c052370c>] (mutex_lock_nested+0x0/0x3a8) from [<c033c9d0>] (driver_detach+0x4c/0xc0)
>> [  241.021026] [<c033c984>] (driver_detach+0x0/0xc0) from [<c033bd24>] (bus_remove_driver+0x94/0xfc)
>> [  241.040100]  r6:c07f5b38 r5:bf0ec410 r4:00000000 r3:00000000
>> [  241.042633] [<c033bc90>] (bus_remove_driver+0x0/0xfc) from [<c033d054>] (driver_unregister+0x58/0x78)
>> [  241.056060]  r6:c07c28a4 r5:bf0ec410 r4:00000000 r3:eb978180
>> [  241.062164] [<c033cffc>] (driver_unregister+0x0/0x78) from [<c033ddc0>] (platform_driver_unregister+0x14/0x18)
>> [  241.072875]  r5:bf0ebc18 r4:c07c2860
>> [  241.075439] [<c033ddac>] (platform_driver_unregister+0x0/0x18) from [<bf0df2c8>] (pdev_remove+0x48/0x54 [omapdrm])
>> [  241.087921] [<bf0df280>] (pdev_remove+0x0/0x54 [omapdrm]) from [<c033dc38>] (platform_drv_remove+0x20/0x24)
>> [  241.098327]  r4:c07c2870 r3:bf0df280
>> [  241.100860] [<c033dc18>] (platform_drv_remove+0x0/0x24) from [<c033bfc4>] (__device_release_driver+0x78/0xd4)
>> [  241.112792] [<c033bf4c>] (__device_release_driver+0x0/0xd4) from [<c033ca40>] (driver_detach+0xbc/0xc0)
>> [  241.113159]  r5:bf0ebc18 r4:c07c2870
>> [  241.122833] [<c033c984>] (driver_detach+0x0/0xc0) from [<c033bd24>] (bus_remove_driver+0x94/0xfc)
>> [  241.136108]  r6:c07f5b38 r5:bf0ebc18 r4:00000000 r3:00000000
>> [  241.142211] [<c033bc90>] (bus_remove_driver+0x0/0xfc) from [<c033d054>] (driver_unregister+0x58/0x78)
>> [  241.152069]  r6:00000880 r5:bf0ebc18 r4:00000000 r3:00000000
>> [  241.153472] [<c033cffc>] (driver_unregister+0x0/0x78) from [<c033ddc0>] (platform_driver_unregister+0x14/0x18)
>> [  241.168823]  r5:bf0ec4a0 r4:00000000
>> [  241.171356] [<c033ddac>] (platform_driver_unregister+0x0/0x18) from [<bf0e9bb8>] (omap_drm_fini+0x28/0x3c [omapdrm])
>> [  241.183990] [<bf0e9b90>] (omap_drm_fini+0x0/0x3c [omapdrm]) from [<c009ef10>] (SyS_delete_module+0x154/0x288)
>> [  241.194610] [<c009edbc>] (SyS_delete_module+0x0/0x288) from [<c0014620>] (ret_fast_syscall+0x0/0x48)
>> [  241.204345]  r7:00000081 r6:006d7264 r5:70616d6f r4:0001aac4
>> [  241.206726] 3 locks held by rmmod/894:
>> [  241.214416]  #0:  (&__lockdep_no_validate__){......}, at: [<c033c9d0>] driver_detach+0x4c/0xc0
>> [  241.223693]  #1:  (&__lockdep_no_validate__){......}, at: [<c033c9dc>] driver_detach+0x58/0xc0
>> [  241.232940]  #2:  (&__lockdep_no_validate__){......}, at: [<c033c9d0>] driver_detach+0x4c/0xc0
>>
>>  Tomi
>>

Re: Multiple issues with omapdrm

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1102 bytes --]

On 05/06/13 13:57, Rob Clark wrote:

> 1) drmOpen("omap") will try to modprobe "omap", not "omapdrm" so we
> need to rename the .ko

Has something been changed that broke that? Or was "omapdrm" just a
badly chosen name from the start? If drmOpen("omapdrm") works now,
doesn't changing the module name break userspace compatibility?

I had a quick look at libdrm. It calls server_info->load_module() but I
couldn't figure out where that call actually goes...

> 2) sorting out the modprobe of panel drivers..  although with the
> current structure of omapdrm+omapdss I can't think of any clean way to
> handle this.  I suppose we could do a hack with a bunch of
> request_module()s

If omapdrm and omapdss were merged, what would be the clean way be? Or
did you mean some other structure?

I'm no expert on this, but my understanding is that udev (or such)
should load the modules for the devices that the board has. If it's a
requirement that the drm drivers are loaded only by a call to drmOpen()
(why is that?), then maybe udev could load only the panel drivers.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Multiple issues with omapdrm

[Rob Clark]

On Wed, Jun 5, 2013 at 7:35 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 05/06/13 13:57, Rob Clark wrote:
>
>> 1) drmOpen("omap") will try to modprobe "omap", not "omapdrm" so we
>> need to rename the .ko
>
> Has something been changed that broke that? Or was "omapdrm" just a
> badly chosen name from the start? If drmOpen("omapdrm") works now,
> doesn't changing the module name break userspace compatibility?

it was broken all along, because you need to drmOpen("omap") so the
module should have been called omap.ko from the beginning

> I had a quick look at libdrm. It calls server_info->load_module() but I
> couldn't figure out where that call actually goes...

it's registered from xserver, fyi

>> 2) sorting out the modprobe of panel drivers..  although with the
>> current structure of omapdrm+omapdss I can't think of any clean way to
>> handle this.  I suppose we could do a hack with a bunch of
>> request_module()s
>
> If omapdrm and omapdss were merged, what would be the clean way be? Or
> did you mean some other structure?

well, then we'd just build it all into one .ko

> I'm no expert on this, but my understanding is that udev (or such)
> should load the modules for the devices that the board has. If it's a
> requirement that the drm drivers are loaded only by a call to drmOpen()
> (why is that?), then maybe udev could load only the panel drivers.

I'm not quite sure the entire history offhand.. maybe drmOpen()
pre-dated that?  At any rate, the main issue is just ensuring the
panel modules (if they are split out into different .ko's) are loaded
first

BR,
-R

>  Tomi
>
>

Re: Multiple issues with omapdrm

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 2962 bytes --]

On 05/06/13 14:52, Rob Clark wrote:
> On Wed, Jun 5, 2013 at 7:35 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On 05/06/13 13:57, Rob Clark wrote:
>>
>>> 1) drmOpen("omap") will try to modprobe "omap", not "omapdrm" so we
>>> need to rename the .ko
>>
>> Has something been changed that broke that? Or was "omapdrm" just a
>> badly chosen name from the start? If drmOpen("omapdrm") works now,
>> doesn't changing the module name break userspace compatibility?
> 
> it was broken all along, because you need to drmOpen("omap") so the
> module should have been called omap.ko from the beginning

But it wasn't, so wouldn't changing it break userspace compatibility?

Why do you "need" drmOpen("omap")? Is it just a convention to use that
kind of name, instead of "omapdrm" style name?

>> I had a quick look at libdrm. It calls server_info->load_module() but I
>> couldn't figure out where that call actually goes...
> 
> it's registered from xserver, fyi

Ah ok. So in my testing, running without X, there's actually no module
loading happening. The test tools print something like "trying to load
module omapdrm...success." but I guess that only means that the module
is already there.

>>> 2) sorting out the modprobe of panel drivers..  although with the
>>> current structure of omapdrm+omapdss I can't think of any clean way to
>>> handle this.  I suppose we could do a hack with a bunch of
>>> request_module()s
>>
>> If omapdrm and omapdss were merged, what would be the clean way be? Or
>> did you mean some other structure?
> 
> well, then we'd just build it all into one .ko

So you didn't actually mean omapdrm+omapdss, but omapdrm + omapdss +
all-the-panel-drivers? And then have some kind of forced method to probe
the panels at omapdrm start?

That would be quite a horrible solution, in my opinion =). It would also
be even worse when we get platform independent panel drivers, as all the
drm drivers using those panel drivers would include all the panel drivers.

>> I'm no expert on this, but my understanding is that udev (or such)
>> should load the modules for the devices that the board has. If it's a
>> requirement that the drm drivers are loaded only by a call to drmOpen()
>> (why is that?), then maybe udev could load only the panel drivers.
> 
> I'm not quite sure the entire history offhand.. maybe drmOpen()
> pre-dated that?  At any rate, the main issue is just ensuring the
> panel modules (if they are split out into different .ko's) are loaded
> first

I think there should be a core display facility in the kernel to which
the panel drivers register the displays in probe().

At boot time udev would go through /sys, find the panel devices somehow
(how? modalias is probably somehow involved..), and load their
respective modules.

Later, when X is started, omapdrm and omapdss are loaded, and at that
point all the panels are already there.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Multiple issues with omapdrm

[Rob Clark]

On Wed, Jun 5, 2013 at 8:16 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 05/06/13 14:52, Rob Clark wrote:
>> On Wed, Jun 5, 2013 at 7:35 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> On 05/06/13 13:57, Rob Clark wrote:
>>>
>>>> 1) drmOpen("omap") will try to modprobe "omap", not "omapdrm" so we
>>>> need to rename the .ko
>>>
>>> Has something been changed that broke that? Or was "omapdrm" just a
>>> badly chosen name from the start? If drmOpen("omapdrm") works now,
>>> doesn't changing the module name break userspace compatibility?
>>
>> it was broken all along, because you need to drmOpen("omap") so the
>> module should have been called omap.ko from the beginning
>
> But it wasn't, so wouldn't changing it break userspace compatibility?

could be udev or something else was triggering the module to load?
Not 100% sure offhand.

> Why do you "need" drmOpen("omap")? Is it just a convention to use that
> kind of name, instead of "omapdrm" style name?

all of the /dev/dri/cardN get opened, and then DRM_IOCTL_VERSION ioctl
to get the driver name/version, which is compared against the name
passed in to drmOpen().  So drmOpen("omapdrm") shouldn't really work

>>> I had a quick look at libdrm. It calls server_info->load_module() but I
>>> couldn't figure out where that call actually goes...
>>
>> it's registered from xserver, fyi
>
> Ah ok. So in my testing, running without X, there's actually no module
> loading happening. The test tools print something like "trying to load
> module omapdrm...success." but I guess that only means that the module
> is already there.

right

>>>> 2) sorting out the modprobe of panel drivers..  although with the
>>>> current structure of omapdrm+omapdss I can't think of any clean way to
>>>> handle this.  I suppose we could do a hack with a bunch of
>>>> request_module()s
>>>
>>> If omapdrm and omapdss were merged, what would be the clean way be? Or
>>> did you mean some other structure?
>>
>> well, then we'd just build it all into one .ko
>
> So you didn't actually mean omapdrm+omapdss, but omapdrm + omapdss +
> all-the-panel-drivers? And then have some kind of forced method to probe
> the panels at omapdrm start?

oh, yeah.  Probably whenever I say 'omapdss' I actually mean
'everything under drivers/video/omap2 except omapfb'

> That would be quite a horrible solution, in my opinion =). It would also
> be even worse when we get platform independent panel drivers, as all the
> drm drivers using those panel drivers would include all the panel drivers.

yeah, it isn't really ideal.. really we'd want them as separate
modules but some sort of dependency to ensure that the panel drivers
actually get loaded.

>>> I'm no expert on this, but my understanding is that udev (or such)
>>> should load the modules for the devices that the board has. If it's a
>>> requirement that the drm drivers are loaded only by a call to drmOpen()
>>> (why is that?), then maybe udev could load only the panel drivers.
>>
>> I'm not quite sure the entire history offhand.. maybe drmOpen()
>> pre-dated that?  At any rate, the main issue is just ensuring the
>> panel modules (if they are split out into different .ko's) are loaded
>> first
>
> I think there should be a core display facility in the kernel to which
> the panel drivers register the displays in probe().
>
> At boot time udev would go through /sys, find the panel devices somehow
> (how? modalias is probably somehow involved..), and load their
> respective modules.
>
> Later, when X is started, omapdrm and omapdss are loaded, and at that
> point all the panels are already there.

yeah, as long as we get the panels (and encoders/bridges/etc) loaded
first, I think it should be ok..

I'm not hugely fond of having it split out of drm, since I think that
just results extra layering and glue, and it makes it harder to
iteratively evolve the infrastructure for sharing panel stuff via
different trees from drm.  But that is a different discussion and is
mostly about where the code lives.

BR,
-R

>  Tomi
>
>

Re: Multiple issues with omapdrm

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1062 bytes --]

On 05/06/13 17:58, Rob Clark wrote:

> could be udev or something else was triggering the module to load?
> Not 100% sure offhand.

Well in my case I load them manually. But if there are applications out
there, that at the moment use drmOpen("omapdrm"), then the change would
break those apps.

>> Why do you "need" drmOpen("omap")? Is it just a convention to use that
>> kind of name, instead of "omapdrm" style name?
> 
> all of the /dev/dri/cardN get opened, and then DRM_IOCTL_VERSION ioctl
> to get the driver name/version, which is compared against the name
> passed in to drmOpen().  So drmOpen("omapdrm") shouldn't really work

Ok, so normally nobody will call drmOpen("omapdrm") explicitly, but
somebody (X?) will just do what you described to get the modules loaded?
If so, I guess it's unlikely that there would be an app with
drmOpen("omapdrm").

Of course, we could also rename the name returned via DRM_IOCTL_VERSION
to be "omapdrm", right? But looking at the names of other drivers,
"omap" is probably better.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Multiple issues with omapdrm

[Rob Clark]

On Thu, Jun 6, 2013 at 3:14 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 05/06/13 17:58, Rob Clark wrote:
>
>> could be udev or something else was triggering the module to load?
>> Not 100% sure offhand.
>
> Well in my case I load them manually. But if there are applications out
> there, that at the moment use drmOpen("omapdrm"), then the change would
> break those apps.

well, drmOpen("omapdrm") shouldn't currently work

>>> Why do you "need" drmOpen("omap")? Is it just a convention to use that
>>> kind of name, instead of "omapdrm" style name?
>>
>> all of the /dev/dri/cardN get opened, and then DRM_IOCTL_VERSION ioctl
>> to get the driver name/version, which is compared against the name
>> passed in to drmOpen().  So drmOpen("omapdrm") shouldn't really work
>
> Ok, so normally nobody will call drmOpen("omapdrm") explicitly, but
> somebody (X?) will just do what you described to get the modules loaded?
> If so, I guess it's unlikely that there would be an app with
> drmOpen("omapdrm").
>
> Of course, we could also rename the name returned via DRM_IOCTL_VERSION
> to be "omapdrm", right? But looking at the names of other drivers,
> "omap" is probably better.

changing the name would also be an abi break ;-)

BR,
-R

>  Tomi
>
>

Re: Multiple issues with omapdrm

[Dave Airlie]

On Thu, Jun 6, 2013 at 9:25 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Thu, Jun 6, 2013 at 3:\
>>>> Why do you "need" drmOpen("omap")? Is it just a convention to use that
>>>> kind of name, instead of "omapdrm" style name?
>>>
>>> all of the /dev/dri/cardN get opened, and then DRM_IOCTL_VERSION ioctl
>>> to get the driver name/version, which is compared against the name
>>> passed in to drmOpen().  So drmOpen("omapdrm") shouldn't really work
>>
>> Ok, so normally nobody will call drmOpen("omapdrm") explicitly, but
>> somebody (X?) will just do what you described to get the modules loaded?
>> If so, I guess it's unlikely that there would be an app with
>> drmOpen("omapdrm").
>>

These days X shouldn't be loading anything, that is DRI1 left overs.

The kernel/udev should be loading all the drivers well before X starts.

Dave.