On Thu, 31 Jul 2025, Imre Deak <imre.d...@intel.com> wrote:
> Hi Rodrigo,
>
> On Tue, Jul 29, 2025 at 07:36:04PM +0300, Jani Nikula wrote:
>> On Tue, 29 Jul 2025, Imre Deak <imre.d...@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.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
>>
>> 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?
Ack.
>
> --Imre
>
>> BR,
>> Jani.
>>
>> >
>> >> >
>> >> > BR,
>> >> > Jani.
>> >> >
>> >> >
>> >> > --
>> >> > Jani Nikula, Intel
>>
>> --
>> Jani Nikula, Intel
--
Jani Nikula, Intel
