Re: [PATCH 02/19] drm/i915/vga: Get rid of intel_vga_reset_io_mem()

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

On Mon, 08 Dec 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Remove the MSR VGA register access from the power well hook, and
> just do it once in intel_vga_disable().
>
> Turns out that the hardware has two levels of MDA vs. CGA decode
> logic: GPU level and display engine level. When we write the MSR
> register MDA/CGA mode selection bit both decode logics are updated
> accordingly, so that whichever register accessed the GPU claims
> will also be claimed by the display engine on the RMbus. If the
> two get out of sync the GPU will claim the register accesses but
> the display engine will not, leading to RMbus NoClaim errors.
>
> The way to get the two decode logics out of sync is by resetting
> the power well housing the VGA stuff, while we are in CGA mode.
> At that point only the display engine level decode logic will be
> updated with the new MSR register reset value (which selects MDA
> mode), and the GPU level decode logic will retain its previous
> state (GGA mode). To avoid the mismatch we just have to switch
> to MDA mode with an explicit MSR register write.
>
> Currently this is being done in a somewhat dodgy manner whenever
> the power well gets enabled. But doing if from the power well
> hook is not actually necessary since the GPU level decode logic
> will retain its state even when the power well is disabled. Ie.
> doing it just the one time is sufficient, and that can be done
> when we're anyway writing other VGA registers while disabling
> the VGA plane.
>
> See commit f9dcb0dfee98 ("drm/i915: touch VGA MSR after we
> enable the power well") for the original details.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I'll take your word for it. Looks plausible.

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

> ---
>  .../i915/display/intel_display_power_well.c   |  3 --
>  drivers/gpu/drm/i915/display/intel_vga.c      | 40 +++++++++----------
>  2 files changed, 20 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> index db185a859133..52b20118ace6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> @@ -204,9 +204,6 @@ int intel_power_well_refcount(struct i915_power_well *power_well)
>  static void hsw_power_well_post_enable(struct intel_display *display,
>  				       u8 irq_pipe_mask, bool has_vga)
>  {
> -	if (has_vga)
> -		intel_vga_reset_io_mem(display);
> -
>  	if (irq_pipe_mask)
>  		gen8_irq_power_well_post_enable(display, irq_pipe_mask);
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_vga.c b/drivers/gpu/drm/i915/display/intel_vga.c
> index f13734cfd904..39c68aec647b 100644
> --- a/drivers/gpu/drm/i915/display/intel_vga.c
> +++ b/drivers/gpu/drm/i915/display/intel_vga.c
> @@ -47,8 +47,8 @@ void intel_vga_disable(struct intel_display *display)
>  	struct pci_dev *pdev = to_pci_dev(display->drm->dev);
>  	i915_reg_t vga_reg = intel_vga_cntrl_reg(display);
>  	enum pipe pipe;
> +	u8 msr, sr1;
>  	u32 tmp;
> -	u8 sr1;
>  
>  	tmp = intel_de_read(display, vga_reg);
>  	if (tmp & VGA_DISP_DISABLE)
> @@ -66,35 +66,35 @@ void intel_vga_disable(struct intel_display *display)
>  
>  	/* WaEnableVGAAccessThroughIOPort:ctg,elk,ilk,snb,ivb,vlv,hsw */
>  	vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
> +
>  	outb(0x01, VGA_SEQ_I);
>  	sr1 = inb(VGA_SEQ_D);
>  	outb(sr1 | VGA_SR01_SCREEN_OFF, VGA_SEQ_D);
> +
> +	msr = inb(VGA_MIS_R);
> +	/*
> +	 * VGA_MIS_COLOR controls both GPU level and display engine level
> +	 * MDA vs. CGA decode logic. But when the register gets reset
> +	 * (reset value has VGA_MIS_COLOR=0) by the power well, only the
> +	 * display engine level decode logic gets notified.
> +	 *
> +	 * Switch to MDA mode to make sure the GPU level decode logic will
> +	 * be in sync with the display engine level decode logic after the
> +	 * power well has been reset. Otherwise the GPU will claim CGA
> +	 * register accesses but the display engine will not, causing
> +	 * RMbus NoClaim errors.
> +	 */
> +	msr &= ~VGA_MIS_COLOR;
> +	outb(msr, VGA_MIS_W);
> +
>  	vga_put(pdev, VGA_RSRC_LEGACY_IO);
> +
>  	udelay(300);
>  
>  	intel_de_write(display, vga_reg, VGA_DISP_DISABLE);
>  	intel_de_posting_read(display, vga_reg);
>  }
>  
> -void intel_vga_reset_io_mem(struct intel_display *display)
> -{
> -	struct pci_dev *pdev = to_pci_dev(display->drm->dev);
> -
> -	/*
> -	 * After we re-enable the power well, if we touch VGA register 0x3d5
> -	 * we'll get unclaimed register interrupts. This stops after we write
> -	 * anything to the VGA MSR register. The vgacon module uses this
> -	 * register all the time, so if we unbind our driver and, as a
> -	 * consequence, bind vgacon, we'll get stuck in an infinite loop at
> -	 * console_unlock(). So make here we touch the VGA MSR register, making
> -	 * sure vgacon can keep working normally without triggering interrupts
> -	 * and error messages.
> -	 */
> -	vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
> -	outb(inb(VGA_MIS_R), VGA_MIS_W);
> -	vga_put(pdev, VGA_RSRC_LEGACY_IO);
> -}
> -
>  static int intel_gmch_vga_set_state(struct intel_display *display, bool enable_decode)
>  {
>  	struct pci_dev *pdev = to_pci_dev(display->drm->dev);

-- 
Jani Nikula, Intel