Re: [PATCH 11/38] drm/i915: Introduce an internal allocator for disposable private objects

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, mika.kuoppala@intel.com
Subject: Re: [PATCH 11/38] drm/i915: Introduce an internal allocator for disposable private objects
Date: Tue, 27 Sep 2016 10:10:48 +0100	[thread overview]
Message-ID: <20160927091048.GS28107@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1474458634.2749.57.camel@linux.intel.com>

On Wed, Sep 21, 2016 at 02:50:34PM +0300, Joonas Lahtinen wrote:
> On ti, 2016-09-20 at 09:29 +0100, 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 swapped backed and not automatically pinned. If they are reaped
> 
> "swap backed"
> 
> > 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 varierty of other internal
> > objects.
> > 
> > In the first iteration, this is limited to the scratch batch buffers we
> > use (for command parsing and state inaitialisation).
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> > +++ b/drivers/gpu/drm/i915/i915_gem_internal.c
> > @@ -0,0 +1,156 @@
> > +/*
> > + * Copyright © 2015 Intel Corporation
> 
> 2016 already.

But the functionality was written in 2014/2015...

> > +#include <drm/drmP.h>
> > +#include <drm/i915_drm.h>
> > +#include "i915_drv.h"
> > +
> > +static void __i915_gem_object_free_pages(struct sg_table *st)
> 
> Name makes me feel like this was be misplaced here. Maybe just
> __object_free_pages or similar.
> 
> > +static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
> > +{
> 
> Much copied from i915_gem_object_get_pages_gtt, not sure if there's
> place for code reuse. Copy paste and modify not my favourite.

The problem is the allocator. I can assume that the next few patches to
break get_pages out of the struct_mutex land and make this allocator
much simpler.

> > +}
> > +
> > +static void i915_gem_object_put_pages_internal(struct drm_i915_gem_object *obj)
> > +{
> > +	__i915_gem_object_free_pages(obj->pages);
> > +
> > +	obj->dirty = 0;
> > +	obj->madv = I915_MADV_WILLNEED;
> 
> This seems so backwards it might be worth a comment. It's written kind
> of inverted just to dodge the usual code paths, right? I'm not sure if
> it helps the code readability.

It's the major facility that enables these objects to simply work as a
cache, i.e. makes the render state and batch pool handling simpler. Once
upon a time these also were used so that PDE were not so special.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx