Re: [PATCH 5/9] drm/i915/de: Introduce display->mmio.lock

[sashiko-bot@kernel.org]

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ä <ville.syrjala@linux.intel.com>

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/20261104221842.18799-1-ville.syrjala@linux.intel.com?part=5