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=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,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 7D583C433E0 for ; Wed, 20 Jan 2021 23:42:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 472662360D for ; Wed, 20 Jan 2021 23:42:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729924AbhATVY5 (ORCPT ); Wed, 20 Jan 2021 16:24:57 -0500 Received: from new3-smtp.messagingengine.com ([66.111.4.229]:40027 "EHLO new3-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2389526AbhATNv2 (ORCPT ); Wed, 20 Jan 2021 08:51:28 -0500 Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailnew.nyi.internal (Postfix) with ESMTP id 4AE60580614; Wed, 20 Jan 2021 08:50:30 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Wed, 20 Jan 2021 08:50:30 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm1; bh=sDxhL51Vhfp7bMJtc05jY3INv15 IU8xKUsLjHr0xiOU=; b=e6RAvVNPXxx7n+xiZF1cUAYNfJs5LTR6PfLc0CImUdL LlXQ2amT0WEqGAlHTdvKd/rcGDF9tSdtsirQSEbuQ3kwVZPiCkw1rM45GLgnvgPm Z4AsvGXKTlP7JsOul0AAUpaxlPAIY0gsrLwL9MiYoYuMQ6L6TO3yzgXJyxnZRvp+ jtbY428Tzk+iEi5DuF8gexjpBHwznO/my3ickth/O3fCvFWfKEcF6MqEAd4+26aI ltp/+9yq6CB5vfxYtjqmtlBszlAGV3LvzyYzkBIp/5m62bEJ7/gvtET2/W1vfPsg vY4oRDFmYVdvqYxQCwBv7aNKZ4xfO67HgJeBjDQpS+Q== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=sDxhL5 1Vhfp7bMJtc05jY3INv15IU8xKUsLjHr0xiOU=; b=EfesV/hyieiND3YvIa/6XB Ybt6zQhLteUCpGeaKh3HJ3r8SPD5icS8VMHvz6edMF/d+NH2mK3+OxnvxfLBsWqV wXAMG7OAE3iMb8Jj0zKpAuRT6rB2AmmPxwaSas/r5vQ17Ork2L0vvST7Yeo2hptr F9gINk9hxuhpdcZ2JGY+OBhDuNww1XOgMwXBZ+XlaGSmxSidkgRe96ytQ5cFQ36z F4/Gafqosel9wOY2VtXYBKWeq1o3cc19d++D7K5l3EhNb0X8J4DKZ3qqgOONCwN6 9jCka/dzFyN+ouChiOWz+bG2KoQKejSligox5c8EC46vLc98c5GQGNldiZTqNdiw == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledruddvgdehkecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomhepofgrgihimhgv ucftihhprghrugcuoehmrgigihhmvgestggvrhhnohdrthgvtghhqeenucggtffrrghtth gvrhhnpeejuddvhfekkefhtdegiefhledutdevtdfhkedtleefjefgleduhfetudevjeeh hfenucffohhmrghinhepfhhrvggvuggvshhkthhophdrohhrghenucfkphepledtrdekle drieekrdejieenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhr ohhmpehmrgigihhmvgestggvrhhnohdrthgvtghh X-ME-Proxy: Received: from localhost (lfbn-tou-1-1502-76.w90-89.abo.wanadoo.fr [90.89.68.76]) by mail.messagingengine.com (Postfix) with ESMTPA id 4410A24005A; Wed, 20 Jan 2021 08:50:27 -0500 (EST) Date: Wed, 20 Jan 2021 14:50:26 +0100 From: Maxime Ripard To: Laurent Pinchart Cc: Maarten Lankhorst , Thomas Zimmermann , Daniel Vetter , David Airlie , dri-devel@lists.freedesktop.org, Alexey Brodkin , Daniel Vetter , Liviu Dudau , Brian Starkey , Russell King , Dave Airlie , Sam Ravnborg , Boris Brezillon , Nicolas Ferre , Alexandre Belloni , Ludovic Desroches , Inki Dae , Joonyoung Shim , Seung-Woo Kim , Kyungmin Park , Krzysztof Kozlowski , Stefan Agner , Alison Wang , Xinliang Liu , Tian Tao , John Stultz , Xinwei Kong , Chen Feng , Laurentiu Palcu , Lucas Stach , Philipp Zabel , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Paul Cercueil , Anitha Chrisanthus , Edmund Dea , Chun-Kuang Hu , Matthias Brugger , Neil Armstrong , Kevin Hilman , Jerome Brunet , Martin Blumenstingl , Rob Clark , Sean Paul , Marek Vasut , Tomi Valkeinen , Gerd Hoffmann , Kieran Bingham , Sandy Huang , Heiko =?utf-8?Q?St=C3=BCbner?= , Benjamin Gaignard , Vincent Abriou , Yannick Fertre , Philippe Cornu , Maxime Coquelin , Alexandre Torgue , Chen-Yu Tsai , Jernej Skrabec , Thierry Reding , Jonathan Hunter , Jyri Sarha , Hans de Goede , Rodrigo Siqueira , Melissa Wen , Haneen Mohammed , VMware Graphics , Roland Scheidegger , Hyun Kwon , Michal Simek , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-amlogic@lists.infradead.org, linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org, virtualization@lists.linux-foundation.org, spice-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, linux-tegra@vger.kernel.org Subject: Re: [PATCH 10/10] drm: Use state helper instead of the plane state pointer Message-ID: <20210120135026.np2ivojt5vnvyota@gilmour> References: <20210115125703.1315064-1-maxime@cerno.tech> <20210115125703.1315064-10-maxime@cerno.tech> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="j7svqftvzagigb3f" Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-tegra@vger.kernel.org --j7svqftvzagigb3f Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Laurent, On Fri, Jan 15, 2021 at 11:20:21PM +0200, Laurent Pinchart wrote: > Hi Maxime, >=20 > Thank you for the patch. >=20 > On Fri, Jan 15, 2021 at 01:57:02PM +0100, Maxime Ripard wrote: > > Many drivers reference the plane->state pointer in order to get the > > current plane state in their atomic_update or atomic_disable hooks, >=20 > Please don't use the word "current", it's ambiguous. Do you mean old > state or new state ? It's kind of the point I was trying to make: plane->state is the current state of the plane, but it's definitely ambiguous and it's fairly easy to be confused when working over several hooks. > > which would be the new plane state in the global atomic state since > > _swap_state happened when those hooks are run. >=20 > Is this relevant ? drm_atomic_helper_swap_state() doesn't change the > old_state and new_state pointers in drm_atomic_state as far as I can > tell. No, but it does change the plane->state pointer: before swap_state it's the old state, after swap_state it's the new state > > Use the drm_atomic_get_new_plane_state helper to get that state to make= it > > more obvious. > >=20 > > This was made using the coccinelle script below: > >=20 > > @ plane_atomic_func @ > > identifier helpers; > > identifier func; > > @@ > >=20 > > ( > > static const struct drm_plane_helper_funcs helpers =3D { > > ..., > > .atomic_disable =3D func, > > ..., > > }; > > | > > static const struct drm_plane_helper_funcs helpers =3D { > > ..., > > .atomic_update =3D func, > > ..., > > }; > > ) > >=20 > > @ adds_new_state @ > > identifier plane_atomic_func.func; > > identifier plane, state; > > identifier new_state; > > @@ > >=20 > > func(struct drm_plane *plane, struct drm_atomic_state *state) > > { > > ... > > - struct drm_plane_state *new_state =3D plane->state; > > + struct drm_plane_state *new_state =3D drm_atomic_get_new_plane_state(= state, plane); > > ... > > } > >=20 > > @ include depends on adds_new_state @ > > @@ > >=20 > > #include > >=20 > > @ no_include depends on !include && adds_new_state @ > > @@ > >=20 > > + #include > > #include > >=20 > > Signed-off-by: Maxime Ripard > > --- >=20 > [snip] >=20 > > drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++-- > > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 ++- > > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 ++- > > drivers/gpu/drm/xlnx/zynqmp_disp.c | 3 ++- >=20 > [snip] >=20 > > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/oma= pdrm/omap_plane.c > > index cd8cf7c786b5..021a94de84a1 100644 > > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > > @@ -44,7 +44,8 @@ static void omap_plane_atomic_update(struct drm_plane= *plane, > > { > > struct omap_drm_private *priv =3D plane->dev->dev_private; > > struct omap_plane *omap_plane =3D to_omap_plane(plane); > > - struct drm_plane_state *new_state =3D plane->state; >=20 > This seems to imply that you're interested in the new state. Well, to be fair, the variable is only called "state" before this series and it's one of the previous patch that renames it to new_state and makes it a more obvious. Otherwise, state =3D plane->state is fairly confusing and error-prone. With that change you would make it really obvious > > + struct drm_plane_state *new_state =3D drm_atomic_get_new_plane_state(= state, > > + plane); >=20 > Does this really make things more obvious ? I guess you're better at remembering this than I am then :) The discussion on whether it's more obvious or not aside, accessing the ->state pointer directly has some culprits, see: https://dri.freedesktop.org/docs/drm/gpu/todo.html#plumb-drm-atomic-state-a= ll-over Maxime --j7svqftvzagigb3f Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYAg1IgAKCRDj7w1vZxhR xW+rAQCaKXuyQvbRubp33hPCkdAY/LXaD+3TQwLv2j7AhTFUtAD/aLv/q1HqwcEO ZfMJ4deD5+JElSu70qt5AOEI/9JMPAg= =q+gU -----END PGP SIGNATURE----- --j7svqftvzagigb3f-- 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=-13.9 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 1C124C433E0 for ; Wed, 20 Jan 2021 14:57:24 +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 74ED923381 for ; Wed, 20 Jan 2021 14:57:23 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 74ED923381 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cerno.tech Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mediatek-bounces+linux-mediatek=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-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-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=neJDgvFqSpg8XooVf+3d5eyjyDdSTryIuLRz4VyFSPg=; b=B7fvw0xv3VHwdHEqjm6ywyrMz vQ147MrSo4RHqP8qgiEuEI9IAimIJLZwPd2vWmPz2KfNBaZFLNDQBti3STZ0G7lG/vPJgHpNh04Gt oNrXZ/spZCAJ/B/1E59QKvR2BtiyJeNx1BKpD4jmCbEYDM/MKcdPavIlK+akOtVkhpNjQ1ea9c/2m cXbbciMgKD43wwNCeYfjP7ZGGvRfvgDxhU+jc/jYOGXAAaDv5hZxEoTldPevsZgOHV02/IoNDv/Rg m1VCBPhxgdgtZ918+ukC9TS2RJ5EU7JnaMkAMWjVzCnl2J/+txR9KikSzIJrFTDvLa5A3MwQPWMa/ fTTYM16+w==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l2Ev6-0001jU-V9; Wed, 20 Jan 2021 14:57:16 +0000 Received: from casper.infradead.org ([2001:8b0:10b:1236::1]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1l2DtD-0004ot-BI; Wed, 20 Jan 2021 13:51:15 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=sDxhL51Vhfp7bMJtc05jY3INv15IU8xKUsLjHr0xiOU=; b=vJdEETATxp3YRtk6aondiv8sV7 l7PlfwUWlF5cknTj757w9bcr9dU0BkBYe+ItJv41Mq0tzjo+/2wsi4/uH1u1dUfQ9l7SX2h60vdTe NHtYpoh0YiZvWDNwDgPgBjR5crl6laBst/cmQ98A03rmID2Z6V1KGGgMoqai+fQcxZZ+pla+ha0uC IqOpB3rtJcXmlvaYaGDAje0s6EDGhR3q0Tx9IOaq3X9ePdJ1layAUDrtDu4pD7lxzofuANOCYDs7N Nm+9zHIP6Vagwi2lUp8dBIs6fu5SRu3gW3FGM0O0DT7BXdG6ThPMid2k+Kc9z+2ppCIH+/cvF4B10 mzwNG2Pg==; Received: from new3-smtp.messagingengine.com ([66.111.4.229]) by casper.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1l2Dsn-00FkNI-N1; Wed, 20 Jan 2021 13:51:02 +0000 Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailnew.nyi.internal (Postfix) with ESMTP id 544A5580616; Wed, 20 Jan 2021 08:50:31 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Wed, 20 Jan 2021 08:50:31 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm1; bh=sDxhL51Vhfp7bMJtc05jY3INv15 IU8xKUsLjHr0xiOU=; b=e6RAvVNPXxx7n+xiZF1cUAYNfJs5LTR6PfLc0CImUdL LlXQ2amT0WEqGAlHTdvKd/rcGDF9tSdtsirQSEbuQ3kwVZPiCkw1rM45GLgnvgPm Z4AsvGXKTlP7JsOul0AAUpaxlPAIY0gsrLwL9MiYoYuMQ6L6TO3yzgXJyxnZRvp+ jtbY428Tzk+iEi5DuF8gexjpBHwznO/my3ickth/O3fCvFWfKEcF6MqEAd4+26aI ltp/+9yq6CB5vfxYtjqmtlBszlAGV3LvzyYzkBIp/5m62bEJ7/gvtET2/W1vfPsg vY4oRDFmYVdvqYxQCwBv7aNKZ4xfO67HgJeBjDQpS+Q== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=sDxhL5 1Vhfp7bMJtc05jY3INv15IU8xKUsLjHr0xiOU=; b=LikqJHGPtkrNylI5xzansd wjJ/KJ/R7Dq22hms9Md8biwwcNcdgUs/TtLHDiHdLWd/KIyZNBRPQyEEuZqOAVQH Zq59rIAwtrVdT/8xqrz21GDUEkDYGNlS2+2VlDDnQMqXZW2egzoG0Z70QoZsOgpH Cv+G0h3ZqaTJuyO4BYEoteVvzAOlKtK1B98G73tE5c5NwbRRkCgLWGFAWw/fAa69 zKxmdFUnM/ToyfHDRec9Q6ytEXmtIFW+kY42SZebw6yv+4kH64ts8JA31fa6TubI bDGw4zZCK5hUm4lShfikDy9nGwJ5aG1okt63wGVtgRstD+gcvL8O6zoqeehmfcBA == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledruddvgdehkecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomhepofgrgihimhgv ucftihhprghrugcuoehmrgigihhmvgestggvrhhnohdrthgvtghhqeenucggtffrrghtth gvrhhnpeejuddvhfekkefhtdegiefhledutdevtdfhkedtleefjefgleduhfetudevjeeh hfenucffohhmrghinhepfhhrvggvuggvshhkthhophdrohhrghenucfkphepledtrdekle drieekrdejieenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhr ohhmpehmrgigihhmvgestggvrhhnohdrthgvtghh X-ME-Proxy: Received: from localhost (lfbn-tou-1-1502-76.w90-89.abo.wanadoo.fr [90.89.68.76]) by mail.messagingengine.com (Postfix) with ESMTPA id 4410A24005A; Wed, 20 Jan 2021 08:50:27 -0500 (EST) Date: Wed, 20 Jan 2021 14:50:26 +0100 From: Maxime Ripard To: Laurent Pinchart Subject: Re: [PATCH 10/10] drm: Use state helper instead of the plane state pointer Message-ID: <20210120135026.np2ivojt5vnvyota@gilmour> References: <20210115125703.1315064-1-maxime@cerno.tech> <20210115125703.1315064-10-maxime@cerno.tech> MIME-Version: 1.0 In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210120_135102_279540_1DB56C47 X-CRM114-Status: GOOD ( 28.41 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Haneen Mohammed , Alexandre Belloni , Heiko =?utf-8?Q?St=C3=BCbner?= , Neil Armstrong , David Airlie , Liviu Dudau , Stefan Agner , Sandy Huang , Paul Cercueil , linux-tegra@vger.kernel.org, Chen-Yu Tsai , Thierry Reding , Rob Clark , Gerd Hoffmann , Benjamin Gaignard , Anitha Chrisanthus , Daniel Vetter , Sam Ravnborg , Michal Simek , Jerome Brunet , Marek Vasut , Yannick Fertre , linux-samsung-soc@vger.kernel.org, Joonyoung Shim , linux-rockchip@lists.infradead.org, Fabio Estevam , Alexey Brodkin , Russell King , Krzysztof Kozlowski , Jonathan Hunter , Roland Scheidegger , Xinliang Liu , Ludovic Desroches , VMware Graphics , NXP Linux Team , linux-arm-msm@vger.kernel.org, Philippe Cornu , Dave Airlie , Xinwei Kong , virtualization@lists.linux-foundation.org, Hyun Kwon , Philipp Zabel , Chun-Kuang Hu , Thomas Zimmermann , Alexandre Torgue , Martin Blumenstingl , Chen Feng , Sascha Hauer , Alison Wang , Maarten Lankhorst , linux-renesas-soc@vger.kernel.org, Inki Dae , Hans de Goede , linux-mediatek@lists.infradead.org, John Stultz , dri-devel@lists.freedesktop.org, Laurentiu Palcu , Matthias Brugger , linux-amlogic@lists.infradead.org, Edmund Dea , Sean Paul , Pengutronix Kernel Team , linux-arm-kernel@lists.infradead.org, Melissa Wen , Maxime Coquelin , Jernej Skrabec , freedreno@lists.freedesktop.org, Rodrigo Siqueira , Tomi Valkeinen , Boris Brezillon , Jyri Sarha , linux-stm32@st-md-mailman.stormreply.com, Seung-Woo Kim , Nicolas Ferre , linux-kernel@vger.kernel.org, Vincent Abriou , Kyungmin Park , Kieran Bingham , spice-devel@lists.freedesktop.org, Daniel Vetter , Kevin Hilman , Tian Tao , Shawn Guo , Brian Starkey , Lucas Stach Content-Type: multipart/mixed; boundary="===============3852755250493856615==" Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org --===============3852755250493856615== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="j7svqftvzagigb3f" Content-Disposition: inline --j7svqftvzagigb3f Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Laurent, On Fri, Jan 15, 2021 at 11:20:21PM +0200, Laurent Pinchart wrote: > Hi Maxime, >=20 > Thank you for the patch. >=20 > On Fri, Jan 15, 2021 at 01:57:02PM +0100, Maxime Ripard wrote: > > Many drivers reference the plane->state pointer in order to get the > > current plane state in their atomic_update or atomic_disable hooks, >=20 > Please don't use the word "current", it's ambiguous. Do you mean old > state or new state ? It's kind of the point I was trying to make: plane->state is the current state of the plane, but it's definitely ambiguous and it's fairly easy to be confused when working over several hooks. > > which would be the new plane state in the global atomic state since > > _swap_state happened when those hooks are run. >=20 > Is this relevant ? drm_atomic_helper_swap_state() doesn't change the > old_state and new_state pointers in drm_atomic_state as far as I can > tell. No, but it does change the plane->state pointer: before swap_state it's the old state, after swap_state it's the new state > > Use the drm_atomic_get_new_plane_state helper to get that state to make= it > > more obvious. > >=20 > > This was made using the coccinelle script below: > >=20 > > @ plane_atomic_func @ > > identifier helpers; > > identifier func; > > @@ > >=20 > > ( > > static const struct drm_plane_helper_funcs helpers =3D { > > ..., > > .atomic_disable =3D func, > > ..., > > }; > > | > > static const struct drm_plane_helper_funcs helpers =3D { > > ..., > > .atomic_update =3D func, > > ..., > > }; > > ) > >=20 > > @ adds_new_state @ > > identifier plane_atomic_func.func; > > identifier plane, state; > > identifier new_state; > > @@ > >=20 > > func(struct drm_plane *plane, struct drm_atomic_state *state) > > { > > ... > > - struct drm_plane_state *new_state =3D plane->state; > > + struct drm_plane_state *new_state =3D drm_atomic_get_new_plane_state(= state, plane); > > ... > > } > >=20 > > @ include depends on adds_new_state @ > > @@ > >=20 > > #include > >=20 > > @ no_include depends on !include && adds_new_state @ > > @@ > >=20 > > + #include > > #include > >=20 > > Signed-off-by: Maxime Ripard > > --- >=20 > [snip] >=20 > > drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++-- > > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 ++- > > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 ++- > > drivers/gpu/drm/xlnx/zynqmp_disp.c | 3 ++- >=20 > [snip] >=20 > > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/oma= pdrm/omap_plane.c > > index cd8cf7c786b5..021a94de84a1 100644 > > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > > @@ -44,7 +44,8 @@ static void omap_plane_atomic_update(struct drm_plane= *plane, > > { > > struct omap_drm_private *priv =3D plane->dev->dev_private; > > struct omap_plane *omap_plane =3D to_omap_plane(plane); > > - struct drm_plane_state *new_state =3D plane->state; >=20 > This seems to imply that you're interested in the new state. Well, to be fair, the variable is only called "state" before this series and it's one of the previous patch that renames it to new_state and makes it a more obvious. Otherwise, state =3D plane->state is fairly confusing and error-prone. With that change you would make it really obvious > > + struct drm_plane_state *new_state =3D drm_atomic_get_new_plane_state(= state, > > + plane); >=20 > Does this really make things more obvious ? I guess you're better at remembering this than I am then :) The discussion on whether it's more obvious or not aside, accessing the ->state pointer directly has some culprits, see: https://dri.freedesktop.org/docs/drm/gpu/todo.html#plumb-drm-atomic-state-a= ll-over Maxime --j7svqftvzagigb3f Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYAg1IgAKCRDj7w1vZxhR xW+rAQCaKXuyQvbRubp33hPCkdAY/LXaD+3TQwLv2j7AhTFUtAD/aLv/q1HqwcEO ZfMJ4deD5+JElSu70qt5AOEI/9JMPAg= =q+gU -----END PGP SIGNATURE----- --j7svqftvzagigb3f-- --===============3852755250493856615== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek --===============3852755250493856615==-- 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=-13.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 3BEADC433E6 for ; Thu, 21 Jan 2021 08:52:59 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (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 A14E9239E7 for ; Thu, 21 Jan 2021 08:52:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A14E9239E7 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cerno.tech Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 8969E6E51D; Thu, 21 Jan 2021 08:52:32 +0000 (UTC) Received: from new3-smtp.messagingengine.com (new3-smtp.messagingengine.com [66.111.4.229]) by gabe.freedesktop.org (Postfix) with ESMTPS id 157C06E2DE; Wed, 20 Jan 2021 13:50:33 +0000 (UTC) Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailnew.nyi.internal (Postfix) with ESMTP id 544A5580616; Wed, 20 Jan 2021 08:50:31 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Wed, 20 Jan 2021 08:50:31 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm1; bh=sDxhL51Vhfp7bMJtc05jY3INv15 IU8xKUsLjHr0xiOU=; b=e6RAvVNPXxx7n+xiZF1cUAYNfJs5LTR6PfLc0CImUdL LlXQ2amT0WEqGAlHTdvKd/rcGDF9tSdtsirQSEbuQ3kwVZPiCkw1rM45GLgnvgPm Z4AsvGXKTlP7JsOul0AAUpaxlPAIY0gsrLwL9MiYoYuMQ6L6TO3yzgXJyxnZRvp+ jtbY428Tzk+iEi5DuF8gexjpBHwznO/my3ickth/O3fCvFWfKEcF6MqEAd4+26aI ltp/+9yq6CB5vfxYtjqmtlBszlAGV3LvzyYzkBIp/5m62bEJ7/gvtET2/W1vfPsg vY4oRDFmYVdvqYxQCwBv7aNKZ4xfO67HgJeBjDQpS+Q== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=sDxhL5 1Vhfp7bMJtc05jY3INv15IU8xKUsLjHr0xiOU=; b=LikqJHGPtkrNylI5xzansd wjJ/KJ/R7Dq22hms9Md8biwwcNcdgUs/TtLHDiHdLWd/KIyZNBRPQyEEuZqOAVQH Zq59rIAwtrVdT/8xqrz21GDUEkDYGNlS2+2VlDDnQMqXZW2egzoG0Z70QoZsOgpH Cv+G0h3ZqaTJuyO4BYEoteVvzAOlKtK1B98G73tE5c5NwbRRkCgLWGFAWw/fAa69 zKxmdFUnM/ToyfHDRec9Q6ytEXmtIFW+kY42SZebw6yv+4kH64ts8JA31fa6TubI bDGw4zZCK5hUm4lShfikDy9nGwJ5aG1okt63wGVtgRstD+gcvL8O6zoqeehmfcBA == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledruddvgdehkecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomhepofgrgihimhgv ucftihhprghrugcuoehmrgigihhmvgestggvrhhnohdrthgvtghhqeenucggtffrrghtth gvrhhnpeejuddvhfekkefhtdegiefhledutdevtdfhkedtleefjefgleduhfetudevjeeh hfenucffohhmrghinhepfhhrvggvuggvshhkthhophdrohhrghenucfkphepledtrdekle drieekrdejieenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhr ohhmpehmrgigihhmvgestggvrhhnohdrthgvtghh X-ME-Proxy: Received: from localhost (lfbn-tou-1-1502-76.w90-89.abo.wanadoo.fr [90.89.68.76]) by mail.messagingengine.com (Postfix) with ESMTPA id 4410A24005A; Wed, 20 Jan 2021 08:50:27 -0500 (EST) Date: Wed, 20 Jan 2021 14:50:26 +0100 From: Maxime Ripard To: Laurent Pinchart Subject: Re: [PATCH 10/10] drm: Use state helper instead of the plane state pointer Message-ID: <20210120135026.np2ivojt5vnvyota@gilmour> References: <20210115125703.1315064-1-maxime@cerno.tech> <20210115125703.1315064-10-maxime@cerno.tech> MIME-Version: 1.0 In-Reply-To: X-Mailman-Approved-At: Thu, 21 Jan 2021 08:52:31 +0000 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Haneen Mohammed , Alexandre Belloni , Neil Armstrong , David Airlie , Liviu Dudau , dri-devel@lists.freedesktop.org, Sandy Huang , Paul Cercueil , linux-tegra@vger.kernel.org, Chen-Yu Tsai , Thierry Reding , Gerd Hoffmann , Anitha Chrisanthus , Daniel Vetter , Sam Ravnborg , Michal Simek , Jerome Brunet , Marek Vasut , Yannick Fertre , linux-samsung-soc@vger.kernel.org, Joonyoung Shim , linux-rockchip@lists.infradead.org, Alexey Brodkin , Russell King , Krzysztof Kozlowski , Jonathan Hunter , Roland Scheidegger , Xinliang Liu , Ludovic Desroches , VMware Graphics , NXP Linux Team , linux-arm-msm@vger.kernel.org, Philippe Cornu , Dave Airlie , Xinwei Kong , virtualization@lists.linux-foundation.org, Hyun Kwon , Chun-Kuang Hu , Thomas Zimmermann , Alexandre Torgue , Martin Blumenstingl , Chen Feng , Sascha Hauer , Alison Wang , linux-renesas-soc@vger.kernel.org, Hans de Goede , linux-mediatek@lists.infradead.org, Laurentiu Palcu , Matthias Brugger , linux-amlogic@lists.infradead.org, Edmund Dea , Sean Paul , Pengutronix Kernel Team , linux-arm-kernel@lists.infradead.org, Melissa Wen , Maxime Coquelin , Jernej Skrabec , freedreno@lists.freedesktop.org, Rodrigo Siqueira , Tomi Valkeinen , Boris Brezillon , Jyri Sarha , linux-stm32@st-md-mailman.stormreply.com, Seung-Woo Kim , Nicolas Ferre , linux-kernel@vger.kernel.org, Vincent Abriou , Kyungmin Park , Kieran Bingham , spice-devel@lists.freedesktop.org, Kevin Hilman , Tian Tao , Shawn Guo Content-Type: multipart/mixed; boundary="===============1126441042==" Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" --===============1126441042== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="j7svqftvzagigb3f" Content-Disposition: inline --j7svqftvzagigb3f Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Laurent, On Fri, Jan 15, 2021 at 11:20:21PM +0200, Laurent Pinchart wrote: > Hi Maxime, >=20 > Thank you for the patch. >=20 > On Fri, Jan 15, 2021 at 01:57:02PM +0100, Maxime Ripard wrote: > > Many drivers reference the plane->state pointer in order to get the > > current plane state in their atomic_update or atomic_disable hooks, >=20 > Please don't use the word "current", it's ambiguous. Do you mean old > state or new state ? It's kind of the point I was trying to make: plane->state is the current state of the plane, but it's definitely ambiguous and it's fairly easy to be confused when working over several hooks. > > which would be the new plane state in the global atomic state since > > _swap_state happened when those hooks are run. >=20 > Is this relevant ? drm_atomic_helper_swap_state() doesn't change the > old_state and new_state pointers in drm_atomic_state as far as I can > tell. No, but it does change the plane->state pointer: before swap_state it's the old state, after swap_state it's the new state > > Use the drm_atomic_get_new_plane_state helper to get that state to make= it > > more obvious. > >=20 > > This was made using the coccinelle script below: > >=20 > > @ plane_atomic_func @ > > identifier helpers; > > identifier func; > > @@ > >=20 > > ( > > static const struct drm_plane_helper_funcs helpers =3D { > > ..., > > .atomic_disable =3D func, > > ..., > > }; > > | > > static const struct drm_plane_helper_funcs helpers =3D { > > ..., > > .atomic_update =3D func, > > ..., > > }; > > ) > >=20 > > @ adds_new_state @ > > identifier plane_atomic_func.func; > > identifier plane, state; > > identifier new_state; > > @@ > >=20 > > func(struct drm_plane *plane, struct drm_atomic_state *state) > > { > > ... > > - struct drm_plane_state *new_state =3D plane->state; > > + struct drm_plane_state *new_state =3D drm_atomic_get_new_plane_state(= state, plane); > > ... > > } > >=20 > > @ include depends on adds_new_state @ > > @@ > >=20 > > #include > >=20 > > @ no_include depends on !include && adds_new_state @ > > @@ > >=20 > > + #include > > #include > >=20 > > Signed-off-by: Maxime Ripard > > --- >=20 > [snip] >=20 > > drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++-- > > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 ++- > > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 ++- > > drivers/gpu/drm/xlnx/zynqmp_disp.c | 3 ++- >=20 > [snip] >=20 > > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/oma= pdrm/omap_plane.c > > index cd8cf7c786b5..021a94de84a1 100644 > > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > > @@ -44,7 +44,8 @@ static void omap_plane_atomic_update(struct drm_plane= *plane, > > { > > struct omap_drm_private *priv =3D plane->dev->dev_private; > > struct omap_plane *omap_plane =3D to_omap_plane(plane); > > - struct drm_plane_state *new_state =3D plane->state; >=20 > This seems to imply that you're interested in the new state. Well, to be fair, the variable is only called "state" before this series and it's one of the previous patch that renames it to new_state and makes it a more obvious. Otherwise, state =3D plane->state is fairly confusing and error-prone. With that change you would make it really obvious > > + struct drm_plane_state *new_state =3D drm_atomic_get_new_plane_state(= state, > > + plane); >=20 > Does this really make things more obvious ? I guess you're better at remembering this than I am then :) The discussion on whether it's more obvious or not aside, accessing the ->state pointer directly has some culprits, see: https://dri.freedesktop.org/docs/drm/gpu/todo.html#plumb-drm-atomic-state-a= ll-over Maxime --j7svqftvzagigb3f Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYAg1IgAKCRDj7w1vZxhR xW+rAQCaKXuyQvbRubp33hPCkdAY/LXaD+3TQwLv2j7AhTFUtAD/aLv/q1HqwcEO ZfMJ4deD5+JElSu70qt5AOEI/9JMPAg= =q+gU -----END PGP SIGNATURE----- --j7svqftvzagigb3f-- --===============1126441042== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1126441042==-- 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=-13.9 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 9B5EDC43332 for ; Wed, 20 Jan 2021 13:51:44 +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 C2AF62336E for ; Wed, 20 Jan 2021 13:51:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C2AF62336E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cerno.tech Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-amlogic-bounces+linux-amlogic=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-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-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=nxYihF9++v+dfa2Yh1v/odwI3pp+/JMmxhbXNigqHKI=; b=IOxqjCL20+ruVeqFMVnxPS0LP ZoFN0QT1IBMPhUYQWBeTii+VnCNcswIUNTHgXhHXlaqfAv7Pnoy75+b2L5oKMovgTGYHGqQ0xWn4/ h/6heZ3yWi73FjNtaGwe0lx3wZYiRdhaFYTrgWf9DL5PHqBjpvlaJLcV62XEUO5aUzscXv9mqloFi pcIvgZabiCDoDUpTJ1i5dM7vnNbc1PhmJH8ufA2hrBnpMibkqOQB5erxck7apoV5wZEauV20EaE10 7Q3tAShZgJ0Z9EBuI9qB5ZVXeE/QjQZMhTHUSwzQYUm5juiuKtxnCPonxCsuJPI7Xhcty0Sw82Jpf mmtVK2QSA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l2DtP-0004u6-AE; Wed, 20 Jan 2021 13:51:27 +0000 Received: from casper.infradead.org ([2001:8b0:10b:1236::1]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1l2DtD-0004ot-BI; Wed, 20 Jan 2021 13:51:15 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=sDxhL51Vhfp7bMJtc05jY3INv15IU8xKUsLjHr0xiOU=; b=vJdEETATxp3YRtk6aondiv8sV7 l7PlfwUWlF5cknTj757w9bcr9dU0BkBYe+ItJv41Mq0tzjo+/2wsi4/uH1u1dUfQ9l7SX2h60vdTe NHtYpoh0YiZvWDNwDgPgBjR5crl6laBst/cmQ98A03rmID2Z6V1KGGgMoqai+fQcxZZ+pla+ha0uC IqOpB3rtJcXmlvaYaGDAje0s6EDGhR3q0Tx9IOaq3X9ePdJ1layAUDrtDu4pD7lxzofuANOCYDs7N Nm+9zHIP6Vagwi2lUp8dBIs6fu5SRu3gW3FGM0O0DT7BXdG6ThPMid2k+Kc9z+2ppCIH+/cvF4B10 mzwNG2Pg==; Received: from new3-smtp.messagingengine.com ([66.111.4.229]) by casper.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1l2Dsn-00FkNI-N1; Wed, 20 Jan 2021 13:51:02 +0000 Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailnew.nyi.internal (Postfix) with ESMTP id 544A5580616; Wed, 20 Jan 2021 08:50:31 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Wed, 20 Jan 2021 08:50:31 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm1; bh=sDxhL51Vhfp7bMJtc05jY3INv15 IU8xKUsLjHr0xiOU=; b=e6RAvVNPXxx7n+xiZF1cUAYNfJs5LTR6PfLc0CImUdL LlXQ2amT0WEqGAlHTdvKd/rcGDF9tSdtsirQSEbuQ3kwVZPiCkw1rM45GLgnvgPm Z4AsvGXKTlP7JsOul0AAUpaxlPAIY0gsrLwL9MiYoYuMQ6L6TO3yzgXJyxnZRvp+ jtbY428Tzk+iEi5DuF8gexjpBHwznO/my3ickth/O3fCvFWfKEcF6MqEAd4+26aI ltp/+9yq6CB5vfxYtjqmtlBszlAGV3LvzyYzkBIp/5m62bEJ7/gvtET2/W1vfPsg vY4oRDFmYVdvqYxQCwBv7aNKZ4xfO67HgJeBjDQpS+Q== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=sDxhL5 1Vhfp7bMJtc05jY3INv15IU8xKUsLjHr0xiOU=; b=LikqJHGPtkrNylI5xzansd wjJ/KJ/R7Dq22hms9Md8biwwcNcdgUs/TtLHDiHdLWd/KIyZNBRPQyEEuZqOAVQH Zq59rIAwtrVdT/8xqrz21GDUEkDYGNlS2+2VlDDnQMqXZW2egzoG0Z70QoZsOgpH Cv+G0h3ZqaTJuyO4BYEoteVvzAOlKtK1B98G73tE5c5NwbRRkCgLWGFAWw/fAa69 zKxmdFUnM/ToyfHDRec9Q6ytEXmtIFW+kY42SZebw6yv+4kH64ts8JA31fa6TubI bDGw4zZCK5hUm4lShfikDy9nGwJ5aG1okt63wGVtgRstD+gcvL8O6zoqeehmfcBA == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledruddvgdehkecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomhepofgrgihimhgv ucftihhprghrugcuoehmrgigihhmvgestggvrhhnohdrthgvtghhqeenucggtffrrghtth gvrhhnpeejuddvhfekkefhtdegiefhledutdevtdfhkedtleefjefgleduhfetudevjeeh hfenucffohhmrghinhepfhhrvggvuggvshhkthhophdrohhrghenucfkphepledtrdekle drieekrdejieenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhr ohhmpehmrgigihhmvgestggvrhhnohdrthgvtghh X-ME-Proxy: Received: from localhost (lfbn-tou-1-1502-76.w90-89.abo.wanadoo.fr [90.89.68.76]) by mail.messagingengine.com (Postfix) with ESMTPA id 4410A24005A; Wed, 20 Jan 2021 08:50:27 -0500 (EST) Date: Wed, 20 Jan 2021 14:50:26 +0100 From: Maxime Ripard To: Laurent Pinchart Subject: Re: [PATCH 10/10] drm: Use state helper instead of the plane state pointer Message-ID: <20210120135026.np2ivojt5vnvyota@gilmour> References: <20210115125703.1315064-1-maxime@cerno.tech> <20210115125703.1315064-10-maxime@cerno.tech> MIME-Version: 1.0 In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210120_135102_279540_1DB56C47 X-CRM114-Status: GOOD ( 28.41 ) X-BeenThere: linux-amlogic@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Haneen Mohammed , Alexandre Belloni , Heiko =?utf-8?Q?St=C3=BCbner?= , Neil Armstrong , David Airlie , Liviu Dudau , Stefan Agner , Sandy Huang , Paul Cercueil , linux-tegra@vger.kernel.org, Chen-Yu Tsai , Thierry Reding , Rob Clark , Gerd Hoffmann , Benjamin Gaignard , Anitha Chrisanthus , Daniel Vetter , Sam Ravnborg , Michal Simek , Jerome Brunet , Marek Vasut , Yannick Fertre , linux-samsung-soc@vger.kernel.org, Joonyoung Shim , linux-rockchip@lists.infradead.org, Fabio Estevam , Alexey Brodkin , Russell King , Krzysztof Kozlowski , Jonathan Hunter , Roland Scheidegger , Xinliang Liu , Ludovic Desroches , VMware Graphics , NXP Linux Team , linux-arm-msm@vger.kernel.org, Philippe Cornu , Dave Airlie , Xinwei Kong , virtualization@lists.linux-foundation.org, Hyun Kwon , Philipp Zabel , Chun-Kuang Hu , Thomas Zimmermann , Alexandre Torgue , Martin Blumenstingl , Chen Feng , Sascha Hauer , Alison Wang , Maarten Lankhorst , linux-renesas-soc@vger.kernel.org, Inki Dae , Hans de Goede , linux-mediatek@lists.infradead.org, John Stultz , dri-devel@lists.freedesktop.org, Laurentiu Palcu , Matthias Brugger , linux-amlogic@lists.infradead.org, Edmund Dea , Sean Paul , Pengutronix Kernel Team , linux-arm-kernel@lists.infradead.org, Melissa Wen , Maxime Coquelin , Jernej Skrabec , freedreno@lists.freedesktop.org, Rodrigo Siqueira , Tomi Valkeinen , Boris Brezillon , Jyri Sarha , linux-stm32@st-md-mailman.stormreply.com, Seung-Woo Kim , Nicolas Ferre , linux-kernel@vger.kernel.org, Vincent Abriou , Kyungmin Park , Kieran Bingham , spice-devel@lists.freedesktop.org, Daniel Vetter , Kevin Hilman , Tian Tao , Shawn Guo , Brian Starkey , Lucas Stach Content-Type: multipart/mixed; boundary="===============7786778832494837848==" Sender: "linux-amlogic" Errors-To: linux-amlogic-bounces+linux-amlogic=archiver.kernel.org@lists.infradead.org --===============7786778832494837848== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="j7svqftvzagigb3f" Content-Disposition: inline --j7svqftvzagigb3f Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Laurent, On Fri, Jan 15, 2021 at 11:20:21PM +0200, Laurent Pinchart wrote: > Hi Maxime, >=20 > Thank you for the patch. >=20 > On Fri, Jan 15, 2021 at 01:57:02PM +0100, Maxime Ripard wrote: > > Many drivers reference the plane->state pointer in order to get the > > current plane state in their atomic_update or atomic_disable hooks, >=20 > Please don't use the word "current", it's ambiguous. Do you mean old > state or new state ? It's kind of the point I was trying to make: plane->state is the current state of the plane, but it's definitely ambiguous and it's fairly easy to be confused when working over several hooks. > > which would be the new plane state in the global atomic state since > > _swap_state happened when those hooks are run. >=20 > Is this relevant ? drm_atomic_helper_swap_state() doesn't change the > old_state and new_state pointers in drm_atomic_state as far as I can > tell. No, but it does change the plane->state pointer: before swap_state it's the old state, after swap_state it's the new state > > Use the drm_atomic_get_new_plane_state helper to get that state to make= it > > more obvious. > >=20 > > This was made using the coccinelle script below: > >=20 > > @ plane_atomic_func @ > > identifier helpers; > > identifier func; > > @@ > >=20 > > ( > > static const struct drm_plane_helper_funcs helpers =3D { > > ..., > > .atomic_disable =3D func, > > ..., > > }; > > | > > static const struct drm_plane_helper_funcs helpers =3D { > > ..., > > .atomic_update =3D func, > > ..., > > }; > > ) > >=20 > > @ adds_new_state @ > > identifier plane_atomic_func.func; > > identifier plane, state; > > identifier new_state; > > @@ > >=20 > > func(struct drm_plane *plane, struct drm_atomic_state *state) > > { > > ... > > - struct drm_plane_state *new_state =3D plane->state; > > + struct drm_plane_state *new_state =3D drm_atomic_get_new_plane_state(= state, plane); > > ... > > } > >=20 > > @ include depends on adds_new_state @ > > @@ > >=20 > > #include > >=20 > > @ no_include depends on !include && adds_new_state @ > > @@ > >=20 > > + #include > > #include > >=20 > > Signed-off-by: Maxime Ripard > > --- >=20 > [snip] >=20 > > drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++-- > > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 ++- > > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 ++- > > drivers/gpu/drm/xlnx/zynqmp_disp.c | 3 ++- >=20 > [snip] >=20 > > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/oma= pdrm/omap_plane.c > > index cd8cf7c786b5..021a94de84a1 100644 > > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > > @@ -44,7 +44,8 @@ static void omap_plane_atomic_update(struct drm_plane= *plane, > > { > > struct omap_drm_private *priv =3D plane->dev->dev_private; > > struct omap_plane *omap_plane =3D to_omap_plane(plane); > > - struct drm_plane_state *new_state =3D plane->state; >=20 > This seems to imply that you're interested in the new state. Well, to be fair, the variable is only called "state" before this series and it's one of the previous patch that renames it to new_state and makes it a more obvious. Otherwise, state =3D plane->state is fairly confusing and error-prone. With that change you would make it really obvious > > + struct drm_plane_state *new_state =3D drm_atomic_get_new_plane_state(= state, > > + plane); >=20 > Does this really make things more obvious ? I guess you're better at remembering this than I am then :) The discussion on whether it's more obvious or not aside, accessing the ->state pointer directly has some culprits, see: https://dri.freedesktop.org/docs/drm/gpu/todo.html#plumb-drm-atomic-state-a= ll-over Maxime --j7svqftvzagigb3f Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYAg1IgAKCRDj7w1vZxhR xW+rAQCaKXuyQvbRubp33hPCkdAY/LXaD+3TQwLv2j7AhTFUtAD/aLv/q1HqwcEO ZfMJ4deD5+JElSu70qt5AOEI/9JMPAg= =q+gU -----END PGP SIGNATURE----- --j7svqftvzagigb3f-- --===============7786778832494837848== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic --===============7786778832494837848==--