[PATCH 1/4] drm/sun4i: dsi: Remove unused drv from driver context

[PATCH 1/4] drm/sun4i: dsi: Remove unused drv from driver context

[Samuel Holland]

This member is never used, so remove it.

Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller support")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 ----
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 1 -
 2 files changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index c958ca9bae63..c07290541fff 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -27,7 +27,6 @@
 #include <drm/drm_probe_helper.h>
 
 #include "sun4i_crtc.h"
-#include "sun4i_drv.h"
 #include "sun4i_tcon.h"
 #include "sun6i_mipi_dsi.h"
 
@@ -1022,15 +1021,12 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 			 void *data)
 {
 	struct drm_device *drm = data;
-	struct sun4i_drv *drv = drm->dev_private;
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 	int ret;
 
 	if (!dsi->panel)
 		return -EPROBE_DEFER;
 
-	dsi->drv = drv;
-
 	drm_encoder_helper_add(&dsi->encoder,
 			       &sun6i_dsi_enc_helper_funcs);
 	ret = drm_encoder_init(drm,
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
index 3f4846f581ef..61e88ea6044d 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -28,7 +28,6 @@ struct sun6i_dsi {
 	struct phy		*dphy;
 
 	struct device		*dev;
-	struct sun4i_drv	*drv;
 	struct mipi_dsi_device	*device;
 	struct drm_panel	*panel;
 };
-- 
2.24.1

[PATCH 2/4] drm/sun4i: dsi: Use NULL to signify "no panel"

[Samuel Holland]

The continued use of an ERR_PTR to signify "no panel" outside of
sun6i_dsi_attach is confusing because it is a double negative. Because
the connector always reports itself as connected, there is also the
possibility of sending an ERR_PTR to drm_panel_get_modes(), which would
crash.

Solve both of these by only storing the panel pointer if it is valid.

Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller support")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index c07290541fff..019fdf4ec274 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -748,7 +748,7 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	phy_configure(dsi->dphy, &opts);
 	phy_power_on(dsi->dphy);
 
-	if (!IS_ERR(dsi->panel))
+	if (dsi->panel)
 		drm_panel_prepare(dsi->panel);
 
 	/*
@@ -763,7 +763,7 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	 * ordering on the panels I've tested it with, so I guess this
 	 * will do for now, until that IP is better understood.
 	 */
-	if (!IS_ERR(dsi->panel))
+	if (dsi->panel)
 		drm_panel_enable(dsi->panel);
 
 	sun6i_dsi_start(dsi, DSI_START_HSC);
@@ -779,7 +779,7 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Disabling DSI output\n");
 
-	if (!IS_ERR(dsi->panel)) {
+	if (dsi->panel) {
 		drm_panel_disable(dsi->panel);
 		drm_panel_unprepare(dsi->panel);
 	}
@@ -941,11 +941,13 @@ 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 = of_drm_find_panel(device->dev.of_node);
 
+	if (IS_ERR(panel))
+		return PTR_ERR(panel);
+
+	dsi->panel = panel;
 	dsi->device = device;
-	dsi->panel = of_drm_find_panel(device->dev.of_node);
-	if (IS_ERR(dsi->panel))
-		return PTR_ERR(dsi->panel);
 
 	dev_info(host->dev, "Attached device %s\n", device->name);
 
-- 
2.24.1

[PATCH 3/4] drm/sun4i: dsi: Allow binding the host without a panel

[Samuel Holland]

Currently, the DSI host blocks binding the display pipeline until the
panel is available. This unnecessarily prevents other display outpus
from working, and adds logspam to dmesg when the panel driver is built
as a module (the component master is unsuccessfully brought up several
times during boot).

Flip the dependency, instead requiring the host to be bound before the
panel is attached. The panel driver provides no functionality outside of
the display pipeline anyway.

Since the panel is now probed after the DRM connector, we need a hotplug
event to turn on the connector after the panel is attached.

This has the added benefit of fixing panel module removal/insertion.
Previously, the panel would be turned off when its module was removed.
But because the connector state was hardcoded, nothing knew to turn the
panel back on when it was re-attached. Now, with hotplug events
available, the connector state will follow the panel module state, and
the panel will be re-enabled properly.

Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller support")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 22 ++++++++++++++++------
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  1 +
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 019fdf4ec274..ef35ce5a9bb0 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -804,7 +804,10 @@ static struct drm_connector_helper_funcs sun6i_dsi_connector_helper_funcs = {
 static enum drm_connector_status
 sun6i_dsi_connector_detect(struct drm_connector *connector, bool force)
 {
-	return connector_status_connected;
+	struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
+
+	return dsi->panel ? connector_status_connected :
+			    connector_status_disconnected;
 }
 
 static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
@@ -945,10 +948,15 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
 
 	if (IS_ERR(panel))
 		return PTR_ERR(panel);
+	if (!dsi->drm)
+		return -EPROBE_DEFER;
 
 	dsi->panel = panel;
 	dsi->device = device;
 
+	drm_panel_attach(dsi->panel, &dsi->connector);
+	drm_kms_helper_hotplug_event(dsi->drm);
+
 	dev_info(host->dev, "Attached device %s\n", device->name);
 
 	return 0;
@@ -958,10 +966,14 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host,
 			    struct mipi_dsi_device *device)
 {
 	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
+	struct drm_panel *panel = dsi->panel;
 
 	dsi->panel = NULL;
 	dsi->device = NULL;
 
+	drm_panel_detach(panel);
+	drm_kms_helper_hotplug_event(dsi->drm);
+
 	return 0;
 }
 
@@ -1026,9 +1038,6 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 	int ret;
 
-	if (!dsi->panel)
-		return -EPROBE_DEFER;
-
 	drm_encoder_helper_add(&dsi->encoder,
 			       &sun6i_dsi_enc_helper_funcs);
 	ret = drm_encoder_init(drm,
@@ -1054,7 +1063,8 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 	}
 
 	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
-	drm_panel_attach(dsi->panel, &dsi->connector);
+
+	dsi->drm = drm;
 
 	return 0;
 
@@ -1068,7 +1078,7 @@ static void sun6i_dsi_unbind(struct device *dev, struct device *master,
 {
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 
-	drm_panel_detach(dsi->panel);
+	dsi->drm = NULL;
 }
 
 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 61e88ea6044d..c863900ae3b4 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -29,6 +29,7 @@ struct sun6i_dsi {
 
 	struct device		*dev;
 	struct mipi_dsi_device	*device;
+	struct drm_device	*drm;
 	struct drm_panel	*panel;
 };
 
-- 
2.24.1

Re: [PATCH 3/4] drm/sun4i: dsi: Allow binding the host without a panel

[Samuel Holland]

Maxime,

On 2/11/20 1:28 AM, Samuel Holland wrote:
> Currently, the DSI host blocks binding the display pipeline until the
> panel is available. This unnecessarily prevents other display outpus
> from working, and adds logspam to dmesg when the panel driver is built
> as a module (the component master is unsuccessfully brought up several
> times during boot).
> 
> Flip the dependency, instead requiring the host to be bound before the
> panel is attached. The panel driver provides no functionality outside of
> the display pipeline anyway.
> 
> Since the panel is now probed after the DRM connector, we need a hotplug
> event to turn on the connector after the panel is attached.
> 
> This has the added benefit of fixing panel module removal/insertion.
> Previously, the panel would be turned off when its module was removed.
> But because the connector state was hardcoded, nothing knew to turn the
> panel back on when it was re-attached. Now, with hotplug events
> available, the connector state will follow the panel module state, and
> the panel will be re-enabled properly.
> 
> Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller support")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 22 ++++++++++++++++------
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  1 +
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 019fdf4ec274..ef35ce5a9bb0 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -804,7 +804,10 @@ static struct drm_connector_helper_funcs sun6i_dsi_connector_helper_funcs = {
>  static enum drm_connector_status
>  sun6i_dsi_connector_detect(struct drm_connector *connector, bool force)
>  {
> -	return connector_status_connected;
> +	struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
> +
> +	return dsi->panel ? connector_status_connected :
> +			    connector_status_disconnected;
>  }
>  
>  static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
> @@ -945,10 +948,15 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>  
>  	if (IS_ERR(panel))
>  		return PTR_ERR(panel);
> +	if (!dsi->drm)
> +		return -EPROBE_DEFER;

There's actually a bug here. If the panel and DSI drivers are loaded in
parallel, sun6i_dsi_attach() can be called after sun6i_dsi_bind() but before
sun4i_framebuffer_init() initializes drm->mode_config.funcs, causing the hotplug
call to crash. This check also needs to consider dsi->drm->registered before
allowing the panel to be added.

I can send a v2 or a follow-up, whichever you prefer.

Thanks,
Samuel

>  	dsi->panel = panel;
>  	dsi->device = device;
>  
> +	drm_panel_attach(dsi->panel, &dsi->connector);
> +	drm_kms_helper_hotplug_event(dsi->drm);
> +
>  	dev_info(host->dev, "Attached device %s\n", device->name);
>  
>  	return 0;
> @@ -958,10 +966,14 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host,
>  			    struct mipi_dsi_device *device)
>  {
>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> +	struct drm_panel *panel = dsi->panel;
>  
>  	dsi->panel = NULL;
>  	dsi->device = NULL;
>  
> +	drm_panel_detach(panel);
> +	drm_kms_helper_hotplug_event(dsi->drm);
> +
>  	return 0;
>  }
>  
> @@ -1026,9 +1038,6 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
>  	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
>  	int ret;
>  
> -	if (!dsi->panel)
> -		return -EPROBE_DEFER;
> -
>  	drm_encoder_helper_add(&dsi->encoder,
>  			       &sun6i_dsi_enc_helper_funcs);
>  	ret = drm_encoder_init(drm,
> @@ -1054,7 +1063,8 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
>  	}
>  
>  	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
> -	drm_panel_attach(dsi->panel, &dsi->connector);
> +
> +	dsi->drm = drm;
>  
>  	return 0;
>  
> @@ -1068,7 +1078,7 @@ static void sun6i_dsi_unbind(struct device *dev, struct device *master,
>  {
>  	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
>  
> -	drm_panel_detach(dsi->panel);
> +	dsi->drm = NULL;
>  }
>  
>  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 61e88ea6044d..c863900ae3b4 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> @@ -29,6 +29,7 @@ struct sun6i_dsi {
>  
>  	struct device		*dev;
>  	struct mipi_dsi_device	*device;
> +	struct drm_device	*drm;
>  	struct drm_panel	*panel;
>  };
>  
> 

[PATCH 4/4] drm/sun4i: dsi: Remove incorrect use of runtime PM

[Samuel Holland]

The driver currently uses runtime PM to perform some of the module
initialization and cleanup. This has three problems:

1) There is no Kconfig dependency on CONFIG_PM, so if runtime PM is
   disabled, the driver will not work at all, since the module will
   never be initialized.

2) The driver does not ensure that the device is suspended when
   sun6i_dsi_probe() fails or when sun6i_dsi_remove() is called. It
   simply disables runtime PM. From the docs of pm_runtime_disable():

      The device can be either active or suspended after its runtime PM
      has been disabled.

   And indeed, the device will likely still be active if sun6i_dsi_probe
   fails. For example, if the panel driver is not yet loaded, we have
   the following sequence:

   sun6i_dsi_probe()
      pm_runtime_enable()
      mipi_dsi_host_register()
         of_mipi_dsi_device_add(child)
            ...device_add()...
               __device_attach()
                 pm_runtime_get_sync(dev->parent) -> Causes resume
                 bus_for_each_drv()
                    __device_attach_driver() -> No match for panel
                 pm_runtime_put(dev->parent) -> Async idle request
      component_add()
         __component_add()
            try_to_bring_up_masters()
               try_to_bring_up_master()
                  sun4i_drv_bind()
                     component_bind_all()
                        component_bind()
                           sun6i_dsi_bind() -> Fails with -EPROBE_DEFER
      mipi_dsi_host_unregister()
      pm_runtime_disable()
         __pm_runtime_disable()
            __pm_runtime_barrier() -> Idle request is still pending
               cancel_work_sync()  -> DSI host is *not* suspended!

   Since the device is not suspended, the clock and regulator are never
   disabled. The imbalance causes a WARN at devres free time.

3) The driver relies on being suspended when sun6i_dsi_encoder_enable()
   is called. The resume callback has a comment that says:

      Some part of it can only be done once we get a number of
      lanes, see sun6i_dsi_inst_init

   And then part of the resume callback only runs if dsi->device is not
   NULL (that is, if sun6i_dsi_attach() has been called). However, as
   the above call graph shows, the resume callback is guaranteed to be
   called before sun6i_dsi_attach(); it is called before child devices
   get their drivers attached.

   Therefore, part of the controller initialization will only run if the
   device is suspended between the calls to mipi_dsi_host_register() and
   component_add() (which ends up calling sun6i_dsi_encoder_enable()).
   Again, as shown by the above call graph, this is not the case. It
   appears that the controller happens to work because it is still
   initialized by the bootloader.

   Because the connector is hardcoded to always be connected, the
   device's runtime PM reference is not dropped until system suspend,
   when sun4i_drv_drm_sys_suspend() ends up calling
   sun6i_dsi_encoder_disable(). However, that is done as a system sleep
   PM hook, and at that point the system PM core has already taken
   another runtime PM reference, so sun6i_dsi_runtime_suspend() is
   not called. Likewise, by the time the PM core releases its reference,
   sun4i_drv_drm_sys_resume() has already re-enabled the encoder.

   So after system suspend and resume, we have *still never called*
   sun6i_dsi_inst_init(), and now that the rest of the display pipeline
   has been reset, the DSI host is unable to communicate with the panel,
   causing VBLANK timeouts.

Fix all of these issues by inlining the runtime PM hooks into the
encoder enable/disable functions, which are guaranteed to run after a
panel is attached. This allows sun6i_dsi_inst_init() to be called
unconditionally. Furthermore, this causes the hardware to be turned off
during system suspend and reinitialized on resume, which was not
happening before.

Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller support")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 89 ++++++++------------------
 1 file changed, 26 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index ef35ce5a9bb0..8c2e326d8e16 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -14,7 +14,6 @@
 #include <linux/phy/phy-mipi-dphy.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
-#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
@@ -721,10 +720,31 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	union phy_configure_opts opts = { 0 };
 	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
 	u16 delay;
+	int err;
 
 	DRM_DEBUG_DRIVER("Enabling DSI output\n");
 
-	pm_runtime_get_sync(dsi->dev);
+	err = regulator_enable(dsi->regulator);
+	if (err)
+		dev_warn(dsi->dev, "failed to enable VCC-DSI supply: %d\n", err);
+
+	reset_control_deassert(dsi->reset);
+	clk_prepare_enable(dsi->mod_clk);
+
+	/*
+	 * Enable the DSI block.
+	 */
+	regmap_write(dsi->regs, SUN6I_DSI_CTL_REG, SUN6I_DSI_CTL_EN);
+
+	regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL0_REG,
+		     SUN6I_DSI_BASIC_CTL0_ECC_EN | SUN6I_DSI_BASIC_CTL0_CRC_EN);
+
+	regmap_write(dsi->regs, SUN6I_DSI_TRANS_START_REG, 10);
+	regmap_write(dsi->regs, SUN6I_DSI_TRANS_ZERO_REG, 0);
+
+	sun6i_dsi_inst_init(dsi, dsi->device);
+
+	regmap_write(dsi->regs, SUN6I_DSI_DEBUG_DATA_REG, 0xff);
 
 	delay = sun6i_dsi_get_video_start_delay(dsi, mode);
 	regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL1_REG,
@@ -787,7 +807,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
 	phy_power_off(dsi->dphy);
 	phy_exit(dsi->dphy);
 
-	pm_runtime_put(dsi->dev);
+	clk_disable_unprepare(dsi->mod_clk);
+	reset_control_assert(dsi->reset);
+	regulator_disable(dsi->regulator);
 }
 
 static int sun6i_dsi_get_modes(struct drm_connector *connector)
@@ -1147,12 +1169,10 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 		goto err_unprotect_clk;
 	}
 
-	pm_runtime_enable(dev);
-
 	ret = mipi_dsi_host_register(&dsi->host);
 	if (ret) {
 		dev_err(dev, "Couldn't register MIPI-DSI host\n");
-		goto err_pm_disable;
+		goto err_unprotect_clk;
 	}
 
 	ret = component_add(&pdev->dev, &sun6i_dsi_ops);
@@ -1165,8 +1185,6 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 
 err_remove_dsi_host:
 	mipi_dsi_host_unregister(&dsi->host);
-err_pm_disable:
-	pm_runtime_disable(dev);
 err_unprotect_clk:
 	clk_rate_exclusive_put(dsi->mod_clk);
 	return ret;
@@ -1179,65 +1197,11 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
 
 	component_del(&pdev->dev, &sun6i_dsi_ops);
 	mipi_dsi_host_unregister(&dsi->host);
-	pm_runtime_disable(dev);
 	clk_rate_exclusive_put(dsi->mod_clk);
 
 	return 0;
 }
 
-static int __maybe_unused sun6i_dsi_runtime_resume(struct device *dev)
-{
-	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
-	int err;
-
-	err = regulator_enable(dsi->regulator);
-	if (err) {
-		dev_err(dsi->dev, "failed to enable VCC-DSI supply: %d\n", err);
-		return err;
-	}
-
-	reset_control_deassert(dsi->reset);
-	clk_prepare_enable(dsi->mod_clk);
-
-	/*
-	 * Enable the DSI block.
-	 *
-	 * Some part of it can only be done once we get a number of
-	 * lanes, see sun6i_dsi_inst_init
-	 */
-	regmap_write(dsi->regs, SUN6I_DSI_CTL_REG, SUN6I_DSI_CTL_EN);
-
-	regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL0_REG,
-		     SUN6I_DSI_BASIC_CTL0_ECC_EN | SUN6I_DSI_BASIC_CTL0_CRC_EN);
-
-	regmap_write(dsi->regs, SUN6I_DSI_TRANS_START_REG, 10);
-	regmap_write(dsi->regs, SUN6I_DSI_TRANS_ZERO_REG, 0);
-
-	if (dsi->device)
-		sun6i_dsi_inst_init(dsi, dsi->device);
-
-	regmap_write(dsi->regs, SUN6I_DSI_DEBUG_DATA_REG, 0xff);
-
-	return 0;
-}
-
-static int __maybe_unused sun6i_dsi_runtime_suspend(struct device *dev)
-{
-	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
-
-	clk_disable_unprepare(dsi->mod_clk);
-	reset_control_assert(dsi->reset);
-	regulator_disable(dsi->regulator);
-
-	return 0;
-}
-
-static const struct dev_pm_ops sun6i_dsi_pm_ops = {
-	SET_RUNTIME_PM_OPS(sun6i_dsi_runtime_suspend,
-			   sun6i_dsi_runtime_resume,
-			   NULL)
-};
-
 static const struct of_device_id sun6i_dsi_of_table[] = {
 	{ .compatible = "allwinner,sun6i-a31-mipi-dsi" },
 	{ }
@@ -1250,7 +1214,6 @@ static struct platform_driver sun6i_dsi_platform_driver = {
 	.driver		= {
 		.name		= "sun6i-mipi-dsi",
 		.of_match_table	= sun6i_dsi_of_table,
-		.pm		= &sun6i_dsi_pm_ops,
 	},
 };
 module_platform_driver(sun6i_dsi_platform_driver);
-- 
2.24.1

Re: [PATCH 4/4] drm/sun4i: dsi: Remove incorrect use of runtime PM

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4988 bytes --]

Hi,

On Tue, Feb 11, 2020 at 01:28:58AM -0600, Samuel Holland wrote:
> The driver currently uses runtime PM to perform some of the module
> initialization and cleanup. This has three problems:
>
> 1) There is no Kconfig dependency on CONFIG_PM, so if runtime PM is
>    disabled, the driver will not work at all, since the module will
>    never be initialized.

That's fairly easy to fix.

> 2) The driver does not ensure that the device is suspended when
>    sun6i_dsi_probe() fails or when sun6i_dsi_remove() is called. It
>    simply disables runtime PM. From the docs of pm_runtime_disable():
>
>       The device can be either active or suspended after its runtime PM
>       has been disabled.
>
>    And indeed, the device will likely still be active if sun6i_dsi_probe
>    fails. For example, if the panel driver is not yet loaded, we have
>    the following sequence:
>
>    sun6i_dsi_probe()
>       pm_runtime_enable()
>       mipi_dsi_host_register()
>          of_mipi_dsi_device_add(child)
>             ...device_add()...
>                __device_attach()
>                  pm_runtime_get_sync(dev->parent) -> Causes resume
>                  bus_for_each_drv()
>                     __device_attach_driver() -> No match for panel
>                  pm_runtime_put(dev->parent) -> Async idle request
>       component_add()
>          __component_add()
>             try_to_bring_up_masters()
>                try_to_bring_up_master()
>                   sun4i_drv_bind()
>                      component_bind_all()
>                         component_bind()
>                            sun6i_dsi_bind() -> Fails with -EPROBE_DEFER
>       mipi_dsi_host_unregister()
>       pm_runtime_disable()
>          __pm_runtime_disable()
>             __pm_runtime_barrier() -> Idle request is still pending
>                cancel_work_sync()  -> DSI host is *not* suspended!
>
>    Since the device is not suspended, the clock and regulator are never
>    disabled. The imbalance causes a WARN at devres free time.

That's interesting. I guess this is shown when you have the panel as a
module?

There's something pretty weird though. The comment in
__pm_runtime_disable states that it will "wait for all operations in
progress to complete" so at the end of __pm_runtime_disable call, the
DSI host will be suspended and we shouldn't have a WARN at all.

> 3) The driver relies on being suspended when sun6i_dsi_encoder_enable()
>    is called. The resume callback has a comment that says:
>
>       Some part of it can only be done once we get a number of
>       lanes, see sun6i_dsi_inst_init
>
>    And then part of the resume callback only runs if dsi->device is not
>    NULL (that is, if sun6i_dsi_attach() has been called). However, as
>    the above call graph shows, the resume callback is guaranteed to be
>    called before sun6i_dsi_attach(); it is called before child devices
>    get their drivers attached.

Isn't it something that has been changed by your previous patch though?

>    Therefore, part of the controller initialization will only run if the
>    device is suspended between the calls to mipi_dsi_host_register() and
>    component_add() (which ends up calling sun6i_dsi_encoder_enable()).
>    Again, as shown by the above call graph, this is not the case. It
>    appears that the controller happens to work because it is still
>    initialized by the bootloader.

We don't have any bootloader support for MIPI-DSI, so no, that's not it.

>    Because the connector is hardcoded to always be connected, the
>    device's runtime PM reference is not dropped until system suspend,
>    when sun4i_drv_drm_sys_suspend() ends up calling
>    sun6i_dsi_encoder_disable(). However, that is done as a system sleep
>    PM hook, and at that point the system PM core has already taken
>    another runtime PM reference, so sun6i_dsi_runtime_suspend() is
>    not called. Likewise, by the time the PM core releases its reference,
>    sun4i_drv_drm_sys_resume() has already re-enabled the encoder.
>
>    So after system suspend and resume, we have *still never called*
>    sun6i_dsi_inst_init(), and now that the rest of the display pipeline
>    has been reset, the DSI host is unable to communicate with the panel,
>    causing VBLANK timeouts.

Either way, I guess just moving the pm_runtime_enable call to
sun6i_dsi_attach will fix this, right? We don't really need to have
the DSI controller powered up before that time anyway.

> Fix all of these issues by inlining the runtime PM hooks into the
> encoder enable/disable functions, which are guaranteed to run after a
> panel is attached. This allows sun6i_dsi_inst_init() to be called
> unconditionally. Furthermore, this causes the hardware to be turned off
> during system suspend and reinitialized on resume, which was not
> happening before.

That's not something we should do really. We're really lacking any
power management, so we should be having more of runtime_pm, not less.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 4/4] drm/sun4i: dsi: Remove incorrect use of runtime PM

[Samuel Holland]

Hi Maxime,

On 2/11/20 2:26 AM, Maxime Ripard wrote:
> On Tue, Feb 11, 2020 at 01:28:58AM -0600, Samuel Holland wrote:
>> The driver currently uses runtime PM to perform some of the module
>> initialization and cleanup. This has three problems:
>>
>> 1) There is no Kconfig dependency on CONFIG_PM, so if runtime PM is
>>    disabled, the driver will not work at all, since the module will
>>    never be initialized.
> 
> That's fairly easy to fix.
> 
>> 2) The driver does not ensure that the device is suspended when
>>    sun6i_dsi_probe() fails or when sun6i_dsi_remove() is called. It
>>    simply disables runtime PM. From the docs of pm_runtime_disable():
>>
>>       The device can be either active or suspended after its runtime PM
>>       has been disabled.
>>
>>    And indeed, the device will likely still be active if sun6i_dsi_probe
>>    fails. For example, if the panel driver is not yet loaded, we have
>>    the following sequence:
>>
>>    sun6i_dsi_probe()
>>       pm_runtime_enable()
>>       mipi_dsi_host_register()
>>          of_mipi_dsi_device_add(child)
>>             ...device_add()...
>>                __device_attach()
>>                  pm_runtime_get_sync(dev->parent) -> Causes resume
>>                  bus_for_each_drv()
>>                     __device_attach_driver() -> No match for panel
>>                  pm_runtime_put(dev->parent) -> Async idle request
>>       component_add()
>>          __component_add()
>>             try_to_bring_up_masters()
>>                try_to_bring_up_master()
>>                   sun4i_drv_bind()
>>                      component_bind_all()
>>                         component_bind()
>>                            sun6i_dsi_bind() -> Fails with -EPROBE_DEFER
>>       mipi_dsi_host_unregister()
>>       pm_runtime_disable()
>>          __pm_runtime_disable()
>>             __pm_runtime_barrier() -> Idle request is still pending
>>                cancel_work_sync()  -> DSI host is *not* suspended!
>>
>>    Since the device is not suspended, the clock and regulator are never
>>    disabled. The imbalance causes a WARN at devres free time.
> 
> That's interesting. I guess this is shown when you have the panel as a
> module?

That's the easiest way to get sun6i_dsi_probe() to fail, yes. Even if the panel
was built-in `modprobe sun6i_dsi; rmmod sun6i_dsi` would likely trigger the
issue, since sun6i_dsi_remove() has the same problem.

> There's something pretty weird though. The comment in
> __pm_runtime_disable states that it will "wait for all operations in
> progress to complete" so at the end of __pm_runtime_disable call, the
> DSI host will be suspended and we shouldn't have a WARN at all.

No, that's not what "operations in progress" means. That only waits for a
callback that is *already running* on another CPU to complete, in other words
`dev->power.runtime_status == RPM_SUSPENDING`.

Here the callback does not get run at all. At the time __pm_runtime_disable() is
called:

dev->power.runtime_status == RPM_ACTIVE
dev->power.request == RPM_REQ_IDLE
dev->power.request_pending == true

because pm_runtime_put() calls rpm_idle() with the RPM_ASYNC flag.

And as I mentioned, that request is thrown away by __pm_runtime_barrier(). So
the device PM core is working as documented.

>> 3) The driver relies on being suspended when sun6i_dsi_encoder_enable()
>>    is called. The resume callback has a comment that says:
>>
>>       Some part of it can only be done once we get a number of
>>       lanes, see sun6i_dsi_inst_init
>>
>>    And then part of the resume callback only runs if dsi->device is not
>>    NULL (that is, if sun6i_dsi_attach() has been called). However, as
>>    the above call graph shows, the resume callback is guaranteed to be
>>    called before sun6i_dsi_attach(); it is called before child devices
>>    get their drivers attached.
> 
> Isn't it something that has been changed by your previous patch though?

No. Before the previous patch, sun6i_dsi_bind() requires sun6i_dsi_attach() to
have been called first. So either the panel driver is not loaded, and issue #2
happens, or the panel driver is loaded, and you get the following modification
to the above call graph:

   mipi_dsi_host_register()
      ...
         __device_attach()
            pm_runtime_get_sync(dev->parent) -> Causes resume
            bus_for_each_drv()
               __device_attach_driver()
                  [panel probe function]
                     mipi_dsi_attach()
                        sun6i_dsi_attach()
            pm_runtime_put(dev->parent) -> Async idle request
   component_add()
      ...
         sun6i_dsi_bind()
      ...
         sun6i_dsi_encoder_enable()
            pm_runtime_get_sync() -> Cancels idle request

And because `dev->power.runtime_status == RPM_ACTIVE` still, the callback is
*not* run. Either way you have the same problem.

>>    Therefore, part of the controller initialization will only run if the
>>    device is suspended between the calls to mipi_dsi_host_register() and
>>    component_add() (which ends up calling sun6i_dsi_encoder_enable()).
>>    Again, as shown by the above call graph, this is not the case. It
>>    appears that the controller happens to work because it is still
>>    initialized by the bootloader.
> 
> We don't have any bootloader support for MIPI-DSI, so no, that's not it.
> 
>>    Because the connector is hardcoded to always be connected, the
>>    device's runtime PM reference is not dropped until system suspend,
>>    when sun4i_drv_drm_sys_suspend() ends up calling
>>    sun6i_dsi_encoder_disable(). However, that is done as a system sleep
>>    PM hook, and at that point the system PM core has already taken
>>    another runtime PM reference, so sun6i_dsi_runtime_suspend() is
>>    not called. Likewise, by the time the PM core releases its reference,
>>    sun4i_drv_drm_sys_resume() has already re-enabled the encoder.
>>
>>    So after system suspend and resume, we have *still never called*
>>    sun6i_dsi_inst_init(), and now that the rest of the display pipeline
>>    has been reset, the DSI host is unable to communicate with the panel,
>>    causing VBLANK timeouts.
> 
> Either way, I guess just moving the pm_runtime_enable call to
> sun6i_dsi_attach will fix this, right? We don't really need to have
> the DSI controller powered up before that time anyway.

Sorry, but no again. It would solve issue #2 (only if the previous patch is
applied), but not issue #3.

Regardless of when runtime PM is enabled, sun6i_dsi_runtime_suspend() will not
be called until the device's usage count drops to 0. And as long as a panel is
bound, the controller's usage count will be >0, *even during system suspend*
while the encoder is turned off.

Before the previous patch, the usage count would never drop to 0 under *any*
circumstance.

>> Fix all of these issues by inlining the runtime PM hooks into the
>> encoder enable/disable functions, which are guaranteed to run after a
>> panel is attached. This allows sun6i_dsi_inst_init() to be called
>> unconditionally. Furthermore, this causes the hardware to be turned off
>> during system suspend and reinitialized on resume, which was not
>> happening before.
> 
> That's not something we should do really. We're really lacking any
> power management, so we should be having more of runtime_pm, not less.

This *is* adding more power management! The current runtime_pm hooks never
actually suspend the device, as described above. And even if they did work, this
would not extend the lifetime during which the device is active! I'm calling the
power-up/power-down routines at exactly the same point they were previously
getting called, except for two changes:

1) The device does not get powered up during mipi_dsi_host_register(), which you
just said was unnecessary.

2) The code in the PM hooks actually gets run when it was intended to be run.

> Maxime

Samuel

Re: [PATCH 4/4] drm/sun4i: dsi: Remove incorrect use of runtime PM

[Samuel Holland]

Maxime,

First, sorry for the tone in my previous message. I wrote it too hastily.

> On 2/11/20 2:26 AM, Maxime Ripard wrote:
>> On Tue, Feb 11, 2020 at 01:28:58AM -0600, Samuel Holland wrote:
>>> 3) The driver relies on being suspended when sun6i_dsi_encoder_enable()
>>>    is called. The resume callback has a comment that says:
>>>
>>>       Some part of it can only be done once we get a number of
>>>       lanes, see sun6i_dsi_inst_init
>>>
>>>    And then part of the resume callback only runs if dsi->device is not
>>>    NULL (that is, if sun6i_dsi_attach() has been called). However, as
>>>    the above call graph shows, the resume callback is guaranteed to be
>>>    called before sun6i_dsi_attach(); it is called before child devices
>>>    get their drivers attached.
>>
>> Isn't it something that has been changed by your previous patch though?
> 
> No. Before the previous patch, sun6i_dsi_bind() requires sun6i_dsi_attach() to
> have been called first. So either the panel driver is not loaded, and issue #2
> happens, or the panel driver is loaded, and you get the following modification
> to the above call graph:
> 
>    mipi_dsi_host_register()
>       ...
>          __device_attach()
>             pm_runtime_get_sync(dev->parent) -> Causes resume
>             bus_for_each_drv()
>                __device_attach_driver()
>                   [panel probe function]
>                      mipi_dsi_attach()
>                         sun6i_dsi_attach()
>             pm_runtime_put(dev->parent) -> Async idle request
>    component_add()
>       ...
>          sun6i_dsi_bind()
>       ...
>          sun6i_dsi_encoder_enable()
>             pm_runtime_get_sync() -> Cancels idle request
> 
> And because `dev->power.runtime_status == RPM_ACTIVE` still, the callback is
> *not* run. Either way you have the same problem.

While the scenario I described is possible (since an unbounded amount of other
work could be queued to pm_wq), I did more testing without these patches, and I
could never trigger it. No matter what combination of module/built-in drivers I
used, there was always enough time between mipi_dsi_host_register() and
sun6i_dsi_encoder_enable() for the device to be suspended. So in practice
sun6i_dsi_inst_init() was always called during boot.

>>>    Therefore, part of the controller initialization will only run if the
>>>    device is suspended between the calls to mipi_dsi_host_register() and
>>>    component_add() (which ends up calling sun6i_dsi_encoder_enable()).
>>>    Again, as shown by the above call graph, this is not the case. It
>>>    appears that the controller happens to work because it is still
>>>    initialized by the bootloader.
>>
>> We don't have any bootloader support for MIPI-DSI, so no, that's not it.

You are correct here. sun6i_dsi_inst_init() was indeed being called at boot. So
my commit log is wrong.

>>>    Because the connector is hardcoded to always be connected, the
>>>    device's runtime PM reference is not dropped until system suspend,
>>>    when sun4i_drv_drm_sys_suspend() ends up calling
>>>    sun6i_dsi_encoder_disable(). However, that is done as a system sleep
>>>    PM hook, and at that point the system PM core has already taken
>>>    another runtime PM reference, so sun6i_dsi_runtime_suspend() is
>>>    not called. Likewise, by the time the PM core releases its reference,
>>>    sun4i_drv_drm_sys_resume() has already re-enabled the encoder.
>>>
>>>    So after system suspend and resume, we have *still never called*
>>>    sun6i_dsi_inst_init(), and now that the rest of the display pipeline
>>>    has been reset, the DSI host is unable to communicate with the panel,
>>>    causing VBLANK timeouts.
>>
>> Either way, I guess just moving the pm_runtime_enable call to
>> sun6i_dsi_attach will fix this, right? We don't really need to have
>> the DSI controller powered up before that time anyway.
> 
> No. It would solve issue #2 (only if the previous patch is
> applied), but not issue #3.
> 
> Regardless of when runtime PM is enabled, sun6i_dsi_runtime_suspend() will not
> be called until the device's usage count drops to 0. And as long as a panel is
> bound, the controller's usage count will be >0, *even during system suspend*
> while the encoder is turned off.
> 
> Before the previous patch, the usage count would never drop to 0 under *any*
> circumstance.

FWIW,
Samuel

Re: [PATCH 4/4] drm/sun4i: dsi: Remove incorrect use of runtime PM

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4468 bytes --]

On Tue, Feb 11, 2020 at 01:28:58AM -0600, Samuel Holland wrote:
> The driver currently uses runtime PM to perform some of the module
> initialization and cleanup. This has three problems:
>
> 1) There is no Kconfig dependency on CONFIG_PM, so if runtime PM is
>    disabled, the driver will not work at all, since the module will
>    never be initialized.
>
> 2) The driver does not ensure that the device is suspended when
>    sun6i_dsi_probe() fails or when sun6i_dsi_remove() is called. It
>    simply disables runtime PM. From the docs of pm_runtime_disable():
>
>       The device can be either active or suspended after its runtime PM
>       has been disabled.
>
>    And indeed, the device will likely still be active if sun6i_dsi_probe
>    fails. For example, if the panel driver is not yet loaded, we have
>    the following sequence:
>
>    sun6i_dsi_probe()
>       pm_runtime_enable()
>       mipi_dsi_host_register()
>          of_mipi_dsi_device_add(child)
>             ...device_add()...
>                __device_attach()
>                  pm_runtime_get_sync(dev->parent) -> Causes resume
>                  bus_for_each_drv()
>                     __device_attach_driver() -> No match for panel
>                  pm_runtime_put(dev->parent) -> Async idle request
>       component_add()
>          __component_add()
>             try_to_bring_up_masters()
>                try_to_bring_up_master()
>                   sun4i_drv_bind()
>                      component_bind_all()
>                         component_bind()
>                            sun6i_dsi_bind() -> Fails with -EPROBE_DEFER
>       mipi_dsi_host_unregister()
>       pm_runtime_disable()
>          __pm_runtime_disable()
>             __pm_runtime_barrier() -> Idle request is still pending
>                cancel_work_sync()  -> DSI host is *not* suspended!
>
>    Since the device is not suspended, the clock and regulator are never
>    disabled. The imbalance causes a WARN at devres free time.
>
> 3) The driver relies on being suspended when sun6i_dsi_encoder_enable()
>    is called. The resume callback has a comment that says:
>
>       Some part of it can only be done once we get a number of
>       lanes, see sun6i_dsi_inst_init
>
>    And then part of the resume callback only runs if dsi->device is not
>    NULL (that is, if sun6i_dsi_attach() has been called). However, as
>    the above call graph shows, the resume callback is guaranteed to be
>    called before sun6i_dsi_attach(); it is called before child devices
>    get their drivers attached.
>
>    Therefore, part of the controller initialization will only run if the
>    device is suspended between the calls to mipi_dsi_host_register() and
>    component_add() (which ends up calling sun6i_dsi_encoder_enable()).
>    Again, as shown by the above call graph, this is not the case. It
>    appears that the controller happens to work because it is still
>    initialized by the bootloader.
>
>    Because the connector is hardcoded to always be connected, the
>    device's runtime PM reference is not dropped until system suspend,
>    when sun4i_drv_drm_sys_suspend() ends up calling
>    sun6i_dsi_encoder_disable(). However, that is done as a system sleep
>    PM hook, and at that point the system PM core has already taken
>    another runtime PM reference, so sun6i_dsi_runtime_suspend() is
>    not called. Likewise, by the time the PM core releases its reference,
>    sun4i_drv_drm_sys_resume() has already re-enabled the encoder.
>
>    So after system suspend and resume, we have *still never called*
>    sun6i_dsi_inst_init(), and now that the rest of the display pipeline
>    has been reset, the DSI host is unable to communicate with the panel,
>    causing VBLANK timeouts.
>
> Fix all of these issues by inlining the runtime PM hooks into the
> encoder enable/disable functions, which are guaranteed to run after a
> panel is attached. This allows sun6i_dsi_inst_init() to be called
> unconditionally. Furthermore, this causes the hardware to be turned off
> during system suspend and reinitialized on resume, which was not
> happening before.
>
> Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller support")
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Applied all 4 patches.

This one failed to apply for some reason (even though the context
looks similar) so I fixed the conflict by hand, you might want to
double check.

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]