dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Maxime Ripard <maxime@cerno.tech>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	David Airlie <airlied@linux.ie>,
	Liviu Dudau <liviu.dudau@arm.com>,
	dri-devel@lists.freedesktop.org, Sandy Huang <hjc@rock-chips.com>,
	Paul Cercueil <paul@crapouillou.net>,
	linux-tegra@vger.kernel.org, Chen-Yu Tsai <wens@csie.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Gerd Hoffmann <kraxel@redhat.com>,
	Anitha Chrisanthus <anitha.chrisanthus@intel.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Sam Ravnborg <sam@ravnborg.org>,
	Michal Simek <michal.simek@xilinx.com>,
	Jerome Brunet <jbrunet@baylibre.com>, Marek Vasut <marex@denx.de>,
	Yannick Fertre <yannick.fertre@st.com>,
	linux-samsung-soc@vger.kernel.org,
	Joonyoung Shim <jy0922.shim@samsung.com>,
	linux-rockchip@lists.infradead.org,
	Alexey Brodkin <abrodkin@synopsys.com>,
	Russell King <linux@armlinux.org.uk>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Roland Scheidegger <sroland@vmware.com>,
	Xinliang Liu <xinliang.liu@linaro.org>,
	Ludovic Desroches <ludovic.desroches@microchip.com>,
	VMware Graphics <linux-graphics-maintainer@vmware.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	linux-arm-msm@vger.kernel.org,
	Philippe Cornu <philippe.cornu@st.com>,
	Dave Airlie <airlied@redhat.com>,
	Xinwei Kong <kong.kongxinwei@hisilicon.com>,
	virtualization@lists.linux-foundation.org,
	Hyun Kwon <hyun.kwon@xilinx.com>,
	Chun-Kuang Hu <chunkuang.hu@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Alexandre Torgue <alexandre.torgue@st.com>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	Chen Feng <puck.chen@hisilicon.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Alison Wang <alison.wang@nxp.com>,
	linux-renesas-soc@vger.kernel.org,
	Hans de Goede <hdegoede@redhat.com>,
	linux-mediatek@lists.infradead.org,
	Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	linux-amlogic@lists.infradead.org,
	Edmund Dea <edmund.j.dea@intel.com>, Sean Paul <sean@poorly.run>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	linux-arm-kernel@lists.infradead.org,
	Melissa Wen <melissa.srw@gmail.com>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	freedreno@lists.freedesktop.org,
	Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>,
	Tomi Valkeinen <tomba@kernel.org>,
	Boris Brezillon <bbrezillon@kernel.org>,
	Jyri Sarha <jyri.sarha@iki.fi>,
	linux-stm32@st-md-mailman.stormreply.com,
	Seung-Woo Kim <sw0312.kim@samsung.com>,
	Nicolas Ferre <nicolas.ferre@microchip.com>,
	linux-kernel@vger.kernel.org,
	Vincent Abriou <vincent.abriou@st.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
	spice-devel@lists.freedesktop.org,
	Kevin Hilman <khilman@baylibre.com>,
	Tian Tao <tiantao6@hisilicon.com>,
	Shawn Guo <shawnguo@kernel.org>
Subject: Re: [PATCH 10/10] drm: Use state helper instead of the plane state pointer
Date: Wed, 20 Jan 2021 14:50:26 +0100	[thread overview]
Message-ID: <20210120135026.np2ivojt5vnvyota@gilmour> (raw)
In-Reply-To: <YAIHFTYiry6ebUsY@pendragon.ideasonboard.com>


[-- Attachment #1.1: Type: text/plain, Size: 3875 bytes --]

Hi Laurent,

On Fri, Jan 15, 2021 at 11:20:21PM +0200, Laurent Pinchart wrote:
> Hi Maxime,
> 
> Thank you for the patch.
> 
> 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,
> 
> 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.
> 
> 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.
> > 
> > This was made using the coccinelle script below:
> > 
> > @ plane_atomic_func @
> > identifier helpers;
> > identifier func;
> > @@
> > 
> > (
> >  static const struct drm_plane_helper_funcs helpers = {
> >  	...,
> >  	.atomic_disable = func,
> > 	...,
> >  };
> > |
> >  static const struct drm_plane_helper_funcs helpers = {
> >  	...,
> >  	.atomic_update = func,
> > 	...,
> >  };
> > )
> > 
> > @ adds_new_state @
> > identifier plane_atomic_func.func;
> > identifier plane, state;
> > identifier new_state;
> > @@
> > 
> >  func(struct drm_plane *plane, struct drm_atomic_state *state)
> >  {
> >  	...
> > -	struct drm_plane_state *new_state = plane->state;
> > +	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);
> > 	...
> >  }
> > 
> > @ include depends on adds_new_state @
> > @@
> > 
> >  #include <drm/drm_atomic.h>
> > 
> > @ no_include depends on !include && adds_new_state @
> > @@
> > 
> > + #include <drm/drm_atomic.h>
> >   #include <drm/...>
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> 
> [snip]
> 
> >  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 ++-
> 
> [snip]
> 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/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 = plane->dev->dev_private;
> >  	struct omap_plane *omap_plane = to_omap_plane(plane);
> > -	struct drm_plane_state *new_state = plane->state;
> 
> 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 = plane->state is fairly confusing and error-prone.
With that change you would make it really obvious

