search

mn416 / QPULib

Language and compiler for the Raspberry Pi GPU
Other
429 stars 64 forks source link

BUGFIX: Enable QPU when accessing Register Map #52

Open wimrijnders opened 6 years ago

wimrijnders commented 6 years ago

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:

The latter is actually a useful method to determine if the VideoCore is running.

mn416 commented 6 years ago

Hi @wimrijnders,

I my recent work I also had to disable compilation of RegisterMap.cpp due to the following error.

Compiling Lib/VideoCore/RegisterMap.cpp
In file included from /opt/vc/include/interface/vcos/vcos_assert.h:149:0,
                 from /opt/vc/include/interface/vcos/vcos.h:114,
                 from /opt/vc/include/interface/vmcs_host/vc_dispmanx.h:33,
                 from /opt/vc/include/bcm_host.h:46,
                 from Lib/VideoCore/RegisterMap.cpp:8:
/opt/vc/include/interface/vcos/vcos_types.h:38:33: fatal error: vcos_platform_types.h: No such file or directory
compilation terminated.
make: *** [obj-qpu/VideoCore/RegisterMap.o] Error 1

Of course I'm on an old version, but it would be nice if running make would still work on old platforms.

wimrijnders commented 6 years ago

I agree it should work on old version, but nevertheless: crap.

I'll see whatever I can find on the vcos headers of old. Would you mind for now to send me a listing of /opt/vc/include/interface/vcos/?

I wish I could test this old stuff somehow. I should have an old Raspbian SD-card somewhere....

wimrijnders commented 6 years ago

In file /opt/vc/include/interface/vcos/vcos_types.h:

#if defined(__unix__) && !defined(__ANDROID__)
#include "interface/vcos/pthreads/vcos_platform_types.h"
#else
#include "vcos_platform_types.h"
#endif

The 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.... EDIT: It's a 'big' SD, the Pi 2 and 3 take only micro's. The Pi 1 is my backup server, hesitate to take it offline. Perhaps I can copy to micro....

wimrijnders commented 6 years ago

Checked all places (3) in include where __unix__ is used within /opt/vc/include, I think we're safe. If anything, setting it will prevent more compilation errors.

wimrijnders commented 6 years ago

@mn416 Added a fix for the usage of __linux__. See if this works for you.

In addition, a fix in detectPlatform for some issues I noticed:

wimrijnders commented 6 years ago 
> obj-qpu/bin/detectPlatform 
Detected platform: Raspberry Pi 2 Model B Rev 1.1
Can't open device file: /dev/vcio
Try creating a device file with: sudo mknod /dev/vcio c 100 0

Well, crap again. This happens on Pi 2 but not on Pi 1. All is well if you run with sudo.

I can track it down to group permissions:

> ls -al  /dev/vcio
crw-rw---- 1 root video 249, 0 Jul  7 14:17 /dev/vcio

# Pi 1
> groups
wim sudo video

# Pi 2
> groups
wim sudo netdev

In other words, the user has to be a member of groupvideo.

It's frightening how things escalate...., doing my best to keep it in check.

wimrijnders commented 6 years ago

This actually implies a way to avoid sudo; just become a member of the relevant group:

> sudo useradd -g video wim
> sudo useradd -g kmem wim

Logout, login and bob's your uncle. No more nag messages about sudo. I checked it, it works. This is something for a 'Good to know' column in the docs.

wimrijnders commented 6 years ago

@mn416 I take previous back about joining specific groups. It works for /dev/vcio but I can't get it to work for /dev/mem. Best to stick with sudo.

mn416 commented 6 years ago

Hi @wimrijnders,

Can you explain the following?

#if defined(LINUX_PREVIOUSLY_UNDEFINED)
#define __linux__
#undef LINUX_PREVIOUSLY_UNDEFINED
#endif

Just wondering, what is the purpose if #define __linux__ here?

wimrijnders commented 6 years ago

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: __linux__ is not used by the compiler of your old distro. It's either that or the ANDROID define is active in some way. So my 'hack' is to define __linux__ before including the bcm headers, if it wasn't defined already.

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.

wimrijnders commented 6 years ago

OH CRAP! I see what you mean. It should be #undef. facepalm. This is the cleanup part.

wimrijnders commented 6 years ago

:blush: There, fixed.

wimrijnders commented 6 years ago

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.

mn416 commented 6 years ago

Hi @wimrijnders,

I gave this a try but I get the same error message as above.

Here is my "vcos" directory:

> ls /opt/vc/include/interface/vcos/
generic              vcos.h                  vcos_queue.h
pthreads             vcos_init.h             vcos_quickslow_mutex.h
user_nodefs.h        vcos_inttypes.h         vcos_reentrant_mutex.h
vcos_assert.h        vcos_isr.h              vcos_semaphore.h
vcos_atomic_flags.h  vcos_legacy_isr.h       vcos_stdbool.h
vcos_attr.h          vcos_logging_control.h  vcos_stdint.h
vcos_blockpool.h     vcos_logging.h          vcos_string.h
vcos_build_info.h    vcos_lowlevel_thread.h  vcos_thread_attr.h
vcos_cfg.h           vcos_mem.h              vcos_thread.h
vcos_cmd.h           vcos_mempool.h          vcos_timer.h
vcos_ctype.h         vcos_msgqueue.h         vcos_tls.h
vcos_dlfcn.h         vcos_mutex.h            vcos_types.h
vcos_event_flags.h   vcos_named_semaphore.h
vcos_event.h         vcos_once.h

I see that vcos_platform_types is here:

> ls /opt/vc/include/interface/vcos/pthreads/
vcos_futex_mutex.h  vcos_platform.h  vcos_platform_types.h
wimrijnders commented 6 years ago

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

... __unix__ is not defined is wrong. The problem must then be that __ANDROID__ is defined. For some reason gcc thinks that you're compiling for an Android machine.

Will fix the #ifdef bit to handle __ANDROID__ instead. f that doesn't work, I'm out of ideas.

wimrijnders commented 6 years ago

Aargh! I'm an idiot. I was #undef-ing __linux__ when it should be __unix__. Try again.

Actually, I'm going to force both.

wimrijnders commented 6 years ago

OK done. Also a minor dependency fix in the Makefile. Try again?

Tell me if you see these while compiling:

#pragma message "__unix__ is NOT defined"

and/or

#pragma message "__ANDROID__ is defined"

I can remove the #pragma message statements aftwards.

mn416 commented 6 years ago

Still no luck, same error.

Looking in /opt/vc/include/interface/vcos/vcos_types.h I see:

#include <stddef.h>
#include "vcos_platform_types.h"
#include "interface/vcos/vcos_attr.h"

There's no mention of interface/vcos/pthreads/vcos_platform_types.h. Maybe this is a bug in the headers that was fixed in subsequent Raspbians. I guess we have to conditionally compile RegisterMap.cpp or just say that my Raspbian is unsupported, and doing the latter makes me sad since everything else works fine.

mn416 commented 6 years ago

Don't know if this helps:

> cat /etc/os-release
PRETTY_NAME="Raspbian GNU/Linux 7 (wheezy)"
NAME="Raspbian GNU/Linux"
VERSION_ID="7"
VERSION="7 (wheezy)"
ID=raspbian
ID_LIKE=debian
ANSI_COLOR="1;31"
HOME_URL="http://www.raspbian.org/"
SUPPORT_URL="http://www.raspbian.org/RaspbianForums"
BUG_REPORT_URL="http://www.raspbian.org/RaspbianBugs"
wimrijnders commented 6 years ago

Crapcrapcrap.

The only sensible thing to do now is prepare an SD-card with wheezy and test with that. So much work....

The silly thing is, I had Pi's at wheezy but upgraded them to latest upon getting seriously involved with this project. But I wasn't expecting issues like this.

There is a hackish workaround: see this doc on peripheral addresses. All bcm_host_get_peripheral_address() is return a memory address which is know and very unlikely to change (although it might). It's not too much a stretch to supply it ourselves.

If we can identify a particular define during compilation which identifies your system, then we can replace the bcm-call with our implementation. Just a thought.

wimrijnders commented 6 years ago

Looking in /opt/vc/include/interface/vcos/vcos_types.h I see:

include

include "vcos_platform_types.h"

include "interface/vcos/vcos_attr.h"

I managed to load in an old Raspbian sd card. I see it now, too. The file is different, my hacky solution doesn't apply.

The solution will stil be a bit hacky. Let me try something.

wimrijnders commented 6 years ago

This time I copied over the old /opt/vc/include files and compiled with those. I managed to reproduce the compile error you see.

The problem is deeper than just include headers in the wrong place; some functions used in RegisterMap are just plain missing in the old distro. So the solution is to supply local implementations for them. By all means scan the code diff's to understand what is happening.

This should work. I'm more confident now because I wasn't groping in the dark this time. Please see if it works now for you.

wimrijnders commented 6 years ago

BTW I will never ever say that this fix is 'elegant'. But it is required....

mn416 commented 6 years ago

Hi @wimrijnders,

Thanks for pursuing on this issue.

I don't feel the added complexity of all the workarounds is warranted at this stage. Correct me if I'm wrong, but only detectPlatform.cpp uses the features of RegisterMap.cpp. Therefore, can I propose we exclude RegisterMap.cpp when the building of the library, and instead only build it as part of the detectPlatform binary? When the features provided by RegisterMap.cpp are actually essential to the library, then we will make the decision on what to do with outdated include files.

wimrijnders commented 6 years ago

I don't feel the added complexity of all the workarounds is warranted at this stage.

There's only one at the moment, and it's based on solid underground. I took the library files of a Wheezy-distro and compiled against that. The previous attempts have been removed. Please try it first.

Correct me if I'm wrong, but only detectPlatform.cpp uses the features of RegisterMap.cpp.

At this particular commit of development, yet. But it's meant as general functionality, and it's tied directly to the VideoCore, not some particular application. #66 will add RegisterMap stuff to Rot3DLib as well, which will make two apps using it. I'm very sure you'll see some use for it RSN.

When the features provided by RegisterMap.cpp are actually essential to the library, then we will make the decision on what to do with outdated include files.

While they may not be essential to the working of the DSL, they do contain essential stuff related to working with the VideoCore, some of which will become important very soon.

...then we will make the decision on what to do with outdated include files.

The logical way out is to upgrade your distro. But since we're in this loop, I went along with providing backward compatibility.

Please test the last fix first, then we'll see further.

mn416 commented 6 years ago

I've switched over to a Pi 2 for faster builds, but I see the same problem:

In file included from /opt/vc/include/interface/vcos/vcos_assert.h:149:0,
                 from /opt/vc/include/interface/vcos/vcos.h:114,
                 from /opt/vc/include/interface/vmcs_host/vc_dispmanx.h:33,
                 from /opt/vc/include/bcm_host.h:50,
                 from Lib/VideoCore/RegisterMap.cpp:56:
/opt/vc/include/interface/vcos/vcos_types.h:38:33: fatal error: vcos_platform_types.h: No such file or directory
compilation terminated.
Makefile:189: recipe for target 'obj-qpu/VideoCore/RegisterMap.o' failed
make: *** [obj-qpu/VideoCore/RegisterMap.o] Error 1
> grep bcm_host_get_peripheral_address /opt/vc/include/bcm_host.h && echo "no" || echo "yes" unsigned bcm_host_get_peripheral_address(void);
no
wimrijnders commented 6 years ago

Did you try with make clean, make all?

> grep bcm_host_get_peripheral_address /opt/vc/include/bcm_host.h && echo "no" || echo "yes" unsigned bcm_host_get_peripheral_address(void);
no

'No' would mean that you have a 'new' distro. With a make clean first it should compile.

wimrijnders commented 6 years ago

OK, that previous doesn't make sense. It should have found the headers, after all the grep succeeded. Weird.

I've switched over to a Pi 2 for faster builds,

Huh? Weren't you compiling on a Pi 2 before? That's what I understood from previous communication.

