[Mesa-dev] [PATCH] st/omx: straighten get/put_screen

[Liu, Leo]

>-----Original Message-----
>From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>Sent: Monday, November 09, 2015 8:17 AM
>To: mesa-dev at lists.freedesktop.org
>Cc: emil.l.velikov at gmail.com; Liu, Leo
>Subject: [PATCH] st/omx: straighten get/put_screen
>
>The current code is busted in a number of ways.
>
> - initially checks for omx_display (rather than omx_screen), which may or may
>not be around.
> - blindly feeds the empty env variable string to loader_open_device()

Should be treating empty string as one of incorrect string, which result in failing open.
Because user's intention is to use to env, if they put the empty string, here should be getting failed.

Either way:
Reviewed-by: Leo Liu <leo.liu at amd.com>

Regards,
Leo

> - reads the env variable every time get_screen is called
> - the latter manifests into memory leaks, and other issues as one sets the
>variable between two get_screen calls.
>
>Additionally it cleans up a couple of extra bits
> - drops unneeded set/check of omx_display.
> - make the teardown (put_screen) order was not symmetrical to the setup
>(get_screen)
>
>Cc: Leo Liu <leo.liu at amd.com>
>Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>---
>
>Leo, feel free to point out if I'm loosing the plot and some of these don't hold
>true.
>
>-Emil
>
> src/gallium/state_trackers/omx/entrypoint.c | 35 ++++++++++++++++-------------
> 1 file changed, 19 insertions(+), 16 deletions(-)
>
>diff --git a/src/gallium/state_trackers/omx/entrypoint.c
>b/src/gallium/state_trackers/omx/entrypoint.c
>index 7df90b1..f99a620 100644
>--- a/src/gallium/state_trackers/omx/entrypoint.c
>+++ b/src/gallium/state_trackers/omx/entrypoint.c
>@@ -33,6 +33,7 @@
>
> #include <assert.h>
> #include <string.h>
>+#include <stdbool.h>
>
> #include <X11/Xlib.h>
>
>@@ -73,28 +74,32 @@ int
>omx_component_library_Setup(stLoaderComponentType **stComponents)
>
> struct vl_screen *omx_get_screen(void)
> {
>+   static bool first_time = true;
>    pipe_mutex_lock(omx_lock);
>
>-   if (!omx_display) {
>-      omx_render_node = debug_get_option("OMX_RENDER_NODE", NULL);
>-      if (!omx_render_node) {
>-         omx_display = XOpenDisplay(NULL);
>-         if (!omx_display)
>-            goto error;
>-      }
>-   }
>-
>    if (!omx_screen) {
>+      if (first_time) {
>+         omx_render_node = debug_get_option("OMX_RENDER_NODE", NULL);
>+         if (omx_render_node && *omx_render_node == '\0')
>+            omx_render_node = NULL;
>+
>+         first_time = false;
>+      }
>       if (omx_render_node) {
>          drm_fd = loader_open_device(omx_render_node);
>          if (drm_fd < 0)
>             goto error;
>+
>          omx_screen = vl_drm_screen_create(drm_fd);
>          if (!omx_screen) {
>             close(drm_fd);
>             goto error;
>          }
>       } else {
>+         omx_display = XOpenDisplay(NULL);
>+         if (!omx_display)
>+            goto error;
>+
>          omx_screen = vl_screen_create(omx_display, 0);
>          if (!omx_screen) {
>             XCloseDisplay(omx_display); @@ -117,16 +122,14 @@ void
>omx_put_screen(void)  {
>    pipe_mutex_lock(omx_lock);
>    if ((--omx_usecount) == 0) {
>-      if (!omx_render_node) {
>-         vl_screen_destroy(omx_screen);
>-         if (omx_display)
>-            XCloseDisplay(omx_display);
>-      } else {
>-         close(drm_fd);
>+      if (omx_render_node) {
>          vl_drm_screen_destroy(omx_screen);
>+         close(drm_fd);
>+      } else {
>+         vl_screen_destroy(omx_screen);
>+         XCloseDisplay(omx_display);
>       }
>       omx_screen = NULL;
>-      omx_display = NULL;
>    }
>    pipe_mutex_unlock(omx_lock);
> }
>--
>2.6.2