Hi Alexander,
On Wed, Jun 07, 2023 at 01:26:03PM +0200, Alexander Stein wrote:
> Am Dienstag, 6. Juni 2023, 17:12:29 CEST schrieb Laurent Pinchart:
> > On Tue, Jun 06, 2023 at 04:48:33PM +0200, Alexander Stein wrote:
> > > When -EPROBE_DEFER is returned do not raise an error, but silently return
> > > this error instead. Fixes error like this:
> > > [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > > /soc@0/bus@30800000/mipi-dsi@30a00000 to encoder None-34: -517
> > > [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > > /soc@0/bus@30800000/mipi-dsi@30a00000 to encoder None-34: -517
> > >
> > > Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> > > ---
> > > dev_err_probe() would be the best, but I am not sure if this function is
> > > always used within a driver's probe() call.
> > >
> > > drivers/gpu/drm/drm_bridge.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index c3d69af02e79d..07773d6441a1f 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -350,6 +350,7 @@ 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);
> >
> > Wrong indentation.
>
> Ah, right. Thanks for pointing out.
>
> > dev_err_probe() could be useful, but this function is likely not called
> > from probe paths only :-S
>
> I was afraid this might be the cause. But I'm wondering in which situation
> this can be the case, hence -EPROBE_DEFER could be returned then.
I've had a quick look, and one example of a non-probe path is in
mcde_modeset_init(), with the call to
drm_simple_display_pipe_attach_bridge() which calls drm_bridge_attach().
mcde_modeset_init() is called from mcde_drm_bind(), the handler for the
.bind() operation in component_master_ops.
I'd argue that we should really drop the component framework and replace
it with something better, but that's beyond the scope of this patch :-)
> > When not called from a probe path, dropping the message will result in a
> > silent error, which would be hard to debug :-(
>
> On the other hand -EPROBE_DEFER is invalid on non-probe path also.
> Assuming dev_err_probe is used here, an error will still be raised, -
> EPROBE_DEFER should not occur then.
I agree that -EPROBE_DEFER shouldn't occur, and in many cases, it won't
for drivers that use the component framework, as the whole purpose of
the framework is to make sure the bridges are available before we try to
attach to them. The framework (or at least the way it's used in drivers)
however doesn't handle chains of components: the main DRM driver will
have its next bridge available by the time it calls drm_bridge_attach(),
but the bridge may then try to acquire the next bridge in the chain, and
get an error that results in a probe deferral. Maybe that's not supposed
to happen though, bridges probably should acquire the next bridge at
probe time, but I can't guarantee this will always be done.
And this is my point: I'm scared that this patch will cause silent and
hard to debug failures in some cases. Those cases may be incorrect usage
of APIs by drivers, but making them silent will make it more difficult
to fix them.
If everybody thinks I'm over-concerned, please feel free to move forward
with this patch, and I'll do my best not to lose sleep :-)
> > > @@ -357,6 +358,7 @@ int drm_bridge_attach(struct drm_encoder *encoder,
> > > struct drm_bridge *bridge,>
> > > DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
> > >
> > > encoder->name, ret);
> > >
> > > #endif
> > >
> > > + }
> > >
> > > return ret;
> > >
> > > }
--
Regards,
Laurent Pinchart
