[PATCH 2/2] drm/amdkfd: Avoid using doorbell_off as offset in process doorbell pages
Yong Zhao
yong.zhao at amd.com
Mon Nov 11 23:06:20 UTC 2019
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 <Felix.Kuehling at amd.com>
>>
> 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<Yong.Zhao at amd.com>
>>> ---
>>> 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20191111/5f7000f9/attachment-0001.html>
More information about the amd-gfx
mailing list