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

2021-03-23 Thread Samuel Holland
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 
> Signed-off-by: Jagan Teki 
> ---
> 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,
> -   , NULL);
> +   >panel, >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(>connector, >encoder);
>  
> - dsi->drm = drm;
> + if (dsi->panel_bridge) {
> + ret = drm_bridge_attach(>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 

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

2021-03-23 Thread Laurent Pinchart
Hi Jagan,

Thank you for the patch.

On Mon, Mar 22, 2021 at 07:31:50PM +0530, Jagan Teki wrote:
> Some display panels would come up with a non-DSI output which

Did you mean input instead of output ?

> 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 
> Signed-off-by: Jagan Teki 
> ---
> 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.
> 
> 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);
>   }

Instead of having code paths that depend on whether you have a panel or
a bridge, it would be better to wrap the panel a bridge (using
drivers/gpu/drm/bridge/panel.c). The dsi->panel_bridge pointer should be
renamed to next_bridge, and all the code (except in probe) can the use
next_bridge without caring if it's a direct connection to a panel or
another bridge.

Furthermore, the encoder should call bridge functions explicitly, this
should be handled by the DRM core.

>  
>   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,
> -   , NULL);
> +   >panel, >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(>connector, >encoder);
>  
> - dsi->drm = drm;
> + if (dsi->panel_bridge) {
> + ret = drm_bridge_attach(>encoder, dsi->panel_bridge, NULL, 
> 0);
> + if (ret) {
> + dev_err(dsi->dev, "Couldn't attach drm bridge\n");
> + goto err_cleanup_connector;
> + }
> + }
>  
>   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);
>  
>