From patchwork Thu Aug 27 12:43:35 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Maarten Lankhorst X-Patchwork-Id: 7084051 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.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 82D379F1C2 for ; Thu, 27 Aug 2015 12:43:42 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 99A7C20987 for ; Thu, 27 Aug 2015 12:43:41 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 9D2C920984 for ; Thu, 27 Aug 2015 12:43:40 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 5FBD46E044; Thu, 27 Aug 2015 05:43:38 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 902006E041; Thu, 27 Aug 2015 05:43:37 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP; 27 Aug 2015 05:43:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,422,1437462000"; d="scan'208";a="791978060" Received: from bmccart1-mobl1.ger.corp.intel.com (HELO patser.lan) ([10.252.23.166]) by orsmga002.jf.intel.com with ESMTP; 27 Aug 2015 05:43:35 -0700 Subject: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2. To: Daniel Stone References: <55C0A330.6050404@linux.intel.com> From: Maarten Lankhorst Message-ID: <55DF05F7.1010308@linux.intel.com> Date: Thu, 27 Aug 2015 14:43:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: Cc: Intel Graphics Development , "dri-devel@lists.freedesktop.org" 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: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-5.6 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 Op 27-08-15 om 14:19 schreef Daniel Stone: > Hi, > > On 4 August 2015 at 12:34, Maarten Lankhorst > wrote: >> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e >> "drm/atomic: Cleanup on error properly in the atomic ioctl." >> cleaned up some error paths, but didn't fix the TEST_ONLY path. >> In the check only case plane->fb shouldn't be updated, and >> the vblank events should be cleared as on failure. > Bikeshedding a bit ... > > An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need > to mention this in the commit message; in this case, the main change > is about plane->{,old_}fb. Even testing with PAGE_FLIP_EVENT would be useful because event && !crtc_state->active should not be allowed. In that case test could succeed but commit could fail. Though I guess you're right and it's currently not allowed. >> @@ -1532,7 +1533,7 @@ retry: >> ret = drm_atomic_check_only(state); >> /* _check_only() does not free state, unlike _commit() */ >> if (!ret) >> - drm_atomic_state_free(state); >> + goto free; >> } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) { >> ret = drm_atomic_async_commit(state); >> } else { >> @@ -1566,6 +1567,7 @@ out: >> } >> >> if (ret) { >> +free: > This is a bit nasty. Can we please move the label above the > conditional and change the condition to (ret || flags & TEST_ONLY)? > Doing that, you could also move the label above the (ret == -EDEADLK) > check, which would cover ->atomic_check needing to grab other states > (global resources?) and failing. If our bookkeeping is correct then it won't be harmful to fixup old_fb. Cleaning up more old_fb for more planes than initially had fb updates can always happen anyway, because a modeset will add all affected planes. How about the below patch? Apply with git am --scissors -------->8------ Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e "drm/atomic: Cleanup on error properly in the atomic ioctl." cleaned up some error paths, but allowed -EDEADLK to leak vblank events. Additionally check_only was updating plane->fb, which should not be done when checking a new configuration only. Signed-off-by: Maarten Lankhorst Reviewed-by: Daniel Stone diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index c2448f42480f..78ffb4965548 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1542,7 +1542,8 @@ retry: copied_props++; } - if (obj->type == DRM_MODE_OBJECT_PLANE && count_props) { + if (obj->type == DRM_MODE_OBJECT_PLANE && count_props && + !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) { plane = obj_to_plane(obj); plane_mask |= (1 << drm_plane_index(plane)); plane->old_fb = plane->fb; @@ -1564,10 +1565,11 @@ retry: } if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { + /* + * Unlike commit, check_only does not clean up state. + * Below we call drm_atomic_state_free for it. + */ ret = drm_atomic_check_only(state); - /* _check_only() does not free state, unlike _commit() */ - if (!ret) - drm_atomic_state_free(state); } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) { ret = drm_atomic_async_commit(state); } else { @@ -1594,25 +1596,24 @@ out: plane->old_fb = NULL; } + if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { + for_each_crtc_in_state(state, crtc, crtc_state, i) { + if (!crtc_state->event) + continue; + + destroy_vblank_event(dev, file_priv, + crtc_state->event); + } + } + if (ret == -EDEADLK) { drm_atomic_state_clear(state); drm_modeset_backoff(&ctx); goto retry; } - if (ret) { - if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { - for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (!crtc_state->event) - continue; - - destroy_vblank_event(dev, file_priv, - crtc_state->event); - } - } - + if (ret || arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) drm_atomic_state_free(state); - } drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx);