| Message ID | 20150225203708.0eb2fa96@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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; }
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