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

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

[Vivek Kasireddy]

From: Vivek Kasireddy <vivek.kasireddy@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.

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew D Roper <matthew.d.roper@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  6 ++--
 drivers/gpu/drm/i915/i915_gem.c            | 13 +++++----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 +--
 drivers/gpu/drm/i915/i915_gem_fence.c      | 47 ++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c       |  4 +--
 5 files changed, 48 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index df37e88..9bd8d11 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3101,10 +3101,12 @@ i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
 }
 
 /* i915_gem_fence.c */
-int __must_check i915_gem_object_get_fence(struct drm_i915_gem_object *obj);
+int __must_check i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
+				      const struct i915_ggtt_view *view);
 int __must_check i915_gem_object_put_fence(struct drm_i915_gem_object *obj);
 
-bool i915_gem_object_pin_fence(struct drm_i915_gem_object *obj);
+bool i915_gem_object_pin_fence(struct drm_i915_gem_object *obj,
+				     const struct i915_ggtt_view *view);
 void i915_gem_object_unpin_fence(struct drm_i915_gem_object *obj);
 
 void i915_gem_restore_fences(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 41263cd..c0c3441 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1789,7 +1789,7 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	if (ret)
 		goto unpin;
 
-	ret = i915_gem_object_get_fence(obj);
+	ret = i915_gem_object_get_fence(obj, NULL);
 	if (ret)
 		goto unpin;
 
@@ -4054,16 +4054,19 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
 			return ret;
 	}
 
-	if (ggtt_view && ggtt_view->type == I915_GGTT_VIEW_NORMAL &&
+	if (ggtt_view && (ggtt_view->type == I915_GGTT_VIEW_NORMAL ||
+	    ggtt_view->type == I915_GGTT_VIEW_ROTATED) &&
 	    (bound ^ vma->bound) & GLOBAL_BIND) {
 		bool mappable, fenceable;
-		u32 fence_size, fence_alignment;
+		u32 fence_size, fence_alignment, view_size;
+
+		view_size = i915_ggtt_view_size(obj, ggtt_view);
 
 		fence_size = i915_gem_get_gtt_size(obj->base.dev,
-						   obj->base.size,
+						   view_size,
 						   obj->tiling_mode);
 		fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
-							     obj->base.size,
+							     view_size,
 							     obj->tiling_mode,
 							     true);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a953d49..1566f88 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -608,11 +608,11 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
 	entry->flags |= __EXEC_OBJECT_HAS_PIN;
 
 	if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) {
-		ret = i915_gem_object_get_fence(obj);
+		ret = i915_gem_object_get_fence(obj, NULL);
 		if (ret)
 			return ret;
 
-		if (i915_gem_object_pin_fence(obj))
+		if (i915_gem_object_pin_fence(obj, NULL))
 			entry->flags |= __EXEC_OBJECT_HAS_FENCE;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c
index 6077dff..336ebee 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence.c
@@ -56,11 +56,13 @@
  */
 
 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;
+
 		val |= (uint64_t)((obj->stride / 128) - 1) << fence_pitch_shift;
 		if (obj->tiling_mode == I915_TILING_Y)
 			val |= 1 << I965_FENCE_TILING_Y_SHIFT;
@@ -196,7 +202,8 @@ inline static bool i915_gem_object_needs_mb(struct drm_i915_gem_object *obj)
 }
 
 static void i915_gem_write_fence(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;
 
@@ -215,7 +222,7 @@ static void i915_gem_write_fence(struct drm_device *dev, int reg,
 	else if (IS_GEN3(dev))
 		i915_write_fence_reg(dev, reg, obj);
 	else if (INTEL_INFO(dev)->gen >= 4)
-		i965_write_fence_reg(dev, reg, obj);
+		i965_write_fence_reg(dev, reg, obj, view);
 
 	/* And similarly be paranoid that no direct access to this region
 	 * is reordered to before the fence is installed.
@@ -232,12 +239,13 @@ static inline int fence_number(struct drm_i915_private *dev_priv,
 
 static void i915_gem_object_update_fence(struct drm_i915_gem_object *obj,
 					 struct drm_i915_fence_reg *fence,
-					 bool enable)
+					 bool enable,
+					 const struct i915_ggtt_view *view)
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
 	int reg = fence_number(dev_priv, fence);
 
-	i915_gem_write_fence(obj->base.dev, reg, enable ? obj : NULL);
+	i915_gem_write_fence(obj->base.dev, reg, enable ? obj : NULL, view);
 
 	if (enable) {
 		obj->fence_reg = reg;
@@ -308,7 +316,7 @@ i915_gem_object_put_fence(struct drm_i915_gem_object *obj)
 		return -EBUSY;
 
 	i915_gem_object_fence_lost(obj);
-	i915_gem_object_update_fence(obj, fence, false);
+	i915_gem_object_update_fence(obj, fence, false, NULL);
 
 	return 0;
 }
@@ -369,7 +377,8 @@ deadlock:
  * 0 on success, negative error code on failure.
  */
 int
-i915_gem_object_get_fence(struct drm_i915_gem_object *obj)
+i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
+				const struct i915_ggtt_view *view)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -414,7 +423,7 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj)
 	} else
 		return 0;
 
-	i915_gem_object_update_fence(obj, reg, enable);
+	i915_gem_object_update_fence(obj, reg, enable, view);
 
 	return 0;
 }
@@ -435,11 +444,18 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj)
  * True if the object has a fence, false otherwise.
  */
 bool
-i915_gem_object_pin_fence(struct drm_i915_gem_object *obj)
+i915_gem_object_pin_fence(struct drm_i915_gem_object *obj,
+				const struct i915_ggtt_view *view)
 {
 	if (obj->fence_reg != I915_FENCE_REG_NONE) {
 		struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
-		struct i915_vma *ggtt_vma = i915_gem_obj_to_ggtt(obj);
+		const struct i915_vma *ggtt_vma;
+
+		if (view)
+			ggtt_vma = i915_gem_obj_to_ggtt_view(obj, view);
+		else
+			ggtt_vma = i915_gem_obj_to_ggtt_view(obj,
+					&i915_ggtt_view_normal);
 
 		WARN_ON(!ggtt_vma ||
 			dev_priv->fence_regs[obj->fence_reg].pin_count >
@@ -489,9 +505,10 @@ void i915_gem_restore_fences(struct drm_device *dev)
 		 */
 		if (reg->obj) {
 			i915_gem_object_update_fence(reg->obj, reg,
-						     reg->obj->tiling_mode);
+						     reg->obj->tiling_mode,
+						     NULL);
 		} else {
-			i915_gem_write_fence(dev, i, NULL);
+			i915_gem_write_fence(dev, i, NULL, NULL);
 		}
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 52fb3f2..b77074f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2357,7 +2357,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 * framebuffer compression.  For simplicity, we always install
 	 * a fence as the cost is not that onerous.
 	 */
-	ret = i915_gem_object_get_fence(obj);
+	ret = i915_gem_object_get_fence(obj, &view);
 	if (ret == -EDEADLK) {
 		/*
 		 * -EDEADLK means there are no free fences
@@ -2372,7 +2372,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	} else if (ret)
 		goto err_unpin;
 
-	i915_gem_object_pin_fence(obj);
+	i915_gem_object_pin_fence(obj, &view);
 
 	dev_priv->mm.interruptible = true;
 	intel_runtime_pm_put(dev_priv);
-- 
2.4.3

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

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

[Chris Wilson]

On Fri, Sep 11, 2015 at 06:44:26PM -0700, Vivek Kasireddy wrote:
> From: Vivek Kasireddy <vivek.kasireddy@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.
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew D Roper <matthew.d.roper@intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>

No. The fenceable decision needs only be made on the vma, which can be
separate for rotated views, partial views etc.

As usual I have such a patch...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

Hi,

On 09/12/2015 02:44 AM, Vivek Kasireddy wrote:
> From: Vivek Kasireddy <vivek.kasireddy@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.

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Vivek Kasireddy]

On Sat, 12 Sep 2015 08:49:08 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, Sep 11, 2015 at 06:44:26PM -0700, Vivek Kasireddy wrote:
> > From: Vivek Kasireddy <vivek.kasireddy@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.
> > 
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Matthew D Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> 
> No. The fenceable decision needs only be made on the vma, which can be
> separate for rotated views, partial views etc.
> 
> As usual I have such a patch...
> -Chris

The fenceable decision is indeed made on the vma; my patch doesn't
change that behavior. I am just fixing the warning raised here:

if (WARN_ON(!obj->map_and_fenceable))

inside i915_gem_object_get_fence(). This warning is raised because
i915_gem_object_do_pin() only looks at normal views and completely
ignores the rotated views for the associated Y-tiled fb objects
that are passed on by i915_gem_object_pin_to_display_plane().

Does your patch solve this problem in a different way?
-Vivek
> 

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

Re: [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@linux.intel.com> wrote:

> 
> Hi,
> 
> On 09/12/2015 02:44 AM, Vivek Kasireddy wrote:
> > From: Vivek Kasireddy <vivek.kasireddy@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

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

Re: [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@linux.intel.com> wrote:
>
>>
>> Hi,
>>
>> On 09/12/2015 02:44 AM, Vivek Kasireddy wrote:
>>> From: Vivek Kasireddy <vivek.kasireddy@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@linux.intel.com> wrote:
> >
> >>
> >> Hi,
> >>
> >> On 09/12/2015 02:44 AM, Vivek Kasireddy wrote:
> >>> From: Vivek Kasireddy <vivek.kasireddy@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

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

[PATCH] drm/i915: Skip fence installation for objects with rotated views

[Vivek Kasireddy]

While pinning a fb object to the display plane, only install a fence
if the object is using a normal view. This corresponds with the
behavior found in i915_gem_object_do_pin() where the fencability
criteria is determined only for objects with normal views.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 52fb3f2..8b3e943 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 * framebuffer compression.  For simplicity, we always install
 	 * a fence as the cost is not that onerous.
 	 */
-	ret = i915_gem_object_get_fence(obj);
+	ret = view.type == I915_GGTT_VIEW_NORMAL ?
+				i915_gem_object_get_fence(obj) : 0;
 	if (ret == -EDEADLK) {
 		/*
 		 * -EDEADLK means there are no free fences
-- 
2.4.3

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

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views

[Chris Wilson]

On Tue, Sep 15, 2015 at 07:05:12PM -0700, Vivek Kasireddy wrote:
> While pinning a fb object to the display plane, only install a fence
> if the object is using a normal view. This corresponds with the
> behavior found in i915_gem_object_do_pin() where the fencability
> criteria is determined only for objects with normal views.
> 
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 52fb3f2..8b3e943 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	 * framebuffer compression.  For simplicity, we always install
>  	 * a fence as the cost is not that onerous.
>  	 */
> -	ret = i915_gem_object_get_fence(obj);
> +	ret = view.type == I915_GGTT_VIEW_NORMAL ?
> +				i915_gem_object_get_fence(obj) : 0;

ret = 0;
if (vma->map_and_fenceable)
	ret = i915_gem_object_get_fence(obj);

is how I wrote it in my patch. One day that will become
i915_vma_get_fence().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views

[Vivek Kasireddy]

On Wed, 16 Sep 2015 09:03:33 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Tue, Sep 15, 2015 at 07:05:12PM -0700, Vivek Kasireddy wrote:
> > While pinning a fb object to the display plane, only install a fence
> > if the object is using a normal view. This corresponds with the
> > behavior found in i915_gem_object_do_pin() where the fencability
> > criteria is determined only for objects with normal views.
> > 
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c index 52fb3f2..8b3e943 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane
> > *plane,
> >  	 * framebuffer compression.  For simplicity, we always
> > install
> >  	 * a fence as the cost is not that onerous.
> >  	 */
> > -	ret = i915_gem_object_get_fence(obj);
> > +	ret = view.type == I915_GGTT_VIEW_NORMAL ?
> > +				i915_gem_object_get_fence(obj) : 0;
> 
> ret = 0;
> if (vma->map_and_fenceable)
> 	ret = i915_gem_object_get_fence(obj);
> 
> is how I wrote it in my patch. One day that will become
> i915_vma_get_fence().
> -Chris
Hi Chris,

Looks like your solution to this problem -- and potentially others --
is better and more comprehensive. When do you plan on sending your
patch out to the mailing list?

Thanks,
Vivek

> 

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

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views

[Tvrtko Ursulin]

On 09/16/2015 06:36 PM, Vivek Kasireddy wrote:
> On Wed, 16 Sep 2015 09:03:33 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
>> On Tue, Sep 15, 2015 at 07:05:12PM -0700, Vivek Kasireddy wrote:
>>> While pinning a fb object to the display plane, only install a fence
>>> if the object is using a normal view. This corresponds with the
>>> behavior found in i915_gem_object_do_pin() where the fencability
>>> criteria is determined only for objects with normal views.
>>>
>>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_display.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>> b/drivers/gpu/drm/i915/intel_display.c index 52fb3f2..8b3e943 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane
>>> *plane,
>>>   	 * framebuffer compression.  For simplicity, we always
>>> install
>>>   	 * a fence as the cost is not that onerous.
>>>   	 */
>>> -	ret = i915_gem_object_get_fence(obj);
>>> +	ret = view.type == I915_GGTT_VIEW_NORMAL ?
>>> +				i915_gem_object_get_fence(obj) : 0;
>>
>> ret = 0;
>> if (vma->map_and_fenceable)
>> 	ret = i915_gem_object_get_fence(obj);
>>
>> is how I wrote it in my patch. One day that will become
>> i915_vma_get_fence().
>> -Chris
> Hi Chris,
>
> Looks like your solution to this problem -- and potentially others --
> is better and more comprehensive. When do you plan on sending your
> patch out to the mailing list?

For a quick fix maybe just respin this patch to use 
obj->map_and_fenceable criteria as Chris suggested?

You should also add a test case for this into igt/kms_addfb_basic.

Regards,

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

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views

[Vivek Kasireddy]

On Thu, 17 Sep 2015 11:25:18 +0100
Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:

> 
> On 09/16/2015 06:36 PM, Vivek Kasireddy wrote:
> > On Wed, 16 Sep 2015 09:03:33 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> >> On Tue, Sep 15, 2015 at 07:05:12PM -0700, Vivek Kasireddy wrote:
> >>> While pinning a fb object to the display plane, only install a
> >>> fence if the object is using a normal view. This corresponds with
> >>> the behavior found in i915_gem_object_do_pin() where the
> >>> fencability criteria is determined only for objects with normal
> >>> views.
> >>>
> >>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/intel_display.c | 3 ++-
> >>>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c
> >>> b/drivers/gpu/drm/i915/intel_display.c index 52fb3f2..8b3e943
> >>> 100644 --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane
> >>> *plane,
> >>>   	 * framebuffer compression.  For simplicity, we always
> >>> install
> >>>   	 * a fence as the cost is not that onerous.
> >>>   	 */
> >>> -	ret = i915_gem_object_get_fence(obj);
> >>> +	ret = view.type == I915_GGTT_VIEW_NORMAL ?
> >>> +				i915_gem_object_get_fence(obj) :
> >>> 0;
> >>
> >> ret = 0;
> >> if (vma->map_and_fenceable)
> >> 	ret = i915_gem_object_get_fence(obj);
> >>
> >> is how I wrote it in my patch. One day that will become
> >> i915_vma_get_fence().
> >> -Chris
> > Hi Chris,
> >
> > Looks like your solution to this problem -- and potentially others
> > -- is better and more comprehensive. When do you plan on sending
> > your patch out to the mailing list?
> 
> For a quick fix maybe just respin this patch to use 
> obj->map_and_fenceable criteria as Chris suggested?
> 
> You should also add a test case for this into igt/kms_addfb_basic.
Hi Tvrtko,
I'll send out a patch soon that will serve as as stop-gap measure until
Chris revamps that part of the code. As far as the igt test case is
concerned, I'll do that in the next few days.


Thanks,
Vivek

> 
> Regards,
> 
> Tvrtko

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

[PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Vivek Kasireddy]

v2:
Look at the object's map_and_fenceable flag to determine whether to
install a fence or not (Chris).

v1:
While pinning a fb object to the display plane, only install a fence
if the object is using a normal view. This corresponds with the
behavior found in i915_gem_object_do_pin() where the fencability
criteria is determined only for objects with normal views.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 52fb3f2..108c000 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 * framebuffer compression.  For simplicity, we always install
 	 * a fence as the cost is not that onerous.
 	 */
-	ret = i915_gem_object_get_fence(obj);
+	if (obj->map_and_fenceable)
+		ret = i915_gem_object_get_fence(obj);
 	if (ret == -EDEADLK) {
 		/*
 		 * -EDEADLK means there are no free fences
-- 
2.4.3

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

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Daniel Vetter]

On Fri, Sep 18, 2015 at 06:57:48PM -0700, Vivek Kasireddy wrote:
> v2:
> Look at the object's map_and_fenceable flag to determine whether to
> install a fence or not (Chris).
> 
> v1:
> While pinning a fb object to the display plane, only install a fence
> if the object is using a normal view. This corresponds with the
> behavior found in i915_gem_object_do_pin() where the fencability
> criteria is determined only for objects with normal views.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>

Where's the igt testcase for this bug that Tvrtko suggested?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 52fb3f2..108c000 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	 * framebuffer compression.  For simplicity, we always install
>  	 * a fence as the cost is not that onerous.
>  	 */
> -	ret = i915_gem_object_get_fence(obj);
> +	if (obj->map_and_fenceable)
> +		ret = i915_gem_object_get_fence(obj);
>  	if (ret == -EDEADLK) {
>  		/*
>  		 * -EDEADLK means there are no free fences
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Vivek]

On Wed, 23 Sep 2015 10:46:39 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Sep 18, 2015 at 06:57:48PM -0700, Vivek Kasireddy wrote:
> > v2:
> > Look at the object's map_and_fenceable flag to determine whether to
> > install a fence or not (Chris).
> > 
> > v1:
> > While pinning a fb object to the display plane, only install a fence
> > if the object is using a normal view. This corresponds with the
> > behavior found in i915_gem_object_do_pin() where the fencability
> > criteria is determined only for objects with normal views.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> 
> Where's the igt testcase for this bug that Tvrtko suggested?
> -Daniel
Hi Daniel,
I'll submit it in the next few days.


Thanks,
Vivek

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c index 52fb3f2..108c000 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane
> > *plane,
> >  	 * framebuffer compression.  For simplicity, we always
> > install
> >  	 * a fence as the cost is not that onerous.
> >  	 */
> > -	ret = i915_gem_object_get_fence(obj);
> > +	if (obj->map_and_fenceable)
> > +		ret = i915_gem_object_get_fence(obj);
> >  	if (ret == -EDEADLK) {
> >  		/*
> >  		 * -EDEADLK means there are no free fences
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Tvrtko Ursulin]

Hi,

On 19/09/15 02:57, Vivek Kasireddy wrote:
> v2:
> Look at the object's map_and_fenceable flag to determine whether to
> install a fence or not (Chris).
>
> v1:
> While pinning a fb object to the display plane, only install a fence
> if the object is using a normal view. This corresponds with the
> behavior found in i915_gem_object_do_pin() where the fencability
> criteria is determined only for objects with normal views.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 52fb3f2..108c000 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>   	 * framebuffer compression.  For simplicity, we always install
>   	 * a fence as the cost is not that onerous.
>   	 */
> -	ret = i915_gem_object_get_fence(obj);
> +	if (obj->map_and_fenceable)
> +		ret = i915_gem_object_get_fence(obj);
>   	if (ret == -EDEADLK) {
>   		/*
>   		 * -EDEADLK means there are no free fences
>

Looks correct to me. Ideally someone with perspective on old platforms, 
like Chris or Daniel could also R-B ?

But the commit message is unusual, v1 block should probably become the 
top section, without the v1 marking.

Regards,

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

[PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Vivek Kasireddy]

While pinning a fb object to the display plane, only install a fence
if the object is using a normal view. This corresponds with the
behavior found in i915_gem_object_do_pin() where the fencability
criteria is determined only for objects with normal views.

v2:
Look at the object's map_and_fenceable flag to determine whether to
install a fence or not (Chris).

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 52fb3f2..108c000 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 * framebuffer compression.  For simplicity, we always install
 	 * a fence as the cost is not that onerous.
 	 */
-	ret = i915_gem_object_get_fence(obj);
+	if (obj->map_and_fenceable)
+		ret = i915_gem_object_get_fence(obj);
 	if (ret == -EDEADLK) {
 		/*
 		 * -EDEADLK means there are no free fences
-- 
2.4.3

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

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Tvrtko Ursulin]

On 27/10/15 01:23, Vivek Kasireddy wrote:
> While pinning a fb object to the display plane, only install a fence
> if the object is using a normal view. This corresponds with the
> behavior found in i915_gem_object_do_pin() where the fencability
> criteria is determined only for objects with normal views.
>
> v2:
> Look at the object's map_and_fenceable flag to determine whether to
> install a fence or not (Chris).
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 52fb3f2..108c000 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>   	 * framebuffer compression.  For simplicity, we always install
>   	 * a fence as the cost is not that onerous.
>   	 */
> -	ret = i915_gem_object_get_fence(obj);
> +	if (obj->map_and_fenceable)
> +		ret = i915_gem_object_get_fence(obj);
>   	if (ret == -EDEADLK) {
>   		/*
>   		 * -EDEADLK means there are no free fences
>

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

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Ville Syrjälä]

On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote:
> While pinning a fb object to the display plane, only install a fence
> if the object is using a normal view. This corresponds with the
> behavior found in i915_gem_object_do_pin() where the fencability
> criteria is determined only for objects with normal views.
> 
> v2:
> Look at the object's map_and_fenceable flag to determine whether to
> install a fence or not (Chris).
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 52fb3f2..108c000 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	 * framebuffer compression.  For simplicity, we always install
>  	 * a fence as the cost is not that onerous.
>  	 */
> -	ret = i915_gem_object_get_fence(obj);
> +	if (obj->map_and_fenceable)

This will now get the fence and pin it for the 90/270 view as well,
even though the fence doesn't even cover that particualr gtt mapping.

> +		ret = i915_gem_object_get_fence(obj);
>  	if (ret == -EDEADLK) {
>  		/*
>  		 * -EDEADLK means there are no free fences
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Tvrtko Ursulin]

On 27/10/15 12:51, Ville Syrjälä wrote:
> On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote:
>> While pinning a fb object to the display plane, only install a fence
>> if the object is using a normal view. This corresponds with the
>> behavior found in i915_gem_object_do_pin() where the fencability
>> criteria is determined only for objects with normal views.
>>
>> v2:
>> Look at the object's map_and_fenceable flag to determine whether to
>> install a fence or not (Chris).
>>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 52fb3f2..108c000 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>>   	 * framebuffer compression.  For simplicity, we always install
>>   	 * a fence as the cost is not that onerous.
>>   	 */
>> -	ret = i915_gem_object_get_fence(obj);
>> +	if (obj->map_and_fenceable)
>
> This will now get the fence and pin it for the 90/270 view as well,
> even though the fence doesn't even cover that particualr gtt mapping.

I don't follow. obj->map_and_fenceable will be true only when normal 
view exists, so this avoids setting up the fence when no normal view 
exists and so avoids the WARN_ON in i915_gem_object_get_fence.

Regards,

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

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Ville Syrjälä]

On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote:
> 
> On 27/10/15 12:51, Ville Syrjälä wrote:
> > On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote:
> >> While pinning a fb object to the display plane, only install a fence
> >> if the object is using a normal view. This corresponds with the
> >> behavior found in i915_gem_object_do_pin() where the fencability
> >> criteria is determined only for objects with normal views.
> >>
> >> v2:
> >> Look at the object's map_and_fenceable flag to determine whether to
> >> install a fence or not (Chris).
> >>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_display.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 52fb3f2..108c000 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> >>   	 * framebuffer compression.  For simplicity, we always install
> >>   	 * a fence as the cost is not that onerous.
> >>   	 */
> >> -	ret = i915_gem_object_get_fence(obj);
> >> +	if (obj->map_and_fenceable)
> >
> > This will now get the fence and pin it for the 90/270 view as well,
> > even though the fence doesn't even cover that particualr gtt mapping.
> 
> I don't follow. obj->map_and_fenceable will be true only when normal 
> view exists, so this avoids setting up the fence when no normal view 
> exists and so avoids the WARN_ON in i915_gem_object_get_fence.

Sure, but it's pointless to use up a fence if all we care about
is the 90/270 mapping.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Tvrtko Ursulin]

On 27/10/15 13:48, Ville Syrjälä wrote:
> On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote:
>>
>> On 27/10/15 12:51, Ville Syrjälä wrote:
>>> On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote:
>>>> While pinning a fb object to the display plane, only install a fence
>>>> if the object is using a normal view. This corresponds with the
>>>> behavior found in i915_gem_object_do_pin() where the fencability
>>>> criteria is determined only for objects with normal views.
>>>>
>>>> v2:
>>>> Look at the object's map_and_fenceable flag to determine whether to
>>>> install a fence or not (Chris).
>>>>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_display.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 52fb3f2..108c000 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>>>>    	 * framebuffer compression.  For simplicity, we always install
>>>>    	 * a fence as the cost is not that onerous.
>>>>    	 */
>>>> -	ret = i915_gem_object_get_fence(obj);
>>>> +	if (obj->map_and_fenceable)
>>>
>>> This will now get the fence and pin it for the 90/270 view as well,
>>> even though the fence doesn't even cover that particualr gtt mapping.
>>
>> I don't follow. obj->map_and_fenceable will be true only when normal
>> view exists, so this avoids setting up the fence when no normal view
>> exists and so avoids the WARN_ON in i915_gem_object_get_fence.
>
> Sure, but it's pointless to use up a fence if all we care about
> is the 90/270 mapping.

After a brief IRC discussion we agreed that the patch doesn't introduce 
any new negative behaviours.

But in general it may be beneficial to revisit the approach of always 
installing fences. Especially as the number of pipes and planes grows 
and if number of fences remains static.

Regards,

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

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Chris Wilson]

On Tue, Oct 27, 2015 at 02:26:55PM +0000, Tvrtko Ursulin wrote:
> 
> On 27/10/15 13:48, Ville Syrjälä wrote:
> >On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 27/10/15 12:51, Ville Syrjälä wrote:
> >>>On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote:
> >>>>While pinning a fb object to the display plane, only install a fence
> >>>>if the object is using a normal view. This corresponds with the
> >>>>behavior found in i915_gem_object_do_pin() where the fencability
> >>>>criteria is determined only for objects with normal views.
> >>>>
> >>>>v2:
> >>>>Look at the object's map_and_fenceable flag to determine whether to
> >>>>install a fence or not (Chris).
> >>>>
> >>>>Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>>>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>Cc: Daniel Vetter <daniel@ffwll.ch>
> >>>>Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >>>>---
> >>>>   drivers/gpu/drm/i915/intel_display.c | 3 ++-
> >>>>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>index 52fb3f2..108c000 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_display.c
> >>>>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>@@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> >>>>   	 * framebuffer compression.  For simplicity, we always install
> >>>>   	 * a fence as the cost is not that onerous.
> >>>>   	 */
> >>>>-	ret = i915_gem_object_get_fence(obj);
> >>>>+	if (obj->map_and_fenceable)
> >>>
> >>>This will now get the fence and pin it for the 90/270 view as well,
> >>>even though the fence doesn't even cover that particualr gtt mapping.
> >>
> >>I don't follow. obj->map_and_fenceable will be true only when normal
> >>view exists, so this avoids setting up the fence when no normal view
> >>exists and so avoids the WARN_ON in i915_gem_object_get_fence.
> >
> >Sure, but it's pointless to use up a fence if all we care about
> >is the 90/270 mapping.
> 
> After a brief IRC discussion we agreed that the patch doesn't
> introduce any new negative behaviours.

Urm, consider

intel_unpin_fb_obj():
	...
	i915_gem_object_unpin_fence(intel_fb_obj(obj));


So if the fb is bound both in rotated and non-rotated modes, we will
have a fence for the object and try to unpin it twice => WARN (Daniel is
being too nice, once upon a time that was rightfully a BUG for a major
screwup).

We want to track the fence state on the vma and associated that vma with
the plane_state so that the tracking doesn't get so easily confused.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Ville Syrjälä]

On Tue, Oct 27, 2015 at 06:03:52PM +0000, Chris Wilson wrote:
> On Tue, Oct 27, 2015 at 02:26:55PM +0000, Tvrtko Ursulin wrote:
> > 
> > On 27/10/15 13:48, Ville Syrjälä wrote:
> > >On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote:
> > >>
> > >>On 27/10/15 12:51, Ville Syrjälä wrote:
> > >>>On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote:
> > >>>>While pinning a fb object to the display plane, only install a fence
> > >>>>if the object is using a normal view. This corresponds with the
> > >>>>behavior found in i915_gem_object_do_pin() where the fencability
> > >>>>criteria is determined only for objects with normal views.
> > >>>>
> > >>>>v2:
> > >>>>Look at the object's map_and_fenceable flag to determine whether to
> > >>>>install a fence or not (Chris).
> > >>>>
> > >>>>Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > >>>>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > >>>>Cc: Daniel Vetter <daniel@ffwll.ch>
> > >>>>Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > >>>>---
> > >>>>   drivers/gpu/drm/i915/intel_display.c | 3 ++-
> > >>>>   1 file changed, 2 insertions(+), 1 deletion(-)
> > >>>>
> > >>>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > >>>>index 52fb3f2..108c000 100644
> > >>>>--- a/drivers/gpu/drm/i915/intel_display.c
> > >>>>+++ b/drivers/gpu/drm/i915/intel_display.c
> > >>>>@@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> > >>>>   	 * framebuffer compression.  For simplicity, we always install
> > >>>>   	 * a fence as the cost is not that onerous.
> > >>>>   	 */
> > >>>>-	ret = i915_gem_object_get_fence(obj);
> > >>>>+	if (obj->map_and_fenceable)
> > >>>
> > >>>This will now get the fence and pin it for the 90/270 view as well,
> > >>>even though the fence doesn't even cover that particualr gtt mapping.
> > >>
> > >>I don't follow. obj->map_and_fenceable will be true only when normal
> > >>view exists, so this avoids setting up the fence when no normal view
> > >>exists and so avoids the WARN_ON in i915_gem_object_get_fence.
> > >
> > >Sure, but it's pointless to use up a fence if all we care about
> > >is the 90/270 mapping.
> > 
> > After a brief IRC discussion we agreed that the patch doesn't
> > introduce any new negative behaviours.
> 
> Urm, consider
> 
> intel_unpin_fb_obj():
> 	...
> 	i915_gem_object_unpin_fence(intel_fb_obj(obj));

We'll have (pointlessly) pinned the fence too, so I think it'll end up
working. I would have just put in view==NORMAL checks myself as an
interim solution to avoid that, but whatever.

> 
> 
> So if the fb is bound both in rotated and non-rotated modes, we will
> have a fence for the object and try to unpin it twice => WARN (Daniel is
> being too nice, once upon a time that was rightfully a BUG for a major
> screwup).
> 
> We want to track the fence state on the vma and associated that vma with
> the plane_state so that the tracking doesn't get so easily confused.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Chris Wilson]

On Tue, Oct 27, 2015 at 08:35:36PM +0200, Ville Syrjälä wrote:
> On Tue, Oct 27, 2015 at 06:03:52PM +0000, Chris Wilson wrote:
> > On Tue, Oct 27, 2015 at 02:26:55PM +0000, Tvrtko Ursulin wrote:
> > > 
> > > On 27/10/15 13:48, Ville Syrjälä wrote:
> > > >On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote:
> > > >>
> > > >>On 27/10/15 12:51, Ville Syrjälä wrote:
> > > >>>On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote:
> > > >>>>While pinning a fb object to the display plane, only install a fence
> > > >>>>if the object is using a normal view. This corresponds with the
> > > >>>>behavior found in i915_gem_object_do_pin() where the fencability
> > > >>>>criteria is determined only for objects with normal views.
> > > >>>>
> > > >>>>v2:
> > > >>>>Look at the object's map_and_fenceable flag to determine whether to
> > > >>>>install a fence or not (Chris).
> > > >>>>
> > > >>>>Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > >>>>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > >>>>Cc: Daniel Vetter <daniel@ffwll.ch>
> > > >>>>Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > >>>>---
> > > >>>>   drivers/gpu/drm/i915/intel_display.c | 3 ++-
> > > >>>>   1 file changed, 2 insertions(+), 1 deletion(-)
> > > >>>>
> > > >>>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > >>>>index 52fb3f2..108c000 100644
> > > >>>>--- a/drivers/gpu/drm/i915/intel_display.c
> > > >>>>+++ b/drivers/gpu/drm/i915/intel_display.c
> > > >>>>@@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> > > >>>>   	 * framebuffer compression.  For simplicity, we always install
> > > >>>>   	 * a fence as the cost is not that onerous.
> > > >>>>   	 */
> > > >>>>-	ret = i915_gem_object_get_fence(obj);
> > > >>>>+	if (obj->map_and_fenceable)
> > > >>>
> > > >>>This will now get the fence and pin it for the 90/270 view as well,
> > > >>>even though the fence doesn't even cover that particualr gtt mapping.
> > > >>
> > > >>I don't follow. obj->map_and_fenceable will be true only when normal
> > > >>view exists, so this avoids setting up the fence when no normal view
> > > >>exists and so avoids the WARN_ON in i915_gem_object_get_fence.
> > > >
> > > >Sure, but it's pointless to use up a fence if all we care about
> > > >is the 90/270 mapping.
> > > 
> > > After a brief IRC discussion we agreed that the patch doesn't
> > > introduce any new negative behaviours.
> > 
> > Urm, consider
> > 
> > intel_unpin_fb_obj():
> > 	...
> > 	i915_gem_object_unpin_fence(intel_fb_obj(obj));
> 
> We'll have (pointlessly) pinned the fence too, so I think it'll end up
> working. I would have just put in view==NORMAL checks myself as an
> interim solution to avoid that, but whatever.

No, it didn't. If we rotated first, we don't get a fence and so don't
pin it. Then we attach an unrotated, grab a fence and pin it. Then we
end up unpinning twice vs a single pin.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Ville Syrjälä]

On Tue, Oct 27, 2015 at 06:47:19PM +0000, Chris Wilson wrote:
> On Tue, Oct 27, 2015 at 08:35:36PM +0200, Ville Syrjälä wrote:
> > On Tue, Oct 27, 2015 at 06:03:52PM +0000, Chris Wilson wrote:
> > > On Tue, Oct 27, 2015 at 02:26:55PM +0000, Tvrtko Ursulin wrote:
> > > > 
> > > > On 27/10/15 13:48, Ville Syrjälä wrote:
> > > > >On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote:
> > > > >>
> > > > >>On 27/10/15 12:51, Ville Syrjälä wrote:
> > > > >>>On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote:
> > > > >>>>While pinning a fb object to the display plane, only install a fence
> > > > >>>>if the object is using a normal view. This corresponds with the
> > > > >>>>behavior found in i915_gem_object_do_pin() where the fencability
> > > > >>>>criteria is determined only for objects with normal views.
> > > > >>>>
> > > > >>>>v2:
> > > > >>>>Look at the object's map_and_fenceable flag to determine whether to
> > > > >>>>install a fence or not (Chris).
> > > > >>>>
> > > > >>>>Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > >>>>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > >>>>Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > >>>>Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > > >>>>---
> > > > >>>>   drivers/gpu/drm/i915/intel_display.c | 3 ++-
> > > > >>>>   1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >>>>
> > > > >>>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > >>>>index 52fb3f2..108c000 100644
> > > > >>>>--- a/drivers/gpu/drm/i915/intel_display.c
> > > > >>>>+++ b/drivers/gpu/drm/i915/intel_display.c
> > > > >>>>@@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> > > > >>>>   	 * framebuffer compression.  For simplicity, we always install
> > > > >>>>   	 * a fence as the cost is not that onerous.
> > > > >>>>   	 */
> > > > >>>>-	ret = i915_gem_object_get_fence(obj);
> > > > >>>>+	if (obj->map_and_fenceable)
> > > > >>>
> > > > >>>This will now get the fence and pin it for the 90/270 view as well,
> > > > >>>even though the fence doesn't even cover that particualr gtt mapping.
> > > > >>
> > > > >>I don't follow. obj->map_and_fenceable will be true only when normal
> > > > >>view exists, so this avoids setting up the fence when no normal view
> > > > >>exists and so avoids the WARN_ON in i915_gem_object_get_fence.
> > > > >
> > > > >Sure, but it's pointless to use up a fence if all we care about
> > > > >is the 90/270 mapping.
> > > > 
> > > > After a brief IRC discussion we agreed that the patch doesn't
> > > > introduce any new negative behaviours.
> > > 
> > > Urm, consider
> > > 
> > > intel_unpin_fb_obj():
> > > 	...
> > > 	i915_gem_object_unpin_fence(intel_fb_obj(obj));
> > 
> > We'll have (pointlessly) pinned the fence too, so I think it'll end up
> > working. I would have just put in view==NORMAL checks myself as an
> > interim solution to avoid that, but whatever.
> 
> No, it didn't. If we rotated first, we don't get a fence and so don't
> pin it. Then we attach an unrotated, grab a fence and pin it. Then we
> end up unpinning twice vs a single pin.

Oh, map_and_fenceable on the _object_ depends whether the normal view is
bound, and yeah it could get bound only after the rotated view got used
for scanout. So view==NORMAL for now seems like good enough solution for
now to me. Anything else means a bigger rework of the code, and I for
one don't want to got there until I've managed to land my already pending
stuff.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v2)

[Tvrtko Ursulin]

On 27/10/15 18:58, Ville Syrjälä wrote:
> On Tue, Oct 27, 2015 at 06:47:19PM +0000, Chris Wilson wrote:
>> On Tue, Oct 27, 2015 at 08:35:36PM +0200, Ville Syrjälä wrote:
>>> On Tue, Oct 27, 2015 at 06:03:52PM +0000, Chris Wilson wrote:
>>>> On Tue, Oct 27, 2015 at 02:26:55PM +0000, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 27/10/15 13:48, Ville Syrjälä wrote:
>>>>>> On Tue, Oct 27, 2015 at 01:34:44PM +0000, Tvrtko Ursulin wrote:
>>>>>>>
>>>>>>> On 27/10/15 12:51, Ville Syrjälä wrote:
>>>>>>>> On Mon, Oct 26, 2015 at 06:23:26PM -0700, Vivek Kasireddy wrote:
>>>>>>>>> While pinning a fb object to the display plane, only install a fence
>>>>>>>>> if the object is using a normal view. This corresponds with the
>>>>>>>>> behavior found in i915_gem_object_do_pin() where the fencability
>>>>>>>>> criteria is determined only for objects with normal views.
>>>>>>>>>
>>>>>>>>> v2:
>>>>>>>>> Look at the object's map_and_fenceable flag to determine whether to
>>>>>>>>> install a fence or not (Chris).
>>>>>>>>>
>>>>>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>>>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>>>>>>>> ---
>>>>>>>>>    drivers/gpu/drm/i915/intel_display.c | 3 ++-
>>>>>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>>>>> index 52fb3f2..108c000 100644
>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>>>>> @@ -2357,7 +2357,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>>>>>>>>>    	 * framebuffer compression.  For simplicity, we always install
>>>>>>>>>    	 * a fence as the cost is not that onerous.
>>>>>>>>>    	 */
>>>>>>>>> -	ret = i915_gem_object_get_fence(obj);
>>>>>>>>> +	if (obj->map_and_fenceable)
>>>>>>>>
>>>>>>>> This will now get the fence and pin it for the 90/270 view as well,
>>>>>>>> even though the fence doesn't even cover that particualr gtt mapping.
>>>>>>>
>>>>>>> I don't follow. obj->map_and_fenceable will be true only when normal
>>>>>>> view exists, so this avoids setting up the fence when no normal view
>>>>>>> exists and so avoids the WARN_ON in i915_gem_object_get_fence.
>>>>>>
>>>>>> Sure, but it's pointless to use up a fence if all we care about
>>>>>> is the 90/270 mapping.
>>>>>
>>>>> After a brief IRC discussion we agreed that the patch doesn't
>>>>> introduce any new negative behaviours.
>>>>
>>>> Urm, consider
>>>>
>>>> intel_unpin_fb_obj():
>>>> 	...
>>>> 	i915_gem_object_unpin_fence(intel_fb_obj(obj));
>>>
>>> We'll have (pointlessly) pinned the fence too, so I think it'll end up
>>> working. I would have just put in view==NORMAL checks myself as an
>>> interim solution to avoid that, but whatever.
>>
>> No, it didn't. If we rotated first, we don't get a fence and so don't
>> pin it. Then we attach an unrotated, grab a fence and pin it. Then we
>> end up unpinning twice vs a single pin.
>
> Oh, map_and_fenceable on the _object_ depends whether the normal view is
> bound, and yeah it could get bound only after the rotated view got used
> for scanout. So view==NORMAL for now seems like good enough solution for
> now to me. Anything else means a bigger rework of the code, and I for
> one don't want to got there until I've managed to land my already pending
> stuff.

Right, so one WARN eliminated, one to go. :)

Vivek would you have the time to add a new test case which will hit a 
WARN in i915_gem_object_unpin_fence with the v2 of your patch and post a 
v3 which fixes it?

Regards,

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

[PATCH] drm/i915: Skip fence installation for objects with rotated views (v3)

[Vivek Kasireddy]

While pinning a fb object to the display plane, only install a fence
if the object is using a normal view. This corresponds with the
behavior found in i915_gem_object_do_pin() where the fencability
criteria is determined only for objects with normal views.

v2:
Look at the object's map_and_fenceable flag to determine whether to
install a fence or not (Chris).

v3:
Pin and unpin a fence only if the current view type is normal.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2fdfca1..ac06f8c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2419,7 +2419,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 * framebuffer compression.  For simplicity, we always install
 	 * a fence as the cost is not that onerous.
 	 */
-	ret = i915_gem_object_get_fence(obj);
+	if (view.type == I915_GGTT_VIEW_NORMAL)
+		ret = i915_gem_object_get_fence(obj);
 	if (ret == -EDEADLK) {
 		/*
 		 * -EDEADLK means there are no free fences
@@ -2460,7 +2461,9 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
 	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
 	WARN_ONCE(ret, "Couldn't get view from plane state!");
 
-	i915_gem_object_unpin_fence(obj);
+	if (view.type == I915_GGTT_VIEW_NORMAL)
+		i915_gem_object_unpin_fence(obj);
+
 	i915_gem_object_unpin_from_display_plane(obj, &view);
 }
 
-- 
2.4.3

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

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v3)

[Tvrtko Ursulin]

On 29/10/15 01:24, Vivek Kasireddy wrote:
> While pinning a fb object to the display plane, only install a fence
> if the object is using a normal view. This corresponds with the
> behavior found in i915_gem_object_do_pin() where the fencability
> criteria is determined only for objects with normal views.
>
> v2:
> Look at the object's map_and_fenceable flag to determine whether to
> install a fence or not (Chris).
>
> v3:
> Pin and unpin a fence only if the current view type is normal.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2fdfca1..ac06f8c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2419,7 +2419,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>   	 * framebuffer compression.  For simplicity, we always install
>   	 * a fence as the cost is not that onerous.
>   	 */
> -	ret = i915_gem_object_get_fence(obj);
> +	if (view.type == I915_GGTT_VIEW_NORMAL)
> +		ret = i915_gem_object_get_fence(obj);
>   	if (ret == -EDEADLK) {
>   		/*
>   		 * -EDEADLK means there are no free fences
> @@ -2460,7 +2461,9 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
>   	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
>   	WARN_ONCE(ret, "Couldn't get view from plane state!");
>
> -	i915_gem_object_unpin_fence(obj);
> +	if (view.type == I915_GGTT_VIEW_NORMAL)
> +		i915_gem_object_unpin_fence(obj);
> +
>   	i915_gem_object_unpin_from_display_plane(obj, &view);
>   }
>
>

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

Ok to merge Chris and Ville ?

Regards,

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

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v3)

[Ville Syrjälä]

On Wed, Oct 28, 2015 at 06:24:19PM -0700, Vivek Kasireddy wrote:
> While pinning a fb object to the display plane, only install a fence
> if the object is using a normal view. This corresponds with the
> behavior found in i915_gem_object_do_pin() where the fencability
> criteria is determined only for objects with normal views.
> 
> v2:
> Look at the object's map_and_fenceable flag to determine whether to
> install a fence or not (Chris).
> 
> v3:
> Pin and unpin a fence only if the current view type is normal.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2fdfca1..ac06f8c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2419,7 +2419,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	 * framebuffer compression.  For simplicity, we always install
>  	 * a fence as the cost is not that onerous.
>  	 */
> -	ret = i915_gem_object_get_fence(obj);
> +	if (view.type == I915_GGTT_VIEW_NORMAL)
> +		ret = i915_gem_object_get_fence(obj);

Missing the same check for the pin_fence().

>  	if (ret == -EDEADLK) {
>  		/*
>  		 * -EDEADLK means there are no free fences
> @@ -2460,7 +2461,9 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
>  	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
>  	WARN_ONCE(ret, "Couldn't get view from plane state!");
>  
> -	i915_gem_object_unpin_fence(obj);
> +	if (view.type == I915_GGTT_VIEW_NORMAL)
> +		i915_gem_object_unpin_fence(obj);
> +
>  	i915_gem_object_unpin_from_display_plane(obj, &view);
>  }
>  
> -- 
> 2.4.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v3)

[Tvrtko Ursulin]

On 29/10/15 13:20, Ville Syrjälä wrote:
> On Wed, Oct 28, 2015 at 06:24:19PM -0700, Vivek Kasireddy wrote:
>> While pinning a fb object to the display plane, only install a fence
>> if the object is using a normal view. This corresponds with the
>> behavior found in i915_gem_object_do_pin() where the fencability
>> criteria is determined only for objects with normal views.
>>
>> v2:
>> Look at the object's map_and_fenceable flag to determine whether to
>> install a fence or not (Chris).
>>
>> v3:
>> Pin and unpin a fence only if the current view type is normal.
>>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 2fdfca1..ac06f8c 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2419,7 +2419,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>>   	 * framebuffer compression.  For simplicity, we always install
>>   	 * a fence as the cost is not that onerous.
>>   	 */
>> -	ret = i915_gem_object_get_fence(obj);
>> +	if (view.type == I915_GGTT_VIEW_NORMAL)
>> +		ret = i915_gem_object_get_fence(obj);
>
> Missing the same check for the pin_fence().

Well spotted. :(

So a fence leak and another testcase needed to exercise it.

Pin normal, then pin rotated, unpin both = leak one fence. Repeat 
num_fences times until no more fences and fail?

Regards,

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

[PATCH] drm/i915: Skip fence installation for objects with rotated views (v4)

[Vivek Kasireddy]

While pinning a fb object to the display plane, only install a fence
if the object is using a normal view. This corresponds with the
behavior found in i915_gem_object_do_pin() where the fencability
criteria is determined only for objects with normal views.

v2:
Look at the object's map_and_fenceable flag to determine whether to
install a fence or not (Chris).

v3:
Pin and unpin a fence only if the current view type is normal.

v4:
Extend the "view type is normal" check for pin_fence as well.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2fdfca1..9c80968 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2419,22 +2419,24 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 * framebuffer compression.  For simplicity, we always install
 	 * a fence as the cost is not that onerous.
 	 */
-	ret = i915_gem_object_get_fence(obj);
-	if (ret == -EDEADLK) {
-		/*
-		 * -EDEADLK means there are no free fences
-		 * no pending flips.
-		 *
-		 * This is propagated to atomic, but it uses
-		 * -EDEADLK to force a locking recovery, so
-		 * change the returned error to -EBUSY.
-		 */
-		ret = -EBUSY;
-		goto err_unpin;
-	} else if (ret)
-		goto err_unpin;
+	if (view.type == I915_GGTT_VIEW_NORMAL) {
+		ret = i915_gem_object_get_fence(obj);
+		if (ret == -EDEADLK) {
+			/*
+			 * -EDEADLK means there are no free fences
+			 * no pending flips.
+			 *
+			 * This is propagated to atomic, but it uses
+			 * -EDEADLK to force a locking recovery, so
+			 * change the returned error to -EBUSY.
+			 */
+			ret = -EBUSY;
+			goto err_unpin;
+		} else if (ret)
+			goto err_unpin;
 
-	i915_gem_object_pin_fence(obj);
+		i915_gem_object_pin_fence(obj);
+	}
 
 	dev_priv->mm.interruptible = true;
 	intel_runtime_pm_put(dev_priv);
@@ -2460,7 +2462,9 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
 	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
 	WARN_ONCE(ret, "Couldn't get view from plane state!");
 
-	i915_gem_object_unpin_fence(obj);
+	if (view.type == I915_GGTT_VIEW_NORMAL)
+		i915_gem_object_unpin_fence(obj);
+
 	i915_gem_object_unpin_from_display_plane(obj, &view);
 }
 
-- 
2.4.3

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

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v4)

[Ville Syrjälä]

On Thu, Oct 29, 2015 at 06:54:38PM -0700, Vivek Kasireddy wrote:
> While pinning a fb object to the display plane, only install a fence
> if the object is using a normal view. This corresponds with the
> behavior found in i915_gem_object_do_pin() where the fencability
> criteria is determined only for objects with normal views.
> 
> v2:
> Look at the object's map_and_fenceable flag to determine whether to
> install a fence or not (Chris).
> 
> v3:
> Pin and unpin a fence only if the current view type is normal.
> 
> v4:
> Extend the "view type is normal" check for pin_fence as well.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>

lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 36 ++++++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2fdfca1..9c80968 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2419,22 +2419,24 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	 * framebuffer compression.  For simplicity, we always install
>  	 * a fence as the cost is not that onerous.
>  	 */
> -	ret = i915_gem_object_get_fence(obj);
> -	if (ret == -EDEADLK) {
> -		/*
> -		 * -EDEADLK means there are no free fences
> -		 * no pending flips.
> -		 *
> -		 * This is propagated to atomic, but it uses
> -		 * -EDEADLK to force a locking recovery, so
> -		 * change the returned error to -EBUSY.
> -		 */
> -		ret = -EBUSY;
> -		goto err_unpin;
> -	} else if (ret)
> -		goto err_unpin;
> +	if (view.type == I915_GGTT_VIEW_NORMAL) {
> +		ret = i915_gem_object_get_fence(obj);
> +		if (ret == -EDEADLK) {
> +			/*
> +			 * -EDEADLK means there are no free fences
> +			 * no pending flips.
> +			 *
> +			 * This is propagated to atomic, but it uses
> +			 * -EDEADLK to force a locking recovery, so
> +			 * change the returned error to -EBUSY.
> +			 */
> +			ret = -EBUSY;
> +			goto err_unpin;
> +		} else if (ret)
> +			goto err_unpin;
>  
> -	i915_gem_object_pin_fence(obj);
> +		i915_gem_object_pin_fence(obj);
> +	}
>  
>  	dev_priv->mm.interruptible = true;
>  	intel_runtime_pm_put(dev_priv);
> @@ -2460,7 +2462,9 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
>  	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
>  	WARN_ONCE(ret, "Couldn't get view from plane state!");
>  
> -	i915_gem_object_unpin_fence(obj);
> +	if (view.type == I915_GGTT_VIEW_NORMAL)
> +		i915_gem_object_unpin_fence(obj);
> +
>  	i915_gem_object_unpin_from_display_plane(obj, &view);
>  }
>  
> -- 
> 2.4.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Skip fence installation for objects with rotated views (v4)

[Jani Nikula]

On Fri, 30 Oct 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Oct 29, 2015 at 06:54:38PM -0700, Vivek Kasireddy wrote:
>> While pinning a fb object to the display plane, only install a fence
>> if the object is using a normal view. This corresponds with the
>> behavior found in i915_gem_object_do_pin() where the fencability
>> criteria is determined only for objects with normal views.
>> 
>> v2:
>> Look at the object's map_and_fenceable flag to determine whether to
>> install a fence or not (Chris).
>> 
>> v3:
>> Pin and unpin a fence only if the current view type is normal.
>> 
>> v4:
>> Extend the "view type is normal" check for pin_fence as well.
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>
> lgtm
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pushed to drm-intel-next-fixes, thanks for the patch and review.

BR,
Jani.

>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 36 ++++++++++++++++++++----------------
>>  1 file changed, 20 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 2fdfca1..9c80968 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2419,22 +2419,24 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>>  	 * framebuffer compression.  For simplicity, we always install
>>  	 * a fence as the cost is not that onerous.
>>  	 */
>> -	ret = i915_gem_object_get_fence(obj);
>> -	if (ret == -EDEADLK) {
>> -		/*
>> -		 * -EDEADLK means there are no free fences
>> -		 * no pending flips.
>> -		 *
>> -		 * This is propagated to atomic, but it uses
>> -		 * -EDEADLK to force a locking recovery, so
>> -		 * change the returned error to -EBUSY.
>> -		 */
>> -		ret = -EBUSY;
>> -		goto err_unpin;
>> -	} else if (ret)
>> -		goto err_unpin;
>> +	if (view.type == I915_GGTT_VIEW_NORMAL) {
>> +		ret = i915_gem_object_get_fence(obj);
>> +		if (ret == -EDEADLK) {
>> +			/*
>> +			 * -EDEADLK means there are no free fences
>> +			 * no pending flips.
>> +			 *
>> +			 * This is propagated to atomic, but it uses
>> +			 * -EDEADLK to force a locking recovery, so
>> +			 * change the returned error to -EBUSY.
>> +			 */
>> +			ret = -EBUSY;
>> +			goto err_unpin;
>> +		} else if (ret)
>> +			goto err_unpin;
>>  
>> -	i915_gem_object_pin_fence(obj);
>> +		i915_gem_object_pin_fence(obj);
>> +	}
>>  
>>  	dev_priv->mm.interruptible = true;
>>  	intel_runtime_pm_put(dev_priv);
>> @@ -2460,7 +2462,9 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
>>  	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
>>  	WARN_ONCE(ret, "Couldn't get view from plane state!");
>>  
>> -	i915_gem_object_unpin_fence(obj);
>> +	if (view.type == I915_GGTT_VIEW_NORMAL)
>> +		i915_gem_object_unpin_fence(obj);
>> +
>>  	i915_gem_object_unpin_from_display_plane(obj, &view);
>>  }
>>  
>> -- 
>> 2.4.3

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx