From: Paul Cercueil <paul@crapouillou.net>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Rob Herring <robh@kernel.org>,
"Rafael J . Wysocki" <rafael@kernel.org>,
David Airlie <airlied@linux.ie>, Sam Ravnborg <sam@ravnborg.org>,
list@opendingux.net, linux-kernel@vger.kernel.org,
linux-mips@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/2] gpu/drm: ingenic: Add workaround for disabled drivers
Date: Tue, 10 Aug 2021 12:33:04 +0200 [thread overview]
Message-ID: <4BDMXQ.S6A97ME8XJUV@crapouillou.net> (raw)
In-Reply-To: <YRJIb8ofHe8r5g1z@phenom.ffwll.local>
Hi Daniel,
Le mar., août 10 2021 at 11:35:43 +0200, Daniel Vetter
<daniel@ffwll.ch> 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@linuxfoundation.org> 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@linuxfoundation.org> 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@crapouillou.net>
>> > > > > ---
>> > > > > 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.
>>
>> 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.
>
> Is this actually a legit use-case?
>
> Like you have hw with a bunch of sub-devices linked, and you decided
> to
> disable some of them, which makes the driver not load.
Yes. I'm facing that issue with a board that has a LCD panel and a HDMI
controller (IT66121). I have a "flasher" program for all the Ingenic
boards, that's basically just a Linux kernel + initramfs booted over
USB (device). I can't realistically enable every single driver for all
the hardware that's on these boards while still having a tiny
footprint. And I shouldn't have to care about it either.
> Why should we care? Is that hdmi driver really that big that we have
> to
> support this use-case?
DRM maintainers work with what embedded devs would call "infinite
resources". It annoys me that CONFIG_DRM pulls the I2C code even though
I may just have a LCD panel, and it annoys me that I have to enable
support for hardware that I'm not even planning to use, just so that
the DRM driver works for the hardware I do want to use.
> I know it's possible to do this, that doesn't mean it's a good idea.
> There's inifinitely more randconfigs that don't boot on my machine
> here
> for various reasons than the ones that do boot. We don't have "fixes"
> for
> all of these to make things still work, despite user misconfiguring
> their
> kernel.
I understand, you can't really expect random configs to work every
time. But it should still be possible to disable drivers for *optional*
hardware in the config and end up with a working system.
Cheers,
-Paul
next prev parent reply other threads:[~2021-08-10 10:33 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-05 19:21 [PATCH 0/2] gpu/drm: ingenic: Handle disabled drivers Paul Cercueil
2021-08-05 19:21 ` [PATCH 1/2] drivers core: Export driver_deferred_probe_check_state() Paul Cercueil
2021-08-05 19:34 ` Greg Kroah-Hartman
2021-08-05 19:21 ` [PATCH 2/2] gpu/drm: ingenic: Add workaround for disabled drivers Paul Cercueil
2021-08-05 19:35 ` Greg Kroah-Hartman
2021-08-05 20:05 ` Paul Cercueil
2021-08-06 10:17 ` Greg Kroah-Hartman
2021-08-06 11:01 ` Paul Cercueil
2021-08-10 9:35 ` Daniel Vetter
2021-08-10 10:33 ` Paul Cercueil [this message]
2021-08-10 10:59 ` Daniel Vetter
2021-08-10 11:58 ` Paul Cercueil
2021-08-10 9:47 ` Greg Kroah-Hartman
2021-08-10 10:40 ` Paul Cercueil
2021-08-10 10:52 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4BDMXQ.S6A97ME8XJUV@crapouillou.net \
--to=paul@crapouillou.net \
--cc=airlied@linux.ie \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=list@opendingux.net \
--cc=rafael@kernel.org \
--cc=robh@kernel.org \
--cc=sam@ravnborg.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.