Re: [PATCH libdrm] amdgpu: Allow amdgpu device creation without merging fds.

On Mon, Jan 7, 2019 at 1:23 PM Christian König ckoenig.leichtzumerken@gmail.com wrote:

Am 06.01.19 um 21:29 schrieb Bas Nieuwenhuizen:

...

On Sun, Jan 6, 2019 at 9:23 PM Christian König ckoenig.leichtzumerken@gmail.com wrote:

...

Am 06.01.19 um 10:46 schrieb Bas Nieuwenhuizen:

...

For radv we want to be able to pass in a master fd and be sure that the created libdrm_amdgpu device also uses that master fd, so we can use it for prioritized submission.

radv does all interaction with other APIs/processes with dma-bufs, so we should not need the functionality in libdrm_amdgpu to only have a single fd for a device in the process.

Signed-off-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl

Well NAK.

This breaks a couple of design assumptions we used for the kernel driver and is strongly discouraged.

What assumptions are these? As far as I can tell everything is per drm fd, not process?

...

Instead radv should not use the master fd for command submission.

High priority submission needs to be done through a master fd

That assumption is incorrect. See file drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c to answer both your questions at the same time.

Hmm, I did not know about the AMDGPU_SCHED ioctl. That would work with the permissions. However as it stands we cannot use it, as it is for the entire drm-fd, not per context.

Would you be open to a patch adding a context parameter to the struct and a AMDGPU_SCHED_OP_CONTEXT_PRIORITY_OVERRIDE?

Additional to the scheduler priority we really don't want more than one fd in a process because of SVM binding overhead.

So that whole approach is a really big NAK.

Regards, Christian.

...

, so not using a master fd is not an option ...

...

Regards, Christian.

...

---

amdgpu/amdgpu-symbol-check | 1 + amdgpu/amdgpu.h | 37 ++++++++++++++++++++++++ amdgpu/amdgpu_device.c | 59 ++++++++++++++++++++++++-------------- 3 files changed, 76 insertions(+), 21 deletions(-)

diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check index 6f5e0f95..bbf48985 100755 --- a/amdgpu/amdgpu-symbol-check +++ b/amdgpu/amdgpu-symbol-check @@ -56,6 +56,7 @@ amdgpu_cs_wait_fences amdgpu_cs_wait_semaphore amdgpu_device_deinitialize amdgpu_device_initialize +amdgpu_device_initialize2 amdgpu_find_bo_by_cpu_mapping amdgpu_get_marketing_name amdgpu_query_buffer_size_alignment diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h index dc51659a..e5ed39bb 100644 --- a/amdgpu/amdgpu.h +++ b/amdgpu/amdgpu.h @@ -66,6 +66,13 @@ struct drm_amdgpu_info_hw_ip; */ #define AMDGPU_QUERY_FENCE_TIMEOUT_IS_ABSOLUTE (1 << 0)

+/**

• • Uses in amdgpu_device_initialize2(), meaning that the passed in fd should
• • not be deduplicated against other libdrm_amdgpu devices referring to the
• • same kernel device.
• */

+#define AMDGPU_DEVICE_DEDICATED_FD (1 << 0)

• /*--------------------------------------------------------------------------*/ /* ----------------------------- Enums ------------------------------------ */ /*--------------------------------------------------------------------------*/

@@ -526,6 +533,36 @@ int amdgpu_device_initialize(int fd, uint32_t *minor_version, amdgpu_device_handle *device_handle);

+/**

• • \param fd - \c [in] File descriptor for AMD GPU device

• •                               received previously as the result of
    

• •                               e.g. drmOpen() call.
    

• •                               For legacy fd type, the DRI2/DRI3
    

• •                               authentication should be done before
    

• •                               calling this function.
    

• • \param flags - \c [in] Bitmask of flags for device creation.
• • \param major_version - \c [out] Major version of library. It is assumed

• •                               that adding new functionality will cause
    

• •                               increase in major version
    

• • \param minor_version - \c [out] Minor version of library
• • \param device_handle - \c [out] Pointer to opaque context which should

• •                               be passed as the first parameter on each
    

• •                               API call
    

• • \return 0 on success\n

• •      <0 - Negative POSIX Error code
    

• • \sa amdgpu_device_deinitialize()

+*/ +int amdgpu_device_initialize2(int fd,

•                       uint32_t flags,
  

•                       uint32_t *major_version,
  

•                       uint32_t *minor_version,
  

•                       amdgpu_device_handle *device_handle);
  

