* [PATCH libdrm 1/3] amdgpu: add missing unlock on drmPrimeFDToHandle() failure @ 2017-01-22 18:48 Emil Velikov [not found] ` <20170122184813.12995-1-emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2017-01-22 18:48 ` [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth() Emil Velikov 0 siblings, 2 replies; 11+ messages in thread From: Emil Velikov @ 2017-01-22 18:48 UTC (permalink / raw) To: dri-devel; +Cc: emil.l.velikov, amd-gfx Cc: amd-gfx@lists.freedesktop.org Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> --- amdgpu/amdgpu_bo.c | 1 + 1 file changed, 1 insertion(+) diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c index d30fd1e7..c9f31587 100644 --- a/amdgpu/amdgpu_bo.c +++ b/amdgpu/amdgpu_bo.c @@ -302,6 +302,7 @@ int amdgpu_bo_import(amdgpu_device_handle dev, /* Get a KMS handle. */ r = drmPrimeFDToHandle(dev->fd, shared_handle, &handle); if (r) { + pthread_mutex_unlock(&dev->bo_table_mutex); return r; } -- 2.11.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 11+ messages in thread
[parent not found: <20170122184813.12995-1-emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* [PATCH libdrm 2/3] amdgpu: don't mess with shared_handle if amdgpu_bo_import() fails [not found] ` <20170122184813.12995-1-emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2017-01-22 18:48 ` Emil Velikov 2017-01-23 16:14 ` Nicolai Hähnle 2017-01-23 16:15 ` [PATCH libdrm 1/3] amdgpu: add missing unlock on drmPrimeFDToHandle() failure Nicolai Hähnle 1 sibling, 1 reply; 11+ messages in thread From: Emil Velikov @ 2017-01-22 18:48 UTC (permalink / raw) To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Do not close the handle if someone else has created it. Afaict there's no change of ownership implied if the function fails. Thus the caller is responsible to doing the right thing - trying again, closing the handle and/or other. Cc: amd-gfx@lists.freedesktop.org Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> --- Not 100% sure if it's the correct thing. --- amdgpu/amdgpu_bo.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c index c9f31587..24fa8880 100644 --- a/amdgpu/amdgpu_bo.c +++ b/amdgpu/amdgpu_bo.c @@ -310,7 +310,6 @@ int amdgpu_bo_import(amdgpu_device_handle dev, size = lseek(shared_handle, 0, SEEK_END); if (size == (off_t)-1) { pthread_mutex_unlock(&dev->bo_table_mutex); - amdgpu_close_kms_handle(dev, handle); return -errno; } lseek(shared_handle, 0, SEEK_SET); @@ -355,9 +354,6 @@ int amdgpu_bo_import(amdgpu_device_handle dev, bo = calloc(1, sizeof(struct amdgpu_bo)); if (!bo) { pthread_mutex_unlock(&dev->bo_table_mutex); - if (type == amdgpu_bo_handle_type_dma_buf_fd) { - amdgpu_close_kms_handle(dev, shared_handle); - } return -ENOMEM; } -- 2.11.0 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm 2/3] amdgpu: don't mess with shared_handle if amdgpu_bo_import() fails 2017-01-22 18:48 ` [PATCH libdrm 2/3] amdgpu: don't mess with shared_handle if amdgpu_bo_import() fails Emil Velikov @ 2017-01-23 16:14 ` Nicolai Hähnle 2017-01-24 3:53 ` Emil Velikov 0 siblings, 1 reply; 11+ messages in thread From: Nicolai Hähnle @ 2017-01-23 16:14 UTC (permalink / raw) To: Emil Velikov, dri-devel; +Cc: amd-gfx I don't think is correct. The incoming handle is in shared_handle, not in handle. Once the code block around line 310 has executed, shared_handle is the handle produced by drmPrimeFDToHandle, and closing it on error (as the code currently does) should be the correct thing to do. The only possible issue is that I'm seeing is that perhaps the handle should be closed _before_ the unlock. Maybe someone else knows the details. Cheers, Nicolai On 22.01.2017 19:48, Emil Velikov wrote: > Do not close the handle if someone else has created it. Afaict there's > no change of ownership implied if the function fails. Thus the caller is > responsible to doing the right thing - trying again, closing the handle > and/or other. > > Cc: amd-gfx@lists.freedesktop.org > Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> > --- > Not 100% sure if it's the correct thing. > --- > amdgpu/amdgpu_bo.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c > index c9f31587..24fa8880 100644 > --- a/amdgpu/amdgpu_bo.c > +++ b/amdgpu/amdgpu_bo.c > @@ -310,7 +310,6 @@ int amdgpu_bo_import(amdgpu_device_handle dev, > size = lseek(shared_handle, 0, SEEK_END); > if (size == (off_t)-1) { > pthread_mutex_unlock(&dev->bo_table_mutex); > - amdgpu_close_kms_handle(dev, handle); > return -errno; > } > lseek(shared_handle, 0, SEEK_SET); > @@ -355,9 +354,6 @@ int amdgpu_bo_import(amdgpu_device_handle dev, > bo = calloc(1, sizeof(struct amdgpu_bo)); > if (!bo) { > pthread_mutex_unlock(&dev->bo_table_mutex); > - if (type == amdgpu_bo_handle_type_dma_buf_fd) { > - amdgpu_close_kms_handle(dev, shared_handle); > - } > return -ENOMEM; > } > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm 2/3] amdgpu: don't mess with shared_handle if amdgpu_bo_import() fails 2017-01-23 16:14 ` Nicolai Hähnle @ 2017-01-24 3:53 ` Emil Velikov 0 siblings, 0 replies; 11+ messages in thread From: Emil Velikov @ 2017-01-24 3:53 UTC (permalink / raw) To: Nicolai Hähnle; +Cc: amd-gfx mailing list, ML dri-devel On 23 January 2017 at 16:14, Nicolai Hähnle <nhaehnle@gmail.com> wrote: > I don't think is correct. The incoming handle is in shared_handle, not in > handle. Once the code block around line 310 has executed, shared_handle is > the handle produced by drmPrimeFDToHandle, and closing it on error (as the > code currently does) should be the correct thing to do. > I've got confused with the shared_handle <> handle naming - read the wrong one where I shouldn't have ;-) You're spot on. Thanks for the correction. Emil _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm 1/3] amdgpu: add missing unlock on drmPrimeFDToHandle() failure [not found] ` <20170122184813.12995-1-emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2017-01-22 18:48 ` [PATCH libdrm 2/3] amdgpu: don't mess with shared_handle if amdgpu_bo_import() fails Emil Velikov @ 2017-01-23 16:15 ` Nicolai Hähnle 1 sibling, 0 replies; 11+ messages in thread From: Nicolai Hähnle @ 2017-01-23 16:15 UTC (permalink / raw) To: Emil Velikov, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 22.01.2017 19:48, Emil Velikov wrote: > Cc: amd-gfx@lists.freedesktop.org > Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Reviewed-by: Nicolai Hähnle <nicolai.haehnle@amd.com> > --- > amdgpu/amdgpu_bo.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c > index d30fd1e7..c9f31587 100644 > --- a/amdgpu/amdgpu_bo.c > +++ b/amdgpu/amdgpu_bo.c > @@ -302,6 +302,7 @@ int amdgpu_bo_import(amdgpu_device_handle dev, > /* Get a KMS handle. */ > r = drmPrimeFDToHandle(dev->fd, shared_handle, &handle); > if (r) { > + pthread_mutex_unlock(&dev->bo_table_mutex); > return r; > } > > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth() 2017-01-22 18:48 [PATCH libdrm 1/3] amdgpu: add missing unlock on drmPrimeFDToHandle() failure Emil Velikov [not found] ` <20170122184813.12995-1-emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2017-01-22 18:48 ` Emil Velikov 2017-03-07 0:45 ` Emil Velikov 2017-08-21 12:31 ` Emil Velikov 1 sibling, 2 replies; 11+ messages in thread From: Emil Velikov @ 2017-01-22 18:48 UTC (permalink / raw) To: dri-devel; +Cc: emil.l.velikov, amd-gfx 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth() 2017-01-22 18:48 ` [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth() Emil Velikov @ 2017-03-07 0:45 ` Emil Velikov [not found] ` <CACvgo50BMP2qZsFzJK7Jmc-sAJkz7NVGKz0XjxZP3dMGs7_wtg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-08-21 12:31 ` Emil Velikov 1 sibling, 1 reply; 11+ messages in thread From: Emil Velikov @ 2017-03-07 0:45 UTC (permalink / raw) To: ML dri-devel; +Cc: Emil Velikov, amd-gfx mailing list 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <CACvgo50BMP2qZsFzJK7Jmc-sAJkz7NVGKz0XjxZP3dMGs7_wtg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth() [not found] ` <CACvgo50BMP2qZsFzJK7Jmc-sAJkz7NVGKz0XjxZP3dMGs7_wtg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2017-03-07 0:47 ` Emil Velikov 0 siblings, 0 replies; 11+ messages in thread From: Emil Velikov @ 2017-03-07 0:47 UTC (permalink / raw) To: ML dri-devel; +Cc: Emil Velikov, amd-gfx mailing list 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 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth() 2017-01-22 18:48 ` [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth() Emil Velikov 2017-03-07 0:45 ` Emil Velikov @ 2017-08-21 12:31 ` Emil Velikov 2017-08-21 12:56 ` Christian König 2017-08-21 16:37 ` Daniel Vetter 1 sibling, 2 replies; 11+ messages in thread From: Emil Velikov @ 2017-08-21 12:31 UTC (permalink / raw) To: ML dri-devel; +Cc: Emil Velikov, amd-gfx mailing list 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 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth() 2017-08-21 12:31 ` Emil Velikov @ 2017-08-21 12:56 ` Christian König 2017-08-21 16:37 ` Daniel Vetter 1 sibling, 0 replies; 11+ messages in thread From: Christian König @ 2017-08-21 12:56 UTC (permalink / raw) To: Emil Velikov, ML dri-devel; +Cc: amd-gfx mailing list 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth() 2017-08-21 12:31 ` Emil Velikov 2017-08-21 12:56 ` Christian König @ 2017-08-21 16:37 ` Daniel Vetter 1 sibling, 0 replies; 11+ messages in thread From: Daniel Vetter @ 2017-08-21 16:37 UTC (permalink / raw) To: Emil Velikov; +Cc: amd-gfx mailing list, ML dri-devel 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-08-21 16:37 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-22 18:48 [PATCH libdrm 1/3] amdgpu: add missing unlock on drmPrimeFDToHandle() failure Emil Velikov [not found] ` <20170122184813.12995-1-emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2017-01-22 18:48 ` [PATCH libdrm 2/3] amdgpu: don't mess with shared_handle if amdgpu_bo_import() fails Emil Velikov 2017-01-23 16:14 ` Nicolai Hähnle 2017-01-24 3:53 ` Emil Velikov 2017-01-23 16:15 ` [PATCH libdrm 1/3] amdgpu: add missing unlock on drmPrimeFDToHandle() failure Nicolai Hähnle 2017-01-22 18:48 ` [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth() Emil Velikov 2017-03-07 0:45 ` Emil Velikov [not found] ` <CACvgo50BMP2qZsFzJK7Jmc-sAJkz7NVGKz0XjxZP3dMGs7_wtg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-03-07 0:47 ` Emil Velikov 2017-08-21 12:31 ` Emil Velikov 2017-08-21 12:56 ` Christian König 2017-08-21 16:37 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.