On 10/29/2025 7:45 PM, Jeff Hugo wrote:
> On 10/28/2025 1:07 AM, Karol Wachowski wrote:
>> @@ -377,7 +379,7 @@ int ivpu_bo_create_ioctl(struct drm_device *dev,
>> void *data, struct drm_file *fi
>> ret = drm_gem_handle_create(file, &bo->base.base,
>> &args->handle);
>> if (ret) {
>> - ivpu_err(vdev, "Failed to create handle for BO: %pe (ctx %u
>> size %llu flags 0x%x)",
>> + ivpu_err(vdev, "Failed to create handle for BO: %pe ctx %u
>> size %llu flags 0x%x\n",
>> bo, file_priv->ctx.id, args->size, args->flags);
>
> This looks like it could be triggered by a user (in an ioctl, and in
> this case the user could exhaust the handle space), so this should be
> changed to a debug message per the commit text, no?
I agree that this one can be changed to debug. I will send new version
of the patch.
>
>> } else {
>> args->vpu_addr = bo->vpu_addr;
>> @@ -406,14 +408,17 @@ ivpu_bo_create(struct ivpu_device *vdev, struct
>> ivpu_mmu_context *ctx,
>> bo = ivpu_bo_alloc(vdev, size, flags);
>> if (IS_ERR(bo)) {
>> - ivpu_err(vdev, "Failed to allocate BO: %pe (vpu_addr 0x%llx
>> size %llu flags 0x%x)",
>> + ivpu_err(vdev, "Failed to allocate BO: %pe vpu_addr 0x%llx
>> size %llu flags 0x%x\n",
>> bo, range->start, size, flags);
>
> Another possible debug message?
ivpu_bo_create() is only used by internal driver's allocations, I want
those to emit error messages. User cannot trigger these. So this should
stay error.
>
>> return NULL;
>> }
>> ret = ivpu_bo_alloc_vpu_addr(bo, ctx, range);
>> - if (ret)
>> + if (ret) {
>> + ivpu_err(vdev, "Failed to allocate NPU address for BO: %pe
>> ctx %u size %llu: %d\n",
>> + bo, ctx->id, size, ret);
>
> Another possible debug message?
Same here, this one is specifically called by ivpu_bo_create(). There's
another one in IOCTL and that one prints debug message only.
>
>> goto err_put;
>> + }
>> ret = ivpu_bo_bind(bo);
>> if (ret)
>> @@ -193,7 +206,7 @@ int ivpu_bo_create_from_userptr_ioctl(struct
>> drm_device *dev, void *data, struct
>> ret = drm_gem_handle_create(file, &bo->base.base,
>> &args->handle);
>> if (ret) {
>> - ivpu_err(vdev, "Failed to create handle for BO: %pe (ctx %u
>> size %llu flags 0x%x)",
>> + ivpu_err(vdev, "Failed to create handle for BO: %pe ctx %u
>> size %llu flags 0x%x\n",
>> bo, file_priv->ctx.id, args->size, args->flags);
>
> Another possible debug message?
Similar to the first one, I will change this to debug.
>
>> } else {
>> ivpu_dbg(vdev, BO, "Created userptr BO: handle=%u
>> vpu_addr=0x%llx size=%llu flags=0x%x\n",
>> @@ -69,12 +71,18 @@ int ivpu_ms_start_ioctl(struct drm_device *dev,
>> void *data, struct drm_file *fil
>> if (ret)
>> goto err_free_ms;
>> - single_buff_size = sample_size *
>> - ((u64)args->read_period_samples * MS_READ_PERIOD_MULTIPLIER);
>> - ms->bo = ivpu_bo_create_global(vdev, PAGE_ALIGN(single_buff_size
>> * MS_NUM_BUFFERS),
>> - DRM_IVPU_BO_CACHED | DRM_IVPU_BO_MAPPABLE);
>> + buf_size = PAGE_ALIGN((u64)args->read_period_samples *
>> sample_size *
>> + MS_READ_PERIOD_MULTIPLIER * MS_NUM_BUFFERS);
>> + if (buf_size > ivpu_hw_range_size(&vdev->hw->ranges.global)) {
>> + ivpu_dbg(vdev, IOCTL, "Requested MS buffer size %llu exceeds
>> range size %llu\n",
>> + buf_size, ivpu_hw_range_size(&vdev->hw->ranges.global));
>> + ret = -EINVAL;
>> + goto err_free_ms;
>> + }
>> +
>> + ms->bo = ivpu_bo_create_global(vdev, buf_size,
>> DRM_IVPU_BO_CACHED | DRM_IVPU_BO_MAPPABLE);
>> if (!ms->bo) {
>> - ivpu_err(vdev, "Failed to allocate MS buffer (size %llu)\n",
>> single_buff_size);
>> + ivpu_err(vdev, "Failed to allocate MS buffer (size %llu)\n",
>> buf_size);
>
> Another possible debug message?
Yes, I think this one also could be debug instead. It can be triggered
by user.
>
>> ret = -ENOMEM;
>> goto err_free_ms;
>> }
I've sent v2 version of the patch with those concerns addressed.
Thank you for your insights, as usual.
- Karol
