Hi,
We've encountered an issue with the RaspberryPi DSI panel that prevented the whole display driver from probing.
The issue is described in detail in the commit 7213246a803f ("drm/vc4: dsi: Only register our component once a DSI device is attached"), but the basic idea is that since the panel is probed through i2c, there's no synchronization between its probe and the registration of the MIPI-DSI host it's attached to.
We initially moved the component framework registration to the MIPI-DSI Host attach hook to make sure we register our component only when we have a DSI device attached to our MIPI-DSI host, and then use lookup our DSI device in our bind hook.
However, all the DSI bridges controlled through i2c are only registering their associated DSI device in their bridge attach hook, meaning with our change above, we never got that far, and therefore ended up in the same situation than the one we were trying to fix for panels.
The best practice to avoid those issues is to register its functions only after all its dependencies are live. We also shouldn't wait any longer than we should to play nice with the other components that are waiting for us, so in our case that would mean moving the DSI device registration to the bridge probe.
If the general approach is agreed upon, other bridge drivers will obviously be converted.
Let me know what you think, Maxime
---
Changes from v2: - Changed the approach as suggested by Andrzej, and aligned the bridge on the panel this time. - Fixed some typos
Changes from v1: - Change the name of drm_of_get_next function to drm_of_get_bridge - Mention the revert of 87154ff86bf6 and squash the two patches that were reverting that commit - Add some documentation - Make drm_panel_attach and _detach succeed when no callback is there
Maxime Ripard (8): drm/bridge: Add documentation sections drm/bridge: Document the probe issue with MIPI-DSI bridges drm/mipi-dsi: Create devm device registration drm/mipi-dsi: Create devm device attachment drm/bridge: ps8640: Switch to devm MIPI-DSI helpers drm/bridge: ps8640: Register and attach our DSI device at probe drm/bridge: sn65dsi83: Switch to devm MIPI-DSI helpers drm/bridge: sn65dsi83: Register and attach our DSI device at probe
Documentation/gpu/drm-kms-helpers.rst | 12 ++++ drivers/gpu/drm/bridge/parade-ps8640.c | 97 ++++++++++++++------------ drivers/gpu/drm/bridge/ti-sn65dsi83.c | 82 +++++++++++----------- drivers/gpu/drm/drm_bridge.c | 70 +++++++++++++++++-- drivers/gpu/drm/drm_mipi_dsi.c | 81 +++++++++++++++++++++ include/drm/drm_mipi_dsi.h | 4 ++ 6 files changed, 256 insertions(+), 90 deletions(-)
The bridge documentation overview is quite packed already, and we'll add some more documentation that isn't part of an overview at all.
Let's add some sections to the documentation to separate each bits.
Reviewed-by: Sam Ravnborg sam@ravnborg.org Signed-off-by: Maxime Ripard maxime@cerno.tech --- Documentation/gpu/drm-kms-helpers.rst | 6 ++++++ drivers/gpu/drm/drm_bridge.c | 14 +++++++++----- 2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index 389892f36185..10f8df7aecc0 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -151,6 +151,12 @@ Overview .. kernel-doc:: drivers/gpu/drm/drm_bridge.c :doc: overview
+Display Driver Integration +-------------------------- + +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c + :doc: display driver integration + Bridge Operations -----------------
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index a8ed66751c2d..baff74ea4a33 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -49,6 +49,15 @@ * Chaining multiple bridges to the output of a bridge, or the same bridge to * the output of different bridges, is not supported. * + * &drm_bridge, like &drm_panel, aren't &drm_mode_object entities like planes, + * CRTCs, encoders or connectors and hence are not visible to userspace. They + * just provide additional hooks to get the desired output at the end of the + * encoder chain. + */ + +/** + * DOC: display driver integration + * * Display drivers are responsible for linking encoders with the first bridge * in the chains. This is done by acquiring the appropriate bridge with * of_drm_find_bridge() or drm_of_find_panel_or_bridge(), or creating it for a @@ -85,11 +94,6 @@ * helper to create the &drm_connector, or implement it manually on top of the * connector-related operations exposed by the bridge (see the overview * documentation of bridge operations for more details). - * - * &drm_bridge, like &drm_panel, aren't &drm_mode_object entities like planes, - * CRTCs, encoders or connectors and hence are not visible to userspace. They - * just provide additional hooks to get the desired output at the end of the - * encoder chain. */
static DEFINE_MUTEX(bridge_lock);
W dniu 23.08.2021 o 10:47, Maxime Ripard pisze:
The bridge documentation overview is quite packed already, and we'll add some more documentation that isn't part of an overview at all.
Let's add some sections to the documentation to separate each bits.
Reviewed-by: Sam Ravnborg sam@ravnborg.org Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Regards Andrzej
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 | 58 +++++++++++++++++++++++++++ 2 files changed, 64 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 baff74ea4a33..794654233cf5 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -96,6 +96,64 @@ * 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 try to find its MIPI-DSI + * host, register as a MIPI-DSI device and attach the MIPI-DSI device + * to its host. The bridge driver is now functional. + * + * - In its &struct mipi_dsi_host_ops.attach hook, the display driver + * can now add its component. Its bind hook will now be called and + * since the bridge driver is attached and registered, we can now look + * for and attach it. + * + * 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);
Hi Maxime,
On 23.08.2021 10:47, 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
Documentation/gpu/drm-kms-helpers.rst | 6 +++ drivers/gpu/drm/drm_bridge.c | 58 +++++++++++++++++++++++++++ 2 files changed, 64 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 baff74ea4a33..794654233cf5 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -96,6 +96,64 @@
- 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.
I guess component_add is leftover from previous iteration (as you wrote few lines below) component_add should be called from dsi host attach callback.
- In its probe hook, the bridge driver must try to find its MIPI-DSI
- host, register as a MIPI-DSI device and attach the MIPI-DSI device
- to its host. The bridge driver is now functional.
- In its &struct mipi_dsi_host_ops.attach hook, the display driver
- can now add its component. Its bind hook will now be called and
- since the bridge driver is attached and registered, we can now look
- for and attach it.
- 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.
- */
Beside small mistake the whole patch looks OK for me. Maybe it would be worth to mention what is the real cause of this "special DSI case" - there is mutual dependency between two following entities in display chain:
1. display driver - it provides DSI bus, and requires drm_bridge or drm_panel provided by child device.
2. bridge or panel with DSI transport - it requires DSI bus provided by display driver, and provides drm_bridge or drm_panel interface required by display driver.
I guess similar issues can appear with other data/control bus-es, apparently DSI case is the most common.
And one more thing - you use "display driver" term but this is also case of any bridge providing DSI bus - there are already 3 such bridges in kernel - cdns, nwl, synopsys, tc358768, maybe "DSI host" would be better term.
And another thing - downstream device can be bridge or *panel*, it would be good to mention that panels also should follow this pattern.
Btw this is another place where word bridge can be 1:1 replaced by word panel - it clearly suggest that DRM subsystem waits for brave men who proposes patches unifying them, we would save lot of words, and lines of code if we could use drm_sink instead of "if (sink is bridge) do sth else do sth-similar-but-with-drm_panel-interface".
Regards
Andrzej
static DEFINE_MUTEX(bridge_lock); static LIST_HEAD(bridge_list);
Hi Andrzej,
On Mon, Aug 23, 2021 at 06:32:11PM +0200, Andrzej Hajda wrote:
Hi Maxime,
On 23.08.2021 10:47, 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
Documentation/gpu/drm-kms-helpers.rst | 6 +++ drivers/gpu/drm/drm_bridge.c | 58 +++++++++++++++++++++++++++ 2 files changed, 64 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 baff74ea4a33..794654233cf5 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -96,6 +96,64 @@
- 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.
I guess component_add is leftover from previous iteration (as you wrote few lines below) component_add should be called from dsi host attach callback.
Indeed, I'll remove it
- In its probe hook, the bridge driver must try to find its MIPI-DSI
- host, register as a MIPI-DSI device and attach the MIPI-DSI device
- to its host. The bridge driver is now functional.
- In its &struct mipi_dsi_host_ops.attach hook, the display driver
- can now add its component. Its bind hook will now be called and
- since the bridge driver is attached and registered, we can now look
- for and attach it.
- 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.
- */
Beside small mistake the whole patch looks OK for me. Maybe it would be worth to mention what is the real cause of this "special DSI case" - there is mutual dependency between two following entities in display chain:
- display driver - it provides DSI bus, and requires drm_bridge or
drm_panel provided by child device.
- bridge or panel with DSI transport - it requires DSI bus provided by
display driver, and provides drm_bridge or drm_panel interface required by display driver.
I was trying to explain it in the first part of this patch. Is there anything misleading there?
I guess similar issues can appear with other data/control bus-es, apparently DSI case is the most common.
The issue only presents itself when it's using a separate control bus actually. If it's controlled through DCS, the panel / bridge will be a children node of the DSI host and will only be probed when the host is registered, so we don't have this issue.
And one more thing - you use "display driver" term but this is also case of any bridge providing DSI bus - there are already 3 such bridges in kernel - cdns, nwl, synopsys, tc358768, maybe "DSI host" would be better term.
Good point, I'll change it.
And another thing - downstream device can be bridge or *panel*, it would be good to mention that panels also should follow this pattern.
We're pretty much forced to do this with panels though. They don't have an attach hook unlike bridges so we don't have much other options than putting it in probe.
Btw this is another place where word bridge can be 1:1 replaced by word panel - it clearly suggest that DRM subsystem waits for brave men who proposes patches unifying them, we would save lot of words, and lines of code if we could use drm_sink instead of "if (sink is bridge) do sth else do sth-similar-but-with-drm_panel-interface".
I agree. In the previous iteration I had this patch that was a step in this direction: https://lore.kernel.org/dri-devel/20210728133229.2247965-3-maxime@cerno.tech...
Even though it's not relevant to this series anymore, I still plan on submitting it and converting as many users as possible (if my coccinelle skills allows me to at least).
Maxime
Devices that take their data through the MIPI-DSI bus but are controlled through a secondary bus like I2C have to register a secondary device on the MIPI-DSI bus through the mipi_dsi_device_register_full() function.
At removal or when an error occurs, that device needs to be removed through a call to mipi_dsi_device_unregister().
Let's create a device-managed variant of the registration function that will automatically unregister the device at unbind.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/drm_mipi_dsi.c | 46 ++++++++++++++++++++++++++++++++++ include/drm/drm_mipi_dsi.h | 3 +++ 2 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 5dd475e82995..ddf67463eaa1 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -246,6 +246,52 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi) } EXPORT_SYMBOL(mipi_dsi_device_unregister);
+static void devm_mipi_dsi_device_unregister(void *arg) +{ + struct mipi_dsi_device *dsi = arg; + + mipi_dsi_device_unregister(dsi); +} + +/** + * devm_mipi_dsi_device_register_full - create a managed MIPI DSI device + * @dev: device to tie the MIPI-DSI device lifetime to + * @host: DSI host to which this device is connected + * @info: pointer to template containing DSI device information + * + * Create a MIPI DSI device by using the device information provided by + * mipi_dsi_device_info template + * + * This is the managed version of mipi_dsi_device_register_full() which + * automatically calls mipi_dsi_device_unregister() when @dev is + * unbound. + * + * Returns: + * A pointer to the newly created MIPI DSI device, or, a pointer encoded + * with an error + */ +struct mipi_dsi_device * +devm_mipi_dsi_device_register_full(struct device *dev, + struct mipi_dsi_host *host, + const struct mipi_dsi_device_info *info) +{ + struct mipi_dsi_device *dsi; + int ret; + + dsi = mipi_dsi_device_register_full(host, info); + if (IS_ERR(dsi)) + return dsi; + + ret = devm_add_action_or_reset(dev, + devm_mipi_dsi_device_unregister, + dsi); + if (ret) + return ERR_PTR(ret); + + return dsi; +} +EXPORT_SYMBOL_GPL(devm_mipi_dsi_device_register_full); + static DEFINE_MUTEX(host_lock); static LIST_HEAD(host_list);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index af7ba8071eb0..d0032e435e08 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -227,6 +227,9 @@ struct mipi_dsi_device * mipi_dsi_device_register_full(struct mipi_dsi_host *host, const struct mipi_dsi_device_info *info); void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi); +struct mipi_dsi_device * +devm_mipi_dsi_device_register_full(struct device *dev, struct mipi_dsi_host *host, + const struct mipi_dsi_device_info *info); struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np); int mipi_dsi_attach(struct mipi_dsi_device *dsi); int mipi_dsi_detach(struct mipi_dsi_device *dsi);
W dniu 23.08.2021 o 10:47, Maxime Ripard pisze:
Devices that take their data through the MIPI-DSI bus but are controlled through a secondary bus like I2C have to register a secondary device on the MIPI-DSI bus through the mipi_dsi_device_register_full() function.
At removal or when an error occurs, that device needs to be removed through a call to mipi_dsi_device_unregister().
Let's create a device-managed variant of the registration function that will automatically unregister the device at unbind.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Regards Andrzej
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/drm_mipi_dsi.c | 35 ++++++++++++++++++++++++++++++++++ include/drm/drm_mipi_dsi.h | 1 + 2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index ddf67463eaa1..18cef04df2f2 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -391,6 +391,41 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi) } EXPORT_SYMBOL(mipi_dsi_detach);
+static void devm_mipi_dsi_detach(void *arg) +{ + struct mipi_dsi_device *dsi = arg; + + mipi_dsi_detach(dsi); +} + +/** + * devm_mipi_dsi_attach - Attach a MIPI-DSI device to its DSI Host + * @dev: device to tie the MIPI-DSI device attachment lifetime to + * @dsi: DSI peripheral + * + * This is the managed version of mipi_dsi_attach() which automatically + * calls mipi_dsi_detach() when @dev is unbound. + * + * Returns: + * 0 on success, a negative error code on failure. + */ +int devm_mipi_dsi_attach(struct device *dev, + struct mipi_dsi_device *dsi) +{ + int ret; + + ret = mipi_dsi_attach(dsi); + if (ret) + return ret; + + ret = devm_add_action_or_reset(dev, devm_mipi_dsi_detach, dsi); + if (ret) + return ret; + + return 0; +} +EXPORT_SYMBOL_GPL(devm_mipi_dsi_attach); + static ssize_t mipi_dsi_device_transfer(struct mipi_dsi_device *dsi, struct mipi_dsi_msg *msg) { diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index d0032e435e08..147e51b6d241 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -233,6 +233,7 @@ devm_mipi_dsi_device_register_full(struct device *dev, struct mipi_dsi_host *hos struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np); int mipi_dsi_attach(struct mipi_dsi_device *dsi); int mipi_dsi_detach(struct mipi_dsi_device *dsi); +int devm_mipi_dsi_attach(struct device *dev, struct mipi_dsi_device *dsi); int mipi_dsi_shutdown_peripheral(struct mipi_dsi_device *dsi); int mipi_dsi_turn_on_peripheral(struct mipi_dsi_device *dsi); int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
W dniu 23.08.2021 o 10:47, Maxime Ripard pisze:
Signed-off-by: Maxime Ripard maxime@cerno.tech
Missing description.
With this fixed:
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Regards Andrzej
Let's switch to the new devm MIPI-DSI function to register and attach our secondary device. This also avoids leaking the device on removal.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/parade-ps8640.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 7bd0affa057a..794c9516b05d 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -241,7 +241,7 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge, if (!host) return -ENODEV;
- dsi = mipi_dsi_device_register_full(host, &info); + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); if (IS_ERR(dsi)) { dev_err(dev, "failed to create dsi device\n"); ret = PTR_ERR(dsi); @@ -255,17 +255,13 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge, MIPI_DSI_MODE_VIDEO_SYNC_PULSE; dsi->format = MIPI_DSI_FMT_RGB888; dsi->lanes = DP_NUM_LANES; - ret = mipi_dsi_attach(dsi); + ret = devm_mipi_dsi_attach(dev, dsi); if (ret) - goto err_dsi_attach; + return ret;
/* Attach the panel-bridge to the dsi bridge */ return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge, &ps_bridge->bridge, flags); - -err_dsi_attach: - mipi_dsi_device_unregister(dsi); - return ret; }
static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
In order to avoid any probe ordering issue, the best practice is to move the secondary MIPI-DSI device registration and attachment to the MIPI-DSI host at probe time. Let's do this.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/parade-ps8640.c | 93 ++++++++++++++------------ 1 file changed, 51 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 794c9516b05d..37f7d166a3c6 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -213,52 +213,10 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); - struct device *dev = &ps_bridge->page[0]->dev; - struct device_node *in_ep, *dsi_node; - struct mipi_dsi_device *dsi; - struct mipi_dsi_host *host; - int ret; - const struct mipi_dsi_device_info info = { .type = "ps8640", - .channel = 0, - .node = NULL, - };
if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) return -EINVAL;
- /* port@0 is ps8640 dsi input port */ - in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); - if (!in_ep) - return -ENODEV; - - dsi_node = of_graph_get_remote_port_parent(in_ep); - of_node_put(in_ep); - if (!dsi_node) - return -ENODEV; - - host = of_find_mipi_dsi_host_by_node(dsi_node); - of_node_put(dsi_node); - if (!host) - return -ENODEV; - - dsi = devm_mipi_dsi_device_register_full(dev, host, &info); - if (IS_ERR(dsi)) { - dev_err(dev, "failed to create dsi device\n"); - ret = PTR_ERR(dsi); - return ret; - } - - ps_bridge->dsi = dsi; - - dsi->host = host; - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | - MIPI_DSI_MODE_VIDEO_SYNC_PULSE; - dsi->format = MIPI_DSI_FMT_RGB888; - dsi->lanes = DP_NUM_LANES; - ret = devm_mipi_dsi_attach(dev, dsi); - if (ret) - return ret; - /* Attach the panel-bridge to the dsi bridge */ return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge, &ps_bridge->bridge, flags); @@ -305,6 +263,53 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { .pre_enable = ps8640_pre_enable, };
+static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridge) +{ + struct device_node *in_ep, *dsi_node; + struct mipi_dsi_device *dsi; + struct mipi_dsi_host *host; + int ret; + const struct mipi_dsi_device_info info = { .type = "ps8640", + .channel = 0, + .node = NULL, + }; + + /* port@0 is ps8640 dsi input port */ + in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); + if (!in_ep) + return -ENODEV; + + dsi_node = of_graph_get_remote_port_parent(in_ep); + of_node_put(in_ep); + if (!dsi_node) + return -ENODEV; + + host = of_find_mipi_dsi_host_by_node(dsi_node); + of_node_put(dsi_node); + if (!host) + return -EPROBE_DEFER; + + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); + if (IS_ERR(dsi)) { + dev_err(dev, "failed to create dsi device\n"); + return PTR_ERR(dsi); + } + + ps_bridge->dsi = dsi; + + dsi->host = host; + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | + MIPI_DSI_MODE_VIDEO_SYNC_PULSE; + dsi->format = MIPI_DSI_FMT_RGB888; + dsi->lanes = DP_NUM_LANES; + + ret = devm_mipi_dsi_attach(dev, dsi); + if (ret) + return ret; + + return 0; +} + static int ps8640_probe(struct i2c_client *client) { struct device *dev = &client->dev; @@ -371,6 +376,10 @@ static int ps8640_probe(struct i2c_client *client)
drm_bridge_add(&ps_bridge->bridge);
+ ret = ps8640_bridge_host_attach(dev, ps_bridge); + if (ret) + return ret; + return 0; }
W dniu 23.08.2021 o 10:47, Maxime Ripard pisze:
In order to avoid any probe ordering issue, the best practice is to move the secondary MIPI-DSI device registration and attachment to the MIPI-DSI host at probe time. Let's do this.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/bridge/parade-ps8640.c | 93 ++++++++++++++------------ 1 file changed, 51 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 794c9516b05d..37f7d166a3c6 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -213,52 +213,10 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
struct device *dev = &ps_bridge->page[0]->dev;
struct device_node *in_ep, *dsi_node;
struct mipi_dsi_device *dsi;
struct mipi_dsi_host *host;
int ret;
const struct mipi_dsi_device_info info = { .type = "ps8640",
.channel = 0,.node = NULL,};if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) return -EINVAL;
/* port@0 is ps8640 dsi input port */
in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
if (!in_ep)
return -ENODEV;dsi_node = of_graph_get_remote_port_parent(in_ep);
of_node_put(in_ep);
if (!dsi_node)
return -ENODEV;host = of_find_mipi_dsi_host_by_node(dsi_node);
of_node_put(dsi_node);
if (!host)
return -ENODEV;dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
if (IS_ERR(dsi)) {
dev_err(dev, "failed to create dsi device\n");ret = PTR_ERR(dsi);return ret;}
ps_bridge->dsi = dsi;
dsi->host = host;
dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
MIPI_DSI_MODE_VIDEO_SYNC_PULSE;dsi->format = MIPI_DSI_FMT_RGB888;
dsi->lanes = DP_NUM_LANES;
ret = devm_mipi_dsi_attach(dev, dsi);
if (ret)
return ret;/* Attach the panel-bridge to the dsi bridge */ return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge, &ps_bridge->bridge, flags);
@@ -305,6 +263,53 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { .pre_enable = ps8640_pre_enable, };
+static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridge) +{
- struct device_node *in_ep, *dsi_node;
- struct mipi_dsi_device *dsi;
- struct mipi_dsi_host *host;
- int ret;
- const struct mipi_dsi_device_info info = { .type = "ps8640",
.channel = 0,.node = NULL,};- /* port@0 is ps8640 dsi input port */
- in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
- if (!in_ep)
return -ENODEV;- dsi_node = of_graph_get_remote_port_parent(in_ep);
- of_node_put(in_ep);
- if (!dsi_node)
return -ENODEV;- host = of_find_mipi_dsi_host_by_node(dsi_node);
- of_node_put(dsi_node);
- if (!host)
return -EPROBE_DEFER;- dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
- if (IS_ERR(dsi)) {
dev_err(dev, "failed to create dsi device\n");return PTR_ERR(dsi);- }
- ps_bridge->dsi = dsi;
- dsi->host = host;
- dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
MIPI_DSI_MODE_VIDEO_SYNC_PULSE;- dsi->format = MIPI_DSI_FMT_RGB888;
- dsi->lanes = DP_NUM_LANES;
- ret = devm_mipi_dsi_attach(dev, dsi);
- if (ret)
return ret;- return 0;
+}
- static int ps8640_probe(struct i2c_client *client) { struct device *dev = &client->dev;
@@ -371,6 +376,10 @@ static int ps8640_probe(struct i2c_client *client)
drm_bridge_add(&ps_bridge->bridge);
- ret = ps8640_bridge_host_attach(dev, ps_bridge);
- if (ret)
return ret;
I do not see drm_bridge_remove on error path, the same for sn65dsi83.
Regards
Andrzej
- return 0; }
Let's switch to the new devm MIPI-DSI function to register and attach our secondary device. This also avoids leaking the device when we detach the bridge but don't remove its driver.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index a32f70bc68ea..db4d39082705 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -262,7 +262,7 @@ static int sn65dsi83_attach(struct drm_bridge *bridge, return -EPROBE_DEFER; }
- dsi = mipi_dsi_device_register_full(host, &info); + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); if (IS_ERR(dsi)) { return dev_err_probe(dev, PTR_ERR(dsi), "failed to create dsi device\n"); @@ -274,18 +274,14 @@ static int sn65dsi83_attach(struct drm_bridge *bridge, dsi->format = MIPI_DSI_FMT_RGB888; dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST;
- ret = mipi_dsi_attach(dsi); + ret = devm_mipi_dsi_attach(dev, dsi); if (ret < 0) { dev_err(dev, "failed to attach dsi to host\n"); - goto err_dsi_attach; + return ret; }
return drm_bridge_attach(bridge->encoder, ctx->panel_bridge, &ctx->bridge, flags); - -err_dsi_attach: - mipi_dsi_device_unregister(dsi); - return ret; }
static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, @@ -697,8 +693,6 @@ static int sn65dsi83_remove(struct i2c_client *client) { struct sn65dsi83 *ctx = i2c_get_clientdata(client);
- mipi_dsi_detach(ctx->dsi); - mipi_dsi_device_unregister(ctx->dsi); drm_bridge_remove(&ctx->bridge); of_node_put(ctx->host_node);
In order to avoid any probe ordering issue, the best practice is to move the secondary MIPI-DSI device registration and attachment to the MIPI-DSI host at probe time. Let's do this.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 76 +++++++++++++++------------ 1 file changed, 42 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index db4d39082705..3a75224b7348 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -245,40 +245,6 @@ static int sn65dsi83_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge); - struct device *dev = ctx->dev; - struct mipi_dsi_device *dsi; - struct mipi_dsi_host *host; - int ret = 0; - - const struct mipi_dsi_device_info info = { - .type = "sn65dsi83", - .channel = 0, - .node = NULL, - }; - - host = of_find_mipi_dsi_host_by_node(ctx->host_node); - if (!host) { - dev_err(dev, "failed to find dsi host\n"); - return -EPROBE_DEFER; - } - - dsi = devm_mipi_dsi_device_register_full(dev, host, &info); - if (IS_ERR(dsi)) { - return dev_err_probe(dev, PTR_ERR(dsi), - "failed to create dsi device\n"); - } - - ctx->dsi = dsi; - - dsi->lanes = ctx->dsi_lanes; - dsi->format = MIPI_DSI_FMT_RGB888; - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST; - - ret = devm_mipi_dsi_attach(dev, dsi); - if (ret < 0) { - dev_err(dev, "failed to attach dsi to host\n"); - return ret; - }
return drm_bridge_attach(bridge->encoder, ctx->panel_bridge, &ctx->bridge, flags); @@ -646,6 +612,44 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) return 0; }
+static int sn65dsi83_host_attach(struct sn65dsi83 *ctx) +{ + struct device *dev = ctx->dev; + struct mipi_dsi_device *dsi; + struct mipi_dsi_host *host; + const struct mipi_dsi_device_info info = { + .type = "sn65dsi83", + .channel = 0, + .node = NULL, + }; + int ret; + + host = of_find_mipi_dsi_host_by_node(ctx->host_node); + if (!host) { + dev_err(dev, "failed to find dsi host\n"); + return -EPROBE_DEFER; + } + + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); + if (IS_ERR(dsi)) + return dev_err_probe(dev, PTR_ERR(dsi), + "failed to create dsi device\n"); + + ctx->dsi = dsi; + + dsi->lanes = ctx->dsi_lanes; + dsi->format = MIPI_DSI_FMT_RGB888; + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST; + + ret = devm_mipi_dsi_attach(dev, dsi); + if (ret < 0) { + dev_err(dev, "failed to attach dsi to host: %d\n", ret); + return ret; + } + + return 0; +} + static int sn65dsi83_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -686,6 +690,10 @@ static int sn65dsi83_probe(struct i2c_client *client, ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge);
+ ret = sn65dsi83_host_attach(ctx); + if (ret) + return ret; + return 0; }
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Maxime Ripard