Re: [PATCH 09/13] drm/vkms: Allow to attach planes and CRTCs

["José Expósito" <jose.exposito89@gmail.com>]

On Wed, Feb 12, 2025 at 03:10:49PM +0100, Louis Chauvet wrote:
> 
> 
> Le 11/02/2025 à 11:47, José Expósito a écrit :
> > On Thu, Jan 30, 2025 at 02:48:21PM +0100, Louis Chauvet wrote:
> > > 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@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>
> > > 
> > > [...]
> > > 
> > > > +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.
> > 
> > In my opinion, this could be either handled as an error or not. I think that
> > there are arguments for both approaches but, for our use case, I think that it
> > is better to return an error.
> > 
> > Since the main (and for the moment only) user of this function will be configfs,
> > it is very convenient to return an error to avoid creating 2 links between
> > plane <-> crtc.
> > 
> > If we allow to create multiple links, and the user deletes one of them, the
> > items would be still linked, which is a bit unexpected.
> > 
> > The same applies to the other vkms_config_*_attach_* functions.
> 
> I see your reasoning, I did not think about this issue.
> We can keep the error, but can we use `EEXIST` to reflect better the status?

Very good point, I'll change it to EEXIST in v3.
 
> And I just think about it, maybe we can add here the check "is the crtc part
> of the same config as the plane" (and return EINVAL if not)? It will remove
> the need to do the check in configFS + avoid errors for future users of
> vkms_config.

Another excellent point. Since we can not use the vkms_config_plane.plane and
vkms_config_crtc.crtc pointers to check if they belong to the same device, the
only solution I see is adding a pointer to the parent vkms_config to every
structure (vkms_config_plane, vkms_config_crtc, etc).

In this way we can do something like:

  if (plane_cfg->config != crtc_cfg->config)
          return -EINVAL;

I tried with container_of(), but I don't think it'll work with the current data
structure.

Unless you can think of a better solution, I'll implement this in v3.

Thanks for the great comments,
Jose

> > For these reasons, I didn't change it in v2, let me know your opinion.
> > Jose
> > 
> > > > +	}
> > > > +
> > > > +	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;
> > > > +		}
> > > 
> > > [...]
> 
> -- 
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>