by Adrian Ratiu

[permalink] [raw]

Subject: Re: [PATCH v8 04/10] drm: bridge: dw_mipi_dsi: allow bridge daisy chaining

On Wed, 03 Jun 2020, Laurent Pinchart
<[email protected]> wrote:
> Hi Adrian,

Hi Laurent,

>
> Thank you for the patch.
>
> On Mon, Apr 27, 2020 at 11:19:46AM +0300, Adrian Ratiu wrote:
>> Up until now the assumption was that the synopsis dsi bridge
>> will directly connect to an encoder provided by the platform
>> driver, but the current practice for drivers is to leave the
>> encoder empty via the simple encoder API and add their logic to
>> their own drm_bridge. Thus we need an ablility to connect the
>> DSI bridge to another bridge provided by the platform driver,
>> so we extend the dw_mipi_dsi bind() API with a new "previous
>> bridge" arg instead of just hardcoding NULL. Cc: Laurent
>> Pinchart <[email protected]> Signed-off-by:
>> Adrian Ratiu <[email protected]> --- New in v8. ---
>> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 6 ++++--
>> drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 2 +-
>> include/drm/bridge/dw_mipi_dsi.h | 5 ++++- 3
>> files changed, 9 insertions(+), 4 deletions(-)
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index
>> 16fd87055e7b7..140ff40fa1b62 100644 ---
>> a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++
>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -1456,11
>> +1456,13 @@ EXPORT_SYMBOL_GPL(dw_mipi_dsi_remove);
>> /*
>> * Bind/unbind API, used from platforms based on the component
>> framework. */
>> -int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi, struct
>> drm_encoder *encoder) +int dw_mipi_dsi_bind(struct dw_mipi_dsi
>> *dsi, + struct drm_encoder *encoder, +
>> struct drm_bridge *prev_bridge)
>> { int ret;
>> - ret = drm_bridge_attach(encoder, &dsi->bridge, NULL, 0); +
>> ret = drm_bridge_attach(encoder, &dsi->bridge, prev_bridge, 0);
>
> Please note that chaining of bridges doesn't work well if
> multiple bridges in the chain try to create a connector. This is
> why a DRM_BRIDGE_ATTACH_NO_CONNECTOR flag has been added, with a
> helper to create a connector for a chain of bridges
> (drm_bridge_connector_init()). This won't play well with the
> component framework. I would recommend using the
> of_drm_find_bridge() instead in the rockchip driver, and
> deprecating dw_mipi_dsi_bind().
>

Thank you for this insight, indeed the bridge dw_mipi_dsi_bind()
is clunky and we're making it even more so by possibly
re-inventing drm_bridge_connector_init() with it in a way which
can't work (well it does work but can lead to those nasty
multiple-encoder corner-cases you mention).

I'll address this before posting v9, to try to move to
of_drm_find_bridge() and remove dw_mipi_dsi_bind().

>> if (ret) {
>> DRM_ERROR("Failed to initialize bridge with drm\n");
>> return ret;
>> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
>> index 3feff0c45b3f7..83ef43be78135 100644
>> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
>> @@ -929,7 +929,7 @@ static int dw_mipi_dsi_rockchip_bind(struct device *dev,
>> return ret;
>> }
>>
>> - ret = dw_mipi_dsi_bind(dsi->dmd, &dsi->encoder);
>> + ret = dw_mipi_dsi_bind(dsi->dmd, &dsi->encoder, NULL);
>> if (ret) {
>> DRM_DEV_ERROR(dev, "Failed to bind: %d\n", ret);
>> return ret;
>> diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h
>> index b0e390b3288e8..699b3531f5b36 100644
>> --- a/include/drm/bridge/dw_mipi_dsi.h
>> +++ b/include/drm/bridge/dw_mipi_dsi.h
>> @@ -14,6 +14,7 @@
>> #include <drm/drm_modes.h>
>>
>> struct drm_display_mode;
>> +struct drm_bridge;
>> struct drm_encoder;
>> struct dw_mipi_dsi;
>> struct mipi_dsi_device;
>> @@ -62,7 +63,9 @@ struct dw_mipi_dsi *dw_mipi_dsi_probe(struct platform_device *pdev,
>> const struct dw_mipi_dsi_plat_data
>> *plat_data);
>> void dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi);
>> -int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi, struct drm_encoder *encoder);
>> +int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi,
>> + struct drm_encoder *encoder,
>> + struct drm_bridge *prev_bridge);
>> void dw_mipi_dsi_unbind(struct dw_mipi_dsi *dsi);
>> void dw_mipi_dsi_set_slave(struct dw_mipi_dsi *dsi, struct dw_mipi_dsi *slave);
>>
>
> --
> Regards,
>
> Laurent Pinchart