Hi Maxime,
On Mon, May 30, 2022 at 03:11:58PM +0200, Maxime Ripard wrote:
>
> Hi,
>
> On Mon, May 30, 2022 at 12:20:17PM +0200, Jos? Exp?sito wrote:
> > Test the conversion from XRGB8888 to RGB332.
> >
> > What is tested?
> >
> > - Different values for the X in XRGB8888 to make sure it is ignored
> > - Different clip values: Single pixel and full and partial buffer
> > - Well know colors: White, black, red, green, blue, magenta, yellow
> > and cyan
> > - Other colors: Randomly picked
> > - Destination pitch
> >
> > Suggested-by: Javier Martinez Canillas <[email protected]>
> > Signed-off-by: Jos? Exp?sito <[email protected]>
>
> It looks mostly good to me, but I think we should Cc
> [email protected] to have their feedback.
Thanks a lot for the quick feedback.
I just cc'ed [email protected]. For anyone joining the
conversation, here is the link to the patch and the cover letter with
some questions:
https://lore.kernel.org/dri-devel/[email protected]/T/
>
> > ---
> > drivers/gpu/drm/Kconfig | 12 ++
> > drivers/gpu/drm/Makefile | 3 +
> > drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> > 3 files changed, 181 insertions(+)
> > create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index e88c497fa010..d92be6faef15 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> > help
> > CRTC helpers for KMS drivers.
> >
> > +config DRM_FORMAR_HELPER_TEST
> > + bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > + depends on DRM && KUNIT=y
> > + select DRM_KMS_HELPER
> > + default KUNIT_ALL_TESTS
> > + help
> > + KUnit tests for the drm_format_helper APIs. This option is not
> > + useful for distributions or general kernels, but only for kernel
> > + developers working on DRM and associated drivers.
> > +
> > + If in doubt, say "N".
> > +
>
> AFAIK, kunit test cases are supposed to have a .kunitconfig too to
> enable the kunit tests easily.
>
> Maxime
A .kuniconfig example is present in the cover letter. My understanding
from the docs:
https://docs.kernel.org/dev-tools/kunit/run_wrapper.html#create-a-kunitconfig-file
Is that, like the .config file, the .kunitconfig file is not meant to
be included in git, but I'm sure someone else will clarify this point.
Thanks again,
Jos? Exp?sito
On Mon, May 30, 2022 at 9:29 AM José Expósito <[email protected]> wrote:
> I just cc'ed [email protected]. For anyone joining the
> conversation, here is the link to the patch and the cover letter with
> some questions:
>
> https://lore.kernel.org/dri-devel/[email protected]/T/
Thanks for the link.
A few initial notes:
a) normally, `select`ing other kconfigs is discouraged. It's not
explicitly called out in
https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries
but this was the conclusion after some debate on the mailing lists
earlier.
b) I see `dst` is allocated with kzalloc but not freed. Should we use
`kunit_kzalloc()` instead so it does get automatically freed?
>
> >
> > > ---
> > > drivers/gpu/drm/Kconfig | 12 ++
> > > drivers/gpu/drm/Makefile | 3 +
> > > drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> > > 3 files changed, 181 insertions(+)
> > > create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> > >
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index e88c497fa010..d92be6faef15 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> > > help
> > > CRTC helpers for KMS drivers.
> > >
> > > +config DRM_FORMAR_HELPER_TEST
> > > + bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > > + depends on DRM && KUNIT=y
> > > + select DRM_KMS_HELPER
From above, a)
Specifically here, it'd be encouraged to instead do
depends on DRM && KUNIT=y && DRM_KMS_HELPER
Ideally, using a .kunitconfig file would make it so having to select
DRM_KMS_HELPER manually isn't that annoying.
> > AFAIK, kunit test cases are supposed to have a .kunitconfig too to
> > enable the kunit tests easily.
> >
> > Maxime
>
> A .kuniconfig example is present in the cover letter. My understanding
> from the docs:
>
> https://docs.kernel.org/dev-tools/kunit/run_wrapper.html#create-a-kunitconfig-file
The bit of the documentation you're looking for is
https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#defining-a-set-of-tests
You can create a drivers/gpu/drm/.kunitconfig file and run with
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_86
The contents of that file would be just like
CONFIG_KUNIT=y
CONFIG_DRM=y
CONFIG_DRM_KMS_HELPER=y # if no `select`
CONFIG_DRM_FORMAR_HELPER_TEST=y
Re "kunit test cases are supposed to have a .kunitconfig too"
As I noted in the docs:
This is a relatively new feature (5.12+) so we don’t have any
conventions yet about on what files should be checked in versus just
kept around locally. It’s up to you and your maintainer to decide if a
config is useful enough to submit (and therefore have to maintain).
So it's up to whatever people think works best/is worth it.
I think in this case, it makes sense to add the file.
> Is that, like the .config file, the .kunitconfig file is not meant to
> be included in git, but I'm sure someone else will clarify this point.
That bit of the docs needs to be rewritten.
You're recommended to check in a drivers/gpu/drm/.kunitconfig file into git.
Context: `kunit.py` used to use the "<root>/.kunitconfig" file.
Anytime you wanted to run more tests, you'd append to that file.
So we agreed that no one should check in that file specifically.
Now kunit.py
* uses "<build-dir>/.kunitconfig", by default: ".kunit/.kunitconfig"
* has the --kunitconfig flag so you can use different files
so it's no longer as relevant.
Hope that helps,
Daniel
Hi Daniel,
Thanks for looking into the patch and for your comments.
On Mon, May 30, 2022 at 03:57:56PM -0700, Daniel Latypov wrote:
> A few initial notes:
> a) normally, `select`ing other kconfigs is discouraged. It's not
> explicitly called out in
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries
> but this was the conclusion after some debate on the mailing lists
> earlier.
>
> b) I see `dst` is allocated with kzalloc but not freed. Should we use
> `kunit_kzalloc()` instead so it does get automatically freed?
Ooops yes, it was in my "I'll handle that once it works" list, but I
forgot to fix it, thanks for pointing it out
> > > > ---
> > > > drivers/gpu/drm/Kconfig | 12 ++
> > > > drivers/gpu/drm/Makefile | 3 +
> > > > drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> > > > 3 files changed, 181 insertions(+)
> > > > create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> > > >
> > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > > index e88c497fa010..d92be6faef15 100644
> > > > --- a/drivers/gpu/drm/Kconfig
> > > > +++ b/drivers/gpu/drm/Kconfig
> > > > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> > > > help
> > > > CRTC helpers for KMS drivers.
> > > >
> > > > +config DRM_FORMAR_HELPER_TEST
> > > > + bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > > > + depends on DRM && KUNIT=y
> > > > + select DRM_KMS_HELPER
>
> From above, a)
> Specifically here, it'd be encouraged to instead do
> depends on DRM && KUNIT=y && DRM_KMS_HELPER
My first attempt was to go with:
depends on KUNIT=y && DRM && DRM_KMS_HELPER
However, when I try to run the tests I get this error:
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_64
Regenerating .config ...
Populating config with:
$ make ARCH=x86_64 olddefconfig O=.kunit
ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
This is probably due to unsatisfied dependencies.
Missing: CONFIG_DRM_KMS_HELPER=y, CONFIG_DRM_FORMAR_HELPER_TEST=y
I wasn't able to figure out why that was happening, so I decided to use
"select", which seems to solve the problem.
Do you know why this could be happening?
> Ideally, using a .kunitconfig file would make it so having to select
> DRM_KMS_HELPER manually isn't that annoying.
>
> > > AFAIK, kunit test cases are supposed to have a .kunitconfig too to
> > > enable the kunit tests easily.
> > >
> > > Maxime
> >
> > A .kuniconfig example is present in the cover letter. My understanding
> > from the docs:
> >
> > https://docs.kernel.org/dev-tools/kunit/run_wrapper.html#create-a-kunitconfig-file
>
> The bit of the documentation you're looking for is
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#defining-a-set-of-tests
> You can create a drivers/gpu/drm/.kunitconfig file and run with
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_86
>
> The contents of that file would be just like
> CONFIG_KUNIT=y
> CONFIG_DRM=y
> CONFIG_DRM_KMS_HELPER=y # if no `select`
> CONFIG_DRM_FORMAR_HELPER_TEST=y
Noted, thanks a lot, I'll include it in the final version of the patch.
By the way, I also included it in an unrelated patch, just in case you
are wondering why I emailed you a random patch:
https://lore.kernel.org/linux-input/[email protected]/T/
Thanks a lot for your help,
José Expósito
> Re "kunit test cases are supposed to have a .kunitconfig too"
> As I noted in the docs:
> This is a relatively new feature (5.12+) so we don’t have any
> conventions yet about on what files should be checked in versus just
> kept around locally. It’s up to you and your maintainer to decide if a
> config is useful enough to submit (and therefore have to maintain).
>
> So it's up to whatever people think works best/is worth it.
> I think in this case, it makes sense to add the file.
>
> > Is that, like the .config file, the .kunitconfig file is not meant to
> > be included in git, but I'm sure someone else will clarify this point.
>
> That bit of the docs needs to be rewritten.
> You're recommended to check in a drivers/gpu/drm/.kunitconfig file into git.
>
> Context: `kunit.py` used to use the "<root>/.kunitconfig" file.
> Anytime you wanted to run more tests, you'd append to that file.
> So we agreed that no one should check in that file specifically.
>
> Now kunit.py
> * uses "<build-dir>/.kunitconfig", by default: ".kunit/.kunitconfig"
> * has the --kunitconfig flag so you can use different files
> so it's no longer as relevant.
>
> Hope that helps,
> Daniel
On Mon, May 30, 2022 at 9:29 AM José Expósito <[email protected]> wrote:
>
> Hi Maxime,
>
> On Mon, May 30, 2022 at 03:11:58PM +0200, Maxime Ripard wrote:
> >
> > Hi,
> >
> > On Mon, May 30, 2022 at 12:20:17PM +0200, José Expósito wrote:
> > > Test the conversion from XRGB8888 to RGB332.
> > >
> > > What is tested?
> > >
> > > - Different values for the X in XRGB8888 to make sure it is ignored
> > > - Different clip values: Single pixel and full and partial buffer
> > > - Well know colors: White, black, red, green, blue, magenta, yellow
> > > and cyan
> > > - Other colors: Randomly picked
> > > - Destination pitch
> > >
> > > Suggested-by: Javier Martinez Canillas <[email protected]>
> > > Signed-off-by: José Expósito <[email protected]>
> >
> > It looks mostly good to me, but I think we should Cc
> > [email protected] to have their feedback.
>
> Thanks a lot for the quick feedback.
>
> I just cc'ed [email protected]. For anyone joining the
> conversation, here is the link to the patch and the cover letter with
> some questions:
>
> https://lore.kernel.org/dri-devel/[email protected]/T/
>
> >
> > > ---
> > > drivers/gpu/drm/Kconfig | 12 ++
> > > drivers/gpu/drm/Makefile | 3 +
> > > drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> > > 3 files changed, 181 insertions(+)
> > > create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> > >
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index e88c497fa010..d92be6faef15 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> > > help
> > > CRTC helpers for KMS drivers.
> > >
> > > +config DRM_FORMAR_HELPER_TEST
> > > + bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > > + depends on DRM && KUNIT=y
> > > + select DRM_KMS_HELPER
> > > + default KUNIT_ALL_TESTS
> > > + help
> > > + KUnit tests for the drm_format_helper APIs. This option is not
> > > + useful for distributions or general kernels, but only for kernel
> > > + developers working on DRM and associated drivers.
> > > +
> > > + If in doubt, say "N".
> > > +
> >
> > AFAIK, kunit test cases are supposed to have a .kunitconfig too to
> > enable the kunit tests easily.
> >
> > Maxime
>
> A .kuniconfig example is present in the cover letter. (...)
FYI: it's also possible to run these tests under UML with the extra options:
CONFIG_VIRTIO_UML=y
CONFIG_UML_PCI_OVER_VIRTIO=y
I suspect it's probably better not to add those to your .kunitconfig,
as they're UML-specific and will therefore break other architectures,
but it does mean the tests can be run with, for example:
./tools/testing/kunit/kunit.py run --kunitconfig
drivers/gpu/drm/.kunitconfig --kconfig_add CONFIG_VIRTIO_UML=y
--kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
Or, without the .kunitconfig:
./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_DRM=y
--kconfig_add CONFIG_DRM_FORMAR_HELPER_TEST=y --kconfig_add
CONFIG_VIRTIO_UML=y --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
'drm-*'
Cheers,
-- David