Re: [PATCH 11/13] drm/vkms: Allow to attach encoders and CRTCs

Louis Chauvet
2025-01-30T13:48:26Z

On 29/01/25 - 12:00, José Expósito wrote:
> Add a list of possible CRTCs to the encoder configuration and helpers to
> attach and detach them.
>
> Now that the default configuration has its encoder and CRTC correctly
> attached, configure the output following the configuration.
>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>

Co-developped-by: Louis Chauvet <louis.chauvet@bootlin.com>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>

[...]

> diff --git a/drivers/gpu/drm/vkms/tests/vkms_config_test.c b/drivers/gpu/drm/vkms/tests/vkms_config_test.c

[...]

> +struct vkms_config_crtc **
> +vkms_config_encoder_get_possible_crtcs(struct vkms_config_encoder *encoder_cfg,
> + size_t *out_length)
> +{
> + struct vkms_config_crtc **array;
> + struct vkms_config_crtc *possible_crtc;
> + unsigned long idx;
> + size_t length = 0;
> + int n = 0;
> +
> + xa_for_each(&encoder_cfg->possible_crtcs, idx, possible_crtc)
> + length++;
> +
> + if (length == 0) {
> + *out_length = 0;
> + return NULL;
> + }
> +
> + array = kmalloc_array(length, sizeof(*array), GFP_KERNEL);
> + if (!array)
> + return ERR_PTR(-ENOMEM);
> +
> + xa_for_each(&encoder_cfg->possible_crtcs, idx, possible_crtc) {
> + array[n] = possible_crtc;
> + n++;
> + }
> +
> + *out_length = length;
> + return array;
> +}

Same as before, can we use an iterator?

> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c

[...]

> @@ -98,22 +143,8 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> goto err_free;
> }
>
> - encoder = drmm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
> - if (!encoder) {
> - DRM_ERROR("Failed to allocate encoder\n");
> - ret = -ENOMEM;
> - goto err_free;
> - }
> - ret = drmm_encoder_init(dev, encoder, NULL,
> - DRM_MODE_ENCODER_VIRTUAL, NULL);
> - if (ret) {
> - DRM_ERROR("Failed to init encoder\n");
> - goto err_free;
> - }
> - encoder->possible_crtcs = drm_crtc_mask(&crtc_cfgs[0]->crtc->crtc);
> -
> /* Attach the encoder and the connector */
> - ret = drm_connector_attach_encoder(&connector->base, encoder);
> + ret = drm_connector_attach_encoder(&connector->base, encoder_cfgs[0]->encoder);

Why do you only attach the first encoder? Can't we attach all of them? …