On 15.07.2015 07:15, Mario Kleiner wrote:
The amdgpu_device for a device node needs its own dup'ed fd, instead
of using the original fd passed in for a screen, to make multi-x-screen
ZaphodHeads configurations work on amdgpu.

The original fd's lifetime differs from that of the amdgpu_device, and from the
one stored in the hash. The hash key is the fd, and in order to compare hash
entries we fstat them, so the fd must be around for as long as the amdgpu_device
is.

This patch for libdrm/amdgpu is a translation of the radeon-winsys ZaphodHeads
fix for mesa's radeon-winsys, from mesa commit 
28dda47ae4d974e3e032d60e8e0965c8c068c6d8

"winsys/radeon: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads."

Signed-off-by: Mario Kleiner <mario.kleiner...@gmail.com>
Cc: Marek Olšák <marek.ol...@amd.com>

Mhm, we originally designed libdrm_amdgpu with the idea that we take ownership of the file descriptor.

That was obviously not implemented like this (we leak the descriptor on destruction), but IIRC there was a reason for doing it like this.

The patch is Reviewed-by: Christian König <christian.koe...@amd.com> for now, but I'm not 100% sure if that's the last word on the topic.

Regards,
Christian.

---
  amdgpu/amdgpu_device.c | 14 ++++++++++----
  1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c
index b50ce26..1b0cd12 100644
--- a/amdgpu/amdgpu_device.c
+++ b/amdgpu/amdgpu_device.c
@@ -34,6 +34,7 @@
  #include <string.h>
  #include <stdio.h>
  #include <stdlib.h>
+#include <unistd.h>
#include "xf86drm.h"
  #include "amdgpu_drm.h"
@@ -153,7 +154,7 @@ int amdgpu_device_initialize(int fd,
                        return r;
                }
                if ((flag_auth) && (!flag_authexist)) {
-                       dev->flink_fd = fd;
+                       dev->flink_fd = dup(fd);
                }
                *major_version = dev->major_version;
                *minor_version = dev->minor_version;
@@ -183,8 +184,8 @@ int amdgpu_device_initialize(int fd,
                goto cleanup;
        }
- dev->fd = fd;
-       dev->flink_fd = fd;
+       dev->fd = dup(fd);
+       dev->flink_fd = dev->fd;
        dev->major_version = version->version_major;
        dev->minor_version = version->version_minor;
        drmFreeVersion(version);
@@ -213,12 +214,14 @@ int amdgpu_device_initialize(int fd,
        *major_version = dev->major_version;
        *minor_version = dev->minor_version;
        *device_handle = dev;
-       util_hash_table_set(fd_tab, UINT_TO_PTR(fd), dev);
+       util_hash_table_set(fd_tab, UINT_TO_PTR(dev->fd), dev);
        pthread_mutex_unlock(&fd_mutex);
return 0; cleanup:
+       if (dev->fd)
+               close(dev->fd);
        free(dev);
        pthread_mutex_unlock(&fd_mutex);
        return r;
@@ -232,6 +235,9 @@ void amdgpu_device_free_internal(amdgpu_device_handle dev)
        pthread_mutex_destroy(&dev->bo_table_mutex);
        pthread_mutex_destroy(&(dev->vamgr.bo_va_mutex));
        util_hash_table_remove(fd_tab, UINT_TO_PTR(dev->fd));
+       close(dev->fd);
+       if (dev->fd != dev->flink_fd)
+               close(dev->flink_fd);
        free(dev);
  }

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to