Re: [PATCH v2 06/15] drm/panthor: Add GEM logical block

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 14 Aug 2023 14:40:25 +0100
Steven Price <steven.price@arm.com> wrote:

> > +/**
> > + * panthor_gem_create_with_handle() - Create a GEM object and attach it to a handle.
> > + * @file: DRM file.
> > + * @ddev: DRM device.
> > + * @exclusive_vm: Exclusive VM. Not NULL if the GEM object can't be shared.
> > + * @size: Size of the GEM object to allocate.
> > + * @flags: Combination of drm_panthor_bo_flags flags.
> > + * @handle: Pointer holding the handle pointing to the new GEM object.
> > + *
> > + * Return: A valid pointer on success, an ERR_PTR() otherwise.
> > + */
> > +struct panthor_gem_object *
> > +panthor_gem_create_with_handle(struct drm_file *file,
> > +			       struct drm_device *ddev,
> > +			       struct panthor_vm *exclusive_vm,
> > +			       size_t size,
> > +			       u32 flags, u32 *handle)
> > +{
> > +	int ret;
> > +	struct drm_gem_shmem_object *shmem;
> > +	struct panthor_gem_object *bo;
> > +
> > +	shmem = drm_gem_shmem_create(ddev, size);
> > +	if (IS_ERR(shmem))
> > +		return ERR_CAST(shmem);
> > +
> > +	bo = to_panthor_bo(&shmem->base);
> > +	bo->flags = flags;
> > +
> > +	if (exclusive_vm) {
> > +		bo->exclusive_vm = panthor_vm_get(exclusive_vm);
> > +		bo->base.base.resv = panthor_vm_resv(exclusive_vm);
> > +	}
> > +
> > +	/*
> > +	 * Allocate an id of idr table where the obj is registered
> > +	 * and handle has the id what user can see.
> > +	 */
> > +	ret = drm_gem_handle_create(file, &shmem->base, handle);
> > +	/* drop reference from allocate - handle holds it now. */
> > +	drm_gem_object_put(&shmem->base);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	return bo;
> > +}  
> 
> This function might be better just returning a simple int. The
> "with_handle" approach means that doing anything much with the returned
> object is dodgy (because another user space thread could have already
> guessed the handle), and anyway the only caller
> (panthor_ioctl_bo_create()) doesn't use the object and just extracts the
> error code (if any).

Totally agree. I'll change the return type for an int.