Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
On Wed, Sep 17, 2014 at 12:27:13PM +0300, Laurent Pinchart wrote: Hi Ajay, On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote: On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote: Hi Ajay, Thank you for the patch. I think we're moving in the right direction, but we're not there yet. On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part. A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow. The bridge devices register themselves on a lookup table when they get probed by calling drm_bridge_add_for_lookup. The parent encoder driver waits till the bridge is available in the lookup table(by calling of_drm_find_bridge) and then continues with its initialization. Before the introduction of the component framework I would have said this is the way to go. Now, I think bridges should register themselves as components, and the DRM master driver should use the component framework to get a reference to the bridges it needs. Well, I have modified the bridge framework exactly the way Thierry wanted it to be, I mean the same way the current panel framework is. And, I don't think there is a problem with that. What problem are you facing with current bridge implementation? What is the advantage of using the component framework to register bridges? There are several advantages. - The component framework has been designed with this exact problem in mind, piecing multiple components into a display device. No. Component framework was designed with multi-device drivers in mind. That is, drivers that need to combine two or more platform devices into a single logical device. Typically that includes display controllers and encoders (in various looks) for DRM. Panels and bridges are in my opinion different because they are outside of the DRM driver. They aren't part of the device complex that an SoC provides. They represent hardware that is external to the SoC and the DRM driver and can be shared across SoCs. Forcing panels and bridges to register as components will require all drivers to implement master/component support solely for accessing this external hardware. What you're suggesting is like saying that clocks or regulators should register as components so that their users can get them that way. In fact by that argument everything that's referenced by phandle would need to register as component (PHYs, LEDs, GPIOs, I2C controllers, ...). This patch set introduces yet another framework, without any compelling reason as far as I can see. Today DRM drivers already need to use three different frameworks (component, I2C slave encoder and panel), and we're adding a fourth one to make the mess even messier. Panel and bridge aren't really frameworks. Rather they are a simple registry to allow drivers to register panels and bridges and display drivers to look them up. This is really a headlong rush, we need to stop and fix the design mistakes. Can you point out specific design mistakes? I don't see any, but I'm obviously biased. - The component framework solves the probe ordering problem. Bridges can use deferred probing, but when a bridge requires a resources (such as a clock for instance) provided by the display controller, this will break. Panel and bridges can support deferred probing without the component framework just fine. Without this patchset, you cannot bring an X server based display on snow and peach_pit. Also, day by day the number of platforms using drm_bridge is increasing. That's exactly why I'd like to use the component framework now, as the conversion will become more complex as time goes by. No it won't. If we ever do decide that component framework is a better fit then the conversion may be more work but it would still be largely mechanical. Thierry pgpQjJajyVwfv.pgp Description: PGP signature
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
Hi Thierry, On Monday 22 September 2014 09:40:38 Thierry Reding wrote: On Wed, Sep 17, 2014 at 12:27:13PM +0300, Laurent Pinchart wrote: On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote: On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote: Hi Ajay, Thank you for the patch. I think we're moving in the right direction, but we're not there yet. On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part. A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow. The bridge devices register themselves on a lookup table when they get probed by calling drm_bridge_add_for_lookup. The parent encoder driver waits till the bridge is available in the lookup table(by calling of_drm_find_bridge) and then continues with its initialization. Before the introduction of the component framework I would have said this is the way to go. Now, I think bridges should register themselves as components, and the DRM master driver should use the component framework to get a reference to the bridges it needs. Well, I have modified the bridge framework exactly the way Thierry wanted it to be, I mean the same way the current panel framework is. And, I don't think there is a problem with that. What problem are you facing with current bridge implementation? What is the advantage of using the component framework to register bridges? There are several advantages. - The component framework has been designed with this exact problem in mind, piecing multiple components into a display device. No. Component framework was designed with multi-device drivers in mind. That is, drivers that need to combine two or more platform devices into a single logical device. Typically that includes display controllers and encoders (in various looks) for DRM. I disagree. AFAIK the component framework was designed to easily combine multiple devices into a single logical device, regardless of which bus each device is connected to. That's what makes the component framework useful : it allows master drivers to build logical devices from heterogeneous components without having to use one API per bus and/or component type. If the only goal had been to combine platform devices on an SoC, simpler device-specific solutions would likely have been used instead. Panels and bridges are in my opinion different because they are outside of the DRM driver. They aren't part of the device complex that an SoC provides. They represent hardware that is external to the SoC and the DRM driver and can be shared across SoCs. They represent hardware external to the SoC, but internal to the logical DRM device. Forcing panels and bridges to register as components will require all drivers to implement master/component support solely for accessing this external hardware. What you're suggesting is like saying that clocks or regulators should register as components so that their users can get them that way. In fact by that argument everything that's referenced by phandle would need to register as component (PHYs, LEDs, GPIOs, I2C controllers, ...). No, that's very different. The device you list are clearly external resources, while the bridges and panels are components part of a logical display device. This patch set introduces yet another framework, without any compelling reason as far as I can see. Today DRM drivers already need to use three different frameworks (component, I2C slave encoder and panel), and we're adding a fourth oneto make the mess even messier. Panel and bridge aren't really frameworks. Rather they are a simple registry to allow drivers to register panels and bridges and display drivers to look them up. Regardless of how you call them, we have three interfaces. This is really a headlong rush, we need to stop and fix the design mistakes. Can you point out specific design mistakes? I don't see any, but I'm obviously biased. The slave encoder / bridge split is what I consider a design mistake. Those two interfaces serve the same purpose, they should really be merged. - The component framework solves the probe ordering problem. Bridges can use deferred probing, but when a bridge requires a resources (such as a clock for instance) provided by the display controller, this will break. Panel and bridges can support deferred probing without the component framework just fine. Not if the bridge requires a clock provided by the display controller, in which case there's a dependency loop. Without this patchset, you cannot bring an X server based display on snow and peach_pit. Also, day by day the number of platforms using drm_bridge is increasing. That's exactly why I'd like to use the component framework now,
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
On Tue, Sep 23, 2014 at 03:29:13AM +0300, Laurent Pinchart wrote: Hi Thierry, On Monday 22 September 2014 09:40:38 Thierry Reding wrote: On Wed, Sep 17, 2014 at 12:27:13PM +0300, Laurent Pinchart wrote: On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote: On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote: Hi Ajay, Thank you for the patch. I think we're moving in the right direction, but we're not there yet. On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part. A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow. The bridge devices register themselves on a lookup table when they get probed by calling drm_bridge_add_for_lookup. The parent encoder driver waits till the bridge is available in the lookup table(by calling of_drm_find_bridge) and then continues with its initialization. Before the introduction of the component framework I would have said this is the way to go. Now, I think bridges should register themselves as components, and the DRM master driver should use the component framework to get a reference to the bridges it needs. Well, I have modified the bridge framework exactly the way Thierry wanted it to be, I mean the same way the current panel framework is. And, I don't think there is a problem with that. What problem are you facing with current bridge implementation? What is the advantage of using the component framework to register bridges? There are several advantages. - The component framework has been designed with this exact problem in mind, piecing multiple components into a display device. No. Component framework was designed with multi-device drivers in mind. That is, drivers that need to combine two or more platform devices into a single logical device. Typically that includes display controllers and encoders (in various looks) for DRM. I disagree. AFAIK the component framework was designed to easily combine multiple devices into a single logical device, regardless of which bus each device is connected to. That's what makes the component framework useful : it allows master drivers to build logical devices from heterogeneous components without having to use one API per bus and/or component type. But this doesn't work really well once you leave the SoC. For component/ master to work you need to usually (i.e. using DT) have access to a device tree node for each of the devices so that you can create a list of needed devices. Once you leave the SoC, the number of combinations that you can have becomes non-deterministic. A driver that wants to pull this off would need to more or less manually look up phandles and traverse from SoC via bridges to panels to find all the devices that make up the logical device. If the only goal had been to combine platform devices on an SoC, simpler device-specific solutions would likely have been used instead. I think one of the goals was to replace any of the simpler device- specific solutions with a generic one. But one of the issues with component/master is that it's viral. That is it requires users to register as master themselves in order to pull in the components. While that makes sense for on-SoC devices I think it's a mistake to use it for external hardware like bridges and panels that can be shared across SoCs. If we make component/master mandatory for bridges and panels, then we also force every driver that wants to use them to implement component/master support. Furthermore I did try a while back to convert the Tegra DRM driver to use component/master and couldn't make it work. When I proposed patches to enhance the API so that it would work for Tegra I got silence on one side and just keep using what you currently have on the other side. So in other words since I can't use component/master on Tegra it means that whatever driver gets added that registers a component can't be used on Tegra either. Panels and bridges are in my opinion different because they are outside of the DRM driver. They aren't part of the device complex that an SoC provides. They represent hardware that is external to the SoC and the DRM driver and can be shared across SoCs. They represent hardware external to the SoC, but internal to the logical DRM device. That depends largely on how you look at it. From my point of view the logical DRM device stops at the connector (well I think it actually stops somewhat earlier already, with connector only being the handle through which the outputs are configured). Panels are, at least theoretically, hotpluggable. That is, if you have a device that has both a panel and HDMI but you never use
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
Hi Laurent, On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart laurent.pinch...@ideasonboard.com wrote: Hi Ajay, Thank you for the patch. I think we're moving in the right direction, but we're not there yet. On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part. A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow. The bridge devices register themselves on a lookup table when they get probed by calling drm_bridge_add_for_lookup. The parent encoder driver waits till the bridge is available in the lookup table(by calling of_drm_find_bridge) and then continues with its initialization. Before the introduction of the component framework I would have said this is the way to go. Now, I think bridges should register themselves as components, and the DRM master driver should use the component framework to get a reference to the bridges it needs. Well, I have modified the bridge framework exactly the way Thierry wanted it to be, I mean the same way the current panel framework is. And, I don't think there is a problem with that. What problem are you facing with current bridge implementation? What is the advantage of using the component framework to register bridges? The encoder driver should call drm_bridge_attach_encoder to pass on the drm_device and the encoder pointers to the bridge object. Now that the drm_device pointer is available, the encoder then calls bridge-funcs-post_encoder_init to allow the bridge to continue registering itself with the drm core. This is what really bothers me with DRM bridge. The framework assumes that a bridge will always bridge an encoder and a connector. Beside lacking support for chained bridges, this creates an artificial split between bridges and encoders by modeling the same components using drm_encoder or drm_bridge depending on their position in the video output pipeline. I would like to see drm_bridge becoming more self-centric, removing the awareness of the upstream encoder and downstream connector. I'll give this a try, but it will conflict with this patch, so I'd like to share opinions and coordinate efforts sooner than later if possible. I am not really able to understand how you want drm_bridge to be. As of now, there are many platforms using drm_bridge and they don't have a problem with current implementation. Regarding chained bridges: Can't you add this once my patchset is merged? As an additional feature? To be honest, I have spent quite sometime for working on this patchset. All I started with was to add drm_panel support to drm_bridge. When I sent the first patchset for that, Daniel, Rob and Thierry raised a concern that current bridge framework itself is not proper and hence they asked me to fix that first. And we have reached till here based on their comments only. Without this patchset, you cannot bring an X server based display on snow and peach_pit. Also, day by day the number of platforms using drm_bridge is increasing. And, I don't really see a problem with the current approach(which is exactly the same way panel framework is). And, I am no decision maker here. I would expect the top guys to comment! Ajay Also, non driver model based ptn3460 driver is removed in this patch. Signed-off-by: Ajay Kumar ajaykumar...@samsung.com --- .../devicetree/bindings/drm/bridge/ptn3460.txt | 27 -- drivers/gpu/drm/Makefile |1 + drivers/gpu/drm/bridge/Kconfig | 12 +- drivers/gpu/drm/bridge/Makefile|2 - drivers/gpu/drm/bridge/ptn3460.c | 343 - drivers/gpu/drm/drm_bridge.c | 89 + drivers/gpu/drm/drm_crtc.c |9 +- drivers/gpu/drm/exynos/Kconfig |3 +- drivers/gpu/drm/exynos/exynos_dp_core.c| 57 ++-- drivers/gpu/drm/exynos/exynos_dp_core.h|1 + drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |3 +- include/drm/bridge/ptn3460.h | 37 --- include/drm/drm_crtc.h | 16 +- 13 files changed, 147 insertions(+), 453 deletions(-) delete mode 100644 Documentation/devicetree/bindings/drm/bridge/ptn3460.txt delete mode 100644 drivers/gpu/drm/bridge/ptn3460.c create mode 100644 drivers/gpu/drm/drm_bridge.c delete mode 100644 include/drm/bridge/ptn3460.h -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
Before the introduction of the component framework I would have said this is the way to go. Now, I think bridges should register themselves as components, and the DRM master driver should use the component framework to get a reference to the bridges it needs. Well, I have modified the bridge framework exactly the way Thierry wanted it to be, I mean the same way the current panel framework is. And, I don't think there is a problem with that. What problem are you facing with current bridge implementation? What is the advantage of using the component framework to register bridges? At this point I'd rather merge something that keep iterating out of tree, if this enables current hw to work and is better than what we have we should merge it, and if we can get interest in using the component framework then we should look at that as a separate problem. Dave. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
Hi Ajay, On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote: On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote: Hi Ajay, Thank you for the patch. I think we're moving in the right direction, but we're not there yet. On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part. A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow. The bridge devices register themselves on a lookup table when they get probed by calling drm_bridge_add_for_lookup. The parent encoder driver waits till the bridge is available in the lookup table(by calling of_drm_find_bridge) and then continues with its initialization. Before the introduction of the component framework I would have said this is the way to go. Now, I think bridges should register themselves as components, and the DRM master driver should use the component framework to get a reference to the bridges it needs. Well, I have modified the bridge framework exactly the way Thierry wanted it to be, I mean the same way the current panel framework is. And, I don't think there is a problem with that. What problem are you facing with current bridge implementation? What is the advantage of using the component framework to register bridges? There are several advantages. - The component framework has been designed with this exact problem in mind, piecing multiple components into a display device. This patch set introduces yet another framework, without any compelling reason as far as I can see. Today DRM drivers already need to use three different frameworks (component, I2C slave encoder and panel), and we're adding a fourth one to make the mess even messier. This is really a headlong rush, we need to stop and fix the design mistakes. - The component framework solves the probe ordering problem. Bridges can use deferred probing, but when a bridge requires a resources (such as a clock for instance) provided by the display controller, this will break. The encoder driver should call drm_bridge_attach_encoder to pass on the drm_device and the encoder pointers to the bridge object. Now that the drm_device pointer is available, the encoder then calls bridge-funcs-post_encoder_init to allow the bridge to continue registering itself with the drm core. This is what really bothers me with DRM bridge. The framework assumes that a bridge will always bridge an encoder and a connector. Beside lacking support for chained bridges, this creates an artificial split between bridges and encoders by modeling the same components using drm_encoder or drm_bridge depending on their position in the video output pipeline. I would like to see drm_bridge becoming more self-centric, removing the awareness of the upstream encoder and downstream connector. I'll give this a try, but it will conflict with this patch, so I'd like to share opinions and coordinate efforts sooner than later if possible. I am not really able to understand how you want drm_bridge to be. As of now, there are many platforms using drm_bridge and they don't have a problem with current implementation. Regarding chained bridges: Can't you add this once my patchset is merged? As an additional feature? Yes, as I mentioned in another e-mail this can be fixed later. I want to start discussing it though. To be honest, I have spent quite sometime for working on this patchset. All I started with was to add drm_panel support to drm_bridge. When I sent the first patchset for that, Daniel, Rob and Thierry raised a concern that current bridge framework itself is not proper and hence they asked me to fix that first. And we have reached till here based on their comments only. Without this patchset, you cannot bring an X server based display on snow and peach_pit. Also, day by day the number of platforms using drm_bridge is increasing. That's exactly why I'd like to use the component framework now, as the conversion will become more complex as time goes by. And, I don't really see a problem with the current approach(which is exactly the same way panel framework is). And, I am no decision maker here. I would expect the top guys to comment! -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
On Wed, Sep 17, 2014 at 2:57 PM, Laurent Pinchart laurent.pinch...@ideasonboard.com wrote: Hi Ajay, On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote: On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote: Hi Ajay, Thank you for the patch. I think we're moving in the right direction, but we're not there yet. On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part. A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow. The bridge devices register themselves on a lookup table when they get probed by calling drm_bridge_add_for_lookup. The parent encoder driver waits till the bridge is available in the lookup table(by calling of_drm_find_bridge) and then continues with its initialization. Before the introduction of the component framework I would have said this is the way to go. Now, I think bridges should register themselves as components, and the DRM master driver should use the component framework to get a reference to the bridges it needs. Well, I have modified the bridge framework exactly the way Thierry wanted it to be, I mean the same way the current panel framework is. And, I don't think there is a problem with that. What problem are you facing with current bridge implementation? What is the advantage of using the component framework to register bridges? There are several advantages. - The component framework has been designed with this exact problem in mind, piecing multiple components into a display device. This patch set introduces yet another framework, without any compelling reason as far as I can see. Without this bridge registration framework, there is no way you can pass on a drm_device pointer to the bridge driver. That is why we added a lookup framework. Today DRM drivers already need to use three different frameworks (component, I2C slave encoder and panel), and we're adding a fourth one to make the mess even messier. This is really a headlong rush, we need to stop and fix the design mistakes. - The component framework solves the probe ordering problem. Bridges can use deferred probing, but when a bridge requires a resources (such as a clock for instance) provided by the display controller, this will break. This is exactly the way sti drm uses bridge I think. It uses component framework to wait till the master device initializes and then passes on a drm_device pointer to the component devices. But please know that it is feasible in case of sti, because the bridge they use must be a embedded chip on the SOC, but not a third party device. And, the case which you mentioned(clock instance need to be passed to bridge driver) happens only in the case of bridges embedded on the SOC, but not a third party device. So, you are always allowed to use component framework for that. But, assume the bridge chip is a third party device(ex: ptn3460 or ps8622) which sits on an i2c bus. In that case, your approach poses the foll problems: The way master and components are binded varies from platform to platform. i.e the way exynos consolidates and adds the components is very much different the way msm/sti/armada does the same! So, when one needs to use a third party device as a bridge, they will end up hacking up their drm layer to support this third party device. With my approach, only the corresponding encoder driver needs to know about the bridge, that too just the phandle to bridge node. The platform specific drm layer can still be unaware of the bridge and stuff. With your approach, the way we would specify the bridge node will change from platform to platform resulting in non-uniformity. Also, the platform specific drm layer needs to be aware of the bridge. And, I assume these are the reasons why drm_panel doesn't use component framework. Because all the panels are often third party and hence can be reused across platforms, and so are ptn3460/ps8622. The encoder driver should call drm_bridge_attach_encoder to pass on the drm_device and the encoder pointers to the bridge object. Now that the drm_device pointer is available, the encoder then calls bridge-funcs-post_encoder_init to allow the bridge to continue registering itself with the drm core. This is what really bothers me with DRM bridge. The framework assumes that a bridge will always bridge an encoder and a connector. Beside lacking support for chained bridges, this creates an artificial split between bridges and encoders by modeling the same components using drm_encoder or drm_bridge depending on their position in the video output pipeline. I would like to see drm_bridge becoming more self-centric, removing the awareness of the upstream encoder and downstream connector. I'll give this a try, but it will conflict with this patch, so I'd like to share opinions and coordinate
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
Hi Ajay, Thank you for the patch. I think we're moving in the right direction, but we're not there yet. On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part. A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow. The bridge devices register themselves on a lookup table when they get probed by calling drm_bridge_add_for_lookup. The parent encoder driver waits till the bridge is available in the lookup table(by calling of_drm_find_bridge) and then continues with its initialization. Before the introduction of the component framework I would have said this is the way to go. Now, I think bridges should register themselves as components, and the DRM master driver should use the component framework to get a reference to the bridges it needs. The encoder driver should call drm_bridge_attach_encoder to pass on the drm_device and the encoder pointers to the bridge object. Now that the drm_device pointer is available, the encoder then calls bridge-funcs-post_encoder_init to allow the bridge to continue registering itself with the drm core. This is what really bothers me with DRM bridge. The framework assumes that a bridge will always bridge an encoder and a connector. Beside lacking support for chained bridges, this creates an artificial split between bridges and encoders by modeling the same components using drm_encoder or drm_bridge depending on their position in the video output pipeline. I would like to see drm_bridge becoming more self-centric, removing the awareness of the upstream encoder and downstream connector. I'll give this a try, but it will conflict with this patch, so I'd like to share opinions and coordinate efforts sooner than later if possible. Also, non driver model based ptn3460 driver is removed in this patch. Signed-off-by: Ajay Kumar ajaykumar...@samsung.com --- .../devicetree/bindings/drm/bridge/ptn3460.txt | 27 -- drivers/gpu/drm/Makefile |1 + drivers/gpu/drm/bridge/Kconfig | 12 +- drivers/gpu/drm/bridge/Makefile|2 - drivers/gpu/drm/bridge/ptn3460.c | 343 - drivers/gpu/drm/drm_bridge.c | 89 + drivers/gpu/drm/drm_crtc.c |9 +- drivers/gpu/drm/exynos/Kconfig |3 +- drivers/gpu/drm/exynos/exynos_dp_core.c| 57 ++-- drivers/gpu/drm/exynos/exynos_dp_core.h|1 + drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |3 +- include/drm/bridge/ptn3460.h | 37 --- include/drm/drm_crtc.h | 16 +- 13 files changed, 147 insertions(+), 453 deletions(-) delete mode 100644 Documentation/devicetree/bindings/drm/bridge/ptn3460.txt delete mode 100644 drivers/gpu/drm/bridge/ptn3460.c create mode 100644 drivers/gpu/drm/drm_bridge.c delete mode 100644 include/drm/bridge/ptn3460.h -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
Hi Javier, On Sat, Aug 23, 2014 at 5:03 AM, Javier Martinez Canillas jav...@dowhile0.org wrote: Hello Ajay, On Thu, Jul 31, 2014 at 12:58 PM, Thierry Reding thierry.red...@gmail.com wrote: On Wed, Jul 30, 2014 at 09:33:28PM +0530, Ajay kumar wrote: On Wed, Jul 30, 2014 at 8:38 PM, Thierry Reding thierry.red...@gmail.com wrote: [...] I think it should even be possible to do this in more separate steps. For example you could add the new bridge infrastructure without touching any of the existing drivers (so that they are completely unaffected by the changes) and then start converting one by one. For some of the changes this may be difficult (such as the dev - drm_dev rename to make room for the new struct device *dev). But that could for example be done in a preparatory patch that first renames the field, so that the infrastructure patch can add the new field without renaming any fields and therefore needing changes to drivers directly. The goal of that whole exercise is to allow display drivers to keep working with the existing API (ptn3460_init()) while we convert the bridge drivers to register with the new framework. Then we can more safely convert each display driver individually to make use of the new framework and once all drivers have been converted the old API can simply be removed. That way there should be no impact on existing functionality at any point. As of now only exynos_dp uses ptn3460_init. And, also only 2 drivers use drm_bridge_init. It should be really easy to bisect if something goes wrong. Still, I will try to divide it so that each patch contains minimal change. Thanks. Do you plan to address Thierry's concerns and re-spin this patch? Same question for patches: drm/bridge: Add i2c based driver for ptn3460 bridge drm/bridge: Add i2c based driver for ps8622/ps8625 bridge Yes, I will. I was caught up with some other work. I will be sending a version ASAP. Ajay -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
Hello Ajay, On Mon, Aug 25, 2014 at 8:11 AM, Ajay kumar ajayn...@gmail.com wrote: Do you plan to address Thierry's concerns and re-spin this patch? Same question for patches: drm/bridge: Add i2c based driver for ptn3460 bridge drm/bridge: Add i2c based driver for ps8622/ps8625 bridge Yes, I will. I was caught up with some other work. I will be sending a version ASAP. Great, glad to know that you will be doing a re-spin! Ajay Thanks a lot and best regards, Javier -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
Hello Ajay, On Thu, Jul 31, 2014 at 12:58 PM, Thierry Reding thierry.red...@gmail.com wrote: On Wed, Jul 30, 2014 at 09:33:28PM +0530, Ajay kumar wrote: On Wed, Jul 30, 2014 at 8:38 PM, Thierry Reding thierry.red...@gmail.com wrote: [...] I think it should even be possible to do this in more separate steps. For example you could add the new bridge infrastructure without touching any of the existing drivers (so that they are completely unaffected by the changes) and then start converting one by one. For some of the changes this may be difficult (such as the dev - drm_dev rename to make room for the new struct device *dev). But that could for example be done in a preparatory patch that first renames the field, so that the infrastructure patch can add the new field without renaming any fields and therefore needing changes to drivers directly. The goal of that whole exercise is to allow display drivers to keep working with the existing API (ptn3460_init()) while we convert the bridge drivers to register with the new framework. Then we can more safely convert each display driver individually to make use of the new framework and once all drivers have been converted the old API can simply be removed. That way there should be no impact on existing functionality at any point. As of now only exynos_dp uses ptn3460_init. And, also only 2 drivers use drm_bridge_init. It should be really easy to bisect if something goes wrong. Still, I will try to divide it so that each patch contains minimal change. Thanks. Do you plan to address Thierry's concerns and re-spin this patch? Same question for patches: drm/bridge: Add i2c based driver for ptn3460 bridge drm/bridge: Add i2c based driver for ps8622/ps8625 bridge Thanks a lot and best regards, Javier -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
On Wed, Jul 30, 2014 at 09:33:28PM +0530, Ajay kumar wrote:
On Wed, Jul 30, 2014 at 8:38 PM, Thierry Reding thierry.red...@gmail.com
wrote:
[...]
I think it should even be possible to do this in more separate steps.
For example you could add the new bridge infrastructure without touching
any of the existing drivers (so that they are completely unaffected by
the changes) and then start converting one by one.
For some of the changes this may be difficult (such as the dev -
drm_dev rename to make room for the new struct device *dev). But that
could for example be done in a preparatory patch that first renames the
field, so that the infrastructure patch can add the new field without
renaming any fields and therefore needing changes to drivers directly.
The goal of that whole exercise is to allow display drivers to keep
working with the existing API (ptn3460_init()) while we convert the
bridge drivers to register with the new framework. Then we can more
safely convert each display driver individually to make use of the new
framework and once all drivers have been converted the old API can
simply be removed.
That way there should be no impact on existing functionality at any
point.
As of now only exynos_dp uses ptn3460_init.
And, also only 2 drivers use drm_bridge_init.
It should be really easy to bisect if something goes wrong.
Still, I will try to divide it so that each patch contains minimal change.
Thanks.
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e529b68..e5a41ad 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -619,6 +619,7 @@ struct drm_plane {
/**
* drm_bridge_funcs - drm_bridge control functions
+ * @post_encoder_init: called by the parent encoder
Maybe rename this to attach to make it more obvious when exactly it's
called?
post_encoder_attach?
post_encoder_ doesn't contain much information, or even ambiguous
information. What does post encoder mean? A bridge is always
attached to an encoder, so encoder can be dropped. Now post has
implications as to the time when it is called, but does it mean after
the encoder has been initialized, or after the encoder has been removed?
Simply attach means it's called by the parent encoder to initialize
the bridge once it's been attached to an encoder. So obviously it's
after the encoder has been initialized. attach has all he information
required. Any prefix is redundant in my opinion and removing prefixes
gives shorter names and reduces the number of keypresses.
Finally, what name it should have?
I originally proposed attach as a more concise name and I still think
that's the best alternative.
* @mode_fixup: Try to fixup (or reject entirely) proposed mode for
this bridge
* @disable: Called right before encoder prepare, disables the bridge
* @post_disable: Called right after encoder prepare, for lockstepped
disable
@@ -628,6 +629,7 @@ struct drm_plane {
* @destroy: make object go away
*/
struct drm_bridge_funcs {
+ int (*post_encoder_init)(struct drm_bridge *bridge);
bool (*mode_fixup)(struct drm_bridge *bridge,
const struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode);
@@ -648,15 +650,19 @@ struct drm_bridge_funcs {
* @base: base mode object
* @funcs: control functions
* @driver_private: pointer to the bridge driver's internal context
+ * @connector_polled: polled flag needed for registering connector
Can you explain why this new field is needed? It seems like a completely
unrelated change.
How do I select this flag for the bridge chip?
Assume if I did select DRM_CONNECTOR_POLL_HPD, where to call
drm_helper_hpd_irq_event in the driver? Is post_encoder_init a right place?
Without the polled flag, I get display very late.
Please throw some light on this!
I just don't understand why it's necessary to implement this field in
the drm_bridge. Every bridge driver will already implement a connector,
in which case it can simply set the connector's .polled field, can't it?
It seems like the only reason you have it in drm_bridge is so that the
encoder driver can set it. But I don't see why it should be doing that.
The polled state is a property of the connector, and the encoder driver
doesn't know anything about it. So if the bridge has a way to detect HPD
then it should be setting up the connector to properly report it. For
example if the bridge has an input pin to detect it, then it could use a
GPIO to receive interrupts and call drm_helper_hpd_irq_event() in the
interrupt handler.
Hmm. Are we allowed to call drm_helper_hpd_irq_event() the way
DSI panels use it? Like the last step in panel probe?
For bridges, it will be in post_encoder_init!
drm_helper_hpd_irq_event() should only be called when a hotplug event is
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
On Sat, Jul 26, 2014 at 12:52:08AM +0530, Ajay Kumar wrote:
This patch tries to seperate drm_bridge implementation
into 2 parts, a drm part and a non_drm part.
A set of helper functions are defined in this patch to make
bridge driver probe independent of the drm flow.
The bridge devices register themselves on a lookup table
when they get probed by calling drm_bridge_add_for_lookup.
The parent encoder driver waits till the bridge is available in the
lookup table(by calling of_drm_find_bridge) and then continues with
its initialization.
The encoder driver should call drm_bridge_attach_encoder to pass on
the drm_device and the encoder pointers to the bridge object.
Now that the drm_device pointer is available, the encoder then calls
bridge-funcs-post_encoder_init to allow the bridge to continue
registering itself with the drm core.
Also, non driver model based ptn3460 driver is removed in this patch.
Why is it removed in this patch? Can't you do this incrementally rather
than remove the driver in this patch and add it again later? If you do
it this way then we'll always have this one commit where devices that
have a ptn3460 don't work, so it becomes impossible to bisect across
this commit.
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
[...]
+int drm_bridge_add_for_lookup(struct drm_bridge *bridge)
+{
+ mutex_lock(bridge_lock);
+ list_add_tail(bridge-head, bridge_lookup);
+ mutex_unlock(bridge_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_bridge_add_for_lookup);
+
+void drm_bridge_remove_from_lookup(struct drm_bridge *bridge)
+{
+ mutex_lock(bridge_lock);
+ list_del_init(bridge-head);
+ mutex_unlock(bridge_lock);
+}
+EXPORT_SYMBOL(drm_bridge_remove_from_lookup);
The _for_lookup and _from_lookup suffixes aren't useful in my
opinion.
+int drm_bridge_attach_encoder(struct drm_bridge *bridge,
+ struct drm_encoder *encoder)
And this could simply be drm_bridge_attach() since we'll only ever
want to attach it to encoders.
+{
+ if (!bridge || !encoder)
+ return -EINVAL;
+
+ if (bridge-encoder)
+ return -EBUSY;
+
+ encoder-bridge = bridge;
+ bridge-encoder = encoder;
+ bridge-drm_dev = encoder-dev;
Should this function perhaps call the bridge's -post_encoder_init()?
And it should probably call drm_bridge_init() too, since the DRM device
is now available.
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
[...]
Maybe since you're introducing a new drm_bridge.c file above already it
would make sense to move out existing drm_bridge related code in a
preparatory patch?
Maybe Sean or Rob can comment on whether there was a specific reason to
include it in drm_crtc.c in the first place.
@@ -1012,8 +1010,7 @@ int drm_bridge_init(struct drm_device *dev, struct
drm_bridge *bridge,
if (ret)
goto out;
- bridge-dev = dev;
- bridge-funcs = funcs;
+ bridge-drm_dev = dev;
This sets -drm_dev, but it was already set in drm_bridge_attach(), so I
think that's one more argument to call this function when attaching.
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
b/drivers/gpu/drm/exynos/exynos_dp_core.c
[...]
@@ -1370,7 +1361,7 @@ static const struct component_ops exynos_dp_ops = {
static int exynos_dp_probe(struct platform_device *pdev)
{
struct device *dev = pdev-dev;
- struct device_node *panel_node;
+ struct device_node *panel_node, *bridge_node;
Nit: I don't think you'll need two variables here, since once you've
obtained the real panel or bridge objects you no longer need the OF
nodes.
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e529b68..e5a41ad 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -619,6 +619,7 @@ struct drm_plane {
/**
* drm_bridge_funcs - drm_bridge control functions
+ * @post_encoder_init: called by the parent encoder
Maybe rename this to attach to make it more obvious when exactly it's
called?
* @mode_fixup: Try to fixup (or reject entirely) proposed mode for this
bridge
* @disable: Called right before encoder prepare, disables the bridge
* @post_disable: Called right after encoder prepare, for lockstepped disable
@@ -628,6 +629,7 @@ struct drm_plane {
* @destroy: make object go away
*/
struct drm_bridge_funcs {
+ int (*post_encoder_init)(struct drm_bridge *bridge);
bool (*mode_fixup)(struct drm_bridge *bridge,
const struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode);
@@ -648,15 +650,19 @@ struct drm_bridge_funcs {
* @base: base mode object
* @funcs: control functions
* @driver_private: pointer to the bridge driver's internal context
+ * @connector_polled: polled flag needed for registering connector
Can you explain why this new field is needed? It seems like a
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
On Wed, Jul 30, 2014 at 4:49 PM, Thierry Reding
thierry.red...@gmail.com wrote:
On Sat, Jul 26, 2014 at 12:52:08AM +0530, Ajay Kumar wrote:
This patch tries to seperate drm_bridge implementation
into 2 parts, a drm part and a non_drm part.
A set of helper functions are defined in this patch to make
bridge driver probe independent of the drm flow.
The bridge devices register themselves on a lookup table
when they get probed by calling drm_bridge_add_for_lookup.
The parent encoder driver waits till the bridge is available in the
lookup table(by calling of_drm_find_bridge) and then continues with
its initialization.
The encoder driver should call drm_bridge_attach_encoder to pass on
the drm_device and the encoder pointers to the bridge object.
Now that the drm_device pointer is available, the encoder then calls
bridge-funcs-post_encoder_init to allow the bridge to continue
registering itself with the drm core.
Also, non driver model based ptn3460 driver is removed in this patch.
Why is it removed in this patch? Can't you do this incrementally rather
than remove the driver in this patch and add it again later? If you do
it this way then we'll always have this one commit where devices that
have a ptn3460 don't work, so it becomes impossible to bisect across
this commit.
Ok. I will try to modify ptn3460 to support driver model in this patch itself.
And then, adding panel support, converting it to gpiod interface and other
cleanups should follow.
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
[...]
+int drm_bridge_add_for_lookup(struct drm_bridge *bridge)
+{
+ mutex_lock(bridge_lock);
+ list_add_tail(bridge-head, bridge_lookup);
+ mutex_unlock(bridge_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_bridge_add_for_lookup);
+
+void drm_bridge_remove_from_lookup(struct drm_bridge *bridge)
+{
+ mutex_lock(bridge_lock);
+ list_del_init(bridge-head);
+ mutex_unlock(bridge_lock);
+}
+EXPORT_SYMBOL(drm_bridge_remove_from_lookup);
The _for_lookup and _from_lookup suffixes aren't useful in my
opinion.
Ok. I will just remove the suffix.
+int drm_bridge_attach_encoder(struct drm_bridge *bridge,
+ struct drm_encoder *encoder)
And this could simply be drm_bridge_attach() since we'll only ever
want to attach it to encoders.
Right.
+{
+ if (!bridge || !encoder)
+ return -EINVAL;
+
+ if (bridge-encoder)
+ return -EBUSY;
+
+ encoder-bridge = bridge;
+ bridge-encoder = encoder;
+ bridge-drm_dev = encoder-dev;
Should this function perhaps call the bridge's -post_encoder_init()?
And it should probably call drm_bridge_init() too, since the DRM device
is now available.
This will cleanup some code in both the drivers. I will change it.
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
[...]
Maybe since you're introducing a new drm_bridge.c file above already it
would make sense to move out existing drm_bridge related code in a
preparatory patch?
Maybe Sean or Rob can comment on whether there was a specific reason to
include it in drm_crtc.c in the first place.
@@ -1012,8 +1010,7 @@ int drm_bridge_init(struct drm_device *dev, struct
drm_bridge *bridge,
if (ret)
goto out;
- bridge-dev = dev;
- bridge-funcs = funcs;
+ bridge-drm_dev = dev;
This sets -drm_dev, but it was already set in drm_bridge_attach(), so I
think that's one more argument to call this function when attaching.
Point accepted.
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
b/drivers/gpu/drm/exynos/exynos_dp_core.c
[...]
@@ -1370,7 +1361,7 @@ static const struct component_ops exynos_dp_ops = {
static int exynos_dp_probe(struct platform_device *pdev)
{
struct device *dev = pdev-dev;
- struct device_node *panel_node;
+ struct device_node *panel_node, *bridge_node;
Nit: I don't think you'll need two variables here, since once you've
obtained the real panel or bridge objects you no longer need the OF
nodes.
Right.
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e529b68..e5a41ad 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -619,6 +619,7 @@ struct drm_plane {
/**
* drm_bridge_funcs - drm_bridge control functions
+ * @post_encoder_init: called by the parent encoder
Maybe rename this to attach to make it more obvious when exactly it's
called?
post_encoder_attach?
* @mode_fixup: Try to fixup (or reject entirely) proposed mode for this
bridge
* @disable: Called right before encoder prepare, disables the bridge
* @post_disable: Called right after encoder prepare, for lockstepped
disable
@@ -628,6 +629,7 @@ struct drm_plane {
* @destroy: make object go away
*/
struct drm_bridge_funcs {
+ int (*post_encoder_init)(struct drm_bridge *bridge);
bool (*mode_fixup)(struct drm_bridge *bridge,
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
On Wed, Jul 30, 2014 at 08:01:44PM +0530, Ajay kumar wrote:
On Wed, Jul 30, 2014 at 4:49 PM, Thierry Reding
thierry.red...@gmail.com wrote:
On Sat, Jul 26, 2014 at 12:52:08AM +0530, Ajay Kumar wrote:
This patch tries to seperate drm_bridge implementation
into 2 parts, a drm part and a non_drm part.
A set of helper functions are defined in this patch to make
bridge driver probe independent of the drm flow.
The bridge devices register themselves on a lookup table
when they get probed by calling drm_bridge_add_for_lookup.
The parent encoder driver waits till the bridge is available in the
lookup table(by calling of_drm_find_bridge) and then continues with
its initialization.
The encoder driver should call drm_bridge_attach_encoder to pass on
the drm_device and the encoder pointers to the bridge object.
Now that the drm_device pointer is available, the encoder then calls
bridge-funcs-post_encoder_init to allow the bridge to continue
registering itself with the drm core.
Also, non driver model based ptn3460 driver is removed in this patch.
Why is it removed in this patch? Can't you do this incrementally rather
than remove the driver in this patch and add it again later? If you do
it this way then we'll always have this one commit where devices that
have a ptn3460 don't work, so it becomes impossible to bisect across
this commit.
Ok. I will try to modify ptn3460 to support driver model in this patch itself.
And then, adding panel support, converting it to gpiod interface and other
cleanups should follow.
I think it should even be possible to do this in more separate steps.
For example you could add the new bridge infrastructure without touching
any of the existing drivers (so that they are completely unaffected by
the changes) and then start converting one by one.
For some of the changes this may be difficult (such as the dev -
drm_dev rename to make room for the new struct device *dev). But that
could for example be done in a preparatory patch that first renames the
field, so that the infrastructure patch can add the new field without
renaming any fields and therefore needing changes to drivers directly.
The goal of that whole exercise is to allow display drivers to keep
working with the existing API (ptn3460_init()) while we convert the
bridge drivers to register with the new framework. Then we can more
safely convert each display driver individually to make use of the new
framework and once all drivers have been converted the old API can
simply be removed.
That way there should be no impact on existing functionality at any
point.
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
[...]
Maybe since you're introducing a new drm_bridge.c file above already it
would make sense to move out existing drm_bridge related code in a
preparatory patch?
Maybe Sean or Rob can comment on whether there was a specific reason to
include it in drm_crtc.c in the first place.
@@ -1012,8 +1010,7 @@ int drm_bridge_init(struct drm_device *dev, struct
drm_bridge *bridge,
if (ret)
goto out;
- bridge-dev = dev;
- bridge-funcs = funcs;
+ bridge-drm_dev = dev;
This sets -drm_dev, but it was already set in drm_bridge_attach(), so I
think that's one more argument to call this function when attaching.
Point accepted.
I forgot to mention earlier. drm_dev seems redundant to me. I'd go with
just drm.
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e529b68..e5a41ad 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -619,6 +619,7 @@ struct drm_plane {
/**
* drm_bridge_funcs - drm_bridge control functions
+ * @post_encoder_init: called by the parent encoder
Maybe rename this to attach to make it more obvious when exactly it's
called?
post_encoder_attach?
post_encoder_ doesn't contain much information, or even ambiguous
information. What does post encoder mean? A bridge is always
attached to an encoder, so encoder can be dropped. Now post has
implications as to the time when it is called, but does it mean after
the encoder has been initialized, or after the encoder has been removed?
Simply attach means it's called by the parent encoder to initialize
the bridge once it's been attached to an encoder. So obviously it's
after the encoder has been initialized. attach has all he information
required. Any prefix is redundant in my opinion and removing prefixes
gives shorter names and reduces the number of keypresses.
* @mode_fixup: Try to fixup (or reject entirely) proposed mode for this
bridge
* @disable: Called right before encoder prepare, disables the bridge
* @post_disable: Called right after encoder prepare, for lockstepped
disable
@@ -628,6 +629,7 @@ struct drm_plane {
* @destroy: make object go away
*/
struct drm_bridge_funcs {
+ int (*post_encoder_init)(struct drm_bridge *bridge);
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
On Wed, Jul 30, 2014 at 8:38 PM, Thierry Reding
thierry.red...@gmail.com wrote:
On Wed, Jul 30, 2014 at 08:01:44PM +0530, Ajay kumar wrote:
On Wed, Jul 30, 2014 at 4:49 PM, Thierry Reding
thierry.red...@gmail.com wrote:
On Sat, Jul 26, 2014 at 12:52:08AM +0530, Ajay Kumar wrote:
This patch tries to seperate drm_bridge implementation
into 2 parts, a drm part and a non_drm part.
A set of helper functions are defined in this patch to make
bridge driver probe independent of the drm flow.
The bridge devices register themselves on a lookup table
when they get probed by calling drm_bridge_add_for_lookup.
The parent encoder driver waits till the bridge is available in the
lookup table(by calling of_drm_find_bridge) and then continues with
its initialization.
The encoder driver should call drm_bridge_attach_encoder to pass on
the drm_device and the encoder pointers to the bridge object.
Now that the drm_device pointer is available, the encoder then calls
bridge-funcs-post_encoder_init to allow the bridge to continue
registering itself with the drm core.
Also, non driver model based ptn3460 driver is removed in this patch.
Why is it removed in this patch? Can't you do this incrementally rather
than remove the driver in this patch and add it again later? If you do
it this way then we'll always have this one commit where devices that
have a ptn3460 don't work, so it becomes impossible to bisect across
this commit.
Ok. I will try to modify ptn3460 to support driver model in this patch
itself.
And then, adding panel support, converting it to gpiod interface and other
cleanups should follow.
I think it should even be possible to do this in more separate steps.
For example you could add the new bridge infrastructure without touching
any of the existing drivers (so that they are completely unaffected by
the changes) and then start converting one by one.
For some of the changes this may be difficult (such as the dev -
drm_dev rename to make room for the new struct device *dev). But that
could for example be done in a preparatory patch that first renames the
field, so that the infrastructure patch can add the new field without
renaming any fields and therefore needing changes to drivers directly.
The goal of that whole exercise is to allow display drivers to keep
working with the existing API (ptn3460_init()) while we convert the
bridge drivers to register with the new framework. Then we can more
safely convert each display driver individually to make use of the new
framework and once all drivers have been converted the old API can
simply be removed.
That way there should be no impact on existing functionality at any
point.
As of now only exynos_dp uses ptn3460_init.
And, also only 2 drivers use drm_bridge_init.
It should be really easy to bisect if something goes wrong.
Still, I will try to divide it so that each patch contains minimal change.
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
[...]
Maybe since you're introducing a new drm_bridge.c file above already it
would make sense to move out existing drm_bridge related code in a
preparatory patch?
Maybe Sean or Rob can comment on whether there was a specific reason to
include it in drm_crtc.c in the first place.
@@ -1012,8 +1010,7 @@ int drm_bridge_init(struct drm_device *dev, struct
drm_bridge *bridge,
if (ret)
goto out;
- bridge-dev = dev;
- bridge-funcs = funcs;
+ bridge-drm_dev = dev;
This sets -drm_dev, but it was already set in drm_bridge_attach(), so I
think that's one more argument to call this function when attaching.
Point accepted.
I forgot to mention earlier. drm_dev seems redundant to me. I'd go with
just drm.
Ok.
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e529b68..e5a41ad 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -619,6 +619,7 @@ struct drm_plane {
/**
* drm_bridge_funcs - drm_bridge control functions
+ * @post_encoder_init: called by the parent encoder
Maybe rename this to attach to make it more obvious when exactly it's
called?
post_encoder_attach?
post_encoder_ doesn't contain much information, or even ambiguous
information. What does post encoder mean? A bridge is always
attached to an encoder, so encoder can be dropped. Now post has
implications as to the time when it is called, but does it mean after
the encoder has been initialized, or after the encoder has been removed?
Simply attach means it's called by the parent encoder to initialize
the bridge once it's been attached to an encoder. So obviously it's
after the encoder has been initialized. attach has all he information
required. Any prefix is redundant in my opinion and removing prefixes
gives shorter names and reduces the number of keypresses.
Finally, what name it should have?
* @mode_fixup: Try to fixup (or reject
