[FFmpeg-devel] [PATCH] lavu/hwcontext_vaapi: Skip 'vgem' driver

[FFmpeg-devel] [PATCH] lavu/hwcontext_vaapi: Skip 'vgem' driver

[Brian Norris]

There can be more than one available render node, and it's not
guaranteed the first node we come across is the correct one. In
particular, 'vgem' devices are common, and are
never VAAPI-enabled and thus not valid here.

We have a 'kernel_driver' arg already for specifying a single driver we
*do* want, but it doesn't support a negation, nor a list. It's easier
just to automatically skip 'vgem' anyway, to avoid foisting this burden
on users.

This has precedent in libva-utils already:

  bfb6b98ed62a exclude vgem node and invalid drm node in vainfo
  https://github.com/intel/libva-utils/commit/bfb6b98ed62ac14a840ba62639ab902a23912258

Signed-off-by: Brian Norris <briannorris@chromium.org>
---

 libavutil/hwcontext_vaapi.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
index 4bcde74f6212..84b03b633251 100644
--- a/libavutil/hwcontext_vaapi.c
+++ b/libavutil/hwcontext_vaapi.c
@@ -1700,6 +1700,7 @@ static int vaapi_device_create(AVHWDeviceContext *ctx, const char *device,
             char path[64];
             int n, max_devices = 8;
 #if CONFIG_LIBDRM
+            drmVersion *info;
             const AVDictionaryEntry *kernel_driver;
             kernel_driver = av_dict_get(opts, "kernel_driver", NULL, 0);
 #endif
@@ -1713,9 +1714,15 @@ static int vaapi_device_create(AVHWDeviceContext *ctx, const char *device,
                     break;
                 }
 #if CONFIG_LIBDRM
+                info = drmGetVersion(priv->drm_fd);
+                if (!info) {
+                    av_log(ctx, AV_LOG_VERBOSE,
+                           "Failed to get DRM version for device %d.\n", n);
+                    close(priv->drm_fd);
+                    priv->drm_fd = -1;
+                    continue;
+                }
                 if (kernel_driver) {
-                    drmVersion *info;
-                    info = drmGetVersion(priv->drm_fd);
                     if (strcmp(kernel_driver->value, info->name)) {
                         av_log(ctx, AV_LOG_VERBOSE, "Ignoring device %d "
                                "with non-matching kernel driver (%s).\n",
@@ -1730,12 +1737,19 @@ static int vaapi_device_create(AVHWDeviceContext *ctx, const char *device,
                            "with matching kernel driver (%s).\n",
                            n, info->name);
                     drmFreeVersion(info);
-                } else
-#endif
-                {
-                    av_log(ctx, AV_LOG_VERBOSE, "Trying to use "
-                           "DRM render node for device %d.\n", n);
+                    break;
+                } else if (!strncmp(info->name, "vgem", 4)) {
+                    av_log(ctx, AV_LOG_VERBOSE,
+                           "Skipping vgem node for device %d.\n", n);
+                    drmFreeVersion(info);
+                    close(priv->drm_fd);
+                    priv->drm_fd = -1;
+                    continue;
                 }
+                drmFreeVersion(info);
+#endif
+                av_log(ctx, AV_LOG_VERBOSE, "Trying to use "
+                       "DRM render node for device %d.\n", n);
                 break;
             }
             if (n >= max_devices)
-- 
2.38.1.584.g0f3c55d4c2-goog

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavu/hwcontext_vaapi: Skip 'vgem' driver

[Anton Khirnov]

Quoting Brian Norris (2022-11-23 22:54:44)
> There can be more than one available render node, and it's not
> guaranteed the first node we come across is the correct one. In
> particular, 'vgem' devices are common, and are
> never VAAPI-enabled and thus not valid here.
> 
> We have a 'kernel_driver' arg already for specifying a single driver we
> *do* want, but it doesn't support a negation, nor a list. It's easier
> just to automatically skip 'vgem' anyway, to avoid foisting this burden
> on users.
> 
> This has precedent in libva-utils already:
> 
>   bfb6b98ed62a exclude vgem node and invalid drm node in vainfo
>   https://github.com/intel/libva-utils/commit/bfb6b98ed62ac14a840ba62639ab902a23912258
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---

I'm not the maintainer for this, but I wonder if it wouldn't be cleaner
to instead add something like a 'kernel_driver_blacklist' option that
would contain a comma-separated list of ignored drivers, and default to
'vgem' (with maybe more added later).

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavu/hwcontext_vaapi: Skip 'vgem' driver

[Brian Norris]

Hi Anton,

On Tue, Nov 29, 2022 at 12:25:11PM +0100, Anton Khirnov wrote:
> I'm not the maintainer for this, but I wonder if it wouldn't be cleaner
> to instead add something like a 'kernel_driver_blacklist' option that
> would contain a comma-separated list of ignored drivers, and default to
> 'vgem' (with maybe more added later).

I could go for a list of some kind to make this more extensible, if it's
worthwhile. But is it important to make it an "option" (as in, CLI
option presumably)? Comma-separated is especially tricky, as I believe
the existing dict/arg parsing uses commas to delimit separate key/value
pairs. I suppose I could use "+", as there's precedent in vulkan
(instance_extensions, etc.).

It also feels like "user-configurable" and "default list" are somewhat
at odds, because the default list has good reason to be there, and yet
might change underneath the user across versions (if there are new
vgem-like drivers, etc.). So any CLI user would have to keep up to date
with the defaults. I suppose maybe that's not a horrible problem; I'm
just trying to think it through, as I wouldn't personally have any need
for it if the default list is good.

Thanks for your thoughts.

Brian
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavu/hwcontext_vaapi: Skip 'vgem' driver

[Xiang, Haihao]

On Wed, 2022-11-23 at 13:54 -0800, Brian Norris wrote:
> There can be more than one available render node, and it's not
> guaranteed the first node we come across is the correct one. In
> particular, 'vgem' devices are common, and are
> never VAAPI-enabled and thus not valid here.
> 
> We have a 'kernel_driver' arg already for specifying a single driver we
> *do* want, but it doesn't support a negation, nor a list. It's easier
> just to automatically skip 'vgem' anyway, to avoid foisting this burden
> on users.
> 
> This has precedent in libva-utils already:
> 
>   bfb6b98ed62a exclude vgem node and invalid drm node in vainfo
>   
> https://github.com/intel/libva-utils/commit/bfb6b98ed62ac14a840ba62639ab902a23912258
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> 
>  libavutil/hwcontext_vaapi.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
> index 4bcde74f6212..84b03b633251 100644
> --- a/libavutil/hwcontext_vaapi.c
> +++ b/libavutil/hwcontext_vaapi.c
> @@ -1700,6 +1700,7 @@ static int vaapi_device_create(AVHWDeviceContext *ctx,
> const char *device,
>              char path[64];
>              int n, max_devices = 8;
>  #if CONFIG_LIBDRM
> +            drmVersion *info;
>              const AVDictionaryEntry *kernel_driver;
>              kernel_driver = av_dict_get(opts, "kernel_driver", NULL, 0);
>  #endif
> @@ -1713,9 +1714,15 @@ static int vaapi_device_create(AVHWDeviceContext *ctx,
> const char *device,
>                      break;
>                  }
>  #if CONFIG_LIBDRM
> +                info = drmGetVersion(priv->drm_fd);
> +                if (!info) {
> +                    av_log(ctx, AV_LOG_VERBOSE,
> +                           "Failed to get DRM version for device %d.\n", n);
> +                    close(priv->drm_fd);
> +                    priv->drm_fd = -1;
> +                    continue;
> +                }
>                  if (kernel_driver) {
> -                    drmVersion *info;
> -                    info = drmGetVersion(priv->drm_fd);
>                      if (strcmp(kernel_driver->value, info->name)) {
>                          av_log(ctx, AV_LOG_VERBOSE, "Ignoring device %d "
>                                 "with non-matching kernel driver (%s).\n",
> @@ -1730,12 +1737,19 @@ static int vaapi_device_create(AVHWDeviceContext *ctx,
> const char *device,
>                             "with matching kernel driver (%s).\n",
>                             n, info->name);
>                      drmFreeVersion(info);
> -                } else
> -#endif
> -                {
> -                    av_log(ctx, AV_LOG_VERBOSE, "Trying to use "
> -                           "DRM render node for device %d.\n", n);
> +                    break;
> +                } else if (!strncmp(info->name, "vgem", 4)) {

The virtual GEM device is vgem, so better to use strcmp here. 

Thanks
Haihao

> +                    av_log(ctx, AV_LOG_VERBOSE,
> +                           "Skipping vgem node for device %d.\n", n);
> +                    drmFreeVersion(info);
> +                    close(priv->drm_fd);
> +                    priv->drm_fd = -1;
> +                    continue;
>                  }
> +                drmFreeVersion(info);
> +#endif
> +                av_log(ctx, AV_LOG_VERBOSE, "Trying to use "
> +                       "DRM render node for device %d.\n", n);
>                  break;
>              }
>              if (n >= max_devices)
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavu/hwcontext_vaapi: Skip 'vgem' driver

[Brian Norris]

On Mon, Dec 05, 2022 at 04:37:29AM +0000, Xiang, Haihao wrote:
> On Wed, 2022-11-23 at 13:54 -0800, Brian Norris wrote:
> > +                } else if (!strncmp(info->name, "vgem", 4)) {
> 
> The virtual GEM device is vgem, so better to use strcmp here. 

Good point, we probably shouldn't be matching "vgem-foo" driver here.
And drmGetVersion() ensures info->name is 0-terminated, so we're not
gaining any safety by providing bounds.

I think I was needlessly matching libva-utils too closely, where they do
the same strncmp().

v2 incoming.

Brian
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".