Re: [PATCH v3 05/14] drm/panthor: Add GEM logical block

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

On Thu, 7 Dec 2023 16:38:33 +0000
Steven Price <steven.price@arm.com> wrote:

> > +
> > +	ret = panthor_vm_unmap_range(vm, bo->va_node.start,
> > +				     panthor_kernel_bo_size(bo));
> > +	if (ret)
> > +		goto out_free_bo;
> > +
> > +	panthor_vm_free_va(vm, &bo->va_node);
> > +	drm_gem_object_put(bo->obj);
> > +
> > +out_free_bo:
> > +	kfree(bo);
> > +}
> > +
> > +/**
> > + * panthor_kernel_bo_create() - Create and map a GEM object to a VM
> > + * @ptdev: Device.
> > + * @vm: VM to map the GEM to. If NULL, the kernel object is not GPU mapped.
> > + * @size: Size of the buffer object.
> > + * @bo_flags: Combination of drm_panthor_bo_flags flags.
> > + * @vm_map_flags: Combination of drm_panthor_vm_bind_op_flags (only those
> > + * that are related to map operations).
> > + * @gpu_va: GPU address assigned when mapping to the VM.
> > + * If gpu_va == PANTHOR_VM_KERNEL_AUTO_VA, the virtual address will be
> > + * automatically allocated.
> > + *
> > + * Return: A valid pointer in case of success, an ERR_PTR() otherwise.
> > + */
> > +struct panthor_kernel_bo *
> > +panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
> > +			 size_t size, u32 bo_flags, u32 vm_map_flags,
> > +			 u64 gpu_va)
> > +{
> > +	struct drm_gem_shmem_object *obj;
> > +	struct panthor_kernel_bo *kbo;
> > +	struct panthor_gem_object *bo;
> > +	int ret;
> > +
> > +	kbo = kzalloc(sizeof(*kbo), GFP_KERNEL);
> > +	if (!kbo)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	obj = drm_gem_shmem_create(&ptdev->base, size);
> > +	if (IS_ERR(obj)) {
> > +		ret = PTR_ERR(obj);
> > +		goto err_free_bo;
> > +	}
> > +
> > +	bo = to_panthor_bo(&obj->base);
> > +	size = obj->base.size;
> > +	kbo->obj = &obj->base;
> > +	bo->flags = bo_flags;
> > +
> > +	if (!vm)
> > +		return 0;  
> 
> This doesn't look right - I'd expect kbo to be returned? (Or an error if
> !vm isn't actually supported).
> 
> I've had a look at the rest of the driver and I can't find a user of the
> !vm case. So either I'm missing something (quite plausible) or we should
> just make the vm argument compulsory and simplify this a bit.

Hm, I can't remember why I made the GPU VM mapping optional for private
BOs, so I'll just make it mandatory as you suggest, and we can revisit
it later if we have an actual use case for that.