Re: [PATCH 04/12] drm/i915/mchbar: WARN when accessing non-MCHBAR registers via intel_mchbar_read*()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 25 Mar 2026, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The intel_mchbar_read*() functions should only be used for
> accessing MCHBAR registers. Warn if someone tries to use
> them for other registers.
>
> I suppose we could even have a dedicated type for MCHBAR
> registers. But that is true for many other special register
> types as well, and so far we haven't bothered adding any
> special types.

There's the i915_mcr_reg_t, and xe uses bits in the offset to identify
the type. But let's roll with this for now.

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_mchbar.c | 49 +++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_mchbar.c b/drivers/gpu/drm/i915/display/intel_mchbar.c
> index 950a36d586c3..82b47c00101a 100644
> --- a/drivers/gpu/drm/i915/display/intel_mchbar.c
> +++ b/drivers/gpu/drm/i915/display/intel_mchbar.c
> @@ -3,15 +3,60 @@
>   * Copyright © 2026 Intel Corporation
>   */
>  
> +#include <linux/minmax.h>
> +
> +#include <drm/drm_print.h>
> +
>  #include "intel_display_core.h"
>  #include "intel_mchbar.h"
> +#include "intel_mchbar_regs.h"
>  #include "intel_uncore.h"
>  
> +static bool has_mchbar_mirror(struct intel_display *display)
> +{
> +	return DISPLAY_VER(display) < 14;
> +}
> +
> +static u32 mchbar_mirror_base(struct intel_display *display)
> +{
> +	if (DISPLAY_VER(display) >= 6)
> +		return MCHBAR_MIRROR_BASE_SNB;
> +	else
> +		return MCHBAR_MIRROR_BASE;
> +}
> +
> +static u32 mchbar_mirror_end(struct intel_display *display)
> +{
> +	if (DISPLAY_VER(display) >= 12 && !display->platform.rocketlake)
> +		return MCHBAR_MIRROR_END_TGL;
> +	else if (DISPLAY_VER(display) >= 11)
> +		return MCHBAR_MIRROR_END_ICL_RKL;
> +	else if (DISPLAY_VER(display) >= 6)
> +		return MCHBAR_MIRROR_END_SNB;
> +	else
> +		return MCHBAR_MIRROR_END;
> +}
> +
> +static u32 mchbar_mirror_len(struct intel_display *display)
> +{
> +	return mchbar_mirror_end(display) - mchbar_mirror_base(display) + 1;
> +}
> +
> +static bool is_mchbar_reg(struct intel_display *display, i915_reg_t reg)
> +{
> +	return has_mchbar_mirror(display) &&
> +		in_range32(i915_mmio_reg_offset(reg),
> +			   mchbar_mirror_base(display),
> +			   mchbar_mirror_len(display));

Pedantically reg offset + size (2, 4 or 8 bytes) could overflow the
range even if the reg offset is within range for 1 byte access.

Not a big deal I guess.

> +}
> +
>  u16 intel_mchbar_read16(struct intel_display *display,
>  			i915_reg_t reg)
>  {
>  	struct intel_uncore *uncore = to_intel_uncore(display->drm);
>  
> +	drm_WARN_ON(display->drm, !is_mchbar_reg(display, reg));

Is the backtrace enough to pinpoint the offending register? No need to
use drm_WARN() with a message indicating the reg? *shrug*

Overall,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +
>  	return intel_uncore_read16(uncore, reg);
>  }
>  
> @@ -20,6 +65,8 @@ u32 intel_mchbar_read(struct intel_display *display,
>  {
>  	struct intel_uncore *uncore = to_intel_uncore(display->drm);
>  
> +	drm_WARN_ON(display->drm, !is_mchbar_reg(display, reg));
> +
>  	return intel_uncore_read(uncore, reg);
>  }
>  
> @@ -28,5 +75,7 @@ u64 intel_mchbar_read64(struct intel_display *display,
>  {
>  	struct intel_uncore *uncore = to_intel_uncore(display->drm);
>  
> +	drm_WARN_ON(display->drm, !is_mchbar_reg(display, reg));
> +
>  	return intel_uncore_read64(uncore, reg);
>  }

-- 
Jani Nikula, Intel