Re: [PATCH 21/66] drm/i915: Move gtt and ppgtt under address space umbrella

From: Daniel Vetter <daniel@ffwll.ch>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 21/66] drm/i915: Move gtt and ppgtt under address space umbrella
Date: Mon, 1 Jul 2013 20:48:36 +0200	[thread overview]
Message-ID: <CAKMK7uHkPwLEF3kTf0o6C02kaspc8F4pynH2PrMzk7PFFTnsGA@mail.gmail.com> (raw)
In-Reply-To: <20130701184027.GD4242@bwidawsk.net>

On Mon, Jul 1, 2013 at 8:40 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Sun, Jun 30, 2013 at 03:12:35PM +0200, Daniel Vetter wrote:
>> On Thu, Jun 27, 2013 at 04:30:22PM -0700, Ben Widawsky wrote:
>> > The GTT and PPGTT can be thought of more generally as GPU address
>> > spaces. Many of their actions (insert entries), state (LRU lists) and
>> > many of their characteristics (size), can be shared. Do that.
>> >
>> > Created a i915_gtt_vm helper macro since for now we always want the
>> > regular GTT address space. Eventually we'll ween ourselves off of using
>> > this except in cases where we obviously want the GGTT (like display).
>> >
>> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>>
>> The i915_gtt_vm #define is imo too evil. Looks like a local variable, but
>> isn't. I think in most places we should just drop it, in others we should
>> add a real local vm variable. I'll punt for now on this one.
>> -Daniel
>
> It's dropped later in the series. It was a temporary bandaid to make the
> diffs a bit easier to swallow. I can certainly get rid of it next time
> around

Again I think (hope) that know that we know what this should look like
in the end, we can cut some diff churn. So e.g. when you move
something from dev_priv->mm to i915_address_space you check each place
you touch in the end-state of your branch and
- either add a local variable if the specific function will look up
the relevant address_space object itself
- or add the right function argument right away if it gets the
address_space passed in from callers.

Equivalently for moving stuff from the gem object to the vma.

Since both address space and vma have pointers to the old place (i.e.
drm_device or gem_object) you can add a temporary variable if a
functions newly gets an address_space/vma while the conversion is
still ongoing. That way we should not end up with needless churn in
function signatures, either.

Again I think more foreshadowing here would be really nice, so that's
what I'm aiming for with this idea.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch