| Message ID | 20191004143418.53039-4-mihail.atanassov@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/komeda: Support for drm_bridge endpoints | expand |
On Fri, Oct 04, 2019 at 02:34:42PM +0000, Mihail Atanassov wrote: > To support transmitters other than the tda998x, we need to allow > non-component framework bridges to be attached to a dummy drm_encoder in > our driver. > > For the existing supported encoder (tda998x), keep the behaviour as-is, > since there's no way to unbind if a drm_bridge module goes away under > our feet. > > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> > --- > .../gpu/drm/arm/display/komeda/komeda_dev.h | 5 + > .../gpu/drm/arm/display/komeda/komeda_drv.c | 58 ++++++-- > .../gpu/drm/arm/display/komeda/komeda_kms.c | 133 +++++++++++++++++- > .../gpu/drm/arm/display/komeda/komeda_kms.h | 5 + > 4 files changed, 187 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h > index e392b8ffc372..64d2902e2e0c 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h > @@ -176,6 +176,11 @@ struct komeda_dev { > > /** @irq: irq number */ > int irq; > + /** @has_components: > + * > + * if true, use the component framework to bind/unbind driver > + */ > + bool has_components; > > /** @lock: used to protect dpmode */ > struct mutex lock; > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c > index 9ed25ffe0e22..34cfc6a4c3e4 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c > @@ -10,6 +10,8 @@ > #include <linux/component.h> > #include <linux/pm_runtime.h> > #include <drm/drm_of.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_encoder.h> > #include "komeda_dev.h" > #include "komeda_kms.h" > > @@ -69,18 +71,35 @@ 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 *master, > + struct komeda_drv *mdrv, > + struct component_match **match, > + 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) { > + if (!remote) > + return 0; > + > + if (komeda_remote_device_is_component(np, port, endpoint)) { > + mdrv->mdev.has_components = true; > drm_of_component_match_add(master, match, compare_of, remote); > - of_node_put(remote); > + goto exit; > + } > + > + bridge = of_drm_find_bridge(remote); > + if (!bridge) { > + ret = -EPROBE_DEFER; > + goto exit; > } > + > +exit: > + of_node_put(remote); > + return ret; > } > > static int komeda_platform_probe(struct platform_device *pdev) > @@ -89,6 +108,7 @@ static int komeda_platform_probe(struct platform_device *pdev) > struct component_match *match = NULL; > struct device_node *child; > struct komeda_drv *mdrv; > + int ret; > > if (!dev->of_node) > return -ENODEV; > @@ -101,14 +121,27 @@ 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(dev, mdrv, &match, child, > + KOMEDA_OF_PORT_OUTPUT, 0); > + if (ret) > + goto free_mdrv; > + ret = komeda_add_slave(dev, mdrv, &match, child, > + KOMEDA_OF_PORT_OUTPUT, 1); > + if (ret) > + goto free_mdrv; > } > > dev_set_drvdata(dev, mdrv); > > - return component_master_add_with_match(dev, &komeda_master_ops, match); > + return mdrv->mdev.has_components > + ? component_master_add_with_match( > + dev, &komeda_master_ops, match) > + : komeda_bind(dev); > + > +free_mdrv: > + devm_kfree(dev, mdrv); > + return ret; > } > > static int komeda_platform_remove(struct platform_device *pdev) > @@ -116,7 +149,10 @@ static int komeda_platform_remove(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct komeda_drv *mdrv = dev_get_drvdata(dev); > > - component_master_del(dev, &komeda_master_ops); > + if (mdrv->mdev.has_components) > + component_master_del(dev, &komeda_master_ops); > + else > + komeda_unbind(dev); > > dev_set_drvdata(dev, NULL); > devm_kfree(dev, mdrv); > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c > index 03dcf1d7688f..6eb52d1b20a0 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c > @@ -6,6 +6,7 @@ > */ > #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 +15,8 @@ > #include <drm/drm_gem_cma_helper.h> > #include <drm/drm_gem_framebuffer_helper.h> > #include <drm/drm_irq.h> > +#include <drm/drm_modes.h> > +#include <drm/drm_of.h> > #include <drm/drm_probe_helper.h> > #include <drm/drm_vblank.h> > > @@ -267,6 +270,130 @@ 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, > +}; > + > +bool komeda_remote_device_is_component(struct device_node *local, > + u32 port, u32 endpoint) > +{ > + struct device_node *remote; > + char const * const component_devices[] = { > + "nxp,tda998x", I really don't think put this dummy_encoder into komeda is a good idea. And I suggest to seperate this dummy_encoder to a individual module which will build the drm_ridge to a standard drm encoder and component based module, which will be enable by DT, totally transparent for komeda. BTW: I really don't like such logic: distingush the SYSTEM configuration by check the connected device name, it's hard to maintain and code sharing, and that's why NOW we have the device-tree. Thanks James > + }; > + int i; > + bool ret = false; > + > + remote = of_graph_get_remote_node(local, port, endpoint); > + if (!remote) > + return false; > + > + /* Coprocessor endpoints are always component based. */ > + if (port != KOMEDA_OF_PORT_OUTPUT) { > + ret = true; > + goto exit; > + } > + > + for (i = 0; i < ARRAY_SIZE(component_devices); ++i) { > + if (of_device_is_compatible(remote, component_devices[i])) { > + ret = true; > + goto exit; > + } > + } > + > +exit: > + of_node_put(remote); > + return ret; > +} > + > +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; > + > + if (komeda_remote_device_is_component(np, KOMEDA_OF_PORT_OUTPUT, 0)) > + return 0; > + > + remote = of_graph_get_remote_node(np, KOMEDA_OF_PORT_OUTPUT, 0); > + if (!remote) > + return 0; > + > + bridge = of_drm_find_bridge(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; > +} > + > +static int komeda_kms_attach_external(struct komeda_kms_dev *kms, > + struct komeda_dev *mdev) > +{ > + int err; > + > + if (mdev->has_components) { > + err = component_bind_all(mdev->dev, kms); > + if (err) > + return err; > + } > + > + err = komeda_encoder_attach_bridges(kms, mdev); > + if (err) > + return err; > + > + return 0; > +} > + > +static void komeda_kms_detach_external(struct komeda_dev *mdev, > + struct drm_device *drm) > +{ > + if (mdev->has_components) > + component_unbind_all(mdev->dev, drm); > +} > + > int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev) > { > struct drm_device *drm; > @@ -301,7 +428,7 @@ int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev) > if (err) > goto cleanup_mode_config; > > - err = component_bind_all(mdev->dev, kms); > + err = komeda_kms_attach_external(kms, mdev); > if (err) > goto cleanup_mode_config; > > @@ -332,7 +459,7 @@ int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev) > drm->irq_enabled = false; > mdev->funcs->disable_irq(mdev); > free_component_binding: > - component_unbind_all(mdev->dev, drm); > + komeda_kms_detach_external(mdev, drm); > cleanup_mode_config: > drm_mode_config_cleanup(drm); > komeda_kms_cleanup_private_objs(kms); > @@ -351,7 +478,7 @@ void komeda_kms_fini(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); > + komeda_kms_detach_external(mdev, 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 e81ceb298034..c2856e19d092 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) > @@ -184,4 +186,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc, > int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev); > void komeda_kms_fini(struct komeda_kms_dev *kms); > > +bool komeda_remote_device_is_component(struct device_node *local, > + u32 port, u32 endpoint); > + > #endif /*_KOMEDA_KMS_H_*/
Hi James, On Wednesday, 9 October 2019 06:54:15 BST james qian wang (Arm Technology China) wrote: > On Fri, Oct 04, 2019 at 02:34:42PM +0000, Mihail Atanassov wrote: > > To support transmitters other than the tda998x, we need to allow > > non-component framework bridges to be attached to a dummy drm_encoder in > > our driver. > > > > For the existing supported encoder (tda998x), keep the behaviour as-is, > > since there's no way to unbind if a drm_bridge module goes away under > > our feet. > > > > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> > > --- > > .../gpu/drm/arm/display/komeda/komeda_dev.h | 5 + > > .../gpu/drm/arm/display/komeda/komeda_drv.c | 58 ++++++-- > > .../gpu/drm/arm/display/komeda/komeda_kms.c | 133 +++++++++++++++++- > > .../gpu/drm/arm/display/komeda/komeda_kms.h | 5 + > > 4 files changed, 187 insertions(+), 14 deletions(-) > > > > [snip] > > > > +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, > > +}; > > + > > +bool komeda_remote_device_is_component(struct device_node *local, > > + u32 port, u32 endpoint) > > +{ > > + struct device_node *remote; > > + char const * const component_devices[] = { > > + "nxp,tda998x", > > I really don't think put this dummy_encoder into komeda is a good > idea. > > And I suggest to seperate this dummy_encoder to a individual module > which will build the drm_ridge to a standard drm encoder and component > based module, which will be enable by DT, totally transparent for komeda. > > BTW: > I really don't like such logic: distingush the SYSTEM configuration > by check the connected device name, it's hard to maintain and code > sharing, and that's why NOW we have the device-tree. +Cc Brian I didn't think DT is the right place for pseudo-devices. The tda998x looks to be in a small group of drivers that contain encoder + bridge + connector; my impression of the current state of affairs is that the drm_encoder tends to live where the CRTC provider is rather than representing a HW entity (hence why drm_bridge based drivers exist in drivers/gpu/drm/bridge). See the overview DOC comment in drm_encoder.c ("drivers are free to use [drm_encoder] however they wish"). I may be completely wrong, so would appreciate some context and comment from others on the Cc list. In any case, converting a do-nothing dummy encoder into its own component-module will add a lot more bloat compared to the current ~10 SLoC implementation of the drm_encoder. probe/remove/bind/unbind, a few extra structs here and there, yet another object file, DT bindings, docs for the same, and maintaining all of that? It's a hard sell for me. I'd prefer if we went ahead with the code as-is and fix it up later if it really proves unwieldy and too hard to maintain. Could this patch be improved? Sure! Can we improve it in follow-up work though, as I think this is valuable enough on its own without any major drawbacks? As per my cover letter, in an ideal world we'd have the encoder locally and do drm_bridge_attach() even for tda998x, but the lifetime issues around bridges inside modules mean that doing that now is a bit of a step back for this specific case. > > Thanks > James > > > [snip] >
Hi, On Wed, Oct 16, 2019 at 03:51:39PM +0000, Mihail Atanassov wrote: > Hi James, > > On Wednesday, 9 October 2019 06:54:15 BST james qian wang (Arm Technology China) wrote: > > On Fri, Oct 04, 2019 at 02:34:42PM +0000, Mihail Atanassov wrote: > > > To support transmitters other than the tda998x, we need to allow > > > non-component framework bridges to be attached to a dummy drm_encoder in > > > our driver. > > > > > > For the existing supported encoder (tda998x), keep the behaviour as-is, > > > since there's no way to unbind if a drm_bridge module goes away under > > > our feet. > > > > > > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> > > > --- > > > .../gpu/drm/arm/display/komeda/komeda_dev.h | 5 + > > > .../gpu/drm/arm/display/komeda/komeda_drv.c | 58 ++++++-- > > > .../gpu/drm/arm/display/komeda/komeda_kms.c | 133 +++++++++++++++++- > > > .../gpu/drm/arm/display/komeda/komeda_kms.h | 5 + > > > 4 files changed, 187 insertions(+), 14 deletions(-) > > > > > > [snip] > > > > > > +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, > > > +}; > > > + > > > +bool komeda_remote_device_is_component(struct device_node *local, > > > + u32 port, u32 endpoint) > > > +{ > > > + struct device_node *remote; > > > + char const * const component_devices[] = { > > > + "nxp,tda998x", > > > > I really don't think put this dummy_encoder into komeda is a good > > idea. > > > > And I suggest to seperate this dummy_encoder to a individual module > > which will build the drm_ridge to a standard drm encoder and component > > based module, which will be enable by DT, totally transparent for komeda. > > > > BTW: > > I really don't like such logic: distingush the SYSTEM configuration > > by check the connected device name, it's hard to maintain and code > > sharing, and that's why NOW we have the device-tree. It's not ideal to have such special cases, for sure. However, I don't see this approach causing us any issues. tda998x really is "special", and as far as I can see the code here scales to other devices pretty easily. > > +Cc Brian > > I didn't think DT is the right place for pseudo-devices. I agree. DT should represent the HW, not the structure of the linux kernel subsystem. > The tda998x > looks to be in a small group of drivers that contain encoder + > bridge + connector; my impression of the current state of affairs is > that the drm_encoder tends to live where the CRTC provider is rather > than representing a HW entity (hence why drm_bridge based drivers > exist in drivers/gpu/drm/bridge). See the overview DOC comment in > drm_encoder.c ("drivers are free to use [drm_encoder] however they > wish"). I may be completely wrong, so would appreciate some > context and comment from others on the Cc list. > > In any case, converting a do-nothing dummy encoder into its own > component-module will add a lot more bloat compared to the current > ~10 SLoC implementation of the drm_encoder. probe/remove/bind/unbind, > a few extra structs here and there, yet another object file, DT > bindings, docs for the same, and maintaining all of that? It's a hard > sell for me. I'd prefer if we went ahead with the code as-is and fix it > up later if it really proves unwieldy and too hard to maintain. Could > this patch be improved? Sure! Can we improve it in follow-up work > though, as I think this is valuable enough on its own without any major > drawbacks? > Even if we implemented a separate component encoder, as far as I can tell there's no way to use it without either: a) sticking it in DT b) invoking it from komeda based on a "of_device_is_compatible" list IMO a) isn't acceptable, and b) doesn't have any advantages over this approach. > As per my cover letter, in an ideal world we'd have the encoder locally > and do drm_bridge_attach() even for tda998x, but the lifetime issues > around bridges inside modules mean that doing that now is a bit of a > step back for this specific case. > Yeah, my feeling is that being able to keep tda998x as a component (for the superior bind/unbind behaviour) is worth the slight ugliness, at least until bridges get the same functionality. If James is strongly against merging this, maybe we just swap wholesale to bridge? But for me, the pragmatic approach would be this stop-gap. Cheers, -Brian > > > > Thanks > > James > > > > > [snip] > > > > -- > Mihail > > >
On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote: > Hi, > > On Wed, Oct 16, 2019 at 03:51:39PM +0000, Mihail Atanassov wrote: > > Hi James, > > > > On Wednesday, 9 October 2019 06:54:15 BST james qian wang (Arm Technology China) wrote: > > > On Fri, Oct 04, 2019 at 02:34:42PM +0000, Mihail Atanassov wrote: > > > > To support transmitters other than the tda998x, we need to allow > > > > non-component framework bridges to be attached to a dummy drm_encoder in > > > > our driver. > > > > > > > > For the existing supported encoder (tda998x), keep the behaviour as-is, > > > > since there's no way to unbind if a drm_bridge module goes away under > > > > our feet. > > > > > > > > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> > > > > --- > > > > .../gpu/drm/arm/display/komeda/komeda_dev.h | 5 + > > > > .../gpu/drm/arm/display/komeda/komeda_drv.c | 58 ++++++-- > > > > .../gpu/drm/arm/display/komeda/komeda_kms.c | 133 +++++++++++++++++- > > > > .../gpu/drm/arm/display/komeda/komeda_kms.h | 5 + > > > > 4 files changed, 187 insertions(+), 14 deletions(-) > > > > > > > > [snip] > > > > > > > > +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, > > > > +}; > > > > + > > > > +bool komeda_remote_device_is_component(struct device_node *local, > > > > + u32 port, u32 endpoint) > > > > +{ > > > > + struct device_node *remote; > > > > + char const * const component_devices[] = { > > > > + "nxp,tda998x", > > > > > > I really don't think put this dummy_encoder into komeda is a good > > > idea. > > > > > > And I suggest to seperate this dummy_encoder to a individual module > > > which will build the drm_ridge to a standard drm encoder and component > > > based module, which will be enable by DT, totally transparent for komeda. > > > > > > BTW: > > > I really don't like such logic: distingush the SYSTEM configuration > > > by check the connected device name, it's hard to maintain and code > > > sharing, and that's why NOW we have the device-tree. > > It's not ideal to have such special cases, for sure. However, I don't > see this approach causing us any issues. tda998x really is "special", > and as far as I can see the code here scales to other devices pretty > easily. > > > > > +Cc Brian > > > > I didn't think DT is the right place for pseudo-devices. > > I agree. DT should represent the HW, not the structure of the > linux kernel subsystem. > > > The tda998x > > looks to be in a small group of drivers that contain encoder + > > bridge + connector; my impression of the current state of affairs is > > that the drm_encoder tends to live where the CRTC provider is rather > > than representing a HW entity (hence why drm_bridge based drivers > > exist in drivers/gpu/drm/bridge). See the overview DOC comment in > > drm_encoder.c ("drivers are free to use [drm_encoder] however they > > wish"). I may be completely wrong, so would appreciate some > > context and comment from others on the Cc list. > > > > In any case, converting a do-nothing dummy encoder into its own > > component-module will add a lot more bloat compared to the current > > ~10 SLoC implementation of the drm_encoder. probe/remove/bind/unbind, > > a few extra structs here and there, yet another object file, DT > > bindings, docs for the same, and maintaining all of that? It's a hard > > sell for me. I'd prefer if we went ahead with the code as-is and fix it > > up later if it really proves unwieldy and too hard to maintain. Could > > this patch be improved? Sure! Can we improve it in follow-up work > > though, as I think this is valuable enough on its own without any major > > drawbacks? > > > > Even if we implemented a separate component encoder, as far as I can > tell there's no way to use it without either: > > a) sticking it in DT > b) invoking it from komeda based on a "of_device_is_compatible" list > > IMO a) isn't acceptable, and b) doesn't have any advantages over this > approach. > > > As per my cover letter, in an ideal world we'd have the encoder locally > > and do drm_bridge_attach() even for tda998x, but the lifetime issues > > around bridges inside modules mean that doing that now is a bit of a > > step back for this specific case. > > > > Yeah, my feeling is that being able to keep tda998x as a component > (for the superior bind/unbind behaviour) is worth the slight ugliness, > at least until bridges get the same functionality. > > If James is strongly against merging this, maybe we just swap > wholesale to bridge? But for me, the pragmatic approach would be this > stop-gap. > This is a good idea, and I vote +ULONG_MAX :) and I also checked tda998x driver, it supports bridge. so swap the wholesale to brige is perfect. :) Thanks James. > Cheers, > -Brian > > > > > > > Thanks > > > James > > > > > > > [snip] > > > > > > > -- > > Mihail > > > > > >
On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote: > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote: > > > > If James is strongly against merging this, maybe we just swap > > wholesale to bridge? But for me, the pragmatic approach would be this > > stop-gap. > > > > This is a good idea, and I vote +ULONG_MAX :) > > and I also checked tda998x driver, it supports bridge. so swap the > wholesale to brige is perfect. :) > Well, as Mihail wrote, it's definitely not perfect. Today, if you rmmod tda998x with the DPU driver still loaded, everything will be unbound gracefully. If we swap to bridge, then rmmod'ing tda998x (or any other bridge driver the DPU is using) with the DPU driver still loaded will result in a crash. So, there really are proper benefits to sticking with the component code for tda998x, which is why I'd like to understand why you're so against this patch? Thanks, -Brian
On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote: > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote: > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote: > > > > > > If James is strongly against merging this, maybe we just swap > > > wholesale to bridge? But for me, the pragmatic approach would be this > > > stop-gap. > > > > > > > This is a good idea, and I vote +ULONG_MAX :) > > > > and I also checked tda998x driver, it supports bridge. so swap the > > wholesale to brige is perfect. :) > > > > Well, as Mihail wrote, it's definitely not perfect. > > Today, if you rmmod tda998x with the DPU driver still loaded, > everything will be unbound gracefully. > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge > driver the DPU is using) with the DPU driver still loaded will result > in a crash. I haven't read the bridge code, but seems this is a bug of drm_bridge, since if the bridge is still in using by others, the rmmod should fail And personally opinion, if the bridge doesn't handle the dependence. for us: - add such support to bridge or - just do the insmod/rmmod in correct order. > So, there really are proper benefits to sticking with the component > code for tda998x, which is why I'd like to understand why you're so > against this patch? > This change handles two different connectors in komeda internally, compare with one interface, it increases the complexity, more risk of bug and more cost of maintainance. So my suggestion is keeping on one single interface in komeda, no matter it is bridge or component, but I'd like it only one, but not them both in komeda. Thanks James > Thanks, > -Brian
On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote: > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote: > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote: > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote: > > > > > > > > If James is strongly against merging this, maybe we just swap > > > > wholesale to bridge? But for me, the pragmatic approach would be this > > > > stop-gap. > > > > > > > > > > This is a good idea, and I vote +ULONG_MAX :) > > > > > > and I also checked tda998x driver, it supports bridge. so swap the > > > wholesale to brige is perfect. :) > > > > > > > Well, as Mihail wrote, it's definitely not perfect. > > > > Today, if you rmmod tda998x with the DPU driver still loaded, > > everything will be unbound gracefully. > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge > > driver the DPU is using) with the DPU driver still loaded will result > > in a crash. > > I haven't read the bridge code, but seems this is a bug of drm_bridge, > since if the bridge is still in using by others, the rmmod should fail > Correct, but there's no fix for that today. You can also take a look at the thread linked from Mihail's cover letter. > And personally opinion, if the bridge doesn't handle the dependence. > for us: > > - add such support to bridge That would certainly be helpful. I don't know if there's consensus on how to do that. > or > - just do the insmod/rmmod in correct order. > > > So, there really are proper benefits to sticking with the component > > code for tda998x, which is why I'd like to understand why you're so > > against this patch? > > > > This change handles two different connectors in komeda internally, compare > with one interface, it increases the complexity, more risk of bug and more > cost of maintainance. > Well, it's only about how to bind the drivers - two different methods of binding, not two different connectors. I would argue that carrying our out-of-tree patches to support both platforms is a larger maintenance burden. Honestly this looks like a win-win to me. We get the superior approach when its supported, and still get to support bridges which are more common. As/when improvements are made to the bridge code we can remove the component bits and not lose anything. > So my suggestion is keeping on one single interface in komeda, no > matter it is bridge or component, but I'd like it only one, but not > them both in komeda. If we can put the effort into fixing bridges then I guess that's the best approach for everyone :-) Might not be easy though! -Brian > > Thanks > James > > > Thanks, > > -Brian
On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote: > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote: > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote: > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote: > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote: > > > > > > > > > > If James is strongly against merging this, maybe we just swap > > > > > wholesale to bridge? But for me, the pragmatic approach would be this > > > > > stop-gap. > > > > > > > > > > > > > This is a good idea, and I vote +ULONG_MAX :) > > > > > > > > and I also checked tda998x driver, it supports bridge. so swap the > > > > wholesale to brige is perfect. :) > > > > > > > > > > Well, as Mihail wrote, it's definitely not perfect. > > > > > > Today, if you rmmod tda998x with the DPU driver still loaded, > > > everything will be unbound gracefully. > > > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge > > > driver the DPU is using) with the DPU driver still loaded will result > > > in a crash. > > > > I haven't read the bridge code, but seems this is a bug of drm_bridge, > > since if the bridge is still in using by others, the rmmod should fail > > > > Correct, but there's no fix for that today. You can also take a look > at the thread linked from Mihail's cover letter. > > > And personally opinion, if the bridge doesn't handle the dependence. > > for us: > > > > - add such support to bridge > > That would certainly be helpful. I don't know if there's consensus on > how to do that. > > > or > > - just do the insmod/rmmod in correct order. > > > > > So, there really are proper benefits to sticking with the component > > > code for tda998x, which is why I'd like to understand why you're so > > > against this patch? > > > > > > > This change handles two different connectors in komeda internally, compare > > with one interface, it increases the complexity, more risk of bug and more > > cost of maintainance. > > > > Well, it's only about how to bind the drivers - two different methods > of binding, not two different connectors. I would argue that carrying > our out-of-tree patches to support both platforms is a larger > maintenance burden. > > Honestly this looks like a win-win to me. We get the superior approach > when its supported, and still get to support bridges which are more > common. > > As/when improvements are made to the bridge code we can remove the > component bits and not lose anything. There was an idea a while back about using the device links code to solve the bridge issue - but at the time the device links code wasn't up to the job. I think that's been resolved now, but I haven't been able to confirm it. I did propose some patches for bridge at the time but they probably need updating.
On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote: > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote: > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote: > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote: > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote: > > > > > > > > > > If James is strongly against merging this, maybe we just swap > > > > > wholesale to bridge? But for me, the pragmatic approach would be this > > > > > stop-gap. > > > > > > > > > > > > > This is a good idea, and I vote +ULONG_MAX :) > > > > > > > > and I also checked tda998x driver, it supports bridge. so swap the > > > > wholesale to brige is perfect. :) > > > > > > > > > > Well, as Mihail wrote, it's definitely not perfect. > > > > > > Today, if you rmmod tda998x with the DPU driver still loaded, > > > everything will be unbound gracefully. > > > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge > > > driver the DPU is using) with the DPU driver still loaded will result > > > in a crash. > > > > I haven't read the bridge code, but seems this is a bug of drm_bridge, > > since if the bridge is still in using by others, the rmmod should fail > > > > Correct, but there's no fix for that today. You can also take a look > at the thread linked from Mihail's cover letter. > > > And personally opinion, if the bridge doesn't handle the dependence. > > for us: > > > > - add such support to bridge > > That would certainly be helpful. I don't know if there's consensus on > how to do that. > > > or > > - just do the insmod/rmmod in correct order. > > > > > So, there really are proper benefits to sticking with the component > > > code for tda998x, which is why I'd like to understand why you're so > > > against this patch? > > > > > > > This change handles two different connectors in komeda internally, compare > > with one interface, it increases the complexity, more risk of bug and more > > cost of maintainance. > > > > Well, it's only about how to bind the drivers - two different methods > of binding, not two different connectors. I would argue that carrying > our out-of-tree patches to support both platforms is a larger > maintenance burden. > > Honestly this looks like a win-win to me. We get the superior approach > when its supported, and still get to support bridges which are more > common. > My consideration is: if we support both link methods, we may suffering - 1. bridge reference cnt problem - 2. maintance two link methods. the 1) seems unavoidable, so swap all to bridage at least can avoid the pain of 2). that's why I thought your idea "swap all to bridage" is good. Thanks James. > As/when improvements are made to the bridge code we can remove the > component bits and not lose anything. > > > So my suggestion is keeping on one single interface in komeda, no > > matter it is bridge or component, but I'd like it only one, but not > > them both in komeda. > > If we can put the effort into fixing bridges then I guess that's the > best approach for everyone :-) Might not be easy though! > > -Brian > > > > > Thanks > > James > > > > > Thanks, > > > -Brian
On Thu, Oct 17, 2019 at 12:41:37PM +0100, Russell King - ARM Linux admin wrote: > On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote: > > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote: > > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote: > > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote: > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote: > > > > > > > > > > > > If James is strongly against merging this, maybe we just swap > > > > > > wholesale to bridge? But for me, the pragmatic approach would be this > > > > > > stop-gap. > > > > > > > > > > > > > > > > This is a good idea, and I vote +ULONG_MAX :) > > > > > > > > > > and I also checked tda998x driver, it supports bridge. so swap the > > > > > wholesale to brige is perfect. :) > > > > > > > > > > > > > Well, as Mihail wrote, it's definitely not perfect. > > > > > > > > Today, if you rmmod tda998x with the DPU driver still loaded, > > > > everything will be unbound gracefully. > > > > > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge > > > > driver the DPU is using) with the DPU driver still loaded will result > > > > in a crash. > > > > > > I haven't read the bridge code, but seems this is a bug of drm_bridge, > > > since if the bridge is still in using by others, the rmmod should fail > > > > > > > Correct, but there's no fix for that today. You can also take a look > > at the thread linked from Mihail's cover letter. > > > > > And personally opinion, if the bridge doesn't handle the dependence. > > > for us: > > > > > > - add such support to bridge > > > > That would certainly be helpful. I don't know if there's consensus on > > how to do that. > > > > > or > > > - just do the insmod/rmmod in correct order. > > > > > > > So, there really are proper benefits to sticking with the component > > > > code for tda998x, which is why I'd like to understand why you're so > > > > against this patch? > > > > > > > > > > This change handles two different connectors in komeda internally, compare > > > with one interface, it increases the complexity, more risk of bug and more > > > cost of maintainance. > > > > > > > Well, it's only about how to bind the drivers - two different methods > > of binding, not two different connectors. I would argue that carrying > > our out-of-tree patches to support both platforms is a larger > > maintenance burden. > > > > Honestly this looks like a win-win to me. We get the superior approach > > when its supported, and still get to support bridges which are more > > common. > > > > As/when improvements are made to the bridge code we can remove the > > component bits and not lose anything. > > There was an idea a while back about using the device links code to > solve the bridge issue - but at the time the device links code wasn't > up to the job. I think that's been resolved now, but I haven't been > able to confirm it. I did propose some patches for bridge at the > time but they probably need updating. Hi Russell: Thank you, that's a good news. Hi Brian: Can this convince you to fully swap to bridge ? Actually even there is no fix, we won't real encounter such rmmod problem, since we always build the bridge/tda998 (by Y) into the image. Thanks James > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up
On Fri, Oct 18, 2019 at 06:57:05AM +0000, james qian wang (Arm Technology China) wrote: > > Hi Brian: > > Can this convince you to fully swap to bridge ? Not until those patches materialise and land, no :-) > > Actually even there is no fix, we won't real encounter such rmmod problem, > since we always build the bridge/tda998 (by Y) into the image. > If you say so. I think the folks here like having drm as a module to make it easy to patch things without a reboot. -Brian > Thanks > James > > -- > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > > According to speedtest.net: 11.9Mbps down 500kbps up
On Friday, 18 October 2019 07:38:59 BST james qian wang (Arm Technology China) wrote: > On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote: > > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote: > > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote: > > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote: > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote: > > > > > > > > > > > > If James is strongly against merging this, maybe we just swap > > > > > > wholesale to bridge? But for me, the pragmatic approach would be this > > > > > > stop-gap. > > > > > > > > > > > > > > > > This is a good idea, and I vote +ULONG_MAX :) > > > > > > > > > > and I also checked tda998x driver, it supports bridge. so swap the > > > > > wholesale to brige is perfect. :) > > > > > > > > > > > > > Well, as Mihail wrote, it's definitely not perfect. > > > > > > > > Today, if you rmmod tda998x with the DPU driver still loaded, > > > > everything will be unbound gracefully. > > > > > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge > > > > driver the DPU is using) with the DPU driver still loaded will result > > > > in a crash. > > > > > > I haven't read the bridge code, but seems this is a bug of drm_bridge, > > > since if the bridge is still in using by others, the rmmod should fail > > > > > > > Correct, but there's no fix for that today. You can also take a look > > at the thread linked from Mihail's cover letter. > > > > > And personally opinion, if the bridge doesn't handle the dependence. > > > for us: > > > > > > - add such support to bridge > > > > That would certainly be helpful. I don't know if there's consensus on > > how to do that. > > > > > or > > > - just do the insmod/rmmod in correct order. > > > > > > > So, there really are proper benefits to sticking with the component > > > > code for tda998x, which is why I'd like to understand why you're so > > > > against this patch? > > > > > > > > > > This change handles two different connectors in komeda internally, compare > > > with one interface, it increases the complexity, more risk of bug and more > > > cost of maintainance. > > > > > > > Well, it's only about how to bind the drivers - two different methods > > of binding, not two different connectors. I would argue that carrying > > our out-of-tree patches to support both platforms is a larger > > maintenance burden. > > > > Honestly this looks like a win-win to me. We get the superior approach > > when its supported, and still get to support bridges which are more > > common. > > > > My consideration is: if we support both link methods, we may suffering > > - 1. bridge reference cnt problem > - 2. maintance two link methods. > > the 1) seems unavoidable, so swap all to bridage at least can avoid > the pain of 2). that's why I thought your idea "swap all to bridage" > is good. > > Thanks > James. > Just to make sure my understanding is clear: If I respin the patch to only use the drm_bridge i/f, you'd be happier with it and we can get it merged? > > As/when improvements are made to the bridge code we can remove the > > component bits and not lose anything. > > > > > So my suggestion is keeping on one single interface in komeda, no > > > matter it is bridge or component, but I'd like it only one, but not > > > them both in komeda. > > > > If we can put the effort into fixing bridges then I guess that's the > > best approach for everyone :-) Might not be easy though! > > > > -Brian > > > > > > > > Thanks > > > James > > > > > > > Thanks, > > > > -Brian >
On Thu, Oct 17, 2019 at 12:41:37PM +0100, Russell King - ARM Linux admin wrote: > On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote: > > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote: > > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote: > > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote: > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote: > > > > > > > > > > > > If James is strongly against merging this, maybe we just swap > > > > > > wholesale to bridge? But for me, the pragmatic approach would be this > > > > > > stop-gap. > > > > > > > > > > > > > > > > This is a good idea, and I vote +ULONG_MAX :) > > > > > > > > > > and I also checked tda998x driver, it supports bridge. so swap the > > > > > wholesale to brige is perfect. :) > > > > > > > > > > > > > Well, as Mihail wrote, it's definitely not perfect. > > > > > > > > Today, if you rmmod tda998x with the DPU driver still loaded, > > > > everything will be unbound gracefully. > > > > > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge > > > > driver the DPU is using) with the DPU driver still loaded will result > > > > in a crash. > > > > > > I haven't read the bridge code, but seems this is a bug of drm_bridge, > > > since if the bridge is still in using by others, the rmmod should fail > > > > > > > Correct, but there's no fix for that today. You can also take a look > > at the thread linked from Mihail's cover letter. > > > > > And personally opinion, if the bridge doesn't handle the dependence. > > > for us: > > > > > > - add such support to bridge > > > > That would certainly be helpful. I don't know if there's consensus on > > how to do that. > > > > > or > > > - just do the insmod/rmmod in correct order. > > > > > > > So, there really are proper benefits to sticking with the component > > > > code for tda998x, which is why I'd like to understand why you're so > > > > against this patch? > > > > > > > > > > This change handles two different connectors in komeda internally, compare > > > with one interface, it increases the complexity, more risk of bug and more > > > cost of maintainance. > > > > > > > Well, it's only about how to bind the drivers - two different methods > > of binding, not two different connectors. I would argue that carrying > > our out-of-tree patches to support both platforms is a larger > > maintenance burden. > > > > Honestly this looks like a win-win to me. We get the superior approach > > when its supported, and still get to support bridges which are more > > common. > > > > As/when improvements are made to the bridge code we can remove the > > component bits and not lose anything. > > There was an idea a while back about using the device links code to > solve the bridge issue - but at the time the device links code wasn't > up to the job. I think that's been resolved now, but I haven't been > able to confirm it. I did propose some patches for bridge at the > time but they probably need updating. I think the only patches that existed where for panel, and we only discussed the bridge case. At least I can only find patches for panel,not bridge, but might be missing something. Either way I think device core is fixed now, so would be really great if someone can give this another stab, and make drm_bridge/panel easier to use without fireworks on unload. -Daniel
On Tue, Oct 22, 2019 at 10:42:10AM +0200, Daniel Vetter wrote: > On Thu, Oct 17, 2019 at 12:41:37PM +0100, Russell King - ARM Linux admin wrote: > > On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote: > > > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote: > > > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote: > > > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote: > > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote: > > > > > > > > > > > > > > If James is strongly against merging this, maybe we just swap > > > > > > > wholesale to bridge? But for me, the pragmatic approach would be this > > > > > > > stop-gap. > > > > > > > > > > > > > > > > > > > This is a good idea, and I vote +ULONG_MAX :) > > > > > > > > > > > > and I also checked tda998x driver, it supports bridge. so swap the > > > > > > wholesale to brige is perfect. :) > > > > > > > > > > > > > > > > Well, as Mihail wrote, it's definitely not perfect. > > > > > > > > > > Today, if you rmmod tda998x with the DPU driver still loaded, > > > > > everything will be unbound gracefully. > > > > > > > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge > > > > > driver the DPU is using) with the DPU driver still loaded will result > > > > > in a crash. > > > > > > > > I haven't read the bridge code, but seems this is a bug of drm_bridge, > > > > since if the bridge is still in using by others, the rmmod should fail > > > > > > > > > > Correct, but there's no fix for that today. You can also take a look > > > at the thread linked from Mihail's cover letter. > > > > > > > And personally opinion, if the bridge doesn't handle the dependence. > > > > for us: > > > > > > > > - add such support to bridge > > > > > > That would certainly be helpful. I don't know if there's consensus on > > > how to do that. > > > > > > > or > > > > - just do the insmod/rmmod in correct order. > > > > > > > > > So, there really are proper benefits to sticking with the component > > > > > code for tda998x, which is why I'd like to understand why you're so > > > > > against this patch? > > > > > > > > > > > > > This change handles two different connectors in komeda internally, compare > > > > with one interface, it increases the complexity, more risk of bug and more > > > > cost of maintainance. > > > > > > > > > > Well, it's only about how to bind the drivers - two different methods > > > of binding, not two different connectors. I would argue that carrying > > > our out-of-tree patches to support both platforms is a larger > > > maintenance burden. > > > > > > Honestly this looks like a win-win to me. We get the superior approach > > > when its supported, and still get to support bridges which are more > > > common. > > > > > > As/when improvements are made to the bridge code we can remove the > > > component bits and not lose anything. > > > > There was an idea a while back about using the device links code to > > solve the bridge issue - but at the time the device links code wasn't > > up to the job. I think that's been resolved now, but I haven't been > > able to confirm it. I did propose some patches for bridge at the > > time but they probably need updating. > > I think the only patches that existed where for panel, and we only > discussed the bridge case. At least I can only find patches for panel,not > bridge, but might be missing something. I had a patches, which is why I raised the problem with the core: 6961edfee26d bridge hacks using device links but it never went further than an experiment at the time because of the problems in the core. As it was a hack, it never got posted. Seems that kernel tree (for the cubox) is still 5.2 based, so has a lot of patches and might need updating to a more recent base before anything can be tested.
On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Tue, Oct 22, 2019 at 10:42:10AM +0200, Daniel Vetter wrote: > > On Thu, Oct 17, 2019 at 12:41:37PM +0100, Russell King - ARM Linux admin wrote: > > > On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote: > > > > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote: > > > > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote: > > > > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote: > > > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote: > > > > > > > > > > > > > > > > If James is strongly against merging this, maybe we just swap > > > > > > > > wholesale to bridge? But for me, the pragmatic approach would be this > > > > > > > > stop-gap. > > > > > > > > > > > > > > > > > > > > > > This is a good idea, and I vote +ULONG_MAX :) > > > > > > > > > > > > > > and I also checked tda998x driver, it supports bridge. so swap the > > > > > > > wholesale to brige is perfect. :) > > > > > > > > > > > > > > > > > > > Well, as Mihail wrote, it's definitely not perfect. > > > > > > > > > > > > Today, if you rmmod tda998x with the DPU driver still loaded, > > > > > > everything will be unbound gracefully. > > > > > > > > > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge > > > > > > driver the DPU is using) with the DPU driver still loaded will result > > > > > > in a crash. > > > > > > > > > > I haven't read the bridge code, but seems this is a bug of drm_bridge, > > > > > since if the bridge is still in using by others, the rmmod should fail > > > > > > > > > > > > > Correct, but there's no fix for that today. You can also take a look > > > > at the thread linked from Mihail's cover letter. > > > > > > > > > And personally opinion, if the bridge doesn't handle the dependence. > > > > > for us: > > > > > > > > > > - add such support to bridge > > > > > > > > That would certainly be helpful. I don't know if there's consensus on > > > > how to do that. > > > > > > > > > or > > > > > - just do the insmod/rmmod in correct order. > > > > > > > > > > > So, there really are proper benefits to sticking with the component > > > > > > code for tda998x, which is why I'd like to understand why you're so > > > > > > against this patch? > > > > > > > > > > > > > > > > This change handles two different connectors in komeda internally, compare > > > > > with one interface, it increases the complexity, more risk of bug and more > > > > > cost of maintainance. > > > > > > > > > > > > > Well, it's only about how to bind the drivers - two different methods > > > > of binding, not two different connectors. I would argue that carrying > > > > our out-of-tree patches to support both platforms is a larger > > > > maintenance burden. > > > > > > > > Honestly this looks like a win-win to me. We get the superior approach > > > > when its supported, and still get to support bridges which are more > > > > common. > > > > > > > > As/when improvements are made to the bridge code we can remove the > > > > component bits and not lose anything. > > > > > > There was an idea a while back about using the device links code to > > > solve the bridge issue - but at the time the device links code wasn't > > > up to the job. I think that's been resolved now, but I haven't been > > > able to confirm it. I did propose some patches for bridge at the > > > time but they probably need updating. > > > > I think the only patches that existed where for panel, and we only > > discussed the bridge case. At least I can only find patches for panel,not > > bridge, but might be missing something. > > I had a patches, which is why I raised the problem with the core: > > 6961edfee26d bridge hacks using device links > > but it never went further than an experiment at the time because of the > problems in the core. As it was a hack, it never got posted. Seems > that kernel tree (for the cubox) is still 5.2 based, so has a lot of > patches and might need updating to a more recent base before anything > can be tested. For reference, the panel patch: https://patchwork.kernel.org/patch/10364873/ And the huge discussion around bridges, that resulted in Rafael Wyzocki fixing all the core issues: https://www.spinics.net/lists/dri-devel/msg201927.html James, do you want to look into this for bridges? Cheers, Daniel
On Tue, Oct 22, 2019 at 10:50:42AM +0200, Daniel Vetter wrote: > On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: > > I had a patches, which is why I raised the problem with the core: > > > > 6961edfee26d bridge hacks using device links > > > > but it never went further than an experiment at the time because of the > > problems in the core. As it was a hack, it never got posted. Seems > > that kernel tree (for the cubox) is still 5.2 based, so has a lot of > > patches and might need updating to a more recent base before anything > > can be tested. > > > For reference, the panel patch: > > https://patchwork.kernel.org/patch/10364873/ > > And the huge discussion around bridges, that resulted in Rafael > Wyzocki fixing all the core issues: > > https://www.spinics.net/lists/dri-devel/msg201927.html > > James, do you want to look into this for bridges? I can now confirm that it does work.
On Tue, Oct 22, 2019 at 03:42:07PM +0100, Russell King - ARM Linux admin wrote: > On Tue, Oct 22, 2019 at 10:50:42AM +0200, Daniel Vetter wrote: > > On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin > > <linux@armlinux.org.uk> wrote: > > > I had a patches, which is why I raised the problem with the core: > > > > > > 6961edfee26d bridge hacks using device links > > > > > > but it never went further than an experiment at the time because of the > > > problems in the core. As it was a hack, it never got posted. Seems > > > that kernel tree (for the cubox) is still 5.2 based, so has a lot of > > > patches and might need updating to a more recent base before anything > > > can be tested. > > > > > > For reference, the panel patch: > > > > https://patchwork.kernel.org/patch/10364873/ > > > > And the huge discussion around bridges, that resulted in Rafael > > Wyzocki fixing all the core issues: > > > > https://www.spinics.net/lists/dri-devel/msg201927.html > > > > James, do you want to look into this for bridges? > > I can now confirm that it does work. Something like this - it's based off an older kernel, so may be missing some of the bridge drivers, but should be sufficient for people to test with. 8<==== From: Russell King <rmk+kernel@armlinux.org.uk> Subject: [PATCH] drm/bridge: add support for device links to bridge 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> --- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 1 + drivers/gpu/drm/bridge/analogix-anx78xx.c | 1 + drivers/gpu/drm/bridge/dumb-vga-dac.c | 1 + drivers/gpu/drm/bridge/lvds-encoder.c | 1 + .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 1 + drivers/gpu/drm/bridge/nxp-ptn3460.c | 1 + drivers/gpu/drm/bridge/panel.c | 1 + drivers/gpu/drm/bridge/parade-ps8622.c | 1 + drivers/gpu/drm/bridge/sii902x.c | 1 + drivers/gpu/drm/bridge/sii9234.c | 1 + drivers/gpu/drm/bridge/sil-sii8620.c | 1 + drivers/gpu/drm/bridge/tc358767.c | 1 + drivers/gpu/drm/bridge/ti-tfp410.c | 1 + drivers/gpu/drm/drm_bridge.c | 48 ++++++++++++++----- drivers/gpu/drm/i2c/tda998x_drv.c | 1 + include/drm/drm_bridge.h | 4 ++ 16 files changed, 53 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index f6d2681f6927..6a5906da58ea 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1217,6 +1217,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) goto err_unregister_cec; adv7511->bridge.funcs = &adv7511_bridge_funcs; + adv7511->bridge.device = dev; adv7511->bridge.of_node = dev->of_node; drm_bridge_add(&adv7511->bridge); diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c index 3c7cc5af735c..77ff17c38037 100644 --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c @@ -1323,6 +1323,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client, mutex_init(&anx78xx->lock); + anx78xx->bridge.device = &client->dev; #if IS_ENABLED(CONFIG_OF) anx78xx->bridge.of_node = client->dev.of_node; #endif diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c index d32885b906ae..40169920560e 100644 --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c @@ -202,6 +202,7 @@ static int dumb_vga_probe(struct platform_device *pdev) } vga->bridge.funcs = &dumb_vga_bridge_funcs; + vga->bridge.device = &pdev->dev; vga->bridge.of_node = pdev->dev.of_node; vga->bridge.timings = of_device_get_match_data(&pdev->dev); diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c index 2ab2c234f26c..5012be35a5fb 100644 --- a/drivers/gpu/drm/bridge/lvds-encoder.c +++ b/drivers/gpu/drm/bridge/lvds-encoder.c @@ -115,6 +115,7 @@ static int lvds_encoder_probe(struct platform_device *pdev) * to look up. */ lvds_encoder->bridge.of_node = dev->of_node; + lvds_encoder->bridge.device = dev; lvds_encoder->bridge.funcs = &funcs; drm_bridge_add(&lvds_encoder->bridge); 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 79311f8354bd..e211c57f6f56 100644 --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c @@ -304,6 +304,7 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c, /* drm bridge initialization */ ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs; + ge_b850v3_lvds_ptr->bridge.device = dev; ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node; drm_bridge_add(&ge_b850v3_lvds_ptr->bridge); diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c index 98bc650b8c95..00097e314575 100644 --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c @@ -323,6 +323,7 @@ static int ptn3460_probe(struct i2c_client *client, } ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs; + ptn_bridge->bridge.device = dev; ptn_bridge->bridge.of_node = dev->of_node; drm_bridge_add(&ptn_bridge->bridge); diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index b12ae3a4c5f1..eab7126f0d61 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -168,6 +168,7 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, panel_bridge->panel = panel; panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs; + panel_bridge->bridge.device = panel->dev; #ifdef CONFIG_OF panel_bridge->bridge.of_node = panel->dev->of_node; #endif diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c index 2d88146e4836..ff79df0ff183 100644 --- a/drivers/gpu/drm/bridge/parade-ps8622.c +++ b/drivers/gpu/drm/bridge/parade-ps8622.c @@ -589,6 +589,7 @@ static int ps8622_probe(struct i2c_client *client, } ps8622->bridge.funcs = &ps8622_bridge_funcs; + ps8622->bridge.device = dev; ps8622->bridge.of_node = dev->of_node; drm_bridge_add(&ps8622->bridge); diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c index dd7aa466b280..ef768b149bee 100644 --- a/drivers/gpu/drm/bridge/sii902x.c +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -991,6 +991,7 @@ static int sii902x_probe(struct i2c_client *client, } sii902x->bridge.funcs = &sii902x_bridge_funcs; + sii902x->bridge.device = dev; sii902x->bridge.of_node = dev->of_node; sii902x->bridge.timings = &default_sii902x_timings; drm_bridge_add(&sii902x->bridge); diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c index 25d4ad8c7ad6..824ffaeff16e 100644 --- a/drivers/gpu/drm/bridge/sii9234.c +++ b/drivers/gpu/drm/bridge/sii9234.c @@ -936,6 +936,7 @@ static int sii9234_probe(struct i2c_client *client, i2c_set_clientdata(client, ctx); ctx->bridge.funcs = &sii9234_bridge_funcs; + ctx->bridge.device = dev; ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge); diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index bd3165ee5354..5bc56c5f6826 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -2333,6 +2333,7 @@ static int sii8620_probe(struct i2c_client *client, i2c_set_clientdata(client, ctx); ctx->bridge.funcs = &sii8620_bridge_funcs; + ctx->bridge.device = dev; ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge); diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 13ade28a36a8..d62c6925c5fe 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1526,6 +1526,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) return ret; tc->bridge.funcs = &tc_bridge_funcs; + tc->bridge.device = dev; tc->bridge.of_node = dev->of_node; drm_bridge_add(&tc->bridge); diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c index dbf35c7bc85e..2f9899d7d4b4 100644 --- a/drivers/gpu/drm/bridge/ti-tfp410.c +++ b/drivers/gpu/drm/bridge/ti-tfp410.c @@ -326,6 +326,7 @@ static int tfp410_init(struct device *dev, bool i2c) dev_set_drvdata(dev, dvi); dvi->bridge.funcs = &tfp410_bridge_funcs; + dvi->bridge.device = dev; dvi->bridge.of_node = dev->of_node; dvi->bridge.timings = &dvi->timings; dvi->dev = dev; diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cba537c99e43..b4561ce63a49 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" @@ -463,6 +464,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 @@ -474,21 +501,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/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index e79507fb225f..5d4122bcf7ff 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -2201,6 +2201,7 @@ static int tda998x_create(struct device *dev) } priv->bridge.funcs = &tda998x_bridge_funcs; + priv->bridge.device = dev; #ifdef CONFIG_OF priv->bridge.of_node = dev->of_node; #endif diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 7616f6562fe4..f8a3af42a382 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; @@ -403,6 +405,8 @@ struct drm_bridge { void drm_bridge_add(struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); 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, Oct 22, 2019 at 10:50:42AM +0200, Daniel Vetter wrote: > On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: > > > > On Tue, Oct 22, 2019 at 10:42:10AM +0200, Daniel Vetter wrote: > > > On Thu, Oct 17, 2019 at 12:41:37PM +0100, Russell King - ARM Linux admin wrote: > > > > On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote: > > > > > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote: > > > > > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote: > > > > > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote: > > > > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote: > > > > > > > > > > > > > > > > > > If James is strongly against merging this, maybe we just swap > > > > > > > > > wholesale to bridge? But for me, the pragmatic approach would be this > > > > > > > > > stop-gap. > > > > > > > > > > > > > > > > > > > > > > > > > This is a good idea, and I vote +ULONG_MAX :) > > > > > > > > > > > > > > > > and I also checked tda998x driver, it supports bridge. so swap the > > > > > > > > wholesale to brige is perfect. :) > > > > > > > > > > > > > > > > > > > > > > Well, as Mihail wrote, it's definitely not perfect. > > > > > > > > > > > > > > Today, if you rmmod tda998x with the DPU driver still loaded, > > > > > > > everything will be unbound gracefully. > > > > > > > > > > > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge > > > > > > > driver the DPU is using) with the DPU driver still loaded will result > > > > > > > in a crash. > > > > > > > > > > > > I haven't read the bridge code, but seems this is a bug of drm_bridge, > > > > > > since if the bridge is still in using by others, the rmmod should fail > > > > > > > > > > > > > > > > Correct, but there's no fix for that today. You can also take a look > > > > > at the thread linked from Mihail's cover letter. > > > > > > > > > > > And personally opinion, if the bridge doesn't handle the dependence. > > > > > > for us: > > > > > > > > > > > > - add such support to bridge > > > > > > > > > > That would certainly be helpful. I don't know if there's consensus on > > > > > how to do that. > > > > > > > > > > > or > > > > > > - just do the insmod/rmmod in correct order. > > > > > > > > > > > > > So, there really are proper benefits to sticking with the component > > > > > > > code for tda998x, which is why I'd like to understand why you're so > > > > > > > against this patch? > > > > > > > > > > > > > > > > > > > This change handles two different connectors in komeda internally, compare > > > > > > with one interface, it increases the complexity, more risk of bug and more > > > > > > cost of maintainance. > > > > > > > > > > > > > > > > Well, it's only about how to bind the drivers - two different methods > > > > > of binding, not two different connectors. I would argue that carrying > > > > > our out-of-tree patches to support both platforms is a larger > > > > > maintenance burden. > > > > > > > > > > Honestly this looks like a win-win to me. We get the superior approach > > > > > when its supported, and still get to support bridges which are more > > > > > common. > > > > > > > > > > As/when improvements are made to the bridge code we can remove the > > > > > component bits and not lose anything. > > > > > > > > There was an idea a while back about using the device links code to > > > > solve the bridge issue - but at the time the device links code wasn't > > > > up to the job. I think that's been resolved now, but I haven't been > > > > able to confirm it. I did propose some patches for bridge at the > > > > time but they probably need updating. > > > > > > I think the only patches that existed where for panel, and we only > > > discussed the bridge case. At least I can only find patches for panel,not > > > bridge, but might be missing something. > > > > I had a patches, which is why I raised the problem with the core: > > > > 6961edfee26d bridge hacks using device links > > > > but it never went further than an experiment at the time because of the > > problems in the core. As it was a hack, it never got posted. Seems > > that kernel tree (for the cubox) is still 5.2 based, so has a lot of > > patches and might need updating to a more recent base before anything > > can be tested. > > > For reference, the panel patch: > > https://patchwork.kernel.org/patch/10364873/ > > And the huge discussion around bridges, that resulted in Rafael > Wyzocki fixing all the core issues: > > https://www.spinics.net/lists/dri-devel/msg201927.html > > James, do you want to look into this for bridges? > Hi Daniel: It's my honour. but I don't have much time in the next 3 weeks. And I talked with Mihail, he will help to check this problem. Thanks James. > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Russell, On Tuesday, 22 October 2019 15:53:29 BST Russell King - ARM Linux admin wrote: > On Tue, Oct 22, 2019 at 03:42:07PM +0100, Russell King - ARM Linux admin wrote: > > On Tue, Oct 22, 2019 at 10:50:42AM +0200, Daniel Vetter wrote: > > > On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin > > > <linux@armlinux.org.uk> wrote: > > > > I had a patches, which is why I raised the problem with the core: > > > > > > > > 6961edfee26d bridge hacks using device links > > > > > > > > but it never went further than an experiment at the time because of the > > > > problems in the core. As it was a hack, it never got posted. Seems > > > > that kernel tree (for the cubox) is still 5.2 based, so has a lot of > > > > patches and might need updating to a more recent base before anything > > > > can be tested. > > > > > > > > > For reference, the panel patch: > > > > > > https://patchwork.kernel.org/patch/10364873/ > > > > > > And the huge discussion around bridges, that resulted in Rafael > > > Wyzocki fixing all the core issues: > > > > > > https://www.spinics.net/lists/dri-devel/msg201927.html > > > > > > James, do you want to look into this for bridges? > > > > I can now confirm that it does work. > > Something like this - it's based off an older kernel, so may be missing > some of the bridge drivers, but should be sufficient for people to test > with. Thanks for the patch, I tested to the limit that our driver allows at the moment -- rmmod'ing the bridge while the driver is not in use -- and I don't see any issues there. komeda successfully gets removed then re-probed once the bridge reappears. For that part, Tested-by: Mihail Atanassov <mihail.atanassov@arm.com> I couldn't sadly check a hot unplug since we have an mm bug or two that I need to sort out first. If anyone else has a hot-unplug capable driver and can test, it'd be good to ensure that also functions properly. > > 8<==== > From: Russell King <rmk+kernel@armlinux.org.uk> > Subject: [PATCH] drm/bridge: add support for device links to bridge > > 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> > --- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 1 + > drivers/gpu/drm/bridge/analogix-anx78xx.c | 1 + > drivers/gpu/drm/bridge/dumb-vga-dac.c | 1 + > drivers/gpu/drm/bridge/lvds-encoder.c | 1 + > .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 1 + > drivers/gpu/drm/bridge/nxp-ptn3460.c | 1 + > drivers/gpu/drm/bridge/panel.c | 1 + > drivers/gpu/drm/bridge/parade-ps8622.c | 1 + > drivers/gpu/drm/bridge/sii902x.c | 1 + > drivers/gpu/drm/bridge/sii9234.c | 1 + > drivers/gpu/drm/bridge/sil-sii8620.c | 1 + > drivers/gpu/drm/bridge/tc358767.c | 1 + > drivers/gpu/drm/bridge/ti-tfp410.c | 1 + > drivers/gpu/drm/drm_bridge.c | 48 ++++++++++++++----- > drivers/gpu/drm/i2c/tda998x_drv.c | 1 + > include/drm/drm_bridge.h | 4 ++ > 16 files changed, 53 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index f6d2681f6927..6a5906da58ea 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -1217,6 +1217,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > goto err_unregister_cec; > > adv7511->bridge.funcs = &adv7511_bridge_funcs; > + adv7511->bridge.device = dev; > adv7511->bridge.of_node = dev->of_node; > > drm_bridge_add(&adv7511->bridge); > diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c > index 3c7cc5af735c..77ff17c38037 100644 > --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c > +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c > @@ -1323,6 +1323,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client, > > mutex_init(&anx78xx->lock); > > + anx78xx->bridge.device = &client->dev; > #if IS_ENABLED(CONFIG_OF) > anx78xx->bridge.of_node = client->dev.of_node; > #endif > diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c > index d32885b906ae..40169920560e 100644 > --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c > +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c > @@ -202,6 +202,7 @@ static int dumb_vga_probe(struct platform_device *pdev) > } > > vga->bridge.funcs = &dumb_vga_bridge_funcs; > + vga->bridge.device = &pdev->dev; > vga->bridge.of_node = pdev->dev.of_node; > vga->bridge.timings = of_device_get_match_data(&pdev->dev); > > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c > index 2ab2c234f26c..5012be35a5fb 100644 > --- a/drivers/gpu/drm/bridge/lvds-encoder.c > +++ b/drivers/gpu/drm/bridge/lvds-encoder.c > @@ -115,6 +115,7 @@ static int lvds_encoder_probe(struct platform_device *pdev) > * to look up. > */ > lvds_encoder->bridge.of_node = dev->of_node; > + lvds_encoder->bridge.device = dev; > lvds_encoder->bridge.funcs = &funcs; > drm_bridge_add(&lvds_encoder->bridge); > > 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 79311f8354bd..e211c57f6f56 100644 > --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c > +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c > @@ -304,6 +304,7 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c, > > /* drm bridge initialization */ > ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs; > + ge_b850v3_lvds_ptr->bridge.device = dev; > ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node; > drm_bridge_add(&ge_b850v3_lvds_ptr->bridge); > > diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c > index 98bc650b8c95..00097e314575 100644 > --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c > +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c > @@ -323,6 +323,7 @@ static int ptn3460_probe(struct i2c_client *client, > } > > ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs; > + ptn_bridge->bridge.device = dev; > ptn_bridge->bridge.of_node = dev->of_node; > drm_bridge_add(&ptn_bridge->bridge); > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index b12ae3a4c5f1..eab7126f0d61 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -168,6 +168,7 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, > panel_bridge->panel = panel; > > panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs; > + panel_bridge->bridge.device = panel->dev; > #ifdef CONFIG_OF > panel_bridge->bridge.of_node = panel->dev->of_node; > #endif > diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c > index 2d88146e4836..ff79df0ff183 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8622.c > +++ b/drivers/gpu/drm/bridge/parade-ps8622.c > @@ -589,6 +589,7 @@ static int ps8622_probe(struct i2c_client *client, > } > > ps8622->bridge.funcs = &ps8622_bridge_funcs; > + ps8622->bridge.device = dev; > ps8622->bridge.of_node = dev->of_node; > drm_bridge_add(&ps8622->bridge); > > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c > index dd7aa466b280..ef768b149bee 100644 > --- a/drivers/gpu/drm/bridge/sii902x.c > +++ b/drivers/gpu/drm/bridge/sii902x.c > @@ -991,6 +991,7 @@ static int sii902x_probe(struct i2c_client *client, > } > > sii902x->bridge.funcs = &sii902x_bridge_funcs; > + sii902x->bridge.device = dev; > sii902x->bridge.of_node = dev->of_node; > sii902x->bridge.timings = &default_sii902x_timings; > drm_bridge_add(&sii902x->bridge); > diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c > index 25d4ad8c7ad6..824ffaeff16e 100644 > --- a/drivers/gpu/drm/bridge/sii9234.c > +++ b/drivers/gpu/drm/bridge/sii9234.c > @@ -936,6 +936,7 @@ static int sii9234_probe(struct i2c_client *client, > i2c_set_clientdata(client, ctx); > > ctx->bridge.funcs = &sii9234_bridge_funcs; > + ctx->bridge.device = dev; > ctx->bridge.of_node = dev->of_node; > drm_bridge_add(&ctx->bridge); > > diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c > index bd3165ee5354..5bc56c5f6826 100644 > --- a/drivers/gpu/drm/bridge/sil-sii8620.c > +++ b/drivers/gpu/drm/bridge/sil-sii8620.c > @@ -2333,6 +2333,7 @@ static int sii8620_probe(struct i2c_client *client, > i2c_set_clientdata(client, ctx); > > ctx->bridge.funcs = &sii8620_bridge_funcs; > + ctx->bridge.device = dev; > ctx->bridge.of_node = dev->of_node; > drm_bridge_add(&ctx->bridge); > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 13ade28a36a8..d62c6925c5fe 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -1526,6 +1526,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > return ret; > > tc->bridge.funcs = &tc_bridge_funcs; > + tc->bridge.device = dev; > tc->bridge.of_node = dev->of_node; > drm_bridge_add(&tc->bridge); > > diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c > index dbf35c7bc85e..2f9899d7d4b4 100644 > --- a/drivers/gpu/drm/bridge/ti-tfp410.c > +++ b/drivers/gpu/drm/bridge/ti-tfp410.c > @@ -326,6 +326,7 @@ static int tfp410_init(struct device *dev, bool i2c) > dev_set_drvdata(dev, dvi); > > dvi->bridge.funcs = &tfp410_bridge_funcs; > + dvi->bridge.device = dev; > dvi->bridge.of_node = dev->of_node; > dvi->bridge.timings = &dvi->timings; > dvi->dev = dev; > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index cba537c99e43..b4561ce63a49 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" > @@ -463,6 +464,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 > @@ -474,21 +501,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/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index e79507fb225f..5d4122bcf7ff 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -2201,6 +2201,7 @@ static int tda998x_create(struct device *dev) > } > > priv->bridge.funcs = &tda998x_bridge_funcs; > + priv->bridge.device = dev; > #ifdef CONFIG_OF > priv->bridge.of_node = dev->of_node; > #endif > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 7616f6562fe4..f8a3af42a382 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; > @@ -403,6 +405,8 @@ struct drm_bridge { > void drm_bridge_add(struct drm_bridge *bridge); > void drm_bridge_remove(struct drm_bridge *bridge); > 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); > >
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h index e392b8ffc372..64d2902e2e0c 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h @@ -176,6 +176,11 @@ struct komeda_dev { /** @irq: irq number */ int irq; + /** @has_components: + * + * if true, use the component framework to bind/unbind driver + */ + bool has_components; /** @lock: used to protect dpmode */ struct mutex lock; diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c index 9ed25ffe0e22..34cfc6a4c3e4 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c @@ -10,6 +10,8 @@ #include <linux/component.h> #include <linux/pm_runtime.h> #include <drm/drm_of.h> +#include <drm/drm_bridge.h> +#include <drm/drm_encoder.h> #include "komeda_dev.h" #include "komeda_kms.h" @@ -69,18 +71,35 @@ 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 *master, + struct komeda_drv *mdrv, + struct component_match **match, + 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) { + if (!remote) + return 0; + + if (komeda_remote_device_is_component(np, port, endpoint)) { + mdrv->mdev.has_components = true; drm_of_component_match_add(master, match, compare_of, remote); - of_node_put(remote); + goto exit; + } + + bridge = of_drm_find_bridge(remote); + if (!bridge) { + ret = -EPROBE_DEFER; + goto exit; } + +exit: + of_node_put(remote); + return ret; } static int komeda_platform_probe(struct platform_device *pdev) @@ -89,6 +108,7 @@ static int komeda_platform_probe(struct platform_device *pdev) struct component_match *match = NULL; struct device_node *child; struct komeda_drv *mdrv; + int ret; if (!dev->of_node) return -ENODEV; @@ -101,14 +121,27 @@ 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(dev, mdrv, &match, child, + KOMEDA_OF_PORT_OUTPUT, 0); + if (ret) + goto free_mdrv; + ret = komeda_add_slave(dev, mdrv, &match, child, + KOMEDA_OF_PORT_OUTPUT, 1); + if (ret) + goto free_mdrv; } dev_set_drvdata(dev, mdrv); - return component_master_add_with_match(dev, &komeda_master_ops, match); + return mdrv->mdev.has_components + ? component_master_add_with_match( + dev, &komeda_master_ops, match) + : komeda_bind(dev); + +free_mdrv: + devm_kfree(dev, mdrv); + return ret; } static int komeda_platform_remove(struct platform_device *pdev) @@ -116,7 +149,10 @@ static int komeda_platform_remove(struct platform_device *pdev) struct device *dev = &pdev->dev; struct komeda_drv *mdrv = dev_get_drvdata(dev); - component_master_del(dev, &komeda_master_ops); + if (mdrv->mdev.has_components) + component_master_del(dev, &komeda_master_ops); + else + komeda_unbind(dev); dev_set_drvdata(dev, NULL); devm_kfree(dev, mdrv); diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c index 03dcf1d7688f..6eb52d1b20a0 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c @@ -6,6 +6,7 @@ */ #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 +15,8 @@ #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_irq.h> +#include <drm/drm_modes.h> +#include <drm/drm_of.h> #include <drm/drm_probe_helper.h> #include <drm/drm_vblank.h> @@ -267,6 +270,130 @@ 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, +}; + +bool komeda_remote_device_is_component(struct device_node *local, + u32 port, u32 endpoint) +{ + struct device_node *remote; + char const * const component_devices[] = { + "nxp,tda998x", + }; + int i; + bool ret = false; + + remote = of_graph_get_remote_node(local, port, endpoint); + if (!remote) + return false; + + /* Coprocessor endpoints are always component based. */ + if (port != KOMEDA_OF_PORT_OUTPUT) { + ret = true; + goto exit; + } + + for (i = 0; i < ARRAY_SIZE(component_devices); ++i) { + if (of_device_is_compatible(remote, component_devices[i])) { + ret = true; + goto exit; + } + } + +exit: + of_node_put(remote); + return ret; +} + +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; + + if (komeda_remote_device_is_component(np, KOMEDA_OF_PORT_OUTPUT, 0)) + return 0; + + remote = of_graph_get_remote_node(np, KOMEDA_OF_PORT_OUTPUT, 0); + if (!remote) + return 0; + + bridge = of_drm_find_bridge(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; +} + +static int komeda_kms_attach_external(struct komeda_kms_dev *kms, + struct komeda_dev *mdev) +{ + int err; + + if (mdev->has_components) { + err = component_bind_all(mdev->dev, kms); + if (err) + return err; + } + + err = komeda_encoder_attach_bridges(kms, mdev); + if (err) + return err; + + return 0; +} + +static void komeda_kms_detach_external(struct komeda_dev *mdev, + struct drm_device *drm) +{ + if (mdev->has_components) + component_unbind_all(mdev->dev, drm); +} + int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev) { struct drm_device *drm; @@ -301,7 +428,7 @@ int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev) if (err) goto cleanup_mode_config; - err = component_bind_all(mdev->dev, kms); + err = komeda_kms_attach_external(kms, mdev); if (err) goto cleanup_mode_config; @@ -332,7 +459,7 @@ int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev) drm->irq_enabled = false; mdev->funcs->disable_irq(mdev); free_component_binding: - component_unbind_all(mdev->dev, drm); + komeda_kms_detach_external(mdev, drm); cleanup_mode_config: drm_mode_config_cleanup(drm); komeda_kms_cleanup_private_objs(kms); @@ -351,7 +478,7 @@ void komeda_kms_fini(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); + komeda_kms_detach_external(mdev, 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 e81ceb298034..c2856e19d092 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) @@ -184,4 +186,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc, int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev); void komeda_kms_fini(struct komeda_kms_dev *kms); +bool komeda_remote_device_is_component(struct device_node *local, + u32 port, u32 endpoint); + #endif /*_KOMEDA_KMS_H_*/
To support transmitters other than the tda998x, we need to allow non-component framework bridges to be attached to a dummy drm_encoder in our driver. For the existing supported encoder (tda998x), keep the behaviour as-is, since there's no way to unbind if a drm_bridge module goes away under our feet. Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> --- .../gpu/drm/arm/display/komeda/komeda_dev.h | 5 + .../gpu/drm/arm/display/komeda/komeda_drv.c | 58 ++++++-- .../gpu/drm/arm/display/komeda/komeda_kms.c | 133 +++++++++++++++++- .../gpu/drm/arm/display/komeda/komeda_kms.h | 5 + 4 files changed, 187 insertions(+), 14 deletions(-)