Re: [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 03/16/2015 02:48 PM, Joonas Lahtinen wrote:
> Hi,
>
> Regression testing completed without problems for BYT, HSW and BDW
> already.
>
> On ma, 2015-03-16 at 13:26 +0000, Tvrtko Ursulin wrote:
>> Hi,
>>
>> On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
>>> GGTT views are only applicable when dealing with GGTT. Change the code to
>>> reject ggtt_view where it should not be used and require it when it should
>>> be.
>>>
>>> v2:
>>> - Dropped _ppgtt_ infixes, allow both types to be passed
>>> - Disregard other but normal views when no view is specified
>>> - More checks that valid parameters are passed
>>> - More readable error checking
>>>
>>> v3:
>>> - Prefer WARN_ONCE over BUG_ON when there is code path for failure
>>
>> [snip]
>>
>>> +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
>>> +		    struct i915_address_space *vm);
>>> +struct i915_vma *
>>> +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
>>> +			  const struct i915_ggtt_view *view);
>>
>> Would i915_gem_obj_to_ggtt_vma be a better name? At least should have
>> vma in the name I think.
>>
>
> The i915_gem_obj_to_ggtt functions doesn't mention _vma either (and
> would cause a lot of changes all around code to change), so I decided to
> stay with the same convention. In that sense it would add more
> confusion, compared to the current *_view function being the same as
> without _view, but with explicitly specified view.

Yes, I overlooked that.

>>> +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
>>> +				     struct i915_address_space *vm)
>>>    {
>>>    	struct i915_vma *vma;
>>> -	list_for_each_entry(vma, &obj->vma_list, vma_link)
>>> -		if (vma->vm == vm && vma->ggtt_view.type == view->type)
>>> +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>> +		if (i915_is_ggtt(vma->vm) &&
>>> +		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
>>
>> Since there are 4-5 instances of this check it may make sense to add a
>> helper like i915_is_normal_ggtt_view(vma), but it is not that important
>> for me.
>>
>
> This will be done in following patch that makes the view struct (minus
> implementation parts like the pages sg_table) define the view.

Cool.

>> The rest looks good to me.
>>
>
> Sound like you could R-B this then?

Yes,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx