| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 20 Jan 2021 14:50:26 +0100
From: Maxime Ripard <maxime@...no.tech>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Thomas Zimmermann <tzimmermann@...e.de>,
Daniel Vetter <daniel.vetter@...el.com>,
David Airlie <airlied@...ux.ie>,
dri-devel@...ts.freedesktop.org,
Alexey Brodkin <abrodkin@...opsys.com>,
Daniel Vetter <daniel@...ll.ch>,
Liviu Dudau <liviu.dudau@....com>,
Brian Starkey <brian.starkey@....com>,
Russell King <linux@...linux.org.uk>,
Dave Airlie <airlied@...hat.com>,
Sam Ravnborg <sam@...nborg.org>,
Boris Brezillon <bbrezillon@...nel.org>,
Nicolas Ferre <nicolas.ferre@...rochip.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Ludovic Desroches <ludovic.desroches@...rochip.com>,
Inki Dae <inki.dae@...sung.com>,
Joonyoung Shim <jy0922.shim@...sung.com>,
Seung-Woo Kim <sw0312.kim@...sung.com>,
Kyungmin Park <kyungmin.park@...sung.com>,
Krzysztof Kozlowski <krzk@...nel.org>,
Stefan Agner <stefan@...er.ch>,
Alison Wang <alison.wang@....com>,
Xinliang Liu <xinliang.liu@...aro.org>,
Tian Tao <tiantao6@...ilicon.com>,
John Stultz <john.stultz@...aro.org>,
Xinwei Kong <kong.kongxinwei@...ilicon.com>,
Chen Feng <puck.chen@...ilicon.com>,
Laurentiu Palcu <laurentiu.palcu@....nxp.com>,
Lucas Stach <l.stach@...gutronix.de>,
Philipp Zabel <p.zabel@...gutronix.de>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>,
NXP Linux Team <linux-imx@....com>,
Paul Cercueil <paul@...pouillou.net>,
Anitha Chrisanthus <anitha.chrisanthus@...el.com>,
Edmund Dea <edmund.j.dea@...el.com>,
Chun-Kuang Hu <chunkuang.hu@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Neil Armstrong <narmstrong@...libre.com>,
Kevin Hilman <khilman@...libre.com>,
Jerome Brunet <jbrunet@...libre.com>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
Marek Vasut <marex@...x.de>, Tomi Valkeinen <tomba@...nel.org>,
Gerd Hoffmann <kraxel@...hat.com>,
Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
Sandy Huang <hjc@...k-chips.com>,
Heiko Stübner <heiko@...ech.de>,
Benjamin Gaignard <benjamin.gaignard@...aro.org>,
Vincent Abriou <vincent.abriou@...com>,
Yannick Fertre <yannick.fertre@...com>,
Philippe Cornu <philippe.cornu@...com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...com>,
Chen-Yu Tsai <wens@...e.org>,
Jernej Skrabec <jernej.skrabec@...l.net>,
Thierry Reding <thierry.reding@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>,
Jyri Sarha <jyri.sarha@....fi>,
Hans de Goede <hdegoede@...hat.com>,
Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>,
Melissa Wen <melissa.srw@...il.com>,
Haneen Mohammed <hamohammed.sa@...il.com>,
VMware Graphics <linux-graphics-maintainer@...are.com>,
Roland Scheidegger <sroland@...are.com>,
Hyun Kwon <hyun.kwon@...inx.com>,
Michal Simek <michal.simek@...inx.com>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-samsung-soc@...r.kernel.org,
linux-mediatek@...ts.infradead.org,
linux-amlogic@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
freedreno@...ts.freedesktop.org,
virtualization@...ts.linux-foundation.org,
spice-devel@...ts.freedesktop.org,
linux-renesas-soc@...r.kernel.org,
linux-rockchip@...ts.infradead.org,
linux-stm32@...md-mailman.stormreply.com,
linux-tegra@...r.kernel.org
Subject: Re: [PATCH 10/10] drm: Use state helper instead of the plane state
pointer
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@...no.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
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists