From patchwork Thu Oct 30 14:08:05 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Imre Deak X-Patchwork-Id: 5197901 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 0078F9F30B for ; Thu, 30 Oct 2014 14:08:16 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id BEA6220165 for ; Thu, 30 Oct 2014 14:08:14 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 504A3201ED for ; Thu, 30 Oct 2014 14:08:13 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 1B7526E093; Thu, 30 Oct 2014 07:08:11 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 323BB6E093; Thu, 30 Oct 2014 07:08:10 -0700 (PDT) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 30 Oct 2014 07:08:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,286,1413270000"; d="scan'208";a="623493079" Received: from intelbox.fi.intel.com (HELO [10.237.72.105]) ([10.237.72.105]) by fmsmga002.fm.intel.com with ESMTP; 30 Oct 2014 07:08:06 -0700 Message-ID: <1414678085.8635.61.camel@intelbox> Subject: Re: [Intel-gfx] [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates From: Imre Deak To: Paulo Zanoni Date: Thu, 30 Oct 2014 16:08:05 +0200 In-Reply-To: References: <1414158695-31605-1-git-send-email-gustavo@padovan.org> <1414158695-31605-2-git-send-email-gustavo@padovan.org> Organization: Intel X-Mailer: Evolution 3.8.4-0ubuntu1 Mime-Version: 1.0 Cc: Intel Graphics Development , Gustavo Padovan , DRI Development X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: imre.deak@intel.com Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, 2014-10-30 at 11:32 -0200, Paulo Zanoni wrote: > 2014-10-24 11:51 GMT-02:00 Gustavo Padovan : > > From: Gustavo Padovan > > > > 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: if (old_obj != obj) { > > > > Signed-off-by: Gustavo Padovan > > Reviewed-by: Ville Syrjälä > > --- > > 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 > > > 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;