[PATCH 0/3] omapdrm: Fix runtime PM issues at module load and unload time

[Laurent Pinchart]

Ho Tomi,

On Thursday, 1 November 2018 13:47:40 EET Tomi Valkeinen wrote:
> On 01/11/18 12:25, Laurent Pinchart wrote:
> > Hello,
> > 
> > This series fixes crashes in the omapdss driver at both load and unload
> > time, due to runtime PM problems related to probe deferral. The bugs got
> > introduced in v4.20-rc, this should thus be considered as v4.20 fixes.
> > 
> > At the core of the problem comes commit 27d624527d99 ("drm/omap: dss:
> > Acquire next dssdev at probe time") which can cause probe deferral for
> > the DSS when the encoder and panel modules are not loaded yet. As the
> > omapdss module contains drivers for the DSS and all its children, this
> > results in the internal encoders being probed before the DSS. Missing
> > runtime PM handling around register access then caused imprecise
> > external aborts.
> > 
> > Patch 1/3 moves population of the DSS children from arch code to the
> > omapdss driver. This prevents the DSS children from being probed before
> > the DSS. The change can be considered as a workaround in the sense that
> > runtime PM of the DSS children should operate correctly even when the
> > DSS probe fail. However, given that reducing the amount of arch code is
> > an improvement in itself, I believe the solution to be acceptable.
> > 
> > Patches 2/3 and 3/3 then ensure that the HDMI4 and DSI devices are
> > active at bind and probe time respectively, in order to access hardware
> > registers there.
> > 
> > Tony, patch 1/3 touches both the mach-omap2 and omapdss. Would you be
> > fine merging it through the DRM tree ? I don't think there's a risk of
> > conflict during the v4.20-rc cycle.
> 
> Thanks for debugging this! I have to say I really don't like these
> (well, 2 is fine), as they feel like hacks.

I assume you also have nothing against the first hunk of patch 3/3.

> We do dispc_runtime_get/put in the HDMI driver's suspend/resume too, so
> don't we need similar hack (as you add in dsi.c) there also?

We would if we had to access HDMI registers at probe time.

> So we have two problems (aside missing runtime_get/put calls in those
> few places):
> 
> - All DSS submodules depend on DSS (dss core)
> 
> - All DSS encoders depend on DISPC. The dependency is there only when
> we're starting the video stream.
> 
> Can we not defer the probe of the submodules until the dependencies have
> been probed?

That would be difficult, as the submodules don't know about the DSS. It goes 
the other way round, it's the DSS that gathers a list of submodules.

Furthermore the DSS probe is deferred due to the devices connected to the DPI 
and SDI outputs not being probed yet. See dpi_init_output_port(), the call to 
omapdss_of_find_connected_device() returns -EPROBE_DEFER. This is why the DSS 
probe is deferred if you load the omapdss module before all the other modules 
for the external components. Getting hold if the next device in the chain was 
needed to reverse the chain control direction.

I would like to also point out that regardless of the underlying issue, I 
think creating the DSS children in the DSS driver instead of mach-omap2 code 
is the right thing to do, as the DSS really handles the bus through which the 
children are accessed. This is similar to how an I2C adapter creates the I2C 
slaves.

> Are there any other ways to manage such device dependencies? Possibly we
> could handle the driver registration so that we only register encoders when
> dss and dispc have been probed, but that feels a bit hacky too.

I also think that's an even bigger hack.

> And considering these issues, is the assumption that "There's no reason
> to delay initialization of most of the driver (such as mapping memory
> I/O or enabling runtime PM) to the component bind handler." still valid?
> edb715dffdee71bb8216ee4d71c0714d932e9acf doesn't really state any
> benefit for this move either. Clearly there's a reason to do this in
> bind, although it doesn't solve the dispc dependency.

We could try moving code back to the bind handler, but that would get in the 
way of probe deferral until bridges are available. In order to solve that we 
would need to convert all bridges to the component framework, which we can't 
do as they're used by display drivers that don't use components. Lovely, isn't 
it ?

There's a nearly infinite amount of problems to fix and I wish I could tackle 
them all at the same time, but that's unfortunately not possible. I would like 
at some point in the future to add an asynchronous bind mechanism to DRM 
bridge itself, whose usage would be optional, unlike the component framework. 
This would help with probe handling, but it's way down the road.

-- 
Regards,

Laurent Pinchart