-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize native video detection #3686
base: master
Are you sure you want to change the base?
Conversation
I should add a check for mesa driver and softgl in get_opengl_target_platform(). |
I think a large core change like this would have been best started as a discussion first. I am not against the overall concept of better automatic setting of flags to support additional platforms but the implementation needs work imho. I don't think we need so many additional flags also (When nothing is using them and other auto detected systems don't use them). The use of kmscube and the logic for setting flags needs a rethink. I made some comments on the code, but even with those changes I can't say I would accept this currently sorry. |
function has_video_output_device() { | ||
# check if there is a video output device with a connected display | ||
if [[ -d /sys/class/drm ]]; then | ||
for d in /sys/class/drm/*/ ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing local declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
scriptmodules/system.sh
Outdated
|
||
function has_render_device() { | ||
# check if there is a render note | ||
[[ ! -z "$(ls /dev/dri/render*)" ]] && return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-n
preferred over ! -z
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
scriptmodules/system.sh
Outdated
else | ||
echo "Unknown system target. Please use multi-user.target or graphical target." | ||
exit | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the additional functions for target if they are only used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this message. I only know multi-user.target and graphical.target. I also removed isGraphicalTarget() and isMultiUserTarget() functions because they were only needed in get_graphics_platform().
scriptmodules/system.sh
Outdated
elif compareVersions "$gles_ver" gt 3.0; then | ||
__platform_flags+=(gles gles2 gles3 gles31) | ||
elif compareVersions "$gles_ver" gt 1.1; then | ||
# Prefer GLES 2.0 over GL 2.1 because GlideN64 runs with GLES 2.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this alone is a reason to make a global choice on flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could create a list with all packages and requiered gl versions to find best fit or check if we can add gles and gl platform flags at the same time. But for the moment i only now one emulator which needs gles2 or gl4.2.
scriptmodules/system.sh
Outdated
elif compareVersions "$gl_ver" gt 1.3; then | ||
# Only allow OpenGL >= 1.4 | ||
__platform_flags+=(gl) | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic needs some work. Eg setting all the flags each time depending on version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified it now. Only new flag are gl2 and softpipe. gl2 is there to build emulationstation with gl1.4 or gl2.1. If softpipe is set we only have software gl and should try to use software renderers if possible.
scriptmodules/system.sh
Outdated
! hasPackage "kmscube" && aptInstall kmscube | ||
|
||
# install killall to stop kmscube | ||
! hasPackage "psmisc" && aptInstall psmisc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using killall you could get the PID and just use kill. Also these could be done as a single call to aptInstall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed kmscube. The solution looked more like a hack.
scriptmodules/system.sh
Outdated
sudo -u $user kmscube &> "$gl_result" & | ||
sleep 1 | ||
killall kmscube &>/dev/null | ||
gles_ver="$(grep 'version: \"OpenGL ES' $gl_result | egrep -o '[0-9]{1}.[0-9]{1}' | head -n 1)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better solution than running kmscube and killing it to extract this data can be found.
get_graphics_platform | ||
get_opengl_target_platform | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think this is needed as a standalone function. This PR relies too much on "something && something &&" and it isn't that readable imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it into platform_native() and removed get_native_video.sh.
get_graphics_platform really just checks if there is a lvds/hdmi/lcd display controller with a kms driver which can be used to output something. On arm platforms display controller and gpus can be separate devices. Display controllers are kms devices and gpus are kms render only (kmsro) devices. With get_opengl_target_platform we check capabilities of the render device.
72485d7
to
bd8fd8e
Compare
If a device platform is unknown platform "native" will be used. Platform native assumes there is a x11 session which supports gl and vulkan. If you want to run from console you can set "force_kms" in retropie.cfg. There is no auto detection. You can not use gles. Add some logic which analyzes the system and sets platform flags according to it. -add helper function get_graphics_platform(): checks enviroment and sets platform flags for video backend. -add helper function get_opengl_target_platform(): tries to get GL and GLES versions from system and sets platform flags for GL or GLES. -add helper function has_render_device(): checks if there is a kmsdrm render note. -add helper function has_video_output_device(): checks if there is a kmsdrm video output device.
-use -DUSE_GL21=On if gl2 is set -use -DUSE_GLES1=On if gles2 is not set
bd8fd8e
to
1cd6b0c
Compare
If a device platform is unknown platform "native" will be used. Platform native assumes there is a x11 session which supports gl and vulkan. If you want to run from console you can set "force_kms" in retropie.cfg. There is no auto detection. You can not use gles. Add some logic which analyzes the system and sets platform flags according to it.
system.sh
emulationstation.sh