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
