| Message ID | 1406269240-24622-1-git-send-email-shawn.guo@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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, > }, > }; >
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
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
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
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
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.
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
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.
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, }, };
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(+)