From: "Rafael J. Wysocki" <rjw@sisk.pl>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Zhang Rui <rui.zhang@intel.com>,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
linux-acpi@vger.kernel.org, intel-gfx@lists.freedesktop.org,
lenb@kernel.org
Subject: Re: [Intel-gfx] [PATCH V2 3/3] i915: ignore lid open event when resuming
Date: Tue, 05 Feb 2013 11:14:15 +0100 [thread overview]
Message-ID: <3004369.zSvdplM23x@vostro.rjw.lan> (raw)
In-Reply-To: <20130205100711.GB5813@phenom.ffwll.local>
On Tuesday, February 05, 2013 11:07:11 AM Daniel Vetter wrote:
> On Tue, Feb 05, 2013 at 03:41:53PM +0800, Zhang Rui wrote:
> > oops, forgot to update the changelog and comments.
> > refreshed patch attached.
> >
> > From b584fcebb6d715a317f192c88606b875ee88ce78 Mon Sep 17 00:00:00 2001
> > From: Zhang Rui <rui.zhang@intel.com>
> > Date: Thu, 24 Jan 2013 13:27:22 +0800
> > Subject: [PATCH V3 3/3] i915: ignore lid open event when resuming
> >
> > i915 driver needs to do modeset when
> > 1. system resumes from sleep
> > 2. lid is opened
>
> Patch applied, thanks. There's been a bit of a merge conflict and one tiny
> checkpatch error, both fixed while applying. I plan to push this patch to
> drm-next for 3.9.
Thanks Daniel!
I will take the other patches from the series, then.
Rafael
> > In PM_SUSPEND_MEM state, all the GPEs are cleared when system resumes,
> > thus it is the i915_resume code does the modeset rather than intel_lid_notify().
> >
> > But in PM_SUSPEND_FREEZE state, this will be broken because
> > system is still responsive to the lid events.
> > 1. When we close the lid in Freeze state, intel_lid_notify() sets modeset_on_lid.
> > 2. When we reopen the lid, intel_lid_notify() will do a modeset,
> > before the system is resumed.
> > here is the error log,
> >
> > [92146.548074] WARNING: at drivers/gpu/drm/i915/intel_display.c:1028 intel_wait_for_pipe_off+0x184/0x190 [i915]()
> > [92146.548076] Hardware name: VGN-Z540N
> > [92146.548078] pipe_off wait timed out
> > [92146.548167] Modules linked in: hid_generic usbhid hid snd_hda_codec_realtek snd_hda_intel snd_hda_codec parport_pc snd_hwdep ppdev snd_pcm_oss i915 snd_mixer_oss snd_pcm arc4 iwldvm snd_seq_dummy mac80211 snd_seq_oss snd_seq_midi fbcon tileblit font bitblit softcursor drm_kms_helper snd_rawmidi snd_seq_midi_event coretemp drm snd_seq kvm btusb bluetooth snd_timer iwlwifi pcmcia tpm_infineon i2c_algo_bit joydev snd_seq_device intel_agp cfg80211 snd intel_gtt yenta_socket pcmcia_rsrc sony_laptop agpgart microcode psmouse tpm_tis serio_raw mxm_wmi soundcore snd_page_alloc tpm acpi_cpufreq lpc_ich pcmcia_core tpm_bios mperf processor lp parport firewire_ohci firewire_core crc_itu_t sdhci_pci sdhci thermal e1000e
> > [92146.548173] Pid: 4304, comm: kworker/0:0 Tainted: G W 3.8.0-rc3-s0i3-v3-test+ #9
> > [92146.548175] Call Trace:
> > [92146.548189] [<c10378e2>] warn_slowpath_common+0x72/0xa0
> > [92146.548227] [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> > [92146.548263] [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> > [92146.548270] [<c10379b3>] warn_slowpath_fmt+0x33/0x40
> > [92146.548307] [<f86398b4>] intel_wait_for_pipe_off+0x184/0x190 [i915]
> > [92146.548344] [<f86399c2>] intel_disable_pipe+0x102/0x190 [i915]
> > [92146.548380] [<f8639ea4>] ? intel_disable_plane+0x64/0x80 [i915]
> > [92146.548417] [<f8639f7c>] i9xx_crtc_disable+0xbc/0x150 [i915]
> > [92146.548456] [<f863ebee>] intel_crtc_update_dpms+0x5e/0x90 [i915]
> > [92146.548493] [<f86437cf>] intel_modeset_setup_hw_state+0x42f/0x8f0 [i915]
> > [92146.548535] [<f8645b0b>] intel_lid_notify+0x9b/0xc0 [i915]
> > [92146.548543] [<c15610d3>] notifier_call_chain+0x43/0x60
> > [92146.548550] [<c105d1e1>] __blocking_notifier_call_chain+0x41/0x80
> > [92146.548556] [<c105d23f>] blocking_notifier_call_chain+0x1f/0x30
> > [92146.548563] [<c131a684>] acpi_lid_send_state+0x78/0xa4
> > [92146.548569] [<c131aa9e>] acpi_button_notify+0x3b/0xf1
> > [92146.548577] [<c12df56a>] ? acpi_os_execute+0x17/0x19
> > [92146.548582] [<c12e591a>] ? acpi_ec_sync_query+0xa5/0xbc
> > [92146.548589] [<c12e2b82>] acpi_device_notify+0x16/0x18
> > [92146.548595] [<c12f4904>] acpi_ev_notify_dispatch+0x38/0x4f
> > [92146.548600] [<c12df0e8>] acpi_os_execute_deferred+0x20/0x2b
> > [92146.548607] [<c1051208>] process_one_work+0x128/0x3f0
> > [92146.548613] [<c1564f73>] ? common_interrupt+0x33/0x38
> > [92146.548618] [<c104f8c0>] ? wake_up_worker+0x30/0x30
> > [92146.548624] [<c12df0c8>] ? acpi_os_wait_events_complete+0x1e/0x1e
> > [92146.548629] [<c10524f9>] worker_thread+0x119/0x3b0
> > [92146.548634] [<c10523e0>] ? manage_workers+0x240/0x240
> > [92146.548640] [<c1056e84>] kthread+0x94/0xa0
> > [92146.548647] [<c1060000>] ? ftrace_raw_output_sched_stat_runtime+0x70/0xf0
> > [92146.548652] [<c15649b7>] ret_from_kernel_thread+0x1b/0x28
> > [92146.548658] [<c1056df0>] ? kthread_create_on_node+0xc0/0xc0
> >
> > three different modeset flags are introduced in this patch
> > MODESET_ON_LID_OPEN: do modeset on next lid open event
> > MODESET_DONE: modeset already done
> > MODESET_SUSPENDED: suspended, only do modeset when system is resumed
> >
> > In this way,
> > 1. when lid is closed, MODESET_ON_LID_OPEN is set so that
> > we'll do modeset on next lid open event.
> > 2. when lid is opened, MODESET_DONE is set
> > so that duplicate lid open events will be ignored.
> > 3. when system suspends, MODESET_SUSPENDED is set.
> > In this case, we will not do modeset on any lid events.
> >
> > Plus, locking mechanism is also introduced to avoid racing.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_dma.c | 1 +
> > drivers/gpu/drm/i915/i915_drv.c | 14 +++++++++-----
> > drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++--
> > drivers/gpu/drm/i915/intel_lvds.c | 33 ++++++++++++++++++++-------------
> > 4 files changed, 39 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 99daa89..a5115d8 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1585,6 +1585,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > spin_lock_init(&dev_priv->dpio_lock);
> >
> > mutex_init(&dev_priv->rps.hw_lock);
> > + mutex_init(&dev_priv->modeset_restore_lock);
> >
> > if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
> > dev_priv->num_pipe = 3;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 1172658..68c6048 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -468,6 +468,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > + /* ignore lid events during suspend */
> > + mutex_lock(&dev_priv->modeset_restore_lock);
> > + dev_priv->modeset_restore = MODESET_SUSPENDED;
> > + mutex_unlock(&dev_priv->modeset_restore_lock);
> > +
> > +
> > drm_kms_helper_poll_disable(dev);
> >
> > pci_save_state(dev->pdev);
> > @@ -492,9 +498,6 @@ static int i915_drm_freeze(struct drm_device *dev)
> >
> > intel_opregion_fini(dev);
> >
> > - /* Modeset on resume, not lid events */
> > - dev_priv->modeset_on_lid = 0;
> > -
> > console_lock();
> > intel_fbdev_set_suspend(dev, 1);
> > console_unlock();
> > @@ -569,8 +572,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
> >
> > intel_opregion_init(dev);
> >
> > - dev_priv->modeset_on_lid = 0;
> > -
> > /*
> > * The console lock can be pretty contented on resume due
> > * to all the printk activity. Try to keep it out of the hot
> > @@ -583,6 +584,9 @@ static int __i915_drm_thaw(struct drm_device *dev)
> > schedule_work(&dev_priv->console_resume_work);
> > }
> >
> > + mutex_lock(&dev_priv->modeset_restore_lock);
> > + dev_priv->modeset_restore = MODESET_DONE;
> > + mutex_unlock(&dev_priv->modeset_restore_lock);
> > return error;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 12ab3bd..0fddad01 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -620,6 +620,12 @@ struct intel_l3_parity {
> > struct work_struct error_work;
> > };
> >
> > +enum modeset_restore {
> > + MODESET_ON_LID_OPEN,
> > + MODESET_DONE,
> > + MODESET_SUSPENDED,
> > +};
> > +
> > typedef struct drm_i915_private {
> > struct drm_device *dev;
> >
> > @@ -750,9 +756,10 @@ typedef struct drm_i915_private {
> >
> > unsigned long quirks;
> >
> > - /* Register state */
> > - bool modeset_on_lid;
> > + enum modeset_restore modeset_restore;
> > + struct mutex modeset_restore_lock;
> >
> > + /* Register state */
> > struct {
> > /** Bridge to intel-gtt-ko */
> > struct intel_gtt *gtt;
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 17aee74..ff89d58 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -492,13 +492,14 @@ static const struct dmi_system_id intel_no_modeset_on_lid[] = {
> > };
> >
> > /*
> > - * Lid events. Note the use of 'modeset_on_lid':
> > - * - we set it on lid close, and reset it on open
> > + * Lid events. Note the use of 'modeset':
> > + * - we set it to MODESET_ON_LID_OPEN on lid close,
> > + * and set it to MODESET_DONE on open
> > * - we use it as a "only once" bit (ie we ignore
> > - * duplicate events where it was already properly
> > - * set/reset)
> > - * - the suspend/resume paths will also set it to
> > - * zero, since they restore the mode ("lid open").
> > + * duplicate events where it was already properly set)
> > + * - the suspend/resume paths will set it to
> > + * MODESET_SUSPENDED and ignore the lid open event,
> > + * because they restore the mode ("lid open").
> > */
> > static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> > void *unused)
> > @@ -512,6 +513,9 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> > if (dev->switch_power_state != DRM_SWITCH_POWER_ON)
> > return NOTIFY_OK;
> >
> > + mutex_lock(&dev_priv->modeset_restore_lock);
> > + if (dev_priv->modeset_restore == MODESET_SUSPENDED)
> > + goto exit;
> > /*
> > * check and update the status of LVDS connector after receiving
> > * the LID nofication event.
> > @@ -520,21 +524,24 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> >
> > /* Don't force modeset on machines where it causes a GPU lockup */
> > if (dmi_check_system(intel_no_modeset_on_lid))
> > - return NOTIFY_OK;
> > + goto exit;
> > if (!acpi_lid_open()) {
> > - dev_priv->modeset_on_lid = 1;
> > - return NOTIFY_OK;
> > + /* do modeset on next lid open event */
> > + dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
> > + goto exit;
> > }
> >
> > - if (!dev_priv->modeset_on_lid)
> > - return NOTIFY_OK;
> > -
> > - dev_priv->modeset_on_lid = 0;
> > + if (dev_priv->modeset_restore == MODESET_DONE)
> > + goto exit;
> >
> > mutex_lock(&dev->mode_config.mutex);
> > intel_modeset_setup_hw_state(dev, true);
> > mutex_unlock(&dev->mode_config.mutex);
> >
> > + dev_priv->modeset_restore = MODESET_DONE;
> > +
> > +exit:
> > + mutex_unlock(&dev_priv->modeset_restore_lock);
> > return NOTIFY_OK;
> > }
> >
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
next prev parent reply other threads:[~2013-02-05 10:14 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-04 7:10 [PATCH V2 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE Zhang Rui
2013-02-04 7:10 ` [PATCH V2 2/3] ACPI: enable ACPI SCI during suspend Zhang Rui
2013-02-04 7:10 ` [PATCH V2 3/3] i915: ignore lid open event when resuming Zhang Rui
2013-02-04 15:25 ` Daniel Vetter
2013-02-04 15:25 ` [Intel-gfx] " Daniel Vetter
2013-02-04 23:58 ` Zhang Rui
2013-02-05 7:41 ` Zhang Rui
2013-02-05 10:07 ` Daniel Vetter
2013-02-05 10:07 ` [Intel-gfx] " Daniel Vetter
2013-02-05 10:14 ` Rafael J. Wysocki [this message]
2013-02-05 12:35 ` Zhang Rui
2013-02-05 12:34 ` Zhang Rui
2013-02-06 1:11 ` [PATCH V2 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE Zhang Rui
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3004369.zSvdplM23x@vostro.rjw.lan \
--to=rjw@sisk.pl \
--cc=daniel@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rui.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.