Re: [PATCH 00/24] drm: Introduce Kunit Tests to VC4

Re: [PATCH 00/24] drm: Introduce Kunit Tests to VC4

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 5496 bytes --]

On Wed, Nov 23, 2022 at 11:28 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> This series introduce Kunit tests to the vc4 KMS driver, but unlike what we
> have been doing so far in KMS, it actually tests the atomic modesetting code.
>
> In order to do so, I've had to improve a fair bit on the Kunit helpers already
> found in the tree in order to register a full blown and somewhat functional KMS
> driver.
>
> It's of course relying on a mock so that we can test it anywhere. The mocking
> approach created a number of issues, the main one being that we need to create
> a decent mock in the first place, see patch 22. The basic idea is that I
> created some structures to provide a decent approximation of the actual
> hardware, and that would support both major architectures supported by vc4.
>
> This is of course meant to evolve over time and support more tests, but I've
> focused on testing the HVS FIFO assignment code which is fairly tricky (and the
> tests have actually revealed one more bug with our current implementation). I
> used to have a userspace implementation of those tests, where I would copy and
> paste the kernel code and run the tests on a regular basis. It's was obviously
> fairly suboptimal, so it seemed like the perfect testbed for that series.
>
> Let me know what you think,
> Maxime
>
> To: David Airlie <airlied@gmail.com>
> To: Daniel Vetter <daniel@ffwll.ch>
> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> To: Maxime Ripard <mripard@kernel.org>
> To: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Maíra Canal <mairacanal@riseup.net>
> Cc: Brendan Higgins <brendan.higgins@linux.dev>
> Cc: David Gow <davidgow@google.com>
> Cc: linux-kselftest@vger.kernel.org
> Cc: kunit-dev@googlegroups.com
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>
> ---

Hi Maxime,

Thanks very much for this! I'm really excited to see these sorts of
tests being written.

I was able to successfully run these under qemu with:
./tools/testing/kunit/kunit.py run --kunitconfig
drivers/gpu/drm/vc4/tests --arch arm64
--cross_compile=aarch64-linux-gnu-
(and also with clang, using --make_options LLVM=1 instead of the
--cross_compile flag)

On the other hand, they don't compile as a module:
ERROR: modpost: missing MODULE_LICENSE() in drivers/gpu/drm/vc4/tests/vc4_mock.o
ERROR: modpost: missing MODULE_LICENSE() in
drivers/gpu/drm/vc4/tests/vc4_mock_crtc.o
ERROR: modpost: missing MODULE_LICENSE() in
drivers/gpu/drm/vc4/tests/vc4_mock_output.o
ERROR: modpost: missing MODULE_LICENSE() in
drivers/gpu/drm/vc4/tests/vc4_mock_plane.o
ERROR: modpost: missing MODULE_LICENSE() in
drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.o
ERROR: modpost: missing MODULE_LICENSE() in
drivers/gpu/drm/tests/drm_managed_test.o
ERROR: modpost: "vc4_drm_driver"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "vc5_drm_driver"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "drm_kunit_helper_alloc_device"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "__drm_kunit_helper_alloc_drm_device_with_driver"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "__vc4_hvs_alloc"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "vc4_dummy_plane"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "vc4_mock_pv" [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "vc4_dummy_output"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "vc4_kms_load" [drivers/gpu/drm/vc4/tests/vc4_mock.ko]
undefined!
ERROR: modpost: "vc4_txp_crtc_data"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
WARNING: modpost: suppressed 17 unresolved symbol warnings because
there were too many)

Most of those are just the need to export some symbols. There's some
work underway to support conditionally exporting symbols only if KUnit
is enabled, which may help:
https://lore.kernel.org/linux-kselftest/20221102175959.2921063-1-rmoar@google.com/

Otherwise, I suspect the better short-term solution would just be to
require that the tests are built-in (or at least compiled into
whatever of the drm/vc4 modules makes most sense).

The only other thing which has me a little confused is the naming of
some of the functions, specifically with the __ prefix. Is it just for
internal functions (many of them aren't static, but maybe they could
use the VISIBLE_IF_KUNIT macro if that makes sense), or for versions
of functions which accept extra arguments? Not a big deal (and maybe
it's a DRM naming convention I'm ignorant of), but I couldn't quite
find a pattern on my first read through.

But on the whole, these look good from a KUnit point-of-view. It's
really to see some solid mocking and driver testing, too. There would
be ways to avoid passing the 'struct kunit' around in more places (or
to store extra data as a kunit_resource), but I think it's better
overall to pass it around like you have in this case -- it's certainly
more compatible with things which might span threads (e.g. the
workqueues).

Thanks a bunch,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH 00/24] drm: Introduce Kunit Tests to VC4

[Maxime Ripard]

Hi David,

On Thu, Nov 24, 2022 at 04:31:14PM +0800, David Gow wrote:
> On Wed, Nov 23, 2022 at 11:28 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi,
> >
> > This series introduce Kunit tests to the vc4 KMS driver, but unlike what we
> > have been doing so far in KMS, it actually tests the atomic modesetting code.
> >
> > In order to do so, I've had to improve a fair bit on the Kunit helpers already
> > found in the tree in order to register a full blown and somewhat functional KMS
> > driver.
> >
> > It's of course relying on a mock so that we can test it anywhere. The mocking
> > approach created a number of issues, the main one being that we need to create
> > a decent mock in the first place, see patch 22. The basic idea is that I
> > created some structures to provide a decent approximation of the actual
> > hardware, and that would support both major architectures supported by vc4.
> >
> > This is of course meant to evolve over time and support more tests, but I've
> > focused on testing the HVS FIFO assignment code which is fairly tricky (and the
> > tests have actually revealed one more bug with our current implementation). I
> > used to have a userspace implementation of those tests, where I would copy and
> > paste the kernel code and run the tests on a regular basis. It's was obviously
> > fairly suboptimal, so it seemed like the perfect testbed for that series.
>
> Thanks very much for this! I'm really excited to see these sorts of
> tests being written.
> 
> I was able to successfully run these under qemu with:
> ./tools/testing/kunit/kunit.py run --kunitconfig
> drivers/gpu/drm/vc4/tests --arch arm64
> --cross_compile=aarch64-linux-gnu-
> (and also with clang, using --make_options LLVM=1 instead of the
> --cross_compile flag)
> 
> On the other hand, they don't compile as a module:
> ERROR: modpost: missing MODULE_LICENSE() in drivers/gpu/drm/vc4/tests/vc4_mock.o
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/gpu/drm/vc4/tests/vc4_mock_crtc.o
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/gpu/drm/vc4/tests/vc4_mock_output.o
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/gpu/drm/vc4/tests/vc4_mock_plane.o
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.o
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/gpu/drm/tests/drm_managed_test.o
> ERROR: modpost: "vc4_drm_driver"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "vc5_drm_driver"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "drm_kunit_helper_alloc_device"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "__drm_kunit_helper_alloc_drm_device_with_driver"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "__vc4_hvs_alloc"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "vc4_dummy_plane"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "vc4_mock_pv" [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "vc4_dummy_output"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "vc4_kms_load" [drivers/gpu/drm/vc4/tests/vc4_mock.ko]
> undefined!
> ERROR: modpost: "vc4_txp_crtc_data"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> WARNING: modpost: suppressed 17 unresolved symbol warnings because
> there were too many)

Thanks I'll fix it

> Most of those are just the need to export some symbols. There's some
> work underway to support conditionally exporting symbols only if KUnit
> is enabled, which may help:
> https://lore.kernel.org/linux-kselftest/20221102175959.2921063-1-rmoar@google.com/

That's awesome :)

The current solution to include the test implementation is not ideal, so
it's great to see a nicer solution being worked on.

> Otherwise, I suspect the better short-term solution would just be to
> require that the tests are built-in (or at least compiled into
> whatever of the drm/vc4 modules makes most sense).
> 
> The only other thing which has me a little confused is the naming of
> some of the functions, specifically with the __ prefix. Is it just for
> internal functions (many of them aren't static, but maybe they could
> use the VISIBLE_IF_KUNIT macro if that makes sense), or for versions
> of functions which accept extra arguments?

