Message-ID: <b05831de-c67b-4ba9-8808-f049d97b3654@bootlin.com>
Date: Wed, 12 Feb 2025 15:10:49 +0100
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: José Expósito <jose.exposito89@...il.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



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@...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.

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?

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.

> 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

Powered by blists - more mailing lists