Various DRM Atomic improvements #5714

LongChair · 2018-04-07T07:45:13Z

Here are a few commits that intend to improve drm atomic support.
The main topic there are taking care of are :

switch the the new API for the drm prime interop.
use atomic modesetting in atomic path.
Add the ability to select planes for video & osd
Add the ability to use a specified osd size that can be different than videomode.

I agree that my changes can be relicensed to LGPL 2.1 or later.

ghost · 2018-04-07T10:13:52Z

video/out/opengl/context_drm_egl.c

+        p->osd_size.width = p->kms->mode.hdisplay;
+        p->osd_size.height = p->kms->mode.vdisplay;
+    } else {
+        if (!p->kms->atomic_context) {


On both of these ifs, remove the !, and switch the if/else bodies.

ghost · 2018-04-07T10:19:05Z

video/out/opengl/libmpv_gl.c

+            ra_add_native_resource(ctx->ra, "drm_params", drm_params);
+        void *drm_osd_size = p->gl->MPGetNativeDisplay("drm_osd_size");
+        if (drm_osd_size)
+            ra_add_native_resource(ctx->ra, "drm_osd_size", drm_osd_size);


This is just compatibility code. It should not be needed since there are no API users which use the legacy opengl-cb API and the new DRM stuff. And a new API user can't use this at all, unless he's using the opengl-cb legacy pseudo GL extension.

Are you meaning that I should have added a param on the new API for both structs ?
If so that would come in a next step I guess. If not can you please gimme some more details :)

Also not sure if I agree that the compat stuff should stay here, since the new API has proper support for it.

I understand what you mean, but unless i missed something, there is no other way to have access to this from the hwdec ... the params array is not passed there.

The quoted code is explicitly for the legacy opengl-cb code, which uses glMPGetNativeDisplay. That doesn't exist in the new render API and is not used there.

xantoz · 2018-04-07T13:02:45Z

video/out/drm_common.c

@@ -47,12 +47,18 @@ const struct m_sub_options drm_conf = {
        OPT_STRING_VALIDATE("drm-connector", drm_connector_spec,
                            0, drm_validate_connector_opt),
        OPT_INT("drm-mode", drm_mode_id, 0),
-        OPT_INT("drm-overlay", drm_overlay_id, 0),
+        OPT_INT("drm-osd-plane-id", drm_osd_plane_id, 0),


Should we not document these options as well? If we do we probably also need to clarify when what plane is used. That only when using drmprime hwdec is the video actually rendered to the video plane, and the "overlay plane" is the normal plane &c.

I guess we could consider it internal/primarily for API use due to the confusing nature...

All options must be documented.

I agree, I thought I had done it, but will add it.

LongChair · 2018-04-08T08:40:21Z

Fixed the comments above, thanks to lemme know if you have any further comments :)

xantoz · 2018-04-08T18:25:18Z

DOCS/man/vo.rst

+        Index is zero based, and related to crtc.
+        (default: first overlay plane)
+
+    ``--drm-video-plane-id=<number>``


Sorry about the nitpicking, but we should probably mention that this is only really the case when we use drmprime hwdec with --vo=gpu --gpu-context=drm, and that otherwise (this includes --vo=drm) the video plane is unused with the osd plane instead being used for both video and OSD.

I know it's a bit late, but I've been thinking that maybe different naming for the planes could be used here to make the matter less confusing. Maybe something like "drm-render-plane-id" (for what is now "drm-osd-plane-id") and "drm-hwdec-video-plane-id" (for what is now "drm-video-plane-id"), for example. (Although this particular naming would of course not be forward-compatible if we decide to use separate planes for video/OSD in the software decoded case in the future.)

What do you think (and if anyone has better naming ideas please say)?

I see what you're saying. Although we have to consider that options should have a not too much technical name. hwdec probably don't mean anything for most users typically :)
But as you say, in some cases both video & osd are on the same plane (in which case btw you probably don't want any of those options, primary plane will be used then).
Maybe mentioning that those options should be used in the case you mentioned would be enough to clarify that case ?

Mentioning that the options should primarily be used together with drmprime hwdec, and that in other cases (--vo=drm, --gpu-context=drm swdec) everything will be rendered to the "osd plane" should be fairly sufficient, should we keep the current names.

Re: the names: yeah, my names were just examples. I think maybe better ones than that could be thought of.

LongChair · 2018-04-10T05:29:18Z

Adjusted documentation description :)

xantoz

Just some small grammar gripes.

xantoz · 2018-04-10T11:42:14Z

DOCS/man/vo.rst

+        Select the DRM planed index to use for video layer.
+        Index is zero based, and related to crtc.
+        This option is only valid when using the drm_prime renderer which supports
+        several layers and with ``vo=gpu`` and ``gpu-context=drm``.


use commas or parentheses around "which supports several layers".
"and with" -> "together with"

also perhaps "only valid" -> "only has an effect", since it is technically valid (i.e. mpv won't complain), it just won't do anything useful.

xantoz · 2018-04-10T11:43:43Z

DOCS/man/vo.rst

+        Select the DRM planed index to use for OSD (or OSD and video).
+        Index is zero based, and related to crtc.
+        When using this option with drm_prime renderer, it will only affect
+        the OSD contents. Otherwise will set OSD & video plane.


"Otherwise will set..." -> "Otherwise it will set..."

LongChair · 2018-04-10T12:19:09Z

grammar gripes processed :)

xantoz · 2018-04-10T15:39:45Z

So I finally got around to testing it on an intel with more than one connector, and I saw some regressions (Sandybridge, linux 4.16.0, mesa-18.0.0). I think it's related to the new atomic modesetting primarily, although I haven't looked too much into it yet so I can't entirely rule out the new connector handling (will do later).

It might also be simply that atomic modesetting is not yet implemented for these old GPU:s, in which case we will need to have a fallback (still use atomic commits, but do the modesetting as before), however I think I've been able to run a kmscube implementation that uses atomic modesetting fine (will test later) on sandybridge.

On to the tests I did:

It works but with a warning on the internal LVDS panel, but probably only because it only supports one resolution, and it will work without modesetting:
lvds--working-but-probably-just-luck.log

On the HDMI-A-1 connector it fails hard:
hdmi-a-1--not-working-atomic-modesetting.log

If I first use the HDMI-A-1 panel using -vo drm, or just an mpv without this PR the fbcon on this VT seems to to change a bit and the LVDS screen gets deactivated (multi-screen setups is something which could use some improvement, but that is another problem for another day. both deinit and in particular VT switching are a bit flakey) and the HDMI-A-1 screen takes on a "main" role. Now I can actually do some of the lower resolutions (with warning messages) on the HDMI, but not above 1024x768 or so (I guess because of the resolution the fbcon picked for HDMI-A-1 after mpv closed). I also tried some VT switching now, to see if I would get more error messages:
hdmi-a-1--kinda-works--also-vtswitching.log

I guess this latter case is similar to the first with LVDS though, that we simply got lucky because the fbcon was already setup somehow.

LongChair · 2018-04-14T16:24:38Z

@xantoz : any more investigations ? Would you have any suggestions regarding this PR ?
I would like to get this going :)

LongChair · 2018-04-14T16:25:04Z

@wm4 : by the way, anymore comments from your side ? :)

ghost · 2018-04-15T09:46:28Z

libmpv/render_gl.h

