Re: [PATCH] drm/i915/gvt: Make use of idr_find and idr_for_each_entry in dmabuf
On 03 3月 23 15:12:16, Zhenyu Wang wrote:
> On 2023.03.02 19:53:18 +0800, Cai Huoqing wrote:
> > This patch uses the already existing IDR mechanism to simplify
> > and improve the dmabuf code.
> >
> > Using 'vgpu.object_idr' directly instead of 'dmabuf_obj_list_head'
> > or 'dmabuf.list', because the dmabuf_obj can be found by 'idr_find'
> > or 'idr_for_each_entry'.
> >
> > Signed-off-by: Cai Huoqing
> > ---
> > drivers/gpu/drm/i915/gvt/dmabuf.c | 69 +++
> > drivers/gpu/drm/i915/gvt/dmabuf.h | 1 -
> > drivers/gpu/drm/i915/gvt/gvt.h| 1 -
> > drivers/gpu/drm/i915/gvt/vgpu.c | 1 -
> > 4 files changed, 16 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c
> > b/drivers/gpu/drm/i915/gvt/dmabuf.c
> > index 6834f9fe40cf..7933bd843ae8 100644
> > --- a/drivers/gpu/drm/i915/gvt/dmabuf.c
> > +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> > @@ -133,21 +133,15 @@ static void dmabuf_gem_object_free(struct kref *kref)
> > struct intel_vgpu_dmabuf_obj *obj =
> > container_of(kref, struct intel_vgpu_dmabuf_obj, kref);
> > struct intel_vgpu *vgpu = obj->vgpu;
> > - struct list_head *pos;
> > struct intel_vgpu_dmabuf_obj *dmabuf_obj;
> > + int id;
> >
> > - if (vgpu && test_bit(INTEL_VGPU_STATUS_ACTIVE, vgpu->status) &&
> > - !list_empty(>dmabuf_obj_list_head)) {
> > - list_for_each(pos, >dmabuf_obj_list_head) {
> > - dmabuf_obj = list_entry(pos, struct
> > intel_vgpu_dmabuf_obj, list);
> > - if (dmabuf_obj == obj) {
> > - list_del(pos);
> > - idr_remove(>object_idr,
> > - dmabuf_obj->dmabuf_id);
> > - kfree(dmabuf_obj->info);
> > - kfree(dmabuf_obj);
> > - break;
> > - }
> > + if (vgpu && test_bit(INTEL_VGPU_STATUS_ACTIVE, vgpu->status)) {
> > + idr_for_each_entry(>object_idr, dmabuf_obj, id) {
> > + idr_remove(>object_idr, id);
> > + kfree(dmabuf_obj->info);
> > + kfree(dmabuf_obj);
>
> This is wrong, it is not to free all dmabuf objects, but just for target one.
Indeed, I will use idr_find for the target.
>
> > + break;
> > }
> > } else {
> > /* Free the orphan dmabuf_objs here */
> > @@ -340,13 +334,11 @@ static struct intel_vgpu_dmabuf_obj *
> > pick_dmabuf_by_info(struct intel_vgpu *vgpu,
> > struct intel_vgpu_fb_info *latest_info)
> > {
> > - struct list_head *pos;
> > struct intel_vgpu_fb_info *fb_info;
> > struct intel_vgpu_dmabuf_obj *dmabuf_obj = NULL;
> > - struct intel_vgpu_dmabuf_obj *ret = NULL;
> > + int id;
> >
> > - list_for_each(pos, >dmabuf_obj_list_head) {
> > - dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj,
> > list);
> > + idr_for_each_entry(>object_idr, dmabuf_obj, id) {
> > if (!dmabuf_obj->info)
> > continue;
> >
> > @@ -357,31 +349,11 @@ pick_dmabuf_by_info(struct intel_vgpu *vgpu,
> > (fb_info->drm_format_mod == latest_info->drm_format_mod) &&
> > (fb_info->drm_format == latest_info->drm_format) &&
> > (fb_info->width == latest_info->width) &&
> > - (fb_info->height == latest_info->height)) {
> > - ret = dmabuf_obj;
> > - break;
> > - }
>
> Maybe just keep original code's use of extra ret to not include this
> cumbersome diff?
Ok, will revert 'ret' related.
Thanks,
Cai-
>
> > - }
> > -
> > - return ret;
> > -}
> > -
> > -static struct intel_vgpu_dmabuf_obj *
> > -pick_dmabuf_by_num(struct intel_vgpu *vgpu, u32 id)
> > -{
> > - struct list_head *pos;
> > - struct intel_vgpu_dmabuf_obj *dmabuf_obj = NULL;
> > - struct intel_vgpu_dmabuf_obj *ret = NULL;
> > -
> > - list_for_each(pos, >dmabuf_obj_list_head) {
> > - dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj,
> > list);
> > - if (dmabuf_obj->dmabuf_id == id) {
> > - ret = dmabuf_obj;
> > - break;
> > - }
> > + (fb_info->height == latest_info->height))
> > + return dmabuf_obj;
> > }
> >
> > - return ret;
> > + return dmabuf_obj;
> > }
> >
> > static void update_fb_info(struct vfio_device_gfx_plane_info *gvt_dmabuf,
> > @@ -477,11 +449,6 @@ int intel_vgpu_query_plane(struct intel_vgpu *vgpu,
> > void *args)
> >
> > update_fb_info(gfx_plane_info, _info);
> >
> > - INIT_LIST_HEAD(_obj->list);
> > - mutex_lock(>dmabuf_lock);
> > - list_add_tail(_obj->list, >dmabuf_obj_list_head);
> > - mutex_unlock(>dmabuf_lock);
> > -
> > gvt_dbg_dpy("vgpu%d: %s new dmabuf_obj ref %d, id %d\n", vgpu->id,
> > __func__, kref_read(_obj->kref),
Re: [PATCH] drm/i915/gvt: Make use of idr_find and idr_for_each_entry in dmabuf
On 2023.03.02 19:53:18 +0800, Cai Huoqing wrote:
> This patch uses the already existing IDR mechanism to simplify
> and improve the dmabuf code.
>
> Using 'vgpu.object_idr' directly instead of 'dmabuf_obj_list_head'
> or 'dmabuf.list', because the dmabuf_obj can be found by 'idr_find'
> or 'idr_for_each_entry'.
>
> Signed-off-by: Cai Huoqing
> ---
> drivers/gpu/drm/i915/gvt/dmabuf.c | 69 +++
> drivers/gpu/drm/i915/gvt/dmabuf.h | 1 -
> drivers/gpu/drm/i915/gvt/gvt.h| 1 -
> drivers/gpu/drm/i915/gvt/vgpu.c | 1 -
> 4 files changed, 16 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c
> b/drivers/gpu/drm/i915/gvt/dmabuf.c
> index 6834f9fe40cf..7933bd843ae8 100644
> --- a/drivers/gpu/drm/i915/gvt/dmabuf.c
> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> @@ -133,21 +133,15 @@ static void dmabuf_gem_object_free(struct kref *kref)
> struct intel_vgpu_dmabuf_obj *obj =
> container_of(kref, struct intel_vgpu_dmabuf_obj, kref);
> struct intel_vgpu *vgpu = obj->vgpu;
> - struct list_head *pos;
> struct intel_vgpu_dmabuf_obj *dmabuf_obj;
> + int id;
>
> - if (vgpu && test_bit(INTEL_VGPU_STATUS_ACTIVE, vgpu->status) &&
> - !list_empty(>dmabuf_obj_list_head)) {
> - list_for_each(pos, >dmabuf_obj_list_head) {
> - dmabuf_obj = list_entry(pos, struct
> intel_vgpu_dmabuf_obj, list);
> - if (dmabuf_obj == obj) {
> - list_del(pos);
> - idr_remove(>object_idr,
> -dmabuf_obj->dmabuf_id);
> - kfree(dmabuf_obj->info);
> - kfree(dmabuf_obj);
> - break;
> - }
> + if (vgpu && test_bit(INTEL_VGPU_STATUS_ACTIVE, vgpu->status)) {
> + idr_for_each_entry(>object_idr, dmabuf_obj, id) {
> + idr_remove(>object_idr, id);
> + kfree(dmabuf_obj->info);
> + kfree(dmabuf_obj);
This is wrong, it is not to free all dmabuf objects, but just for target one.
> + break;
> }
> } else {
> /* Free the orphan dmabuf_objs here */
> @@ -340,13 +334,11 @@ static struct intel_vgpu_dmabuf_obj *
> pick_dmabuf_by_info(struct intel_vgpu *vgpu,
> struct intel_vgpu_fb_info *latest_info)
> {
> - struct list_head *pos;
> struct intel_vgpu_fb_info *fb_info;
> struct intel_vgpu_dmabuf_obj *dmabuf_obj = NULL;
> - struct intel_vgpu_dmabuf_obj *ret = NULL;
> + int id;
>
> - list_for_each(pos, >dmabuf_obj_list_head) {
> - dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj,
> list);
> + idr_for_each_entry(>object_idr, dmabuf_obj, id) {
> if (!dmabuf_obj->info)
> continue;
>
> @@ -357,31 +349,11 @@ pick_dmabuf_by_info(struct intel_vgpu *vgpu,
> (fb_info->drm_format_mod == latest_info->drm_format_mod) &&
> (fb_info->drm_format == latest_info->drm_format) &&
> (fb_info->width == latest_info->width) &&
> - (fb_info->height == latest_info->height)) {
> - ret = dmabuf_obj;
> - break;
> - }
Maybe just keep original code's use of extra ret to not include this cumbersome
diff?
> - }
> -
> - return ret;
> -}
> -
> -static struct intel_vgpu_dmabuf_obj *
> -pick_dmabuf_by_num(struct intel_vgpu *vgpu, u32 id)
> -{
> - struct list_head *pos;
> - struct intel_vgpu_dmabuf_obj *dmabuf_obj = NULL;
> - struct intel_vgpu_dmabuf_obj *ret = NULL;
> -
> - list_for_each(pos, >dmabuf_obj_list_head) {
> - dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj,
> list);
> - if (dmabuf_obj->dmabuf_id == id) {
> - ret = dmabuf_obj;
> - break;
> - }
> + (fb_info->height == latest_info->height))
> + return dmabuf_obj;
> }
>
> - return ret;
> + return dmabuf_obj;
> }
>
> static void update_fb_info(struct vfio_device_gfx_plane_info *gvt_dmabuf,
> @@ -477,11 +449,6 @@ int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void
> *args)
>
> update_fb_info(gfx_plane_info, _info);
>
> - INIT_LIST_HEAD(_obj->list);
> - mutex_lock(>dmabuf_lock);
> - list_add_tail(_obj->list, >dmabuf_obj_list_head);
> - mutex_unlock(>dmabuf_lock);
> -
> gvt_dbg_dpy("vgpu%d: %s new dmabuf_obj ref %d, id %d\n", vgpu->id,
> __func__, kref_read(_obj->kref), ret);
>
> @@ -508,7 +475,7 @@ int intel_vgpu_get_dmabuf(struct intel_vgpu *vgpu,
> unsigned int dmabuf_id)
>
> mutex_lock(>dmabuf_lock);
>
> - dmabuf_obj = pick_dmabuf_by_num(vgpu, dmabuf_id);
> + dmabuf_obj =
Re: [PATCH] drm/i915/gvt: Make use of idr_find and idr_for_each_entry in dmabuf
On 02 3月 23 19:53:18, Cai Huoqing wrote:
> This patch uses the already existing IDR mechanism to simplify
> and improve the dmabuf code.
>
> Using 'vgpu.object_idr' directly instead of 'dmabuf_obj_list_head'
> or 'dmabuf.list', because the dmabuf_obj can be found by 'idr_find'
> or 'idr_for_each_entry'.
>
> Signed-off-by: Cai Huoqing
> ---
> drivers/gpu/drm/i915/gvt/dmabuf.c | 69 +++
> drivers/gpu/drm/i915/gvt/dmabuf.h | 1 -
> drivers/gpu/drm/i915/gvt/gvt.h| 1 -
> drivers/gpu/drm/i915/gvt/vgpu.c | 1 -
> 4 files changed, 16 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c
> b/drivers/gpu/drm/i915/gvt/dmabuf.c
> index 6834f9fe40cf..7933bd843ae8 100644
> --- a/drivers/gpu/drm/i915/gvt/dmabuf.c
> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> @@ -133,21 +133,15 @@ static void dmabuf_gem_object_free(struct kref *kref)
> struct intel_vgpu_dmabuf_obj *obj =
> container_of(kref, struct intel_vgpu_dmabuf_obj, kref);
> struct intel_vgpu *vgpu = obj->vgpu;
> - struct list_head *pos;
> struct intel_vgpu_dmabuf_obj *dmabuf_obj;
> + int id;
>
> - if (vgpu && test_bit(INTEL_VGPU_STATUS_ACTIVE, vgpu->status) &&
> - !list_empty(>dmabuf_obj_list_head)) {
> - list_for_each(pos, >dmabuf_obj_list_head) {
> - dmabuf_obj = list_entry(pos, struct
> intel_vgpu_dmabuf_obj, list);
> - if (dmabuf_obj == obj) {
> - list_del(pos);
> - idr_remove(>object_idr,
> -dmabuf_obj->dmabuf_id);
> - kfree(dmabuf_obj->info);
> - kfree(dmabuf_obj);
> - break;
> - }
> + if (vgpu && test_bit(INTEL_VGPU_STATUS_ACTIVE, vgpu->status)) {
Here should add '&& !idr_is_empty()' like '&& !list_empty()',
I will fix it in patch v2
Thanks
Cai-
> + idr_for_each_entry(>object_idr, dmabuf_obj, id) {
> + idr_remove(>object_idr, id);
> + kfree(dmabuf_obj->info);
> + kfree(dmabuf_obj);
> + break;
> }
> } else {
> /* Free the orphan dmabuf_objs here */
> @@ -340,13 +334,11 @@ static struct intel_vgpu_dmabuf_obj *
> pick_dmabuf_by_info(struct intel_vgpu *vgpu,
> struct intel_vgpu_fb_info *latest_info)
> {
> - struct list_head *pos;
> struct intel_vgpu_fb_info *fb_info;
> struct intel_vgpu_dmabuf_obj *dmabuf_obj = NULL;
> - struct intel_vgpu_dmabuf_obj *ret = NULL;
> + int id;
>
> - list_for_each(pos, >dmabuf_obj_list_head) {
> - dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj,
> list);
> + idr_for_each_entry(>object_idr, dmabuf_obj, id) {
> if (!dmabuf_obj->info)
> continue;
>
> @@ -357,31 +349,11 @@ pick_dmabuf_by_info(struct intel_vgpu *vgpu,
> (fb_info->drm_format_mod == latest_info->drm_format_mod) &&
> (fb_info->drm_format == latest_info->drm_format) &&
> (fb_info->width == latest_info->width) &&
> - (fb_info->height == latest_info->height)) {
> - ret = dmabuf_obj;
> - break;
> - }
> - }
> -
> - return ret;
> -}
> -
> -static struct intel_vgpu_dmabuf_obj *
> -pick_dmabuf_by_num(struct intel_vgpu *vgpu, u32 id)
> -{
> - struct list_head *pos;
> - struct intel_vgpu_dmabuf_obj *dmabuf_obj = NULL;
> - struct intel_vgpu_dmabuf_obj *ret = NULL;
> -
> - list_for_each(pos, >dmabuf_obj_list_head) {
> - dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj,
> list);
> - if (dmabuf_obj->dmabuf_id == id) {
> - ret = dmabuf_obj;
> - break;
> - }
> + (fb_info->height == latest_info->height))
> + return dmabuf_obj;
> }
>
> - return ret;
> + return dmabuf_obj;
> }
>
> static void update_fb_info(struct vfio_device_gfx_plane_info *gvt_dmabuf,
> @@ -477,11 +449,6 @@ int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void
> *args)
>
> update_fb_info(gfx_plane_info, _info);
>
> - INIT_LIST_HEAD(_obj->list);
> - mutex_lock(>dmabuf_lock);
> - list_add_tail(_obj->list, >dmabuf_obj_list_head);
> - mutex_unlock(>dmabuf_lock);
> -
> gvt_dbg_dpy("vgpu%d: %s new dmabuf_obj ref %d, id %d\n", vgpu->id,
> __func__, kref_read(_obj->kref), ret);
>
> @@ -508,7 +475,7 @@ int intel_vgpu_get_dmabuf(struct intel_vgpu *vgpu,
> unsigned int dmabuf_id)
>
> mutex_lock(>dmabuf_lock);
>
> - dmabuf_obj = pick_dmabuf_by_num(vgpu, dmabuf_id);
> + dmabuf_obj = idr_find(>object_idr, dmabuf_id);
> if (dmabuf_obj == NULL) {
>
[PATCH] drm/i915/gvt: Make use of idr_find and idr_for_each_entry in dmabuf
This patch uses the already existing IDR mechanism to simplify
and improve the dmabuf code.
Using 'vgpu.object_idr' directly instead of 'dmabuf_obj_list_head'
or 'dmabuf.list', because the dmabuf_obj can be found by 'idr_find'
or 'idr_for_each_entry'.
Signed-off-by: Cai Huoqing
---
drivers/gpu/drm/i915/gvt/dmabuf.c | 69 +++
drivers/gpu/drm/i915/gvt/dmabuf.h | 1 -
drivers/gpu/drm/i915/gvt/gvt.h| 1 -
drivers/gpu/drm/i915/gvt/vgpu.c | 1 -
4 files changed, 16 insertions(+), 56 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c
b/drivers/gpu/drm/i915/gvt/dmabuf.c
index 6834f9fe40cf..7933bd843ae8 100644
--- a/drivers/gpu/drm/i915/gvt/dmabuf.c
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -133,21 +133,15 @@ static void dmabuf_gem_object_free(struct kref *kref)
struct intel_vgpu_dmabuf_obj *obj =
container_of(kref, struct intel_vgpu_dmabuf_obj, kref);
struct intel_vgpu *vgpu = obj->vgpu;
- struct list_head *pos;
struct intel_vgpu_dmabuf_obj *dmabuf_obj;
+ int id;
- if (vgpu && test_bit(INTEL_VGPU_STATUS_ACTIVE, vgpu->status) &&
- !list_empty(>dmabuf_obj_list_head)) {
- list_for_each(pos, >dmabuf_obj_list_head) {
- dmabuf_obj = list_entry(pos, struct
intel_vgpu_dmabuf_obj, list);
- if (dmabuf_obj == obj) {
- list_del(pos);
- idr_remove(>object_idr,
- dmabuf_obj->dmabuf_id);
- kfree(dmabuf_obj->info);
- kfree(dmabuf_obj);
- break;
- }
+ if (vgpu && test_bit(INTEL_VGPU_STATUS_ACTIVE, vgpu->status)) {
+ idr_for_each_entry(>object_idr, dmabuf_obj, id) {
+ idr_remove(>object_idr, id);
+ kfree(dmabuf_obj->info);
+ kfree(dmabuf_obj);
+ break;
}
} else {
/* Free the orphan dmabuf_objs here */
@@ -340,13 +334,11 @@ static struct intel_vgpu_dmabuf_obj *
pick_dmabuf_by_info(struct intel_vgpu *vgpu,
struct intel_vgpu_fb_info *latest_info)
{
- struct list_head *pos;
struct intel_vgpu_fb_info *fb_info;
struct intel_vgpu_dmabuf_obj *dmabuf_obj = NULL;
- struct intel_vgpu_dmabuf_obj *ret = NULL;
+ int id;
- list_for_each(pos, >dmabuf_obj_list_head) {
- dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj,
list);
+ idr_for_each_entry(>object_idr, dmabuf_obj, id) {
if (!dmabuf_obj->info)
continue;
@@ -357,31 +349,11 @@ pick_dmabuf_by_info(struct intel_vgpu *vgpu,
(fb_info->drm_format_mod == latest_info->drm_format_mod) &&
(fb_info->drm_format == latest_info->drm_format) &&
(fb_info->width == latest_info->width) &&
- (fb_info->height == latest_info->height)) {
- ret = dmabuf_obj;
- break;
- }
- }
-
- return ret;
-}
-
-static struct intel_vgpu_dmabuf_obj *
-pick_dmabuf_by_num(struct intel_vgpu *vgpu, u32 id)
-{
- struct list_head *pos;
- struct intel_vgpu_dmabuf_obj *dmabuf_obj = NULL;
- struct intel_vgpu_dmabuf_obj *ret = NULL;
-
- list_for_each(pos, >dmabuf_obj_list_head) {
- dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj,
list);
- if (dmabuf_obj->dmabuf_id == id) {
- ret = dmabuf_obj;
- break;
- }
+ (fb_info->height == latest_info->height))
+ return dmabuf_obj;
}
- return ret;
+ return dmabuf_obj;
}
static void update_fb_info(struct vfio_device_gfx_plane_info *gvt_dmabuf,
@@ -477,11 +449,6 @@ int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void
*args)
update_fb_info(gfx_plane_info, _info);
- INIT_LIST_HEAD(_obj->list);
- mutex_lock(>dmabuf_lock);
- list_add_tail(_obj->list, >dmabuf_obj_list_head);
- mutex_unlock(>dmabuf_lock);
-
gvt_dbg_dpy("vgpu%d: %s new dmabuf_obj ref %d, id %d\n", vgpu->id,
__func__, kref_read(_obj->kref), ret);
@@ -508,7 +475,7 @@ int intel_vgpu_get_dmabuf(struct intel_vgpu *vgpu, unsigned
int dmabuf_id)
mutex_lock(>dmabuf_lock);
- dmabuf_obj = pick_dmabuf_by_num(vgpu, dmabuf_id);
+ dmabuf_obj = idr_find(>object_idr, dmabuf_id);
if (dmabuf_obj == NULL) {
gvt_vgpu_err("invalid dmabuf id:%d\n", dmabuf_id);
ret = -EINVAL;
@@ -570,23 +537,19 @@ int intel_vgpu_get_dmabuf(struct intel_vgpu *vgpu,
unsigned int dmabuf_id)
void intel_vgpu_dmabuf_cleanup(struct intel_vgpu *vgpu)
{
-
