[PATCH v2] drm/xe/display: Block hpd during suspend

[Imre Deak]

Hi Rodrigo,

On Tue, Jul 29, 2025 at 07:36:04PM +0300, Jani Nikula wrote:
> On Tue, 29 Jul 2025, Imre Deak <imre.deak at intel.com> wrote:
> > 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.lankhorst at 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.vivi at intel.com>
> >> > >> 
> >> > >> from either side...
> >> > >> 
> >> > >>>
> >> > >>> Reviewed-by: Imre Deak <imre.deak at 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.lankhorst at 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
> 
> If that work is actually in progress and happening, then fine, let's go
> with this.

If the above is acceptable, then this change would be also needed for
i915. If the patch is merged to xe trees, then not sure if/when it would
be merged back to i915. So maybe it would make more sense to merge it to
i915 trees instead, considering it has more display changes already.
Would you be ok with that?

--Imre

> BR,
> Jani.
> 
> >
> >> > 
> >> > BR,
> >> > Jani.
> >> > 
> >> > 
> >> > -- 
> >> > Jani Nikula, Intel
> 
> -- 
> Jani Nikula, Intel