Re: [PATCH 1/3] drm/ttm: split BO structure initialization into a separate function

[Nicolai Hähnle <nicolai.haehnle@xxxxxxx>]

On 15.02.2017 04:16, zhoucm1 wrote:
> On 2017年02月14日 18:37, Nicolai Hähnle wrote:
> > From: Nicolai Hähnle <nicolai.haehnle@xxxxxxx>
> >
> > Allow callers to opt out of calling ttm_bo_validate immediately. This
> > allows more flexibility in how locking of the reservation object is
> > done, which is needed to fix a locking bug (destroy locked mutex)
> > in amdgpu.
> >
> > Signed-off-by: Nicolai Hähnle <nicolai.haehnle@xxxxxxx>
> > ---
> >   drivers/gpu/drm/ttm/ttm_bo.c | 62
> > +++++++++++++++++++++++++++++---------------
> >   include/drm/ttm/ttm_bo_api.h | 45 ++++++++++++++++++++++++++++++++
> >   2 files changed, 86 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> > index 76bee42..ce4c0f5 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > @@ -1120,41 +1120,30 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
> >   }
> >   EXPORT_SYMBOL(ttm_bo_validate);
> >   -int ttm_bo_init(struct ttm_bo_device *bdev,
> > -        struct ttm_buffer_object *bo,
> > -        unsigned long size,
> > -        enum ttm_bo_type type,
> > -        struct ttm_placement *placement,
> > -        uint32_t page_alignment,
> > -        bool interruptible,
> > -        struct file *persistent_swap_storage,
> > -        size_t acc_size,
> > -        struct sg_table *sg,
> > -        struct reservation_object *resv,
> > -        void (*destroy) (struct ttm_buffer_object *))
> > +int ttm_bo_init_top(struct ttm_bo_device *bdev,
> > +            struct ttm_buffer_object *bo,
> > +            unsigned long size,
> > +            enum ttm_bo_type type,
> > +            uint32_t page_alignment,
> > +            struct file *persistent_swap_storage,
> > +            size_t acc_size,
> > +            struct sg_table *sg,
> > +            struct reservation_object *resv,
> > +            void (*destroy) (struct ttm_buffer_object *))
> >   {
> >       int ret = 0;
> >       unsigned long num_pages;
> >       struct ttm_mem_global *mem_glob = bdev->glob->mem_glob;
> > -    bool locked;
> >         ret = ttm_mem_global_alloc(mem_glob, acc_size, false, false);
> >       if (ret) {
> >           pr_err("Out of kernel memory\n");
> > -        if (destroy)
> > -            (*destroy)(bo);
> > -        else
> > -            kfree(bo);
> >           return -ENOMEM;
> >       }
> >         num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> >       if (num_pages == 0) {
> >           pr_err("Illegal buffer object size\n");
> > -        if (destroy)
> > -            (*destroy)(bo);
> > -        else
> > -            kfree(bo);
> >           ttm_mem_global_free(mem_glob, acc_size);
> >           return -EINVAL;
> >       }
> > @@ -1204,6 +1193,37 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
> >           ret = drm_vma_offset_add(&bdev->vma_manager, &bo->vma_node,
> >                        bo->mem.num_pages);
> if (ret && !resv), we should call
> reservation_object_fini(&bo->ttm_resv), right?

FWIW, you were right about this (and also mutex_destroy needs to be 
called for wu_mutex, etc.). But I'm following Christian's suggestion of 
having the caller use ttm_bo_unref for error cleanup, so all this error 
cleanup needn't be duplicated.

Cheers,
Nicola
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel