Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/i915/de: intel_de_init() skipped when HAS_DISPLAY() is initially false --
commit 86812b856eea85785d3fee2a5eb70c5246338748 Author: Ville Syrjälä <[email protected]> drm/i915/de: Introduce display->mmio.lock This commit introduces a display-specific spinlock, display->mmio.lock, to serialize non-fw display register accesses on SNB+ platforms in i915 and switches explicit display users of uncore.lock over to display->mmio.lock. > diff --git a/drivers/gpu/drm/i915/display/intel_de.c > b/drivers/gpu/drm/i915/display/intel_de.c > index 66de0d6be5699..8b7a4e696ff96 100644 > --- a/drivers/gpu/drm/i915/display/intel_de.c > +++ b/drivers/gpu/drm/i915/display/intel_de.c [ ... ] > @@ -245,12 +288,30 @@ static const struct intel_mmio_funcs lnl_mmio_funcs = { > .write = lnl_mmio_write, > }; > > +static bool driver_is_i915(void) > +{ > +#ifdef I915 > + return true; > +#else > + return false; > +#endif > +} > + > void intel_de_init(struct intel_display *display) > { > display->mmio.uncore = to_intel_uncore(display->drm); > > - if (DISPLAY_VER(display) >= 20) > - display->mmio.funcs = &lnl_mmio_funcs; > - else > - display->mmio.funcs = &i9xx_mmio_funcs; > + spin_lock_init(&display->mmio.lock); [Severity: High] Can display->mmio.lock and display->mmio.uncore remain uninitialized when HAS_DISPLAY(display) is false? During i915_driver_mmio_probe(), intel_display_device_info_runtime_init() only calls __intel_display_device_info_runtime_init() -> intel_de_init() when HAS_DISPLAY(display) is true: drivers/gpu/drm/i915/display/intel_display_device.c: intel_display_device_info_runtime_init() { if (HAS_DISPLAY(display)) __intel_display_device_info_runtime_init(display); ... } When intel_display_device_probe() takes the no_display path (such as on INTEL_IVB_Q_IDS via has_no_display(), INTEL_ATS_M_IDS via !find_platform_desc(), or GMD_ID platforms where GMD_ID_DISPLAY reads 0 in probe_gmdid_display()), pipe_mask is 0, so HAS_DISPLAY(display) is false and intel_de_init() is skipped. However, several paths in i915 still access display registers when HAS_DISPLAY(display) is false: 1. On INTEL_IVB_Q_IDS, ilk_irq_handler() unconditionally calls ilk_display_irq_master_disable() and ilk_display_irq_master_enable(), which call intel_de_read_fw(display, DEIER) and intel_de_write_fw(display, DEIER, ...) and dereference display->mmio.uncore while it is NULL. 2. On headless Gen11+ and GMD_ID platforms (such as INTEL_ATS_M_IDS), gen11_irq_handler() and dg1_irq_handler() unconditionally call gen11_gu_misc_irq_ack(), which calls intel_de_read_fw(display, GEN11_GU_MISC_IIR) when GEN11_GU_MISC_IRQ is signalled. 3. During resume, i915_drm_resume() unconditionally calls intel_gmbus_reset(display), which calls intel_de_write_fw(display, GMBUS0(display), 0). Should intel_de_init() be called unconditionally even when HAS_DISPLAY(display) is false so that display->mmio.uncore, display->mmio.lock, and display->mmio.funcs are always initialized? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
