| Message ID | 20170615204130.19255-2-eric@anholt.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/16/2017 02:11 AM, Eric Anholt wrote: > If the panel-bridge is being set up after the drm_mode_config_reset(), > then the connector's state would never get initialized, and we'd > dereference the NULL in the hotplug path. We also need to register > the connector, so that userspace can get at it. > Shouldn't the KMS driver make sure the panel-bridge is set up before drm_mode_config_reset? Is it the case when we're inserting the panel-bridge driver as a module? All the connectors that have been added are registered automatically when drm_dev_register() is called by the KMS driver. Registering a connector in the middle of setting up our driver is prone to race conditions if the userspace decides to use them immediately. Thanks, Archit > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > drivers/gpu/drm/bridge/panel.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index 67fe19e5a9c6..8ed8a70799c7 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -82,11 +82,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge) > > drm_mode_connector_attach_encoder(&panel_bridge->connector, > bridge->encoder); > + drm_atomic_helper_connector_reset(&panel_bridge->connector); > > ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); > if (ret < 0) > return ret; > > + drm_connector_register(&panel_bridge->connector); > + > return 0; > } > >
Archit Taneja <architt@codeaurora.org> writes: > On 06/16/2017 02:11 AM, Eric Anholt wrote: >> If the panel-bridge is being set up after the drm_mode_config_reset(), >> then the connector's state would never get initialized, and we'd >> dereference the NULL in the hotplug path. We also need to register >> the connector, so that userspace can get at it. >> > > Shouldn't the KMS driver make sure the panel-bridge is set up before > drm_mode_config_reset? Is it the case when we're inserting the > panel-bridge driver as a module? > > > All the connectors that have been added are registered automatically > when drm_dev_register() is called by the KMS driver. Registering a > connector in the middle of setting up our driver is prone to race > conditions if the userspace decides to use them immediately. Yeah, this is fixing initializing panel_bridge at DSI host_attach time, which in the case of a panel module that creates the DSI device (adv7533-style, like you said I should use as a reference) will be after drm_mode_config_reset() and drm_dev_register().
On 06/16/2017 08:13 PM, Eric Anholt wrote: > Archit Taneja <architt@codeaurora.org> writes: > >> On 06/16/2017 02:11 AM, Eric Anholt wrote: >>> If the panel-bridge is being set up after the drm_mode_config_reset(), >>> then the connector's state would never get initialized, and we'd >>> dereference the NULL in the hotplug path. We also need to register >>> the connector, so that userspace can get at it. >>> >> >> Shouldn't the KMS driver make sure the panel-bridge is set up before >> drm_mode_config_reset? Is it the case when we're inserting the >> panel-bridge driver as a module? >> >> >> All the connectors that have been added are registered automatically >> when drm_dev_register() is called by the KMS driver. Registering a >> connector in the middle of setting up our driver is prone to race >> conditions if the userspace decides to use them immediately. > > Yeah, this is fixing initializing panel_bridge at DSI host_attach time, > which in the case of a panel module that creates the DSI device > (adv7533-style, like you said I should use as a reference) will be after > drm_mode_config_reset() and drm_dev_register(). Okay. In the case of the msm kms driver, we defer probe until the adv7533 module is inserted, only then we proceed to drm_mode_config_reset() and drm_dev_register(). I assumed this was the general practice followed by most kms drivers. I.,e the kms driver defers probe until all connector related modules are inserted, and only then proceed to create a drm device. Feedback from others on this would be appreciated, though. Thanks, Archit
Hi Archit, On Tuesday 20 Jun 2017 09:18:00 Archit Taneja wrote: > On 06/16/2017 08:13 PM, Eric Anholt wrote: > > Archit Taneja <architt@codeaurora.org> writes: > >> On 06/16/2017 02:11 AM, Eric Anholt wrote: > >>> If the panel-bridge is being set up after the drm_mode_config_reset(), > >>> then the connector's state would never get initialized, and we'd > >>> dereference the NULL in the hotplug path. We also need to register > >>> the connector, so that userspace can get at it. > >> > >> Shouldn't the KMS driver make sure the panel-bridge is set up before > >> drm_mode_config_reset? Is it the case when we're inserting the > >> panel-bridge driver as a module? > >> > >> > >> All the connectors that have been added are registered automatically > >> when drm_dev_register() is called by the KMS driver. Registering a > >> connector in the middle of setting up our driver is prone to race > >> conditions if the userspace decides to use them immediately. > > > > Yeah, this is fixing initializing panel_bridge at DSI host_attach time, > > which in the case of a panel module that creates the DSI device > > (adv7533-style, like you said I should use as a reference) will be after > > drm_mode_config_reset() and drm_dev_register(). > > Okay. In the case of the msm kms driver, we defer probe until the > adv7533 module is inserted, only then we proceed to drm_mode_config_reset() > and drm_dev_register(). I assumed this was the general practice followed by > most kms drivers. I.,e the kms driver defers probe until all connector > related modules are inserted, and only then proceed to create a drm device. I'd love to see support for a more dynamic approach that would allow registering outputs at runtime. Until that's implemented, however, I agree with your statement, drivers should wait until all components are available before registering the DRM device. > Feedback from others on this would be appreciated, though.
Archit Taneja <architt@codeaurora.org> writes: > On 06/16/2017 08:13 PM, Eric Anholt wrote: >> Archit Taneja <architt@codeaurora.org> writes: >> >>> On 06/16/2017 02:11 AM, Eric Anholt wrote: >>>> If the panel-bridge is being set up after the drm_mode_config_reset(), >>>> then the connector's state would never get initialized, and we'd >>>> dereference the NULL in the hotplug path. We also need to register >>>> the connector, so that userspace can get at it. >>>> >>> >>> Shouldn't the KMS driver make sure the panel-bridge is set up before >>> drm_mode_config_reset? Is it the case when we're inserting the >>> panel-bridge driver as a module? >>> >>> >>> All the connectors that have been added are registered automatically >>> when drm_dev_register() is called by the KMS driver. Registering a >>> connector in the middle of setting up our driver is prone to race >>> conditions if the userspace decides to use them immediately. >> >> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, >> which in the case of a panel module that creates the DSI device >> (adv7533-style, like you said I should use as a reference) will be after >> drm_mode_config_reset() and drm_dev_register(). > > Okay. In the case of the msm kms driver, we defer probe until the > adv7533 module is inserted, only then we proceed to drm_mode_config_reset() > and drm_dev_register(). I assumed this was the general practice followed by > most kms drivers. I.,e the kms driver defers probe until all connector > related modules are inserted, and only then proceed to create a drm device. The problem, though, is the panel driver needs the MIPI DSI host to exist to call mipi_dsi_device_register_full() during the probe process. The adv7533 driver gets around this by registering the DSI device in the bridge attach step, but drm_panel doesn't have an attach step. Another alternative is my original version of the panel driver that was a mipi_dsi_device driver that registered the panel during the DSI device probe. That's why vc4's panel lookup is during the MIPI DSI attach phase, currently.
2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: > Archit Taneja <architt@codeaurora.org> writes: > >> On 06/16/2017 08:13 PM, Eric Anholt wrote: >>> Archit Taneja <architt@codeaurora.org> writes: >>> >>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: >>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), >>>>> then the connector's state would never get initialized, and we'd >>>>> dereference the NULL in the hotplug path. We also need to register >>>>> the connector, so that userspace can get at it. >>>>> >>>> >>>> Shouldn't the KMS driver make sure the panel-bridge is set up before >>>> drm_mode_config_reset? Is it the case when we're inserting the >>>> panel-bridge driver as a module? >>>> >>>> >>>> All the connectors that have been added are registered automatically >>>> when drm_dev_register() is called by the KMS driver. Registering a >>>> connector in the middle of setting up our driver is prone to race >>>> conditions if the userspace decides to use them immediately. >>> >>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, >>> which in the case of a panel module that creates the DSI device >>> (adv7533-style, like you said I should use as a reference) will be after >>> drm_mode_config_reset() and drm_dev_register(). >> >> Okay. In the case of the msm kms driver, we defer probe until the >> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() >> and drm_dev_register(). I assumed this was the general practice followed by >> most kms drivers. I.,e the kms driver defers probe until all connector >> related modules are inserted, and only then proceed to create a drm device. > > The problem, though, is the panel driver needs the MIPI DSI host to > exist to call mipi_dsi_device_register_full() during the probe process. > The adv7533 driver gets around this by registering the DSI device in the > bridge attach step, but drm_panel doesn't have an attach step. > > Another alternative is my original version of the panel driver that was > a mipi_dsi_device driver that registered the panel during the DSI device > probe. That's why vc4's panel lookup is during the MIPI DSI attach > phase, currently. + Philippe in copy because we have the same probing issue when adding panel-bridge with the dsi bridge > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: > 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: >> Archit Taneja <architt@codeaurora.org> writes: >> >>> On 06/16/2017 08:13 PM, Eric Anholt wrote: >>>> Archit Taneja <architt@codeaurora.org> writes: >>>> >>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: >>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), >>>>>> then the connector's state would never get initialized, and we'd >>>>>> dereference the NULL in the hotplug path. We also need to register >>>>>> the connector, so that userspace can get at it. >>>>>> >>>>> >>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before >>>>> drm_mode_config_reset? Is it the case when we're inserting the >>>>> panel-bridge driver as a module? >>>>> >>>>> >>>>> All the connectors that have been added are registered automatically >>>>> when drm_dev_register() is called by the KMS driver. Registering a >>>>> connector in the middle of setting up our driver is prone to race >>>>> conditions if the userspace decides to use them immediately. >>>> >>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, >>>> which in the case of a panel module that creates the DSI device >>>> (adv7533-style, like you said I should use as a reference) will be after >>>> drm_mode_config_reset() and drm_dev_register(). >>> >>> Okay. In the case of the msm kms driver, we defer probe until the >>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() >>> and drm_dev_register(). I assumed this was the general practice followed by >>> most kms drivers. I.,e the kms driver defers probe until all connector >>> related modules are inserted, and only then proceed to create a drm device. >> >> The problem, though, is the panel driver needs the MIPI DSI host to >> exist to call mipi_dsi_device_register_full() during the probe process. >> The adv7533 driver gets around this by registering the DSI device in the >> bridge attach step, but drm_panel doesn't have an attach step. I'm not sure how we can get around this. We had discussion about this on irc recently, but couldn't come up with a good conclusion. We could come up with a panel_attach() callback to make it similar to bridges, but that's just us avoiding the real issue. >> >> Another alternative is my original version of the panel driver that was >> a mipi_dsi_device driver that registered the panel during the DSI device >> probe. That's why vc4's panel lookup is during the MIPI DSI attach >> phase, currently. > This would require you to have a DSI device node in DT, rather than an i2c node, right? I don't know if we should do that because of a limitation in our drm_mipi_dsi and drm_panel frameworks. Does anyone have better ideas? Thanks, Archit > + Philippe in copy because we have the same probing issue when adding > panel-bridge > with the dsi bridge > >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>
On Thu, 22 Jun 2017 13:47:43 +0530 Archit Taneja <architt@codeaurora.org> wrote: > On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: > > 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: > >> Archit Taneja <architt@codeaurora.org> writes: > >> > >>> On 06/16/2017 08:13 PM, Eric Anholt wrote: > >>>> Archit Taneja <architt@codeaurora.org> writes: > >>>> > >>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: > >>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), > >>>>>> then the connector's state would never get initialized, and we'd > >>>>>> dereference the NULL in the hotplug path. We also need to register > >>>>>> the connector, so that userspace can get at it. > >>>>>> > >>>>> > >>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before > >>>>> drm_mode_config_reset? Is it the case when we're inserting the > >>>>> panel-bridge driver as a module? > >>>>> > >>>>> > >>>>> All the connectors that have been added are registered automatically > >>>>> when drm_dev_register() is called by the KMS driver. Registering a > >>>>> connector in the middle of setting up our driver is prone to race > >>>>> conditions if the userspace decides to use them immediately. > >>>> > >>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, > >>>> which in the case of a panel module that creates the DSI device > >>>> (adv7533-style, like you said I should use as a reference) will be after > >>>> drm_mode_config_reset() and drm_dev_register(). > >>> > >>> Okay. In the case of the msm kms driver, we defer probe until the > >>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() > >>> and drm_dev_register(). I assumed this was the general practice followed by > >>> most kms drivers. I.,e the kms driver defers probe until all connector > >>> related modules are inserted, and only then proceed to create a drm device. > >> > >> The problem, though, is the panel driver needs the MIPI DSI host to > >> exist to call mipi_dsi_device_register_full() during the probe process. > >> The adv7533 driver gets around this by registering the DSI device in the > >> bridge attach step, but drm_panel doesn't have an attach step. > > I'm not sure how we can get around this. We had discussion about this on irc > recently, but couldn't come up with a good conclusion. We could come up with a > panel_attach() callback to make it similar to bridges, but that's just us avoiding > the real issue. How about making DSI dev registration fully asynchronous, that is, DSI devs declared in the DT under the DSI host node will be registered/attached at probe time, and devs using another control bus (like the adv7533 controller over i2c) will be registered afterwards. That implies moving the drm_brige registration logic outside of the DSI host ->probe() path. The idea would be to check if all devs connected to the DSI bus are ready at dsi_host->attach() time. If they are, we can finally register the XXX -> DSI bridge. If they're not (because some devs connected to the DSI bus have not been probed yet), then we do not register the drm_bridge and wait for the next dsi_host->attach() event. With this solution, I think we can avoid the chicken&egg problem where the adv7533 dev is waiting for the DSI host to be probed to be able to register a DSI dev with mipi_dsi_device_register_full() and the DSI host needs all devs to be registered to not return -EPROBE_DEFER. > > >> > >> Another alternative is my original version of the panel driver that was > >> a mipi_dsi_device driver that registered the panel during the DSI device > >> probe. That's why vc4's panel lookup is during the MIPI DSI attach > >> phase, currently. > > > > This would require you to have a DSI device node in DT, rather than an i2c > node, right? I don't know if we should do that because of a limitation in > our drm_mipi_dsi and drm_panel frameworks. > > Does anyone have better ideas? Well, these mixed-bus cases are really unpractical to represent in the DT (and also not easy to deal with the device-model). I understand that the initial solution was to put the device under the control-bus, and not the data-bus, but then it leads to the current situation where we don't know exactly when things are ready to be used. Theoretically, we should wait for both the i2c and DSI ends to be attached to their respective controller before starting using the device. But that requires representing things with a 3rd DT node aggregating both components: i2c-bus { ... adv7533_i2c@xx { ... adv,adv7533-master = <&adv7533>; }; }; ... dsi-bus { ... adv7533_dsi: adv7533_dsi@xx { ... adv,adv7533-master = <&adv7533>; }; }; adv7533: adv7533 { ... adv,adv7533-dsi = <&adv7533_dsi>; adv,adv7533-i2c = <&adv7533_i2c>; }; I guess this kind of representation has already been discussed and rejected for good reasons (note that we could also use OF graph representation to link adv7533 master and its components).
On 06/22/2017 10:17 AM, Archit Taneja wrote: > > > On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: >> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: >>> Archit Taneja <architt@codeaurora.org> writes: >>> >>>> On 06/16/2017 08:13 PM, Eric Anholt wrote: >>>>> Archit Taneja <architt@codeaurora.org> writes: >>>>> >>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: >>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), >>>>>>> then the connector's state would never get initialized, and we'd >>>>>>> dereference the NULL in the hotplug path. We also need to register >>>>>>> the connector, so that userspace can get at it. >>>>>>> >>>>>> >>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before >>>>>> drm_mode_config_reset? Is it the case when we're inserting the >>>>>> panel-bridge driver as a module? >>>>>> >>>>>> >>>>>> All the connectors that have been added are registered automatically >>>>>> when drm_dev_register() is called by the KMS driver. Registering a >>>>>> connector in the middle of setting up our driver is prone to race >>>>>> conditions if the userspace decides to use them immediately. >>>>> >>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, >>>>> which in the case of a panel module that creates the DSI device >>>>> (adv7533-style, like you said I should use as a reference) will be after >>>>> drm_mode_config_reset() and drm_dev_register(). >>>> >>>> Okay. In the case of the msm kms driver, we defer probe until the >>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() >>>> and drm_dev_register(). I assumed this was the general practice followed by >>>> most kms drivers. I.,e the kms driver defers probe until all connector >>>> related modules are inserted, and only then proceed to create a drm device. >>> >>> The problem, though, is the panel driver needs the MIPI DSI host to >>> exist to call mipi_dsi_device_register_full() during the probe process. >>> The adv7533 driver gets around this by registering the DSI device in the >>> bridge attach step, but drm_panel doesn't have an attach step. > > I'm not sure how we can get around this. We had discussion about this on irc > recently, but couldn't come up with a good conclusion. We could come up with a > panel_attach() callback to make it similar to bridges, but that's just us avoiding > the real issue. > >>> >>> Another alternative is my original version of the panel driver that was >>> a mipi_dsi_device driver that registered the panel during the DSI device >>> probe. That's why vc4's panel lookup is during the MIPI DSI attach >>> phase, currently. >> > > This would require you to have a DSI device node in DT, rather than an i2c > node, right? I don't know if we should do that because of a limitation in > our drm_mipi_dsi and drm_panel frameworks. > > Does anyone have better ideas? > > Thanks, > Archit > >> + Philippe in copy because we have the same probing issue when adding >> panel-bridge >> with the dsi bridge >> When adding panel-bridge support to the Synopsys DesignWare DSI bridge, I came to the conclusion that the only solution to make it works properly (without patching drm) was to "add the DSI bridge" at the end of the dw_mipi_dsi_host_attach() (see https://lists.freedesktop.org/archives/dri-devel/2017-June/144717.html) ie. defers crtc & encoder probing until the panel-bridge connector is created. Before that, I spent a lot of time trying different solutions like patching panel-bridge as Eric did (drm_connector_register...), adding bind/unbind() everywhere, debugging around __drm_mode_object_add... Surely DSI Host & device mechanism + panels add complexity to the related connector creation... After reading this thread, I have no good solution to suggest... and "deferring probing until connector registration" works fine now on my side and seems to be the way others drivers work... Philippe >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >
On 22.06.2017 11:23, Boris Brezillon wrote: > On Thu, 22 Jun 2017 13:47:43 +0530 > Archit Taneja <architt@codeaurora.org> wrote: > >> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: >>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: >>>> Archit Taneja <architt@codeaurora.org> writes: >>>> >>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote: >>>>>> Archit Taneja <architt@codeaurora.org> writes: >>>>>> >>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: >>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), >>>>>>>> then the connector's state would never get initialized, and we'd >>>>>>>> dereference the NULL in the hotplug path. We also need to register >>>>>>>> the connector, so that userspace can get at it. >>>>>>>> >>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before >>>>>>> drm_mode_config_reset? Is it the case when we're inserting the >>>>>>> panel-bridge driver as a module? >>>>>>> >>>>>>> >>>>>>> All the connectors that have been added are registered automatically >>>>>>> when drm_dev_register() is called by the KMS driver. Registering a >>>>>>> connector in the middle of setting up our driver is prone to race >>>>>>> conditions if the userspace decides to use them immediately. >>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, >>>>>> which in the case of a panel module that creates the DSI device >>>>>> (adv7533-style, like you said I should use as a reference) will be after >>>>>> drm_mode_config_reset() and drm_dev_register(). >>>>> Okay. In the case of the msm kms driver, we defer probe until the >>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() >>>>> and drm_dev_register(). I assumed this was the general practice followed by >>>>> most kms drivers. I.,e the kms driver defers probe until all connector >>>>> related modules are inserted, and only then proceed to create a drm device. >>>> The problem, though, is the panel driver needs the MIPI DSI host to >>>> exist to call mipi_dsi_device_register_full() during the probe process. >>>> The adv7533 driver gets around this by registering the DSI device in the >>>> bridge attach step, but drm_panel doesn't have an attach step. >> I'm not sure how we can get around this. We had discussion about this on irc >> recently, but couldn't come up with a good conclusion. We could come up with a >> panel_attach() callback to make it similar to bridges, but that's just us avoiding >> the real issue. > How about making DSI dev registration fully asynchronous, that is, DSI > devs declared in the DT under the DSI host node will be > registered/attached at probe time, and devs using another control bus > (like the adv7533 controller over i2c) will be registered afterwards. > > That implies moving the drm_brige registration logic outside of the DSI > host ->probe() path. The idea would be to check if all devs connected > to the DSI bus are ready at dsi_host->attach() time. If they are, we > can finally register the XXX -> DSI bridge. If they're not (because > some devs connected to the DSI bus have not been probed yet), then we > do not register the drm_bridge and wait for the next dsi_host->attach() > event. I guess you assumes that dsi-host knows all devs connected to it, thanks to: - subnodes of the host - ie. devices controlled via dsi bus, - graph links from host ports/endpoints - ie. devices controlled by other buses, for example adv7533. I would separate both abstractions to make it more clear: 1. MIPI bus should be registered early - to allow create/bind devices on it, 2. drm_bridge should be registered only if all required sinks (bridges/panels) are registered. First point seems OK, I am not sure about the 2nd one - if used consistently, it would require building pipeline from sink to source. By the way is there any pipeline with two consecutive external bridges in the mainline? Regards Andrzej
On Thu, 22 Jun 2017 14:29:07 +0200 Andrzej Hajda <a.hajda@samsung.com> wrote: > On 22.06.2017 11:23, Boris Brezillon wrote: > > On Thu, 22 Jun 2017 13:47:43 +0530 > > Archit Taneja <architt@codeaurora.org> wrote: > > > >> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: > >>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: > >>>> Archit Taneja <architt@codeaurora.org> writes: > >>>> > >>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote: > >>>>>> Archit Taneja <architt@codeaurora.org> writes: > >>>>>> > >>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: > >>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), > >>>>>>>> then the connector's state would never get initialized, and we'd > >>>>>>>> dereference the NULL in the hotplug path. We also need to register > >>>>>>>> the connector, so that userspace can get at it. > >>>>>>>> > >>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before > >>>>>>> drm_mode_config_reset? Is it the case when we're inserting the > >>>>>>> panel-bridge driver as a module? > >>>>>>> > >>>>>>> > >>>>>>> All the connectors that have been added are registered automatically > >>>>>>> when drm_dev_register() is called by the KMS driver. Registering a > >>>>>>> connector in the middle of setting up our driver is prone to race > >>>>>>> conditions if the userspace decides to use them immediately. > >>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, > >>>>>> which in the case of a panel module that creates the DSI device > >>>>>> (adv7533-style, like you said I should use as a reference) will be after > >>>>>> drm_mode_config_reset() and drm_dev_register(). > >>>>> Okay. In the case of the msm kms driver, we defer probe until the > >>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() > >>>>> and drm_dev_register(). I assumed this was the general practice followed by > >>>>> most kms drivers. I.,e the kms driver defers probe until all connector > >>>>> related modules are inserted, and only then proceed to create a drm device. > >>>> The problem, though, is the panel driver needs the MIPI DSI host to > >>>> exist to call mipi_dsi_device_register_full() during the probe process. > >>>> The adv7533 driver gets around this by registering the DSI device in the > >>>> bridge attach step, but drm_panel doesn't have an attach step. > >> I'm not sure how we can get around this. We had discussion about this on irc > >> recently, but couldn't come up with a good conclusion. We could come up with a > >> panel_attach() callback to make it similar to bridges, but that's just us avoiding > >> the real issue. > > How about making DSI dev registration fully asynchronous, that is, DSI > > devs declared in the DT under the DSI host node will be > > registered/attached at probe time, and devs using another control bus > > (like the adv7533 controller over i2c) will be registered afterwards. > > > > That implies moving the drm_brige registration logic outside of the DSI > > host ->probe() path. The idea would be to check if all devs connected > > to the DSI bus are ready at dsi_host->attach() time. If they are, we > > can finally register the XXX -> DSI bridge. If they're not (because > > some devs connected to the DSI bus have not been probed yet), then we > > do not register the drm_bridge and wait for the next dsi_host->attach() > > event. > > I guess you assumes that dsi-host knows all devs connected to it, thanks to: > - subnodes of the host - ie. devices controlled via dsi bus, > - graph links from host ports/endpoints - ie. devices controlled by > other buses, for example adv7533. Yep, but I think that's already a requirement when populating devices with the OF graph method (if one of the DSI output endpoint does not have a drm_bridge/panel attached to it, the DSI host driver returns -EPROBE_DEFER). > > I would separate both abstractions to make it more clear: > 1. MIPI bus should be registered early - to allow create/bind devices on it, Exactly. > 2. drm_bridge should be registered only if all required sinks > (bridges/panels) are registered. That's true, until we find a solution to support add DRM bridge hotplug. > > First point seems OK, I am not sure about the 2nd one - if used > consistently, it would require building pipeline from sink to source. Yes. > By the way is there any pipeline with two consecutive external bridges > in the mainline? I don't know if it exists in mainline, but I had to do that on my FPGA platform when developing/testing Cadence DSI host driver. I had the following chain and it worked just fine: CRTC -> DPI encoder -> DPI to DSI bridge -> DSI to DPI bridge -> DPI to HDMI bridge (adv7511) -> HDMI connector
On 22.06.2017 14:41, Boris Brezillon wrote: > On Thu, 22 Jun 2017 14:29:07 +0200 > Andrzej Hajda <a.hajda@samsung.com> wrote: > >> On 22.06.2017 11:23, Boris Brezillon wrote: >>> On Thu, 22 Jun 2017 13:47:43 +0530 >>> Archit Taneja <architt@codeaurora.org> wrote: >>> >>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: >>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: >>>>>> Archit Taneja <architt@codeaurora.org> writes: >>>>>> >>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote: >>>>>>>> Archit Taneja <architt@codeaurora.org> writes: >>>>>>>> >>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: >>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), >>>>>>>>>> then the connector's state would never get initialized, and we'd >>>>>>>>>> dereference the NULL in the hotplug path. We also need to register >>>>>>>>>> the connector, so that userspace can get at it. >>>>>>>>>> >>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before >>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the >>>>>>>>> panel-bridge driver as a module? >>>>>>>>> >>>>>>>>> >>>>>>>>> All the connectors that have been added are registered automatically >>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a >>>>>>>>> connector in the middle of setting up our driver is prone to race >>>>>>>>> conditions if the userspace decides to use them immediately. >>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, >>>>>>>> which in the case of a panel module that creates the DSI device >>>>>>>> (adv7533-style, like you said I should use as a reference) will be after >>>>>>>> drm_mode_config_reset() and drm_dev_register(). >>>>>>> Okay. In the case of the msm kms driver, we defer probe until the >>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() >>>>>>> and drm_dev_register(). I assumed this was the general practice followed by >>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector >>>>>>> related modules are inserted, and only then proceed to create a drm device. >>>>>> The problem, though, is the panel driver needs the MIPI DSI host to >>>>>> exist to call mipi_dsi_device_register_full() during the probe process. >>>>>> The adv7533 driver gets around this by registering the DSI device in the >>>>>> bridge attach step, but drm_panel doesn't have an attach step. >>>> I'm not sure how we can get around this. We had discussion about this on irc >>>> recently, but couldn't come up with a good conclusion. We could come up with a >>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding >>>> the real issue. >>> How about making DSI dev registration fully asynchronous, that is, DSI >>> devs declared in the DT under the DSI host node will be >>> registered/attached at probe time, and devs using another control bus >>> (like the adv7533 controller over i2c) will be registered afterwards. >>> >>> That implies moving the drm_brige registration logic outside of the DSI >>> host ->probe() path. The idea would be to check if all devs connected >>> to the DSI bus are ready at dsi_host->attach() time. If they are, we >>> can finally register the XXX -> DSI bridge. If they're not (because >>> some devs connected to the DSI bus have not been probed yet), then we >>> do not register the drm_bridge and wait for the next dsi_host->attach() >>> event. >> I guess you assumes that dsi-host knows all devs connected to it, thanks to: >> - subnodes of the host - ie. devices controlled via dsi bus, >> - graph links from host ports/endpoints - ie. devices controlled by >> other buses, for example adv7533. > Yep, but I think that's already a requirement when populating devices > with the OF graph method (if one of the DSI output endpoint does not > have a drm_bridge/panel attached to it, the DSI host driver returns > -EPROBE_DEFER). > >> I would separate both abstractions to make it more clear: >> 1. MIPI bus should be registered early - to allow create/bind devices on it, > Exactly. > >> 2. drm_bridge should be registered only if all required sinks >> (bridges/panels) are registered. > That's true, until we find a solution to support add DRM bridge hotplug. > >> First point seems OK, I am not sure about the 2nd one - if used >> consistently, it would require building pipeline from sink to source. > Yes. Since drm_bridge_attach requires encoder to be not null pipeline creation would be painful: 1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before registering bridge and cache the result for later use. 2. After encoder finds directly connected bridge, it can attach it. 3. attach callback of every bridge should attach subsequent bridge. Quite complicated, maybe bridges should be chained w/o available encoder, and later attached to encoder with other helper, for example drm_bridge_chain_attach. Regards Andrzej > >> By the way is there any pipeline with two consecutive external bridges >> in the mainline? > I don't know if it exists in mainline, but I had to do that on my FPGA > platform when developing/testing Cadence DSI host driver. I had the > following chain and it worked just fine: > > CRTC -> DPI encoder -> DPI to DSI bridge -> DSI to DPI bridge -> DPI to HDMI bridge (adv7511) -> HDMI connector > > > >
On Thu, 22 Jun 2017 15:16:47 +0200 Andrzej Hajda <a.hajda@samsung.com> wrote: > On 22.06.2017 14:41, Boris Brezillon wrote: > > On Thu, 22 Jun 2017 14:29:07 +0200 > > Andrzej Hajda <a.hajda@samsung.com> wrote: > > > >> On 22.06.2017 11:23, Boris Brezillon wrote: > >>> On Thu, 22 Jun 2017 13:47:43 +0530 > >>> Archit Taneja <architt@codeaurora.org> wrote: > >>> > >>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: > >>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: > >>>>>> Archit Taneja <architt@codeaurora.org> writes: > >>>>>> > >>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote: > >>>>>>>> Archit Taneja <architt@codeaurora.org> writes: > >>>>>>>> > >>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: > >>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), > >>>>>>>>>> then the connector's state would never get initialized, and we'd > >>>>>>>>>> dereference the NULL in the hotplug path. We also need to register > >>>>>>>>>> the connector, so that userspace can get at it. > >>>>>>>>>> > >>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before > >>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the > >>>>>>>>> panel-bridge driver as a module? > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> All the connectors that have been added are registered automatically > >>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a > >>>>>>>>> connector in the middle of setting up our driver is prone to race > >>>>>>>>> conditions if the userspace decides to use them immediately. > >>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, > >>>>>>>> which in the case of a panel module that creates the DSI device > >>>>>>>> (adv7533-style, like you said I should use as a reference) will be after > >>>>>>>> drm_mode_config_reset() and drm_dev_register(). > >>>>>>> Okay. In the case of the msm kms driver, we defer probe until the > >>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() > >>>>>>> and drm_dev_register(). I assumed this was the general practice followed by > >>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector > >>>>>>> related modules are inserted, and only then proceed to create a drm device. > >>>>>> The problem, though, is the panel driver needs the MIPI DSI host to > >>>>>> exist to call mipi_dsi_device_register_full() during the probe process. > >>>>>> The adv7533 driver gets around this by registering the DSI device in the > >>>>>> bridge attach step, but drm_panel doesn't have an attach step. > >>>> I'm not sure how we can get around this. We had discussion about this on irc > >>>> recently, but couldn't come up with a good conclusion. We could come up with a > >>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding > >>>> the real issue. > >>> How about making DSI dev registration fully asynchronous, that is, DSI > >>> devs declared in the DT under the DSI host node will be > >>> registered/attached at probe time, and devs using another control bus > >>> (like the adv7533 controller over i2c) will be registered afterwards. > >>> > >>> That implies moving the drm_brige registration logic outside of the DSI > >>> host ->probe() path. The idea would be to check if all devs connected > >>> to the DSI bus are ready at dsi_host->attach() time. If they are, we > >>> can finally register the XXX -> DSI bridge. If they're not (because > >>> some devs connected to the DSI bus have not been probed yet), then we > >>> do not register the drm_bridge and wait for the next dsi_host->attach() > >>> event. > >> I guess you assumes that dsi-host knows all devs connected to it, thanks to: > >> - subnodes of the host - ie. devices controlled via dsi bus, > >> - graph links from host ports/endpoints - ie. devices controlled by > >> other buses, for example adv7533. > > Yep, but I think that's already a requirement when populating devices > > with the OF graph method (if one of the DSI output endpoint does not > > have a drm_bridge/panel attached to it, the DSI host driver returns > > -EPROBE_DEFER). > > > >> I would separate both abstractions to make it more clear: > >> 1. MIPI bus should be registered early - to allow create/bind devices on it, > > Exactly. > > > >> 2. drm_bridge should be registered only if all required sinks > >> (bridges/panels) are registered. > > That's true, until we find a solution to support add DRM bridge hotplug. > > > >> First point seems OK, I am not sure about the 2nd one - if used > >> consistently, it would require building pipeline from sink to source. > > Yes. > > Since drm_bridge_attach requires encoder to be not null pipeline > creation would be painful: > 1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before > registering bridge and cache the result for later use. Shouldn't be hard to do since dsi_host->attach() is called each time a sink is added (and ready to use). All you need to do is retrieve the bridge pointer and put it in a list embedded in the DSI host priv struct. Once you have all sinks added to this list (can be checked by counting the number of endpoints and DSI devs at probe time), you can register the DSI bridge and wait for someone to call ->attach() on it. In the ->attach() hook of the DSI bridge, you'll have to attach all sinks stored in the list to the DSI bridge. Note that right now you have a 1:1 relationship, which prevents you from having one DSI bridge that can attach to different bridges available on the DSI bus (e.g. DSI -> HDMI bridge on channel 0 and DSI -> LVDS bridge on channel 1). > 2. After encoder finds directly connected bridge, it can attach it. I don't get that one. > 3. attach callback of every bridge should attach subsequent bridge. Yep. > > Quite complicated, Not sure it's more complicated than what we have right now. > maybe bridges should be chained w/o available > encoder, and later attached to encoder with other helper, for example > drm_bridge_chain_attach. That's also a solution.
On 22.06.2017 15:34, Boris Brezillon wrote: > On Thu, 22 Jun 2017 15:16:47 +0200 > Andrzej Hajda <a.hajda@samsung.com> wrote: > >> On 22.06.2017 14:41, Boris Brezillon wrote: >>> On Thu, 22 Jun 2017 14:29:07 +0200 >>> Andrzej Hajda <a.hajda@samsung.com> wrote: >>> >>>> On 22.06.2017 11:23, Boris Brezillon wrote: >>>>> On Thu, 22 Jun 2017 13:47:43 +0530 >>>>> Archit Taneja <architt@codeaurora.org> wrote: >>>>> >>>>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: >>>>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: >>>>>>>> Archit Taneja <architt@codeaurora.org> writes: >>>>>>>> >>>>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote: >>>>>>>>>> Archit Taneja <architt@codeaurora.org> writes: >>>>>>>>>> >>>>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: >>>>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), >>>>>>>>>>>> then the connector's state would never get initialized, and we'd >>>>>>>>>>>> dereference the NULL in the hotplug path. We also need to register >>>>>>>>>>>> the connector, so that userspace can get at it. >>>>>>>>>>>> >>>>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before >>>>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the >>>>>>>>>>> panel-bridge driver as a module? >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> All the connectors that have been added are registered automatically >>>>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a >>>>>>>>>>> connector in the middle of setting up our driver is prone to race >>>>>>>>>>> conditions if the userspace decides to use them immediately. >>>>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, >>>>>>>>>> which in the case of a panel module that creates the DSI device >>>>>>>>>> (adv7533-style, like you said I should use as a reference) will be after >>>>>>>>>> drm_mode_config_reset() and drm_dev_register(). >>>>>>>>> Okay. In the case of the msm kms driver, we defer probe until the >>>>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() >>>>>>>>> and drm_dev_register(). I assumed this was the general practice followed by >>>>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector >>>>>>>>> related modules are inserted, and only then proceed to create a drm device. >>>>>>>> The problem, though, is the panel driver needs the MIPI DSI host to >>>>>>>> exist to call mipi_dsi_device_register_full() during the probe process. >>>>>>>> The adv7533 driver gets around this by registering the DSI device in the >>>>>>>> bridge attach step, but drm_panel doesn't have an attach step. >>>>>> I'm not sure how we can get around this. We had discussion about this on irc >>>>>> recently, but couldn't come up with a good conclusion. We could come up with a >>>>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding >>>>>> the real issue. >>>>> How about making DSI dev registration fully asynchronous, that is, DSI >>>>> devs declared in the DT under the DSI host node will be >>>>> registered/attached at probe time, and devs using another control bus >>>>> (like the adv7533 controller over i2c) will be registered afterwards. >>>>> >>>>> That implies moving the drm_brige registration logic outside of the DSI >>>>> host ->probe() path. The idea would be to check if all devs connected >>>>> to the DSI bus are ready at dsi_host->attach() time. If they are, we >>>>> can finally register the XXX -> DSI bridge. If they're not (because >>>>> some devs connected to the DSI bus have not been probed yet), then we >>>>> do not register the drm_bridge and wait for the next dsi_host->attach() >>>>> event. >>>> I guess you assumes that dsi-host knows all devs connected to it, thanks to: >>>> - subnodes of the host - ie. devices controlled via dsi bus, >>>> - graph links from host ports/endpoints - ie. devices controlled by >>>> other buses, for example adv7533. >>> Yep, but I think that's already a requirement when populating devices >>> with the OF graph method (if one of the DSI output endpoint does not >>> have a drm_bridge/panel attached to it, the DSI host driver returns >>> -EPROBE_DEFER). >>> >>>> I would separate both abstractions to make it more clear: >>>> 1. MIPI bus should be registered early - to allow create/bind devices on it, >>> Exactly. >>> >>>> 2. drm_bridge should be registered only if all required sinks >>>> (bridges/panels) are registered. >>> That's true, until we find a solution to support add DRM bridge hotplug. >>> >>>> First point seems OK, I am not sure about the 2nd one - if used >>>> consistently, it would require building pipeline from sink to source. >>> Yes. >> Since drm_bridge_attach requires encoder to be not null pipeline >> creation would be painful: >> 1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before >> registering bridge and cache the result for later use. > Shouldn't be hard to do since dsi_host->attach() is called each time a > sink is added (and ready to use). All you need to do is retrieve the > bridge pointer and put it in a list embedded in the DSI host priv > struct. Once you have all sinks added to this list (can be checked by > counting the number of endpoints and DSI devs at probe time), you can > register the DSI bridge and wait for someone to call ->attach() on it. > > In the ->attach() hook of the DSI bridge, you'll have to attach all > sinks stored in the list to the DSI bridge. Note that right now you have > a 1:1 relationship, which prevents you from having one DSI bridge that > can attach to different bridges available on the DSI bus (e.g. DSI -> > HDMI bridge on channel 0 and DSI -> LVDS bridge on channel 1). > >> 2. After encoder finds directly connected bridge, it can attach it. > I don't get that one. If you have pipeline: crtc -> encoder -> bridge1 -> bridge2 -> panel encoder knows only about bridge1, and must wait till it is registered, before attaching it, and assuming bridge must wait for its sinks before registration the whole pipeline construction will look like: 0. encoder waits for bridge1, bridge1 waits for bridge2, bridge2 waits for panel, usually by deferring. 1. panel is registered. 2. bridge2 finds panel and is registered. 3. bridge1 finds bridge2 and is registered. 4. encoder finds bridge1 and attach it to encoder, 4a. bridge1->attach() attach bridge2 to encoder after bridge1 4b. bridge2->attach() attach panel to bridge2 This is why it seems for me quite complicated. Regards Andrzej
On Fri, 23 Jun 2017 09:22:15 +0200 Andrzej Hajda <a.hajda@samsung.com> wrote: > On 22.06.2017 15:34, Boris Brezillon wrote: > > On Thu, 22 Jun 2017 15:16:47 +0200 > > Andrzej Hajda <a.hajda@samsung.com> wrote: > > > >> On 22.06.2017 14:41, Boris Brezillon wrote: > >>> On Thu, 22 Jun 2017 14:29:07 +0200 > >>> Andrzej Hajda <a.hajda@samsung.com> wrote: > >>> > >>>> On 22.06.2017 11:23, Boris Brezillon wrote: > >>>>> On Thu, 22 Jun 2017 13:47:43 +0530 > >>>>> Archit Taneja <architt@codeaurora.org> wrote: > >>>>> > >>>>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: > >>>>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: > >>>>>>>> Archit Taneja <architt@codeaurora.org> writes: > >>>>>>>> > >>>>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote: > >>>>>>>>>> Archit Taneja <architt@codeaurora.org> writes: > >>>>>>>>>> > >>>>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: > >>>>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), > >>>>>>>>>>>> then the connector's state would never get initialized, and we'd > >>>>>>>>>>>> dereference the NULL in the hotplug path. We also need to register > >>>>>>>>>>>> the connector, so that userspace can get at it. > >>>>>>>>>>>> > >>>>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before > >>>>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the > >>>>>>>>>>> panel-bridge driver as a module? > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> All the connectors that have been added are registered automatically > >>>>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a > >>>>>>>>>>> connector in the middle of setting up our driver is prone to race > >>>>>>>>>>> conditions if the userspace decides to use them immediately. > >>>>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, > >>>>>>>>>> which in the case of a panel module that creates the DSI device > >>>>>>>>>> (adv7533-style, like you said I should use as a reference) will be after > >>>>>>>>>> drm_mode_config_reset() and drm_dev_register(). > >>>>>>>>> Okay. In the case of the msm kms driver, we defer probe until the > >>>>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() > >>>>>>>>> and drm_dev_register(). I assumed this was the general practice followed by > >>>>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector > >>>>>>>>> related modules are inserted, and only then proceed to create a drm device. > >>>>>>>> The problem, though, is the panel driver needs the MIPI DSI host to > >>>>>>>> exist to call mipi_dsi_device_register_full() during the probe process. > >>>>>>>> The adv7533 driver gets around this by registering the DSI device in the > >>>>>>>> bridge attach step, but drm_panel doesn't have an attach step. > >>>>>> I'm not sure how we can get around this. We had discussion about this on irc > >>>>>> recently, but couldn't come up with a good conclusion. We could come up with a > >>>>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding > >>>>>> the real issue. > >>>>> How about making DSI dev registration fully asynchronous, that is, DSI > >>>>> devs declared in the DT under the DSI host node will be > >>>>> registered/attached at probe time, and devs using another control bus > >>>>> (like the adv7533 controller over i2c) will be registered afterwards. > >>>>> > >>>>> That implies moving the drm_brige registration logic outside of the DSI > >>>>> host ->probe() path. The idea would be to check if all devs connected > >>>>> to the DSI bus are ready at dsi_host->attach() time. If they are, we > >>>>> can finally register the XXX -> DSI bridge. If they're not (because > >>>>> some devs connected to the DSI bus have not been probed yet), then we > >>>>> do not register the drm_bridge and wait for the next dsi_host->attach() > >>>>> event. > >>>> I guess you assumes that dsi-host knows all devs connected to it, thanks to: > >>>> - subnodes of the host - ie. devices controlled via dsi bus, > >>>> - graph links from host ports/endpoints - ie. devices controlled by > >>>> other buses, for example adv7533. > >>> Yep, but I think that's already a requirement when populating devices > >>> with the OF graph method (if one of the DSI output endpoint does not > >>> have a drm_bridge/panel attached to it, the DSI host driver returns > >>> -EPROBE_DEFER). > >>> > >>>> I would separate both abstractions to make it more clear: > >>>> 1. MIPI bus should be registered early - to allow create/bind devices on it, > >>> Exactly. > >>> > >>>> 2. drm_bridge should be registered only if all required sinks > >>>> (bridges/panels) are registered. > >>> That's true, until we find a solution to support add DRM bridge hotplug. > >>> > >>>> First point seems OK, I am not sure about the 2nd one - if used > >>>> consistently, it would require building pipeline from sink to source. > >>> Yes. > >> Since drm_bridge_attach requires encoder to be not null pipeline > >> creation would be painful: > >> 1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before > >> registering bridge and cache the result for later use. > > Shouldn't be hard to do since dsi_host->attach() is called each time a > > sink is added (and ready to use). All you need to do is retrieve the > > bridge pointer and put it in a list embedded in the DSI host priv > > struct. Once you have all sinks added to this list (can be checked by > > counting the number of endpoints and DSI devs at probe time), you can > > register the DSI bridge and wait for someone to call ->attach() on it. > > > > In the ->attach() hook of the DSI bridge, you'll have to attach all > > sinks stored in the list to the DSI bridge. Note that right now you have > > a 1:1 relationship, which prevents you from having one DSI bridge that > > can attach to different bridges available on the DSI bus (e.g. DSI -> > > HDMI bridge on channel 0 and DSI -> LVDS bridge on channel 1). > > > >> 2. After encoder finds directly connected bridge, it can attach it. > > I don't get that one. > > If you have pipeline: > > crtc -> encoder -> bridge1 -> bridge2 -> panel > > encoder knows only about bridge1, and must wait till it is registered, > before attaching it, > and assuming bridge must wait for its sinks before registration the > whole pipeline construction will look like: > > 0. encoder waits for bridge1, bridge1 waits for bridge2, bridge2 waits > for panel, usually by deferring. > 1. panel is registered. > 2. bridge2 finds panel and is registered. > 3. bridge1 finds bridge2 and is registered. > 4. encoder finds bridge1 and attach it to encoder, > 4a. bridge1->attach() attach bridge2 to encoder after bridge1 > 4b. bridge2->attach() attach panel to bridge2 > > This is why it seems for me quite complicated. But that's already what happens in most drivers today (probably because things were designed before connector hotplug was supported). I agree, it's far from ideal, but until we get full-hotplug support, we'll have to rely on such hacks.
On Thu, Jun 15, 2017 at 01:41:24PM -0700, Eric Anholt wrote: > If the panel-bridge is being set up after the drm_mode_config_reset(), > then the connector's state would never get initialized, and we'd > dereference the NULL in the hotplug path. We also need to register > the connector, so that userspace can get at it. > > Signed-off-by: Eric Anholt <eric@anholt.net> I've not read all the details of the discussion here, just a comment on what this implies if we go with this. > --- > drivers/gpu/drm/bridge/panel.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index 67fe19e5a9c6..8ed8a70799c7 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -82,11 +82,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge) > > drm_mode_connector_attach_encoder(&panel_bridge->connector, > bridge->encoder); > + drm_atomic_helper_connector_reset(&panel_bridge->connector); I'm not sure we want to tie the ->reset stuff in with other helpers. One of the design goals I had with all things atomic was to make helpers much more modular, and imo the reset is one such piece - a driver might want to instead reconstruct state from hw using something like i915's hw state readout/compare/verify framework. If we can solve this init ordering issue without adding a depency on the reset stuff that would be a bit neater imo. But that's just my 2 cents. Cheers, Daniel > > ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); > if (ret < 0) > return ret; > > + drm_connector_register(&panel_bridge->connector); > + > return 0; > } > > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 6/22/2017 7:04 PM, Boris Brezillon wrote: > On Thu, 22 Jun 2017 15:16:47 +0200 > Andrzej Hajda <a.hajda@samsung.com> wrote: > >> On 22.06.2017 14:41, Boris Brezillon wrote: >>> On Thu, 22 Jun 2017 14:29:07 +0200 >>> Andrzej Hajda <a.hajda@samsung.com> wrote: >>> >>>> On 22.06.2017 11:23, Boris Brezillon wrote: >>>>> On Thu, 22 Jun 2017 13:47:43 +0530 >>>>> Archit Taneja <architt@codeaurora.org> wrote: >>>>> >>>>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: >>>>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: >>>>>>>> Archit Taneja <architt@codeaurora.org> writes: >>>>>>>> >>>>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote: >>>>>>>>>> Archit Taneja <architt@codeaurora.org> writes: >>>>>>>>>> >>>>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: >>>>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), >>>>>>>>>>>> then the connector's state would never get initialized, and we'd >>>>>>>>>>>> dereference the NULL in the hotplug path. We also need to register >>>>>>>>>>>> the connector, so that userspace can get at it. >>>>>>>>>>>> >>>>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before >>>>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the >>>>>>>>>>> panel-bridge driver as a module? >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> All the connectors that have been added are registered automatically >>>>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a >>>>>>>>>>> connector in the middle of setting up our driver is prone to race >>>>>>>>>>> conditions if the userspace decides to use them immediately. >>>>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, >>>>>>>>>> which in the case of a panel module that creates the DSI device >>>>>>>>>> (adv7533-style, like you said I should use as a reference) will be after >>>>>>>>>> drm_mode_config_reset() and drm_dev_register(). >>>>>>>>> Okay. In the case of the msm kms driver, we defer probe until the >>>>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() >>>>>>>>> and drm_dev_register(). I assumed this was the general practice followed by >>>>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector >>>>>>>>> related modules are inserted, and only then proceed to create a drm device. >>>>>>>> The problem, though, is the panel driver needs the MIPI DSI host to >>>>>>>> exist to call mipi_dsi_device_register_full() during the probe process. >>>>>>>> The adv7533 driver gets around this by registering the DSI device in the >>>>>>>> bridge attach step, but drm_panel doesn't have an attach step. >>>>>> I'm not sure how we can get around this. We had discussion about this on irc >>>>>> recently, but couldn't come up with a good conclusion. We could come up with a >>>>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding >>>>>> the real issue. >>>>> How about making DSI dev registration fully asynchronous, that is, DSI >>>>> devs declared in the DT under the DSI host node will be >>>>> registered/attached at probe time, and devs using another control bus >>>>> (like the adv7533 controller over i2c) will be registered afterwards. >>>>> >>>>> That implies moving the drm_brige registration logic outside of the DSI >>>>> host ->probe() path. The idea would be to check if all devs connected >>>>> to the DSI bus are ready at dsi_host->attach() time. If they are, we >>>>> can finally register the XXX -> DSI bridge. If they're not (because >>>>> some devs connected to the DSI bus have not been probed yet), then we >>>>> do not register the drm_bridge and wait for the next dsi_host->attach() >>>>> event. >>>> I guess you assumes that dsi-host knows all devs connected to it, thanks to: >>>> - subnodes of the host - ie. devices controlled via dsi bus, >>>> - graph links from host ports/endpoints - ie. devices controlled by >>>> other buses, for example adv7533. >>> Yep, but I think that's already a requirement when populating devices >>> with the OF graph method (if one of the DSI output endpoint does not >>> have a drm_bridge/panel attached to it, the DSI host driver returns >>> -EPROBE_DEFER). >>> >>>> I would separate both abstractions to make it more clear: >>>> 1. MIPI bus should be registered early - to allow create/bind devices on it, >>> Exactly. >>> >>>> 2. drm_bridge should be registered only if all required sinks >>>> (bridges/panels) are registered. >>> That's true, until we find a solution to support add DRM bridge hotplug. >>> >>>> First point seems OK, I am not sure about the 2nd one - if used >>>> consistently, it would require building pipeline from sink to source. >>> Yes. >> >> Since drm_bridge_attach requires encoder to be not null pipeline >> creation would be painful: >> 1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before >> registering bridge and cache the result for later use. > > Shouldn't be hard to do since dsi_host->attach() is called each time a > sink is added (and ready to use). All you need to do is retrieve the > bridge pointer and put it in a list embedded in the DSI host priv > struct. Once you have all sinks added to this list (can be checked by > counting the number of endpoints and DSI devs at probe time), you can > register the DSI bridge and wait for someone to call ->attach() on it. Some of the kms drivers (including vc4) currently do the bridge registration, and call drm_bridge_attach() in the DSI host driver's probe path itself. I guess some of them would require some restructuring to work with the above approach. Other kms drivers (msm for example) already have a separate modeset_init path where different drm objects (crtcs, encoders, bridges) are created. That's the one that can defer rather than defer-ing the DSI host driver. We'd want people be willing to convert their kms drivers to go ahead with the approach. Thanks for continuing the discussion, it was quite informative. Regards, Archit > > In the ->attach() hook of the DSI bridge, you'll have to attach all > sinks stored in the list to the DSI bridge. Note that right now you have > a 1:1 relationship, which prevents you from having one DSI bridge that > can attach to different bridges available on the DSI bus (e.g. DSI -> > HDMI bridge on channel 0 and DSI -> LVDS bridge on channel 1). > >> 2. After encoder finds directly connected bridge, it can attach it. > > I don't get that one. > >> 3. attach callback of every bridge should attach subsequent bridge. > > Yep. > >> >> Quite complicated, > > Not sure it's more complicated than what we have right now. > >> maybe bridges should be chained w/o available >> encoder, and later attached to encoder with other helper, for example >> drm_bridge_chain_attach. > > That's also a solution. >
Archit Taneja <architt@codeaurora.org> writes: > On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: >> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: >>> Archit Taneja <architt@codeaurora.org> writes: >>> >>>> On 06/16/2017 08:13 PM, Eric Anholt wrote: >>>>> Archit Taneja <architt@codeaurora.org> writes: >>>>> >>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: >>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), >>>>>>> then the connector's state would never get initialized, and we'd >>>>>>> dereference the NULL in the hotplug path. We also need to register >>>>>>> the connector, so that userspace can get at it. >>>>>>> >>>>>> >>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before >>>>>> drm_mode_config_reset? Is it the case when we're inserting the >>>>>> panel-bridge driver as a module? >>>>>> >>>>>> >>>>>> All the connectors that have been added are registered automatically >>>>>> when drm_dev_register() is called by the KMS driver. Registering a >>>>>> connector in the middle of setting up our driver is prone to race >>>>>> conditions if the userspace decides to use them immediately. >>>>> >>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, >>>>> which in the case of a panel module that creates the DSI device >>>>> (adv7533-style, like you said I should use as a reference) will be after >>>>> drm_mode_config_reset() and drm_dev_register(). >>>> >>>> Okay. In the case of the msm kms driver, we defer probe until the >>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() >>>> and drm_dev_register(). I assumed this was the general practice followed by >>>> most kms drivers. I.,e the kms driver defers probe until all connector >>>> related modules are inserted, and only then proceed to create a drm device. >>> >>> The problem, though, is the panel driver needs the MIPI DSI host to >>> exist to call mipi_dsi_device_register_full() during the probe process. >>> The adv7533 driver gets around this by registering the DSI device in the >>> bridge attach step, but drm_panel doesn't have an attach step. > > I'm not sure how we can get around this. We had discussion about this on irc > recently, but couldn't come up with a good conclusion. We could come up with a > panel_attach() callback to make it similar to bridges, but that's just us avoiding > the real issue. > >>> >>> Another alternative is my original version of the panel driver that was >>> a mipi_dsi_device driver that registered the panel during the DSI device >>> probe. That's why vc4's panel lookup is during the MIPI DSI attach >>> phase, currently. >> > > This would require you to have a DSI device node in DT, rather than an i2c > node, right? I don't know if we should do that because of a limitation in > our drm_mipi_dsi and drm_panel frameworks. All versions of this patch have had one of those, because either that's where you attach the driver (the first, no-core-changes version of the driver) or you need it for the of-graph connections anyway. These later versions just haven't had a compatible string on the DSI device node.
Boris Brezillon <boris.brezillon@free-electrons.com> writes: > On Thu, 22 Jun 2017 13:47:43 +0530 > Archit Taneja <architt@codeaurora.org> wrote: > >> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: >> > 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: >> >> Archit Taneja <architt@codeaurora.org> writes: >> >> >> >>> On 06/16/2017 08:13 PM, Eric Anholt wrote: >> >>>> Archit Taneja <architt@codeaurora.org> writes: >> >>>> >> >>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: >> >>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), >> >>>>>> then the connector's state would never get initialized, and we'd >> >>>>>> dereference the NULL in the hotplug path. We also need to register >> >>>>>> the connector, so that userspace can get at it. >> >>>>>> >> >>>>> >> >>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before >> >>>>> drm_mode_config_reset? Is it the case when we're inserting the >> >>>>> panel-bridge driver as a module? >> >>>>> >> >>>>> >> >>>>> All the connectors that have been added are registered automatically >> >>>>> when drm_dev_register() is called by the KMS driver. Registering a >> >>>>> connector in the middle of setting up our driver is prone to race >> >>>>> conditions if the userspace decides to use them immediately. >> >>>> >> >>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, >> >>>> which in the case of a panel module that creates the DSI device >> >>>> (adv7533-style, like you said I should use as a reference) will be after >> >>>> drm_mode_config_reset() and drm_dev_register(). >> >>> >> >>> Okay. In the case of the msm kms driver, we defer probe until the >> >>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() >> >>> and drm_dev_register(). I assumed this was the general practice followed by >> >>> most kms drivers. I.,e the kms driver defers probe until all connector >> >>> related modules are inserted, and only then proceed to create a drm device. >> >> >> >> The problem, though, is the panel driver needs the MIPI DSI host to >> >> exist to call mipi_dsi_device_register_full() during the probe process. >> >> The adv7533 driver gets around this by registering the DSI device in the >> >> bridge attach step, but drm_panel doesn't have an attach step. >> >> I'm not sure how we can get around this. We had discussion about this on irc >> recently, but couldn't come up with a good conclusion. We could come up with a >> panel_attach() callback to make it similar to bridges, but that's just us avoiding >> the real issue. > > How about making DSI dev registration fully asynchronous, that is, DSI > devs declared in the DT under the DSI host node will be > registered/attached at probe time, and devs using another control bus > (like the adv7533 controller over i2c) will be registered afterwards. > > That implies moving the drm_brige registration logic outside of the DSI > host ->probe() path. The idea would be to check if all devs connected > to the DSI bus are ready at dsi_host->attach() time. If they are, we > can finally register the XXX -> DSI bridge. If they're not (because > some devs connected to the DSI bus have not been probed yet), then we > do not register the drm_bridge and wait for the next dsi_host->attach() > event. > > With this solution, I think we can avoid the chicken&egg problem where > the adv7533 dev is waiting for the DSI host to be probed to be able to > register a DSI dev with mipi_dsi_device_register_full() and the DSI > host needs all devs to be registered to not return -EPROBE_DEFER. I've now tried having mipi_dsi_device_register_full() succeed early and just do the device-add part when the host shows up. The problem is there's still mipi_dsi_attach(), which needs to be delayed until the panel driver fills in the rest of mipi_dsi_device's fields. Why aren't those part of the info?
On 06/24/2017 03:20 AM, Eric Anholt wrote: > Boris Brezillon <boris.brezillon@free-electrons.com> writes: > >> On Thu, 22 Jun 2017 13:47:43 +0530 >> Archit Taneja <architt@codeaurora.org> wrote: >> >>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: >>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>: >>>>> Archit Taneja <architt@codeaurora.org> writes: >>>>> >>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote: >>>>>>> Archit Taneja <architt@codeaurora.org> writes: >>>>>>> >>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: >>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), >>>>>>>>> then the connector's state would never get initialized, and we'd >>>>>>>>> dereference the NULL in the hotplug path. We also need to register >>>>>>>>> the connector, so that userspace can get at it. >>>>>>>>> >>>>>>>> >>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before >>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the >>>>>>>> panel-bridge driver as a module? >>>>>>>> >>>>>>>> >>>>>>>> All the connectors that have been added are registered automatically >>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a >>>>>>>> connector in the middle of setting up our driver is prone to race >>>>>>>> conditions if the userspace decides to use them immediately. >>>>>>> >>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, >>>>>>> which in the case of a panel module that creates the DSI device >>>>>>> (adv7533-style, like you said I should use as a reference) will be after >>>>>>> drm_mode_config_reset() and drm_dev_register(). >>>>>> >>>>>> Okay. In the case of the msm kms driver, we defer probe until the >>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() >>>>>> and drm_dev_register(). I assumed this was the general practice followed by >>>>>> most kms drivers. I.,e the kms driver defers probe until all connector >>>>>> related modules are inserted, and only then proceed to create a drm device. >>>>> >>>>> The problem, though, is the panel driver needs the MIPI DSI host to >>>>> exist to call mipi_dsi_device_register_full() during the probe process. >>>>> The adv7533 driver gets around this by registering the DSI device in the >>>>> bridge attach step, but drm_panel doesn't have an attach step. >>> >>> I'm not sure how we can get around this. We had discussion about this on irc >>> recently, but couldn't come up with a good conclusion. We could come up with a >>> panel_attach() callback to make it similar to bridges, but that's just us avoiding >>> the real issue. >> >> How about making DSI dev registration fully asynchronous, that is, DSI >> devs declared in the DT under the DSI host node will be >> registered/attached at probe time, and devs using another control bus >> (like the adv7533 controller over i2c) will be registered afterwards. >> >> That implies moving the drm_brige registration logic outside of the DSI >> host ->probe() path. The idea would be to check if all devs connected >> to the DSI bus are ready at dsi_host->attach() time. If they are, we >> can finally register the XXX -> DSI bridge. If they're not (because >> some devs connected to the DSI bus have not been probed yet), then we >> do not register the drm_bridge and wait for the next dsi_host->attach() >> event. >> >> With this solution, I think we can avoid the chicken&egg problem where >> the adv7533 dev is waiting for the DSI host to be probed to be able to >> register a DSI dev with mipi_dsi_device_register_full() and the DSI >> host needs all devs to be registered to not return -EPROBE_DEFER. > > I've now tried having mipi_dsi_device_register_full() succeed early and > just do the device-add part when the host shows up. The problem is > there's still mipi_dsi_attach(), which needs to be delayed until the > panel driver fills in the rest of mipi_dsi_device's fields. Why aren't > those part of the info? In the past, the only way to create mipi_dsi_devices was to add them as children DT nodes under the DSI host node. The KMS driver calls mipi_dsi_host_register(), which resulted in the creation of all the children mipi_dsi_devices. The DSI host was still unaware of parameters like number of lanes, mode_flags etc. mipi_dsi_attach() was a way by which the DSI device could share these with its DSI host. Another use of mipi_dsi_attach()/detach() is if the panel driver wants to notify the DSI host an update in one of the device's fields (for example, a change in the number of lanes). We've recently added another way of creating mipi_dsi_devices (i.e, by making a driver call mipi_dsi_device_register_full) for cases where there isn't a DT representation of the DSI device. For the second method, the need to call mipi_dsi_attach() separately becomes sort of redundant. We could consider embedding the other mipi_dsi_device fields (lanes etc) in mipi_dsi_device_info itself, and invoke the DSI host's ->attach() callback through mipi_dsi_device_register_full() itself. Would that help resolving your issue? Archit
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 67fe19e5a9c6..8ed8a70799c7 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -82,11 +82,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge) drm_mode_connector_attach_encoder(&panel_bridge->connector, bridge->encoder); + drm_atomic_helper_connector_reset(&panel_bridge->connector); ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); if (ret < 0) return ret; + drm_connector_register(&panel_bridge->connector); + return 0; }
If the panel-bridge is being set up after the drm_mode_config_reset(), then the connector's state would never get initialized, and we'd dereference the NULL in the hotplug path. We also need to register the connector, so that userspace can get at it. Signed-off-by: Eric Anholt <eric@anholt.net> --- drivers/gpu/drm/bridge/panel.c | 3 +++ 1 file changed, 3 insertions(+)