It was for internal functions that would definitely benefit from
VISIBLE_IF_KUNIT indeed

> Not a big deal (and maybe it's a DRM naming convention I'm ignorant
> of), but I couldn't quite find a pattern on my first read through.
> 
> But on the whole, these look good from a KUnit point-of-view. It's
> really to see some solid mocking and driver testing, too. There would
> be ways to avoid passing the 'struct kunit' around in more places (or
> to store extra data as a kunit_resource), but I think it's better
> overall to pass it around like you have in this case -- it's certainly
> more compatible with things which might span threads (e.g. the
> workqueues).

One thing I'm really unsure about and would like your input on is
basically the entire device instantiation code in drm_kunit_helpers.c

It's a little fishy since it will allocate a platform_device while the
driver might expect some other bus device. And the code to bind the
driver based around probe and workqueues seems like a hack.

This is something that would benefit from having proper functions in
kunit to allocate a proper device for a given test. This is already
something that other unit test suites seems to get wrong, and I'm sure
there's some bugs somewhere in the helpers I did for DRM. What do you
think?

Maxime

Re: [PATCH 02/24] drm/tests: helpers: Remove the name parameter

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> The device name isn't really useful, we can just define it instead of
> exposing it in the API.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/tests/drm_client_modeset_test.c | 2 +-
>  drivers/gpu/drm/tests/drm_kunit_helpers.c       | 6 ++++--
>  drivers/gpu/drm/tests/drm_kunit_helpers.h       | 3 +--
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> index e469d1634e2d..6920c3ffdfdf 100644
> --- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
> +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> @@ -41,7 +41,7 @@ static int drm_client_modeset_test_init(struct kunit *test)
>  
>  	test->priv = priv;
>  
> -	priv->drm = drm_kunit_helper_alloc_drm_device(test, DRIVER_MODESET, "drm-client-modeset-test");
> +	priv->drm = drm_kunit_helper_alloc_drm_device(test, DRIVER_MODESET);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->drm);
>  
>  	ret = drmm_connector_init(priv->drm, &priv->connector,
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> index a4ad030ed101..7f69f56f5892 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -9,6 +9,8 @@
>  
>  #include "drm_kunit_helpers.h"
>  
> +#define FAKE_DEVICE_NAME	"drm-kunit-fake-device"
> +

I think that KUNIT_DEVICE_NAME and "drm-kunit-mock-device" are more descriptive
names here. But I'm also OK with the patch as is.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 03/24] drm/tests: helpers: Create the device in another function

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> We'll need in some tests to control when the device needs to be added
> and removed, so let's split the device creation from the DRM device
> creation function.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

[...]

> -static int dev_init(struct kunit_resource *res, void *ptr)
> +struct device *drm_kunit_helper_alloc_device(struct kunit *test)
>  {
> -	char *name = ptr;
> -	struct device *dev;
> -
> -	dev = root_device_register(name);
> -	if (IS_ERR(dev))
> -		return PTR_ERR(dev);
> -
> -	res->data = dev;
> -	return 0;
> +	return root_device_register(FAKE_DEVICE_NAME);

I was going to ask if a platform device could be used here, to prevent the
issue you mentioned the other day of root_device not having a bus set. But
I see now that you are doing that change in the next patch.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 04/24] drm/tests: helpers: Switch to a platform_device

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> The device managed resources are ran if the device has bus, which is not
> the case of a root_device.
> 
> Let's use a platform_device instead.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/tests/drm_kunit_helpers.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> index 2f974f45c5b4..651aa93a98a8 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -6,6 +6,7 @@
>  #include <kunit/resource.h>
>  
>  #include <linux/device.h>
> +#include <linux/platform_device.h>
>  
>  #include "drm_kunit_helpers.h"
>  
> @@ -20,12 +21,26 @@ static const struct drm_mode_config_funcs drm_mode_config_funcs = {
>  
>  struct device *drm_kunit_helper_alloc_device(struct kunit *test)
>  {
> -	return root_device_register(FAKE_DEVICE_NAME);
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	ret = platform_driver_register(&fake_platform_driver);

Maybe the platform_driver_register() is something that should be done
in the drm_client_modeset_test_init() callback ?

I would also as a part of this series rename the kunit_suite from
"drm_test_pick_cmdline_test_suite" to "drm_test_modeset_test_suite"
or something like that. Since you are already setting the .init and
.exit callbacks to allocate and release the devices. So it is more
than just a test suite for cmdline params testing.

> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	pdev = platform_device_alloc(FAKE_DEVICE_NAME, PLATFORM_DEVID_NONE);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> +
> +	ret = platform_device_add(pdev);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	return &pdev->dev;
>  }
>

Probably won't do it as a part of this series to avoid making it cross
subsystem, but I think there's room to further consolidation since this
helper seems to be quite generic and something that would be useful for
other subsystems. So it would make sense for kunit to have helpers to
manage platform drivers and devices.

In fact, I see that drivers/base/test/test_async_driver_probe.c already
has similar helpers that could be exposed to the rest of the kernel.

The patch looks good to me though and any of these changes could be done
as a follow-up:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 05/24] drm/tests: helpers: Make sure the device is bound

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> The device managed resources are freed when the device is detached, so
> it has to be bound in the first place.
> 
> Let's create a fake driver that we will bind to our fake device to
> benefit from the device managed cleanups in our tests.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/tests/drm_kunit_helpers.c | 60 +++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>

If I understood the platform core code correctly, the probe is always sync in
the case of platform drivers. Unless .probe_type = PROBE_PREFER_ASYNCHRONOUS is
set or a module is loaded using with the "async_probe=1" parameter. So I believe
the wait queue won't be needed. The only DRM driver that forces an async probe is
drivers/gpu/drm/hyperv/hyperv_drm_drv.c AFAICT.

So I would drop this patch from the set for now.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 06/24] drm/tests: kunit: Allow for a custom device struct to be allocated

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> The current helper to allocate a DRM device doesn't allow for any
> subclassing by drivers, which is going to be troublesome as we work on
> getting some kunit testing on atomic modesetting code.
> 
> Let's use a similar pattern to the other allocation helpers by providing
> the structure size and offset as arguments.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 07/24] drm/tests: helpers: Allow to pass a custom drm_driver

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> Some tests will need to provide their own drm_driver instead of relying
> on the dumb one in the helpers, so let's create a helper that allows to
> do so.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 08/24] drm/tests: Add a test for DRM managed actions

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> DRM-managed actions are supposed to be ran whenever the device is
> released. Let's introduce a basic unit test to make sure it happens.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/tests/Makefile           |  1 +
>  drivers/gpu/drm/tests/drm_managed_test.c | 68 ++++++++++++++++++++++++++++++++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> index b29ef1085cad..35962c6ef0c4 100644
> --- a/drivers/gpu/drm/tests/Makefile
> +++ b/drivers/gpu/drm/tests/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
>  	drm_format_test.o \
>  	drm_framebuffer_test.o \
>  	drm_kunit_helpers.o \
> +	drm_managed_test.o \
>  	drm_mm_test.o \
>  	drm_plane_helper_test.o \
>  	drm_rect_test.o
> diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
> new file mode 100644
> index 000000000000..4fc11b289d9e
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_managed_test.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <drm/drm_drv.h>
> +#include <drm/drm_managed.h>
> +
> +#include <kunit/resource.h>
> +
> +#include <linux/device.h>
> +
> +#include "drm_kunit_helpers.h"
> +
> +#define TEST_TIMEOUT_MS	100
> +

Why did you choose this timeout value? Maybe we can add some comments
explaining the rationale.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 09/24] drm/atomic: Constify the old/new state accessors

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> The drm_atomic_get_(old|new)_*_state don't modify the passed
> drm_atomic_state, so we can make it const.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: (subset) [PATCH 09/24] drm/atomic: Constify the old/new state accessors

[Maxime Ripard]

On Wed, 23 Nov 2022 16:25:51 +0100, Maxime Ripard wrote:
> The drm_atomic_get_(old|new)_*_state don't modify the passed
> drm_atomic_state, so we can make it const.
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: [PATCH 11/24] drm/vc4: Constify container_of wrappers

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> None of our wrappers around container_of to access our objects from the
> DRM object pointer actually modify the latter.
> 
> Let's make them const.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: (subset) [PATCH 11/24] drm/vc4: Constify container_of wrappers

[Maxime Ripard]

On Wed, 23 Nov 2022 16:25:53 +0100, Maxime Ripard wrote:
> None of our wrappers around container_of to access our objects from the
> DRM object pointer actually modify the latter.
> 
> Let's make them const.
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: [PATCH 12/24] drm/vc4: Move HVS state to main header

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> In order to introduce unit tests for the HVS state computation, we'll
> need access to the vc4_hvs_state struct definition and its associated
> helpers.
> 
> Let's move them in our driver header.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 17/24] drm/vc4: crtc: Pass the device and data in vc4_crtc_init

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> Both users of vc4_crtc_init need the same extra initialization to set
> the pointer to the platform_device and the CRTC data. Since it's
> mandatory, let's make them both arguments of vc4_crtc_init().
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: (subset) [PATCH 17/24] drm/vc4: crtc: Pass the device and data in vc4_crtc_init

[Maxime Ripard]

On Wed, 23 Nov 2022 16:25:59 +0100, Maxime Ripard wrote:
> Both users of vc4_crtc_init need the same extra initialization to set
> the pointer to the platform_device and the CRTC data. Since it's
> mandatory, let's make them both arguments of vc4_crtc_init().
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: [PATCH 18/24] drm/vc4: crtc: Introduce a lower-level crtc init helper

[Javier Martinez Canillas]

On 11/23/22 16:26, Maxime Ripard wrote:
> The current vc4_crtc_init() helper assumes that we will be using
> hardware planes and calls vc4_plane_init().
> 
> While it's a reasonable assumption, we'll want to mock the plane and
> thus provide our own. Let's create a helper that will take the plane as
> an argument.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 19/24] drm/vc4: crtc: Make encoder lookup helper public

[Javier Martinez Canillas]

On 11/23/22 16:26, Maxime Ripard wrote:
> We'll need a function that looks up an encoder by its vc4_encoder_type.
> Such a function is already present in the CRTC code, so let's make it
> public so that we can reuse it in the unit tests.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 21/24] drm/vc4: hvs: Provide a function to initialize the HVS structure

[Javier Martinez Canillas]

On 11/23/22 16:26, Maxime Ripard wrote:
> We'll need to initialize the HVS structure without a backing device to
> create a mock we'll use for testing.
> 
> Split the structure initialization part into a separate function.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 01/24] drm/tests: helpers: Rename the device init helper

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> The name doesn't really fit the conventions for the other helpers in
> DRM/KMS, so let's rename it to make it obvious that we allocate a new
> DRM device.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 01/24] drm/tests: helpers: Rename the device init helper

[Maíra Canal]

On 11/23/22 12:25, Maxime Ripard wrote:
> The name doesn't really fit the conventions for the other helpers in
> DRM/KMS, so let's rename it to make it obvious that we allocate a new
> DRM device.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Although I believe using "drm_device" on the function name is a bit 
redundant (maybe drm_kunit_helper_alloc_dev or 
drm_kunit_helper_alloc_device would be cleaner),

Reviewed-by: Maíra Canal <mairacanal@riseup.net>

Best Regards,
- Maíra Canal

> ---
>   drivers/gpu/drm/tests/drm_client_modeset_test.c | 2 +-
>   drivers/gpu/drm/tests/drm_kunit_helpers.c       | 4 +++-
>   drivers/gpu/drm/tests/drm_kunit_helpers.h       | 5 ++++-
>   3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> index 362a5fbd82f5..e469d1634e2d 100644
> --- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
> +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> @@ -41,7 +41,7 @@ static int drm_client_modeset_test_init(struct kunit *test)
>   
>   	test->priv = priv;
>   
> -	priv->drm = drm_kunit_device_init(test, DRIVER_MODESET, "drm-client-modeset-test");
> +	priv->drm = drm_kunit_helper_alloc_drm_device(test, DRIVER_MODESET, "drm-client-modeset-test");
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->drm);
>   
>   	ret = drmm_connector_init(priv->drm, &priv->connector,
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> index f1662091f250..a4ad030ed101 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -36,7 +36,9 @@ static void dev_free(struct kunit_resource *res)
>   	root_device_unregister(dev);
>   }
>   
> -struct drm_device *drm_kunit_device_init(struct kunit *test, u32 features, char *name)
> +struct drm_device *
> +drm_kunit_helper_alloc_drm_device(struct kunit *test,
> +				  u32 features, char *name)
>   {
>   	struct kunit_dev *kdev;
>   	struct drm_device *drm;
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.h b/drivers/gpu/drm/tests/drm_kunit_helpers.h
> index 20ab6eec4c89..e9870c7911fe 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.h
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.h
> @@ -6,6 +6,9 @@
>   struct drm_device;
>   struct kunit;
>   
> -struct drm_device *drm_kunit_device_init(struct kunit *test, u32 features, char *name);
> +struct drm_device *
> +drm_kunit_helper_alloc_drm_device(struct kunit *test,
> +				  u32 features,
> +				  char *name);
>   
>   #endif // DRM_KUNIT_HELPERS_H_
> 

Re: [PATCH 01/24] drm/tests: helpers: Rename the device init helper

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

Hi,

On Fri, Nov 25, 2022 at 11:10:02AM -0300, Maíra Canal wrote:
> On 11/23/22 12:25, Maxime Ripard wrote:
> > The name doesn't really fit the conventions for the other helpers in
> > DRM/KMS, so let's rename it to make it obvious that we allocate a new
> > DRM device.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> Although I believe using "drm_device" on the function name is a bit
> redundant (maybe drm_kunit_helper_alloc_dev or drm_kunit_helper_alloc_device
> would be cleaner),

Yeah, I don't quite like the name either, but we'll need to also
allocate a struct device in the next few patches so we need to make the
distinction between a struct drm_device and a struct device.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 10/24] drm/vc4: kms: Sort the CRTCs by output before assigning them

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> On the vc4 devices (and later), the blending is done by a single device
> called the HVS. The HVS has three FIFO that can operate in parallel, and
> route their output to 6 CRTCs and 7 encoders on the BCM2711.
> 
> Each of these CRTCs and encoders have some contraints on which FIFO they

constraints.

> can feed from, so we need some code to take all those constraints into
> account and assign FIFOs to CRTCs.
> 
> The problem can be simplified by assigning those FIFOs to CRTCs by
> ascending output index number. We had a comment mentioning it already,
> but we were never actually enforcing it.
> 
> It was working still in most situations because the probe order is
> roughly equivalent, except for the (optional, and fairly rarely used on
> the Pi4) VEC which was last in the probe order sequence, but one of the
> earliest device to assign.
> 
> This resulted in configurations that were rejected by our code but were
> still valid with a different assignment.
> 
> We can fix this by making sure we assign CRTCs to FIFOs by ordering
> them by ascending HVS output index.
> 
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

[...]

