On Wed, 04 Nov 2026, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > - [High] drm/i915/de: NULL display->mmio.uncore when intel_de_init() is > skipped > -- > > commit 0c47f694d22974ac99844e39b462f72389dd014b > Author: Ville Syrjälä <[email protected]> > > drm/i915/de: Introduce dislay->mmio and intel_de_init() > > This commit adds a display->mmio struct and intel_de_init() to initialize > display->mmio.uncore, and updates __to_uncore() to return display->mmio.uncore > instead of calling to_intel_uncore(display->drm). > > [ ... ] > >> diff --git a/drivers/gpu/drm/i915/display/intel_de.h >> b/drivers/gpu/drm/i915/display/intel_de.h >> index d0a239a369807..f1135ee5071b4 100644 >> --- a/drivers/gpu/drm/i915/display/intel_de.h >> +++ b/drivers/gpu/drm/i915/display/intel_de.h >> @@ -14,9 +14,11 @@ >> >> static inline struct intel_uncore *__to_uncore(struct intel_display >> *display) >> { >> - return to_intel_uncore(display->drm); >> + return display->mmio.uncore; >> } > > [ ... ] > >> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c >> b/drivers/gpu/drm/i915/display/intel_display_device.c >> index c74256359c253..f1bd1252a9602 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_device.c >> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c >> @@ -1798,6 +1798,13 @@ static void >> __intel_display_device_info_runtime_init(struct intel_display *displ >> struct intel_display_runtime_info *display_runtime = >> DISPLAY_RUNTIME_INFO(display); >> enum pipe pipe; >> >> + /* >> + * TODO end of intel_display_device_probe() would be a more >> + * logical place, but at the moment i915 calls that super >> + * early when things aren't fully set up yet. >> + */ >> + intel_de_init(display); > > [Severity: High] > Can this leave display->mmio.uncore NULL and cause a NULL pointer dereference > when HAS_DISPLAY(display) is already false before runtime init? > > In drivers/gpu/drm/i915/display/intel_display_device.c, > intel_display_device_info_runtime_init() only calls > __intel_display_device_info_runtime_init() when HAS_DISPLAY(display) is true: > > void intel_display_device_info_runtime_init(struct intel_display *display) > { > if (HAS_DISPLAY(display)) > __intel_display_device_info_runtime_init(display); > > On devices where intel_display_device_probe() sets DISPLAY_INFO() to > no_display—such as Ivybridge Quanta server SKUs (INTEL_IVB_Q_IDS via > has_no_display()), PCI IDs without an entry in intel_display_ids like Arctic > Sound-M (INTEL_ATS_M_IDS where find_platform_desc() returns NULL), or GMD_ID > platforms (INTEL_MTL_IDS / INTEL_ARL_IDS) where probe_gmdid_display() reads 0 > from GMD_ID_DISPLAY—HAS_DISPLAY(display) is false and > __intel_display_device_info_runtime_init() is skipped. > > Because __to_uncore(display) in drivers/gpu/drm/i915/display/intel_de.h now > returns display->mmio.uncore instead of computing > to_intel_uncore(display->drm), any subsequent unconditional intel_de_*() call > will pass a NULL struct intel_uncore pointer to intel_uncore_read*() or > intel_uncore_write*(): > > 1. On INTEL_IVB_Q_IDS, ilk_irq_handler() in drivers/gpu/drm/i915/i915_irq.c > unconditionally calls ilk_display_irq_master_disable() and > ilk_display_irq_master_enable() in > drivers/gpu/drm/i915/display/intel_display_irq.c on every interrupt, which > call intel_de_read_fw(display, DEIER) and intel_de_write_fw(display, DEIER, > ...). > > 2. On INTEL_ATS_M_IDS and headless GMD_ID platforms, gen11_irq_handler() and > dg1_irq_handler() unconditionally call gen11_gu_misc_irq_ack() in > drivers/gpu/drm/i915/display/intel_display_irq.c when GEN11_GU_MISC_IRQ is > set, which calls intel_de_read_fw(display, GEN11_GU_MISC_IIR) and > intel_de_write_fw(display, GEN11_GU_MISC_IIR, iir). > > 3. During system resume, i915_drm_resume() in > drivers/gpu/drm/i915/i915_driver.c unconditionally calls > intel_gmbus_reset(display) in drivers/gpu/drm/i915/display/intel_gmbus.c, > which calls intel_de_write_fw(display, GMBUS0(display), 0) and > intel_de_write_fw(display, GMBUS4(display), 0). > > Should intel_de_init(display) be called unconditionally at the start of > intel_display_device_info_runtime_init() before the HAS_DISPLAY(display) > check?
Seems annoying but valid, at least until we figure something else out. BR, Jani. -- Jani Nikula, Intel
