On Wed, Nov 04, 2026 at 10:36:37PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s)
> to consider:
> - [High] drm/i915/de: NULL deref on displayless devices in unclaimed MMIO
> checks
> - [Medium] drm/i915/de: intel_de_init() enables FPGA_DBG check on Ivy Bridge
> - [Medium] drm/i915/de: unclaimed MMIO status is not cleared on initialization
> - [Low] drm/i915/display: unused mmio.mmio_debug field in struct intel_display
> --
>
> commit 74c572411ef669251a5e2e347d14a7771407e433
> Author: Ville Syrjälä <[email protected]>
>
> drm/i915/de: Move unclaimed mmio detection into display code
>
> This commit moves the FPGA_DBG and CLAIM_ER unclaimed MMIO access detection
> from intel_uncore into the display code, protecting the tracking state with
> display->mmio.lock.
>
> > diff --git a/drivers/gpu/drm/i915/display/intel_de.c
> > b/drivers/gpu/drm/i915/display/intel_de.c
> > index 8b7a4e696ff96..0a51105e0fbad 100644
> > --- a/drivers/gpu/drm/i915/display/intel_de.c
> > +++ b/drivers/gpu/drm/i915/display/intel_de.c
>
> [ ... ]
<snip>
> > @@ -304,7 +470,11 @@ void intel_de_init(struct intel_display *display)
> > spin_lock_init(&display->mmio.lock);
> >
> > if (driver_is_i915()) {
> > - if (display->platform.valleyview ||
> > display->platform.cherryview)
> > + if (display->platform.valleyview ||
> > display->platform.cherryview)
> > + display->mmio.funcs = &vlv_mmio_funcs;
> > + else if (DISPLAY_VER(display) >= 7)
> > + display->mmio.funcs = &ivb_mmio_funcs;
> > + else if (DISPLAY_VER(display) >= 6)
> > display->mmio.funcs = &snb_mmio_funcs;
> > else
> > display->mmio.funcs = &i9xx_mmio_funcs;
>
> [Severity: Medium]
> Does checking DISPLAY_VER(display) >= 7 in intel_de_init() unintentionally
> enable FPGA_DBG unclaimed MMIO checks on Ivy Bridge?
Yes, but only because I misremembered that IVB has the feature already.
IVB does have the FPGA_DBG register already (which is the part I
remembered correctly), but the unclaimed register access bit is not
there until HSW. I guess that makes perfect since since HSW is the
one that also introduced us to power wells. So I'll need to
s/ivb/hsw/ here.
>
> Previously, intel_uncore_init_mmio() gated FPGA_DBG unclaimed MMIO detection
> on HAS_FPGA_DBG_UNCLAIMED(display). In
> drivers/gpu/drm/i915/display/intel_display_device.c, has_fpga_dbg is set on
> hsw_desc and bdw_desc onwards, whereas ivb_display has ip.ver = 7 and
> has_fpga_dbg = 0. Checking DISPLAY_VER(display) >= 7 assigns ivb_mmio_funcs to
> Ivy Bridge as well, and leaves HAS_FPGA_DBG_UNCLAIMED(display) unused.
>
> Should this check HAS_FPGA_DBG_UNCLAIMED(display) instead?
>
> > @@ -314,4 +484,7 @@ void intel_de_init(struct intel_display *display)
> > else
> > display->mmio.funcs = &i9xx_mmio_funcs;
> > }
> > +
> > + if (display->mmio.funcs->check_for_unclaimed_mmio)
> > + display->mmio.unclaimed_mmio_check = 1;
> > }
>
> [Severity: Medium]
> Previously, intel_uncore_init_mmio() cleared any stale unclaimed MMIO bit left
> set by BIOS or pre-boot firmware during initialization:
>
> /* clear out unclaimed reg detection bit */
> if (intel_uncore_unclaimed_mmio(uncore))
> drm_dbg(&i915->drm, "unclaimed mmio detected on uncore init,
> clearing\n");
>
> With that removed and not replaced in intel_de_init(), if mmio_debug is
Hmm. I thought I took care of that, but looks like I only adjusted the
resume path, not the init path.
> disabled by default, won't a stale FPGA_DBG_RM_NOCLAIM or CLAIM_ER bit from
> boot firmware remain latched until intel_de_arm_unclaimed_mmio_detection()
> runs at the end of the first modeset or in
> intel_display_driver_mmio_suspend_late(), triggering a false-positive error or
> falsely arming oneshot mmio_debug reporting?
>
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
> > b/drivers/gpu/drm/i915/display/intel_display_core.h
> > index aded2af8c92a6..cb18ee040b5d6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> > @@ -539,11 +539,18 @@ struct intel_display {
> > * serializes all display register accesses done
> > * using intel_de_{read,write}(), but not those
> > * done using intel_de_{read,write}_fw().
> > + *
> > + * The lock also protects mmio.mmio_debug and
> > + * mmio.unclaimed_mmio_check.
> > */
> > spinlock_t lock;
> >
> > struct intel_uncore *uncore;
> > const struct intel_mmio_funcs *funcs;
> > +
> > + /* protected by mmio.lock */
> > + int mmio_debug;
> > + int unclaimed_mmio_check;
> > } mmio;
>
> [Severity: Low]
> This isn't a bug, but display->mmio.mmio_debug is never used since
> check_unclaimed_mmio_pre(), check_unclaimed_mmio_post(), and
> intel_de_arm_unclaimed_mmio_detection() all use display->params.mmio_debug.
> Should mmio.mmio_debug be dropped from struct intel_display?
Aye. Looks like some crap I accidentally left in.
--
Ville Syrjälä
Intel
