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

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

On Thu, 13 Nov 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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.

Oh, neat, seems cleaner.

> 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.

I'm just trying to dodge *that* specific part of the mess during the
refactoring! ;D

BR,
Jani.

>
>>  		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

-- 
Jani Nikula, Intel