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 <[email protected]>
> ---
[...]
> +
> +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 <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Hi Javier,
On Wed, Nov 30, 2022 at 10:59:37AM +0100, Javier Martinez Canillas wrote:
> 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 <[email protected]>
> > ---
>
> [...]
>
> > +
> > +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 ?
>
> [...]
You're right, but the rework suggested by Ma?ra will add a select to the
helpers Kconfig symbol there so we should be safe.
> > +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 ?
vc4_dev is the main structure in the driver for the DRM device, so we
can't rename it easily.
Generally speaking the driver was (and still is) called vc4 after the IP
name in the original RaspberryPi SoC.
There's been a new generation since, but we supported it through the vc4
driver. Even if it's a bit ambiguous, vc4 refers to both the driver name
and is used extensively in the infrastructure, but also refers to the
initial generation we supported. vc5 is only the new generation.
I'm not sure removing the number would be less confusing.
> > +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.
Yeah, that's what I'd expect at some point as well :)
> > +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.
I'm not sure. It adds an interface for something we don't really need,
so I'm not sure if it's really beneficial.
David pointed at that patch though, which seems more promising:
https://lore.kernel.org/linux-kselftest/[email protected]/
Maxime