Re: [PATCH 22/27] drm/i915/overlay: Drop struct_mutex guard

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 25/09/2019 13:53, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-25 13:43:46)
> > On 25/09/2019 11:01, Chris Wilson wrote:
> > > The overlay uses the modeset mutex to control itself and only required
> > > the struct_mutex for requests, which is now obsolete.
> > >
> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >    drivers/gpu/drm/i915/display/intel_display.c |  7 +------
> > >    drivers/gpu/drm/i915/display/intel_overlay.c | 13 -------------
> > >    drivers/gpu/drm/i915/gt/intel_reset.c        |  4 ----
> > >    3 files changed, 1 insertion(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 99ddbb7bedbc..038f14d9b5c9 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -5738,13 +5738,8 @@ void hsw_disable_ips(const struct intel_crtc_state *crtc_state)
> > >    
> > >    static void intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc)
> > >    {
> > > -     if (intel_crtc->overlay) {
> > > -             struct drm_device *dev = intel_crtc->base.dev;
> > > -
> > > -             mutex_lock(&dev->struct_mutex);
> > > +     if (intel_crtc->overlay)
> > >                (void) intel_overlay_switch_off(intel_crtc->overlay);
> > > -             mutex_unlock(&dev->struct_mutex);
> > > -     }
> > >    
> > >        /* Let userspace switch the overlay on again. In most cases userspace
> > >         * has to recompute where to put it anyway.
> > > diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
> > > index e12e1a753af0..daea112cbb87 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> > > @@ -439,8 +439,6 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
> > >        struct i915_request *rq;
> > >        u32 *cs;
> > >    
> > > -     lockdep_assert_held(&dev_priv->drm.struct_mutex);
> > > -
> > >        /*
> > >         * Only wait if there is actually an old frame to release to
> > >         * guarantee forward progress.
> > > @@ -751,7 +749,6 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
> > >        struct i915_vma *vma;
> > >        int ret, tmp_width;
> > >    
> > > -     lockdep_assert_held(&dev_priv->drm.struct_mutex);
> > >        WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
> > >    
> > >        ret = intel_overlay_release_old_vid(overlay);
> > > @@ -852,7 +849,6 @@ int intel_overlay_switch_off(struct intel_overlay *overlay)
> > >        struct drm_i915_private *dev_priv = overlay->i915;
> > >        int ret;
> > >    
> > > -     lockdep_assert_held(&dev_priv->drm.struct_mutex);
> > >        WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
> > >    
> > >        ret = intel_overlay_recover_from_interrupt(overlay);
> > > @@ -1068,11 +1064,7 @@ int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data,
> > >    
> > >        if (!(params->flags & I915_OVERLAY_ENABLE)) {
> > >                drm_modeset_lock_all(dev);
> > > -             mutex_lock(&dev->struct_mutex);
> > > -
> > >                ret = intel_overlay_switch_off(overlay);
> > > -
> > > -             mutex_unlock(&dev->struct_mutex);
> > >                drm_modeset_unlock_all(dev);
> > >    
> > >                return ret;
> > > @@ -1088,7 +1080,6 @@ int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data,
> > >                return -ENOENT;
> > >    
> > >        drm_modeset_lock_all(dev);
> > > -     mutex_lock(&dev->struct_mutex);
> > >    
> > >        if (i915_gem_object_is_tiled(new_bo)) {
> > >                DRM_DEBUG_KMS("buffer used for overlay image can not be tiled\n");
> > > @@ -1152,14 +1143,12 @@ int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data,
> > >        if (ret != 0)
> > >                goto out_unlock;
> > >    
> > > -     mutex_unlock(&dev->struct_mutex);
> > >        drm_modeset_unlock_all(dev);
> > >        i915_gem_object_put(new_bo);
> > >    
> > >        return 0;
> > >    
> > >    out_unlock:
> > > -     mutex_unlock(&dev->struct_mutex);
> > >        drm_modeset_unlock_all(dev);
> > >        i915_gem_object_put(new_bo);
> > >    
> > > @@ -1233,7 +1222,6 @@ int intel_overlay_attrs_ioctl(struct drm_device *dev, void *data,
> > >        }
> > >    
> > >        drm_modeset_lock_all(dev);
> > > -     mutex_lock(&dev->struct_mutex);
> > >    
> > >        ret = -EINVAL;
> > >        if (!(attrs->flags & I915_OVERLAY_UPDATE_ATTRS)) {
> > > @@ -1290,7 +1278,6 @@ int intel_overlay_attrs_ioctl(struct drm_device *dev, void *data,
> > >    
> > >        ret = 0;
> > >    out_unlock:
> > > -     mutex_unlock(&dev->struct_mutex);
> > >        drm_modeset_unlock_all(dev);
> > >    
> > >        return ret;
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> > > index 2685c0103e90..ee0c5c24282c 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> > > @@ -1265,10 +1265,6 @@ int intel_gt_terminally_wedged(struct intel_gt *gt)
> > >        if (!test_bit(I915_RESET_BACKOFF, &gt->reset.flags))
> > >                return -EIO;
> > >    
> > > -     /* XXX intel_reset_finish() still takes struct_mutex!!! */
> > > -     if (mutex_is_locked(&gt->i915->drm.struct_mutex))
> > > -             return -EAGAIN;
> > > -
> >
> > What is this hunk doing here?
>
> Because the overlay used struct_mutex, now it does not.

Now you see why I insist on comments and even better if they are up to 
date! :) (intel_reset_finish?!)

If you are confident overlay is the only place which can interfere here 
then:

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

If not we'll find out sooner or later.

Regards,

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