| Message ID | 1391699988-1890-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 6, 2014 at 4:19 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > This function is used to discover which swsci callbacks the machine > supports. It calls swsci() 3 times, and usually takes a considerable > number of milliseconds to finish. > > I don't see a reason for a change on the supported callbacks between > boot and resume, so use the values retrieved at boot time forever, > making resume a little bit faster. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> I think we could simply not call opregion_setup in our thaw function. The actual runtime setup is already split out into _init/fini functions, so if anything in opregion_setup needs to be done at resume we could simply move that. But afaict the split looks sane already. That way the init sequence is much less tricky than the conditional swsci_setup in your patch. The only write we have in _setup is ASLE_ARDY_NOT_READY, but that's the default state - the driver only needs to signal readiness once everything is up (in _init) and tell the bios again that it's gone in _fini. -Daniel
On Thu, Feb 06, 2014 at 01:19:48PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > This function is used to discover which swsci callbacks the machine > supports. It calls swsci() 3 times, and usually takes a considerable > number of milliseconds to finish. > > I don't see a reason for a change on the supported callbacks between > boot and resume, so use the values retrieved at boot time forever, > making resume a little bit faster. This looks unsafe following a hibernate resume where the system could change around us. Across a suspend resume, you can argue that we do not need to shootdown OpRegion (and not just swsci) at all. -Chris
On Thu, Feb 6, 2014 at 4:32 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Thu, Feb 06, 2014 at 01:19:48PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> This function is used to discover which swsci callbacks the machine >> supports. It calls swsci() 3 times, and usually takes a considerable >> number of milliseconds to finish. >> >> I don't see a reason for a change on the supported callbacks between >> boot and resume, so use the values retrieved at boot time forever, >> making resume a little bit faster. > > This looks unsafe following a hibernate resume where the system could > change around us. Across a suspend resume, you can argue that we do not > need to shootdown OpRegion (and not just swsci) at all. See my other mail, but I think _init/_fini take care of all this. The only exception would be if the bios moves the opregion around, but that's only really possible if you update it or change your machine otherwise. Then you get both pieces anyway. -Daniel
On Thu, Feb 06, 2014 at 04:35:10PM +0100, Daniel Vetter wrote: > On Thu, Feb 6, 2014 at 4:32 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Thu, Feb 06, 2014 at 01:19:48PM -0200, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> This function is used to discover which swsci callbacks the machine > >> supports. It calls swsci() 3 times, and usually takes a considerable > >> number of milliseconds to finish. > >> > >> I don't see a reason for a change on the supported callbacks between > >> boot and resume, so use the values retrieved at boot time forever, > >> making resume a little bit faster. > > > > This looks unsafe following a hibernate resume where the system could > > change around us. Across a suspend resume, you can argue that we do not > > need to shootdown OpRegion (and not just swsci) at all. > > See my other mail, but I think _init/_fini take care of all this. The > only exception would be if the bios moves the opregion around, but > that's only really possible if you update it or change your machine > otherwise. Then you get both pieces anyway. Exactly, that's why I think trusting the layout post hibernate is danagerous. -Chris
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 258b1be..f57e220 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1616,6 +1616,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) intel_setup_mchbar(dev); intel_setup_gmbus(dev); intel_opregion_setup(dev); + intel_swsci_setup(dev); intel_setup_bios(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 728b9c3..4f65e12 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2507,6 +2507,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable); extern int intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state); +extern void intel_swsci_setup(struct drm_device *dev); #else static inline void intel_opregion_init(struct drm_device *dev) { return; } static inline void intel_opregion_fini(struct drm_device *dev) { return; } @@ -2521,6 +2522,7 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state) { return 0; } +static inline void intel_swsci_setup(struct drm_device *dev) { return; } #endif /* intel_acpi.c */ diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index 6845960..579bf24 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -780,13 +780,16 @@ void intel_opregion_fini(struct drm_device *dev) opregion->lid_state = NULL; } -static void swsci_setup(struct drm_device *dev) +void intel_swsci_setup(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_opregion *opregion = &dev_priv->opregion; bool requested_callbacks = false; u32 tmp; + if (!opregion->swsci) + return; + /* Sub-function code 0 is okay, let's allow them. */ opregion->swsci_gbda_sub_functions = 1; opregion->swsci_sbcb_sub_functions = 1; @@ -836,8 +839,6 @@ static void swsci_setup(struct drm_device *dev) opregion->swsci_gbda_sub_functions, opregion->swsci_sbcb_sub_functions); } -#else /* CONFIG_ACPI */ -static inline void swsci_setup(struct drm_device *dev) {} #endif /* CONFIG_ACPI */ int intel_opregion_setup(struct drm_device *dev) @@ -885,7 +886,6 @@ int intel_opregion_setup(struct drm_device *dev) if (mboxes & MBOX_SWSCI) { DRM_DEBUG_DRIVER("SWSCI supported\n"); opregion->swsci = base + OPREGION_SWSCI_OFFSET; - swsci_setup(dev); } if (mboxes & MBOX_ASLE) { DRM_DEBUG_DRIVER("ASLE supported\n");