Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d9f088d1d548c8735b393a15d5a16dbd914ddeca.camel@collabora.com>
Subject: Re: [PATCH 2/2] media: verisilicon: change confusingly named
 relaxed register access
From:   Nicolas Dufresne <nicolas.dufresne@collabora.com>
To:     Arnd Bergmann <arnd@arndb.de>, Arnd Bergmann <arnd@kernel.org>,
        Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Mauro Carvalho Chehab <mchehab@kernel.org>
Cc:     Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Benjamin Gaignard <benjamin.gaignard@collabora.com>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
        linux-kernel@vger.kernel.org
Date:   Mon, 19 Jun 2023 14:29:38 -0400
In-Reply-To: <063a8886-fd31-425f-901c-fc830512eca3@app.fastmail.com>
References: <20230616144854.3818934-1-arnd@kernel.org>
         <20230616144854.3818934-2-arnd@kernel.org>
         <e264ac3a15e0f115aa7e941a77eb312429b8f65e.camel@collabora.com>
         <063a8886-fd31-425f-901c-fc830512eca3@app.fastmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.3 (3.48.3-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

Le lundi 19 juin 2023 =C3=A0 16:49 +0200, Arnd Bergmann a =C3=A9crit=C2=A0:
> On Mon, Jun 19, 2023, at 16:41, Nicolas Dufresne wrote:
> > Le vendredi 16 juin 2023 =C3=A0 16:48 +0200, Arnd Bergmann a =C3=A9crit=
=C2=A0:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >=20
> > > The register abstraction has wrappers around both the normal writel()
> > > and its writel_relaxed() counterpart, but this has led to a lot of us=
ers
> > > ending up with the relaxed version.
> > >=20
> > > There is sometimes a need to intentionally pick the relaxed accessor =
for
> > > performance critical functions, but I noticed that each hantro_reg_wr=
ite()
> > > call also contains a non-relaxed readl(), which is typically much mor=
e
> > > expensive than a writel, so there is little benefit here but an added
> > > risk of missing a serialization against DMA.
> > >=20
> > > To make this behave like other interfaces, use the normal accessor by
> > > default and only provide the relaxed version as an alternative for
> > > performance critical code. hantro_postproc.c is the only place that
> > > used both the relaxed and normal writel, but this does not seem
> > > cricital either, so change it all to the normal ones.
> >=20
> > In this text you spoke about potential performance side effects of exis=
ting code
> > and your changes, but its left all very vague and theoretical. Have you=
 done any
> > measurement ? Do you need help with the manner ?
>=20
> I don't have this hardware and have not done any measurements.
> Obviously the only point of using relaxed accessors is to
> improve performance in critical code paths, but from the way they
> are used here it seems that this was instead just an accident
> and nobody else did any comparisons either.
>=20
> My guess would be that if one wanted to speed up the register
> access, a better way would be to use a regmap cache to avoid
> reading registers when the contents are already known.

All I know is that for the majority of registers when programming stateless
codecs, each 32bit word of registers are fully written too, the read value =
is
not always meaningful (its a value from last time the HW has been triggered=
) and
should be ignored, so better to not do that. As for regmap, there is folks =
that
have reported regmap to be completely overkill for this type of hardware.

That discussion highlight my concern, which is that this specific patch sho=
uld
require a Tested-by before being merged. A clearer note to say that this pa=
tch
is not tested could have helped.

regards,
Nicolas

>=20
>      Arnd