On Thu, 13 Nov 2025, Ville Syrjälä <[email protected]> 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 <[email protected]>
>> ---
>> 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
