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

[Vivek Kasireddy]

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

-Vivek