| Message ID | 20240105112218.351265-10-jacek.lawrynowicz@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | accel/ivpu fixes for 6.8 | expand |
On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote: > Make debug logs more readable and consistent: > - don't print handle as it is not always available for all buffers > - use hashed ivpu_bo ptr as main buffer identifier > - remove unused fields from ivpu_bo_print_info() > > Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com> > --- > drivers/accel/ivpu/ivpu_gem.c | 72 +++++++++++------------------------ > drivers/accel/ivpu/ivpu_gem.h | 1 - > 2 files changed, 23 insertions(+), 50 deletions(-) > > diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c > index 8cb4d337552e..dd327d7eda0d 100644 > --- a/drivers/accel/ivpu/ivpu_gem.c > +++ b/drivers/accel/ivpu/ivpu_gem.c > @@ -24,14 +24,11 @@ static const struct drm_gem_object_funcs ivpu_gem_funcs; > > static inline void ivpu_dbg_bo(struct ivpu_device *vdev, struct ivpu_bo *bo, const char *action) > { > - if (bo->ctx) > - ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u ctx %d vpu_addr 0x%llx mmu_mapped %d\n", > - action, ivpu_bo_size(bo), (bool)bo->base.pages, (bool)bo->base.sgt, > - bo->handle, bo->ctx->id, bo->vpu_addr, bo->mmu_mapped); > - else > - ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u (not added to context)\n", > - action, ivpu_bo_size(bo), (bool)bo->base.pages, (bool)bo->base.sgt, > - bo->handle); > + ivpu_dbg(vdev, BO, > + "%6s: bo %8p vpu_addr %9llx size %8zu ctx %d has_pages %d dma_mapped %d mmu_mapped %d wc %d imported %d\n", > + action, bo, bo->vpu_addr, ivpu_bo_size(bo), bo->ctx ? bo->ctx->id : 0, > + (bool)bo->base.pages, (bool)bo->base.sgt, bo->mmu_mapped, bo->base.map_wc, > + (bool)bo->base.base.import_attach); > } > > /* > @@ -49,12 +46,7 @@ int __must_check ivpu_bo_pin(struct ivpu_bo *bo) > mutex_lock(&bo->lock); > > ivpu_dbg_bo(vdev, bo, "pin"); > - > - if (!bo->ctx) { > - ivpu_err(vdev, "vpu_addr not allocated for BO %d\n", bo->handle); > - ret = -EINVAL; > - goto unlock; > - } > + drm_WARN_ON(&vdev->drm, !bo->ctx); > > if (!bo->mmu_mapped) { > struct sg_table *sgt = drm_gem_shmem_get_pages_sgt(&bo->base); > @@ -108,9 +100,7 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo) > { > struct ivpu_device *vdev = ivpu_bo_to_vdev(bo); > > - lockdep_assert_held(&bo->lock); > - > - ivpu_dbg_bo(vdev, bo, "unbind"); > + lockdep_assert(lockdep_is_held(&bo->lock) || !kref_read(&bo->base.base.refcount)); > > if (bo->mmu_mapped) { > drm_WARN_ON(&vdev->drm, !bo->ctx); > @@ -122,7 +112,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo) > > if (bo->ctx) { > ivpu_mmu_context_remove_node(bo->ctx, &bo->mm_node); > - bo->vpu_addr = 0; > bo->ctx = NULL; > } > > @@ -139,13 +128,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo) > dma_resv_unlock(bo->base.base.resv); > } > > -static void ivpu_bo_unbind(struct ivpu_bo *bo) > -{ > - mutex_lock(&bo->lock); > - ivpu_bo_unbind_locked(bo); > - mutex_unlock(&bo->lock); > -} This does not seem to be related to $SUBJECT > - > void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx) > { > struct ivpu_bo *bo; > @@ -156,8 +138,10 @@ void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_m > mutex_lock(&vdev->bo_list_lock); > list_for_each_entry(bo, &vdev->bo_list, bo_list_node) { > mutex_lock(&bo->lock); > - if (bo->ctx == ctx) > + if (bo->ctx == ctx) { > + ivpu_dbg_bo(vdev, bo, "unbind"); > ivpu_bo_unbind_locked(bo); > + } > mutex_unlock(&bo->lock); > } > mutex_unlock(&vdev->bo_list_lock); > @@ -209,9 +193,6 @@ ivpu_bo_create(struct ivpu_device *vdev, u64 size, u32 flags) > list_add_tail(&bo->bo_list_node, &vdev->bo_list); > mutex_unlock(&vdev->bo_list_lock); > > - ivpu_dbg(vdev, BO, "create: vpu_addr 0x%llx size %zu flags 0x%x\n", > - bo->vpu_addr, bo->base.base.size, flags); > - > return bo; > } > > @@ -243,15 +224,15 @@ static void ivpu_bo_free(struct drm_gem_object *obj) > struct ivpu_device *vdev = to_ivpu_device(obj->dev); > struct ivpu_bo *bo = to_ivpu_bo(obj); > > + ivpu_dbg_bo(vdev, bo, "free"); > + > mutex_lock(&vdev->bo_list_lock); > list_del(&bo->bo_list_node); > mutex_unlock(&vdev->bo_list_lock); > > drm_WARN_ON(&vdev->drm, !dma_resv_test_signaled(obj->resv, DMA_RESV_USAGE_READ)); > > - ivpu_dbg_bo(vdev, bo, "free"); > - > - ivpu_bo_unbind(bo); > + ivpu_bo_unbind_locked(bo); This does not seem to be related to $SUBJECT
On 05.01.2024 18:03, Jeffrey Hugo wrote: > On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote: >> Make debug logs more readable and consistent: >> - don't print handle as it is not always available for all buffers >> - use hashed ivpu_bo ptr as main buffer identifier >> - remove unused fields from ivpu_bo_print_info() >> >> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com> >> --- >> drivers/accel/ivpu/ivpu_gem.c | 72 +++++++++++------------------------ >> drivers/accel/ivpu/ivpu_gem.h | 1 - >> 2 files changed, 23 insertions(+), 50 deletions(-) >> >> diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c >> index 8cb4d337552e..dd327d7eda0d 100644 >> --- a/drivers/accel/ivpu/ivpu_gem.c >> +++ b/drivers/accel/ivpu/ivpu_gem.c >> @@ -24,14 +24,11 @@ static const struct drm_gem_object_funcs ivpu_gem_funcs; >> static inline void ivpu_dbg_bo(struct ivpu_device *vdev, struct ivpu_bo *bo, const char *action) >> { >> - if (bo->ctx) >> - ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u ctx %d vpu_addr 0x%llx mmu_mapped %d\n", >> - action, ivpu_bo_size(bo), (bool)bo->base.pages, (bool)bo->base.sgt, >> - bo->handle, bo->ctx->id, bo->vpu_addr, bo->mmu_mapped); >> - else >> - ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u (not added to context)\n", >> - action, ivpu_bo_size(bo), (bool)bo->base.pages, (bool)bo->base.sgt, >> - bo->handle); >> + ivpu_dbg(vdev, BO, >> + "%6s: bo %8p vpu_addr %9llx size %8zu ctx %d has_pages %d dma_mapped %d mmu_mapped %d wc %d imported %d\n", >> + action, bo, bo->vpu_addr, ivpu_bo_size(bo), bo->ctx ? bo->ctx->id : 0, >> + (bool)bo->base.pages, (bool)bo->base.sgt, bo->mmu_mapped, bo->base.map_wc, >> + (bool)bo->base.base.import_attach); >> } >> /* >> @@ -49,12 +46,7 @@ int __must_check ivpu_bo_pin(struct ivpu_bo *bo) >> mutex_lock(&bo->lock); >> ivpu_dbg_bo(vdev, bo, "pin"); >> - >> - if (!bo->ctx) { >> - ivpu_err(vdev, "vpu_addr not allocated for BO %d\n", bo->handle); >> - ret = -EINVAL; >> - goto unlock; >> - } >> + drm_WARN_ON(&vdev->drm, !bo->ctx); >> if (!bo->mmu_mapped) { >> struct sg_table *sgt = drm_gem_shmem_get_pages_sgt(&bo->base); >> @@ -108,9 +100,7 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo) >> { >> struct ivpu_device *vdev = ivpu_bo_to_vdev(bo); >> - lockdep_assert_held(&bo->lock); >> - >> - ivpu_dbg_bo(vdev, bo, "unbind"); >> + lockdep_assert(lockdep_is_held(&bo->lock) || !kref_read(&bo->base.base.refcount)); >> if (bo->mmu_mapped) { >> drm_WARN_ON(&vdev->drm, !bo->ctx); >> @@ -122,7 +112,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo) >> if (bo->ctx) { >> ivpu_mmu_context_remove_node(bo->ctx, &bo->mm_node); >> - bo->vpu_addr = 0; >> bo->ctx = NULL; >> } >> @@ -139,13 +128,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo) >> dma_resv_unlock(bo->base.base.resv); >> } >> -static void ivpu_bo_unbind(struct ivpu_bo *bo) >> -{ >> - mutex_lock(&bo->lock); >> - ivpu_bo_unbind_locked(bo); >> - mutex_unlock(&bo->lock); >> -} > > This does not seem to be related to $SUBJECT OK, I'll drop this. >> - >> void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx) >> { >> struct ivpu_bo *bo; >> @@ -156,8 +138,10 @@ void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_m >> mutex_lock(&vdev->bo_list_lock); >> list_for_each_entry(bo, &vdev->bo_list, bo_list_node) { >> mutex_lock(&bo->lock); >> - if (bo->ctx == ctx) >> + if (bo->ctx == ctx) { >> + ivpu_dbg_bo(vdev, bo, "unbind"); >> ivpu_bo_unbind_locked(bo); >> + } >> mutex_unlock(&bo->lock); >> } >> mutex_unlock(&vdev->bo_list_lock); >> @@ -209,9 +193,6 @@ ivpu_bo_create(struct ivpu_device *vdev, u64 size, u32 flags) >> list_add_tail(&bo->bo_list_node, &vdev->bo_list); >> mutex_unlock(&vdev->bo_list_lock); >> - ivpu_dbg(vdev, BO, "create: vpu_addr 0x%llx size %zu flags 0x%x\n", >> - bo->vpu_addr, bo->base.base.size, flags); >> - >> return bo; >> } >> @@ -243,15 +224,15 @@ static void ivpu_bo_free(struct drm_gem_object *obj) >> struct ivpu_device *vdev = to_ivpu_device(obj->dev); >> struct ivpu_bo *bo = to_ivpu_bo(obj); >> + ivpu_dbg_bo(vdev, bo, "free"); >> + >> mutex_lock(&vdev->bo_list_lock); >> list_del(&bo->bo_list_node); >> mutex_unlock(&vdev->bo_list_lock); >> drm_WARN_ON(&vdev->drm, !dma_resv_test_signaled(obj->resv, DMA_RESV_USAGE_READ)); >> - ivpu_dbg_bo(vdev, bo, "free"); >> - >> - ivpu_bo_unbind(bo); >> + ivpu_bo_unbind_locked(bo); > > This does not seem to be related to $SUBJECT OK, I'll drop this.
diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c index 8cb4d337552e..dd327d7eda0d 100644 --- a/drivers/accel/ivpu/ivpu_gem.c +++ b/drivers/accel/ivpu/ivpu_gem.c @@ -24,14 +24,11 @@ static const struct drm_gem_object_funcs ivpu_gem_funcs; static inline void ivpu_dbg_bo(struct ivpu_device *vdev, struct ivpu_bo *bo, const char *action) { - if (bo->ctx) - ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u ctx %d vpu_addr 0x%llx mmu_mapped %d\n", - action, ivpu_bo_size(bo), (bool)bo->base.pages, (bool)bo->base.sgt, - bo->handle, bo->ctx->id, bo->vpu_addr, bo->mmu_mapped); - else - ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u (not added to context)\n", - action, ivpu_bo_size(bo), (bool)bo->base.pages, (bool)bo->base.sgt, - bo->handle); + ivpu_dbg(vdev, BO, + "%6s: bo %8p vpu_addr %9llx size %8zu ctx %d has_pages %d dma_mapped %d mmu_mapped %d wc %d imported %d\n", + action, bo, bo->vpu_addr, ivpu_bo_size(bo), bo->ctx ? bo->ctx->id : 0, + (bool)bo->base.pages, (bool)bo->base.sgt, bo->mmu_mapped, bo->base.map_wc, + (bool)bo->base.base.import_attach); } /* @@ -49,12 +46,7 @@ int __must_check ivpu_bo_pin(struct ivpu_bo *bo) mutex_lock(&bo->lock); ivpu_dbg_bo(vdev, bo, "pin"); - - if (!bo->ctx) { - ivpu_err(vdev, "vpu_addr not allocated for BO %d\n", bo->handle); - ret = -EINVAL; - goto unlock; - } + drm_WARN_ON(&vdev->drm, !bo->ctx); if (!bo->mmu_mapped) { struct sg_table *sgt = drm_gem_shmem_get_pages_sgt(&bo->base); @@ -108,9 +100,7 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo) { struct ivpu_device *vdev = ivpu_bo_to_vdev(bo); - lockdep_assert_held(&bo->lock); - - ivpu_dbg_bo(vdev, bo, "unbind"); + lockdep_assert(lockdep_is_held(&bo->lock) || !kref_read(&bo->base.base.refcount)); if (bo->mmu_mapped) { drm_WARN_ON(&vdev->drm, !bo->ctx); @@ -122,7 +112,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo) if (bo->ctx) { ivpu_mmu_context_remove_node(bo->ctx, &bo->mm_node); - bo->vpu_addr = 0; bo->ctx = NULL; } @@ -139,13 +128,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo) dma_resv_unlock(bo->base.base.resv); } -static void ivpu_bo_unbind(struct ivpu_bo *bo) -{ - mutex_lock(&bo->lock); - ivpu_bo_unbind_locked(bo); - mutex_unlock(&bo->lock); -} - void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx) { struct ivpu_bo *bo; @@ -156,8 +138,10 @@ void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_m mutex_lock(&vdev->bo_list_lock); list_for_each_entry(bo, &vdev->bo_list, bo_list_node) { mutex_lock(&bo->lock); - if (bo->ctx == ctx) + if (bo->ctx == ctx) { + ivpu_dbg_bo(vdev, bo, "unbind"); ivpu_bo_unbind_locked(bo); + } mutex_unlock(&bo->lock); } mutex_unlock(&vdev->bo_list_lock); @@ -209,9 +193,6 @@ ivpu_bo_create(struct ivpu_device *vdev, u64 size, u32 flags) list_add_tail(&bo->bo_list_node, &vdev->bo_list); mutex_unlock(&vdev->bo_list_lock); - ivpu_dbg(vdev, BO, "create: vpu_addr 0x%llx size %zu flags 0x%x\n", - bo->vpu_addr, bo->base.base.size, flags); - return bo; } @@ -243,15 +224,15 @@ static void ivpu_bo_free(struct drm_gem_object *obj) struct ivpu_device *vdev = to_ivpu_device(obj->dev); struct ivpu_bo *bo = to_ivpu_bo(obj); + ivpu_dbg_bo(vdev, bo, "free"); + mutex_lock(&vdev->bo_list_lock); list_del(&bo->bo_list_node); mutex_unlock(&vdev->bo_list_lock); drm_WARN_ON(&vdev->drm, !dma_resv_test_signaled(obj->resv, DMA_RESV_USAGE_READ)); - ivpu_dbg_bo(vdev, bo, "free"); - - ivpu_bo_unbind(bo); + ivpu_bo_unbind_locked(bo); mutex_destroy(&bo->lock); drm_WARN_ON(obj->dev, bo->base.pages_use_count > 1); @@ -293,11 +274,9 @@ int ivpu_bo_create_ioctl(struct drm_device *dev, void *data, struct drm_file *fi return PTR_ERR(bo); } - ret = drm_gem_handle_create(file, &bo->base.base, &bo->handle); - if (!ret) { + ret = drm_gem_handle_create(file, &bo->base.base, &args->handle); + if (!ret) args->vpu_addr = bo->vpu_addr; - args->handle = bo->handle; - } drm_gem_object_put(&bo->base.base); @@ -415,19 +394,11 @@ int ivpu_bo_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file static void ivpu_bo_print_info(struct ivpu_bo *bo, struct drm_printer *p) { - unsigned long dma_refcount = 0; - mutex_lock(&bo->lock); - if (bo->base.base.dma_buf && bo->base.base.dma_buf->file) - dma_refcount = atomic_long_read(&bo->base.base.dma_buf->file->f_count); - - drm_printf(p, "%-3u %-6d 0x%-12llx %-10lu 0x%-8x %-4u %-8lu", - bo->ctx->id, bo->handle, bo->vpu_addr, bo->base.base.size, - bo->flags, kref_read(&bo->base.base.refcount), dma_refcount); - - if (bo->base.base.import_attach) - drm_printf(p, " imported"); + drm_printf(p, "%-9p %-3u 0x%-12llx %-10lu 0x%-8x %-4u", + bo, bo->ctx->id, bo->vpu_addr, bo->base.base.size, + bo->flags, kref_read(&bo->base.base.refcount)); if (bo->base.pages) drm_printf(p, " has_pages"); @@ -435,6 +406,9 @@ static void ivpu_bo_print_info(struct ivpu_bo *bo, struct drm_printer *p) if (bo->mmu_mapped) drm_printf(p, " mmu_mapped"); + if (bo->base.base.import_attach) + drm_printf(p, " imported"); + drm_printf(p, "\n"); mutex_unlock(&bo->lock); @@ -445,8 +419,8 @@ void ivpu_bo_list(struct drm_device *dev, struct drm_printer *p) struct ivpu_device *vdev = to_ivpu_device(dev); struct ivpu_bo *bo; - drm_printf(p, "%-3s %-6s %-14s %-10s %-10s %-4s %-8s %s\n", - "ctx", "handle", "vpu_addr", "size", "flags", "refs", "dma_refs", "attribs"); + drm_printf(p, "%-9s %-3s %-14s %-10s %-10s %-4s %s\n", + "bo", "ctx", "vpu_addr", "size", "flags", "refs", "attribs"); mutex_lock(&vdev->bo_list_lock); list_for_each_entry(bo, &vdev->bo_list, bo_list_node) diff --git a/drivers/accel/ivpu/ivpu_gem.h b/drivers/accel/ivpu/ivpu_gem.h index d75cad0d3c74..5cb1dda3e58e 100644 --- a/drivers/accel/ivpu/ivpu_gem.h +++ b/drivers/accel/ivpu/ivpu_gem.h @@ -19,7 +19,6 @@ struct ivpu_bo { struct mutex lock; /* Protects: ctx, mmu_mapped, vpu_addr */ u64 vpu_addr; - u32 handle; u32 flags; u32 job_status; /* Valid only for command buffer */ bool mmu_mapped;
Make debug logs more readable and consistent: - don't print handle as it is not always available for all buffers - use hashed ivpu_bo ptr as main buffer identifier - remove unused fields from ivpu_bo_print_info() Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com> --- drivers/accel/ivpu/ivpu_gem.c | 72 +++++++++++------------------------ drivers/accel/ivpu/ivpu_gem.h | 1 - 2 files changed, 23 insertions(+), 50 deletions(-)