| Message ID | 20250206-hotplug-drm-bridge-v6-8-9d6f2c9c3058@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for hot-pluggable DRM bridges | expand |
On Thu, Feb 06, 2025 at 07:14:23PM +0100, Luca Ceresoli wrote: > Adding a panel does currently not add a panel_bridge wrapping it. Usually > the panel_bridge creation happens when some other driver (e.g. the previous > bridge or the encoder) calls *_of_get_bridge() and the following element in > the pipeline is a panel. > > This has some drawbacks: > > * the panel_bridge is not created in the context of the driver of the > underlying physical device (the panel driver), but of some other driver > * that "other driver" is not aware of whether the returned drm_bridge > pointer is a panel_bridge created on the fly, a pre-existing > panel_bridge or a non-panel bridge > * removal of a panel_bridge requires calling drm_panel_bridge_remove(), > but the "other driver" doesn't know whether this is needed because it > doesn't know whether it has created a panel_bridge or not > > So far this approach has been working because devm and drmm ensure the > panel bridge would be dealloacted at some later point. However with the > upcoming implementation of dynamic bridge lifetime this will get more > complicated. > > Correct removal of a panel_bridge might possibly be obtained by adding more > devm/drmm technology to have it freed correctly at all times. However this > would add more complexity and not help making lifetime more understandable. > > Use a different approach instead: always create a panel_bridge with a > drm_panel, thus matching the lifetime of the drm_panel and the panel_bridge > wrapping it. This makes lifetime much more straightforward to understand > and to further develop on. > > With the panel_bridge always created, the functions to get a bridge > [devm_drm_of_get_bridge() and drmm_of_get_bridge()] become simpler because > the bridge they are looking for exists already (if it can exist at all). In > turn, this is implemented based on a variant of > drm_of_find_panel_or_bridge() that only looks for panels: > of_drm_find_bridge_by_endpoint(). In the future > drm_of_find_panel_or_bridge() can be progressively removed because there > will never be a panel not exposing a bridge. > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > --- > > This patch was added in v6. > --- > drivers/gpu/drm/bridge/panel.c | 74 +++++++++++++++++++++++++++++++++--------- > include/drm/drm_panel.h | 8 +++++ > 2 files changed, 66 insertions(+), 16 deletions(-) > LGTM, minor issue below. > @@ -1018,6 +1067,11 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev, > { > struct drm_bridge **ptr, *bridge; > > + if (panel->bridge) { > + DRM_DEBUG("panel %s: returning existing bridge=%p", dev_name(dev), panel->bridge); > + return panel->bridge; > + } Shouldn't the rest of the function also be removed as you do in other cases? > + > ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr), > GFP_KERNEL); > if (!ptr)
On Fri, 7 Feb 2025 04:49:21 +0200 Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > On Thu, Feb 06, 2025 at 07:14:23PM +0100, Luca Ceresoli wrote: > > Adding a panel does currently not add a panel_bridge wrapping it. Usually > > the panel_bridge creation happens when some other driver (e.g. the previous > > bridge or the encoder) calls *_of_get_bridge() and the following element in > > the pipeline is a panel. > > > > This has some drawbacks: > > > > * the panel_bridge is not created in the context of the driver of the > > underlying physical device (the panel driver), but of some other driver > > * that "other driver" is not aware of whether the returned drm_bridge > > pointer is a panel_bridge created on the fly, a pre-existing > > panel_bridge or a non-panel bridge > > * removal of a panel_bridge requires calling drm_panel_bridge_remove(), > > but the "other driver" doesn't know whether this is needed because it > > doesn't know whether it has created a panel_bridge or not > > > > So far this approach has been working because devm and drmm ensure the > > panel bridge would be dealloacted at some later point. However with the > > upcoming implementation of dynamic bridge lifetime this will get more > > complicated. > > > > Correct removal of a panel_bridge might possibly be obtained by adding more > > devm/drmm technology to have it freed correctly at all times. However this > > would add more complexity and not help making lifetime more understandable. > > > > Use a different approach instead: always create a panel_bridge with a > > drm_panel, thus matching the lifetime of the drm_panel and the panel_bridge > > wrapping it. This makes lifetime much more straightforward to understand > > and to further develop on. > > > > With the panel_bridge always created, the functions to get a bridge > > [devm_drm_of_get_bridge() and drmm_of_get_bridge()] become simpler because > > the bridge they are looking for exists already (if it can exist at all). In > > turn, this is implemented based on a variant of > > drm_of_find_panel_or_bridge() that only looks for panels: > > of_drm_find_bridge_by_endpoint(). In the future > > drm_of_find_panel_or_bridge() can be progressively removed because there > > will never be a panel not exposing a bridge. > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > > > --- > > > > This patch was added in v6. > > --- > > drivers/gpu/drm/bridge/panel.c | 74 +++++++++++++++++++++++++++++++++--------- > > include/drm/drm_panel.h | 8 +++++ > > 2 files changed, 66 insertions(+), 16 deletions(-) > > > > LGTM, minor issue below. > > > @@ -1018,6 +1067,11 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev, > > { > > struct drm_bridge **ptr, *bridge; > > > > + if (panel->bridge) { > > + DRM_DEBUG("panel %s: returning existing bridge=%p", dev_name(dev), panel->bridge); > > + return panel->bridge; > > + } > > Shouldn't the rest of the function also be removed as you do in other > cases? Indeed it should. And even more, I now realize drm_panel_bridge_add_typed() should also become a simple 'return panel->bridge', like its devm and drmm variants, and its code, implementing the actual creation of a panel bridge, move to an internal function. Otherwise this patch is a bug: existing drivers which do call drm_panel_bridge_add_typed() would end up in having two panel_bridges for the same panel. I must say the process of developing this patch together with the hotplug work was "convoluted" to say the least, which probably explains why this got unnoticed so far. Luca
On Fri, Feb 07, 2025 at 09:54:28AM +0100, Luca Ceresoli wrote: > On Fri, 7 Feb 2025 04:49:21 +0200 > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > On Thu, Feb 06, 2025 at 07:14:23PM +0100, Luca Ceresoli wrote: > > > Adding a panel does currently not add a panel_bridge wrapping it. Usually > > > the panel_bridge creation happens when some other driver (e.g. the previous > > > bridge or the encoder) calls *_of_get_bridge() and the following element in > > > the pipeline is a panel. > > > > > > This has some drawbacks: > > > > > > * the panel_bridge is not created in the context of the driver of the > > > underlying physical device (the panel driver), but of some other driver > > > * that "other driver" is not aware of whether the returned drm_bridge > > > pointer is a panel_bridge created on the fly, a pre-existing > > > panel_bridge or a non-panel bridge > > > * removal of a panel_bridge requires calling drm_panel_bridge_remove(), > > > but the "other driver" doesn't know whether this is needed because it > > > doesn't know whether it has created a panel_bridge or not > > > > > > So far this approach has been working because devm and drmm ensure the > > > panel bridge would be dealloacted at some later point. However with the > > > upcoming implementation of dynamic bridge lifetime this will get more > > > complicated. > > > > > > Correct removal of a panel_bridge might possibly be obtained by adding more > > > devm/drmm technology to have it freed correctly at all times. However this > > > would add more complexity and not help making lifetime more understandable. > > > > > > Use a different approach instead: always create a panel_bridge with a > > > drm_panel, thus matching the lifetime of the drm_panel and the panel_bridge > > > wrapping it. This makes lifetime much more straightforward to understand > > > and to further develop on. > > > > > > With the panel_bridge always created, the functions to get a bridge > > > [devm_drm_of_get_bridge() and drmm_of_get_bridge()] become simpler because > > > the bridge they are looking for exists already (if it can exist at all). In > > > turn, this is implemented based on a variant of > > > drm_of_find_panel_or_bridge() that only looks for panels: > > > of_drm_find_bridge_by_endpoint(). In the future > > > drm_of_find_panel_or_bridge() can be progressively removed because there > > > will never be a panel not exposing a bridge. > > > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > > > > > --- > > > > > > This patch was added in v6. > > > --- > > > drivers/gpu/drm/bridge/panel.c | 74 +++++++++++++++++++++++++++++++++--------- > > > include/drm/drm_panel.h | 8 +++++ > > > 2 files changed, 66 insertions(+), 16 deletions(-) > > > > > > > LGTM, minor issue below. > > > > > @@ -1018,6 +1067,11 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev, > > > { > > > struct drm_bridge **ptr, *bridge; > > > > > > + if (panel->bridge) { > > > + DRM_DEBUG("panel %s: returning existing bridge=%p", dev_name(dev), panel->bridge); > > > + return panel->bridge; > > > + } > > > > Shouldn't the rest of the function also be removed as you do in other > > cases? > > Indeed it should. > > And even more, I now realize drm_panel_bridge_add_typed() should also > become a simple 'return panel->bridge', like its devm and drmm > variants, and its code, implementing the actual creation of a panel > bridge, move to an internal function. Otherwise this patch is a bug: > existing drivers which do call drm_panel_bridge_add_typed() would end > up in having two panel_bridges for the same panel. > > I must say the process of developing this patch together with the > hotplug work was "convoluted" to say the least, which probably explains > why this got unnoticed so far. That's why I suggested to post this series separately - it saves you from rebasing hotplug work on top. > > Luca > > -- > Luca Ceresoli, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hi Dmitry, On Fri, 7 Feb 2025 21:43:26 +0200 Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > > @@ -1018,6 +1067,11 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev, > > > > { > > > > struct drm_bridge **ptr, *bridge; > > > > > > > > + if (panel->bridge) { > > > > + DRM_DEBUG("panel %s: returning existing bridge=%p", dev_name(dev), panel->bridge); > > > > + return panel->bridge; > > > > + } > > > > > > Shouldn't the rest of the function also be removed as you do in other > > > cases? > > > > Indeed it should. > > > > And even more, I now realize drm_panel_bridge_add_typed() should also > > become a simple 'return panel->bridge', like its devm and drmm > > variants, and its code, implementing the actual creation of a panel > > bridge, move to an internal function. Otherwise this patch is a bug: > > existing drivers which do call drm_panel_bridge_add_typed() would end > > up in having two panel_bridges for the same panel. > > > > I must say the process of developing this patch together with the > > hotplug work was "convoluted" to say the least, which probably explains > > why this got unnoticed so far. > > That's why I suggested to post this series separately - it saves you > from rebasing hotplug work on top. Yes, that's sure, but not keeping my hotplug patches on top of the panel_bridge ones makes it much harder for me to test on real hardware, so each way has pros and cons. However I might send only the panel_bridge patches at the next iteration. Luca
On Thu, Feb 06, 2025 at 07:14:23PM +0100, Luca Ceresoli wrote: > Adding a panel does currently not add a panel_bridge wrapping it. Usually > the panel_bridge creation happens when some other driver (e.g. the previous > bridge or the encoder) calls *_of_get_bridge() and the following element in > the pipeline is a panel. > > This has some drawbacks: > > * the panel_bridge is not created in the context of the driver of the > underlying physical device (the panel driver), but of some other driver > * that "other driver" is not aware of whether the returned drm_bridge > pointer is a panel_bridge created on the fly, a pre-existing > panel_bridge or a non-panel bridge > * removal of a panel_bridge requires calling drm_panel_bridge_remove(), > but the "other driver" doesn't know whether this is needed because it > doesn't know whether it has created a panel_bridge or not > > So far this approach has been working because devm and drmm ensure the > panel bridge would be dealloacted at some later point. However with the > upcoming implementation of dynamic bridge lifetime this will get more > complicated. > > Correct removal of a panel_bridge might possibly be obtained by adding more > devm/drmm technology to have it freed correctly at all times. However this > would add more complexity and not help making lifetime more understandable. > > Use a different approach instead: always create a panel_bridge with a > drm_panel, thus matching the lifetime of the drm_panel and the panel_bridge > wrapping it. This makes lifetime much more straightforward to understand > and to further develop on. > > With the panel_bridge always created, the functions to get a bridge > [devm_drm_of_get_bridge() and drmm_of_get_bridge()] become simpler because > the bridge they are looking for exists already (if it can exist at all). In > turn, this is implemented based on a variant of > drm_of_find_panel_or_bridge() that only looks for panels: > of_drm_find_bridge_by_endpoint(). In the future > drm_of_find_panel_or_bridge() can be progressively removed because there > will never be a panel not exposing a bridge. > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > --- > > This patch was added in v6. > --- > drivers/gpu/drm/bridge/panel.c | 74 +++++++++++++++++++++++++++++++++--------- > include/drm/drm_panel.h | 8 +++++ > 2 files changed, 66 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index 58570ff6952ca313b3def084262c9bb3772272ef..6995de605e7317dd1eb153afd475746ced764712 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -69,6 +69,9 @@ EXPORT_SYMBOL(drm_panel_init); > */ > void drm_panel_add(struct drm_panel *panel) > { > + panel->bridge = drm_panel_bridge_add(panel); > + WARN_ON(!panel->bridge); > + > mutex_lock(&panel_lock); > list_add_tail(&panel->list, &panel_list); > mutex_unlock(&panel_lock); > @@ -86,6 +89,9 @@ void drm_panel_remove(struct drm_panel *panel) > mutex_lock(&panel_lock); > list_del_init(&panel->list); > mutex_unlock(&panel_lock); > + > + drm_panel_bridge_remove(panel->bridge); > + panel->bridge = NULL; > } > EXPORT_SYMBOL(drm_panel_remove); Given that drm_panel_add and drm_panel_remove are typically called at probe/remove, it's pretty much equivalent to using devm. Both of these solutions aren't safe, and the drm_panel lifetime is still broken. I'd rather work on a solution that actually fixes those lifetime issues. Maxime
On Tue, Feb 11, 2025 at 2:34 AM Maxime Ripard <mripard@kernel.org> wrote: > > On Thu, Feb 06, 2025 at 07:14:23PM +0100, Luca Ceresoli wrote: > > Adding a panel does currently not add a panel_bridge wrapping it. Usually > > the panel_bridge creation happens when some other driver (e.g. the previous > > bridge or the encoder) calls *_of_get_bridge() and the following element in > > the pipeline is a panel. > > > > This has some drawbacks: > > > > * the panel_bridge is not created in the context of the driver of the > > underlying physical device (the panel driver), but of some other driver > > * that "other driver" is not aware of whether the returned drm_bridge > > pointer is a panel_bridge created on the fly, a pre-existing > > panel_bridge or a non-panel bridge > > * removal of a panel_bridge requires calling drm_panel_bridge_remove(), > > but the "other driver" doesn't know whether this is needed because it > > doesn't know whether it has created a panel_bridge or not > > > > So far this approach has been working because devm and drmm ensure the > > panel bridge would be dealloacted at some later point. However with the > > upcoming implementation of dynamic bridge lifetime this will get more > > complicated. > > > > Correct removal of a panel_bridge might possibly be obtained by adding more > > devm/drmm technology to have it freed correctly at all times. However this > > would add more complexity and not help making lifetime more understandable. > > > > Use a different approach instead: always create a panel_bridge with a > > drm_panel, thus matching the lifetime of the drm_panel and the panel_bridge > > wrapping it. This makes lifetime much more straightforward to understand > > and to further develop on. > > > > With the panel_bridge always created, the functions to get a bridge > > [devm_drm_of_get_bridge() and drmm_of_get_bridge()] become simpler because > > the bridge they are looking for exists already (if it can exist at all). In > > turn, this is implemented based on a variant of > > drm_of_find_panel_or_bridge() that only looks for panels: > > of_drm_find_bridge_by_endpoint(). In the future > > drm_of_find_panel_or_bridge() can be progressively removed because there > > will never be a panel not exposing a bridge. > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > > > --- > > > > This patch was added in v6. > > --- > > drivers/gpu/drm/bridge/panel.c | 74 +++++++++++++++++++++++++++++++++--------- > > include/drm/drm_panel.h | 8 +++++ > > 2 files changed, 66 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > > index 58570ff6952ca313b3def084262c9bb3772272ef..6995de605e7317dd1eb153afd475746ced764712 100644 > > --- a/drivers/gpu/drm/bridge/panel.c > > +++ b/drivers/gpu/drm/bridge/panel.c > > @@ -69,6 +69,9 @@ EXPORT_SYMBOL(drm_panel_init); > > */ > > void drm_panel_add(struct drm_panel *panel) > > { > > + panel->bridge = drm_panel_bridge_add(panel); > > + WARN_ON(!panel->bridge); > > + > > mutex_lock(&panel_lock); > > list_add_tail(&panel->list, &panel_list); > > mutex_unlock(&panel_lock); > > @@ -86,6 +89,9 @@ void drm_panel_remove(struct drm_panel *panel) > > mutex_lock(&panel_lock); > > list_del_init(&panel->list); > > mutex_unlock(&panel_lock); > > + > > + drm_panel_bridge_remove(panel->bridge); > > + panel->bridge = NULL; > > } > > EXPORT_SYMBOL(drm_panel_remove); > > Given that drm_panel_add and drm_panel_remove are typically called at > probe/remove, it's pretty much equivalent to using devm. Both of these > solutions aren't safe, and the drm_panel lifetime is still broken. FWIW I believe this solves the panel vs panel_bridge lifetime inconsistencies we previously reported [1]. Of course, as you rightly point out, any pointers to the bridge become stale if the panel gets removed. > I'd rather work on a solution that actually fixes those lifetime issues. I think that can happen once the bridges are ref-counted? Instead of removing the bridge from the list, it can just clear the panel pointer and have all the callbacks skip any operations involving the panel. The other option is to have the panel itself be ref-counted. I don't think that's worth pursuing if the idea is to move everything over to panel_bridge and things are somewhat ready. ChenYu [1] https://lore.kernel.org/dri-devel/20241009052402.411978-1-fshao@chromium.org/
On Tue, Feb 18, 2025 at 05:43:43PM +0800, Chen-Yu Tsai wrote: > On Tue, Feb 11, 2025 at 2:34 AM Maxime Ripard <mripard@kernel.org> wrote: > > > > On Thu, Feb 06, 2025 at 07:14:23PM +0100, Luca Ceresoli wrote: > > > Adding a panel does currently not add a panel_bridge wrapping it. Usually > > > the panel_bridge creation happens when some other driver (e.g. the previous > > > bridge or the encoder) calls *_of_get_bridge() and the following element in > > > the pipeline is a panel. > > > > > > This has some drawbacks: > > > > > > * the panel_bridge is not created in the context of the driver of the > > > underlying physical device (the panel driver), but of some other driver > > > * that "other driver" is not aware of whether the returned drm_bridge > > > pointer is a panel_bridge created on the fly, a pre-existing > > > panel_bridge or a non-panel bridge > > > * removal of a panel_bridge requires calling drm_panel_bridge_remove(), > > > but the "other driver" doesn't know whether this is needed because it > > > doesn't know whether it has created a panel_bridge or not > > > > > > So far this approach has been working because devm and drmm ensure the > > > panel bridge would be dealloacted at some later point. However with the > > > upcoming implementation of dynamic bridge lifetime this will get more > > > complicated. > > > > > > Correct removal of a panel_bridge might possibly be obtained by adding more > > > devm/drmm technology to have it freed correctly at all times. However this > > > would add more complexity and not help making lifetime more understandable. > > > > > > Use a different approach instead: always create a panel_bridge with a > > > drm_panel, thus matching the lifetime of the drm_panel and the panel_bridge > > > wrapping it. This makes lifetime much more straightforward to understand > > > and to further develop on. > > > > > > With the panel_bridge always created, the functions to get a bridge > > > [devm_drm_of_get_bridge() and drmm_of_get_bridge()] become simpler because > > > the bridge they are looking for exists already (if it can exist at all). In > > > turn, this is implemented based on a variant of > > > drm_of_find_panel_or_bridge() that only looks for panels: > > > of_drm_find_bridge_by_endpoint(). In the future > > > drm_of_find_panel_or_bridge() can be progressively removed because there > > > will never be a panel not exposing a bridge. > > > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > > > > > --- > > > > > > This patch was added in v6. > > > --- > > > drivers/gpu/drm/bridge/panel.c | 74 +++++++++++++++++++++++++++++++++--------- > > > include/drm/drm_panel.h | 8 +++++ > > > 2 files changed, 66 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > > > index 58570ff6952ca313b3def084262c9bb3772272ef..6995de605e7317dd1eb153afd475746ced764712 100644 > > > --- a/drivers/gpu/drm/bridge/panel.c > > > +++ b/drivers/gpu/drm/bridge/panel.c > > > @@ -69,6 +69,9 @@ EXPORT_SYMBOL(drm_panel_init); > > > */ > > > void drm_panel_add(struct drm_panel *panel) > > > { > > > + panel->bridge = drm_panel_bridge_add(panel); > > > + WARN_ON(!panel->bridge); > > > + > > > mutex_lock(&panel_lock); > > > list_add_tail(&panel->list, &panel_list); > > > mutex_unlock(&panel_lock); > > > @@ -86,6 +89,9 @@ void drm_panel_remove(struct drm_panel *panel) > > > mutex_lock(&panel_lock); > > > list_del_init(&panel->list); > > > mutex_unlock(&panel_lock); > > > + > > > + drm_panel_bridge_remove(panel->bridge); > > > + panel->bridge = NULL; > > > } > > > EXPORT_SYMBOL(drm_panel_remove); > > > > Given that drm_panel_add and drm_panel_remove are typically called at > > probe/remove, it's pretty much equivalent to using devm. Both of these > > solutions aren't safe, and the drm_panel lifetime is still broken. > > FWIW I believe this solves the panel vs panel_bridge lifetime > inconsistencies we previously reported [1]. Of course, as you rightly > point out, any pointers to the bridge become stale if the panel gets > removed. > > > I'd rather work on a solution that actually fixes those lifetime issues. > > I think that can happen once the bridges are ref-counted? Not all panel users use a bridge, some are using the panel API directly. > Instead of removing the bridge from the list, it can just clear the > panel pointer and have all the callbacks skip any operations involving > the panel. > > The other option is to have the panel itself be ref-counted. I don't > think that's worth pursuing if the idea is to move everything over to > panel_bridge and things are somewhat ready. Yeah, maybe. I'm kind of skeptical it would be easier than just doing the work we're doing here though. Most of the "easy" users have been converted already, and only the problematic ones remain, which will be difficult to move over. sun4i_rgb and sun6i_dsi, for example, won't be trivial to switch to panel_bridge. And given that sun4i_rgb is mostly obsolete, and sun6i_dsi unmaintainable, I don't think it's likely to happen. So I don't think it's fair to expect that work from such a series, or to rely on it eventually being fixed. I think a solution where we have a bridge in drm_panel is a better solution (and easier to convert to), but that requires a new panel allocation API too. We've started to work on it as well, so everything might just click. Maxime
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 58570ff6952ca313b3def084262c9bb3772272ef..6995de605e7317dd1eb153afd475746ced764712 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -69,6 +69,9 @@ EXPORT_SYMBOL(drm_panel_init); */ void drm_panel_add(struct drm_panel *panel) { + panel->bridge = drm_panel_bridge_add(panel); + WARN_ON(!panel->bridge); + mutex_lock(&panel_lock); list_add_tail(&panel->list, &panel_list); mutex_unlock(&panel_lock); @@ -86,6 +89,9 @@ void drm_panel_remove(struct drm_panel *panel) mutex_lock(&panel_lock); list_del_init(&panel->list); mutex_unlock(&panel_lock); + + drm_panel_bridge_remove(panel->bridge); + panel->bridge = NULL; } EXPORT_SYMBOL(drm_panel_remove); @@ -412,6 +418,49 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, } EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); +/** + * of_drm_find_bridge_by_endpoint - return drm_bridge connected to an endpoint + * @np: device tree node containing encoder output ports + * @port: port in the device tree node + * @endpoint: endpoint in the device tree node + * @bridge: pointer to hold returned drm_bridge (must not be NULL) + * + * Given a DT node's port and endpoint number, find the connected node and + * return the associated struct drm_bridge. + * + * Returns zero if successful, or one of the standard error codes if it fails. + */ +static int of_drm_find_bridge_by_endpoint(const struct device_node *np, + int port, int endpoint, + struct drm_bridge **bridge) +{ + int ret = -EPROBE_DEFER; + struct device_node *remote; + + if (!bridge) + return -EINVAL; + + /* + * of_graph_get_remote_node() produces a noisy error message if port + * node isn't found and the absence of the port is a legit case here, + * so at first we silently check whether graph presents in the + * device-tree node. + */ + if (!of_graph_is_present(np)) + return -ENODEV; + + remote = of_graph_get_remote_node(np, port, endpoint); + if (!remote) + return -ENODEV; + + *bridge = of_drm_find_bridge(remote); + if (*bridge) + ret = 0; + + of_node_put(remote); + return ret; +} + /** * of_drm_get_panel_orientation - look up the orientation of the panel through * the "rotation" binding from a device tree node @@ -1018,6 +1067,11 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev, { struct drm_bridge **ptr, *bridge; + if (panel->bridge) { + DRM_DEBUG("panel %s: returning existing bridge=%p", dev_name(dev), panel->bridge); + return panel->bridge; + } + ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr), GFP_KERNEL); if (!ptr) @@ -1106,8 +1160,7 @@ EXPORT_SYMBOL(drm_panel_bridge_connector); * @endpoint: endpoint in the device tree node * * Given a DT node's port and endpoint number, finds the connected node - * and returns the associated bridge if any, or creates and returns a - * drm panel bridge instance if a panel is connected. + * and returns the associated bridge if any. * * Returns a pointer to the bridge if successful, or an error pointer * otherwise. @@ -1117,17 +1170,12 @@ struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, u32 port, u32 endpoint) { struct drm_bridge *bridge; - struct drm_panel *panel; int ret; - ret = drm_of_find_panel_or_bridge(np, port, endpoint, - &panel, &bridge); + ret = of_drm_find_bridge_by_endpoint(np, port, endpoint, &bridge); if (ret) return ERR_PTR(ret); - if (panel) - bridge = devm_drm_panel_bridge_add(dev, panel); - return bridge; } EXPORT_SYMBOL(devm_drm_of_get_bridge); @@ -1140,8 +1188,7 @@ EXPORT_SYMBOL(devm_drm_of_get_bridge); * @endpoint: endpoint in the device tree node * * Given a DT node's port and endpoint number, finds the connected node - * and returns the associated bridge if any, or creates and returns a - * drm panel bridge instance if a panel is connected. + * and returns the associated bridge if any. * * Returns a drmm managed pointer to the bridge if successful, or an error * pointer otherwise. @@ -1151,17 +1198,12 @@ struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, u32 port, u32 endpoint) { struct drm_bridge *bridge; - struct drm_panel *panel; int ret; - ret = drm_of_find_panel_or_bridge(np, port, endpoint, - &panel, &bridge); + ret = of_drm_find_bridge_by_endpoint(np, port, endpoint, &bridge); if (ret) return ERR_PTR(ret); - if (panel) - bridge = drmm_panel_bridge_add(drm, panel); - return bridge; } EXPORT_SYMBOL(drmm_of_get_bridge); diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 10015891b056f816c7a992a2052b36fd26943c5b..7ace6c1389c5353c08de5ac46127e46e1de69359 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -196,6 +196,14 @@ struct drm_panel { */ struct device *dev; + /** + * @bridge: + * + * Pointer to the panel bridge that allows accessing this panel as + * a DRM bridge. + */ + struct drm_bridge *bridge; + /** * @backlight: *
Adding a panel does currently not add a panel_bridge wrapping it. Usually the panel_bridge creation happens when some other driver (e.g. the previous bridge or the encoder) calls *_of_get_bridge() and the following element in the pipeline is a panel. This has some drawbacks: * the panel_bridge is not created in the context of the driver of the underlying physical device (the panel driver), but of some other driver * that "other driver" is not aware of whether the returned drm_bridge pointer is a panel_bridge created on the fly, a pre-existing panel_bridge or a non-panel bridge * removal of a panel_bridge requires calling drm_panel_bridge_remove(), but the "other driver" doesn't know whether this is needed because it doesn't know whether it has created a panel_bridge or not So far this approach has been working because devm and drmm ensure the panel bridge would be dealloacted at some later point. However with the upcoming implementation of dynamic bridge lifetime this will get more complicated. Correct removal of a panel_bridge might possibly be obtained by adding more devm/drmm technology to have it freed correctly at all times. However this would add more complexity and not help making lifetime more understandable. Use a different approach instead: always create a panel_bridge with a drm_panel, thus matching the lifetime of the drm_panel and the panel_bridge wrapping it. This makes lifetime much more straightforward to understand and to further develop on. With the panel_bridge always created, the functions to get a bridge [devm_drm_of_get_bridge() and drmm_of_get_bridge()] become simpler because the bridge they are looking for exists already (if it can exist at all). In turn, this is implemented based on a variant of drm_of_find_panel_or_bridge() that only looks for panels: of_drm_find_bridge_by_endpoint(). In the future drm_of_find_panel_or_bridge() can be progressively removed because there will never be a panel not exposing a bridge. Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- This patch was added in v6. --- drivers/gpu/drm/bridge/panel.c | 74 +++++++++++++++++++++++++++++++++--------- include/drm/drm_panel.h | 8 +++++ 2 files changed, 66 insertions(+), 16 deletions(-)