On 18/02/25 - 18:07, José Expósito wrote:
> When a plane is created, add a `type` file to allow to set the type:
>
> - 0 overlay
> - 1 primary
> - 2 cursor
>
> Co-developed-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
> Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
> Signed-off-by: José Expósito <jose.exposito89@xxxxxxxxx>
> ---
> Documentation/gpu/vkms.rst | 4 +++
> drivers/gpu/drm/vkms/vkms_configfs.c | 51 ++++++++++++++++++++++++++++
> 2 files changed, 55 insertions(+)
>
> diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> index bf23d0da33fe..d95f228de05b 100644
> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst
> @@ -84,6 +84,10 @@ Start by creating one or more planes::
>
> sudo mkdir /config/vkms/my-vkms/planes/plane0
>
> +Planes have 1 configurable attribute:
> +
> +- type: Plane type: 0 overlay, 1 primary, 2 cursor
> +
Maybe we can say:
- type: Plane type: 0 overlay, 1 primary, 2 cursor (same values as those
exposed by the "type" property of a plane)
> Once you are done configuring the VKMS instance, enable it::
>
> echo "1" | sudo tee /config/vkms/my-vkms/enabled
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index dd9dfe51eb3a..093735f47858 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -54,6 +54,56 @@ struct vkms_configfs_plane {
> #define plane_item_to_vkms_configfs_plane(item) \
> container_of(to_config_group((item)), struct vkms_configfs_plane, group)
>
> +static ssize_t plane_type_show(struct config_item *item, char *page)
> +{
> + struct vkms_configfs_plane *plane;
> + enum drm_plane_type type;
> +
> + plane = plane_item_to_vkms_configfs_plane(item);
> +
> + mutex_lock(&plane->dev->lock);
> + type = vkms_config_plane_get_type(plane->config);
> + mutex_unlock(&plane->dev->lock);
I think we should consider using scoped_gard instead of mutex_lock/unlock.
This may avoid issues (in many other functions there are early returns,
where a scoped_guard may help).
I am not against "pure" mutex_lock/unlock (I am not 100% sure that
scoped_guard really improve the readability), but I think we should be
consistent: if we want to profit from scoped_guard in some functions, we
should use it everywhere in this file.
(all my R-by are valid with or without scoped_guard)
> +
> + return sprintf(page, "%u", type);
> +}
> +
> +static ssize_t plane_type_store(struct config_item *item, const char *page,
> + size_t count)
> +{
> + struct vkms_configfs_plane *plane;
> + enum drm_plane_type type;
> +
> + plane = plane_item_to_vkms_configfs_plane(item);
> +
> + if (kstrtouint(page, 10, &type))
> + return -EINVAL;
> +
> + if (type != DRM_PLANE_TYPE_OVERLAY && type != DRM_PLANE_TYPE_PRIMARY &&
> + type != DRM_PLANE_TYPE_CURSOR)
> + return -EINVAL;
> +
> + mutex_lock(&plane->dev->lock);
> +
> + if (plane->dev->enabled) {
> + mutex_unlock(&plane->dev->lock);
> + return -EPERM;
I just tough about it, but is -EPERM also the return value when you don't
have the right to write on the file? What do you think about -EBUSY? It
also describe a bit better the situation (Device or resource busy vs
Operation not permitted).
Thanks,
Louis Chauvet
> + }
> +
> + vkms_config_plane_set_type(plane->config, type);
> +
> + mutex_unlock(&plane->dev->lock);
> +
> + return (ssize_t)count;
> +}
> +
> +CONFIGFS_ATTR(plane_, type);
> +
> +static struct configfs_attribute *plane_item_attrs[] = {
> + &plane_attr_type,
> + NULL,
> +};
> +
> static void plane_release(struct config_item *item)
> {
> struct vkms_configfs_plane *plane;
> @@ -73,6 +123,7 @@ static struct configfs_item_operations plane_item_operations = {
> };
>
> static const struct config_item_type plane_item_type = {
> + .ct_attrs = plane_item_attrs,
> .ct_item_ops = &plane_item_operations,
> .ct_owner = THIS_MODULE,
> };
> --
> 2.48.1
>
