Re: [PATCH 2/6] drm/i915: Support for creating Stolen memory backed objects

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Dec 11, 2015 at 12:49:37PM +0000, Dave Gordon wrote:
> On 11/12/15 12:19, Tvrtko Ursulin wrote:
> >
> >On 11/12/15 11:22, Ankitprasad Sharma wrote:
> >>On Wed, 2015-12-09 at 14:06 +0000, Tvrtko Ursulin wrote:
> >>>Hi,
> >>>
> >>>On 09/12/15 12:46, ankitprasad.r.sharma@intel.com wrote:
> >>>>From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> >>>>
> [snip!]
> >>>>+    /**
> >>>>+     * Requested flags (currently used for placement
> >>>>+     * (which memory domain))
> >>>>+     *
> >>>>+     * You can request that the object be created from special memory
> >>>>+     * rather than regular system pages using this parameter. Such
> >>>>+     * irregular objects may have certain restrictions (such as CPU
> >>>>+     * access to a stolen object is verboten).
> >>>>+     *
> >>>>+     * This can be used in the future for other purposes too
> >>>>+     * e.g. specifying tiling/caching/madvise
> >>>>+     */
> >>>>+    __u32 flags;
> >>>>+#define I915_CREATE_PLACEMENT_STOLEN     (1<<0) /* Cannot use CPU
> >>>>mmaps */
> >>>>+#define __I915_CREATE_UNKNOWN_FLAGS
> >>>>-(I915_CREATE_PLACEMENT_STOLEN << 1)
> >>>
> >>>I've asked in another reply, now that userspace can create a stolen
> >>>object, what happens if it tries to use it for a batch buffer?
> >>>
> >>>Can it end up in the relocate_entry_cpu with a batch buffer allocated
> >>>from stolen, which would then call i915_gem_object_get_page and crash?
> >>Thanks for pointing it out.
> >>Yes, this is definitely a possibility, if we allocate batchbuffers from
> >>the stolen region. I have started working on that, to do
> >>relocate_entry_stolen() if the object is allocated from stolen.
> >
> >Or perhaps it would be OK to just fail the execbuf?
> >
> >Just thinking to simplify things. Is it required (or expected) that
> >users will need or want to create batch buffers from stolen?
> >
> >Regards,
> >Tvrtko
> 
> Let's NOT have batchbuffers in stolen. Or anywhere else exotic, just in
> regular shmfs-backed GEM objects (no phys, userptr, or dma_buf either).
> And I'd rather contexts and ringbuffers weren't placed there either, because
> the CPU needs to write those all the time. All special-purpose GEM objects
> should be usable ONLY as data buffers for the GPU, or for CPU access with
> pread/pwrite. The objects that the kernel needs to understand and manipulate
> (contexts, ringbuffers, and batches) should always be default (shmfs-backed)
> GEM objects, so that we don't have to propagate the understanding of all the
> exceptional cases into a multitude of different kernel functions.

Yeah, rejecting stolen batches makes sense I'd say.

> Oh, and I'd suggest that once we have more than two GEM object types, the
> pread/pwrite operations should be extracted and turned into vfuncs rather
> than adding complexity to the common ioctl/shmfs path.

While we discuss clenups around obj backing storage abstraction: Another
thing worth considering is completing our extraction of the different
types of obj into files: We already have dma-buf, stolen, userptr, and
could extract shmem and phys_obj. Then pull them all together into a
section about gem backing storage types in the docbook.

Should at least allow the next person to see through this maze without
first reading a few thousand git commits ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx