Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Javier,

On 2017-01-19 15:56, Javier Martinez Canillas wrote:
Thanks a lot for your feedback.

On 01/19/2017 11:17 AM, Marek Szyprowski wrote:
On 2017-01-18 01:30, Javier Martinez Canillas wrote:
Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when
CONFIG_PM is unset") removed the implicit dependency that the driver
had with CONFIG_PM, since it relied on the config option to be enabled.

In order to work with !CONFIG_PM, the GSC reset logic that happens in
the runtime resume callback had to be executed on the probe function.

The problem is that if CONFIG_PM is enabled, the power domain for the
GSC could be disabled and so an attempt to write to the GSC_SW_RESET
register leads to an unhandled fault / imprecise external abort error:
Driver core ensures that driver's probe() is called with respective power
domain turned on, so this is not the right reason for the proposed change.
Ok, I misunderstood the relationship between runtime PM and the power domains
then. I thought the power domains were only powered on when the runtime PM
framework resumed an associated device (i.e: pm_runtime_get_sync() is called).

Power domains are implemented transparently for the drivers. Even when driver doesn't support runtime pm, but its device is in the power domain, the core will ensure that the domain will be turned on all the time the driver is bound to the
device.

But even if this isn't the case, shouldn't the reset in probe only be needed
if CONFIG_PM isn't enabled? (IOW, $SUBJECT but with another commit message).

This looks like an over-engineering. I don't like polluting driver code with
conditional statements like IS_ENABLED(CONFIG_*). It should not hurt to reset
the device in driver probe, especially just in case the device was left in
some partially configured/working state by bootloader or previous kernel run
(if started from kexec). Adding this conditional code to avoid some issues
with power domain or clocks configuration also suggests that one should
instead solve the problem elsewhere. Driver should be able to access device
registers in its probe() in any case without the additional hacks.

[   10.178825] Unhandled fault: imprecise external abort (0x1406) at 0x00000000
[   10.186982] pgd = ed728000
[   10.190847] [00000000] *pgd=00000000
[   10.195553] Internal error: : 1406 [#1] PREEMPT SMP ARM
[   10.229761] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   10.237134] task: ed49e400 task.stack: ed724000
[   10.242934] PC is at gsc_wait_reset+0x5c/0x6c [exynos_gsc]
[   10.249710] LR is at gsc_probe+0x300/0x33c [exynos_gsc]
[   10.256139] pc : [<bf2429e0>]    lr : [<bf240734>]    psr: 60070013
[   10.256139] sp : ed725d30  ip : 00000000  fp : 00000001
[   10.271492] r10: eea74800  r9 : ecd6a2c0  r8 : ed7d8854
[   10.277912] r7 : ed7d8c08  r6 : ed7d8810  r5 : ffff8ecd  r4 : c0c03900
[   10.285664] r3 : 00000000  r2 : 00000001  r1 : ed7d8b98  r0 : ed7d8810

So only do a GSC reset if CONFIG_PM is disabled, since if is enabled the
runtime PM resume callback will be called by the VIDIOC_STREAMON ioctl,
making the reset in probe unneeded.

Fixes: 15f90ab57acc ("[media] exynos-gsc: Make driver functional when CONFIG_PM is unset")
Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
Frankly, I don't get why this change is needed.

Yes, it seems $SUBJECT is just papering over the real issue. There's
something really wrong with the Exynos power domains, I see that PDs
can't be disabled by the genpd framework, exynos_pd_power_off() fail:

# dmesg | grep power-domain
[    4.893318] Power domain power-domain@10044020 disable failed
[    4.893342] Power domain power-domain@10044120 disable failed
[    4.893711] Power domain power-domain@10044000 disable failed
[   12.690052] Power domain power-domain@10044000 disable failed
[   12.703963] Power domain power-domain@10044000 disable failed

So PD are kept on even when unused / attached devices are suspended.
Only the mfc_pd (power-domain@10044060) is correctly turned off.

# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain                          status          slaves
     /device                                             runtime status
----------------------------------------------------------------------
power-domain@100440C0           on
     /devices/platform/soc/14450000.mixer                active
     /devices/platform/soc/14530000.hdmi                 active
power-domain@10044120           on
power-domain@10044060           off-0
     /devices/platform/soc/11000000.codec                suspended
power-domain@10044020           on
power-domain@10044000           on
     /devices/platform/soc/13e00000.video-scaler         suspended
     /devices/platform/soc/13e10000.video-scaler         suspended

Also when removing the exynos_gsc driver, I get the same error:

# rmmod s5p_mfc
[  106.405972] s5p-mfc 11000000.codec: Removing 11000000.codec
# rmmod exynos_gsc
[  227.008559] Unhandled fault: imprecise external abort (0x1c06) at 0x00048e14
[  227.015116] pgd = ed5dc000
[  227.017213] [00048e14] *pgd=b17c6835
[  227.020889] Internal error: : 1c06 [#1] PREEMPT SMP ARM
...
[  227.241585] [<bf2429bc>] (gsc_wait_reset [exynos_gsc]) from [<bf24009c>] (gsc_runtime_resume+0x9c/0xec [exynos_gsc])
[  227.252331] [<bf24009c>] (gsc_runtime_resume [exynos_gsc]) from [<c042e488>] (genpd_runtime_resume+0x120/0x1d4)
[  227.262294] [<c042e488>] (genpd_runtime_resume) from [<c04241c0>] (__rpm_callback+0xc8/0x218)

# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain                          status          slaves
     /device                                             runtime status
----------------------------------------------------------------------
power-domain@100440C0           on
     /devices/platform/soc/14450000.mixer                active
     /devices/platform/soc/14530000.hdmi                 active
power-domain@10044120           on
power-domain@10044060           off-0
power-domain@10044020           on
power-domain@10044000           on
     /devices/platform/soc/13e00000.video-scaler         suspended
     /devices/platform/soc/13e10000.video-scaler         resuming

This seems to be caused by some needed clocks to access the power domains
to be gated, since I don't get these erros when passing clk_ignore_unused
as parameter in the kernel command line.

I think that those issues were fixes by the following patch:
https://patchwork.kernel.org/patch/9484607/
It still didn't reach mainline, but I hope it will go as a fix to v4.10.

Please test if this solves your issue. Please not that adding more clocks
to the power domain drivers will solve only the problem with turning domain
on/off, but the are more cases where those clocks should be turned on (like
IOMMU integration), so marking them as critical solves that problem too.

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux