[PATCH] drm/i915: Unpin framebuffers when crtc is deconfigured.

[PATCH] drm/i915: Unpin framebuffers when crtc is deconfigured.

[Maarten Lankhorst]

When setcrtc is called and steals the last connector away from a crtc
it's turned off because it can't stay configured without connectors.

The framebuffer is still preserved however, and that causes troubles
in the IGT stress test kms_flip.flips-vs-fences which tries to use
as many pins as possible and hangs on the third crtc because of
framebuffer pins on the first 2 crtc's.

Cc: stable@vger.kernel.org #v4.3
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5e520ae5f42e..d95d8acae51f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13571,15 +13571,17 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
+	struct drm_crtc_state *crtc_state;
 	int ret = 0;
 
 	if (!obj && !old_obj)
 		return 0;
 
-	if (old_obj) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_existing_crtc_state(new_state->state, plane->state->crtc);
+	crtc_state = drm_atomic_get_existing_crtc_state(new_state->state,
+							new_state->crtc ?:
+							plane->state->crtc);
 
+	if (old_obj) {
 		/* Big Hammer, we also need to ensure that any pending
 		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
 		 * current scanout is retired before unpinning the old
@@ -13599,7 +13601,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 			return ret;
 	}
 
-	if (!obj) {
+	if (!obj || !crtc_state->enable) {
 		ret = 0;
 	} else if (plane->type == DRM_PLANE_TYPE_CURSOR &&
 	    INTEL_INFO(dev)->cursor_needs_physical) {
@@ -13644,15 +13646,22 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 	struct intel_plane_state *old_intel_state;
 	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
 	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
+	struct drm_crtc_state *old_crtc_state;
 
 	old_intel_state = to_intel_plane_state(old_state);
 
 	if (!obj && !old_obj)
 		return;
 
-	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
-	    !INTEL_INFO(dev)->cursor_needs_physical))
+	old_crtc_state = drm_atomic_get_existing_crtc_state(old_state->state,
+							    old_state->crtc ?:
+							    plane->state->crtc);
+
+	if (old_obj && old_crtc_state->enable &&
+	    (plane->type != DRM_PLANE_TYPE_CURSOR ||
+	     !INTEL_INFO(dev)->cursor_needs_physical)) {
 		intel_unpin_fb_obj(old_state->fb, old_state);
+	}
 
 	/* prepare_fb aborted? */
 	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
-- 
2.1.0

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

Re: [PATCH] drm/i915: Unpin framebuffers when crtc is deconfigured.

[Daniel Vetter]

On Wed, Nov 04, 2015 at 02:43:31PM +0100, Maarten Lankhorst wrote:
> When setcrtc is called and steals the last connector away from a crtc
> it's turned off because it can't stay configured without connectors.
> 
> The framebuffer is still preserved however, and that causes troubles
> in the IGT stress test kms_flip.flips-vs-fences which tries to use
> as many pins as possible and hangs on the third crtc because of
> framebuffer pins on the first 2 crtc's.
> 
> Cc: stable@vger.kernel.org #v4.3
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

drm_atomic_helper_set_config clears the fb for the primary plane if we
clear the mode. Where is the leak?

Also where exactly does it hang - we should have plenty of fences left for
3x2 pinned framebuffers. If you allow more than 2 pinned fb per plane then
that's a bug somewhere with pageflips getting ahead of the unpin worker.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5e520ae5f42e..d95d8acae51f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13571,15 +13571,17 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
> +	struct drm_crtc_state *crtc_state;
>  	int ret = 0;
>  
>  	if (!obj && !old_obj)
>  		return 0;
>  
> -	if (old_obj) {
> -		struct drm_crtc_state *crtc_state =
> -			drm_atomic_get_existing_crtc_state(new_state->state, plane->state->crtc);
> +	crtc_state = drm_atomic_get_existing_crtc_state(new_state->state,
> +							new_state->crtc ?:
> +							plane->state->crtc);
>  
> +	if (old_obj) {
>  		/* Big Hammer, we also need to ensure that any pending
>  		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
>  		 * current scanout is retired before unpinning the old
> @@ -13599,7 +13601,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  			return ret;
>  	}
>  
> -	if (!obj) {
> +	if (!obj || !crtc_state->enable) {
>  		ret = 0;
>  	} else if (plane->type == DRM_PLANE_TYPE_CURSOR &&
>  	    INTEL_INFO(dev)->cursor_needs_physical) {
> @@ -13644,15 +13646,22 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	struct intel_plane_state *old_intel_state;
>  	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
>  	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
> +	struct drm_crtc_state *old_crtc_state;
>  
>  	old_intel_state = to_intel_plane_state(old_state);
>  
>  	if (!obj && !old_obj)
>  		return;
>  
> -	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
> -	    !INTEL_INFO(dev)->cursor_needs_physical))
> +	old_crtc_state = drm_atomic_get_existing_crtc_state(old_state->state,
> +							    old_state->crtc ?:
> +							    plane->state->crtc);
> +
> +	if (old_obj && old_crtc_state->enable &&
> +	    (plane->type != DRM_PLANE_TYPE_CURSOR ||
> +	     !INTEL_INFO(dev)->cursor_needs_physical)) {
>  		intel_unpin_fb_obj(old_state->fb, old_state);
> +	}
>  
>  	/* prepare_fb aborted? */
>  	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH] drm/i915: Unpin framebuffers when crtc is deconfigured.

[Maarten Lankhorst]

Op 17-11-15 om 19:54 schreef Daniel Vetter:
> On Wed, Nov 04, 2015 at 02:43:31PM +0100, Maarten Lankhorst wrote:
>> When setcrtc is called and steals the last connector away from a crtc
>> it's turned off because it can't stay configured without connectors.
>>
>> The framebuffer is still preserved however, and that causes troubles
>> in the IGT stress test kms_flip.flips-vs-fences which tries to use
>> as many pins as possible and hangs on the third crtc because of
>> framebuffer pins on the first 2 crtc's.
>>
>> Cc: stable@vger.kernel.org #v4.3
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> drm_atomic_helper_set_config clears the fb for the primary plane if we
> clear the mode. Where is the leak?
Steal the last connector from another crtc. The crtc is deconfigured because it has no connectors any more, its the planes are left untouched.

> Also where exactly does it hang - we should have plenty of fences left for
> 3x2 pinned framebuffers. If you allow more than 2 pinned fb per plane then
> that's a bug somewhere with pageflips getting ahead of the unpin worker.
> -Daniel
>
Just try that stress test, it submits max_fences-a few so you will get -ENOSPC because it ran out out of fences. :)

~Maarten