On Mon, Jan 7, 2019 at 1:23 PM Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> >> 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
