[PATCH libdrm 1/3] amdgpu: add missing unlock on drmPrimeFDToHandle() failure

[PATCH libdrm 1/3] amdgpu: add missing unlock on drmPrimeFDToHandle() failure

[Emil Velikov]

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

[PATCH libdrm 2/3] amdgpu: don't mess with shared_handle if amdgpu_bo_import() fails

[Emil Velikov]

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

Re: [PATCH libdrm 2/3] amdgpu: don't mess with shared_handle if amdgpu_bo_import() fails

[Nicolai Hähnle]

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

Re: [PATCH libdrm 2/3] amdgpu: don't mess with shared_handle if amdgpu_bo_import() fails

[Emil Velikov]

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

Re: [PATCH libdrm 1/3] amdgpu: add missing unlock on drmPrimeFDToHandle() failure

[Nicolai Hähnle]

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

[PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth()

[Emil Velikov]

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

Re: [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth()

[Emil Velikov]

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

Re: [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth()

[Emil Velikov]

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

Re: [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth()

[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) {
>                         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

Re: [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth()

[Christian König]

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

Re: [PATCH libdrm 3/3] amdgpu: rework and remove amdgpu_get_auth()

[Daniel Vetter]

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