On 4/16/2025 12:48 PM, Tvrtko Ursulin wrote:
On 15/04/2025 19:43, Sunil Khatri wrote:change the DRM_ERROR to drm_file_err which gives the drm device information too which is useful in case of multiple GPU's and also add process information. Signed-off-by: Sunil Khatri <sunil.kha...@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 59 +++++++++++-------- 1 file changed, 33 insertions(+), 26 deletions(-)diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.cindex 05c1ee27a319..e07dff14256c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c@@ -123,25 +123,25 @@ int amdgpu_userqueue_create_object(struct amdgpu_userq_mgr *uq_mgr,r = amdgpu_bo_create(adev, &bp, &userq_obj->obj); if (r) { - DRM_ERROR("Failed to allocate BO for userqueue (%d)", r);+ drm_file_err(uq_mgr->file, "Failed to allocate BO for userqueue (%d)", r);return r; } r = amdgpu_bo_reserve(userq_obj->obj, true); if (r) { - DRM_ERROR("Failed to reserve BO to map (%d)", r);+ drm_file_err(uq_mgr->file, "Failed to reserve BO to map (%d)", r);goto free_obj; } r = amdgpu_ttm_alloc_gart(&(userq_obj->obj)->tbo); if (r) { - DRM_ERROR("Failed to alloc GART for userqueue object (%d)", r);+ drm_file_err(uq_mgr->file, "Failed to alloc GART for userqueue object (%d)", r);goto unresv; } r = amdgpu_bo_kmap(userq_obj->obj, &userq_obj->cpu_ptr); if (r) { - DRM_ERROR("Failed to map BO for userqueue (%d)", r);+ drm_file_err(uq_mgr->file, "Failed to map BO for userqueue (%d)", r);goto unresv; }@@ -177,7 +177,7 @@ amdgpu_userqueue_get_doorbell_index(struct amdgpu_userq_mgr *uq_mgr,gobj = drm_gem_object_lookup(filp, db_info->doorbell_handle); if (gobj == NULL) { - DRM_ERROR("Can't find GEM object for doorbell\n");+ drm_file_err(uq_mgr->file, "Can't find GEM object for doorbell\n");return -EINVAL; }@@ -187,13 +187,15 @@ amdgpu_userqueue_get_doorbell_index(struct amdgpu_userq_mgr *uq_mgr, /* Pin the BO before generating the index, unpin in queue destroy */r = amdgpu_bo_pin(db_obj->obj, AMDGPU_GEM_DOMAIN_DOORBELL); if (r) { - DRM_ERROR("[Usermode queues] Failed to pin doorbell object\n"); + drm_file_err(uq_mgr->file, + "[Usermode queues] Failed to pin doorbell object\n");Indentation could be off here (and a few more below), if it isn't my email client not displaying it properly.
Noted, will check again for indentation. regards Sunil
Sure will split the patch and update commit message to precisely say what is being done.goto unref_bo; } r = amdgpu_bo_reserve(db_obj->obj, true); if (r) { - DRM_ERROR("[Usermode queues] Failed to pin doorbell object\n"); + drm_file_err(uq_mgr->file, + "[Usermode queues] Failed to pin doorbell object\n"); goto unpin_bo; }@@ -215,14 +217,16 @@ amdgpu_userqueue_get_doorbell_index(struct amdgpu_userq_mgr *uq_mgr,break; default:- DRM_ERROR("[Usermode queues] IP %d not support\n", db_info->queue_type);+ drm_file_err(uq_mgr->file,+ "[Usermode queues] IP %d not support\n", db_info->queue_type);r = -EINVAL; goto unpin_bo; } index = amdgpu_doorbell_index_on_bar(uq_mgr->adev, db_obj->obj, db_info->doorbell_offset, db_size); - DRM_DEBUG_DRIVER("[Usermode queues] doorbell index=%lld\n", index); + drm_dbg_driver(adev_to_drm(uq_mgr->adev), + "[Usermode queues] doorbell index=%lld\n", index);This and others are technically okay but not what the commit message says. I'd say either split them into a separate patch or change the commit message to just say something like "Add device and client information to userq logging" so you give patch a wider mandate. ;)
Regards Sunil Khatri
amdgpu_bo_unreserve(db_obj->obj); return index;@@ -249,7 +253,7 @@ amdgpu_userqueue_destroy(struct drm_file *filp, int queue_id)queue = amdgpu_userqueue_find(uq_mgr, queue_id); if (!queue) { - DRM_DEBUG_DRIVER("Invalid queue id to destroy\n");+ drm_dbg_driver(adev_to_drm(uq_mgr->adev), "Invalid queue id to destroy\n");mutex_unlock(&uq_mgr->userq_mutex); return -EINVAL; }@@ -282,7 +286,8 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)if (args->in.ip_type != AMDGPU_HW_IP_GFX && args->in.ip_type != AMDGPU_HW_IP_DMA && args->in.ip_type != AMDGPU_HW_IP_COMPUTE) {- DRM_ERROR("Usermode queue doesn't support IP type %u\n", args->in.ip_type);+ drm_file_err(uq_mgr->file,+ "Usermode queue doesn't support IP type %u\n", args->in.ip_type);return -EINVAL; }@@ -304,14 +309,16 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)uq_funcs = adev->userq_funcs[args->in.ip_type]; if (!uq_funcs) {- DRM_ERROR("Usermode queue is not supported for this IP (%u)\n", args->in.ip_type);+ drm_file_err(uq_mgr->file, + "Usermode queue is not supported for this IP (%u)\n", + args->in.ip_type); r = -EINVAL; goto unlock; }queue = kzalloc(sizeof(struct amdgpu_usermode_queue), GFP_KERNEL);if (!queue) { - DRM_ERROR("Failed to allocate memory for queue\n");+ drm_file_err(uq_mgr->file, "Failed to allocate memory for queue\n");r = -ENOMEM; goto unlock; }@@ -327,7 +334,7 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args) /* Convert relative doorbell offset into absolute doorbell index */ index = amdgpu_userqueue_get_doorbell_index(uq_mgr, &db_info, filp);if (index == (uint64_t)-EINVAL) { - DRM_ERROR("Failed to get doorbell for queue\n");+ drm_file_err(uq_mgr->file, "Failed to get doorbell for queue\n");kfree(queue); goto unlock; }@@ -336,13 +343,13 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)xa_init_flags(&queue->fence_drv_xa, XA_FLAGS_ALLOC); r = amdgpu_userq_fence_driver_alloc(adev, queue); if (r) { - DRM_ERROR("Failed to alloc fence driver\n"); + drm_file_err(uq_mgr->file, "Failed to alloc fence driver\n"); goto unlock; } r = uq_funcs->mqd_create(uq_mgr, &args->in, queue); if (r) { - DRM_ERROR("Failed to create Queue\n"); + drm_file_err(uq_mgr->file, "Failed to create Queue\n");My OCD is upset by inconsistencies of queue vs Queue and queue vs usermode queue vs user queue. Looks like a good opportunity to tidy things up while touching the lines.
I see the frustration. let me update those to all say the same thing.
amdgpu_userq_fence_driver_free(queue); kfree(queue); goto unlock;@@ -350,7 +357,7 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args) qid = idr_alloc(&uq_mgr->userq_idr, queue, 1, AMDGPU_MAX_USERQ_COUNT, GFP_KERNEL);if (qid < 0) { - DRM_ERROR("Failed to allocate a queue id\n"); + drm_file_err(uq_mgr->file, "Failed to allocate a queue id\n"); amdgpu_userq_fence_driver_free(queue); uq_funcs->mqd_destroy(uq_mgr, queue); kfree(queue);@@ -360,7 +367,7 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)r = uq_funcs->map(uq_mgr, queue); if (r) { - DRM_ERROR("Failed to map Queue\n"); + drm_file_err(uq_mgr->file, "Failed to map Queue\n"); idr_remove(&uq_mgr->userq_idr, qid); amdgpu_userq_fence_driver_free(queue); uq_funcs->mqd_destroy(uq_mgr, queue);@@ -388,7 +395,7 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,return -EINVAL; r = amdgpu_userqueue_create(filp, args); if (r) - DRM_ERROR("Failed to create usermode queue\n"); + drm_file_err(filp, "Failed to create usermode queue\n");Not really a kernel wide error if userspace passed invalid arguements to the ioctl. Usually it is good to avoid allowing userspace at will log spamming.
I would prefer to handle all such things separately. Might be touching a lot other places too, hope thats fine.
regards Sunil
Regards, Tvrtkobreak; case AMDGPU_USERQ_OP_FREE:@@ -406,11 +413,11 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,return -EINVAL; r = amdgpu_userqueue_destroy(filp, args->in.queue_id); if (r) - DRM_ERROR("Failed to destroy usermode queue\n"); + drm_file_err(filp, "Failed to destroy usermode queue\n"); break; default:- DRM_DEBUG_DRIVER("Invalid user queue op specified: %d\n", args->in.op); + drm_dbg_driver(dev, "Invalid user queue op specified: %d\n", args->in.op);return -EINVAL; }@@ -479,7 +486,7 @@ amdgpu_userqueue_validate_bos(struct amdgpu_userq_mgr *uq_mgr)ret = amdgpu_vm_lock_pd(vm, &exec, 2); drm_exec_retry_on_contention(&exec); if (unlikely(ret)) { - DRM_ERROR("Failed to lock PD\n"); + drm_file_err(uq_mgr->file, "Failed to lock PD\n"); goto unlock_all; }@@ -519,7 +526,7 @@ amdgpu_userqueue_validate_bos(struct amdgpu_userq_mgr *uq_mgr)bo = bo_va->base.bo; ret = amdgpu_userqueue_validate_vm_bo(NULL, bo); if (ret) { - DRM_ERROR("Failed to validate BO\n"); + drm_file_err(uq_mgr->file, "Failed to validate BO\n"); goto unlock_all; }@@ -550,7 +557,7 @@ amdgpu_userqueue_validate_bos(struct amdgpu_userq_mgr *uq_mgr) ret = amdgpu_eviction_fence_replace_fence(&fpriv->evf_mgr, &exec);if (ret) - DRM_ERROR("Failed to replace eviction fence\n");+ drm_file_err(uq_mgr->file, "Failed to replace eviction fence\n");unlock_all: drm_exec_fini(&exec);@@ -569,13 +576,13 @@ static void amdgpu_userqueue_resume_worker(struct work_struct *work)ret = amdgpu_userqueue_validate_bos(uq_mgr); if (ret) { - DRM_ERROR("Failed to validate BOs to restore\n");+ drm_file_err(uq_mgr->file, "Failed to validate BOs to restore\n");goto unlock; } ret = amdgpu_userqueue_resume_all(uq_mgr); if (ret) { - DRM_ERROR("Failed to resume all queues\n"); + drm_file_err(uq_mgr->file, "Failed to resume all queues\n"); goto unlock; }
