[v4,2/8] kunit: drm/tests: move generic helpers
Commit Message
The creation of a dummy device in order to test managed interfaces is a
generally useful test feature. The drm test helpers
test_kunit_helper_alloc_device() and test_kunit_helper_free_device()
are doing exactly this. It makes no sense that each and every component
which intends to be testing managed interfaces will create similar
helpers.
Move these functions to place where it is more obvious they can be used
also by other subsystems but drm.
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Please note that there's something similat ongoing in the CCF:
https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/
I do like the simplicity of these DRM-originated helpers so I think
they're worth. I do equally like the Stephen's idea of having the
"dummy platform device" related helpers under drivers/base and the
header being in include/kunit/platform_device.h which is similar to real
platform device under include/linux/platform_device.h - so, in the end
of the day I hope Stephen's changes as well as the changes this patch
introduces to end up in those files. This, however, will require some
co-operation to avoid conflicts.
---
drivers/base/test/Kconfig | 5 ++
drivers/base/test/Makefile | 2 +
drivers/base/test/test_kunit_device.c | 83 +++++++++++++++++++
drivers/gpu/drm/Kconfig | 2 +
.../gpu/drm/tests/drm_client_modeset_test.c | 1 +
drivers/gpu/drm/tests/drm_kunit_helpers.c | 69 ---------------
drivers/gpu/drm/tests/drm_managed_test.c | 1 +
drivers/gpu/drm/tests/drm_modes_test.c | 1 +
drivers/gpu/drm/tests/drm_probe_helper_test.c | 1 +
drivers/gpu/drm/vc4/Kconfig | 1 +
drivers/gpu/drm/vc4/tests/vc4_mock.c | 1 +
.../gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 1 +
include/drm/drm_kunit_helpers.h | 3 -
13 files changed, 99 insertions(+), 72 deletions(-)
create mode 100644 drivers/base/test/test_kunit_device.c
Comments
Hi Matti,
On Fri, Mar 17, 2023 at 04:42:25PM +0200, Matti Vaittinen wrote:
> The creation of a dummy device in order to test managed interfaces is a
> generally useful test feature. The drm test helpers
> test_kunit_helper_alloc_device() and test_kunit_helper_free_device()
> are doing exactly this. It makes no sense that each and every component
> which intends to be testing managed interfaces will create similar
> helpers.
>
> Move these functions to place where it is more obvious they can be used
> also by other subsystems but drm.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>
> ---
>
> Please note that there's something similat ongoing in the CCF:
> https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/
>
> I do like the simplicity of these DRM-originated helpers so I think
> they're worth. I do equally like the Stephen's idea of having the
> "dummy platform device" related helpers under drivers/base and the
> header being in include/kunit/platform_device.h which is similar to real
> platform device under include/linux/platform_device.h - so, in the end
> of the day I hope Stephen's changes as well as the changes this patch
> introduces to end up in those files. This, however, will require some
> co-operation to avoid conflicts.
I think you would have an easier time if you just copied and renamed
them into the kunit folder as an preparation series.
That way, you wouldn't have to coordinate DRM, CCF and IIO, you'd just
create new helpers that can be reused/converted to by everyone eventually.
Maxime
Hi Maxime & All
First of all - I am sorry. During the last minute rebase I accidentally
dropped the header file from this series. Will fix that for v5. (Also
the build bot pointed this mistake).
On 3/17/23 17:09, Maxime Ripard wrote:
> Hi Matti,
>
> On Fri, Mar 17, 2023 at 04:42:25PM +0200, Matti Vaittinen wrote:
>> The creation of a dummy device in order to test managed interfaces is a
>> generally useful test feature. The drm test helpers
>> test_kunit_helper_alloc_device() and test_kunit_helper_free_device()
>> are doing exactly this. It makes no sense that each and every component
>> which intends to be testing managed interfaces will create similar
>> helpers.
>>
>> Move these functions to place where it is more obvious they can be used
>> also by other subsystems but drm.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>>
>> Please note that there's something similat ongoing in the CCF:
>> https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/
>>
>> I do like the simplicity of these DRM-originated helpers so I think
>> they're worth. I do equally like the Stephen's idea of having the
>> "dummy platform device" related helpers under drivers/base and the
>> header being in include/kunit/platform_device.h which is similar to real
>> platform device under include/linux/platform_device.h - so, in the end
>> of the day I hope Stephen's changes as well as the changes this patch
>> introduces to end up in those files. This, however, will require some
>> co-operation to avoid conflicts.
>
> I think you would have an easier time if you just copied and renamed
> them into the kunit folder as an preparation series.
Yes. That would simplify the syncing between the trees. It slightly bugs
me to add dublicate code in kernel-but the clean-up series for DRM users
could be prepared at the same time. It would be even possible to just
change the drm-helper to be a wrapper for the generic one - and leave
the callers intact - although it leaves some seemingly unnecessary
"onion code" there.
> That way, you wouldn't have to coordinate DRM, CCF and IIO, you'd just
> create new helpers that can be reused/converted to by everyone eventually
Yes. Thanks - I think I may go with this approach for the v5 :)
Yours,
-- Matti
Morning Stephen,
On 3/20/23 21:23, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2023-03-18 23:36:20)
>>>
>>> I think you would have an easier time if you just copied and renamed
>>> them into the kunit folder as an preparation series.
>>
>> Yes. That would simplify the syncing between the trees. It slightly bugs
>> me to add dublicate code in kernel-but the clean-up series for DRM users
>> could be prepared at the same time. It would be even possible to just
>> change the drm-helper to be a wrapper for the generic one - and leave
>> the callers intact - although it leaves some seemingly unnecessary
>> "onion code" there.
>>
>>> That way, you wouldn't have to coordinate DRM, CCF and IIO, you'd just
>>> create new helpers that can be reused/converted to by everyone eventually
>>
>> Yes. Thanks - I think I may go with this approach for the v5 :)
>
> Which kunit directory?
I was thinking of adding the platform_device.h (I liked your suggestion)
in the include/kunit/
> I imagine if there are conflicts they will be
> trivial so it probably doesn't matter.
Probably so. Still, I am not the one who needs to deal with the
conflicts. Hence I like at least asking if people see good way to avoid
them in the first place.
Besides, I was not sure if you were planning to add similar helper or
just wrappers to individual functions. Wanted to ping you just in case
this has some impact to what you do.
> Have you Cced kunit folks and the
> list on the kunit patches? They may have some opinion.
This patch was should have contained the
include/kunit/platform_device.h. That file was pulling the Kunit people
in recipients but I messed up things with last minute changes so both
the header and people were dropped. I'll fix this for v5.
Yours,
-- Matti
On 3/21/23 20:59, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2023-03-20 22:45:52)
>> Morning Stephen,
>>
>> On 3/20/23 21:23, Stephen Boyd wrote:
>>> Quoting Matti Vaittinen (2023-03-18 23:36:20)
>> Besides, I was not sure if you were planning to add similar helper or
>> just wrappers to individual functions. Wanted to ping you just in case
>> this has some impact to what you do.
>
> I don't have a need to bind a device to a driver to satisfy devm APIs
> currently. I could probably use it though to test some devm code in the
> clk APIs. The only impact is that we're modifying the same files.
Thanks for clarifying this.
>>> Have you Cced kunit folks and the
>>> list on the kunit patches? They may have some opinion.
>>
>> This patch was should have contained the
>> include/kunit/platform_device.h. That file was pulling the Kunit people
>> in recipients but I messed up things with last minute changes so both
>> the header and people were dropped. I'll fix this for v5.
>>
>
> Ok, I'll be on the lookout for v5. From what I can tell kunit goes
> through the kernel selftest tree and there isn't a kunit git tree as
> such.
Right. I am not sure what will be the best tree to carry the testability
changes. It seems that all of the IIO-tests in v5 will depend on the
kunit stuff, and I think I will also include the DRM test fixes in this
series as well just to keep things sorted in my mailbox. Anyways, I hope
to finish the changes for v5 soon(ish) - maybe already Today and in any
case during this week. I'll be CC:ing you and Brendan with (relevant
patches of) v5 as well.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Wed, Mar 22, 2023 at 06:16:27AM +0000, Vaittinen, Matti wrote:
> On 3/21/23 20:59, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2023-03-20 22:45:52)
> >> On 3/20/23 21:23, Stephen Boyd wrote:
> >>> Quoting Matti Vaittinen (2023-03-18 23:36:20)
> >> Besides, I was not sure if you were planning to add similar helper or
> >> just wrappers to individual functions. Wanted to ping you just in case
> >> this has some impact to what you do.
> >
> > I don't have a need to bind a device to a driver to satisfy devm APIs
> > currently. I could probably use it though to test some devm code in the
> > clk APIs. The only impact is that we're modifying the same files.
>
> Thanks for clarifying this.
>
> >>> Have you Cced kunit folks and the
> >>> list on the kunit patches? They may have some opinion.
> >>
> >> This patch was should have contained the
> >> include/kunit/platform_device.h. That file was pulling the Kunit people
> >> in recipients but I messed up things with last minute changes so both
> >> the header and people were dropped. I'll fix this for v5.
> >
> > Ok, I'll be on the lookout for v5. From what I can tell kunit goes
> > through the kernel selftest tree and there isn't a kunit git tree as
> > such.
>
> Right. I am not sure what will be the best tree to carry the testability
> changes. It seems that all of the IIO-tests in v5 will depend on the
> kunit stuff, and I think I will also include the DRM test fixes in this
> series as well just to keep things sorted in my mailbox. Anyways, I hope
> to finish the changes for v5 soon(ish) - maybe already Today and in any
> case during this week. I'll be CC:ing you and Brendan with (relevant
> patches of) v5 as well.
Thank you, folks, for doing this. Let's make Linux kernel greater
(than it is already is).
@@ -1,4 +1,9 @@
# SPDX-License-Identifier: GPL-2.0
+
+config TEST_KUNIT_DEVICE_HELPERS
+ depends on KUNIT
+ tristate
+
config TEST_ASYNC_DRIVER_PROBE
tristate "Build kernel module to test asynchronous driver probing"
depends on m
@@ -1,5 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o
+obj-$(CONFIG_TEST_KUNIT_DEVICE_HELPERS) += test_kunit_device.o
+
obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o
CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
new file mode 100644
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * These helpers have been extracted from drm test code at
+ * drm_kunit_helpers.c which was authored by
+ * Maxime Ripard <maxime@cerno.tech>
+ */
+
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+#include <kunit/platform_device.h>
+
+#define KUNIT_DEVICE_NAME "test-kunit-mock-device"
+
+static int fake_probe(struct platform_device *pdev)
+{
+ return 0;
+}
+
+static int fake_remove(struct platform_device *pdev)
+{
+ return 0;
+}
+
+static struct platform_driver fake_platform_driver = {
+ .probe = fake_probe,
+ .remove = fake_remove,
+ .driver = {
+ .name = KUNIT_DEVICE_NAME,
+ },
+};
+
+/**
+ * test_kunit_helper_alloc_device - Allocate a mock device for a KUnit test
+ * @test: The test context object
+ *
+ * This allocates a fake struct &device to create a mock for a KUnit
+ * test. The device will also be bound to a fake driver. It will thus be
+ * able to leverage the usual infrastructure and most notably the
+ * device-managed resources just like a "real" device.
+ *
+ * Callers need to make sure test_kunit_helper_free_device() on the
+ * device when done.
+ *
+ * Returns:
+ * A pointer to the new device, or an ERR_PTR() otherwise.
+ */
+struct device *test_kunit_helper_alloc_device(struct kunit *test)
+{
+ struct platform_device *pdev;
+ int ret;
+
+ ret = platform_driver_register(&fake_platform_driver);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ ret = platform_device_add(pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ return &pdev->dev;
+}
+EXPORT_SYMBOL_GPL(test_kunit_helper_alloc_device);
+
+/**
+ * test_kunit_helper_free_device - Frees a mock device
+ * @test: The test context object
+ * @dev: The device to free
+ *
+ * Frees a device allocated with test_kunit_helper_alloc_device().
+ */
+void test_kunit_helper_free_device(struct kunit *test, struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+
+ platform_device_unregister(pdev);
+ platform_driver_unregister(&fake_platform_driver);
+}
+EXPORT_SYMBOL_GPL(test_kunit_helper_free_device);
+
+MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
+MODULE_LICENSE("GPL");
@@ -66,6 +66,7 @@ config DRM_USE_DYNAMIC_DEBUG
config DRM_KUNIT_TEST_HELPERS
tristate
depends on DRM && KUNIT
+ select TEST_KUNIT_DEVICE_HELPERS
help
KUnit Helpers for KMS drivers.
@@ -80,6 +81,7 @@ config DRM_KUNIT_TEST
select DRM_BUDDY
select DRM_EXPORT_FOR_TESTS if m
select DRM_KUNIT_TEST_HELPERS
+ select TEST_KUNIT_DEVICE_HELPERS
default KUNIT_ALL_TESTS
help
This builds unit tests for DRM. This option is not useful for
@@ -3,6 +3,7 @@
* Copyright (c) 2022 Maxime Ripard <mripard@kernel.org>
*/
+#include <kunit/platform_device.h>
#include <kunit/test.h>
#include <drm/drm_connector.h>
@@ -9,78 +9,9 @@
#include <linux/device.h>
#include <linux/platform_device.h>
-#define KUNIT_DEVICE_NAME "drm-kunit-mock-device"
-
static const struct drm_mode_config_funcs drm_mode_config_funcs = {
};
-static int fake_probe(struct platform_device *pdev)
-{
- return 0;
-}
-
-static int fake_remove(struct platform_device *pdev)
-{
- return 0;
-}
-
-static struct platform_driver fake_platform_driver = {
- .probe = fake_probe,
- .remove = fake_remove,
- .driver = {
- .name = KUNIT_DEVICE_NAME,
- },
-};
-
-/**
- * test_kunit_helper_alloc_device - Allocate a mock device for a KUnit test
- * @test: The test context object
- *
- * This allocates a fake struct &device to create a mock for a KUnit
- * test. The device will also be bound to a fake driver. It will thus be
- * able to leverage the usual infrastructure and most notably the
- * device-managed resources just like a "real" device.
- *
- * Callers need to make sure test_kunit_helper_free_device() on the
- * device when done.
- *
- * Returns:
- * A pointer to the new device, or an ERR_PTR() otherwise.
- */
-struct device *test_kunit_helper_alloc_device(struct kunit *test)
-{
- struct platform_device *pdev;
- int ret;
-
- ret = platform_driver_register(&fake_platform_driver);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
-
- ret = platform_device_add(pdev);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- return &pdev->dev;
-}
-EXPORT_SYMBOL_GPL(test_kunit_helper_alloc_device);
-
-/**
- * test_kunit_helper_free_device - Frees a mock device
- * @test: The test context object
- * @dev: The device to free
- *
- * Frees a device allocated with test_kunit_helper_alloc_device().
- */
-void test_kunit_helper_free_device(struct kunit *test, struct device *dev)
-{
- struct platform_device *pdev = to_platform_device(dev);
-
- platform_device_unregister(pdev);
- platform_driver_unregister(&fake_platform_driver);
-}
-EXPORT_SYMBOL_GPL(test_kunit_helper_free_device);
-
struct drm_device *
__drm_kunit_helper_alloc_drm_device_with_driver(struct kunit *test,
struct device *dev,
@@ -4,6 +4,7 @@
#include <drm/drm_kunit_helpers.h>
#include <drm/drm_managed.h>
+#include <kunit/platform_device.h>
#include <kunit/resource.h>
#include <linux/device.h>
@@ -7,6 +7,7 @@
#include <drm/drm_kunit_helpers.h>
#include <drm/drm_modes.h>
+#include <kunit/platform_device.h>
#include <kunit/test.h>
#include <linux/units.h>
@@ -13,6 +13,7 @@
#include <drm/drm_modeset_helper_vtables.h>
#include <drm/drm_probe_helper.h>
+#include <kunit/platform_device.h>
#include <kunit/test.h>
struct drm_probe_helper_test_priv {
@@ -39,6 +39,7 @@ config DRM_VC4_KUNIT_TEST
tristate "KUnit tests for VC4" if !KUNIT_ALL_TESTS
depends on DRM_VC4 && KUNIT
select DRM_KUNIT_TEST_HELPERS
+ select TEST_KUNIT_DEVICE_HELPERS
default KUNIT_ALL_TESTS
help
This builds unit tests for the VC4 DRM/KMS driver. This option is
@@ -3,6 +3,7 @@
#include <drm/drm_drv.h>
#include <drm/drm_kunit_helpers.h>
+#include <kunit/platform_device.h>
#include <kunit/test.h>
#include "vc4_mock.h"
@@ -12,6 +12,7 @@
#include <drm/drm_modeset_helper_vtables.h>
#include <drm/drm_plane.h>
+#include <kunit/platform_device.h>
#include <kunit/test.h>
#include "../vc4_drv.h"
@@ -8,9 +8,6 @@
struct drm_device;
struct kunit;
-struct device *test_kunit_helper_alloc_device(struct kunit *test);
-void test_kunit_helper_free_device(struct kunit *test, struct device *dev);
-
struct drm_device *
__drm_kunit_helper_alloc_drm_device_with_driver(struct kunit *test,
struct device *dev,