Re: [PATCH 03/14] drm/bridge: make bridge registration independent of drm flow
On Fri, Jan 30, 2015 at 10:37:19AM -0500, Rob Clark wrote: On Tue, Jan 20, 2015 at 11:38 AM, Ajay Kumar ajaykumar...@samsung.com wrote: Currently, third party bridge drivers(ptn3460) are dependent on the corresponding encoder driver init, since bridge driver needs a drm_device pointer to finish drm initializations. The encoder driver passes the drm_device pointer to the bridge driver. Because of this dependency, third party drivers like ptn3460 doesn't adhere to the driver model. In this patch, we reframe the bridge registration framework so that bridge initialization is split into 2 steps, and bridge registration happens independent of drm flow: --Step 1: gather all the bridge settings independent of drm and add the bridge onto a global list of bridges. --Step 2: when the encoder driver is probed, call drm_bridge_attach for the corresponding bridge so that the bridge receives drm_device pointer and continues with connector and other drm initializations. The old set of bridge helpers are removed, and a set of new helpers are added to accomplish the 2 step initialization. The bridge devices register themselves onto global list of bridges when they get probed by calling drm_bridge_add. 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 also call drm_bridge_attach to pass on the drm_device to the bridge object. drm_bridge_attach inturn calls bridge-funcs-attach so that bridge can continue with drm related initializations. ok, so I probably should have had a closer look at this before it landed in drm-next, so if it is too late to revert (and deal w/ untangling subsequent patches that depend on this) some of these issues we'll just have to fix with follow-on patches. 1) needs headerdoc for new fxns in drm_bridge.c, and needs to be added in drm.tmpl drm_panel.c is missing kerneldoc as well, though I have a local patch to add it. If nobody else steps up I'll take this. 2) as far as I can tell, the new approach to cleaning up bridge objects is to just let them leak !?! With this series bridges are no longer part of the DRM device and it's the driver that provides them that needs to free them (in -remove()). It's not a completely perfect solution yet, but with the registry patch that I proposed a while back all the remaining issues should go away. Now if I could get anybody to look at that patch... Thierry pgpaoaZZoKn_V.pgp Description: PGP signature
Re: [PATCH 03/14] drm/bridge: make bridge registration independent of drm flow
Hi Rob, On Fri, Jan 30, 2015 at 9:07 PM, Rob Clark robdcl...@gmail.com wrote: On Tue, Jan 20, 2015 at 11:38 AM, Ajay Kumar ajaykumar...@samsung.com wrote: Currently, third party bridge drivers(ptn3460) are dependent on the corresponding encoder driver init, since bridge driver needs a drm_device pointer to finish drm initializations. The encoder driver passes the drm_device pointer to the bridge driver. Because of this dependency, third party drivers like ptn3460 doesn't adhere to the driver model. In this patch, we reframe the bridge registration framework so that bridge initialization is split into 2 steps, and bridge registration happens independent of drm flow: --Step 1: gather all the bridge settings independent of drm and add the bridge onto a global list of bridges. --Step 2: when the encoder driver is probed, call drm_bridge_attach for the corresponding bridge so that the bridge receives drm_device pointer and continues with connector and other drm initializations. The old set of bridge helpers are removed, and a set of new helpers are added to accomplish the 2 step initialization. The bridge devices register themselves onto global list of bridges when they get probed by calling drm_bridge_add. 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 also call drm_bridge_attach to pass on the drm_device to the bridge object. drm_bridge_attach inturn calls bridge-funcs-attach so that bridge can continue with drm related initializations. ok, so I probably should have had a closer look at this before it landed in drm-next, so if it is too late to revert (and deal w/ untangling subsequent patches that depend on this) some of these issues we'll just have to fix with follow-on patches. 1) needs headerdoc for new fxns in drm_bridge.c, and needs to be added in drm.tmpl Ohh, I totally forgot. I will do this. Just point me to some recent patch which updates docbook. 2) as far as I can tell, the new approach to cleaning up bridge objects is to just let them leak !?! I just checked. Only MSM hdmi_bridge is leaking, and this is because it doesn't use devm_kzalloc. All other bridges use devm_kzalloc, and hence that memory is automatically freed. For MSM HDMI, we need to find a place to call hdmi_bridge_destroy. I'll also need to update the new bridge in the msm edp code.. although that isn't such a big deal if I knew how this was *supposed* to work.. You just need to convert drm_bridge_init to drm_bridge_attach, and remove destroy callback. Refer this: http://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-nextid=b5217bf4692218d202d3d2cd772864fa1e10be4d Regards, Ajay Kumar since what is there now at least doesn't look right.. BR, -R Signed-off-by: Ajay Kumar ajaykumar...@samsung.com Acked-by: Inki Dae inki@samsung.com Tested-by: Rahul Sharma rahul.sha...@samsung.com Tested-by: Javier Martinez Canillas javier.marti...@collabora.co.uk Tested-by: Gustavo Padovan gustavo.pado...@collabora.co.uk Tested-by: Sjoerd Simons sjoerd.sim...@collabora.co.uk --- drivers/gpu/drm/Makefile |2 +- drivers/gpu/drm/bridge/ptn3460.c | 27 +- drivers/gpu/drm/drm_bridge.c | 91 drivers/gpu/drm/drm_crtc.c | 67 --- drivers/gpu/drm/msm/hdmi/hdmi.c|4 +- drivers/gpu/drm/msm/hdmi/hdmi.h|1 + drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |6 +-- drivers/gpu/drm/sti/sti_hda.c | 10 +--- drivers/gpu/drm/sti/sti_hdmi.c | 10 +--- include/drm/bridge/ptn3460.h |8 +++ include/drm/drm_crtc.h | 26 - 11 files changed, 133 insertions(+), 119 deletions(-) create mode 100644 drivers/gpu/drm/drm_bridge.c diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index e620807..c83ef2d 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -14,7 +14,7 @@ drm-y :=drm_auth.o drm_bufs.o drm_cache.o \ drm_info.o drm_debugfs.o drm_encoder_slave.o \ drm_trace_points.o drm_global.o drm_prime.o \ drm_rect.o drm_vma_manager.o drm_flip_work.o \ - drm_modeset_lock.o drm_atomic.o + drm_modeset_lock.o drm_atomic.o drm_bridge.o drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c index a2ddc8d..4a818c1 100644 --- a/drivers/gpu/drm/bridge/ptn3460.c +++ b/drivers/gpu/drm/bridge/ptn3460.c @@ -176,24 +176,11 @@ static void ptn3460_post_disable(struct drm_bridge *bridge) { } -static void ptn3460_bridge_destroy(struct drm_bridge *bridge) -{ - struct ptn3460_bridge
Re: [PATCH 03/14] drm/bridge: make bridge registration independent of drm flow
Hi, On Fri, Jan 30, 2015 at 9:29 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Fri, Jan 30, 2015 at 10:37:19AM -0500, Rob Clark wrote: On Tue, Jan 20, 2015 at 11:38 AM, Ajay Kumar ajaykumar...@samsung.com wrote: I'll also need to update the new bridge in the msm edp code.. although that isn't such a big deal if I knew how this was *supposed* to work.. since what is there now at least doesn't look right.. I wonder whether the new dw-hdmi bridge code get fixed up too... I think its already fixed. Check this: http://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-nextid=b5217bf4692218d202d3d2cd772864fa1e10be4d Regards, Ajay Kumar -- 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 03/14] drm/bridge: make bridge registration independent of drm flow
On Tue, Jan 20, 2015 at 11:38 AM, Ajay Kumar ajaykumar...@samsung.com wrote: Currently, third party bridge drivers(ptn3460) are dependent on the corresponding encoder driver init, since bridge driver needs a drm_device pointer to finish drm initializations. The encoder driver passes the drm_device pointer to the bridge driver. Because of this dependency, third party drivers like ptn3460 doesn't adhere to the driver model. In this patch, we reframe the bridge registration framework so that bridge initialization is split into 2 steps, and bridge registration happens independent of drm flow: --Step 1: gather all the bridge settings independent of drm and add the bridge onto a global list of bridges. --Step 2: when the encoder driver is probed, call drm_bridge_attach for the corresponding bridge so that the bridge receives drm_device pointer and continues with connector and other drm initializations. The old set of bridge helpers are removed, and a set of new helpers are added to accomplish the 2 step initialization. The bridge devices register themselves onto global list of bridges when they get probed by calling drm_bridge_add. 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 also call drm_bridge_attach to pass on the drm_device to the bridge object. drm_bridge_attach inturn calls bridge-funcs-attach so that bridge can continue with drm related initializations. ok, so I probably should have had a closer look at this before it landed in drm-next, so if it is too late to revert (and deal w/ untangling subsequent patches that depend on this) some of these issues we'll just have to fix with follow-on patches. 1) needs headerdoc for new fxns in drm_bridge.c, and needs to be added in drm.tmpl 2) as far as I can tell, the new approach to cleaning up bridge objects is to just let them leak !?! I'll also need to update the new bridge in the msm edp code.. although that isn't such a big deal if I knew how this was *supposed* to work.. since what is there now at least doesn't look right.. BR, -R Signed-off-by: Ajay Kumar ajaykumar...@samsung.com Acked-by: Inki Dae inki@samsung.com Tested-by: Rahul Sharma rahul.sha...@samsung.com Tested-by: Javier Martinez Canillas javier.marti...@collabora.co.uk Tested-by: Gustavo Padovan gustavo.pado...@collabora.co.uk Tested-by: Sjoerd Simons sjoerd.sim...@collabora.co.uk --- drivers/gpu/drm/Makefile |2 +- drivers/gpu/drm/bridge/ptn3460.c | 27 +- drivers/gpu/drm/drm_bridge.c | 91 drivers/gpu/drm/drm_crtc.c | 67 --- drivers/gpu/drm/msm/hdmi/hdmi.c|4 +- drivers/gpu/drm/msm/hdmi/hdmi.h|1 + drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |6 +-- drivers/gpu/drm/sti/sti_hda.c | 10 +--- drivers/gpu/drm/sti/sti_hdmi.c | 10 +--- include/drm/bridge/ptn3460.h |8 +++ include/drm/drm_crtc.h | 26 - 11 files changed, 133 insertions(+), 119 deletions(-) create mode 100644 drivers/gpu/drm/drm_bridge.c diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index e620807..c83ef2d 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -14,7 +14,7 @@ drm-y :=drm_auth.o drm_bufs.o drm_cache.o \ drm_info.o drm_debugfs.o drm_encoder_slave.o \ drm_trace_points.o drm_global.o drm_prime.o \ drm_rect.o drm_vma_manager.o drm_flip_work.o \ - drm_modeset_lock.o drm_atomic.o + drm_modeset_lock.o drm_atomic.o drm_bridge.o drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c index a2ddc8d..4a818c1 100644 --- a/drivers/gpu/drm/bridge/ptn3460.c +++ b/drivers/gpu/drm/bridge/ptn3460.c @@ -176,24 +176,11 @@ static void ptn3460_post_disable(struct drm_bridge *bridge) { } -static void ptn3460_bridge_destroy(struct drm_bridge *bridge) -{ - struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge); - - drm_bridge_cleanup(bridge); - if (gpio_is_valid(ptn_bridge-gpio_pd_n)) - gpio_free(ptn_bridge-gpio_pd_n); - if (gpio_is_valid(ptn_bridge-gpio_rst_n)) - gpio_free(ptn_bridge-gpio_rst_n); - /* Nothing else to free, we've got devm allocated memory */ -} - static struct drm_bridge_funcs ptn3460_bridge_funcs = { .pre_enable = ptn3460_pre_enable, .enable = ptn3460_enable, .disable = ptn3460_disable, .post_disable = ptn3460_post_disable, - .destroy = ptn3460_bridge_destroy, }; static int ptn3460_get_modes(struct
Re: [PATCH 03/14] drm/bridge: make bridge registration independent of drm flow
On Fri, Jan 30, 2015 at 10:37:19AM -0500, Rob Clark wrote: On Tue, Jan 20, 2015 at 11:38 AM, Ajay Kumar ajaykumar...@samsung.com wrote: I'll also need to update the new bridge in the msm edp code.. although that isn't such a big deal if I knew how this was *supposed* to work.. since what is there now at least doesn't look right.. I wonder whether the new dw-hdmi bridge code get fixed up too... -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- 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 03/14] drm/bridge: make bridge registration independent of drm flow
Hi, On 30 January 2015 at 15:37, Rob Clark robdcl...@gmail.com wrote: ok, so I probably should have had a closer look at this before it landed in drm-next, so if it is too late to revert (and deal w/ untangling subsequent patches that depend on this) some of these issues we'll just have to fix with follow-on patches. 1) needs headerdoc for new fxns in drm_bridge.c, and needs to be added in drm.tmpl 2) as far as I can tell, the new approach to cleaning up bridge objects is to just let them leak !?! I'll also need to update the new bridge in the msm edp code.. although that isn't such a big deal if I knew how this was *supposed* to work.. since what is there now at least doesn't look right.. Given how long these patches sat around doing being passively NACKed by discussions going around in circles, and how useful they are, I'd be much more in favour of fixing them up than reverting and going through the whole circus again ... Cheers, Daniel -- 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
[PATCH 03/14] drm/bridge: make bridge registration independent of drm flow
Currently, third party bridge drivers(ptn3460) are dependent on the corresponding encoder driver init, since bridge driver needs a drm_device pointer to finish drm initializations. The encoder driver passes the drm_device pointer to the bridge driver. Because of this dependency, third party drivers like ptn3460 doesn't adhere to the driver model. In this patch, we reframe the bridge registration framework so that bridge initialization is split into 2 steps, and bridge registration happens independent of drm flow: --Step 1: gather all the bridge settings independent of drm and add the bridge onto a global list of bridges. --Step 2: when the encoder driver is probed, call drm_bridge_attach for the corresponding bridge so that the bridge receives drm_device pointer and continues with connector and other drm initializations. The old set of bridge helpers are removed, and a set of new helpers are added to accomplish the 2 step initialization. The bridge devices register themselves onto global list of bridges when they get probed by calling drm_bridge_add. 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 also call drm_bridge_attach to pass on the drm_device to the bridge object. drm_bridge_attach inturn calls bridge-funcs-attach so that bridge can continue with drm related initializations. Signed-off-by: Ajay Kumar ajaykumar...@samsung.com Acked-by: Inki Dae inki@samsung.com Tested-by: Rahul Sharma rahul.sha...@samsung.com Tested-by: Javier Martinez Canillas javier.marti...@collabora.co.uk Tested-by: Gustavo Padovan gustavo.pado...@collabora.co.uk Tested-by: Sjoerd Simons sjoerd.sim...@collabora.co.uk --- drivers/gpu/drm/Makefile |2 +- drivers/gpu/drm/bridge/ptn3460.c | 27 +- drivers/gpu/drm/drm_bridge.c | 91 drivers/gpu/drm/drm_crtc.c | 67 --- drivers/gpu/drm/msm/hdmi/hdmi.c|4 +- drivers/gpu/drm/msm/hdmi/hdmi.h|1 + drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |6 +-- drivers/gpu/drm/sti/sti_hda.c | 10 +--- drivers/gpu/drm/sti/sti_hdmi.c | 10 +--- include/drm/bridge/ptn3460.h |8 +++ include/drm/drm_crtc.h | 26 - 11 files changed, 133 insertions(+), 119 deletions(-) create mode 100644 drivers/gpu/drm/drm_bridge.c diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index e620807..c83ef2d 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -14,7 +14,7 @@ drm-y :=drm_auth.o drm_bufs.o drm_cache.o \ drm_info.o drm_debugfs.o drm_encoder_slave.o \ drm_trace_points.o drm_global.o drm_prime.o \ drm_rect.o drm_vma_manager.o drm_flip_work.o \ - drm_modeset_lock.o drm_atomic.o + drm_modeset_lock.o drm_atomic.o drm_bridge.o drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c index a2ddc8d..4a818c1 100644 --- a/drivers/gpu/drm/bridge/ptn3460.c +++ b/drivers/gpu/drm/bridge/ptn3460.c @@ -176,24 +176,11 @@ static void ptn3460_post_disable(struct drm_bridge *bridge) { } -static void ptn3460_bridge_destroy(struct drm_bridge *bridge) -{ - struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge); - - drm_bridge_cleanup(bridge); - if (gpio_is_valid(ptn_bridge-gpio_pd_n)) - gpio_free(ptn_bridge-gpio_pd_n); - if (gpio_is_valid(ptn_bridge-gpio_rst_n)) - gpio_free(ptn_bridge-gpio_rst_n); - /* Nothing else to free, we've got devm allocated memory */ -} - static struct drm_bridge_funcs ptn3460_bridge_funcs = { .pre_enable = ptn3460_pre_enable, .enable = ptn3460_enable, .disable = ptn3460_disable, .post_disable = ptn3460_post_disable, - .destroy = ptn3460_bridge_destroy, }; static int ptn3460_get_modes(struct drm_connector *connector) @@ -314,7 +301,7 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, } ptn_bridge-bridge.funcs = ptn3460_bridge_funcs; - ret = drm_bridge_init(dev, ptn_bridge-bridge); + ret = drm_bridge_attach(dev, ptn_bridge-bridge); if (ret) { DRM_ERROR(Failed to initialize bridge with drm\n); goto err; @@ -343,3 +330,15 @@ err: return ret; } EXPORT_SYMBOL(ptn3460_init); + +void ptn3460_destroy(struct drm_bridge *bridge) +{ + struct ptn3460_bridge *ptn_bridge = bridge-driver_private; + + if (gpio_is_valid(ptn_bridge-gpio_pd_n)) + gpio_free(ptn_bridge-gpio_pd_n); + if (gpio_is_valid(ptn_bridge-gpio_rst_n)) +