bsd, uvd/vce, fractional interval and xcbdl

[trek00]

I would like to receive comments about this patchset. I think it's ready to be committed, but it still not tested on radeon, sorry. Thank you for review!

[clbr]

Please don't group unrelated changes in a massive PR like this, I'm busy as is...

[clbr]

At least the *xcb changes are a no. That's just added complexity for no discernible reason.

[trek00]

On Tue, 29 Oct 2019 00:11:26 -0700 clbr notifications@github.com wrote:

At least the *xcb changes are a no. That's just added complexity for no discernible reason.

good question, i should add some description

it seems to me libradeontop_xcb was made to let distribution packagers declare the libxcb dependency as optional

after quick look i found nearly all packages include libxcb as hard-dependency, defeating the purpose to dynamically link libradeontop_xcb at runtime (eg. you can't install radeontop on a headless server without installing the full Xorg stack)

usually packagers simply run a script to identify libraries needed by installed binary files, thus libxcb is automatically detected when checking libradeontop_xcb dependencies (eg. dpkg-shlibdeps)

in addition, you need to install libradeontop_xcb in order to run radeontop, so basically you can't:

• run an arbitrary version of radeontop without installing it
• have multiple versions of radeontop installed
• debug radeontop without tweaking LD_PRELOAD variable
• produce a statically linked binary (eg. for profiling)

now we are in the worst situation, because the full Xorg stack needs to be installed, all the complexity of dynamic linking is here and radeontop is not working alone

this patch should make packagers life simpler and give users the expected feature (no Xorg requirement)

anyway there is no hurry to commit

thank you for reading!

[clbr]

Yes, it's an important feature that an X-enabled build runs without X installed. These changes enable a binary that hard depends on X, which is not wanted (non-DL define). Packagers declaring a hard dep is unfortunate, but anyone setting up such a system can download the package only.

If you're debugging radeontop etc, you have root rights anyway; the unlikely case of hacking radeontop on someone else's system does not outweigh the uglier code, increased LOC and decreased efficiency (multiple dynamic symbol resolutions vs one).

[trek00]

well, i'm ok with your decision, it's your software, but it seems to me my english is uglier that my code, so this time i'll try to clarify your doubts only with code :)

These changes enable a binary that hard depends on X

no, if you compile latest patch you can check with:

$ make clean all && ldd radeontop | grep xcb

the output is empty, so no hard dependency on X, but if you do:

$ make clean all xcbdl=0 && ldd radeontop | grep xcb

you get: libxcb.so.1 => /usr/lib/x86_64-linux-gnu/libxcb.so.1 libxcb-dri2.so.0 => /usr/lib/x86_64-linux-gnu/libxcb-dri2.so.0

this instead make an hard dependency on X

decreased efficiency (multiple dynamic symbol resolutions vs one)

at binary level, it's the opposite and you can check with:

$ make clean all && DISPLAY=invalid strace ./radeontop -p /dev/dri/card0 2>&1 | wc -l

the output is: 200

if you try without the patch:

$ git checkout 4a26fed && make clean all && DISPLAY=invalid strace ./radeontop -p /dev/dri/card0 2>&1 | wc -l

the output 10 syscalls more: 210

in fact, with the patch it needs to do one less symbol resolution, that is authenticate_drm_xcb and it opens a one less file, that is libradeontop_xcb.so:

openat(AT_FDCWD, "/usr/lib/libradeontop_xcb.so", O_RDONLY|O_CLOEXEC) read(4,"\177ELF\2\1\1\0\0\0\0\0\0\0"..., 832) fstat(4, {st_mode=S_IFREG|0644, st_size=14424, ...}) mmap(NULL, 16488, PROT_READ, MAP_PRIVATE|MAP_DENYWRITE, 4, 0) mmap(0x7fac4fcd1000, 4096, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 4, 0x1000) mmap(0x7fac4fcd2000, 4096, PROT_READ, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 4, 0x2000) mmap(0x7fac4fcd3000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 4, 0x2000)

about the need to debug with LD_PRELOAD you can check:

$ git checkout -f drm5 && sed -i 's/void auth.magic) {/&printf("We are here!\n");/' auth.c && make clean all && ./radeontop -p /dev/dri/card0

it prints: We are here!

but without patch:

$ git checkout -f 4a26fed && sed -i 's/void auth.magic) {/&printf("We are here!\n");/' auth.c && make clean all && ./radeontop -p /dev/dri/card0

it prints nothing! but you can force it with:

$ LD_PRELOAD=./libradeontop_xcb.so ./radeontop -p /dev/dri/card0

and it finally prints: We are here!

anyway i think we could focus more on the other patches and hold on these last two

if you think these patches can be improved is some way, please to tell me, i'm willing to refine them :)

ciao!

[trek00]

any news? can I help in some way? ciao :)

[dgcampea]

any news? can I help in some way? ciao :)

Splitting this patchset into multiple branches/PRs? Makes it simpler to review/accept the changes.

[trek00]

do you mean a single merge request for each patch?

[dgcampea]

do you mean a single merge request for each patch?

no, single merge request per feature

[userofryzen]

@trek00 maybe its time yo start using your version.. too much time without updates here