[PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Chris Wilson]

A long time ago (before 3.14) we relied on a permanent pinning of the
ifbdev to lock the fb in place inside the GGTT. However, the
introduction of stealing the BIOS framebuffer and reusing its address in
the GGTT for the fbdev has muddied waters and we use an inherited fb.
However, the inherited fb is only pinned whilst it is active and we no
longer have an explicit pin for the info->system_base mmapping used by
the fbdev. The result is that after some aperture pressure the fbdev may
be evicted, but we continue to write the fbcon into the same GGTT
address - overwriting anything else that may be put into that offset.
The effect is most pronounced across suspend/resume as
intel_fbdev_set_suspend() does a full clear over the whole scanout.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Goel, Akash" <akash.goel@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 96476d7d7ed2..082f2938ec97 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	obj = intel_fb->obj;
 	size = obj->base.size;
 
+	/* The fb constructor will have already pinned us (or inherited a
+	 * GGTT region from the BIOS) suitable for a scanout, so
+	 * this should just be a no-op and increment the pin count for the
+	 * fbdev mmapping. It does have a useful side-effect of validating
+	 * the pin for fbdev's use via a GGTT mmapping.
+	 */
+	ret = i915_gem_object_ggtt_pin(obj, NULL, 0, PIN_MAPPABLE);
+	if (ret)
+		goto out_unlock;
+
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
 out_destroy_fbi:
 	drm_fb_helper_release_fbi(helper);
 out_unpin:
+	/* Once for info->screen_base mmaping... */
+	i915_gem_object_ggtt_unpin(obj);
+	/* ...and once for the intel_fb */
 	i915_gem_object_ggtt_unpin(obj);
 	drm_gem_object_unreference(&obj->base);
 out_unlock:
@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
 static void intel_fbdev_destroy(struct drm_device *dev,
 				struct intel_fbdev *ifbdev)
 {
+	/* Release the pinning for the info->screen_base mmaping. */
+	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
 
 	drm_fb_helper_unregister_fbi(&ifbdev->helper);
 	drm_fb_helper_release_fbi(&ifbdev->helper);
-- 
2.5.0

Re: [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Deepak]

On 08/25/2015 10:18 PM, Chris Wilson wrote:
> A long time ago (before 3.14) we relied on a permanent pinning of the
> ifbdev to lock the fb in place inside the GGTT. However, the
> introduction of stealing the BIOS framebuffer and reusing its address in
> the GGTT for the fbdev has muddied waters and we use an inherited fb.
> However, the inherited fb is only pinned whilst it is active and we no
> longer have an explicit pin for the info->system_base mmapping used by
> the fbdev. The result is that after some aperture pressure the fbdev may
> be evicted, but we continue to write the fbcon into the same GGTT
> address - overwriting anything else that may be put into that offset.
> The effect is most pronounced across suspend/resume as
> intel_fbdev_set_suspend() does a full clear over the whole scanout.

Yup this is a critical fix :) by keeping the internal FB pinned we avoid 
alloc of buffer within same FB GTT offset
Reviewed-by: Deepak S <deepak.s@linux.intel.com>

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 96476d7d7ed2..082f2938ec97 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	obj = intel_fb->obj;
>   	size = obj->base.size;
>   
> +	/* The fb constructor will have already pinned us (or inherited a
> +	 * GGTT region from the BIOS) suitable for a scanout, so
> +	 * this should just be a no-op and increment the pin count for the
> +	 * fbdev mmapping. It does have a useful side-effect of validating
> +	 * the pin for fbdev's use via a GGTT mmapping.
> +	 */
> +	ret = i915_gem_object_ggtt_pin(obj, NULL, 0, PIN_MAPPABLE);
> +	if (ret)
> +		goto out_unlock;
> +
>   	info = drm_fb_helper_alloc_fbi(helper);
>   	if (IS_ERR(info)) {
>   		ret = PTR_ERR(info);
> @@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   out_destroy_fbi:
>   	drm_fb_helper_release_fbi(helper);
>   out_unpin:
> +	/* Once for info->screen_base mmaping... */
> +	i915_gem_object_ggtt_unpin(obj);
> +	/* ...and once for the intel_fb */
>   	i915_gem_object_ggtt_unpin(obj);
>   	drm_gem_object_unreference(&obj->base);
>   out_unlock:
> @@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>   static void intel_fbdev_destroy(struct drm_device *dev,
>   				struct intel_fbdev *ifbdev)
>   {
> +	/* Release the pinning for the info->screen_base mmaping. */
> +	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
>   
>   	drm_fb_helper_unregister_fbi(&ifbdev->helper);
>   	drm_fb_helper_release_fbi(&ifbdev->helper);

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

[PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects

[Chris Wilson]

As we mark the preallocated objects as bound, we should also flag them
correctly as being map-and-fenceable (if appropriate!) so that latter
users do not get confused and try and rebind the pinned vma in order to
get a map-and-fenceable binding.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Goel, Akash" <akash.goel@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/i915_gem.c     | 43 +++++++++++++++++++++----------------
 drivers/gpu/drm/i915/i915_gem_gtt.c |  1 +
 3 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 55611d81ec6c..ec731e6db126 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2798,6 +2798,7 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
 
 int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 		  u32 flags);
+void __i915_vma_set_map_and_fenceable(struct i915_vma *vma);
 int __must_check i915_vma_unbind(struct i915_vma *vma);
 int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
 void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 407b6b3576ae..39571e67f9a5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3980,6 +3980,29 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
 	return false;
 }
 
+void __i915_vma_set_map_and_fenceable(struct i915_vma *vma)
+{
+	struct drm_i915_gem_object *obj = vma->obj;
+	bool mappable, fenceable;
+	u32 fence_size, fence_alignment;
+
+	fence_size = i915_gem_get_gtt_size(obj->base.dev,
+					   obj->base.size,
+					   obj->tiling_mode);
+	fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
+						     obj->base.size,
+						     obj->tiling_mode,
+						     true);
+
+	fenceable = (vma->node.size == fence_size &&
+		     (vma->node.start & (fence_alignment - 1)) == 0);
+
+	mappable = (vma->node.start + fence_size <=
+		    to_i915(obj->base.dev)->gtt.mappable_end);
+
+	obj->map_and_fenceable = mappable && fenceable;
+}
+
 static int
 i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
 		       struct i915_address_space *vm,
@@ -4047,25 +4070,7 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
 
 	if (ggtt_view && ggtt_view->type == I915_GGTT_VIEW_NORMAL &&
 	    (bound ^ vma->bound) & GLOBAL_BIND) {
-		bool mappable, fenceable;
-		u32 fence_size, fence_alignment;
-
-		fence_size = i915_gem_get_gtt_size(obj->base.dev,
-						   obj->base.size,
-						   obj->tiling_mode);
-		fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
-							     obj->base.size,
-							     obj->tiling_mode,
-							     true);
-
-		fenceable = (vma->node.size == fence_size &&
-			     (vma->node.start & (fence_alignment - 1)) == 0);
-
-		mappable = (vma->node.start + fence_size <=
-			    dev_priv->gtt.mappable_end);
-
-		obj->map_and_fenceable = mappable && fenceable;
-
+		__i915_vma_set_map_and_fenceable(vma);
 		WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4a76807143b1..112d84c32257 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2586,6 +2586,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
 			return ret;
 		}
 		vma->bound |= GLOBAL_BIND;
+		__i915_vma_set_map_and_fenceable(vma);
 	}
 
 	/* Clear any non-preallocated blocks */
-- 
2.5.0

Re: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects

[Daniel Vetter]

On Wed, Aug 26, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
> As we mark the preallocated objects as bound, we should also flag them
> correctly as being map-and-fenceable (if appropriate!) so that latter
> users do not get confused and try and rebind the pinned vma in order to
> get a map-and-fenceable binding.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: stable@vger.kernel.org

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Jani, can you please pick up both? And some bugzilla references for either
would be great too - Chris?

Oh and does patch 1 fix the execlist resume troubles? Execlist having
bigger contexts might be enough explanations for the apparent regression.

