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>
> ---
> 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
