[PATCH] drm/tegra: dsi: Add suspend/resume support

[PATCH] drm/tegra: dsi: Add suspend/resume support

[Mark Zhang]

This patch adds the suspend/resume support for Tegra drm
driver by calling the corresponding DPMS functions.

Signed-off-by: Mark Zhang <markz-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Hi,

This patch hooks DSI driver's suspend/resume to implement the whole
display system's suspend/resume. I know this is a super ugly way,
but as we all know, Tegra DRM driver doesn't have a dedicate drm device
so honestly I didn't find a better way to do that.

Thanks,
Mark

 drivers/gpu/drm/tegra/dsi.c | 96 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 90 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index 33f67fd601c6..25cd0d93f392 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -61,6 +61,9 @@ struct tegra_dsi {
 	struct tegra_dsi *slave;
 };
 
+static int tegra_dsi_pad_calibrate(struct tegra_dsi *);
+static int tegra_dsi_ganged_setup(struct tegra_dsi *dsi);
+
 static inline struct tegra_dsi *
 host1x_client_to_dsi(struct host1x_client *client)
 {
@@ -805,6 +808,20 @@ static int tegra_output_dsi_setup_clock(struct tegra_output *output,
 
 	lanes = tegra_dsi_get_lanes(dsi);
 
+	err = tegra_dsi_pad_calibrate(dsi);
+	if (err < 0) {
+		dev_err(dsi->dev, "MIPI calibration failed: %d\n", err);
+		return err;
+	}
+	if (dsi->slave) {
+		err = tegra_dsi_pad_calibrate(dsi->slave);
+		if (err < 0) {
+			dev_err(dsi->slave->dev,
+				"MIPI calibration failed: %d\n", err);
+			return err;
+		}
+	}
+
 	err = tegra_dsi_get_muldiv(dsi->format, &mul, &div);
 	if (err < 0)
 		return err;
@@ -833,6 +850,13 @@ static int tegra_output_dsi_setup_clock(struct tegra_output *output,
 		dev_err(dsi->dev, "failed to set parent clock: %d\n", err);
 		return err;
 	}
+	if (dsi->slave) {
+		err = tegra_dsi_ganged_setup(dsi);
+		if (err < 0) {
+			dev_err(dsi->dev, "DSI ganged setup failed: %d\n", err);
+			return err;
+		}
+	}
 
 	err = clk_set_rate(dsi->clk_parent, plld);
 	if (err < 0) {
@@ -1470,12 +1494,6 @@ static int tegra_dsi_probe(struct platform_device *pdev)
 		goto disable_vdd;
 	}
 
-	err = tegra_dsi_pad_calibrate(dsi);
-	if (err < 0) {
-		dev_err(dsi->dev, "MIPI calibration failed: %d\n", err);
-		goto mipi_free;
-	}
-
 	dsi->host.ops = &tegra_dsi_host_ops;
 	dsi->host.dev = &pdev->dev;
 
@@ -1544,6 +1562,67 @@ static int tegra_dsi_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int tegra_dsi_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct tegra_dsi *dsi = platform_get_drvdata(pdev);
+	struct tegra_drm *tegra = dev_get_drvdata(dsi->client.parent);
+	struct drm_connector *connector;
+
+	if (dsi->master) {
+		regulator_disable(dsi->vdd);
+		return 0;
+	}
+
+	drm_modeset_lock_all(tegra->drm);
+	list_for_each_entry(connector, &tegra->drm->mode_config.connector_list,
+			    head) {
+		int old_dpms = connector->dpms;
+
+		if (connector->funcs->dpms)
+			connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);
+
+		/* Set the old mode back to the connector for resume */
+		connector->dpms = old_dpms;
+	}
+	drm_modeset_unlock_all(tegra->drm);
+
+	regulator_disable(dsi->vdd);
+
+	return 0;
+}
+
+static int tegra_dsi_resume(struct platform_device *pdev)
+{
+	struct tegra_dsi *dsi = platform_get_drvdata(pdev);
+	struct tegra_drm *tegra = dev_get_drvdata(dsi->client.parent);
+	struct drm_connector *connector;
+	int err = 0;
+
+	err = regulator_enable(dsi->vdd);
+	if (err < 0) {
+		dev_err(&pdev->dev, "Enable DSI vdd failed: %d\n", err);
+		return err;
+	}
+
+	if (dsi->master)
+		return 0;
+
+	drm_modeset_lock_all(tegra->drm);
+	list_for_each_entry(connector, &tegra->drm->mode_config.connector_list,
+			    head) {
+		if (connector->funcs->dpms)
+			connector->funcs->dpms(connector, connector->dpms);
+	}
+	drm_modeset_unlock_all(tegra->drm);
+
+	drm_helper_resume_force_mode(tegra->drm);
+
+	return 0;
+}
+#endif
+
+
 static const struct of_device_id tegra_dsi_of_match[] = {
 	{ .compatible = "nvidia,tegra114-dsi", },
 	{ },
@@ -1557,4 +1636,9 @@ struct platform_driver tegra_dsi_driver = {
 	},
 	.probe = tegra_dsi_probe,
 	.remove = tegra_dsi_remove,
+#ifdef CONFIG_PM
+	.suspend = tegra_dsi_suspend,
+	.resume = tegra_dsi_resume,
+#endif
+
 };
-- 
1.8.1.5

Re: [PATCH] drm/tegra: dsi: Add suspend/resume support

[Sean Paul]

On Sun, Dec 7, 2014 at 10:40 PM, Mark Zhang <markz@nvidia.com> wrote:
> This patch adds the suspend/resume support for Tegra drm
> driver by calling the corresponding DPMS functions.
>
> Signed-off-by: Mark Zhang <markz@nvidia.com>
> ---
> Hi,
>
> This patch hooks DSI driver's suspend/resume to implement the whole
> display system's suspend/resume. I know this is a super ugly way,
> but as we all know, Tegra DRM driver doesn't have a dedicate drm device
> so honestly I didn't find a better way to do that.
>
> Thanks,
> Mark
>

Hi Mark,
Thanks for posting this. I've reproduced my Gerrit comments from the
CrOS tree below for the benefit of others.

>  drivers/gpu/drm/tegra/dsi.c | 96 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 90 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index 33f67fd601c6..25cd0d93f392 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -61,6 +61,9 @@ struct tegra_dsi {
>         struct tegra_dsi *slave;
>  };
>
> +static int tegra_dsi_pad_calibrate(struct tegra_dsi *);
> +static int tegra_dsi_ganged_setup(struct tegra_dsi *dsi);
> +
>  static inline struct tegra_dsi *
>  host1x_client_to_dsi(struct host1x_client *client)
>  {
> @@ -805,6 +808,20 @@ static int tegra_output_dsi_setup_clock(struct tegra_output *output,
>
>         lanes = tegra_dsi_get_lanes(dsi);
>
> +       err = tegra_dsi_pad_calibrate(dsi);
> +       if (err < 0) {
> +               dev_err(dsi->dev, "MIPI calibration failed: %d\n", err);
> +               return err;
> +       }
> +       if (dsi->slave) {
> +               err = tegra_dsi_pad_calibrate(dsi->slave);
> +               if (err < 0) {
> +                       dev_err(dsi->slave->dev,
> +                               "MIPI calibration failed: %d\n", err);
> +                       return err;
> +               }
> +       }

Move this duplicate call into tegra_dsi_pad_calibrate() to be
consistent with the other functions in this file (eg:
tegra_dsi_set_timeout).

> +
>         err = tegra_dsi_get_muldiv(dsi->format, &mul, &div);
>         if (err < 0)
>                 return err;
> @@ -833,6 +850,13 @@ static int tegra_output_dsi_setup_clock(struct tegra_output *output,
>                 dev_err(dsi->dev, "failed to set parent clock: %d\n", err);
>                 return err;
>         }
> +       if (dsi->slave) {
> +               err = tegra_dsi_ganged_setup(dsi);
> +               if (err < 0) {
> +                       dev_err(dsi->dev, "DSI ganged setup failed: %d\n", err);
> +                       return err;
> +               }
> +       }

I think this belongs in ->enable() instead of here.

>
>         err = clk_set_rate(dsi->clk_parent, plld);
>         if (err < 0) {
> @@ -1470,12 +1494,6 @@ static int tegra_dsi_probe(struct platform_device *pdev)
>                 goto disable_vdd;
>         }
>
> -       err = tegra_dsi_pad_calibrate(dsi);
> -       if (err < 0) {
> -               dev_err(dsi->dev, "MIPI calibration failed: %d\n", err);
> -               goto mipi_free;
> -       }
> -
>         dsi->host.ops = &tegra_dsi_host_ops;
>         dsi->host.dev = &pdev->dev;
>
> @@ -1544,6 +1562,67 @@ static int tegra_dsi_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +#ifdef CONFIG_PM
> +static int tegra_dsi_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +       struct tegra_dsi *dsi = platform_get_drvdata(pdev);
> +       struct tegra_drm *tegra = dev_get_drvdata(dsi->client.parent);
> +       struct drm_connector *connector;
> +
> +       if (dsi->master) {
> +               regulator_disable(dsi->vdd);
> +               return 0;
> +       }
> +
> +       drm_modeset_lock_all(tegra->drm);
> +       list_for_each_entry(connector, &tegra->drm->mode_config.connector_list,
> +                           head) {
> +               int old_dpms = connector->dpms;
> +
> +               if (connector->funcs->dpms)
> +                       connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);
> +
> +               /* Set the old mode back to the connector for resume */
> +               connector->dpms = old_dpms;
> +       }
> +       drm_modeset_unlock_all(tegra->drm);
> +
> +       regulator_disable(dsi->vdd);
> +
> +       return 0;
> +}
> +
> +static int tegra_dsi_resume(struct platform_device *pdev)
> +{
> +       struct tegra_dsi *dsi = platform_get_drvdata(pdev);
> +       struct tegra_drm *tegra = dev_get_drvdata(dsi->client.parent);
> +       struct drm_connector *connector;
> +       int err = 0;
> +
> +       err = regulator_enable(dsi->vdd);
> +       if (err < 0) {
> +               dev_err(&pdev->dev, "Enable DSI vdd failed: %d\n", err);
> +               return err;
> +       }
> +
> +       if (dsi->master)
> +               return 0;
> +
> +       drm_modeset_lock_all(tegra->drm);
> +       list_for_each_entry(connector, &tegra->drm->mode_config.connector_list,
> +                           head) {
> +               if (connector->funcs->dpms)
> +                       connector->funcs->dpms(connector, connector->dpms);
> +       }
> +       drm_modeset_unlock_all(tegra->drm);
> +
> +       drm_helper_resume_force_mode(tegra->drm);
> +
> +       return 0;
> +}
> +#endif

So this is the tricky chunk, it definitely does not belong here. I
think it makes most sense for this to live in drm.c, however
host1x_driver doesn't have suspend/resume hooks.

I suspect the correct thing to do here is to add them to
host1x_driver, but I'm not sure how much effort is involved in doing
that.

Sean

> +
> +
>  static const struct of_device_id tegra_dsi_of_match[] = {
>         { .compatible = "nvidia,tegra114-dsi", },
>         { },
> @@ -1557,4 +1636,9 @@ struct platform_driver tegra_dsi_driver = {
>         },
>         .probe = tegra_dsi_probe,
>         .remove = tegra_dsi_remove,
> +#ifdef CONFIG_PM
> +       .suspend = tegra_dsi_suspend,
> +       .resume = tegra_dsi_resume,
> +#endif
> +
>  };
> --
> 1.8.1.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tegra: dsi: Add suspend/resume support

[Mark Zhang]

On 12/10/2014 03:29 AM, Sean Paul wrote:
> On Sun, Dec 7, 2014 at 10:40 PM, Mark Zhang <markz-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> This patch adds the suspend/resume support for Tegra drm
>> driver by calling the corresponding DPMS functions.
[...]
>> +       if (dsi->slave) {
>> +               err = tegra_dsi_pad_calibrate(dsi->slave);
>> +               if (err < 0) {
>> +                       dev_err(dsi->slave->dev,
>> +                               "MIPI calibration failed: %d\n", err);
>> +                       return err;
>> +               }
>> +       }
> 
> Move this duplicate call into tegra_dsi_pad_calibrate() to be
> consistent with the other functions in this file (eg:
> tegra_dsi_set_timeout).
> 

Yeah, will do.

>> +
>>         err = tegra_dsi_get_muldiv(dsi->format, &mul, &div);
>>         if (err < 0)
>>                 return err;
>> @@ -833,6 +850,13 @@ static int tegra_output_dsi_setup_clock(struct tegra_output *output,
>>                 dev_err(dsi->dev, "failed to set parent clock: %d\n", err);
>>                 return err;
>>         }
>> +       if (dsi->slave) {
>> +               err = tegra_dsi_ganged_setup(dsi);
>> +               if (err < 0) {
>> +                       dev_err(dsi->dev, "DSI ganged setup failed: %d\n", err);
>> +                       return err;
>> +               }
>> +       }
> 
> I think this belongs in ->enable() instead of here.
> 

Actually "tegra_dsi_ganged_setup" is not needed any more. This function
ensures that DSIA & DSIB use the same PLL in ganged mode. But if you
read the Tegra124/Tegra132 TRM, you'll find there is no way to select
the clock source for DSIB. I.E, DSIB always uses the same clock source
with DSIA. Besides, PLLD is the only clock source for DSIA. I.E,
"clk_get_parent"/"clk_set_parent" doesn't make sense for dsia & dsib
clock now.

I've posted a patch(in tegra clock driver) to fix this:

http://article.gmane.org/gmane.linux.ports.tegra/20313

And the corresponding changes in dsi driver will arrive soon.

>>
>>         err = clk_set_rate(dsi->clk_parent, plld);
>>         if (err < 0) {
[...]
>> +
>> +       drm_modeset_lock_all(tegra->drm);
>> +       list_for_each_entry(connector, &tegra->drm->mode_config.connector_list,
>> +                           head) {
>> +               if (connector->funcs->dpms)
>> +                       connector->funcs->dpms(connector, connector->dpms);
>> +       }
>> +       drm_modeset_unlock_all(tegra->drm);
>> +
>> +       drm_helper_resume_force_mode(tegra->drm);
>> +
>> +       return 0;
>> +}
>> +#endif
> 
> So this is the tricky chunk, it definitely does not belong here. I
> think it makes most sense for this to live in drm.c, however
> host1x_driver doesn't have suspend/resume hooks.
> 

Agree. drm.c is the best place for putting this.

> I suspect the correct thing to do here is to add them to
> host1x_driver, but I'm not sure how much effort is involved in doing
> that.
> 

I thought about this before. If we do it in host1x driver, IIUC, it
means that when host1x starts suspending, it will suspend all host1x
client devices, right? Honestly I feel this is not a good thing despite
I can't tell what's the problem in this right now...

Mark
> Sean
> 
>> +
>> +
>>  static const struct of_device_id tegra_dsi_of_match[] = {
>>         { .compatible = "nvidia,tegra114-dsi", },
>>         { },
>> @@ -1557,4 +1636,9 @@ struct platform_driver tegra_dsi_driver = {
>>         },
>>         .probe = tegra_dsi_probe,
>>         .remove = tegra_dsi_remove,
>> +#ifdef CONFIG_PM
>> +       .suspend = tegra_dsi_suspend,
>> +       .resume = tegra_dsi_resume,
>> +#endif
>> +
>>  };
>> --
>> 1.8.1.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] drm/tegra: dsi: Add suspend/resume support

[Thierry Reding]

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

On Wed, Dec 10, 2014 at 10:08:57AM +0800, Mark Zhang wrote:
> On 12/10/2014 03:29 AM, Sean Paul wrote:
> > On Sun, Dec 7, 2014 at 10:40 PM, Mark Zhang <markz-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
[...]
> >>
> >>         err = clk_set_rate(dsi->clk_parent, plld);
> >>         if (err < 0) {
> [...]
> >> +
> >> +       drm_modeset_lock_all(tegra->drm);
> >> +       list_for_each_entry(connector, &tegra->drm->mode_config.connector_list,
> >> +                           head) {
> >> +               if (connector->funcs->dpms)
> >> +                       connector->funcs->dpms(connector, connector->dpms);
> >> +       }
> >> +       drm_modeset_unlock_all(tegra->drm);
> >> +
> >> +       drm_helper_resume_force_mode(tegra->drm);
> >> +
> >> +       return 0;
> >> +}
> >> +#endif
> > 
> > So this is the tricky chunk, it definitely does not belong here. I
> > think it makes most sense for this to live in drm.c, however
> > host1x_driver doesn't have suspend/resume hooks.
> > 
> 
> Agree. drm.c is the best place for putting this.
> 
> > I suspect the correct thing to do here is to add them to
> > host1x_driver, but I'm not sure how much effort is involved in doing
> > that.
> > 
> 
> I thought about this before. If we do it in host1x driver, IIUC, it
> means that when host1x starts suspending, it will suspend all host1x
> client devices, right? Honestly I feel this is not a good thing despite
> I can't tell what's the problem in this right now...

I've just sent out patches for review that add the missing
infrastructure to properly do suspend/resume. The idea behind this is
that the DRM host1x device's ->pm_ops are responsible for dealing with
the suspend/resume at a subsystem level (call connectors' ->dpms()
callbacks, etc.) whereas more fine-grained suspend/resume can further be
done in the ->pm_ops of the individual subdevices. The infrastructure
makes sure that these get called in the right order.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]