| Message ID | 20221123-rpi-kunit-tests-v2-1-efe5ed518b63@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Introduce Kunit Tests to VC4 | expand |
On 11/28/22 15:53, Maxime Ripard wrote: > We'll need to use those helpers from drivers too, so let's move it to a > more visible location. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/tests/drm_client_modeset_test.c | 3 +-- > drivers/gpu/drm/tests/drm_kunit_helpers.c | 3 +-- > drivers/gpu/drm/tests/drm_modes_test.c | 3 +-- > drivers/gpu/drm/tests/drm_probe_helper_test.c | 3 +-- > {drivers/gpu/drm/tests => include/drm}/drm_kunit_helpers.h | 0 > 5 files changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c > index 52929536a158..ed2f62e92fea 100644 > --- a/drivers/gpu/drm/tests/drm_client_modeset_test.c > +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c > @@ -8,12 +8,11 @@ > #include <drm/drm_connector.h> > #include <drm/drm_edid.h> > #include <drm/drm_drv.h> > +#include <drm/drm_kunit_helpers.h> I wonder if now that this header was moved outside of the tests directory, if we should add stub functions in the header file that are just defined but do nothing if CONFIG_DRM_KUNIT_TEST isn't enabled. So that including it in drivers will be a no-op. Or do you plan to conditionally include this header file in drivers? So that is only included when CONFIG_DRM_KUNIT_TEST is enabled? Another thing that wondered is if we want a different namespace for this header, i.e: <drm/testing/drm_kunit_helpers.h>, to make it clear that is not part of the DRM API but just for testing helpers. But these are open questions really, and they can be done as follow-up: Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Hi Javier, On Wed, Nov 30, 2022 at 09:00:03AM +0100, Javier Martinez Canillas wrote: > On 11/28/22 15:53, Maxime Ripard wrote: > > We'll need to use those helpers from drivers too, so let's move it to a > > more visible location. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/gpu/drm/tests/drm_client_modeset_test.c | 3 +-- > > drivers/gpu/drm/tests/drm_kunit_helpers.c | 3 +-- > > drivers/gpu/drm/tests/drm_modes_test.c | 3 +-- > > drivers/gpu/drm/tests/drm_probe_helper_test.c | 3 +-- > > {drivers/gpu/drm/tests => include/drm}/drm_kunit_helpers.h | 0 > > 5 files changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c > > index 52929536a158..ed2f62e92fea 100644 > > --- a/drivers/gpu/drm/tests/drm_client_modeset_test.c > > +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c > > @@ -8,12 +8,11 @@ > > #include <drm/drm_connector.h> > > #include <drm/drm_edid.h> > > #include <drm/drm_drv.h> > > +#include <drm/drm_kunit_helpers.h> > > I wonder if now that this header was moved outside of the tests directory, > if we should add stub functions in the header file that are just defined > but do nothing if CONFIG_DRM_KUNIT_TEST isn't enabled. So that including > it in drivers will be a no-op. > > Or do you plan to conditionally include this header file in drivers? So > that is only included when CONFIG_DRM_KUNIT_TEST is enabled? I'm not entirely sure. I'd expect only the tests to include it, and thus would depend on DRM_KUNIT_TEST already. But we can always add the stubs if it's ever included in a different context. > Another thing that wondered is if we want a different namespace for this > header, i.e: <drm/testing/drm_kunit_helpers.h>, to make it clear that is > not part of the DRM API but just for testing helpers. If there's a single header, I don't think we need to create the directory. This is also something we can consolidate later on if needed. > But these are open questions really, and they can be done as follow-up: > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Thanks :) Maxime
Hello Maxime, On 12/1/22 11:27, Maxime Ripard wrote: [...] >> >> I wonder if now that this header was moved outside of the tests directory, >> if we should add stub functions in the header file that are just defined >> but do nothing if CONFIG_DRM_KUNIT_TEST isn't enabled. So that including >> it in drivers will be a no-op. >> >> Or do you plan to conditionally include this header file in drivers? So >> that is only included when CONFIG_DRM_KUNIT_TEST is enabled? > > I'm not entirely sure. I'd expect only the tests to include it, and thus > would depend on DRM_KUNIT_TEST already. But we can always add the stubs > if it's ever included in a different context. > >> Another thing that wondered is if we want a different namespace for this >> header, i.e: <drm/testing/drm_kunit_helpers.h>, to make it clear that is >> not part of the DRM API but just for testing helpers. > > If there's a single header, I don't think we need to create the > directory. This is also something we can consolidate later on if needed. > Agree on both. It's better to land as is and then figure out if needs to be changed once other drivers add more tests. >> But these are open questions really, and they can be done as follow-up: >> >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > Thanks :) You are welcome!
diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c index 52929536a158..ed2f62e92fea 100644 --- a/drivers/gpu/drm/tests/drm_client_modeset_test.c +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c @@ -8,12 +8,11 @@ #include <drm/drm_connector.h> #include <drm/drm_edid.h> #include <drm/drm_drv.h> +#include <drm/drm_kunit_helpers.h> #include <drm/drm_modes.h> #include <drm/drm_modeset_helper_vtables.h> #include <drm/drm_probe_helper.h> -#include "drm_kunit_helpers.h" - struct drm_client_modeset_test_priv { struct drm_device *drm; struct drm_connector connector; diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c index 8c738384a992..6600a4db3158 100644 --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c @@ -1,14 +1,13 @@ // SPDX-License-Identifier: GPL-2.0 #include <drm/drm_drv.h> +#include <drm/drm_kunit_helpers.h> #include <drm/drm_managed.h> #include <kunit/resource.h> #include <linux/device.h> -#include "drm_kunit_helpers.h" - struct kunit_dev { struct drm_device base; }; diff --git a/drivers/gpu/drm/tests/drm_modes_test.c b/drivers/gpu/drm/tests/drm_modes_test.c index 9358a885c58b..3953e478c4d0 100644 --- a/drivers/gpu/drm/tests/drm_modes_test.c +++ b/drivers/gpu/drm/tests/drm_modes_test.c @@ -4,14 +4,13 @@ */ #include <drm/drm_drv.h> +#include <drm/drm_kunit_helpers.h> #include <drm/drm_modes.h> #include <kunit/test.h> #include <linux/units.h> -#include "drm_kunit_helpers.h" - struct drm_test_modes_priv { struct drm_device *drm; }; diff --git a/drivers/gpu/drm/tests/drm_probe_helper_test.c b/drivers/gpu/drm/tests/drm_probe_helper_test.c index 7e938258c742..1f3941c150ae 100644 --- a/drivers/gpu/drm/tests/drm_probe_helper_test.c +++ b/drivers/gpu/drm/tests/drm_probe_helper_test.c @@ -7,6 +7,7 @@ #include <drm/drm_connector.h> #include <drm/drm_device.h> #include <drm/drm_drv.h> +#include <drm/drm_kunit_helpers.h> #include <drm/drm_mode.h> #include <drm/drm_modes.h> #include <drm/drm_modeset_helper_vtables.h> @@ -14,8 +15,6 @@ #include <kunit/test.h> -#include "drm_kunit_helpers.h" - struct drm_probe_helper_test_priv { struct drm_device *drm; struct drm_connector connector; diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h similarity index 100% rename from drivers/gpu/drm/tests/drm_kunit_helpers.h rename to include/drm/drm_kunit_helpers.h
We'll need to use those helpers from drivers too, so let's move it to a more visible location. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/tests/drm_client_modeset_test.c | 3 +-- drivers/gpu/drm/tests/drm_kunit_helpers.c | 3 +-- drivers/gpu/drm/tests/drm_modes_test.c | 3 +-- drivers/gpu/drm/tests/drm_probe_helper_test.c | 3 +-- {drivers/gpu/drm/tests => include/drm}/drm_kunit_helpers.h | 0 5 files changed, 4 insertions(+), 8 deletions(-)