[PATCH 0/2] gpu/drm: ingenic: Handle disabled drivers

[PATCH 0/2] gpu/drm: ingenic: Handle disabled drivers

[Paul Cercueil]

Hi,

A small patchset to make sure that the ingenic-drm driver won't fail to
probe if an optional device (e.g. HDMI chip) has its driver disabled in
the kernel config.

Since most of the boards using the ingenic-drm driver have a LCD panel
attached, the drivers for external devices (be it a HDMI transmitter or
a TV encoder) really are optional and shouldn't prevent the DRM driver
from probing.

Cheers,
-Paul

Paul Cercueil (2):
  drivers core: Export driver_deferred_probe_check_state()
  gpu/drm: ingenic: Add workaround for disabled drivers

 drivers/base/dd.c                         |  1 +
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 12 ++++++++++++
 2 files changed, 13 insertions(+)

-- 
2.30.2

[PATCH 1/2] drivers core: Export driver_deferred_probe_check_state()

[Paul Cercueil]

Export this function as a GPL symbol, so that it can be used from
modules.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index daeb9b5763ae..658f1527a58b 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -296,6 +296,7 @@ int driver_deferred_probe_check_state(struct device *dev)
 
 	return -EPROBE_DEFER;
 }
+EXPORT_SYMBOL_GPL(driver_deferred_probe_check_state);
 
 static void deferred_probe_timeout_work_func(struct work_struct *work)
 {
-- 
2.30.2

Re: [PATCH 1/2] drivers core: Export driver_deferred_probe_check_state()

[Greg Kroah-Hartman]

On Thu, Aug 05, 2021 at 09:21:08PM +0200, Paul Cercueil wrote:
> Export this function as a GPL symbol, so that it can be used from
> modules.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/base/dd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index daeb9b5763ae..658f1527a58b 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -296,6 +296,7 @@ int driver_deferred_probe_check_state(struct device *dev)
>  
>  	return -EPROBE_DEFER;
>  }
> +EXPORT_SYMBOL_GPL(driver_deferred_probe_check_state);

Drivers should never need to mess with this, it is for only a small set
of busses to use only.

Why do you think this is needed by a driver?

thanks,

greg k-h

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

[Paul Cercueil]

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;
+			}
 			if (ret == -ENODEV)
 				break; /* we're done */
 			if (ret != -EPROBE_DEFER)
-- 
2.30.2

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

[Greg Kroah-Hartman]

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?  :(

Please use the existing driver core functionality for this type of
thing, it is not unique, no need for this function to be called.

thanks,

greg k-h

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

[Paul Cercueil]

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.

> 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.

Cheers,
-Paul

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

[Greg Kroah-Hartman]

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.

> > 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?

Much like the device link code, have you looked at that?

thanks,

greg k-h

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

[Paul Cercueil]

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.

Cheers,
-Paul

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

[Daniel Vetter]

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.

Why should we care? Is that hdmi driver really that big that we have to
support this use-case?

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Paul Cercueil]

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

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

[Daniel Vetter]

On Tue, Aug 10, 2021 at 12:33:04PM +0200, Paul Cercueil wrote:
> 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.

I think this is were things go wrong.

Either you have a generic image, where all the needed drivers are included
as modules.

Or you have a bespoke image, where you just built-in what you actually
needed.

Asking for both is a bit much ...

> > 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.

No.

What I'm rejecting is when people add Kconfig knobs for a single function,
while we have everything else as fairly monolithic dependencies in drm.
That makes no sense.

The only requirement for tiny drm that I have is that whomever proposes
it:

- does an overall survey and shows that we do actually get rid of the big
  pieces, and not just of the pieces which are easy to make optional

- has a solid answer to the maintainance cost this will incur. This one is
  very important, because for historical reasons we have a bunch of
  optional things like backlight, and the trend is to make them less
  optional because a) those are all rather invalid configs anyway b) no on
  has time to review the constant flux of busy-work bugfixes when
  something inevitably breaks again

Thus far all I got from people who want to make drm tiny is some wishlist
items and stand-alone patches that don't make sense stand-alone. Until
that's fixed and someone invests serious amounts of time here drm will be
a big behemoth. And that's because we're very much not operating in
an "infinite resource" world, the most constraint supply we have is
contributor, reviewer and maintainer bandwidth.

Which yes means occasionally things will continue to suck because no one
cared enough about it yet.

> > 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.

The thing is, right now that stuff just isn't optional. Except if you
patch your dt, which I think is the other approach that's been discussed
here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Paul Cercueil]

Hi Daniel,

Le mar., août 10 2021 at 12:59:53 +0200, Daniel Vetter 
<daniel@ffwll.ch> a écrit :
> On Tue, Aug 10, 2021 at 12:33:04PM +0200, Paul Cercueil wrote:
>>  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.
> 
> I think this is were things go wrong.
> 
> Either you have a generic image, where all the needed drivers are 
> included
> as modules.
> 
> Or you have a bespoke image, where you just built-in what you actually
> needed.

Yes, and that's what I want - a kernel with just the minimal number of 
built-in drivers that I need (so, not the HDMI chip).

> Asking for both is a bit much ...
> 
>>  > 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.
> 
> No.
> 
> What I'm rejecting is when people add Kconfig knobs for a single 
> function,
> while we have everything else as fairly monolithic dependencies in 
> drm.
> That makes no sense.
> 
> The only requirement for tiny drm that I have is that whomever 
> proposes
> it:
> 
> - does an overall survey and shows that we do actually get rid of the 
> big
>   pieces, and not just of the pieces which are easy to make optional
> 
> - has a solid answer to the maintainance cost this will incur. This 
> one is
>   very important, because for historical reasons we have a bunch of
>   optional things like backlight, and the trend is to make them less
>   optional because a) those are all rather invalid configs anyway b) 
> no on
>   has time to review the constant flux of busy-work bugfixes when
>   something inevitably breaks again
> 
> Thus far all I got from people who want to make drm tiny is some 
> wishlist
> items and stand-alone patches that don't make sense stand-alone. Until
> that's fixed and someone invests serious amounts of time here drm 
> will be
> a big behemoth. And that's because we're very much not operating in
> an "infinite resource" world, the most constraint supply we have is
> contributor, reviewer and maintainer bandwidth.
> 
> Which yes means occasionally things will continue to suck because no 
> one
> cared enough about it yet.

Alright, I can totally understand that.

>>  > 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.
> 
> The thing is, right now that stuff just isn't optional. Except if you
> patch your dt, which I think is the other approach that's been 
> discussed
> here.

It's not been discussed yet - I think we talked about it a few weeks 
ago on IRC.

DT is supposed to represent the hardware though, and not a specific 
configuration, which would be policy; so this isn't really the best 
solution either.

Cheers,
-Paul

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

[Greg Kroah-Hartman]

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.

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.

thanks,

greg k-h

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@linuxfoundation.org> 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.
> 
> 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

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

[Daniel Vetter]

On Tue, Aug 10, 2021 at 12:40:39PM +0200, Paul Cercueil wrote:
> Hi Greg,
> 
> Le mar., août 10 2021 at 11:47:32 +0200, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> 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.
> > 
> > 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.

The conflict is that drm wants to load the entire driver, including any
bridges.

Paul wants to load just the driver parts he wants to put into the image.

This doesn't work well together.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch