[PATCH v2] imx-drm: imx-drm-core: add suspend/resume support

[PATCH v2] imx-drm: imx-drm-core: add suspend/resume support

[Shawn Guo]

HDMI currently stops working after a system suspend/resume cycle.  The
cause is that the mode setting states in hardware gets lost and isn't
restored across the suspend/resume cycle.

The patch adds a very basic suspend/resume support to imx-drm driver,
and calls drm_helper_resume_force_mode() in .resume hook to restore the
mode setting states, so that HDMI can continue working after a system
suspend/resume cycle.

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
Changs since v1:
 - Do not walk through connector->funcs->dpms() but only call
   drm_helper_resume_force_mode() in .resume.

 drivers/staging/imx-drm/imx-drm-core.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
index def8280d7ee6..ab41152089a3 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -696,6 +696,29 @@ static int imx_drm_platform_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int imx_drm_suspend(struct device *dev)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+
+	drm_kms_helper_poll_disable(drm_dev);
+
+	return 0;
+}
+
+static int imx_drm_resume(struct device *dev)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+
+	drm_helper_resume_force_mode(drm_dev);
+	drm_kms_helper_poll_enable(drm_dev);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(imx_drm_pm_ops, imx_drm_suspend, imx_drm_resume);
+
 static const struct of_device_id imx_drm_dt_ids[] = {
 	{ .compatible = "fsl,imx-display-subsystem", },
 	{ /* sentinel */ },
@@ -708,6 +731,7 @@ static struct platform_driver imx_drm_pdrv = {
 	.driver		= {
 		.owner	= THIS_MODULE,
 		.name	= "imx-drm",
+		.pm	= &imx_drm_pm_ops,
 		.of_match_table = imx_drm_dt_ids,
 	},
 };
-- 
1.9.1

[PATCH v2] imx-drm: imx-drm-core: add suspend/resume support

[Shawn Guo]

On Fri, Jul 25, 2014 at 02:20:40PM +0800, Shawn Guo wrote:
> HDMI currently stops working after a system suspend/resume cycle.  The
> cause is that the mode setting states in hardware gets lost and isn't
> restored across the suspend/resume cycle.
> 
> The patch adds a very basic suspend/resume support to imx-drm driver,
> and calls drm_helper_resume_force_mode() in .resume hook to restore the
> mode setting states, so that HDMI can continue working after a system
> suspend/resume cycle.
> 
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>

Hi Russell,

What's your take on this patch?

Shawn

> ---
> Changs since v1:
>  - Do not walk through connector->funcs->dpms() but only call
>    drm_helper_resume_force_mode() in .resume.
> 
>  drivers/staging/imx-drm/imx-drm-core.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
> index def8280d7ee6..ab41152089a3 100644
> --- a/drivers/staging/imx-drm/imx-drm-core.c
> +++ b/drivers/staging/imx-drm/imx-drm-core.c
> @@ -696,6 +696,29 @@ static int imx_drm_platform_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int imx_drm_suspend(struct device *dev)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +
> +	drm_kms_helper_poll_disable(drm_dev);
> +
> +	return 0;
> +}
> +
> +static int imx_drm_resume(struct device *dev)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +
> +	drm_helper_resume_force_mode(drm_dev);
> +	drm_kms_helper_poll_enable(drm_dev);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(imx_drm_pm_ops, imx_drm_suspend, imx_drm_resume);
> +
>  static const struct of_device_id imx_drm_dt_ids[] = {
>  	{ .compatible = "fsl,imx-display-subsystem", },
>  	{ /* sentinel */ },
> @@ -708,6 +731,7 @@ static struct platform_driver imx_drm_pdrv = {
>  	.driver		= {
>  		.owner	= THIS_MODULE,
>  		.name	= "imx-drm",
> +		.pm	= &imx_drm_pm_ops,
>  		.of_match_table = imx_drm_dt_ids,
>  	},
>  };
> -- 
> 1.9.1
> 

[PATCH v2] imx-drm: imx-drm-core: add suspend/resume support

[Russell King - ARM Linux]

On Mon, Aug 18, 2014 at 02:25:34PM +0800, Shawn Guo wrote:
> On Fri, Jul 25, 2014 at 02:20:40PM +0800, Shawn Guo wrote:
> > HDMI currently stops working after a system suspend/resume cycle.  The
> > cause is that the mode setting states in hardware gets lost and isn't
> > restored across the suspend/resume cycle.
> > 
> > The patch adds a very basic suspend/resume support to imx-drm driver,
> > and calls drm_helper_resume_force_mode() in .resume hook to restore the
> > mode setting states, so that HDMI can continue working after a system
> > suspend/resume cycle.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> 
> Hi Russell,
> 
> What's your take on this patch?

I've mostly stepped away from collecting imx-drm patches for Greg, and
I'm probably going to be dropping those patches which I had queued up
a while back for Greg.

Greg wishes me to use signed tags for pull requests.  This is not how I
work, and would mean having to rewrite a bunch of my scripts to cater
for Greg's special case.  No one else requires this.

I haven't had the time to start looking at what this would require
during the last three months, so the only other solution is to step
down from dealing with imx-drm patches.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v2] imx-drm: imx-drm-core: add suspend/resume support

[Greg KH]

On Tue, Aug 19, 2014 at 09:08:08AM +0100, Russell King - ARM Linux wrote:
> On Mon, Aug 18, 2014 at 02:25:34PM +0800, Shawn Guo wrote:
> > On Fri, Jul 25, 2014 at 02:20:40PM +0800, Shawn Guo wrote:
> > > HDMI currently stops working after a system suspend/resume cycle.  The
> > > cause is that the mode setting states in hardware gets lost and isn't
> > > restored across the suspend/resume cycle.
> > > 
> > > The patch adds a very basic suspend/resume support to imx-drm driver,
> > > and calls drm_helper_resume_force_mode() in .resume hook to restore the
> > > mode setting states, so that HDMI can continue working after a system
> > > suspend/resume cycle.
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > 
> > Hi Russell,
> > 
> > What's your take on this patch?
> 
> I've mostly stepped away from collecting imx-drm patches for Greg, and
> I'm probably going to be dropping those patches which I had queued up
> a while back for Greg.
> 
> Greg wishes me to use signed tags for pull requests.  This is not how I
> work, and would mean having to rewrite a bunch of my scripts to cater
> for Greg's special case.  No one else requires this.

"special case"?  That's what Linus wants from everyone as well, how hard
is it to add a simple line to your scripts that does:
	git tag -u ${KEY} -s ${TREE} ${BRANCH}

before doing:
	git request-pull .... ${TREE}

It's just a simple one line addition, right?  What am I missing here?

> I haven't had the time to start looking at what this would require
> during the last three months, so the only other solution is to step
> down from dealing with imx-drm patches.

I never said I would refuse to take patches from you if you didn't do
this, I only asked if you could, as it makes me feel more comfortable
taking pull requests from you as you are the only one that doesn't do
this for trees I pull.

You should be using signed tags for your other trees as well, it's not
just me who should be asking you for this...

thanks,

greg k-h

[PATCH v2] imx-drm: imx-drm-core: add suspend/resume support

[Russell King - ARM Linux]

On Tue, Aug 19, 2014 at 04:16:41AM -0500, Greg KH wrote:
> On Tue, Aug 19, 2014 at 09:08:08AM +0100, Russell King - ARM Linux wrote:
> > On Mon, Aug 18, 2014 at 02:25:34PM +0800, Shawn Guo wrote:
> > > On Fri, Jul 25, 2014 at 02:20:40PM +0800, Shawn Guo wrote:
> > > > HDMI currently stops working after a system suspend/resume cycle.  The
> > > > cause is that the mode setting states in hardware gets lost and isn't
> > > > restored across the suspend/resume cycle.
> > > > 
> > > > The patch adds a very basic suspend/resume support to imx-drm driver,
> > > > and calls drm_helper_resume_force_mode() in .resume hook to restore the
> > > > mode setting states, so that HDMI can continue working after a system
> > > > suspend/resume cycle.
> > > > 
> > > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > > 
> > > Hi Russell,
> > > 
> > > What's your take on this patch?
> > 
> > I've mostly stepped away from collecting imx-drm patches for Greg, and
> > I'm probably going to be dropping those patches which I had queued up
> > a while back for Greg.
> > 
> > Greg wishes me to use signed tags for pull requests.  This is not how I
> > work, and would mean having to rewrite a bunch of my scripts to cater
> > for Greg's special case.  No one else requires this.
> 
> "special case"?  That's what Linus wants from everyone as well, how hard
> is it to add a simple line to your scripts that does:
> 	git tag -u ${KEY} -s ${TREE} ${BRANCH}

Linus is perfectly happy with my pull requests as is.  That's because he
knows their format (which pre-dates git) and it includes protection
against modification of the requested git tree by including the SHA
hash of the top most commit, which was agreed at the time of the
kernel.org break-in as an acceptable alternative approach to signed
tags.

> before doing:
> 	git request-pull .... ${TREE}
> 
> It's just a simple one line addition, right?  What am I missing here?

I don't use git request-pull.  I have my own suite of scripts which
generate a set of formatted patches in mbox format, combined patches,
and the pull request email template, and uploading the appropriate
branches and patches to the server.  They're also more than 10 years
old, which shows how long Linus has been happy with my process.

> You should be using signed tags for your other trees as well, it's not
> just me who should be asking you for this...

You are the only one who has been asking me for it.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v2] imx-drm: imx-drm-core: add suspend/resume support

[Andrzej Hajda]

Hi Shawn,


On 07/25/2014 08:20 AM, Shawn Guo wrote:
> HDMI currently stops working after a system suspend/resume cycle.  The
> cause is that the mode setting states in hardware gets lost and isn't
> restored across the suspend/resume cycle.
> 
> The patch adds a very basic suspend/resume support to imx-drm driver,
> and calls drm_helper_resume_force_mode() in .resume hook to restore the
> mode setting states, so that HDMI can continue working after a system
> suspend/resume cycle.
> 
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> ---
> Changs since v1:
>  - Do not walk through connector->funcs->dpms() but only call
>    drm_helper_resume_force_mode() in .resume.
> 
>  drivers/staging/imx-drm/imx-drm-core.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
> index def8280d7ee6..ab41152089a3 100644
> --- a/drivers/staging/imx-drm/imx-drm-core.c
> +++ b/drivers/staging/imx-drm/imx-drm-core.c
> @@ -696,6 +696,29 @@ static int imx_drm_platform_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int imx_drm_suspend(struct device *dev)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +
> +	drm_kms_helper_poll_disable(drm_dev);

drm_dev can be NULL here. You should add check before.

> +
> +	return 0;
> +}
> +
> +static int imx_drm_resume(struct device *dev)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +
> +	drm_helper_resume_force_mode(drm_dev);

ditto

Regards
Andrzej

> +	drm_kms_helper_poll_enable(drm_dev);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(imx_drm_pm_ops, imx_drm_suspend, imx_drm_resume);
> +
>  static const struct of_device_id imx_drm_dt_ids[] = {
>  	{ .compatible = "fsl,imx-display-subsystem", },
>  	{ /* sentinel */ },
> @@ -708,6 +731,7 @@ static struct platform_driver imx_drm_pdrv = {
>  	.driver		= {
>  		.owner	= THIS_MODULE,
>  		.name	= "imx-drm",
> +		.pm	= &imx_drm_pm_ops,
>  		.of_match_table = imx_drm_dt_ids,
>  	},
>  };
> 

[PATCH v2] imx-drm: imx-drm-core: add suspend/resume support

[Shawn Guo]

Hi Andrzej,

On Mon, Aug 18, 2014 at 09:43:19AM +0200, Andrzej Hajda wrote:
> > diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
> > index def8280d7ee6..ab41152089a3 100644
> > --- a/drivers/staging/imx-drm/imx-drm-core.c
> > +++ b/drivers/staging/imx-drm/imx-drm-core.c
> > @@ -696,6 +696,29 @@ static int imx_drm_platform_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_PM_SLEEP
> > +static int imx_drm_suspend(struct device *dev)
> > +{
> > +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> > +
> > +	drm_kms_helper_poll_disable(drm_dev);
> 
> drm_dev can be NULL here. You should add check before.

The drvdata is set to drm_dev in drm_driver .load() hook.  So are you
saying that .suspend() hook could possibly be called before the .load()
hook gets called?

Shawn

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_drm_resume(struct device *dev)
> > +{
> > +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> > +
> > +	drm_helper_resume_force_mode(drm_dev);
> 
> ditto

[PATCH v2] imx-drm: imx-drm-core: add suspend/resume support

[Andrzej Hajda]

On 08/18/2014 02:38 PM, Shawn Guo wrote:
> Hi Andrzej,
>
> On Mon, Aug 18, 2014 at 09:43:19AM +0200, Andrzej Hajda wrote:
>>> diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
>>> index def8280d7ee6..ab41152089a3 100644
>>> --- a/drivers/staging/imx-drm/imx-drm-core.c
>>> +++ b/drivers/staging/imx-drm/imx-drm-core.c
>>> @@ -696,6 +696,29 @@ static int imx_drm_platform_remove(struct platform_device *pdev)
>>>  	return 0;
>>>  }
>>>  
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int imx_drm_suspend(struct device *dev)
>>> +{
>>> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
>>> +
>>> +	drm_kms_helper_poll_disable(drm_dev);
>> drm_dev can be NULL here. You should add check before.
> The drvdata is set to drm_dev in drm_driver .load() hook.  So are you
> saying that .suspend() hook could possibly be called before the .load()
> hook gets called?

.load hook is called after all components are attached to the master.
So if suspend happen after probe of the master and before attaching the last
component you will have NULL here, I guess.

Regards
Andrzej

>
> Shawn
>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int imx_drm_resume(struct device *dev)
>>> +{
>>> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
>>> +
>>> +	drm_helper_resume_force_mode(drm_dev);
>> ditto

[PATCH v2] imx-drm: imx-drm-core: add suspend/resume support

[Andrzej Hajda]

On 08/18/2014 02:58 PM, Andrzej Hajda wrote:
> On 08/18/2014 02:38 PM, Shawn Guo wrote:
>> Hi Andrzej,
>>
>> On Mon, Aug 18, 2014 at 09:43:19AM +0200, Andrzej Hajda wrote:
>>>> diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
>>>> index def8280d7ee6..ab41152089a3 100644
>>>> --- a/drivers/staging/imx-drm/imx-drm-core.c
>>>> +++ b/drivers/staging/imx-drm/imx-drm-core.c
>>>> @@ -696,6 +696,29 @@ static int imx_drm_platform_remove(struct platform_device *pdev)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +#ifdef CONFIG_PM_SLEEP
>>>> +static int imx_drm_suspend(struct device *dev)
>>>> +{
>>>> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
>>>> +
>>>> +	drm_kms_helper_poll_disable(drm_dev);
>>> drm_dev can be NULL here. You should add check before.
>> The drvdata is set to drm_dev in drm_driver .load() hook.  So are you
>> saying that .suspend() hook could possibly be called before the .load()
>> hook gets called?
> .load hook is called after all components are attached to the master.
> So if suspend happen after probe of the master and before attaching the last
> component you will have NULL here, I guess.

Probably you can test it by disabling driver for one of drm components
and putting board in sleep mode.

Moreover I guess drvdata should be cleaned in .unload callback,
otherwise if for some reason one of components will be detached and suspend
happens drvdata will contain invalid pointer.

Regards
Andrzej

>
> Regards
> Andrzej
>
>> Shawn
>>
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int imx_drm_resume(struct device *dev)
>>>> +{
>>>> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
>>>> +
>>>> +	drm_helper_resume_force_mode(drm_dev);
>>> ditto

[PATCH v2] imx-drm: imx-drm-core: add suspend/resume support

[Shawn Guo]

On Mon, Aug 18, 2014 at 03:11:54PM +0200, Andrzej Hajda wrote:
> On 08/18/2014 02:58 PM, Andrzej Hajda wrote:
> > .load hook is called after all components are attached to the master.
> > So if suspend happen after probe of the master and before attaching the last
> > component you will have NULL here, I guess.
> 
> Probably you can test it by disabling driver for one of drm components
> and putting board in sleep mode.
> 
> Moreover I guess drvdata should be cleaned in .unload callback,
> otherwise if for some reason one of components will be detached and suspend
> happens drvdata will contain invalid pointer.

You're right, Andrzej.  I will fix it in v3.  Thanks for the comment.

Shawn