diff mbox series

[RFC,13/17] drm/vkms: Allow to configure multiple encoders via configfs

Message ID 20240813105134.17439-14-jose.exposito89@gmail.com (mailing list archive)
State New, archived
Headers show
Series VKMS: Add configfs support | expand

Commit Message

José Expósito Aug. 13, 2024, 10:44 a.m. UTC 
Create a default subgroup at /config/vkms/encoders to allow to create as
many encoders as required. When the encoder is created the
possible_crtcs subgroup is created allowing to link encoders and CRTCs.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 Documentation/gpu/vkms.rst           |  14 ++-
 drivers/gpu/drm/vkms/vkms_config.h   |   3 +
 drivers/gpu/drm/vkms/vkms_configfs.c | 127 +++++++++++++++++++++++++--
 3 files changed, 136 insertions(+), 8 deletions(-)

Comments

Louis Chauvet Aug. 13, 2024, 5:58 p.m. UTC | #1 
Le 13/08/24 - 12:44, José Expósito a écrit :
> Create a default subgroup at /config/vkms/encoders to allow to create as
> many encoders as required. When the encoder is created the
> possible_crtcs subgroup is created allowing to link encoders and CRTCs.

I did not test, but the issue about index for crtc are maybe the same for 
encoders.
 
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---
>  Documentation/gpu/vkms.rst           |  14 ++-
>  drivers/gpu/drm/vkms/vkms_config.h   |   3 +
>  drivers/gpu/drm/vkms/vkms_configfs.c | 127 +++++++++++++++++++++++++--
>  3 files changed, 136 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> index 0886349ad4a0..c69be063d3b4 100644
> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst
> @@ -76,7 +76,8 @@ And directories are created for each configurable item of the display pipeline::
>    tree /config/vkms/my-vkms
>      /config/vkms/my-vkms
>      ├── crtcs
> -    └── enabled
> +    ├── enabled
> +    └── encoders
>  
>  To add items to the display pipeline, create one or more directories under the
>  available paths.
> @@ -90,6 +91,15 @@ CRTCs have 2 configurable attributes:
>  - cursor: Enable or disable cursor plane support
>  - writeback: Enable or disable writeback connector support
>  
> +Continue by creating one or more encoders::
> +
> +  sudo mkdir /config/vkms/my-vkms/encoders/encoder0
> +
> +Encoders can be linked to CRTCs by creating a symbolic link under
> +``possible_crtcs``::
> +
> +  sudo ln -s /config/vkms/my-vkms/crtcs/crtc0 /config/vkms/my-vkms/encoders/encoder0/possible_crtcs
> +
>  Once you are done configuring the VKMS instance, enable it::
>  
>    echo "1" | sudo tee /config/vkms/my-vkms/enabled
> @@ -100,6 +110,8 @@ Finally, you can remove the VKMS instance disabling it::
>  
>  Or removing the top level directory and its subdirectories::
>  
> +  sudo rm /config/vkms/my-vkms/encoders/*/possible_crtcs/*
> +  sudo rmdir /config/vkms/my-vkms/encoders/*
>    sudo rmdir /config/vkms/my-vkms/crtcs/*
>    sudo rmdir /config/vkms/my-vkms

Same comment as previously, I think we should forbid this way to delete an 
instance.

> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index f96a0456a3d7..f9423533a7f3 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -29,6 +29,9 @@ struct vkms_config_encoder {
>  	struct list_head list;
>  	unsigned int index;
>  	uint32_t possible_crtcs;
> +	/* only used if created from configfs */
> +	struct config_group encoder_group;
> +	struct config_group possible_crtcs_group;

Again, I don't like mixing configfs and vkms_config. Can we keep both 
stuff splitted?

>  };
>  
>  struct vkms_config_connector {
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index 04278a39cd3c..c0b62fb93dc0 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -19,6 +19,8 @@ static bool is_configfs_registered;
>   * Initialized when a new directory is created under "/config/vkms/"
>   * @crtcs_group: Default subgroup of @device_group at "/config/vkms/crtcs".
>   * Each of its items represent a CRTC
> + * @encoders_group: Default subgroup of @device_group at
> + * "/config/vkms/encoders". Each of its items represent a encoder
>   * @lock: Lock used to project concurrent access to the configuration attributes
>   * @enabled: Protected by @lock. The device is created or destroyed when this
>   * option changes
> @@ -27,6 +29,7 @@ struct vkms_configfs {
>  	struct vkms_config *vkms_config;
>  	struct config_group device_group;
>  	struct config_group crtcs_group;
> +	struct config_group encoders_group;
>  
>  	/* protected by @lock */
>  	struct mutex lock;
> @@ -45,6 +48,15 @@ struct vkms_configfs {
>  #define crtcs_item_to_vkms_config_crtc(item) \
>  	container_of(to_config_group(item), struct vkms_config_crtc, crtc_group)
>  
> +#define encoders_group_to_vkms_configfs(group) \
> +	container_of(group, struct vkms_configfs, encoders_group)
> +
> +#define encoders_item_to_vkms_config_encoder(item) \
> +	container_of(to_config_group(item), struct vkms_config_encoder, encoder_group)
> +
> +#define encoder_possible_crtcs_item_to_vkms_config_encoder(item) \
> +	container_of(to_config_group(item), struct vkms_config_encoder, possible_crtcs_group)
> +
>  static ssize_t crtc_cursor_show(struct config_item *item, char *page)
>  {
>  	struct vkms_config_crtc *crtc_cfg = crtcs_item_to_vkms_config_crtc(item);
> @@ -171,6 +183,110 @@ static struct config_item_type crtcs_group_type = {
>  	.ct_owner = THIS_MODULE,
>  };
>  
> +static int encoder_possible_crtcs_allow_link(struct config_item *src,
> +					     struct config_item *target)
> +{
> +	struct vkms_config_encoder *encoder_cfg;
> +	struct vkms_config_crtc *crtc_cfg;
> +
> +	if (target->ci_type != &crtc_group_type)
> +		return -EINVAL;

I also need to check on my implementation, be we also need to check if the 
target is in the correct device, not only the correct type :)

> +
> +	encoder_cfg = encoder_possible_crtcs_item_to_vkms_config_encoder(src);
> +	crtc_cfg = crtcs_item_to_vkms_config_crtc(target);
> +
> +	if (encoder_cfg->possible_crtcs & BIT(crtc_cfg->index))
> +		return -EINVAL;
> +
> +	encoder_cfg->possible_crtcs |= BIT(crtc_cfg->index);
> +
> +	return 0;
> +}
> +
> +static void encoder_possible_crtcs_drop_link(struct config_item *src,
> +					     struct config_item *target)
> +{
> +	struct vkms_config_encoder *encoder_cfg;
> +	struct vkms_config_crtc *crtc_cfg;
> +
> +	encoder_cfg = encoder_possible_crtcs_item_to_vkms_config_encoder(src);
> +	crtc_cfg = crtcs_item_to_vkms_config_crtc(target);
> +
> +	encoder_cfg->possible_crtcs &= ~BIT(crtc_cfg->index);
> +}
> +
> +static struct configfs_item_operations encoder_possible_crtcs_item_ops = {
> +	.allow_link = &encoder_possible_crtcs_allow_link,
> +	.drop_link = &encoder_possible_crtcs_drop_link,
> +};
> +
> +static struct config_item_type encoder_possible_crtcs_group_type = {
> +	.ct_item_ops = &encoder_possible_crtcs_item_ops,
> +	.ct_owner = THIS_MODULE,
> +};
> +
> +static const struct config_item_type encoder_group_type = {
> +	.ct_owner = THIS_MODULE,
> +};
> +
> +static struct config_group *make_encoders_group(struct config_group *group,
> +						const char *name)
> +{
> +	struct vkms_configfs *configfs = encoders_group_to_vkms_configfs(group);
> +	struct vkms_config_encoder *encoder_cfg;
> +	int ret;
> +
> +	mutex_lock(&configfs->lock);
> +
> +	if (configfs->enabled) {
> +		ret = -EINVAL;
> +		goto err_unlock;
> +	}
> +
> +	encoder_cfg = vkms_config_add_encoder(configfs->vkms_config, 0);
> +	if (IS_ERR(encoder_cfg)) {
> +		ret = PTR_ERR(encoder_cfg);
> +		goto err_unlock;
> +	}
> +
> +	config_group_init_type_name(&encoder_cfg->encoder_group, name,
> +				    &encoder_group_type);
> +
> +	config_group_init_type_name(&encoder_cfg->possible_crtcs_group,
> +				    "possible_crtcs",
> +				    &encoder_possible_crtcs_group_type);
> +	configfs_add_default_group(&encoder_cfg->possible_crtcs_group,
> +				   &encoder_cfg->encoder_group);
> +
> +	mutex_unlock(&configfs->lock);
> +
> +	return &encoder_cfg->encoder_group;
> +
> +err_unlock:
> +	mutex_unlock(&configfs->lock);
> +	return ERR_PTR(ret);
> +}
> +
> +static void drop_encoders_group(struct config_group *group,
> +				struct config_item *item)
> +{
> +	struct vkms_configfs *configfs = encoders_group_to_vkms_configfs(group);
> +	struct vkms_config_encoder *encoder_cfg =
> +		encoders_item_to_vkms_config_encoder(item);
> +
> +	vkms_config_destroy_encoder(configfs->vkms_config, encoder_cfg);
> +}

Same, I think there should not be any free in drop.

> +
> +static struct configfs_group_operations encoders_group_ops = {
> +	.make_group = &make_encoders_group,
> +	.drop_item = &drop_encoders_group,
> +};
> +
> +static struct config_item_type encoders_group_type = {
> +	.ct_group_ops = &encoders_group_ops,
> +	.ct_owner = THIS_MODULE,
> +};
> +
>  static ssize_t device_enabled_show(struct config_item *item, char *page)
>  {
>  	struct vkms_configfs *configfs = config_item_to_vkms_configfs(item);
> @@ -225,7 +341,6 @@ static struct config_group *make_device_group(struct config_group *group,
>  					      const char *name)
>  {
>  	struct vkms_configfs *configfs;
> -	struct vkms_config_encoder *encoder_cfg = NULL;
>  	struct vkms_config_connector *connector_cfg = NULL;
>  	char *config_name;
>  	int ret;
> @@ -252,11 +367,10 @@ static struct config_group *make_device_group(struct config_group *group,
>  	configfs_add_default_group(&configfs->crtcs_group,
>  				   &configfs->device_group);
>  
> -	encoder_cfg = vkms_config_add_encoder(configfs->vkms_config, BIT(0));
> -	if (IS_ERR(encoder_cfg)) {
> -		ret = PTR_ERR(encoder_cfg);
> -		goto err_kfree;
> -	}
> +	config_group_init_type_name(&configfs->encoders_group, "encoders",
> +				    &encoders_group_type);
> +	configfs_add_default_group(&configfs->encoders_group,
> +				   &configfs->device_group);
>  
>  	connector_cfg = vkms_config_add_connector(configfs->vkms_config, BIT(0),
>  						  connector_status_connected);
> @@ -269,7 +383,6 @@ static struct config_group *make_device_group(struct config_group *group,
>  
>  err_kfree:
>  	kfree(configfs);
> -	kfree(encoder_cfg);
>  	kfree(connector_cfg);
>  	return ERR_PTR(ret);
>  }
> -- 
> 2.46.0
>
diff mbox series

Patch

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 0886349ad4a0..c69be063d3b4 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -76,7 +76,8 @@  And directories are created for each configurable item of the display pipeline::
   tree /config/vkms/my-vkms
     /config/vkms/my-vkms
     ├── crtcs
-    └── enabled
+    ├── enabled
+    └── encoders
 
 To add items to the display pipeline, create one or more directories under the
 available paths.
@@ -90,6 +91,15 @@  CRTCs have 2 configurable attributes:
 - cursor: Enable or disable cursor plane support
 - writeback: Enable or disable writeback connector support
 
+Continue by creating one or more encoders::
+
+  sudo mkdir /config/vkms/my-vkms/encoders/encoder0
+
+Encoders can be linked to CRTCs by creating a symbolic link under
+``possible_crtcs``::
+
+  sudo ln -s /config/vkms/my-vkms/crtcs/crtc0 /config/vkms/my-vkms/encoders/encoder0/possible_crtcs
+
 Once you are done configuring the VKMS instance, enable it::
 
   echo "1" | sudo tee /config/vkms/my-vkms/enabled
@@ -100,6 +110,8 @@  Finally, you can remove the VKMS instance disabling it::
 
 Or removing the top level directory and its subdirectories::
 
+  sudo rm /config/vkms/my-vkms/encoders/*/possible_crtcs/*
+  sudo rmdir /config/vkms/my-vkms/encoders/*
   sudo rmdir /config/vkms/my-vkms/crtcs/*
   sudo rmdir /config/vkms/my-vkms
 
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index f96a0456a3d7..f9423533a7f3 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -29,6 +29,9 @@  struct vkms_config_encoder {
 	struct list_head list;
 	unsigned int index;
 	uint32_t possible_crtcs;
+	/* only used if created from configfs */
+	struct config_group encoder_group;
+	struct config_group possible_crtcs_group;
 };
 
 struct vkms_config_connector {
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 04278a39cd3c..c0b62fb93dc0 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -19,6 +19,8 @@  static bool is_configfs_registered;
  * Initialized when a new directory is created under "/config/vkms/"
  * @crtcs_group: Default subgroup of @device_group at "/config/vkms/crtcs".
  * Each of its items represent a CRTC
+ * @encoders_group: Default subgroup of @device_group at
+ * "/config/vkms/encoders". Each of its items represent a encoder
  * @lock: Lock used to project concurrent access to the configuration attributes
  * @enabled: Protected by @lock. The device is created or destroyed when this
  * option changes
@@ -27,6 +29,7 @@  struct vkms_configfs {
 	struct vkms_config *vkms_config;
 	struct config_group device_group;
 	struct config_group crtcs_group;
+	struct config_group encoders_group;
 
 	/* protected by @lock */
 	struct mutex lock;
@@ -45,6 +48,15 @@  struct vkms_configfs {
 #define crtcs_item_to_vkms_config_crtc(item) \
 	container_of(to_config_group(item), struct vkms_config_crtc, crtc_group)
 
+#define encoders_group_to_vkms_configfs(group) \
+	container_of(group, struct vkms_configfs, encoders_group)
+
+#define encoders_item_to_vkms_config_encoder(item) \
+	container_of(to_config_group(item), struct vkms_config_encoder, encoder_group)
+
+#define encoder_possible_crtcs_item_to_vkms_config_encoder(item) \
+	container_of(to_config_group(item), struct vkms_config_encoder, possible_crtcs_group)
+
 static ssize_t crtc_cursor_show(struct config_item *item, char *page)
 {
 	struct vkms_config_crtc *crtc_cfg = crtcs_item_to_vkms_config_crtc(item);
@@ -171,6 +183,110 @@  static struct config_item_type crtcs_group_type = {
 	.ct_owner = THIS_MODULE,
 };
 
+static int encoder_possible_crtcs_allow_link(struct config_item *src,
+					     struct config_item *target)
+{
+	struct vkms_config_encoder *encoder_cfg;
+	struct vkms_config_crtc *crtc_cfg;
+
+	if (target->ci_type != &crtc_group_type)
+		return -EINVAL;
+
+	encoder_cfg = encoder_possible_crtcs_item_to_vkms_config_encoder(src);
+	crtc_cfg = crtcs_item_to_vkms_config_crtc(target);
+
+	if (encoder_cfg->possible_crtcs & BIT(crtc_cfg->index))
+		return -EINVAL;
+
+	encoder_cfg->possible_crtcs |= BIT(crtc_cfg->index);
+
+	return 0;
+}
+
+static void encoder_possible_crtcs_drop_link(struct config_item *src,
+					     struct config_item *target)
+{
+	struct vkms_config_encoder *encoder_cfg;
+	struct vkms_config_crtc *crtc_cfg;
+
+	encoder_cfg = encoder_possible_crtcs_item_to_vkms_config_encoder(src);
+	crtc_cfg = crtcs_item_to_vkms_config_crtc(target);
+
+	encoder_cfg->possible_crtcs &= ~BIT(crtc_cfg->index);
+}
+
+static struct configfs_item_operations encoder_possible_crtcs_item_ops = {
+	.allow_link = &encoder_possible_crtcs_allow_link,
+	.drop_link = &encoder_possible_crtcs_drop_link,
+};
+
+static struct config_item_type encoder_possible_crtcs_group_type = {
+	.ct_item_ops = &encoder_possible_crtcs_item_ops,
+	.ct_owner = THIS_MODULE,
+};
+
+static const struct config_item_type encoder_group_type = {
+	.ct_owner = THIS_MODULE,
+};
+
+static struct config_group *make_encoders_group(struct config_group *group,
+						const char *name)
+{
+	struct vkms_configfs *configfs = encoders_group_to_vkms_configfs(group);
+	struct vkms_config_encoder *encoder_cfg;
+	int ret;
+
+	mutex_lock(&configfs->lock);
+
+	if (configfs->enabled) {
+		ret = -EINVAL;
+		goto err_unlock;
+	}
+
+	encoder_cfg = vkms_config_add_encoder(configfs->vkms_config, 0);
+	if (IS_ERR(encoder_cfg)) {
+		ret = PTR_ERR(encoder_cfg);
+		goto err_unlock;
+	}
+
+	config_group_init_type_name(&encoder_cfg->encoder_group, name,
+				    &encoder_group_type);
+
+	config_group_init_type_name(&encoder_cfg->possible_crtcs_group,
+				    "possible_crtcs",
+				    &encoder_possible_crtcs_group_type);
+	configfs_add_default_group(&encoder_cfg->possible_crtcs_group,
+				   &encoder_cfg->encoder_group);
+
+	mutex_unlock(&configfs->lock);
+
+	return &encoder_cfg->encoder_group;
+
+err_unlock:
+	mutex_unlock(&configfs->lock);
+	return ERR_PTR(ret);
+}
+
+static void drop_encoders_group(struct config_group *group,
+				struct config_item *item)
+{
+	struct vkms_configfs *configfs = encoders_group_to_vkms_configfs(group);
+	struct vkms_config_encoder *encoder_cfg =
+		encoders_item_to_vkms_config_encoder(item);
+
+	vkms_config_destroy_encoder(configfs->vkms_config, encoder_cfg);
+}
+
+static struct configfs_group_operations encoders_group_ops = {
+	.make_group = &make_encoders_group,
+	.drop_item = &drop_encoders_group,
+};
+
+static struct config_item_type encoders_group_type = {
+	.ct_group_ops = &encoders_group_ops,
+	.ct_owner = THIS_MODULE,
+};
+
 static ssize_t device_enabled_show(struct config_item *item, char *page)
 {
 	struct vkms_configfs *configfs = config_item_to_vkms_configfs(item);
@@ -225,7 +341,6 @@  static struct config_group *make_device_group(struct config_group *group,
 					      const char *name)
 {
 	struct vkms_configfs *configfs;
-	struct vkms_config_encoder *encoder_cfg = NULL;
 	struct vkms_config_connector *connector_cfg = NULL;
 	char *config_name;
 	int ret;
@@ -252,11 +367,10 @@  static struct config_group *make_device_group(struct config_group *group,
 	configfs_add_default_group(&configfs->crtcs_group,
 				   &configfs->device_group);
 
-	encoder_cfg = vkms_config_add_encoder(configfs->vkms_config, BIT(0));
-	if (IS_ERR(encoder_cfg)) {
-		ret = PTR_ERR(encoder_cfg);
-		goto err_kfree;
-	}
+	config_group_init_type_name(&configfs->encoders_group, "encoders",
+				    &encoders_group_type);
+	configfs_add_default_group(&configfs->encoders_group,
+				   &configfs->device_group);
 
 	connector_cfg = vkms_config_add_connector(configfs->vkms_config, BIT(0),
 						  connector_status_connected);
@@ -269,7 +383,6 @@  static struct config_group *make_device_group(struct config_group *group,
 
 err_kfree:
 	kfree(configfs);
-	kfree(encoder_cfg);
 	kfree(connector_cfg);
 	return ERR_PTR(ret);
 }