November 2022 - Linux-kselftest-mirror

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

by 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(a)cerno.tech> > --- Reviewed-by: Javier Martinez Canillas <javierm(a)redhat.com> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat

2 years

1
0
0 0

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

by 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(a)cerno.tech> > --- Reviewed-by: Javier Martinez Canillas <javierm(a)redhat.com> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat

2 years

1
0
0 0

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

by 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(a)cerno.tech> > --- Reviewed-by: Javier Martinez Canillas <javierm(a)redhat.com> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat

2 years

1
0
0 0

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

by 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(a)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(a)redhat.com> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat

2 years

1
0
0 0

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

by 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(a)cerno.tech> > --- Reviewed-by: Javier Martinez Canillas <javierm(a)redhat.com> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat

2 years

1
0
0 0

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

by 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(a)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(a)redhat.com> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat

2 years

1
0
0 0

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

by 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(a)cerno.tech> > --- Reviewed-by: Javier Martinez Canillas <javierm(a)redhat.com> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat

2 years

1
0
0 0

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

by 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(a)cerno.tech> > --- Reviewed-by: Javier Martinez Canillas <javierm(a)redhat.com> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat

2 years

1
0
0 0

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

by 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(a)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

2 years

1
0
0 0

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

by 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(a)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(a)redhat.com> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat

2 years

1
0
0 0

2024

2023

2022

2021

2020

2019

2018

2017

Linux-kselftest-mirror November 2022