Re: [Intel-gfx] [PATCH] drm/i915: Stop calling intel_opregion unregister/register in suspend/resume
On Mon, Oct 08, 2018 at 09:44:08AM +0300, Jani Nikula wrote:
> On Fri, 05 Oct 2018, Chris Wilson 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
> > Cc: Imre Deak
> > Cc: Jani Nikula
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
> > ---
> > 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 = _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(>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 = _priv->opregion;
> > -
> > - if (!opregion->header)
> > - return;
> > -
> > - if (opregion->asle)
> > - opregion->asle->ardy = ASLE_ARDY_NOT_READY;
> > -
> > - cancel_work_sync(_priv->opregion.asle_work);
> > -
> > - if (opregion->acpi) {
> > - opregion->acpi->drdy = 0;
> > -
> > - unregister_acpi_notifier(>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;
> > -
Re: [Intel-gfx] [PATCH] drm/i915: Stop calling intel_opregion unregister/register in suspend/resume
On Fri, 05 Oct 2018, Chris Wilson 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
> Cc: Imre Deak
> Cc: Jani Nikula
> ---
> 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 = _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(>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 = _priv->opregion;
> -
> - if (!opregion->header)
> - return;
> -
> - if (opregion->asle)
> - opregion->asle->ardy = ASLE_ARDY_NOT_READY;
> -
> - cancel_work_sync(_priv->opregion.asle_work);
> -
> - if (opregion->acpi) {
> - opregion->acpi->drdy = 0;
> -
> - unregister_acpi_notifier(>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 = _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 = >opregion;
> +
> + if
[Intel-gfx] [PATCH] drm/i915: Stop calling intel_opregion unregister/register in suspend/resume
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.
Signed-off-by: Chris Wilson
Cc: Imre Deak
Cc: Jani Nikula
---
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 = _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(>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 = _priv->opregion;
-
- if (!opregion->header)
- return;
-
- if (opregion->asle)
- opregion->asle->ardy = ASLE_ARDY_NOT_READY;
-
- cancel_work_sync(_priv->opregion.asle_work);
-
- if (opregion->acpi) {
- opregion->acpi->drdy = 0;
-
- unregister_acpi_notifier(>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 = _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 = >opregion;
+
+ if (!opregion->header)
+ return;
+
+ if (opregion->acpi) {
+ opregion->acpi_notifier.notifier_call =
intel_opregion_video_event;
+ register_acpi_notifier(>acpi_notifier);
+ }
+
+ intel_opregion_resume(i915);
+}
+
+void intel_opregion_resume(struct drm_i915_private
