Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Missing Wakelock / DC State Hardware Hang Pre-existing issues: - [High] Sleeping in Atomic Context (Incomplete Fix) -- commit b928342b55781fe54de15efd0d013221c43c3091 Author: Maarten Lankhorst <[email protected]> drm/i915/display: Use intel_de_write_fw in intel_pipe_fastset This commit updates multiple display configuration functions to use the _fw register write variants to bypass DMC wakelock acquisition. This is intended to fix a sleep-in-atomic-context deadlock on PREEMPT_RT when modifying display registers during vblank evasion. > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 5bc8e6ea10a50..f5dac380aa7ff 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c [ ... ] > @@ -2607,14 +2607,14 @@ void intel_set_m_n(struct intel_display *display, > intel_reg_t data_m_reg, intel_reg_t data_n_reg, > intel_reg_t link_m_reg, intel_reg_t link_n_reg) > { > - intel_de_write(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m); > - intel_de_write(display, data_n_reg, m_n->data_n); > - intel_de_write(display, link_m_reg, m_n->link_m); > + intel_de_write_fw(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m); > + intel_de_write_fw(display, data_n_reg, m_n->data_n); > + intel_de_write_fw(display, link_m_reg, m_n->link_m); > /* > * On BDW+ writing LINK_N arms the double buffered update > * of all the M/N registers, so it must be written last. > */ > - intel_de_write(display, link_n_reg, m_n->link_n); > + intel_de_write_fw(display, link_n_reg, m_n->link_n); [Severity: High] Does using intel_de_write_fw() here risk writing to hardware while it is in a deep sleep DC state? When intel_set_m_n() is called asynchronously by intel_drrs_downclock_work() through intel_drrs_set_refresh_rate_m_n(): drivers/gpu/drm/i915/display/intel_drrs.c:intel_drrs_set_refresh_rate_m_n() { ... intel_cpu_transcoder_set_m1_n1(crtc, crtc->drrs.cpu_transcoder, refresh_rate == DRRS_REFRESH_RATE_LOW ? &crtc->drrs.m2_n2 : &crtc->drrs.m_n); ... } This async work runs when the display is idle and holds no outer DMC wakelocks. Bypassing the implicit wakelock acquisition could lead to the transcoder M/N registers being written without waking up the DMC, potentially causing dropped writes or hardware hangs. > } > > bool intel_cpu_transcoder_has_m2_n2(struct intel_display *display, [ ... ] > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c > b/drivers/gpu/drm/i915/display/intel_vrr.c > index bffbdee76ee1c..5b09a17185b9c 100644 > --- a/drivers/gpu/drm/i915/display/intel_vrr.c > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c [ ... ] > @@ -923,8 +923,8 @@ static void intel_vrr_tg_enable(const struct > intel_crtc_state *crtc_state, > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > u32 vrr_ctl; > > - intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), > - trans_vrr_push(crtc_state, false)); > + intel_de_write_fw(display, TRANS_PUSH(display, cpu_transcoder), > + trans_vrr_push(crtc_state, false)); > > vrr_ctl = VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state); > [Severity: High] This is a pre-existing issue, but while you are converting these functions to avoid sleeping in atomic context, do intel_vrr_set_transcoder_timings(), intel_vrr_set_vrr_timings(), and intel_vrr_enable_dc_balancing() also need to be converted to use the _fw variants? They are called during the vblank evasion loop (which disables interrupts and preemption) and still use intel_de_write(), which acquires the DMC wakelock and uncore spinlock, both of which can sleep on PREEMPT_RT: intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), trans_vrr_ctl(crtc_state)); Will leaving these unconverted result in the same sleep-in-atomic-context deadlocks during display mode sets or fastsets with VRR enabled? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
