| Message ID | 20180504135212.26977-27-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04.05.2018 15:52, Peter Rosin wrote: > If the bridge supplier is unbound, this will bring the bridge consumer > down along with the bridge. Thus, there will no longer linger any > dangling pointers from the bridge consumer (the drm_device) to some > non-existent bridge supplier. I understand rationales behind this patch, but it is another step into making drm_dev one big driver with subcomponents, where drm will work only if every subcomponent is working/loaded. Do we need to go this way? In case of many platforms such approach results in display turned on very late on boot for example due to late initialization of some regulator exposed by some i2c device, which is used by hdmi bridge. And this hdmi bridge is just to provide alternative(rarely used) display path, the main display path would work anyway. So the main question to drm maintainers is about evolution of bridges, if drm_bridges should become mandatory components of drm device or they could be added/removed dynamically? Regards Andrzej > > Signed-off-by: Peter Rosin <peda@axentia.se> > --- > drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ > include/drm/drm_bridge.h | 2 ++ > 2 files changed, 20 insertions(+) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 78d186b6831b..0259f0a3ff27 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -26,6 +26,7 @@ > #include <linux/mutex.h> > > #include <drm/drm_bridge.h> > +#include <drm/drm_device.h> > #include <drm/drm_encoder.h> > > #include "drm_crtc_internal.h" > @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > if (bridge->dev) > return -EBUSY; > > + if (encoder->dev->dev != bridge->odev) { > + bridge->link = device_link_add(encoder->dev->dev, > + bridge->odev, 0); > + if (!bridge->link) { > + dev_err(bridge->odev, "failed to link bridge to %s\n", > + dev_name(encoder->dev->dev)); > + return -EINVAL; > + } > + } > + > bridge->dev = encoder->dev; > bridge->encoder = encoder; > > if (bridge->funcs->attach) { > ret = bridge->funcs->attach(bridge); > if (ret < 0) { > + if (bridge->link) > + device_link_del(bridge->link); > + bridge->link = NULL; > bridge->dev = NULL; > bridge->encoder = NULL; > return ret; > @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) > if (bridge->funcs->detach) > bridge->funcs->detach(bridge); > > + if (bridge->link) > + device_link_del(bridge->link); > + bridge->link = NULL; > + > bridge->dev = NULL; > } > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index b656e505d11e..804189c63a4c 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -261,6 +261,7 @@ struct drm_bridge_timings { > * @list: to keep track of all added bridges > * @timings: the timing specification for the bridge, if any (may > * be NULL) > + * @link: drm consumer <-> bridge supplier > * @funcs: control functions > * @driver_private: pointer to the bridge driver's internal context > */ > @@ -271,6 +272,7 @@ struct drm_bridge { > struct drm_bridge *next; > struct list_head list; > const struct drm_bridge_timings *timings; > + struct device_link *link; > > const struct drm_bridge_funcs *funcs; > void *driver_private;
On 2018-05-07 14:59, Andrzej Hajda wrote: > On 04.05.2018 15:52, Peter Rosin wrote: >> If the bridge supplier is unbound, this will bring the bridge consumer >> down along with the bridge. Thus, there will no longer linger any >> dangling pointers from the bridge consumer (the drm_device) to some >> non-existent bridge supplier. > > I understand rationales behind this patch, but it is another step into > making drm_dev one big driver with subcomponents, where drm will work > only if every subcomponent is working/loaded. The step is very small IMHO. Just a device-link, which is very easy to remove once whatever other solution is ready. > Do we need to go this way? If the drivers expect the parts to be there, and there is no other safety net in place if they are not, what is the (short-term) alternative? > In case of many platforms such approach results in display turned on > very late on boot for example due to late initialization of some > regulator exposed by some i2c device, which is used by hdmi bridge. And > this hdmi bridge is just to provide alternative(rarely used) display > path, the main display path would work anyway. This patch does not contribute to any late init and any such delay is not affected by this. At all. > So the main question to drm maintainers is about evolution of bridges, > if drm_bridges should become mandatory components of drm device or they > could be added/removed dynamically? That is a much bigger question than this patch/series. Conflating the two is not fair IMHO. You could run this very same argument for every driver that gets added, since any additional driver will just make it harder to make everything dynamic. Should we stop development right away? Besides, as long as the drm devices are in fact acting as big static drivers (built from smaller parts), this should be considered a bug-fix that will prevent dereference of stale pointers. Or will some other solution appear and magically make all bridges and drm drivers capable of dynamic reconfiguration in the next few weeks? Yeah, right... Cheers, Peter > Regards > Andrzej > > >> >> Signed-off-by: Peter Rosin <peda@axentia.se> >> --- >> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ >> include/drm/drm_bridge.h | 2 ++ >> 2 files changed, 20 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >> index 78d186b6831b..0259f0a3ff27 100644 >> --- a/drivers/gpu/drm/drm_bridge.c >> +++ b/drivers/gpu/drm/drm_bridge.c >> @@ -26,6 +26,7 @@ >> #include <linux/mutex.h> >> >> #include <drm/drm_bridge.h> >> +#include <drm/drm_device.h> >> #include <drm/drm_encoder.h> >> >> #include "drm_crtc_internal.h" >> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, >> if (bridge->dev) >> return -EBUSY; >> >> + if (encoder->dev->dev != bridge->odev) { >> + bridge->link = device_link_add(encoder->dev->dev, >> + bridge->odev, 0); >> + if (!bridge->link) { >> + dev_err(bridge->odev, "failed to link bridge to %s\n", >> + dev_name(encoder->dev->dev)); >> + return -EINVAL; >> + } >> + } >> + >> bridge->dev = encoder->dev; >> bridge->encoder = encoder; >> >> if (bridge->funcs->attach) { >> ret = bridge->funcs->attach(bridge); >> if (ret < 0) { >> + if (bridge->link) >> + device_link_del(bridge->link); >> + bridge->link = NULL; >> bridge->dev = NULL; >> bridge->encoder = NULL; >> return ret; >> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) >> if (bridge->funcs->detach) >> bridge->funcs->detach(bridge); >> >> + if (bridge->link) >> + device_link_del(bridge->link); >> + bridge->link = NULL; >> + >> bridge->dev = NULL; >> } >> >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >> index b656e505d11e..804189c63a4c 100644 >> --- a/include/drm/drm_bridge.h >> +++ b/include/drm/drm_bridge.h >> @@ -261,6 +261,7 @@ struct drm_bridge_timings { >> * @list: to keep track of all added bridges >> * @timings: the timing specification for the bridge, if any (may >> * be NULL) >> + * @link: drm consumer <-> bridge supplier >> * @funcs: control functions >> * @driver_private: pointer to the bridge driver's internal context >> */ >> @@ -271,6 +272,7 @@ struct drm_bridge { >> struct drm_bridge *next; >> struct list_head list; >> const struct drm_bridge_timings *timings; >> + struct device_link *link; >> >> const struct drm_bridge_funcs *funcs; >> void *driver_private; > >
On Mon, May 07, 2018 at 02:59:45PM +0200, Andrzej Hajda wrote: > On 04.05.2018 15:52, Peter Rosin wrote: > > If the bridge supplier is unbound, this will bring the bridge consumer > > down along with the bridge. Thus, there will no longer linger any > > dangling pointers from the bridge consumer (the drm_device) to some > > non-existent bridge supplier. > > I understand rationales behind this patch, but it is another step into > making drm_dev one big driver with subcomponents, where drm will work > only if every subcomponent is working/loaded. Do we need to go this way? > In case of many platforms such approach results in display turned on > very late on boot for example due to late initialization of some > regulator exposed by some i2c device, which is used by hdmi bridge. And > this hdmi bridge is just to provide alternative(rarely used) display > path, the main display path would work anyway. > > So the main question to drm maintainers is about evolution of bridges, > if drm_bridges should become mandatory components of drm device or they > could be added/removed dynamically? This is already the case. You currently cannot hotplug a drm_bridge, everything must be present. I don't think it makes sense to change that until we have physically hotpluggable drm_bridges in real hardware. I definitely don't want to refcount stuff to work around driver load bonghits on DT platforms, because refcounting is way too hard to get right :-) Afaik there's out-of-tree patches to solve 99% of the driver load fun on DT platforms, but because it's not a 100% solution it's blocked since forever. -Daniel > > Regards > Andrzej > > > > > > Signed-off-by: Peter Rosin <peda@axentia.se> > > --- > > drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ > > include/drm/drm_bridge.h | 2 ++ > > 2 files changed, 20 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > index 78d186b6831b..0259f0a3ff27 100644 > > --- a/drivers/gpu/drm/drm_bridge.c > > +++ b/drivers/gpu/drm/drm_bridge.c > > @@ -26,6 +26,7 @@ > > #include <linux/mutex.h> > > > > #include <drm/drm_bridge.h> > > +#include <drm/drm_device.h> > > #include <drm/drm_encoder.h> > > > > #include "drm_crtc_internal.h" > > @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > > if (bridge->dev) > > return -EBUSY; > > > > + if (encoder->dev->dev != bridge->odev) { > > + bridge->link = device_link_add(encoder->dev->dev, > > + bridge->odev, 0); > > + if (!bridge->link) { > > + dev_err(bridge->odev, "failed to link bridge to %s\n", > > + dev_name(encoder->dev->dev)); > > + return -EINVAL; > > + } > > + } > > + > > bridge->dev = encoder->dev; > > bridge->encoder = encoder; > > > > if (bridge->funcs->attach) { > > ret = bridge->funcs->attach(bridge); > > if (ret < 0) { > > + if (bridge->link) > > + device_link_del(bridge->link); > > + bridge->link = NULL; > > bridge->dev = NULL; > > bridge->encoder = NULL; > > return ret; > > @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) > > if (bridge->funcs->detach) > > bridge->funcs->detach(bridge); > > > > + if (bridge->link) > > + device_link_del(bridge->link); > > + bridge->link = NULL; > > + > > bridge->dev = NULL; > > } > > > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > > index b656e505d11e..804189c63a4c 100644 > > --- a/include/drm/drm_bridge.h > > +++ b/include/drm/drm_bridge.h > > @@ -261,6 +261,7 @@ struct drm_bridge_timings { > > * @list: to keep track of all added bridges > > * @timings: the timing specification for the bridge, if any (may > > * be NULL) > > + * @link: drm consumer <-> bridge supplier > > * @funcs: control functions > > * @driver_private: pointer to the bridge driver's internal context > > */ > > @@ -271,6 +272,7 @@ struct drm_bridge { > > struct drm_bridge *next; > > struct list_head list; > > const struct drm_bridge_timings *timings; > > + struct device_link *link; > > > > const struct drm_bridge_funcs *funcs; > > void *driver_private; > >
On 07.05.2018 15:53, Daniel Vetter wrote: > On Mon, May 07, 2018 at 02:59:45PM +0200, Andrzej Hajda wrote: >> On 04.05.2018 15:52, Peter Rosin wrote: >>> If the bridge supplier is unbound, this will bring the bridge consumer >>> down along with the bridge. Thus, there will no longer linger any >>> dangling pointers from the bridge consumer (the drm_device) to some >>> non-existent bridge supplier. >> I understand rationales behind this patch, but it is another step into >> making drm_dev one big driver with subcomponents, where drm will work >> only if every subcomponent is working/loaded. Do we need to go this way? >> In case of many platforms such approach results in display turned on >> very late on boot for example due to late initialization of some >> regulator exposed by some i2c device, which is used by hdmi bridge. And >> this hdmi bridge is just to provide alternative(rarely used) display >> path, the main display path would work anyway. >> >> So the main question to drm maintainers is about evolution of bridges, >> if drm_bridges should become mandatory components of drm device or they >> could be added/removed dynamically? > This is already the case. You currently cannot hotplug a drm_bridge, > everything must be present. Are you sure? DRM core is changing quite fast, so maybe I have missed something, but AFAIK core calls bridge code only if full display pipeline is created and connector is in connected state. So adding and removing bridges from inactive pipelines should work if coded properly. > I don't think it makes sense to change that > until we have physically hotpluggable drm_bridges in real hardware. But kernel core already assumes that device drivers are hot-pluggable :), even this patch is created to solve issues regarding driver hot unplugging. > I > definitely don't want to refcount stuff to work around driver load > bonghits on DT platforms, because refcounting is way too hard to get right > :-) I am not sure about bridges, but I have successfully (IMO) experimented with hot (un)plugging panel driver, see panel-samsung-s6e8aa0.c driver and exynos_drm_dsi.c, panel driver can be safely plugged/unplugged/replugged without any refcounting (but with help of mipi_dsi attach/detach callbacks, which are not available for non-mipi-dsi drivers). Regards Andrzej > > Afaik there's out-of-tree patches to solve 99% of the driver load fun on > DT platforms, but because it's not a 100% solution it's blocked since > forever. > -Daniel > >> Regards >> Andrzej >> >> >>> Signed-off-by: Peter Rosin <peda@axentia.se> >>> --- >>> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ >>> include/drm/drm_bridge.h | 2 ++ >>> 2 files changed, 20 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >>> index 78d186b6831b..0259f0a3ff27 100644 >>> --- a/drivers/gpu/drm/drm_bridge.c >>> +++ b/drivers/gpu/drm/drm_bridge.c >>> @@ -26,6 +26,7 @@ >>> #include <linux/mutex.h> >>> >>> #include <drm/drm_bridge.h> >>> +#include <drm/drm_device.h> >>> #include <drm/drm_encoder.h> >>> >>> #include "drm_crtc_internal.h" >>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, >>> if (bridge->dev) >>> return -EBUSY; >>> >>> + if (encoder->dev->dev != bridge->odev) { >>> + bridge->link = device_link_add(encoder->dev->dev, >>> + bridge->odev, 0); >>> + if (!bridge->link) { >>> + dev_err(bridge->odev, "failed to link bridge to %s\n", >>> + dev_name(encoder->dev->dev)); >>> + return -EINVAL; >>> + } >>> + } >>> + >>> bridge->dev = encoder->dev; >>> bridge->encoder = encoder; >>> >>> if (bridge->funcs->attach) { >>> ret = bridge->funcs->attach(bridge); >>> if (ret < 0) { >>> + if (bridge->link) >>> + device_link_del(bridge->link); >>> + bridge->link = NULL; >>> bridge->dev = NULL; >>> bridge->encoder = NULL; >>> return ret; >>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) >>> if (bridge->funcs->detach) >>> bridge->funcs->detach(bridge); >>> >>> + if (bridge->link) >>> + device_link_del(bridge->link); >>> + bridge->link = NULL; >>> + >>> bridge->dev = NULL; >>> } >>> >>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >>> index b656e505d11e..804189c63a4c 100644 >>> --- a/include/drm/drm_bridge.h >>> +++ b/include/drm/drm_bridge.h >>> @@ -261,6 +261,7 @@ struct drm_bridge_timings { >>> * @list: to keep track of all added bridges >>> * @timings: the timing specification for the bridge, if any (may >>> * be NULL) >>> + * @link: drm consumer <-> bridge supplier >>> * @funcs: control functions >>> * @driver_private: pointer to the bridge driver's internal context >>> */ >>> @@ -271,6 +272,7 @@ struct drm_bridge { >>> struct drm_bridge *next; >>> struct list_head list; >>> const struct drm_bridge_timings *timings; >>> + struct device_link *link; >>> >>> const struct drm_bridge_funcs *funcs; >>> void *driver_private; >>
On 07.05.2018 15:43, Peter Rosin wrote: > On 2018-05-07 14:59, Andrzej Hajda wrote: >> On 04.05.2018 15:52, Peter Rosin wrote: >>> If the bridge supplier is unbound, this will bring the bridge consumer >>> down along with the bridge. Thus, there will no longer linger any >>> dangling pointers from the bridge consumer (the drm_device) to some >>> non-existent bridge supplier. >> I understand rationales behind this patch, but it is another step into >> making drm_dev one big driver with subcomponents, where drm will work >> only if every subcomponent is working/loaded. > The step is very small IMHO. Just a device-link, which is very easy to > remove once whatever other solution is ready. > >> Do we need to go this way? > If the drivers expect the parts to be there, and there is no other safety > net in place if they are not, what is the (short-term) alternative? > >> In case of many platforms such approach results in display turned on >> very late on boot for example due to late initialization of some >> regulator exposed by some i2c device, which is used by hdmi bridge. And >> this hdmi bridge is just to provide alternative(rarely used) display >> path, the main display path would work anyway. > This patch does not contribute to any late init and any such delay is not > affected by this. At all. > >> So the main question to drm maintainers is about evolution of bridges, >> if drm_bridges should become mandatory components of drm device or they >> could be added/removed dynamically? > That is a much bigger question than this patch/series. Conflating the > two is not fair IMHO. You could run this very same argument for every > driver that gets added, since any additional driver will just make it > harder to make everything dynamic. Should we stop development right > away? > > Besides, as long as the drm devices are in fact acting as big static > drivers (built from smaller parts), not true > this should be considered a bug-fix > that will prevent dereference of stale pointers. > > Or will some other solution appear and magically make all bridges and > drm drivers capable of dynamic reconfiguration in the next few weeks? > Yeah, right... You are not changing single driver, you are changing framework and it affects all the drivers using it, being more cautious about such patches seems quite natural. Anyway, I have realized that since drm_bridge_detach will remove the link, so with properly written dynamic bridge removal, your patch should not be a blocker. Regards Andrzej > > Cheers, > Peter > >> Regards >> Andrzej >> >> >>> Signed-off-by: Peter Rosin <peda@axentia.se> >>> --- >>> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ >>> include/drm/drm_bridge.h | 2 ++ >>> 2 files changed, 20 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >>> index 78d186b6831b..0259f0a3ff27 100644 >>> --- a/drivers/gpu/drm/drm_bridge.c >>> +++ b/drivers/gpu/drm/drm_bridge.c >>> @@ -26,6 +26,7 @@ >>> #include <linux/mutex.h> >>> >>> #include <drm/drm_bridge.h> >>> +#include <drm/drm_device.h> >>> #include <drm/drm_encoder.h> >>> >>> #include "drm_crtc_internal.h" >>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, >>> if (bridge->dev) >>> return -EBUSY; >>> >>> + if (encoder->dev->dev != bridge->odev) { >>> + bridge->link = device_link_add(encoder->dev->dev, >>> + bridge->odev, 0); >>> + if (!bridge->link) { >>> + dev_err(bridge->odev, "failed to link bridge to %s\n", >>> + dev_name(encoder->dev->dev)); >>> + return -EINVAL; >>> + } >>> + } >>> + >>> bridge->dev = encoder->dev; >>> bridge->encoder = encoder; >>> >>> if (bridge->funcs->attach) { >>> ret = bridge->funcs->attach(bridge); >>> if (ret < 0) { >>> + if (bridge->link) >>> + device_link_del(bridge->link); >>> + bridge->link = NULL; >>> bridge->dev = NULL; >>> bridge->encoder = NULL; >>> return ret; >>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) >>> if (bridge->funcs->detach) >>> bridge->funcs->detach(bridge); >>> >>> + if (bridge->link) >>> + device_link_del(bridge->link); >>> + bridge->link = NULL; >>> + >>> bridge->dev = NULL; >>> } >>> >>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >>> index b656e505d11e..804189c63a4c 100644 >>> --- a/include/drm/drm_bridge.h >>> +++ b/include/drm/drm_bridge.h >>> @@ -261,6 +261,7 @@ struct drm_bridge_timings { >>> * @list: to keep track of all added bridges >>> * @timings: the timing specification for the bridge, if any (may >>> * be NULL) >>> + * @link: drm consumer <-> bridge supplier >>> * @funcs: control functions >>> * @driver_private: pointer to the bridge driver's internal context >>> */ >>> @@ -271,6 +272,7 @@ struct drm_bridge { >>> struct drm_bridge *next; >>> struct list_head list; >>> const struct drm_bridge_timings *timings; >>> + struct device_link *link; >>> >>> const struct drm_bridge_funcs *funcs; >>> void *driver_private; >> > > >
On 04.05.2018 15:52, Peter Rosin wrote: > If the bridge supplier is unbound, this will bring the bridge consumer > down along with the bridge. Thus, there will no longer linger any > dangling pointers from the bridge consumer (the drm_device) to some > non-existent bridge supplier. > > Signed-off-by: Peter Rosin <peda@axentia.se> > --- > drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ > include/drm/drm_bridge.h | 2 ++ > 2 files changed, 20 insertions(+) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 78d186b6831b..0259f0a3ff27 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -26,6 +26,7 @@ > #include <linux/mutex.h> > > #include <drm/drm_bridge.h> > +#include <drm/drm_device.h> > #include <drm/drm_encoder.h> > > #include "drm_crtc_internal.h" > @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > if (bridge->dev) > return -EBUSY; > > + if (encoder->dev->dev != bridge->odev) { I wonder why device_link_add does not handle this case (self dependency) silently as noop, as it seems to be a correct behavior. > + bridge->link = device_link_add(encoder->dev->dev, > + bridge->odev, 0); > + if (!bridge->link) { > + dev_err(bridge->odev, "failed to link bridge to %s\n", > + dev_name(encoder->dev->dev)); > + return -EINVAL; > + } > + } > + > bridge->dev = encoder->dev; > bridge->encoder = encoder; > > if (bridge->funcs->attach) { > ret = bridge->funcs->attach(bridge); > if (ret < 0) { > + if (bridge->link) > + device_link_del(bridge->link); > + bridge->link = NULL; > bridge->dev = NULL; > bridge->encoder = NULL; > return ret; > @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) > if (bridge->funcs->detach) > bridge->funcs->detach(bridge); > > + if (bridge->link) > + device_link_del(bridge->link); > + bridge->link = NULL; > + > bridge->dev = NULL; > } > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index b656e505d11e..804189c63a4c 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -261,6 +261,7 @@ struct drm_bridge_timings { > * @list: to keep track of all added bridges > * @timings: the timing specification for the bridge, if any (may > * be NULL) > + * @link: drm consumer <-> bridge supplier Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer to the bridge" would be better. Anyway: Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej > * @funcs: control functions > * @driver_private: pointer to the bridge driver's internal context > */ > @@ -271,6 +272,7 @@ struct drm_bridge { > struct drm_bridge *next; > struct list_head list; > const struct drm_bridge_timings *timings; > + struct device_link *link; > > const struct drm_bridge_funcs *funcs; > void *driver_private;
On 2018-05-10 10:10, Andrzej Hajda wrote: > On 04.05.2018 15:52, Peter Rosin wrote: >> If the bridge supplier is unbound, this will bring the bridge consumer >> down along with the bridge. Thus, there will no longer linger any >> dangling pointers from the bridge consumer (the drm_device) to some >> non-existent bridge supplier. >> >> Signed-off-by: Peter Rosin <peda@axentia.se> >> --- >> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ >> include/drm/drm_bridge.h | 2 ++ >> 2 files changed, 20 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >> index 78d186b6831b..0259f0a3ff27 100644 >> --- a/drivers/gpu/drm/drm_bridge.c >> +++ b/drivers/gpu/drm/drm_bridge.c >> @@ -26,6 +26,7 @@ >> #include <linux/mutex.h> >> >> #include <drm/drm_bridge.h> >> +#include <drm/drm_device.h> >> #include <drm/drm_encoder.h> >> >> #include "drm_crtc_internal.h" >> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, >> if (bridge->dev) >> return -EBUSY; >> >> + if (encoder->dev->dev != bridge->odev) { > > I wonder why device_link_add does not handle this case (self dependency) > silently as noop, as it seems to be a correct behavior. It's kind-of a silly corner-case though, so perfectly understandable that it isn't handled. >> + bridge->link = device_link_add(encoder->dev->dev, >> + bridge->odev, 0); >> + if (!bridge->link) { >> + dev_err(bridge->odev, "failed to link bridge to %s\n", >> + dev_name(encoder->dev->dev)); >> + return -EINVAL; >> + } >> + } >> + >> bridge->dev = encoder->dev; >> bridge->encoder = encoder; >> >> if (bridge->funcs->attach) { >> ret = bridge->funcs->attach(bridge); >> if (ret < 0) { >> + if (bridge->link) >> + device_link_del(bridge->link); >> + bridge->link = NULL; >> bridge->dev = NULL; >> bridge->encoder = NULL; >> return ret; >> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) >> if (bridge->funcs->detach) >> bridge->funcs->detach(bridge); >> >> + if (bridge->link) >> + device_link_del(bridge->link); >> + bridge->link = NULL; >> + >> bridge->dev = NULL; >> } >> >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >> index b656e505d11e..804189c63a4c 100644 >> --- a/include/drm/drm_bridge.h >> +++ b/include/drm/drm_bridge.h >> @@ -261,6 +261,7 @@ struct drm_bridge_timings { >> * @list: to keep track of all added bridges >> * @timings: the timing specification for the bridge, if any (may >> * be NULL) >> + * @link: drm consumer <-> bridge supplier > > Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer > to the bridge" would be better. I meant "<->" to indicate that the link is bidirectional, not that the relationship is in any way symmetric. I wasn't aware of any implication of a symmetric relationship when using "<->", do you have a reference? But I guess the different arrow notations in math are somewhat overloaded and that someone at some point must have used "<->" to indicate a symmetric relationship... > Anyway: > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> Thanks! Cheers, Peter > -- > Regards > Andrzej > >> * @funcs: control functions >> * @driver_private: pointer to the bridge driver's internal context >> */ >> @@ -271,6 +272,7 @@ struct drm_bridge { >> struct drm_bridge *next; >> struct list_head list; >> const struct drm_bridge_timings *timings; >> + struct device_link *link; >> >> const struct drm_bridge_funcs *funcs; >> void *driver_private; > >
On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote: > On 2018-05-10 10:10, Andrzej Hajda wrote: > > On 04.05.2018 15:52, Peter Rosin wrote: > >> If the bridge supplier is unbound, this will bring the bridge consumer > >> down along with the bridge. Thus, there will no longer linger any > >> dangling pointers from the bridge consumer (the drm_device) to some > >> non-existent bridge supplier. > >> > >> Signed-off-by: Peter Rosin <peda@axentia.se> > >> --- > >> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ > >> include/drm/drm_bridge.h | 2 ++ > >> 2 files changed, 20 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > >> index 78d186b6831b..0259f0a3ff27 100644 > >> --- a/drivers/gpu/drm/drm_bridge.c > >> +++ b/drivers/gpu/drm/drm_bridge.c > >> @@ -26,6 +26,7 @@ > >> #include <linux/mutex.h> > >> > >> #include <drm/drm_bridge.h> > >> +#include <drm/drm_device.h> > >> #include <drm/drm_encoder.h> > >> > >> #include "drm_crtc_internal.h" > >> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > >> if (bridge->dev) > >> return -EBUSY; > >> > >> + if (encoder->dev->dev != bridge->odev) { > > > > I wonder why device_link_add does not handle this case (self dependency) > > silently as noop, as it seems to be a correct behavior. > > It's kind-of a silly corner-case though, so perfectly understandable > that it isn't handled. > > >> + bridge->link = device_link_add(encoder->dev->dev, > >> + bridge->odev, 0); > >> + if (!bridge->link) { > >> + dev_err(bridge->odev, "failed to link bridge to %s\n", > >> + dev_name(encoder->dev->dev)); > >> + return -EINVAL; > >> + } > >> + } > >> + > >> bridge->dev = encoder->dev; > >> bridge->encoder = encoder; > >> > >> if (bridge->funcs->attach) { > >> ret = bridge->funcs->attach(bridge); > >> if (ret < 0) { > >> + if (bridge->link) > >> + device_link_del(bridge->link); > >> + bridge->link = NULL; > >> bridge->dev = NULL; > >> bridge->encoder = NULL; > >> return ret; > >> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) > >> if (bridge->funcs->detach) > >> bridge->funcs->detach(bridge); > >> > >> + if (bridge->link) > >> + device_link_del(bridge->link); > >> + bridge->link = NULL; > >> + > >> bridge->dev = NULL; > >> } > >> > >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > >> index b656e505d11e..804189c63a4c 100644 > >> --- a/include/drm/drm_bridge.h > >> +++ b/include/drm/drm_bridge.h > >> @@ -261,6 +261,7 @@ struct drm_bridge_timings { > >> * @list: to keep track of all added bridges > >> * @timings: the timing specification for the bridge, if any (may > >> * be NULL) > >> + * @link: drm consumer <-> bridge supplier > > > > Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer > > to the bridge" would be better. > > I meant "<->" to indicate that the link is bidirectional, not that the > relationship is in any way symmetric. I wasn't aware of any implication > of a symmetric relationship when using "<->", do you have a reference? > But I guess the different arrow notations in math are somewhat overloaded > and that someone at some point must have used "<->" to indicate a > symmetric relationship... Yeah I agree with Andrzej here, for me <-> implies a symmetric relationship. Spelling it out like Andrzej suggested sounds like the better idea. -Daniel > > > Anyway: > > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > > Thanks! > > Cheers, > Peter > > > -- > > Regards > > Andrzej > > > >> * @funcs: control functions > >> * @driver_private: pointer to the bridge driver's internal context > >> */ > >> @@ -271,6 +272,7 @@ struct drm_bridge { > >> struct drm_bridge *next; > >> struct list_head list; > >> const struct drm_bridge_timings *timings; > >> + struct device_link *link; > >> > >> const struct drm_bridge_funcs *funcs; > >> void *driver_private; > > > > >
On 2018-05-14 18:28, Daniel Vetter wrote: > On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote: >> On 2018-05-10 10:10, Andrzej Hajda wrote: >>> On 04.05.2018 15:52, Peter Rosin wrote: >>>> If the bridge supplier is unbound, this will bring the bridge consumer >>>> down along with the bridge. Thus, there will no longer linger any >>>> dangling pointers from the bridge consumer (the drm_device) to some >>>> non-existent bridge supplier. >>>> >>>> Signed-off-by: Peter Rosin <peda@axentia.se> >>>> --- >>>> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ >>>> include/drm/drm_bridge.h | 2 ++ >>>> 2 files changed, 20 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >>>> index 78d186b6831b..0259f0a3ff27 100644 >>>> --- a/drivers/gpu/drm/drm_bridge.c >>>> +++ b/drivers/gpu/drm/drm_bridge.c >>>> @@ -26,6 +26,7 @@ >>>> #include <linux/mutex.h> >>>> >>>> #include <drm/drm_bridge.h> >>>> +#include <drm/drm_device.h> >>>> #include <drm/drm_encoder.h> >>>> >>>> #include "drm_crtc_internal.h" >>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, >>>> if (bridge->dev) >>>> return -EBUSY; >>>> >>>> + if (encoder->dev->dev != bridge->odev) { >>> >>> I wonder why device_link_add does not handle this case (self dependency) >>> silently as noop, as it seems to be a correct behavior. >> >> It's kind-of a silly corner-case though, so perfectly understandable >> that it isn't handled. >> >>>> + bridge->link = device_link_add(encoder->dev->dev, >>>> + bridge->odev, 0); >>>> + if (!bridge->link) { >>>> + dev_err(bridge->odev, "failed to link bridge to %s\n", >>>> + dev_name(encoder->dev->dev)); >>>> + return -EINVAL; >>>> + } >>>> + } >>>> + >>>> bridge->dev = encoder->dev; >>>> bridge->encoder = encoder; >>>> >>>> if (bridge->funcs->attach) { >>>> ret = bridge->funcs->attach(bridge); >>>> if (ret < 0) { >>>> + if (bridge->link) >>>> + device_link_del(bridge->link); >>>> + bridge->link = NULL; >>>> bridge->dev = NULL; >>>> bridge->encoder = NULL; >>>> return ret; >>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) >>>> if (bridge->funcs->detach) >>>> bridge->funcs->detach(bridge); >>>> >>>> + if (bridge->link) >>>> + device_link_del(bridge->link); >>>> + bridge->link = NULL; >>>> + >>>> bridge->dev = NULL; >>>> } >>>> >>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >>>> index b656e505d11e..804189c63a4c 100644 >>>> --- a/include/drm/drm_bridge.h >>>> +++ b/include/drm/drm_bridge.h >>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings { >>>> * @list: to keep track of all added bridges >>>> * @timings: the timing specification for the bridge, if any (may >>>> * be NULL) >>>> + * @link: drm consumer <-> bridge supplier >>> >>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer >>> to the bridge" would be better. >> >> I meant "<->" to indicate that the link is bidirectional, not that the >> relationship is in any way symmetric. I wasn't aware of any implication >> of a symmetric relationship when using "<->", do you have a reference? >> But I guess the different arrow notations in math are somewhat overloaded >> and that someone at some point must have used "<->" to indicate a >> symmetric relationship... > > Yeah I agree with Andrzej here, for me <-> implies a symmetric > relationship. Spelling it out like Andrzej suggested sounds like the > better idea. > -Daniel Ok, I guess that means I have to do a v3 after all. Or can this trivial documentation update be done by the committer? I hate to spam everyone with another volley... Or perhaps I should squash patches 2-23 that are all rather similar and mechanic? I separated them to allow for easier review from individual driver maintainers, but that didn't seem to happen anyway... Cheers, Peter > >> >>> Anyway: >>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >> >> Thanks! >> >> Cheers, >> Peter >> >>> -- >>> Regards >>> Andrzej >>> >>>> * @funcs: control functions >>>> * @driver_private: pointer to the bridge driver's internal context >>>> */ >>>> @@ -271,6 +272,7 @@ struct drm_bridge { >>>> struct drm_bridge *next; >>>> struct list_head list; >>>> const struct drm_bridge_timings *timings; >>>> + struct device_link *link; >>>> >>>> const struct drm_bridge_funcs *funcs; >>>> void *driver_private; >>> >>> >> >
On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <peda@axentia.se> wrote: > On 2018-05-14 18:28, Daniel Vetter wrote: >> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote: >>> On 2018-05-10 10:10, Andrzej Hajda wrote: >>>> On 04.05.2018 15:52, Peter Rosin wrote: >>>>> If the bridge supplier is unbound, this will bring the bridge consumer >>>>> down along with the bridge. Thus, there will no longer linger any >>>>> dangling pointers from the bridge consumer (the drm_device) to some >>>>> non-existent bridge supplier. >>>>> >>>>> Signed-off-by: Peter Rosin <peda@axentia.se> >>>>> --- >>>>> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ >>>>> include/drm/drm_bridge.h | 2 ++ >>>>> 2 files changed, 20 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >>>>> index 78d186b6831b..0259f0a3ff27 100644 >>>>> --- a/drivers/gpu/drm/drm_bridge.c >>>>> +++ b/drivers/gpu/drm/drm_bridge.c >>>>> @@ -26,6 +26,7 @@ >>>>> #include <linux/mutex.h> >>>>> >>>>> #include <drm/drm_bridge.h> >>>>> +#include <drm/drm_device.h> >>>>> #include <drm/drm_encoder.h> >>>>> >>>>> #include "drm_crtc_internal.h" >>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, >>>>> if (bridge->dev) >>>>> return -EBUSY; >>>>> >>>>> + if (encoder->dev->dev != bridge->odev) { >>>> >>>> I wonder why device_link_add does not handle this case (self dependency) >>>> silently as noop, as it seems to be a correct behavior. >>> >>> It's kind-of a silly corner-case though, so perfectly understandable >>> that it isn't handled. >>> >>>>> + bridge->link = device_link_add(encoder->dev->dev, >>>>> + bridge->odev, 0); >>>>> + if (!bridge->link) { >>>>> + dev_err(bridge->odev, "failed to link bridge to %s\n", >>>>> + dev_name(encoder->dev->dev)); >>>>> + return -EINVAL; >>>>> + } >>>>> + } >>>>> + >>>>> bridge->dev = encoder->dev; >>>>> bridge->encoder = encoder; >>>>> >>>>> if (bridge->funcs->attach) { >>>>> ret = bridge->funcs->attach(bridge); >>>>> if (ret < 0) { >>>>> + if (bridge->link) >>>>> + device_link_del(bridge->link); >>>>> + bridge->link = NULL; >>>>> bridge->dev = NULL; >>>>> bridge->encoder = NULL; >>>>> return ret; >>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) >>>>> if (bridge->funcs->detach) >>>>> bridge->funcs->detach(bridge); >>>>> >>>>> + if (bridge->link) >>>>> + device_link_del(bridge->link); >>>>> + bridge->link = NULL; >>>>> + >>>>> bridge->dev = NULL; >>>>> } >>>>> >>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >>>>> index b656e505d11e..804189c63a4c 100644 >>>>> --- a/include/drm/drm_bridge.h >>>>> +++ b/include/drm/drm_bridge.h >>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings { >>>>> * @list: to keep track of all added bridges >>>>> * @timings: the timing specification for the bridge, if any (may >>>>> * be NULL) >>>>> + * @link: drm consumer <-> bridge supplier >>>> >>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer >>>> to the bridge" would be better. >>> >>> I meant "<->" to indicate that the link is bidirectional, not that the >>> relationship is in any way symmetric. I wasn't aware of any implication >>> of a symmetric relationship when using "<->", do you have a reference? >>> But I guess the different arrow notations in math are somewhat overloaded >>> and that someone at some point must have used "<->" to indicate a >>> symmetric relationship... >> >> Yeah I agree with Andrzej here, for me <-> implies a symmetric >> relationship. Spelling it out like Andrzej suggested sounds like the >> better idea. >> -Daniel > > Ok, I guess that means I have to do a v3 after all. Or can this > trivial documentation update be done by the committer? I hate to > spam everyone with another volley... > > Or perhaps I should squash patches 2-23 that are all rather similar > and mechanic? I separated them to allow for easier review from > individual driver maintainers, but that didn't seem to happen > anyway... Do another volley of the full set, or in-reply-to to just replace the patch that needs to be respun (but some people don't like that). When resending just make sure you're picking up all the acks/r-bs you have already. -Daniel > Cheers, > Peter > >> >>> >>>> Anyway: >>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >>> >>> Thanks! >>> >>> Cheers, >>> Peter >>> >>>> -- >>>> Regards >>>> Andrzej >>>> >>>>> * @funcs: control functions >>>>> * @driver_private: pointer to the bridge driver's internal context >>>>> */ >>>>> @@ -271,6 +272,7 @@ struct drm_bridge { >>>>> struct drm_bridge *next; >>>>> struct list_head list; >>>>> const struct drm_bridge_timings *timings; >>>>> + struct device_link *link; >>>>> >>>>> const struct drm_bridge_funcs *funcs; >>>>> void *driver_private; >>>> >>>> >>> >> > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2018-05-15 12:22, Daniel Vetter wrote: > On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <peda@axentia.se> wrote: >> On 2018-05-14 18:28, Daniel Vetter wrote: >>> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote: >>>> On 2018-05-10 10:10, Andrzej Hajda wrote: >>>>> On 04.05.2018 15:52, Peter Rosin wrote: >>>>>> If the bridge supplier is unbound, this will bring the bridge consumer >>>>>> down along with the bridge. Thus, there will no longer linger any >>>>>> dangling pointers from the bridge consumer (the drm_device) to some >>>>>> non-existent bridge supplier. >>>>>> >>>>>> Signed-off-by: Peter Rosin <peda@axentia.se> >>>>>> --- >>>>>> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ >>>>>> include/drm/drm_bridge.h | 2 ++ >>>>>> 2 files changed, 20 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >>>>>> index 78d186b6831b..0259f0a3ff27 100644 >>>>>> --- a/drivers/gpu/drm/drm_bridge.c >>>>>> +++ b/drivers/gpu/drm/drm_bridge.c >>>>>> @@ -26,6 +26,7 @@ >>>>>> #include <linux/mutex.h> >>>>>> >>>>>> #include <drm/drm_bridge.h> >>>>>> +#include <drm/drm_device.h> >>>>>> #include <drm/drm_encoder.h> >>>>>> >>>>>> #include "drm_crtc_internal.h" >>>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, >>>>>> if (bridge->dev) >>>>>> return -EBUSY; >>>>>> >>>>>> + if (encoder->dev->dev != bridge->odev) { >>>>> >>>>> I wonder why device_link_add does not handle this case (self dependency) >>>>> silently as noop, as it seems to be a correct behavior. >>>> >>>> It's kind-of a silly corner-case though, so perfectly understandable >>>> that it isn't handled. >>>> >>>>>> + bridge->link = device_link_add(encoder->dev->dev, >>>>>> + bridge->odev, 0); >>>>>> + if (!bridge->link) { >>>>>> + dev_err(bridge->odev, "failed to link bridge to %s\n", >>>>>> + dev_name(encoder->dev->dev)); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> bridge->dev = encoder->dev; >>>>>> bridge->encoder = encoder; >>>>>> >>>>>> if (bridge->funcs->attach) { >>>>>> ret = bridge->funcs->attach(bridge); >>>>>> if (ret < 0) { >>>>>> + if (bridge->link) >>>>>> + device_link_del(bridge->link); >>>>>> + bridge->link = NULL; >>>>>> bridge->dev = NULL; >>>>>> bridge->encoder = NULL; >>>>>> return ret; >>>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) >>>>>> if (bridge->funcs->detach) >>>>>> bridge->funcs->detach(bridge); >>>>>> >>>>>> + if (bridge->link) >>>>>> + device_link_del(bridge->link); >>>>>> + bridge->link = NULL; >>>>>> + >>>>>> bridge->dev = NULL; >>>>>> } >>>>>> >>>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >>>>>> index b656e505d11e..804189c63a4c 100644 >>>>>> --- a/include/drm/drm_bridge.h >>>>>> +++ b/include/drm/drm_bridge.h >>>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings { >>>>>> * @list: to keep track of all added bridges >>>>>> * @timings: the timing specification for the bridge, if any (may >>>>>> * be NULL) >>>>>> + * @link: drm consumer <-> bridge supplier >>>>> >>>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer >>>>> to the bridge" would be better. >>>> >>>> I meant "<->" to indicate that the link is bidirectional, not that the >>>> relationship is in any way symmetric. I wasn't aware of any implication >>>> of a symmetric relationship when using "<->", do you have a reference? >>>> But I guess the different arrow notations in math are somewhat overloaded >>>> and that someone at some point must have used "<->" to indicate a >>>> symmetric relationship... >>> >>> Yeah I agree with Andrzej here, for me <-> implies a symmetric >>> relationship. Spelling it out like Andrzej suggested sounds like the >>> better idea. >>> -Daniel >> >> Ok, I guess that means I have to do a v3 after all. Or can this >> trivial documentation update be done by the committer? I hate to >> spam everyone with another volley... >> >> Or perhaps I should squash patches 2-23 that are all rather similar >> and mechanic? I separated them to allow for easier review from >> individual driver maintainers, but that didn't seem to happen >> anyway... > > Do another volley of the full set, or in-reply-to to just replace the > patch that needs to be respun (but some people don't like that). > > When resending just make sure you're picking up all the acks/r-bs you > have already. Right, I always try to do that. One Ack that I did not include in v2 was the one you had on v1 24/24 (i.e. this patch). The reason I did not add your Ack for v2 even on the patch where it obviously applied was that I didn't know if you'd barf on the odev name. But it was (and still is) a bit unclear if that was on Ack on the last patch only, or if it was for the whole series? I think it might have been for the whole series, but I'm not sure and I hate to be a presumptuous idiot... Cheers, Peter > -Daniel >> Cheers, >> Peter >> >>> >>>> >>>>> Anyway: >>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >>>> >>>> Thanks! >>>> >>>> Cheers, >>>> Peter >>>> >>>>> -- >>>>> Regards >>>>> Andrzej >>>>> >>>>>> * @funcs: control functions >>>>>> * @driver_private: pointer to the bridge driver's internal context >>>>>> */ >>>>>> @@ -271,6 +272,7 @@ struct drm_bridge { >>>>>> struct drm_bridge *next; >>>>>> struct list_head list; >>>>>> const struct drm_bridge_timings *timings; >>>>>> + struct device_link *link; >>>>>> >>>>>> const struct drm_bridge_funcs *funcs; >>>>>> void *driver_private; >>>>> >>>>> >>>> >>> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > >
On Tue, May 15, 2018 at 01:09:59PM +0200, Peter Rosin wrote: > On 2018-05-15 12:22, Daniel Vetter wrote: > > On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <peda@axentia.se> wrote: > >> On 2018-05-14 18:28, Daniel Vetter wrote: > >>> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote: > >>>> On 2018-05-10 10:10, Andrzej Hajda wrote: > >>>>> On 04.05.2018 15:52, Peter Rosin wrote: > >>>>>> If the bridge supplier is unbound, this will bring the bridge consumer > >>>>>> down along with the bridge. Thus, there will no longer linger any > >>>>>> dangling pointers from the bridge consumer (the drm_device) to some > >>>>>> non-existent bridge supplier. > >>>>>> > >>>>>> Signed-off-by: Peter Rosin <peda@axentia.se> > >>>>>> --- > >>>>>> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ > >>>>>> include/drm/drm_bridge.h | 2 ++ > >>>>>> 2 files changed, 20 insertions(+) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > >>>>>> index 78d186b6831b..0259f0a3ff27 100644 > >>>>>> --- a/drivers/gpu/drm/drm_bridge.c > >>>>>> +++ b/drivers/gpu/drm/drm_bridge.c > >>>>>> @@ -26,6 +26,7 @@ > >>>>>> #include <linux/mutex.h> > >>>>>> > >>>>>> #include <drm/drm_bridge.h> > >>>>>> +#include <drm/drm_device.h> > >>>>>> #include <drm/drm_encoder.h> > >>>>>> > >>>>>> #include "drm_crtc_internal.h" > >>>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > >>>>>> if (bridge->dev) > >>>>>> return -EBUSY; > >>>>>> > >>>>>> + if (encoder->dev->dev != bridge->odev) { > >>>>> > >>>>> I wonder why device_link_add does not handle this case (self dependency) > >>>>> silently as noop, as it seems to be a correct behavior. > >>>> > >>>> It's kind-of a silly corner-case though, so perfectly understandable > >>>> that it isn't handled. > >>>> > >>>>>> + bridge->link = device_link_add(encoder->dev->dev, > >>>>>> + bridge->odev, 0); > >>>>>> + if (!bridge->link) { > >>>>>> + dev_err(bridge->odev, "failed to link bridge to %s\n", > >>>>>> + dev_name(encoder->dev->dev)); > >>>>>> + return -EINVAL; > >>>>>> + } > >>>>>> + } > >>>>>> + > >>>>>> bridge->dev = encoder->dev; > >>>>>> bridge->encoder = encoder; > >>>>>> > >>>>>> if (bridge->funcs->attach) { > >>>>>> ret = bridge->funcs->attach(bridge); > >>>>>> if (ret < 0) { > >>>>>> + if (bridge->link) > >>>>>> + device_link_del(bridge->link); > >>>>>> + bridge->link = NULL; > >>>>>> bridge->dev = NULL; > >>>>>> bridge->encoder = NULL; > >>>>>> return ret; > >>>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) > >>>>>> if (bridge->funcs->detach) > >>>>>> bridge->funcs->detach(bridge); > >>>>>> > >>>>>> + if (bridge->link) > >>>>>> + device_link_del(bridge->link); > >>>>>> + bridge->link = NULL; > >>>>>> + > >>>>>> bridge->dev = NULL; > >>>>>> } > >>>>>> > >>>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > >>>>>> index b656e505d11e..804189c63a4c 100644 > >>>>>> --- a/include/drm/drm_bridge.h > >>>>>> +++ b/include/drm/drm_bridge.h > >>>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings { > >>>>>> * @list: to keep track of all added bridges > >>>>>> * @timings: the timing specification for the bridge, if any (may > >>>>>> * be NULL) > >>>>>> + * @link: drm consumer <-> bridge supplier > >>>>> > >>>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer > >>>>> to the bridge" would be better. > >>>> > >>>> I meant "<->" to indicate that the link is bidirectional, not that the > >>>> relationship is in any way symmetric. I wasn't aware of any implication > >>>> of a symmetric relationship when using "<->", do you have a reference? > >>>> But I guess the different arrow notations in math are somewhat overloaded > >>>> and that someone at some point must have used "<->" to indicate a > >>>> symmetric relationship... > >>> > >>> Yeah I agree with Andrzej here, for me <-> implies a symmetric > >>> relationship. Spelling it out like Andrzej suggested sounds like the > >>> better idea. > >>> -Daniel > >> > >> Ok, I guess that means I have to do a v3 after all. Or can this > >> trivial documentation update be done by the committer? I hate to > >> spam everyone with another volley... > >> > >> Or perhaps I should squash patches 2-23 that are all rather similar > >> and mechanic? I separated them to allow for easier review from > >> individual driver maintainers, but that didn't seem to happen > >> anyway... > > > > Do another volley of the full set, or in-reply-to to just replace the > > patch that needs to be respun (but some people don't like that). > > > > When resending just make sure you're picking up all the acks/r-bs you > > have already. > > Right, I always try to do that. One Ack that I did not include in v2 > was the one you had on v1 24/24 (i.e. this patch). The reason I did > not add your Ack for v2 even on the patch where it obviously applied > was that I didn't know if you'd barf on the odev name. > > But it was (and still is) a bit unclear if that was on Ack on the > last patch only, or if it was for the whole series? I think it might > have been for the whole series, but I'm not sure and I hate to be a > presumptuous idiot... Ack on the overall concept, and I'm ok with odev too. But definitely get acks from relevant people (bridge/driver maintainers). In terms of patches, I'd say patch 1 + 24-26 have my Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> -Daniel > > Cheers, > Peter > > > -Daniel > >> Cheers, > >> Peter > >> > >>> > >>>> > >>>>> Anyway: > >>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > >>>> > >>>> Thanks! > >>>> > >>>> Cheers, > >>>> Peter > >>>> > >>>>> -- > >>>>> Regards > >>>>> Andrzej > >>>>> > >>>>>> * @funcs: control functions > >>>>>> * @driver_private: pointer to the bridge driver's internal context > >>>>>> */ > >>>>>> @@ -271,6 +272,7 @@ struct drm_bridge { > >>>>>> struct drm_bridge *next; > >>>>>> struct list_head list; > >>>>>> const struct drm_bridge_timings *timings; > >>>>>> + struct device_link *link; > >>>>>> > >>>>>> const struct drm_bridge_funcs *funcs; > >>>>>> void *driver_private; > >>>>> > >>>>> > >>>> > >>> > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > >
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 78d186b6831b..0259f0a3ff27 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -26,6 +26,7 @@ #include <linux/mutex.h> #include <drm/drm_bridge.h> +#include <drm/drm_device.h> #include <drm/drm_encoder.h> #include "drm_crtc_internal.h" @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, if (bridge->dev) return -EBUSY; + if (encoder->dev->dev != bridge->odev) { + bridge->link = device_link_add(encoder->dev->dev, + bridge->odev, 0); + if (!bridge->link) { + dev_err(bridge->odev, "failed to link bridge to %s\n", + dev_name(encoder->dev->dev)); + return -EINVAL; + } + } + bridge->dev = encoder->dev; bridge->encoder = encoder; if (bridge->funcs->attach) { ret = bridge->funcs->attach(bridge); if (ret < 0) { + if (bridge->link) + device_link_del(bridge->link); + bridge->link = NULL; bridge->dev = NULL; bridge->encoder = NULL; return ret; @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) if (bridge->funcs->detach) bridge->funcs->detach(bridge); + if (bridge->link) + device_link_del(bridge->link); + bridge->link = NULL; + bridge->dev = NULL; } diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index b656e505d11e..804189c63a4c 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -261,6 +261,7 @@ struct drm_bridge_timings { * @list: to keep track of all added bridges * @timings: the timing specification for the bridge, if any (may * be NULL) + * @link: drm consumer <-> bridge supplier * @funcs: control functions * @driver_private: pointer to the bridge driver's internal context */ @@ -271,6 +272,7 @@ struct drm_bridge { struct drm_bridge *next; struct list_head list; const struct drm_bridge_timings *timings; + struct device_link *link; const struct drm_bridge_funcs *funcs; void *driver_private;
If the bridge supplier is unbound, this will bring the bridge consumer down along with the bridge. Thus, there will no longer linger any dangling pointers from the bridge consumer (the drm_device) to some non-existent bridge supplier. Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ include/drm/drm_bridge.h | 2 ++ 2 files changed, 20 insertions(+)