[v6,1/6] drm: sun4i: dsi: Drop DRM bind race with bridge attach

Message ID 20211210111711.2072660-2-jagan@amarulasolutions.com
State New
Headers show
Series
  • drm: sun4i: dsi: Bridge support
Related show

Commit Message

Jagan Teki Dec. 10, 2021, 11:17 a.m. UTC
Existing host driver will keep looking for DRM pointer in
sun6i_dsi_attach and defers even if the particular DSI device
is found for the first time. Meanwhile it triggers the bind
callback and gets the DRM pointer and then continues the
sun6i_dsi_attach.

This makes a deadlock situation if sun6i_dsi_attach is trying
to find the bridge.

If interface bridge is trying to call host attach, then host
sun6i_dsi_attach is trying to find bridge and defers the
interface bridge even if it found the bridge as bind callback
does not complete at the movement. So, this sun6i_dsi_attach
defers interface bridge and triggers the bind callback and
tries to attach the bridge with a bridge pointer which is not
available at the moment.

Eventually these callbacks are triggered recursively, as
sun6i_dsi_attach defers interface bridge and bind callback
defers sun6i_dsi_attach due to invalid bridge ponter.

This patch prevents this situation by probing all DSI devices
on the pipeline first and then triggers the bind callback by
dropping exing DRM binding logic.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v6:
- none
Changes for v5:
- new patch

 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 10 +---------
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  1 -
 2 files changed, 1 insertion(+), 10 deletions(-)

Comments

Maxime Ripard Dec. 13, 2021, 5:02 p.m. UTC | #1
On Fri, Dec 10, 2021 at 04:47:06PM +0530, Jagan Teki wrote:
> Existing host driver will keep looking for DRM pointer in
> sun6i_dsi_attach and defers even if the particular DSI device
> is found for the first time. Meanwhile it triggers the bind
> callback and gets the DRM pointer and then continues the
> sun6i_dsi_attach.
> 
> This makes a deadlock situation if sun6i_dsi_attach is trying
> to find the bridge.

I'm not sure what you mean by deadlock here, there's no lock involved?

> If interface bridge is trying to call host attach, then host
> sun6i_dsi_attach is trying to find bridge and defers the
> interface bridge even if it found the bridge as bind callback
> does not complete at the movement. So, this sun6i_dsi_attach
> defers interface bridge and triggers the bind callback and
> tries to attach the bridge with a bridge pointer which is not
> available at the moment.
>
> Eventually these callbacks are triggered recursively, as
> sun6i_dsi_attach defers interface bridge and bind callback
> defers sun6i_dsi_attach due to invalid bridge ponter.

                                                ^ pointer

> 
> This patch prevents this situation by probing all DSI devices
> on the pipeline first and then triggers the bind callback by
> dropping exing DRM binding logic.

           ^ existing I guess?

> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v6:
> - none
> Changes for v5:
> - new patch
> 
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 10 +---------
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  1 -
>  2 files changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 527c7b2474da..4bdcce8f1d84 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -967,14 +967,10 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>  
>  	if (IS_ERR(panel))
>  		return PTR_ERR(panel);
> -	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);
>  
>  	return 0;
> @@ -988,8 +984,6 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host,
>  	dsi->panel = NULL;
>  	dsi->device = NULL;
>  
> -	drm_kms_helper_hotplug_event(dsi->drm);
> -
>  	return 0;
>  }
>  
> @@ -1077,8 +1071,6 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
>  
>  	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
>  
> -	dsi->drm = drm;
> -
>  	return 0;
>  
>  err_cleanup_connector:
> @@ -1091,7 +1083,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..61e88ea6044d 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> @@ -29,7 +29,6 @@ struct sun6i_dsi {
>  
>  	struct device		*dev;
>  	struct mipi_dsi_device	*device;
> -	struct drm_device	*drm;
>  	struct drm_panel	*panel;
>  };
>  
> -- 
> 2.25.1
>
Jagan Teki Jan. 17, 2022, 3:42 p.m. UTC | #2
On Mon, Dec 13, 2021 at 10:32 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Fri, Dec 10, 2021 at 04:47:06PM +0530, Jagan Teki wrote:
> > Existing host driver will keep looking for DRM pointer in
> > sun6i_dsi_attach and defers even if the particular DSI device
> > is found for the first time. Meanwhile it triggers the bind
> > callback and gets the DRM pointer and then continues the
> > sun6i_dsi_attach.
> >
> > This makes a deadlock situation if sun6i_dsi_attach is trying
> > to find the bridge.
>
> I'm not sure what you mean by deadlock here, there's no lock involved?

deadlock parse here for general understanding, where bind is trying to
attach bridge but drm pointer is not available that point and drm
pointer will available only when bind done. This is what I'm calling
as deadlock here.

Anyway, now I'm able to support both panel and bridge to support
hotplug so no need to drop the hotplug support.

Please let me know, if you have any questions so-that I can send next
version series.

Thanks,
Jagan.

Patch

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 527c7b2474da..4bdcce8f1d84 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -967,14 +967,10 @@  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
 
 	if (IS_ERR(panel))
 		return PTR_ERR(panel);
-	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);
 
 	return 0;
@@ -988,8 +984,6 @@  static int sun6i_dsi_detach(struct mipi_dsi_host *host,
 	dsi->panel = NULL;
 	dsi->device = NULL;
 
-	drm_kms_helper_hotplug_event(dsi->drm);
-
 	return 0;
 }
 
@@ -1077,8 +1071,6 @@  static int sun6i_dsi_bind(struct device *dev, struct device *master,
 
 	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
 
-	dsi->drm = drm;
-
 	return 0;
 
 err_cleanup_connector:
@@ -1091,7 +1083,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..61e88ea6044d 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -29,7 +29,6 @@  struct sun6i_dsi {
 
 	struct device		*dev;
 	struct mipi_dsi_device	*device;
-	struct drm_device	*drm;
 	struct drm_panel	*panel;
 };