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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
