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;
Feedback-ID: i8771445c:Fastmail
Date:   Fri, 23 Sep 2022 13:14:05 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Thomas Zimmermann <tzimmermann@suse.de>
Cc:     Javier Martinez Canillas <javierm@redhat.com>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Ben Skeggs <bskeggs@redhat.com>,
        David Airlie <airlied@linux.ie>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Emma Anholt <emma@anholt.net>,
        Karol Herbst <kherbst@redhat.com>,
        Samuel Holland <samuel@sholland.org>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Daniel Vetter <daniel@ffwll.ch>, Lyude Paul <lyude@redhat.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        Chen-Yu Tsai <wens@csie.org>, Dom Cobley <dom@raspberrypi.com>,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        Phil Elwell <phil@raspberrypi.com>,
        nouveau@lists.freedesktop.org, intel-gfx@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>,
        Hans de Goede <hdegoede@redhat.com>,
        Noralf =?utf-8?Q?Tr=C3=B8nnes?= <noralf@tronnes.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        linux-sunxi@lists.linux.dev, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 13/33] drm/client: Add some tests for
 drm_connector_pick_cmdline_mode()
Message-ID: <20220923111405.mnspcuiwfzxyxix6@houat>
References: <20220728-rpi-analog-tv-properties-v2-0-f733a0ed9f90@cerno.tech>
 <20220728-rpi-analog-tv-properties-v2-13-f733a0ed9f90@cerno.tech>
 <49ea7c7c-7d4c-8348-ea75-e0f376111e4c@suse.de>
 <b3c4ee65-fc56-f54c-3946-b6524fb36f72@redhat.com>
 <93969920-b5ed-ff15-48d4-02e2f9c23505@suse.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="4cwm4b5gx3ebmiqr"
Content-Disposition: inline
In-Reply-To: <93969920-b5ed-ff15-48d4-02e2f9c23505@suse.de>
Precedence: bulk


--4cwm4b5gx3ebmiqr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Sep 23, 2022 at 12:30:09PM +0200, Thomas Zimmermann wrote:
> Am 23.09.22 um 11:26 schrieb Javier Martinez Canillas:
> > On 9/23/22 11:15, Thomas Zimmermann wrote:
> > > Hi
> > >=20
> > > Am 22.09.22 um 16:25 schrieb Maxime Ripard:
> > > > drm_connector_pick_cmdline_mode() is in charge of finding a proper
> > > > drm_display_mode from the definition we got in the video=3D command=
 line
> > > > argument.
> > > >=20
> > > > Let's add some unit tests to make sure we're not getting any regres=
sions
> > > > there.
> > > >=20
> > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > >=20
> > > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm=
/drm_client_modeset.c
> > > > index bbc535cc50dd..d553e793e673 100644
> > > > --- a/drivers/gpu/drm/drm_client_modeset.c
> > > > +++ b/drivers/gpu/drm/drm_client_modeset.c
> > > > @@ -1237,3 +1237,7 @@ int drm_client_modeset_dpms(struct drm_client=
_dev *client, int mode)
> > > >    	return ret;
> > > >    }
> > > >    EXPORT_SYMBOL(drm_client_modeset_dpms);
> > > > +
> > > > +#ifdef CONFIG_DRM_KUNIT_TEST
> > > > +#include "tests/drm_client_modeset_test.c"
> > > > +#endif
> > >=20
> > > I strongly dislike this style of including source files in each other.
> > > It's a recipe for all kind of build errors. Can you do something else?
> > >=20
> >=20
> > This seems to be the convention used to test static functions and what's
> > documented in the Kunit docs: https://kunit.dev/third_party/kernel/docs=
/tips.html#testing-static-functions
>=20
> That document says "...one option is to conditionally #include the test
> file...". This doesn't sound like a strong requirement.

No, but this is the only option documented, which still indicates a very
strong preference.

> > I agree with you that's not ideal but I think that consistency with how
> > it is done by other subsystems is also important.
> > > As the tested function is an internal interface, maybe export a wrapp=
er
> > > if tests are enabled, like this:
> > >=20
> > > #ifdef CONFIG_DRM_KUNIT_TEST
> > > struct drm_display_mode *
> > > drm_connector_pick_cmdline_mode_kunit(drm_conenctor)
> > > {
> > >     return drm_connector_pick_cmdline_mode(connector)
> > > }
> > > EXPORT_SYMBOL(drm_connector_pick_cmdline_mode_kunit)
> > > #endif
> > >=20
> > > The wrapper's declaration can be located in the kunit test file.

And I'm afraid this just doesn't scale. If we start testing more and
more static functions, do we really want to have that wrapper for each
of them?

> > But that's also not nice since we are artificially exposing these only
> > to allow the static functions to be called from unit tests. And would
> > be a different approach than the one used by all other subsystems...
> >=20
>=20
> There's the problem of interference between the source files when building
> the code. It's also not the same source code after including the test fil=
e.
> At a minimum, including the tests' source file further includes more file=
s.
> <kunit/tests.h> also includes quite a few of Linux header files.
>=20
> IMHO the current convention (if any) is far from optimal and we should
> consider breaking it.

I mean... this is a discussion about theoretical issues. If there is
indeed some regular build errors on this, then sure, we can change it.

I'm confident that will affect pretty much every one using kunit equally
though, so I'm fairly sure the documentation itself will have changed.

But right now we're discussing an alternative because of a problem we
never experienced. I don't see the point of breaking the consistency
provided by the documentation for something not backed by any actual
problem.

Maxime

--4cwm4b5gx3ebmiqr
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYy2U/QAKCRDj7w1vZxhR
xbjnAQDguqVberZj4t3P9J7J+oQg19WB9p14JwDj9eYuF79RiAEAjgZAHDUtrTKk
vCbaI/ojPoO94K+YindeHthKWzTZrwo=
=uI0t
-----END PGP SIGNATURE-----

--4cwm4b5gx3ebmiqr--