On Wed, Jul 30, 2025 at 09:21:08AM +0200, Maarten Lankhorst wrote:
> Hey,
>
> Den 2025-07-29 kl. 17:03, skrev Imre Deak:
> > 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
> >>> <maarten.lankho...@linux.intel.com> 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 <rodrigo.v...@intel.com>
> >>>>>
> >>>>> from either side...
> >>>>>
> >>>>>>
> >>>>>> Reviewed-by: Imre Deak <imre.d...@intel.com>
> >>>> 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 <maarten.lankho...@linux.intel.com>
> >>>>
> >>>> 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
>
> Since you put the interrupt disabling before dmc suspend, perhaps you need
> a variation of
>
> https://patchwork.freedesktop.org/series/151728/ too?
Yes, I think it is also need, before intel_opregion_suspend(), due to
asle_work scheduled by a display IRQ.
That step could be also shared later, after making i915
enabling/disabling the display IRQs separately as well (vs. its current
intel_irq_resume()/intel_irq_suspend() enabling/disabling all IRQs).
