Re: [PATCH] drm/i915/gmch: find bridge device locally

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 13, 2025 at 03:38:06PM +0200, Jani Nikula wrote:
> We don't really need the cached i915->gmch.pdev reference. Look up the
> bridge device locally, and remove the final dependency on struct
> drm_i915_private and i915_drv.h.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_gmch.c | 25 ++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_gmch.c b/drivers/gpu/drm/i915/display/intel_gmch.c
> index 475f2b6ce39e..9bf36f02a062 100644
> --- a/drivers/gpu/drm/i915/display/intel_gmch.c
> +++ b/drivers/gpu/drm/i915/display/intel_gmch.c
> @@ -9,7 +9,6 @@
>  #include <drm/drm_print.h>
>  #include <drm/intel/i915_drm.h>
>  
> -#include "i915_drv.h"
>  #include "intel_display_core.h"
>  #include "intel_display_types.h"
>  #include "intel_gmch.h"
> @@ -17,18 +16,26 @@
>  
>  static int intel_gmch_vga_set_state(struct intel_display *display, bool enable_decode)
>  {
> -	struct drm_i915_private *i915 = to_i915(display->drm);
> -	struct pci_dev *bridge = i915->gmch.pdev;
> +	struct pci_dev *pdev = to_pci_dev(display->drm->dev);
> +	struct pci_dev *bridge;
>  	unsigned int reg = DISPLAY_VER(display) >= 6 ? SNB_GMCH_CTRL : INTEL_GMCH_CTRL;
>  	u16 gmch_ctrl;
> +	int ret = 0;
> +
> +	bridge = pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus), 0, PCI_DEVFN(0, 0));
> +	if (!bridge) {
> +		drm_err(display->drm, "bridge device not found\n");
> +		return -EIO;
> +	}
>  
>  	if (pci_read_config_word(bridge, reg, &gmch_ctrl)) {

I think you could just use pci_bus_{read,write}_config_word() and then
you don't need the pci_dev reference. That's what I've used in the
overlay workaround code as well.

I was pondering how this even works on discrete GPUs, but there it
seems the GPU PCI device is devfn=0.0 sitting on its own bus. So it
seems that it should work. Well, work in the sense that it accesses
the correct register. But in reality this code is complete nonsense
as this register is locked by the BIOS and so can't actually be
written by the driver.

The alternative approach would be to use the actual GPU PCI device
on SNB+ since the GGC register is also mirrored there (and I think
also mirrored in MCHBAR, so we could also use MMIO to access it
instead). I suppose it's technically the mirror that we're accessing
on dGPUs here always. On integrated we could choose to use either one.

>  		drm_err(display->drm, "failed to read control word\n");
> -		return -EIO;
> +		ret = -EIO;
> +		goto out;
>  	}
>  
>  	if (!!(gmch_ctrl & INTEL_GMCH_VGA_DISABLE) == !enable_decode)
> -		return 0;
> +		goto out;
>  
>  	if (enable_decode)
>  		gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
> @@ -37,10 +44,14 @@ static int intel_gmch_vga_set_state(struct intel_display *display, bool enable_d
>  
>  	if (pci_write_config_word(bridge, reg, gmch_ctrl)) {
>  		drm_err(display->drm, "failed to write control word\n");
> -		return -EIO;
> +		ret = -EIO;
> +		goto out;
>  	}
>  
> -	return 0;
> +out:
> +	pci_dev_put(bridge);
> +
> +	return ret;
>  }
>  
>  unsigned int intel_gmch_vga_set_decode(struct pci_dev *pdev, bool enable_decode)
> -- 
> 2.47.3

-- 
Ville Syrjälä
Intel