On 29/01/25 - 12:00, José Expósito wrote:
> Add a list of possible CRTCs to the plane configuration and helpers to
> attach, detach and get the primary and cursor planes attached to a CRTC.
>
> Now that the default configuration has its planes and CRTC correctly
> attached, configure the output following the configuration.
>
> 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/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
[...]
> -static bool valid_plane_type(struct vkms_config *config)
> +static bool valid_plane_type(struct vkms_config *config,
> + struct vkms_config_crtc *crtc_cfg)
What do you think about renaming it to "valid_planes_for_crtc" to reflect
the fact you tests if a CRTC is attached to a valid combination of planes?
> {
> struct vkms_config_plane *plane_cfg;
> bool has_primary_plane = false;
> bool has_cursor_plane = false;
>
> list_for_each_entry(plane_cfg, &config->planes, link) {
> + struct vkms_config_crtc *possible_crtc;
> + unsigned long idx = 0;
> enum drm_plane_type type;
>
> type = vkms_config_plane_get_type(plane_cfg);
>
> - if (type == DRM_PLANE_TYPE_PRIMARY) {
> - if (has_primary_plane) {
> - pr_err("Multiple primary planes\n");
> - return false;
> - }
> + xa_for_each(&plane_cfg->possible_crtcs, idx, possible_crtc) {
> + if (possible_crtc != crtc_cfg)
> + continue;
>
> - has_primary_plane = true;
> - } else if (type == DRM_PLANE_TYPE_CURSOR) {
> - if (has_cursor_plane) {
> - pr_err("Multiple cursor planes\n");
> - return false;
> - }
> + if (type == DRM_PLANE_TYPE_PRIMARY) {
> + if (has_primary_plane) {
> + pr_err("Multiple primary planes\n");
> + return false;
> + }
>
> - has_cursor_plane = true;
> + has_primary_plane = true;
> + } else if (type == DRM_PLANE_TYPE_CURSOR) {
> + if (has_cursor_plane) {
> + pr_err("Multiple cursor planes\n");
> + return false;
> + }
> +
> + has_cursor_plane = true;
> + }
> }
> }
[...]
> +int __must_check vkms_config_plane_attach_crtc(struct vkms_config_plane *plane_cfg,
> + struct vkms_config_crtc *crtc_cfg)
> +{
> + struct vkms_config_crtc *possible_crtc;
> + unsigned long idx = 0;
> + u32 crtc_idx = 0;
> +
> + xa_for_each(&plane_cfg->possible_crtcs, idx, possible_crtc) {
> + if (possible_crtc == crtc_cfg)
> + return -EINVAL;
Is it really an error? After this call, we expect plane and crtc to be
attached, so if the plane is already attached, I don't see any issue.
> + }
> +
> + return xa_alloc(&plane_cfg->possible_crtcs, &crtc_idx, crtc_cfg,
> + xa_limit_32b, GFP_KERNEL);
> +}
> +
[...]
> +struct vkms_config_crtc **vkms_config_plane_get_possible_crtcs(struct vkms_config_plane *plane_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(&plane_cfg->possible_crtcs, idx, possible_crtc)
> + length++;
> +
> + if (length == 0) {
> + *out_length = length;
> + return NULL;
> + }
> +
> + array = kmalloc_array(length, sizeof(*array), GFP_KERNEL);
> + if (!array)
> + return ERR_PTR(-ENOMEM);
> +
> + xa_for_each(&plane_cfg->possible_crtcs, idx, possible_crtc) {
> + array[n] = possible_crtc;
> + n++;
> + }
> +
> + *out_length = length;
> + return array;
> +}
Same as before, can we use an iterator?
> +static struct vkms_config_plane *vkms_config_crtc_get_plane(const struct vkms_config *config,
> + struct vkms_config_crtc *crtc_cfg,
> + enum drm_plane_type type)
Even if this is a private function, can we add a comment explaning that
the returned value is only one of the available planes of this type?
/**
* vkms_config_crtc_get_plane() - Get the first attached plane
* found of a specific type
* @config: configuration containing the crtc and the planes
* @crtc_cfg: Only find planes attached to this CRTC
* @type: Plane type to search
*
* Returns:
* The first plane found attached to @crtc_cfg with the type
* @type.
*/
> +{
> + struct vkms_config_plane *plane_cfg;
> + struct vkms_config_crtc *possible_crtc;
> + enum drm_plane_type current_type;
> + unsigned long idx;
> +
> + list_for_each_entry(plane_cfg, &config->planes, link) {
> + current_type = vkms_config_plane_get_type(plane_cfg);
> +
> + xa_for_each(&plane_cfg->possible_crtcs, idx, possible_crtc) {
> + if (possible_crtc == crtc_cfg && current_type == type)
> + return plane_cfg;
> + }
> + }
> +
> + return NULL;
> +}
[...]
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
[...]
> +/**
> + * vkms_config_crtc_primary_plane() - Return the primary plane for a CRTC
> + * @config: Configuration containing the CRTC
> + * @crtc_config: Target CRTC
> + *
> + * Returns:
> + * The primary plane or NULL if none is assigned yet.
> + */
Same as above, can you speficy that it is one of the primary plane?
> +struct vkms_config_plane *vkms_config_crtc_primary_plane(const struct vkms_config *config,
> + struct vkms_config_crtc *crtc_cfg);
> +
> +/**
> + * vkms_config_crtc_cursor_plane() - Return the cursor plane for a CRTC
Ditto
[...]
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
[...]
> @@ -35,19 +41,54 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> ret = PTR_ERR(plane_cfg->plane);
> goto err_free;
> }
> + }
> +
> + for (n = 0; n < n_crtcs; n++) {
> + struct vkms_config_crtc *crtc_cfg;
> + struct vkms_config_plane *primary, *cursor;
> +
> + crtc_cfg = crtc_cfgs[n];
> + primary = vkms_config_crtc_primary_plane(vkmsdev->config, crtc_cfg);
> + cursor = vkms_config_crtc_cursor_plane(vkmsdev->config, crtc_cfg);
Linked with a previous comment: here we have no garantee that primary is a
valid pointer, can we check it or call vkms_config_is_valid to ensure it?
> + crtc_cfg->crtc = vkms_crtc_init(dev, &primary->plane->base,
> + cursor ? &cursor->plane->base : NULL);
> + if (IS_ERR(crtc_cfg->crtc)) {
> + DRM_ERROR("Failed to allocate CRTC\n");
> + ret = PTR_ERR(crtc_cfg->crtc);
> + goto err_free;
> + }
[...]
