On Tue, Jul 29, 2025 at 10:35:48AM -0400, Rodrigo Vivi wrote: > On Tue, Jul 29, 2025 at 12:44:47PM +0300, Jani Nikula wrote: > > On Thu, 24 Jul 2025, Maarten Lankhorst <[email protected]> > > wrote: > > > Hey, > > > [...] > > >>>> diff --git a/drivers/gpu/drm/xe/display/xe_display.c > > >>>> b/drivers/gpu/drm/xe/display/xe_display.c > > >>>> index e2e0771cf274..9e984a045059 100644 > > >>>> --- a/drivers/gpu/drm/xe/display/xe_display.c > > >>>> +++ b/drivers/gpu/drm/xe/display/xe_display.c > > >>>> @@ -96,6 +96,7 @@ static void xe_display_fini_early(void *arg) > > >>>> if (!xe->info.probe_display) > > >>>> return; > > >>>> > > >>>> + intel_hpd_cancel_work(display); > > >>>> intel_display_driver_remove_nogem(display); > > >>>> intel_display_driver_remove_noirq(display); > > >>>> intel_opregion_cleanup(display); > > >>>> @@ -340,6 +341,8 @@ void xe_display_pm_suspend(struct xe_device *xe) > > >>>> > > >>>> xe_display_flush_cleanup_work(xe); > > >>>> > > >>>> + intel_encoder_block_all_hpds(display); > > >>>> + > > >>>> intel_hpd_cancel_work(display); > > >>>> > > >>>> if (has_display(xe)) { > > >>>> @@ -369,6 +372,7 @@ void xe_display_pm_shutdown(struct xe_device *xe) > > >>>> } > > >>>> > > >>>> xe_display_flush_cleanup_work(xe); > > >>>> + intel_encoder_block_all_hpds(display); > > >>> > > >>> MST still needs HPD IRQs for side-band messaging, so the HPD IRQs must > > >>> be blocked only after intel_dp_mst_suspend(). > > >>> > > >>> Otherwise the patch looks ok to me, so with the above fixed and provided > > >>> that Maarten is ok to disable all display IRQs only later: > > >> > > >> Also probably good to identify the patch as both xe and i915 in the > > >> subject > > >> drm/{i915,xe}/display: > > >> > > >> and Maarten or Imre, any preference on which branch to go? any chance of > > >> conflicting with any of work you might be doing in any side? > > >> > > >> From my side I believe that any conflict might be easy to handle, so > > >> > > >> Acked-by: Rodrigo Vivi <[email protected]> > > >> > > >> from either side... > > >> > > >>> > > >>> Reviewed-by: Imre Deak <[email protected]> > > > We had a discussion on this approach, and it seems that completely > > > disabling interrupts here like in i915 would fail too. > > > > > > Reviewed-by: Maarten Lankhorst <[email protected]> > > > > > > I don't mind either branch. As long as it applies. :-) > > > > Please do not merge through *any* tree. > > > > How come you all think it's okay to add this xe specific thing, and make > > unification harder? > > I lost any moral or rights to complain here since I couldn't move with my > tasks of unification of the pm flow :( > > double sorry! > > > > > intel_encoder_block_all_hpds() is *way* too specific for a high level > > function. Neither xe nor i915 should never call something like that > > directly. > > that's a valid point indeed. But I cannot see another way to fix the > current issue right now without trying to move with the full unification > faster. Do you?
Imo, this should be fixed first in xe without affecting i915. Then a related fix would be needed in i915, which disables all display IRQs too early now, as in: https://github.com/ideak/linux/commit/0fbe02b20e062 After that the xe and i915 system suspend/resume and shutdown sequences could be unified mostly. Fwiw I put together that now on top of Dibin's patch: https://github.com/ideak/linux/commits/suspend-shutdown-refactor > > > > BR, > > Jani. > > > > > > -- > > Jani Nikula, Intel
