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 <[email protected]> > Signed-off-by: José Expósito <[email protected]>
Co-developped-by: Louis Chauvet <[email protected]> Signed-off-by: Louis Chauvet <[email protected]> Signed-off-by: José Expósito <[email protected]> [...] > 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; > + } [...]
