Hi all,
This series adds device links support to drm_bridge. The motivation behind it is that a drm_bridge in a module could get removed under the feet of the bridge user without warning, so we need a way to remove and reprobe the client as needed to avoid peering into the void.
1: Add a drm_bridge_init() function which wraps all initialisation of the structure prior to calling drm_bridge_add().
2-26,28: Apply the drm_bridge_init() refactor to every bridge that uses drm_bridge_add().
27: Minor cleanup in rcar-du.
29: Add of_drm_find_bridge_devlink() which functions the same as of_drm_find_bridge() plus adds a device device link from the owning drm_device to the bridge device.
30: As a motivating example, convert komeda to exclusively use drm_bridge for its pipe outputs; this isn't a regression in usability any more since device links bring the same automatic remove/reprobe feature as components.
Mihail Atanassov (29): drm: Introduce drm_bridge_init() drm/bridge: adv7511: Use drm_bridge_init() drm/bridge: anx6345: Use drm_bridge_init() drm/bridge: anx78xx: Use drm_bridge_init() drm/bridge: cdns: Use drm_bridge_init() drm/bridge: dumb-vga-dac: Use drm_bridge_init() drm/bridge: lvds-encoder: Use drm_bridge_init() drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: Use drm_bridge_init() drm/bridge: nxp-ptn3460: Use drm_bridge_init() drm/bridge: panel: Use drm_bridge_init() drm/bridge: ps8622: Use drm_bridge_init() drm/bridge: sii902x: Use drm_bridge_init() gpu: drm: bridge: sii9234: Use drm_bridge_init() drm/bridge: sil_sii8620: Use drm_bridge_init() drm/bridge: dw-hdmi: Use drm_bridge_init() drm/bridge/synopsys: dsi: Use drm_bridge_init() drm/bridge: tc358764: Use drm_bridge_init() drm/bridge: tc358767: Use drm_bridge_init() drm/bridge: thc63: Use drm_bridge_init() drm/bridge: ti-sn65dsi86: Use drm_bridge_init() drm/bridge: ti-tfp410: Use drm_bridge_init() drm/exynos: mic: Use drm_bridge_init() drm/i2c: tda998x: Use drm_bridge_init() drm/mcde: dsi: Use drm_bridge_init() drm/mediatek: hdmi: Use drm_bridge_init() drm: rcar-du: lvds: Use drm_bridge_init() drm: rcar-du: lvds: Don't set drm_bridge private pointer drm/sti: sti_vdo: Use drm_bridge_init() drm/komeda: Use drm_bridge interface for pipe outputs
Russell King (1): drm/bridge: add support for device links to bridge
.../gpu/drm/arm/display/komeda/komeda_drv.c | 54 ++++++------- .../gpu/drm/arm/display/komeda/komeda_kms.c | 77 ++++++++++++++++-- .../gpu/drm/arm/display/komeda/komeda_kms.h | 2 + drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 +- .../drm/bridge/analogix/analogix-anx6345.c | 5 +- .../drm/bridge/analogix/analogix-anx78xx.c | 8 +- drivers/gpu/drm/bridge/cdns-dsi.c | 4 +- drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +- drivers/gpu/drm/bridge/lvds-encoder.c | 7 +- .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 4 +- drivers/gpu/drm/bridge/nxp-ptn3460.c | 4 +- drivers/gpu/drm/bridge/panel.c | 7 +- drivers/gpu/drm/bridge/parade-ps8622.c | 3 +- drivers/gpu/drm/bridge/sii902x.c | 5 +- drivers/gpu/drm/bridge/sii9234.c | 3 +- drivers/gpu/drm/bridge/sil-sii8620.c | 3 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 7 +- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 7 +- drivers/gpu/drm/bridge/tc358764.c | 4 +- drivers/gpu/drm/bridge/tc358767.c | 3 +- drivers/gpu/drm/bridge/thc63lvd1024.c | 7 +- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +- drivers/gpu/drm/bridge/ti-tfp410.c | 5 +- drivers/gpu/drm/drm_bridge.c | 78 +++++++++++++++---- drivers/gpu/drm/exynos/exynos_drm_mic.c | 8 +- drivers/gpu/drm/i2c/tda998x_drv.c | 6 +- drivers/gpu/drm/mcde/mcde_dsi.c | 3 +- drivers/gpu/drm/mediatek/mtk_hdmi.c | 4 +- drivers/gpu/drm/rcar-du/rcar_lvds.c | 5 +- drivers/gpu/drm/sti/sti_dvo.c | 4 +- include/drm/drm_bridge.h | 8 ++ 31 files changed, 217 insertions(+), 134 deletions(-)
A simple convenience function to initialize the struct drm_bridge.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 4 ++++ 2 files changed, 33 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cba537c99e43..cbe680aa6eac 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge) } EXPORT_SYMBOL(drm_bridge_remove);
+/** + * drm_bridge_init - initialise a drm_bridge structure + * + * @bridge: bridge control structure + * @funcs: control functions + * @dev: device + * @timings: timing specification for the bridge; optional (may be NULL) + * @driver_private: pointer to the bridge driver internal context (may be NULL) + */ +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev, + const struct drm_bridge_funcs *funcs, + const struct drm_bridge_timings *timings, + void *driver_private) +{ + WARN_ON(!funcs); + + bridge->dev = NULL; + bridge->encoder = NULL; + bridge->next = NULL; + +#ifdef CONFIG_OF + bridge->of_node = dev->of_node; +#endif + bridge->timings = timings; + bridge->funcs = funcs; + bridge->driver_private = driver_private; +} +EXPORT_SYMBOL(drm_bridge_init); + /** * drm_bridge_attach - attach the bridge to an encoder's chain * diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index c0a2286a81e9..d6d9d5301551 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -402,6 +402,10 @@ struct drm_bridge {
void drm_bridge_add(struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev, + const struct drm_bridge_funcs *funcs, + const struct drm_bridge_timings *timings, + void *driver_private); struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);
On Tue, Nov 26, 2019 at 01:15:59PM +0000, Mihail Atanassov wrote:
A simple convenience function to initialize the struct drm_bridge.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
The commit message here leaves figuring out why we need this to the reader. Reading ahead the reasons seems to be to roll out bridge->dev setting for everyone, so that we can set the device_link. Please explain that in the commit message so the patch is properly motivated.
drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 4 ++++ 2 files changed, 33 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cba537c99e43..cbe680aa6eac 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge) } EXPORT_SYMBOL(drm_bridge_remove);
+/**
- drm_bridge_init - initialise a drm_bridge structure
- @bridge: bridge control structure
- @funcs: control functions
- @dev: device
- @timings: timing specification for the bridge; optional (may be NULL)
- @driver_private: pointer to the bridge driver internal context (may be NULL)
Please also sprinkle some links to this new function to relevant places, I'd add at least:
"Drivers should call drm_bridge_init() first." to the kerneldoc for drm_bridge_add. drm_bridge_add should also mention drm_bridge_remove as the undo function.
And perhaps a longer paragraph to &struct drm_bridge:
"Bridge drivers should call drm_bridge_init() to initialized a bridge driver, and then register it with drm_bridge_add().
"Users of bridges link a bridge driver into their overall display output pipeline by calling drm_bridge_attach()."
- */
+void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
const struct drm_bridge_funcs *funcs,const struct drm_bridge_timings *timings,void *driver_private)+{
- WARN_ON(!funcs);
- bridge->dev = NULL;
Given that the goal here is to get bridge->dev set up, why not
WARN_ON(!dev); bridge->dev = dev;
That should help us to really move forward with all this. -Daniel
- bridge->encoder = NULL;
- bridge->next = NULL;
+#ifdef CONFIG_OF
- bridge->of_node = dev->of_node;
+#endif
- bridge->timings = timings;
- bridge->funcs = funcs;
- bridge->driver_private = driver_private;
+} +EXPORT_SYMBOL(drm_bridge_init);
/**
- drm_bridge_attach - attach the bridge to an encoder's chain
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index c0a2286a81e9..d6d9d5301551 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -402,6 +402,10 @@ struct drm_bridge {
void drm_bridge_add(struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
const struct drm_bridge_funcs *funcs,const struct drm_bridge_timings *timings,void *driver_private);struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous); -- 2.23.0
Hi Daniel,
Thanks for the quick review.
On Tuesday, 26 November 2019 14:26:10 GMT Daniel Vetter wrote:
On Tue, Nov 26, 2019 at 01:15:59PM +0000, Mihail Atanassov wrote:
A simple convenience function to initialize the struct drm_bridge.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
The commit message here leaves figuring out why we need this to the reader. Reading ahead the reasons seems to be to roll out bridge->dev setting for everyone, so that we can set the device_link. Please explain that in the commit message so the patch is properly motivated.
Ack, but with one caveat: bridge->dev is the struct drm_device that is the bridge client, we need to add a bridge->device (patch 29 in this series) which is the struct device that will manage the bridge lifetime.
drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 4 ++++ 2 files changed, 33 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cba537c99e43..cbe680aa6eac 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge) } EXPORT_SYMBOL(drm_bridge_remove);
+/**
- drm_bridge_init - initialise a drm_bridge structure
- @bridge: bridge control structure
- @funcs: control functions
- @dev: device
- @timings: timing specification for the bridge; optional (may be NULL)
- @driver_private: pointer to the bridge driver internal context (may be NULL)
Please also sprinkle some links to this new function to relevant places, I'd add at least:
"Drivers should call drm_bridge_init() first." to the kerneldoc for drm_bridge_add. drm_bridge_add should also mention drm_bridge_remove as the undo function.
And perhaps a longer paragraph to &struct drm_bridge:
"Bridge drivers should call drm_bridge_init() to initialized a bridge driver, and then register it with drm_bridge_add().
"Users of bridges link a bridge driver into their overall display output pipeline by calling drm_bridge_attach()."
Will do.
- */
+void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
const struct drm_bridge_funcs *funcs,const struct drm_bridge_timings *timings,void *driver_private)+{
- WARN_ON(!funcs);
- bridge->dev = NULL;
Given that the goal here is to get bridge->dev set up, why not
WARN_ON(!dev); bridge->dev = dev;
See above struct device vs struct drm_device. I add a
bridge->device = dev;
in patch 29, which takes care of that. I skipped the warn since there's a dereference of dev, but I now realized it's behind CONFIG_OF, so I'll add it in for v2.
Yes, 'device' isn't the best of names, but I took Russell's patch almost as-is, I didn't have any better ideas for bikeshedding.
That should help us to really move forward with all this. -Daniel
- bridge->encoder = NULL;
- bridge->next = NULL;
+#ifdef CONFIG_OF
- bridge->of_node = dev->of_node;
+#endif
- bridge->timings = timings;
- bridge->funcs = funcs;
- bridge->driver_private = driver_private;
+} +EXPORT_SYMBOL(drm_bridge_init);
/**
- drm_bridge_attach - attach the bridge to an encoder's chain
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index c0a2286a81e9..d6d9d5301551 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -402,6 +402,10 @@ struct drm_bridge {
void drm_bridge_add(struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
const struct drm_bridge_funcs *funcs,const struct drm_bridge_timings *timings,void *driver_private);struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);
On Tue, Nov 26, 2019 at 4:55 PM Mihail Atanassov Mihail.Atanassov@arm.com wrote:
Hi Daniel,
Thanks for the quick review.
On Tuesday, 26 November 2019 14:26:10 GMT Daniel Vetter wrote:
On Tue, Nov 26, 2019 at 01:15:59PM +0000, Mihail Atanassov wrote:
A simple convenience function to initialize the struct drm_bridge.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
The commit message here leaves figuring out why we need this to the reader. Reading ahead the reasons seems to be to roll out bridge->dev setting for everyone, so that we can set the device_link. Please explain that in the commit message so the patch is properly motivated.
Ack, but with one caveat: bridge->dev is the struct drm_device that is the bridge client, we need to add a bridge->device (patch 29 in this series) which is the struct device that will manage the bridge lifetime.
Ah yes, ->dev is for drm_bridge_attach.
drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 4 ++++ 2 files changed, 33 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cba537c99e43..cbe680aa6eac 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge) } EXPORT_SYMBOL(drm_bridge_remove);
+/**
- drm_bridge_init - initialise a drm_bridge structure
- @bridge: bridge control structure
- @funcs: control functions
- @dev: device
- @timings: timing specification for the bridge; optional (may be NULL)
- @driver_private: pointer to the bridge driver internal context (may be NULL)
Please also sprinkle some links to this new function to relevant places, I'd add at least:
"Drivers should call drm_bridge_init() first." to the kerneldoc for drm_bridge_add. drm_bridge_add should also mention drm_bridge_remove as the undo function.
And perhaps a longer paragraph to &struct drm_bridge:
"Bridge drivers should call drm_bridge_init() to initialized a bridge driver, and then register it with drm_bridge_add().
"Users of bridges link a bridge driver into their overall display output pipeline by calling drm_bridge_attach()."
Will do.
- */
+void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
const struct drm_bridge_funcs *funcs,const struct drm_bridge_timings *timings,void *driver_private)+{
- WARN_ON(!funcs);
- bridge->dev = NULL;
Given that the goal here is to get bridge->dev set up, why not
WARN_ON(!dev); bridge->dev = dev;See above struct device vs struct drm_device. I add a
bridge->device = dev;in patch 29, which takes care of that. I skipped the warn since there's a dereference of dev, but I now realized it's behind CONFIG_OF, so I'll add it in for v2.
Ok, sounds good. Having the WARN_ON in patch 1 should also help making sure all the conversion patches dtrt (and any future users). -Daniel
Yes, 'device' isn't the best of names, but I took Russell's patch almost as-is, I didn't have any better ideas for bikeshedding.
That should help us to really move forward with all this. -Daniel
- bridge->encoder = NULL;
- bridge->next = NULL;
+#ifdef CONFIG_OF
- bridge->of_node = dev->of_node;
+#endif
- bridge->timings = timings;
- bridge->funcs = funcs;
- bridge->driver_private = driver_private;
+} +EXPORT_SYMBOL(drm_bridge_init);
/**
- drm_bridge_attach - attach the bridge to an encoder's chain
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index c0a2286a81e9..d6d9d5301551 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -402,6 +402,10 @@ struct drm_bridge {
void drm_bridge_add(struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
const struct drm_bridge_funcs *funcs,const struct drm_bridge_timings *timings,void *driver_private);struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);
-- Mihail
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Mihail.
Ack, but with one caveat: bridge->dev is the struct drm_device that is the bridge client, we need to add a bridge->device (patch 29 in this series) which is the struct device that will manage the bridge lifetime.
Other places uses the variable name "drm" for a drm_device. This is less confusion than the "dev" name.
It seems a recent trend to use the variable name "drm" so you can find a lot of places using "dev".
bike-shedding - but also about readability.
Sam
On Tuesday, 26 November 2019 19:24:45 GMT Sam Ravnborg wrote:
Hi Mihail.
Hi Sam,
Ack, but with one caveat: bridge->dev is the struct drm_device that is the bridge client, we need to add a bridge->device (patch 29 in this series) which is the struct device that will manage the bridge lifetime.
Other places uses the variable name "drm" for a drm_device. This is less confusion than the "dev" name.
It seems a recent trend to use the variable name "drm" so you can find a lot of places using "dev".
bike-shedding - but also about readability.
Sam
I'm okay with the idea, I can do a follow-up patch or series for the rename; I expect it would be a bit hefty to do it prior to this.
@Daniel, thoughts on s/bridge.dev/bridge.drm/ and s/bridge.device/bridge.dev/ after this series?
On Wed, Nov 27, 2019 at 11:05:56AM +0000, Mihail Atanassov wrote:
On Tuesday, 26 November 2019 19:24:45 GMT Sam Ravnborg wrote:
Hi Mihail.
Hi Sam,
Ack, but with one caveat: bridge->dev is the struct drm_device that is the bridge client, we need to add a bridge->device (patch 29 in this series) which is the struct device that will manage the bridge lifetime.
Other places uses the variable name "drm" for a drm_device. This is less confusion than the "dev" name.
It seems a recent trend to use the variable name "drm" so you can find a lot of places using "dev".
bike-shedding - but also about readability.
Sam
I'm okay with the idea, I can do a follow-up patch or series for the rename; I expect it would be a bit hefty to do it prior to this.
@Daniel, thoughts on s/bridge.dev/bridge.drm/ and s/bridge.device/bridge.dev/ after this series?
I'm firmly in the "no opionon" on this. -Daniel
On Tue, Nov 26, 2019 at 01:15:59PM +0000, Mihail Atanassov wrote:
A simple convenience function to initialize the struct drm_bridge.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 4 ++++ 2 files changed, 33 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cba537c99e43..cbe680aa6eac 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge) } EXPORT_SYMBOL(drm_bridge_remove);
+/**
- drm_bridge_init - initialise a drm_bridge structure
- @bridge: bridge control structure
- @funcs: control functions
- @dev: device
- @timings: timing specification for the bridge; optional (may be NULL)
- @driver_private: pointer to the bridge driver internal context (may be NULL)
- */
+void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
const struct drm_bridge_funcs *funcs,const struct drm_bridge_timings *timings,void *driver_private)+{
- WARN_ON(!funcs);
- bridge->dev = NULL;
- bridge->encoder = NULL;
- bridge->next = NULL;
+#ifdef CONFIG_OF
- bridge->of_node = dev->of_node;
+#endif
- bridge->timings = timings;
- bridge->funcs = funcs;
- bridge->driver_private = driver_private;
Can we directly put drm_bridge_add() here. then - User always need to call bridge_init and add together. - Consistent with others like drm_plane/crtc_init which directly has drm_mode_object_add() in it.
James.
+} +EXPORT_SYMBOL(drm_bridge_init);
/**
- drm_bridge_attach - attach the bridge to an encoder's chain
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index c0a2286a81e9..d6d9d5301551 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -402,6 +402,10 @@ struct drm_bridge {
void drm_bridge_add(struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
const struct drm_bridge_funcs *funcs,const struct drm_bridge_timings *timings,void *driver_private);struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);
On Mon, Dec 02, 2019 at 05:55:06AM +0000, james qian wang (Arm Technology China) wrote:
On Tue, Nov 26, 2019 at 01:15:59PM +0000, Mihail Atanassov wrote:
A simple convenience function to initialize the struct drm_bridge.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 4 ++++ 2 files changed, 33 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cba537c99e43..cbe680aa6eac 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge) } EXPORT_SYMBOL(drm_bridge_remove);
+/**
- drm_bridge_init - initialise a drm_bridge structure
- @bridge: bridge control structure
- @funcs: control functions
- @dev: device
- @timings: timing specification for the bridge; optional (may be NULL)
- @driver_private: pointer to the bridge driver internal context (may be NULL)
- */
+void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
const struct drm_bridge_funcs *funcs,const struct drm_bridge_timings *timings,void *driver_private)+{
- WARN_ON(!funcs);
- bridge->dev = NULL;
- bridge->encoder = NULL;
- bridge->next = NULL;
+#ifdef CONFIG_OF
- bridge->of_node = dev->of_node;
+#endif
- bridge->timings = timings;
- bridge->funcs = funcs;
- bridge->driver_private = driver_private;
Can we directly put drm_bridge_add() here. then
- User always need to call bridge_init and add together.
- Consistent with others like drm_plane/crtc_init which directly has drm_mode_object_add() in it.
Uh no, the trouble here is that drm_bridge_add should actually be called _register, because it publishes the bridge to the world. I think we even have a todo item to rename _add to _register ... Once that's done the bridge can't be changed anymore, all init code must have completed. So often you need a bit of code between _init() and _register().
drm_mode_object_add is different since for mode objects it doesn't publish it to the world, that's done with drm_dev_register and drm_connector_register. drm_mode_object_add just does a bit of internal house keeping. -Daniel
James.
+} +EXPORT_SYMBOL(drm_bridge_init);
/**
- drm_bridge_attach - attach the bridge to an encoder's chain
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index c0a2286a81e9..d6d9d5301551 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -402,6 +402,10 @@ struct drm_bridge {
void drm_bridge_add(struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
const struct drm_bridge_funcs *funcs,const struct drm_bridge_timings *timings,void *driver_private);struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Dec 02, 2019 at 09:49:35AM +0100, Daniel Vetter wrote:
On Mon, Dec 02, 2019 at 05:55:06AM +0000, james qian wang (Arm Technology China) wrote:
On Tue, Nov 26, 2019 at 01:15:59PM +0000, Mihail Atanassov wrote:
A simple convenience function to initialize the struct drm_bridge.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 4 ++++ 2 files changed, 33 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cba537c99e43..cbe680aa6eac 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge) } EXPORT_SYMBOL(drm_bridge_remove);
+/**
- drm_bridge_init - initialise a drm_bridge structure
- @bridge: bridge control structure
- @funcs: control functions
- @dev: device
- @timings: timing specification for the bridge; optional (may be NULL)
- @driver_private: pointer to the bridge driver internal context (may be NULL)
- */
+void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
const struct drm_bridge_funcs *funcs,const struct drm_bridge_timings *timings,void *driver_private)+{
- WARN_ON(!funcs);
- bridge->dev = NULL;
- bridge->encoder = NULL;
- bridge->next = NULL;
+#ifdef CONFIG_OF
- bridge->of_node = dev->of_node;
+#endif
- bridge->timings = timings;
- bridge->funcs = funcs;
- bridge->driver_private = driver_private;
Can we directly put drm_bridge_add() here. then
- User always need to call bridge_init and add together.
- Consistent with others like drm_plane/crtc_init which directly has drm_mode_object_add() in it.
Uh no, the trouble here is that drm_bridge_add should actually be called _register, because it publishes the bridge to the world. I think we even have a todo item to rename _add to _register ... Once that's done the bridge can't be changed anymore, all init code must have completed. So often you need a bit of code between _init() and _register().
drm_mode_object_add is different since for mode objects it doesn't publish it to the world, that's done with drm_dev_register and drm_connector_register. drm_mode_object_add just does a bit of internal house keeping. -Daniel
Yes, the name _register() is more better.
And thank you for such detailed explanation.
Thanks James
James.
+} +EXPORT_SYMBOL(drm_bridge_init);
/**
- drm_bridge_attach - attach the bridge to an encoder's chain
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index c0a2286a81e9..d6d9d5301551 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -402,6 +402,10 @@ struct drm_bridge {
void drm_bridge_add(struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
const struct drm_bridge_funcs *funcs,const struct drm_bridge_timings *timings,void *driver_private);struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 9e13e466e72c..73600d8766f8 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1216,9 +1216,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) if (ret) goto err_unregister_cec;
- adv7511->bridge.funcs = &adv7511_bridge_funcs; - adv7511->bridge.of_node = dev->of_node; - + drm_bridge_init(&adv7511->bridge, dev, &adv7511_bridge_funcs, + NULL, NULL); drm_bridge_add(&adv7511->bridge);
adv7511_audio_init(dev, adv7511);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/analogix/analogix-anx6345.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c index b4f3a923a52a..130d5c3a07ef 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c @@ -696,8 +696,6 @@ static int anx6345_i2c_probe(struct i2c_client *client,
mutex_init(&anx6345->lock);
- anx6345->bridge.of_node = client->dev.of_node; - anx6345->client = client; i2c_set_clientdata(client, anx6345);
@@ -760,7 +758,8 @@ static int anx6345_i2c_probe(struct i2c_client *client, /* Look for supported chip ID */ anx6345_poweron(anx6345); if (anx6345_get_chip_id(anx6345)) { - anx6345->bridge.funcs = &anx6345_bridge_funcs; + drm_bridge_init(&anx6345->bridge, &client->dev, + &anx6345_bridge_funcs, NULL, NULL); drm_bridge_add(&anx6345->bridge);
return 0;
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c index 41867be03751..e37892cdc9cf 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c @@ -1214,10 +1214,6 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
mutex_init(&anx78xx->lock);
-#if IS_ENABLED(CONFIG_OF) - anx78xx->bridge.of_node = client->dev.of_node; -#endif - anx78xx->client = client; i2c_set_clientdata(client, anx78xx);
@@ -1321,8 +1317,8 @@ static int anx78xx_i2c_probe(struct i2c_client *client, goto err_poweroff; }
- anx78xx->bridge.funcs = &anx78xx_bridge_funcs; - + drm_bridge_init(&anx78xx->bridge, &client->dev, &anx78xx_bridge_funcs, + NULL, NULL); drm_bridge_add(&anx78xx->bridge);
/* If cable is pulled out, just poweroff and wait for HPD event */
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/cdns-dsi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c index 3a5bd4e7fd1e..58b2aa8b6c24 100644 --- a/drivers/gpu/drm/bridge/cdns-dsi.c +++ b/drivers/gpu/drm/bridge/cdns-dsi.c @@ -1233,8 +1233,8 @@ static int cdns_dsi_drm_probe(struct platform_device *pdev) * CDNS_DPI_INPUT. */ input->id = CDNS_DPI_INPUT; - input->bridge.funcs = &cdns_dsi_bridge_funcs; - input->bridge.of_node = pdev->dev.of_node; + drm_bridge_init(&input->bridge, &pdev->dev, &cdns_dsi_bridge_funcs, + NULL, NULL);
/* Mask all interrupts before registering the IRQ handler. */ writel(0, dsi->regs + MCTL_MAIN_STS_CTL);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c index cc33dc411b9e..896f27272e38 100644 --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c @@ -205,10 +205,8 @@ static int dumb_vga_probe(struct platform_device *pdev) } }
- vga->bridge.funcs = &dumb_vga_bridge_funcs; - vga->bridge.of_node = pdev->dev.of_node; - vga->bridge.timings = of_device_get_match_data(&pdev->dev); - + drm_bridge_init(&vga->bridge, &pdev->dev, &dumb_vga_bridge_funcs, + of_device_get_match_data(&pdev->dev), NULL); drm_bridge_add(&vga->bridge);
return 0;
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/lvds-encoder.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c index e2132a8d5106..155406510416 100644 --- a/drivers/gpu/drm/bridge/lvds-encoder.c +++ b/drivers/gpu/drm/bridge/lvds-encoder.c @@ -112,11 +112,10 @@ static int lvds_encoder_probe(struct platform_device *pdev) return PTR_ERR(lvds_encoder->panel_bridge);
/* The panel_bridge bridge is attached to the panel's of_node, - * but we need a bridge attached to our of_node for our user - * to look up. + * but we need a bridge attached to our of_node (in dev->of_node) + * for our user to look up. */ - lvds_encoder->bridge.of_node = dev->of_node; - lvds_encoder->bridge.funcs = &funcs; + drm_bridge_init(&lvds_encoder->bridge, dev, &funcs, NULL, NULL); drm_bridge_add(&lvds_encoder->bridge);
platform_set_drvdata(pdev, lvds_encoder);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c index e8a49f6146c6..d567cd63810f 100644 --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c @@ -303,8 +303,8 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c, i2c_set_clientdata(stdp4028_i2c, ge_b850v3_lvds_ptr);
/* drm bridge initialization */ - ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs; - ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node; + drm_bridge_init(&ge_b850v3_lvds_ptr->bridge, dev, &ge_b850v3_lvds_funcs, + NULL, NULL); drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
/* Clear pending interrupts since power up. */
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/nxp-ptn3460.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c index 57ff01339559..2656a188b434 100644 --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c @@ -320,8 +320,8 @@ static int ptn3460_probe(struct i2c_client *client, return ret; }
- ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs; - ptn_bridge->bridge.of_node = dev->of_node; + drm_bridge_init(&ptn_bridge->bridge, dev, &ptn3460_bridge_funcs, + NULL, NULL); drm_bridge_add(&ptn_bridge->bridge);
i2c_set_clientdata(client, ptn_bridge);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/panel.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index f4e293e7cf64..91d68d0337cc 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -192,11 +192,8 @@ struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel, panel_bridge->connector_type = connector_type; panel_bridge->panel = panel;
- panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs; -#ifdef CONFIG_OF - panel_bridge->bridge.of_node = panel->dev->of_node; -#endif - + drm_bridge_init(&panel_bridge->bridge, panel->dev, + &panel_bridge_bridge_funcs, NULL, NULL); drm_bridge_add(&panel_bridge->bridge);
return &panel_bridge->bridge;
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/parade-ps8622.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c index b7a72dfdcac3..8454dbb238bb 100644 --- a/drivers/gpu/drm/bridge/parade-ps8622.c +++ b/drivers/gpu/drm/bridge/parade-ps8622.c @@ -588,8 +588,7 @@ static int ps8622_probe(struct i2c_client *client, ps8622->bl->props.brightness = PS8622_MAX_BRIGHTNESS; }
- ps8622->bridge.funcs = &ps8622_bridge_funcs; - ps8622->bridge.of_node = dev->of_node; + drm_bridge_init(&ps8622->bridge, dev, &ps8622_bridge_funcs, NULL, NULL); drm_bridge_add(&ps8622->bridge);
i2c_set_clientdata(client, ps8622);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/sii902x.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c index b70e8c5cf2e1..2a9db621484d 100644 --- a/drivers/gpu/drm/bridge/sii902x.c +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -1017,9 +1017,8 @@ static int sii902x_probe(struct i2c_client *client, return ret; }
- sii902x->bridge.funcs = &sii902x_bridge_funcs; - sii902x->bridge.of_node = dev->of_node; - sii902x->bridge.timings = &default_sii902x_timings; + drm_bridge_init(&sii902x->bridge, dev, &sii902x_bridge_funcs, + &default_sii902x_timings, NULL); drm_bridge_add(&sii902x->bridge);
sii902x_audio_codec_init(sii902x, dev);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/sii9234.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c index f81f81b7051f..bfd3832baa1a 100644 --- a/drivers/gpu/drm/bridge/sii9234.c +++ b/drivers/gpu/drm/bridge/sii9234.c @@ -925,8 +925,7 @@ static int sii9234_probe(struct i2c_client *client,
i2c_set_clientdata(client, ctx);
- ctx->bridge.funcs = &sii9234_bridge_funcs; - ctx->bridge.of_node = dev->of_node; + drm_bridge_init(&ctx->bridge, dev, &sii9234_bridge_funcs, NULL, NULL); drm_bridge_add(&ctx->bridge);
sii9234_cable_in(ctx);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 4c0eef406eb1..482dc2291350 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -2337,8 +2337,7 @@ static int sii8620_probe(struct i2c_client *client,
i2c_set_clientdata(client, ctx);
- ctx->bridge.funcs = &sii8620_bridge_funcs; - ctx->bridge.of_node = dev->of_node; + drm_bridge_init(&ctx->bridge, dev, &sii8620_bridge_funcs, NULL, NULL); drm_bridge_add(&ctx->bridge);
if (!ctx->extcon)
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index dbe38a54870b..6c71ffc9df5a 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2898,11 +2898,8 @@ __dw_hdmi_probe(struct platform_device *pdev, hdmi->ddc = NULL; }
- hdmi->bridge.driver_private = hdmi; - hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; -#ifdef CONFIG_OF - hdmi->bridge.of_node = pdev->dev.of_node; -#endif + drm_bridge_init(&hdmi->bridge, &pdev->dev, &dw_hdmi_bridge_funcs, + NULL, hdmi);
memset(&pdevinfo, 0, sizeof(pdevinfo)); pdevinfo.parent = dev;
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index b6e793bb653c..051f9aaf5867 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -1052,11 +1052,8 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, return ERR_PTR(ret); }
- dsi->bridge.driver_private = dsi; - dsi->bridge.funcs = &dw_mipi_dsi_bridge_funcs; -#ifdef CONFIG_OF - dsi->bridge.of_node = pdev->dev.of_node; -#endif + drm_bridge_init(&dsi->bridge, &pdev->dev, &dw_mipi_dsi_bridge_funcs, + NULL, dsi);
return dsi; }
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/tc358764.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c index db298f550a5a..861d4df687ee 100644 --- a/drivers/gpu/drm/bridge/tc358764.c +++ b/drivers/gpu/drm/bridge/tc358764.c @@ -457,9 +457,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi) if (ret < 0) return ret;
- ctx->bridge.funcs = &tc358764_bridge_funcs; - ctx->bridge.of_node = dev->of_node; - + drm_bridge_init(&ctx->bridge, dev, &tc358764_bridge_funcs, NULL, NULL); drm_bridge_add(&ctx->bridge);
ret = mipi_dsi_attach(dsi);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/tc358767.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 8029478ffebb..7846c1925cbb 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1671,8 +1671,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) if (ret) return ret;
- tc->bridge.funcs = &tc_bridge_funcs; - tc->bridge.of_node = dev->of_node; + drm_bridge_init(&tc->bridge, dev, &tc_bridge_funcs, NULL, NULL); drm_bridge_add(&tc->bridge);
i2c_set_clientdata(client, tc);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/thc63lvd1024.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c index 3d74129b2995..abe806db5f4d 100644 --- a/drivers/gpu/drm/bridge/thc63lvd1024.c +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c @@ -218,11 +218,8 @@ static int thc63_probe(struct platform_device *pdev) if (ret) return ret;
- thc63->bridge.driver_private = thc63; - thc63->bridge.of_node = pdev->dev.of_node; - thc63->bridge.funcs = &thc63_bridge_func; - thc63->bridge.timings = &thc63->timings; - + drm_bridge_init(&thc63->bridge, &pdev->dev, &thc63_bridge_func, + &thc63->timings, thc63); drm_bridge_add(&thc63->bridge);
return 0;
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 43abf01ebd4c..4e236b46f913 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -765,9 +765,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, pdata->aux.transfer = ti_sn_aux_transfer; drm_dp_aux_register(&pdata->aux);
- pdata->bridge.funcs = &ti_sn_bridge_funcs; - pdata->bridge.of_node = client->dev.of_node; - + drm_bridge_init(&pdata->bridge, &client->dev, &ti_sn_bridge_funcs, + NULL, NULL); drm_bridge_add(&pdata->bridge);
ti_sn_debugfs_init(pdata);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/bridge/ti-tfp410.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c index aa3198dc9903..377eca989ff3 100644 --- a/drivers/gpu/drm/bridge/ti-tfp410.c +++ b/drivers/gpu/drm/bridge/ti-tfp410.c @@ -328,9 +328,8 @@ static int tfp410_init(struct device *dev, bool i2c) return -ENOMEM; dev_set_drvdata(dev, dvi);
- dvi->bridge.funcs = &tfp410_bridge_funcs; - dvi->bridge.of_node = dev->of_node; - dvi->bridge.timings = &dvi->timings; + drm_bridge_init(&dvi->bridge, dev, &tfp410_bridge_funcs, &dvi->timings, + NULL); dvi->dev = dev;
ret = tfp410_parse_timings(dvi, i2c);
No functional change: no logic depends on driver_private being NULL, so it's safe to set it earlier in exynos_mic_probe.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/exynos/exynos_drm_mic.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c b/drivers/gpu/drm/exynos/exynos_drm_mic.c index f41d75923557..caad348a5646 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_mic.c +++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c @@ -309,10 +309,6 @@ static const struct drm_bridge_funcs mic_bridge_funcs = { static int exynos_mic_bind(struct device *dev, struct device *master, void *data) { - struct exynos_mic *mic = dev_get_drvdata(dev); - - mic->bridge.driver_private = mic; - return 0; }
@@ -422,9 +418,7 @@ static int exynos_mic_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, mic);
- mic->bridge.funcs = &mic_bridge_funcs; - mic->bridge.of_node = dev->of_node; - + drm_bridge_init(&mic->bridge, dev, &mic_bridge_funcs, NULL, mic); drm_bridge_add(&mic->bridge);
pm_runtime_enable(dev);
19. 11. 26. 오후 10:16에 Mihail Atanassov 이(가) 쓴 글:
No functional change: no logic depends on driver_private being NULL, so it's safe to set it earlier in exynos_mic_probe.
Acked-by: Inki Dae inki.dae@samsung.com
And tested this patch on TM2 and TM2E boards. Tested-by: Inki Dae inki.dae@samsung.com
Thanks, Inki Dae
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
drivers/gpu/drm/exynos/exynos_drm_mic.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c b/drivers/gpu/drm/exynos/exynos_drm_mic.c index f41d75923557..caad348a5646 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_mic.c +++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c @@ -309,10 +309,6 @@ static const struct drm_bridge_funcs mic_bridge_funcs = { static int exynos_mic_bind(struct device *dev, struct device *master, void *data) {
- struct exynos_mic *mic = dev_get_drvdata(dev);
- mic->bridge.driver_private = mic;
- return 0;
}
@@ -422,9 +418,7 @@ static int exynos_mic_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, mic);
- mic->bridge.funcs = &mic_bridge_funcs;
- mic->bridge.of_node = dev->of_node;
drm_bridge_init(&mic->bridge, dev, &mic_bridge_funcs, NULL, mic); drm_bridge_add(&mic->bridge);
pm_runtime_enable(dev);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/i2c/tda998x_drv.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index a63790d32d75..f7dfa694aff7 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1974,11 +1974,7 @@ static int tda998x_create(struct device *dev) goto fail; }
- priv->bridge.funcs = &tda998x_bridge_funcs; -#ifdef CONFIG_OF - priv->bridge.of_node = dev->of_node; -#endif - + drm_bridge_init(&priv->bridge, dev, &tda998x_bridge_funcs, NULL, NULL); drm_bridge_add(&priv->bridge);
return 0;
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/mcde/mcde_dsi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c index 42fff811653e..d9b9253acccf 100644 --- a/drivers/gpu/drm/mcde/mcde_dsi.c +++ b/drivers/gpu/drm/mcde/mcde_dsi.c @@ -955,8 +955,7 @@ static int mcde_dsi_bind(struct device *dev, struct device *master, d->bridge_out = bridge;
/* Create a bridge for this DSI channel */ - d->bridge.funcs = &mcde_dsi_bridge_funcs; - d->bridge.of_node = dev->of_node; + drm_bridge_init(&d->bridge, dev, &mcde_dsi_bridge_funcs, NULL, NULL); drm_bridge_add(&d->bridge);
/* TODO: first come first serve, use a list */
On Tue, Nov 26, 2019 at 2:16 PM Mihail Atanassov Mihail.Atanassov@arm.com wrote:
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
Acked-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/mediatek/mtk_hdmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index f684947c5243..9761a80674d9 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -1708,8 +1708,8 @@ static int mtk_drm_hdmi_probe(struct platform_device *pdev)
mtk_hdmi_register_audio_driver(dev);
- hdmi->bridge.funcs = &mtk_hdmi_bridge_funcs; - hdmi->bridge.of_node = pdev->dev.of_node; + drm_bridge_init(&hdmi->bridge, &pdev->dev, &mtk_hdmi_bridge_funcs, + NULL, NULL); drm_bridge_add(&hdmi->bridge);
ret = mtk_hdmi_clk_enable_audio(hdmi);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/rcar-du/rcar_lvds.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 8c6c172bbf2e..ac1f29bacfcb 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -881,9 +881,8 @@ static int rcar_lvds_probe(struct platform_device *pdev) if (ret < 0) return ret;
- lvds->bridge.driver_private = lvds; - lvds->bridge.funcs = &rcar_lvds_bridge_ops; - lvds->bridge.of_node = pdev->dev.of_node; + drm_bridge_init(&lvds->bridge, &pdev->dev, &rcar_lvds_bridge_ops, + NULL, lvds);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); lvds->mmio = devm_ioremap_resource(&pdev->dev, mem);
No functional change: it's not used anywhere.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/rcar-du/rcar_lvds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index ac1f29bacfcb..168a718cbcbd 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -882,7 +882,7 @@ static int rcar_lvds_probe(struct platform_device *pdev) return ret;
drm_bridge_init(&lvds->bridge, &pdev->dev, &rcar_lvds_bridge_ops, - NULL, lvds); + NULL, NULL);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); lvds->mmio = devm_ioremap_resource(&pdev->dev, mem);
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/sti/sti_dvo.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c index 68289b0b063a..20a3956b33bc 100644 --- a/drivers/gpu/drm/sti/sti_dvo.c +++ b/drivers/gpu/drm/sti/sti_dvo.c @@ -462,9 +462,7 @@ static int sti_dvo_bind(struct device *dev, struct device *master, void *data) if (!bridge) return -ENOMEM;
- bridge->driver_private = dvo; - bridge->funcs = &sti_dvo_bridge_funcs; - bridge->of_node = dvo->dev.of_node; + drm_bridge_init(bridge, &dvo->dev, &sti_dvo_bridge_funcs, NULL, dvo); drm_bridge_add(bridge);
err = drm_bridge_attach(encoder, bridge, NULL);
Hi Mihail.
On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote:
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
drivers/gpu/drm/sti/sti_dvo.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c index 68289b0b063a..20a3956b33bc 100644 --- a/drivers/gpu/drm/sti/sti_dvo.c +++ b/drivers/gpu/drm/sti/sti_dvo.c @@ -462,9 +462,7 @@ static int sti_dvo_bind(struct device *dev, struct device *master, void *data) if (!bridge) return -ENOMEM;
- bridge->driver_private = dvo;
- bridge->funcs = &sti_dvo_bridge_funcs;
- bridge->of_node = dvo->dev.of_node;
drm_bridge_init(bridge, &dvo->dev, &sti_dvo_bridge_funcs, NULL, dvo); drm_bridge_add(bridge);
err = drm_bridge_attach(encoder, bridge, NULL);
I can see from grepping that bridge.driver_private is used in a couple of other files in sti/
Like sti_hdmi.c: bridge->driver_private = hdmi; bridge->funcs = &sti_hdmi_bridge_funcs; drm_bridge_attach(encoder, bridge, NULL);
I wonder if a drm_bridge_init() should be added there. I did not look closely - but it looked suspisiously.
Sam
On Tuesday, 26 November 2019 19:37:40 GMT Sam Ravnborg wrote:
Hi Mihail.
Hi Sam,
On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote:
No functional change.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
drivers/gpu/drm/sti/sti_dvo.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c index 68289b0b063a..20a3956b33bc 100644 --- a/drivers/gpu/drm/sti/sti_dvo.c +++ b/drivers/gpu/drm/sti/sti_dvo.c @@ -462,9 +462,7 @@ static int sti_dvo_bind(struct device *dev, struct device *master, void *data) if (!bridge) return -ENOMEM;
- bridge->driver_private = dvo;
- bridge->funcs = &sti_dvo_bridge_funcs;
- bridge->of_node = dvo->dev.of_node;
drm_bridge_init(bridge, &dvo->dev, &sti_dvo_bridge_funcs, NULL, dvo); drm_bridge_add(bridge);
err = drm_bridge_attach(encoder, bridge, NULL);
I can see from grepping that bridge.driver_private is used in a couple of other files in sti/
Like sti_hdmi.c: bridge->driver_private = hdmi; bridge->funcs = &sti_hdmi_bridge_funcs; drm_bridge_attach(encoder, bridge, NULL);
I wonder if a drm_bridge_init() should be added there. I did not look closely - but it looked suspisiously.
My goal with drm_bridge_init() was to get devlinks sorted out for cross-module uses of a drm_bridge (via of_drm_find_bridge()), so I only considered locations where drm_bridge_add/remove() were used.
Would you be okay with a promise to push a cleanup of this one and the one in sti_hda.c after patch 1/30 lands in some form? I'd rather not make this series much longer, it's already pushing it at 30 :).
Sam
Hi Mihail.
I can see from grepping that bridge.driver_private is used in a couple of other files in sti/
Like sti_hdmi.c: bridge->driver_private = hdmi; bridge->funcs = &sti_hdmi_bridge_funcs; drm_bridge_attach(encoder, bridge, NULL);
I wonder if a drm_bridge_init() should be added there. I did not look closely - but it looked suspisiously.
My goal with drm_bridge_init() was to get devlinks sorted out for cross-module uses of a drm_bridge (via of_drm_find_bridge()), so I only considered locations where drm_bridge_add/remove() were used.
Would you be okay with a promise to push a cleanup of this one and the one in sti_hda.c after patch 1/30 lands in some form? I'd rather not make this series much longer, it's already pushing it at 30 :).
Absolutely - my drive-by comment was more out of concern if this was missing. A clean-up later souns good.
Sam
Le mer. 27 nov. 2019 à 17:19, Sam Ravnborg sam@ravnborg.org a écrit :
Hi Mihail.
I can see from grepping that bridge.driver_private is used in a couple of other files in sti/
Like sti_hdmi.c: bridge->driver_private = hdmi; bridge->funcs = &sti_hdmi_bridge_funcs; drm_bridge_attach(encoder, bridge, NULL);
I wonder if a drm_bridge_init() should be added there. I did not look closely - but it looked suspisiously.
My goal with drm_bridge_init() was to get devlinks sorted out for cross-module uses of a drm_bridge (via of_drm_find_bridge()), so I only considered locations where drm_bridge_add/remove() were used.
Would you be okay with a promise to push a cleanup of this one and the one in sti_hda.c after patch 1/30 lands in some form? I'd rather not make this series much longer, it's already pushing it at 30 :).
Absolutely - my drive-by comment was more out of concern if this was missing. A clean-up later souns good.
Or you can just do the changes for sti_hdmi and sti_hda in this patch too.
Benjamin
Sam
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
From: Russell King rmk+kernel@armlinux.org.uk
Bridge devices have been a potential for kernel oops as their lifetime is independent of the DRM device that they are bound to. Hence, if a bridge device is unbound while the parent DRM device is using them, the parent happily continues to use the bridge device, calling the driver and accessing its objects that have been freed.
This can cause kernel memory corruption and kernel oops.
To control this, use device links to ensure that the parent DRM device is unbound when the bridge device is unbound, and when the bridge device is re-bound, automatically rebind the parent DRM device.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk Tested-by: Mihail Atanassov mihail.atanassov@arm.com [reworked to use drm_bridge_init() for setting bridge->device] Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- drivers/gpu/drm/drm_bridge.c | 49 ++++++++++++++++++++++++++---------- include/drm/drm_bridge.h | 4 +++ 2 files changed, 40 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cbe680aa6eac..e1f8db84651a 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" @@ -109,6 +110,7 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev, bridge->encoder = NULL; bridge->next = NULL;
+ bridge->device = dev; #ifdef CONFIG_OF bridge->of_node = dev->of_node; #endif @@ -492,6 +494,32 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge, EXPORT_SYMBOL(drm_atomic_bridge_enable);
#ifdef CONFIG_OF +static struct drm_bridge *drm_bridge_find(struct drm_device *dev, + struct device_node *np, bool link) +{ + struct drm_bridge *bridge, *found = NULL; + struct device_link *dl; + + mutex_lock(&bridge_lock); + + list_for_each_entry(bridge, &bridge_list, list) + if (bridge->of_node == np) { + found = bridge; + break; + } + + if (found && link) { + dl = device_link_add(dev->dev, found->device, + DL_FLAG_AUTOPROBE_CONSUMER); + if (!dl) + found = NULL; + } + + mutex_unlock(&bridge_lock); + + return found; +} + /** * of_drm_find_bridge - find the bridge corresponding to the device node in * the global bridge list @@ -503,21 +531,16 @@ EXPORT_SYMBOL(drm_atomic_bridge_enable); */ struct drm_bridge *of_drm_find_bridge(struct device_node *np) { - struct drm_bridge *bridge; - - mutex_lock(&bridge_lock); - - list_for_each_entry(bridge, &bridge_list, list) { - if (bridge->of_node == np) { - mutex_unlock(&bridge_lock); - return bridge; - } - } - - mutex_unlock(&bridge_lock); - return NULL; + return drm_bridge_find(NULL, np, false); } EXPORT_SYMBOL(of_drm_find_bridge); + +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev, + struct device_node *np) +{ + return drm_bridge_find(dev, np, true); +} +EXPORT_SYMBOL(of_drm_find_bridge_devlink); #endif
MODULE_AUTHOR("Ajay Kumar ajaykumar.rs@samsung.com"); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index d6d9d5301551..68b27c69cc3d 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -382,6 +382,8 @@ struct drm_bridge { struct drm_encoder *encoder; /** @next: the next bridge in the encoder chain */ struct drm_bridge *next; + /** @device: Linux driver model device */ + struct device *device; #ifdef CONFIG_OF /** @of_node: device node pointer to the bridge */ struct device_node *of_node; @@ -407,6 +409,8 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev, const struct drm_bridge_timings *timings, void *driver_private); struct drm_bridge *of_drm_find_bridge(struct device_node *np); +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev, + struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);
On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote:
From: Russell King rmk+kernel@armlinux.org.uk
Bridge devices have been a potential for kernel oops as their lifetime is independent of the DRM device that they are bound to. Hence, if a bridge device is unbound while the parent DRM device is using them, the parent happily continues to use the bridge device, calling the driver and accessing its objects that have been freed.
This can cause kernel memory corruption and kernel oops.
To control this, use device links to ensure that the parent DRM device is unbound when the bridge device is unbound, and when the bridge device is re-bound, automatically rebind the parent DRM device.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk Tested-by: Mihail Atanassov mihail.atanassov@arm.com [reworked to use drm_bridge_init() for setting bridge->device] Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
So I thought the big plan was to put the device_link setup into drm_bridge_attach, so that it's done for everyone. And we could then slowly go through the existing drivers that use the component framework to get this handled correctly.
So my questions: - is there a problem if we add the device_link for everyone? - is there an issue if we only add it at drm_bridge_attach time? I kinda assumed that it's not needed before that (EPROBE_DEFER should handle load dependencies as before), but it could be that some drivers ask for a bridge and then check more stuff and then drop the bridge without calling drm_bridge_attach. We probably don't have a case like this yet, but better robust than sorry.
Anyway, I scrolled through the bridge patches, looked all good, huge thanks for tackling this! Once we have some agreement on the bigger questions here I'll try to go through them and review.
Cheers, Daniel
drivers/gpu/drm/drm_bridge.c | 49 ++++++++++++++++++++++++++---------- include/drm/drm_bridge.h | 4 +++ 2 files changed, 40 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cbe680aa6eac..e1f8db84651a 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" @@ -109,6 +110,7 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev, bridge->encoder = NULL; bridge->next = NULL;
- bridge->device = dev;
#ifdef CONFIG_OF bridge->of_node = dev->of_node; #endif @@ -492,6 +494,32 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge, EXPORT_SYMBOL(drm_atomic_bridge_enable);
#ifdef CONFIG_OF +static struct drm_bridge *drm_bridge_find(struct drm_device *dev,
struct device_node *np, bool link)+{
- struct drm_bridge *bridge, *found = NULL;
- struct device_link *dl;
- mutex_lock(&bridge_lock);
- list_for_each_entry(bridge, &bridge_list, list)
if (bridge->of_node == np) {found = bridge;break;}- if (found && link) {
dl = device_link_add(dev->dev, found->device,DL_FLAG_AUTOPROBE_CONSUMER);if (!dl)found = NULL;- }
- mutex_unlock(&bridge_lock);
- return found;
+}
/**
- of_drm_find_bridge - find the bridge corresponding to the device node in
the global bridge list@@ -503,21 +531,16 @@ EXPORT_SYMBOL(drm_atomic_bridge_enable); */ struct drm_bridge *of_drm_find_bridge(struct device_node *np) {
- struct drm_bridge *bridge;
- mutex_lock(&bridge_lock);
- list_for_each_entry(bridge, &bridge_list, list) {
if (bridge->of_node == np) {mutex_unlock(&bridge_lock);return bridge;}- }
- mutex_unlock(&bridge_lock);
- return NULL;
- return drm_bridge_find(NULL, np, false);
} EXPORT_SYMBOL(of_drm_find_bridge);
+struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
struct device_node *np)+{
- return drm_bridge_find(dev, np, true);
+} +EXPORT_SYMBOL(of_drm_find_bridge_devlink); #endif
MODULE_AUTHOR("Ajay Kumar ajaykumar.rs@samsung.com"); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index d6d9d5301551..68b27c69cc3d 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -382,6 +382,8 @@ struct drm_bridge { struct drm_encoder *encoder; /** @next: the next bridge in the encoder chain */ struct drm_bridge *next;
- /** @device: Linux driver model device */
- struct device *device;
#ifdef CONFIG_OF /** @of_node: device node pointer to the bridge */ struct device_node *of_node; @@ -407,6 +409,8 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev, const struct drm_bridge_timings *timings, void *driver_private); struct drm_bridge *of_drm_find_bridge(struct device_node *np); +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
struct device_node *np);int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);
-- 2.23.0
On Tuesday, 26 November 2019 14:35:34 GMT Daniel Vetter wrote:
On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote:
From: Russell King rmk+kernel@armlinux.org.uk
Bridge devices have been a potential for kernel oops as their lifetime is independent of the DRM device that they are bound to. Hence, if a bridge device is unbound while the parent DRM device is using them, the parent happily continues to use the bridge device, calling the driver and accessing its objects that have been freed.
This can cause kernel memory corruption and kernel oops.
To control this, use device links to ensure that the parent DRM device is unbound when the bridge device is unbound, and when the bridge device is re-bound, automatically rebind the parent DRM device.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk Tested-by: Mihail Atanassov mihail.atanassov@arm.com [reworked to use drm_bridge_init() for setting bridge->device] Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
So I thought the big plan was to put the device_link setup into drm_bridge_attach, so that it's done for everyone. And we could then slowly go through the existing drivers that use the component framework to get this handled correctly.
So my questions:
- is there a problem if we add the device_link for everyone?
Possibly not, but I didn't want to stir the entire pot :). This is safer in the sense that it's an opt-in, so a lower chance of regressions in code and setups that I can't possibly test. If you think it's worth sticking it in the existing code paths, I can certainly do that too.
- is there an issue if we only add it at drm_bridge_attach time? I kinda assumed that it's not needed before that (EPROBE_DEFER should handle load dependencies as before), but it could be that some drivers ask for a bridge and then check more stuff and then drop the bridge without calling drm_bridge_attach. We probably don't have a case like this yet, but better robust than sorry.
I think there would be a race there:
- bridge driver calls drm_bridge_add() in their probe() - client driver calls of_drm_find_bridge() in their probe() - bridge driver gets removed, calls drm_bridge_remove() - client driver uses the now invalid drm_bridge pointer from above to do drm_bridge_attach()
With of_drm_bridge_find_devlink(), you get the device_link inside the bridge_lock so the reference to the drm_bridge will either be valid, or your driver gets removed right after it's probed, so that the bridge can be removed, too.
In patch 30/30 I use both the _devlink and the non-_devlink versions of of_drm_find_bridge(), but I guess there's no harm adding another refcount on the link, it'll get destroyed if the bridge is removed regardless, although that may need a DL_FLAG_AUTOREMOVE_CONSUMER.
Anyway, I scrolled through the bridge patches, looked all good, huge thanks for tackling this! Once we have some agreement on the bigger questions here I'll try to go through them and review.
Cheers, Daniel
drivers/gpu/drm/drm_bridge.c | 49 ++++++++++++++++++++++++++---------- include/drm/drm_bridge.h | 4 +++ 2 files changed, 40 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cbe680aa6eac..e1f8db84651a 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" @@ -109,6 +110,7 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev, bridge->encoder = NULL; bridge->next = NULL;
- bridge->device = dev;
#ifdef CONFIG_OF bridge->of_node = dev->of_node; #endif @@ -492,6 +494,32 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge, EXPORT_SYMBOL(drm_atomic_bridge_enable);
#ifdef CONFIG_OF +static struct drm_bridge *drm_bridge_find(struct drm_device *dev,
struct device_node *np, bool link)+{
- struct drm_bridge *bridge, *found = NULL;
- struct device_link *dl;
- mutex_lock(&bridge_lock);
- list_for_each_entry(bridge, &bridge_list, list)
if (bridge->of_node == np) {found = bridge;break;}- if (found && link) {
dl = device_link_add(dev->dev, found->device,DL_FLAG_AUTOPROBE_CONSUMER);if (!dl)mutexfound = NULL;- }
- mutex_unlock(&bridge_lock);
- return found;
+}
/**
- of_drm_find_bridge - find the bridge corresponding to the device node in
the global bridge list@@ -503,21 +531,16 @@ EXPORT_SYMBOL(drm_atomic_bridge_enable); */ struct drm_bridge *of_drm_find_bridge(struct device_node *np) {
- struct drm_bridge *bridge;
- mutex_lock(&bridge_lock);
- list_for_each_entry(bridge, &bridge_list, list) {
if (bridge->of_node == np) {mutex_unlock(&bridge_lock);return bridge;}- }
- mutex_unlock(&bridge_lock);
- return NULL;
- return drm_bridge_find(NULL, np, false);
} EXPORT_SYMBOL(of_drm_find_bridge);
+struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
struct device_node *np)+{
- return drm_bridge_find(dev, np, true);
+} +EXPORT_SYMBOL(of_drm_find_bridge_devlink); #endif
MODULE_AUTHOR("Ajay Kumar ajaykumar.rs@samsung.com"); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index d6d9d5301551..68b27c69cc3d 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -382,6 +382,8 @@ struct drm_bridge { struct drm_encoder *encoder; /** @next: the next bridge in the encoder chain */ struct drm_bridge *next;
- /** @device: Linux driver model device */
- struct device *device;
#ifdef CONFIG_OF /** @of_node: device node pointer to the bridge */ struct device_node *of_node; @@ -407,6 +409,8 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev, const struct drm_bridge_timings *timings, void *driver_private); struct drm_bridge *of_drm_find_bridge(struct device_node *np); +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
struct device_node *np);int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);
On Tuesday, 26 November 2019 14:35:34 GMT Daniel Vetter wrote:
On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote:
From: Russell King rmk+kernel@armlinux.org.uk
Bridge devices have been a potential for kernel oops as their lifetime is independent of the DRM device that they are bound to. Hence, if a bridge device is unbound while the parent DRM device is using them, the parent happily continues to use the bridge device, calling the driver and accessing its objects that have been freed.
This can cause kernel memory corruption and kernel oops.
To control this, use device links to ensure that the parent DRM device is unbound when the bridge device is unbound, and when the bridge device is re-bound, automatically rebind the parent DRM device.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk Tested-by: Mihail Atanassov mihail.atanassov@arm.com [reworked to use drm_bridge_init() for setting bridge->device] Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com
So I thought the big plan was to put the device_link setup into drm_bridge_attach, so that it's done for everyone. And we could then slowly go through the existing drivers that use the component framework to get this handled correctly.
So my questions:
- is there a problem if we add the device_link for everyone?
So after spending time looking at the code and thinking about it, I'm slowly coming to the conclusion that getting device links right for everyone in one go is a much bigger task than this opt-in quick-fix here. I've hit, at the very least, the following snags in trying to apply it universally:
panel_bridge - removing one via drm_of_panel_bridge_remove() uses of_drm_find_bridge(), which would add a devlink at a very inopportune time;
mipi_dsi_host - attach/detach, where e.g. dw-mipi-dsi.c handles bridge creation/destruction, doesn't correspond directly to a struct device's lifetime, so the device link would linger longer than is required;
others that add/remove bridges at times different from probe/remove (drivers using the component framework?).
I think it'd still be valuable even with limiting the scope to drivers that get their bridge in probe() and drop it in remove() for now, and only roll it out as an opt-in. Thoughts?
I think to get it right we need to use the links' refcount, with e.g. of_drm_find_bridge() giving you a refcount of 1, and bridge_detach() maybe dropping the refcount, but I can envision ways where this breaks too, so maybe just an of_drm_{get,put}_bridge()?
- is there an issue if we only add it at drm_bridge_attach time? I kinda assumed that it's not needed before that (EPROBE_DEFER should handle load dependencies as before), but it could be that some drivers ask for a bridge and then check more stuff and then drop the bridge without calling drm_bridge_attach. We probably don't have a case like this yet, but better robust than sorry.
Anyway, I scrolled through the bridge patches, looked all good, huge thanks for tackling this! Once we have some agreement on the bigger questions here I'll try to go through them and review.
Cheers, Daniel
drivers/gpu/drm/drm_bridge.c | 49 ++++++++++++++++++++++++++---------- include/drm/drm_bridge.h | 4 +++ 2 files changed, 40 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cbe680aa6eac..e1f8db84651a 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" @@ -109,6 +110,7 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev, bridge->encoder = NULL; bridge->next = NULL;
- bridge->device = dev;
#ifdef CONFIG_OF bridge->of_node = dev->of_node; #endif @@ -492,6 +494,32 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge, EXPORT_SYMBOL(drm_atomic_bridge_enable);
#ifdef CONFIG_OF +static struct drm_bridge *drm_bridge_find(struct drm_device *dev,
struct device_node *np, bool link)+{
- struct drm_bridge *bridge, *found = NULL;
- struct device_link *dl;
- mutex_lock(&bridge_lock);
- list_for_each_entry(bridge, &bridge_list, list)
if (bridge->of_node == np) {found = bridge;break;}- if (found && link) {
dl = device_link_add(dev->dev, found->device,DL_FLAG_AUTOPROBE_CONSUMER);if (!dl)found = NULL;- }
- mutex_unlock(&bridge_lock);
- return found;
+}
/**
- of_drm_find_bridge - find the bridge corresponding to the device node in
the global bridge list@@ -503,21 +531,16 @@ EXPORT_SYMBOL(drm_atomic_bridge_enable); */ struct drm_bridge *of_drm_find_bridge(struct device_node *np) {
- struct drm_bridge *bridge;
- mutex_lock(&bridge_lock);
- list_for_each_entry(bridge, &bridge_list, list) {
if (bridge->of_node == np) {mutex_unlock(&bridge_lock);return bridge;}- }
- mutex_unlock(&bridge_lock);
- return NULL;
- return drm_bridge_find(NULL, np, false);
} EXPORT_SYMBOL(of_drm_find_bridge);
+struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
struct device_node *np)+{
- return drm_bridge_find(dev, np, true);
+} +EXPORT_SYMBOL(of_drm_find_bridge_devlink); #endif
MODULE_AUTHOR("Ajay Kumar ajaykumar.rs@samsung.com"); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index d6d9d5301551..68b27c69cc3d 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -382,6 +382,8 @@ struct drm_bridge { struct drm_encoder *encoder; /** @next: the next bridge in the encoder chain */ struct drm_bridge *next;
- /** @device: Linux driver model device */
- struct device *device;
#ifdef CONFIG_OF /** @of_node: device node pointer to the bridge */ struct device_node *of_node; @@ -407,6 +409,8 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev, const struct drm_bridge_timings *timings, void *driver_private); struct drm_bridge *of_drm_find_bridge(struct device_node *np); +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
struct device_node *np);int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);
To support more transmitters, we need to allow non-component framework bridges to be attached to a dummy drm_encoder in our driver.
A/B tested for equivalence against tda998x, and also tested ti_tfp410 as an alternate transmitter.
Signed-off-by: Mihail Atanassov mihail.atanassov@arm.com --- .../gpu/drm/arm/display/komeda/komeda_drv.c | 54 ++++++------- .../gpu/drm/arm/display/komeda/komeda_kms.c | 77 +++++++++++++++++-- .../gpu/drm/arm/display/komeda/komeda_kms.h | 2 + 3 files changed, 100 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c index d6c2222c5d33..2870123bef37 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c @@ -6,10 +6,10 @@ */ #include <linux/module.h> #include <linux/kernel.h> +#include <linux/of_graph.h> #include <linux/platform_device.h> -#include <linux/component.h> #include <linux/pm_runtime.h> -#include <drm/drm_of.h> +#include <drm/drm_bridge.h> #include "komeda_dev.h" #include "komeda_kms.h"
@@ -72,35 +72,29 @@ static int komeda_bind(struct device *dev) return err; }
-static const struct component_master_ops komeda_master_ops = { - .bind = komeda_bind, - .unbind = komeda_unbind, -}; - -static int compare_of(struct device *dev, void *data) -{ - return dev->of_node == data; -} - -static void komeda_add_slave(struct device *master, - struct component_match **match, - struct device_node *np, - u32 port, u32 endpoint) +static int komeda_add_slave(struct device_node *np, u32 port, u32 endpoint) { struct device_node *remote; + struct drm_bridge *bridge; + int ret = 0;
remote = of_graph_get_remote_node(np, port, endpoint); - if (remote) { - drm_of_component_match_add(master, match, compare_of, remote); - of_node_put(remote); - } + if (!remote) + return 0; + + bridge = of_drm_find_bridge(remote); + if (!bridge) + ret = -EPROBE_DEFER; + + of_node_put(remote); + return ret; }
static int komeda_platform_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct component_match *match = NULL; struct device_node *child; + int ret;
if (!dev->of_node) return -ENODEV; @@ -109,17 +103,25 @@ static int komeda_platform_probe(struct platform_device *pdev) if (of_node_cmp(child->name, "pipeline") != 0) continue;
- /* add connector */ - komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 0); - komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 1); + /* attach any remote devices if present */ + ret = komeda_add_slave(child, KOMEDA_OF_PORT_OUTPUT, 0); + if (ret) { + of_node_put(child); + return ret; + } + ret = komeda_add_slave(child, KOMEDA_OF_PORT_OUTPUT, 1); + if (ret) { + of_node_put(child); + return ret; + } }
- return component_master_add_with_match(dev, &komeda_master_ops, match); + return komeda_bind(dev); }
static int komeda_platform_remove(struct platform_device *pdev) { - component_master_del(&pdev->dev, &komeda_master_ops); + komeda_unbind(&pdev->dev); return 0; }
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c index e30a5b43caa9..2fc6cd9956fd 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c @@ -4,8 +4,8 @@ * Author: James.Qian.Wang james.qian.wang@arm.com * */ -#include <linux/component.h> #include <linux/interrupt.h> +#include <linux/of_graph.h>
#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> @@ -14,6 +14,9 @@ #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_irq.h> +#include <drm/drm_of.h> +#include <drm/drm_encoder.h> +#include <drm/drm_bridge.h> #include <drm/drm_probe_helper.h> #include <drm/drm_vblank.h>
@@ -257,6 +260,69 @@ static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms, config->helper_private = &komeda_mode_config_helpers; }
+static void komeda_encoder_destroy(struct drm_encoder *encoder) +{ + drm_encoder_cleanup(encoder); +} + +static const struct drm_encoder_funcs komeda_dummy_enc_funcs = { + .destroy = komeda_encoder_destroy, +}; + +static int komeda_encoder_attach_bridge(struct komeda_dev *mdev, + struct komeda_kms_dev *kms, + struct drm_encoder *encoder, + struct device_node *np) +{ + struct device_node *remote; + struct drm_bridge *bridge; + u32 crtcs = 0; + int ret = 0; + + remote = of_graph_get_remote_node(np, KOMEDA_OF_PORT_OUTPUT, 0); + if (!remote) + return 0; + + bridge = of_drm_find_bridge_devlink(&kms->base, remote); + if (!bridge) { + ret = -EINVAL; + goto exit; + } + + crtcs = drm_of_find_possible_crtcs(&kms->base, remote); + + encoder->possible_crtcs = crtcs ? crtcs : 1; + + ret = drm_encoder_init(&kms->base, encoder, &komeda_dummy_enc_funcs, + DRM_MODE_ENCODER_TMDS, NULL); + if (ret) + goto exit; + + ret = drm_bridge_attach(encoder, bridge, NULL); + if (ret) + goto exit; + +exit: + of_node_put(remote); + return ret; +} + +static int komeda_encoder_attach_bridges(struct komeda_kms_dev *kms, + struct komeda_dev *mdev) +{ + int i, err; + + for (i = 0; i < kms->n_crtcs; i++) { + err = komeda_encoder_attach_bridge( + mdev, kms, + &kms->encoders[i], mdev->pipelines[i]->of_node); + if (err) + return err; + } + + return 0; +} + struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev) { struct komeda_kms_dev *kms = kzalloc(sizeof(*kms), GFP_KERNEL); @@ -295,7 +361,7 @@ struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev) if (err) goto cleanup_mode_config;
- err = component_bind_all(mdev->dev, kms); + err = komeda_encoder_attach_bridges(kms, mdev); if (err) goto cleanup_mode_config;
@@ -305,11 +371,11 @@ struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev) komeda_kms_irq_handler, IRQF_SHARED, drm->driver->name, drm); if (err) - goto free_component_binding; + goto cleanup_mode_config;
err = mdev->funcs->enable_irq(mdev); if (err) - goto free_component_binding; + goto cleanup_mode_config;
drm->irq_enabled = true;
@@ -325,8 +391,6 @@ struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev) drm_kms_helper_poll_fini(drm); drm->irq_enabled = false; mdev->funcs->disable_irq(mdev); -free_component_binding: - component_unbind_all(mdev->dev, drm); cleanup_mode_config: drm_mode_config_cleanup(drm); komeda_kms_cleanup_private_objs(kms); @@ -347,7 +411,6 @@ void komeda_kms_detach(struct komeda_kms_dev *kms) drm_atomic_helper_shutdown(drm); drm->irq_enabled = false; mdev->funcs->disable_irq(mdev); - component_unbind_all(mdev->dev, drm); drm_mode_config_cleanup(drm); komeda_kms_cleanup_private_objs(kms); drm->dev_private = NULL; diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h index 456f3c435719..b40ba0f476b1 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h @@ -12,6 +12,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_device.h> +#include <drm/drm_encoder.h> #include <drm/drm_writeback.h> #include <drm/drm_print.h>
@@ -123,6 +124,7 @@ struct komeda_kms_dev { int n_crtcs; /** @crtcs: crtcs list */ struct komeda_crtc crtcs[KOMEDA_MAX_PIPELINES]; + struct drm_encoder encoders[KOMEDA_MAX_PIPELINES]; };
#define to_kplane(p) container_of(p, struct komeda_plane, base)
What happened with the patches I posted doing exactly this?
On Tue, Nov 26, 2019 at 01:15:58PM +0000, Mihail Atanassov wrote:
Hi all,
This series adds device links support to drm_bridge. The motivation behind it is that a drm_bridge in a module could get removed under the feet of the bridge user without warning, so we need a way to remove and reprobe the client as needed to avoid peering into the void.
1: Add a drm_bridge_init() function which wraps all initialisation of the structure prior to calling drm_bridge_add().
2-26,28: Apply the drm_bridge_init() refactor to every bridge that uses drm_bridge_add().
27: Minor cleanup in rcar-du.
29: Add of_drm_find_bridge_devlink() which functions the same as of_drm_find_bridge() plus adds a device device link from the owning drm_device to the bridge device.
30: As a motivating example, convert komeda to exclusively use drm_bridge for its pipe outputs; this isn't a regression in usability any more since device links bring the same automatic remove/reprobe feature as components.
Mihail Atanassov (29): drm: Introduce drm_bridge_init() drm/bridge: adv7511: Use drm_bridge_init() drm/bridge: anx6345: Use drm_bridge_init() drm/bridge: anx78xx: Use drm_bridge_init() drm/bridge: cdns: Use drm_bridge_init() drm/bridge: dumb-vga-dac: Use drm_bridge_init() drm/bridge: lvds-encoder: Use drm_bridge_init() drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: Use drm_bridge_init() drm/bridge: nxp-ptn3460: Use drm_bridge_init() drm/bridge: panel: Use drm_bridge_init() drm/bridge: ps8622: Use drm_bridge_init() drm/bridge: sii902x: Use drm_bridge_init() gpu: drm: bridge: sii9234: Use drm_bridge_init() drm/bridge: sil_sii8620: Use drm_bridge_init() drm/bridge: dw-hdmi: Use drm_bridge_init() drm/bridge/synopsys: dsi: Use drm_bridge_init() drm/bridge: tc358764: Use drm_bridge_init() drm/bridge: tc358767: Use drm_bridge_init() drm/bridge: thc63: Use drm_bridge_init() drm/bridge: ti-sn65dsi86: Use drm_bridge_init() drm/bridge: ti-tfp410: Use drm_bridge_init() drm/exynos: mic: Use drm_bridge_init() drm/i2c: tda998x: Use drm_bridge_init() drm/mcde: dsi: Use drm_bridge_init() drm/mediatek: hdmi: Use drm_bridge_init() drm: rcar-du: lvds: Use drm_bridge_init() drm: rcar-du: lvds: Don't set drm_bridge private pointer drm/sti: sti_vdo: Use drm_bridge_init() drm/komeda: Use drm_bridge interface for pipe outputs
Russell King (1): drm/bridge: add support for device links to bridge
.../gpu/drm/arm/display/komeda/komeda_drv.c | 54 ++++++------- .../gpu/drm/arm/display/komeda/komeda_kms.c | 77 ++++++++++++++++-- .../gpu/drm/arm/display/komeda/komeda_kms.h | 2 + drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 +- .../drm/bridge/analogix/analogix-anx6345.c | 5 +- .../drm/bridge/analogix/analogix-anx78xx.c | 8 +- drivers/gpu/drm/bridge/cdns-dsi.c | 4 +- drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +- drivers/gpu/drm/bridge/lvds-encoder.c | 7 +- .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 4 +- drivers/gpu/drm/bridge/nxp-ptn3460.c | 4 +- drivers/gpu/drm/bridge/panel.c | 7 +- drivers/gpu/drm/bridge/parade-ps8622.c | 3 +- drivers/gpu/drm/bridge/sii902x.c | 5 +- drivers/gpu/drm/bridge/sii9234.c | 3 +- drivers/gpu/drm/bridge/sil-sii8620.c | 3 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 7 +- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 7 +- drivers/gpu/drm/bridge/tc358764.c | 4 +- drivers/gpu/drm/bridge/tc358767.c | 3 +- drivers/gpu/drm/bridge/thc63lvd1024.c | 7 +- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +- drivers/gpu/drm/bridge/ti-tfp410.c | 5 +- drivers/gpu/drm/drm_bridge.c | 78 +++++++++++++++---- drivers/gpu/drm/exynos/exynos_drm_mic.c | 8 +- drivers/gpu/drm/i2c/tda998x_drv.c | 6 +- drivers/gpu/drm/mcde/mcde_dsi.c | 3 +- drivers/gpu/drm/mediatek/mtk_hdmi.c | 4 +- drivers/gpu/drm/rcar-du/rcar_lvds.c | 5 +- drivers/gpu/drm/sti/sti_dvo.c | 4 +- include/drm/drm_bridge.h | 8 ++ 31 files changed, 217 insertions(+), 134 deletions(-)
-- 2.23.0
On Tuesday, 26 November 2019 15:27:16 GMT Russell King - ARM Linux admin wrote:
What happened with the patches I posted doing exactly this?
Hi Russell,
[snip]
Russell King (1): drm/bridge: add support for device links to bridge
^^^ Do you mean this one? It's 29/30 in the series, you're Cc'd on it. I've kept the non-trivial part identical to https://patchwork.freedesktop.org/patch/337181/ , which is the only recent patch of yours that I'm aware of on the topic. I've preserved the authorship.
.../gpu/drm/arm/display/komeda/komeda_drv.c | 54 ++++++------- .../gpu/drm/arm/display/komeda/komeda_kms.c | 77 ++++++++++++++++-- .../gpu/drm/arm/display/komeda/komeda_kms.h | 2 + drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 +- .../drm/bridge/analogix/analogix-anx6345.c | 5 +- .../drm/bridge/analogix/analogix-anx78xx.c | 8 +- drivers/gpu/drm/bridge/cdns-dsi.c | 4 +- drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +- drivers/gpu/drm/bridge/lvds-encoder.c | 7 +- .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 4 +- drivers/gpu/drm/bridge/nxp-ptn3460.c | 4 +- drivers/gpu/drm/bridge/panel.c | 7 +- drivers/gpu/drm/bridge/parade-ps8622.c | 3 +- drivers/gpu/drm/bridge/sii902x.c | 5 +- drivers/gpu/drm/bridge/sii9234.c | 3 +- drivers/gpu/drm/bridge/sil-sii8620.c | 3 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 7 +- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 7 +- drivers/gpu/drm/bridge/tc358764.c | 4 +- drivers/gpu/drm/bridge/tc358767.c | 3 +- drivers/gpu/drm/bridge/thc63lvd1024.c | 7 +- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +- drivers/gpu/drm/bridge/ti-tfp410.c | 5 +- drivers/gpu/drm/drm_bridge.c | 78 +++++++++++++++---- drivers/gpu/drm/exynos/exynos_drm_mic.c | 8 +- drivers/gpu/drm/i2c/tda998x_drv.c | 6 +- drivers/gpu/drm/mcde/mcde_dsi.c | 3 +- drivers/gpu/drm/mediatek/mtk_hdmi.c | 4 +- drivers/gpu/drm/rcar-du/rcar_lvds.c | 5 +- drivers/gpu/drm/sti/sti_dvo.c | 4 +- include/drm/drm_bridge.h | 8 ++ 31 files changed, 217 insertions(+), 134 deletions(-)
dri-devel@lists.freedesktop.org
-
Benjamin Gaignard -
Daniel Vetter -
Inki Dae -
james qian wang (Arm Technology China) -
Linus Walleij -
Mihail Atanassov -
Russell King - ARM Linux admin -
Sam Ravnborg