clbr / radeontop

GNU General Public License v3.0
810 stars 72 forks source link

Fix bus ID with gpu autodetection #70

Closed grische closed 5 years ago

grische commented 6 years ago

Follow-up of #64

When no bus is specified via -b xx, the bus shows as 00 in both the dump as well as the UI as it is never being set properly.

This PR basically returns (i.e. copies) the whole pci_device from init_pci() to make it fully accessible in main(). This allows to print and forward even more information from the PCI device in the future.

Any opinions?

clbr commented 6 years ago

The struct no longer exists when you return to main, as pciaccess has been cleaned then. How about replacing these two commits with a much simpler approach, making the bus parameter a pointer?

grische commented 6 years ago

I thought about this, but if we were to make bus a pointer, we should consider also adding a parameter unsigned int* device_id as I don't see a reason why one is more important than the other. As a result one could return an error code. Similar to:

enum pci_error_code init_pci(unsigned char* bus, unsigned int* device id, const unsigned char forcemem);

At the same time, returning the full pci_device gives flexibility to read out more data like dev->dev or dev->func for the UI or the dump ( considering 713ffa6a16339d084e27ec057348b967216d9520 ).

I don't mind changing init_pci to the above if you prefer it that way.

clbr commented 6 years ago

It's not useful to return errors, since we can't continue anyway. There is no current need to read anything from the pci_device struct, and we don't want to read freed memory. If there is a future need to read something else, it should be returned in a separate parameter.

grische commented 6 years ago

Fair enough, so as far as I understood you want something like

void init_pci(unsigned char* bus, unsigned int* device id, const unsigned char forcemem);
clbr commented 6 years ago

Yes, that'd be good.