On 2022-02-11 7:16 p.m., Jonathan Gray wrote:
> On Fri, Feb 11, 2022 at 06:42:11PM -0700, Ted Bullock wrote:
>>
>> There is more to this function that I think is problematic,
>> especially from a MI point of view but I thought I would leave it
>> off here. Below is a diff that resolves all my notes above. I
>> generated the diff with the got tool, so if it doesn't apply I
>> definitely blame stephan :P
>>
>> Some words on how I approached this. Many of the failures tested
>> here need to be followed by radeon_fatal_error = 1 so I moved all
>> the BAR fiddling into it's own function with a single check.
>>
>> I don't know if its really worth checking for IO registers, this
>> has been broken for a few years now and nobody noticed.
>>
>> I also don't know if its worth checking for 64bit FB BARs, I
>> couldn't find any examples of such a radeon devices, but I left
>> the logic in because the original writers thought it was worth
>> handling. In either case the BAR is mapped in 32bit space to keep
>> it MI for 32bit arch.
>>
>> Using pci_mapreg_probe like I have done here seems like a much
>> stronger idiom for device polling, especially for devices that may
>> be shaped all weird like the massive number of radeon parts this
>> matches too. I am aware there is toctou implications with how far
>> pci_mapreg_probe is from when it's used; does this matter on pcie
>> devices?
>>
>> pci_mapreg_probe.9 manpage is still incorrect, I sent a diff out a
>> while back but it didn't manage to catch much attention.
>>
>> I have tested this extensively on sparc64 with xvr-100, I believe
>> it should work for all other arch too that use radeondrm. I don't
>> have a POWER9 or PPC machine though, or a radeon x86 computer so
>> that needs other peoples eyes.
>>
>> This code doesn't exist upstream so we can be fairly abusive to it
>> without making jonathans life any more difficult.
>>
>> I have put many recent hours into reviewing my change here, I hope
>> it's sufficiently not insane.
>
> I'm not so keen on another function and putting bar information into
> local arrays seems odd.
Are there technical reasons for not wanting a function? I know that
pulling in the linux kernel code is probably a big headache, does it
have to do with that?
As to the arrays, I chose to use a idiom for searching the BARs
deliberately that (to me) reads as dramatically simpler and clearer. The
BAR code already in tree for this driver is a mess with inline global
defines sometimes, direct hex codes other times and assigned variables
yet other times.
> It would be easier to review changes if you would stick to either
> changing or moving code, not both at the same time.
Yeah, moving/changing does make the review harder sorry about that :(
> For example when iterating over bars you lost the part that skips one
> if a bar is 64-bit.
No. This was deliberate and not lost, I looked at this very carefully.
bar[i] && PCI_MAPREG_TYPE(type[i]) == PCI_MAPREG_TYPE_IO
This should not match to the top half of a 64bit memory mapped
BAR, so having a special code to modify the loop to handle such a
condition is just adding needless complexity.
For a memory BAR, bit 0 is always 0.
For an IO BAR, bit 0 is 1.
Since this loop is explicitly searching for an IO bar, we should not
need to handle MM semantics when searching, the simple match for the
first bit should be enough. Is this incorrect?
> Generally bool is avoided in kernel code, it was added to not have
> to change drm code.
No problem, the function is probably more correctly written with integer
returns accurately describing problems from errno.h
> We use 'if (' not 'if(' as well.
Oops; I missed this despite staring at the code for a week.
Thanks for taking the time to review :) Much appreciated, tentatively I
left the function in but with the changed return type and adjusted to
your other notes, if it is a problem this can of course be amended.
diff b5c3be43fcdaf7cbd7d070c07746b451413f6b4a
ec22ab3eae34402e7c0b5269b98b531810553aa2
blob - a2d53f752bccb1ab54993ec2a7d5791ec2216e0a
blob + b367e6b0a0a6dce01389033fbd62ba2aa14455b4
--- sys/dev/pci/drm/radeon/radeon_kms.c
+++ sys/dev/pci/drm/radeon/radeon_kms.c
@@ -76,6 +76,9 @@ int radeondrm_activate_kms(struct device *, int);
void radeondrm_attachhook(struct device *);
int radeondrm_forcedetach(struct radeon_device *);
+/* KMS attach setup */
+int radeondrm_conf_bar(struct radeon_device *, struct pci_attach_args *);
+
bool radeon_msi_ok(struct radeon_device *);
irqreturn_t radeon_driver_irq_handler_kms(void *);
@@ -486,6 +489,125 @@ out:
}
#endif
+/* Configure and map radeon registers */
+int
+radeondrm_conf_bar(struct radeon_device *rdev, struct pci_attach_args *pa)
+{
+ uint8_t i, mm;
+ pcireg_t type[6];
+ int bar[6];
+ const uint8_t BAR[6] = {0x10, 0x14, 0x18, 0x1C, 0x20, 0x24};
+ int error;
+
+ if (rdev == NULL || pa == NULL)
+ panic("%s parameters cannot be NULL", __func__);
+
+ 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]);
+
+ /* Framebuffer offset is saved at BAR0 */
+ if (bar[0] && PCI_MAPREG_TYPE(type[0]) == PCI_MAPREG_TYPE_MEM) {
+ 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 retrieve FB parameters from BAR0.\n");
+ return error;
+ }
+
+ 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);
+
+ 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.\n");
+ return error;
+ }
+
+ /* Set FB aperature to 32bit space for MI purposes */
+ switch (PCI_MAPREG_MEM_TYPE(type[0])) {
+ default:
+ printf(": Unhandled BAR0 memory type.\n");
+ return (ENOTSUP);
+ 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;
+ }
+ } else {
+ printf(": BAR0 (framebuffer) is not memory mapped.\n");
+ return (ENODEV);
+ }
+
+ /* 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 at BAR%u. ", i);
+#endif
+ break;
+ }
+ }
+
+ /* 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) {
+ mm = 5;
+ if (bar[2] && PCI_MAPREG_TYPE(type[2]) == PCI_MAPREG_TYPE_MEM) {
+ 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 space.\n");
+ return error;
+ }
+ rdev->doorbell.ptr =
+ bus_space_vaddr(rdev->memt, rdev->doorbell.bsh);
+ } else {
+ printf(": Unable to memory map BAR2 Doorbell.\n");
+ return (ENODEV);
+ }
+ }
+
+ if (bar[mm] && PCI_MAPREG_TYPE(type[mm]) == PCI_MAPREG_TYPE_MEM) {
+ 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 space\n");
+ return error;
+ }
+ } else {
+ printf(": BAR%d (MMIO) is not memory mapped\n", mm);
+ return (ENODEV);
+ }
+
+ rdev->rmmio = bus_space_vaddr(rdev->memt, rdev->rmmio_bsh);
+ return 0;
+}
+
void
radeondrm_attach_kms(struct device *parent, struct device *self, void *aux)
{
@@ -494,14 +616,9 @@ 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
#if defined(__sparc64__) || defined(__macppc__)
extern int fbnode;
@@ -542,75 +659,11 @@ radeondrm_attach_kms(struct device *parent, struct dev
#endif
#endif
-#define RADEON_PCI_MEM 0x10
-
- 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");
+ if (radeondrm_conf_bar(rdev, pa)) {
+ radeon_fatal_error = 1;
return;
}
-#if !defined(__sparc64__)
- if (rdev->fb_aper_offset == 0) {
- bus_size_t start, end;
- bus_addr_t base;
- 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");
- 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);
- 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,
- &rdev->rio_mem, NULL, &rdev->rio_mem_size, 0);
- break;
- }
- if (type == PCI_MAPREG_MEM_TYPE_64BIT)
- i += 4;
- }
-
- 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");
- 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");
- return;
- }
- rdev->rmmio = bus_space_vaddr(rdev->memt, rdev->rmmio_bsh);
-
-#if !defined(__sparc64__)
/*
* Make sure we have a base address for the ROM such that we
* can map it later.
@@ -633,7 +686,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 +717,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]>
