After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 796516 - kms: add support for kms atomic api
kms: add support for kms atomic api
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-06-06 17:03 UTC by Randy Li (ayaka)
Modified: 2018-11-03 14:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
A early version (7.46 KB, patch)
2018-06-06 17:03 UTC, Randy Li (ayaka)
none Details | Review
Move can sacle checking to a early place (15.90 KB, patch)
2018-06-08 08:58 UTC, Randy Li (ayaka)
none Details | Review
Only add quard for drm properties (16.51 KB, patch)
2018-06-11 09:09 UTC, Randy Li (ayaka)
none Details | Review
Only add quarks for drm properties (18.67 KB, patch)
2018-06-11 10:23 UTC, Randy Li (ayaka)
none Details | Review
Add atromic modeset (21.81 KB, patch)
2018-06-11 17:44 UTC, Randy Li (ayaka)
none Details | Review
All the basic features are done (22.66 KB, patch)
2018-06-18 01:12 UTC, Randy Li (ayaka)
none Details | Review
kms atomic support except page flip (22.63 KB, patch)
2018-06-21 17:30 UTC, Randy Li (ayaka)
none Details | Review
try to add page flip (4.62 KB, patch)
2018-06-21 17:35 UTC, Randy Li (ayaka)
none Details | Review
Both page flip and sync commit should work (25.78 KB, patch)
2018-07-04 03:01 UTC, Randy Li (ayaka)
none Details | Review
Update patch and store more information (36.04 KB, patch)
2018-10-17 11:48 UTC, Randy Li (ayaka)
none Details | Review

Description Randy Li (ayaka) 2018-06-06 17:03:13 UTC
Created attachment 372577 [details] [review]
A early version

This patch only add a basic flow for atomic API.
I have not done yet.
Except removing kms sync mechanism there is not much different to legacy API.
I am planning to install some kms properties to gobject like v4l2 does. Also GstVideoAggregator is a important feature that would be introduced, as kms atomic api supports commit multiple plane at the same time. With the helper of that, it is possible to show both video and video subtitle with this plugin.
Comment 1 Nicolas Dufresne (ndufresne) 2018-06-06 17:34:13 UTC
Review of attachment 372577 [details] [review]:

Nice start, it proves it won't be too complicated. We will also be able to probe with TryCommit to set can-scale properly and earlier.

::: sys/kms/gstkmssink.c
@@ +77,3 @@
+    drmModeObjectProperties *props;
+    drmModePropertyRes **props_info;
+  } conn;

Unnamed Structure is C11 feature, I like it, but I'd like to know if that's a way we want to go globally in GStreamer project.

@@ +94,3 @@
 #define parent_class gst_kms_sink_parent_class
 G_DEFINE_TYPE_WITH_CODE (GstKMSSink, gst_kms_sink, GST_TYPE_VIDEO_SINK,
+    G_ADD_PRIVATE (GstKMSSink);

The entire GstKMSSink class is private (only the plugin is exposed), no need for private structure.

@@ +715,3 @@
   }
 
+  drmSetClientCap (self->fd, DRM_CLIENT_CAP_ATOMIC, 1);

This will need a return check to detect non-atomic drivers.

@@ +1586,3 @@
+  req = drmModeAtomicAlloc ();
+
+  add_plane_property (self->priv, req, self->plane_id, "FB_ID", fb_id);

You could maybe just pass self in order to reduce the length of these lines ?
Comment 2 Randy Li (ayaka) 2018-06-07 01:20:10 UTC
(In reply to Nicolas Dufresne (ndufresne) from comment #1)
> Review of attachment 372577 [details] [review] [review]:
> 
> Nice start, it proves it won't be too complicated. We will also be able to
> probe with TryCommit to set can-scale properly and earlier.
> 
yes, I will some addition work on kms_sink_start() as well to store all the caps info of all the plane, then it would be possible to choose a proper drm plane for future processing.
> ::: sys/kms/gstkmssink.c
> @@ +77,3 @@
> +    drmModeObjectProperties *props;
> +    drmModePropertyRes **props_info;
> +  } conn;
> 
> Unnamed Structure is C11 feature, I like it, but I'd like to know if that's
> a way we want to go globally in GStreamer project.
> 
It is supported from gcc 4.x, since the kms can't be used beyond the Linux, I think it is a good way to do that.
> @@ +94,3 @@
>  #define parent_class gst_kms_sink_parent_class
>  G_DEFINE_TYPE_WITH_CODE (GstKMSSink, gst_kms_sink, GST_TYPE_VIDEO_SINK,
> +    G_ADD_PRIVATE (GstKMSSink);
> 
> The entire GstKMSSink class is private (only the plugin is exposed), no need
> for private structure.
I know, but the problem is I don't want to include those drm headers in gstkmssink.h and some others header for it. Maybe I can declear an structure in the header files, but I think use the private structure is a better choice.
> 
> @@ +715,3 @@
>    }
>  
> +  drmSetClientCap (self->fd, DRM_CLIENT_CAP_ATOMIC, 1);
Yes, I am thinking to move it to get_drm_caps()
> 
> This will need a return check to detect non-atomic drivers.
> 
> @@ +1586,3 @@
> +  req = drmModeAtomicAlloc ();
> +
> +  add_plane_property (self->priv, req, self->plane_id, "FB_ID", fb_id);
> 
> You could maybe just pass self in order to reduce the length of these lines ?
Comment 3 Víctor Manuel Jáquez Leal 2018-06-08 07:53:13 UTC
Review of attachment 372577 [details] [review]:

::: sys/kms/gstkmssink.c
@@ +117,3 @@
 
+static gint
+add_connector_property (GstKMSSinkPrivate * priv, drmModeAtomicReq * req,

I don't see this function being used.

@@ +140,3 @@
+static gint
+add_crtc_property (GstKMSSinkPrivate * priv, drmModeAtomicReq * req,
+    uint32_t obj_id, const gchar * name, uint64_t value)

ditto

@@ +146,3 @@
+
+  for (i = 0; i < priv->crtc.props->count_props; i++) {
+    if (strcmp (priv->crtc.props_info[i]->name, name) == 0) {

afaik, string comparison is expensive. Isn't other way to match these properties?

@@ -1500,3 @@
-  if (!gst_kms_sink_sync (self)) {
-    GST_OBJECT_UNLOCK (self);
-    goto bail;

we should refactor this to enable both behavior: for non-atomic sync an the atomic commit.
Comment 4 Randy Li (ayaka) 2018-06-08 08:58:50 UTC
Created attachment 372604 [details] [review]
Move can sacle checking to a early place

I check the kmscube to do drm modeset, it is very complex, and it must be merged into show frame stage, I wonder whether it is necessary to use the new atomic api or legacy api would work.
The other important basic feature is the frame sync, the current commit flag doesn't bring any benefit than before. Also I have no idea how does the page flip on the atomic. The vblank may be implicit at atomic api, I remember I read that in a document.
Both ARM Mali on Linux(Android does) and the drm driver of the rockchip doesn't support page flip, I may need to update the drm driver first or I can't verify it.
Comment 5 Nicolas Dufresne (ndufresne) 2018-06-08 19:43:38 UTC
Review of attachment 372604 [details] [review]:

::: sys/kms/gstkmssink.c
@@ +75,3 @@
+
+#define fill_device_connector(self)                                     \
+        fill_drm_properties(self, conn, CONNECTOR)

I think it's better to use inline then macros. And I don't really like macros that uses values from the upper context.
Comment 6 Randy Li (ayaka) 2018-06-11 09:09:34 UTC
Created attachment 372631 [details] [review]
Only add quard for drm properties

I didn't make any progress on the TODO list.
Comment 7 Randy Li (ayaka) 2018-06-11 10:23:35 UTC
Created attachment 372634 [details] [review]
Only add quarks for drm properties

The previous skip the fixup for scaling, the mode settings is not done yet, it may be moved to show_frame.
Comment 8 Randy Li (ayaka) 2018-06-11 17:44:51 UTC
Created attachment 372641 [details] [review]
Add atromic modeset

I try, but it doesn't work. No error report, I will leave this problem and move forward to page flip, I read the weston, it looks there is nothing special in page flip at drm atomic, it doesn't need a callback function.
Comment 9 Randy Li (ayaka) 2018-06-18 01:12:58 UTC
Created attachment 372708 [details] [review]
All the basic features are done

I have verified the modeseting, but I don't know whether I use the correct way to do that.
I am not sure about the page flip part, I read the weston code, but I am not sure the correct way or be necessary to implement the page flip callback as it does in legacy API. I don't have a device with a driver support page flip now, so it is not a good time for me to verify it.
I will move forward to the multiple sink ports work.
Comment 10 Randy Li (ayaka) 2018-06-21 17:30:13 UTC
Created attachment 372747 [details] [review]
kms atomic support except page flip
Comment 11 Randy Li (ayaka) 2018-06-21 17:35:46 UTC
Created attachment 372748 [details] [review]
try to add page flip

I found the page flip doesn't want as I though in the drm atomic API.
I wish to push a bunch of atomic request to drm system, then driver would decide which frame can be displayed or dropped.
But I can't get the event from those dropped frame, so I don't have a chance to unreference them, making a big problem in buffer management.
And I meet serious screen stretched, frame dropped and screen frozen temporarily during playback with that.
May I should just wait the getting the event of the previous frame, then display the current frame?
Comment 12 Nicolas Dufresne (ndufresne) 2018-06-21 17:44:39 UTC
No, drm accepts only one request at a time. Queuing and dropping needs to be handled by userspace.
Comment 13 Randy Li (ayaka) 2018-07-04 03:01:03 UTC
Created attachment 372945 [details] [review]
Both page flip and sync commit should work

I forgot to upload this patch.
I wasted time on merging kms legacy and atomic in the same plugin, but I found I would export many functions or create two plugins.
The work flow is quite different.
Comment 14 Nicolas Dufresne (ndufresne) 2018-07-12 02:18:03 UTC
Just for your interest, here's a dual legacy/atomic implementation:

https://github.com/mpv-player/mpv/blob/master/video/out/opengl/hwdec_drmprime_drm.c
Comment 15 Randy Li (ayaka) 2018-07-12 02:28:17 UTC
So we don't need three files but just put them into the same file?
If you want this design, I can refresh a patch for that.
Comment 16 Nicolas Dufresne (ndufresne) 2018-07-12 03:08:35 UTC
Does the number of files really matter ? It has to be clean and readable. I've posted that one, because it's clean.
Comment 17 Nicolas Dufresne (ndufresne) 2018-07-14 01:29:35 UTC
Review of attachment 372945 [details] [review]:

::: configure.ac
@@ +1537,3 @@
 AG_GST_CHECK_FEATURE(KMS, [drm/kms libraries], kms, [
   AG_GST_PKG_CHECK_MODULES(GST_ALLOCATORS, gstreamer-allocators-1.0)
+  PKG_CHECK_MODULES([KMS_DRM], [libdrm >= 2.4.78], HAVE_KMS=yes, HAVE_KMS=no)

Update the meson file too please.

::: sys/kms/gstkmssink.c
@@ +62,3 @@
+#define GST_PLUGIN_DESC "Video sink using the Linux kernel drm atomic API"
+
+#define cache_drm_properties(TYPE, filed, self)                         \

This macro and the following, including all the following helpers Should be move to their own C file. This macro should be turned into function.

@@ +219,3 @@
 
+static void
+destroy_drm_prop (gpointer data)

drm_prop_free ?

@@ +225,3 @@
+
+static gboolean
+cache_conn_properities (GstKMSSink * self)

cache_connector_properties(), full name + typo.

@@ +229,3 @@
+  gboolean ret = TRUE;
+  cache_drm_properties (CONNECTOR, conn, self);
+  return ret;

What's the point of ret here ?

@@ +237,3 @@
+  gboolean ret = TRUE;
+  cache_drm_properties (CRTC, crtc, self);
+  return ret;

Same.

@@ +244,3 @@
+{
+  gboolean ret = TRUE;
+  cache_drm_properties (PLANE, plane, self);

You have a wrapper, so it's not much to access self->pane_* once.

@@ +253,3 @@
+{
+  gint ret = 0;
+  add_drm_property (conn, self, req, name, value);

Same for conn.

@@ +262,3 @@
+{
+  gint ret = 0;
+  add_drm_property (crtc, self, req, name, value);

Same for crtc.

@@ +271,3 @@
+{
+  gint ret = 0;
+  add_drm_property (plane, self, req, name, value);

Same for plane.

@@ +547,3 @@
     goto mode_failed;
 
+  err = drmModeCreatePropertyBlob (self->fd, mode, sizeof (*mode), &blob_id);

Please don't drop legacy APIs, remember if you are atomic or not and choose the right path.

@@ +713,3 @@
+
+again:
+  ret = gst_poll_wait (self->poll, 3 * GST_SECOND);

If this poll is properly unlocked by unlock() virtual, no need for this timeout.

@@ +1077,3 @@
 
+static gboolean
+gst_kms_sink_check_scale (GstKMSSink * self, GstVideoInfo * const vinfo)

All this is not new code, you should consider refactoring patches prior to this one.
Comment 18 Randy Li (ayaka) 2018-07-16 16:28:25 UTC
(In reply to Nicolas Dufresne (ndufresne) from comment #17)
> Review of attachment 372945 [details] [review] [review]:
> 
> ::: configure.ac
> @@ +1537,3 @@
>  AG_GST_CHECK_FEATURE(KMS, [drm/kms libraries], kms, [
>    AG_GST_PKG_CHECK_MODULES(GST_ALLOCATORS, gstreamer-allocators-1.0)
> +  PKG_CHECK_MODULES([KMS_DRM], [libdrm >= 2.4.78], HAVE_KMS=yes,
> HAVE_KMS=no)
> 
> Update the meson file too please.
ACK
> 
> ::: sys/kms/gstkmssink.c
> @@ +62,3 @@
> +#define GST_PLUGIN_DESC "Video sink using the Linux kernel drm atomic API"
> +
> +#define cache_drm_properties(TYPE, filed, self)                         \
> 
> This macro and the following, including all the following helpers Should be
> move to their own C file. This macro should be turned into function.
> 
I see, gstkmsutil.c would be a good place.
> @@ +219,3 @@
>  
> +static void
> +destroy_drm_prop (gpointer data)
> 
> drm_prop_free ?
> 
I don't want to its name looks similar to the one in libdrm.
> @@ +225,3 @@
> +
> +static gboolean
> +cache_conn_properities (GstKMSSink * self)
> 
> cache_connector_properties(), full name + typo.
I would looks too long, but I will take it.
> 
> @@ +229,3 @@
> +  gboolean ret = TRUE;
> +  cache_drm_properties (CONNECTOR, conn, self);
> +  return ret;
> 
> What's the point of ret here ?
In the above macro.
> 
> @@ +237,3 @@
> +  gboolean ret = TRUE;
> +  cache_drm_properties (CRTC, crtc, self);
> +  return ret;
> 
> Same.
> 
> @@ +244,3 @@
> +{
> +  gboolean ret = TRUE;
> +  cache_drm_properties (PLANE, plane, self);
> 
> You have a wrapper, so it's not much to access self->pane_* once.
We won't how many planes are in current device, so I don't think we would create
such a properties.
> 
> @@ +253,3 @@
> +{
> +  gint ret = 0;
> +  add_drm_property (conn, self, req, name, value);
> 
> Same for conn.
> 
> @@ +262,3 @@
> +{
> +  gint ret = 0;
> +  add_drm_property (crtc, self, req, name, value);
> 
> Same for crtc.
> 
> @@ +271,3 @@
> +{
> +  gint ret = 0;
> +  add_drm_property (plane, self, req, name, value);
> 
> Same for plane.
> 
> @@ +547,3 @@
>      goto mode_failed;
>  
> +  err = drmModeCreatePropertyBlob (self->fd, mode, sizeof (*mode),
> &blob_id);
> 
> Please don't drop legacy APIs, remember if you are atomic or not and choose
> the right path.
> 
I think using atomic would be much quick and avoiding bugs.
> @@ +713,3 @@
> +
> +again:
> +  ret = gst_poll_wait (self->poll, 3 * GST_SECOND);
> 
> If this poll is properly unlocked by unlock() virtual, no need for this
> timeout.
> 
ack
> @@ +1077,3 @@
>  
> +static gboolean
> +gst_kms_sink_check_scale (GstKMSSink * self, GstVideoInfo * const vinfo)
> 
> All this is not new code, you should consider refactoring patches prior to
> this one.
Sorry, I don't know what I should do?(English problem)
Comment 19 Randy Li (ayaka) 2018-10-17 11:48:41 UTC
Created attachment 373949 [details] [review]
Update  patch and store more information
Comment 20 GStreamer system administrator 2018-11-03 14:25:15 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/726.