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

[Dave Gordon]

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