On 18/06/15 21:12, Chris Wilson wrote: > 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. This particular object wasn't allocated with that function; that's only used for objects that need to be permanently accessible by the GuC (context pool, GuC logbuffer, per-client structure). As I already mentioned in another reply, /this/ one was pinned (and will be unpinned) by the *immediate caller* of this function. .Dave. > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
