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?
