On Sun, Feb 13, 2022 at 12:22:38PM -0700, Ted Bullock wrote:
> On 2022-02-12 6:46 p.m., Jonathan Gray wrote:
> > I will review further when you drop the function.
>
> Alright try this again,
I have committed some parts of this, with one commit per specific issue.
pa_memex NULL test
sparc64 ifndef
drm_attach_pci return test
the result of pci_mapreg_type() is already fine as it does
_PCI_MAPREG_TYPEBITS() which which masks the bits
I still find this diff hard to follow as you are moving code around.
The arrays of bar information can be dropped.
>
> diff b5c3be43fcdaf7cbd7d070c07746b451413f6b4a
> 453e49da7f0804392d6d51fb578cfd8255f4fb77
> blob - a2d53f752bccb1ab54993ec2a7d5791ec2216e0a
> blob + f6353eb5f76e0ae0e21277f82e86b70b36dd401d
> --- sys/dev/pci/drm/radeon/radeon_kms.c
> +++ sys/dev/pci/drm/radeon/radeon_kms.c
> @@ -494,14 +494,13 @@ radeondrm_attach_kms(struct device *parent, struct dev
> struct pci_attach_args *pa = aux;
> const struct pci_device_id *id_entry;
> int is_agp;
> - pcireg_t type;
> - int i;
> - uint8_t rmmio_bar;
> paddr_t fb_aper;
> -#if !defined(__sparc64__)
> pcireg_t addr, mask;
> - int s;
> -#endif
> + int s, error;
> + uint8_t i, mm;
> + pcireg_t type[6];
> + int bar[6];
> + const uint8_t BAR[6] = {0x10,0x14,0x18,0x1C,0x20,0x24};
>
> #if defined(__sparc64__) || defined(__macppc__)
> extern int fbnode;
> @@ -542,75 +541,115 @@ radeondrm_attach_kms(struct device *parent, struct dev
> #endif
> #endif
>
> -#define RADEON_PCI_MEM 0x10
> + /* Start PCI BAR mappings */
> + bar[0] = pci_mapreg_probe(rdev->pc, rdev->pa_tag, BAR[0], &type[0]);
> + bar[1] = pci_mapreg_probe(rdev->pc, rdev->pa_tag, BAR[1], &type[1]);
> + bar[2] = pci_mapreg_probe(rdev->pc, rdev->pa_tag, BAR[2], &type[2]);
> + bar[3] = pci_mapreg_probe(rdev->pc, rdev->pa_tag, BAR[3], &type[3]);
> + bar[4] = pci_mapreg_probe(rdev->pc, rdev->pa_tag, BAR[4], &type[4]);
> + bar[5] = pci_mapreg_probe(rdev->pc, rdev->pa_tag, BAR[5], &type[5]);
>
> - type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, RADEON_PCI_MEM);
> - if (PCI_MAPREG_TYPE(type) != PCI_MAPREG_TYPE_MEM ||
> - pci_mapreg_info(pa->pa_pc, pa->pa_tag, RADEON_PCI_MEM,
> - type, &rdev->fb_aper_offset, &rdev->fb_aper_size, NULL)) {
> - printf(": can't get frambuffer info\n");
> + /* Framebuffer offset is saved at BAR0 */
> + if (!bar[0] || PCI_MAPREG_TYPE(type[0]) != PCI_MAPREG_TYPE_MEM) {
> + printf(": BAR0 (framebuffer) is not memory mapped.\n");
> + radeon_fatal_error = 1;
> return;
> }
> -#if !defined(__sparc64__)
> +
> + error = pci_mapreg_info(rdev->pc, rdev->pa_tag, BAR[0],
> + type[0], &rdev->fb_aper_offset, &rdev->fb_aper_size, NULL);
> + if (error) {
> + printf(": Cannot get FB parameters from BAR0 (%d).\n", error);
> + radeon_fatal_error = 1;
> + return;
> + }
> +
> if (rdev->fb_aper_offset == 0) {
> bus_size_t start, end;
> bus_addr_t base;
>
> + KASSERT(pa->pa_memex != NULL);
> +
> start = max(PCI_MEM_START, pa->pa_memex->ex_start);
> end = min(PCI_MEM_END, pa->pa_memex->ex_end);
> - if (pa->pa_memex == NULL ||
> - extent_alloc_subregion(pa->pa_memex, start, end,
> - rdev->fb_aper_size, rdev->fb_aper_size, 0, 0, 0, &base)) {
> - printf(": can't reserve framebuffer space\n");
> +
> + error = extent_alloc_subregion(pa->pa_memex, start, end,
> + rdev->fb_aper_size, rdev->fb_aper_size, 0, 0, 0, &base);
> + if (error) {
> + printf(": Cannot allocate framebuffer (%d).\n", error);
> + radeon_fatal_error = 1;
> return;
> }
> - pci_conf_write(pa->pa_pc, pa->pa_tag, RADEON_PCI_MEM, base);
> - if (PCI_MAPREG_MEM_TYPE(type) == PCI_MAPREG_MEM_TYPE_64BIT)
> - pci_conf_write(pa->pa_pc, pa->pa_tag,
> - RADEON_PCI_MEM + 4, (uint64_t)base >> 32);
> +
> + /* Set FB aperature to 32bit space for MI purposes */
> + switch (PCI_MAPREG_MEM_TYPE(type[0])) {
> + default:
> + printf(": Unhandled BAR0 memory type.\n");
> + radeon_fatal_error = 1;
> + return;
> + case PCI_MAPREG_MEM_TYPE_64BIT:
> + pci_conf_write(pa->pa_pc, pa->pa_tag, BAR[1], 0);
> + /* FALLTHROUGH */
> + case PCI_MAPREG_MEM_TYPE_32BIT:
> + pci_conf_write(pa->pa_pc, pa->pa_tag, BAR[0], base);
> + }
> rdev->fb_aper_offset = base;
> }
> -#endif
>
> - for (i = PCI_MAPREG_START; i < PCI_MAPREG_END; i += 4) {
> - type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, i);
> - if (type == PCI_MAPREG_TYPE_IO) {
> - pci_mapreg_map(pa, i, type, 0, NULL,
> + /* Search BARs for IO registers (if supported/available) usually BAR1 */
> + for (i = 0; i < 6; i++) {
> + if (bar[i] && PCI_MAPREG_TYPE(type[i]) == PCI_MAPREG_TYPE_IO) {
> + error = pci_mapreg_map(pa, BAR[i], type[i], 0, NULL,
> &rdev->rio_mem, NULL, &rdev->rio_mem_size, 0);
> + /* Non-fatal failure, for alternative access use mmio */
> +#ifdef DEBUG
> + if (error)
> + printf(": IO map unavailable (%d). ", error);
> +#endif
> break;
> }
> - if (type == PCI_MAPREG_MEM_TYPE_64BIT)
> - i += 4;
> }
>
> + /* Radeons older than Bonaire have MMIO BAR here */
> + mm = 2;
> +
> + /* New ICs add a doorbell and moved the MMIO BAR register to BAR5 */
> if (rdev->family >= CHIP_BONAIRE) {
> - type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, 0x18);
> - if (PCI_MAPREG_TYPE(type) != PCI_MAPREG_TYPE_MEM ||
> - pci_mapreg_map(pa, 0x18, type, BUS_SPACE_MAP_LINEAR, NULL,
> - &rdev->doorbell.bsh, &rdev->doorbell.base,
> - &rdev->doorbell.size, 0)) {
> - printf(": can't map doorbell space\n");
> + mm = 5;
> + if (!bar[2] || PCI_MAPREG_TYPE(type[2]) != PCI_MAPREG_TYPE_MEM)
> {
> + printf(": Unable to memory map BAR2 Doorbell.\n");
> + radeon_fatal_error = 1;
> return;
> }
> - rdev->doorbell.ptr = bus_space_vaddr(rdev->memt,
> - rdev->doorbell.bsh);
> +
> + error = pci_mapreg_map(pa, BAR[2], type[2],
> + BUS_SPACE_MAP_LINEAR, NULL, &rdev->doorbell.bsh,
> + &rdev->doorbell.base, &rdev->doorbell.size, 0);
> + if (error) {
> + printf(": Cannot map doorbell at BAR2 (%d).\n", error);
> + radeon_fatal_error = 1;
> + return;
> + }
> + rdev->doorbell.ptr =
> + bus_space_vaddr(rdev->memt, rdev->doorbell.bsh);
> }
>
> - if (rdev->family >= CHIP_BONAIRE)
> - rmmio_bar = 0x24;
> - else
> - rmmio_bar = 0x18;
> -
> - type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, rmmio_bar);
> - if (PCI_MAPREG_TYPE(type) != PCI_MAPREG_TYPE_MEM ||
> - pci_mapreg_map(pa, rmmio_bar, type, BUS_SPACE_MAP_LINEAR, NULL,
> - &rdev->rmmio_bsh, &rdev->rmmio_base, &rdev->rmmio_size, 0)) {
> - printf(": can't map rmmio space\n");
> + if (!bar[mm] || PCI_MAPREG_TYPE(type[mm]) != PCI_MAPREG_TYPE_MEM) {
> + printf(": BAR%d (MMIO) is not memory mapped\n", mm);
> + radeon_fatal_error = 1;
> return;
> }
> + error = pci_mapreg_map(pa, BAR[mm], type[mm], BUS_SPACE_MAP_LINEAR,
> + NULL, &rdev->rmmio_bsh, &rdev->rmmio_base, &rdev->rmmio_size, 0);
> + if (error) {
> + printf(": unable to map MMIO registers (%d)\n", error);
> + radeon_fatal_error = 1;
> + return;
> + }
> +
> rdev->rmmio = bus_space_vaddr(rdev->memt, rdev->rmmio_bsh);
> + /* Finished PCI BAR mapping */
>
> -#if !defined(__sparc64__)
> /*
> * Make sure we have a base address for the ROM such that we
> * can map it later.
> @@ -633,7 +672,6 @@ radeondrm_attach_kms(struct device *parent, struct dev
> size, 0, 0, 0, &base) == 0)
> pci_conf_write(pa->pa_pc, pa->pa_tag, PCI_ROM_REG,
> base);
> }
> -#endif
>
> #ifdef notyet
> mtx_init(&rdev->swi_lock, IPL_TTY);
> @@ -665,6 +703,11 @@ radeondrm_attach_kms(struct device *parent, struct dev
>
> dev = drm_attach_pci(&kms_driver, pa, is_agp, rdev->primary,
> self, NULL);
> + if (dev == NULL) {
> + printf("%s: drm_attach_pci failed\n", rdev->self.dv_xname);
> + radeon_fatal_error = 1;
> + return;
> + }
> rdev->ddev = dev;
> rdev->pdev = dev->pdev;
>
>
>
> --
> Ted Bullock <[email protected]>
>
