Otherwise logs are filled with
[drm:drm_bridge_attach] *ERROR* failed to attach bridge /soc@0/bus@30800000/mipi-dsi@30a0 0000 to encoder None-34: -517
when the bridge isn't ready yet.
Fixes: fb8d617f8fd6 ("drm/bridge: Centralize error message when bridge attach fails") Signed-off-by: Guido Günther agx@sigxcpu.org --- drivers/gpu/drm/drm_bridge.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index a8ed66751c2d..f0508e85ae98 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -227,14 +227,15 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = NULL; list_del(&bridge->chain_node);
+ if (ret != -EPROBE_DEFER) { #ifdef CONFIG_OF - DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n", - bridge->of_node, encoder->name, ret); + DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n", + bridge->of_node, encoder->name, ret); #else - DRM_ERROR("failed to attach bridge to encoder %s: %d\n", - encoder->name, ret); + DRM_ERROR("failed to attach bridge to encoder %s: %d\n", + encoder->name, ret); #endif - + } return ret; } EXPORT_SYMBOL(drm_bridge_attach);
Hi Guido,
On Tue, Oct 12, 2021 at 09:58:58PM +0200, Guido Günther wrote:
Otherwise logs are filled with
[drm:drm_bridge_attach] *ERROR* failed to attach bridge /soc@0/bus@30800000/mipi-dsi@30a0 0000 to encoder None-34: -517
when the bridge isn't ready yet.
Fixes: fb8d617f8fd6 ("drm/bridge: Centralize error message when bridge attach fails") Signed-off-by: Guido Günther agx@sigxcpu.org
drivers/gpu/drm/drm_bridge.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index a8ed66751c2d..f0508e85ae98 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -227,14 +227,15 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = NULL; list_del(&bridge->chain_node);
- if (ret != -EPROBE_DEFER) {
#ifdef CONFIG_OF
- DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
bridge->of_node, encoder->name, ret);
DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",bridge->of_node, encoder->name, ret);
It would be better to use drm_probe_err().
Sam
#else
- DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
encoder->name, ret);
DRM_ERROR("failed to attach bridge to encoder %s: %d\n",encoder->name, ret);#endif
- } return ret;
} EXPORT_SYMBOL(drm_bridge_attach); -- 2.33.0
Hi, On Tue, Oct 12, 2021 at 10:08:28PM +0200, Sam Ravnborg wrote:
Hi Guido,
On Tue, Oct 12, 2021 at 09:58:58PM +0200, Guido Günther wrote:
Otherwise logs are filled with
[drm:drm_bridge_attach] *ERROR* failed to attach bridge /soc@0/bus@30800000/mipi-dsi@30a0 0000 to encoder None-34: -517
when the bridge isn't ready yet.
Fixes: fb8d617f8fd6 ("drm/bridge: Centralize error message when bridge attach fails") Signed-off-by: Guido Günther agx@sigxcpu.org
drivers/gpu/drm/drm_bridge.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index a8ed66751c2d..f0508e85ae98 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -227,14 +227,15 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = NULL; list_del(&bridge->chain_node);
- if (ret != -EPROBE_DEFER) {
#ifdef CONFIG_OF
- DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
bridge->of_node, encoder->name, ret);
DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",bridge->of_node, encoder->name, ret);It would be better to use drm_probe_err().
That's what i thought initially but since the rest here uses DRM_* logging i stuck with it. Happy to change that though. Cheers, -- Guido
Sam
#else
- DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
encoder->name, ret);
DRM_ERROR("failed to attach bridge to encoder %s: %d\n",encoder->name, ret);#endif
- } return ret;
} EXPORT_SYMBOL(drm_bridge_attach); -- 2.33.0
Hi Guido,
- if (ret != -EPROBE_DEFER) {
#ifdef CONFIG_OF
- DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
bridge->of_node, encoder->name, ret);
DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",bridge->of_node, encoder->name, ret);It would be better to use drm_probe_err().
That's what i thought initially but since the rest here uses DRM_* logging i stuck with it. Happy to change that though.
If we continue to use the deprecated DRM_ logging functions then we will never get rid of them. And then I like the simplicity of drm_probe_err().
In the end it is up to you.
Sam
Hi Sam, On Thu, Oct 14, 2021 at 09:35:20PM +0200, Sam Ravnborg wrote:
Hi Guido,
- if (ret != -EPROBE_DEFER) {
#ifdef CONFIG_OF
- DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
bridge->of_node, encoder->name, ret);
DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",bridge->of_node, encoder->name, ret);It would be better to use drm_probe_err().
That's what i thought initially but since the rest here uses DRM_* logging i stuck with it. Happy to change that though.
If we continue to use the deprecated DRM_ logging functions then we will never get rid of them. And then I like the simplicity of drm_probe_err().
I wasn't aware those are deprecated. I'll fix that up in case this patch is needed, which
https://lore.kernel.org/dri-devel/CAMty3ZAsn4K0WFRC_FNN2Mina0gSu4Nc1y1zVsoZ8...
makes it a bit look like it. Cheers, -- Guido
In the end it is up to you.
Sam
Hi Guido,
Thank you for the patch.
On Tue, Oct 12, 2021 at 09:58:58PM +0200, Guido Günther wrote:
Otherwise logs are filled with
[drm:drm_bridge_attach] *ERROR* failed to attach bridge /soc@0/bus@30800000/mipi-dsi@30a0 0000 to encoder None-34: -517
when the bridge isn't ready yet.
Fixes: fb8d617f8fd6 ("drm/bridge: Centralize error message when bridge attach fails") Signed-off-by: Guido Günther agx@sigxcpu.org
drivers/gpu/drm/drm_bridge.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index a8ed66751c2d..f0508e85ae98 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -227,14 +227,15 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = NULL; list_del(&bridge->chain_node);
- if (ret != -EPROBE_DEFER) {
#ifdef CONFIG_OF
- DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
bridge->of_node, encoder->name, ret);
DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",bridge->of_node, encoder->name, ret);#else
- DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
encoder->name, ret);
DRM_ERROR("failed to attach bridge to encoder %s: %d\n",encoder->name, ret);#endif
- }
This looks fine as such, but I'm concerned about the direction it's taking. Ideally, probe deferral should happen at probe time, way before the bridge is attached. Doing otherwise is a step in the wrong direction in my opinion, and something we'll end up regretting when we'll feel the pain it inflicts.
return ret; } EXPORT_SYMBOL(drm_bridge_attach);
Hi Laurent, On Tue, Oct 12, 2021 at 11:17:07PM +0300, Laurent Pinchart wrote:
Hi Guido,
Thank you for the patch.
On Tue, Oct 12, 2021 at 09:58:58PM +0200, Guido Günther wrote:
Otherwise logs are filled with
[drm:drm_bridge_attach] *ERROR* failed to attach bridge /soc@0/bus@30800000/mipi-dsi@30a0 0000 to encoder None-34: -517
when the bridge isn't ready yet.
Fixes: fb8d617f8fd6 ("drm/bridge: Centralize error message when bridge attach fails") Signed-off-by: Guido Günther agx@sigxcpu.org
drivers/gpu/drm/drm_bridge.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index a8ed66751c2d..f0508e85ae98 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -227,14 +227,15 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = NULL; list_del(&bridge->chain_node);
- if (ret != -EPROBE_DEFER) {
#ifdef CONFIG_OF
- DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
bridge->of_node, encoder->name, ret);
DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",bridge->of_node, encoder->name, ret);#else
- DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
encoder->name, ret);
DRM_ERROR("failed to attach bridge to encoder %s: %d\n",encoder->name, ret);#endif
- }
This looks fine as such, but I'm concerned about the direction it's taking. Ideally, probe deferral should happen at probe time, way before the bridge is attached. Doing otherwise is a step in the wrong direction in my opinion, and something we'll end up regretting when we'll feel the pain it inflicts.
The particular case I'm seeing this is the nwl driver probe deferrals if the panel bridge isn't ready (which needs a bunch of components (dsi, panel, backlight wrapped led, ...) and it probes fine later on so I wonder where you see the actual error cause? That downstream of the bridge isn't ready or that the display controller is already attaching the bridge?
Cheers, -- Guido
return ret; } EXPORT_SYMBOL(drm_bridge_attach);
-- Regards,
Laurent Pinchart
Hi, On Tue, Oct 12, 2021 at 10:47:14PM +0200, Guido Günther wrote:
Hi Laurent, On Tue, Oct 12, 2021 at 11:17:07PM +0300, Laurent Pinchart wrote:
Hi Guido,
Thank you for the patch.
On Tue, Oct 12, 2021 at 09:58:58PM +0200, Guido Günther wrote:
Otherwise logs are filled with
[drm:drm_bridge_attach] *ERROR* failed to attach bridge /soc@0/bus@30800000/mipi-dsi@30a0 0000 to encoder None-34: -517
when the bridge isn't ready yet.
Fixes: fb8d617f8fd6 ("drm/bridge: Centralize error message when bridge attach fails") Signed-off-by: Guido Günther agx@sigxcpu.org
drivers/gpu/drm/drm_bridge.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index a8ed66751c2d..f0508e85ae98 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -227,14 +227,15 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = NULL; list_del(&bridge->chain_node);
- if (ret != -EPROBE_DEFER) {
#ifdef CONFIG_OF
- DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
bridge->of_node, encoder->name, ret);
DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",bridge->of_node, encoder->name, ret);#else
- DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
encoder->name, ret);
DRM_ERROR("failed to attach bridge to encoder %s: %d\n",encoder->name, ret);#endif
- }
This looks fine as such, but I'm concerned about the direction it's taking. Ideally, probe deferral should happen at probe time, way before the bridge is attached. Doing otherwise is a step in the wrong direction in my opinion, and something we'll end up regretting when we'll feel the pain it inflicts.
The particular case I'm seeing this is the nwl driver probe deferrals if the panel bridge isn't ready (which needs a bunch of components (dsi, panel, backlight wrapped led, ...) and it probes fine later on so I wonder where you see the actual error cause? That downstream of the bridge isn't ready or that the display controller is already attaching the bridge?
I should add that mxsfb does a `dev_err_probe()` already when checking the return value of `drm_bridge_attach()` so the error printed is triggered by the additional check added in the above function while the code path already ignored -EPROBE_DEFER before. This looks sensible to me since upper layers can't known when all the downstream bridges are done probing or am I missing something?
Cheers, -- Guido
Cheers, -- Guido
return ret; } EXPORT_SYMBOL(drm_bridge_attach);
-- Regards,
Laurent Pinchart
On 12.10.2021 22:47, Guido Günther wrote:
Hi Laurent, On Tue, Oct 12, 2021 at 11:17:07PM +0300, Laurent Pinchart wrote:
Hi Guido,
Thank you for the patch.
On Tue, Oct 12, 2021 at 09:58:58PM +0200, Guido Günther wrote:
Otherwise logs are filled with
[drm:drm_bridge_attach] *ERROR* failed to attach bridge /soc@0/bus@30800000/mipi-dsi@30a0 0000 to encoder None-34: -517
when the bridge isn't ready yet.
Fixes: fb8d617f8fd6 ("drm/bridge: Centralize error message when bridge attach fails") Signed-off-by: Guido Günther agx@sigxcpu.org
drivers/gpu/drm/drm_bridge.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index a8ed66751c2d..f0508e85ae98 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -227,14 +227,15 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = NULL; list_del(&bridge->chain_node);
- if (ret != -EPROBE_DEFER) { #ifdef CONFIG_OF
- DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
bridge->of_node, encoder->name, ret);
DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",bridge->of_node, encoder->name, ret);
- DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
encoder->name, ret);
DRM_ERROR("failed to attach bridge to encoder %s: %d\n",encoder->name, ret);
- }
This looks fine as such, but I'm concerned about the direction it's taking. Ideally, probe deferral should happen at probe time, way before the bridge is attached. Doing otherwise is a step in the wrong direction in my opinion, and something we'll end up regretting when we'll feel the pain it inflicts.
The particular case I'm seeing this is the nwl driver probe deferrals if the panel bridge isn't ready (which needs a bunch of components (dsi, panel, backlight wrapped led, ...) and it probes fine later on so I wonder where you see the actual error cause? That downstream of the bridge isn't ready or that the display controller is already attaching the bridge?
So it is something wrong there, nwl should not publish bridge interface until it gather its resources (the panel in this case).
Regards Andrzej
Cheers, -- Guido
return ret; } EXPORT_SYMBOL(drm_bridge_attach);
-- Regards,
Laurent Pinchart
Hi, On Wed, Oct 13, 2021 at 08:48:32AM +0200, Andrzej Hajda wrote:
On 12.10.2021 22:47, Guido Günther wrote:
Hi Laurent, On Tue, Oct 12, 2021 at 11:17:07PM +0300, Laurent Pinchart wrote:
Hi Guido,
Thank you for the patch.
On Tue, Oct 12, 2021 at 09:58:58PM +0200, Guido Günther wrote:
Otherwise logs are filled with
[drm:drm_bridge_attach] *ERROR* failed to attach bridge /soc@0/bus@30800000/mipi-dsi@30a0 0000 to encoder None-34: -517
when the bridge isn't ready yet.
Fixes: fb8d617f8fd6 ("drm/bridge: Centralize error message when bridge attach fails") Signed-off-by: Guido Günther agx@sigxcpu.org
drivers/gpu/drm/drm_bridge.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index a8ed66751c2d..f0508e85ae98 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -227,14 +227,15 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = NULL; list_del(&bridge->chain_node);
- if (ret != -EPROBE_DEFER) { #ifdef CONFIG_OF
- DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
bridge->of_node, encoder->name, ret);
DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",bridge->of_node, encoder->name, ret);
- DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
encoder->name, ret);
DRM_ERROR("failed to attach bridge to encoder %s: %d\n",encoder->name, ret);
- }
This looks fine as such, but I'm concerned about the direction it's taking. Ideally, probe deferral should happen at probe time, way before the bridge is attached. Doing otherwise is a step in the wrong direction in my opinion, and something we'll end up regretting when we'll feel the pain it inflicts.
The particular case I'm seeing this is the nwl driver probe deferrals if the panel bridge isn't ready (which needs a bunch of components (dsi, panel, backlight wrapped led, ...) and it probes fine later on so I wonder where you see the actual error cause? That downstream of the bridge isn't ready or that the display controller is already attaching the bridge?
So it is something wrong there, nwl should not publish bridge interface until it gather its resources (the panel in this case).
That helps, I'll look at that. Thanks! -- Guido
Regards Andrzej
Cheers, -- Guido
return ret; } EXPORT_SYMBOL(drm_bridge_attach);
-- Regards,
Laurent Pinchart
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Guido Günther -
Laurent Pinchart -
Sam Ravnborg