Re: [Intel-gfx] [PATCH 3/4] drm/i915: Disable displays at the user's request
On Fri, Oct 19, 2018 at 09:39:22AM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2018-10-19 09:22:15) > > On Mon, Oct 15, 2018 at 12:17:41PM +0100, Chris Wilson wrote: > > > If the user passes i915.disable_display=1 we want to disable all the > > > displays and associated HW like the powerwells on their behalf. Instead > > > of short circuiting the HW probe, let it run and setup all the > > > bookkeeping for the known HW. Afterwards, instead of taking over the > > > BIOS fb and installing the fbcon, we shutdown all the outputs and > > > teardown the bookkeeping, leaving us with no attached outputs or crtcs, > > > and all the HW powered down. > > > > > > Open: wq flushes should be required but seem to deadlock the modprobe > > > under CI. > > > > > > Signed-off-by: Chris Wilson > > > Cc: Imre Deak > > > Cc: Ville Syrjälä > > > > i915.disable_display was for those server chips where doing all the init > > resulted in a dead machine. So not sure we want this. > > For those server chips, we don't use i915.disable_display but detect when > the fuses are lies and directly set num_pipes == 0. If we had such a > machine in CI, you would already have seen a lot of the fun with KMS being > allowed without any backing hw. Hence why Ville suggested we disable KMS > for machines without pipes to avoid having to add a lot of defense > around the driver. > > > What's the issue with power wells still being on and all that? On real hw > > without display they won't exist, and I don't understand why we'd care for > > testing. > > For testing. We do use .disable_display and expect rpm to still work, and > to not get random display related failures interfering in displayless > tests. > > Quite clearly we haven't been testing the displayless setups at all. I definitely like the idea of testing this without requiring special hardware. I guess another way to achieve the result of turning everything off would be to 'modprobe i915 disable_display=0; rmmod i915; modprobe i915 disable_display=1'. That should avoid the need to have a special codepath for shutthing things down. Would that suffice or is there a compelling reason for supporting this without requiring the driver reload? -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Disable displays at the user's request
Quoting Daniel Vetter (2018-10-19 09:22:15) > On Mon, Oct 15, 2018 at 12:17:41PM +0100, Chris Wilson wrote: > > If the user passes i915.disable_display=1 we want to disable all the > > displays and associated HW like the powerwells on their behalf. Instead > > of short circuiting the HW probe, let it run and setup all the > > bookkeeping for the known HW. Afterwards, instead of taking over the > > BIOS fb and installing the fbcon, we shutdown all the outputs and > > teardown the bookkeeping, leaving us with no attached outputs or crtcs, > > and all the HW powered down. > > > > Open: wq flushes should be required but seem to deadlock the modprobe > > under CI. > > > > Signed-off-by: Chris Wilson > > Cc: Imre Deak > > Cc: Ville Syrjälä > > i915.disable_display was for those server chips where doing all the init > resulted in a dead machine. So not sure we want this. For those server chips, we don't use i915.disable_display but detect when the fuses are lies and directly set num_pipes == 0. If we had such a machine in CI, you would already have seen a lot of the fun with KMS being allowed without any backing hw. Hence why Ville suggested we disable KMS for machines without pipes to avoid having to add a lot of defense around the driver. > What's the issue with power wells still being on and all that? On real hw > without display they won't exist, and I don't understand why we'd care for > testing. For testing. We do use .disable_display and expect rpm to still work, and to not get random display related failures interfering in displayless tests. Quite clearly we haven't been testing the displayless setups at all. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Disable displays at the user's request
On Mon, Oct 15, 2018 at 12:17:41PM +0100, Chris Wilson wrote:
> If the user passes i915.disable_display=1 we want to disable all the
> displays and associated HW like the powerwells on their behalf. Instead
> of short circuiting the HW probe, let it run and setup all the
> bookkeeping for the known HW. Afterwards, instead of taking over the
> BIOS fb and installing the fbcon, we shutdown all the outputs and
> teardown the bookkeeping, leaving us with no attached outputs or crtcs,
> and all the HW powered down.
>
> Open: wq flushes should be required but seem to deadlock the modprobe
> under CI.
>
> Signed-off-by: Chris Wilson
> Cc: Imre Deak
> Cc: Ville Syrjälä
i915.disable_display was for those server chips where doing all the init
resulted in a dead machine. So not sure we want this.
What's the issue with power wells still being on and all that? On real hw
without display they won't exist, and I don't understand why we'd care for
testing.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.c | 53 +---
> drivers/gpu/drm/i915/intel_device_info.c | 9 ++--
> drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
> 3 files changed, 43 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index c14855f167b9..d71add64948b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -689,22 +689,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
>
> intel_setup_overlay(dev_priv);
>
> - if (INTEL_INFO(dev_priv)->num_pipes == 0)
> - return 0;
> -
> - ret = intel_fbdev_init(dev_priv);
> - if (ret)
> - goto cleanup_gem;
> -
> - /* Only enable hotplug handling once the fbdev is fully set up. */
> - intel_hpd_init(dev_priv);
> -
> return 0;
>
> -cleanup_gem:
> - if (i915_gem_suspend(dev_priv))
> - DRM_ERROR("failed to idle hardware; continuing to unload!\n");
> - i915_gem_fini(dev_priv);
> cleanup_modeset:
> intel_modeset_cleanup(dev);
> cleanup_irq:
> @@ -1667,6 +1653,17 @@ static void i915_driver_destroy(struct
> drm_i915_private *i915)
> pci_set_drvdata(pdev, NULL);
> }
>
> +static void disable_display(struct drm_i915_private *i915)
> +{
> + drm_atomic_helper_shutdown(>drm);
> +
> +#if 0 /* XXX flushes deadlock under modprobe??? */
> + flush_workqueue(i915->modeset_wq);
> + flush_work(>atomic_helper.free_work);
> + flush_scheduled_work();
> +#endif
> +}
> +
> /**
> * i915_driver_load - setup chip and create an initial config
> * @pdev: PCI device
> @@ -1727,6 +1724,34 @@ int i915_driver_load(struct pci_dev *pdev, const
> struct pci_device_id *ent)
> if (ret < 0)
> goto out_cleanup_hw;
>
> + /*
> + * After completing our HW probe; tear it all down again (at the
> + * user's request)!
> + *
> + * Along side the CRTCs and connectors, there is a medley of
> + * auxiliary HW which control various powerwells and interact with
> + * other state (such as the BIOS framebuffer occupying a portion
> + * of reserved memory). If the user tells us to run without any
> + * displays enabled, we still need to register all the display and
> + * auxiliary HW in order to safely disable them.
> + */
> + if (i915_modparams.disable_display) {
> + DRM_INFO("Display disabled (module parameter)\n");
> + disable_display(dev_priv);
> + mkwrite_device_info(dev_priv)->num_pipes = 0;
> + }
> +
> + if (INTEL_INFO(dev_priv)->num_pipes == 0) {
> + drm_mode_config_cleanup(_priv->drm);
> + dev_priv->drm.driver_features &=
> + ~(DRIVER_MODESET | DRIVER_ATOMIC);
> + dev_priv->psr.sink_support = false;
> + }
> +
> + /* Only enable hotplug handling once the fbdev is fully set up. */
> + if (intel_fbdev_init(dev_priv) == 0)
> + intel_hpd_init(dev_priv);
> +
> i915_driver_register(dev_priv);
>
> intel_init_ipc(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> b/drivers/gpu/drm/i915/intel_device_info.c
> index 03df4e33763d..5f6b12986ce9 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -775,12 +775,9 @@ void intel_device_info_runtime_init(struct
> intel_device_info *info)
> info->num_sprites[pipe] = 1;
> }
>
> - if (i915_modparams.disable_display) {
> - DRM_INFO("Display disabled (module parameter)\n");
> - info->num_pipes = 0;
> - } else if (info->num_pipes > 0 &&
> -(IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) &&
> -HAS_PCH_SPLIT(dev_priv)) {
> + if (info->num_pipes > 0 &&
> + (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) &&
> + HAS_PCH_SPLIT(dev_priv)) {
> u32 fuse_strap =
[Intel-gfx] [PATCH 3/4] drm/i915: Disable displays at the user's request
If the user passes i915.disable_display=1 we want to disable all the
displays and associated HW like the powerwells on their behalf. Instead
of short circuiting the HW probe, let it run and setup all the
bookkeeping for the known HW. Afterwards, instead of taking over the
BIOS fb and installing the fbcon, we shutdown all the outputs and
teardown the bookkeeping, leaving us with no attached outputs or crtcs,
and all the HW powered down.
Open: wq flushes should be required but seem to deadlock the modprobe
under CI.
Signed-off-by: Chris Wilson
Cc: Imre Deak
Cc: Ville Syrjälä
---
drivers/gpu/drm/i915/i915_drv.c | 53 +---
drivers/gpu/drm/i915/intel_device_info.c | 9 ++--
drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
3 files changed, 43 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c14855f167b9..d71add64948b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -689,22 +689,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
intel_setup_overlay(dev_priv);
- if (INTEL_INFO(dev_priv)->num_pipes == 0)
- return 0;
-
- ret = intel_fbdev_init(dev_priv);
- if (ret)
- goto cleanup_gem;
-
- /* Only enable hotplug handling once the fbdev is fully set up. */
- intel_hpd_init(dev_priv);
-
return 0;
-cleanup_gem:
- if (i915_gem_suspend(dev_priv))
- DRM_ERROR("failed to idle hardware; continuing to unload!\n");
- i915_gem_fini(dev_priv);
cleanup_modeset:
intel_modeset_cleanup(dev);
cleanup_irq:
@@ -1667,6 +1653,17 @@ static void i915_driver_destroy(struct drm_i915_private
*i915)
pci_set_drvdata(pdev, NULL);
}
+static void disable_display(struct drm_i915_private *i915)
+{
+ drm_atomic_helper_shutdown(>drm);
+
+#if 0 /* XXX flushes deadlock under modprobe??? */
+ flush_workqueue(i915->modeset_wq);
+ flush_work(>atomic_helper.free_work);
+ flush_scheduled_work();
+#endif
+}
+
/**
* i915_driver_load - setup chip and create an initial config
* @pdev: PCI device
@@ -1727,6 +1724,34 @@ int i915_driver_load(struct pci_dev *pdev, const struct
pci_device_id *ent)
if (ret < 0)
goto out_cleanup_hw;
+ /*
+* After completing our HW probe; tear it all down again (at the
+* user's request)!
+*
+* Along side the CRTCs and connectors, there is a medley of
+* auxiliary HW which control various powerwells and interact with
+* other state (such as the BIOS framebuffer occupying a portion
+* of reserved memory). If the user tells us to run without any
+* displays enabled, we still need to register all the display and
+* auxiliary HW in order to safely disable them.
+*/
+ if (i915_modparams.disable_display) {
+ DRM_INFO("Display disabled (module parameter)\n");
+ disable_display(dev_priv);
+ mkwrite_device_info(dev_priv)->num_pipes = 0;
+ }
+
+ if (INTEL_INFO(dev_priv)->num_pipes == 0) {
+ drm_mode_config_cleanup(_priv->drm);
+ dev_priv->drm.driver_features &=
+ ~(DRIVER_MODESET | DRIVER_ATOMIC);
+ dev_priv->psr.sink_support = false;
+ }
+
+ /* Only enable hotplug handling once the fbdev is fully set up. */
+ if (intel_fbdev_init(dev_priv) == 0)
+ intel_hpd_init(dev_priv);
+
i915_driver_register(dev_priv);
intel_init_ipc(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_device_info.c
b/drivers/gpu/drm/i915/intel_device_info.c
index 03df4e33763d..5f6b12986ce9 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -775,12 +775,9 @@ void intel_device_info_runtime_init(struct
intel_device_info *info)
info->num_sprites[pipe] = 1;
}
- if (i915_modparams.disable_display) {
- DRM_INFO("Display disabled (module parameter)\n");
- info->num_pipes = 0;
- } else if (info->num_pipes > 0 &&
- (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) &&
- HAS_PCH_SPLIT(dev_priv)) {
+ if (info->num_pipes > 0 &&
+ (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) &&
+ HAS_PCH_SPLIT(dev_priv)) {
u32 fuse_strap = I915_READ(FUSE_STRAP);
u32 sfuse_strap = I915_READ(SFUSE_STRAP);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
b/drivers/gpu/drm/i915/intel_fbdev.c
index a75082813669..5442a13bba63 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -677,7 +677,7 @@ int intel_fbdev_init(struct drm_i915_private *i915)
struct intel_fbdev *ifbdev;
int ret;
- if (WARN_ON(INTEL_INFO(i915)->num_pipes == 0))
+ if
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Disable displays at the user's request
Quoting Chris Wilson (2018-09-13 14:16:28)
> + /*
> +* After completing our HW probe; tear it all down again (at the
> +* user's request)!
> +*
> +* Along side the CRTCs and connectors, there is a medley of
> +* auxiliary HW which control various powerwells and interact with
> +* other state (such as the BIOS framebuffer occupying a portion
> +* of reserved memory). If the user tells us to run without any
> +* displays enabled, we still need to register all the display and
> +* auxiliary HW in order to safely disable them.
> +*/
> + if (i915_modparams.disable_display) {
> + DRM_INFO("Display disabled (module parameter)\n");
> + disable_display(dev_priv);
> + mkwrite_device_info(dev_priv)->num_pipes = 0;
> + }
> +
> + if (INTEL_INFO(dev_priv)->num_pipes == 0) {
> + dev_priv->psr.sink_support = false;
> + drm_mode_config_cleanup(_priv->drm);
> + driver.driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
Removing the feature seems like a good idea on surface (as we then don't
have to worry about hitting internals that expect crtcs and planes and
such), however more work required in declaring igt requirements than
simply having no pipes :)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] drm/i915: Disable displays at the user's request
If the user passes i915.disable_display=1 we want to disable all the
displays and associated HW like the powerwells on their behalf. Instead
of short circuiting the HW probe, let it run and setup all the
bookkeeping for the known HW. Afterwards, instead of taking over the
BIOS fb and installing the fbcon, we shutdown all the outputs and
teardown the bookkeeping, leaving us with no attached outputs or crtcs,
and all the HW powered down.
Open: wq flushes should be required but seem to deadlock the modprobe
under CI.
Signed-off-by: Chris Wilson
Cc: Imre Deak
Cc: Ville Syrjälä
---
drivers/gpu/drm/i915/i915_drv.c | 52 +---
drivers/gpu/drm/i915/intel_device_info.c | 9 ++--
drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
3 files changed, 42 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a74de4428c79..f34a1f5b99e5 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -689,22 +689,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
intel_setup_overlay(dev_priv);
- if (INTEL_INFO(dev_priv)->num_pipes == 0)
- return 0;
-
- ret = intel_fbdev_init(dev_priv);
- if (ret)
- goto cleanup_gem;
-
- /* Only enable hotplug handling once the fbdev is fully set up. */
- intel_hpd_init(dev_priv);
-
return 0;
-cleanup_gem:
- if (i915_gem_suspend(dev_priv))
- DRM_ERROR("failed to idle hardware; continuing to unload!\n");
- i915_gem_fini(dev_priv);
cleanup_modeset:
intel_modeset_cleanup(dev);
cleanup_irq:
@@ -1366,6 +1352,17 @@ static void i915_driver_destroy(struct drm_i915_private
*i915)
pci_set_drvdata(pdev, NULL);
}
+static void disable_display(struct drm_i915_private *i915)
+{
+ drm_atomic_helper_shutdown(>drm);
+
+#if 0 /* XXX flushes deadlock under modprobe??? */
+ flush_workqueue(i915->modeset_wq);
+ flush_work(>atomic_helper.free_work);
+ flush_scheduled_work();
+#endif
+}
+
/**
* i915_driver_load - setup chip and create an initial config
* @pdev: PCI device
@@ -1426,6 +1423,33 @@ int i915_driver_load(struct pci_dev *pdev, const struct
pci_device_id *ent)
if (ret < 0)
goto out_cleanup_hw;
+ /*
+* After completing our HW probe; tear it all down again (at the
+* user's request)!
+*
+* Along side the CRTCs and connectors, there is a medley of
+* auxiliary HW which control various powerwells and interact with
+* other state (such as the BIOS framebuffer occupying a portion
+* of reserved memory). If the user tells us to run without any
+* displays enabled, we still need to register all the display and
+* auxiliary HW in order to safely disable them.
+*/
+ if (i915_modparams.disable_display) {
+ DRM_INFO("Display disabled (module parameter)\n");
+ disable_display(dev_priv);
+ mkwrite_device_info(dev_priv)->num_pipes = 0;
+ }
+
+ if (INTEL_INFO(dev_priv)->num_pipes == 0) {
+ dev_priv->psr.sink_support = false;
+ drm_mode_config_cleanup(_priv->drm);
+ driver.driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
+ }
+
+ /* Only enable hotplug handling once the fbdev is fully set up. */
+ if (intel_fbdev_init(dev_priv) == 0)
+ intel_hpd_init(dev_priv);
+
i915_driver_register(dev_priv);
intel_init_ipc(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_device_info.c
b/drivers/gpu/drm/i915/intel_device_info.c
index 0ef0c6448d53..e50ea5b2576b 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -776,12 +776,9 @@ void intel_device_info_runtime_init(struct
intel_device_info *info)
info->num_sprites[pipe] = 1;
}
- if (i915_modparams.disable_display) {
- DRM_INFO("Display disabled (module parameter)\n");
- info->num_pipes = 0;
- } else if (info->num_pipes > 0 &&
- (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) &&
- HAS_PCH_SPLIT(dev_priv)) {
+ if (info->num_pipes > 0 &&
+ (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) &&
+ HAS_PCH_SPLIT(dev_priv)) {
u32 fuse_strap = I915_READ(FUSE_STRAP);
u32 sfuse_strap = I915_READ(SFUSE_STRAP);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
b/drivers/gpu/drm/i915/intel_fbdev.c
index 84ebd8102215..9dd50c5ec558 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -668,7 +668,7 @@ int intel_fbdev_init(struct drm_i915_private *i915)
struct intel_fbdev *ifbdev;
int ret;
- if (WARN_ON(INTEL_INFO(i915)->num_pipes == 0))
+ if (INTEL_INFO(i915)->num_pipes == 0)
