Re: [PATCH 05/15] drm/i915: GuC-specific firmware loader

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Yu Dai <yu.dai@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 05/15] drm/i915: GuC-specific firmware loader
Date: Thu, 18 Jun 2015 21:12:19 +0100	[thread overview]
Message-ID: <20150618201219.GE14386@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <55830586.4000501@intel.com>

On Thu, Jun 18, 2015 at 10:53:10AM -0700, Yu Dai wrote:
> 
> 
> On 06/15/2015 01:30 PM, Chris Wilson wrote:
> >On Mon, Jun 15, 2015 at 07:36:23PM +0100, Dave Gordon wrote:
> >> +	/* Set the source address for the new blob */
> >> +	offset = i915_gem_obj_ggtt_offset(fw_obj);
> >
> >Why would it even have a GGTT vma? There's no precondition here to
> >assert that it should.
> It is pinned into GGTT inside gem_allocate_guc_obj.

The basic rules when reviewing is pinning is:
- is there a reason for this pin?
- is the lifetime of the pin bound to the hardware access?
- are the pad-to-size/alignment correct?
- is the vma in the wrong location?

Pinning early (and then not even stating in the function preamble that
you expect the object to be pinned) makes it hard to review both the
reason and check the lifetime. An easy solution to avoiding the
assumption of having a pinned object is to pass around the vma instead.
Though because you pin too early it is not clear the reason for the pin
nor that you only pin it for the lifetime of the hardware access, and
you have to scour the code to ensure that the pin isn't randomly dropped
or reused for another access.
-Chris

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