Re: [RFC] drm/i915: Add variable gem object size support to i915

["Volkin, Bradley D" <bradley.d.volkin@intel.com>]

On Sat, May 10, 2014 at 06:42:32AM -0700, Siluvery, Arun wrote:
> On 09/05/2014 22:18, Volkin, Bradley D wrote:
> > On Mon, Apr 28, 2014 at 08:01:29AM -0700, arun.siluvery@linux.intel.com wrote:
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	if (!i915_gem_obj_bound(obj, vm)) {
> >> +		ret = i915_gem_object_bind_to_vm(obj, vm, 0, true, false);
> >> +		if (ret)
> >> +			goto unlock;
> >> +
> >> +		if (!dev_priv->mm.aliasing_ppgtt)
> >> +			i915_gem_gtt_bind_object(obj, obj->cache_level);
> >> +	}
> >> +
> >> +	drm_gem_object_reference(&obj->base);
> >> +
> >> +	vma = i915_gem_obj_to_vma(obj, vm);
> >> +	if (!vma) {
> >> +		ret = -EINVAL;
> >> +		goto out;
> >> +	}
> >> +
> >> +	ret = i915_vma_unbind(vma);
> >> +	if (ret)
> >> +		goto out;
> >
> > Hmm, can you elaborate on the need for this section a bit? I don't
> > really follow what we're doing here. I can see needing to unbind an
> > object that is bound in order to change the page table entries. I
> > guess I just don't understand the specific implementation.
> >
> > For example, why do we need to bind an unbound object just to unbind
> > it again? Should we even allow fallocate() on such an object? And we
> > only bind/unbind from GGTT; what about PPGTTs?
> >
> This is the bit I am not clear as well.
> This is mainly added to cover the case where an object is created but 
> not yet bound. I don't know whether it is to be allowed or not.
> I can change this if we should not allow fallocate on unbound objects.
> 
> bind/unbind functions are already considering aliased ppgtt case.

Ok. Chris or someone will have to provide some guidance on exactly what
steps to do if the object is/isn't bound. I'd propose something but I'm
pretty sure I'll be wrong :)

I also wonder if i915_gem_obj_fallocate() should share more code with
i915_gem_object_get_pages_gtt() for the case of unmarking scratch.

For the PPGTT part, I was thinking about True PPGTT. Looking more closely,
this patch apparently isn't against the latest drm-intel-nightly branch, so
you probably don't have to worry about True PPGTT for the moment. But
eventually you'll need to account for it, which might be as simple as using
i915_gem_obj_bound_any() and adding some loops.

Brad

> 
> >> +
> >> +	mark_scratch =
> >> +		(args->mode & I915_GEM_FALLOC_MARK_SCRATCH) ? true : false;
> >> +	ret = i915_gem_obj_fallocate(obj, mark_scratch, start, length);
> >> +	if (ret) {
> >> +		DRM_ERROR("fallocating specified obj range failed\n");
> >> +		goto out;
> >> +	}
> >> +
> >> +	ret = i915_gem_object_bind_to_vm(obj, vm, 0, true, false);
> >> +	if (ret)
> >> +		DRM_ERROR("object couldn't be bound after falloc\n");
> >> +
> >> +out:
> >> +	drm_gem_object_unreference(&obj->base);
> >> +unlock:
> >> +	mutex_unlock(&dev->struct_mutex);
> >> +	return ret;
> >> +}
> >> +
> >>   static inline int
> >>   __copy_to_user_swizzled(char __user *cpu_vaddr,
> >>   			const char *gpu_vaddr, int gpu_offset,
> >> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> >> index aa8469e..0d63fc8 100644
> >> --- a/include/uapi/drm/i915_drm.h
> >> +++ b/include/uapi/drm/i915_drm.h
> >> @@ -275,6 +275,7 @@ struct csc_coeff {
> >>   #define DRM_I915_GET_RESET_STATS	0x32
> >>   #define DRM_I915_SET_PLANE_ZORDER	0x33
> >>   #define DRM_I915_GEM_USERPTR		0x34
> >> +#define DRM_I915_GEM_FALLOCATE		0x35
> >>   #define DRM_I915_SET_PLANE_180_ROTATION 0x36
> >>   #define DRM_I915_ENABLE_PLANE_RESERVED_REG_BIT_2	0x37
> >>   #define DRM_I915_SET_CSC		0x39
> >> @@ -339,6 +340,9 @@ struct csc_coeff {
> >>   #define DRM_IOCTL_I915_GEM_USERPTR \
> >>   		DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_USERPTR, \
> >>   				struct drm_i915_gem_userptr)
> >> +#define DRM_IOCTL_I915_GEM_FALLOCATE \
> >> +		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_FALLOCATE, \
> >> +				struct drm_i915_gem_fallocate)
> >
> > We're not returning any data in the struct, so no need for DRM_IOWR.
> > Just DRM_IOW should be fine.
> 
> will fix it in next revision.
> 
> >
> >>   #define DRM_IOCTL_I915_SET_PLANE_ALPHA		\
> >>   			DRM_IOW(DRM_COMMAND_BASE + DRM_I915_SET_PLANE_ALPHA, \
> >>   			struct drm_i915_set_plane_alpha)
> >> @@ -523,6 +527,33 @@ struct drm_i915_gem_create {
> >>   	__u32 pad;
> >>   };
> >>
> >> +struct drm_i915_gem_fallocate {
> >> +	/**
> >> +	 * Start position of the range
> >> +	 *
> >> +	 * If the given value is not page-aligned it will be rounded internally.
> >> +	 */
> >> +	__u64 start;
> >> +	/**
> >> +	 * Length of the range
> >> +	 *
> >> +	 * If the given value is not page-aligned it will be rounded internally.
> >> +	 */
> >> +	__u64 length;
> >> +	/**
> >> +	 * Mode applied to the range
> >> +	 */
> >> +	__u32 mode;
> >> +#define I915_GEM_FALLOC_MARK_SCRATCH        0x01
> >> +#define I915_GEM_FALLOC_UNMARK_SCRATCH      0x02
> >> +	/**
> >> +	 * Returned handle for the object.
> >> +	 *
> >> +	 * Object handles are nonzero.
> >> +	 */
> >
> > We're not actually returning the handle, it's only an input.
> 
> will fix it next revision.
> 
> >
> > Thanks,
> > Brad
> >
> >> +	__u32 handle;
> >> +};
> >> +
> >>   struct drm_i915_gem_pread {
> >>   	/** Handle for the object being read. */
> >>   	__u32 handle;
> >> --
> >> 1.9.2
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
>