Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
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 > > > 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 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 > 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 > --- > 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-stdp-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 = _bridge_funcs; > + adv7511->bridge.device = dev; > adv7511->bridge.of_node = dev->of_node; > > drm_bridge_add(>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(>lock); > > + anx78xx->bridge.device = >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) >
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
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 > 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 +, Brian Starkey wrote: > > > > > On Thu, Oct 17, 2019 at 10:21:03AM +, james qian wang (Arm > > > > > Technology China) wrote: > > > > > > On Thu, Oct 17, 2019 at 08:20:56AM +, Brian Starkey wrote: > > > > > > > On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm > > > > > > > Technology China) wrote: > > > > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +, 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
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
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 > > 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 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 --- 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-stdp-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 = _bridge_funcs; + adv7511->bridge.device = dev; adv7511->bridge.of_node = dev->of_node; drm_bridge_add(>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(>lock); + anx78xx->bridge.device = >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 = _vga_bridge_funcs; + vga->bridge.device = >dev; vga->bridge.of_node = pdev->dev.of_node; vga->bridge.timings = of_device_get_match_data(>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 = drm_bridge_add(_encoder->bridge); diff --git a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
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 > 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. -- 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
Re: [RFC, 3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin 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 +, Brian Starkey wrote: > > > > On Thu, Oct 17, 2019 at 10:21:03AM +, james qian wang (Arm > > > > Technology China) wrote: > > > > > On Thu, Oct 17, 2019 at 08:20:56AM +, Brian Starkey wrote: > > > > > > On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm > > > > > > Technology China) wrote: > > > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +, 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
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 +, Brian Starkey wrote: > > > On Thu, Oct 17, 2019 at 10:21:03AM +, james qian wang (Arm Technology > > > China) wrote: > > > > On Thu, Oct 17, 2019 at 08:20:56AM +, Brian Starkey wrote: > > > > > On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm > > > > > Technology China) wrote: > > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +, 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. -- 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
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 +, Brian Starkey wrote: > > On Thu, Oct 17, 2019 at 10:21:03AM +, james qian wang (Arm Technology > > China) wrote: > > > On Thu, Oct 17, 2019 at 08:20:56AM +, Brian Starkey wrote: > > > > On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm > > > > Technology China) wrote: > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +, 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
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 +, Brian Starkey wrote: > > On Thu, Oct 17, 2019 at 10:21:03AM +, james qian wang (Arm Technology > > China) wrote: > > > On Thu, Oct 17, 2019 at 08:20:56AM +, Brian Starkey wrote: > > > > On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm > > > > Technology China) wrote: > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +, 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 > -- Mihail ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Fri, Oct 18, 2019 at 06:57:05AM +, 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
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 +, Brian Starkey wrote: > > On Thu, Oct 17, 2019 at 10:21:03AM +, james qian wang (Arm Technology > > China) wrote: > > > On Thu, Oct 17, 2019 at 08:20:56AM +, Brian Starkey wrote: > > > > On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm > > > > Technology China) wrote: > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +, 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Thu, Oct 17, 2019 at 10:48:12AM +, Brian Starkey wrote: > On Thu, Oct 17, 2019 at 10:21:03AM +, james qian wang (Arm Technology > China) wrote: > > On Thu, Oct 17, 2019 at 08:20:56AM +, Brian Starkey wrote: > > > On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm Technology > > > China) wrote: > > > > On Wed, Oct 16, 2019 at 04:22:07PM +, 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
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Thu, Oct 17, 2019 at 10:48:12AM +, Brian Starkey wrote: > On Thu, Oct 17, 2019 at 10:21:03AM +, james qian wang (Arm Technology > China) wrote: > > On Thu, Oct 17, 2019 at 08:20:56AM +, Brian Starkey wrote: > > > On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm Technology > > > China) wrote: > > > > On Wed, Oct 16, 2019 at 04:22:07PM +, 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. -- 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Thu, Oct 17, 2019 at 10:21:03AM +, james qian wang (Arm Technology China) wrote: > On Thu, Oct 17, 2019 at 08:20:56AM +, Brian Starkey wrote: > > On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm Technology > > China) wrote: > > > On Wed, Oct 16, 2019 at 04:22:07PM +, 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
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Thu, Oct 17, 2019 at 08:20:56AM +, Brian Starkey wrote: > On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm Technology > China) wrote: > > On Wed, Oct 16, 2019 at 04:22:07PM +, 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
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm Technology China) wrote: > On Wed, Oct 16, 2019 at 04:22:07PM +, 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
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Wed, Oct 16, 2019 at 04:22:07PM +, Brian Starkey wrote:
> Hi,
>
> On Wed, Oct 16, 2019 at 03:51:39PM +, 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 +, 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
> > > > ---
> > > > .../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
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
Hi,
On Wed, Oct 16, 2019 at 03:51:39PM +, 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 +, 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
> > > ---
> > > .../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
>
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
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 +, 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
> > ---
> > .../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]
>
--
Mihail
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Fri, Oct 04, 2019 at 02:34:42PM +, 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
> ---
> .../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
> #include
> #include
> +#include
> +#include
> #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, , child, KOMEDA_OF_PORT_OUTPUT, 0);
> - komeda_add_slave(dev, , child, KOMEDA_OF_PORT_OUTPUT, 1);
> + /* attach any remote devices if present */
> + ret = komeda_add_slave(dev, mdrv, , child,
> +KOMEDA_OF_PORT_OUTPUT, 0);
> + if (ret)
> + goto free_mdrv;
> + ret = komeda_add_slave(dev, mdrv, , child,
> +KOMEDA_OF_PORT_OUTPUT, 1);
> + if (ret)
> + goto free_mdrv;
> }
>
> dev_set_drvdata(dev, mdrv);
>
> - return component_master_add_with_match(dev, _master_ops, match);
> + return mdrv->mdev.has_components
> + ? component_master_add_with_match(
> + dev, _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 = >dev;
> struct komeda_drv *mdrv = dev_get_drvdata(dev);
>
> - component_master_del(dev, _master_ops);
> + if (mdrv->mdev.has_components)
> +
