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

[Jani Nikula]

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.

BR,
Jani.



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

-- 
Jani Nikula, Intel