Re: [Intel-gfx] [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates

[Imre Deak <imre.deak@intel.com>]

On Thu, 2014-10-30 at 11:32 -0200, Paulo Zanoni wrote:
> 2014-10-24 11:51 GMT-02:00 Gustavo Padovan <gustavo@padovan.org>:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > take out pin_fb code so the commit phase can't fail anymore.
> >
> 
> According to my bisection results, this is the first bad commit of
> https://bugs.freedesktop.org/show_bug.cgi?id=85634.

I saw the same issue, when checking #82939. AFAICS when disabling the
crtc intel_plane->obj will be set to NULL and the object will be
unpinned, but plane->fb remains set. Next if drm_set_plane with the same
fb is called intel_prepare_sprite_plane() won't pin the object since it
sees that same fb is still set, but sets intel_plane->obj. Next if
drm_set_plane called with a NULL fb, it will try unpin the object, and
the unbalanced refcount throws a WARN. The following got rid of the
problem for me:

diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index 8b80d68..36762b5 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1198,9 +1198,10 @@ intel_prepare_sprite_plane(struct drm_plane
*plane,
 	struct drm_crtc *crtc = state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
+	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int ret;
 
 	if (old_obj != obj) {

> 
> 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++--------------
> >  1 file changed, 40 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 2c060ad..3631b0e 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1192,34 +1192,18 @@ intel_check_sprite_plane(struct drm_plane *plane,
> >  }
> >
> >  static int
> > -intel_commit_sprite_plane(struct drm_plane *plane,
> > -                         struct intel_plane_state *state)
> > +intel_prepare_sprite_plane(struct drm_plane *plane,
> > +                          struct intel_plane_state *state)
> >  {
> >         struct drm_device *dev = plane->dev;
> >         struct drm_crtc *crtc = state->crtc;
> >         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -       struct intel_plane *intel_plane = to_intel_plane(plane);
> >         enum pipe pipe = intel_crtc->pipe;
> >         struct drm_framebuffer *fb = state->fb;
> > -       struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > -       struct drm_i915_gem_object *obj = intel_fb->obj;
> > -       struct drm_i915_gem_object *old_obj = intel_plane->obj;
> > -       int crtc_x, crtc_y;
> > -       unsigned int crtc_w, crtc_h;
> > -       uint32_t src_x, src_y, src_w, src_h;
> > -       struct drm_rect *dst = &state->dst;
> > -       const struct drm_rect *clip = &state->clip;
> > -       bool primary_enabled;
> > +       struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > +       struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> >         int ret;
> >
> > -       /*
> > -        * If the sprite is completely covering the primary plane,
> > -        * we can disable the primary and save power.
> > -        */
> > -       primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
> > -       WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
> > -
> > -
> >         if (old_obj != obj) {
> >                 mutex_lock(&dev->struct_mutex);
> >
> > @@ -1238,6 +1222,36 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> >                         return ret;
> >         }
> >
> > +       return 0;
> > +}
> > +
> > +static void
> > +intel_commit_sprite_plane(struct drm_plane *plane,
> > +                         struct intel_plane_state *state)
> > +{
> > +       struct drm_device *dev = plane->dev;
> > +       struct drm_crtc *crtc = state->crtc;
> > +       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +       struct intel_plane *intel_plane = to_intel_plane(plane);
> > +       enum pipe pipe = intel_crtc->pipe;
> > +       struct drm_framebuffer *fb = state->fb;
> > +       struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > +       struct drm_i915_gem_object *obj = intel_fb->obj;
> > +       struct drm_i915_gem_object *old_obj = intel_plane->obj;
> > +       int crtc_x, crtc_y;
> > +       unsigned int crtc_w, crtc_h;
> > +       uint32_t src_x, src_y, src_w, src_h;
> > +       struct drm_rect *dst = &state->dst;
> > +       const struct drm_rect *clip = &state->clip;
> > +       bool primary_enabled;
> > +
> > +       /*
> > +        * If the sprite is completely covering the primary plane,
> > +        * we can disable the primary and save power.
> > +        */
> > +       primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
> > +       WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
> > +
> >         intel_plane->crtc_x = state->orig_dst.x1;
> >         intel_plane->crtc_y = state->orig_dst.y1;
> >         intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> > @@ -1298,8 +1312,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> >                 intel_unpin_fb_obj(old_obj);
> >                 mutex_unlock(&dev->struct_mutex);
> >         }
> > -
> > -       return 0;
> >  }
> >
> >  static int
> > @@ -1339,7 +1351,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >         if (ret)
> >                 return ret;
> >
> > -       return intel_commit_sprite_plane(plane, &state);
> > +       ret = intel_prepare_sprite_plane(plane, &state);
> > +       if (ret)
> > +               return ret;
> > +
> > +       intel_commit_sprite_plane(plane, &state);
> > +       return 0;
> >  }
> >
> >  static int
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel