On Sat, Oct 08, 2016 at 09:12:13AM +0100, Tvrtko Ursulin wrote:
>
> On 07/10/2016 18:08, Chris Wilson wrote:
> >On Fri, Oct 07, 2016 at 05:52:47PM +0100, Tvrtko Ursulin wrote:
> >>On 07/10/2016 10:46, Chris Wilson wrote:
> >>>Quite a few of our objects used for internal hardware programming do not
> >>>benefit from being swappable or from being zero initialised. As such
> >>>they do not benefit from using a shmemfs backing storage and since they
> >>>are internal and never directly exposed to the user, we do not need to
> >>>worry about providing a filp. For these we can use an
> >>>drm_i915_gem_object wrapper around a sg_table of plain struct page. They
> >>>are not swap backed and not automatically pinned. If they are reaped
> >>>by the shrinker, the pages are released and the contents discarded. For
> >>>the internal use case, this is fine as for example, ringbuffers are
> >>>pinned from being written by a request to be read by the hardware. Once
> >>>they are idle, they can be discarded entirely. As such they are a good
> >>>match for execlist ringbuffers and a small variety of other internal
> >>>objects.
> >>>
> >>>In the first iteration, this is limited to the scratch batch buffers we
> >>>use (for command parsing and state initialisation).
> >>>
> >>>Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >>>---
> >>> drivers/gpu/drm/i915/Makefile | 1 +
> >>> drivers/gpu/drm/i915/i915_drv.h | 5 +
> >>> drivers/gpu/drm/i915/i915_gem_batch_pool.c | 28 ++---
> >>> drivers/gpu/drm/i915/i915_gem_internal.c | 161 +++++++++++++++++++++++++++
> >>> drivers/gpu/drm/i915/i915_gem_render_state.c | 2 +-
> >>> drivers/gpu/drm/i915/intel_engine_cs.c | 2 +-
> >>> drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++-
> >>> 7 files changed, 189 insertions(+), 24 deletions(-)
> >>> create mode 100644 drivers/gpu/drm/i915/i915_gem_internal.c
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> >>>index a998c2bce70a..b94a90f34d2d 100644
> >>>--- a/drivers/gpu/drm/i915/Makefile
> >>>+++ b/drivers/gpu/drm/i915/Makefile
> >>>@@ -35,6 +35,7 @@ i915-y += i915_cmd_parser.o \
> >>> i915_gem_execbuffer.o \
> >>> i915_gem_fence.o \
> >>> i915_gem_gtt.o \
> >>>+ i915_gem_internal.o \
> >>> i915_gem.o \
> >>> i915_gem_render_state.o \
> >>> i915_gem_request.o \
> >>>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>>index fee5cc92e2f2..bad97f1e5265 100644
> >>>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>>@@ -3538,6 +3538,11 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> >>> u32 gtt_offset,
> >>> u32 size);
> >>>+/* i915_gem_internal.c */
> >>>+struct drm_i915_gem_object *
> >>>+i915_gem_object_create_internal(struct drm_device *dev,
> >>>+ unsigned int size);
> >>>+
> >>Wasn't size_t our convention for GEM objects?
> >Not our convention, no. Using size_t has caused too many bugs, that we
> >started to erradicate it (in our usual piecemeal approach).
>
> Oh wow, I missed that decision. Last thing I remember was that
> someone was trying to convert it all to size_t. Fine by me.
Who? They probably didn't have to spend the time fixing size_t variables
being wrong on 32bit.
> >>>+ /* 965gm cannot relocate objects above 4GiB. */
> >>>+ gfp &= ~__GFP_HIGHMEM;
> >>>+ gfp |= __GFP_DMA32;
> >>>+ }
> >>>+
> >>>+ for (i = 0; i < npages; i++) {
> >>>+ struct page *page;
> >>>+
> >>>+ page = alloc_page(gfp);
> >>>+ if (!page)
> >>>+ goto err;
> >>>+
> >>>+#ifdef CONFIG_SWIOTLB
> >>>+ if (swiotlb_nr_tbl()) {
> >>>+ st->nents++;
> >>>+ sg_set_page(sg, page, PAGE_SIZE, 0);
> >>>+ sg = sg_next(sg);
> >>>+ continue;
> >>>+ }
> >>>+#endif
> >>>+ if (!i || page_to_pfn(page) != last_pfn + 1) {
> >>>+ if (i)
> >>>+ sg = sg_next(sg);
> >>>+ st->nents++;
> >>>+ sg_set_page(sg, page, PAGE_SIZE, 0);
> >>>+ } else {
> >>>+ sg->length += PAGE_SIZE;
> >>>+ }
> >>>+ last_pfn = page_to_pfn(page);
> >>>+ }
> >>>+#ifdef CONFIG_SWIOTLB
> >>>+ if (!swiotlb_nr_tbl())
> >>>+#endif
> >>>+ sg_mark_end(sg);
> >>Looks like the loop above could be moved into a helper and shared
> >>with i915_gem_object_get_pages_gtt. Maybe just a page-alloc and
> >>page-alloc-error callbacks would be required.
> >So just the entire thing as a callback... I would have thought you might
> >suggest trying high order allocations and falling back to low order.
>
> I am not sure higher order attempts would be worth it.
Looks like it is. :)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
