From patchwork Fri Nov 21 15:21:25 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 5355601 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 0962A9F2F1 for ; Fri, 21 Nov 2014 15:21:16 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 1DE4220160 for ; Fri, 21 Nov 2014 15:21:15 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id CE9282015D for ; Fri, 21 Nov 2014 15:21:13 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 731466F021; Fri, 21 Nov 2014 07:21:12 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wi0-f169.google.com (mail-wi0-f169.google.com [209.85.212.169]) by gabe.freedesktop.org (Postfix) with ESMTP id B610B6F01C for ; Fri, 21 Nov 2014 07:21:11 -0800 (PST) Received: by mail-wi0-f169.google.com with SMTP id r20so2970589wiv.4 for ; Fri, 21 Nov 2014 07:21:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=+YkFCoMdefbeNTVGDumUWBhJurjZV2csiXlUds0UGbE=; b=S8Msu/kRw0ss+QTTu006TRQvV/Q2NbAOYRwxAzFanzJhTzI9Z7h9EThNFlCK4nykE+ pOkpzN6QgdveT3aCEcwv7NR6ptNkScNTj5jbAPWJBt4rxW2zUuceyr8SrIZ5/VohWIOK 5w6rapCGyEc5kk/0TECf6sMDI6KAM6QkkPt6c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-type:content-disposition :in-reply-to:user-agent; bh=+YkFCoMdefbeNTVGDumUWBhJurjZV2csiXlUds0UGbE=; b=YVMA0Flz6PrLE9NkBBOWvxr/dKEHIcw/p4xAOtcCGT0vwCqdbOX/sYcYpADR3mFUGE AdjNZfdd4rXKw5/INsb26LPc3Zl5epqV2LPZa6g3B4rywMCsCupU5IVzQ3COZKjmagdF SoEK5nSeJ2xlxiJ9AnBGi39BBa2aFhTm/4w9VrikSNMsKmtYf9SseVXdLOyDToTerVO9 tVABbITu+fsbR+V3l/7IQUDpZvPqEjhBpPA+yhQtd56OtX7FP8AtDvxVqInJDxflqdsJ GJbjZjGr5JHpBVDfcVBkvtmXlFo3uRPxxHwQn2anLgOR55UW21Qk5tQgDPqwCTdyUcmR Vt+Q== X-Gm-Message-State: ALoCoQlZmP28IGWFXAx6JtJMEp7qyRaYCZBfSh1CJHa9JjbN7buPyGue6mBX2FMSsNUb0qs1poxA X-Received: by 10.194.81.6 with SMTP id v6mr8346557wjx.39.1416583263691; Fri, 21 Nov 2014 07:21:03 -0800 (PST) Received: from phenom.ffwll.local ([84.73.67.144]) by mx.google.com with ESMTPSA id wx3sm8463037wjc.19.2014.11.21.07.21.01 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 21 Nov 2014 07:21:02 -0800 (PST) Date: Fri, 21 Nov 2014 16:21:25 +0100 From: Daniel Vetter To: Rob Clark Subject: Re: [PATCH] drm_atomic_helper: Copy/paste fix for calling already disabled planes Message-ID: <20141121152125.GP25711@phenom.ffwll.local> References: <1416542355-5715-1-git-send-email-jstpierre@mecheye.net> <20141121083110.GO25711@phenom.ffwll.local> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Operating-System: Linux phenom 3.16-2-amd64 User-Agent: Mutt/1.5.23 (2014-03-12) Cc: Daniel Vetter , "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=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED, T_DKIM_INVALID, T_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 Fri, Nov 21, 2014 at 07:17:48AM -0500, Rob Clark wrote: > On Fri, Nov 21, 2014 at 3:31 AM, Daniel Vetter wrote: > > On Thu, Nov 20, 2014 at 07:59:15PM -0800, Jasper St. Pierre wrote: > >> This code was in drm_plane_helper, but missing from drm_atomic_helper, > >> causing various crashes when the plane was already disabled. Just copy > >> over the quick return there to prevent a crash. > >> > >> Signed-off-by: Jasper St. Pierre > >> Reviewed-by: Rob Clark > >> Cc: Daniel Vetter > >> --- > >> drivers/gpu/drm/drm_atomic_helper.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >> index fad2b93..d96dac3 100644 > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >> @@ -1246,6 +1246,11 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane) > >> struct drm_plane_state *plane_state; > >> int ret = 0; > >> > >> + /* crtc helpers love to call disable functions for already disabled hw > >> + * functions. So cope with that. */ > > > > Comment here is misleading since this isn't called by the crtc helpers but > > directly by the drm core code to handle the setplane ioctl. > > > > Now the real problem with this is that it's at the wrong spot. If we > > really need to filter this then it should be done in the atomic_commit > > function as a noop and not here. This is just the legacy ->disable_plane > > entry hook, userspace could still throw multiple disable plane calls at > > the driver. And they would get past this check. > > > > As-is it's actually a design feature of the atomic plane helpers that they > > don't filter out any no-op updates, but just pass the new state into the > > ->atomic_update hook (through the plane->state pointer). We could extend > > that (Thierry is iirc working ona an ->atomic_disable callback), and then > > it would make sense to have some filtering in the helpers. But if we add > > that it must be done in drm_atomic_helper_commit_planes not here. > > I guess I'm (a) not entirely sure what the point of a no-op disable > is, and (b) quite sure how you plane to make a > 'drm_modeset_legacy_acquire_ctx(plane->crtc)' work if plane->crtc is > NULL.. For a) atm atomic helpers just don't filter that out, and imo the core should (since it's really tricky to figure out whether userspace has done an elaborate noop ioctl when there's driver-specific properties and maybe some autodetection involved). For b) the commit message completely failed to mention the Oops. The fix for that is I somehow missed that implication of the "pick crtc acquire context or if that's not there the global one". -Daniel diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c index 474e4d12a2d8..93d28269e3bd 100644 --- a/drivers/gpu/drm/drm_modeset_lock.c +++ b/drivers/gpu/drm/drm_modeset_lock.c @@ -209,7 +209,7 @@ EXPORT_SYMBOL(drm_modeset_lock_crtc); struct drm_modeset_acquire_ctx * drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc) { - if (crtc->acquire_ctx) + if (crtc && crtc->acquire_ctx) return crtc->acquire_ctx; WARN_ON(!crtc->dev->mode_config.acquire_ctx);