>  
> -	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> -		struct vc4_crtc_state *old_vc4_crtc_state =
> -			to_vc4_crtc_state(old_crtc_state);
> -		struct vc4_crtc_state *new_vc4_crtc_state =
> -			to_vc4_crtc_state(new_crtc_state);
> -		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> +	/*
> +	 * The problem we have to solve here is that we have up to 7
> +	 * encoders, connected to up to 6 CRTCs.
> +	 *
> +	 * Those CRTCs, depending on the instance, can be routed to 1, 2
> +	 * or 3 HVS FIFOs, and we need to set the change the muxing

This sentence sounds a little bit off to me. Did you mean:

"we need to set the muxing between" or "we need to change the muxing" ?

I'm not familiar with VC4 but the patch seems to do what the commit message
says, so the changes look good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: (subset) [PATCH 10/24] drm/vc4: kms: Sort the CRTCs by output before assigning them

[Maxime Ripard]

On Wed, 23 Nov 2022 16:25:52 +0100, Maxime Ripard wrote:
> On the vc4 devices (and later), the blending is done by a single device
> called the HVS. The HVS has three FIFO that can operate in parallel, and
> route their output to 6 CRTCs and 7 encoders on the BCM2711.
> 
> Each of these CRTCs and encoders have some contraints on which FIFO they
> can feed from, so we need some code to take all those constraints into
> account and assign FIFOs to CRTCs.
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: [PATCH 13/24] drm/vc4: kms: Constify the HVS old/new state helpers

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> The vc4_hvs_get_(old|new)_global_state functions don't modify the
> drm_atomic_state passed as an argument, so let's make it const.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: (subset) [PATCH 13/24] drm/vc4: kms: Constify the HVS old/new state helpers

[Maxime Ripard]

On Wed, 23 Nov 2022 16:25:55 +0100, Maxime Ripard wrote:
> The vc4_hvs_get_(old|new)_global_state functions don't modify the
> drm_atomic_state passed as an argument, so let's make it const.
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: [PATCH 14/24] drm/vc4: txp: Reorder the variable assignments

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> The current order of variable assignments is unneccessarily complex,
> let's make it simpler.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
Much easier to follow indeed.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: (subset) [PATCH 14/24] drm/vc4: txp: Reorder the variable assignments

[Maxime Ripard]

On Wed, 23 Nov 2022 16:25:56 +0100, Maxime Ripard wrote:
> The current order of variable assignments is unneccessarily complex,
> let's make it simpler.
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: [PATCH 15/24] drm/vc4: Add TXP encoder type

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> The TXP is integrated as a separate CRTC/Encoder/Connector combo, but
> for some reason doesn't rely on the vc4_encoder type and it's associated
> type.
> 
> Let's create a type to make it consistent with the other encoders.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: (subset) [PATCH 15/24] drm/vc4: Add TXP encoder type

[Maxime Ripard]

On Wed, 23 Nov 2022 16:25:57 +0100, Maxime Ripard wrote:
> The TXP is integrated as a separate CRTC/Encoder/Connector combo, but
> for some reason doesn't rely on the vc4_encoder type and it's associated
> type.
> 
> Let's create a type to make it consistent with the other encoders.
> 
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: [PATCH 16/24] drm/vc4: txp: Initialise the CRTC before the encoder and connector

[Javier Martinez Canillas]

On 11/23/22 16:25, Maxime Ripard wrote:
> It makes more sense to register the CRTC before the encoder and
> connectors, so let's move our call around.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: (subset) [PATCH 16/24] drm/vc4: txp: Initialise the CRTC before the encoder and connector

[Maxime Ripard]

On Wed, 23 Nov 2022 16:25:58 +0100, Maxime Ripard wrote:
> It makes more sense to register the CRTC before the encoder and
> connectors, so let's move our call around.
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: [PATCH 20/24] drm/vc4: crtc: Provide a CRTC name

[Javier Martinez Canillas]

On 11/23/22 16:26, Maxime Ripard wrote:
> It's fairly hard to figure out the instance of the CRTC affected by an
> atomic change using the default name.
> 
> Since we can provide our own to the CRTC initialization functions, let's
> do so to make the debugging sessions easier.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 10 +++++++++-
>  drivers/gpu/drm/vc4/vc4_drv.h  |  2 ++
>  drivers/gpu/drm/vc4/vc4_txp.c  |  1 +
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 8bc30ad0904b..59e473059fa2 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -1118,6 +1118,7 @@ static const struct drm_crtc_helper_funcs vc4_crtc_helper_funcs = {
>  
>  static const struct vc4_pv_data bcm2835_pv0_data = {
>  	.base = {
> +		.name = "pixelvalve-0",

I wonder if would make sense to add the SoC name too, but either way:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: (subset) [PATCH 20/24] drm/vc4: crtc: Provide a CRTC name

[Maxime Ripard]

On Wed, 23 Nov 2022 16:26:02 +0100, Maxime Ripard wrote:
> It's fairly hard to figure out the instance of the CRTC affected by an
> atomic change using the default name.
> 
> Since we can provide our own to the CRTC initialization functions, let's
> do so to make the debugging sessions easier.
> 
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime