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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4D659C4321E for ; Thu, 24 Nov 2022 14:04:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229630AbiKXOEC (ORCPT ); Thu, 24 Nov 2022 09:04:02 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60704 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230265AbiKXODf (ORCPT ); Thu, 24 Nov 2022 09:03:35 -0500 Received: from wnew3-smtp.messagingengine.com (wnew3-smtp.messagingengine.com [64.147.123.17]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3548C11E829; Thu, 24 Nov 2022 06:01:10 -0800 (PST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailnew.west.internal (Postfix) with ESMTP id 10C032B06842; Thu, 24 Nov 2022 09:01:06 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Thu, 24 Nov 2022 09:01:09 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h=cc :cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm2; t=1669298466; x= 1669305666; bh=4jIAn2lueWeOwVkOG/WC5OUeXxRyuqGWxy80FbSFYkU=; b=e qE3tHPPtqSf2ZlRLKRvMe8490eUorbxIPbR0H4ULV8nQghhKqp5ss/9vTme8mymX Wa3py471Rdh+fPeIAzMVkoQl6aeMBnfzjwQFfmit3QF0c0tpDpOImZXbCmkvYpqH 0Mgh+v2jXIpsIhJ2kuRbfTwJ5OpmdaDdnugMwWgjP6RbGOcH0dKgu0q2txkgDEUA YvcogazJb1A/8UAuHke2hu4dbEfN+BJz7QoK/h3OPExqr19VR7oXa8bfSl+2eu+E 8nw3hafR0oekv2iU1RwuNptkZycN/KvvC6RTAtHTDLBJHUrk8tG3RAf/NETJwYbu 3+DqHJoEWbVAyFO9ceiGQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1669298466; x= 1669305666; bh=4jIAn2lueWeOwVkOG/WC5OUeXxRyuqGWxy80FbSFYkU=; b=l T/mRiRFHe9A8a30ZwTwRCn7Atjq0eCn00PtFaChL5BXbD1p2ENm1wDe21wcAksFs auGh1enyGhzlnelBo3eNDJ/uOpMX9J7tZYgRt/nAPlONro6RUYbhccaaXtzH1JCS tNomDb+/7p9NVkzhX4q4xrvcnHo3rSKNvC8Kqv2dvMeP72TG8Hi33RrYo4iPlVQf 0Rw+qlK9SEYrojPSnFEOOZmhPRDc/BVN6IsGgOp6x0Q3Q1tqvCZUTKZh2VdD5yYX Uiz8Mf5s8/XhZDbZ8ZnTIPD8A7UDSKlBTXyNx8tHLlINbYpknGbuveNLDrkXeTPl QZ1nWcu2JE5j3/iErJSYw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvgedrieefgdeitdcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvvefukfhfgggtugfgjgesthhqredttddtvdenucfhrhhomhepofgrgihi mhgvucftihhprghrugcuoehmrgigihhmvgestggvrhhnohdrthgvtghhqeenucggtffrrg htthgvrhhnpeeitdeuffevieeufedtuddvffffffegfffgkeeihfelleektdelhfevhfdu udfhgfenucffohhmrghinhepkhgvrhhnvghlrdhorhhgnecuvehluhhsthgvrhfuihiivg eptdenucfrrghrrghmpehmrghilhhfrhhomhepmhgrgihimhgvsegtvghrnhhordhtvggt hh X-ME-Proxy: Feedback-ID: i8771445c:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 24 Nov 2022 09:01:04 -0500 (EST) Date: Thu, 24 Nov 2022 15:01:03 +0100 From: Maxime Ripard To: David Gow Cc: Maarten Lankhorst , Daniel Vetter , David Airlie , Thomas Zimmermann , linaro-mm-sig@lists.linaro.org, Greg Kroah-Hartman , linux-kselftest@vger.kernel.org, =?utf-8?B?TWHDrXJh?= Canal , linux-media@vger.kernel.org, Javier Martinez Canillas , kunit-dev@googlegroups.com, dri-devel@lists.freedesktop.org, Brendan Higgins , linux-kernel@vger.kernel.org, Dave Stevenson Subject: Re: [PATCH 00/24] drm: Introduce Kunit Tests to VC4 Message-ID: <20221124140103.saf2fyal75dscoot@houat> References: <20221123-rpi-kunit-tests-v1-0-051a0bb60a16@cerno.tech> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi David, On Thu, Nov 24, 2022 at 04:31:14PM +0800, David Gow wrote: > On Wed, Nov 23, 2022 at 11:28 PM Maxime Ripard wrote: > > > > Hi, > > > > This series introduce Kunit tests to the vc4 KMS driver, but unlike wha= t we > > have been doing so far in KMS, it actually tests the atomic modesetting= code. > > > > In order to do so, I've had to improve a fair bit on the Kunit helpers = already > > found in the tree in order to register a full blown and somewhat functi= onal KMS > > driver. > > > > It's of course relying on a mock so that we can test it anywhere. The m= ocking > > approach created a number of issues, the main one being that we need to= create > > a decent mock in the first place, see patch 22. The basic idea is that I > > created some structures to provide a decent approximation of the actual > > hardware, and that would support both major architectures supported by = vc4. > > > > This is of course meant to evolve over time and support more tests, but= I've > > focused on testing the HVS FIFO assignment code which is fairly tricky = (and the > > tests have actually revealed one more bug with our current implementati= on). I > > used to have a userspace implementation of those tests, where I would c= opy and > > paste the kernel code and run the tests on a regular basis. It's was ob= viously > > fairly suboptimal, so it seemed like the perfect testbed for that serie= s. > > Thanks very much for this! I'm really excited to see these sorts of > tests being written. >=20 > I was able to successfully run these under qemu with: > ./tools/testing/kunit/kunit.py run --kunitconfig > drivers/gpu/drm/vc4/tests --arch arm64 > --cross_compile=3Daarch64-linux-gnu- > (and also with clang, using --make_options LLVM=3D1 instead of the > --cross_compile flag) >=20 > On the other hand, they don't compile as a module: > ERROR: modpost: missing MODULE_LICENSE() in drivers/gpu/drm/vc4/tests/vc4= _mock.o > ERROR: modpost: missing MODULE_LICENSE() in > drivers/gpu/drm/vc4/tests/vc4_mock_crtc.o > ERROR: modpost: missing MODULE_LICENSE() in > drivers/gpu/drm/vc4/tests/vc4_mock_output.o > ERROR: modpost: missing MODULE_LICENSE() in > drivers/gpu/drm/vc4/tests/vc4_mock_plane.o > ERROR: modpost: missing MODULE_LICENSE() in > drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.o > ERROR: modpost: missing MODULE_LICENSE() in > drivers/gpu/drm/tests/drm_managed_test.o > ERROR: modpost: "vc4_drm_driver" > [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined! > ERROR: modpost: "vc5_drm_driver" > [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined! > ERROR: modpost: "drm_kunit_helper_alloc_device" > [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined! > ERROR: modpost: "__drm_kunit_helper_alloc_drm_device_with_driver" > [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined! > ERROR: modpost: "__vc4_hvs_alloc" > [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined! > ERROR: modpost: "vc4_dummy_plane" > [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined! > ERROR: modpost: "vc4_mock_pv" [drivers/gpu/drm/vc4/tests/vc4_mock.ko] und= efined! > ERROR: modpost: "vc4_dummy_output" > [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined! > ERROR: modpost: "vc4_kms_load" [drivers/gpu/drm/vc4/tests/vc4_mock.ko] > undefined! > ERROR: modpost: "vc4_txp_crtc_data" > [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined! > WARNING: modpost: suppressed 17 unresolved symbol warnings because > there were too many) Thanks I'll fix it > Most of those are just the need to export some symbols. There's some > work underway to support conditionally exporting symbols only if KUnit > is enabled, which may help: > https://lore.kernel.org/linux-kselftest/20221102175959.2921063-1-rmoar@go= ogle.com/ That's awesome :) The current solution to include the test implementation is not ideal, so it's great to see a nicer solution being worked on. > Otherwise, I suspect the better short-term solution would just be to > require that the tests are built-in (or at least compiled into > whatever of the drm/vc4 modules makes most sense). >=20 > The only other thing which has me a little confused is the naming of > some of the functions, specifically with the __ prefix. Is it just for > internal functions (many of them aren't static, but maybe they could > use the VISIBLE_IF_KUNIT macro if that makes sense), or for versions > of functions which accept extra arguments? It was for internal functions that would definitely benefit from VISIBLE_IF_KUNIT indeed > Not a big deal (and maybe it's a DRM naming convention I'm ignorant > of), but I couldn't quite find a pattern on my first read through. >=20 > But on the whole, these look good from a KUnit point-of-view. It's > really to see some solid mocking and driver testing, too. There would > be ways to avoid passing the 'struct kunit' around in more places (or > to store extra data as a kunit_resource), but I think it's better > overall to pass it around like you have in this case -- it's certainly > more compatible with things which might span threads (e.g. the > workqueues). One thing I'm really unsure about and would like your input on is basically the entire device instantiation code in drm_kunit_helpers.c It's a little fishy since it will allocate a platform_device while the driver might expect some other bus device. And the code to bind the driver based around probe and workqueues seems like a hack. This is something that would benefit from having proper functions in kunit to allocate a proper device for a given test. This is already something that other unit test suites seems to get wrong, and I'm sure there's some bugs somewhere in the helpers I did for DRM. What do you think? Maxime