| Message ID | 20240104083008.2715733-8-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix HPD handling during driver init/shutdown | expand |
On Thu, 2024-01-04 at 10:30 +0200, Imre Deak wrote: > An unexpected modeset or connector detection by a user (user space or > FB > console) during the initialization/shutdown sequence is possible > either > via a hotplug IRQ handling work or via the connector sysfs > (status/detect) interface. These modesets/detections should be > prevented > by disabling/flushing all related hotplug handling work and > unregistering the interfaces that can start them at the beginning of > the > shutdown sequence. Some of this - disabling all related intel_hotplug > work - will be done by the next patch, but others - for instance > disabling the MST hotplug works - require a bigger rework. > > It makes sense - for diagnostic purpose, even with all the above work > and > interface disabled - to detect and reject any such user access. This > patch does that for modeset accesses and a follow-up patch for > connector > detection. > > After the display is disabled during the shutdown sequence, no > modeset > should happen so it's disabled for both users and the shutdown > thread. Can you please explain in commit message why intel_display_driver_disable_user_access and intel_display_driver_resume_access are allowing modeset for "current" process? BR, Jouni Högander > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 3 + > .../gpu/drm/i915/display/intel_display_core.h | 7 ++ > .../drm/i915/display/intel_display_driver.c | 74 > +++++++++++++++++++ > .../drm/i915/display/intel_display_driver.h | 6 ++ > drivers/gpu/drm/i915/i915_driver.c | 16 +++- > 5 files changed, 104 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 927d124457b61..31a6a82c12616 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -6310,6 +6310,9 @@ int intel_atomic_check(struct drm_device *dev, > int ret, i; > bool any_ms = false; > > + if (!intel_display_driver_check_access(dev_priv)) > + return -ENODEV; > + > for_each_oldnew_intel_crtc_in_state(state, crtc, > old_crtc_state, > new_crtc_state, i) { > /* > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h > b/drivers/gpu/drm/i915/display/intel_display_core.h > index 47297ed858223..0b130ca9e6698 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_core.h > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > @@ -28,6 +28,8 @@ > #include "intel_opregion.h" > #include "intel_wm_types.h" > > +struct task_struct; > + > struct drm_i915_private; > struct drm_property; > struct drm_property_blob; > @@ -298,6 +300,11 @@ struct intel_display { > const struct intel_audio_funcs *audio; > } funcs; > > + struct { > + bool any_task_allowed; > + struct task_struct *allowed_task; > + } access; > + > struct { > /* backlight registers and fields in struct > intel_panel */ > struct mutex lock; > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c > b/drivers/gpu/drm/i915/display/intel_display_driver.c > index 1974f2394a518..b2441ab9822c2 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > @@ -45,6 +45,7 @@ > #include "intel_hdcp.h" > #include "intel_hotplug.h" > #include "intel_hti.h" > +#include "intel_modeset_lock.h" > #include "intel_modeset_setup.h" > #include "intel_opregion.h" > #include "intel_overlay.h" > @@ -276,6 +277,71 @@ int intel_display_driver_probe_noirq(struct > drm_i915_private *i915) > return ret; > } > > +static void set_display_access(struct drm_i915_private *i915, > + bool any_task_allowed, > + struct task_struct *allowed_task) > +{ > + struct drm_modeset_acquire_ctx ctx; > + int err; > + > + intel_modeset_lock_ctx_retry(&ctx, NULL, 0, err) { > + err = drm_modeset_lock_all_ctx(&i915->drm, &ctx); > + if (err) > + continue; > + > + i915->display.access.any_task_allowed = > any_task_allowed; > + i915->display.access.allowed_task = allowed_task; > + } > + > + drm_WARN_ON(&i915->drm, err); > +} > + > +void intel_display_driver_enable_user_access(struct drm_i915_private > *i915) > +{ > + set_display_access(i915, true, NULL); > +} > + > +void intel_display_driver_disable_user_access(struct > drm_i915_private *i915) > +{ > + set_display_access(i915, false, current); > +} > + > +void intel_display_driver_suspend_access(struct drm_i915_private > *i915) > +{ > + set_display_access(i915, false, NULL); > +} > + > +void intel_display_driver_resume_access(struct drm_i915_private > *i915) > +{ > + set_display_access(i915, false, current); > +} > + > +bool intel_display_driver_check_access(struct drm_i915_private > *i915) > +{ > + char comm[TASK_COMM_LEN]; > + char current_task[TASK_COMM_LEN + 16]; > + char allowed_task[TASK_COMM_LEN + 16] = "none"; > + > + if (i915->display.access.any_task_allowed || > + i915->display.access.allowed_task == current) > + return true; > + > + snprintf(current_task, sizeof(current_task), "%s[%d]", > + get_task_comm(comm, current), > + task_pid_vnr(current)); > + > + if (i915->display.access.allowed_task) > + snprintf(allowed_task, sizeof(allowed_task), > "%s[%d]", > + get_task_comm(comm, i915- > >display.access.allowed_task), > + task_pid_vnr(i915- > >display.access.allowed_task)); > + > + drm_dbg_kms(&i915->drm, > + "Reject display access from task %s (allowed to > %s)\n", > + current_task, allowed_task); > + > + return false; > +} > + > /* part #2: call after irq install, but before gem init */ > int intel_display_driver_probe_nogem(struct drm_i915_private *i915) > { > @@ -326,6 +392,8 @@ int intel_display_driver_probe_nogem(struct > drm_i915_private *i915) > intel_vga_disable(i915); > intel_setup_outputs(i915); > > + intel_display_driver_disable_user_access(i915); > + > drm_modeset_lock_all(dev); > intel_modeset_setup_hw_state(i915, dev- > >mode_config.acquire_ctx); > intel_acpi_assign_connector_fwnodes(i915); > @@ -393,6 +461,8 @@ void intel_display_driver_register(struct > drm_i915_private *i915) > > intel_audio_init(i915); > > + intel_display_driver_enable_user_access(i915); > + > intel_display_debugfs_register(i915); > > /* > @@ -440,6 +510,8 @@ void intel_display_driver_remove_noirq(struct > drm_i915_private *i915) > if (!HAS_DISPLAY(i915)) > return; > > + intel_display_driver_suspend_access(i915); > + > /* > * Due to the hpd irq storm handling the hotplug work can re- > arm the > * poll handlers. Hence disable polling after hpd handling is > shut down. > @@ -493,6 +565,8 @@ void intel_display_driver_unregister(struct > drm_i915_private *i915) > */ > drm_kms_helper_poll_fini(&i915->drm); > > + intel_display_driver_disable_user_access(i915); > + > intel_audio_deinit(i915); > > drm_atomic_helper_shutdown(&i915->drm); > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.h > b/drivers/gpu/drm/i915/display/intel_display_driver.h > index c276a58ee3293..42cc4af6d3fd5 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_driver.h > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.h > @@ -32,5 +32,11 @@ int __intel_display_driver_resume(struct > drm_i915_private *i915, > struct drm_atomic_state *state, > struct drm_modeset_acquire_ctx > *ctx); > > +void intel_display_driver_enable_user_access(struct drm_i915_private > *i915); > +void intel_display_driver_disable_user_access(struct > drm_i915_private *i915); > +void intel_display_driver_suspend_access(struct drm_i915_private > *i915); > +void intel_display_driver_resume_access(struct drm_i915_private > *i915); > +bool intel_display_driver_check_access(struct drm_i915_private > *i915); > + > #endif /* __INTEL_DISPLAY_DRIVER_H__ */ > > diff --git a/drivers/gpu/drm/i915/i915_driver.c > b/drivers/gpu/drm/i915/i915_driver.c > index 166476948c896..068ca48e05323 100644 > --- a/drivers/gpu/drm/i915/i915_driver.c > +++ b/drivers/gpu/drm/i915/i915_driver.c > @@ -1006,6 +1006,7 @@ void i915_driver_shutdown(struct > drm_i915_private *i915) > intel_fbdev_set_suspend(&i915->drm, FBINFO_STATE_SUSPENDED, > true); > if (HAS_DISPLAY(i915)) { > drm_kms_helper_poll_disable(&i915->drm); > + intel_display_driver_disable_user_access(i915); > > drm_atomic_helper_shutdown(&i915->drm); > } > @@ -1015,6 +1016,8 @@ void i915_driver_shutdown(struct > drm_i915_private *i915) > intel_runtime_pm_disable_interrupts(i915); > intel_hpd_cancel_work(i915); > > + intel_display_driver_suspend_access(i915); > + > intel_suspend_encoders(i915); > intel_shutdown_encoders(i915); > > @@ -1082,8 +1085,10 @@ static int i915_drm_suspend(struct drm_device > *dev) > * properly. */ > intel_power_domains_disable(dev_priv); > intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true); > - if (HAS_DISPLAY(dev_priv)) > + if (HAS_DISPLAY(dev_priv)) { > drm_kms_helper_poll_disable(dev); > + intel_display_driver_disable_user_access(dev_priv); > + } > > pci_save_state(pdev); > > @@ -1094,6 +1099,8 @@ static int i915_drm_suspend(struct drm_device > *dev) > intel_runtime_pm_disable_interrupts(dev_priv); > intel_hpd_cancel_work(dev_priv); > > + intel_display_driver_suspend_access(dev_priv); > + > intel_suspend_encoders(dev_priv); > > /* Must be called before GGTT is suspended. */ > @@ -1243,14 +1250,19 @@ static int i915_drm_resume(struct drm_device > *dev) > intel_display_driver_init_hw(dev_priv); > > intel_clock_gating_init(dev_priv); > + > + intel_display_driver_resume_access(dev_priv); > + > intel_hpd_init(dev_priv); > > /* MST sideband requires HPD interrupts enabled */ > intel_dp_mst_resume(dev_priv); > intel_display_driver_resume(dev_priv); > > - if (HAS_DISPLAY(dev_priv)) > + if (HAS_DISPLAY(dev_priv)) { > + intel_display_driver_enable_user_access(dev_priv); > drm_kms_helper_poll_enable(dev); > + } > intel_hpd_poll_disable(dev_priv); > > intel_opregion_resume(dev_priv);
On Mon, Jan 08, 2024 at 10:31:07AM +0200, Hogander, Jouni wrote: > On Thu, 2024-01-04 at 10:30 +0200, Imre Deak wrote: > > An unexpected modeset or connector detection by a user (user space > > or FB console) during the initialization/shutdown sequence is > > possible either via a hotplug IRQ handling work or via the connector > > sysfs (status/detect) interface. These modesets/detections should be > > prevented by disabling/flushing all related hotplug handling work > > and unregistering the interfaces that can start them at the > > beginning of the shutdown sequence. Some of this - disabling all > > related intel_hotplug work - will be done by the next patch, but > > others - for instance disabling the MST hotplug works - require a > > bigger rework. > > > > It makes sense - for diagnostic purpose, even with all the above > > work and interface disabled - to detect and reject any such user > > access. This patch does that for modeset accesses and a follow-up > > patch for connector detection. > > > > After the display is disabled during the shutdown sequence, no > > modeset should happen so it's disabled for both users and the > > shutdown thread. > > Can you please explain in commit message why > intel_display_driver_disable_user_access and > intel_display_driver_resume_access are allowing modeset for "current" > process? Is it ok if I add the following instead of the last paragraph?: """ During driver loading/unloading/system suspend/shutdown and during system resume after calling intel_display_driver_disable_user_access() or intel_display_driver_resume_access() correspondigly, the current thread is allowed to modeset (as this thread requires to do an initial/restoring modeset or a disabling modeset), other threads (the user threads) are not allowed to modeset. During driver loading/system resume after calling intel_display_driver_enable_user_access() all threads are allowed to modeset. During driver unloading/system suspend/shutdown after calling intel_display_driver_suspend_access() no threads are allowed to modeset (as the HW got disabled and should stay in this state). """ > > BR, > > Jouni Högander > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 3 + > > .../gpu/drm/i915/display/intel_display_core.h | 7 ++ > > .../drm/i915/display/intel_display_driver.c | 74 > > +++++++++++++++++++ > > .../drm/i915/display/intel_display_driver.h | 6 ++ > > drivers/gpu/drm/i915/i915_driver.c | 16 +++- > > 5 files changed, 104 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index 927d124457b61..31a6a82c12616 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -6310,6 +6310,9 @@ int intel_atomic_check(struct drm_device *dev, > > int ret, i; > > bool any_ms = false; > > > > + if (!intel_display_driver_check_access(dev_priv)) > > + return -ENODEV; > > + > > for_each_oldnew_intel_crtc_in_state(state, crtc, > > old_crtc_state, > > new_crtc_state, i) { > > /* > > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h > > b/drivers/gpu/drm/i915/display/intel_display_core.h > > index 47297ed858223..0b130ca9e6698 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > @@ -28,6 +28,8 @@ > > #include "intel_opregion.h" > > #include "intel_wm_types.h" > > > > +struct task_struct; > > + > > struct drm_i915_private; > > struct drm_property; > > struct drm_property_blob; > > @@ -298,6 +300,11 @@ struct intel_display { > > const struct intel_audio_funcs *audio; > > } funcs; > > > > + struct { > > + bool any_task_allowed; > > + struct task_struct *allowed_task; > > + } access; > > + > > struct { > > /* backlight registers and fields in struct > > intel_panel */ > > struct mutex lock; > > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c > > b/drivers/gpu/drm/i915/display/intel_display_driver.c > > index 1974f2394a518..b2441ab9822c2 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > > @@ -45,6 +45,7 @@ > > #include "intel_hdcp.h" > > #include "intel_hotplug.h" > > #include "intel_hti.h" > > +#include "intel_modeset_lock.h" > > #include "intel_modeset_setup.h" > > #include "intel_opregion.h" > > #include "intel_overlay.h" > > @@ -276,6 +277,71 @@ int intel_display_driver_probe_noirq(struct > > drm_i915_private *i915) > > return ret; > > } > > > > +static void set_display_access(struct drm_i915_private *i915, > > + bool any_task_allowed, > > + struct task_struct *allowed_task) > > +{ > > + struct drm_modeset_acquire_ctx ctx; > > + int err; > > + > > + intel_modeset_lock_ctx_retry(&ctx, NULL, 0, err) { > > + err = drm_modeset_lock_all_ctx(&i915->drm, &ctx); > > + if (err) > > + continue; > > + > > + i915->display.access.any_task_allowed = > > any_task_allowed; > > + i915->display.access.allowed_task = allowed_task; > > + } > > + > > + drm_WARN_ON(&i915->drm, err); > > +} > > + > > +void intel_display_driver_enable_user_access(struct drm_i915_private > > *i915) > > +{ > > + set_display_access(i915, true, NULL); > > +} > > + > > +void intel_display_driver_disable_user_access(struct > > drm_i915_private *i915) > > +{ > > + set_display_access(i915, false, current); > > +} > > + > > +void intel_display_driver_suspend_access(struct drm_i915_private > > *i915) > > +{ > > + set_display_access(i915, false, NULL); > > +} > > + > > +void intel_display_driver_resume_access(struct drm_i915_private > > *i915) > > +{ > > + set_display_access(i915, false, current); > > +} > > + > > +bool intel_display_driver_check_access(struct drm_i915_private > > *i915) > > +{ > > + char comm[TASK_COMM_LEN]; > > + char current_task[TASK_COMM_LEN + 16]; > > + char allowed_task[TASK_COMM_LEN + 16] = "none"; > > + > > + if (i915->display.access.any_task_allowed || > > + i915->display.access.allowed_task == current) > > + return true; > > + > > + snprintf(current_task, sizeof(current_task), "%s[%d]", > > + get_task_comm(comm, current), > > + task_pid_vnr(current)); > > + > > + if (i915->display.access.allowed_task) > > + snprintf(allowed_task, sizeof(allowed_task), > > "%s[%d]", > > + get_task_comm(comm, i915- > > >display.access.allowed_task), > > + task_pid_vnr(i915- > > >display.access.allowed_task)); > > + > > + drm_dbg_kms(&i915->drm, > > + "Reject display access from task %s (allowed to > > %s)\n", > > + current_task, allowed_task); > > + > > + return false; > > +} > > + > > /* part #2: call after irq install, but before gem init */ > > int intel_display_driver_probe_nogem(struct drm_i915_private *i915) > > { > > @@ -326,6 +392,8 @@ int intel_display_driver_probe_nogem(struct > > drm_i915_private *i915) > > intel_vga_disable(i915); > > intel_setup_outputs(i915); > > > > + intel_display_driver_disable_user_access(i915); > > + > > drm_modeset_lock_all(dev); > > intel_modeset_setup_hw_state(i915, dev- > > >mode_config.acquire_ctx); > > intel_acpi_assign_connector_fwnodes(i915); > > @@ -393,6 +461,8 @@ void intel_display_driver_register(struct > > drm_i915_private *i915) > > > > intel_audio_init(i915); > > > > + intel_display_driver_enable_user_access(i915); > > + > > intel_display_debugfs_register(i915); > > > > /* > > @@ -440,6 +510,8 @@ void intel_display_driver_remove_noirq(struct > > drm_i915_private *i915) > > if (!HAS_DISPLAY(i915)) > > return; > > > > + intel_display_driver_suspend_access(i915); > > + > > /* > > * Due to the hpd irq storm handling the hotplug work can re- > > arm the > > * poll handlers. Hence disable polling after hpd handling is > > shut down. > > @@ -493,6 +565,8 @@ void intel_display_driver_unregister(struct > > drm_i915_private *i915) > > */ > > drm_kms_helper_poll_fini(&i915->drm); > > > > + intel_display_driver_disable_user_access(i915); > > + > > intel_audio_deinit(i915); > > > > drm_atomic_helper_shutdown(&i915->drm); > > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.h > > b/drivers/gpu/drm/i915/display/intel_display_driver.h > > index c276a58ee3293..42cc4af6d3fd5 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_driver.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.h > > @@ -32,5 +32,11 @@ int __intel_display_driver_resume(struct > > drm_i915_private *i915, > > struct drm_atomic_state *state, > > struct drm_modeset_acquire_ctx > > *ctx); > > > > +void intel_display_driver_enable_user_access(struct drm_i915_private > > *i915); > > +void intel_display_driver_disable_user_access(struct > > drm_i915_private *i915); > > +void intel_display_driver_suspend_access(struct drm_i915_private > > *i915); > > +void intel_display_driver_resume_access(struct drm_i915_private > > *i915); > > +bool intel_display_driver_check_access(struct drm_i915_private > > *i915); > > + > > #endif /* __INTEL_DISPLAY_DRIVER_H__ */ > > > > diff --git a/drivers/gpu/drm/i915/i915_driver.c > > b/drivers/gpu/drm/i915/i915_driver.c > > index 166476948c896..068ca48e05323 100644 > > --- a/drivers/gpu/drm/i915/i915_driver.c > > +++ b/drivers/gpu/drm/i915/i915_driver.c > > @@ -1006,6 +1006,7 @@ void i915_driver_shutdown(struct > > drm_i915_private *i915) > > intel_fbdev_set_suspend(&i915->drm, FBINFO_STATE_SUSPENDED, > > true); > > if (HAS_DISPLAY(i915)) { > > drm_kms_helper_poll_disable(&i915->drm); > > + intel_display_driver_disable_user_access(i915); > > > > drm_atomic_helper_shutdown(&i915->drm); > > } > > @@ -1015,6 +1016,8 @@ void i915_driver_shutdown(struct > > drm_i915_private *i915) > > intel_runtime_pm_disable_interrupts(i915); > > intel_hpd_cancel_work(i915); > > > > + intel_display_driver_suspend_access(i915); > > + > > intel_suspend_encoders(i915); > > intel_shutdown_encoders(i915); > > > > @@ -1082,8 +1085,10 @@ static int i915_drm_suspend(struct drm_device > > *dev) > > * properly. */ > > intel_power_domains_disable(dev_priv); > > intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true); > > - if (HAS_DISPLAY(dev_priv)) > > + if (HAS_DISPLAY(dev_priv)) { > > drm_kms_helper_poll_disable(dev); > > + intel_display_driver_disable_user_access(dev_priv); > > + } > > > > pci_save_state(pdev); > > > > @@ -1094,6 +1099,8 @@ static int i915_drm_suspend(struct drm_device > > *dev) > > intel_runtime_pm_disable_interrupts(dev_priv); > > intel_hpd_cancel_work(dev_priv); > > > > + intel_display_driver_suspend_access(dev_priv); > > + > > intel_suspend_encoders(dev_priv); > > > > /* Must be called before GGTT is suspended. */ > > @@ -1243,14 +1250,19 @@ static int i915_drm_resume(struct drm_device > > *dev) > > intel_display_driver_init_hw(dev_priv); > > > > intel_clock_gating_init(dev_priv); > > + > > + intel_display_driver_resume_access(dev_priv); > > + > > intel_hpd_init(dev_priv); > > > > /* MST sideband requires HPD interrupts enabled */ > > intel_dp_mst_resume(dev_priv); > > intel_display_driver_resume(dev_priv); > > > > - if (HAS_DISPLAY(dev_priv)) > > + if (HAS_DISPLAY(dev_priv)) { > > + intel_display_driver_enable_user_access(dev_priv); > > drm_kms_helper_poll_enable(dev); > > + } > > intel_hpd_poll_disable(dev_priv); > > > > intel_opregion_resume(dev_priv); >
On Mon, 2024-01-08 at 11:20 +0200, Imre Deak wrote: > On Mon, Jan 08, 2024 at 10:31:07AM +0200, Hogander, Jouni wrote: > > On Thu, 2024-01-04 at 10:30 +0200, Imre Deak wrote: > > > > An unexpected modeset or connector detection by a user (user > > > space > > > or FB console) during the initialization/shutdown sequence is > > > possible either via a hotplug IRQ handling work or via the > > > connector > > > sysfs (status/detect) interface. These modesets/detections should > > > be > > > prevented by disabling/flushing all related hotplug handling work > > > and unregistering the interfaces that can start them at the > > > beginning of the shutdown sequence. Some of this - disabling all > > > related intel_hotplug work - will be done by the next patch, but > > > others - for instance disabling the MST hotplug works - require a > > > bigger rework. > > > > > > It makes sense - for diagnostic purpose, even with all the above > > > work and interface disabled - to detect and reject any such user > > > access. This patch does that for modeset accesses and a follow-up > > > patch for connector detection. > > > > > > After the display is disabled during the shutdown sequence, no > > > modeset should happen so it's disabled for both users and the > > > shutdown thread. > > > > Can you please explain in commit message why > > intel_display_driver_disable_user_access and > > intel_display_driver_resume_access are allowing modeset for > > "current" > > process? > > Is it ok if I add the following instead of the last paragraph?: Yes. That helps to understand the patch. You could consider to add some comments into code as well. For me the commit message change is enough. BR, Jouni Högander > > """ > During driver loading/unloading/system suspend/shutdown and during > system resume after calling > intel_display_driver_disable_user_access() > or intel_display_driver_resume_access() correspondigly, the current > thread is allowed to modeset (as this thread requires to do an > initial/restoring modeset or a disabling modeset), other threads (the > user threads) are not allowed to modeset. > > During driver loading/system resume after calling > intel_display_driver_enable_user_access() all threads are allowed to > modeset. > > During driver unloading/system suspend/shutdown after calling > intel_display_driver_suspend_access() no threads are allowed to > modeset > (as the HW got disabled and should stay in this state). > """ > > > > > BR, > > > > Jouni Högander > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 3 + > > > .../gpu/drm/i915/display/intel_display_core.h | 7 ++ > > > .../drm/i915/display/intel_display_driver.c | 74 > > > +++++++++++++++++++ > > > .../drm/i915/display/intel_display_driver.h | 6 ++ > > > drivers/gpu/drm/i915/i915_driver.c | 16 +++- > > > 5 files changed, 104 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index 927d124457b61..31a6a82c12616 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -6310,6 +6310,9 @@ int intel_atomic_check(struct drm_device > > > *dev, > > > int ret, i; > > > bool any_ms = false; > > > > > > + if (!intel_display_driver_check_access(dev_priv)) > > > + return -ENODEV; > > > + > > > for_each_oldnew_intel_crtc_in_state(state, crtc, > > > old_crtc_state, > > > new_crtc_state, i) { > > > /* > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h > > > b/drivers/gpu/drm/i915/display/intel_display_core.h > > > index 47297ed858223..0b130ca9e6698 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > > @@ -28,6 +28,8 @@ > > > #include "intel_opregion.h" > > > #include "intel_wm_types.h" > > > > > > +struct task_struct; > > > + > > > struct drm_i915_private; > > > struct drm_property; > > > struct drm_property_blob; > > > @@ -298,6 +300,11 @@ struct intel_display { > > > const struct intel_audio_funcs *audio; > > > } funcs; > > > > > > + struct { > > > + bool any_task_allowed; > > > + struct task_struct *allowed_task; > > > + } access; > > > + > > > struct { > > > /* backlight registers and fields in struct > > > intel_panel */ > > > struct mutex lock; > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c > > > b/drivers/gpu/drm/i915/display/intel_display_driver.c > > > index 1974f2394a518..b2441ab9822c2 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > > > @@ -45,6 +45,7 @@ > > > #include "intel_hdcp.h" > > > #include "intel_hotplug.h" > > > #include "intel_hti.h" > > > +#include "intel_modeset_lock.h" > > > #include "intel_modeset_setup.h" > > > #include "intel_opregion.h" > > > #include "intel_overlay.h" > > > @@ -276,6 +277,71 @@ int intel_display_driver_probe_noirq(struct > > > drm_i915_private *i915) > > > return ret; > > > } > > > > > > +static void set_display_access(struct drm_i915_private *i915, > > > + bool any_task_allowed, > > > + struct task_struct *allowed_task) > > > +{ > > > + struct drm_modeset_acquire_ctx ctx; > > > + int err; > > > + > > > + intel_modeset_lock_ctx_retry(&ctx, NULL, 0, err) { > > > + err = drm_modeset_lock_all_ctx(&i915->drm, &ctx); > > > + if (err) > > > + continue; > > > + > > > + i915->display.access.any_task_allowed = > > > any_task_allowed; > > > + i915->display.access.allowed_task = allowed_task; > > > + } > > > + > > > + drm_WARN_ON(&i915->drm, err); > > > +} > > > + > > > +void intel_display_driver_enable_user_access(struct > > > drm_i915_private > > > *i915) > > > +{ > > > + set_display_access(i915, true, NULL); > > > +} > > > + > > > +void intel_display_driver_disable_user_access(struct > > > drm_i915_private *i915) > > > +{ > > > + set_display_access(i915, false, current); > > > +} > > > + > > > +void intel_display_driver_suspend_access(struct drm_i915_private > > > *i915) > > > +{ > > > + set_display_access(i915, false, NULL); > > > +} > > > + > > > +void intel_display_driver_resume_access(struct drm_i915_private > > > *i915) > > > +{ > > > + set_display_access(i915, false, current); > > > +} > > > + > > > +bool intel_display_driver_check_access(struct drm_i915_private > > > *i915) > > > +{ > > > + char comm[TASK_COMM_LEN]; > > > + char current_task[TASK_COMM_LEN + 16]; > > > + char allowed_task[TASK_COMM_LEN + 16] = "none"; > > > + > > > + if (i915->display.access.any_task_allowed || > > > + i915->display.access.allowed_task == current) > > > + return true; > > > + > > > + snprintf(current_task, sizeof(current_task), "%s[%d]", > > > + get_task_comm(comm, current), > > > + task_pid_vnr(current)); > > > + > > > + if (i915->display.access.allowed_task) > > > + snprintf(allowed_task, sizeof(allowed_task), > > > "%s[%d]", > > > + get_task_comm(comm, i915- > > > > display.access.allowed_task), > > > + task_pid_vnr(i915- > > > > display.access.allowed_task)); > > > + > > > + drm_dbg_kms(&i915->drm, > > > + "Reject display access from task %s (allowed > > > to > > > %s)\n", > > > + current_task, allowed_task); > > > + > > > + return false; > > > +} > > > + > > > /* part #2: call after irq install, but before gem init */ > > > int intel_display_driver_probe_nogem(struct drm_i915_private > > > *i915) > > > { > > > @@ -326,6 +392,8 @@ int intel_display_driver_probe_nogem(struct > > > drm_i915_private *i915) > > > intel_vga_disable(i915); > > > intel_setup_outputs(i915); > > > > > > + intel_display_driver_disable_user_access(i915); > > > + > > > drm_modeset_lock_all(dev); > > > intel_modeset_setup_hw_state(i915, dev- > > > > mode_config.acquire_ctx); > > > intel_acpi_assign_connector_fwnodes(i915); > > > @@ -393,6 +461,8 @@ void intel_display_driver_register(struct > > > drm_i915_private *i915) > > > > > > intel_audio_init(i915); > > > > > > + intel_display_driver_enable_user_access(i915); > > > + > > > intel_display_debugfs_register(i915); > > > > > > /* > > > @@ -440,6 +510,8 @@ void intel_display_driver_remove_noirq(struct > > > drm_i915_private *i915) > > > if (!HAS_DISPLAY(i915)) > > > return; > > > > > > + intel_display_driver_suspend_access(i915); > > > + > > > /* > > > * Due to the hpd irq storm handling the hotplug work can > > > re- > > > arm the > > > * poll handlers. Hence disable polling after hpd > > > handling is > > > shut down. > > > @@ -493,6 +565,8 @@ void intel_display_driver_unregister(struct > > > drm_i915_private *i915) > > > */ > > > drm_kms_helper_poll_fini(&i915->drm); > > > > > > + intel_display_driver_disable_user_access(i915); > > > + > > > intel_audio_deinit(i915); > > > > > > drm_atomic_helper_shutdown(&i915->drm); > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.h > > > b/drivers/gpu/drm/i915/display/intel_display_driver.h > > > index c276a58ee3293..42cc4af6d3fd5 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_driver.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.h > > > @@ -32,5 +32,11 @@ int __intel_display_driver_resume(struct > > > drm_i915_private *i915, > > > struct drm_atomic_state *state, > > > struct drm_modeset_acquire_ctx > > > *ctx); > > > > > > +void intel_display_driver_enable_user_access(struct > > > drm_i915_private > > > *i915); > > > +void intel_display_driver_disable_user_access(struct > > > drm_i915_private *i915); > > > +void intel_display_driver_suspend_access(struct drm_i915_private > > > *i915); > > > +void intel_display_driver_resume_access(struct drm_i915_private > > > *i915); > > > +bool intel_display_driver_check_access(struct drm_i915_private > > > *i915); > > > + > > > #endif /* __INTEL_DISPLAY_DRIVER_H__ */ > > > > > > diff --git a/drivers/gpu/drm/i915/i915_driver.c > > > b/drivers/gpu/drm/i915/i915_driver.c > > > index 166476948c896..068ca48e05323 100644 > > > --- a/drivers/gpu/drm/i915/i915_driver.c > > > +++ b/drivers/gpu/drm/i915/i915_driver.c > > > @@ -1006,6 +1006,7 @@ void i915_driver_shutdown(struct > > > drm_i915_private *i915) > > > intel_fbdev_set_suspend(&i915->drm, > > > FBINFO_STATE_SUSPENDED, > > > true); > > > if (HAS_DISPLAY(i915)) { > > > drm_kms_helper_poll_disable(&i915->drm); > > > + intel_display_driver_disable_user_access(i915); > > > > > > drm_atomic_helper_shutdown(&i915->drm); > > > } > > > @@ -1015,6 +1016,8 @@ void i915_driver_shutdown(struct > > > drm_i915_private *i915) > > > intel_runtime_pm_disable_interrupts(i915); > > > intel_hpd_cancel_work(i915); > > > > > > + intel_display_driver_suspend_access(i915); > > > + > > > intel_suspend_encoders(i915); > > > intel_shutdown_encoders(i915); > > > > > > @@ -1082,8 +1085,10 @@ static int i915_drm_suspend(struct > > > drm_device > > > *dev) > > > * properly. */ > > > intel_power_domains_disable(dev_priv); > > > intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, > > > true); > > > - if (HAS_DISPLAY(dev_priv)) > > > + if (HAS_DISPLAY(dev_priv)) { > > > drm_kms_helper_poll_disable(dev); > > > + > > > intel_display_driver_disable_user_access(dev_priv); > > > + } > > > > > > pci_save_state(pdev); > > > > > > @@ -1094,6 +1099,8 @@ static int i915_drm_suspend(struct > > > drm_device > > > *dev) > > > intel_runtime_pm_disable_interrupts(dev_priv); > > > intel_hpd_cancel_work(dev_priv); > > > > > > + intel_display_driver_suspend_access(dev_priv); > > > + > > > intel_suspend_encoders(dev_priv); > > > > > > /* Must be called before GGTT is suspended. */ > > > @@ -1243,14 +1250,19 @@ static int i915_drm_resume(struct > > > drm_device > > > *dev) > > > intel_display_driver_init_hw(dev_priv); > > > > > > intel_clock_gating_init(dev_priv); > > > + > > > + intel_display_driver_resume_access(dev_priv); > > > + > > > intel_hpd_init(dev_priv); > > > > > > /* MST sideband requires HPD interrupts enabled */ > > > intel_dp_mst_resume(dev_priv); > > > intel_display_driver_resume(dev_priv); > > > > > > - if (HAS_DISPLAY(dev_priv)) > > > + if (HAS_DISPLAY(dev_priv)) { > > > + > > > intel_display_driver_enable_user_access(dev_priv); > > > drm_kms_helper_poll_enable(dev); > > > + } > > > intel_hpd_poll_disable(dev_priv); > > > > > > intel_opregion_resume(dev_priv); > >
On Mon, 2024-01-08 at 11:44 +0200, Hogander, Jouni wrote: > On Mon, 2024-01-08 at 11:20 +0200, Imre Deak wrote: > > On Mon, Jan 08, 2024 at 10:31:07AM +0200, Hogander, Jouni wrote: > > > On Thu, 2024-01-04 at 10:30 +0200, Imre Deak wrote: > > > > > > An unexpected modeset or connector detection by a user (user > > > > space > > > > or FB console) during the initialization/shutdown sequence is > > > > possible either via a hotplug IRQ handling work or via the > > > > connector > > > > sysfs (status/detect) interface. These modesets/detections > > > > should > > > > be > > > > prevented by disabling/flushing all related hotplug handling > > > > work > > > > and unregistering the interfaces that can start them at the > > > > beginning of the shutdown sequence. Some of this - disabling > > > > all > > > > related intel_hotplug work - will be done by the next patch, > > > > but > > > > others - for instance disabling the MST hotplug works - require > > > > a > > > > bigger rework. > > > > > > > > It makes sense - for diagnostic purpose, even with all the > > > > above > > > > work and interface disabled - to detect and reject any such > > > > user > > > > access. This patch does that for modeset accesses and a follow- > > > > up > > > > patch for connector detection. > > > > > > > > After the display is disabled during the shutdown sequence, no > > > > modeset should happen so it's disabled for both users and the > > > > shutdown thread. > > > > > > Can you please explain in commit message why > > > intel_display_driver_disable_user_access and > > > intel_display_driver_resume_access are allowing modeset for > > > "current" > > > process? > > > > Is it ok if I add the following instead of the last paragraph?: > > Yes. That helps to understand the patch. You could consider to add > some > comments into code as well. For me the commit message change is > enough. And with commit message addition it is: Reviewed-by: Jouni Högander <jouni.hogander@intel.com> > > BR, > > Jouni Högander > > > > > """ > > During driver loading/unloading/system suspend/shutdown and during > > system resume after calling > > intel_display_driver_disable_user_access() > > or intel_display_driver_resume_access() correspondigly, the current > > thread is allowed to modeset (as this thread requires to do an > > initial/restoring modeset or a disabling modeset), other threads > > (the > > user threads) are not allowed to modeset. > > > > During driver loading/system resume after calling > > intel_display_driver_enable_user_access() all threads are allowed > > to > > modeset. > > > > During driver unloading/system suspend/shutdown after calling > > intel_display_driver_suspend_access() no threads are allowed to > > modeset > > (as the HW got disabled and should stay in this state). > > """ > > > > > > > > BR, > > > > > > Jouni Högander > > > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display.c | 3 + > > > > .../gpu/drm/i915/display/intel_display_core.h | 7 ++ > > > > .../drm/i915/display/intel_display_driver.c | 74 > > > > +++++++++++++++++++ > > > > .../drm/i915/display/intel_display_driver.h | 6 ++ > > > > drivers/gpu/drm/i915/i915_driver.c | 16 +++- > > > > 5 files changed, 104 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > > index 927d124457b61..31a6a82c12616 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > @@ -6310,6 +6310,9 @@ int intel_atomic_check(struct drm_device > > > > *dev, > > > > int ret, i; > > > > bool any_ms = false; > > > > > > > > + if (!intel_display_driver_check_access(dev_priv)) > > > > + return -ENODEV; > > > > + > > > > for_each_oldnew_intel_crtc_in_state(state, crtc, > > > > old_crtc_state, > > > > new_crtc_state, i) > > > > { > > > > /* > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h > > > > b/drivers/gpu/drm/i915/display/intel_display_core.h > > > > index 47297ed858223..0b130ca9e6698 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > > > @@ -28,6 +28,8 @@ > > > > #include "intel_opregion.h" > > > > #include "intel_wm_types.h" > > > > > > > > +struct task_struct; > > > > + > > > > struct drm_i915_private; > > > > struct drm_property; > > > > struct drm_property_blob; > > > > @@ -298,6 +300,11 @@ struct intel_display { > > > > const struct intel_audio_funcs *audio; > > > > } funcs; > > > > > > > > + struct { > > > > + bool any_task_allowed; > > > > + struct task_struct *allowed_task; > > > > + } access; > > > > + > > > > struct { > > > > /* backlight registers and fields in struct > > > > intel_panel */ > > > > struct mutex lock; > > > > diff --git > > > > a/drivers/gpu/drm/i915/display/intel_display_driver.c > > > > b/drivers/gpu/drm/i915/display/intel_display_driver.c > > > > index 1974f2394a518..b2441ab9822c2 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > > > > @@ -45,6 +45,7 @@ > > > > #include "intel_hdcp.h" > > > > #include "intel_hotplug.h" > > > > #include "intel_hti.h" > > > > +#include "intel_modeset_lock.h" > > > > #include "intel_modeset_setup.h" > > > > #include "intel_opregion.h" > > > > #include "intel_overlay.h" > > > > @@ -276,6 +277,71 @@ int > > > > intel_display_driver_probe_noirq(struct > > > > drm_i915_private *i915) > > > > return ret; > > > > } > > > > > > > > +static void set_display_access(struct drm_i915_private *i915, > > > > + bool any_task_allowed, > > > > + struct task_struct > > > > *allowed_task) > > > > +{ > > > > + struct drm_modeset_acquire_ctx ctx; > > > > + int err; > > > > + > > > > + intel_modeset_lock_ctx_retry(&ctx, NULL, 0, err) { > > > > + err = drm_modeset_lock_all_ctx(&i915->drm, > > > > &ctx); > > > > + if (err) > > > > + continue; > > > > + > > > > + i915->display.access.any_task_allowed = > > > > any_task_allowed; > > > > + i915->display.access.allowed_task = > > > > allowed_task; > > > > + } > > > > + > > > > + drm_WARN_ON(&i915->drm, err); > > > > +} > > > > + > > > > +void intel_display_driver_enable_user_access(struct > > > > drm_i915_private > > > > *i915) > > > > +{ > > > > + set_display_access(i915, true, NULL); > > > > +} > > > > + > > > > +void intel_display_driver_disable_user_access(struct > > > > drm_i915_private *i915) > > > > +{ > > > > + set_display_access(i915, false, current); > > > > +} > > > > + > > > > +void intel_display_driver_suspend_access(struct > > > > drm_i915_private > > > > *i915) > > > > +{ > > > > + set_display_access(i915, false, NULL); > > > > +} > > > > + > > > > +void intel_display_driver_resume_access(struct > > > > drm_i915_private > > > > *i915) > > > > +{ > > > > + set_display_access(i915, false, current); > > > > +} > > > > + > > > > +bool intel_display_driver_check_access(struct drm_i915_private > > > > *i915) > > > > +{ > > > > + char comm[TASK_COMM_LEN]; > > > > + char current_task[TASK_COMM_LEN + 16]; > > > > + char allowed_task[TASK_COMM_LEN + 16] = "none"; > > > > + > > > > + if (i915->display.access.any_task_allowed || > > > > + i915->display.access.allowed_task == current) > > > > + return true; > > > > + > > > > + snprintf(current_task, sizeof(current_task), "%s[%d]", > > > > + get_task_comm(comm, current), > > > > + task_pid_vnr(current)); > > > > + > > > > + if (i915->display.access.allowed_task) > > > > + snprintf(allowed_task, sizeof(allowed_task), > > > > "%s[%d]", > > > > + get_task_comm(comm, i915- > > > > > display.access.allowed_task), > > > > + task_pid_vnr(i915- > > > > > display.access.allowed_task)); > > > > + > > > > + drm_dbg_kms(&i915->drm, > > > > + "Reject display access from task %s > > > > (allowed > > > > to > > > > %s)\n", > > > > + current_task, allowed_task); > > > > + > > > > + return false; > > > > +} > > > > + > > > > /* part #2: call after irq install, but before gem init */ > > > > int intel_display_driver_probe_nogem(struct drm_i915_private > > > > *i915) > > > > { > > > > @@ -326,6 +392,8 @@ int intel_display_driver_probe_nogem(struct > > > > drm_i915_private *i915) > > > > intel_vga_disable(i915); > > > > intel_setup_outputs(i915); > > > > > > > > + intel_display_driver_disable_user_access(i915); > > > > + > > > > drm_modeset_lock_all(dev); > > > > intel_modeset_setup_hw_state(i915, dev- > > > > > mode_config.acquire_ctx); > > > > intel_acpi_assign_connector_fwnodes(i915); > > > > @@ -393,6 +461,8 @@ void intel_display_driver_register(struct > > > > drm_i915_private *i915) > > > > > > > > intel_audio_init(i915); > > > > > > > > + intel_display_driver_enable_user_access(i915); > > > > + > > > > intel_display_debugfs_register(i915); > > > > > > > > /* > > > > @@ -440,6 +510,8 @@ void > > > > intel_display_driver_remove_noirq(struct > > > > drm_i915_private *i915) > > > > if (!HAS_DISPLAY(i915)) > > > > return; > > > > > > > > + intel_display_driver_suspend_access(i915); > > > > + > > > > /* > > > > * Due to the hpd irq storm handling the hotplug work > > > > can > > > > re- > > > > arm the > > > > * poll handlers. Hence disable polling after hpd > > > > handling is > > > > shut down. > > > > @@ -493,6 +565,8 @@ void intel_display_driver_unregister(struct > > > > drm_i915_private *i915) > > > > */ > > > > drm_kms_helper_poll_fini(&i915->drm); > > > > > > > > + intel_display_driver_disable_user_access(i915); > > > > + > > > > intel_audio_deinit(i915); > > > > > > > > drm_atomic_helper_shutdown(&i915->drm); > > > > diff --git > > > > a/drivers/gpu/drm/i915/display/intel_display_driver.h > > > > b/drivers/gpu/drm/i915/display/intel_display_driver.h > > > > index c276a58ee3293..42cc4af6d3fd5 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display_driver.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.h > > > > @@ -32,5 +32,11 @@ int __intel_display_driver_resume(struct > > > > drm_i915_private *i915, > > > > struct drm_atomic_state > > > > *state, > > > > struct > > > > drm_modeset_acquire_ctx > > > > *ctx); > > > > > > > > +void intel_display_driver_enable_user_access(struct > > > > drm_i915_private > > > > *i915); > > > > +void intel_display_driver_disable_user_access(struct > > > > drm_i915_private *i915); > > > > +void intel_display_driver_suspend_access(struct > > > > drm_i915_private > > > > *i915); > > > > +void intel_display_driver_resume_access(struct > > > > drm_i915_private > > > > *i915); > > > > +bool intel_display_driver_check_access(struct drm_i915_private > > > > *i915); > > > > + > > > > #endif /* __INTEL_DISPLAY_DRIVER_H__ */ > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_driver.c > > > > b/drivers/gpu/drm/i915/i915_driver.c > > > > index 166476948c896..068ca48e05323 100644 > > > > --- a/drivers/gpu/drm/i915/i915_driver.c > > > > +++ b/drivers/gpu/drm/i915/i915_driver.c > > > > @@ -1006,6 +1006,7 @@ void i915_driver_shutdown(struct > > > > drm_i915_private *i915) > > > > intel_fbdev_set_suspend(&i915->drm, > > > > FBINFO_STATE_SUSPENDED, > > > > true); > > > > if (HAS_DISPLAY(i915)) { > > > > drm_kms_helper_poll_disable(&i915->drm); > > > > + intel_display_driver_disable_user_access(i915); > > > > > > > > drm_atomic_helper_shutdown(&i915->drm); > > > > } > > > > @@ -1015,6 +1016,8 @@ void i915_driver_shutdown(struct > > > > drm_i915_private *i915) > > > > intel_runtime_pm_disable_interrupts(i915); > > > > intel_hpd_cancel_work(i915); > > > > > > > > + intel_display_driver_suspend_access(i915); > > > > + > > > > intel_suspend_encoders(i915); > > > > intel_shutdown_encoders(i915); > > > > > > > > @@ -1082,8 +1085,10 @@ static int i915_drm_suspend(struct > > > > drm_device > > > > *dev) > > > > * properly. */ > > > > intel_power_domains_disable(dev_priv); > > > > intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, > > > > true); > > > > - if (HAS_DISPLAY(dev_priv)) > > > > + if (HAS_DISPLAY(dev_priv)) { > > > > drm_kms_helper_poll_disable(dev); > > > > + > > > > intel_display_driver_disable_user_access(dev_priv); > > > > + } > > > > > > > > pci_save_state(pdev); > > > > > > > > @@ -1094,6 +1099,8 @@ static int i915_drm_suspend(struct > > > > drm_device > > > > *dev) > > > > intel_runtime_pm_disable_interrupts(dev_priv); > > > > intel_hpd_cancel_work(dev_priv); > > > > > > > > + intel_display_driver_suspend_access(dev_priv); > > > > + > > > > intel_suspend_encoders(dev_priv); > > > > > > > > /* Must be called before GGTT is suspended. */ > > > > @@ -1243,14 +1250,19 @@ static int i915_drm_resume(struct > > > > drm_device > > > > *dev) > > > > intel_display_driver_init_hw(dev_priv); > > > > > > > > intel_clock_gating_init(dev_priv); > > > > + > > > > + intel_display_driver_resume_access(dev_priv); > > > > + > > > > intel_hpd_init(dev_priv); > > > > > > > > /* MST sideband requires HPD interrupts enabled */ > > > > intel_dp_mst_resume(dev_priv); > > > > intel_display_driver_resume(dev_priv); > > > > > > > > - if (HAS_DISPLAY(dev_priv)) > > > > + if (HAS_DISPLAY(dev_priv)) { > > > > + > > > > intel_display_driver_enable_user_access(dev_priv); > > > > drm_kms_helper_poll_enable(dev); > > > > + } > > > > intel_hpd_poll_disable(dev_priv); > > > > > > > > intel_opregion_resume(dev_priv); > > > >
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 927d124457b61..31a6a82c12616 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -6310,6 +6310,9 @@ int intel_atomic_check(struct drm_device *dev, int ret, i; bool any_ms = false; + if (!intel_display_driver_check_access(dev_priv)) + return -ENODEV; + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { /* diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h index 47297ed858223..0b130ca9e6698 100644 --- a/drivers/gpu/drm/i915/display/intel_display_core.h +++ b/drivers/gpu/drm/i915/display/intel_display_core.h @@ -28,6 +28,8 @@ #include "intel_opregion.h" #include "intel_wm_types.h" +struct task_struct; + struct drm_i915_private; struct drm_property; struct drm_property_blob; @@ -298,6 +300,11 @@ struct intel_display { const struct intel_audio_funcs *audio; } funcs; + struct { + bool any_task_allowed; + struct task_struct *allowed_task; + } access; + struct { /* backlight registers and fields in struct intel_panel */ struct mutex lock; diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c index 1974f2394a518..b2441ab9822c2 100644 --- a/drivers/gpu/drm/i915/display/intel_display_driver.c +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c @@ -45,6 +45,7 @@ #include "intel_hdcp.h" #include "intel_hotplug.h" #include "intel_hti.h" +#include "intel_modeset_lock.h" #include "intel_modeset_setup.h" #include "intel_opregion.h" #include "intel_overlay.h" @@ -276,6 +277,71 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915) return ret; } +static void set_display_access(struct drm_i915_private *i915, + bool any_task_allowed, + struct task_struct *allowed_task) +{ + struct drm_modeset_acquire_ctx ctx; + int err; + + intel_modeset_lock_ctx_retry(&ctx, NULL, 0, err) { + err = drm_modeset_lock_all_ctx(&i915->drm, &ctx); + if (err) + continue; + + i915->display.access.any_task_allowed = any_task_allowed; + i915->display.access.allowed_task = allowed_task; + } + + drm_WARN_ON(&i915->drm, err); +} + +void intel_display_driver_enable_user_access(struct drm_i915_private *i915) +{ + set_display_access(i915, true, NULL); +} + +void intel_display_driver_disable_user_access(struct drm_i915_private *i915) +{ + set_display_access(i915, false, current); +} + +void intel_display_driver_suspend_access(struct drm_i915_private *i915) +{ + set_display_access(i915, false, NULL); +} + +void intel_display_driver_resume_access(struct drm_i915_private *i915) +{ + set_display_access(i915, false, current); +} + +bool intel_display_driver_check_access(struct drm_i915_private *i915) +{ + char comm[TASK_COMM_LEN]; + char current_task[TASK_COMM_LEN + 16]; + char allowed_task[TASK_COMM_LEN + 16] = "none"; + + if (i915->display.access.any_task_allowed || + i915->display.access.allowed_task == current) + return true; + + snprintf(current_task, sizeof(current_task), "%s[%d]", + get_task_comm(comm, current), + task_pid_vnr(current)); + + if (i915->display.access.allowed_task) + snprintf(allowed_task, sizeof(allowed_task), "%s[%d]", + get_task_comm(comm, i915->display.access.allowed_task), + task_pid_vnr(i915->display.access.allowed_task)); + + drm_dbg_kms(&i915->drm, + "Reject display access from task %s (allowed to %s)\n", + current_task, allowed_task); + + return false; +} + /* part #2: call after irq install, but before gem init */ int intel_display_driver_probe_nogem(struct drm_i915_private *i915) { @@ -326,6 +392,8 @@ int intel_display_driver_probe_nogem(struct drm_i915_private *i915) intel_vga_disable(i915); intel_setup_outputs(i915); + intel_display_driver_disable_user_access(i915); + drm_modeset_lock_all(dev); intel_modeset_setup_hw_state(i915, dev->mode_config.acquire_ctx); intel_acpi_assign_connector_fwnodes(i915); @@ -393,6 +461,8 @@ void intel_display_driver_register(struct drm_i915_private *i915) intel_audio_init(i915); + intel_display_driver_enable_user_access(i915); + intel_display_debugfs_register(i915); /* @@ -440,6 +510,8 @@ void intel_display_driver_remove_noirq(struct drm_i915_private *i915) if (!HAS_DISPLAY(i915)) return; + intel_display_driver_suspend_access(i915); + /* * Due to the hpd irq storm handling the hotplug work can re-arm the * poll handlers. Hence disable polling after hpd handling is shut down. @@ -493,6 +565,8 @@ void intel_display_driver_unregister(struct drm_i915_private *i915) */ drm_kms_helper_poll_fini(&i915->drm); + intel_display_driver_disable_user_access(i915); + intel_audio_deinit(i915); drm_atomic_helper_shutdown(&i915->drm); diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.h b/drivers/gpu/drm/i915/display/intel_display_driver.h index c276a58ee3293..42cc4af6d3fd5 100644 --- a/drivers/gpu/drm/i915/display/intel_display_driver.h +++ b/drivers/gpu/drm/i915/display/intel_display_driver.h @@ -32,5 +32,11 @@ int __intel_display_driver_resume(struct drm_i915_private *i915, struct drm_atomic_state *state, struct drm_modeset_acquire_ctx *ctx); +void intel_display_driver_enable_user_access(struct drm_i915_private *i915); +void intel_display_driver_disable_user_access(struct drm_i915_private *i915); +void intel_display_driver_suspend_access(struct drm_i915_private *i915); +void intel_display_driver_resume_access(struct drm_i915_private *i915); +bool intel_display_driver_check_access(struct drm_i915_private *i915); + #endif /* __INTEL_DISPLAY_DRIVER_H__ */ diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c index 166476948c896..068ca48e05323 100644 --- a/drivers/gpu/drm/i915/i915_driver.c +++ b/drivers/gpu/drm/i915/i915_driver.c @@ -1006,6 +1006,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915) intel_fbdev_set_suspend(&i915->drm, FBINFO_STATE_SUSPENDED, true); if (HAS_DISPLAY(i915)) { drm_kms_helper_poll_disable(&i915->drm); + intel_display_driver_disable_user_access(i915); drm_atomic_helper_shutdown(&i915->drm); } @@ -1015,6 +1016,8 @@ void i915_driver_shutdown(struct drm_i915_private *i915) intel_runtime_pm_disable_interrupts(i915); intel_hpd_cancel_work(i915); + intel_display_driver_suspend_access(i915); + intel_suspend_encoders(i915); intel_shutdown_encoders(i915); @@ -1082,8 +1085,10 @@ static int i915_drm_suspend(struct drm_device *dev) * properly. */ intel_power_domains_disable(dev_priv); intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true); - if (HAS_DISPLAY(dev_priv)) + if (HAS_DISPLAY(dev_priv)) { drm_kms_helper_poll_disable(dev); + intel_display_driver_disable_user_access(dev_priv); + } pci_save_state(pdev); @@ -1094,6 +1099,8 @@ static int i915_drm_suspend(struct drm_device *dev) intel_runtime_pm_disable_interrupts(dev_priv); intel_hpd_cancel_work(dev_priv); + intel_display_driver_suspend_access(dev_priv); + intel_suspend_encoders(dev_priv); /* Must be called before GGTT is suspended. */ @@ -1243,14 +1250,19 @@ static int i915_drm_resume(struct drm_device *dev) intel_display_driver_init_hw(dev_priv); intel_clock_gating_init(dev_priv); + + intel_display_driver_resume_access(dev_priv); + intel_hpd_init(dev_priv); /* MST sideband requires HPD interrupts enabled */ intel_dp_mst_resume(dev_priv); intel_display_driver_resume(dev_priv); - if (HAS_DISPLAY(dev_priv)) + if (HAS_DISPLAY(dev_priv)) { + intel_display_driver_enable_user_access(dev_priv); drm_kms_helper_poll_enable(dev); + } intel_hpd_poll_disable(dev_priv); intel_opregion_resume(dev_priv);
An unexpected modeset or connector detection by a user (user space or FB console) during the initialization/shutdown sequence is possible either via a hotplug IRQ handling work or via the connector sysfs (status/detect) interface. These modesets/detections should be prevented by disabling/flushing all related hotplug handling work and unregistering the interfaces that can start them at the beginning of the shutdown sequence. Some of this - disabling all related intel_hotplug work - will be done by the next patch, but others - for instance disabling the MST hotplug works - require a bigger rework. It makes sense - for diagnostic purpose, even with all the above work and interface disabled - to detect and reject any such user access. This patch does that for modeset accesses and a follow-up patch for connector detection. After the display is disabled during the shutdown sequence, no modeset should happen so it's disabled for both users and the shutdown thread. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 3 + .../gpu/drm/i915/display/intel_display_core.h | 7 ++ .../drm/i915/display/intel_display_driver.c | 74 +++++++++++++++++++ .../drm/i915/display/intel_display_driver.h | 6 ++ drivers/gpu/drm/i915/i915_driver.c | 16 +++- 5 files changed, 104 insertions(+), 2 deletions(-)