Re: [PATCH v2 12/15] drm/panthor: Add the driver frontend block

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

On Wed, 6 Sep 2023 14:38:15 +0200
Ketil Johnsen <ketil.johnsen@arm.com> wrote:

> On 8/9/23 18:53, Boris Brezillon wrote:
> > +static int panthor_ioctl_vm_create(struct drm_device *ddev, void *data,
> > +				   struct drm_file *file)
> > +{
> > +	struct panthor_device *ptdev = container_of(ddev, struct panthor_device, base);
> > +	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);
> > +	struct panthor_file *pfile = file->driver_priv;
> > +	struct drm_panthor_vm_create *args = data;
> > +	u64 kernel_va_start = 0;
> > +	int cookie, ret;
> > +
> > +	if (!drm_dev_enter(ddev, &cookie))
> > +		return -ENODEV;
> > +
> > +	if (args->flags & ~PANTHOR_VM_CREATE_FLAGS) {
> > +		ret = -EINVAL;
> > +		goto out_dev_exit;
> > +	}
> > +
> > +	if (drm_WARN_ON(ddev, !va_bits) || args->kernel_va_range > (1ull << (va_bits - 1))) {
> > +		ret = -EINVAL;
> > +		goto out_dev_exit;
> > +	}
> > +
> > +	if (args->kernel_va_range)
> > +		kernel_va_start = (1 << (va_bits - 1)) - args->kernel_va_range;  
> 
> Bug here if user space provides kernel_va_range, which is the intention 
> of the current Mesa proposal.
> 
> I think the desired calculation should be something like:
> kernel_va_start = (1ull << va_bits) - args->kernel_va_range;
> 
> PS: There is currently also a bug in the accompanying Mesa changes which 
> accidentally makes kernel_va_range always zero, thus bypassing this 
> kernel bug.
> The Mesa bug is due to va_bits always being zero because mmu_features 
> field is not copied in panthor_dev_query_props().

Yep, I noticed/fixed the problem recently, when working on 32-bit
enablement. Anyway, thanks for the reporting those bugs.