> > +	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
> > +									   plane);
> 
> 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-all-over

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2021-01-21  8:52 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-15 12:56 [PATCH 01/10] drm/atomic: Pass the full state to planes async atomic check and update Maxime Ripard
2021-01-15 12:56 ` [PATCH 02/10] drm: Rename plane atomic_check state names Maxime Ripard
2021-01-15 13:46   ` Thomas Zimmermann
2021-01-15 14:13     ` Maxime Ripard
2021-01-15 20:28   ` Laurent Pinchart
2021-01-15 12:56 ` [PATCH 03/10] drm/atmel-hlcdc: Rename custom plane state variable Maxime Ripard
2021-01-15 20:43   ` Sam Ravnborg
2021-01-18 17:02     ` Maxime Ripard
2021-01-15 12:56 ` [PATCH 04/10] drm/atomic: Pass the full state to planes atomic_check Maxime Ripard
2021-01-15 20:39   ` Laurent Pinchart
2021-01-15 12:56 ` [PATCH 05/10] drm: Use the state pointer directly in " Maxime Ripard
2021-01-15 20:42   ` Laurent Pinchart
2021-01-15 12:56 ` [PATCH 06/10] drm: Use state helper instead of plane state pointer in atomic_check Maxime Ripard
2021-01-15 12:56 ` [PATCH 07/10] drm: Store new plane state in a variable for atomic_update and disable Maxime Ripard
2021-01-15 20:57   ` Laurent Pinchart
2021-01-15 12:57 ` [PATCH 08/10] drm: Rename plane->state variables in atomic update " Maxime Ripard
2021-01-15 12:57 ` [PATCH 09/10] drm/atomic: Pass the full state to planes atomic disable and update Maxime Ripard
2021-01-15 21:06   ` Laurent Pinchart
2021-01-15 12:57 ` [PATCH 10/10] drm: Use state helper instead of the plane state pointer Maxime Ripard
2021-01-15 21:20   ` Laurent Pinchart
2021-01-20 13:50     ` Maxime Ripard [this message]
2021-01-15 13:34 ` [PATCH 01/10] drm/atomic: Pass the full state to planes async atomic check and update Thomas Zimmermann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210120135026.np2ivojt5vnvyota@gilmour \
    --to=maxime@cerno.tech \
    --cc=abrodkin@synopsys.com \
    --cc=airlied@linux.ie \
    --cc=airlied@redhat.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=alexandre.torgue@st.com \
    --cc=alison.wang@nxp.com \
    --cc=anitha.chrisanthus@intel.com \
    --cc=bbrezillon@kernel.org \
    --cc=chunkuang.hu@kernel.org \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=edmund.j.dea@intel.com \
    --cc=freedreno@lists.freedesktop.org \
    --cc=hamohammed.sa@gmail.com \
    --cc=hdegoede@redhat.com \
    --cc=hjc@rock-chips.com \
    --cc=hyun.kwon@xilinx.com \
    --cc=jbrunet@baylibre.com \
    --cc=jernej.skrabec@siol.net \
    --cc=jonathanh@nvidia.com \
    --cc=jy0922.shim@samsung.com \
    --cc=jyri.sarha@iki.fi \
    --cc=kernel@pengutronix.de \
    --cc=khilman@baylibre.com \
    --cc=kieran.bingham+renesas@ideasonboard.com \
    --cc=kong.kongxinwei@hisilicon.com \
    --cc=kraxel@redhat.com \
    --cc=krzk@kernel.org \
    --cc=kyungmin.park@samsung.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=laurentiu.palcu@oss.nxp.com \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-graphics-maintainer@vmware.com \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=linux-tegra@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=liviu.dudau@arm.com \
    --cc=ludovic.desroches@microchip.com \
    --cc=marex@denx.de \
    --cc=martin.blumenstingl@googlemail.com \
    --cc=matthias.bgg@gmail.com \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=melissa.srw@gmail.com \
    --cc=michal.simek@xilinx.com \
    --cc=narmstrong@baylibre.com \
    --cc=nicolas.ferre@microchip.com \
    --cc=paul@crapouillou.net \
    --cc=philippe.cornu@st.com \
    --cc=puck.chen@hisilicon.com \
    --cc=rodrigosiqueiramelo@gmail.com \
    --cc=s.hauer@pengutronix.de \
    --cc=sam@ravnborg.org \
    --cc=sean@poorly.run \
    --cc=shawnguo@kernel.org \
    --cc=spice-devel@lists.freedesktop.org \
    --cc=sroland@vmware.com \
    --cc=sw0312.kim@samsung.com \
    --cc=thierry.reding@gmail.com \
    --cc=tiantao6@hisilicon.com \
    --cc=tomba@kernel.org \
    --cc=tzimmermann@suse.de \
    --cc=vincent.abriou@st.com \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=wens@csie.org \
    --cc=xinliang.liu@linaro.org \
    --cc=yannick.fertre@st.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).