[Mesa-dev] [PATCH v2 4/4] st/omx: add headless support

[Emil Velikov]

Hi Leo,

I've suggested a few things before, yet I might have been too subtle.
Let me try again.

On 6 November 2015 at 16:40, Leo Liu <leo.liu at amd.com> wrote:
> This will allow dec/enc/transcode without X
Please add "running" in the above sentence "... without running X" or similar.

>
> v2:  -use env override even with X,
>      -use loader_open_device instead of open
>
> Signed-off-by: Leo Liu <leo.liu at amd.com>
> Reviewed-by: Christian König <christian.koenig at amd.com>
> ---
>  src/gallium/state_trackers/omx/entrypoint.c | 37 ++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/src/gallium/state_trackers/omx/entrypoint.c b/src/gallium/state_trackers/omx/entrypoint.c
> index a765666..2ddb1aa 100644
> --- a/src/gallium/state_trackers/omx/entrypoint.c
> +++ b/src/gallium/state_trackers/omx/entrypoint.c
> @@ -33,11 +33,13 @@
>
>  #include <assert.h>
>  #include <string.h>
> +#include <fcntl.h>
>
With the loader.h in place you shouldn't need this.

>  #include <X11/Xlib.h>
>
>  #include "os/os_thread.h"
>  #include "util/u_memory.h"
> +#include "loader/loader.h"
>
>  #include "entrypoint.h"
>  #include "vid_dec.h"
> @@ -47,6 +49,7 @@ pipe_static_mutex(omx_lock);
>  static Display *omx_display = NULL;
>  static struct vl_screen *omx_screen = NULL;
>  static unsigned omx_usecount = 0;
> +static const char *omx_render_node = NULL;
>
>  int omx_component_library_Setup(stLoaderComponentType **stComponents)
>  {
> @@ -73,33 +76,49 @@ struct vl_screen *omx_get_screen(void)
>     pipe_mutex_lock(omx_lock);
>
>     if (!omx_display) {
> +      omx_render_node = debug_get_option("OMX_RENDER_NODES", NULL);
>        omx_display = XOpenDisplay(NULL);
You don't need/want to do this, if the variable is set. Environment
variables tend to override default behaviour.
Although as getenv() can return empty string, imho one can just ignore
the variable in that case.

>        if (!omx_display) {
> -         pipe_mutex_unlock(omx_lock);
> -         return NULL;
> +         if (!omx_render_node)
> +            goto error;
>        }
>     }
>
>     if (!omx_screen) {
> -      omx_screen = vl_screen_create(omx_display, 0);
> -      if (!omx_screen) {
> -         pipe_mutex_unlock(omx_lock);
> -         return NULL;
> -      }
> +      if (omx_render_node) {
> +         int fd = loader_open_device(omx_render_node);
> +         if (fd < 0)
> +            goto error;
> +         omx_screen = vl_drm_screen_create(fd);
> +      } else
> +         omx_screen = vl_screen_create(omx_display, 0);
> +
> +      if (!omx_screen)
> +         goto error;
>     }
>
>     ++omx_usecount;
>
>     pipe_mutex_unlock(omx_lock);
>     return omx_screen;
> +
> +error:
> +   if (omx_display)
> +      XCloseDisplay(omx_display);
Missing close(fd) ?

> +   pipe_mutex_unlock(omx_lock);
> +   return NULL;
>  }
>
>  void omx_put_screen(void)
>  {
>     pipe_mutex_lock(omx_lock);
>     if ((--omx_usecount) == 0) {
> -      vl_screen_destroy(omx_screen);
> -      XCloseDisplay(omx_display);
> +      if (!omx_render_node) {
> +         vl_screen_destroy(omx_screen);
> +         if (omx_display)
> +            XCloseDisplay(omx_display);
> +      } else
> +         vl_drm_screen_destroy(omx_screen);
Missing close(fd) ? Which reminds me that patch 2/4 needs an update.

Regards,
Emil