Re: [PATCH 2/2] gpu/drm: ingenic: Add workaround for disabled drivers

[Paul Cercueil]

Hi Greg,

Le mar., août 10 2021 at 11:47:32 +0200, Greg Kroah-Hartman 
<gregkh@xxxxxxxxxxxxxxxxxxx> a écrit :
> On Fri, Aug 06, 2021 at 01:01:33PM +0200, Paul Cercueil wrote:
> >  Hi Greg,
> >
> >  Le ven., août 6 2021 at 12:17:55 +0200, Greg Kroah-Hartman
> >  <gregkh@xxxxxxxxxxxxxxxxxxx> a écrit :
> >  > On Thu, Aug 05, 2021 at 10:05:27PM +0200, Paul Cercueil wrote:
> >  > >  Hi Greg,
> >  > >
> >  > >  Le jeu., août 5 2021 at 21:35:34 +0200, Greg Kroah-Hartman
> >  > >  <gregkh@xxxxxxxxxxxxxxxxxxx> a écrit :
> >  > >  > On Thu, Aug 05, 2021 at 09:21:09PM +0200, Paul Cercueil 
> > wrote:
> >  > >  > >  When the drivers of remote devices (e.g. HDMI chip) are
> >  > > disabled in
> >  > >  > > the
> >  > >  > >  config, we want the ingenic-drm driver to be able to probe
> >  > >  > > nonetheless
> >  > >  > >  with the other devices (e.g. internal LCD panel) that are
> >  > > enabled.
> >  > >  > >
> >  > >  > >  Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> >  > >  > >  ---
> >  > >  > >   drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 12 
> > ++++++++++++
> >  > >  > >   1 file changed, 12 insertions(+)
> >  > >  > >
> >  > >  > >  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >  > >  > > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >  > >  > >  index d261f7a03b18..5e1fdbb0ba6b 100644
> >  > >  > >  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >  > >  > >  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >  > >  > >  @@ -1058,6 +1058,18 @@ static int ingenic_drm_bind(struct
> >  > > device
> >  > >  > > *dev, bool has_components)
> >  > >  > >   	for (i = 0; ; i++) {
> >  > >  > >   		ret = drm_of_find_panel_or_bridge(dev->of_node, 0, i,
> >  > > &panel,
> >  > >  > > &bridge);
> >  > >  > >   		if (ret) {
> >  > >  > >  +			/*
> >  > >  > >  +			 * Workaround for the case where the drivers for the
> >  > >  > >  +			 * remote devices are not enabled. When that happens,
> >  > >  > >  +			 * drm_of_find_panel_or_bridge() returns -EPROBE_DEFER
> >  > >  > >  +			 * endlessly, which prevents the ingenic-drm driver 
> > from
> >  > >  > >  +			 * working at all.
> >  > >  > >  +			 */
> >  > >  > >  +			if (ret == -EPROBE_DEFER) {
> >  > >  > >  +				ret = driver_deferred_probe_check_state(dev);
> >  > >  > >  +				if (ret == -ENODEV || ret == -ETIMEDOUT)
> >  > >  > >  +					continue;
> >  > >  > >  +			}
> >  > >  >
> >  > >  > So you are mucking around with devices on other busses 
> > within this
> >  > >  > driver?  What could go wrong?  :(
> >  > >
> >  > >  I'm doing the same thing as everybody else. This is the DRM 
> > driver,
> >  > > and
> >  > >  there is a driver for the external HDMI chip which gives us a 
> > DRM
> >  > > bridge
> >  > >  that we can obtain from the device tree.
> >  >
> >  > But then why do you need to call this function that is there for 
> > a bus,
> >  > not for a driver.
> >
> >  The documentation disagrees with you :)
> >
> >  And, if that has any weight, this solution was proposed by Rob.
> >
> >  > >  > Please use the existing driver core functionality for this 
> > type of
> >  > >  > thing, it is not unique, no need for this function to be 
> > called.
> >  > >
> >  > >  I'm not sure you understand what I'm doing here. This driver 
> > calls
> >  > >  drm_of_find_panel_or_bridge(), without guarantee that the 
> > driver
> >  > > for the
> >  > >  remote device (connected via DT graph) has been enabled in the
> >  > > kernel
> >  > >  config. In that case it will always return -EPROBE_DEFER and 
> > the
> >  > > ingenic-drm
> >  > >  driver will never probe.
> >  > >
> >  > >  This patch makes sure that the driver can probe if the HDMI 
> > driver
> >  > > has been
> >  > >  disabled in the kernel config, nothing more.
> >  >
> >  > That should not be an issue as you do not care if the config is 
> > enabled,
> >  > you just want to do something in the future if the driver shows 
> > up,
> >  > right?
> >
> >  Well, the DRM subsystem doesn't really seem to handle hotplug of 
> > hardware.
> >  Right now all the drivers for the connected hardware need to probe 
> > before
> >  the main DRM driver. So I need to know that a remote device 
> > (connected via
> >  DT graph) will never probe.
>
> But you never really know that.  That is what the recent driver core
> changes were all about, to handle this very issue.  Only when the 
> child
> device shows up will you need to care about it.
>
> >  Give me a of_graph_remote_device_driver_will_never_probe() and I'll 
> > use
> >  that.
> >
> >  > Much like the device link code, have you looked at that?
> >
> >  I don't see how that would help in any way. The device link code 
> > would allow
> >  me to set a dependency between the remote hardware (HDMI chip, 
> > provider) and
> >  the LCD controller (consumer), but I already have that dependency 
> > though the
> >  DT graph. What I need is a way for the consumer to continue probing 
> > if the
> >  provider is not going to probe.
>
> But again, you never know that, probing is async, and could happen in 
> a
> few milliseconds, or a few hours, your driver should never care about
> this at all.
>
> Just knowing if the kernel configuration is something is not the
> solution here, please fix this properly like all other driver
> interactions are in the kernel tree.

A proper fix means reworking the DRM core so that it supports 
hot-plugging bridges. Until then there is nothing else I can do.

-Paul