wimrijnders commented 6 years ago

Well, there is a way out. Use the 'workaround' exclusively. This means having OLD_PI=yes on always for your build.

It's not like it's complicated. This adds two one-liner replacement methods for the bcm-functions.

wimrijnders commented 6 years ago

Last commit avoids the need to include the bcm headers entirely, by default.

This will work on a Pi 2, please test this.

I will just have to remember to change the Makefile when required, to enable the bcm header detection. This will only be needed on my Pi 1.

Please tell me if you're going to compile on anything other than a Pi 1.

mn416 commented 6 years ago

Yes, was building from a clean state. If I set OLD_PI=yes then I get

In file included from Lib/VideoCore/RegisterMap.cpp:3:0:
Lib/VideoCore/RegisterMap.h:34:21: error: function definition does not declare parameters
Lib/VideoCore/RegisterMap.h:35:11: error: function definition does not declare parameters
Lib/VideoCore/RegisterMap.cpp: In constructor ‘QPULib::RegisterMap::RegisterMap()’:
Lib/VideoCore/RegisterMap.cpp:75:2: error: ‘m_size’ was not declared in this scope
Lib/VideoCore/RegisterMap.cpp:80:2: error: ‘m_addr’ was not declared in this scope
Lib/VideoCore/RegisterMap.cpp: In destructor ‘QPULib::RegisterMap::~RegisterMap()’:
Lib/VideoCore/RegisterMap.cpp:87:20: error: ‘m_addr’ was not declared in this scope
Lib/VideoCore/RegisterMap.cpp:87:28: error: ‘m_size’ was not declared in this scope
Lib/VideoCore/RegisterMap.cpp: In member function ‘uint32_t QPULib::RegisterMap::read(int) const’:
Lib/VideoCore/RegisterMap.cpp:103:9: error: ‘m_addr’ was not declared in this scope
Lib/VideoCore/RegisterMap.cpp: In static member function ‘static void QPULib::RegisterMap::check_page_align(unsigned int)’:
Lib/VideoCore/RegisterMap.cpp:179:52: error: ‘errno’ was not declared in this scope
Lib/VideoCore/RegisterMap.cpp:180:10: error: ‘exit’ was not declared in this scope
Lib/VideoCore/RegisterMap.cpp:185:10: error: ‘exit’ was not declared in this scope
Makefile:183: recipe for target 'obj-qpu/VideoCore/RegisterMap.o' failed
make: *** [obj-qpu/VideoCore/RegisterMap.o] Error 1
wimrijnders commented 6 years ago

Huh? that's a strange error. The function doesn't declare parameters because there aren't any.

Compiles perfectly in my case.

wimrijnders commented 6 years ago

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 c++0x of your compiler can't deal with nullptr. Far-fetched, I know. Perhaps it's time to upgrade it to c++11, the other one is a transient gcc-specific thing anyway.

EDIT: The initialization with {} might be the thing. It would make somewhat sense given the error message. The fix for that is to old-style initialization lists in the constructor. Or you upgrade your distro.

wimrijnders commented 6 years ago

Replaced inline init with constructor initialization list.

This is to determine what the problem is. I still would prefer to use -std=c++11.

mn416 commented 6 years ago

Still seeing compile errors:

Compiling Lib/VideoCore/RegisterMap.cpp
In file included from Lib/VideoCore/RegisterMap.cpp:3:0:
Lib/VideoCore/RegisterMap.h:35:11: error: function definition does not declare parameters
Lib/VideoCore/RegisterMap.cpp: In constructor ‘QPULib::RegisterMap::RegisterMap()’:
Lib/VideoCore/RegisterMap.cpp:75:2: error: ‘m_size’ was not declared in this scope
Lib/VideoCore/RegisterMap.cpp: In destructor ‘QPULib::RegisterMap::~RegisterMap()’:
Lib/VideoCore/RegisterMap.cpp:87:28: error: ‘m_size’ was not declared in this scope
Lib/VideoCore/RegisterMap.cpp: In static member function ‘static void QPULib::RegisterMap::check_page_align(unsigned int)’:
Lib/VideoCore/RegisterMap.cpp:179:52: error: ‘errno’ was not declared in this scope
Lib/VideoCore/RegisterMap.cpp:180:10: error: ‘exit’ was not declared in this scope
Lib/VideoCore/RegisterMap.cpp:185:10: error: ‘exit’ was not declared in this scope
Makefile:190: recipe for target 'obj-qpu/VideoCore/RegisterMap.o' failed
make: *** [obj-qpu/VideoCore/RegisterMap.o] Error 1
wimrijnders commented 6 years ago

Ah right, same problem, different variable. Hold on.

This is a compiler issue I believe; it can't handle the full c++11 syntax.

wimrijnders commented 6 years ago

'Fixed' in last commit, please test. I'm running unit tests in the meantime.

EDIT: Unit tests passed.

mn416 commented 6 years ago

Looks like you're just missing #include for stdlib.h and errno.h. If I add those, it compiles.

Compiling Lib/VideoCore/RegisterMap.cpp
Lib/VideoCore/RegisterMap.cpp: In static member function ‘static void
QPULib::RegisterMap::check_page_align(unsigned int)’:
Lib/VideoCore/RegisterMap.cpp:179:52: error: ‘errno’ was not declared
in this scope
Lib/VideoCore/RegisterMap.cpp:180:10: error: ‘exit’ was not declared
in this scope
Lib/VideoCore/RegisterMap.cpp:185:10: error: ‘exit’ was not declared
in this scope
Makefile:190: recipe for target 'obj-qpu/VideoCore/RegisterMap.o'
failed
make: *** [obj-qpu/VideoCore/RegisterMap.o] Error 1
wimrijnders commented 6 years ago

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 sudo obj/qpu/bin/detectPlatform after make DEBUG=1 detectPlatform? That would be an indication that the register map is working as expected.

wimrijnders commented 6 years ago

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.

mn416 commented 6 years ago

Did you manage to run sudo obj/qpu/bin/detectPlatform

Works on Pi 2

mn416 commented 6 years ago

Builds and runs on Pi 1, but takes a few seconds to complete and gives unexpected output:

Hardware revision: 000d
Number of slices: 15
QPUs per slice: 15
wimrijnders commented 6 years ago

Yes, because the peripheral address is different like I mentioned. But thanks for checking anyway. Will fix it tomorrow - I actually took a shortcut in the code because I thought you were running Pi 2 only. Oh well.

wimrijnders commented 6 years ago

Great that you mentioned the hardware revision as well. Useful.

wimrijnders commented 6 years ago

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.

wimrijnders commented 6 years ago

@mn416 Would you mind testing the following?

# Enable following
USE_BCM_HEADERS:= $(shell grep bcm_host_get_peripheral_address /opt/vc/include/bcm_host.h && echo "no" || echo "yes")
# Disable: USE_BCM_HEADERS:=no
CXX_FLAGS = -Wconversion -std=c++11 -I $(ROOT) -MMD -MP -MF"$(@:%.o=%.d)" -g  # Add debug info: -g
#                             ^^^^^   Change to this

Do these work as expected now on your Pi's?

mn416 commented 6 years ago

Need to include stdio.h. Then builds on a Pi 1 but when running:

This is a Pi platform
Hardware revision: 000d
Can't open device file: /dev/vcio

The device file exists.

mn416 commented 6 years ago

c++11 not available on my Pi 1.

mn416 commented 6 years ago

Doesn't build on Pi 1 if I use the following

USE_BCM_HEADERS:= $(shell grep bcm_host_get_peripheral_address /opt/vc/include/bcm_host.h && echo "no" || echo "yes")

wimrijnders commented 6 years ago

OK. Silly thing is I removed it previous commit.

c++11 not available on my Pi 1.

OK, thanks for checking. So the baseline is c++0x. The frustrating thing is that the c++0x of my gcc version is different from yours.

Doesn't build on Pi 1 if I use the following

Thanks for checking. OK, I'll leave USE_BCM_HEADER:=no as default then. The comments covers the use of the variable for newer platforms.