• /**
  • When access to such library does not needed any more the special

diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c index 362494b1..b4bf3f76 100644 --- a/amdgpu/amdgpu_device.c +++ b/amdgpu/amdgpu_device.c @@ -100,7 +100,8 @@ static void amdgpu_device_free_internal(amdgpu_device_handle dev) pthread_mutex_lock(&fd_mutex); while (*node != dev && (*node)->next) node = &(*node)->next;

• *node = (*node)->next;
  

• if (*node == dev)
  

•         *node = (*node)->next;
   pthread_mutex_unlock(&fd_mutex);
  
   close(dev->fd);
  

@@ -144,6 +145,16 @@ drm_public int amdgpu_device_initialize(int fd, uint32_t *major_version, uint32_t *minor_version, amdgpu_device_handle *device_handle) +{

• return amdgpu_device_initialize2(fd, 0, major_version, minor_version,
  

•                                  device_handle);
  

+}

+drm_public int amdgpu_device_initialize2(int fd,

•                                  uint32_t flags,
  

•                                  uint32_t *major_version,
  

•                                  uint32_t *minor_version,
  

•                                  amdgpu_device_handle *device_handle)
  

  { struct amdgpu_device *dev; drmVersionPtr version;

@@ -164,26 +175,28 @@ drm_public int amdgpu_device_initialize(int fd, return r; }

• for (dev = fd_list; dev; dev = dev->next)
  

•         if (fd_compare(dev->fd, fd) == 0)
  

•                 break;
  

• if (dev) {
  

•         r = amdgpu_get_auth(dev->fd, &flag_authexist);
  

•         if (r) {
  

•                 fprintf(stderr, "%s: amdgpu_get_auth (2) failed (%i)\n",
  

•                         __func__, r);
  

• if (!(flags & AMDGPU_DEVICE_DEDICATED_FD)) {
  

•         for (dev = fd_list; dev; dev = dev->next)
  

•                 if (fd_compare(dev->fd, fd) == 0)
  

•                         break;
  

•         if (dev) {
  

•                 r = amdgpu_get_auth(dev->fd, &flag_authexist);
  

•                 if (r) {
  

•                         fprintf(stderr, "%s: amdgpu_get_auth (2) failed (%i)\n",
  

•                                 __func__, r);
  

•                         pthread_mutex_unlock(&fd_mutex);
  

•                         return r;
  

•                 }
  

•                 if ((flag_auth) && (!flag_authexist)) {
  

•                         dev->flink_fd = fcntl(fd, F_DUPFD_CLOEXEC, 0);
  

•                 }
  

•                 *major_version = dev->major_version;
  

•                 *minor_version = dev->minor_version;
  

•                 amdgpu_device_reference(device_handle, dev);
                   pthread_mutex_unlock(&fd_mutex);
  

•                 return r;
  

•         }
  

•         if ((flag_auth) && (!flag_authexist)) {
  

•                 dev->flink_fd = fcntl(fd, F_DUPFD_CLOEXEC, 0);
  

•                 return 0;
           }
  

•         *major_version = dev->major_version;
  

•         *minor_version = dev->minor_version;
  

•         amdgpu_device_reference(device_handle, dev);
  

•         pthread_mutex_unlock(&fd_mutex);
  

•         return 0;
   }
  
   dev = calloc(1, sizeof(struct amdgpu_device));
  

@@ -265,8 +278,12 @@ drm_public int amdgpu_device_initialize(int fd, *major_version = dev->major_version; *minor_version = dev->minor_version; *device_handle = dev;

• dev->next = fd_list;
  

• fd_list = dev;
  

• if (!(flags & AMDGPU_DEVICE_DEDICATED_FD)) {
  

•         dev->next = fd_list;
  

•         fd_list = dev;
  

• }
  

•  pthread_mutex_unlock(&fd_mutex);
  
   return 0;
  

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel