Re: [FOR CI 8/8] drm/i915/display: Use intel_de_write_fw in intel_pipe_fastset

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 31 Oct 2025, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Thu, Oct 30, 2025 at 05:25:36PM -0300, Gustavo Sousa wrote:
>> Quoting Maarten Lankhorst (2025-10-30 17:00:29-03:00)
>> >Hey,
>> >
>> >Den 2025-10-30 kl. 19:54, skrev Gustavo Sousa:
>> >> Quoting Maarten Lankhorst (2025-10-30 15:47:10-03:00)
>> >>> intel_set_pipe_src_size(), hsw_set_linetime_wm(),
>> >>> intel_cpu_transcoder_set_m1_n1() and intel_set_transcoder_timings_lrr()
>> >>> are called from an atomic context on PREEMPT_RT, and should be using the
>> >>> _fw functions.
>> >>>
>> >>> Again noticed when trying to disable preemption in vblank evasion:
>> >>> <3> BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
>> >>> <3> in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1505, name: kms_cursor_lega
>> >>> <3> preempt_count: 1, expected: 0
>> >>> <3> RCU nest depth: 0, expected: 0
>> >>> <4> 4 locks held by kms_cursor_lega/1505:
>> >>> <4>  #0: ffffc90003c6f988 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_mode_atomic_ioctl+0x13b/0xe90
>> >>> <4>  #1: ffffc90003c6f9b0 (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_mode_atomic_ioctl+0x13b/0xe90
>> >>> <4>  #2: ffff888135b838b8 (&intel_dp->psr.lock){+.+.}-{3:3}, at: intel_psr_lock+0xc5/0xf0 [xe]
>> >>> <4>  #3: ffff88812607bbc0 (&wl->lock){+.+.}-{2:2}, at: intel_dmc_wl_get+0x3c/0x140 [xe]
>> >>> <4> CPU: 6 UID: 0 PID: 1505 Comm: kms_cursor_lega Tainted: G     U              6.18.0-rc3-lgci-xe-xe-pw-156729v1+ #1 PREEMPT_{RT,(lazy)}
>> >>> <4> Tainted: [U]=USER
>> >>> <4> Hardware name: Intel Corporation Panther Lake Client Platform/PTL-UH LP5 T3 RVP1, BIOS PTLPFWI1.R00.3383.D02.2509240621 09/24/2025
>> >>> <4> Call Trace:
>> >>> <4>  <TASK>
>> >>> <4>  dump_stack_lvl+0xc1/0xf0
>> >>> <4>  dump_stack+0x10/0x20
>> >>> <4>  __might_resched+0x174/0x260
>> >>> <4>  rt_spin_lock+0x63/0x200
>> >>> <4>  ? intel_dmc_wl_get+0x3c/0x140 [xe]
>> >>> <4>  intel_dmc_wl_get+0x3c/0x140 [xe]
>> >> 
>> >> Isn't the real problem reported here that we are using a regular
>> >> spinlock for DMC wakelock instead of a raw spinlock?
>> >
>> >Regardless whether it is, we should be using the _fw variant here.
>
> Right. I do believe the _fw variant is the best choice here. We just need to
> review the registers accessed to ensure that they are not in any range of the fw
> for the none of the platforms, including the old ones.
>
> But I believe the patch deserves a different commit message.
>
>> >The idea of the pipe_start/end() sections are that all relevant locks are taken,
>> >and then complete as deterministically as possible. That's a lot easier when all
>> >locks are taken in advance. If the wakelock was needed, it needed to be taken
>> >before entering the critical section between pipe_start/pipe_end
>> >
>> >You're pointing out the related problem that the DMC wakelock implementation is
>> >incorrect right now.
>> >
>> >I believe that too, but we should aim for a better solution. The DMC wakelock
>> >implementation should not hide the fact it exists, instead it should be
>> >acquired explicitly like the xe_force_wake_get/put() implementation.
>> >
>> >This may be a bit of work, but it will be more deterministic than the implicit
>> >api used in i915.
>> >
>> >For correctness we could validate and print a debug error if the DMC wakelock
>> >was not taken.
>> >
>> >intel_de_read() can still optionally validate that the DMC wakelock was taken for
>> >ranges that need it if debuggig is enabled, but we should aim to remove the
>> >current spinlock/refcount implementation.
>> 
>> That's an interesting view.
>> 
>> Adding display maintainers to the discussion here. Jani, Rodrigo, how to
>> you see DMC wakelock being changed in the way proposed by Maarten?
>
> My opinion might be a bit biased here because I prefer the explicit FW
> as handled by the Xe rather than hidden in the mmio call like i915.
>
> So, perhaps the explicit WL is also a good path. But I haven't looked the
> code deep enough to get to a final conclusion.
>
> But the quickest fix for the message in the commit message seems to be the
> raw spinlock. Perhaps we do this and we run a calm analysis on the explicit
> WL or not on a follow up?

The patch says "FOR CI". I'm reading that as "just sending for CI, not
for review", since there are no Reviewed-bys here. That's also what the
cover letter says.

So I've just mostly been ignoring the series.


BR,
Jani.


>
>> 
>> --
>> Gustavo Sousa
>> 
>> >
>> >Kind regards,
>> >~Maarten Lankhorst

-- 
Jani Nikula, Intel