+} mpv_opengl_drm_params;
+
+typedef struct mpv_opengl_drm_osd_size {
+


Weird newline.

ghost · 2018-04-15T09:53:29Z

libmpv/render.h

+     * Valid for mpv_render_context_create().
+     * Type : struct mpv_opengl_drm_osd_size*
+     */
+    MPV_RENDER_PARAM_DRM_OSD_SIZE = 11,


Apparently your code expects that the pointers the user passes are valid for the life time of the context, which I'd find surprising. I think the structs really should be copied (and then use pointers for fields which somehow need to be changed per frame or something). That would require changes in libmpv_gpu.c (init() function). I guess native_resource_map needs to be changed into an array of structs, so a struct size to copy can be specified.

The drmModeAtomicRequest pointer which is what you're referring to as changing every frame is in that struct and already passed as a pointer to the pointer address. So copying this should still be possible.
But yes you are right on the fact that I have been assuming that the struct keeps being alive during the whole playback. If some change is made to the native_resource_map, we can probably use a copy to overcome this.

Added such a change here: 0bfdb00
You'd simply set the .size field to the struct sizes, and it'd copy them.

That's part of #5716 btw., which somehow turned into a PR for general changes.

xantoz · 2018-04-16T19:34:15Z

@LongChair I'm still working on it, but I can already tell you I get problems already with commit 540fec5 (can't use --drm-format=xrgb2101010 with -vo gpu. when it does work I get black bars through it. next commit fixes this though, but introduces the modesetting errors. have yet to test with a multi-screen setup) so it is probably not solely atomic modesetting related.

xantoz · 2018-04-16T19:37:46Z

video/out/drm_atomic.c

-struct drm_atomic_context *drm_atomic_create_context(struct mp_log *log, int fd,
-                                                     int crtc_id, int overlay_id)
+struct drm_atomic_context *drm_atomic_create_context(struct mp_log *log, int fd, int crtc_id,
+													 int connector_id, int osd_plane_id, int video_plane_id)


do not use tabs for indentation

xantoz · 2018-04-16T19:38:10Z

video/out/drm_atomic.c

