| Message ID | 20181012215218.5119-6-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/16] drm/i915: Properly set PCH as NOP when display is disabled | expand |
On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote: > Display features should not be initialized or deinitialized when > display is disabled. > > With this changes no plane, CRTC, encoder or connector > is being registered in drm when display is disabled so it was also > necessary unset DRIVER_MODESET and DRIVER_ATOMIC features from driver > otherwise it will crash when registering driver in drm. > > There is still more modeset/display calls that will be removed in > futher patches. > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > squash do not call modeset > --- > drivers/gpu/drm/i915/i915_drv.c | 155 +++++++++++++++--------- > drivers/gpu/drm/i915/i915_suspend.c | 24 ++-- > drivers/gpu/drm/i915/intel_runtime_pm.c | 21 ++-- > 3 files changed, 124 insertions(+), 76 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 3f7514e981d5..8334d1797df7 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -899,7 +899,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) > intel_wopcm_init_early(&dev_priv->wopcm); > intel_uc_init_early(dev_priv); > intel_pm_setup(dev_priv); > - intel_init_dpio(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) > + intel_init_dpio(dev_priv); > ret = intel_power_domains_init(dev_priv); > if (ret < 0) > goto err_uc; > @@ -907,8 +908,10 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) > intel_hangcheck_init(dev_priv); > intel_init_display_hooks(dev_priv); > intel_init_clock_gating_hooks(dev_priv); > - intel_init_audio_hooks(dev_priv); > - intel_display_crc_init(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) { > + intel_init_audio_hooks(dev_priv); > + intel_display_crc_init(dev_priv); > + } > > intel_detect_preproduction_hw(dev_priv); > > @@ -1539,23 +1542,26 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) > if (IS_GEN5(dev_priv)) > intel_gpu_ips_init(dev_priv); > > - intel_audio_init(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) { > + intel_audio_init(dev_priv); > > - /* > - * Some ports require correctly set-up hpd registers for detection to > - * work properly (leading to ghost connected connector status), e.g. VGA > - * on gm45. Hence we can only set up the initial fbdev config after hpd > - * irqs are fully enabled. We do it last so that the async config > - * cannot run before the connectors are registered. > - */ > - intel_fbdev_initial_config_async(dev); > + /* > + * Some ports require correctly set-up hpd registers for > + * detection to work properly (leading to ghost connected > + * connector status), e.g. VGA on gm45. Hence we can only set > + * up the initial fbdev config after hpd irqs are fully enabled. > + * We do it last so that the async config cannot run before the > + * connectors are registered. > + */ > + intel_fbdev_initial_config_async(dev); So for all of the above that we're in control of, I'd put the display check and early return at the beginning of each function, and call them unconditionally from here. This particularly so for all the higher level functions. They become cluttered and hard to grasp. It's slightly different at the lower levels. I think we need a macro #define HAS_DISPLAY(dev_priv) (INTEL_INFO(dev_priv)->num_pipes != 0) and use that for anything that checks for the binary has display or not. We may need to redefine that later on, and I don't want to touch all of these places again. > > - /* > - * We need to coordinate the hotplugs with the asynchronous fbdev > - * configuration, for which we use the fbdev->async_cookie. > - */ > - if (INTEL_INFO(dev_priv)->num_pipes) > + /* > + * We need to coordinate the hotplugs with the asynchronous > + * fbdev configuration, for which we use the > + * fbdev->async_cookie. > + */ > drm_kms_helper_poll_init(dev); > + } Obviously calls to drm helpers need to be wrapped around HAS_DISPLAY. > > intel_power_domains_enable(dev_priv); > intel_runtime_pm_enable(dev_priv); > @@ -1570,15 +1576,17 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) > intel_runtime_pm_disable(dev_priv); > intel_power_domains_disable(dev_priv); > > - intel_fbdev_unregister(dev_priv); > - intel_audio_deinit(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) { > + intel_fbdev_unregister(dev_priv); > + intel_audio_deinit(dev_priv); All of these I want to be able to call unconditionally too, and instead of checking for HAS_DISPLAY(), they might be better off checking whether the stuff has been initialized. Indeed, I think the above two already work like this; they can be safely called even when audio or fbdev hasn't been initialized. BR, Jani. > > - /* > - * After flushing the fbdev (incl. a late async config which will > - * have delayed queuing of a hotplug event), then flush the hotplug > - * events. > - */ > - drm_kms_helper_poll_fini(&dev_priv->drm); > + /* > + * After flushing the fbdev (incl. a late async config which > + * will have delayed queuing of a hotplug event), then flush the > + * hotplug events. > + */ > + drm_kms_helper_poll_fini(&dev_priv->drm); > + } > > intel_gpu_ips_teardown(); > acpi_video_unregister(); > @@ -1641,6 +1649,7 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent) > if (i915_modparams.disable_display) { > DRM_INFO("Display disabled (module parameter)\n"); > device_info->num_pipes = 0; > + i915->drm.driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC); > } > > BUILD_BUG_ON(INTEL_MAX_PLATFORMS > > @@ -1721,9 +1730,11 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) > if (ret) > goto cleanup_irq; > > - ret = i915_load_modeset_init(&dev_priv->drm); > - if (ret < 0) > - goto cleanup_gem; > + if (INTEL_INFO(dev_priv)->num_pipes) { > + ret = i915_load_modeset_init(&dev_priv->drm); > + if (ret < 0) > + goto cleanup_gem; > + } > > i915_driver_register(dev_priv); > > @@ -1780,11 +1791,13 @@ void i915_driver_unload(struct drm_device *dev) > if (i915_gem_suspend(dev_priv)) > DRM_ERROR("failed to idle hardware; continuing to unload!\n"); > > - drm_atomic_helper_shutdown(dev); > + if (INTEL_INFO(dev_priv)->num_pipes) > + drm_atomic_helper_shutdown(dev); > > intel_gvt_cleanup(dev_priv); > > - intel_modeset_cleanup_prepare(dev); > + if (INTEL_INFO(dev_priv)->num_pipes) > + intel_modeset_cleanup_prepare(dev); > > /* > * Interrupts and polling as the first thing to avoid creating havoc. > @@ -1793,9 +1806,11 @@ void i915_driver_unload(struct drm_device *dev) > */ > intel_irq_uninstall(dev_priv); > > - intel_modeset_cleanup(dev); > + if (INTEL_INFO(dev_priv)->num_pipes) { > + intel_modeset_cleanup(dev); > > - i915_modeset_unload(dev); > + i915_modeset_unload(dev); > + } > > /* Free error state after interrupts are fully disabled. */ > cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); > @@ -1847,8 +1862,12 @@ static int i915_driver_open(struct drm_device *dev, struct drm_file *file) > */ > static void i915_driver_lastclose(struct drm_device *dev) > { > - intel_fbdev_restore_mode(dev); > - vga_switcheroo_process_delayed_switch(); > + struct drm_i915_private *dev_priv = to_i915(dev); > + > + if (INTEL_INFO(dev_priv)->num_pipes) { > + intel_fbdev_restore_mode(dev); > + vga_switcheroo_process_delayed_switch(); > + } > } > > static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file) > @@ -1918,19 +1937,24 @@ static int i915_drm_suspend(struct drm_device *dev) > /* We do a lot of poking in a lot of registers, make sure they work > * properly. */ > intel_power_domains_disable(dev_priv); > - > - drm_kms_helper_poll_disable(dev); > + if (INTEL_INFO(dev_priv)->num_pipes) > + drm_kms_helper_poll_disable(dev); > > pci_save_state(pdev); > > - intel_display_suspend(dev); > + if (INTEL_INFO(dev_priv)->num_pipes) { > + intel_display_suspend(dev); > > - intel_dp_mst_suspend(dev_priv); > + intel_dp_mst_suspend(dev_priv); > + } > > intel_runtime_pm_disable_interrupts(dev_priv); > - intel_hpd_cancel_work(dev_priv); > > - intel_suspend_encoders(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) { > + intel_hpd_cancel_work(dev_priv); > + > + intel_suspend_encoders(dev_priv); > + } > > intel_suspend_hw(dev_priv); > > @@ -1943,11 +1967,13 @@ static int i915_drm_suspend(struct drm_device *dev) > > intel_opregion_unregister(dev_priv); > > - intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true); > + if (INTEL_INFO(dev_priv)->num_pipes) > + intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true); > > dev_priv->suspend_count++; > > - intel_csr_ucode_suspend(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) > + intel_csr_ucode_suspend(dev_priv); > > enable_rpm_wakeref_asserts(dev_priv); > > @@ -2056,10 +2082,12 @@ static int i915_drm_resume(struct drm_device *dev) > if (ret) > DRM_ERROR("failed to re-enable GGTT\n"); > > - intel_csr_ucode_resume(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) > + intel_csr_ucode_resume(dev_priv); > > i915_restore_state(dev_priv); > - intel_pps_unlock_regs_wa(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) > + intel_pps_unlock_regs_wa(dev_priv); > intel_opregion_setup(dev_priv); > > intel_init_pch_refclk(dev_priv); > @@ -2076,7 +2104,8 @@ static int i915_drm_resume(struct drm_device *dev) > */ > intel_runtime_pm_enable_interrupts(dev_priv); > > - drm_mode_config_reset(dev); > + if (INTEL_INFO(dev_priv)->num_pipes) > + drm_mode_config_reset(dev); > > i915_gem_resume(dev_priv); > > @@ -2084,27 +2113,30 @@ static int i915_drm_resume(struct drm_device *dev) > intel_init_clock_gating(dev_priv); > > spin_lock_irq(&dev_priv->irq_lock); > - if (dev_priv->display.hpd_irq_setup) > + if (dev_priv->display.hpd_irq_setup && INTEL_INFO(dev_priv)->num_pipes) > dev_priv->display.hpd_irq_setup(dev_priv); > spin_unlock_irq(&dev_priv->irq_lock); > > - intel_dp_mst_resume(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) { > + intel_dp_mst_resume(dev_priv); > > - intel_display_resume(dev); > + intel_display_resume(dev); > > - drm_kms_helper_poll_enable(dev); > + drm_kms_helper_poll_enable(dev); > > - /* > - * ... but also need to make sure that hotplug processing > - * doesn't cause havoc. Like in the driver load code we don't > - * bother with the tiny race here where we might lose hotplug > - * notifications. > - * */ > - intel_hpd_init(dev_priv); > + /* > + * ... but also need to make sure that hotplug processing > + * doesn't cause havoc. Like in the driver load code we don't > + * bother with the tiny race here where we might lose hotplug > + * notifications. > + */ > + intel_hpd_init(dev_priv); > + } > > intel_opregion_register(dev_priv); > > - intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false); > + if (INTEL_INFO(dev_priv)->num_pipes) > + intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false); > > intel_opregion_notify_adapter(dev_priv, PCI_D0); > > @@ -3000,7 +3032,8 @@ static int intel_runtime_suspend(struct device *kdev) > > assert_forcewakes_inactive(dev_priv); > > - if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) > + if (INTEL_INFO(dev_priv)->num_pipes && (!IS_VALLEYVIEW(dev_priv) && > + !IS_CHERRYVIEW(dev_priv))) > intel_hpd_poll_init(dev_priv); > > DRM_DEBUG_KMS("Device suspended\n"); > @@ -3057,10 +3090,12 @@ static int intel_runtime_resume(struct device *kdev) > * power well, so hpd is reinitialized from there. For > * everyone else do it here. > */ > - if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) > + if (INTEL_INFO(dev_priv)->num_pipes && (!IS_VALLEYVIEW(dev_priv) && > + !IS_CHERRYVIEW(dev_priv))) > intel_hpd_init(dev_priv); > > - intel_enable_ipc(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) > + intel_enable_ipc(dev_priv); > > enable_rpm_wakeref_asserts(dev_priv); > > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c > index 8f3aa4dc0c98..f697865236a6 100644 > --- a/drivers/gpu/drm/i915/i915_suspend.c > +++ b/drivers/gpu/drm/i915/i915_suspend.c > @@ -63,11 +63,13 @@ int i915_save_state(struct drm_i915_private *dev_priv) > > mutex_lock(&dev_priv->drm.struct_mutex); > > - i915_save_display(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) { > + i915_save_display(dev_priv); > > - if (IS_GEN4(dev_priv)) > - pci_read_config_word(pdev, GCDGMBUS, > - &dev_priv->regfile.saveGCDGMBUS); > + if (IS_GEN4(dev_priv)) > + pci_read_config_word(pdev, GCDGMBUS, > + &dev_priv->regfile.saveGCDGMBUS); > + } > > /* Cache mode state */ > if (INTEL_GEN(dev_priv) < 7) > @@ -108,10 +110,13 @@ int i915_restore_state(struct drm_i915_private *dev_priv) > > mutex_lock(&dev_priv->drm.struct_mutex); > > - if (IS_GEN4(dev_priv)) > - pci_write_config_word(pdev, GCDGMBUS, > - dev_priv->regfile.saveGCDGMBUS); > - i915_restore_display(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) { > + if (IS_GEN4(dev_priv)) > + pci_write_config_word(pdev, GCDGMBUS, > + dev_priv->regfile.saveGCDGMBUS); > + > + i915_restore_display(dev_priv); > + } > > /* Cache mode state */ > if (INTEL_GEN(dev_priv) < 7) > @@ -143,7 +148,8 @@ int i915_restore_state(struct drm_i915_private *dev_priv) > > mutex_unlock(&dev_priv->drm.struct_mutex); > > - intel_i2c_reset(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) > + intel_i2c_reset(dev_priv); > > return 0; > } > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 3cf8533e0834..15fd88bb35f7 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -625,7 +625,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv) > > DRM_DEBUG_KMS("Enabling DC9\n"); > > - intel_power_sequencer_reset(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) > + intel_power_sequencer_reset(dev_priv); > gen9_set_dc_state(dev_priv, DC_STATE_EN_DC9); > } > > @@ -1039,10 +1040,12 @@ static void vlv_display_power_well_deinit(struct drm_i915_private *dev_priv) > /* make sure we're done processing display irqs */ > synchronize_irq(dev_priv->drm.irq); > > - intel_power_sequencer_reset(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) > + intel_power_sequencer_reset(dev_priv); > > /* Prevent us from re-enabling polling on accident in late suspend */ > - if (!dev_priv->drm.dev->power.is_suspended) > + if (INTEL_INFO(dev_priv)->num_pipes && > + !dev_priv->drm.dev->power.is_suspended) > intel_hpd_poll_init(dev_priv); > } > > @@ -1284,11 +1287,14 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv, > > if (power_well->desc->id == VLV_DISP_PW_DPIO_CMN_BC) { > phy = DPIO_PHY0; > - assert_pll_disabled(dev_priv, PIPE_A); > - assert_pll_disabled(dev_priv, PIPE_B); > + if (INTEL_INFO(dev_priv)->num_pipes) { > + assert_pll_disabled(dev_priv, PIPE_A); > + assert_pll_disabled(dev_priv, PIPE_B); > + } > } else { > phy = DPIO_PHY1; > - assert_pll_disabled(dev_priv, PIPE_C); > + if (INTEL_INFO(dev_priv)->num_pipes) > + assert_pll_disabled(dev_priv, PIPE_C); > } > > dev_priv->chv_phy_control &= ~PHY_COM_LANE_RESET_DEASSERT(phy); > @@ -1302,7 +1308,8 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv, > /* PHY is fully reset now, so we can enable the PHY state asserts */ > dev_priv->chv_phy_assert[phy] = true; > > - assert_chv_phy_status(dev_priv); > + if (INTEL_INFO(dev_priv)->num_pipes) > + assert_chv_phy_status(dev_priv); > } > > static void assert_chv_phy_powergate(struct drm_i915_private *dev_priv, enum dpio_phy phy,
Quoting Jani Nikula (2018-10-22 09:25:45) > On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote: > > Display features should not be initialized or deinitialized when > > display is disabled. I completely disagree with this assertion. If the display is disabled, so must all the associated hw so that we can power down the entire chipset when idle. That means we have to complete the probe (so we continue to rely on fuses and in place of accurate fuses pci-id quirks for the infamous chipsets) and switch it off. -Chris
On Mon, 22 Oct 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Jani Nikula (2018-10-22 09:25:45) >> On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote: >> > Display features should not be initialized or deinitialized when >> > display is disabled. > > I completely disagree with this assertion. If the display is disabled, > so must all the associated hw so that we can power down the entire > chipset when idle. That means we have to complete the probe (so we > continue to rely on fuses and in place of accurate fuses pci-id quirks > for the infamous chipsets) and switch it off. That actually doesn't contradict with what I said about HAS_DISPLAY(). In many cases I think the early return on no display is the right thing to do. However, no display isn't the same as display disabled by module parameter (or whatnot)... which does require probe before disable to achieve the power down. But is the power down on display disable by module parameter a requirement for us? BR, Jani.
On Mon, 2018-10-22 at 12:00 +0300, Jani Nikula wrote: > On Mon, 22 Oct 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Jani Nikula (2018-10-22 09:25:45) > > > On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> > > > wrote: > > > > Display features should not be initialized or deinitialized > > > > when > > > > display is disabled. > > > > I completely disagree with this assertion. If the display is > > disabled, > > so must all the associated hw so that we can power down the entire > > chipset when idle. That means we have to complete the probe (so we > > continue to rely on fuses and in place of accurate fuses pci-id > > quirks > > for the infamous chipsets) and switch it off. > > That actually doesn't contradict with what I said about > HAS_DISPLAY(). In many cases I think the early return on no display > is > the right thing to do. However, no display isn't the same as display > disabled by module parameter (or whatnot)... which does require probe > before disable to achieve the power down. > > But is the power down on display disable by module parameter a > requirement for us? Okay so I will continue with the current approach to not initialize display stuff when HAS_DISPLAY() == false. Also Jani could you take a look into the first 5 patches? Those are moving some display initialization/uninitialization to proper functions. > > BR, > Jani. > >
Quoting Jani Nikula (2018-10-22 10:00:39) > On Mon, 22 Oct 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Jani Nikula (2018-10-22 09:25:45) > >> On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote: > >> > Display features should not be initialized or deinitialized when > >> > display is disabled. > > > > I completely disagree with this assertion. If the display is disabled, > > so must all the associated hw so that we can power down the entire > > chipset when idle. That means we have to complete the probe (so we > > continue to rely on fuses and in place of accurate fuses pci-id quirks > > for the infamous chipsets) and switch it off. > > That actually doesn't contradict with what I said about > HAS_DISPLAY(). In many cases I think the early return on no display is > the right thing to do. However, no display isn't the same as display > disabled by module parameter (or whatnot)... which does require probe > before disable to achieve the power down. > > But is the power down on display disable by module parameter a > requirement for us? We still see this error in BAT roughly every day: <7> [557.273023] [drm:intel_power_well_enable [i915]] enabling display <7> [557.273553] [drm:i915_redisable_vga_power_on [i915]] Something enabled VGA plane, disabling it <4> [557.274207] ------------[ cut here ]------------ <4> [557.274420] Unclaimed write to register 0x44200 <4> [557.274637] WARNING: CPU: 2 PID: 370 at drivers/gpu/drm/i915/intel_uncore.c:1034 __unclaimed_reg_debug+0x40/0x50 [i915] <4> [557.274643] Modules linked in: i915(+) amdgpu gpu_sched ttm vgem snd_hda_codec_hdmi coretemp btusb crct10dif_pclmul btrtl crc32_pclmul btbcm btintel ghash_clmulni_intel bluetooth cdc_ether usbnet r8152 mii ecdh_generic snd_hda_codec snd_hwdep snd_hda_core snd_pcm prime_numbers pinctrl_cherryview lpc_ich [last unloaded: i915] <4> [557.274686] CPU: 2 PID: 370 Comm: rngd Tainted: G U 5.1.0-rc4-CI-CI_DRM_5891+ #1 <4> [557.274690] Hardware name: GOOGLE Cyan/Cyan, BIOS MrChromebox 02/15/2018 <4> [557.274829] RIP: 0010:__unclaimed_reg_debug+0x40/0x50 [i915] <4> [557.274836] Code: 74 05 5b 5d 41 5c c3 45 84 e4 48 c7 c0 2b 6a 7b a0 48 c7 c6 21 6a 7b a0 48 0f 44 f0 89 ea 48 c7 c7 34 6a 7b a0 e8 b0 a3 a1 e0 <0f> 0b 83 2d 97 46 1b 00 01 5b 5d 41 5c c3 66 90 41 56 41 55 41 89 <4> [557.274841] RSP: 0018:ffff888079903e30 EFLAGS: 00010082 <4> [557.274847] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 <4> [557.274851] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000000ffffffff <4> [557.274856] RBP: 0000000000044200 R08: 0000000000000000 R09: 0000000000000001 <4> [557.274860] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 <4> [557.274865] R13: ffff88806d820eb0 R14: 0000000000000006 R15: 0000000000000000 <4> [557.274870] FS: 00007f7c805d2740(0000) GS:ffff888079900000(0000) knlGS:0000000000000000 <4> [557.274875] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 <4> [557.274879] CR2: 00005647db5bc7d8 CR3: 000000006476c000 CR4: 00000000001006e0 <4> [557.274884] Call Trace: <4> [557.274891] <IRQ> <4> [557.275026] fwtable_write32+0x25f/0x2c0 [i915] <4> [557.275149] cherryview_irq_handler+0x180/0x210 [i915] <4> [557.275170] __handle_irq_event_percpu+0x41/0x2d0 <4> [557.275177] ? handle_irq_event+0x27/0x50 <4> [557.275188] handle_irq_event_percpu+0x2b/0x70 <4> [557.275197] handle_irq_event+0x2f/0x50 <4> [557.275207] handle_edge_irq+0xee/0x1a0 <4> [557.275216] handle_irq+0x67/0x160 <4> [557.275229] do_IRQ+0x5e/0x130 <4> [557.275240] common_interrupt+0xf/0xf Precisely because the display is not powered down on request. -Chris
Quoting Chris Wilson (2019-04-08 21:50:28) > Quoting Jani Nikula (2018-10-22 10:00:39) > > On Mon, 22 Oct 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > Quoting Jani Nikula (2018-10-22 09:25:45) > > >> On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote: > > >> > Display features should not be initialized or deinitialized when > > >> > display is disabled. > > > > > > I completely disagree with this assertion. If the display is disabled, > > > so must all the associated hw so that we can power down the entire > > > chipset when idle. That means we have to complete the probe (so we > > > continue to rely on fuses and in place of accurate fuses pci-id quirks > > > for the infamous chipsets) and switch it off. > > > > That actually doesn't contradict with what I said about > > HAS_DISPLAY(). In many cases I think the early return on no display is > > the right thing to do. However, no display isn't the same as display > > disabled by module parameter (or whatnot)... which does require probe > > before disable to achieve the power down. > > > > But is the power down on display disable by module parameter a > > requirement for us? > > We still see this error in BAT roughly every day: > > <7> [557.273023] [drm:intel_power_well_enable [i915]] enabling display > <7> [557.273553] [drm:i915_redisable_vga_power_on [i915]] Something enabled VGA plane, disabling it > <4> [557.274207] ------------[ cut here ]------------ > <4> [557.274420] Unclaimed write to register 0x44200 > <4> [557.274637] WARNING: CPU: 2 PID: 370 at drivers/gpu/drm/i915/intel_uncore.c:1034 __unclaimed_reg_debug+0x40/0x50 [i915] > <4> [557.274643] Modules linked in: i915(+) amdgpu gpu_sched ttm vgem snd_hda_codec_hdmi coretemp btusb crct10dif_pclmul btrtl crc32_pclmul btbcm btintel ghash_clmulni_intel bluetooth cdc_ether usbnet r8152 mii ecdh_generic snd_hda_codec snd_hwdep snd_hda_core snd_pcm prime_numbers pinctrl_cherryview lpc_ich [last unloaded: i915] > <4> [557.274686] CPU: 2 PID: 370 Comm: rngd Tainted: G U 5.1.0-rc4-CI-CI_DRM_5891+ #1 > <4> [557.274690] Hardware name: GOOGLE Cyan/Cyan, BIOS MrChromebox 02/15/2018 > <4> [557.274829] RIP: 0010:__unclaimed_reg_debug+0x40/0x50 [i915] > <4> [557.274836] Code: 74 05 5b 5d 41 5c c3 45 84 e4 48 c7 c0 2b 6a 7b a0 48 c7 c6 21 6a 7b a0 48 0f 44 f0 89 ea 48 c7 c7 34 6a 7b a0 e8 b0 a3 a1 e0 <0f> 0b 83 2d 97 46 1b 00 01 5b 5d 41 5c c3 66 90 41 56 41 55 41 89 > <4> [557.274841] RSP: 0018:ffff888079903e30 EFLAGS: 00010082 > <4> [557.274847] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 > <4> [557.274851] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000000ffffffff > <4> [557.274856] RBP: 0000000000044200 R08: 0000000000000000 R09: 0000000000000001 > <4> [557.274860] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 > <4> [557.274865] R13: ffff88806d820eb0 R14: 0000000000000006 R15: 0000000000000000 > <4> [557.274870] FS: 00007f7c805d2740(0000) GS:ffff888079900000(0000) knlGS:0000000000000000 > <4> [557.274875] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > <4> [557.274879] CR2: 00005647db5bc7d8 CR3: 000000006476c000 CR4: 00000000001006e0 > <4> [557.274884] Call Trace: > <4> [557.274891] <IRQ> > <4> [557.275026] fwtable_write32+0x25f/0x2c0 [i915] > <4> [557.275149] cherryview_irq_handler+0x180/0x210 [i915] > <4> [557.275170] __handle_irq_event_percpu+0x41/0x2d0 > <4> [557.275177] ? handle_irq_event+0x27/0x50 > <4> [557.275188] handle_irq_event_percpu+0x2b/0x70 > <4> [557.275197] handle_irq_event+0x2f/0x50 > <4> [557.275207] handle_edge_irq+0xee/0x1a0 > <4> [557.275216] handle_irq+0x67/0x160 > <4> [557.275229] do_IRQ+0x5e/0x130 > <4> [557.275240] common_interrupt+0xf/0xf > > Precisely because the display is not powered down on request. We still see this. Every day. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 3f7514e981d5..8334d1797df7 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -899,7 +899,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) intel_wopcm_init_early(&dev_priv->wopcm); intel_uc_init_early(dev_priv); intel_pm_setup(dev_priv); - intel_init_dpio(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) + intel_init_dpio(dev_priv); ret = intel_power_domains_init(dev_priv); if (ret < 0) goto err_uc; @@ -907,8 +908,10 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) intel_hangcheck_init(dev_priv); intel_init_display_hooks(dev_priv); intel_init_clock_gating_hooks(dev_priv); - intel_init_audio_hooks(dev_priv); - intel_display_crc_init(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) { + intel_init_audio_hooks(dev_priv); + intel_display_crc_init(dev_priv); + } intel_detect_preproduction_hw(dev_priv); @@ -1539,23 +1542,26 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) if (IS_GEN5(dev_priv)) intel_gpu_ips_init(dev_priv); - intel_audio_init(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) { + intel_audio_init(dev_priv); - /* - * Some ports require correctly set-up hpd registers for detection to - * work properly (leading to ghost connected connector status), e.g. VGA - * on gm45. Hence we can only set up the initial fbdev config after hpd - * irqs are fully enabled. We do it last so that the async config - * cannot run before the connectors are registered. - */ - intel_fbdev_initial_config_async(dev); + /* + * Some ports require correctly set-up hpd registers for + * detection to work properly (leading to ghost connected + * connector status), e.g. VGA on gm45. Hence we can only set + * up the initial fbdev config after hpd irqs are fully enabled. + * We do it last so that the async config cannot run before the + * connectors are registered. + */ + intel_fbdev_initial_config_async(dev); - /* - * We need to coordinate the hotplugs with the asynchronous fbdev - * configuration, for which we use the fbdev->async_cookie. - */ - if (INTEL_INFO(dev_priv)->num_pipes) + /* + * We need to coordinate the hotplugs with the asynchronous + * fbdev configuration, for which we use the + * fbdev->async_cookie. + */ drm_kms_helper_poll_init(dev); + } intel_power_domains_enable(dev_priv); intel_runtime_pm_enable(dev_priv); @@ -1570,15 +1576,17 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) intel_runtime_pm_disable(dev_priv); intel_power_domains_disable(dev_priv); - intel_fbdev_unregister(dev_priv); - intel_audio_deinit(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) { + intel_fbdev_unregister(dev_priv); + intel_audio_deinit(dev_priv); - /* - * After flushing the fbdev (incl. a late async config which will - * have delayed queuing of a hotplug event), then flush the hotplug - * events. - */ - drm_kms_helper_poll_fini(&dev_priv->drm); + /* + * After flushing the fbdev (incl. a late async config which + * will have delayed queuing of a hotplug event), then flush the + * hotplug events. + */ + drm_kms_helper_poll_fini(&dev_priv->drm); + } intel_gpu_ips_teardown(); acpi_video_unregister(); @@ -1641,6 +1649,7 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent) if (i915_modparams.disable_display) { DRM_INFO("Display disabled (module parameter)\n"); device_info->num_pipes = 0; + i915->drm.driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC); } BUILD_BUG_ON(INTEL_MAX_PLATFORMS > @@ -1721,9 +1730,11 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret) goto cleanup_irq; - ret = i915_load_modeset_init(&dev_priv->drm); - if (ret < 0) - goto cleanup_gem; + if (INTEL_INFO(dev_priv)->num_pipes) { + ret = i915_load_modeset_init(&dev_priv->drm); + if (ret < 0) + goto cleanup_gem; + } i915_driver_register(dev_priv); @@ -1780,11 +1791,13 @@ void i915_driver_unload(struct drm_device *dev) if (i915_gem_suspend(dev_priv)) DRM_ERROR("failed to idle hardware; continuing to unload!\n"); - drm_atomic_helper_shutdown(dev); + if (INTEL_INFO(dev_priv)->num_pipes) + drm_atomic_helper_shutdown(dev); intel_gvt_cleanup(dev_priv); - intel_modeset_cleanup_prepare(dev); + if (INTEL_INFO(dev_priv)->num_pipes) + intel_modeset_cleanup_prepare(dev); /* * Interrupts and polling as the first thing to avoid creating havoc. @@ -1793,9 +1806,11 @@ void i915_driver_unload(struct drm_device *dev) */ intel_irq_uninstall(dev_priv); - intel_modeset_cleanup(dev); + if (INTEL_INFO(dev_priv)->num_pipes) { + intel_modeset_cleanup(dev); - i915_modeset_unload(dev); + i915_modeset_unload(dev); + } /* Free error state after interrupts are fully disabled. */ cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); @@ -1847,8 +1862,12 @@ static int i915_driver_open(struct drm_device *dev, struct drm_file *file) */ static void i915_driver_lastclose(struct drm_device *dev) { - intel_fbdev_restore_mode(dev); - vga_switcheroo_process_delayed_switch(); + struct drm_i915_private *dev_priv = to_i915(dev); + + if (INTEL_INFO(dev_priv)->num_pipes) { + intel_fbdev_restore_mode(dev); + vga_switcheroo_process_delayed_switch(); + } } static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file) @@ -1918,19 +1937,24 @@ static int i915_drm_suspend(struct drm_device *dev) /* We do a lot of poking in a lot of registers, make sure they work * properly. */ intel_power_domains_disable(dev_priv); - - drm_kms_helper_poll_disable(dev); + if (INTEL_INFO(dev_priv)->num_pipes) + drm_kms_helper_poll_disable(dev); pci_save_state(pdev); - intel_display_suspend(dev); + if (INTEL_INFO(dev_priv)->num_pipes) { + intel_display_suspend(dev); - intel_dp_mst_suspend(dev_priv); + intel_dp_mst_suspend(dev_priv); + } intel_runtime_pm_disable_interrupts(dev_priv); - intel_hpd_cancel_work(dev_priv); - intel_suspend_encoders(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) { + intel_hpd_cancel_work(dev_priv); + + intel_suspend_encoders(dev_priv); + } intel_suspend_hw(dev_priv); @@ -1943,11 +1967,13 @@ static int i915_drm_suspend(struct drm_device *dev) intel_opregion_unregister(dev_priv); - intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true); + if (INTEL_INFO(dev_priv)->num_pipes) + intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true); dev_priv->suspend_count++; - intel_csr_ucode_suspend(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) + intel_csr_ucode_suspend(dev_priv); enable_rpm_wakeref_asserts(dev_priv); @@ -2056,10 +2082,12 @@ static int i915_drm_resume(struct drm_device *dev) if (ret) DRM_ERROR("failed to re-enable GGTT\n"); - intel_csr_ucode_resume(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) + intel_csr_ucode_resume(dev_priv); i915_restore_state(dev_priv); - intel_pps_unlock_regs_wa(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) + intel_pps_unlock_regs_wa(dev_priv); intel_opregion_setup(dev_priv); intel_init_pch_refclk(dev_priv); @@ -2076,7 +2104,8 @@ static int i915_drm_resume(struct drm_device *dev) */ intel_runtime_pm_enable_interrupts(dev_priv); - drm_mode_config_reset(dev); + if (INTEL_INFO(dev_priv)->num_pipes) + drm_mode_config_reset(dev); i915_gem_resume(dev_priv); @@ -2084,27 +2113,30 @@ static int i915_drm_resume(struct drm_device *dev) intel_init_clock_gating(dev_priv); spin_lock_irq(&dev_priv->irq_lock); - if (dev_priv->display.hpd_irq_setup) + if (dev_priv->display.hpd_irq_setup && INTEL_INFO(dev_priv)->num_pipes) dev_priv->display.hpd_irq_setup(dev_priv); spin_unlock_irq(&dev_priv->irq_lock); - intel_dp_mst_resume(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) { + intel_dp_mst_resume(dev_priv); - intel_display_resume(dev); + intel_display_resume(dev); - drm_kms_helper_poll_enable(dev); + drm_kms_helper_poll_enable(dev); - /* - * ... but also need to make sure that hotplug processing - * doesn't cause havoc. Like in the driver load code we don't - * bother with the tiny race here where we might lose hotplug - * notifications. - * */ - intel_hpd_init(dev_priv); + /* + * ... but also need to make sure that hotplug processing + * doesn't cause havoc. Like in the driver load code we don't + * bother with the tiny race here where we might lose hotplug + * notifications. + */ + intel_hpd_init(dev_priv); + } intel_opregion_register(dev_priv); - intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false); + if (INTEL_INFO(dev_priv)->num_pipes) + intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false); intel_opregion_notify_adapter(dev_priv, PCI_D0); @@ -3000,7 +3032,8 @@ static int intel_runtime_suspend(struct device *kdev) assert_forcewakes_inactive(dev_priv); - if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) + if (INTEL_INFO(dev_priv)->num_pipes && (!IS_VALLEYVIEW(dev_priv) && + !IS_CHERRYVIEW(dev_priv))) intel_hpd_poll_init(dev_priv); DRM_DEBUG_KMS("Device suspended\n"); @@ -3057,10 +3090,12 @@ static int intel_runtime_resume(struct device *kdev) * power well, so hpd is reinitialized from there. For * everyone else do it here. */ - if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) + if (INTEL_INFO(dev_priv)->num_pipes && (!IS_VALLEYVIEW(dev_priv) && + !IS_CHERRYVIEW(dev_priv))) intel_hpd_init(dev_priv); - intel_enable_ipc(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) + intel_enable_ipc(dev_priv); enable_rpm_wakeref_asserts(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c index 8f3aa4dc0c98..f697865236a6 100644 --- a/drivers/gpu/drm/i915/i915_suspend.c +++ b/drivers/gpu/drm/i915/i915_suspend.c @@ -63,11 +63,13 @@ int i915_save_state(struct drm_i915_private *dev_priv) mutex_lock(&dev_priv->drm.struct_mutex); - i915_save_display(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) { + i915_save_display(dev_priv); - if (IS_GEN4(dev_priv)) - pci_read_config_word(pdev, GCDGMBUS, - &dev_priv->regfile.saveGCDGMBUS); + if (IS_GEN4(dev_priv)) + pci_read_config_word(pdev, GCDGMBUS, + &dev_priv->regfile.saveGCDGMBUS); + } /* Cache mode state */ if (INTEL_GEN(dev_priv) < 7) @@ -108,10 +110,13 @@ int i915_restore_state(struct drm_i915_private *dev_priv) mutex_lock(&dev_priv->drm.struct_mutex); - if (IS_GEN4(dev_priv)) - pci_write_config_word(pdev, GCDGMBUS, - dev_priv->regfile.saveGCDGMBUS); - i915_restore_display(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) { + if (IS_GEN4(dev_priv)) + pci_write_config_word(pdev, GCDGMBUS, + dev_priv->regfile.saveGCDGMBUS); + + i915_restore_display(dev_priv); + } /* Cache mode state */ if (INTEL_GEN(dev_priv) < 7) @@ -143,7 +148,8 @@ int i915_restore_state(struct drm_i915_private *dev_priv) mutex_unlock(&dev_priv->drm.struct_mutex); - intel_i2c_reset(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) + intel_i2c_reset(dev_priv); return 0; } diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 3cf8533e0834..15fd88bb35f7 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -625,7 +625,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv) DRM_DEBUG_KMS("Enabling DC9\n"); - intel_power_sequencer_reset(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) + intel_power_sequencer_reset(dev_priv); gen9_set_dc_state(dev_priv, DC_STATE_EN_DC9); } @@ -1039,10 +1040,12 @@ static void vlv_display_power_well_deinit(struct drm_i915_private *dev_priv) /* make sure we're done processing display irqs */ synchronize_irq(dev_priv->drm.irq); - intel_power_sequencer_reset(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) + intel_power_sequencer_reset(dev_priv); /* Prevent us from re-enabling polling on accident in late suspend */ - if (!dev_priv->drm.dev->power.is_suspended) + if (INTEL_INFO(dev_priv)->num_pipes && + !dev_priv->drm.dev->power.is_suspended) intel_hpd_poll_init(dev_priv); } @@ -1284,11 +1287,14 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv, if (power_well->desc->id == VLV_DISP_PW_DPIO_CMN_BC) { phy = DPIO_PHY0; - assert_pll_disabled(dev_priv, PIPE_A); - assert_pll_disabled(dev_priv, PIPE_B); + if (INTEL_INFO(dev_priv)->num_pipes) { + assert_pll_disabled(dev_priv, PIPE_A); + assert_pll_disabled(dev_priv, PIPE_B); + } } else { phy = DPIO_PHY1; - assert_pll_disabled(dev_priv, PIPE_C); + if (INTEL_INFO(dev_priv)->num_pipes) + assert_pll_disabled(dev_priv, PIPE_C); } dev_priv->chv_phy_control &= ~PHY_COM_LANE_RESET_DEASSERT(phy); @@ -1302,7 +1308,8 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv, /* PHY is fully reset now, so we can enable the PHY state asserts */ dev_priv->chv_phy_assert[phy] = true; - assert_chv_phy_status(dev_priv); + if (INTEL_INFO(dev_priv)->num_pipes) + assert_chv_phy_status(dev_priv); } static void assert_chv_phy_powergate(struct drm_i915_private *dev_priv, enum dpio_phy phy,
Display features should not be initialized or deinitialized when display is disabled. With this changes no plane, CRTC, encoder or connector is being registered in drm when display is disabled so it was also necessary unset DRIVER_MODESET and DRIVER_ATOMIC features from driver otherwise it will crash when registering driver in drm. There is still more modeset/display calls that will be removed in futher patches. Signed-off-by: José Roberto de Souza <jose.souza@intel.com> squash do not call modeset --- drivers/gpu/drm/i915/i915_drv.c | 155 +++++++++++++++--------- drivers/gpu/drm/i915/i915_suspend.c | 24 ++-- drivers/gpu/drm/i915/intel_runtime_pm.c | 21 ++-- 3 files changed, 124 insertions(+), 76 deletions(-)