| Message ID | 20210720134525.563936-5-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: Make panel and bridge probe order consistent | expand |
On Tue, Jul 20, 2021 at 03:45:19PM +0200, Maxime Ripard wrote: > Interactions between bridges, panels, MIPI-DSI host and the component > framework are not trivial and can lead to probing issues when > implementing a display driver. Let's document the various cases we need > too consider, and the solution to support all the cases. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> I still have this dream that eventually we resurrect a patch to add device_link to bridges/panels (ideally automatically), to help with some of the suspend/resume issues around here. Will this make things worse? I think it'd be really good to figure that out with some coding, since if we have incompatible solution to handle probe issues vs suspend/resume issues, we're screwed. Atm the duct-tape is to carefully move things around between suspend and suspend_early hooks (and resume and resume_late) and hope it all works ... -Daniel > --- > Documentation/gpu/drm-kms-helpers.rst | 6 +++ > drivers/gpu/drm/drm_bridge.c | 60 +++++++++++++++++++++++++++ > 2 files changed, 66 insertions(+) > > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst > index 10f8df7aecc0..ec2f65b31930 100644 > --- a/Documentation/gpu/drm-kms-helpers.rst > +++ b/Documentation/gpu/drm-kms-helpers.rst > @@ -157,6 +157,12 @@ Display Driver Integration > .. kernel-doc:: drivers/gpu/drm/drm_bridge.c > :doc: display driver integration > > +Special Care with MIPI-DSI bridges > +---------------------------------- > + > +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c > + :doc: special care dsi > + > Bridge Operations > ----------------- > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index c9a950bfdfe5..81f8dac12367 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -95,6 +95,66 @@ > * documentation of bridge operations for more details). > */ > > +/** > + * DOC: special care dsi > + * > + * The interaction between the bridges and other frameworks involved in > + * the probing of the display driver and the bridge driver can be > + * challenging. Indeed, there's multiple cases that needs to be > + * considered: > + * > + * - The display driver doesn't use the component framework and isn't a > + * MIPI-DSI host. In this case, the bridge driver will probe at some > + * point and the display driver should try to probe again by returning > + * EPROBE_DEFER as long as the bridge driver hasn't probed. > + * > + * - The display driver doesn't use the component framework, but is a > + * MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be > + * controlled. In this case, the bridge device is a child of the > + * display device and when it will probe it's assured that the display > + * device (and MIPI-DSI host) is present. The display driver will be > + * assured that the bridge driver is connected between the > + * &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations. > + * Therefore, it must run mipi_dsi_host_register() in its probe > + * function, and then run drm_bridge_attach() in its > + * &mipi_dsi_host_ops.attach hook. > + * > + * - The display driver uses the component framework and is a MIPI-DSI > + * host. The bridge device uses the MIPI-DCS commands to be > + * controlled. This is the same situation than above, and can run > + * mipi_dsi_host_register() in either its probe or bind hooks. > + * > + * - The display driver uses the component framework and is a MIPI-DSI > + * host. The bridge device uses a separate bus (such as I2C) to be > + * controlled. In this case, there's no correlation between the probe > + * of the bridge and display drivers, so care must be taken to avoid > + * an endless EPROBE_DEFER loop, with each driver waiting for the > + * other to probe. > + * > + * The ideal pattern to cover the last item (and all the others in the > + * display driver case) is to split the operations like this: > + * > + * - In the display driver must run mipi_dsi_host_register() and > + * component_add in its probe hook. It will make sure that the > + * MIPI-DSI host sticks around, and that the driver's bind can be > + * called. > + * > + * - In its probe hook, the bridge driver must not try to find its > + * MIPI-DSI host or register as a MIPI-DSI device. As far as the > + * framework is concerned, it must only call drm_bridge_add(). > + * > + * - In its bind hook, the display driver must try to find the bridge > + * and return -EPROBE_DEFER if it doesn't find it. If it's there, it > + * must call drm_bridge_attach(). The MIPI-DSI host is now functional. > + * > + * - In its &drm_bridge_funcs.attach hook, the bridge driver can now try > + * to find its MIPI-DSI host and can register as a MIPI-DSI device. > + * > + * At this point, we're now certain that both the display driver and the > + * bridge driver are functional and we can't have a deadlock-like > + * situation when probing. > + */ > + > static DEFINE_MUTEX(bridge_lock); > static LIST_HEAD(bridge_list); > > -- > 2.31.1 >
On Wed, Jul 21, 2021 at 02:05:01PM +0200, Daniel Vetter wrote: > On Tue, Jul 20, 2021 at 03:45:19PM +0200, Maxime Ripard wrote: > > Interactions between bridges, panels, MIPI-DSI host and the component > > framework are not trivial and can lead to probing issues when > > implementing a display driver. Let's document the various cases we need > > too consider, and the solution to support all the cases. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > I still have this dream that eventually we resurrect a patch to add > device_link to bridges/panels (ideally automatically), to help with some > of the suspend/resume issues around here. > > Will this make things worse? > > I think it'd be really good to figure that out with some coding, since if > we have incompatible solution to handle probe issues vs suspend/resume > issues, we're screwed. > > Atm the duct-tape is to carefully move things around between suspend and > suspend_early hooks (and resume and resume_late) and hope it all works ... Just remembered: The other reason for device links was module ordering fun. Especially when you have parts managed as a component. Currently if you unload a bridge driver then in some cases the drm_device doesn't rebind. -Daniel > > > --- > > Documentation/gpu/drm-kms-helpers.rst | 6 +++ > > drivers/gpu/drm/drm_bridge.c | 60 +++++++++++++++++++++++++++ > > 2 files changed, 66 insertions(+) > > > > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst > > index 10f8df7aecc0..ec2f65b31930 100644 > > --- a/Documentation/gpu/drm-kms-helpers.rst > > +++ b/Documentation/gpu/drm-kms-helpers.rst > > @@ -157,6 +157,12 @@ Display Driver Integration > > .. kernel-doc:: drivers/gpu/drm/drm_bridge.c > > :doc: display driver integration > > > > +Special Care with MIPI-DSI bridges > > +---------------------------------- > > + > > +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c > > + :doc: special care dsi > > + > > Bridge Operations > > ----------------- > > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > index c9a950bfdfe5..81f8dac12367 100644 > > --- a/drivers/gpu/drm/drm_bridge.c > > +++ b/drivers/gpu/drm/drm_bridge.c > > @@ -95,6 +95,66 @@ > > * documentation of bridge operations for more details). > > */ > > > > +/** > > + * DOC: special care dsi > > + * > > + * The interaction between the bridges and other frameworks involved in > > + * the probing of the display driver and the bridge driver can be > > + * challenging. Indeed, there's multiple cases that needs to be > > + * considered: > > + * > > + * - The display driver doesn't use the component framework and isn't a > > + * MIPI-DSI host. In this case, the bridge driver will probe at some > > + * point and the display driver should try to probe again by returning > > + * EPROBE_DEFER as long as the bridge driver hasn't probed. > > + * > > + * - The display driver doesn't use the component framework, but is a > > + * MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be > > + * controlled. In this case, the bridge device is a child of the > > + * display device and when it will probe it's assured that the display > > + * device (and MIPI-DSI host) is present. The display driver will be > > + * assured that the bridge driver is connected between the > > + * &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations. > > + * Therefore, it must run mipi_dsi_host_register() in its probe > > + * function, and then run drm_bridge_attach() in its > > + * &mipi_dsi_host_ops.attach hook. > > + * > > + * - The display driver uses the component framework and is a MIPI-DSI > > + * host. The bridge device uses the MIPI-DCS commands to be > > + * controlled. This is the same situation than above, and can run > > + * mipi_dsi_host_register() in either its probe or bind hooks. > > + * > > + * - The display driver uses the component framework and is a MIPI-DSI > > + * host. The bridge device uses a separate bus (such as I2C) to be > > + * controlled. In this case, there's no correlation between the probe > > + * of the bridge and display drivers, so care must be taken to avoid > > + * an endless EPROBE_DEFER loop, with each driver waiting for the > > + * other to probe. > > + * > > + * The ideal pattern to cover the last item (and all the others in the > > + * display driver case) is to split the operations like this: > > + * > > + * - In the display driver must run mipi_dsi_host_register() and > > + * component_add in its probe hook. It will make sure that the > > + * MIPI-DSI host sticks around, and that the driver's bind can be > > + * called. > > + * > > + * - In its probe hook, the bridge driver must not try to find its > > + * MIPI-DSI host or register as a MIPI-DSI device. As far as the > > + * framework is concerned, it must only call drm_bridge_add(). > > + * > > + * - In its bind hook, the display driver must try to find the bridge > > + * and return -EPROBE_DEFER if it doesn't find it. If it's there, it > > + * must call drm_bridge_attach(). The MIPI-DSI host is now functional. > > + * > > + * - In its &drm_bridge_funcs.attach hook, the bridge driver can now try > > + * to find its MIPI-DSI host and can register as a MIPI-DSI device. > > + * > > + * At this point, we're now certain that both the display driver and the > > + * bridge driver are functional and we can't have a deadlock-like > > + * situation when probing. > > + */ > > + > > static DEFINE_MUTEX(bridge_lock); > > static LIST_HEAD(bridge_list); > > > > -- > > 2.31.1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Hi Daniel, On Wed, Jul 21, 2021 at 02:05:01PM +0200, Daniel Vetter wrote: > On Tue, Jul 20, 2021 at 03:45:19PM +0200, Maxime Ripard wrote: > > Interactions between bridges, panels, MIPI-DSI host and the component > > framework are not trivial and can lead to probing issues when > > implementing a display driver. Let's document the various cases we need > > too consider, and the solution to support all the cases. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > I still have this dream that eventually we resurrect a patch to add > device_link to bridges/panels (ideally automatically), to help with some > of the suspend/resume issues around here. > > Will this make things worse? > > I think it'd be really good to figure that out with some coding, since if > we have incompatible solution to handle probe issues vs suspend/resume > issues, we're screwed. > > Atm the duct-tape is to carefully move things around between suspend and > suspend_early hooks (and resume and resume_late) and hope it all works ... My initial idea to fix this was indeed to use device links. I gave up after a while since it doesn't look like there's a way to add a device link before either the bridge or encoder probes. Indeed the OF-Graph representation is device-specific, so it can't be generic, and if you need to probe to add that link, well, it's already too late for the probe ordering :) Maxime
On Mon, Jul 26, 2021 at 05:16:57PM +0200, Maxime Ripard wrote: > Hi Daniel, > > On Wed, Jul 21, 2021 at 02:05:01PM +0200, Daniel Vetter wrote: > > On Tue, Jul 20, 2021 at 03:45:19PM +0200, Maxime Ripard wrote: > > > Interactions between bridges, panels, MIPI-DSI host and the component > > > framework are not trivial and can lead to probing issues when > > > implementing a display driver. Let's document the various cases we need > > > too consider, and the solution to support all the cases. > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > > I still have this dream that eventually we resurrect a patch to add > > device_link to bridges/panels (ideally automatically), to help with some > > of the suspend/resume issues around here. > > > > Will this make things worse? > > > > I think it'd be really good to figure that out with some coding, since if > > we have incompatible solution to handle probe issues vs suspend/resume > > issues, we're screwed. > > > > Atm the duct-tape is to carefully move things around between suspend and > > suspend_early hooks (and resume and resume_late) and hope it all works ... > > My initial idea to fix this was indeed to use device links. I gave up > after a while since it doesn't look like there's a way to add a device > link before either the bridge or encoder probes. > > Indeed the OF-Graph representation is device-specific, so it can't be > generic, and if you need to probe to add that link, well, it's already > too late for the probe ordering :) But don't we still need the device_link for suspend/resume and module reload? All very annoying indeed anyway. -Daniel
On Tue, Jul 20, 2021 at 7:15 PM Maxime Ripard <maxime@cerno.tech> wrote: > > Interactions between bridges, panels, MIPI-DSI host and the component > framework are not trivial and can lead to probing issues when > implementing a display driver. Let's document the various cases we need > too consider, and the solution to support all the cases. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > Documentation/gpu/drm-kms-helpers.rst | 6 +++ > drivers/gpu/drm/drm_bridge.c | 60 +++++++++++++++++++++++++++ > 2 files changed, 66 insertions(+) > > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst > index 10f8df7aecc0..ec2f65b31930 100644 > --- a/Documentation/gpu/drm-kms-helpers.rst > +++ b/Documentation/gpu/drm-kms-helpers.rst > @@ -157,6 +157,12 @@ Display Driver Integration > .. kernel-doc:: drivers/gpu/drm/drm_bridge.c > :doc: display driver integration > > +Special Care with MIPI-DSI bridges > +---------------------------------- > + > +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c > + :doc: special care dsi > + > Bridge Operations > ----------------- > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index c9a950bfdfe5..81f8dac12367 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -95,6 +95,66 @@ > * documentation of bridge operations for more details). > */ > > +/** > + * DOC: special care dsi > + * > + * The interaction between the bridges and other frameworks involved in > + * the probing of the display driver and the bridge driver can be > + * challenging. Indeed, there's multiple cases that needs to be > + * considered: > + * > + * - The display driver doesn't use the component framework and isn't a > + * MIPI-DSI host. In this case, the bridge driver will probe at some > + * point and the display driver should try to probe again by returning > + * EPROBE_DEFER as long as the bridge driver hasn't probed. > + * > + * - The display driver doesn't use the component framework, but is a > + * MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be > + * controlled. In this case, the bridge device is a child of the > + * display device and when it will probe it's assured that the display > + * device (and MIPI-DSI host) is present. The display driver will be > + * assured that the bridge driver is connected between the > + * &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations. > + * Therefore, it must run mipi_dsi_host_register() in its probe > + * function, and then run drm_bridge_attach() in its > + * &mipi_dsi_host_ops.attach hook. > + * > + * - The display driver uses the component framework and is a MIPI-DSI > + * host. The bridge device uses the MIPI-DCS commands to be > + * controlled. This is the same situation than above, and can run > + * mipi_dsi_host_register() in either its probe or bind hooks. > + * > + * - The display driver uses the component framework and is a MIPI-DSI > + * host. The bridge device uses a separate bus (such as I2C) to be > + * controlled. In this case, there's no correlation between the probe > + * of the bridge and display drivers, so care must be taken to avoid > + * an endless EPROBE_DEFER loop, with each driver waiting for the > + * other to probe. > + * > + * The ideal pattern to cover the last item (and all the others in the > + * display driver case) is to split the operations like this: > + * > + * - In the display driver must run mipi_dsi_host_register() and > + * component_add in its probe hook. It will make sure that the > + * MIPI-DSI host sticks around, and that the driver's bind can be > + * called. > + * > + * - In its probe hook, the bridge driver must not try to find its > + * MIPI-DSI host or register as a MIPI-DSI device. As far as the > + * framework is concerned, it must only call drm_bridge_add(). > + * > + * - In its bind hook, the display driver must try to find the bridge > + * and return -EPROBE_DEFER if it doesn't find it. If it's there, it > + * must call drm_bridge_attach(). The MIPI-DSI host is now functional. There is an another problem occur for this scenario in the case of kms hotplug driver, sun6i_mipi_dsi.c. When host attach wait till drm device pointer found and drm device pointer would found only when bind done, and bind would complete only when &drm_bridge_funcs.attach hooks are complete. But, If DSI driver is fully bridge driven then this attach in bind will trigger panel_bridge hook attach and at this point we cannot get panel_bridge at all which indeed second attach would would failed. This is one of the reason I'm trying to use drm_bridge_attach host attach itself instead of component bind, not yet succeeded. Thanks, Jagan.
Hi, On Tue, Jul 27, 2021 at 11:20:54AM +0200, Daniel Vetter wrote: > On Mon, Jul 26, 2021 at 05:16:57PM +0200, Maxime Ripard wrote: > > Hi Daniel, > > > > On Wed, Jul 21, 2021 at 02:05:01PM +0200, Daniel Vetter wrote: > > > On Tue, Jul 20, 2021 at 03:45:19PM +0200, Maxime Ripard wrote: > > > > Interactions between bridges, panels, MIPI-DSI host and the component > > > > framework are not trivial and can lead to probing issues when > > > > implementing a display driver. Let's document the various cases we need > > > > too consider, and the solution to support all the cases. > > > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > > > > I still have this dream that eventually we resurrect a patch to add > > > device_link to bridges/panels (ideally automatically), to help with some > > > of the suspend/resume issues around here. > > > > > > Will this make things worse? > > > > > > I think it'd be really good to figure that out with some coding, since if > > > we have incompatible solution to handle probe issues vs suspend/resume > > > issues, we're screwed. > > > > > > Atm the duct-tape is to carefully move things around between suspend and > > > suspend_early hooks (and resume and resume_late) and hope it all works ... > > > > My initial idea to fix this was indeed to use device links. I gave up > > after a while since it doesn't look like there's a way to add a device > > link before either the bridge or encoder probes. > > > > Indeed the OF-Graph representation is device-specific, so it can't be > > generic, and if you need to probe to add that link, well, it's already > > too late for the probe ordering :) > > But don't we still need the device_link for suspend/resume and module > reload? All very annoying indeed anyway. I guess we would still need it for proper suspend and resume ordering (but I never really worked on that part, so I'm not sure), but it's a bit orthogonal to the issue here since those can be added after probe Maxime
Hi Jagan, On Tue, Jul 27, 2021 at 03:12:09PM +0530, Jagan Teki wrote: > On Tue, Jul 20, 2021 at 7:15 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > Interactions between bridges, panels, MIPI-DSI host and the component > > framework are not trivial and can lead to probing issues when > > implementing a display driver. Let's document the various cases we need > > too consider, and the solution to support all the cases. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > Documentation/gpu/drm-kms-helpers.rst | 6 +++ > > drivers/gpu/drm/drm_bridge.c | 60 +++++++++++++++++++++++++++ > > 2 files changed, 66 insertions(+) > > > > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst > > index 10f8df7aecc0..ec2f65b31930 100644 > > --- a/Documentation/gpu/drm-kms-helpers.rst > > +++ b/Documentation/gpu/drm-kms-helpers.rst > > @@ -157,6 +157,12 @@ Display Driver Integration > > .. kernel-doc:: drivers/gpu/drm/drm_bridge.c > > :doc: display driver integration > > > > +Special Care with MIPI-DSI bridges > > +---------------------------------- > > + > > +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c > > + :doc: special care dsi > > + > > Bridge Operations > > ----------------- > > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > index c9a950bfdfe5..81f8dac12367 100644 > > --- a/drivers/gpu/drm/drm_bridge.c > > +++ b/drivers/gpu/drm/drm_bridge.c > > @@ -95,6 +95,66 @@ > > * documentation of bridge operations for more details). > > */ > > > > +/** > > + * DOC: special care dsi > > + * > > + * The interaction between the bridges and other frameworks involved in > > + * the probing of the display driver and the bridge driver can be > > + * challenging. Indeed, there's multiple cases that needs to be > > + * considered: > > + * > > + * - The display driver doesn't use the component framework and isn't a > > + * MIPI-DSI host. In this case, the bridge driver will probe at some > > + * point and the display driver should try to probe again by returning > > + * EPROBE_DEFER as long as the bridge driver hasn't probed. > > + * > > + * - The display driver doesn't use the component framework, but is a > > + * MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be > > + * controlled. In this case, the bridge device is a child of the > > + * display device and when it will probe it's assured that the display > > + * device (and MIPI-DSI host) is present. The display driver will be > > + * assured that the bridge driver is connected between the > > + * &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations. > > + * Therefore, it must run mipi_dsi_host_register() in its probe > > + * function, and then run drm_bridge_attach() in its > > + * &mipi_dsi_host_ops.attach hook. > > + * > > + * - The display driver uses the component framework and is a MIPI-DSI > > + * host. The bridge device uses the MIPI-DCS commands to be > > + * controlled. This is the same situation than above, and can run > > + * mipi_dsi_host_register() in either its probe or bind hooks. > > + * > > + * - The display driver uses the component framework and is a MIPI-DSI > > + * host. The bridge device uses a separate bus (such as I2C) to be > > + * controlled. In this case, there's no correlation between the probe > > + * of the bridge and display drivers, so care must be taken to avoid > > + * an endless EPROBE_DEFER loop, with each driver waiting for the > > + * other to probe. > > + * > > + * The ideal pattern to cover the last item (and all the others in the > > + * display driver case) is to split the operations like this: > > + * > > + * - In the display driver must run mipi_dsi_host_register() and > > + * component_add in its probe hook. It will make sure that the > > + * MIPI-DSI host sticks around, and that the driver's bind can be > > + * called. > > + * > > + * - In its probe hook, the bridge driver must not try to find its > > + * MIPI-DSI host or register as a MIPI-DSI device. As far as the > > + * framework is concerned, it must only call drm_bridge_add(). > > + * > > + * - In its bind hook, the display driver must try to find the bridge > > + * and return -EPROBE_DEFER if it doesn't find it. If it's there, it > > + * must call drm_bridge_attach(). The MIPI-DSI host is now functional. > > There is an another problem occur for this scenario in the case of kms > hotplug driver, sun6i_mipi_dsi.c. When host attach wait till drm > device pointer found and drm device pointer would found only when bind > done, and bind would complete only when &drm_bridge_funcs.attach hooks > are complete. But, If DSI driver is fully bridge driven then this > attach in bind will trigger panel_bridge hook attach and at this point > we cannot get panel_bridge at all which indeed second attach would > would failed. > > This is one of the reason I'm trying to use drm_bridge_attach host > attach itself instead of component bind, not yet succeeded. I'm not really sure what you mean, but if you mention the code we have in the DSI driver to make sure we can probe without our panel, then it's not something that we really can support. Bridges cannot be hotplugged in DRM and having some inconsistencies between drivers (since none of them behave the same way there) and between what's plugged on the other side of the DSI bus feels weird. Maxime
Hi Maxime, On Wed, Jul 28, 2021 at 7:05 PM Maxime Ripard <maxime@cerno.tech> wrote: > > Hi Jagan, > > On Tue, Jul 27, 2021 at 03:12:09PM +0530, Jagan Teki wrote: > > On Tue, Jul 20, 2021 at 7:15 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > Interactions between bridges, panels, MIPI-DSI host and the component > > > framework are not trivial and can lead to probing issues when > > > implementing a display driver. Let's document the various cases we need > > > too consider, and the solution to support all the cases. > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > --- > > > Documentation/gpu/drm-kms-helpers.rst | 6 +++ > > > drivers/gpu/drm/drm_bridge.c | 60 +++++++++++++++++++++++++++ > > > 2 files changed, 66 insertions(+) > > > > > > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst > > > index 10f8df7aecc0..ec2f65b31930 100644 > > > --- a/Documentation/gpu/drm-kms-helpers.rst > > > +++ b/Documentation/gpu/drm-kms-helpers.rst > > > @@ -157,6 +157,12 @@ Display Driver Integration > > > .. kernel-doc:: drivers/gpu/drm/drm_bridge.c > > > :doc: display driver integration > > > > > > +Special Care with MIPI-DSI bridges > > > +---------------------------------- > > > + > > > +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c > > > + :doc: special care dsi > > > + > > > Bridge Operations > > > ----------------- > > > > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > > index c9a950bfdfe5..81f8dac12367 100644 > > > --- a/drivers/gpu/drm/drm_bridge.c > > > +++ b/drivers/gpu/drm/drm_bridge.c > > > @@ -95,6 +95,66 @@ > > > * documentation of bridge operations for more details). > > > */ > > > > > > +/** > > > + * DOC: special care dsi > > > + * > > > + * The interaction between the bridges and other frameworks involved in > > > + * the probing of the display driver and the bridge driver can be > > > + * challenging. Indeed, there's multiple cases that needs to be > > > + * considered: > > > + * > > > + * - The display driver doesn't use the component framework and isn't a > > > + * MIPI-DSI host. In this case, the bridge driver will probe at some > > > + * point and the display driver should try to probe again by returning > > > + * EPROBE_DEFER as long as the bridge driver hasn't probed. > > > + * > > > + * - The display driver doesn't use the component framework, but is a > > > + * MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be > > > + * controlled. In this case, the bridge device is a child of the > > > + * display device and when it will probe it's assured that the display > > > + * device (and MIPI-DSI host) is present. The display driver will be > > > + * assured that the bridge driver is connected between the > > > + * &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations. > > > + * Therefore, it must run mipi_dsi_host_register() in its probe > > > + * function, and then run drm_bridge_attach() in its > > > + * &mipi_dsi_host_ops.attach hook. > > > + * > > > + * - The display driver uses the component framework and is a MIPI-DSI > > > + * host. The bridge device uses the MIPI-DCS commands to be > > > + * controlled. This is the same situation than above, and can run > > > + * mipi_dsi_host_register() in either its probe or bind hooks. > > > + * > > > + * - The display driver uses the component framework and is a MIPI-DSI > > > + * host. The bridge device uses a separate bus (such as I2C) to be > > > + * controlled. In this case, there's no correlation between the probe > > > + * of the bridge and display drivers, so care must be taken to avoid > > > + * an endless EPROBE_DEFER loop, with each driver waiting for the > > > + * other to probe. > > > + * > > > + * The ideal pattern to cover the last item (and all the others in the > > > + * display driver case) is to split the operations like this: > > > + * > > > + * - In the display driver must run mipi_dsi_host_register() and > > > + * component_add in its probe hook. It will make sure that the > > > + * MIPI-DSI host sticks around, and that the driver's bind can be > > > + * called. > > > + * > > > + * - In its probe hook, the bridge driver must not try to find its > > > + * MIPI-DSI host or register as a MIPI-DSI device. As far as the > > > + * framework is concerned, it must only call drm_bridge_add(). > > > + * > > > + * - In its bind hook, the display driver must try to find the bridge > > > + * and return -EPROBE_DEFER if it doesn't find it. If it's there, it > > > + * must call drm_bridge_attach(). The MIPI-DSI host is now functional. > > > > There is an another problem occur for this scenario in the case of kms > > hotplug driver, sun6i_mipi_dsi.c. When host attach wait till drm > > device pointer found and drm device pointer would found only when bind > > done, and bind would complete only when &drm_bridge_funcs.attach hooks > > are complete. But, If DSI driver is fully bridge driven then this > > attach in bind will trigger panel_bridge hook attach and at this point > > we cannot get panel_bridge at all which indeed second attach would > > would failed. > > > > This is one of the reason I'm trying to use drm_bridge_attach host > > attach itself instead of component bind, not yet succeeded. > > I'm not really sure what you mean, but if you mention the code we have > in the DSI driver to make sure we can probe without our panel, then it's > not something that we really can support. Bridges cannot be hotplugged > in DRM and having some inconsistencies between drivers (since none of > them behave the same way there) and between what's plugged on the other > side of the DSI bus feels weird. Yes, but for associated bridges to attach on component base DSI drivers the panel_bridge or bridge pointer look necessary. Here is the pseudo code for sun6i_mipi_dsi which support the DSI probe without panel, by waiting for drm pointer to found, but the same seems not possible for bridge cases. static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { return drm_bridge_attach(bridge->encoder, dsi->bridge, bridge, flags); } static int sun6i_dsi_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { ... if (!dsi->drm || !dsi->drm->registered) return -EPROBE_DEFER; panel = of_drm_find_panel(device->dev.of_node); if (IS_ERR(panel)) { panel = NULL; bridge = of_drm_find_bridge(device->dev.of_node); if (IS_ERR(bridge)) { dev_err(dsi->dev, "failed to find bridge\n"); return PTR_ERR(bridge); } } else { bridge = NULL; } dsi->panel = panel; dsi->bridge = bridge; .... } static int sun6i_dsi_bind(struct device *dev, struct device *master, void *data) { ... ret = drm_bridge_attach(&dsi->encoder, dsi->bridge, NULL, 0); .. dsi->drm = drm; } I believe some-sort bridge handling in hotpulg would necessary to keep the hotplug probing happens for bridge pointers as well. Thanks, Jagan.
diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index 10f8df7aecc0..ec2f65b31930 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -157,6 +157,12 @@ Display Driver Integration .. kernel-doc:: drivers/gpu/drm/drm_bridge.c :doc: display driver integration +Special Care with MIPI-DSI bridges +---------------------------------- + +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c + :doc: special care dsi + Bridge Operations ----------------- diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c9a950bfdfe5..81f8dac12367 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -95,6 +95,66 @@ * documentation of bridge operations for more details). */ +/** + * DOC: special care dsi + * + * The interaction between the bridges and other frameworks involved in + * the probing of the display driver and the bridge driver can be + * challenging. Indeed, there's multiple cases that needs to be + * considered: + * + * - The display driver doesn't use the component framework and isn't a + * MIPI-DSI host. In this case, the bridge driver will probe at some + * point and the display driver should try to probe again by returning + * EPROBE_DEFER as long as the bridge driver hasn't probed. + * + * - The display driver doesn't use the component framework, but is a + * MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be + * controlled. In this case, the bridge device is a child of the + * display device and when it will probe it's assured that the display + * device (and MIPI-DSI host) is present. The display driver will be + * assured that the bridge driver is connected between the + * &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations. + * Therefore, it must run mipi_dsi_host_register() in its probe + * function, and then run drm_bridge_attach() in its + * &mipi_dsi_host_ops.attach hook. + * + * - The display driver uses the component framework and is a MIPI-DSI + * host. The bridge device uses the MIPI-DCS commands to be + * controlled. This is the same situation than above, and can run + * mipi_dsi_host_register() in either its probe or bind hooks. + * + * - The display driver uses the component framework and is a MIPI-DSI + * host. The bridge device uses a separate bus (such as I2C) to be + * controlled. In this case, there's no correlation between the probe + * of the bridge and display drivers, so care must be taken to avoid + * an endless EPROBE_DEFER loop, with each driver waiting for the + * other to probe. + * + * The ideal pattern to cover the last item (and all the others in the + * display driver case) is to split the operations like this: + * + * - In the display driver must run mipi_dsi_host_register() and + * component_add in its probe hook. It will make sure that the + * MIPI-DSI host sticks around, and that the driver's bind can be + * called. + * + * - In its probe hook, the bridge driver must not try to find its + * MIPI-DSI host or register as a MIPI-DSI device. As far as the + * framework is concerned, it must only call drm_bridge_add(). + * + * - In its bind hook, the display driver must try to find the bridge + * and return -EPROBE_DEFER if it doesn't find it. If it's there, it + * must call drm_bridge_attach(). The MIPI-DSI host is now functional. + * + * - In its &drm_bridge_funcs.attach hook, the bridge driver can now try + * to find its MIPI-DSI host and can register as a MIPI-DSI device. + * + * At this point, we're now certain that both the display driver and the + * bridge driver are functional and we can't have a deadlock-like + * situation when probing. + */ + static DEFINE_MUTEX(bridge_lock); static LIST_HEAD(bridge_list);
Interactions between bridges, panels, MIPI-DSI host and the component framework are not trivial and can lead to probing issues when implementing a display driver. Let's document the various cases we need too consider, and the solution to support all the cases. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- Documentation/gpu/drm-kms-helpers.rst | 6 +++ drivers/gpu/drm/drm_bridge.c | 60 +++++++++++++++++++++++++++ 2 files changed, 66 insertions(+)