This patch is v3 of the first 2 patches from my RFC series ("drm/dp: Improvements
for DP AUX channel") [1]. I've broken the series in two so we can make
progress on the two halves separately.
v2 of this series tries to incorporate all the feedback from v1. Hopefully
things are less confusing and simpler this time around. The one thing that got
slightly more confusing is that the done_probing() callback can't return
-EPROBE_DEFER in most cases so we have to adjust drivers a little more.
v3 takes Dmitry's advice on v2. This now introduces
devm_drm_bridge_add() (in an extra patch), splits some fixups into
their own patch, uses a new name for functions, and requires an
explicit call to done_probing if you have no children.
The idea for this series came up during the review process of
Sankeerth's series trying to add eDP for Qualcomm SoCs [2].
This _doesn't_ attempt to fix the Analogix driver. If this works out,
ideally someone can post a patch up to do that.
NOTE: I don't have any ps8640 devices that _don't_ use the aux panel
underneath them, so I'm relying on code inspection to make sure I
didn't break those. If someone sees that I did something wrong for
that case then please yell!
[1] https://lore.kernel.org/r/[email protected]/
[2] https://lore.kernel.org/r/[email protected]/
Changes in v3:
- Adapt to v3 changes in aux bus.
- Don't call done_probing() if there are no children; return -ENODEV.
- Patch ("drm/bridge: Add devm_drm_bridge_add()") new for v3.
- Patch ("drm/dp: Export symbol / kerneldoc fixes...") split for v3.
- Split out EXPORT_SYMBOL and kerneldoc fixes to its own patch.
- Use devm_drm_bridge_add() to simplify.
- Used Dmitry's proposed name: of_dp_aux_populate_bus()
Changes in v2:
- Change to assume exactly one device.
- Have a probe callback instead of an extra sub device.
- Rewrote atop new method introduced by patch #1.
Douglas Anderson (4):
drm/dp: Export symbol / kerneldoc fixes for DP AUX bus
drm/dp: Add callbacks to make using DP AUX bus properly easier
drm/bridge: Add devm_drm_bridge_add()
drm/bridge: parade-ps8640: Handle DP AUX more properly
drivers/gpu/drm/bridge/parade-ps8640.c | 74 +++++---
drivers/gpu/drm/display/drm_dp_aux_bus.c | 211 +++++++++++++++--------
drivers/gpu/drm/drm_bridge.c | 23 +++
include/drm/display/drm_dp_aux_bus.h | 34 +++-
include/drm/drm_bridge.h | 1 +
5 files changed, 238 insertions(+), 105 deletions(-)
--
2.36.0.550.gb090851708-goog
While it works, for the most part, to assume that the panel has
finished probing when devm_of_dp_aux_populate_ep_devices() returns,
it's a bit fragile. This is talked about at length in commit
a1e3667a9835 ("drm/bridge: ti-sn65dsi86: Promote the AUX channel to
its own sub-dev").
When reviewing the ps8640 code, I managed to convince myself that it
was OK not to worry about it there and that maybe it wasn't really
_that_ fragile. However, it turns out that it really is. Simply
hardcoding panel_edp_probe() to return -EPROBE_DEFER was enough to put
the boot process into an infinite loop. I believe this manages to trip
the same issues that we used to trip with the main MSM code where
something about our actions trigger Linux to re-probe previously
deferred devices right away and each time we try again we re-trigger
Linux to re-probe.
Let's fix this using the callback introduced in the patch ("drm/dp:
Callbacks to make it easier for drivers to use DP AUX bus properly").
When using the new callback, we have to be a little careful. The
probe_done() callback is no longer always called in the context of
our probe routine. That means we can't rely on being able to return
-EPROBE_DEFER from it. We re-jigger the order of things a bit to
account for that.
With this change, the device still boots (though obviously the panel
doesn't come up) if I force panel-edp to always return
-EPROBE_DEFER. If I fake it and make the panel probe exactly once it
also works.
Signed-off-by: Douglas Anderson <[email protected]>
---
Changes in v3:
- Adapt to v3 changes in aux bus.
- Use devm_drm_bridge_add() to simplify.
Changes in v2:
- Rewrote atop new method introduced by patch #1.
drivers/gpu/drm/bridge/parade-ps8640.c | 74 ++++++++++++++++----------
1 file changed, 46 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index e2467e58b5b7..ff4227f6d800 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -537,7 +537,7 @@ 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)
+static int ps8640_bridge_get_dsi_resources(struct device *dev, struct ps8640 *ps_bridge)
{
struct device_node *in_ep, *dsi_node;
struct mipi_dsi_device *dsi;
@@ -576,13 +576,40 @@ static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridg
dsi->format = MIPI_DSI_FMT_RGB888;
dsi->lanes = NUM_MIPI_LANES;
- return devm_mipi_dsi_attach(dev, dsi);
+ return 0;
+}
+
+static int ps8640_bridge_link_panel(struct drm_dp_aux *aux)
+{
+ struct ps8640 *ps_bridge = aux_to_ps8640(aux);
+ struct device *dev = aux->dev;
+ struct device_node *np = dev->of_node;
+ int ret;
+
+ /*
+ * NOTE about returning -EPROBE_DEFER from this function: if we
+ * return an error (most relevant to -EPROBE_DEFER) it will only
+ * be passed out to ps8640_probe() if it called this directly (AKA the
+ * panel isn't under the "aux-bus" node). That should be fine because
+ * if the panel is under "aux-bus" it's guaranteed to have probed by
+ * the time this function has been called.
+ */
+
+ /* port@1 is ps8640 output port */
+ ps_bridge->panel_bridge = devm_drm_of_get_bridge(dev, np, 1, 0);
+ if (IS_ERR(ps_bridge->panel_bridge))
+ return PTR_ERR(ps_bridge->panel_bridge);
+
+ ret = devm_drm_bridge_add(dev, &ps_bridge->bridge);
+ if (ret)
+ return ret;
+
+ return devm_mipi_dsi_attach(dev, ps_bridge->dsi);
}
static int ps8640_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
- struct device_node *np = dev->of_node;
struct ps8640 *ps_bridge;
int ret;
u32 i;
@@ -623,6 +650,14 @@ static int ps8640_probe(struct i2c_client *client)
if (!ps8640_of_panel_on_aux_bus(&client->dev))
ps_bridge->bridge.ops = DRM_BRIDGE_OP_EDID;
+ /*
+ * Get MIPI DSI resources early. These can return -EPROBE_DEFER so
+ * we want to get them out of the way sooner.
+ */
+ ret = ps8640_bridge_get_dsi_resources(&client->dev, ps_bridge);
+ if (ret)
+ return ret;
+
ps_bridge->page[PAGE0_DP_CNTL] = client;
ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config);
@@ -665,35 +700,19 @@ static int ps8640_probe(struct i2c_client *client)
if (ret)
return ret;
- devm_of_dp_aux_populate_ep_devices(&ps_bridge->aux);
+ ret = devm_of_dp_aux_populate_bus(&ps_bridge->aux, ps8640_bridge_link_panel);
- /* port@1 is ps8640 output port */
- ps_bridge->panel_bridge = devm_drm_of_get_bridge(dev, np, 1, 0);
- if (IS_ERR(ps_bridge->panel_bridge))
- return PTR_ERR(ps_bridge->panel_bridge);
-
- drm_bridge_add(&ps_bridge->bridge);
-
- ret = ps8640_bridge_host_attach(dev, ps_bridge);
- if (ret)
- goto err_bridge_remove;
-
- return 0;
+ /*
+ * If devm_of_dp_aux_populate_bus() returns -ENODEV then it's up to
+ * usa to call ps8640_bridge_link_panel() directly. NOTE: in this case
+ * the function is allowed to -EPROBE_DEFER.
+ */
+ if (ret == -ENODEV)
+ return ps8640_bridge_link_panel(&ps_bridge->aux);
-err_bridge_remove:
- drm_bridge_remove(&ps_bridge->bridge);
return ret;
}
-static int ps8640_remove(struct i2c_client *client)
-{
- struct ps8640 *ps_bridge = i2c_get_clientdata(client);
-
- drm_bridge_remove(&ps_bridge->bridge);
-
- return 0;
-}
-
static const struct of_device_id ps8640_match[] = {
{ .compatible = "parade,ps8640" },
{ }
@@ -702,7 +721,6 @@ MODULE_DEVICE_TABLE(of, ps8640_match);
static struct i2c_driver ps8640_driver = {
.probe_new = ps8640_probe,
- .remove = ps8640_remove,
.driver = {
.name = "ps8640",
.of_match_table = ps8640_match,
--
2.36.0.550.gb090851708-goog
This adds a devm managed version of drm_bridge_add(). Like other
"devm" function listed in drm_bridge.h, this function takes an
explicit "dev" to use for the lifetime management. A few notes:
* In general we have a "struct device" for bridges that makes a good
candidate for where the lifetime matches exactly what we want.
* The "bridge->dev->dev" device appears to be the encoder
device. That's not the right device to use for lifetime management.
Suggested-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---
Changes in v3:
- Patch ("drm/bridge: Add devm_drm_bridge_add()") new for v3.
drivers/gpu/drm/drm_bridge.c | 23 +++++++++++++++++++++++
include/drm/drm_bridge.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c96847fc0ebc..e275b4ca344b 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -170,6 +170,29 @@ void drm_bridge_add(struct drm_bridge *bridge)
}
EXPORT_SYMBOL(drm_bridge_add);
+static void drm_bridge_remove_void(void *bridge)
+{
+ drm_bridge_remove(bridge);
+}
+
+/**
+ * devm_drm_bridge_add - devm managed version of drm_bridge_add()
+ *
+ * @dev: device to tie the bridge lifetime to
+ * @bridge: bridge control structure
+ *
+ * This is the managed version of drm_bridge_add() which automatically
+ * calls drm_bridge_remove() when @dev is unbound.
+ *
+ * Return: 0 if no error or negative error code.
+ */
+int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge)
+{
+ drm_bridge_add(bridge);
+ return devm_add_action_or_reset(dev, drm_bridge_remove_void, bridge);
+}
+EXPORT_SYMBOL(devm_drm_bridge_add);
+
/**
* drm_bridge_remove - remove the given bridge from the global bridge list
*
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index f27b4060faa2..42aec8612f37 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -796,6 +796,7 @@ drm_priv_to_bridge(struct drm_private_obj *priv)
}
void drm_bridge_add(struct drm_bridge *bridge);
+int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge);
void drm_bridge_remove(struct drm_bridge *bridge);
int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
struct drm_bridge *previous,
--
2.36.0.550.gb090851708-goog
On Tue, 10 May 2022 at 22:30, Douglas Anderson <[email protected]> wrote:
>
> This adds a devm managed version of drm_bridge_add(). Like other
> "devm" function listed in drm_bridge.h, this function takes an
> explicit "dev" to use for the lifetime management. A few notes:
> * In general we have a "struct device" for bridges that makes a good
> candidate for where the lifetime matches exactly what we want.
> * The "bridge->dev->dev" device appears to be the encoder
> device. That's not the right device to use for lifetime management.
>
> Suggested-by: Dmitry Baryshkov <[email protected]>
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
> ---
>
> Changes in v3:
> - Patch ("drm/bridge: Add devm_drm_bridge_add()") new for v3.
>
> drivers/gpu/drm/drm_bridge.c | 23 +++++++++++++++++++++++
> include/drm/drm_bridge.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index c96847fc0ebc..e275b4ca344b 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -170,6 +170,29 @@ void drm_bridge_add(struct drm_bridge *bridge)
> }
> EXPORT_SYMBOL(drm_bridge_add);
>
> +static void drm_bridge_remove_void(void *bridge)
> +{
> + drm_bridge_remove(bridge);
> +}
> +
> +/**
> + * devm_drm_bridge_add - devm managed version of drm_bridge_add()
> + *
> + * @dev: device to tie the bridge lifetime to
> + * @bridge: bridge control structure
> + *
> + * This is the managed version of drm_bridge_add() which automatically
> + * calls drm_bridge_remove() when @dev is unbound.
> + *
> + * Return: 0 if no error or negative error code.
> + */
> +int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge)
> +{
> + drm_bridge_add(bridge);
> + return devm_add_action_or_reset(dev, drm_bridge_remove_void, bridge);
> +}
> +EXPORT_SYMBOL(devm_drm_bridge_add);
> +
> /**
> * drm_bridge_remove - remove the given bridge from the global bridge list
> *
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index f27b4060faa2..42aec8612f37 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -796,6 +796,7 @@ drm_priv_to_bridge(struct drm_private_obj *priv)
> }
>
> void drm_bridge_add(struct drm_bridge *bridge);
> +int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge);
> void drm_bridge_remove(struct drm_bridge *bridge);
> int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> struct drm_bridge *previous,
> --
> 2.36.0.550.gb090851708-goog
>
--
With best wishes
Dmitry
While working on the DP AUX bus code I found a few small things that
should be fixed. Namely the non-devm version of
of_dp_aux_populate_ep_devices() was missing an export. There was also
an extra blank line in a kerneldoc and a kerneldoc that incorrectly
documented a return value. Fix these.
Fixes: aeb33699fc2c ("drm: Introduce the DP AUX bus")
Signed-off-by: Douglas Anderson <[email protected]>
---
None of these seem critical, so my plan is to land this in
drm-misc-next and not drm-misc-fixes. This will avoid merge conflicts
with future patches.
Changes in v3:
- Patch ("drm/dp: Export symbol / kerneldoc fixes...") split for v3.
drivers/gpu/drm/display/drm_dp_aux_bus.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c
index dccf3e2ea323..552f949cff59 100644
--- a/drivers/gpu/drm/display/drm_dp_aux_bus.c
+++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c
@@ -66,7 +66,6 @@ static int dp_aux_ep_probe(struct device *dev)
* @dev: The device to remove.
*
* Calls through to the endpoint driver remove.
- *
*/
static void dp_aux_ep_remove(struct device *dev)
{
@@ -120,8 +119,6 @@ ATTRIBUTE_GROUPS(dp_aux_ep_dev);
/**
* dp_aux_ep_dev_release() - Free memory for the dp_aux_ep device
* @dev: The device to free.
- *
- * Return: 0 if no error or negative error code.
*/
static void dp_aux_ep_dev_release(struct device *dev)
{
@@ -256,6 +253,7 @@ int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
return 0;
}
+EXPORT_SYMBOL_GPL(of_dp_aux_populate_ep_devices);
static void of_dp_aux_depopulate_ep_devices_void(void *data)
{
--
2.36.0.550.gb090851708-goog
As talked about in this patch in the kerneldoc of
of_dp_aux_populate_ep_device() and also in the past in commit
a1e3667a9835 ("drm/bridge: ti-sn65dsi86: Promote the AUX channel to
its own sub-dev"), it can be difficult for eDP controller drivers to
know when the panel has finished probing when they're using
of_dp_aux_populate_ep_devices().
The ti-sn65dsi86 driver managed to solve this because it was already
broken up into a bunch of sub-drivers. That means we could solve the
problem there by adding a new sub-driver to get the panel. We could
use the traditional -EPROBE_DEFER retry mechansim to handle the case
where the panel hadn't probed yet.
In parade-ps8640 we didn't really solve this. The code just expects
the panel to be ready right away. While reviewing the code originally
I had managed to convince myself it was fine to just expect the panel
right away, but additional testing has shown that not to be the
case. We could fix parade-ps8640 like we did ti-sn65dsi86 but it's
pretty cumbersome (since we're not already broken into multiple
drivers) and requires a bunch of boilerplate code.
After discussion [1] it seems like the best solution for most people
is:
- Accept that there's always at most one device that will probe as a
result of the DP AUX bus (it may have sub-devices, but there will be
one device _directly_ probed).
- When that device finishes probing, we can just have a call back.
This patch implements that idea. We'll now take a callback as an
argument to the populate function. To make this easier to land in
pieces, we'll make wrappers for the old functions. The functions with
the new name (which make it clear that we only have one child) will
take the callback and the functions with the old name will temporarily
wrap.
[1] https://lore.kernel.org/r/CAD=FV=Ur3afHhsXe7a3baWEnD=MFKFeKRbhFU+bt3P67G0MVzQ@mail.gmail.com
Signed-off-by: Douglas Anderson <[email protected]>
---
Changes in v3:
- Don't call done_probing() if there are no children; return -ENODEV.
- Split out EXPORT_SYMBOL and kerneldoc fixes to its own patch.
- Used Dmitry's proposed name: of_dp_aux_populate_bus()
Changes in v2:
- Change to assume exactly one device.
- Have a probe callback instead of an extra sub device.
drivers/gpu/drm/display/drm_dp_aux_bus.c | 209 +++++++++++++++--------
include/drm/display/drm_dp_aux_bus.h | 34 +++-
2 files changed, 168 insertions(+), 75 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c
index 552f949cff59..f5741b45ca07 100644
--- a/drivers/gpu/drm/display/drm_dp_aux_bus.c
+++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c
@@ -3,10 +3,10 @@
* Copyright 2021 Google Inc.
*
* The DP AUX bus is used for devices that are connected over a DisplayPort
- * AUX bus. The devices on the far side of the bus are referred to as
- * endpoints in this code.
+ * AUX bus. The device on the far side of the bus is referred to as an
+ * endpoint in this code.
*
- * Commonly there is only one device connected to the DP AUX bus: a panel.
+ * There is only one device connected to the DP AUX bus: an eDP panel.
* Though historically panels (even DP panels) have been modeled as simple
* platform devices, putting them under the DP AUX bus allows the panel driver
* to perform transactions on that bus.
@@ -22,6 +22,11 @@
#include <drm/display/drm_dp_aux_bus.h>
#include <drm/display/drm_dp_helper.h>
+struct dp_aux_ep_device_with_data {
+ struct dp_aux_ep_device aux_ep;
+ int (*done_probing)(struct drm_dp_aux *aux);
+};
+
/**
* dp_aux_ep_match() - The match function for the dp_aux_bus.
* @dev: The device to match.
@@ -48,6 +53,8 @@ static int dp_aux_ep_probe(struct device *dev)
{
struct dp_aux_ep_driver *aux_ep_drv = to_dp_aux_ep_drv(dev->driver);
struct dp_aux_ep_device *aux_ep = to_dp_aux_ep_dev(dev);
+ struct dp_aux_ep_device_with_data *aux_ep_with_data =
+ container_of(aux_ep, struct dp_aux_ep_device_with_data, aux_ep);
int ret;
ret = dev_pm_domain_attach(dev, true);
@@ -56,7 +63,32 @@ static int dp_aux_ep_probe(struct device *dev)
ret = aux_ep_drv->probe(aux_ep);
if (ret)
- dev_pm_domain_detach(dev, true);
+ goto err_attached;
+
+ if (aux_ep_with_data->done_probing) {
+ ret = aux_ep_with_data->done_probing(aux_ep->aux);
+ if (ret) {
+ /*
+ * The done_probing() callback should not return
+ * -EPROBE_DEFER to us. If it does, we treat it as an
+ * error. Passing it on as-is would cause the _panel_
+ * to defer.
+ */
+ if (ret == -EPROBE_DEFER) {
+ dev_err(dev,
+ "DP AUX done_probing() can't defer\n");
+ ret = -EINVAL;
+ }
+ goto err_probed;
+ }
+ }
+
+ return 0;
+err_probed:
+ if (aux_ep_drv->remove)
+ aux_ep_drv->remove(aux_ep);
+err_attached:
+ dev_pm_domain_detach(dev, true);
return ret;
}
@@ -122,7 +154,11 @@ ATTRIBUTE_GROUPS(dp_aux_ep_dev);
*/
static void dp_aux_ep_dev_release(struct device *dev)
{
- kfree(to_dp_aux_ep_dev(dev));
+ struct dp_aux_ep_device *aux_ep = to_dp_aux_ep_dev(dev);
+ struct dp_aux_ep_device_with_data *aux_ep_with_data =
+ container_of(aux_ep, struct dp_aux_ep_device_with_data, aux_ep);
+
+ kfree(aux_ep_with_data);
}
static struct device_type dp_aux_device_type_type = {
@@ -136,12 +172,14 @@ static struct device_type dp_aux_device_type_type = {
* @dev: The device to destroy.
* @data: Not used
*
- * This is just used as a callback by of_dp_aux_depopulate_ep_devices() and
+ * This is just used as a callback by of_dp_aux_depopulate_bus() and
* is called for _all_ of the child devices of the device providing the AUX bus.
* We'll only act on those that are of type "dp_aux_bus_type".
*
- * This function is effectively an inverse of what's in the loop
- * in of_dp_aux_populate_ep_devices().
+ * This function is effectively an inverse of what's in
+ * of_dp_aux_populate_bus(). NOTE: since we only populate one child
+ * then it's expected that only one device will match all the "if" tests in
+ * this function and get to the device_unregister().
*
* Return: 0 if no error or negative error code.
*/
@@ -164,123 +202,150 @@ static int of_dp_aux_ep_destroy(struct device *dev, void *data)
}
/**
- * of_dp_aux_depopulate_ep_devices() - Undo of_dp_aux_populate_ep_devices
- * @aux: The AUX channel whose devices we want to depopulate
+ * of_dp_aux_depopulate_bus() - Undo of_dp_aux_populate_bus
+ * @aux: The AUX channel whose device we want to depopulate
*
- * This will destroy all devices that were created
- * by of_dp_aux_populate_ep_devices().
+ * This will destroy the device that was created
+ * by of_dp_aux_populate_bus().
*/
-void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux)
+void of_dp_aux_depopulate_bus(struct drm_dp_aux *aux)
{
device_for_each_child_reverse(aux->dev, NULL, of_dp_aux_ep_destroy);
}
-EXPORT_SYMBOL_GPL(of_dp_aux_depopulate_ep_devices);
+EXPORT_SYMBOL_GPL(of_dp_aux_depopulate_bus);
/**
- * of_dp_aux_populate_ep_devices() - Populate the endpoint devices on the DP AUX
- * @aux: The AUX channel whose devices we want to populate. It is required that
+ * of_dp_aux_populate_bus() - Populate the endpoint device on the DP AUX
+ * @aux: The AUX channel whose device we want to populate. It is required that
* drm_dp_aux_init() has already been called for this AUX channel.
+ * @done_probing: Callback functions to call after EP device finishes probing.
+ * Will not be called if there are no EP devices and this
+ * function will return -ENODEV.
*
- * This will populate all the devices under the "aux-bus" node of the device
- * providing the AUX channel (AKA aux->dev).
+ * This will populate the device (expected to be an eDP panel) under the
+ * "aux-bus" node of the device providing the AUX channel (AKA aux->dev).
*
* When this function finishes, it is _possible_ (but not guaranteed) that
- * our sub-devices will have finished probing. It should be noted that if our
- * sub-devices return -EPROBE_DEFER that we will not return any error codes
- * ourselves but our sub-devices will _not_ have actually probed successfully
- * yet. There may be other cases (maybe added in the future?) where sub-devices
- * won't have been probed yet when this function returns, so it's best not to
- * rely on that.
+ * our sub-device will have finished probing. It should be noted that if our
+ * sub-device returns -EPROBE_DEFER or is probing asynchronously for some
+ * reason that we will not return any error codes ourselves but our
+ * sub-device will _not_ have actually probed successfully yet.
+ *
+ * In many cases it's important for the caller of this function to be notified
+ * when our sub device finishes probing. Our sub device is expected to be an
+ * eDP panel and the caller is expected to be an eDP controller. The eDP
+ * controller needs to be able to get a reference to the panel when it finishes
+ * probing. For this reason the caller can pass in a function pointer that
+ * will be called when our sub-device finishes probing.
*
* If this function succeeds you should later make sure you call
- * of_dp_aux_depopulate_ep_devices() to undo it, or just use the devm version
+ * of_dp_aux_depopulate_bus() to undo it, or just use the devm version
* of this function.
*
- * Return: 0 if no error or negative error code.
+ * Return: 0 if no error or negative error code; returns -ENODEV if there are
+ * no children. The done_probing() function won't be called in that
+ * case.
*/
-int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
+int of_dp_aux_populate_bus(struct drm_dp_aux *aux,
+ int (*done_probing)(struct drm_dp_aux *aux))
{
- struct device_node *bus, *np;
+ struct device_node *bus = NULL, *np = NULL;
struct dp_aux_ep_device *aux_ep;
+ struct dp_aux_ep_device_with_data *aux_ep_with_data;
int ret;
/* drm_dp_aux_init() should have been called already; warn if not */
WARN_ON_ONCE(!aux->ddc.algo);
if (!aux->dev->of_node)
- return 0;
-
+ return -ENODEV;
bus = of_get_child_by_name(aux->dev->of_node, "aux-bus");
if (!bus)
- return 0;
+ return -ENODEV;
- for_each_available_child_of_node(bus, np) {
- if (of_node_test_and_set_flag(np, OF_POPULATED))
- continue;
+ np = of_get_next_available_child(bus, NULL);
+ of_node_put(bus);
+ if (!np)
+ return -ENODEV;
- aux_ep = kzalloc(sizeof(*aux_ep), GFP_KERNEL);
- if (!aux_ep)
- continue;
- aux_ep->aux = aux;
+ if (of_node_test_and_set_flag(np, OF_POPULATED)) {
+ dev_err(aux->dev, "DP AUX EP device already populated\n");
+ ret = -EINVAL;
+ goto err_did_get_np;
+ }
- aux_ep->dev.parent = aux->dev;
- aux_ep->dev.bus = &dp_aux_bus_type;
- aux_ep->dev.type = &dp_aux_device_type_type;
- aux_ep->dev.of_node = of_node_get(np);
- dev_set_name(&aux_ep->dev, "aux-%s", dev_name(aux->dev));
+ aux_ep_with_data = kzalloc(sizeof(*aux_ep_with_data), GFP_KERNEL);
+ if (!aux_ep_with_data) {
+ ret = -ENOMEM;
+ goto err_did_set_populated;
+ }
- ret = device_register(&aux_ep->dev);
- if (ret) {
- dev_err(aux->dev, "Failed to create AUX EP for %pOF: %d\n", np, ret);
- of_node_clear_flag(np, OF_POPULATED);
- of_node_put(np);
+ aux_ep_with_data->done_probing = done_probing;
- /*
- * As per docs of device_register(), call this instead
- * of kfree() directly for error cases.
- */
- put_device(&aux_ep->dev);
+ aux_ep = &aux_ep_with_data->aux_ep;
+ aux_ep->aux = aux;
+ aux_ep->dev.parent = aux->dev;
+ aux_ep->dev.bus = &dp_aux_bus_type;
+ aux_ep->dev.type = &dp_aux_device_type_type;
+ aux_ep->dev.of_node = of_node_get(np);
+ dev_set_name(&aux_ep->dev, "aux-%s", dev_name(aux->dev));
- /*
- * Following in the footsteps of of_i2c_register_devices(),
- * we won't fail the whole function here--we'll just
- * continue registering any other devices we find.
- */
- }
- }
+ ret = device_register(&aux_ep->dev);
+ if (ret) {
+ dev_err(aux->dev, "Failed to create AUX EP for %pOF: %d\n", np, ret);
- of_node_put(bus);
+ /*
+ * As per docs of device_register(), call this instead
+ * of kfree() directly for error cases.
+ */
+ put_device(&aux_ep->dev);
+
+ goto err_did_set_populated;
+ }
return 0;
+
+err_did_set_populated:
+ of_node_clear_flag(np, OF_POPULATED);
+
+err_did_get_np:
+ of_node_put(np);
+
+ return ret;
}
-EXPORT_SYMBOL_GPL(of_dp_aux_populate_ep_devices);
+EXPORT_SYMBOL_GPL(of_dp_aux_populate_bus);
-static void of_dp_aux_depopulate_ep_devices_void(void *data)
+static void of_dp_aux_depopulate_bus_void(void *data)
{
- of_dp_aux_depopulate_ep_devices(data);
+ of_dp_aux_depopulate_bus(data);
}
/**
- * devm_of_dp_aux_populate_ep_devices() - devm wrapper for of_dp_aux_populate_ep_devices()
- * @aux: The AUX channel whose devices we want to populate
+ * devm_of_dp_aux_populate_bus() - devm wrapper for of_dp_aux_populate_bus()
+ * @aux: The AUX channel whose device we want to populate
+ * @done_probing: Callback functions to call after EP device finishes probing.
+ * Will not be called if there are no EP devices and this
+ * function will return -ENODEV.
*
* Handles freeing w/ devm on the device "aux->dev".
*
- * Return: 0 if no error or negative error code.
+ * Return: 0 if no error or negative error code; returns -ENODEV if there are
+ * no children. The done_probing() function won't be called in that
+ * case.
*/
-int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
+int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
+ int (*done_probing)(struct drm_dp_aux *aux))
{
int ret;
- ret = of_dp_aux_populate_ep_devices(aux);
+ ret = of_dp_aux_populate_bus(aux, done_probing);
if (ret)
return ret;
return devm_add_action_or_reset(aux->dev,
- of_dp_aux_depopulate_ep_devices_void,
- aux);
+ of_dp_aux_depopulate_bus_void, aux);
}
-EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_ep_devices);
+EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_bus);
int __dp_aux_dp_driver_register(struct dp_aux_ep_driver *drv, struct module *owner)
{
diff --git a/include/drm/display/drm_dp_aux_bus.h b/include/drm/display/drm_dp_aux_bus.h
index 4f19b20b1dd6..8a0a486383c5 100644
--- a/include/drm/display/drm_dp_aux_bus.h
+++ b/include/drm/display/drm_dp_aux_bus.h
@@ -44,9 +44,37 @@ static inline struct dp_aux_ep_driver *to_dp_aux_ep_drv(struct device_driver *dr
return container_of(drv, struct dp_aux_ep_driver, driver);
}
-int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux);
-void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux);
-int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux);
+int of_dp_aux_populate_bus(struct drm_dp_aux *aux,
+ int (*done_probing)(struct drm_dp_aux *aux));
+void of_dp_aux_depopulate_bus(struct drm_dp_aux *aux);
+int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
+ int (*done_probing)(struct drm_dp_aux *aux));
+
+/* Deprecated versions of the above functions. To be removed when no callers. */
+static inline int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
+{
+ int ret;
+
+ ret = of_dp_aux_populate_bus(aux, NULL);
+
+ /* New API returns -ENODEV for no child case; adapt to old assumption */
+ return (ret != -ENODEV) ? ret : 0;
+}
+
+static inline int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
+{
+ int ret;
+
+ ret = devm_of_dp_aux_populate_bus(aux, NULL);
+
+ /* New API returns -ENODEV for no child case; adapt to old assumption */
+ return (ret != -ENODEV) ? ret : 0;
+}
+
+static inline void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux)
+{
+ of_dp_aux_depopulate_bus(aux);
+}
#define dp_aux_dp_driver_register(aux_ep_drv) \
__dp_aux_dp_driver_register(aux_ep_drv, THIS_MODULE)
--
2.36.0.550.gb090851708-goog
Hi,
On Tue, May 10, 2022 at 5:22 PM Dmitry Baryshkov
<[email protected]> wrote:
>
> On Tue, 10 May 2022 at 22:30, Douglas Anderson <[email protected]> wrote:
> >
> > This adds a devm managed version of drm_bridge_add(). Like other
> > "devm" function listed in drm_bridge.h, this function takes an
> > explicit "dev" to use for the lifetime management. A few notes:
> > * In general we have a "struct device" for bridges that makes a good
> > candidate for where the lifetime matches exactly what we want.
> > * The "bridge->dev->dev" device appears to be the encoder
> > device. That's not the right device to use for lifetime management.
> >
> > Suggested-by: Dmitry Baryshkov <[email protected]>
> > Signed-off-by: Douglas Anderson <[email protected]>
>
> Reviewed-by: Dmitry Baryshkov <[email protected]>
Thanks for the review! For now I'll hold off on landing this until
sometime has time to review the other patches in the series. While not
technically required, it seems weird to add the devm function without
any callers.
-Doug
Hi,
On Tue, May 10, 2022 at 12:30 PM Douglas Anderson <[email protected]> wrote:
>
> While working on the DP AUX bus code I found a few small things that
> should be fixed. Namely the non-devm version of
> of_dp_aux_populate_ep_devices() was missing an export. There was also
> an extra blank line in a kerneldoc and a kerneldoc that incorrectly
> documented a return value. Fix these.
>
> Fixes: aeb33699fc2c ("drm: Introduce the DP AUX bus")
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
> None of these seem critical, so my plan is to land this in
> drm-misc-next and not drm-misc-fixes. This will avoid merge conflicts
> with future patches.
>
> Changes in v3:
> - Patch ("drm/dp: Export symbol / kerneldoc fixes...") split for v3.
>
> drivers/gpu/drm/display/drm_dp_aux_bus.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
I landed this one in drm-misc-next:
39c28cdfb719 drm/dp: Export symbol / kerneldoc fixes for DP AUX bus
I chose "drm-misc-next" instead of a "fixes" branch because:
* It's not super urgent.
* I'm still hoping to get review for the other patches in this series
and it would be nice to avoid the conflicts when landing.
-Doug
>
> diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c
> index dccf3e2ea323..552f949cff59 100644
> --- a/drivers/gpu/drm/display/drm_dp_aux_bus.c
> +++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c
> @@ -66,7 +66,6 @@ static int dp_aux_ep_probe(struct device *dev)
> * @dev: The device to remove.
> *
> * Calls through to the endpoint driver remove.
> - *
> */
> static void dp_aux_ep_remove(struct device *dev)
> {
> @@ -120,8 +119,6 @@ ATTRIBUTE_GROUPS(dp_aux_ep_dev);
> /**
> * dp_aux_ep_dev_release() - Free memory for the dp_aux_ep device
> * @dev: The device to free.
> - *
> - * Return: 0 if no error or negative error code.
> */
> static void dp_aux_ep_dev_release(struct device *dev)
> {
> @@ -256,6 +253,7 @@ int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(of_dp_aux_populate_ep_devices);
>
> static void of_dp_aux_depopulate_ep_devices_void(void *data)
> {
> --
> 2.36.0.550.gb090851708-goog
>
Hi,
On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> This adds a devm managed version of drm_bridge_add(). Like other
> "devm" function listed in drm_bridge.h, this function takes an
> explicit "dev" to use for the lifetime management. A few notes:
> * In general we have a "struct device" for bridges that makes a good
> candidate for where the lifetime matches exactly what we want.
> * The "bridge->dev->dev" device appears to be the encoder
> device. That's not the right device to use for lifetime management.
>
> Suggested-by: Dmitry Baryshkov <[email protected]>
> Signed-off-by: Douglas Anderson <[email protected]>
If we are to introduce more managed helpers, I think it'd be wiser to
introduce them as DRM-managed, and not device managed.
Otherwise, you'll end up in a weird state when a device has been removed
but the DRM device is still around.
Maxime
Hi,
On Sat, May 21, 2022 at 2:17 AM Maxime Ripard <[email protected]> wrote:
>
> Hi,
>
> On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > This adds a devm managed version of drm_bridge_add(). Like other
> > "devm" function listed in drm_bridge.h, this function takes an
> > explicit "dev" to use for the lifetime management. A few notes:
> > * In general we have a "struct device" for bridges that makes a good
> > candidate for where the lifetime matches exactly what we want.
> > * The "bridge->dev->dev" device appears to be the encoder
> > device. That's not the right device to use for lifetime management.
> >
> > Suggested-by: Dmitry Baryshkov <[email protected]>
> > Signed-off-by: Douglas Anderson <[email protected]>
>
> If we are to introduce more managed helpers, I think it'd be wiser to
> introduce them as DRM-managed, and not device managed.
>
> Otherwise, you'll end up in a weird state when a device has been removed
> but the DRM device is still around.
I'm kinda confused. In this case there is no DRM device for the bridge
and, as per my CL description, "bridge-dev->dev" appears to be the
encoder device. I wasn't personally involved in discussions about it,
but I was under the impression that this was expected / normal. Thus
we can't make this DRM-managed.
-Doug
Hi,
On Tue, May 10, 2022 at 12:30 PM Douglas Anderson <[email protected]> wrote:
>
> This patch is v3 of the first 2 patches from my RFC series ("drm/dp: Improvements
> for DP AUX channel") [1]. I've broken the series in two so we can make
> progress on the two halves separately.
>
> v2 of this series tries to incorporate all the feedback from v1. Hopefully
> things are less confusing and simpler this time around. The one thing that got
> slightly more confusing is that the done_probing() callback can't return
> -EPROBE_DEFER in most cases so we have to adjust drivers a little more.
>
> v3 takes Dmitry's advice on v2. This now introduces
> devm_drm_bridge_add() (in an extra patch), splits some fixups into
> their own patch, uses a new name for functions, and requires an
> explicit call to done_probing if you have no children.
>
> The idea for this series came up during the review process of
> Sankeerth's series trying to add eDP for Qualcomm SoCs [2].
>
> This _doesn't_ attempt to fix the Analogix driver. If this works out,
> ideally someone can post a patch up to do that.
>
> NOTE: I don't have any ps8640 devices that _don't_ use the aux panel
> underneath them, so I'm relying on code inspection to make sure I
> didn't break those. If someone sees that I did something wrong for
> that case then please yell!
>
> [1] https://lore.kernel.org/r/[email protected]/
> [2] https://lore.kernel.org/r/[email protected]/
>
> Changes in v3:
> - Adapt to v3 changes in aux bus.
> - Don't call done_probing() if there are no children; return -ENODEV.
> - Patch ("drm/bridge: Add devm_drm_bridge_add()") new for v3.
> - Patch ("drm/dp: Export symbol / kerneldoc fixes...") split for v3.
> - Split out EXPORT_SYMBOL and kerneldoc fixes to its own patch.
> - Use devm_drm_bridge_add() to simplify.
> - Used Dmitry's proposed name: of_dp_aux_populate_bus()
>
> Changes in v2:
> - Change to assume exactly one device.
> - Have a probe callback instead of an extra sub device.
> - Rewrote atop new method introduced by patch #1.
>
> Douglas Anderson (4):
> drm/dp: Export symbol / kerneldoc fixes for DP AUX bus
> drm/dp: Add callbacks to make using DP AUX bus properly easier
> drm/bridge: Add devm_drm_bridge_add()
> drm/bridge: parade-ps8640: Handle DP AUX more properly
>
> drivers/gpu/drm/bridge/parade-ps8640.c | 74 +++++---
> drivers/gpu/drm/display/drm_dp_aux_bus.c | 211 +++++++++++++++--------
> drivers/gpu/drm/drm_bridge.c | 23 +++
> include/drm/display/drm_dp_aux_bus.h | 34 +++-
> include/drm/drm_bridge.h | 1 +
> 5 files changed, 238 insertions(+), 105 deletions(-)
I'm hoping to get some review for this series. Anyone? Dmitry: I know
you looked at earlier versions of this series. I guess you're happy
enough with it now but don't feel enough ownership to give a full
Reviewed-by?
-Doug
Maxime,
On Mon, May 23, 2022 at 10:00 AM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Sat, May 21, 2022 at 2:17 AM Maxime Ripard <[email protected]> wrote:
> >
> > Hi,
> >
> > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > This adds a devm managed version of drm_bridge_add(). Like other
> > > "devm" function listed in drm_bridge.h, this function takes an
> > > explicit "dev" to use for the lifetime management. A few notes:
> > > * In general we have a "struct device" for bridges that makes a good
> > > candidate for where the lifetime matches exactly what we want.
> > > * The "bridge->dev->dev" device appears to be the encoder
> > > device. That's not the right device to use for lifetime management.
> > >
> > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > Signed-off-by: Douglas Anderson <[email protected]>
> >
> > If we are to introduce more managed helpers, I think it'd be wiser to
> > introduce them as DRM-managed, and not device managed.
> >
> > Otherwise, you'll end up in a weird state when a device has been removed
> > but the DRM device is still around.
>
> I'm kinda confused. In this case there is no DRM device for the bridge
> and, as per my CL description, "bridge-dev->dev" appears to be the
> encoder device. I wasn't personally involved in discussions about it,
> but I was under the impression that this was expected / normal. Thus
> we can't make this DRM-managed.
Since I didn't hear a reply, I'll assume that my response addressed
your concerns. Assuming I get reviews for the other two patches in
this series I'll plan to land this with Dmitry's review.
-Doug
On 10/05/2022 22:29, Douglas Anderson wrote:
> While working on the DP AUX bus code I found a few small things that
> should be fixed. Namely the non-devm version of
> of_dp_aux_populate_ep_devices() was missing an export. There was also
> an extra blank line in a kerneldoc and a kerneldoc that incorrectly
> documented a return value. Fix these.
>
> Fixes: aeb33699fc2c ("drm: Introduce the DP AUX bus")
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
> ---
> None of these seem critical, so my plan is to land this in
> drm-misc-next and not drm-misc-fixes. This will avoid merge conflicts
> with future patches.
>
> Changes in v3:
> - Patch ("drm/dp: Export symbol / kerneldoc fixes...") split for v3.
>
> drivers/gpu/drm/display/drm_dp_aux_bus.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c
> index dccf3e2ea323..552f949cff59 100644
> --- a/drivers/gpu/drm/display/drm_dp_aux_bus.c
> +++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c
> @@ -66,7 +66,6 @@ static int dp_aux_ep_probe(struct device *dev)
> * @dev: The device to remove.
> *
> * Calls through to the endpoint driver remove.
> - *
> */
> static void dp_aux_ep_remove(struct device *dev)
> {
> @@ -120,8 +119,6 @@ ATTRIBUTE_GROUPS(dp_aux_ep_dev);
> /**
> * dp_aux_ep_dev_release() - Free memory for the dp_aux_ep device
> * @dev: The device to free.
> - *
> - * Return: 0 if no error or negative error code.
> */
> static void dp_aux_ep_dev_release(struct device *dev)
> {
> @@ -256,6 +253,7 @@ int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(of_dp_aux_populate_ep_devices);
>
> static void of_dp_aux_depopulate_ep_devices_void(void *data)
> {
--
With best wishes
Dmitry
On 10/05/2022 22:29, Douglas Anderson wrote:
> As talked about in this patch in the kerneldoc of
> of_dp_aux_populate_ep_device() and also in the past in commit
> a1e3667a9835 ("drm/bridge: ti-sn65dsi86: Promote the AUX channel to
> its own sub-dev"), it can be difficult for eDP controller drivers to
> know when the panel has finished probing when they're using
> of_dp_aux_populate_ep_devices().
>
> The ti-sn65dsi86 driver managed to solve this because it was already
> broken up into a bunch of sub-drivers. That means we could solve the
> problem there by adding a new sub-driver to get the panel. We could
> use the traditional -EPROBE_DEFER retry mechansim to handle the case
> where the panel hadn't probed yet.
>
> In parade-ps8640 we didn't really solve this. The code just expects
> the panel to be ready right away. While reviewing the code originally
> I had managed to convince myself it was fine to just expect the panel
> right away, but additional testing has shown that not to be the
> case. We could fix parade-ps8640 like we did ti-sn65dsi86 but it's
> pretty cumbersome (since we're not already broken into multiple
> drivers) and requires a bunch of boilerplate code.
>
> After discussion [1] it seems like the best solution for most people
> is:
> - Accept that there's always at most one device that will probe as a
> result of the DP AUX bus (it may have sub-devices, but there will be
> one device _directly_ probed).
> - When that device finishes probing, we can just have a call back.
>
> This patch implements that idea. We'll now take a callback as an
> argument to the populate function. To make this easier to land in
> pieces, we'll make wrappers for the old functions. The functions with
> the new name (which make it clear that we only have one child) will
> take the callback and the functions with the old name will temporarily
> wrap.
>
> [1] https://lore.kernel.org/r/CAD=FV=Ur3afHhsXe7a3baWEnD=MFKFeKRbhFU+bt3P67G0MVzQ@mail.gmail.com
>
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
> ---
>
> Changes in v3:
> - Don't call done_probing() if there are no children; return -ENODEV.
> - Split out EXPORT_SYMBOL and kerneldoc fixes to its own patch.
> - Used Dmitry's proposed name: of_dp_aux_populate_bus()
>
> Changes in v2:
> - Change to assume exactly one device.
> - Have a probe callback instead of an extra sub device.
>
> drivers/gpu/drm/display/drm_dp_aux_bus.c | 209 +++++++++++++++--------
> include/drm/display/drm_dp_aux_bus.h | 34 +++-
> 2 files changed, 168 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c
> index 552f949cff59..f5741b45ca07 100644
> --- a/drivers/gpu/drm/display/drm_dp_aux_bus.c
> +++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c
> @@ -3,10 +3,10 @@
> * Copyright 2021 Google Inc.
> *
> * The DP AUX bus is used for devices that are connected over a DisplayPort
> - * AUX bus. The devices on the far side of the bus are referred to as
> - * endpoints in this code.
> + * AUX bus. The device on the far side of the bus is referred to as an
> + * endpoint in this code.
> *
> - * Commonly there is only one device connected to the DP AUX bus: a panel.
> + * There is only one device connected to the DP AUX bus: an eDP panel.
> * Though historically panels (even DP panels) have been modeled as simple
> * platform devices, putting them under the DP AUX bus allows the panel driver
> * to perform transactions on that bus.
> @@ -22,6 +22,11 @@
> #include <drm/display/drm_dp_aux_bus.h>
> #include <drm/display/drm_dp_helper.h>
>
> +struct dp_aux_ep_device_with_data {
> + struct dp_aux_ep_device aux_ep;
> + int (*done_probing)(struct drm_dp_aux *aux);
> +};
> +
> /**
> * dp_aux_ep_match() - The match function for the dp_aux_bus.
> * @dev: The device to match.
> @@ -48,6 +53,8 @@ static int dp_aux_ep_probe(struct device *dev)
> {
> struct dp_aux_ep_driver *aux_ep_drv = to_dp_aux_ep_drv(dev->driver);
> struct dp_aux_ep_device *aux_ep = to_dp_aux_ep_dev(dev);
> + struct dp_aux_ep_device_with_data *aux_ep_with_data =
> + container_of(aux_ep, struct dp_aux_ep_device_with_data, aux_ep);
> int ret;
>
> ret = dev_pm_domain_attach(dev, true);
> @@ -56,7 +63,32 @@ static int dp_aux_ep_probe(struct device *dev)
>
> ret = aux_ep_drv->probe(aux_ep);
> if (ret)
> - dev_pm_domain_detach(dev, true);
> + goto err_attached;
> +
> + if (aux_ep_with_data->done_probing) {
> + ret = aux_ep_with_data->done_probing(aux_ep->aux);
> + if (ret) {
> + /*
> + * The done_probing() callback should not return
> + * -EPROBE_DEFER to us. If it does, we treat it as an
> + * error. Passing it on as-is would cause the _panel_
> + * to defer.
> + */
> + if (ret == -EPROBE_DEFER) {
> + dev_err(dev,
> + "DP AUX done_probing() can't defer\n");
> + ret = -EINVAL;
> + }
> + goto err_probed;
> + }
> + }
> +
> + return 0;
> +err_probed:
> + if (aux_ep_drv->remove)
> + aux_ep_drv->remove(aux_ep);
> +err_attached:
> + dev_pm_domain_detach(dev, true);
>
> return ret;
> }
> @@ -122,7 +154,11 @@ ATTRIBUTE_GROUPS(dp_aux_ep_dev);
> */
> static void dp_aux_ep_dev_release(struct device *dev)
> {
> - kfree(to_dp_aux_ep_dev(dev));
> + struct dp_aux_ep_device *aux_ep = to_dp_aux_ep_dev(dev);
> + struct dp_aux_ep_device_with_data *aux_ep_with_data =
> + container_of(aux_ep, struct dp_aux_ep_device_with_data, aux_ep);
> +
> + kfree(aux_ep_with_data);
> }
>
> static struct device_type dp_aux_device_type_type = {
> @@ -136,12 +172,14 @@ static struct device_type dp_aux_device_type_type = {
> * @dev: The device to destroy.
> * @data: Not used
> *
> - * This is just used as a callback by of_dp_aux_depopulate_ep_devices() and
> + * This is just used as a callback by of_dp_aux_depopulate_bus() and
> * is called for _all_ of the child devices of the device providing the AUX bus.
> * We'll only act on those that are of type "dp_aux_bus_type".
> *
> - * This function is effectively an inverse of what's in the loop
> - * in of_dp_aux_populate_ep_devices().
> + * This function is effectively an inverse of what's in
> + * of_dp_aux_populate_bus(). NOTE: since we only populate one child
> + * then it's expected that only one device will match all the "if" tests in
> + * this function and get to the device_unregister().
> *
> * Return: 0 if no error or negative error code.
> */
> @@ -164,123 +202,150 @@ static int of_dp_aux_ep_destroy(struct device *dev, void *data)
> }
>
> /**
> - * of_dp_aux_depopulate_ep_devices() - Undo of_dp_aux_populate_ep_devices
> - * @aux: The AUX channel whose devices we want to depopulate
> + * of_dp_aux_depopulate_bus() - Undo of_dp_aux_populate_bus
> + * @aux: The AUX channel whose device we want to depopulate
> *
> - * This will destroy all devices that were created
> - * by of_dp_aux_populate_ep_devices().
> + * This will destroy the device that was created
> + * by of_dp_aux_populate_bus().
> */
> -void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux)
> +void of_dp_aux_depopulate_bus(struct drm_dp_aux *aux)
> {
> device_for_each_child_reverse(aux->dev, NULL, of_dp_aux_ep_destroy);
> }
> -EXPORT_SYMBOL_GPL(of_dp_aux_depopulate_ep_devices);
> +EXPORT_SYMBOL_GPL(of_dp_aux_depopulate_bus);
>
> /**
> - * of_dp_aux_populate_ep_devices() - Populate the endpoint devices on the DP AUX
> - * @aux: The AUX channel whose devices we want to populate. It is required that
> + * of_dp_aux_populate_bus() - Populate the endpoint device on the DP AUX
> + * @aux: The AUX channel whose device we want to populate. It is required that
> * drm_dp_aux_init() has already been called for this AUX channel.
> + * @done_probing: Callback functions to call after EP device finishes probing.
> + * Will not be called if there are no EP devices and this
> + * function will return -ENODEV.
> *
> - * This will populate all the devices under the "aux-bus" node of the device
> - * providing the AUX channel (AKA aux->dev).
> + * This will populate the device (expected to be an eDP panel) under the
> + * "aux-bus" node of the device providing the AUX channel (AKA aux->dev).
> *
> * When this function finishes, it is _possible_ (but not guaranteed) that
> - * our sub-devices will have finished probing. It should be noted that if our
> - * sub-devices return -EPROBE_DEFER that we will not return any error codes
> - * ourselves but our sub-devices will _not_ have actually probed successfully
> - * yet. There may be other cases (maybe added in the future?) where sub-devices
> - * won't have been probed yet when this function returns, so it's best not to
> - * rely on that.
> + * our sub-device will have finished probing. It should be noted that if our
> + * sub-device returns -EPROBE_DEFER or is probing asynchronously for some
> + * reason that we will not return any error codes ourselves but our
> + * sub-device will _not_ have actually probed successfully yet.
> + *
> + * In many cases it's important for the caller of this function to be notified
> + * when our sub device finishes probing. Our sub device is expected to be an
> + * eDP panel and the caller is expected to be an eDP controller. The eDP
> + * controller needs to be able to get a reference to the panel when it finishes
> + * probing. For this reason the caller can pass in a function pointer that
> + * will be called when our sub-device finishes probing.
> *
> * If this function succeeds you should later make sure you call
> - * of_dp_aux_depopulate_ep_devices() to undo it, or just use the devm version
> + * of_dp_aux_depopulate_bus() to undo it, or just use the devm version
> * of this function.
> *
> - * Return: 0 if no error or negative error code.
> + * Return: 0 if no error or negative error code; returns -ENODEV if there are
> + * no children. The done_probing() function won't be called in that
> + * case.
> */
> -int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
> +int of_dp_aux_populate_bus(struct drm_dp_aux *aux,
> + int (*done_probing)(struct drm_dp_aux *aux))
> {
> - struct device_node *bus, *np;
> + struct device_node *bus = NULL, *np = NULL;
> struct dp_aux_ep_device *aux_ep;
> + struct dp_aux_ep_device_with_data *aux_ep_with_data;
> int ret;
>
> /* drm_dp_aux_init() should have been called already; warn if not */
> WARN_ON_ONCE(!aux->ddc.algo);
>
> if (!aux->dev->of_node)
> - return 0;
> -
> + return -ENODEV;
> bus = of_get_child_by_name(aux->dev->of_node, "aux-bus");
> if (!bus)
> - return 0;
> + return -ENODEV;
>
> - for_each_available_child_of_node(bus, np) {
> - if (of_node_test_and_set_flag(np, OF_POPULATED))
> - continue;
> + np = of_get_next_available_child(bus, NULL);
> + of_node_put(bus);
> + if (!np)
> + return -ENODEV;
>
> - aux_ep = kzalloc(sizeof(*aux_ep), GFP_KERNEL);
> - if (!aux_ep)
> - continue;
> - aux_ep->aux = aux;
> + if (of_node_test_and_set_flag(np, OF_POPULATED)) {
> + dev_err(aux->dev, "DP AUX EP device already populated\n");
> + ret = -EINVAL;
> + goto err_did_get_np;
> + }
>
> - aux_ep->dev.parent = aux->dev;
> - aux_ep->dev.bus = &dp_aux_bus_type;
> - aux_ep->dev.type = &dp_aux_device_type_type;
> - aux_ep->dev.of_node = of_node_get(np);
> - dev_set_name(&aux_ep->dev, "aux-%s", dev_name(aux->dev));
> + aux_ep_with_data = kzalloc(sizeof(*aux_ep_with_data), GFP_KERNEL);
> + if (!aux_ep_with_data) {
> + ret = -ENOMEM;
> + goto err_did_set_populated;
> + }
>
> - ret = device_register(&aux_ep->dev);
> - if (ret) {
> - dev_err(aux->dev, "Failed to create AUX EP for %pOF: %d\n", np, ret);
> - of_node_clear_flag(np, OF_POPULATED);
> - of_node_put(np);
> + aux_ep_with_data->done_probing = done_probing;
>
> - /*
> - * As per docs of device_register(), call this instead
> - * of kfree() directly for error cases.
> - */
> - put_device(&aux_ep->dev);
> + aux_ep = &aux_ep_with_data->aux_ep;
> + aux_ep->aux = aux;
> + aux_ep->dev.parent = aux->dev;
> + aux_ep->dev.bus = &dp_aux_bus_type;
> + aux_ep->dev.type = &dp_aux_device_type_type;
> + aux_ep->dev.of_node = of_node_get(np);
> + dev_set_name(&aux_ep->dev, "aux-%s", dev_name(aux->dev));
>
> - /*
> - * Following in the footsteps of of_i2c_register_devices(),
> - * we won't fail the whole function here--we'll just
> - * continue registering any other devices we find.
> - */
> - }
> - }
> + ret = device_register(&aux_ep->dev);
> + if (ret) {
> + dev_err(aux->dev, "Failed to create AUX EP for %pOF: %d\n", np, ret);
>
> - of_node_put(bus);
> + /*
> + * As per docs of device_register(), call this instead
> + * of kfree() directly for error cases.
> + */
> + put_device(&aux_ep->dev);
> +
> + goto err_did_set_populated;
> + }
>
> return 0;
> +
> +err_did_set_populated:
> + of_node_clear_flag(np, OF_POPULATED);
> +
> +err_did_get_np:
> + of_node_put(np);
> +
> + return ret;
> }
> -EXPORT_SYMBOL_GPL(of_dp_aux_populate_ep_devices);
> +EXPORT_SYMBOL_GPL(of_dp_aux_populate_bus);
>
> -static void of_dp_aux_depopulate_ep_devices_void(void *data)
> +static void of_dp_aux_depopulate_bus_void(void *data)
> {
> - of_dp_aux_depopulate_ep_devices(data);
> + of_dp_aux_depopulate_bus(data);
> }
>
> /**
> - * devm_of_dp_aux_populate_ep_devices() - devm wrapper for of_dp_aux_populate_ep_devices()
> - * @aux: The AUX channel whose devices we want to populate
> + * devm_of_dp_aux_populate_bus() - devm wrapper for of_dp_aux_populate_bus()
> + * @aux: The AUX channel whose device we want to populate
> + * @done_probing: Callback functions to call after EP device finishes probing.
> + * Will not be called if there are no EP devices and this
> + * function will return -ENODEV.
> *
> * Handles freeing w/ devm on the device "aux->dev".
> *
> - * Return: 0 if no error or negative error code.
> + * Return: 0 if no error or negative error code; returns -ENODEV if there are
> + * no children. The done_probing() function won't be called in that
> + * case.
> */
> -int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
> +int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
> + int (*done_probing)(struct drm_dp_aux *aux))
> {
> int ret;
>
> - ret = of_dp_aux_populate_ep_devices(aux);
> + ret = of_dp_aux_populate_bus(aux, done_probing);
> if (ret)
> return ret;
>
> return devm_add_action_or_reset(aux->dev,
> - of_dp_aux_depopulate_ep_devices_void,
> - aux);
> + of_dp_aux_depopulate_bus_void, aux);
> }
> -EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_ep_devices);
> +EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_bus);
>
> int __dp_aux_dp_driver_register(struct dp_aux_ep_driver *drv, struct module *owner)
> {
> diff --git a/include/drm/display/drm_dp_aux_bus.h b/include/drm/display/drm_dp_aux_bus.h
> index 4f19b20b1dd6..8a0a486383c5 100644
> --- a/include/drm/display/drm_dp_aux_bus.h
> +++ b/include/drm/display/drm_dp_aux_bus.h
> @@ -44,9 +44,37 @@ static inline struct dp_aux_ep_driver *to_dp_aux_ep_drv(struct device_driver *dr
> return container_of(drv, struct dp_aux_ep_driver, driver);
> }
>
> -int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux);
> -void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux);
> -int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux);
> +int of_dp_aux_populate_bus(struct drm_dp_aux *aux,
> + int (*done_probing)(struct drm_dp_aux *aux));
> +void of_dp_aux_depopulate_bus(struct drm_dp_aux *aux);
> +int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
> + int (*done_probing)(struct drm_dp_aux *aux));
> +
> +/* Deprecated versions of the above functions. To be removed when no callers. */
> +static inline int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
> +{
> + int ret;
> +
> + ret = of_dp_aux_populate_bus(aux, NULL);
> +
> + /* New API returns -ENODEV for no child case; adapt to old assumption */
> + return (ret != -ENODEV) ? ret : 0;
> +}
> +
> +static inline int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
> +{
> + int ret;
> +
> + ret = devm_of_dp_aux_populate_bus(aux, NULL);
> +
> + /* New API returns -ENODEV for no child case; adapt to old assumption */
> + return (ret != -ENODEV) ? ret : 0;
> +}
> +
> +static inline void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux)
> +{
> + of_dp_aux_depopulate_bus(aux);
> +}
>
> #define dp_aux_dp_driver_register(aux_ep_drv) \
> __dp_aux_dp_driver_register(aux_ep_drv, THIS_MODULE)
--
With best wishes
Dmitry
On 10/05/2022 22:29, Douglas Anderson wrote:
> While it works, for the most part, to assume that the panel has
> finished probing when devm_of_dp_aux_populate_ep_devices() returns,
> it's a bit fragile. This is talked about at length in commit
> a1e3667a9835 ("drm/bridge: ti-sn65dsi86: Promote the AUX channel to
> its own sub-dev").
>
> When reviewing the ps8640 code, I managed to convince myself that it
> was OK not to worry about it there and that maybe it wasn't really
> _that_ fragile. However, it turns out that it really is. Simply
> hardcoding panel_edp_probe() to return -EPROBE_DEFER was enough to put
> the boot process into an infinite loop. I believe this manages to trip
> the same issues that we used to trip with the main MSM code where
> something about our actions trigger Linux to re-probe previously
> deferred devices right away and each time we try again we re-trigger
> Linux to re-probe.
>
> Let's fix this using the callback introduced in the patch ("drm/dp:
> Callbacks to make it easier for drivers to use DP AUX bus properly").
> When using the new callback, we have to be a little careful. The
> probe_done() callback is no longer always called in the context of
> our probe routine. That means we can't rely on being able to return
> -EPROBE_DEFER from it. We re-jigger the order of things a bit to
> account for that.
>
> With this change, the device still boots (though obviously the panel
> doesn't come up) if I force panel-edp to always return
> -EPROBE_DEFER. If I fake it and make the panel probe exactly once it
> also works.
>
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
> ---
>
> Changes in v3:
> - Adapt to v3 changes in aux bus.
> - Use devm_drm_bridge_add() to simplify.
>
> Changes in v2:
> - Rewrote atop new method introduced by patch #1.
>
> drivers/gpu/drm/bridge/parade-ps8640.c | 74 ++++++++++++++++----------
> 1 file changed, 46 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index e2467e58b5b7..ff4227f6d800 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -537,7 +537,7 @@ 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)
> +static int ps8640_bridge_get_dsi_resources(struct device *dev, struct ps8640 *ps_bridge)
> {
> struct device_node *in_ep, *dsi_node;
> struct mipi_dsi_device *dsi;
> @@ -576,13 +576,40 @@ static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridg
> dsi->format = MIPI_DSI_FMT_RGB888;
> dsi->lanes = NUM_MIPI_LANES;
>
> - return devm_mipi_dsi_attach(dev, dsi);
> + return 0;
> +}
> +
> +static int ps8640_bridge_link_panel(struct drm_dp_aux *aux)
> +{
> + struct ps8640 *ps_bridge = aux_to_ps8640(aux);
> + struct device *dev = aux->dev;
> + struct device_node *np = dev->of_node;
> + int ret;
> +
> + /*
> + * NOTE about returning -EPROBE_DEFER from this function: if we
> + * return an error (most relevant to -EPROBE_DEFER) it will only
> + * be passed out to ps8640_probe() if it called this directly (AKA the
> + * panel isn't under the "aux-bus" node). That should be fine because
> + * if the panel is under "aux-bus" it's guaranteed to have probed by
> + * the time this function has been called.
> + */
> +
> + /* port@1 is ps8640 output port */
> + ps_bridge->panel_bridge = devm_drm_of_get_bridge(dev, np, 1, 0);
> + if (IS_ERR(ps_bridge->panel_bridge))
> + return PTR_ERR(ps_bridge->panel_bridge);
> +
> + ret = devm_drm_bridge_add(dev, &ps_bridge->bridge);
> + if (ret)
> + return ret;
> +
> + return devm_mipi_dsi_attach(dev, ps_bridge->dsi);
> }
>
> static int ps8640_probe(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
> - struct device_node *np = dev->of_node;
> struct ps8640 *ps_bridge;
> int ret;
> u32 i;
> @@ -623,6 +650,14 @@ static int ps8640_probe(struct i2c_client *client)
> if (!ps8640_of_panel_on_aux_bus(&client->dev))
> ps_bridge->bridge.ops = DRM_BRIDGE_OP_EDID;
>
> + /*
> + * Get MIPI DSI resources early. These can return -EPROBE_DEFER so
> + * we want to get them out of the way sooner.
> + */
> + ret = ps8640_bridge_get_dsi_resources(&client->dev, ps_bridge);
> + if (ret)
> + return ret;
> +
> ps_bridge->page[PAGE0_DP_CNTL] = client;
>
> ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config);
> @@ -665,35 +700,19 @@ static int ps8640_probe(struct i2c_client *client)
> if (ret)
> return ret;
>
> - devm_of_dp_aux_populate_ep_devices(&ps_bridge->aux);
> + ret = devm_of_dp_aux_populate_bus(&ps_bridge->aux, ps8640_bridge_link_panel);
>
> - /* port@1 is ps8640 output port */
> - ps_bridge->panel_bridge = devm_drm_of_get_bridge(dev, np, 1, 0);
> - if (IS_ERR(ps_bridge->panel_bridge))
> - return PTR_ERR(ps_bridge->panel_bridge);
> -
> - drm_bridge_add(&ps_bridge->bridge);
> -
> - ret = ps8640_bridge_host_attach(dev, ps_bridge);
> - if (ret)
> - goto err_bridge_remove;
> -
> - return 0;
> + /*
> + * If devm_of_dp_aux_populate_bus() returns -ENODEV then it's up to
> + * usa to call ps8640_bridge_link_panel() directly. NOTE: in this case
> + * the function is allowed to -EPROBE_DEFER.
> + */
> + if (ret == -ENODEV)
> + return ps8640_bridge_link_panel(&ps_bridge->aux);
>
> -err_bridge_remove:
> - drm_bridge_remove(&ps_bridge->bridge);
> return ret;
> }
>
> -static int ps8640_remove(struct i2c_client *client)
> -{
> - struct ps8640 *ps_bridge = i2c_get_clientdata(client);
> -
> - drm_bridge_remove(&ps_bridge->bridge);
> -
> - return 0;
> -}
> -
> static const struct of_device_id ps8640_match[] = {
> { .compatible = "parade,ps8640" },
> { }
> @@ -702,7 +721,6 @@ MODULE_DEVICE_TABLE(of, ps8640_match);
>
> static struct i2c_driver ps8640_driver = {
> .probe_new = ps8640_probe,
> - .remove = ps8640_remove,
> .driver = {
> .name = "ps8640",
> .of_match_table = ps8640_match,
--
With best wishes
Dmitry
On Fri, Jun 03, 2022 at 06:52:05AM -0700, Doug Anderson wrote:
> On Fri, Jun 3, 2022 at 3:19 AM Dmitry Baryshkov
> <[email protected]> wrote:
> > On Fri, 3 Jun 2022 at 11:21, Maxime Ripard <[email protected]> wrote:
> > >
> > > On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> > > > On Mon, May 23, 2022 at 10:00 AM Doug Anderson <[email protected]> wrote:
> > > > > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard <[email protected]> wrote:
> > > > > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > > > candidate for where the lifetime matches exactly what we want.
> > > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > > > device. That's not the right device to use for lifetime management.
> > > > > > >
> > > > > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > > > >
> > > > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > > > introduce them as DRM-managed, and not device managed.
> > > > > >
> > > > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > > > but the DRM device is still around.
> > > > >
> > > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > > encoder device. I wasn't personally involved in discussions about it,
> > > > > but I was under the impression that this was expected / normal. Thus
> > > > > we can't make this DRM-managed.
> > > >
> > > > Since I didn't hear a reply,
> > >
> > > Gah, I replied but it looks like somehow it never reached the ML...
> > >
> > > Here was my original reply:
> > >
> > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > > candidate for where the lifetime matches exactly what we want.
> > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > > device. That's not the right device to use for lifetime management.
> > > > > >
> > > > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > > >
> > > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > > introduce them as DRM-managed, and not device managed.
> > > > >
> > > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > > but the DRM device is still around.
> > > >=20
> > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > encoder device.
> > >
> > > bridge->dev seems right though?
> > >
> > > > I wasn't personally involved in discussions about it, but I was under
> > > > the impression that this was expected / normal. Thus we can't make
> > > > this DRM-managed.
> > >
> > > Still, I don't think devm is the right solution to this either.
> > >
> > > The underlying issue is two-fold:
> > >
> > > - Encoders can have a pointer to a bridge through of_drm_find_bridge
> > > or similar. However, bridges are traditionally tied to their device
> > > lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
> > > in remove). Encoders will typically be tied to the DRM device
> > > however, and that one sticks around until the last application
> > > closes it. We can thus very easily end up with a dangling pointer,
> > > and a use-after-free.
> > >
> > > - It's not the case yet, but it doesn't seem far fetch to expose
> > > properties of bridges to the userspace. In that case, the userspace
> > > would be likely to still hold references to objects that aren't
> > > there anymore when the bridge is gone.
> > >
> > > The first is obviously a larger concern, but if we can find a solution
> > > that would accomodate the second it would be great.
> > >
> > > As far as I can see, we should fix in two steps:
> > >
> > > - in drm_bridge_attach, we should add a device-managed call that will
> > > unregister the main DRM device. We don't allow to probe the main DRM
> > > device when the bridge isn't there yet in most case, so it makes
> > > sense to remove it once the bridge is no longer there as well.
> >
> > The problem is that I do not see a good way to unregister the main DRM
> > device outside of it's driver code.
> >
> > >
> > > - When the DRM device is removed, have the core cleanup any bridge
> > > registered. That will remove the need to have drm_bridge_remove in
> > > the first place.
> > >
> > > > I'll assume that my response addressed your concerns. Assuming I get
> > > > reviews for the other two patches in this series I'll plan to land
> > > > this with Dmitry's review.
> > >
> > > I still don't think it's a good idea to merge it. It gives an illusion
> > > of being safe, but it's really far from it.
> >
> > It is more of removing the boilerplate code spread over all the
> > drivers rather than about particular safety.
> >
> > I'd propose to land devm_drm_bridge_add (and deprecate calling
> > drm_bridge_remove from the bridge driver at some point) and work on
> > the whole drm_device <-> drm_bridge problem in the meantime.
>
> At this point it has been landed in drm-misc-next as per my response
> to the cover letter. If need be we can revert it and rework the ps8640
> driver to stop using it but it wouldn't change the lifetime of the
> bridge. I'm not going to rework the bridge lifetime rules here. If
> nothing else it seems like having the devm function at least would
> make it obvious which drivers need to be fixed whenever the bridge
> lifetime problem gets solved.
Not really, no. The issue exists whether or not the driver would be
using devm. Anyway, what's done is done.
Could you please ping earlier than a few minutes before applying the
patch next time though? We could have easily prevented that situation.
Maxime
Hi,
On Fri, Jun 3, 2022 at 7:14 AM Maxime Ripard <[email protected]> wrote:
>
> On Fri, Jun 03, 2022 at 01:19:16PM +0300, Dmitry Baryshkov wrote:
> > On Fri, 3 Jun 2022 at 11:21, Maxime Ripard <[email protected]> wrote:
> > >
> > > On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> > > > On Mon, May 23, 2022 at 10:00 AM Doug Anderson <[email protected]> wrote:
> > > > > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard <[email protected]> wrote:
> > > > > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > > > candidate for where the lifetime matches exactly what we want.
> > > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > > > device. That's not the right device to use for lifetime management.
> > > > > > >
> > > > > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > > > >
> > > > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > > > introduce them as DRM-managed, and not device managed.
> > > > > >
> > > > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > > > but the DRM device is still around.
> > > > >
> > > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > > encoder device. I wasn't personally involved in discussions about it,
> > > > > but I was under the impression that this was expected / normal. Thus
> > > > > we can't make this DRM-managed.
> > > >
> > > > Since I didn't hear a reply,
> > >
> > > Gah, I replied but it looks like somehow it never reached the ML...
> > >
> > > Here was my original reply:
> > >
> > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > > candidate for where the lifetime matches exactly what we want.
> > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > > device. That's not the right device to use for lifetime management.
> > > > > >
> > > > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > > >
> > > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > > introduce them as DRM-managed, and not device managed.
> > > > >
> > > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > > but the DRM device is still around.
> > > >=20
> > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > encoder device.
> > >
> > > bridge->dev seems right though?
> > >
> > > > I wasn't personally involved in discussions about it, but I was under
> > > > the impression that this was expected / normal. Thus we can't make
> > > > this DRM-managed.
> > >
> > > Still, I don't think devm is the right solution to this either.
> > >
> > > The underlying issue is two-fold:
> > >
> > > - Encoders can have a pointer to a bridge through of_drm_find_bridge
> > > or similar. However, bridges are traditionally tied to their device
> > > lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
> > > in remove). Encoders will typically be tied to the DRM device
> > > however, and that one sticks around until the last application
> > > closes it. We can thus very easily end up with a dangling pointer,
> > > and a use-after-free.
> > >
> > > - It's not the case yet, but it doesn't seem far fetch to expose
> > > properties of bridges to the userspace. In that case, the userspace
> > > would be likely to still hold references to objects that aren't
> > > there anymore when the bridge is gone.
> > >
> > > The first is obviously a larger concern, but if we can find a solution
> > > that would accomodate the second it would be great.
> > >
> > > As far as I can see, we should fix in two steps:
> > >
> > > - in drm_bridge_attach, we should add a device-managed call that will
> > > unregister the main DRM device. We don't allow to probe the main DRM
> > > device when the bridge isn't there yet in most case, so it makes
> > > sense to remove it once the bridge is no longer there as well.
> >
> > The problem is that I do not see a good way to unregister the main DRM
> > device outside of it's driver code.
>
> That's what drmm helpers are doing though: they'll defer the cleanup
> until the last user has closed its fd.
I'm a bit confused here. I'll take the concrete example of ps8640
since that's what I was working on here.
...right now the fact that we're using devm means that
drm_bridge_remove() will get called when a ps8640 device is unbound,
right? I guess you're saying that the "drm_bridge" memory needs to
outlast this, right? That being said, even if the actual memory for
drm_bridge outlasts the ps8640 driver lifetime, much of the data would
need to be marked invalid I think. If nothing else all function
pointers that point into the driver would have to be made NULL, right?
Once the device has been unbound it's possible that the underlying
module might be removed. I suspect that we'd need to do more than just
bogus-up the function pointers, though.
...so it feels like any solution here needs to take into account
_both_ the lifetime of the "struct device" and the "struct
drm_device". If the "struct device" goes away but the "struct
drm_device" is still around then we need to essentially transition the
"struct drm_device" over to a dummy, right? In my perhaps naive view
that means that a dmm_bridge_add() wouldn't be enough because it
wouldn't know when the "struct device" went away.
> > > - When the DRM device is removed, have the core cleanup any bridge
> > > registered. That will remove the need to have drm_bridge_remove in
> > > the first place.
> > >
> > > > I'll assume that my response addressed your concerns. Assuming I get
> > > > reviews for the other two patches in this series I'll plan to land
> > > > this with Dmitry's review.
> > >
> > > I still don't think it's a good idea to merge it. It gives an illusion
> > > of being safe, but it's really far from it.
> >
> > It is more of removing the boilerplate code spread over all the
> > drivers rather than about particular safety.
> >
> > I'd propose to land devm_drm_bridge_add (and deprecate calling
> > drm_bridge_remove from the bridge driver at some point) and work on
> > the whole drm_device <-> drm_bridge problem in the meantime.
>
> Do you really expect that to happen? :)
>
> Anyway, it's been merged, it's too late now anyway. I don't really feel
> like it's a good thing, but it doesn't really make the situation worse
> either.
A revert is really not that hard to do if the consensus is that we
really don't want this.
-Doug
On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> On Mon, May 23, 2022 at 10:00 AM Doug Anderson <[email protected]> wrote:
> > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard <[email protected]> wrote:
> > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > * In general we have a "struct device" for bridges that makes a good
> > > > candidate for where the lifetime matches exactly what we want.
> > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > device. That's not the right device to use for lifetime management.
> > > >
> > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > Signed-off-by: Douglas Anderson <[email protected]>
> > >
> > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > introduce them as DRM-managed, and not device managed.
> > >
> > > Otherwise, you'll end up in a weird state when a device has been removed
> > > but the DRM device is still around.
> >
> > I'm kinda confused. In this case there is no DRM device for the bridge
> > and, as per my CL description, "bridge-dev->dev" appears to be the
> > encoder device. I wasn't personally involved in discussions about it,
> > but I was under the impression that this was expected / normal. Thus
> > we can't make this DRM-managed.
>
> Since I didn't hear a reply,
Gah, I replied but it looks like somehow it never reached the ML...
Here was my original reply:
> > > This adds a devm managed version of drm_bridge_add(). Like other
> > > "devm" function listed in drm_bridge.h, this function takes an
> > > explicit "dev" to use for the lifetime management. A few notes:
> > > * In general we have a "struct device" for bridges that makes a good
> > > candidate for where the lifetime matches exactly what we want.
> > > * The "bridge->dev->dev" device appears to be the encoder
> > > device. That's not the right device to use for lifetime management.
> > >
> > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > Signed-off-by: Douglas Anderson <[email protected]>
> >
> > If we are to introduce more managed helpers, I think it'd be wiser to
> > introduce them as DRM-managed, and not device managed.
> >
> > Otherwise, you'll end up in a weird state when a device has been removed
> > but the DRM device is still around.
>=20
> I'm kinda confused. In this case there is no DRM device for the bridge
> and, as per my CL description, "bridge-dev->dev" appears to be the
> encoder device.
bridge->dev seems right though?
> I wasn't personally involved in discussions about it, but I was under
> the impression that this was expected / normal. Thus we can't make
> this DRM-managed.
Still, I don't think devm is the right solution to this either.
The underlying issue is two-fold:
- Encoders can have a pointer to a bridge through of_drm_find_bridge
or similar. However, bridges are traditionally tied to their device
lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
in remove). Encoders will typically be tied to the DRM device
however, and that one sticks around until the last application
closes it. We can thus very easily end up with a dangling pointer,
and a use-after-free.
- It's not the case yet, but it doesn't seem far fetch to expose
properties of bridges to the userspace. In that case, the userspace
would be likely to still hold references to objects that aren't
there anymore when the bridge is gone.
The first is obviously a larger concern, but if we can find a solution
that would accomodate the second it would be great.
As far as I can see, we should fix in two steps:
- in drm_bridge_attach, we should add a device-managed call that will
unregister the main DRM device. We don't allow to probe the main DRM
device when the bridge isn't there yet in most case, so it makes
sense to remove it once the bridge is no longer there as well.
- When the DRM device is removed, have the core cleanup any bridge
registered. That will remove the need to have drm_bridge_remove in
the first place.
> I'll assume that my response addressed your concerns. Assuming I get
> reviews for the other two patches in this series I'll plan to land
> this with Dmitry's review.
I still don't think it's a good idea to merge it. It gives an illusion
of being safe, but it's really far from it.
Maxime
Hi,
On Fri, Jun 3, 2022 at 3:19 AM Dmitry Baryshkov
<[email protected]> wrote:
>
> On Fri, 3 Jun 2022 at 11:21, Maxime Ripard <[email protected]> wrote:
> >
> > On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> > > On Mon, May 23, 2022 at 10:00 AM Doug Anderson <[email protected]> wrote:
> > > > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard <[email protected]> wrote:
> > > > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > > candidate for where the lifetime matches exactly what we want.
> > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > > device. That's not the right device to use for lifetime management.
> > > > > >
> > > > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > > >
> > > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > > introduce them as DRM-managed, and not device managed.
> > > > >
> > > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > > but the DRM device is still around.
> > > >
> > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > encoder device. I wasn't personally involved in discussions about it,
> > > > but I was under the impression that this was expected / normal. Thus
> > > > we can't make this DRM-managed.
> > >
> > > Since I didn't hear a reply,
> >
> > Gah, I replied but it looks like somehow it never reached the ML...
> >
> > Here was my original reply:
> >
> > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > candidate for where the lifetime matches exactly what we want.
> > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > device. That's not the right device to use for lifetime management.
> > > > >
> > > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > >
> > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > introduce them as DRM-managed, and not device managed.
> > > >
> > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > but the DRM device is still around.
> > >=20
> > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > encoder device.
> >
> > bridge->dev seems right though?
> >
> > > I wasn't personally involved in discussions about it, but I was under
> > > the impression that this was expected / normal. Thus we can't make
> > > this DRM-managed.
> >
> > Still, I don't think devm is the right solution to this either.
> >
> > The underlying issue is two-fold:
> >
> > - Encoders can have a pointer to a bridge through of_drm_find_bridge
> > or similar. However, bridges are traditionally tied to their device
> > lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
> > in remove). Encoders will typically be tied to the DRM device
> > however, and that one sticks around until the last application
> > closes it. We can thus very easily end up with a dangling pointer,
> > and a use-after-free.
> >
> > - It's not the case yet, but it doesn't seem far fetch to expose
> > properties of bridges to the userspace. In that case, the userspace
> > would be likely to still hold references to objects that aren't
> > there anymore when the bridge is gone.
> >
> > The first is obviously a larger concern, but if we can find a solution
> > that would accomodate the second it would be great.
> >
> > As far as I can see, we should fix in two steps:
> >
> > - in drm_bridge_attach, we should add a device-managed call that will
> > unregister the main DRM device. We don't allow to probe the main DRM
> > device when the bridge isn't there yet in most case, so it makes
> > sense to remove it once the bridge is no longer there as well.
>
> The problem is that I do not see a good way to unregister the main DRM
> device outside of it's driver code.
>
> >
> > - When the DRM device is removed, have the core cleanup any bridge
> > registered. That will remove the need to have drm_bridge_remove in
> > the first place.
> >
> > > I'll assume that my response addressed your concerns. Assuming I get
> > > reviews for the other two patches in this series I'll plan to land
> > > this with Dmitry's review.
> >
> > I still don't think it's a good idea to merge it. It gives an illusion
> > of being safe, but it's really far from it.
>
> It is more of removing the boilerplate code spread over all the
> drivers rather than about particular safety.
>
> I'd propose to land devm_drm_bridge_add (and deprecate calling
> drm_bridge_remove from the bridge driver at some point) and work on
> the whole drm_device <-> drm_bridge problem in the meantime.
At this point it has been landed in drm-misc-next as per my response
to the cover letter. If need be we can revert it and rework the ps8640
driver to stop using it but it wouldn't change the lifetime of the
bridge. I'm not going to rework the bridge lifetime rules here. If
nothing else it seems like having the devm function at least would
make it obvious which drivers need to be fixed whenever the bridge
lifetime problem gets solved.
-Doug
Hi,
On Tue, May 10, 2022 at 12:30 PM Douglas Anderson <[email protected]> wrote:
>
> This patch is v3 of the first 2 patches from my RFC series ("drm/dp: Improvements
> for DP AUX channel") [1]. I've broken the series in two so we can make
> progress on the two halves separately.
>
> v2 of this series tries to incorporate all the feedback from v1. Hopefully
> things are less confusing and simpler this time around. The one thing that got
> slightly more confusing is that the done_probing() callback can't return
> -EPROBE_DEFER in most cases so we have to adjust drivers a little more.
>
> v3 takes Dmitry's advice on v2. This now introduces
> devm_drm_bridge_add() (in an extra patch), splits some fixups into
> their own patch, uses a new name for functions, and requires an
> explicit call to done_probing if you have no children.
>
> The idea for this series came up during the review process of
> Sankeerth's series trying to add eDP for Qualcomm SoCs [2].
>
> This _doesn't_ attempt to fix the Analogix driver. If this works out,
> ideally someone can post a patch up to do that.
>
> NOTE: I don't have any ps8640 devices that _don't_ use the aux panel
> underneath them, so I'm relying on code inspection to make sure I
> didn't break those. If someone sees that I did something wrong for
> that case then please yell!
>
> [1] https://lore.kernel.org/r/[email protected]/
> [2] https://lore.kernel.org/r/[email protected]/
>
> Changes in v3:
> - Adapt to v3 changes in aux bus.
> - Don't call done_probing() if there are no children; return -ENODEV.
> - Patch ("drm/bridge: Add devm_drm_bridge_add()") new for v3.
> - Patch ("drm/dp: Export symbol / kerneldoc fixes...") split for v3.
> - Split out EXPORT_SYMBOL and kerneldoc fixes to its own patch.
> - Use devm_drm_bridge_add() to simplify.
> - Used Dmitry's proposed name: of_dp_aux_populate_bus()
>
> Changes in v2:
> - Change to assume exactly one device.
> - Have a probe callback instead of an extra sub device.
> - Rewrote atop new method introduced by patch #1.
>
> Douglas Anderson (4):
> drm/dp: Export symbol / kerneldoc fixes for DP AUX bus
> drm/dp: Add callbacks to make using DP AUX bus properly easier
> drm/bridge: Add devm_drm_bridge_add()
> drm/bridge: parade-ps8640: Handle DP AUX more properly
>
> drivers/gpu/drm/bridge/parade-ps8640.c | 74 +++++---
> drivers/gpu/drm/display/drm_dp_aux_bus.c | 211 +++++++++++++++--------
> drivers/gpu/drm/drm_bridge.c | 23 +++
> include/drm/display/drm_dp_aux_bus.h | 34 +++-
> include/drm/drm_bridge.h | 1 +
> 5 files changed, 238 insertions(+), 105 deletions(-)
I'd previously pushed patch #1. Now I've pushed the rest of the
patches to drm-misc-next with Dmitry's review:
10e619f1f31c drm/bridge: parade-ps8640: Handle DP AUX more properly
50e156bd8a9d drm/bridge: Add devm_drm_bridge_add()
3800b1710946 drm/dp: Add callbacks to make using DP AUX bus properly easier
-Doug
On Fri, Jun 03, 2022 at 01:19:16PM +0300, Dmitry Baryshkov wrote:
> On Fri, 3 Jun 2022 at 11:21, Maxime Ripard <[email protected]> wrote:
> >
> > On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> > > On Mon, May 23, 2022 at 10:00 AM Doug Anderson <[email protected]> wrote:
> > > > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard <[email protected]> wrote:
> > > > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > > candidate for where the lifetime matches exactly what we want.
> > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > > device. That's not the right device to use for lifetime management.
> > > > > >
> > > > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > > >
> > > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > > introduce them as DRM-managed, and not device managed.
> > > > >
> > > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > > but the DRM device is still around.
> > > >
> > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > encoder device. I wasn't personally involved in discussions about it,
> > > > but I was under the impression that this was expected / normal. Thus
> > > > we can't make this DRM-managed.
> > >
> > > Since I didn't hear a reply,
> >
> > Gah, I replied but it looks like somehow it never reached the ML...
> >
> > Here was my original reply:
> >
> > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > candidate for where the lifetime matches exactly what we want.
> > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > device. That's not the right device to use for lifetime management.
> > > > >
> > > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > >
> > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > introduce them as DRM-managed, and not device managed.
> > > >
> > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > but the DRM device is still around.
> > >=20
> > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > encoder device.
> >
> > bridge->dev seems right though?
> >
> > > I wasn't personally involved in discussions about it, but I was under
> > > the impression that this was expected / normal. Thus we can't make
> > > this DRM-managed.
> >
> > Still, I don't think devm is the right solution to this either.
> >
> > The underlying issue is two-fold:
> >
> > - Encoders can have a pointer to a bridge through of_drm_find_bridge
> > or similar. However, bridges are traditionally tied to their device
> > lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
> > in remove). Encoders will typically be tied to the DRM device
> > however, and that one sticks around until the last application
> > closes it. We can thus very easily end up with a dangling pointer,
> > and a use-after-free.
> >
> > - It's not the case yet, but it doesn't seem far fetch to expose
> > properties of bridges to the userspace. In that case, the userspace
> > would be likely to still hold references to objects that aren't
> > there anymore when the bridge is gone.
> >
> > The first is obviously a larger concern, but if we can find a solution
> > that would accomodate the second it would be great.
> >
> > As far as I can see, we should fix in two steps:
> >
> > - in drm_bridge_attach, we should add a device-managed call that will
> > unregister the main DRM device. We don't allow to probe the main DRM
> > device when the bridge isn't there yet in most case, so it makes
> > sense to remove it once the bridge is no longer there as well.
>
> The problem is that I do not see a good way to unregister the main DRM
> device outside of it's driver code.
That's what drmm helpers are doing though: they'll defer the cleanup
until the last user has closed its fd.
> > - When the DRM device is removed, have the core cleanup any bridge
> > registered. That will remove the need to have drm_bridge_remove in
> > the first place.
> >
> > > I'll assume that my response addressed your concerns. Assuming I get
> > > reviews for the other two patches in this series I'll plan to land
> > > this with Dmitry's review.
> >
> > I still don't think it's a good idea to merge it. It gives an illusion
> > of being safe, but it's really far from it.
>
> It is more of removing the boilerplate code spread over all the
> drivers rather than about particular safety.
>
> I'd propose to land devm_drm_bridge_add (and deprecate calling
> drm_bridge_remove from the bridge driver at some point) and work on
> the whole drm_device <-> drm_bridge problem in the meantime.
Do you really expect that to happen? :)
Anyway, it's been merged, it's too late now anyway. I don't really feel
like it's a good thing, but it doesn't really make the situation worse
either.
Maxime
On Fri, 3 Jun 2022 at 11:21, Maxime Ripard <[email protected]> wrote:
>
> On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> > On Mon, May 23, 2022 at 10:00 AM Doug Anderson <[email protected]> wrote:
> > > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard <[email protected]> wrote:
> > > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > candidate for where the lifetime matches exactly what we want.
> > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > device. That's not the right device to use for lifetime management.
> > > > >
> > > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > >
> > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > introduce them as DRM-managed, and not device managed.
> > > >
> > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > but the DRM device is still around.
> > >
> > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > encoder device. I wasn't personally involved in discussions about it,
> > > but I was under the impression that this was expected / normal. Thus
> > > we can't make this DRM-managed.
> >
> > Since I didn't hear a reply,
>
> Gah, I replied but it looks like somehow it never reached the ML...
>
> Here was my original reply:
>
> > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > * In general we have a "struct device" for bridges that makes a good
> > > > candidate for where the lifetime matches exactly what we want.
> > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > device. That's not the right device to use for lifetime management.
> > > >
> > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > Signed-off-by: Douglas Anderson <[email protected]>
> > >
> > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > introduce them as DRM-managed, and not device managed.
> > >
> > > Otherwise, you'll end up in a weird state when a device has been removed
> > > but the DRM device is still around.
> >=20
> > I'm kinda confused. In this case there is no DRM device for the bridge
> > and, as per my CL description, "bridge-dev->dev" appears to be the
> > encoder device.
>
> bridge->dev seems right though?
>
> > I wasn't personally involved in discussions about it, but I was under
> > the impression that this was expected / normal. Thus we can't make
> > this DRM-managed.
>
> Still, I don't think devm is the right solution to this either.
>
> The underlying issue is two-fold:
>
> - Encoders can have a pointer to a bridge through of_drm_find_bridge
> or similar. However, bridges are traditionally tied to their device
> lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
> in remove). Encoders will typically be tied to the DRM device
> however, and that one sticks around until the last application
> closes it. We can thus very easily end up with a dangling pointer,
> and a use-after-free.
>
> - It's not the case yet, but it doesn't seem far fetch to expose
> properties of bridges to the userspace. In that case, the userspace
> would be likely to still hold references to objects that aren't
> there anymore when the bridge is gone.
>
> The first is obviously a larger concern, but if we can find a solution
> that would accomodate the second it would be great.
>
> As far as I can see, we should fix in two steps:
>
> - in drm_bridge_attach, we should add a device-managed call that will
> unregister the main DRM device. We don't allow to probe the main DRM
> device when the bridge isn't there yet in most case, so it makes
> sense to remove it once the bridge is no longer there as well.
The problem is that I do not see a good way to unregister the main DRM
device outside of it's driver code.
>
> - When the DRM device is removed, have the core cleanup any bridge
> registered. That will remove the need to have drm_bridge_remove in
> the first place.
>
> > I'll assume that my response addressed your concerns. Assuming I get
> > reviews for the other two patches in this series I'll plan to land
> > this with Dmitry's review.
>
> I still don't think it's a good idea to merge it. It gives an illusion
> of being safe, but it's really far from it.
It is more of removing the boilerplate code spread over all the
drivers rather than about particular safety.
I'd propose to land devm_drm_bridge_add (and deprecate calling
drm_bridge_remove from the bridge driver at some point) and work on
the whole drm_device <-> drm_bridge problem in the meantime.
--
With best wishes
Dmitry
On Sat, May 21, 2022 at 11:17:51AM +0200, Maxime Ripard wrote:
> Hi,
>
> On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > This adds a devm managed version of drm_bridge_add(). Like other
> > "devm" function listed in drm_bridge.h, this function takes an
> > explicit "dev" to use for the lifetime management. A few notes:
> > * In general we have a "struct device" for bridges that makes a good
> > candidate for where the lifetime matches exactly what we want.
> > * The "bridge->dev->dev" device appears to be the encoder
> > device. That's not the right device to use for lifetime management.
> >
> > Suggested-by: Dmitry Baryshkov <[email protected]>
> > Signed-off-by: Douglas Anderson <[email protected]>
>
> If we are to introduce more managed helpers, I think it'd be wiser to
> introduce them as DRM-managed, and not device managed.
>
> Otherwise, you'll end up in a weird state when a device has been removed
> but the DRM device is still around.
Top-level post since I didn't see any good place to reply in the thread
below:
- devm is for device stuff, which drm_bridge is (it's not uapi visible in
any way or fasion)
- drmm is for uapi visible stuff (like drm_encoder)
Yes the uapi-visible stuff can outlive the device-related pieces. The way
to handle this is:
- drm_dev_unplug() when the device disappears underneath you (or just a
part, I guess the infra for that doesn't exist yet and maybe we should
add it).
- drm_dev_enter/exit wrapped around the device related parts.
Iow, this patch here I think is the right direction, and gets my
Reviewed-by: Daniel Vetter <[email protected]>
But also, it's definitely not a complete solution as the discussion in the
thread here points out.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Fri, Jun 03, 2022 at 07:56:16AM -0700, Doug Anderson wrote:
> Hi,
>
> On Fri, Jun 3, 2022 at 7:14 AM Maxime Ripard <[email protected]> wrote:
> >
> > On Fri, Jun 03, 2022 at 01:19:16PM +0300, Dmitry Baryshkov wrote:
> > > On Fri, 3 Jun 2022 at 11:21, Maxime Ripard <[email protected]> wrote:
> > > >
> > > > On Tue, May 31, 2022 at 02:06:34PM -0700, Doug Anderson wrote:
> > > > > On Mon, May 23, 2022 at 10:00 AM Doug Anderson <[email protected]> wrote:
> > > > > > On Sat, May 21, 2022 at 2:17 AM Maxime Ripard <[email protected]> wrote:
> > > > > > > On Tue, May 10, 2022 at 12:29:43PM -0700, Douglas Anderson wrote:
> > > > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > > > > candidate for where the lifetime matches exactly what we want.
> > > > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > > > > device. That's not the right device to use for lifetime management.
> > > > > > > >
> > > > > > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > > > > >
> > > > > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > > > > introduce them as DRM-managed, and not device managed.
> > > > > > >
> > > > > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > > > > but the DRM device is still around.
> > > > > >
> > > > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > > > encoder device. I wasn't personally involved in discussions about it,
> > > > > > but I was under the impression that this was expected / normal. Thus
> > > > > > we can't make this DRM-managed.
> > > > >
> > > > > Since I didn't hear a reply,
> > > >
> > > > Gah, I replied but it looks like somehow it never reached the ML...
> > > >
> > > > Here was my original reply:
> > > >
> > > > > > > This adds a devm managed version of drm_bridge_add(). Like other
> > > > > > > "devm" function listed in drm_bridge.h, this function takes an
> > > > > > > explicit "dev" to use for the lifetime management. A few notes:
> > > > > > > * In general we have a "struct device" for bridges that makes a good
> > > > > > > candidate for where the lifetime matches exactly what we want.
> > > > > > > * The "bridge->dev->dev" device appears to be the encoder
> > > > > > > device. That's not the right device to use for lifetime management.
> > > > > > >
> > > > > > > Suggested-by: Dmitry Baryshkov <[email protected]>
> > > > > > > Signed-off-by: Douglas Anderson <[email protected]>
> > > > > >
> > > > > > If we are to introduce more managed helpers, I think it'd be wiser to
> > > > > > introduce them as DRM-managed, and not device managed.
> > > > > >
> > > > > > Otherwise, you'll end up in a weird state when a device has been removed
> > > > > > but the DRM device is still around.
> > > > >=20
> > > > > I'm kinda confused. In this case there is no DRM device for the bridge
> > > > > and, as per my CL description, "bridge-dev->dev" appears to be the
> > > > > encoder device.
> > > >
> > > > bridge->dev seems right though?
> > > >
> > > > > I wasn't personally involved in discussions about it, but I was under
> > > > > the impression that this was expected / normal. Thus we can't make
> > > > > this DRM-managed.
> > > >
> > > > Still, I don't think devm is the right solution to this either.
> > > >
> > > > The underlying issue is two-fold:
> > > >
> > > > - Encoders can have a pointer to a bridge through of_drm_find_bridge
> > > > or similar. However, bridges are traditionally tied to their device
> > > > lifetime (by calling drm_bridge_add in probe, and drm_bridge_remove
> > > > in remove). Encoders will typically be tied to the DRM device
> > > > however, and that one sticks around until the last application
> > > > closes it. We can thus very easily end up with a dangling pointer,
> > > > and a use-after-free.
> > > >
> > > > - It's not the case yet, but it doesn't seem far fetch to expose
> > > > properties of bridges to the userspace. In that case, the userspace
> > > > would be likely to still hold references to objects that aren't
> > > > there anymore when the bridge is gone.
> > > >
> > > > The first is obviously a larger concern, but if we can find a solution
> > > > that would accomodate the second it would be great.
> > > >
> > > > As far as I can see, we should fix in two steps:
> > > >
> > > > - in drm_bridge_attach, we should add a device-managed call that will
> > > > unregister the main DRM device. We don't allow to probe the main DRM
> > > > device when the bridge isn't there yet in most case, so it makes
> > > > sense to remove it once the bridge is no longer there as well.
> > >
> > > The problem is that I do not see a good way to unregister the main DRM
> > > device outside of it's driver code.
> >
> > That's what drmm helpers are doing though: they'll defer the cleanup
> > until the last user has closed its fd.
>
> I'm a bit confused here. I'll take the concrete example of ps8640
> since that's what I was working on here.
>
> ...right now the fact that we're using devm means that
> drm_bridge_remove() will get called when a ps8640 device is unbound,
> right?
Yes
> I guess you're saying that the "drm_bridge" memory needs to
> outlast this, right?
Since drm_bridge isn't exposing anything to userspace, it would mostly
be its connector. But they are usually allocated in the same structure,
so it's pretty much equivalent here.
> That being said, even if the actual memory for drm_bridge outlasts the
> ps8640 driver lifetime, much of the data would need to be marked
> invalid I think.
All the device resources, yes. So things like IO mappings, clocks, reset
lines, regulators, etc.
> If nothing else all function pointers that point into the driver would
> have to be made NULL, right? Once the device has been unbound it's
> possible that the underlying module might be removed. I suspect that
> we'd need to do more than just bogus-up the function pointers, though.
I ... didn't think of the module memory being freed. I don't know the
module handling code, but if it's an option we could get a reference to
the module memory to make sure the memory stays around until everything
has been freed.
If we can't, then we could relocate all the functions inside the kernel
for the teardown, but I'm sure it's going to be a mess.
> ...so it feels like any solution here needs to take into account
> _both_ the lifetime of the "struct device" and the "struct
> drm_device". If the "struct device" goes away but the "struct
> drm_device" is still around then we need to essentially transition the
> "struct drm_device" over to a dummy, right?
So we want to make sure we won't access the device resources if they
aren't there anymore, during the timeframe between the device being
unbound and the DRM device being unregistered (which can be arbitrarily
long). Fortunately, drm_device->registered is being toggled as soon as
we start the unbinding process, and drm_dev_enter()/drm_dev_exit() is
there to make sure the device is registered.
So we would need to make sure that all device resource access is
protected by a call to those functions. It's tedious, but it works
today.
It's a bit more complicated in the case of bridges (as opposed to any
other entity) because you don't have access to the DRM device when you
probe, only when you are attached. So you also need to make sure the
private structure you allocated in probe (using devm_) is properly
converted to be DRM-managed and freed later on.
Maxime