On 11/28/22 15:53, Maxime Ripard wrote:
In order to test the current atomic_check hooks we need to have a DRM device that has roughly the same capabilities and layout that the actual hardware. We'll also need a bunch of functions to create arbitrary atomic states.
Let's create some helpers to create a device that behaves like the real one, and some helpers to maintain the atomic state we want to check.
Signed-off-by: Maxime Ripard maxime@cerno.tech
[...]
+config DRM_VC4_KUNIT_TEST
- bool "KUnit tests for VC4" if !KUNIT_ALL_TESTS
- depends on DRM_VC4 && KUNIT
shouldn't this depend on DRM_KUNIT_TEST instead ?
[...]
+static struct vc4_dev *__mock_device(struct kunit *test, bool is_vc5) +{
- struct drm_device *drm;
- const struct drm_driver *drv = is_vc5 ? &vc5_drm_driver : &vc4_drm_driver;
- const struct vc4_mock_desc *desc = is_vc5 ? &vc5_mock : &vc4_mock;
- struct vc4_dev *vc4;
Since it could be vc4 or vc5, maybe can be renamed to just struct vc_dev *vc ?
+struct vc4_dummy_plane *vc4_dummy_plane(struct kunit *test,
struct drm_device *drm,enum drm_plane_type type)+{
- struct vc4_dummy_plane *dummy_plane;
- struct drm_plane *plane;
- dummy_plane = drmm_universal_plane_alloc(drm,
struct vc4_dummy_plane, plane.base,0,&vc4_dummy_plane_funcs,vc4_dummy_plane_formats,ARRAY_SIZE(vc4_dummy_plane_formats),NULL,DRM_PLANE_TYPE_PRIMARY,NULL);- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy_plane);
- plane = &dummy_plane->plane.base;
- drm_plane_helper_add(plane, &vc4_dummy_plane_helper_funcs);
- return dummy_plane;
+}
I guess many of these helpers could grow to be generic, like this one since most drivers support the DRM_FORMAT_XRGB8888 format for their primary plane.
[...]
+extern const struct vc4_pv_data bcm2835_pv0_data; +extern const struct vc4_pv_data bcm2835_pv1_data; +extern const struct vc4_pv_data bcm2835_pv2_data; +extern const struct vc4_pv_data bcm2711_pv0_data; +extern const struct vc4_pv_data bcm2711_pv1_data; +extern const struct vc4_pv_data bcm2711_pv2_data; +extern const struct vc4_pv_data bcm2711_pv3_data; +extern const struct vc4_pv_data bcm2711_pv4_data;
Maybe the driver could expose a helper function to get the pixelvalve data and avoid having to expose all of these variables? For example you could define an enum vc4_pixelvalve type and have something like the following:
const struct vc4_pv_data *vc4_crtc_get_pixelvalve_data(enum vc4_pixelvalve pv);
All these are small nits though, the patch looks great to me and I think is awesome to have this level of testing with KUnit. Hope other drivers follow your lead.
Reviewed-by: Javier Martinez Canillas javierm@redhat.com