Re: [PATCH] drm/i915: Stop calling intel_opregion unregister/register in suspend/resume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Oct 08, 2018 at 09:44:08AM +0300, Jani Nikula wrote:
> On Fri, 05 Oct 2018, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> > If we reduce the suspend function for intel_opregion to do the minimum
> > required, the resume function can also do the simple task of notifier
> > the ACPI bios that we are back. This avoid some nasty restrictions on
> > the likes of register_acpi_notifier() that are not allowed during the
> > early phase of resume.
> 
> Something like this has been on the back of my mind for a long time, but
> never got around to it. Couple of nitpicks/observations inline.
> 
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>

Simplifies the task of moving some steps from the suspend/resume hooks
to the suspend_late/resume_early hooks, which is needed by the audio/
CDCLK workaround we are working towards, so:

Acked-by: Imre Deak <imre.deak@xxxxxxxxx>

> > ---
> >  drivers/gpu/drm/i915/i915_drv.c       |   9 +-
> >  drivers/gpu/drm/i915/intel_opregion.c | 155 +++++++++++++++-----------
> >  drivers/gpu/drm/i915/intel_opregion.h |  15 +++
> >  3 files changed, 108 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 193023427b40..9b0887746bac 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1919,9 +1919,7 @@ static int i915_drm_suspend(struct drm_device *dev)
> >  	i915_save_state(dev_priv);
> >  
> >  	opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold;
> > -	intel_opregion_notify_adapter(dev_priv, opregion_target_state);
> > -
> > -	intel_opregion_unregister(dev_priv);
> > +	intel_opregion_suspend(dev_priv, opregion_target_state);
> >  
> >  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
> >  
> > @@ -2040,7 +2038,6 @@ static int i915_drm_resume(struct drm_device *dev)
> >  
> >  	i915_restore_state(dev_priv);
> >  	intel_pps_unlock_regs_wa(dev_priv);
> > -	intel_opregion_setup(dev_priv);
> >  
> >  	intel_init_pch_refclk(dev_priv);
> >  
> > @@ -2082,12 +2079,10 @@ static int i915_drm_resume(struct drm_device *dev)
> >  	 * */
> >  	intel_hpd_init(dev_priv);
> >  
> > -	intel_opregion_register(dev_priv);
> > +	intel_opregion_resume(dev_priv);
> >  
> >  	intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);
> >  
> > -	intel_opregion_notify_adapter(dev_priv, PCI_D0);
> > -
> >  	intel_power_domains_enable(dev_priv);
> >  
> >  	enable_rpm_wakeref_asserts(dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> > index e034b4166d32..379e8c64a248 100644
> > --- a/drivers/gpu/drm/i915/intel_opregion.c
> > +++ b/drivers/gpu/drm/i915/intel_opregion.c
> > @@ -773,70 +773,6 @@ static void intel_setup_cadls(struct drm_i915_private *dev_priv)
> >  		opregion->acpi->cadl[i] = 0;
> >  }
> >  
> > -void intel_opregion_register(struct drm_i915_private *dev_priv)
> > -{
> > -	struct intel_opregion *opregion = &dev_priv->opregion;
> > -
> > -	if (!opregion->header)
> > -		return;
> > -
> > -	if (opregion->acpi) {
> > -		intel_didl_outputs(dev_priv);
> > -		intel_setup_cadls(dev_priv);
> > -
> > -		/* Notify BIOS we are ready to handle ACPI video ext notifs.
> > -		 * Right now, all the events are handled by the ACPI video module.
> > -		 * We don't actually need to do anything with them. */
> > -		opregion->acpi->csts = 0;
> > -		opregion->acpi->drdy = 1;
> > -
> > -		opregion->acpi_notifier.notifier_call = intel_opregion_video_event;
> > -		register_acpi_notifier(&opregion->acpi_notifier);
> > -	}
> > -
> > -	if (opregion->asle) {
> > -		opregion->asle->tche = ASLE_TCHE_BLC_EN;
> > -		opregion->asle->ardy = ASLE_ARDY_READY;
> > -	}
> > -}
> > -
> > -void intel_opregion_unregister(struct drm_i915_private *dev_priv)
> > -{
> > -	struct intel_opregion *opregion = &dev_priv->opregion;
> > -
> > -	if (!opregion->header)
> > -		return;
> > -
> > -	if (opregion->asle)
> > -		opregion->asle->ardy = ASLE_ARDY_NOT_READY;
> > -
> > -	cancel_work_sync(&dev_priv->opregion.asle_work);
> > -
> > -	if (opregion->acpi) {
> > -		opregion->acpi->drdy = 0;
> > -
> > -		unregister_acpi_notifier(&opregion->acpi_notifier);
> > -		opregion->acpi_notifier.notifier_call = NULL;
> > -	}
> > -
> > -	/* just clear all opregion memory pointers now */
> > -	memunmap(opregion->header);
> > -	if (opregion->rvda) {
> > -		memunmap(opregion->rvda);
> > -		opregion->rvda = NULL;
> > -	}
> > -	if (opregion->vbt_firmware) {
> > -		kfree(opregion->vbt_firmware);
> > -		opregion->vbt_firmware = NULL;
> > -	}
> > -	opregion->header = NULL;
> > -	opregion->acpi = NULL;
> > -	opregion->swsci = NULL;
> > -	opregion->asle = NULL;
> > -	opregion->vbt = NULL;
> > -	opregion->lid_state = NULL;
> > -}
> > -
> >  static void swsci_setup(struct drm_i915_private *dev_priv)
> >  {
> >  	struct intel_opregion *opregion = &dev_priv->opregion;
> > @@ -1115,3 +1051,94 @@ intel_opregion_get_panel_type(struct drm_i915_private *dev_priv)
> >  
> >  	return ret - 1;
> >  }
> > +
> > +void intel_opregion_register(struct drm_i915_private *i915)
> > +{
> > +	struct intel_opregion *opregion = &i915->opregion;
> > +
> > +	if (!opregion->header)
> > +		return;
> > +
> > +	if (opregion->acpi) {
> > +		opregion->acpi_notifier.notifier_call = intel_opregion_video_event;
> > +		register_acpi_notifier(&opregion->acpi_notifier);
> > +	}
> > +
> > +	intel_opregion_resume(i915);
> > +}
> > +
> > +void intel_opregion_resume(struct drm_i915_private *i915)
> > +{
> > +	struct intel_opregion *opregion = &i915->opregion;
> > +
> > +	if (!opregion->header)
> > +		return;
> > +
> > +	if (opregion->acpi) {
> > +		intel_didl_outputs(i915);
> > +		intel_setup_cadls(i915);
> > +
> > +		/* Notify BIOS we are ready to handle ACPI video ext notifs.
> > +		 * Right now, all the events are handled by the ACPI video module.
> > +		 * We don't actually need to do anything with them. */
> > +		opregion->acpi->csts = 0;
> > +		opregion->acpi->drdy = 1;
> > +	}
> > +
> > +	if (opregion->asle) {
> > +		opregion->asle->tche = ASLE_TCHE_BLC_EN;
> > +		opregion->asle->ardy = ASLE_ARDY_READY;
> > +	}
> > +
> > +	intel_opregion_notify_adapter(i915, PCI_D0);
> > +}
> > +
> > +void intel_opregion_suspend(struct drm_i915_private *i915, pci_power_t state)
> > +{
> > +	struct intel_opregion *opregion = &i915->opregion;
> > +
> > +	if (!opregion->header)
> > +		return;
> > +
> > +	if (opregion->asle)
> > +		opregion->asle->ardy = ASLE_ARDY_NOT_READY;
> > +
> > +	cancel_work_sync(&i915->opregion.asle_work);
> > +
> > +	if (opregion->acpi)
> > +		opregion->acpi->drdy = 0;
> > +
> > +	intel_opregion_notify_adapter(i915, state);
> 
> The order between drdy clear and notify changes. Depends on the BIOS
> whether that has any significance, i.e. I have no clue. If you think the
> reordering is justified, please split it out to a separate patch. Or
> just drop the order change completely. With that,
> 
> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> 
> Side note, it would seem like drdy clear should happen before canceling
> the work. In theory that should block new asle interrupts. But that's
> another change anyway, and perhaps not worth the risk.
> 
> > +}
> > +
> > +void intel_opregion_unregister(struct drm_i915_private *i915)
> > +{
> > +	struct intel_opregion *opregion = &i915->opregion;
> > +
> > +	intel_opregion_suspend(i915, PCI_D1);
> > +
> > +	if (!opregion->header)
> > +		return;
> > +
> > +	if (opregion->acpi_notifier.notifier_call) {
> > +		unregister_acpi_notifier(&opregion->acpi_notifier);
> > +		opregion->acpi_notifier.notifier_call = NULL;
> > +	}
> > +
> > +	/* just clear all opregion memory pointers now */
> > +	memunmap(opregion->header);
> > +	if (opregion->rvda) {
> > +		memunmap(opregion->rvda);
> > +		opregion->rvda = NULL;
> > +	}
> > +	if (opregion->vbt_firmware) {
> > +		kfree(opregion->vbt_firmware);
> > +		opregion->vbt_firmware = NULL;
> > +	}
> > +	opregion->header = NULL;
> > +	opregion->acpi = NULL;
> > +	opregion->swsci = NULL;
> > +	opregion->asle = NULL;
> > +	opregion->vbt = NULL;
> > +	opregion->lid_state = NULL;
> 
> Side note #2, this patch highlights (but does not change) how we have
> two init functions setup and register undone by a single unregister
> function. The asymmetry has always bugged me, but it's no fault of this
> patch.
> 
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_opregion.h b/drivers/gpu/drm/i915/intel_opregion.h
> > index e8498a8cda3d..d84b6d2d2fae 100644
> > --- a/drivers/gpu/drm/i915/intel_opregion.h
> > +++ b/drivers/gpu/drm/i915/intel_opregion.h
> > @@ -57,8 +57,14 @@ struct intel_opregion {
> >  #ifdef CONFIG_ACPI
> >  
> >  int intel_opregion_setup(struct drm_i915_private *dev_priv);
> > +
> >  void intel_opregion_register(struct drm_i915_private *dev_priv);
> >  void intel_opregion_unregister(struct drm_i915_private *dev_priv);
> > +
> > +void intel_opregion_resume(struct drm_i915_private *dev_priv);
> > +void intel_opregion_suspend(struct drm_i915_private *dev_priv,
> > +			    pci_power_t state);
> > +
> >  void intel_opregion_asle_intr(struct drm_i915_private *dev_priv);
> >  int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
> >  				  bool enable);
> > @@ -81,6 +87,15 @@ static inline void intel_opregion_unregister(struct drm_i915_private *dev_priv)
> >  {
> >  }
> >  
> > +void intel_opregion_resume(struct drm_i915_private *dev_priv)
> > +{
> > +}
> > +
> > +void intel_opregion_suspend(struct drm_i915_private *dev_priv,
> > +			    pci_power_t state)
> > +{
> > +}
> > +
> >  static inline void intel_opregion_asle_intr(struct drm_i915_private *dev_priv)
> >  {
> >  }
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux