Re: [PATCH 00/48] omapdrm: Merge omapdrm and omapdss

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

On Wednesday, 18 October 2017 12:56:12 EET Tomi Valkeinen wrote:
> On 18/10/17 12:46, Tomi Valkeinen wrote:
> > On 13/10/17 17:58, Laurent Pinchart wrote:
> >> Hello,
> >> 
> >> This patch series merges the omapdrm and omapdss drivers into a single
> >> driver called omapdrm. The split in two drivers was historical, in order
> >> to support the FBDEV, V4L2 and DRM/KMS APIs. Now that the driver
> >> supports DRM/KMS only there's no need to keep two seperate drivers.

[snip]

> >> The series has been tested on a Pandaboard with the DVI and HDMI output.
> > 
> > Here's what I get on AM5 EVM:

[snip]

> [   14.783558] dmm 4e000000.dmm: initialized all PAT entries
> [   14.805775] DSS: OMAP DSS rev 6.1
> [   14.809989] omapdss_dss 58000000.dss: bound 58001000.dispc (ops
> dispc_component_ops [omapdrm])
> [   14.821844] omapdss_dss 58000000.dss: bound 58040000.encoder (ops
> hdmi5_component_ops [omapdrm])
> [   14.833482] omapdss_dss 58000000.dss: master bind failed: -517
> 
> When I remove modules, something is left enabled as I get:
> 
> [   99.623954] platform 58000000.dss: enabled after unload, idling

I initially thought this would disappear after fixing the other issues, but it 
seems to be a problem of its own, which I have thus investigated. I had to 
trace through the runtime PM code to understand what was going on, which 
wasn't a pleasant experience, but I managed to find the root cause and to 
create a fix.

The culprit is initialization of runtime PM in the DSS component master bind 
handler coupled with probe deferral from within that handler.

The bind handler performs the following sequence of PM operations:

        pm_runtime_enable(dev);
        pm_runtime_get_sync(dev);

        ... (access the hardware to read the device revision) ... 

        pm_runtime_put_sync(dev);

If a failure occurs at this point, the error path calls pm_runtime_disable() 
to balance the pm_runtime_enable() call.

Now it should be noted that the bind handler is called when one of the 
component registers itself, which happens in the component's probe handler. 
Furthermore, as the components are children of the DSS, the device core calls 
pm_runtime_get_sync() on the DSS platform device before calling the 
component's probe handler. This increases the DSS power usage count but 
doesn't runtime resume the device, as runtime PM is disabled at that point.

The bind handler is thus called with runtime PM disabled, with the device 
runtime suspended, but with the power usage count larger than 0. The 
pm_runtime_get_sync() call will thus further increase the power usage count 
and runtime resume the device. The pm_runtime_put_sync() handler will decrease 
the power usage count to a non-zero value and will thus not suspend the 
device. Finally, the pm_runtime_disable() call will disable runtime PM, 
preventing the pm_runtime_put() call in the device core from runtime 
suspending the device. The DSS device is thus left powered on. 

To fix this, I've created a patch that moves the runtime PM initialization 
code (as well as most of the rest of the initialization code) from the bind 
handler to the probe handler. I'll post it in v2 of this series.

-- 
Regards,

Laurent Pinchart

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