Messages in this thread | | | Date | Wed, 30 Nov 2022 10:59:37 +0100 | Subject | Re: [PATCH v2 15/17] drm/vc4: tests: Introduce a mocking infrastructure | From | Javier Martinez Canillas <> |
| |
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>
-- Best regards,
Javier Martinez Canillas Core Platforms Red Hat
| |