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

[Vivek Kasireddy]

Hi Tvrtko,
On Tue, 15 Sep 2015 10:29:27 +0100
Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> wrote:

> 
> 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.

Yes, I am using SET_TILING to set Y-tiling on the fb obj. 

> 
> 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.

Skipping fence installation for fb objects with rotated views seems to
fix the issue I am seeing. I'll send out a new patch that just does
this.


Thanks,
Vivek

> 
> Regards,
> 
> Tvrtko