Re: [PATCH 7/9] drm/i915/de: Move unclaimed mmio detection into display code

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Nov 04, 2026 at 10:36:37PM +0000, sashiko-bot@kernel.org 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ä <ville.syrjala@linux.intel.com>
> 
> 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