[Bug 796516] kms: add support for kms atomic api

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=796516

--- Comment #18 from Randy Li (ayaka) <ayaka at soulik.info> ---
(In reply to Nicolas Dufresne (ndufresne) from comment #17)
> 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.
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)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.