[Intel-gfx] [PATCH] drm/i915: Make sure fb objects with rotated views are also fenceable

[Tvrtko Ursulin]

On 09/15/2015 03:09 AM, Vivek Kasireddy wrote:
> On Mon, 14 Sep 2015 10:08:19 +0100
> Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> wrote:
>
>>
>> Hi,
>>
>> On 09/12/2015 02:44 AM, Vivek Kasireddy wrote:
>>> From: Vivek Kasireddy <vivek.kasireddy at intel.com>
>>>
>>> Currently, fb objects with rotated views are ignored while pinning.
>>> Therefore, include the rotated view type and use the view size
>>> instead of the object's size to determine if it is fenceable. And,
>>> look at the view and its offset while writing and pinning to the
>>> fence registers.
>>
>> I didn't figure out from the commit message if something is broken or?
>>
>> AFAIR rotated views deliberately skip on fencing since rotated view
>> has shuffled pages in memory so it would be a weird view for
>> userspace to handle.
>
> Hi Tvrtko,
> I apologize for the short and ambiguous commit message.
> As I mentioned in my other reply to Chris, the reason for this patch is
> because of this:
> 	if (WARN_ON(!obj->map_and_fenceable))
> 		return -EINVAL;
> inside i915_find_fence_reg(). I am trying to enable 90 degree rotation
> for the Weston compositor as part of which I need to allocate and flip
> a Y-tiled fb obj. This fails because i915_gem_object_do_pin() ignores

The use case itself has been exercised in general and it does work.

However I suspect you are using SET_TILING on the object to set it to Y 
tiled? That would explain i915_gem_object_get_fence trying to enable 
fencing.

This indeed looks like an omission in test coverage, needs a test case 
in kms_addfb_basic/addfb25 test group.

> the rotated view passed by i915_gem_object_pin_to_display_plane() and
> hence obj->map_and_fenceable never gets set.
>
>>
>> Especially this below:
>>
>>>    static void i965_write_fence_reg(struct drm_device *dev, int reg,
>>> -				 struct drm_i915_gem_object *obj)
>>> +				 struct drm_i915_gem_object *obj,
>>> +				 const struct i915_ggtt_view *view)
>>>    {
>>>    	struct drm_i915_private *dev_priv = dev->dev_private;
>>>    	int fence_reg;
>>>    	int fence_pitch_shift;
>>> +	const struct i915_ggtt_view *ggtt_view = view;
>>>
>>>    	if (INTEL_INFO(dev)->gen >= 6) {
>>>    		fence_reg = FENCE_REG_SANDYBRIDGE_0;
>>> @@ -95,9 +97,13 @@ static void i965_write_fence_reg(struct
>>> drm_device *dev, int reg, size = (size / row_size) * row_size;
>>>    		}
>>>
>>> -		val = (uint64_t)((i915_gem_obj_ggtt_offset(obj) +
>>> size - 4096) &
>>> -				 0xfffff000) << 32;
>>> -		val |= i915_gem_obj_ggtt_offset(obj) & 0xfffff000;
>>> +		if (!ggtt_view)
>>> +			ggtt_view = &i915_ggtt_view_normal;
>>> +
>>> +		val =
>>> (uint64_t)((i915_gem_obj_ggtt_offset_view((obj), ggtt_view)
>>> +				  + size - 4096) & 0xfffff000) <<
>>> 32;
>>> +		val |= i915_gem_obj_ggtt_offset_view((obj),
>>> ggtt_view) & 0xfffff000; +
>>
>> Looks like the code can be setting up a fence with a rotated view
>> GGTT address which looks wrong? Is this really what is wanted and why?
>>
>> Regards,
>>
>> Tvrtko
> Well, i915_gem_obj_ggtt_offset() always calls
> i915_gem_obj_ggtt_offset_view() with the default normal view type which
> I suppose is incorrect right? When a rotated view is passed from
> i915_gem_object_pin_to_display_plane(), i915_gem_obj_ggtt_offset_view()
> will not be able to find the associated vma.

I think it is correct since for setting up fences we do want the normal 
view. What would you do that in userspace with fenced rotated view and 
would it even work? And it wouldn't even be predictable which type of 
view you got for your mmap since it would depend on view instantiation 
ordering, no?

I think what might work is that we skip fence creation on rotated view 
pinning, and instead do it when mmap is requested, instantiating a 
normal view at that time if not present.

Regards,

Tvrtko