[PATCH] OMAP: DSS: DPI: disable vt-switch on suspend/resume.

[PATCH] OMAP: DSS: DPI: disable vt-switch on suspend/resume.

[NeilBrown]

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


These devices do not need to return to non-graphic console
for suspend, so disable that option.
This means there is less work to do in the suspend/resume cycle,
making it smoother and cheaper.


Signed-off-by: NeilBrown <neil@brown.name>

--
Hi Tomi,
 I wonder if you would consider this patch too.  It works for me and
removes pointless vt switching.  I assume no-one would need or want that
switching on suspend/resume but I guess I cannot be certain.

Maybe a module-parameter could be used if there was any uncertainty.

thanks,
NeilBrown


diff --git a/drivers/video/fbdev/omap2/dss/dpi.c b/drivers/video/fbdev/omap2/dss/dpi.c
index f83e7b030249..4a29bab4ade3 100644
--- a/drivers/video/fbdev/omap2/dss/dpi.c
+++ b/drivers/video/fbdev/omap2/dss/dpi.c
@@ -32,6 +32,7 @@
 #include <linux/string.h>
 #include <linux/of.h>
 #include <linux/clk.h>
+#include <linux/suspend.h>
 
 #include <video/omapdss.h>
 
@@ -798,6 +799,8 @@ static int omap_dpi_probe(struct platform_device *pdev)
 	mutex_init(&dpi->lock);
 
 	dpi_init_output(pdev);
+	/* No need to vt_switch in suspend/resume */
+	pm_set_vt_switch(0);
 
 	return 0;
 }

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] OMAP: DSS: DPI: disable vt-switch on suspend/resume.

[Tomi Valkeinen]

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

On 25/02/15 11:37, NeilBrown wrote:
> 
> These devices do not need to return to non-graphic console
> for suspend, so disable that option.
> This means there is less work to do in the suspend/resume cycle,
> making it smoother and cheaper.
> 
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> 
> --
> Hi Tomi,
>  I wonder if you would consider this patch too.  It works for me and
> removes pointless vt switching.  I assume no-one would need or want that
> switching on suspend/resume but I guess I cannot be certain.
> 
> Maybe a module-parameter could be used if there was any uncertainty.

I was just yesterday picking patches from TI's kernel on top of
mainline, and there's a similar one for omapfb and for omapdrm. Here's
for omapfb:

http://git.ti.com/android-sdk/kernel-video/commit/5915d8744c927307987b12b14bc6aa37b3bac05c?format=patch

The patch in TI's kernel claims it's needed to make resume work. You're
saying it just optimizes suspend/resume. I hadn't picked this up
earlier, because I didn't understand why pm_set_vt_switch() would be
needed to make resume work. And never had time to study it, so I still
don't know what it's about...

Looking at the drivers/tty/vt/vt_ioctl.c, it sounds to me that we should
always do pm_set_vt_switch(0), as omapdss restores everything just fine
on resume. Although it makes me wonder how it works if there are two
display controllers, one needing the switch and the other not...

Your patch does the call in a bit odd place, so I'll be queuing the
patches for omapfb and omapdrm. Or actually, maybe the call could be
done in one place in omapdss driver, as you do, but in omap_dsshw_probe().

 Tomi



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

Re: [PATCH] OMAP: DSS: DPI: disable vt-switch on suspend/resume.

[NeilBrown]

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

On Wed, 25 Feb 2015 12:03:36 +0200 Tomi Valkeinen <tomi.valkeinen@ti.com>
wrote:

> On 25/02/15 11:37, NeilBrown wrote:
> > 
> > These devices do not need to return to non-graphic console
> > for suspend, so disable that option.
> > This means there is less work to do in the suspend/resume cycle,
> > making it smoother and cheaper.
> > 
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > 
> > --
> > Hi Tomi,
> >  I wonder if you would consider this patch too.  It works for me and
> > removes pointless vt switching.  I assume no-one would need or want that
> > switching on suspend/resume but I guess I cannot be certain.
> > 
> > Maybe a module-parameter could be used if there was any uncertainty.
> 
> I was just yesterday picking patches from TI's kernel on top of
> mainline, and there's a similar one for omapfb and for omapdrm. Here's
> for omapfb:
> 
> http://git.ti.com/android-sdk/kernel-video/commit/5915d8744c927307987b12b14bc6aa37b3bac05c?format=patch
> 
> The patch in TI's kernel claims it's needed to make resume work. You're
> saying it just optimizes suspend/resume. I hadn't picked this up
> earlier, because I didn't understand why pm_set_vt_switch() would be
> needed to make resume work. And never had time to study it, so I still
> don't know what it's about...
> 
> Looking at the drivers/tty/vt/vt_ioctl.c, it sounds to me that we should
> always do pm_set_vt_switch(0), as omapdss restores everything just fine
> on resume. Although it makes me wonder how it works if there are two
> display controllers, one needing the switch and the other not...

I wondered that too.  It would seem to make more sense for
pm_set_vt_switch(0) to be the default, that drivers which need the textmode
switch should call (1).  But I suspect there are "historical reasons" for the
current situation.

My experience is that suspend works just find without this setting, and I
don't even notice the vt switch, unless I have DEBUG_DRIVERS which slows
suspend/resume down so much (writing lots of test to a serial console) that
the transition is noticeable.
I made the patch because I think it is the right thing to do, rather than
because it fixed a particular symptom.

I suspect that if suspend doesn't work without this patch, then something very
different is being done in user-space.  Maybe some other display manager, not
X.  Maybe the X server is running on vt1 rather than vt7.



> 
> Your patch does the call in a bit odd place, so I'll be queuing the
> patches for omapfb and omapdrm. Or actually, maybe the call could be
> done in one place in omapdss driver, as you do, but in omap_dsshw_probe().

I just figured that it had to be in some 'probe' function somewhere - I have
no opinion about which one (maybe all?-).  I'm perfectly happy with it
appearing anywhere that affects omap dss devices.

One thing I was reminded while testing and may as well mention:
I usually blank my display before suspending (using
FBIOBLANK/FB_BLANK_POWERDOWN ioctls).  However if I don't then I get a
warning:

[   87.261077] WARNING: CPU: 0 PID: 1901 at ../drivers/video/fbdev/omap2/dss/dispc.c:558 dispc_mgr_go+0x20/0x8c()
[   87.261138] Modules linked in: ipv6 usb_f_ecm g_ether usb_f_rndis u_ether libcomposite configfs hso w1_bq27000 libertas_sdio libertas cfg80211 omap_hdq itg3200 ehci_omap ehci_hcd uio_pdrv_genirq uio
[   87.261169] CPU: 0 PID: 1901 Comm: Xorg Not tainted 4.0.0-rc1-gta04+ #538
[   87.261169] Hardware name: Generic OMAP36xx (Flattened Device Tree)
[   87.261199] [<c00137e0>] (unwind_backtrace) from [<c00113a4>] (show_stack+0x10/0x14)
[   87.261230] [<c00113a4>] (show_stack) from [<c0033938>] (warn_slowpath_common+0x80/0xa8)
[   87.261260] [<c0033938>] (warn_slowpath_common) from [<c0033978>] (warn_slowpath_null+0x18/0x1c)
[   87.261291] [<c0033978>] (warn_slowpath_null) from [<c0232538>] (dispc_mgr_go+0x20/0x8c)
[   87.261291] [<c0232538>] (dispc_mgr_go) from [<c023e30c>] (dss_set_go_bits+0xd4/0x100)
[   87.261322] [<c023e30c>] (dss_set_go_bits) from [<c023f308>] (omap_dss_mgr_apply+0x16c/0x1b8)
[   87.261352] [<c023f308>] (omap_dss_mgr_apply) from [<c0250790>] (omapfb_apply_changes+0x428/0x488)
[   87.261352] [<c0250790>] (omapfb_apply_changes) from [<c0251024>] (omapfb_set_par+0x74/0xb0)
[   87.261383] [<c0251024>] (omapfb_set_par) from [<c02281a4>] (fb_set_var+0x250/0x330)
[   87.261413] [<c02281a4>] (fb_set_var) from [<c021fa24>] (fbcon_blank+0x6c/0x260)
[   87.261444] [<c021fa24>] (fbcon_blank) from [<c0275464>] (do_unblank_screen+0xf8/0x19c)
[   87.261474] [<c0275464>] (do_unblank_screen) from [<c026c268>] (complete_change_console+0x50/0xcc)
[   87.261474] [<c026c268>] (complete_change_console) from [<c026ccd8>] (vt_ioctl+0x9f4/0x1238)
[   87.261505] [<c026ccd8>] (vt_ioctl) from [<c02621e8>] (tty_ioctl+0xc48/0xcac)
[   87.261535] [<c02621e8>] (tty_ioctl) from [<c00dc344>] (do_vfs_ioctl+0x5b0/0x61c)
[   87.261566] [<c00dc344>] (do_vfs_ioctl) from [<c00dc3e4>] (SyS_ioctl+0x34/0x58)
[   87.261566] [<c00dc3e4>] (SyS_ioctl) from [<c000ea40>] (ret_fast_syscall+0x0/0x34)

I think the Xserver is responding to a 'suspend' notification and is blanking
the screen, maybe at an awkward time.

I haven't looked into further details yet.  I don't really expect you to, but
I thought I would mention it.

Thanks,
NeilBrown


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] OMAP: DSS: DPI: disable vt-switch on suspend/resume.

[Pavel Machek]

Hi!

> > Looking at the drivers/tty/vt/vt_ioctl.c, it sounds to me that we should
> > always do pm_set_vt_switch(0), as omapdss restores everything just fine
> > on resume. Although it makes me wonder how it works if there are two
> > display controllers, one needing the switch and the other not...
> 
> I wondered that too.  It would seem to make more sense for
> pm_set_vt_switch(0) to be the default, that drivers which need the textmode
> switch should call (1).  But I suspect there are "historical reasons" for the
> current situation.

Well... historically, we did not have drivers for graphics cards --
kernel did not know how to handle it, only X knew. For cards that do
not have kernel driver, only X one, we need to switch vts.

For cards that have proper driver, we should be able to survive
without VT switch.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html