Public
________________________________ From: Lazar, Lijo <[email protected]> Sent: Tuesday, June 2, 2026 1:43 AM To: Kasiviswanathan, Harish <[email protected]>; [email protected] <[email protected]> Subject: Re: [PATCH v2 1/2] drm/amdgpu: Add enum for PCIe BAR regions On 02-Jun-26 2:25 AM, Harish Kasiviswanathan wrote: > Use enum instead of hard coded values. There is no functional change. > > v2: > - Add amdgpu_get_bar_idx() to map BAR roles to PCI BAR indices > > Signed-off-by: Harish Kasiviswanathan <[email protected]> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 17 ++++++ > drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 56 ++++++++++++++----- > .../gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c | 9 ++- > drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 6 +- > drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c | 6 +- > drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c | 6 +- > drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c | 6 +- > drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 6 +- > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 6 +- > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 9 ++- > 11 files changed, 100 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index 5d7bfa59424a..1944d1bece86 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -1405,6 +1405,23 @@ bool amdgpu_device_supports_boco(struct amdgpu_device > *adev); > bool amdgpu_device_supports_smart_shift(struct amdgpu_device *adev); > int amdgpu_device_supports_baco(struct amdgpu_device *adev); > void amdgpu_device_detect_runtime_pm_mode(struct amdgpu_device *adev); > + > +/** > + * enum amdgpu_pcie_bar - PCIe BAR role identifiers > + * @AMDGPU_PCIE_BAR_VRAM: VRAM aperture > + * @AMDGPU_PCIE_BAR_DOORBELL: Doorbell aperture (Bonaire+) > + * @AMDGPU_PCIE_BAR_MMIO: MMIO register aperture > + * > + * Use amdgpu_get_bar_idx() to map a role to the PCI BAR index on a given > ASIC. > + */ > +enum amdgpu_pcie_bar { > + AMDGPU_PCIE_BAR_VRAM = 0, > + AMDGPU_PCIE_BAR_DOORBELL = 2, > + AMDGPU_PCIE_BAR_MMIO = 5, > +}; This may just be treated as an enum, it's not required to designate numbers for BAR index. [HK]: Makes sense. I will make the change. > + > +int amdgpu_get_bar_idx(struct amdgpu_device *adev, enum amdgpu_pcie_bar bar); > + > bool amdgpu_device_is_peer_accessible(struct amdgpu_device *adev, > struct amdgpu_device *peer_adev); > int amdgpu_device_baco_enter(struct amdgpu_device *adev); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > index aa039e148a5e..7e253bb35434 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > @@ -111,11 +111,13 @@ static bool amdgpu_read_bios_from_vram(struct > amdgpu_device *adev) > return false; > > /* FB BAR not enabled */ > - if (pci_resource_len(adev->pdev, 0) == 0) > + if (pci_resource_len(adev->pdev, > + amdgpu_get_bar_idx(adev, AMDGPU_PCIE_BAR_VRAM)) > == 0) > return false; > > adev->bios = NULL; > - vram_base = pci_resource_start(adev->pdev, 0); > + vram_base = pci_resource_start(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > > adev->bios = kmalloc(size, GFP_KERNEL); > if (!adev->bios) > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 5ff224163bab..2e1e5791f123 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -1103,6 +1103,34 @@ void amdgpu_device_wb_free(struct amdgpu_device *adev, > u32 wb) > spin_unlock_irqrestore(&adev->wb.lock, flags); > } > > +/** > + * amdgpu_get_bar_idx - map a BAR role to the PCI BAR index > + * @adev: amdgpu_device pointer > + * @bar: BAR role to look up > + * > + * Return the PCI BAR index for @bar on @adev. > + * > + * VRAM is always BAR 0. Doorbells were introduced at Bonaire (CIK): > pre-Bonaire > + * ASICs have no dedicated doorbell BAR, and BAR 2 is used for MMIO > registers. > + * Bonaire and newer expose a doorbell aperture at BAR 2 and MMIO at BAR 5. > + */ > +int amdgpu_get_bar_idx(struct amdgpu_device *adev, enum amdgpu_pcie_bar bar) > +{ > + switch (bar) { > + case AMDGPU_PCIE_BAR_VRAM: > + return AMDGPU_PCIE_BAR_VRAM; > + case AMDGPU_PCIE_BAR_DOORBELL: > + return AMDGPU_PCIE_BAR_DOORBELL; > + case AMDGPU_PCIE_BAR_MMIO: > + if (adev->asic_type >= CHIP_BONAIRE) > + return AMDGPU_PCIE_BAR_MMIO; > + return AMDGPU_PCIE_BAR_DOORBELL; Was not expecting this function to return the enum itself. Input is enum and output is a hardcoded index like 0, 2, 5. [HK]: Yes, this sort of follows up from previous comment. > + default: > + WARN_ON(1); pci_resource_len() expects a valid bar number. pci_resource_len(adev->pdev, amdgpu_get_bar_idx()) usage may invoke some static code analyzer warnings. Thinking again, a one-time assignment of below ones somewhere in early init amdgpu_init_pci_bars() may be simpler to deal with. gmc.aper_bar_idx rmmio_bar_idx doorbell.bar_idx [HK]: Don't know about this. This sort of scatters the initialization unless we add a new structure. Something like u8 pcie_bar_idx[PCIE_BAR__COUNT] to amdgpu_device. Thanks, Lijo > + return -EINVAL; > + } > +} > + > /** > * amdgpu_device_resize_fb_bar - try to resize FB BAR > * > @@ -1146,7 +1174,8 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device > *adev) > > /* skip if the bios has already enabled large BAR */ > if (adev->gmc.real_vram_size && > - (pci_resource_len(adev->pdev, 0) >= adev->gmc.real_vram_size)) > + (pci_resource_len(adev->pdev, > + amdgpu_get_bar_idx(adev, AMDGPU_PCIE_BAR_VRAM)) > >= adev->gmc.real_vram_size)) > return 0; > > /* Check if the root BUS has 64bit memory resources */ > @@ -1165,7 +1194,8 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device > *adev) > return 0; > > /* Limit the BAR size to what is available */ > - max_size = pci_rebar_get_max_size(adev->pdev, 0); > + max_size = pci_rebar_get_max_size(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > if (max_size < 0) > return 0; > rbar_size = min(max_size, rbar_size); > @@ -1178,9 +1208,11 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device > *adev) > /* Tear down doorbell as resizing will release BARs */ > amdgpu_doorbell_fini(adev); > > - r = pci_resize_resource(adev->pdev, 0, rbar_size, > - (adev->asic_type >= CHIP_BONAIRE) ? 1 << 5 > - : 1 << 2); > + /* Resize the VRAM BAR. Exclude the MMIO BAR from being released. */ > + r = pci_resize_resource(adev->pdev, > + amdgpu_get_bar_idx(adev, AMDGPU_PCIE_BAR_VRAM), > + rbar_size, > + BIT(amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_MMIO))); > if (r == -ENOSPC) > dev_info(adev->dev, > "Not enough PCI address space for a large BAR."); > @@ -1191,7 +1223,8 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device > *adev) > * using the device. > */ > r = amdgpu_doorbell_init(adev); > - if (r || (pci_resource_flags(adev->pdev, 0) & IORESOURCE_UNSET)) > + if (r || (pci_resource_flags(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)) & IORESOURCE_UNSET)) > return -ENODEV; > > pci_write_config_word(adev->pdev, PCI_COMMAND, cmd); > @@ -3825,13 +3858,10 @@ int amdgpu_device_init(struct amdgpu_device *adev, > > /* Registers mapping */ > /* TODO: block userspace mapping of io register */ > - if (adev->asic_type >= CHIP_BONAIRE) { > - adev->rmmio_base = pci_resource_start(adev->pdev, 5); > - adev->rmmio_size = pci_resource_len(adev->pdev, 5); > - } else { > - adev->rmmio_base = pci_resource_start(adev->pdev, 2); > - adev->rmmio_size = pci_resource_len(adev->pdev, 2); > - } > + adev->rmmio_base = pci_resource_start(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_MMIO)); > + adev->rmmio_size = pci_resource_len(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_MMIO)); > > for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++) > atomic_set(&adev->pm.pwr_state[i], POWER_STATE_UNKNOWN); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c > index bc7858567321..826b80481908 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c > @@ -201,14 +201,17 @@ int amdgpu_doorbell_init(struct amdgpu_device *adev) > return 0; > } > > - if (pci_resource_flags(adev->pdev, 2) & IORESOURCE_UNSET) > + if (pci_resource_flags(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_DOORBELL)) & IORESOURCE_UNSET) > return -EINVAL; > > amdgpu_asic_init_doorbell_index(adev); > > /* doorbell bar mapping */ > - adev->doorbell.base = pci_resource_start(adev->pdev, 2); > - adev->doorbell.size = pci_resource_len(adev->pdev, 2); > + adev->doorbell.base = pci_resource_start(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_DOORBELL)); > + adev->doorbell.size = pci_resource_len(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_DOORBELL)); > > adev->doorbell.num_kernel_doorbells = > min_t(u32, adev->doorbell.size / sizeof(u32), > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c > b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c > index 8523833a74fb..728d93e96ea1 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c > @@ -694,8 +694,10 @@ static int gmc_v10_0_mc_init(struct amdgpu_device *adev) > if (r) > return r; > } > - adev->gmc.aper_base = pci_resource_start(adev->pdev, 0); > - adev->gmc.aper_size = pci_resource_len(adev->pdev, 0); > + adev->gmc.aper_base = pci_resource_start(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > + adev->gmc.aper_size = pci_resource_len(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > > #ifdef CONFIG_X86_64 > if ((adev->flags & AMD_IS_APU) && !amdgpu_passthrough(adev)) { > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c > b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c > index 16388e3caea3..a16e681b4de0 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c > @@ -694,8 +694,10 @@ static int gmc_v11_0_mc_init(struct amdgpu_device *adev) > if (r) > return r; > } > - adev->gmc.aper_base = pci_resource_start(adev->pdev, 0); > - adev->gmc.aper_size = pci_resource_len(adev->pdev, 0); > + adev->gmc.aper_base = pci_resource_start(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > + adev->gmc.aper_size = pci_resource_len(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > > #ifdef CONFIG_X86_64 > if ((adev->flags & AMD_IS_APU) && !amdgpu_passthrough(adev)) { > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c > b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c > index 586703ec0dfa..5c2b4d2f3f16 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c > @@ -746,8 +746,10 @@ static int gmc_v12_0_mc_init(struct amdgpu_device *adev) > return r; > } > > - adev->gmc.aper_base = pci_resource_start(adev->pdev, 0); > - adev->gmc.aper_size = pci_resource_len(adev->pdev, 0); > + adev->gmc.aper_base = pci_resource_start(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > + adev->gmc.aper_size = pci_resource_len(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > > #ifdef CONFIG_X86_64 > if (((adev->flags & AMD_IS_APU) && !amdgpu_passthrough(adev)) || > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c > b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c > index af6944d2d330..5bdcc276244c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c > @@ -323,8 +323,10 @@ static int gmc_v6_0_mc_init(struct amdgpu_device *adev) > if (r) > return r; > } > - adev->gmc.aper_base = pci_resource_start(adev->pdev, 0); > - adev->gmc.aper_size = pci_resource_len(adev->pdev, 0); > + adev->gmc.aper_base = pci_resource_start(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > + adev->gmc.aper_size = pci_resource_len(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > adev->gmc.visible_vram_size = adev->gmc.aper_size; > > /* set the gart size */ > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c > b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c > index 93cf283191fa..d8b81d8ec954 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c > @@ -379,8 +379,10 @@ static int gmc_v7_0_mc_init(struct amdgpu_device *adev) > if (r) > return r; > } > - adev->gmc.aper_base = pci_resource_start(adev->pdev, 0); > - adev->gmc.aper_size = pci_resource_len(adev->pdev, 0); > + adev->gmc.aper_base = pci_resource_start(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > + adev->gmc.aper_size = pci_resource_len(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > > #ifdef CONFIG_X86_64 > if ((adev->flags & AMD_IS_APU) && > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c > b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c > index 1d3ddffd5a11..99bfd4e42977 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c > @@ -572,8 +572,10 @@ static int gmc_v8_0_mc_init(struct amdgpu_device *adev) > if (r) > return r; > } > - adev->gmc.aper_base = pci_resource_start(adev->pdev, 0); > - adev->gmc.aper_size = pci_resource_len(adev->pdev, 0); > + adev->gmc.aper_base = pci_resource_start(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > + adev->gmc.aper_size = pci_resource_len(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > > #ifdef CONFIG_X86_64 > if ((adev->flags & AMD_IS_APU) && !amdgpu_passthrough(adev)) { > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > index ced0f3941863..774e93590389 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > @@ -1588,7 +1588,8 @@ static int gmc_v9_0_early_init(struct amdgpu_ip_block > *ip_block) > * mode. > */ > adev->gmc.is_app_apu = (pkg_type == AMDGPU_PKG_TYPE_APU && > - !pci_resource_len(adev->pdev, 0)); > + !pci_resource_len(adev->pdev, > + > amdgpu_get_bar_idx(adev, AMDGPU_PCIE_BAR_VRAM))); > } > > gmc_v9_0_set_gmc_funcs(adev); > @@ -1700,8 +1701,10 @@ static int gmc_v9_0_mc_init(struct amdgpu_device *adev) > if (r) > return r; > } > - adev->gmc.aper_base = pci_resource_start(adev->pdev, 0); > - adev->gmc.aper_size = pci_resource_len(adev->pdev, 0); > + adev->gmc.aper_base = pci_resource_start(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > + adev->gmc.aper_size = pci_resource_len(adev->pdev, > + amdgpu_get_bar_idx(adev, > AMDGPU_PCIE_BAR_VRAM)); > > #ifdef CONFIG_X86_64 > /*
