The NULL pointer is not an issue, because for DIQ, the if (q) condition, which guards the section but is now shown, will never be satisfied. Anyway, I still added the NULL pointer check. With that, I have pushed the change. Yong On 2019-11-11 3:51 p.m., Felix Kuehling wrote: > On 2019-11-11 15:43, Felix Kuehling wrote: >> On 2019-11-01 16:10, Zhao, Yong wrote: >>> dorbell_off in the queue properties is mainly used for the doorbell dw >>> offset in pci bar. We should not set it to the doorbell byte offset in >>> process doorbell pages. This makes the code much easier to read. >> >> I kind of agree. I think what's confusing is that the >> queue_properties structure is used for two different purposes. >> >> 1. For storing queue properties provided by user mode through KFD ioctls >> 2. A subset of struct queue passed to mqd_manager and elsewhere >> (that's why some driver state is creeping into it) >> >> Maybe a follow-up could cleanly separate the queue properties from >> the queue driver state. That would probably change some internal >> interfaces to use struct queue instead of queue_properties. >> >> Anyway, this patch is >> >> Reviewed-by: Felix Kuehling >> > I pointed out a missing NULL pointer check inline near the end of the > patch. I should have mentioned it here. Please fix that before you submit. > > Thanks, >   Felix > > >>> Change-Id: I553045ff9fcb3676900c92d10426f2ceb3660005 >>> Signed-off-by: Yong Zhao >>> --- >>> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 12 ++++++------ >>> drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c | 2 +- >>> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 3 ++- >>> .../gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 8 ++++++-- >>> 4 files changed, 15 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>> index d9e36dbf13d5..b91993753b82 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>> @@ -258,6 +258,7 @@ static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p, >>> unsigned int queue_id; >>> struct kfd_process_device *pdd; >>> struct queue_properties q_properties; >>> + uint32_t doorbell_offset_in_process = 0; >>> >>> memset(&q_properties, 0, sizeof(struct queue_properties)); >>> >>> @@ -286,7 +287,8 @@ static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p, >>> p->pasid, >>> dev->id); >>> >>> - err = pqm_create_queue(&p->pqm, dev, filep, &q_properties, &queue_id); >>> + err = pqm_create_queue(&p->pqm, dev, filep, &q_properties, &queue_id, >>> + &doorbell_offset_in_process); >>> if (err != 0) >>> goto err_create_queue; >>> >>> @@ -298,12 +300,10 @@ static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p, >>> args->doorbell_offset |= KFD_MMAP_GPU_ID(args->gpu_id); >>> args->doorbell_offset <<= PAGE_SHIFT; >>> if (KFD_IS_SOC15(dev->device_info->asic_family)) >>> - /* On SOC15 ASICs, doorbell allocation must be >>> - * per-device, and independent from the per-process >>> - * queue_id. Return the doorbell offset within the >>> - * doorbell aperture to user mode. >>> + /* On SOC15 ASICs, include the doorbell offset within the >>> + * process doorbell frame, which could be 1 page or 2 pages. >>> */ >>> - args->doorbell_offset |= q_properties.doorbell_off; >>> + args->doorbell_offset |= doorbell_offset_in_process; >>> >>> mutex_unlock(&p->mutex); >>> >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c b/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c >>> index d59f2cd056c6..1d33c4f25263 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c >>> @@ -185,7 +185,7 @@ static int dbgdev_register_diq(struct kfd_dbgdev *dbgdev) >>> properties.type = KFD_QUEUE_TYPE_DIQ; >>> >>> status = pqm_create_queue(dbgdev->pqm, dbgdev->dev, NULL, >>> - &properties, &qid); >>> + &properties, &qid, NULL); >>> >>> if (status) { >>> pr_err("Failed to create DIQ\n"); >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >>> index 7c561c98f2e2..66bae8f2dad1 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >>> @@ -907,7 +907,8 @@ int pqm_create_queue(struct process_queue_manager *pqm, >>> struct kfd_dev *dev, >>> struct file *f, >>> struct queue_properties *properties, >>> - unsigned int *qid); >>> + unsigned int *qid, >>> + uint32_t *p_doorbell_offset_in_process); >>> int pqm_destroy_queue(struct process_queue_manager *pqm, unsigned int qid); >>> int pqm_update_queue(struct process_queue_manager *pqm, unsigned int qid, >>> struct queue_properties *p); >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c >>> index 8509814a6ff0..48185d2957e9 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c >>> @@ -192,7 +192,8 @@ int pqm_create_queue(struct process_queue_manager *pqm, >>> struct kfd_dev *dev, >>> struct file *f, >>> struct queue_properties *properties, >>> - unsigned int *qid) >>> + unsigned int *qid, >>> + uint32_t *p_doorbell_offset_in_process) >>> { >>> int retval; >>> struct kfd_process_device *pdd; >>> @@ -307,8 +308,11 @@ int pqm_create_queue(struct process_queue_manager *pqm, >>> /* Return the doorbell offset within the doorbell page >>> * to the caller so it can be passed up to user mode >>> * (in bytes). >>> + * There are always 1024 doorbells per process, so in case >>> + * of 8-byte doorbells, there are two doorbell pages per >>> + * process. >>> */ >>> - properties->doorbell_off = >>> *p_doorbell_offset_in_process = >> >> You need a NULL pointer check here because you call this with a NULL >> pointer from the DIQ code. >> It is not an issue, because for DIQ, the if (q) condition, which guards this section but is now shown, will never be satisfied. Anyway, I still added the NULL pointer check. With that, I have pushed the change. >> >>> (q->properties.doorbell_off * sizeof(uint32_t)) & >>> (kfd_doorbell_process_slice(dev) - 1); >>> >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx