Re: [PATCH] drm/i915/dmc: fix an unlikely NULL pointer deference at probe

[Imre Deak <imre.deak@intel.com>]

On Tue, Dec 02, 2025 at 08:39:50PM +0200, Jani Nikula wrote:
> intel_dmc_update_dc6_allowed_count() oopses when DMC hasn't been
> initialized, and dmc is thus NULL.
> 
> That would be the case when the call path is
> intel_power_domains_init_hw() -> {skl,bxt,icl}_display_core_init() ->
> gen9_set_dc_state() -> intel_dmc_update_dc6_allowed_count(), as
> intel_power_domains_init_hw() is called *before* intel_dmc_init().
> 
> However, gen9_set_dc_state() calls intel_dmc_update_dc6_allowed_count()
> conditionally, depending on the current and target DC states. At probe,
> the target is disabled, but if DC6 is enabled, the function is called,
> and an oops follows. Apparently it's quite unlikely that DC6 is enabled
> at probe, as we haven't seen this failure mode before.
> 
> Add NULL checks and switch the dmc->display references to just display.
> 
> Fixes: 88c1f9a4d36d ("drm/i915/dmc: Create debugfs entry for dc6 counter")
> Cc: Mohammed Thasleem <mohammed.thasleem@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: <stable@vger.kernel.org> # v6.16+
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> Rare case, but this may also throw off the rc6 counting in debugfs when
> it does happen.

Yes, I missed the case where the driver is being loaded while DC6 is
enabled, this is what happens for the reporter:

i915 0000:00:04.0: [drm] *ERROR* DC state mismatch (0x0 -> 0x2)

That's odd, as DC6 requires the DMC firmware, which - if it's indeed
loaded by BIOS for instance - will be overwritten by the driver, not a
well specified sequence (even though the driver is trying to handle it
correctly by disabling any active firmware handler).

But as you pointed out this would also throw off the cooked-up DC6
counter tracking, so could instead the counter update depend on the
driver's DC state instead of the HW state? I.e. set
gen9_set_dc_state()/dc6_was_enabled,
intel_dmc_get_dc6_allowed_count()/dc6_enable if power_domains->dc_state
says that DC6 was indeed enabled by the driver (instead of checking the
HW state).

That would fix the reporter's oops when calling
intel_dmc_update_dc6_allowed_count(start_tracking=false), by not calling
it if the driver hasn't actually enabled DC6 and it would also keep the
DC6 counter tracking correct.

intel_dmc_update_dc6_allowed_count(start_tracking=true) would be also
guaranteed to be called only once the firmware is loaded, as until that
point enabling DC6 is blocked (by holding a reference on the DC_off
power well).

> ---
>  drivers/gpu/drm/i915/display/intel_dmc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> index 2fb6fec6dc99..169bbbc91f6d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> @@ -1570,10 +1570,10 @@ void intel_dmc_update_dc6_allowed_count(struct intel_display *display,
>  	struct intel_dmc *dmc = display_to_dmc(display);
>  	u32 dc5_cur_count;
>  
> -	if (DISPLAY_VER(dmc->display) < 14)
> +	if (!dmc || DISPLAY_VER(display) < 14)
>  		return;
>  
> -	dc5_cur_count = intel_de_read(dmc->display, DG1_DMC_DEBUG_DC5_COUNT);
> +	dc5_cur_count = intel_de_read(display, DG1_DMC_DEBUG_DC5_COUNT);
>  
>  	if (!start_tracking)
>  		dmc->dc6_allowed.count += dc5_cur_count - dmc->dc6_allowed.dc5_start;
> @@ -1587,7 +1587,7 @@ static bool intel_dmc_get_dc6_allowed_count(struct intel_display *display, u32 *
>  	struct intel_dmc *dmc = display_to_dmc(display);
>  	bool dc6_enabled;
>  
> -	if (DISPLAY_VER(display) < 14)
> +	if (!dmc || DISPLAY_VER(display) < 14)
>  		return false;
>  
>  	mutex_lock(&power_domains->lock);
> -- 
> 2.47.3
>