Re: [PATCH 08/13] drm/vkms: Allow to configure multiple CRTCs
From: Louis Chauvet
Date: Wed Feb 12 2025 - 09:12:56 EST
Le 11/02/2025 à 11:44, José Expósito a écrit :
On Thu, Jan 30, 2025 at 02:48:20PM +0100, Louis Chauvet wrote:
On 29/01/25 - 12:00, José Expósito wrote:
Add a list of CRTCs to vkms_config and helper functions to add and
remove as many CRTCs as wanted.
For backwards compatibility, add one CRTC to the default configuration.
A future patch will allow to attach planes and CRTCs, but for the
moment there are no changes in the way the output is configured.
Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
Signed-off-by: José Expósito <jose.exposito89@xxxxxxxxx>
Co-developped-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
Signed-off-by: José Expósito <jose.exposito89@xxxxxxxxx>
[...]
diff --git a/drivers/gpu/drm/vkms/tests/vkms_config_test.c b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
[...]
+static void vkms_config_test_valid_crtc_number(struct kunit *test)
+{
+ struct vkms_config *config;
+ struct vkms_config_crtc *crtc_cfg;
+ int n;
+
+ config = vkms_config_default_create(false, false, false);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, config);
+
+ /* Invalid: No CRTCs */
+ crtc_cfg = list_first_entry(&config->crtcs, typeof(*crtc_cfg), link);
+ vkms_config_destroy_crtc(config, crtc_cfg);
+ KUNIT_EXPECT_FALSE(test, vkms_config_is_valid(config));
+
+ /* Invalid: Too many CRTCs */
+ for (n = 0; n <= 32; n++)
+ vkms_config_add_crtc(config);
+
+ KUNIT_EXPECT_FALSE(test, vkms_config_is_valid(config));
+
+ vkms_config_destroy(config);
+}
Same as before, can you rename the fonction to
vkms_config_test_invalid_crtc_number
[...]
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
[...]
+struct vkms_config_crtc **vkms_config_get_crtcs(const struct vkms_config *config,
+ size_t *out_length)
+{
+ struct vkms_config_crtc **array;
+ struct vkms_config_crtc *crtc_cfg;
+ size_t length;
+ int n = 0;
+
+ length = list_count_nodes((struct list_head *)&config->crtcs);
+ if (length == 0) {
+ *out_length = length;
+ return NULL;
+ }
+
+ array = kmalloc_array(length, sizeof(*array), GFP_KERNEL);
+ if (!array)
+ return ERR_PTR(-ENOMEM);
+
+ list_for_each_entry(crtc_cfg, &config->crtcs, link) {
+ array[n] = crtc_cfg;
+ n++;
+ }
+
+ *out_length = length;
+ return array;
+}
Same as before, can't we use an iterator?
[...]
+static bool valid_crtc_number(struct vkms_config *config)
+{
+ size_t n_crtcs;
+
+ n_crtcs = list_count_nodes(&config->crtcs);
+ if (n_crtcs <= 0 || n_crtcs >= 32) {
+ pr_err("The number of CRTCs must be between 1 and 31\n");
I agree we need some logs, but I think pr_err is too agressive (i.e may
be considered as an error by some test tools).
I think we should at least:
- lower to warn/notice/info
- use drm variants of the macro
+ return false;
+ }
+
+ return true;
+}
+
[...]
+struct vkms_config_crtc *vkms_config_add_crtc(struct vkms_config *config)
+{
+ struct vkms_config_crtc *crtc_cfg;
+
+ crtc_cfg = kzalloc(sizeof(*crtc_cfg), GFP_KERNEL);
+ if (!crtc_cfg)
+ return ERR_PTR(-ENOMEM);
+
+ vkms_config_crtc_set_writeback(crtc_cfg, false);
+
+ list_add_tail(&crtc_cfg->link, &config->crtcs);
+
+ return crtc_cfg;
+}
+
+void vkms_config_destroy_crtc(struct vkms_config *config,
+ struct vkms_config_crtc *crtc_cfg)
+{
+ list_del(&crtc_cfg->link);
+ kfree(crtc_cfg);
+}
Same as before, the pair add/destroy seems strange.
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -181,7 +181,8 @@ static int vkms_create(struct vkms_config *config)
goto out_devres;
}
- ret = drm_vblank_init(&vkms_device->drm, 1);
+ ret = drm_vblank_init(&vkms_device->drm,
+ vkms_config_get_num_crtcs(config));
At this point we only create one crtc, can you move this change in the
commit where you create multiple crtc?
Since the code to add more than one CRTCs is unused but technically present, I
think that this is the right patch to use this function.
I don't totally agree: you can create multiple vkms_config_crtc, but the
code only creates one drm_crtc.
For me it is more logical to keep 1 here, as the rest of the code only
creates one CRTC. With the next patch, the code will create more than
one CRTC, so it makes sense to use vkms_config_get_num_crtcs.
It is not a strong blocking point, but if a v3 is needed, could you
change it?
Anyway, at the moment it'll always return 1, so it is a no-op.
The current user is only default_config, so yes I agree, it is always 1.
I didn't change it in v2, let me know if it works for you.
Thanks,
Jose
if (ret) {
DRM_ERROR("Failed to vblank\n");
goto out_devres;
--
2.48.1
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com