On Thu, Jul 31, 2025 at 03:32:51PM +0300, Jani Nikula wrote:
> 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.
Agreed as well
>
> >
> > --Imre
> >
> >> BR,
> >> Jani.
> >>
> >> >
> >> >> >
> >> >> > BR,
> >> >> > Jani.
> >> >> >
> >> >> >
> >> >> > --
> >> >> > Jani Nikula, Intel
> >>
> >> --
> >> Jani Nikula, Intel
>
> --
> Jani Nikula, Intel
