Re: [PATCH 01/19] drm/i915/vga: Register vgaarb client later

[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>
>
> Currently we register to vgaarb way too early. Thus it is
> possible that the entire VGA decode logic gets nuked via
> GMCH_CTRL before intel_vga_disable() has even disabled the
> VGA plane. This could even cause a system hang because
> we'll be unable to turn off the VGA plane gracefully.
>
> Move the vgaarb registration into intel_display_driver_register().
> I suppose we could do it a bit earlier (after intel_vga_disable()),
> but not convinced there's any point.
>
> Also the error handling here is pointless since the
> registration can't fail (unless the device isn't a VGA class
> in which case all VGA decode logic should aleady be disabled
> by the BIOS via GMCH_CTRL). But let's toss in a WARN to catch
> any future breakage of vga_client_register().
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

After this, intel_vga_register() is only called for HAS_DISPLAY(), while
previously it was called unconditionally.

It's probably fine?

Other than that,

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

> ---
>  .../drm/i915/display/intel_display_driver.c    | 18 +++++++-----------
>  drivers/gpu/drm/i915/display/intel_vga.c       |  7 ++-----
>  drivers/gpu/drm/i915/display/intel_vga.h       |  2 +-
>  3 files changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 7e000ba3e08b..b149976f527b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -214,16 +214,12 @@ int intel_display_driver_probe_noirq(struct intel_display *display)
>  
>  	intel_bios_init(display);
>  
> -	ret = intel_vga_register(display);
> -	if (ret)
> -		goto cleanup_bios;
> -
>  	intel_psr_dc5_dc6_wa_init(display);
>  
>  	/* FIXME: completely on the wrong abstraction layer */
>  	ret = intel_power_domains_init(display);
>  	if (ret < 0)
> -		goto cleanup_vga;
> +		goto cleanup_bios;
>  
>  	intel_pmdemand_init_early(display);
>  
> @@ -235,7 +231,7 @@ int intel_display_driver_probe_noirq(struct intel_display *display)
>  	display->hotplug.dp_wq = alloc_ordered_workqueue("intel-dp", 0);
>  	if (!display->hotplug.dp_wq) {
>  		ret = -ENOMEM;
> -		goto cleanup_vga_client_pw_domain_dmc;
> +		goto cleanup_pw_domain_dmc;
>  	}
>  
>  	display->wq.modeset = alloc_ordered_workqueue("i915_modeset", 0);
> @@ -307,11 +303,9 @@ int intel_display_driver_probe_noirq(struct intel_display *display)
>  	destroy_workqueue(display->wq.modeset);
>  cleanup_wq_dp:
>  	destroy_workqueue(display->hotplug.dp_wq);
> -cleanup_vga_client_pw_domain_dmc:
> +cleanup_pw_domain_dmc:
>  	intel_dmc_fini(display);
>  	intel_power_domains_driver_remove(display);
> -cleanup_vga:
> -	intel_vga_unregister(display);
>  cleanup_bios:
>  	intel_bios_driver_remove(display);
>  
> @@ -566,6 +560,8 @@ void intel_display_driver_register(struct intel_display *display)
>  	if (!HAS_DISPLAY(display))
>  		return;
>  
> +	intel_vga_register(display);
> +
>  	/* Must be done after probing outputs */
>  	intel_opregion_register(display);
>  	intel_acpi_video_register(display);
> @@ -658,8 +654,6 @@ void intel_display_driver_remove_nogem(struct intel_display *display)
>  
>  	intel_power_domains_driver_remove(display);
>  
> -	intel_vga_unregister(display);
> -
>  	intel_bios_driver_remove(display);
>  }
>  
> @@ -687,6 +681,8 @@ void intel_display_driver_unregister(struct intel_display *display)
>  
>  	acpi_video_unregister();
>  	intel_opregion_unregister(display);
> +
> +	intel_vga_unregister(display);
>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/display/intel_vga.c b/drivers/gpu/drm/i915/display/intel_vga.c
> index c45c4bbc3f95..f13734cfd904 100644
> --- a/drivers/gpu/drm/i915/display/intel_vga.c
> +++ b/drivers/gpu/drm/i915/display/intel_vga.c
> @@ -135,7 +135,7 @@ static unsigned int intel_gmch_vga_set_decode(struct pci_dev *pdev, bool enable_
>  		return VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
>  }
>  
> -int intel_vga_register(struct intel_display *display)
> +void intel_vga_register(struct intel_display *display)
>  {
>  
>  	struct pci_dev *pdev = to_pci_dev(display->drm->dev);
> @@ -150,10 +150,7 @@ int intel_vga_register(struct intel_display *display)
>  	 * vga_client_register() fails with -ENODEV.
>  	 */
>  	ret = vga_client_register(pdev, intel_gmch_vga_set_decode);
> -	if (ret && ret != -ENODEV)
> -		return ret;
> -
> -	return 0;
> +	drm_WARN_ON(display->drm, ret && ret != -ENODEV);
>  }
>  
>  void intel_vga_unregister(struct intel_display *display)
> diff --git a/drivers/gpu/drm/i915/display/intel_vga.h b/drivers/gpu/drm/i915/display/intel_vga.h
> index 16d699f3b641..80084265c6cd 100644
> --- a/drivers/gpu/drm/i915/display/intel_vga.h
> +++ b/drivers/gpu/drm/i915/display/intel_vga.h
> @@ -10,7 +10,7 @@ struct intel_display;
>  
>  void intel_vga_reset_io_mem(struct intel_display *display);
>  void intel_vga_disable(struct intel_display *display);
> -int intel_vga_register(struct intel_display *display);
> +void intel_vga_register(struct intel_display *display);
>  void intel_vga_unregister(struct intel_display *display);
>  
>  #endif /* __INTEL_VGA_H__ */

-- 
Jani Nikula, Intel