Re: Multiple issues with radeondrm startup code
On 2022-02-14 12:50 a.m., Jonathan Gray wrote:
On Mon, Feb 14, 2022 at 12:05:44AM -0700, Ted Bullock wrote:
On 2022-02-13 11:02 p.m., Jonathan Gray wrote:
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.
When you cherrypicked fixes, you missed the for loop as per the initial
mail, the type checks are incorrect and won't match. You need the helper
macros since those types are bitmaps not types. This has been like this
since 1.71 (Oct 2020)
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;
}
type = _PCI_MAPREG_TYPEBITS(pci_conf_read(pc, tag, reg));
if (type == 1) {
pci_mapreg_map();
break;
}
if (type == 4)
i += 4;
---
x = pci_conf_read(pc, tag, reg);
if ((x & 1) == 1)
type = x & 1;
else
type = x & 7;
if (type == 1) {
pci_mapreg_map();
break;
}
if (type == 4)
i += 4;
which other bits do you expect?
001 io space
000 32-bit mem space
100 64-bit mem space
See below quote
if (type == PCI_MAPREG_TYPE_IO) {
This is a bitmap, you cannot use it like this, the correct usage
would be PCI_MAPREG_TYPE(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)
Same as above, this is incorrect usage
i += 4;
A note about this for loop. This loop was broken in revision 1.72 to
support radeon devices on POWER9 systems. These registers are available
(I believe) through the MMIO BAR so people haven't noticed that it's
broken. I don't know if ALL devices work like that, though certainly
most do, if so this entire loop could possibly be removed and just rely
on MMIO access to these registers.
From my initial mail on the issues I saw. Is this incorrect and isn't
PCI_MAPREG_TYPE needed for checking type? I think so, but maybe I'm wrong.
--
Ted Bullock
Re: Multiple issues with radeondrm startup code
On Mon, Feb 14, 2022 at 12:05:44AM -0700, Ted Bullock wrote:
>
> On 2022-02-13 11:02 p.m., Jonathan Gray wrote:
> > 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.
> When you cherrypicked fixes, you missed the for loop as per the initial
> mail, the type checks are incorrect and won't match. You need the helper
> macros since those types are bitmaps not types. This has been like this
> since 1.71 (Oct 2020)
>
> 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;
> }
type = _PCI_MAPREG_TYPEBITS(pci_conf_read(pc, tag, reg));
if (type == 1) {
pci_mapreg_map();
break;
}
if (type == 4)
i += 4;
---
x = pci_conf_read(pc, tag, reg);
if ((x & 1) == 1)
type = x & 1;
else
type = x & 7;
if (type == 1) {
pci_mapreg_map();
break;
}
if (type == 4)
i += 4;
which other bits do you expect?
001 io space
000 32-bit mem space
100 64-bit mem space
Re: Multiple issues with radeondrm startup code
On 2022-02-13 11:02 p.m., Jonathan Gray wrote:
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.
When you cherrypicked fixes, you missed the for loop as per the initial
mail, the type checks are incorrect and won't match. You need the helper
macros since those types are bitmaps not types. This has been like this
since 1.71 (Oct 2020)
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;
}
--
Ted Bullock
Re: Multiple issues with radeondrm startup code
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_tBAR[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_wr
Re: Multiple issues with radeondrm startup code
On 2022-02-12 6:46 p.m., Jonathan Gray wrote:
> I will review further when you drop the function.
Alright try this again,
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_tBAR[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++) {
+
Re: Multiple issues with radeondrm startup code
On Sat, Feb 12, 2022 at 02:42:16PM -0700, Ted Bullock wrote:
> On 2022-02-11 7:16 p.m., Jonathan Gray wrote:
> > 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?
Look at other pci drivers and you will see they deal with
bars in attach.
>
> 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.
Drivers normally look at one bar and have a define for the offset
used in attach. Iterating over bars and generation specific tests
aren't the norm. The linux interfaces all want to deal with a bar
number instead of of offset. For radeon linux does it in
radeon_device_init().
>
> > 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.
I will review further when you drop the function.
Re: Multiple issues with radeondrm startup code
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 *);
intradeondrm_forcedetach(struct radeon_device *);
+/* KMS attach setup */
+intradeondrm_conf_bar(struct radeon_device *, struct pci_attach_args *);
+
bool radeon_msi_ok(struct radeon_device *);
irqreturn_tradeon_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_tBAR[6] = {0x10, 0x14, 0x18, 0x1C, 0x20, 0x24};
+ int error;
+
+
Re: Multiple issues with radeondrm startup code
On Fri, Feb 11, 2022 at 06:42:11PM -0700, Ted Bullock wrote:
> Hey,
>
> I've found some problems with the radeondrm initialization
> codepath (radeon_kms.c). Before I start, I should mention that I am
> working on some diffs to remove a bunch of the sparc64 MD ifdef's as
> well. In radeondrm_attach_kms:
>
> starting from line 545
> ==
> #define RADEON_PCI_MEM0x10
> ^^ inline defines for bar registers reads as weird/bad to me
>
> 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");
> return;
> ^^
> Should set radeon_fatal_error = 1 before returning
> }
> #if !defined(__sparc64__)
> ^^ I've tested this on sparc64 xvr-100, this ifdef isn't needed
> 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);
> Potential NULL dereference ->
> end = min(PCI_MEM_END, pa->pa_memex->ex_end);
> And here --->^^
> if (pa->pa_memex == NULL ||
>
> Obviously just move this NULL check up two lines
> 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;
> ^^
> Should set radeon_fatal_error = 1 before returning
> }
> 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);
> This evaluates to zero on all architectures --->
> rdev->fb_aper_offset = base;
> }
> #endif
>
> for (i = PCI_MAPREG_START; i < PCI_MAPREG_END; i += 4) {
> ^^
> This is an awkward (bad) idiom to iterate through BARs
> type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, i);
> if (type == PCI_MAPREG_TYPE_IO) {
>
> This is a bitmap, you cannot use it like this, the correct usage
> would be PCI_MAPREG_TYPE(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)
>
> Same as above, this is incorrect usage
> i += 4;
> }
>
> A note about this for loop. This loop was broken in revision 1.72 to
> support radeon devices on POWER9 systems. These registers are available
> (I believe) through the MMIO BAR so people haven't noticed that it's
> broken. I don't know if ALL devices work like that, though certainly
> most do, if so this entire loop could possibly be removed and just rely
> on MMIO access to these registers.
>
> if (rdev->family >= CHIP_BONAIRE) {
> type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, 0x18);
>-->
> If putting an inline #define was appropriate earlier why not here
> if (PCI_MAPREG_TYPE(type) != PCI_MAPREG_TYPE_MEM ||
> pci_mapreg_map(pa, 0x18, type, BUS_SPACE_MAP_LINEAR, NULL,
> And here >
> &rdev->doorbell.bsh, &rdev->doorbell.base,
> &rdev->doorbell.size, 0)) {
> printf(": can't map doorbell space\n");
> return;
> ^^
> Should set radeon_fatal_error = 1 before returning
> }
> 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;
> ^^
> Should set radeon_fatal_error = 1 before returning
> }
> rdev->rmmio = bus_space_vaddr(rdev->memt, rdev->rmmio_bsh);
Multiple issues with radeondrm startup code
Hey,
I've found some problems with the radeondrm initialization
codepath (radeon_kms.c). Before I start, I should mention that I am
working on some diffs to remove a bunch of the sparc64 MD ifdef's as
well. In radeondrm_attach_kms:
starting from line 545
==
#define RADEON_PCI_MEM 0x10
^^ inline defines for bar registers reads as weird/bad to me
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");
return;
^^
Should set radeon_fatal_error = 1 before returning
}
#if !defined(__sparc64__)
^^ I've tested this on sparc64 xvr-100, this ifdef isn't needed
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);
Potential NULL dereference ->
end = min(PCI_MEM_END, pa->pa_memex->ex_end);
And here --->^^
if (pa->pa_memex == NULL ||
Obviously just move this NULL check up two lines
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;
^^
Should set radeon_fatal_error = 1 before returning
}
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);
This evaluates to zero on all architectures --->
rdev->fb_aper_offset = base;
}
#endif
for (i = PCI_MAPREG_START; i < PCI_MAPREG_END; i += 4) {
^^
This is an awkward (bad) idiom to iterate through BARs
type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, i);
if (type == PCI_MAPREG_TYPE_IO) {
This is a bitmap, you cannot use it like this, the correct usage
would be PCI_MAPREG_TYPE(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)
Same as above, this is incorrect usage
i += 4;
}
A note about this for loop. This loop was broken in revision 1.72 to
support radeon devices on POWER9 systems. These registers are available
(I believe) through the MMIO BAR so people haven't noticed that it's
broken. I don't know if ALL devices work like that, though certainly
most do, if so this entire loop could possibly be removed and just rely
on MMIO access to these registers.
if (rdev->family >= CHIP_BONAIRE) {
type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, 0x18);
-->
If putting an inline #define was appropriate earlier why not here
if (PCI_MAPREG_TYPE(type) != PCI_MAPREG_TYPE_MEM ||
pci_mapreg_map(pa, 0x18, type, BUS_SPACE_MAP_LINEAR, NULL,
And here >
&rdev->doorbell.bsh, &rdev->doorbell.base,
&rdev->doorbell.size, 0)) {
printf(": can't map doorbell space\n");
return;
^^
Should set radeon_fatal_error = 1 before returning
}
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;
^^
Should set radeon_fatal_error = 1 before returning
}
rdev->rmmio = bus_space_vaddr(rdev->memt, rdev->rmmio_bsh);
#if !defined(__sparc64__)
^
I've tested without this MD ifdef on sparc64, there is no adverse
behavior when using xvr-100 that I have noti
