BUGFIX: Enable QPU when accessing Register Map #52
Conversation
Accessing the register map won't work in the VideoCore is powered up. This adds code to `detectPlatform` to ensure that VideoCore is enabled before reading the registers. In addition: - Adjusted some code for better debugging. Notably, a call to `perror` has been added in `MailBox.cpp`. - Left in debug printf's where useful. There are commented out in general. - Added RegisterMap::enabled(), which checks if registers are accessible. The latter is actually a useful method to determine if the VideoCore is running.
|
Hi @wimrijnders, I my recent work I also had to disable compilation of Of course I'm on an old version, but it would be nice if running |
|
I agree it should work on old version, but nevertheless: crap. I'll see whatever I can find on the I wish I could test this old stuff somehow. I should have an old Raspbian SD-card somewhere.... |
|
In file #if defined(__unix__) && !defined(__ANDROID__)
#include "interface/vcos/pthreads/vcos_platform_types.h"
#else
#include "vcos_platform_types.h"
#endifThe first case is used for my system. I'm thinking that in your case, the first should be selected as well but isn't. So I think the best way is to force the #define's upon use. Found an old Raspbian SD-card BTW. Now to see how to use it, if it's usable at all.... |
|
Checked all places (3) in include where |
|
@mn416 Added a fix for the usage of In addition, a fix in
|
Well, crap again. This happens on Pi 2 but not on Pi 1. All is well if you run with I can track it down to group permissions: In other words, the user has to be a member of group It's frightening how things escalate...., doing my best to keep it in check. |
|
This actually implies a way to avoid Logout, login and bob's your uncle. No more nag messages about |
|
@mn416 I take previous back about joining specific groups. It works for |
Now that DMA is enabled, it's interesting to know what the actual size is of the VPM. This adds it to the output of `detectPlatform`. Example output: ``` > sudo obj-qpu/bin/detectPlatform Detected platform: Raspberry Pi 2 Model B Rev 1.1 Hardware revision: a01041 Number of slices: 3 Number of QPU's per slice: 4 Size of VPM: 12KB # <-- This is new ``` **NOTE:* This will probably not work on your machine until mn416#52 and mn416#53 have been merged.
|
Hi @wimrijnders, Can you explain the following? Just wondering, what is the purpose if |
|
See this comment. This is what appears in the location of the compile error. The if-part of the include is the one that's required. It gets picked up OK on my Pi's but not on yours; so, one of the define's must be wrong in your case. This is my assumption: Does this make sense? Note that it gets cleaned up afterward again. Edit: The question implies that I should put a comment there to explain it. It's not complicated, but the reason it's there is quite obscure. |
|
OH CRAP! I see what you mean. It should be |
|
😊 There, fixed. |
|
Have you considered using the review features in the 'Files changed' BTW? Perhaps you're not aware that if you hover on a '+' before a code line, you get a clickable '+' icon which opens a comment block specific for that line. If you are aware of this, forgive my well-intended pedantry. |
|
Hi @wimrijnders, I gave this a try but I get the same error message as above. Here is my "vcos" directory: I see that |
|
This is the same as my Pi's, in other words, as expected. So, the assumption that in: #if defined(__unix__) && !defined(__ANDROID__)
#include "interface/vcos/pthreads/vcos_platform_types.h"
#else
#include "vcos_platform_types.h"
#endif... Will fix the #ifdef bit to handle |
|
Aargh! I'm an idiot. I was #undef-ing Actually, I'm going to force both. |
|
OK done. Also a minor dependency fix in the Makefile. Try again? Tell me if you see these while compiling: and/or I can remove the |
|
Yes, was building from a clean state. If I set OLD_PI=yes then I get |
|
Huh? that's a strange error. The function doesn't declare parameters because there aren't any. Compiles perfectly in my case. |
|
It's not even a function: volatile uint32_t *m_addr{nullptr};And there's no way the OLD_PI setting can influence this, since it's only used in the cpp, not the include file. The only thing I can think of is that the c++ version setting EDIT: The initialization with |
|
Replaced inline init with constructor initialization list. This is to determine what the problem is. I still would prefer to use |
|
Still seeing compile errors: |
|
Ah right, same problem, different variable. Hold on. This is a compiler issue I believe; it can't handle the full |
|
'Fixed' in last commit, please test. I'm running unit tests in the meantime. EDIT: Unit tests passed. |
|
Looks like you're just missing |
|
Thanks for reporting. I've just been racking my brains to figure out why it compiles fine without those on my machines. I can't find a reason and will leave it at that. Just one of those things. Did you manage to run |
|
Also, important, I want to know if you also compile on a Pi 1. Because you mentioned somewhere that you switched to Pi 2 for faster compilation. This is important for the bcm peripheral address, which is different on the Pi 1. |
Works on Pi 2 |
|
Builds and runs on Pi 1, but takes a few seconds to complete and gives unexpected output: |
|
Yes, because the peripheral address is different like I mentioned. But thanks for checking anyway. |
|
Great that you mentioned the hardware revision as well. Useful. |
|
Last commit adds explicit detection of Pi model using hardware revision for determining the peripheral address. This should now also work on your Pi 1. In addition I added some descriptive commenting, also in the Makefile. |
|
@mn416 Would you mind testing the following?
Do these work as expected now on your Pi's? |
|
Need to include The device file exists. |
|
c++11 not available on my Pi 1. |
|
Doesn't build on Pi 1 if I use the following
|
|
OK. Silly thing is I removed it previous commit.
OK, thanks for checking. So the baseline is
Thanks for checking. OK, I'll leave |
|
These differences between platforms and compilers make a case for using makefile generation tools such as Not anything I would implement right now, but it's a thing to think about. |
|
Actually, you don't need to generate the whole makefile, just a file that can be included with platform-specific settings, which could be generated with e.g. Cutting this line of thought now, have other things to do. |
|
@mn416 Will be leaving for 20 days vacation tomorrow. I would love to see this PR get merged before leaving, is it possible? Otherwise, no harm done. If this gets accepted, it opens up a whole slew of functionality. Looking forward to that (for when I get back). |
|
Only outstanding issue is that As I said before, the device file exists. Is this behaviour expected? |
|
No, not expected, this should just work. Will look at it when I return from vacation. |
Accessing the register map won't work if the VideoCore is not powered up. This PR adds code to
detectPlatform()to ensure that VideoCore is enabled before reading the registers.In addition:
perrorhas beenadded in
MailBox.cpp.RegisterMap::enabled(), which checks if the VideoCore registers are accessible.The latter is actually a useful method to determine if the VideoCore is running.