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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 30, 2014 at 11:32:42AM -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.
> 
> 
> > 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)
<snip>
> > -       struct drm_i915_gem_object *old_obj = intel_plane->obj;
> > +       struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);

I think this change is the culprit here. The problem is that during a
modeset we call intel_plane_disable()->intel_disable_plane() (yeah, yeah
the names suck big time :) which will unpin the current buffer and clear
intel_plane->obj, but plane->fb will still be set, so the next time
.update_plane() is called it will think the buffer was already pinned,
and not attempt to pin it again.

So just using intel_plane->obj to figure out the old obj should fix it.
But my plan sort of was that even if userspace "enables" a plane with
the crtc off, we'd still pin the buffer. Then we can be more sure that
when the crtc does get enabled the plane can actually be enabled as well
(ie. the pinning can't fail at that point anymore). So if we follow that
design I think we need to split intel_disable_plane() into two parts,
one which just does the plane disable, and the second part just does
unpinning. And then we need to just call the non-unpin variant from
intel_plane_disable() so that we don't unpin the buffer during crtc
disable. And then we should also kill intel_plane->obj.


> >         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
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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