Created attachment 370340 [details] [review]
Initial patch to fix the issue on ARM

Review of attachment 370340 [details] [review]:

Thanks for the patch.

Just a couple nitpicks.

::: gst-libs/gst/vaapi/gstvaapidisplay_drm.c
@@ +118,3 @@
       }
 
+

nitpick: I guess this extra line is not part of the patch

@@ +210,1 @@
 

Another nitpick: this block could be more compact if

if (strncmp (busid, "pci:", 4) == 0)
  busid += 4;
else if (strncmp (busid, "platform:", 9) == 0)
  busid += 9;
else
  goto end;

busid_length = strlen (busid)

But if you add a static structure with a loop, would be much more neat :)

I didn't know that there is ARM device using gstreamer-vaapi!

@Paul:
I'm interested to know more about the use cases, platform or any details (if you can share) :)

(In reply to Víctor Manuel Jáquez Leal from comment #2)
> Review of attachment 370340 [details] [review] [review]:
> 
> Thanks for the patch.
> 
> Just a couple nitpicks.
> 
> ::: gst-libs/gst/vaapi/gstvaapidisplay_drm.c
> @@ +118,3 @@
>        }
>  
> +
> 
> nitpick: I guess this extra line is not part of the patch

Good catch, I'll remove it in the next version.

> @@ +210,1 @@
>  
> 
> Another nitpick: this block could be more compact if
> 
> if (strncmp (busid, "pci:", 4) == 0)
>   busid += 4;
> else if (strncmp (busid, "platform:", 9) == 0)
>   busid += 9;
> else
>   goto end;
> 
> busid_length = strlen (busid)
> 
> But if you add a static structure with a loop, would be much more neat :)

I'll probably go with a structure/loop approach so that new subsystems can be added more easily, thanks for the suggestion!

(In reply to sreerenj from comment #3)
> I didn't know that there is ARM device using gstreamer-vaapi!
> 
> @Paul:
> I'm interested to know more about the use cases, platform or any details (if
> you can share) :)

The big picture here is that I'm adding support for the VPU found in Allwinner platforms in mainline Linux. You can find details about the effort here: https://linux-sunxi.org/Sunxi-Cedrus

Sofar, we have a libva implementation that handles the sunxi-cedrus mem2mem V4L2 driver (that was submitted to linux-media, but is still in early stages). Since our VPU is stateless, we have to rely on the Request API to synchronize metadata with frame data, that is work in progress.

At this point, I'm hitting a segfault related to memory management (looks like a double-free but I'm still debugging it). Note that I'm paulk-gagarine on #gstreamer @freenode, so feel free to ping me if you'd like to chat about this!

Created attachment 370555 [details] [review]
Version 2 of the patch to fix the issue on ARM

Here is a new version with the requested changes.

Review of attachment 370555 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapidisplay_drm.c
@@ +79,3 @@
   GstVaapiDisplayDRMPrivate *const priv =
       GST_VAAPI_DISPLAY_DRM_PRIVATE (display);
+  const gchar *allowed_subsystems[] = { "pci", "platform", NULL };

would be possible unify this constant array into one, global for this file?

Created attachment 370599 [details] [review]
Version 3 of the patch to fix the issue on ARM

Thanks for the review! Please consider this version based on your suggestions.

Review of attachment 370599 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapidisplay_drm.c
@@ +210,3 @@
+
+    if (strncmp (busid, allowed_subsystems[i], busid_length) == 0) {
+      busid += busid_length;

Aren't you forgetting to consider the ':' in the busid??

then it would be

/* add the trailing ':' char */
busid += busid_length + 1;

attachment 370599 [details] [review] pushed as commit 5ee46f67 - display: drm: Allow finding DRM paths out of the PCI subsystem

Hum, looks like I fell behind on this one. Sorry about that and thank-you for applying the change you suggested!