Skip to content
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

context_drm_egl: allow autoprobe selection #9033

Merged
merged 1 commit into from
Jul 23, 2021

Conversation

Dudemanguy
Copy link
Member

Sometimes I use drm and I got annoyed by having to type --gpu-context=drm. The drm context has been explicitly coded to disallow the autoprobe almost its entire existence (since: f757163). @rr-: do you remember what the reason for that commit was?

This was explictly coded to avoid the autoprobe way back when in
2015[1] with no real explanation. In theory, there shouldn't be any
issues allowing this. If a user runs mpv in tty, this is probably what
they actually want in most cases. Perhaps something strange happens on
nvidia, but it should just fail anyway during the autoprobe.

[1]: mpv-player@f757163
@rr-
Copy link
Member

rr- commented Jul 23, 2021

@Dudemanguy #2459
sadly I don't have IRC logs from that year

@Dudemanguy
Copy link
Member Author

Thanks, I didn't realize that had a PR.

wm4 expressed fair concern on IRC that it might cause severe problems to running X servers. I remember we had problems with vo_drm on certain machines

I guess the crux is whether or not this still true. At least, I've never had any problems.

@philipl
Copy link
Member

philipl commented Jul 23, 2021

I'm comfortable allowing autoprobe. I've never seen conflicts with running X servers when testing drm_egl before.

@Dudemanguy Dudemanguy merged commit 747b152 into mpv-player:master Jul 23, 2021
@Dudemanguy Dudemanguy deleted the drm-autoprobe branch July 23, 2021 17:55
@xantoz
Copy link
Member

xantoz commented Jul 24, 2021

I think there are some (maybe rare these days) configurations where trying to do these DRM things will rip your X server to shreds.
Like some really old drivers that haven't fully converted to KMS. Or say, if you're running X11 on the fbdev driver (why you would do that instead of the modesetting driver when you have proper modesetting is beyond me, however), or perhaps weston with its fbdev backend. In this case mpv will iikely steal your entire display, and you'll lose control over mpv if you manage to unfocus the terminal window you ran mpv from (what if you didn't even run mpv from a terminal window?), when what you likely actually wanted was vo_x11 or vo_wayland.
You might need to run mpv as root to see it cause you any actual trouble, however. Most modern drivers shouldn't really have any issues, though, and in practice I don't think I have seen any actual conflicts with this.
So I guess it's fine?

There are still some potential issues around VT switching though, and just other general roughness of this output that speaks against enabling it by default, and having it be a conscious choice by an experienced user who knows what they are getting into.

Speaking of the VT switching issues. Since a few kernels back drmSetMaster/drmDropMaster work w/o root privileges. We should probably make a PR to start using these to make VT switching less likely of blowing your X11 server/wayland server up (in my experience even with this, sometimes things go south though!)

@Dudemanguy
Copy link
Member Author

Technically speaking, displayvk has no autoprobe detection/check so prior to this commit if i was to run mpv --no-config in tty, it would pick that backend (and it would for anyone else assuming they had vulkan drivers). That has no VT restoration at all in it currently so it would actually mess up your stuff everytime guaranteed. So in practice, this should actually be a safer default for most people I think.

The other option would have been to add an autoprobe check to displayvk I suppose. I figure if a user actually wants to run mpv in an tty or is using fbdev on x11/wayland, they understand what they are doing.

@philipl
Copy link
Member

philipl commented Jul 24, 2021

Yeah. You can argue I should have disabled autoprobe but you don't even try using mpv on a try unless you want to get one of these backends working. The sharp edges don't come in to play unless you're looking for them.

@evelikov
Copy link
Contributor

evelikov commented Jul 24, 2021

Btw there's a multitude of bugs that got fixed in the area - there's the laundry-list and some details behind each one

  • We had bug or two in mesa where driver state will leak, so trying egl/x11 and then egl/drm (or vice-versa) will misbehave.
    That has been resolved for at least 1 year or so.

  • Apps should be using the render node for rendering. If they still use the card node for rendering, but do not call drmGetMagic/drmAuthMagic then kernel 5.4 and later has this workaround

  • On the VT side - apps must call drmDrop/SetMaster, which seems to be disabled in mpv ☹️ see:

    static void release_vt(void *data)
    and
    static void release_vt(void *data)

As the comment says, the functions may fail, but not calling them in the first place is just bonkers. Note that with linux v5.10 and later aka this kernel commit the root requirement was reworked and mpv should just work.

Please re-enable the drmSet/DropMaster calls, or I'll try to get to it tomorrow.

Disclaimer: I've wrote the kernel patches, plus some of the mesa ones. I don't recall all the required mesa patches, hence no links.

@rr-
Copy link
Member

rr- commented Jul 25, 2021

I remember having issues with drmSet/DropMaster. They required root privileges, and no one on the Internet could explain why, which is why we didn't enable them. Apparently that is no longer the case so maybe whatever the original issue was will resolve automatically.

not calling them in the first place is just bonkers.

I disagree. I remember calling these functions crashing my running X server on VT2 without any warning. Now that is bonkers.

In general this stuff was severely underdocumented back in the day, and our main source of knowledge was a project called kms cube.

@xantoz
Copy link
Member

xantoz commented Jul 25, 2021

I've been meaning to get to the VT side of things, but I'm no longer very active in this project. Patches welcome! If drmSetMaster fails VT switching should be altogether disabled, methinks.

What about the (admittedly somewhat odd) case when someone is using X11 or say weston with the fbdev backend, despite being on modesetting supported HW, running mpv will steal the screen from fbdev, when I as a user would probably expect vo_x11 or vo_wayland to be autodetected. There's also the danger of losing control over mpv if the user unfocuses the controlling terminal (with the DRM backend mpv just uses the controlling terminal for input, because it's simple and requires no extra code for input handling... although implementing proper input handling might be a project for the future if someone is interested enough in it)

@rr-
On recent kernels drmSetMaster and drmDropMaster works perfectly fine without root privileges, so I agree in that we should start using them now. Actually not using them can crash when you VT-switch from a running mpv back to an X11-server...

@evelikov
Copy link
Contributor

The commit linked explains why the ioctls require root access. Although the mpv team is always welcome to reached out and ask - DRM folks don't bite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants