Re: [PATCH v4 2/4] drm: sun4i: dsi: Add bridge support

[Samuel Holland <samuel@sholland.org>]

On 3/22/21 9:01 AM, Jagan Teki wrote:
> Some display panels would come up with a non-DSI output which
> can have an option to connect DSI interface by means of bridge
> converter.
> 
> This DSI to non-DSI bridge converter would require a bridge
> driver that would communicate the DSI controller for bridge
> functionalities.
> 
> So, add support for bridge functionalities in Allwinner DSI
> controller.
> 
> Cc: Samuel Holland <samuel@sholland.org>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Note: 
> Samuel Holland, The existing kms hotplug dropped in order to 
> attach the bridge properly. 
> 
> However, I did try several ways to support hotplug with the 
> bridge but it's resulting in a deadlock where bind never attach 
> bridge until bridge pointer found and bridge pointer cannot 
> found until bind finishes. Any inputs on this would be appreciated.

The intended behavior is that sun6i_dsi_bind() is independent of any DSI
device. And sun6i_dsi_attach() must only be called after bind completes
and the DRM device is registered. This design allows the rest of the
display engine (such as the HDMI output) to work even if no panel is
listed in the device tree, or if a panel driver is missing.

> Changes for v4:
> - none
> Changes for v3:
> - updated with new API's 
> 
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 34 +++++++++++++++++---------
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  2 +-
>  2 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 2e9e7b2d4145..39321299dc27 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -773,6 +773,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>  	if (dsi->panel)
>  		drm_panel_prepare(dsi->panel);
>  
> +	if (dsi->panel_bridge)
> +		dsi->panel_bridge->funcs->pre_enable(dsi->panel_bridge);
> +
>  	/*
>  	 * FIXME: This should be moved after the switch to HS mode.
>  	 *
> @@ -788,6 +791,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>  	if (dsi->panel)
>  		drm_panel_enable(dsi->panel);
>  
> +	if (dsi->panel_bridge)
> +		dsi->panel_bridge->funcs->enable(dsi->panel_bridge);
> +
>  	sun6i_dsi_start(dsi, DSI_START_HSC);
>  
>  	udelay(1000);
> @@ -804,6 +810,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
>  	if (dsi->panel) {
>  		drm_panel_disable(dsi->panel);
>  		drm_panel_unprepare(dsi->panel);
> +	} else if (dsi->panel_bridge) {
> +		dsi->panel_bridge->funcs->disable(dsi->panel_bridge);
> +		dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);
>  	}
>  
>  	phy_power_off(dsi->dphy);
> @@ -964,23 +973,17 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>  			    struct mipi_dsi_device *device)
>  {
>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> -	struct drm_panel *panel;
>  	int ret;
>  
>  	ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> -					  &panel, NULL);
> +					  &dsi->panel, &dsi->panel_bridge);
>  	if (ret)
>  		return ret;
>  
> -	if (!dsi->drm || !dsi->drm->registered)
> -		return -EPROBE_DEFER;
> -
> -	dsi->panel = panel;
>  	dsi->device = device;
>  
> -	drm_kms_helper_hotplug_event(dsi->drm);
> -
> -	dev_info(host->dev, "Attached device %s\n", device->name);
> +	dev_info(host->dev, "Attached %s %s\n",
> +		 device->name, dsi->panel ? "panel" : "bridge");
>  
>  	return 0;
>  }
> @@ -991,9 +994,10 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host,
>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
>  
>  	dsi->panel = NULL;
> +	dsi->panel_bridge = NULL;
>  	dsi->device = NULL;
>  
> -	drm_kms_helper_hotplug_event(dsi->drm);
> +	drm_of_panel_bridge_remove(dsi->dev->of_node, 0, 0);
>  
>  	return 0;
>  }
> @@ -1082,7 +1086,13 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
>  
>  	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
>  
> -	dsi->drm = drm;
> +	if (dsi->panel_bridge) {
> +		ret = drm_bridge_attach(&dsi->encoder, dsi->panel_bridge, NULL, 0);
> +		if (ret) {
> +			dev_err(dsi->dev, "Couldn't attach drm bridge\n");
> +			goto err_cleanup_connector;
> +		}
> +	}

You initialize dsi->panel_bridge in sun6i_dsi_attach() above, but that
function may run long after sun6i_dsi_bind() has completed -- for
example if this driver is built in, but the panel/bridge driver is built
as a module. So you cannot reference dsi->panel_bridge from this function.

Regards,
Samuel

>  
>  	return 0;
>  
> @@ -1096,7 +1106,7 @@ static void sun6i_dsi_unbind(struct device *dev, struct device *master,
>  {
>  	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
>  
> -	dsi->drm = NULL;
> +	drm_encoder_cleanup(&dsi->encoder);
>  }
>  
>  static const struct component_ops sun6i_dsi_ops = {
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> index c863900ae3b4..370ecb356a63 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> @@ -29,8 +29,8 @@ struct sun6i_dsi {
>  
>  	struct device		*dev;
>  	struct mipi_dsi_device	*device;
> -	struct drm_device	*drm;
>  	struct drm_panel	*panel;
> +	struct drm_bridge	*panel_bridge;
>  };
>  
>  static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)
>