| Message ID | 20170122184813.12995-3-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22 January 2017 at 18:48, Emil Velikov <emil.l.velikov@gmail.com> wrote: > All one needs is to establish if dev->fd is the flink (primary/card) > node, rather than use DRM_IOCTL_GET_CLIENT to query the auth status. > > The latter is [somewhat] deprecated and incorrect. We need to know [and > store] the primary node FD, since we're going to use it [at a later > stage] for buffer import/export sharing. > > Cc: amd-gfx@lists.freedesktop.org > Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> > --- > Again not 100% sure but things look quite fishy as-is... The > conditionals might be off. > > Note: original code [and this one] do not consider if flink_fd is > already set, thus as we dup we'll leak it. > --- > amdgpu/amdgpu_device.c | 43 ++----------------------------------------- > 1 file changed, 2 insertions(+), 41 deletions(-) > > diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c > index f4ede031..6f04d936 100644 > --- a/amdgpu/amdgpu_device.c > +++ b/amdgpu/amdgpu_device.c > @@ -101,34 +101,6 @@ static int fd_compare(void *key1, void *key2) > return result; > } > > -/** > -* Get the authenticated form fd, > -* > -* \param fd - \c [in] File descriptor for AMD GPU device > -* \param auth - \c [out] Pointer to output the fd is authenticated or not > -* A render node fd, output auth = 0 > -* A legacy fd, get the authenticated for compatibility root > -* > -* \return 0 on success\n > -* >0 - AMD specific error code\n > -* <0 - Negative POSIX Error code > -*/ > -static int amdgpu_get_auth(int fd, int *auth) > -{ > - int r = 0; > - drm_client_t client = {}; > - > - if (drmGetNodeTypeFromFd(fd) == DRM_NODE_RENDER) > - *auth = 0; > - else { > - client.idx = 0; > - r = drmIoctl(fd, DRM_IOCTL_GET_CLIENT, &client); > - if (!r) > - *auth = client.auth; > - } > - return r; > -} > - > static void amdgpu_device_free_internal(amdgpu_device_handle dev) > { > amdgpu_vamgr_deinit(dev->vamgr); > @@ -175,8 +147,6 @@ int amdgpu_device_initialize(int fd, > struct amdgpu_device *dev; > drmVersionPtr version; > int r; > - int flag_auth = 0; > - int flag_authexist=0; > uint32_t accel_working = 0; > uint64_t start, max; > > @@ -185,19 +155,10 @@ int amdgpu_device_initialize(int fd, > pthread_mutex_lock(&fd_mutex); > if (!fd_tab) > fd_tab = util_hash_table_create(fd_hash, fd_compare); > - r = amdgpu_get_auth(fd, &flag_auth); > - if (r) { > - pthread_mutex_unlock(&fd_mutex); > - return r; > - } > dev = util_hash_table_get(fd_tab, UINT_TO_PTR(fd)); > if (dev) { > - r = amdgpu_get_auth(dev->fd, &flag_authexist); > - if (r) { > - pthread_mutex_unlock(&fd_mutex); > - return r; > - } > - if ((flag_auth) && (!flag_authexist)) { > + if (drmGetNodeTypeFromFd(fd) == DRM_NODE_RENDER && > + drmGetNodeTypeFromFd(dev->fd) == DRM_NODE_PRIMARY) { > dev->flink_fd = dup(fd); > } > *major_version = dev->major_version; > -- Seems like this and 1/3 has fallen through the cracks. Note that 2/3 is wrong, as pointed by Nicolai. Can we get these moving in some shape or form - I have another ~20 patch series that builds on top ;-) Thanks Emil
On 7 March 2017 at 00:45, Emil Velikov <emil.l.velikov@gmail.com> wrote: > I have another ~20 patch series that builds on top ;-) > Correction - those are xf86-video-amdgpu ones independent of this series. Pardon for the noise. Emil
Hi all, Can anyone skim through this patch? Thanks Emil On 22 January 2017 at 18:48, Emil Velikov <emil.l.velikov@gmail.com> wrote: > All one needs is to establish if dev->fd is the flink (primary/card) > node, rather than use DRM_IOCTL_GET_CLIENT to query the auth status. > > The latter is [somewhat] deprecated and incorrect. We need to know [and > store] the primary node FD, since we're going to use it [at a later > stage] for buffer import/export sharing. > > Cc: amd-gfx@lists.freedesktop.org > Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> > --- > Again not 100% sure but things look quite fishy as-is... The > conditionals might be off. > > Note: original code [and this one] do not consider if flink_fd is > already set, thus as we dup we'll leak it. > --- > amdgpu/amdgpu_device.c | 43 ++----------------------------------------- > 1 file changed, 2 insertions(+), 41 deletions(-) > > diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c > index f4ede031..6f04d936 100644 > --- a/amdgpu/amdgpu_device.c > +++ b/amdgpu/amdgpu_device.c > @@ -101,34 +101,6 @@ static int fd_compare(void *key1, void *key2) > return result; > } > > -/** > -* Get the authenticated form fd, > -* > -* \param fd - \c [in] File descriptor for AMD GPU device > -* \param auth - \c [out] Pointer to output the fd is authenticated or not > -* A render node fd, output auth = 0 > -* A legacy fd, get the authenticated for compatibility root > -* > -* \return 0 on success\n > -* >0 - AMD specific error code\n > -* <0 - Negative POSIX Error code > -*/ > -static int amdgpu_get_auth(int fd, int *auth) > -{ > - int r = 0; > - drm_client_t client = {}; > - > - if (drmGetNodeTypeFromFd(fd) == DRM_NODE_RENDER) > - *auth = 0; > - else { > - client.idx = 0; > - r = drmIoctl(fd, DRM_IOCTL_GET_CLIENT, &client); > - if (!r) > - *auth = client.auth; > - } > - return r; > -} > - > static void amdgpu_device_free_internal(amdgpu_device_handle dev) > { > amdgpu_vamgr_deinit(dev->vamgr); > @@ -175,8 +147,6 @@ int amdgpu_device_initialize(int fd, > struct amdgpu_device *dev; > drmVersionPtr version; > int r; > - int flag_auth = 0; > - int flag_authexist=0; > uint32_t accel_working = 0; > uint64_t start, max; > > @@ -185,19 +155,10 @@ int amdgpu_device_initialize(int fd, > pthread_mutex_lock(&fd_mutex); > if (!fd_tab) > fd_tab = util_hash_table_create(fd_hash, fd_compare); > - r = amdgpu_get_auth(fd, &flag_auth); > - if (r) { > - pthread_mutex_unlock(&fd_mutex); > - return r; > - } > dev = util_hash_table_get(fd_tab, UINT_TO_PTR(fd)); > if (dev) { > - r = amdgpu_get_auth(dev->fd, &flag_authexist); > - if (r) { > - pthread_mutex_unlock(&fd_mutex); > - return r; > - } > - if ((flag_auth) && (!flag_authexist)) { > + if (drmGetNodeTypeFromFd(fd) == DRM_NODE_RENDER && > + drmGetNodeTypeFromFd(dev->fd) == DRM_NODE_PRIMARY) { > dev->flink_fd = dup(fd); > } > *major_version = dev->major_version; > -- > 2.11.0 >
Am 21.08.2017 um 14:31 schrieb Emil Velikov: > Hi all, > > Can anyone skim through this patch? > > Thanks > Emil > > On 22 January 2017 at 18:48, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> All one needs is to establish if dev->fd is the flink (primary/card) >> node, rather than use DRM_IOCTL_GET_CLIENT to query the auth status. >> >> The latter is [somewhat] deprecated and incorrect. We need to know [and >> store] the primary node FD, since we're going to use it [at a later >> stage] for buffer import/export sharing. >> >> Cc: amd-gfx@lists.freedesktop.org >> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> >> --- >> Again not 100% sure but things look quite fishy as-is... The >> conditionals might be off. >> >> Note: original code [and this one] do not consider if flink_fd is >> already set, thus as we dup we'll leak it. >> --- >> amdgpu/amdgpu_device.c | 43 ++----------------------------------------- >> 1 file changed, 2 insertions(+), 41 deletions(-) >> >> diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c >> index f4ede031..6f04d936 100644 >> --- a/amdgpu/amdgpu_device.c >> +++ b/amdgpu/amdgpu_device.c >> @@ -101,34 +101,6 @@ static int fd_compare(void *key1, void *key2) >> return result; >> } >> >> -/** >> -* Get the authenticated form fd, >> -* >> -* \param fd - \c [in] File descriptor for AMD GPU device >> -* \param auth - \c [out] Pointer to output the fd is authenticated or not >> -* A render node fd, output auth = 0 >> -* A legacy fd, get the authenticated for compatibility root >> -* >> -* \return 0 on success\n >> -* >0 - AMD specific error code\n >> -* <0 - Negative POSIX Error code >> -*/ >> -static int amdgpu_get_auth(int fd, int *auth) >> -{ >> - int r = 0; >> - drm_client_t client = {}; >> - >> - if (drmGetNodeTypeFromFd(fd) == DRM_NODE_RENDER) >> - *auth = 0; >> - else { >> - client.idx = 0; >> - r = drmIoctl(fd, DRM_IOCTL_GET_CLIENT, &client); >> - if (!r) >> - *auth = client.auth; >> - } >> - return r; >> -} >> - >> static void amdgpu_device_free_internal(amdgpu_device_handle dev) >> { >> amdgpu_vamgr_deinit(dev->vamgr); >> @@ -175,8 +147,6 @@ int amdgpu_device_initialize(int fd, >> struct amdgpu_device *dev; >> drmVersionPtr version; >> int r; >> - int flag_auth = 0; >> - int flag_authexist=0; >> uint32_t accel_working = 0; >> uint64_t start, max; >> >> @@ -185,19 +155,10 @@ int amdgpu_device_initialize(int fd, >> pthread_mutex_lock(&fd_mutex); >> if (!fd_tab) >> fd_tab = util_hash_table_create(fd_hash, fd_compare); >> - r = amdgpu_get_auth(fd, &flag_auth); >> - if (r) { >> - pthread_mutex_unlock(&fd_mutex); >> - return r; >> - } >> dev = util_hash_table_get(fd_tab, UINT_TO_PTR(fd)); >> if (dev) { >> - r = amdgpu_get_auth(dev->fd, &flag_authexist); >> - if (r) { >> - pthread_mutex_unlock(&fd_mutex); >> - return r; >> - } >> - if ((flag_auth) && (!flag_authexist)) { >> + if (drmGetNodeTypeFromFd(fd) == DRM_NODE_RENDER && >> + drmGetNodeTypeFromFd(dev->fd) == DRM_NODE_PRIMARY) { First of all that looks inversed the logic. In other words we want to keep the primary as flink_fd, not the render node one. Second you are relaxing the test here which I can't judge if it's a good idea or not. Sorry that I can't help much more, Christian. >> dev->flink_fd = dup(fd); >> } >> *major_version = dev->major_version; >> -- >> 2.11.0 >> > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Mon, Aug 21, 2017 at 01:31:28PM +0100, Emil Velikov wrote: > Hi all, > > Can anyone skim through this patch? > > Thanks > Emil > > On 22 January 2017 at 18:48, Emil Velikov <emil.l.velikov@gmail.com> wrote: > > All one needs is to establish if dev->fd is the flink (primary/card) > > node, rather than use DRM_IOCTL_GET_CLIENT to query the auth status. > > > > The latter is [somewhat] deprecated and incorrect. We need to know [and > > store] the primary node FD, since we're going to use it [at a later > > stage] for buffer import/export sharing. > > > > Cc: amd-gfx@lists.freedesktop.org > > Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> > > --- > > Again not 100% sure but things look quite fishy as-is... The > > conditionals might be off. > > > > Note: original code [and this one] do not consider if flink_fd is > > already set, thus as we dup we'll leak it. > > --- > > amdgpu/amdgpu_device.c | 43 ++----------------------------------------- > > 1 file changed, 2 insertions(+), 41 deletions(-) > > > > diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c > > index f4ede031..6f04d936 100644 > > --- a/amdgpu/amdgpu_device.c > > +++ b/amdgpu/amdgpu_device.c > > @@ -101,34 +101,6 @@ static int fd_compare(void *key1, void *key2) > > return result; > > } > > > > -/** > > -* Get the authenticated form fd, > > -* > > -* \param fd - \c [in] File descriptor for AMD GPU device > > -* \param auth - \c [out] Pointer to output the fd is authenticated or not > > -* A render node fd, output auth = 0 > > -* A legacy fd, get the authenticated for compatibility root > > -* > > -* \return 0 on success\n > > -* >0 - AMD specific error code\n > > -* <0 - Negative POSIX Error code > > -*/ > > -static int amdgpu_get_auth(int fd, int *auth) > > -{ > > - int r = 0; > > - drm_client_t client = {}; > > - > > - if (drmGetNodeTypeFromFd(fd) == DRM_NODE_RENDER) > > - *auth = 0; > > - else { > > - client.idx = 0; > > - r = drmIoctl(fd, DRM_IOCTL_GET_CLIENT, &client); > > - if (!r) > > - *auth = client.auth; > > - } > > - return r; > > -} GETCLIENT is 99% nerfed, but this part here is uabi and hence will keep working forever. Not sure what's the value in removing it (beside maybe cleaning stuff up). -Daniel > > - > > static void amdgpu_device_free_internal(amdgpu_device_handle dev) > > { > > amdgpu_vamgr_deinit(dev->vamgr); > > @@ -175,8 +147,6 @@ int amdgpu_device_initialize(int fd, > > struct amdgpu_device *dev; > > drmVersionPtr version; > > int r; > > - int flag_auth = 0; > > - int flag_authexist=0; > > uint32_t accel_working = 0; > > uint64_t start, max; > > > > @@ -185,19 +155,10 @@ int amdgpu_device_initialize(int fd, > > pthread_mutex_lock(&fd_mutex); > > if (!fd_tab) > > fd_tab = util_hash_table_create(fd_hash, fd_compare); > > - r = amdgpu_get_auth(fd, &flag_auth); > > - if (r) { > > - pthread_mutex_unlock(&fd_mutex); > > - return r; > > - } > > dev = util_hash_table_get(fd_tab, UINT_TO_PTR(fd)); > > if (dev) { > > - r = amdgpu_get_auth(dev->fd, &flag_authexist); > > - if (r) { > > - pthread_mutex_unlock(&fd_mutex); > > - return r; > > - } > > - if ((flag_auth) && (!flag_authexist)) { > > + if (drmGetNodeTypeFromFd(fd) == DRM_NODE_RENDER && > > + drmGetNodeTypeFromFd(dev->fd) == DRM_NODE_PRIMARY) { > > dev->flink_fd = dup(fd); > > } > > *major_version = dev->major_version; > > -- > > 2.11.0 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c index f4ede031..6f04d936 100644 --- a/amdgpu/amdgpu_device.c +++ b/amdgpu/amdgpu_device.c @@ -101,34 +101,6 @@ static int fd_compare(void *key1, void *key2) return result; } -/** -* Get the authenticated form fd, -* -* \param fd - \c [in] File descriptor for AMD GPU device -* \param auth - \c [out] Pointer to output the fd is authenticated or not -* A render node fd, output auth = 0 -* A legacy fd, get the authenticated for compatibility root -* -* \return 0 on success\n -* >0 - AMD specific error code\n -* <0 - Negative POSIX Error code -*/ -static int amdgpu_get_auth(int fd, int *auth) -{ - int r = 0; - drm_client_t client = {}; - - if (drmGetNodeTypeFromFd(fd) == DRM_NODE_RENDER) - *auth = 0; - else { - client.idx = 0; - r = drmIoctl(fd, DRM_IOCTL_GET_CLIENT, &client); - if (!r) - *auth = client.auth; - } - return r; -} - static void amdgpu_device_free_internal(amdgpu_device_handle dev) { amdgpu_vamgr_deinit(dev->vamgr); @@ -175,8 +147,6 @@ int amdgpu_device_initialize(int fd, struct amdgpu_device *dev; drmVersionPtr version; int r; - int flag_auth = 0; - int flag_authexist=0; uint32_t accel_working = 0; uint64_t start, max; @@ -185,19 +155,10 @@ int amdgpu_device_initialize(int fd, pthread_mutex_lock(&fd_mutex); if (!fd_tab) fd_tab = util_hash_table_create(fd_hash, fd_compare); - r = amdgpu_get_auth(fd, &flag_auth); - if (r) { - pthread_mutex_unlock(&fd_mutex); - return r; - } dev = util_hash_table_get(fd_tab, UINT_TO_PTR(fd)); if (dev) { - r = amdgpu_get_auth(dev->fd, &flag_authexist); - if (r) { - pthread_mutex_unlock(&fd_mutex); - return r; - } - if ((flag_auth) && (!flag_authexist)) { + if (drmGetNodeTypeFromFd(fd) == DRM_NODE_RENDER && + drmGetNodeTypeFromFd(dev->fd) == DRM_NODE_PRIMARY) { dev->flink_fd = dup(fd); } *major_version = dev->major_version;
All one needs is to establish if dev->fd is the flink (primary/card) node, rather than use DRM_IOCTL_GET_CLIENT to query the auth status. The latter is [somewhat] deprecated and incorrect. We need to know [and store] the primary node FD, since we're going to use it [at a later stage] for buffer import/export sharing. Cc: amd-gfx@lists.freedesktop.org Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> --- Again not 100% sure but things look quite fishy as-is... The conditionals might be off. Note: original code [and this one] do not consider if flink_fd is already set, thus as we dup we'll leak it. --- amdgpu/amdgpu_device.c | 43 ++----------------------------------------- 1 file changed, 2 insertions(+), 41 deletions(-)