| Message ID | 20190627151740.2277-1-matt.redfearn@thinci.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/bridge: adv7511: Attach to DSI host at probe time | expand |
On 27.06.2019 17:18, Matt Redfearn wrote: > In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel > which attach to the DSI host via mipi_dsi_attach() at probe time, the > ADV7533 bridge device does not. Instead it defers this to the point that > the upstream device connects to its bridge via drm_bridge_attach(). > The generic Synopsys MIPI DSI host driver does not register it's own > drm_bridge until the MIPI DSI has attached. But it does not call > drm_bridge_attach() on the downstream device until the upstream device > has attached. This leads to a chicken and the egg failure and the DRM > pipeline does not complete. > Since all other mipi_dsi_device drivers call mipi_dsi_attach() in > probe(), make the adv7533 mipi_dsi_device do the same. This ensures that > the Synopsys MIPI DSI host registers it's bridge such that it is > available for the upstream device to connect to. > > Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> Queued to drm-misc-next. Regards Andrzej > > --- > > Changes in v2: > Cleanup if adv7533_attach_dsi fails. > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index e7ddd3e3db9..807827bd910 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -874,9 +874,6 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge) > &adv7511_connector_helper_funcs); > drm_connector_attach_encoder(&adv->connector, bridge->encoder); > > - if (adv->type == ADV7533) > - ret = adv7533_attach_dsi(adv); > - > if (adv->i2c_main->irq) > regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0), > ADV7511_INT0_HPD); > @@ -1222,8 +1219,17 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > drm_bridge_add(&adv7511->bridge); > > adv7511_audio_init(dev, adv7511); > + > + if (adv7511->type == ADV7533) { > + ret = adv7533_attach_dsi(adv7511); > + if (ret) > + goto err_remove_bridge; > + } > + > return 0; > > +err_remove_bridge: > + drm_bridge_remove(&adv7511->bridge); > err_unregister_cec: > i2c_unregister_device(adv7511->i2c_cec); > if (adv7511->cec_clk)
On Thu, Jun 27, 2019 at 8:18 AM Matt Redfearn <matt.redfearn@thinci.com> wrote: > > In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel > which attach to the DSI host via mipi_dsi_attach() at probe time, the > ADV7533 bridge device does not. Instead it defers this to the point that > the upstream device connects to its bridge via drm_bridge_attach(). > The generic Synopsys MIPI DSI host driver does not register it's own > drm_bridge until the MIPI DSI has attached. But it does not call > drm_bridge_attach() on the downstream device until the upstream device > has attached. This leads to a chicken and the egg failure and the DRM > pipeline does not complete. > Since all other mipi_dsi_device drivers call mipi_dsi_attach() in > probe(), make the adv7533 mipi_dsi_device do the same. This ensures that > the Synopsys MIPI DSI host registers it's bridge such that it is > available for the upstream device to connect to. > > Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> > > --- > > Changes in v2: > Cleanup if adv7533_attach_dsi fails. > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index e7ddd3e3db9..807827bd910 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -874,9 +874,6 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge) > &adv7511_connector_helper_funcs); > drm_connector_attach_encoder(&adv->connector, bridge->encoder); > > - if (adv->type == ADV7533) > - ret = adv7533_attach_dsi(adv); > - > if (adv->i2c_main->irq) > regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0), > ADV7511_INT0_HPD); > @@ -1222,8 +1219,17 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > drm_bridge_add(&adv7511->bridge); > > adv7511_audio_init(dev, adv7511); > + > + if (adv7511->type == ADV7533) { > + ret = adv7533_attach_dsi(adv7511); > + if (ret) > + goto err_remove_bridge; > + } > + > return 0; > > +err_remove_bridge: > + drm_bridge_remove(&adv7511->bridge); > err_unregister_cec: > i2c_unregister_device(adv7511->i2c_cec); > if (adv7511->cec_clk) > -- As a heads up, I just did some testing on drm-misc-next and this patch seems to be breaking the HiKey board. On bootup, I'm seeing: [ 4.209615] adv7511 2-0039: 2-0039 supply avdd not found, using dummy regulator [ 4.217075] adv7511 2-0039: 2-0039 supply dvdd not found, using dummy regulator [ 4.224453] adv7511 2-0039: 2-0039 supply pvdd not found, using dummy regulator [ 4.231804] adv7511 2-0039: 2-0039 supply a2vdd not found, using dummy regulator [ 4.239242] adv7511 2-0039: 2-0039 supply v3p3 not found, using dummy regulator [ 4.246615] adv7511 2-0039: 2-0039 supply v1p2 not found, using dummy regulator [ 4.272970] adv7511 2-0039: failed to find dsi host over and over. The dummy regulator messages are normal, but usually [ 4.444315] kirin-drm f4100000.ade: bound f4107800.dsi (ops dsi_ops) Starts up right afterward. Reverting the change above seems to get things working again. I've not had much time to dig as to whats going wrong, but will keep looking and wanted to raise the issue in the meantime. thanks -john
On Mon, Aug 19, 2019 at 3:27 PM John Stultz <john.stultz@linaro.org> wrote: > On Thu, Jun 27, 2019 at 8:18 AM Matt Redfearn <matt.redfearn@thinci.com> wrote: > > > > In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel > > which attach to the DSI host via mipi_dsi_attach() at probe time, the > > ADV7533 bridge device does not. Instead it defers this to the point that > > the upstream device connects to its bridge via drm_bridge_attach(). > > The generic Synopsys MIPI DSI host driver does not register it's own > > drm_bridge until the MIPI DSI has attached. But it does not call > > drm_bridge_attach() on the downstream device until the upstream device > > has attached. This leads to a chicken and the egg failure and the DRM > > pipeline does not complete. > > Since all other mipi_dsi_device drivers call mipi_dsi_attach() in > > probe(), make the adv7533 mipi_dsi_device do the same. This ensures that > > the Synopsys MIPI DSI host registers it's bridge such that it is > > available for the upstream device to connect to. > > > > Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> > > > > --- > > > > Changes in v2: > > Cleanup if adv7533_attach_dsi fails. > > > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > index e7ddd3e3db9..807827bd910 100644 > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > @@ -874,9 +874,6 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge) > > &adv7511_connector_helper_funcs); > > drm_connector_attach_encoder(&adv->connector, bridge->encoder); > > > > - if (adv->type == ADV7533) > > - ret = adv7533_attach_dsi(adv); > > - > > if (adv->i2c_main->irq) > > regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0), > > ADV7511_INT0_HPD); > > @@ -1222,8 +1219,17 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > > drm_bridge_add(&adv7511->bridge); > > > > adv7511_audio_init(dev, adv7511); > > + > > + if (adv7511->type == ADV7533) { > > + ret = adv7533_attach_dsi(adv7511); > > + if (ret) > > + goto err_remove_bridge; > > + } > > + > > return 0; > > > > +err_remove_bridge: > > + drm_bridge_remove(&adv7511->bridge); > > err_unregister_cec: > > i2c_unregister_device(adv7511->i2c_cec); > > if (adv7511->cec_clk) > > -- > > As a heads up, I just did some testing on drm-misc-next and this patch > seems to be breaking the HiKey board. On bootup, I'm seeing: > [ 4.209615] adv7511 2-0039: 2-0039 supply avdd not found, using > dummy regulator > [ 4.217075] adv7511 2-0039: 2-0039 supply dvdd not found, using > dummy regulator > [ 4.224453] adv7511 2-0039: 2-0039 supply pvdd not found, using > dummy regulator > [ 4.231804] adv7511 2-0039: 2-0039 supply a2vdd not found, using > dummy regulator > [ 4.239242] adv7511 2-0039: 2-0039 supply v3p3 not found, using > dummy regulator > [ 4.246615] adv7511 2-0039: 2-0039 supply v1p2 not found, using > dummy regulator > [ 4.272970] adv7511 2-0039: failed to find dsi host > > over and over. I also just got a report that our testing is seeing this on the db410c board as well: https://bugs.linaro.org/show_bug.cgi?id=5345 It seems like the change is making it try to attach to the dsi too early before the dsi has registered? thanks -john
On Mon, Aug 19, 2019 at 3:27 PM John Stultz <john.stultz@linaro.org> wrote: > On Thu, Jun 27, 2019 at 8:18 AM Matt Redfearn <matt.redfearn@thinci.com> wrote: > > > > In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel > > which attach to the DSI host via mipi_dsi_attach() at probe time, the > > ADV7533 bridge device does not. Instead it defers this to the point that > > the upstream device connects to its bridge via drm_bridge_attach(). > > The generic Synopsys MIPI DSI host driver does not register it's own > > drm_bridge until the MIPI DSI has attached. But it does not call > > drm_bridge_attach() on the downstream device until the upstream device > > has attached. This leads to a chicken and the egg failure and the DRM > > pipeline does not complete. > > Since all other mipi_dsi_device drivers call mipi_dsi_attach() in > > probe(), make the adv7533 mipi_dsi_device do the same. This ensures that > > the Synopsys MIPI DSI host registers it's bridge such that it is > > available for the upstream device to connect to. > > > > Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> > > > > --- > > As a heads up, I just did some testing on drm-misc-next and this patch > seems to be breaking the HiKey board. On bootup, I'm seeing: > [ 4.209615] adv7511 2-0039: 2-0039 supply avdd not found, using > dummy regulator > [ 4.217075] adv7511 2-0039: 2-0039 supply dvdd not found, using > dummy regulator > [ 4.224453] adv7511 2-0039: 2-0039 supply pvdd not found, using > dummy regulator > [ 4.231804] adv7511 2-0039: 2-0039 supply a2vdd not found, using > dummy regulator > [ 4.239242] adv7511 2-0039: 2-0039 supply v3p3 not found, using > dummy regulator > [ 4.246615] adv7511 2-0039: 2-0039 supply v1p2 not found, using > dummy regulator > [ 4.272970] adv7511 2-0039: failed to find dsi host > > over and over. The dummy regulator messages are normal, but usually > [ 4.444315] kirin-drm f4100000.ade: bound f4107800.dsi (ops dsi_ops) > > Starts up right afterward. Hey Matt, Andrzej, I just wanted to follow up on this as this patch is breaking a couple of boards. Any sense of what might be missing, or is this something we should revert? I'm happy to test any patch ideas you have. thanks -john
On 27.08.2019 22:03, John Stultz wrote: > On Mon, Aug 19, 2019 at 3:27 PM John Stultz <john.stultz@linaro.org> wrote: >> On Thu, Jun 27, 2019 at 8:18 AM Matt Redfearn <matt.redfearn@thinci.com> wrote: >>> In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel >>> which attach to the DSI host via mipi_dsi_attach() at probe time, the >>> ADV7533 bridge device does not. Instead it defers this to the point that >>> the upstream device connects to its bridge via drm_bridge_attach(). >>> The generic Synopsys MIPI DSI host driver does not register it's own >>> drm_bridge until the MIPI DSI has attached. But it does not call >>> drm_bridge_attach() on the downstream device until the upstream device >>> has attached. This leads to a chicken and the egg failure and the DRM >>> pipeline does not complete. >>> Since all other mipi_dsi_device drivers call mipi_dsi_attach() in >>> probe(), make the adv7533 mipi_dsi_device do the same. This ensures that >>> the Synopsys MIPI DSI host registers it's bridge such that it is >>> available for the upstream device to connect to. >>> >>> Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> >>> >>> --- >> As a heads up, I just did some testing on drm-misc-next and this patch >> seems to be breaking the HiKey board. On bootup, I'm seeing: >> [ 4.209615] adv7511 2-0039: 2-0039 supply avdd not found, using >> dummy regulator >> [ 4.217075] adv7511 2-0039: 2-0039 supply dvdd not found, using >> dummy regulator >> [ 4.224453] adv7511 2-0039: 2-0039 supply pvdd not found, using >> dummy regulator >> [ 4.231804] adv7511 2-0039: 2-0039 supply a2vdd not found, using >> dummy regulator >> [ 4.239242] adv7511 2-0039: 2-0039 supply v3p3 not found, using >> dummy regulator >> [ 4.246615] adv7511 2-0039: 2-0039 supply v1p2 not found, using >> dummy regulator >> [ 4.272970] adv7511 2-0039: failed to find dsi host >> >> over and over. The dummy regulator messages are normal, but usually >> [ 4.444315] kirin-drm f4100000.ade: bound f4107800.dsi (ops dsi_ops) >> >> Starts up right afterward. > Hey Matt, Andrzej, > I just wanted to follow up on this as this patch is breaking a > couple of boards. Any sense of what might be missing, or is this > something we should revert? > > I'm happy to test any patch ideas you have. I guess this is circular dependency issue: - adv waits for dsi-host, then it creates drm_bridge, - dsi-driver waits for drm_bridge, then it creates dsi host. The patch introduces proper order: - 1st we should register devices buses, - then we should wait for drm components. So the best solution would be to fix f4107800.dsi driver - it shouldn't look for drm_bridge in probe, instead it should register dsi_host, and in dsi host attach callback look for drm_bridge, then call component_add (if all required resources are gathered), see dw_mipi_dsi_rockchip_host_attach for example. Regards Andrzej > > thanks > -john > >
On 28/08/2019 08:05, Andrzej Hajda wrote: > On 27.08.2019 22:03, John Stultz wrote: >> On Mon, Aug 19, 2019 at 3:27 PM John Stultz <john.stultz@linaro.org> wrote: >>> On Thu, Jun 27, 2019 at 8:18 AM Matt Redfearn <matt.redfearn@thinci.com> wrote: >>>> In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel >>>> which attach to the DSI host via mipi_dsi_attach() at probe time, the >>>> ADV7533 bridge device does not. Instead it defers this to the point that >>>> the upstream device connects to its bridge via drm_bridge_attach(). >>>> The generic Synopsys MIPI DSI host driver does not register it's own >>>> drm_bridge until the MIPI DSI has attached. But it does not call >>>> drm_bridge_attach() on the downstream device until the upstream device >>>> has attached. This leads to a chicken and the egg failure and the DRM >>>> pipeline does not complete. >>>> Since all other mipi_dsi_device drivers call mipi_dsi_attach() in >>>> probe(), make the adv7533 mipi_dsi_device do the same. This ensures that >>>> the Synopsys MIPI DSI host registers it's bridge such that it is >>>> available for the upstream device to connect to. >>>> >>>> Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> >>>> >>>> --- >>> As a heads up, I just did some testing on drm-misc-next and this patch >>> seems to be breaking the HiKey board. On bootup, I'm seeing: >>> [ 4.209615] adv7511 2-0039: 2-0039 supply avdd not found, using >>> dummy regulator >>> [ 4.217075] adv7511 2-0039: 2-0039 supply dvdd not found, using >>> dummy regulator >>> [ 4.224453] adv7511 2-0039: 2-0039 supply pvdd not found, using >>> dummy regulator >>> [ 4.231804] adv7511 2-0039: 2-0039 supply a2vdd not found, using >>> dummy regulator >>> [ 4.239242] adv7511 2-0039: 2-0039 supply v3p3 not found, using >>> dummy regulator >>> [ 4.246615] adv7511 2-0039: 2-0039 supply v1p2 not found, using >>> dummy regulator >>> [ 4.272970] adv7511 2-0039: failed to find dsi host >>> >>> over and over. The dummy regulator messages are normal, but usually >>> [ 4.444315] kirin-drm f4100000.ade: bound f4107800.dsi (ops dsi_ops) >>> >>> Starts up right afterward. >> Hey Matt, Andrzej, >> I just wanted to follow up on this as this patch is breaking a >> couple of boards. Any sense of what might be missing, or is this >> something we should revert? >> >> I'm happy to test any patch ideas you have. > > > I guess this is circular dependency issue: > > - adv waits for dsi-host, then it creates drm_bridge, > > - dsi-driver waits for drm_bridge, then it creates dsi host. > > > The patch introduces proper order: > > - 1st we should register devices buses, > > - then we should wait for drm components. > > > So the best solution would be to fix f4107800.dsi driver - it shouldn't > look for drm_bridge in probe, instead it should register dsi_host, and > in dsi host attach callback look for drm_bridge, then call component_add > (if all required resources are gathered), see > dw_mipi_dsi_rockchip_host_attach for example. Hi all, Sorry that my patch seems to have broken things. It was using the generic synopsys dsi driver (drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c) that I found the circular dependency fixed by my patch. It appears that the patch has broken things for the platform specific synopsys dsi driver for Kirin. So another approach (though a lot more work) would be to switch Kirin over to using the generic synopsys dsi driver so that we have less redundancy in the tree. Thanks, Matt > > > Regards > > Andrzej > > > >> >> thanks >> -john >> >> >
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index e7ddd3e3db9..807827bd910 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -874,9 +874,6 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge) &adv7511_connector_helper_funcs); drm_connector_attach_encoder(&adv->connector, bridge->encoder); - if (adv->type == ADV7533) - ret = adv7533_attach_dsi(adv); - if (adv->i2c_main->irq) regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0), ADV7511_INT0_HPD); @@ -1222,8 +1219,17 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) drm_bridge_add(&adv7511->bridge); adv7511_audio_init(dev, adv7511); + + if (adv7511->type == ADV7533) { + ret = adv7533_attach_dsi(adv7511); + if (ret) + goto err_remove_bridge; + } + return 0; +err_remove_bridge: + drm_bridge_remove(&adv7511->bridge); err_unregister_cec: i2c_unregister_device(adv7511->i2c_cec); if (adv7511->cec_clk)
In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel which attach to the DSI host via mipi_dsi_attach() at probe time, the ADV7533 bridge device does not. Instead it defers this to the point that the upstream device connects to its bridge via drm_bridge_attach(). The generic Synopsys MIPI DSI host driver does not register it's own drm_bridge until the MIPI DSI has attached. But it does not call drm_bridge_attach() on the downstream device until the upstream device has attached. This leads to a chicken and the egg failure and the DRM pipeline does not complete. Since all other mipi_dsi_device drivers call mipi_dsi_attach() in probe(), make the adv7533 mipi_dsi_device do the same. This ensures that the Synopsys MIPI DSI host registers it's bridge such that it is available for the upstream device to connect to. Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> --- Changes in v2: Cleanup if adv7533_attach_dsi fails. drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)