From: Wei Yongjun <[email protected]>
Add the missing unlock before return from function i915_gem_init_hw()
in the error handling case.
Signed-off-by: Wei Yongjun <[email protected]>
---
drivers/gpu/drm/i915/i915_gem.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d2ba315..3eeb2d0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4879,8 +4879,10 @@ i915_gem_init_hw(struct drm_device *dev)
i915_gem_init_swizzling(dev);
ret = dev_priv->gt.init_rings(dev);
- if (ret)
+ if (ret) {
+ mutex_unlock(&dev->struct_mutex);
return ret;
+ }
for (i = 0; i < NUM_L3_SLICES(dev); i++)
i915_gem_l3_remap(&dev_priv->ring[RCS], i);
Wei Yongjun,
On Fri, Dec 05, 2014 at 08:55:59AM +0800, [email protected] wrote:
> From: Wei Yongjun <[email protected]>
>
> Add the missing unlock before return from function i915_gem_init_hw()
> in the error handling case.
>
> Signed-off-by: Wei Yongjun <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d2ba315..3eeb2d0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4879,8 +4879,10 @@ i915_gem_init_hw(struct drm_device *dev)
> i915_gem_init_swizzling(dev);
>
> ret = dev_priv->gt.init_rings(dev);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&dev->struct_mutex);
> return ret;
> + }
>
There are other places in i915_gem_init_hw() where it returns without
unlocking the mutex. Why is it only necessary here and not any of the
other places?
> for (i = 0; i < NUM_L3_SLICES(dev); i++)
> i915_gem_l3_remap(&dev_priv->ring[RCS], i);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
- Jeremiah Mahler
On Fri, Dec 05, 2014 at 05:03:14AM -0800, Jeremiah Mahler wrote:
> Jani,
>
> On Fri, Dec 05, 2014 at 02:17:42PM +0200, Jani Nikula wrote:
> > Release struct_mutex if init_rings() fails.
> >
> > This is a regression introduced in
> > commit 35a57ffbb10840af219eeaf64718434242bb7c76
> > Author: Daniel Vetter <[email protected]>
> > Date: Thu Nov 20 00:33:07 2014 +0100
> >
> > drm/i915: Only init engines once
> >
> > Reported-by: Wei Yongjun <[email protected]>
> > Signed-off-by: Jani Nikula <[email protected]>
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 16 +++++++---------
> > 1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index c1c11418231b..e3ce4bef22a3 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4860,22 +4860,18 @@ int i915_gem_init(struct drm_device *dev)
> > }
> >
> > ret = i915_gem_init_userptr(dev);
> > - if (ret) {
> > - mutex_unlock(&dev->struct_mutex);
> > - return ret;
> > - }
> > + if (ret)
> > + goto out_unlock;
> >
> > i915_gem_init_global_gtt(dev);
> >
> > ret = i915_gem_context_init(dev);
> > - if (ret) {
> > - mutex_unlock(&dev->struct_mutex);
> > - return ret;
> > - }
> > + if (ret)
> > + goto out_unlock;
> >
> > ret = dev_priv->gt.init_rings(dev);
> > if (ret)
> > - return ret;
> > + goto out_unlock;
> >
> > ret = i915_gem_init_hw(dev);
> > if (ret == -EIO) {
> > @@ -4887,6 +4883,8 @@ int i915_gem_init(struct drm_device *dev)
> > atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter);
> > ret = 0;
> > }
> > +
> > +out_unlock:
> > mutex_unlock(&dev->struct_mutex);
> >
> > return ret;
> > --
> > 2.1.3
> >
>
> Yes, this looks much better.
Oh well confusion in my mailbox. I've dropped Wei's patch and merged this
one instead.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Jani,
On Fri, Dec 05, 2014 at 02:17:42PM +0200, Jani Nikula wrote:
> Release struct_mutex if init_rings() fails.
>
> This is a regression introduced in
> commit 35a57ffbb10840af219eeaf64718434242bb7c76
> Author: Daniel Vetter <[email protected]>
> Date: Thu Nov 20 00:33:07 2014 +0100
>
> drm/i915: Only init engines once
>
> Reported-by: Wei Yongjun <[email protected]>
> Signed-off-by: Jani Nikula <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c1c11418231b..e3ce4bef22a3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4860,22 +4860,18 @@ int i915_gem_init(struct drm_device *dev)
> }
>
> ret = i915_gem_init_userptr(dev);
> - if (ret) {
> - mutex_unlock(&dev->struct_mutex);
> - return ret;
> - }
> + if (ret)
> + goto out_unlock;
>
> i915_gem_init_global_gtt(dev);
>
> ret = i915_gem_context_init(dev);
> - if (ret) {
> - mutex_unlock(&dev->struct_mutex);
> - return ret;
> - }
> + if (ret)
> + goto out_unlock;
>
> ret = dev_priv->gt.init_rings(dev);
> if (ret)
> - return ret;
> + goto out_unlock;
>
> ret = i915_gem_init_hw(dev);
> if (ret == -EIO) {
> @@ -4887,6 +4883,8 @@ int i915_gem_init(struct drm_device *dev)
> atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter);
> ret = 0;
> }
> +
> +out_unlock:
> mutex_unlock(&dev->struct_mutex);
>
> return ret;
> --
> 2.1.3
>
Yes, this looks much better.
--
- Jeremiah Mahler
On Fri, Dec 05, 2014 at 08:55:59AM +0800, [email protected] wrote:
> From: Wei Yongjun <[email protected]>
>
> Add the missing unlock before return from function i915_gem_init_hw()
> in the error handling case.
>
> Signed-off-by: Wei Yongjun <[email protected]>
Applied, thanks for the patch. Two minor comments:
- Please mention the commit that introduced the issue next time around.
I've added that while applying.
- The usual patter is
if (ret)
goto out;
/* more code */
out:
mutex_unlock();
return ret;
This would work really well in i915_gem_init_hw and besides the
code-cleanup also prevents such a fumble in the future. If you feel like
please submit that patch to convert init_hw to this shared unlock code
pattern, too.
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/i915_gem.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d2ba315..3eeb2d0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4879,8 +4879,10 @@ i915_gem_init_hw(struct drm_device *dev)
> i915_gem_init_swizzling(dev);
>
> ret = dev_priv->gt.init_rings(dev);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&dev->struct_mutex);
> return ret;
> + }
>
> for (i = 0; i < NUM_L3_SLICES(dev); i++)
> i915_gem_l3_remap(&dev_priv->ring[RCS], i);
>
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Fri, 05 Dec 2014, Jeremiah Mahler <[email protected]> wrote:
> There are other places in i915_gem_init_hw() where it returns without
> unlocking the mutex. Why is it only necessary here and not any of the
> other places?
There's probably some rebase/merge confusion going on. The following
patch is against drm-intel-next-queued where the problem is
present. Thanks for reporting this.
BR,
Jani.
Jani Nikula (1):
drm/i915: release struct_mutex on the i915_gem_init_hw fail path
drivers/gpu/drm/i915/i915_gem.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
--
2.1.3
Release struct_mutex if init_rings() fails.
This is a regression introduced in
commit 35a57ffbb10840af219eeaf64718434242bb7c76
Author: Daniel Vetter <[email protected]>
Date: Thu Nov 20 00:33:07 2014 +0100
drm/i915: Only init engines once
Reported-by: Wei Yongjun <[email protected]>
Signed-off-by: Jani Nikula <[email protected]>
---
drivers/gpu/drm/i915/i915_gem.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c1c11418231b..e3ce4bef22a3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4860,22 +4860,18 @@ int i915_gem_init(struct drm_device *dev)
}
ret = i915_gem_init_userptr(dev);
- if (ret) {
- mutex_unlock(&dev->struct_mutex);
- return ret;
- }
+ if (ret)
+ goto out_unlock;
i915_gem_init_global_gtt(dev);
ret = i915_gem_context_init(dev);
- if (ret) {
- mutex_unlock(&dev->struct_mutex);
- return ret;
- }
+ if (ret)
+ goto out_unlock;
ret = dev_priv->gt.init_rings(dev);
if (ret)
- return ret;
+ goto out_unlock;
ret = i915_gem_init_hw(dev);
if (ret == -EIO) {
@@ -4887,6 +4883,8 @@ int i915_gem_init(struct drm_device *dev)
atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter);
ret = 0;
}
+
+out_unlock:
mutex_unlock(&dev->struct_mutex);
return ret;
--
2.1.3
On Fri, 05 Dec 2014, Jeremiah Mahler <[email protected]> wrote:
> There are other places in i915_gem_init_hw() where it returns without
> unlocking the mutex. Why is it only necessary here and not any of the
> other places?
There's probably some rebase/merge confusion going on. The following
patch is against drm-intel-next-queued where the problem is
present. Thanks for reporting this.
BR,
Jani.
Jani Nikula (1):
drm/i915: release struct_mutex on the i915_gem_init_hw fail path
drivers/gpu/drm/i915/i915_gem.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
--
2.1.3