Message-ID: <Z6sq0h0lKxjmBcxk@fedora>
Date: Tue, 11 Feb 2025 11:47:46 +0100
From: José Expósito <jose.exposito89@...il.com>
To: Louis Chauvet <louis.chauvet@...tlin.com>
Cc: hamohammed.sa@...il.com, simona@...ll.ch, melissa.srw@...il.com,
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
	tzimmermann@...e.de, airlied@...il.com,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 09/13] drm/vkms: Allow to attach planes and CRTCs

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@...tlin.com>
> > Signed-off-by: José Expósito <jose.exposito89@...il.com>
> 
> Co-developped-by: Louis Chauvet <louis.chauvet@...tlin.com>
> Signed-off-by: Louis Chauvet <louis.chauvet@...tlin.com>
> Signed-off-by: José Expósito <jose.exposito89@...il.com>
> 
> [...]
> 
> > 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.

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.

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;
> > +		}
> 
> [...]

Powered by blists - more mailing lists