| Message ID | 20250206-hotplug-drm-bridge-v6-15-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:30PM +0100, Luca Ceresoli wrote: > Add a devm/drmm action to these functions so the bridge reference is > dropped automatically when the caller is removed. I think the get() should go to the underlying of_drm_bridge_find() function. Also it really feels like it's an overkill to keep the wrappers. After getting bridge being handled by the panel code would it be possible to drop all of them? Then this patch might introduce one new devm_ function? Or are drmm_ functions actually being used to store data in the drmm-managed memory? > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > --- > > This patch was added in v6. > --- > drivers/gpu/drm/drm_bridge.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index f694b32ca59cb91c32846bc00b43360df41cc1ad..497ec06dfcb05ab5dee8ea5e8f1eafb9c2807612 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -33,6 +33,7 @@ > #include <drm/drm_edid.h> > #include <drm/drm_encoder.h> > #include <drm/drm_file.h> > +#include <drm/drm_managed.h> > #include <drm/drm_of.h> > #include <drm/drm_print.h> > > @@ -1459,6 +1460,13 @@ static int of_drm_find_bridge_by_endpoint(const struct device_node *np, > return ret; > } > > +static void devm_drm_bridge_put_void(void *data) > +{ > + struct drm_bridge *bridge = (struct drm_bridge *)data; > + > + drm_bridge_put(bridge); > +} > + > /** > * devm_drm_of_get_bridge - Return next bridge in the chain > * @dev: device to tie the bridge lifetime to > @@ -1469,6 +1477,10 @@ static int of_drm_find_bridge_by_endpoint(const struct device_node *np, > * Given a DT node's port and endpoint number, finds the connected node > * and returns the associated bridge if any. > * > + * The refcount of the returned bridge is incremented, but the caller does > + * not have to call drm_bridge_put() when done with the bridge. It will be > + * done by devres when @dev is removed. > + * > * Returns a pointer to the bridge if successful, or an error pointer > * otherwise. > */ > @@ -1483,6 +1495,10 @@ struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, > if (ret) > return ERR_PTR(ret); > > + ret = devm_add_action_or_reset(dev, devm_drm_bridge_put_void, bridge); > + if (ret) > + return ERR_PTR(ret); > + > return bridge; > } > EXPORT_SYMBOL(devm_drm_of_get_bridge); > @@ -1497,6 +1513,10 @@ EXPORT_SYMBOL(devm_drm_of_get_bridge); > * graph link search is not enough, e.g. for drivers that need to support > * panels described only as subnodes. > * > + * The refcount of the returned bridge is incremented, but the caller does > + * not have to call drm_bridge_put() when done with the bridge. It will be > + * done by devres when @dev is removed. > + * > * RETURNS: > * A pointer to the bridge if successful, or an error pointer otherwise. > */ > @@ -1513,10 +1533,21 @@ struct drm_bridge *devm_drm_of_get_bridge_by_node(struct device *dev, > if (!bridge) > return ERR_PTR(-ENODEV); > > + ret = devm_add_action_or_reset(dev, devm_drm_bridge_put_void, bridge); > + if (ret) > + return ERR_PTR(ret); > + > return bridge; > } > EXPORT_SYMBOL(devm_drm_of_get_bridge_by_node); > > +static void drmm_bridge_put_void(struct drm_device *drm, void *ptr) > +{ > + struct drm_bridge *bridge = ptr; > + > + drm_bridge_put(bridge); > +} > + > /** > * drmm_of_get_bridge - Return next bridge in the chain > * @drm: device to tie the bridge lifetime to > @@ -1527,6 +1558,10 @@ EXPORT_SYMBOL(devm_drm_of_get_bridge_by_node); > * Given a DT node's port and endpoint number, finds the connected node > * and returns the associated bridge if any. > * > + * The refcount of the returned bridge is incremented, but the caller does > + * not have to call drm_bridge_put() when done with the bridge. It will be > + * done by drmm when @dev is removed. > + * > * Returns a drmm managed pointer to the bridge if successful, or an error > * pointer otherwise. > */ > @@ -1541,6 +1576,10 @@ struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, > if (ret) > return ERR_PTR(ret); > > + ret = drmm_add_action_or_reset(drm, drmm_bridge_put_void, bridge); > + if (ret) > + return ERR_PTR(ret); > + > return bridge; > } > EXPORT_SYMBOL(drmm_of_get_bridge); > > -- > 2.34.1 >
On Fri, 7 Feb 2025 05:17:43 +0200 Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > On Thu, Feb 06, 2025 at 07:14:30PM +0100, Luca Ceresoli wrote: > > Add a devm/drmm action to these functions so the bridge reference is > > dropped automatically when the caller is removed. > > I think the get() should go to the underlying of_drm_bridge_find() function. It is done in the following patch. Indeed I could swap patches 15 and 16 for clarity. Or I could squash together patches 14+15+16, as they are various parts or the refcounted bridge implementation, but I felt like keeping them separated would help reviewing. > Also it really feels like it's an overkill to keep the wrappers. After > getting bridge being handled by the panel code would it be possible to > drop all of them? Do you mean having only drm_of_get_bridge_by_node(), without any devm or drmm variant? I'm not sure it is a good idea. Most DRM code (well, all of it, technically) is currently unable of working with refcounted bridges, but if they use the devm variant they will put the ref when they disappear. > Then this patch might introduce one new devm_ > function? Or are drmm_ functions actually being used to store data in > the drmm-managed memory? Which devm function are you thinking about? Sorry, I'm not following here. Luca
On Fri, Feb 07, 2025 at 11:44:01AM +0100, Luca Ceresoli wrote: > On Fri, 7 Feb 2025 05:17:43 +0200 > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > On Thu, Feb 06, 2025 at 07:14:30PM +0100, Luca Ceresoli wrote: > > > Add a devm/drmm action to these functions so the bridge reference is > > > dropped automatically when the caller is removed. > > > > I think the get() should go to the underlying of_drm_bridge_find() function. > > It is done in the following patch. > > Indeed I could swap patches 15 and 16 for clarity. Or I could squash > together patches 14+15+16, as they are various parts or the refcounted > bridge implementation, but I felt like keeping them separated would > help reviewing. I think most of refcounting should be introduced as a single patch, otherwise you risk having memory leaks or disappearing bridges. > > > Also it really feels like it's an overkill to keep the wrappers. After > > getting bridge being handled by the panel code would it be possible to > > drop all of them? > > Do you mean having only drm_of_get_bridge_by_node(), without any devm > or drmm variant? I'm not sure it is a good idea. Most DRM code (well, > all of it, technically) is currently unable of working with refcounted > bridges, but if they use the devm variant they will put the ref when > they disappear. > > > Then this patch might introduce one new devm_ > > function? Or are drmm_ functions actually being used to store data in > > the drmm-managed memory? > > Which devm function are you thinking about? Sorry, I'm not following > here. drmm_of_get_bridge() / devm_of_get_bridge(). I have a feeling (which of course might be wrong), that they were used somewhat randomly in some cases. > > Luca > > -- > Luca Ceresoli, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hi Dmitry, On Fri, 7 Feb 2025 21:57:30 +0200 Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > On Fri, Feb 07, 2025 at 11:44:01AM +0100, Luca Ceresoli wrote: > > On Fri, 7 Feb 2025 05:17:43 +0200 > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > > > On Thu, Feb 06, 2025 at 07:14:30PM +0100, Luca Ceresoli wrote: > > > > Add a devm/drmm action to these functions so the bridge reference is > > > > dropped automatically when the caller is removed. > > > > > > I think the get() should go to the underlying of_drm_bridge_find() function. > > > > It is done in the following patch. > > > > Indeed I could swap patches 15 and 16 for clarity. Or I could squash > > together patches 14+15+16, as they are various parts or the refcounted > > bridge implementation, but I felt like keeping them separated would > > help reviewing. > > I think most of refcounting should be introduced as a single patch, > otherwise you risk having memory leaks or disappearing bridges. In principle there is no need to add all of this atomically in a single commit, provided that all patches adding refcounting in the infrastructure code is applied before patches to drivers using refcounting. > > > Also it really feels like it's an overkill to keep the wrappers. After > > > getting bridge being handled by the panel code would it be possible to > > > drop all of them? > > > > Do you mean having only drm_of_get_bridge_by_node(), without any devm > > or drmm variant? I'm not sure it is a good idea. Most DRM code (well, > > all of it, technically) is currently unable of working with refcounted > > bridges, but if they use the devm variant they will put the ref when > > they disappear. > > > > > Then this patch might introduce one new devm_ > > > function? Or are drmm_ functions actually being used to store data in > > > the drmm-managed memory? > > > > Which devm function are you thinking about? Sorry, I'm not following > > here. > > drmm_of_get_bridge() / devm_of_get_bridge(). I have a feeling (which of > course might be wrong), that they were used somewhat randomly in some > cases. Again not sure I get you, sorry. What's wrong in devm_drm_of_get_bridge(), and what would the new function you proposed be doing? I think a couple lines of pseudocode would clarify this. Luca
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index f694b32ca59cb91c32846bc00b43360df41cc1ad..497ec06dfcb05ab5dee8ea5e8f1eafb9c2807612 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -33,6 +33,7 @@ #include <drm/drm_edid.h> #include <drm/drm_encoder.h> #include <drm/drm_file.h> +#include <drm/drm_managed.h> #include <drm/drm_of.h> #include <drm/drm_print.h> @@ -1459,6 +1460,13 @@ static int of_drm_find_bridge_by_endpoint(const struct device_node *np, return ret; } +static void devm_drm_bridge_put_void(void *data) +{ + struct drm_bridge *bridge = (struct drm_bridge *)data; + + drm_bridge_put(bridge); +} + /** * devm_drm_of_get_bridge - Return next bridge in the chain * @dev: device to tie the bridge lifetime to @@ -1469,6 +1477,10 @@ static int of_drm_find_bridge_by_endpoint(const struct device_node *np, * Given a DT node's port and endpoint number, finds the connected node * and returns the associated bridge if any. * + * The refcount of the returned bridge is incremented, but the caller does + * not have to call drm_bridge_put() when done with the bridge. It will be + * done by devres when @dev is removed. + * * Returns a pointer to the bridge if successful, or an error pointer * otherwise. */ @@ -1483,6 +1495,10 @@ struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, if (ret) return ERR_PTR(ret); + ret = devm_add_action_or_reset(dev, devm_drm_bridge_put_void, bridge); + if (ret) + return ERR_PTR(ret); + return bridge; } EXPORT_SYMBOL(devm_drm_of_get_bridge); @@ -1497,6 +1513,10 @@ EXPORT_SYMBOL(devm_drm_of_get_bridge); * graph link search is not enough, e.g. for drivers that need to support * panels described only as subnodes. * + * The refcount of the returned bridge is incremented, but the caller does + * not have to call drm_bridge_put() when done with the bridge. It will be + * done by devres when @dev is removed. + * * RETURNS: * A pointer to the bridge if successful, or an error pointer otherwise. */ @@ -1513,10 +1533,21 @@ struct drm_bridge *devm_drm_of_get_bridge_by_node(struct device *dev, if (!bridge) return ERR_PTR(-ENODEV); + ret = devm_add_action_or_reset(dev, devm_drm_bridge_put_void, bridge); + if (ret) + return ERR_PTR(ret); + return bridge; } EXPORT_SYMBOL(devm_drm_of_get_bridge_by_node); +static void drmm_bridge_put_void(struct drm_device *drm, void *ptr) +{ + struct drm_bridge *bridge = ptr; + + drm_bridge_put(bridge); +} + /** * drmm_of_get_bridge - Return next bridge in the chain * @drm: device to tie the bridge lifetime to @@ -1527,6 +1558,10 @@ EXPORT_SYMBOL(devm_drm_of_get_bridge_by_node); * Given a DT node's port and endpoint number, finds the connected node * and returns the associated bridge if any. * + * The refcount of the returned bridge is incremented, but the caller does + * not have to call drm_bridge_put() when done with the bridge. It will be + * done by drmm when @dev is removed. + * * Returns a drmm managed pointer to the bridge if successful, or an error * pointer otherwise. */ @@ -1541,6 +1576,10 @@ struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, if (ret) return ERR_PTR(ret); + ret = drmm_add_action_or_reset(drm, drmm_bridge_put_void, bridge); + if (ret) + return ERR_PTR(ret); + return bridge; } EXPORT_SYMBOL(drmm_of_get_bridge);
Add a devm/drmm action to these functions so the bridge reference is dropped automatically when the caller is removed. Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- This patch was added in v6. --- drivers/gpu/drm/drm_bridge.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)