-                    }
-                    else if (value == DRM_PLANE_TYPE_PRIMARY) {
-                        ctx->primary_plane = plane;
+                } else {                    


trailing whitespace

xantoz · 2018-04-16T19:44:32Z

DOCS/man/vo.rst

-        Select the DRM overlay index to use.
-        Overlay index is zero based, and related to crtc.
-        (default: 0)
+    ``--drm-osd-plane-id=<number>``


Move the documentation of these options (--drm-osd-plane-id, --drm-video-plane-id) from this commit, to commit 540fec5 when they are introduced in the code.
Like now the documentation will have been introduced before the options.

xantoz · 2018-04-16T19:50:09Z

video/out/opengl/context_drm_egl.c

-
+    ra_add_native_resource(ctx->ra, "drm_params", &p->drm_params);
+    ra_add_native_resource(ctx->ra, "drm_osd_size", &p->osd_size);
+


trailing whitespace

xantoz · 2018-04-16T22:24:25Z

On 540fec5, selecting --drm-osd-plane-id=0 fixes the problem with not being able to use XRGB2101010, so there might be something related to the plane ID:s. I think maybe the way we select ID:s in drm_atomic_create_context needs to be rethought.
I'll be back with more info tomorrow/tomorrow night.

xantoz · 2018-04-17T13:58:56Z

video/out/drm_atomic.c

-    if (!ctx->primary_plane) {
-        mp_err(log, "Failed to find primary plane\n");
-        goto fail;
+    // default OSD plane to overlay if unspecified


Swap this around? OSD plane defaults to primary plane if unspecified. Then behavior would be like before, especially w.r.t. to software decoding, and --drm-format=xrgb2101010 would still work on intel without specifying the primary plane with --drm-osd-plane-id

An idea to make this nicer to use would be to add the options --drm-osd-plane-id=(primary|overlay) and --drm-video-plane-id=(primary|overlay), which will pick any matching overlay/primary plane instead of a specific numbered one. (Although I would still argue that --drm-osd-plane-id should default to primary, and --drm-video-plane-id default to overlay in this case, but even if it didn't I could just put drm-osd-plane-id=primary in my config file and it would Just Work (TM))

xantoz · 2018-04-17T13:59:17Z

video/out/drm_atomic.c

-    if (!ctx->overlay_plane) {
-        mp_err(log, "Failed to find overlay plane with id=%d\n", overlay_id);
-        goto fail;
+    // default video plane to primary if unspecified


default video plane to overlay

xantoz · 2018-04-18T15:34:47Z

You've missed to change "primary plane" to "OSD plane" (or whatever we're going to end up calling it) in a few log messages and comments in context_drm_egl.c

xantoz · 2018-04-24T19:23:51Z

I have made a pull request on LongChair:s atomic-additions branch with the changes needed to get this PR working for me (LongChair#1)

Now waiting for LongChair to test that these changes are compatible with his use-case.

That new API was introduced and allows to have several native resources. Thisuses that mechanisma for drm resources rather than the deprecated opengl-cb structs. This patch therefore add two structs that can be used with the drm atomic interop. - mpv_opengl_drm_params : which will hold all the drm handles - mpv_opengl_drm_osd_size : which will hold osd layer size This commit adds a drm-osd-size=WxH parameter to commandline which allows to define the OSD plane dimension. OSD can be upscaled to screen resolution when having OSD at video resolution is too heavy. This is especially useful for UHD modes on embedded devices where the GPU cannot handle UHD modes at a decent framerate.

This patch adds - DRM connector object to atomic context. - fd property to the drm atomic object as well as a method to read blob type properties. This allows to ensure that the proper connector is picked up, especially when specifying it from the commandline, and also allows to make sure we're using the right one when embedding with interop into an application.

LongChair · 2018-04-29T13:30:03Z

Updated this PR according to comments.
Also added a couple commits from @xantoz as he submitted a PR.
I also had to chnage the atomic_request member of the drm parameters struct type to a double pointer as we're now copying the render API params.

We are currently using primary / overlay planes drm objects, assuming that primary plane is osd and overlay plane is video. This commit is doing two things : - replace the primary / overlay planes members with osd and video planes member without the assumption - Add two more options to determine which one of the primary / overlay is associated to osd / video. - It will default osd to overlay and video to primary if unspecified

This commit allows to add atomic modesetting when using the atomic renderer. This is actually needed when using and osd with a smaller size than screen resolution. It will also make the drm atomic path more consistent

This patch will make sure that the video plane is hidden when unused. When using high resolution modes, typically UHD, and embedding mpv, having the video plane sitting in the back when you don't play any video is eating a lot of memory bandwidth for compositing. That patch makes sure that the video layer is just disabled before and after playback.

Add some properties which where forgotten in crtc_setup_atomic. In both change to not use DRM_MODE_PAGE_FLIP_EVENT | DRM_MODE_ATOMIC_NONBLOCK flags. This should make it more similar to the drmSetCrtc which it aims to replace (take effect directly, and blocking call). This also saves us the trouble of having to set up a poll to wait for pageflip, which would've been neccesary with DRM_MODE_PAGE_FLIP_EVENT, in both crtc_setup_atomic and crtc_release_atomic.

… updated since the plane rename commit

Inspired by kmscube, first try to pick the Encoder and CRTC already associated with the selected Connector, if any. Otherwise try to find the first matching encoder & CRTC like before. The previous behavior had problems when using atomic modesetting (crtc_setup_atomic) when we picked an Encoder & CRTC that was currently being used by the fbcon together with another Encoder. drmModeSetCrtc was able to "steal" the CRTC in this case, but using atomic modesetting we do not seem to get this behavior automatically. This should also improve behavior somewhat when run on a multi screen setup with regards to deinit and VT switching (still sometimes you end up with a blank screen where you previously had a cloned display of your fbcon)

xantoz · 2018-04-30T16:53:35Z

Looks generally good to me.

I'd like to improve the options a little bit (So that we can specify --drm-osd-plane-id=primary or --drm-osd-plane-id=overlay, for getting a generic plane for instance, since using the indexes like now is hard. Maybe also adding a help option?), but I will do that in a later PR which I create, so I think we should merge this first.

LongChair · 2018-04-30T17:14:01Z

@wm4 : any further comments from your side ? :)

ghost · 2018-04-30T17:31:38Z

No.

jeeb

Generally LGTM. Will push a minor typo fixup commit after merging that fixes some planeds in vo.rst :)

jeeb · 2018-05-01T17:47:54Z

Have tested that it compiles, and that generally the code seems to look OK and documentation is added for new options. @LongChair is pretty much the DRM maintainer and this PR touches almost fully just it.

ghost reviewed Apr 7, 2018

View reviewed changes

xantoz reviewed Apr 7, 2018

View reviewed changes

LongChair force-pushed the atomic-additions branch from cda4db6 to 1562411 Compare April 8, 2018 08:39

xantoz reviewed Apr 8, 2018

View reviewed changes

LongChair force-pushed the atomic-additions branch from 1562411 to 6c30576 Compare April 10, 2018 05:28

xantoz requested changes Apr 10, 2018

View reviewed changes

LongChair force-pushed the atomic-additions branch from 6c30576 to b5932dd Compare April 10, 2018 12:18

ghost reviewed Apr 15, 2018

View reviewed changes

libmpv/render_gl.h Outdated

} mpv_opengl_drm_params;

typedef struct mpv_opengl_drm_osd_size {

Copy link

ghost Apr 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird newline.

ghost reviewed Apr 15, 2018

View reviewed changes

xantoz reviewed Apr 16, 2018

View reviewed changes

xantoz reviewed Apr 17, 2018

View reviewed changes

xantoz mentioned this pull request Apr 26, 2018

drm_common: Be smarter when deciding on which CRTC and Encoder to use #5769

Closed

LongChair added 2 commits April 29, 2018 15:20

LongChair force-pushed the atomic-additions branch from b5932dd to ff58919 Compare April 29, 2018 13:24

LongChair and others added 7 commits April 29, 2018 17:46

drm/atomic: add atomic modesetting.

7ef48a9

This commit allows to add atomic modesetting when using the atomic renderer. This is actually needed when using and osd with a smaller size than screen resolution. It will also make the drm atomic path more consistent

context_drm_egl: fix some comments and log messages that had not been…

0bf1b3a

… updated since the plane rename commit

drm_atomic: Disallow selecting cursor planes using the options

56d4541

LongChair force-pushed the atomic-additions branch from ff58919 to 56d4541 Compare April 29, 2018 15:47

jeeb approved these changes May 1, 2018

View reviewed changes

jeeb merged commit a9c2a8e into mpv-player:master May 1, 2018

		} mpv_opengl_drm_params;

		typedef struct mpv_opengl_drm_osd_size {


		ra_add_native_resource(ctx->ra, "drm_params", &p->drm_params);
		ra_add_native_resource(ctx->ra, "drm_osd_size", &p->osd_size);

Various DRM Atomic improvements #5714

Various DRM Atomic improvements #5714

Conversation

LongChair commented Apr 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xantoz Apr 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongChair commented Apr 8, 2018

xantoz Apr 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xantoz Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

LongChair commented Apr 10, 2018

xantoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongChair commented Apr 10, 2018

xantoz commented Apr 10, 2018 • edited Loading

LongChair commented Apr 14, 2018

LongChair commented Apr 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongChair Apr 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xantoz commented Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xantoz Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xantoz commented Apr 16, 2018 • edited Loading

xantoz Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xantoz commented Apr 18, 2018

xantoz commented Apr 24, 2018 • edited Loading

LongChair commented Apr 29, 2018

xantoz commented Apr 30, 2018

LongChair commented Apr 30, 2018

ghost commented Apr 30, 2018

jeeb left a comment

Choose a reason for hiding this comment

jeeb commented May 1, 2018

xantoz Apr 7, 2018 •

edited

Loading

xantoz Apr 8, 2018 •

edited

Loading

xantoz Apr 9, 2018 •

edited

Loading

xantoz commented Apr 10, 2018 •

edited

Loading

LongChair Apr 15, 2018 •

edited

Loading

xantoz commented Apr 16, 2018 •

edited

Loading

xantoz Apr 16, 2018 •

edited

Loading

xantoz commented Apr 16, 2018 •

edited

Loading

xantoz Apr 17, 2018 •

edited

Loading

xantoz commented Apr 24, 2018 •

edited

Loading