And can we igt patch 1 somehow? E.g. with memory pressure plus doing an
mmap on the legacy fbdev ...
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  drivers/gpu/drm/i915/i915_gem.c     | 43 +++++++++++++++++++++----------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c |  1 +
>  3 files changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 55611d81ec6c..ec731e6db126 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2798,6 +2798,7 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
>  
>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  		  u32 flags);
> +void __i915_vma_set_map_and_fenceable(struct i915_vma *vma);
>  int __must_check i915_vma_unbind(struct i915_vma *vma);
>  int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
>  void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 407b6b3576ae..39571e67f9a5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3980,6 +3980,29 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
>  	return false;
>  }
>  
> +void __i915_vma_set_map_and_fenceable(struct i915_vma *vma)
> +{
> +	struct drm_i915_gem_object *obj = vma->obj;
> +	bool mappable, fenceable;
> +	u32 fence_size, fence_alignment;
> +
> +	fence_size = i915_gem_get_gtt_size(obj->base.dev,
> +					   obj->base.size,
> +					   obj->tiling_mode);
> +	fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
> +						     obj->base.size,
> +						     obj->tiling_mode,
> +						     true);
> +
> +	fenceable = (vma->node.size == fence_size &&
> +		     (vma->node.start & (fence_alignment - 1)) == 0);
> +
> +	mappable = (vma->node.start + fence_size <=
> +		    to_i915(obj->base.dev)->gtt.mappable_end);
> +
> +	obj->map_and_fenceable = mappable && fenceable;
> +}
> +
>  static int
>  i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
>  		       struct i915_address_space *vm,
> @@ -4047,25 +4070,7 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
>  
>  	if (ggtt_view && ggtt_view->type == I915_GGTT_VIEW_NORMAL &&
>  	    (bound ^ vma->bound) & GLOBAL_BIND) {
> -		bool mappable, fenceable;
> -		u32 fence_size, fence_alignment;
> -
> -		fence_size = i915_gem_get_gtt_size(obj->base.dev,
> -						   obj->base.size,
> -						   obj->tiling_mode);
> -		fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
> -							     obj->base.size,
> -							     obj->tiling_mode,
> -							     true);
> -
> -		fenceable = (vma->node.size == fence_size &&
> -			     (vma->node.start & (fence_alignment - 1)) == 0);
> -
> -		mappable = (vma->node.start + fence_size <=
> -			    dev_priv->gtt.mappable_end);
> -
> -		obj->map_and_fenceable = mappable && fenceable;
> -
> +		__i915_vma_set_map_and_fenceable(vma);
>  		WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 4a76807143b1..112d84c32257 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2586,6 +2586,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>  			return ret;
>  		}
>  		vma->bound |= GLOBAL_BIND;
> +		__i915_vma_set_map_and_fenceable(vma);
>  	}
>  
>  	/* Clear any non-preallocated blocks */
> -- 
> 2.5.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects

[Daniel Vetter]

On Wed, Aug 26, 2015 at 03:06:59PM +0200, Daniel Vetter wrote:
> On Wed, Aug 26, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
> > As we mark the preallocated objects as bound, we should also flag them
> > correctly as being map-and-fenceable (if appropriate!) so that latter
> > users do not get confused and try and rebind the pinned vma in order to
> > get a map-and-fenceable binding.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: "Goel, Akash" <akash.goel@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Cc: stable@vger.kernel.org
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Jani, can you please pick up both? And some bugzilla references for either
> would be great too - Chris?
> 
> Oh and does patch 1 fix the execlist resume troubles? Execlist having
> bigger contexts might be enough explanations for the apparent regression.
> 
> And can we igt patch 1 somehow? E.g. with memory pressure plus doing an
> mmap on the legacy fbdev ...

Actually add Jani ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects

[Daniel Vetter]

On Wed, Aug 26, 2015 at 03:06:59PM +0200, Daniel Vetter wrote:
> On Wed, Aug 26, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
> > As we mark the preallocated objects as bound, we should also flag them
> > correctly as being map-and-fenceable (if appropriate!) so that latter
> > users do not get confused and try and rebind the pinned vma in order to
> > get a map-and-fenceable binding.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: "Goel, Akash" <akash.goel@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Cc: stable@vger.kernel.org
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Jani, can you please pick up both? And some bugzilla references for either
> would be great too - Chris?
> 
> Oh and does patch 1 fix the execlist resume troubles? Execlist having
> bigger contexts might be enough explanations for the apparent regression.
> 
> And can we igt patch 1 somehow? E.g. with memory pressure plus doing an
> mmap on the legacy fbdev ...

Actually add Jani ...
-Daniel
-- 
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: Set the map-and-fenceable flag for preallocated objects

[Chris Wilson]

On Wed, Aug 26, 2015 at 03:06:59PM +0200, Daniel Vetter wrote:
> On Wed, Aug 26, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
> > As we mark the preallocated objects as bound, we should also flag them
> > correctly as being map-and-fenceable (if appropriate!) so that latter
> > users do not get confused and try and rebind the pinned vma in order to
> > get a map-and-fenceable binding.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: "Goel, Akash" <akash.goel@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Cc: stable@vger.kernel.org
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Jani, can you please pick up both? And some bugzilla references for either
> would be great too - Chris?

There's a few candidate "overwrite of address 0" bugs, but no way to
really tell if that is the fbcon or userspace without trial and error.
 
> Oh and does patch 1 fix the execlist resume troubles? Execlist having
> bigger contexts might be enough explanations for the apparent regression.

Hmm, plausible.
 
> And can we igt patch 1 somehow? E.g. with memory pressure plus doing an
> mmap on the legacy fbdev ...

Look at i915_gem_framebuffer for the fbcon gtt address before/after
aperture thrashing. Would also need to switch to a user framebuffer to
drop the scanout pinning.

Or just assert that it is still pinned following a modeset away from
fbcon.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects

[Jani Nikula]

On Wed, 26 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Aug 26, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
>> As we mark the preallocated objects as bound, we should also flag them
>> correctly as being map-and-fenceable (if appropriate!) so that latter
>> users do not get confused and try and rebind the pinned vma in order to
>> get a map-and-fenceable binding.
>> 
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: "Goel, Akash" <akash.goel@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Cc: stable@vger.kernel.org
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Jani, can you please pick up both? And some bugzilla references for either
> would be great too - Chris?

Both pushed to drm-intel-next-fixes. Thanks for the patches and review.

BR,
Jani.

>
> Oh and does patch 1 fix the execlist resume troubles? Execlist having
> bigger contexts might be enough explanations for the apparent regression.
>
> And can we igt patch 1 somehow? E.g. with memory pressure plus doing an
> mmap on the legacy fbdev ...
> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>>  drivers/gpu/drm/i915/i915_gem.c     | 43 +++++++++++++++++++++----------------
>>  drivers/gpu/drm/i915/i915_gem_gtt.c |  1 +
>>  3 files changed, 26 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 55611d81ec6c..ec731e6db126 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2798,6 +2798,7 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
>>  
>>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>>  		  u32 flags);
>> +void __i915_vma_set_map_and_fenceable(struct i915_vma *vma);
>>  int __must_check i915_vma_unbind(struct i915_vma *vma);
>>  int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
>>  void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 407b6b3576ae..39571e67f9a5 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3980,6 +3980,29 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
>>  	return false;
>>  }
>>  
>> +void __i915_vma_set_map_and_fenceable(struct i915_vma *vma)
>> +{
>> +	struct drm_i915_gem_object *obj = vma->obj;
>> +	bool mappable, fenceable;
>> +	u32 fence_size, fence_alignment;
>> +
>> +	fence_size = i915_gem_get_gtt_size(obj->base.dev,
>> +					   obj->base.size,
>> +					   obj->tiling_mode);
>> +	fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
>> +						     obj->base.size,
>> +						     obj->tiling_mode,
>> +						     true);
>> +
>> +	fenceable = (vma->node.size == fence_size &&
>> +		     (vma->node.start & (fence_alignment - 1)) == 0);
>> +
>> +	mappable = (vma->node.start + fence_size <=
>> +		    to_i915(obj->base.dev)->gtt.mappable_end);
>> +
>> +	obj->map_and_fenceable = mappable && fenceable;
>> +}
>> +
>>  static int
>>  i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
>>  		       struct i915_address_space *vm,
>> @@ -4047,25 +4070,7 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
>>  
>>  	if (ggtt_view && ggtt_view->type == I915_GGTT_VIEW_NORMAL &&
>>  	    (bound ^ vma->bound) & GLOBAL_BIND) {
>> -		bool mappable, fenceable;
>> -		u32 fence_size, fence_alignment;
>> -
>> -		fence_size = i915_gem_get_gtt_size(obj->base.dev,
>> -						   obj->base.size,
>> -						   obj->tiling_mode);
>> -		fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
>> -							     obj->base.size,
>> -							     obj->tiling_mode,
>> -							     true);
>> -
>> -		fenceable = (vma->node.size == fence_size &&
>> -			     (vma->node.start & (fence_alignment - 1)) == 0);
>> -
>> -		mappable = (vma->node.start + fence_size <=
>> -			    dev_priv->gtt.mappable_end);
>> -
>> -		obj->map_and_fenceable = mappable && fenceable;
>> -
>> +		__i915_vma_set_map_and_fenceable(vma);
>>  		WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
>>  	}
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 4a76807143b1..112d84c32257 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2586,6 +2586,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>>  			return ret;
>>  		}
>>  		vma->bound |= GLOBAL_BIND;
>> +		__i915_vma_set_map_and_fenceable(vma);
>>  	}
>>  
>>  	/* Clear any non-preallocated blocks */
>> -- 
>> 2.5.0
>> 
>
> -- 
> 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

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects

[Jani Nikula]

On Wed, 26 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Aug 26, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
>> As we mark the preallocated objects as bound, we should also flag them
>> correctly as being map-and-fenceable (if appropriate!) so that latter
>> users do not get confused and try and rebind the pinned vma in order to
>> get a map-and-fenceable binding.
>> 
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: "Goel, Akash" <akash.goel@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Cc: stable@vger.kernel.org
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Jani, can you please pick up both? And some bugzilla references for either
> would be great too - Chris?

Both pushed to drm-intel-next-fixes. Thanks for the patches and review.

BR,
Jani.

>
> Oh and does patch 1 fix the execlist resume troubles? Execlist having
> bigger contexts might be enough explanations for the apparent regression.
>
> And can we igt patch 1 somehow? E.g. with memory pressure plus doing an
> mmap on the legacy fbdev ...
> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>>  drivers/gpu/drm/i915/i915_gem.c     | 43 +++++++++++++++++++++----------------
>>  drivers/gpu/drm/i915/i915_gem_gtt.c |  1 +
>>  3 files changed, 26 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 55611d81ec6c..ec731e6db126 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2798,6 +2798,7 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
>>  
>>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>>  		  u32 flags);
>> +void __i915_vma_set_map_and_fenceable(struct i915_vma *vma);
>>  int __must_check i915_vma_unbind(struct i915_vma *vma);
>>  int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
>>  void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 407b6b3576ae..39571e67f9a5 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3980,6 +3980,29 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
>>  	return false;
>>  }
>>  
>> +void __i915_vma_set_map_and_fenceable(struct i915_vma *vma)
>> +{
>> +	struct drm_i915_gem_object *obj = vma->obj;
>> +	bool mappable, fenceable;
>> +	u32 fence_size, fence_alignment;
>> +
>> +	fence_size = i915_gem_get_gtt_size(obj->base.dev,
>> +					   obj->base.size,
>> +					   obj->tiling_mode);
>> +	fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
>> +						     obj->base.size,
>> +						     obj->tiling_mode,
>> +						     true);
>> +
>> +	fenceable = (vma->node.size == fence_size &&
>> +		     (vma->node.start & (fence_alignment - 1)) == 0);
>> +
>> +	mappable = (vma->node.start + fence_size <=
>> +		    to_i915(obj->base.dev)->gtt.mappable_end);
>> +
>> +	obj->map_and_fenceable = mappable && fenceable;
>> +}
>> +
>>  static int
>>  i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
>>  		       struct i915_address_space *vm,
>> @@ -4047,25 +4070,7 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
>>  
>>  	if (ggtt_view && ggtt_view->type == I915_GGTT_VIEW_NORMAL &&
>>  	    (bound ^ vma->bound) & GLOBAL_BIND) {
>> -		bool mappable, fenceable;
>> -		u32 fence_size, fence_alignment;
>> -
>> -		fence_size = i915_gem_get_gtt_size(obj->base.dev,
>> -						   obj->base.size,
>> -						   obj->tiling_mode);
>> -		fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
>> -							     obj->base.size,
>> -							     obj->tiling_mode,
>> -							     true);
>> -
>> -		fenceable = (vma->node.size == fence_size &&
>> -			     (vma->node.start & (fence_alignment - 1)) == 0);
>> -
>> -		mappable = (vma->node.start + fence_size <=
>> -			    dev_priv->gtt.mappable_end);
>> -
>> -		obj->map_and_fenceable = mappable && fenceable;
>> -
>> +		__i915_vma_set_map_and_fenceable(vma);
>>  		WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
>>  	}
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 4a76807143b1..112d84c32257 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2586,6 +2586,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>>  			return ret;
>>  		}
>>  		vma->bound |= GLOBAL_BIND;
>> +		__i915_vma_set_map_and_fenceable(vma);
>>  	}
>>  
>>  	/* Clear any non-preallocated blocks */
>> -- 
>> 2.5.0
>> 
>
> -- 
> 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

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

Re: [Intel-gfx] [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects

[Jesse Barnes]

On 08/27/2015 01:36 AM, Jani Nikula wrote:
> On Wed, 26 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Aug 26, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
>>> As we mark the preallocated objects as bound, we should also flag them
>>> correctly as being map-and-fenceable (if appropriate!) so that latter
>>> users do not get confused and try and rebind the pinned vma in order to
>>> get a map-and-fenceable binding.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: "Goel, Akash" <akash.goel@intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>>> Cc: stable@vger.kernel.org
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Jani, can you please pick up both? And some bugzilla references for either
>> would be great too - Chris?
> 
> Both pushed to drm-intel-next-fixes. Thanks for the patches and review.

This one breaks my HSW.  I hit the warn in 


int
i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
			 const struct i915_ggtt_view *view,
			 uint32_t alignment,
			 uint64_t flags)
{
	if (WARN_ONCE(!view, "no view specified"))
		return -EINVAL;

	return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view,
				      alignment, flags | PIN_GLOBAL);
}

and the fb console doesn't come up.  Is this a merge error somehow?  I don't see how it could have worked... maybe w/o fbdev enabled or something?

Thanks,
Jesse

Re: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects

[Jesse Barnes]

On 08/27/2015 01:36 AM, Jani Nikula wrote:
> On Wed, 26 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Aug 26, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
>>> As we mark the preallocated objects as bound, we should also flag them
>>> correctly as being map-and-fenceable (if appropriate!) so that latter
>>> users do not get confused and try and rebind the pinned vma in order to
>>> get a map-and-fenceable binding.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: "Goel, Akash" <akash.goel@intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>>> Cc: stable@vger.kernel.org
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Jani, can you please pick up both? And some bugzilla references for either
>> would be great too - Chris?
> 
> Both pushed to drm-intel-next-fixes. Thanks for the patches and review.

This one breaks my HSW.  I hit the warn in 


int
i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
			 const struct i915_ggtt_view *view,
			 uint32_t alignment,
			 uint64_t flags)
{
	if (WARN_ONCE(!view, "no view specified"))
		return -EINVAL;

	return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view,
				      alignment, flags | PIN_GLOBAL);
}

and the fb console doesn't come up.  Is this a merge error somehow?  I don't see how it could have worked... maybe w/o fbdev enabled or something?

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

Re: [Intel-gfx] [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects

[Chris Wilson]

On Thu, Aug 27, 2015 at 09:19:18AM -0700, Jesse Barnes wrote:
> On 08/27/2015 01:36 AM, Jani Nikula wrote:
> > On Wed, 26 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> On Wed, Aug 26, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
> >>> As we mark the preallocated objects as bound, we should also flag them
> >>> correctly as being map-and-fenceable (if appropriate!) so that latter
> >>> users do not get confused and try and rebind the pinned vma in order to
> >>> get a map-and-fenceable binding.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: "Goel, Akash" <akash.goel@intel.com>
> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> >>> Cc: stable@vger.kernel.org
> >>
> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >> Jani, can you please pick up both? And some bugzilla references for either
> >> would be great too - Chris?
> > 
> > Both pushed to drm-intel-next-fixes. Thanks for the patches and review.
> 
> This one breaks my HSW.  I hit the warn in 
> 
> 
> int
> i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> 			 const struct i915_ggtt_view *view,
> 			 uint32_t alignment,
> 			 uint64_t flags)
> {
> 	if (WARN_ONCE(!view, "no view specified"))
> 		return -EINVAL;
> 
> 	return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view,
> 				      alignment, flags | PIN_GLOBAL);
> }
> 
> and the fb console doesn't come up.  Is this a merge error somehow?  I don't see how it could have worked... maybe w/o fbdev enabled or something?

No, it was just written against a different pin interface.

Use i915_gem_obj_ggtt_pin() instead.

Exactly.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects

[Jani Nikula]

On Thu, 27 Aug 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Aug 27, 2015 at 09:19:18AM -0700, Jesse Barnes wrote:
>> On 08/27/2015 01:36 AM, Jani Nikula wrote:
>> > On Wed, 26 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> >> On Wed, Aug 26, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
>> >>> As we mark the preallocated objects as bound, we should also flag them
>> >>> correctly as being map-and-fenceable (if appropriate!) so that latter
>> >>> users do not get confused and try and rebind the pinned vma in order to
>> >>> get a map-and-fenceable binding.
>> >>>
>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> >>> Cc: "Goel, Akash" <akash.goel@intel.com>
>> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> >>> Cc: stable@vger.kernel.org
>> >>
>> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >>
>> >> Jani, can you please pick up both? And some bugzilla references for either
>> >> would be great too - Chris?
>> > 
>> > Both pushed to drm-intel-next-fixes. Thanks for the patches and review.
>> 
>> This one breaks my HSW.  I hit the warn in 
>> 
>> 
>> int
>> i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
>> 			 const struct i915_ggtt_view *view,
>> 			 uint32_t alignment,
>> 			 uint64_t flags)
>> {
>> 	if (WARN_ONCE(!view, "no view specified"))
>> 		return -EINVAL;
>> 
>> 	return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view,
>> 				      alignment, flags | PIN_GLOBAL);
>> }
>> 
>> and the fb console doesn't come up.  Is this a merge error somehow?  I don't see how it could have worked... maybe w/o fbdev enabled or something?
>
> No, it was just written against a different pin interface.
>
> Use i915_gem_obj_ggtt_pin() instead.
>
> Exactly.

Both patches dropped from drm-intel-next-fixes.

BR,
Jani.


> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects

[Jani Nikula]

On Thu, 27 Aug 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Aug 27, 2015 at 09:19:18AM -0700, Jesse Barnes wrote:
>> On 08/27/2015 01:36 AM, Jani Nikula wrote:
>> > On Wed, 26 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> >> On Wed, Aug 26, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
>> >>> As we mark the preallocated objects as bound, we should also flag them
>> >>> correctly as being map-and-fenceable (if appropriate!) so that latter
>> >>> users do not get confused and try and rebind the pinned vma in order to
>> >>> get a map-and-fenceable binding.
>> >>>
>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> >>> Cc: "Goel, Akash" <akash.goel@intel.com>
>> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> >>> Cc: stable@vger.kernel.org
>> >>
>> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >>
>> >> Jani, can you please pick up both? And some bugzilla references for either
>> >> would be great too - Chris?
>> > 
>> > Both pushed to drm-intel-next-fixes. Thanks for the patches and review.
>> 
>> This one breaks my HSW.  I hit the warn in 
>> 
>> 
>> int
>> i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
>> 			 const struct i915_ggtt_view *view,
>> 			 uint32_t alignment,
>> 			 uint64_t flags)
>> {
>> 	if (WARN_ONCE(!view, "no view specified"))
>> 		return -EINVAL;
>> 
>> 	return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view,
>> 				      alignment, flags | PIN_GLOBAL);
>> }
>> 
>> and the fb console doesn't come up.  Is this a merge error somehow?  I don't see how it could have worked... maybe w/o fbdev enabled or something?
>
> No, it was just written against a different pin interface.
>
> Use i915_gem_obj_ggtt_pin() instead.
>
> Exactly.

Both patches dropped from drm-intel-next-fixes.

BR,
Jani.


> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

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

Re: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7262
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -1              302/302              301/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                                  283/283              283/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@flip-vs-dpms-interruptible      PASS(1)      DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Boyer, Wayne]

On 8/26/15, 1:23 AM, Deepak <deepak.s@linux.intel.com> wrote:


>
>
>On 08/25/2015 10:18 PM, Chris Wilson wrote:
>> A long time ago (before 3.14) we relied on a permanent pinning of the
>> ifbdev to lock the fb in place inside the GGTT. However, the
>> introduction of stealing the BIOS framebuffer and reusing its address in
>> the GGTT for the fbdev has muddied waters and we use an inherited fb.
>> However, the inherited fb is only pinned whilst it is active and we no
>> longer have an explicit pin for the info->system_base mmapping used by
>> the fbdev. The result is that after some aperture pressure the fbdev may
>> be evicted, but we continue to write the fbcon into the same GGTT
>> address - overwriting anything else that may be put into that offset.
>> The effect is most pronounced across suspend/resume as
>> intel_fbdev_set_suspend() does a full clear over the whole scanout.
>
>Yup this is a critical fix :) by keeping the internal FB pinned we avoid
>alloc of buffer within same FB GTT offset
>Reviewed-by: Deepak S <deepak.s@linux.intel.com>

Daniel,

Is there anything else that needs to happen before this gets pulled in?

Thanks,
Wayne

>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: "Goel, Akash" <akash.goel@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Cc: stable@vger.kernel.org
>> ---
>>   drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
>>b/drivers/gpu/drm/i915/intel_fbdev.c
>> index 96476d7d7ed2..082f2938ec97 100644
>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>> @@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper
>>*helper,
>>   	obj = intel_fb->obj;
>>   	size = obj->base.size;
>>   
>> +	/* The fb constructor will have already pinned us (or inherited a
>> +	 * GGTT region from the BIOS) suitable for a scanout, so
>> +	 * this should just be a no-op and increment the pin count for the
>> +	 * fbdev mmapping. It does have a useful side-effect of validating
>> +	 * the pin for fbdev's use via a GGTT mmapping.
>> +	 */
>> +	ret = i915_gem_object_ggtt_pin(obj, NULL, 0, PIN_MAPPABLE);
>> +	if (ret)
>> +		goto out_unlock;
>> +
>>   	info = drm_fb_helper_alloc_fbi(helper);
>>   	if (IS_ERR(info)) {
>>   		ret = PTR_ERR(info);
>> @@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper
>>*helper,
>>   out_destroy_fbi:
>>   	drm_fb_helper_release_fbi(helper);
>>   out_unpin:
>> +	/* Once for info->screen_base mmaping... */
>> +	i915_gem_object_ggtt_unpin(obj);
>> +	/* ...and once for the intel_fb */
>>   	i915_gem_object_ggtt_unpin(obj);
>>   	drm_gem_object_unreference(&obj->base);
>>   out_unlock:
>> @@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs
>>intel_fb_helper_funcs = {
>>   static void intel_fbdev_destroy(struct drm_device *dev,
>>   				struct intel_fbdev *ifbdev)
>>   {
>> +	/* Release the pinning for the info->screen_base mmaping. */
>> +	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
>>   
>>   	drm_fb_helper_unregister_fbi(&ifbdev->helper);
>>   	drm_fb_helper_release_fbi(&ifbdev->helper);
>
>_______________________________________________
>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: Pin the ifbdev for the info->system_base GGTT mmapping

[Daniel Vetter]

On Fri, Oct 02, 2015 at 10:16:26PM +0000, Boyer, Wayne wrote:
> On 8/26/15, 1:23 AM, Deepak <deepak.s@linux.intel.com> wrote:
> 
> 
> >
> >
> >On 08/25/2015 10:18 PM, Chris Wilson wrote:
> >> A long time ago (before 3.14) we relied on a permanent pinning of the
> >> ifbdev to lock the fb in place inside the GGTT. However, the
> >> introduction of stealing the BIOS framebuffer and reusing its address in
> >> the GGTT for the fbdev has muddied waters and we use an inherited fb.
> >> However, the inherited fb is only pinned whilst it is active and we no
> >> longer have an explicit pin for the info->system_base mmapping used by
> >> the fbdev. The result is that after some aperture pressure the fbdev may
> >> be evicted, but we continue to write the fbcon into the same GGTT
> >> address - overwriting anything else that may be put into that offset.
> >> The effect is most pronounced across suspend/resume as
> >> intel_fbdev_set_suspend() does a full clear over the whole scanout.
> >
> >Yup this is a critical fix :) by keeping the internal FB pinned we avoid
> >alloc of buffer within same FB GTT offset
> >Reviewed-by: Deepak S <deepak.s@linux.intel.com>
> 
> Daniel,
> 
> Is there anything else that needs to happen before this gets pulled in?

Someone needs to send out the rebased-on-upstream version. The patches
have been merged but then had to be dropped again since they where based
on top of Chris' tree.
-Daniel

> 
> Thanks,
> Wayne
> 
> >
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: "Goel, Akash" <akash.goel@intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> Cc: stable@vger.kernel.org
> >> ---
> >>   drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
> >>   1 file changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
> >>b/drivers/gpu/drm/i915/intel_fbdev.c
> >> index 96476d7d7ed2..082f2938ec97 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> >> @@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper
> >>*helper,
> >>   	obj = intel_fb->obj;
> >>   	size = obj->base.size;
> >>   
> >> +	/* The fb constructor will have already pinned us (or inherited a
> >> +	 * GGTT region from the BIOS) suitable for a scanout, so
> >> +	 * this should just be a no-op and increment the pin count for the
> >> +	 * fbdev mmapping. It does have a useful side-effect of validating
> >> +	 * the pin for fbdev's use via a GGTT mmapping.
> >> +	 */
> >> +	ret = i915_gem_object_ggtt_pin(obj, NULL, 0, PIN_MAPPABLE);
> >> +	if (ret)
> >> +		goto out_unlock;
> >> +
> >>   	info = drm_fb_helper_alloc_fbi(helper);
> >>   	if (IS_ERR(info)) {
> >>   		ret = PTR_ERR(info);
> >> @@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper
> >>*helper,
> >>   out_destroy_fbi:
> >>   	drm_fb_helper_release_fbi(helper);
> >>   out_unpin:
> >> +	/* Once for info->screen_base mmaping... */
> >> +	i915_gem_object_ggtt_unpin(obj);
> >> +	/* ...and once for the intel_fb */
> >>   	i915_gem_object_ggtt_unpin(obj);
> >>   	drm_gem_object_unreference(&obj->base);
> >>   out_unlock:
> >> @@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs
> >>intel_fb_helper_funcs = {
> >>   static void intel_fbdev_destroy(struct drm_device *dev,
> >>   				struct intel_fbdev *ifbdev)
> >>   {
> >> +	/* Release the pinning for the info->screen_base mmaping. */
> >> +	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
> >>   
> >>   	drm_fb_helper_unregister_fbi(&ifbdev->helper);
> >>   	drm_fb_helper_release_fbi(&ifbdev->helper);
> >
> >_______________________________________________
> >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

[PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Wayne Boyer]

From: Chris Wilson <chris@chris-wilson.co.uk>

A long time ago (before 3.14) we relied on a permanent pinning of the
ifbdev to lock the fb in place inside the GGTT. However, the
introduction of stealing the BIOS framebuffer and reusing its address in
the GGTT for the fbdev has muddied waters and we use an inherited fb.
However, the inherited fb is only pinned whilst it is active and we no
longer have an explicit pin for the info->system_base mmapping used by
the fbdev. The result is that after some aperture pressure the fbdev may
be evicted, but we continue to write the fbcon into the same GGTT
address - overwriting anything else that may be put into that offset.
The effect is most pronounced across suspend/resume as
intel_fbdev_set_suspend() does a full clear over the whole scanout.

v2: rebased on latest nightly (Wayne)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Goel, Akash" <akash.goel@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: stable@vger.kernel.org
Reviewed-by: Deepak S <deepak.s@linux.intel.com>
Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6532912..c6aa4f9 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	obj = intel_fb->obj;
 	size = obj->base.size;
 
+	/* The fb constructor will have already pinned us (or inherited a
+	 * GGTT region from the BIOS) suitable for a scanout, so
+	 * this should just be a no-op and increment the pin count for the
+	 * fbdev mmapping. It does have a useful side-effect of validating
+	 * the pin for fbdev's use via a GGTT mmapping.
+	 */
+	ret = i915_gem_object_ggtt_pin(obj, NULL, 0, PIN_MAPPABLE);
+	if (ret)
+		goto out_unlock;
+
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
 out_destroy_fbi:
 	drm_fb_helper_release_fbi(helper);
 out_unpin:
+	/* Once for info->screen_base mmaping... */
+	i915_gem_object_ggtt_unpin(obj);
+	/* ...and once for the intel_fb */
 	i915_gem_object_ggtt_unpin(obj);
 	drm_gem_object_unreference(&obj->base);
 out_unlock:
@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
 static void intel_fbdev_destroy(struct drm_device *dev,
 				struct intel_fbdev *ifbdev)
 {
+	/* Release the pinning for the info->screen_base mmaping. */
+	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
 
 	drm_fb_helper_unregister_fbi(&ifbdev->helper);
 	drm_fb_helper_release_fbi(&ifbdev->helper);
-- 
1.9.1

[PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Wayne Boyer]

From: Chris Wilson <chris@chris-wilson.co.uk>

A long time ago (before 3.14) we relied on a permanent pinning of the
ifbdev to lock the fb in place inside the GGTT. However, the
introduction of stealing the BIOS framebuffer and reusing its address in
the GGTT for the fbdev has muddied waters and we use an inherited fb.
However, the inherited fb is only pinned whilst it is active and we no
longer have an explicit pin for the info->system_base mmapping used by
the fbdev. The result is that after some aperture pressure the fbdev may
be evicted, but we continue to write the fbcon into the same GGTT
address - overwriting anything else that may be put into that offset.
The effect is most pronounced across suspend/resume as
intel_fbdev_set_suspend() does a full clear over the whole scanout.

v2: rebased on latest nightly (Wayne)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Goel, Akash" <akash.goel@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: stable@vger.kernel.org
Reviewed-by: Deepak S <deepak.s@linux.intel.com>
Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6532912..c6aa4f9 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	obj = intel_fb->obj;
 	size = obj->base.size;
 
+	/* The fb constructor will have already pinned us (or inherited a
+	 * GGTT region from the BIOS) suitable for a scanout, so
+	 * this should just be a no-op and increment the pin count for the
+	 * fbdev mmapping. It does have a useful side-effect of validating
+	 * the pin for fbdev's use via a GGTT mmapping.
+	 */
+	ret = i915_gem_object_ggtt_pin(obj, NULL, 0, PIN_MAPPABLE);
+	if (ret)
+		goto out_unlock;
+
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
 out_destroy_fbi:
 	drm_fb_helper_release_fbi(helper);
 out_unpin:
+	/* Once for info->screen_base mmaping... */
+	i915_gem_object_ggtt_unpin(obj);
+	/* ...and once for the intel_fb */
 	i915_gem_object_ggtt_unpin(obj);
 	drm_gem_object_unreference(&obj->base);
 out_unlock:
@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
 static void intel_fbdev_destroy(struct drm_device *dev,
 				struct intel_fbdev *ifbdev)
 {
+	/* Release the pinning for the info->screen_base mmaping. */
+	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
 
 	drm_fb_helper_unregister_fbi(&ifbdev->helper);
 	drm_fb_helper_release_fbi(&ifbdev->helper);
-- 
1.9.1

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

Re: [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Chris Wilson]

On Wed, Oct 07, 2015 at 11:34:17AM -0700, Wayne Boyer wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> A long time ago (before 3.14) we relied on a permanent pinning of the
> ifbdev to lock the fb in place inside the GGTT. However, the
> introduction of stealing the BIOS framebuffer and reusing its address in
> the GGTT for the fbdev has muddied waters and we use an inherited fb.
> However, the inherited fb is only pinned whilst it is active and we no
> longer have an explicit pin for the info->system_base mmapping used by
> the fbdev. The result is that after some aperture pressure the fbdev may
> be evicted, but we continue to write the fbcon into the same GGTT
> address - overwriting anything else that may be put into that offset.
> The effect is most pronounced across suspend/resume as
> intel_fbdev_set_suspend() does a full clear over the whole scanout.
> 
> v2: rebased on latest nightly (Wayne)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: stable@vger.kernel.org
> Reviewed-by: Deepak S <deepak.s@linux.intel.com>
> Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6532912..c6aa4f9 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	obj = intel_fb->obj;
>  	size = obj->base.size;
>  
> +	/* The fb constructor will have already pinned us (or inherited a
> +	 * GGTT region from the BIOS) suitable for a scanout, so
> +	 * this should just be a no-op and increment the pin count for the
> +	 * fbdev mmapping. It does have a useful side-effect of validating
> +	 * the pin for fbdev's use via a GGTT mmapping.
> +	 */
> +	ret = i915_gem_object_ggtt_pin(obj, NULL, 0, PIN_MAPPABLE);

This should be i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
At which point I just rage quit.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Jesse Barnes]

On 10/08/2015 02:07 AM, Chris Wilson wrote:
> On Wed, Oct 07, 2015 at 11:34:17AM -0700, Wayne Boyer wrote:
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> A long time ago (before 3.14) we relied on a permanent pinning of the
>> ifbdev to lock the fb in place inside the GGTT. However, the
>> introduction of stealing the BIOS framebuffer and reusing its address in
>> the GGTT for the fbdev has muddied waters and we use an inherited fb.
>> However, the inherited fb is only pinned whilst it is active and we no
>> longer have an explicit pin for the info->system_base mmapping used by
>> the fbdev. The result is that after some aperture pressure the fbdev may
>> be evicted, but we continue to write the fbcon into the same GGTT
>> address - overwriting anything else that may be put into that offset.
>> The effect is most pronounced across suspend/resume as
>> intel_fbdev_set_suspend() does a full clear over the whole scanout.
>>
>> v2: rebased on latest nightly (Wayne)
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: "Goel, Akash" <akash.goel@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Cc: stable@vger.kernel.org
>> Reviewed-by: Deepak S <deepak.s@linux.intel.com>
>> Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
>> index 6532912..c6aa4f9 100644
>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>> @@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
>>  	obj = intel_fb->obj;
>>  	size = obj->base.size;
>>  
>> +	/* The fb constructor will have already pinned us (or inherited a
>> +	 * GGTT region from the BIOS) suitable for a scanout, so
>> +	 * this should just be a no-op and increment the pin count for the
>> +	 * fbdev mmapping. It does have a useful side-effect of validating
>> +	 * the pin for fbdev's use via a GGTT mmapping.
>> +	 */
>> +	ret = i915_gem_object_ggtt_pin(obj, NULL, 0, PIN_MAPPABLE);
> 
> This should be i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> At which point I just rage quit.

LOL GEM naming strikes again!

Jesse

Re: [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Jesse Barnes]

On 10/08/2015 02:07 AM, Chris Wilson wrote:
> On Wed, Oct 07, 2015 at 11:34:17AM -0700, Wayne Boyer wrote:
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> A long time ago (before 3.14) we relied on a permanent pinning of the
>> ifbdev to lock the fb in place inside the GGTT. However, the
>> introduction of stealing the BIOS framebuffer and reusing its address in
>> the GGTT for the fbdev has muddied waters and we use an inherited fb.
>> However, the inherited fb is only pinned whilst it is active and we no
>> longer have an explicit pin for the info->system_base mmapping used by
>> the fbdev. The result is that after some aperture pressure the fbdev may
>> be evicted, but we continue to write the fbcon into the same GGTT
>> address - overwriting anything else that may be put into that offset.
>> The effect is most pronounced across suspend/resume as
>> intel_fbdev_set_suspend() does a full clear over the whole scanout.
>>
>> v2: rebased on latest nightly (Wayne)
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: "Goel, Akash" <akash.goel@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Cc: stable@vger.kernel.org
>> Reviewed-by: Deepak S <deepak.s@linux.intel.com>
>> Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
>> index 6532912..c6aa4f9 100644
>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>> @@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
>>  	obj = intel_fb->obj;
>>  	size = obj->base.size;
>>  
>> +	/* The fb constructor will have already pinned us (or inherited a
>> +	 * GGTT region from the BIOS) suitable for a scanout, so
>> +	 * this should just be a no-op and increment the pin count for the
>> +	 * fbdev mmapping. It does have a useful side-effect of validating
>> +	 * the pin for fbdev's use via a GGTT mmapping.
>> +	 */
>> +	ret = i915_gem_object_ggtt_pin(obj, NULL, 0, PIN_MAPPABLE);
> 
> This should be i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> At which point I just rage quit.

LOL GEM naming strikes again!

Jesse

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

[PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Wayne Boyer]

From: Chris Wilson <chris@chris-wilson.co.uk>

A long time ago (before 3.14) we relied on a permanent pinning of the
ifbdev to lock the fb in place inside the GGTT. However, the
introduction of stealing the BIOS framebuffer and reusing its address in
the GGTT for the fbdev has muddied waters and we use an inherited fb.
However, the inherited fb is only pinned whilst it is active and we no
longer have an explicit pin for the info->system_base mmapping used by
the fbdev. The result is that after some aperture pressure the fbdev may
be evicted, but we continue to write the fbcon into the same GGTT
address - overwriting anything else that may be put into that offset.
The effect is most pronounced across suspend/resume as
intel_fbdev_set_suspend() does a full clear over the whole scanout.

v2: rebased on latest nightly (Wayne)
v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based
on Chris' review. (Wayne)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Goel, Akash" <akash.goel@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: stable@vger.kernel.org
Reviewed-by: Deepak S <deepak.s@linux.intel.com>
Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6532912..0ad46521 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	obj = intel_fb->obj;
 	size = obj->base.size;
 
+	/* The fb constructor will have already pinned us (or inherited a
+	 * GGTT region from the BIOS) suitable for a scanout, so
+	 * this should just be a no-op and increment the pin count for the
+	 * fbdev mmapping. It does have a useful side-effect of validating
+	 * the pin for fbdev's use via a GGTT mmapping.
+	 */
+	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
+	if (ret)
+		goto out_unlock;
+
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
 out_destroy_fbi:
 	drm_fb_helper_release_fbi(helper);
 out_unpin:
+	/* Once for info->screen_base mmaping... */
+	i915_gem_object_ggtt_unpin(obj);
+	/* ...and once for the intel_fb */
 	i915_gem_object_ggtt_unpin(obj);
 	drm_gem_object_unreference(&obj->base);
 out_unlock:
@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
 static void intel_fbdev_destroy(struct drm_device *dev,
 				struct intel_fbdev *ifbdev)
 {
+	/* Release the pinning for the info->screen_base mmaping. */
+	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
 
 	drm_fb_helper_unregister_fbi(&ifbdev->helper);
 	drm_fb_helper_release_fbi(&ifbdev->helper);
-- 
1.9.1

Re: [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Chris Wilson]

On Thu, Oct 08, 2015 at 01:50:21PM -0700, Wayne Boyer wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> A long time ago (before 3.14) we relied on a permanent pinning of the
> ifbdev to lock the fb in place inside the GGTT. However, the
> introduction of stealing the BIOS framebuffer and reusing its address in
> the GGTT for the fbdev has muddied waters and we use an inherited fb.
> However, the inherited fb is only pinned whilst it is active and we no
> longer have an explicit pin for the info->system_base mmapping used by
> the fbdev. The result is that after some aperture pressure the fbdev may
> be evicted, but we continue to write the fbcon into the same GGTT
> address - overwriting anything else that may be put into that offset.
> The effect is most pronounced across suspend/resume as
> intel_fbdev_set_suspend() does a full clear over the whole scanout.
> 
> v2: rebased on latest nightly (Wayne)
> v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based
> on Chris' review. (Wayne)

Note that this patch also depends on the

	drm/i915: Set the map-and-fenceable flag for preallocated objects

fix as well
http://patchwork.freedesktop.org/patch/58026/
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Jani Nikula]

On Fri, 09 Oct 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Oct 08, 2015 at 01:50:21PM -0700, Wayne Boyer wrote:
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>> 
>> A long time ago (before 3.14) we relied on a permanent pinning of the
>> ifbdev to lock the fb in place inside the GGTT. However, the
>> introduction of stealing the BIOS framebuffer and reusing its address in
>> the GGTT for the fbdev has muddied waters and we use an inherited fb.
>> However, the inherited fb is only pinned whilst it is active and we no
>> longer have an explicit pin for the info->system_base mmapping used by
>> the fbdev. The result is that after some aperture pressure the fbdev may
>> be evicted, but we continue to write the fbcon into the same GGTT
>> address - overwriting anything else that may be put into that offset.
>> The effect is most pronounced across suspend/resume as
>> intel_fbdev_set_suspend() does a full clear over the whole scanout.
>> 
>> v2: rebased on latest nightly (Wayne)
>> v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based
>> on Chris' review. (Wayne)
>
> Note that this patch also depends on the
>
> 	drm/i915: Set the map-and-fenceable flag for preallocated objects
>
> fix as well
> http://patchwork.freedesktop.org/patch/58026/

Jesse, please provide your Tested-by on that plus this patch, since you
reported the breakage [1] that got the two patches reverted in the first
place.

Thanks,
Jani.


[1] http://mid.gmane.org/55DF3886.1060001@virtuousgeek.org

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

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Jani Nikula]

On Fri, 09 Oct 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Oct 08, 2015 at 01:50:21PM -0700, Wayne Boyer wrote:
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>> 
>> A long time ago (before 3.14) we relied on a permanent pinning of the
>> ifbdev to lock the fb in place inside the GGTT. However, the
>> introduction of stealing the BIOS framebuffer and reusing its address in
>> the GGTT for the fbdev has muddied waters and we use an inherited fb.
>> However, the inherited fb is only pinned whilst it is active and we no
>> longer have an explicit pin for the info->system_base mmapping used by
>> the fbdev. The result is that after some aperture pressure the fbdev may
>> be evicted, but we continue to write the fbcon into the same GGTT
>> address - overwriting anything else that may be put into that offset.
>> The effect is most pronounced across suspend/resume as
>> intel_fbdev_set_suspend() does a full clear over the whole scanout.
>> 
>> v2: rebased on latest nightly (Wayne)
>> v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based
>> on Chris' review. (Wayne)
>
> Note that this patch also depends on the
>
> 	drm/i915: Set the map-and-fenceable flag for preallocated objects
>
> fix as well
> http://patchwork.freedesktop.org/patch/58026/

Jesse, please provide your Tested-by on that plus this patch, since you
reported the breakage [1] that got the two patches reverted in the first
place.

Thanks,
Jani.


[1] http://mid.gmane.org/55DF3886.1060001@virtuousgeek.org

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

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

Re: [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Dave Gordon]

On 08/10/15 21:50, Wayne Boyer wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
>
> A long time ago (before 3.14) we relied on a permanent pinning of the
> ifbdev to lock the fb in place inside the GGTT. However, the
> introduction of stealing the BIOS framebuffer and reusing its address in
> the GGTT for the fbdev has muddied waters and we use an inherited fb.
> However, the inherited fb is only pinned whilst it is active and we no
> longer have an explicit pin for the info->system_base mmapping used by
> the fbdev. The result is that after some aperture pressure the fbdev may
> be evicted, but we continue to write the fbcon into the same GGTT
> address - overwriting anything else that may be put into that offset.
> The effect is most pronounced across suspend/resume as
> intel_fbdev_set_suspend() does a full clear over the whole scanout.
>
> v2: rebased on latest nightly (Wayne)
> v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based
> on Chris' review. (Wayne)
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: stable@vger.kernel.org
> Reviewed-by: Deepak S <deepak.s@linux.intel.com>
> Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6532912..0ad46521 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	obj = intel_fb->obj;
>   	size = obj->base.size;
>
> +	/* The fb constructor will have already pinned us (or inherited a
> +	 * GGTT region from the BIOS) suitable for a scanout, so
> +	 * this should just be a no-op and increment the pin count for the
> +	 * fbdev mmapping. It does have a useful side-effect of validating
> +	 * the pin for fbdev's use via a GGTT mmapping.
> +	 */
> +	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> +	if (ret)
> +		goto out_unlock;
> +
>   	info = drm_fb_helper_alloc_fbi(helper);
>   	if (IS_ERR(info)) {
>   		ret = PTR_ERR(info);
> @@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   out_destroy_fbi:
>   	drm_fb_helper_release_fbi(helper);
>   out_unpin:
> +	/* Once for info->screen_base mmaping... */
> +	i915_gem_object_ggtt_unpin(obj);
> +	/* ...and once for the intel_fb */
>   	i915_gem_object_ggtt_unpin(obj);
>   	drm_gem_object_unreference(&obj->base);
>   out_unlock:
> @@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>   static void intel_fbdev_destroy(struct drm_device *dev,
>   				struct intel_fbdev *ifbdev)
>   {
> +	/* Release the pinning for the info->screen_base mmaping. */
> +	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
>
>   	drm_fb_helper_unregister_fbi(&ifbdev->helper);
>   	drm_fb_helper_release_fbi(&ifbdev->helper);

Hmm .. pinning now done by i915_gem_obj_ggtt_pin(), but the unpinning 
function is i915_gem_object_ggtt_unpin(). Just the sort of asymmetry 
that helps everyone understand what's going on :(

Could we not have a mass rename of the various i915_gem_obj{ect} 
functions to ONE consistent naming convention? (Personally I prefer 
'obj' because it's shorter, but consistency is more important than 
saving just 3 letters).

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

Re: [Intel-gfx] [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Lukas Wunner]

Hi,

On Thu, Oct 08, 2015 at 01:50:21PM -0700, Wayne Boyer wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> A long time ago (before 3.14) we relied on a permanent pinning of the
> ifbdev to lock the fb in place inside the GGTT. However, the
> introduction of stealing the BIOS framebuffer and reusing its address in
> the GGTT for the fbdev has muddied waters and we use an inherited fb.
> However, the inherited fb is only pinned whilst it is active and we no
> longer have an explicit pin for the info->system_base mmapping used by
> the fbdev. The result is that after some aperture pressure the fbdev may
> be evicted, but we continue to write the fbcon into the same GGTT
> address - overwriting anything else that may be put into that offset.
> The effect is most pronounced across suspend/resume as
> intel_fbdev_set_suspend() does a full clear over the whole scanout.
> 
> v2: rebased on latest nightly (Wayne)
> v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based
> on Chris' review. (Wayne)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: stable@vger.kernel.org
> Reviewed-by: Deepak S <deepak.s@linux.intel.com>
> Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6532912..0ad46521 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	obj = intel_fb->obj;
>  	size = obj->base.size;
>  
> +	/* The fb constructor will have already pinned us (or inherited a
> +	 * GGTT region from the BIOS) suitable for a scanout, so
> +	 * this should just be a no-op and increment the pin count for the
> +	 * fbdev mmapping. It does have a useful side-effect of validating
> +	 * the pin for fbdev's use via a GGTT mmapping.
> +	 */
> +	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> +	if (ret)
> +		goto out_unlock;

I think a DRM_ERROR() should be added here, otherwise if this check fails,
the user will never be notified thereof. It's not sufficient to just
return a negative int because this just gets passed up the call stack to
intel_fbdev_initial_config() which ignores it.

I'm adding DRM_ERROR() invocations to the two existing error conditions in
this function with patch 4 of the series I just posted to intel-gfx:

Link: http://lists.freedesktop.org/archives/intel-gfx/2015-October/078837.html
Message-Id: <e08c113c04191ecd66904423da4f1908f48b5ef6.1445771693.git.lukas@wunner.de>

Best regards,

Lukas


> +
>  	info = drm_fb_helper_alloc_fbi(helper);
>  	if (IS_ERR(info)) {
>  		ret = PTR_ERR(info);
> @@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  out_destroy_fbi:
>  	drm_fb_helper_release_fbi(helper);
>  out_unpin:
> +	/* Once for info->screen_base mmaping... */
> +	i915_gem_object_ggtt_unpin(obj);
> +	/* ...and once for the intel_fb */
>  	i915_gem_object_ggtt_unpin(obj);
>  	drm_gem_object_unreference(&obj->base);
>  out_unlock:
> @@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>  static void intel_fbdev_destroy(struct drm_device *dev,
>  				struct intel_fbdev *ifbdev)
>  {
> +	/* Release the pinning for the info->screen_base mmaping. */
> +	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
>  
>  	drm_fb_helper_unregister_fbi(&ifbdev->helper);
>  	drm_fb_helper_release_fbi(&ifbdev->helper);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Lukas Wunner]

Hi,

On Thu, Oct 08, 2015 at 01:50:21PM -0700, Wayne Boyer wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> A long time ago (before 3.14) we relied on a permanent pinning of the
> ifbdev to lock the fb in place inside the GGTT. However, the
> introduction of stealing the BIOS framebuffer and reusing its address in
> the GGTT for the fbdev has muddied waters and we use an inherited fb.
> However, the inherited fb is only pinned whilst it is active and we no
> longer have an explicit pin for the info->system_base mmapping used by
> the fbdev. The result is that after some aperture pressure the fbdev may
> be evicted, but we continue to write the fbcon into the same GGTT
> address - overwriting anything else that may be put into that offset.
> The effect is most pronounced across suspend/resume as
> intel_fbdev_set_suspend() does a full clear over the whole scanout.
> 
> v2: rebased on latest nightly (Wayne)
> v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based
> on Chris' review. (Wayne)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: stable@vger.kernel.org
> Reviewed-by: Deepak S <deepak.s@linux.intel.com>
> Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6532912..0ad46521 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	obj = intel_fb->obj;
>  	size = obj->base.size;
>  
> +	/* The fb constructor will have already pinned us (or inherited a
> +	 * GGTT region from the BIOS) suitable for a scanout, so
> +	 * this should just be a no-op and increment the pin count for the
> +	 * fbdev mmapping. It does have a useful side-effect of validating
> +	 * the pin for fbdev's use via a GGTT mmapping.
> +	 */
> +	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> +	if (ret)
> +		goto out_unlock;

I think a DRM_ERROR() should be added here, otherwise if this check fails,
the user will never be notified thereof. It's not sufficient to just
return a negative int because this just gets passed up the call stack to
intel_fbdev_initial_config() which ignores it.

I'm adding DRM_ERROR() invocations to the two existing error conditions in
this function with patch 4 of the series I just posted to intel-gfx:

Link: http://lists.freedesktop.org/archives/intel-gfx/2015-October/078837.html
Message-Id: <e08c113c04191ecd66904423da4f1908f48b5ef6.1445771693.git.lukas@wunner.de>

Best regards,

Lukas


> +
>  	info = drm_fb_helper_alloc_fbi(helper);
>  	if (IS_ERR(info)) {
>  		ret = PTR_ERR(info);
> @@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  out_destroy_fbi:
>  	drm_fb_helper_release_fbi(helper);
>  out_unpin:
> +	/* Once for info->screen_base mmaping... */
> +	i915_gem_object_ggtt_unpin(obj);
> +	/* ...and once for the intel_fb */
>  	i915_gem_object_ggtt_unpin(obj);
>  	drm_gem_object_unreference(&obj->base);
>  out_unlock:
> @@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>  static void intel_fbdev_destroy(struct drm_device *dev,
>  				struct intel_fbdev *ifbdev)
>  {
> +	/* Release the pinning for the info->screen_base mmaping. */
> +	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
>  
>  	drm_fb_helper_unregister_fbi(&ifbdev->helper);
>  	drm_fb_helper_release_fbi(&ifbdev->helper);
> -- 
> 1.9.1
> 
> _______________________________________________
> 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: [Intel-gfx] [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Lukas Wunner]

Another observation that occurred to me only after sending away my
previous message (sorry):

On Thu, Oct 08, 2015 at 01:50:21PM -0700, Wayne Boyer wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> A long time ago (before 3.14) we relied on a permanent pinning of the
> ifbdev to lock the fb in place inside the GGTT. However, the
> introduction of stealing the BIOS framebuffer and reusing its address in
> the GGTT for the fbdev has muddied waters and we use an inherited fb.
> However, the inherited fb is only pinned whilst it is active and we no
> longer have an explicit pin for the info->system_base mmapping used by
> the fbdev. The result is that after some aperture pressure the fbdev may
> be evicted, but we continue to write the fbcon into the same GGTT
> address - overwriting anything else that may be put into that offset.
> The effect is most pronounced across suspend/resume as
> intel_fbdev_set_suspend() does a full clear over the whole scanout.

Since this only concerns the case when the fb was inherited from BIOS,
why don't you invoke i915_gem_obj_ggtt_pin() only in this particular case?
It's discernible from the prealloc variable.

I think then you also don't need to call i915_gem_object_ggtt_unpin() twice.


> 
> v2: rebased on latest nightly (Wayne)
> v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based
> on Chris' review. (Wayne)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: stable@vger.kernel.org
> Reviewed-by: Deepak S <deepak.s@linux.intel.com>
> Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6532912..0ad46521 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	obj = intel_fb->obj;
>  	size = obj->base.size;
>  
> +	/* The fb constructor will have already pinned us (or inherited a
> +	 * GGTT region from the BIOS) suitable for a scanout, so
> +	 * this should just be a no-op and increment the pin count for the
> +	 * fbdev mmapping. It does have a useful side-effect of validating
> +	 * the pin for fbdev's use via a GGTT mmapping.
> +	 */
> +	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> +	if (ret)
> +		goto out_unlock;
> +
>  	info = drm_fb_helper_alloc_fbi(helper);
>  	if (IS_ERR(info)) {
>  		ret = PTR_ERR(info);
> @@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  out_destroy_fbi:
>  	drm_fb_helper_release_fbi(helper);
>  out_unpin:
> +	/* Once for info->screen_base mmaping... */
> +	i915_gem_object_ggtt_unpin(obj);
> +	/* ...and once for the intel_fb */
>  	i915_gem_object_ggtt_unpin(obj);
>  	drm_gem_object_unreference(&obj->base);
>  out_unlock:
> @@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>  static void intel_fbdev_destroy(struct drm_device *dev,
>  				struct intel_fbdev *ifbdev)
>  {
> +	/* Release the pinning for the info->screen_base mmaping. */
> +	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
>  
>  	drm_fb_helper_unregister_fbi(&ifbdev->helper);
>  	drm_fb_helper_release_fbi(&ifbdev->helper);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

[Lukas Wunner]

Another observation that occurred to me only after sending away my
previous message (sorry):

On Thu, Oct 08, 2015 at 01:50:21PM -0700, Wayne Boyer wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> A long time ago (before 3.14) we relied on a permanent pinning of the
> ifbdev to lock the fb in place inside the GGTT. However, the
> introduction of stealing the BIOS framebuffer and reusing its address in
> the GGTT for the fbdev has muddied waters and we use an inherited fb.
> However, the inherited fb is only pinned whilst it is active and we no
> longer have an explicit pin for the info->system_base mmapping used by
> the fbdev. The result is that after some aperture pressure the fbdev may
> be evicted, but we continue to write the fbcon into the same GGTT
> address - overwriting anything else that may be put into that offset.
> The effect is most pronounced across suspend/resume as
> intel_fbdev_set_suspend() does a full clear over the whole scanout.

Since this only concerns the case when the fb was inherited from BIOS,
why don't you invoke i915_gem_obj_ggtt_pin() only in this particular case?
It's discernible from the prealloc variable.

I think then you also don't need to call i915_gem_object_ggtt_unpin() twice.


> 
> v2: rebased on latest nightly (Wayne)
> v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based
> on Chris' review. (Wayne)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: stable@vger.kernel.org
> Reviewed-by: Deepak S <deepak.s@linux.intel.com>
> Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6532912..0ad46521 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	obj = intel_fb->obj;
>  	size = obj->base.size;
>  
> +	/* The fb constructor will have already pinned us (or inherited a
> +	 * GGTT region from the BIOS) suitable for a scanout, so
> +	 * this should just be a no-op and increment the pin count for the
> +	 * fbdev mmapping. It does have a useful side-effect of validating
> +	 * the pin for fbdev's use via a GGTT mmapping.
> +	 */
> +	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> +	if (ret)
> +		goto out_unlock;
> +
>  	info = drm_fb_helper_alloc_fbi(helper);
>  	if (IS_ERR(info)) {
>  		ret = PTR_ERR(info);
> @@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  out_destroy_fbi:
>  	drm_fb_helper_release_fbi(helper);
>  out_unpin:
> +	/* Once for info->screen_base mmaping... */
> +	i915_gem_object_ggtt_unpin(obj);
> +	/* ...and once for the intel_fb */
>  	i915_gem_object_ggtt_unpin(obj);
>  	drm_gem_object_unreference(&obj->base);
>  out_unlock:
> @@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>  static void intel_fbdev_destroy(struct drm_device *dev,
>  				struct intel_fbdev *ifbdev)
>  {
> +	/* Release the pinning for the info->screen_base mmaping. */
> +	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
>  
>  	drm_fb_helper_unregister_fbi(&ifbdev->helper);
>  	drm_fb_helper_release_fbi(&ifbdev->helper);
> -- 
> 1.9.1
> 
> _______________________________________________
> 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: Pin the ifbdev for the info->system_base GGTT mmapping

[Daniel Vetter]

On Fri, Oct 23, 2015 at 10:17:44PM +0100, Dave Gordon wrote:
> On 08/10/15 21:50, Wayne Boyer wrote:
> >From: Chris Wilson <chris@chris-wilson.co.uk>
> >
> >A long time ago (before 3.14) we relied on a permanent pinning of the
> >ifbdev to lock the fb in place inside the GGTT. However, the
> >introduction of stealing the BIOS framebuffer and reusing its address in
> >the GGTT for the fbdev has muddied waters and we use an inherited fb.
> >However, the inherited fb is only pinned whilst it is active and we no
> >longer have an explicit pin for the info->system_base mmapping used by
> >the fbdev. The result is that after some aperture pressure the fbdev may
> >be evicted, but we continue to write the fbcon into the same GGTT
> >address - overwriting anything else that may be put into that offset.
> >The effect is most pronounced across suspend/resume as
> >intel_fbdev_set_suspend() does a full clear over the whole scanout.
> >
> >v2: rebased on latest nightly (Wayne)
> >v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based
> >on Chris' review. (Wayne)
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: "Goel, Akash" <akash.goel@intel.com>
> >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> >Cc: stable@vger.kernel.org
> >Reviewed-by: Deepak S <deepak.s@linux.intel.com>
> >Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> >index 6532912..0ad46521 100644
> >--- a/drivers/gpu/drm/i915/intel_fbdev.c
> >+++ b/drivers/gpu/drm/i915/intel_fbdev.c
> >@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  	obj = intel_fb->obj;
> >  	size = obj->base.size;
> >
> >+	/* The fb constructor will have already pinned us (or inherited a
> >+	 * GGTT region from the BIOS) suitable for a scanout, so
> >+	 * this should just be a no-op and increment the pin count for the
> >+	 * fbdev mmapping. It does have a useful side-effect of validating
> >+	 * the pin for fbdev's use via a GGTT mmapping.
> >+	 */
> >+	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> >+	if (ret)
> >+		goto out_unlock;
> >+
> >  	info = drm_fb_helper_alloc_fbi(helper);
> >  	if (IS_ERR(info)) {
> >  		ret = PTR_ERR(info);
> >@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  out_destroy_fbi:
> >  	drm_fb_helper_release_fbi(helper);
> >  out_unpin:
> >+	/* Once for info->screen_base mmaping... */
> >+	i915_gem_object_ggtt_unpin(obj);
> >+	/* ...and once for the intel_fb */
> >  	i915_gem_object_ggtt_unpin(obj);
> >  	drm_gem_object_unreference(&obj->base);
> >  out_unlock:
> >@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
> >  static void intel_fbdev_destroy(struct drm_device *dev,
> >  				struct intel_fbdev *ifbdev)
> >  {
> >+	/* Release the pinning for the info->screen_base mmaping. */
> >+	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
> >
> >  	drm_fb_helper_unregister_fbi(&ifbdev->helper);
> >  	drm_fb_helper_release_fbi(&ifbdev->helper);
> 
> Hmm .. pinning now done by i915_gem_obj_ggtt_pin(), but the unpinning
> function is i915_gem_object_ggtt_unpin(). Just the sort of asymmetry that
> helps everyone understand what's going on :(
> 
> Could we not have a mass rename of the various i915_gem_obj{ect} functions
> to ONE consistent naming convention? (Personally I prefer 'obj' because it's
> shorter, but consistency is more important than saving just 3 letters).

Of course, just needs someone to do it, and make sure to not step onto too
many toes. I'd love if more people actually take charge of gem instead of
piling more on top.
-Daniel
-- 
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: Pin the ifbdev for the info->system_base GGTT mmapping

[Chris Wilson]

On Fri, Oct 30, 2015 at 05:18:15PM +0100, Daniel Vetter wrote:
> On Fri, Oct 23, 2015 at 10:17:44PM +0100, Dave Gordon wrote:
> > On 08/10/15 21:50, Wayne Boyer wrote:
> > >From: Chris Wilson <chris@chris-wilson.co.uk>
> > >
> > >A long time ago (before 3.14) we relied on a permanent pinning of the
> > >ifbdev to lock the fb in place inside the GGTT. However, the
> > >introduction of stealing the BIOS framebuffer and reusing its address in
> > >the GGTT for the fbdev has muddied waters and we use an inherited fb.
> > >However, the inherited fb is only pinned whilst it is active and we no
> > >longer have an explicit pin for the info->system_base mmapping used by
> > >the fbdev. The result is that after some aperture pressure the fbdev may
> > >be evicted, but we continue to write the fbcon into the same GGTT
> > >address - overwriting anything else that may be put into that offset.
> > >The effect is most pronounced across suspend/resume as
> > >intel_fbdev_set_suspend() does a full clear over the whole scanout.
> > >
> > >v2: rebased on latest nightly (Wayne)
> > >v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based
> > >on Chris' review. (Wayne)
> > >
> > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > >Cc: "Goel, Akash" <akash.goel@intel.com>
> > >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > >Cc: stable@vger.kernel.org
> > >Reviewed-by: Deepak S <deepak.s@linux.intel.com>
> > >Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
> > >---
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > >diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > >index 6532912..0ad46521 100644
> > >--- a/drivers/gpu/drm/i915/intel_fbdev.c
> > >+++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > >@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  	obj = intel_fb->obj;
> > >  	size = obj->base.size;
> > >
> > >+	/* The fb constructor will have already pinned us (or inherited a
> > >+	 * GGTT region from the BIOS) suitable for a scanout, so
> > >+	 * this should just be a no-op and increment the pin count for the
> > >+	 * fbdev mmapping. It does have a useful side-effect of validating
> > >+	 * the pin for fbdev's use via a GGTT mmapping.
> > >+	 */
> > >+	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> > >+	if (ret)
> > >+		goto out_unlock;
> > >+
> > >  	info = drm_fb_helper_alloc_fbi(helper);
> > >  	if (IS_ERR(info)) {
> > >  		ret = PTR_ERR(info);
> > >@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  out_destroy_fbi:
> > >  	drm_fb_helper_release_fbi(helper);
> > >  out_unpin:
> > >+	/* Once for info->screen_base mmaping... */
> > >+	i915_gem_object_ggtt_unpin(obj);
> > >+	/* ...and once for the intel_fb */
> > >  	i915_gem_object_ggtt_unpin(obj);
> > >  	drm_gem_object_unreference(&obj->base);
> > >  out_unlock:
> > >@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
> > >  static void intel_fbdev_destroy(struct drm_device *dev,
> > >  				struct intel_fbdev *ifbdev)
> > >  {
> > >+	/* Release the pinning for the info->screen_base mmaping. */
> > >+	i915_gem_object_ggtt_unpin(ifbdev->fb->obj);
> > >
> > >  	drm_fb_helper_unregister_fbi(&ifbdev->helper);
> > >  	drm_fb_helper_release_fbi(&ifbdev->helper);
> > 
> > Hmm .. pinning now done by i915_gem_obj_ggtt_pin(), but the unpinning
> > function is i915_gem_object_ggtt_unpin(). Just the sort of asymmetry that
> > helps everyone understand what's going on :(
> > 
> > Could we not have a mass rename of the various i915_gem_obj{ect} functions
> > to ONE consistent naming convention? (Personally I prefer 'obj' because it's
> > shorter, but consistency is more important than saving just 3 letters).
> 
> Of course, just needs someone to do it, and make sure to not step onto too
> many toes. I'd love if more people actually take charge of gem instead of
> piling more on top.

I have patches to remove as much of the nonsense as I could. I have sent
some of them before, but no one looked at them it seems. Now they are
about 150 patches from the top of the queue.
-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