| Message ID | 20190106094613.19371-1-bas@basnieuwenhuizen.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [libdrm] amdgpu: Allow amdgpu device creation without merging fds. | expand |
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. Instead radv should not use the master fd for command submission. 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;
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, 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; >
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. 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
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 >
Am 07.01.19 um 14:05 schrieb Bas Nieuwenhuizen: > 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? Certainly yeah. Overriding the whole process priority was never my favorite approach. Regards, Christian. PS: I'm at the start of a bad cold / sinusitis, so sorry if my responses are short and maybe delayed. > >> 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
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;
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> --- amdgpu/amdgpu-symbol-check | 1 + amdgpu/amdgpu.h | 37 ++++++++++++++++++++++++ amdgpu/amdgpu_device.c | 59 ++++++++++++++++++++++++-------------- 3 files changed, 76 insertions(+), 21 deletions(-)