[PATCH] Revert "drm/i915: Make prepare_plane_fb fully interruptible."

[PATCH] Revert "drm/i915: Make prepare_plane_fb fully interruptible."

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This reverts commit b26a6b35581c84124bd78b68cc02d171fbd572c9.

commit b26a6b35581c ("drm/i915: Make prepare_plane_fb fully interruptible.")
breaks GPU reset on gen3/4 machines. Go back to to non-interruptible.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7687708..2b70151 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2386,10 +2386,11 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 */
 	intel_runtime_pm_get(dev_priv);
 
+	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined,
 						   pipelined_request, &view);
 	if (ret)
-		goto err_pm;
+		goto err_interruptible;
 
 	/* Install a fence for tiled scan-out. Pre-i965 always needs a
 	 * fence, whereas 965+ only requires a fence if using
@@ -2413,12 +2414,14 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 
 	i915_gem_object_pin_fence(obj);
 
+	dev_priv->mm.interruptible = true;
 	intel_runtime_pm_put(dev_priv);
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin_from_display_plane(obj, &view);
-err_pm:
+err_interruptible:
+	dev_priv->mm.interruptible = true;
 	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
@@ -13487,9 +13490,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	if (!obj && !old_obj)
 		return 0;
 
-	ret = i915_mutex_lock_interruptible(dev);
-	if (ret)
-		return ret;
+	mutex_lock(&dev->struct_mutex);
 
 	if (!obj) {
 		ret = 0;
-- 
2.4.10

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

Re: [PATCH] Revert "drm/i915: Make prepare_plane_fb fully interruptible."

[Maarten Lankhorst]

Hey,

Op 30-10-15 om 22:06 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> This reverts commit b26a6b35581c84124bd78b68cc02d171fbd572c9.
>
> commit b26a6b35581c ("drm/i915: Make prepare_plane_fb fully interruptible.")
> breaks GPU reset on gen3/4 machines. Go back to to non-interruptible.
More context please? I don't think reverting is the way to go here.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Revert "drm/i915: Make prepare_plane_fb fully interruptible."

[Maarten Lankhorst]

Hey,

Op 30-10-15 om 22:06 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> This reverts commit b26a6b35581c84124bd78b68cc02d171fbd572c9.
>
> commit b26a6b35581c ("drm/i915: Make prepare_plane_fb fully interruptible.")
> breaks GPU reset on gen3/4 machines. Go back to to non-interruptible.
>
I've done some digging and by forcing an unconditional modeset during reset I was able to trigger it on my system. It should be fixed by applying the rest of the interruptible
series so we can mask EIO, followed by replacing i915_mutex_lock_interruptible with mutex_lock_interruptible so there will be no waiting for gpu reset.
This is what's causing the deadlock here. :)

~Maarten

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 22c0f8a54053..df6dbbc85855 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3207,9 +3207,11 @@ void intel_prepare_reset(struct drm_device *dev)
 	if (IS_GEN2(dev))
 		return;
 
+#if 0
 	/* reset doesn't touch the display */
 	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
 		return;
+#endif
 
 	drm_modeset_lock_all(dev);
 	/*
@@ -3245,7 +3247,12 @@ void intel_finish_reset(struct drm_device *dev)
 		 * FIXME: Atomic will make this obsolete since we won't schedule
 		 * CS-based flips (which might get lost in gpu resets) any more.
 		 */
+#if 0
 		intel_update_primary_planes(dev);
+#else
+		intel_display_resume(dev);
+		drm_modeset_unlock_all(dev);
+#endif
 		return;
 	}
 
@@ -13174,12 +13181,12 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 			flush_workqueue(dev_priv->wq);
 	}
 
-	ret = i915_mutex_lock_interruptible(dev);
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		return ret;
 
 	ret = drm_atomic_helper_prepare_planes(dev, state);
-	if (!ret && !async) {
+	if (!ret && !async && !i915_reset_in_progress(&dev_priv->gpu_error)) {
 		u32 reset_counter;
 
 		reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);

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

Re: [PATCH] Revert "drm/i915: Make prepare_plane_fb fully interruptible."

[Ville Syrjälä]

On Mon, Nov 02, 2015 at 01:41:03PM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> Op 30-10-15 om 22:06 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > This reverts commit b26a6b35581c84124bd78b68cc02d171fbd572c9.
> >
> > commit b26a6b35581c ("drm/i915: Make prepare_plane_fb fully interruptible.")
> > breaks GPU reset on gen3/4 machines. Go back to to non-interruptible.
> >
> I've done some digging and by forcing an unconditional modeset during reset I was able to trigger it on my system.

Hmm, maybe we should add some kind of debug knob to do just that. That
way we could test most of the gen3/4 reset path with gen5+.

I thought we already had that for load detection, but I may have
imagined it considering that load detection seems to have been 
broken now for a while.

> It should be fixed by applying the rest of the interruptible
> series so we can mask EIO, followed by replacing i915_mutex_lock_interruptible with mutex_lock_interruptible so there will be no waiting for gpu reset.
> This is what's causing the deadlock here. :)
> 
> ~Maarten
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 22c0f8a54053..df6dbbc85855 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3207,9 +3207,11 @@ void intel_prepare_reset(struct drm_device *dev)
>  	if (IS_GEN2(dev))
>  		return;
>  
> +#if 0
>  	/* reset doesn't touch the display */
>  	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
>  		return;
> +#endif
>  
>  	drm_modeset_lock_all(dev);
>  	/*
> @@ -3245,7 +3247,12 @@ void intel_finish_reset(struct drm_device *dev)
>  		 * FIXME: Atomic will make this obsolete since we won't schedule
>  		 * CS-based flips (which might get lost in gpu resets) any more.
>  		 */
> +#if 0
>  		intel_update_primary_planes(dev);
> +#else
> +		intel_display_resume(dev);
> +		drm_modeset_unlock_all(dev);
> +#endif
>  		return;
>  	}
>  
> @@ -13174,12 +13181,12 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>  			flush_workqueue(dev_priv->wq);
>  	}
>  
> -	ret = i915_mutex_lock_interruptible(dev);
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
>  	if (ret)
>  		return ret;
>  
>  	ret = drm_atomic_helper_prepare_planes(dev, state);
> -	if (!ret && !async) {
> +	if (!ret && !async && !i915_reset_in_progress(&dev_priv->gpu_error)) {
>  		u32 reset_counter;
>  
>  		reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);

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

Re: [PATCH] Revert "drm/i915: Make prepare_plane_fb fully interruptible."

[Daniel Vetter]

On Mon, Nov 02, 2015 at 05:38:07PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 02, 2015 at 01:41:03PM +0100, Maarten Lankhorst wrote:
> > Hey,
> > 
> > Op 30-10-15 om 22:06 schreef ville.syrjala@linux.intel.com:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > This reverts commit b26a6b35581c84124bd78b68cc02d171fbd572c9.
> > >
> > > commit b26a6b35581c ("drm/i915: Make prepare_plane_fb fully interruptible.")
> > > breaks GPU reset on gen3/4 machines. Go back to to non-interruptible.
> > >
> > I've done some digging and by forcing an unconditional modeset during reset I was able to trigger it on my system.
> 
> Hmm, maybe we should add some kind of debug knob to do just that. That
> way we could test most of the gen3/4 reset path with gen5+.
> 
> I thought we already had that for load detection, but I may have
> imagined it considering that load detection seems to have been 
> broken now for a while.

We still have it for load detect, including an igt. No one looks at igt
results though :(

Maarten, can you please take care of both of these before pushing more
atomic work? And I guess that means that we should apply the revert first.
So Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> on the revert.

Cheers, Daniel

> 
> > It should be fixed by applying the rest of the interruptible
> > series so we can mask EIO, followed by replacing i915_mutex_lock_interruptible with mutex_lock_interruptible so there will be no waiting for gpu reset.
> > This is what's causing the deadlock here. :)
> > 
> > ~Maarten
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 22c0f8a54053..df6dbbc85855 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3207,9 +3207,11 @@ void intel_prepare_reset(struct drm_device *dev)
> >  	if (IS_GEN2(dev))
> >  		return;
> >  
> > +#if 0
> >  	/* reset doesn't touch the display */
> >  	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
> >  		return;
> > +#endif
> >  
> >  	drm_modeset_lock_all(dev);
> >  	/*
> > @@ -3245,7 +3247,12 @@ void intel_finish_reset(struct drm_device *dev)
> >  		 * FIXME: Atomic will make this obsolete since we won't schedule
> >  		 * CS-based flips (which might get lost in gpu resets) any more.
> >  		 */
> > +#if 0
> >  		intel_update_primary_planes(dev);
> > +#else
> > +		intel_display_resume(dev);
> > +		drm_modeset_unlock_all(dev);
> > +#endif
> >  		return;
> >  	}
> >  
> > @@ -13174,12 +13181,12 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
> >  			flush_workqueue(dev_priv->wq);
> >  	}
> >  
> > -	ret = i915_mutex_lock_interruptible(dev);
> > +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> >  	if (ret)
> >  		return ret;
> >  
> >  	ret = drm_atomic_helper_prepare_planes(dev, state);
> > -	if (!ret && !async) {
> > +	if (!ret && !async && !i915_reset_in_progress(&dev_priv->gpu_error)) {
> >  		u32 reset_counter;
> >  
> >  		reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] Revert "drm/i915: Make prepare_plane_fb fully interruptible."

[Maarten Lankhorst]

Op 17-11-15 om 18:44 schreef Daniel Vetter:
> On Mon, Nov 02, 2015 at 05:38:07PM +0200, Ville Syrjälä wrote:
>> On Mon, Nov 02, 2015 at 01:41:03PM +0100, Maarten Lankhorst wrote:
>>> Hey,
>>>
>>> Op 30-10-15 om 22:06 schreef ville.syrjala@linux.intel.com:
>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>
>>>> This reverts commit b26a6b35581c84124bd78b68cc02d171fbd572c9.
>>>>
>>>> commit b26a6b35581c ("drm/i915: Make prepare_plane_fb fully interruptible.")
>>>> breaks GPU reset on gen3/4 machines. Go back to to non-interruptible.
>>>>
>>> I've done some digging and by forcing an unconditional modeset during reset I was able to trigger it on my system.
>> Hmm, maybe we should add some kind of debug knob to do just that. That
>> way we could test most of the gen3/4 reset path with gen5+.
>>
>> I thought we already had that for load detection, but I may have
>> imagined it considering that load detection seems to have been 
>> broken now for a while.
> We still have it for load detect, including an igt. No one looks at igt
> results though :(
>
> Maarten, can you please take care of both of these before pushing more
> atomic work? And I guess that means that we should apply the revert first.
> So Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> on the revert.
>
> Cheers, Daniel
>
Reset is already fixed upstream so reverting wont help.

What igt is used for load detection then?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Revert "drm/i915: Make prepare_plane_fb fully interruptible."

[Daniel Vetter]

On Wed, Nov 18, 2015 at 09:36:46AM +0100, Maarten Lankhorst wrote:
> Op 17-11-15 om 18:44 schreef Daniel Vetter:
> > On Mon, Nov 02, 2015 at 05:38:07PM +0200, Ville Syrjälä wrote:
> >> On Mon, Nov 02, 2015 at 01:41:03PM +0100, Maarten Lankhorst wrote:
> >>> Hey,
> >>>
> >>> Op 30-10-15 om 22:06 schreef ville.syrjala@linux.intel.com:
> >>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>
> >>>> This reverts commit b26a6b35581c84124bd78b68cc02d171fbd572c9.
> >>>>
> >>>> commit b26a6b35581c ("drm/i915: Make prepare_plane_fb fully interruptible.")
> >>>> breaks GPU reset on gen3/4 machines. Go back to to non-interruptible.
> >>>>
> >>> I've done some digging and by forcing an unconditional modeset during reset I was able to trigger it on my system.
> >> Hmm, maybe we should add some kind of debug knob to do just that. That
> >> way we could test most of the gen3/4 reset path with gen5+.
> >>
> >> I thought we already had that for load detection, but I may have
> >> imagined it considering that load detection seems to have been 
> >> broken now for a while.
> > We still have it for load detect, including an igt. No one looks at igt
> > results though :(
> >
> > Maarten, can you please take care of both of these before pushing more
> > atomic work? And I guess that means that we should apply the revert first.
> > So Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> on the revert.
> >
> > Cheers, Daniel
> >
> Reset is already fixed upstream so reverting wont help.

Sorry, I missed that the patch landed already.

> What igt is used for load detection then?

We're missing it it seems. Iirc Ander was working on it, but only the
debugfs part to force-enable load-detect logic was merged into the kernel.
-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] Revert "drm/i915: Make prepare_plane_fb fully interruptible."

[Ander Conselvan De Oliveira]

On Wed, 2015-11-18 at 09:55 +0100, Daniel Vetter wrote:
> On Wed, Nov 18, 2015 at 09:36:46AM +0100, Maarten Lankhorst wrote:
> > Op 17-11-15 om 18:44 schreef Daniel Vetter:
> > > On Mon, Nov 02, 2015 at 05:38:07PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Nov 02, 2015 at 01:41:03PM +0100, Maarten Lankhorst wrote:
> > > > > Hey,
> > > > > 
> > > > > Op 30-10-15 om 22:06 schreef ville.syrjala@linux.intel.com:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > 
> > > > > > This reverts commit b26a6b35581c84124bd78b68cc02d171fbd572c9.
> > > > > > 
> > > > > > commit b26a6b35581c ("drm/i915: Make prepare_plane_fb fully
> > > > > > interruptible.")
> > > > > > breaks GPU reset on gen3/4 machines. Go back to to non
> > > > > > -interruptible.
> > > > > > 
> > > > > I've done some digging and by forcing an unconditional modeset during
> > > > > reset I was able to trigger it on my system.
> > > > Hmm, maybe we should add some kind of debug knob to do just that. That
> > > > way we could test most of the gen3/4 reset path with gen5+.
> > > > 
> > > > I thought we already had that for load detection, but I may have
> > > > imagined it considering that load detection seems to have been 
> > > > broken now for a while.
> > > We still have it for load detect, including an igt. No one looks at igt
> > > results though :(
> > > 
> > > Maarten, can you please take care of both of these before pushing more
> > > atomic work? And I guess that means that we should apply the revert first.
> > > So Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> on the revert.
> > > 
> > > Cheers, Daniel
> > > 
> > Reset is already fixed upstream so reverting wont help.
> 
> Sorry, I missed that the patch landed already.
> 
> > What igt is used for load detection then?
> 
> We're missing it it seems. Iirc Ander was working on it, but only the
> debugfs part to force-enable load-detect logic was merged into the kernel.

That wasn't me. Perhaps it was Matt?

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