From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3F711C63697 for ; Fri, 13 Nov 2020 21:03:22 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C278F2224D for ; Fri, 13 Nov 2020 21:03:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="rVHNefGs"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="YEc1MMKT" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C278F2224D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=RztKpysQ+Es4CuUfKFO0OlOYTHLz3VA+VY5d+yrJQWk=; b=rVHNefGsRAEuQg4/G0kdA2LD8 RfioEmiF8lyKiJ69qilHLOwEpC509IA9I5d2ISNudT98k1e1T52O39iSrEnGGP50P8h7dSTw9lllE R2TRPY5gSjjarfyEEITqSzkVUIWnwFMMtrEzmRlT1udwOFZMu7tKVJc/xIvp/dBKXTcya7YpRqiiR IEb4R8Kk2IkeheY1WD4VphrLk0UzWnKJaq3co9iQg6FnHSxTCBtn3R+pVvrb7HBTdN5fmVtD+2jHp V8/o0aCdBdNJYZFgTN/VVvFOkv2pK/NNMMTFE0F78FnAaWC90MUWVGimuuChGfV/EYLjFuMdm+UQv AaJgpADuw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdgDZ-00088N-TK; Fri, 13 Nov 2020 21:02:49 +0000 Received: from mail-wm1-x342.google.com ([2a00:1450:4864:20::342]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdgDW-00086k-8J for linux-arm-kernel@lists.infradead.org; Fri, 13 Nov 2020 21:02:47 +0000 Received: by mail-wm1-x342.google.com with SMTP id 19so10914484wmf.1 for ; Fri, 13 Nov 2020 13:02:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=/9yPJV30pVcKkDL4/XdGIjl9qOHXH9yCBC3h+QTAx7c=; b=YEc1MMKTaPTIbTHLZH+Nl6FZMZP4Lkir8aiylqp9RQK6iKBY5jL+vOxEBj+y34os6d nOUtM3HxEhPE0+sFhHFUYLMVqu62BfN2JFJ4vIpZwQXxCDiN7qT7CjY8A0SOpyPz8DiJ 643tv9q2wOSUHZN/40brvQ8oMycTrCv7AtO0M= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=/9yPJV30pVcKkDL4/XdGIjl9qOHXH9yCBC3h+QTAx7c=; b=AI4VVPUecrbk2gEsvPnY5fvxnh9A0+tqCxBn0Wf6HYO7ffA3NDuRLSOnQdGwxDpZ2b EYHrr9KOWUns3jMOUBuWgJSRwi1CkmPT4hI2X+OpxfvYWM+tElnIkD3SMwBJzpysTuLI N+NTnh8WvwCXZF5EWyo8acFTEpIhYdj2h/tQklH/obZisckw0m7cBi7jPsXBe01MWIJ/ mHos8Eam/f9keeDWXb3gcNU5vzblSDzMdFjzpVh3gXwpiCKOKFELl35PAawkA8z7J0P9 lFBxyd66BiWpd/poJmoVVlKPpH6lv4INqd9cs+CXI4g/XgyqcBu6+39y7BvXohde4Y2h j+2g== X-Gm-Message-State: AOAM530HG8kDrXO23ihjFfoB+DP77TMa5zYWrZ/ENnEb11hPKnbI2mrE pTItVqt45iQcnRu83L41OXmPZw== X-Google-Smtp-Source: ABdhPJycSjeEZ4roZjLqZrcoeszqo3iWVlMz1UIZF1iy5IlZnr3YJRfEkA8Z653H7SISTQAeFFhW3A== X-Received: by 2002:a05:600c:2244:: with SMTP id a4mr4393082wmm.144.1605301363350; Fri, 13 Nov 2020 13:02:43 -0800 (PST) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id m21sm20856770wmi.3.2020.11.13.13.02.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 13 Nov 2020 13:02:42 -0800 (PST) Date: Fri, 13 Nov 2020 22:02:40 +0100 From: Daniel Vetter To: Maxime Ripard Subject: Re: [PATCH 1/8] drm: Introduce an atomic_commit_setup function Message-ID: <20201113210240.GX401619@phenom.ffwll.local> References: <20201113152956.139663-1-maxime@cerno.tech> <20201113152956.139663-2-maxime@cerno.tech> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201113152956.139663-2-maxime@cerno.tech> X-Operating-System: Linux phenom 5.7.0-1-amd64 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201113_160246_309643_7A31C675 X-CRM114-Status: GOOD ( 34.90 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , devicetree@vger.kernel.org, Tim Gover , Dave Stevenson , David Airlie , Daniel Vetter , Maarten Lankhorst , dri-devel@lists.freedesktop.org, Eric Anholt , Rob Herring , bcm-kernel-feedback-list@broadcom.com, linux-rpi-kernel@lists.infradead.org, Thomas Zimmermann , Daniel Vetter , Frank Rowand , Phil Elwell , linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Nov 13, 2020 at 04:29:49PM +0100, Maxime Ripard wrote: > Private objects storing a state shared across all CRTCs need to be > carefully handled to avoid a use-after-free issue. > > The proper way to do this to track all the commits using that shared > state and wait for the previous commits to be done before going on with > the current one to avoid the reordering of commits that could occur. > > However, this commit setup needs to be done after > drm_atomic_helper_setup_commit(), because before the CRTC commit > structure hasn't been allocated before, and before the workqueue is > scheduled, because we would be potentially reordered already otherwise. > > That means that drivers currently have to roll their own > drm_atomic_helper_commit() function, even though it would be identical > if not for the commit setup. > > Let's introduce a hook to do so that would be called as part of > drm_atomic_helper_commit, allowing us to reuse the atomic helpers. > > Suggested-by: Daniel Vetter > Signed-off-by: Maxime Ripard Should probably wait with merging until we have the entire vc4 user ready too. And I think the kerneldoc needs to be further improved, see suggestions below. > --- > drivers/gpu/drm/drm_atomic_helper.c | 6 ++++++ > include/drm/drm_modeset_helper_vtables.h | 18 ++++++++++++++++++ > 2 files changed, 24 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index ddd0e3239150..7d69c7844dfc 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2083,8 +2083,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, > struct drm_plane *plane; > struct drm_plane_state *old_plane_state, *new_plane_state; > struct drm_crtc_commit *commit; > + const struct drm_mode_config_helper_funcs *funcs; > int i, ret; > > + funcs = state->dev->mode_config.helper_private; > + > for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { > commit = kzalloc(sizeof(*commit), GFP_KERNEL); > if (!commit) > @@ -2169,6 +2172,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, > new_plane_state->commit = drm_crtc_commit_get(commit); > } > > + if (funcs && funcs->atomic_commit_setup) > + return funcs->atomic_commit_setup(state); > + > return 0; > } > EXPORT_SYMBOL(drm_atomic_helper_setup_commit); > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index f2de050085be..56470baf0513 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -1396,6 +1396,24 @@ struct drm_mode_config_helper_funcs { > * drm_atomic_helper_commit_tail(). > */ > void (*atomic_commit_tail)(struct drm_atomic_state *state); > + > + /** > + * @atomic_commit_setup: > + * > + * This hook is used by the default atomic_commit() hook implemented in > + * drm_atomic_helper_commit() together with the nonblocking helpers (see > + * drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It I think a link from drm_atomic_helper_setup_commit to this new hook here would be useful too, maybe together with a note that waiting for these additional synchronization points can be done at the start of atomic_commit_tail. > + * is not used by the atomic helpers. > + * > + * This function is called at the end of > + * drm_atomic_helper_setup_commit(), so once the commit has been > + * properly setup across the generic DRM object states. It allows > + * drivers to do some additional commit tracking that isn't related to a > + * CRTC, plane or connector, typically a private object. > + * > + * This hook is optional. > + */ > + int (*atomic_commit_setup)(struct drm_atomic_state *state); I think the kerneldoc for drm_private_obj should also explain when it is necessary to use this, and when not: - when the private state is a tied to an existing drm object (drm_crtc, drm_plane or drm_crtc) and never moves, then sufficient synchronization is already guaranteed by that superior object. This could even hold when the private object can be e.g. reassigned between planes, but always stays on the same crtc. - if the private state object can be globally reassigned, then drm_crtc_commit synchronization points need to be set up in ->atomic_commit_setup and waited on as the first step in ->atomic_commit_tail Also I just realized that a drm_private_state->obj backpointer to drm_private_obj might be rather useful. Or at least more consistent with all the other state objects. Cheers, Daniel > }; > > #endif > -- > 2.28.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel