On Wed, Nov 04, 2026 at 10:31:47PM +0000, [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, > ...).
This one's a bit funny because it uses the 'no_display' device info, and that one doesn't even populate ip.ver. So once all the mmio stuff is fully implemented inside the display code this will fall through the platform if-ladder all the way to the i9xx mmio funcs. But that should be fine since it only uses the _fw() stuff anyway and there won't be any difference for those. Only LNL will need special _fw() stuff due to the nasty read vs. write ordering workaround. > > 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). I think I'll just move the irq_ack() part to i915_irq.c where it belongs, and leave the handler part in the display. The handler should be fine to call without display since it just calls some opregion thing that does nothing if opregion wasn't initialized. Although I suppose we shouldn't even unmask the GSE interrupt if the display isn't there. Shouldn't be hard. > > 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). Yeah, I remember seeing that before and thinking that it's very much misplaced. I'll look into moving that into the display code. > > Should intel_de_init(display) be called unconditionally at the start of > intel_display_device_info_runtime_init() before the HAS_DISPLAY(display) > check? > > -- > Sashiko AI review · > https://sashiko.dev/#/patchset/[email protected]?part=1 -- Ville Syrjälä Intel
