diff mbox series

[RFC,06/17] drm/vkms: Allow to configure multiple encoders

Message ID 20240813105134.17439-7-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 
Add a list of encoder configurations to vkms_config and add as many
encoders as configured during output initialization.

For backwards compatibility, create a single encoder in the default
configuration.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_config.c | 50 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_config.h | 13 ++++++++
 drivers/gpu/drm/vkms/vkms_output.c | 14 ++++++---
 3 files changed, 73 insertions(+), 4 deletions(-)

Comments

Louis Chauvet Aug. 13, 2024, 5:58 p.m. UTC | #1 
Le 13/08/24 - 12:44, José Expósito a écrit :
> Add a list of encoder configurations to vkms_config and add as many
> encoders as configured during output initialization.
> 
> For backwards compatibility, create a single encoder in the default
> configuration.

You don't manage the deletion of crtc in vkms_config_destroy_crtc. As you 
use vkms_config_encoder.possible_crtc to initialize a CRTC, it may be an 
issue for DRM.

> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---
>  drivers/gpu/drm/vkms/vkms_config.c | 50 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_config.h | 13 ++++++++
>  drivers/gpu/drm/vkms/vkms_output.c | 14 ++++++---
>  3 files changed, 73 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index 3af750071f04..6a8dfebee24e 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -18,6 +18,7 @@ struct vkms_config *vkms_config_create(char *dev_name)
>  
>  	config->dev_name = dev_name;
>  	config->crtcs = (struct list_head)LIST_HEAD_INIT(config->crtcs);
> +	config->encoders = (struct list_head)LIST_HEAD_INIT(config->encoders);

Again, why there is a cast? And why you don't use INIT_LIST_HEAD? I think 
LIST_HEAD_INIT is used to initialize static structure and 
INIT_LIST_HEAD for dynamic structures.
  
>  	return config;
>  }
> @@ -28,6 +29,7 @@ struct vkms_config *vkms_config_default_create(bool enable_cursor,
>  {
>  	struct vkms_config *config;
>  	struct vkms_config_crtc *crtc_cfg;
> +	struct vkms_config_encoder *encoder_cfg;
>  
>  	config = vkms_config_create(DEFAULT_DEVICE_NAME);
>  	if (IS_ERR(config))
> @@ -40,16 +42,24 @@ struct vkms_config *vkms_config_default_create(bool enable_cursor,
>  	if (IS_ERR(crtc_cfg))
>  		return ERR_CAST(crtc_cfg);
>  
> +	encoder_cfg = vkms_config_add_encoder(config, BIT(0));

Why do you use a magic value here and not what is set by 
vkms_config_add_crtc?

	encoder_cfg = vkms_config_add_encoder(config, crtc_cfg->index);

> +	if (IS_ERR(encoder_cfg))
> +		return ERR_CAST(encoder_cfg);
> +

Here the kzalloc from vkms_config_create is leaked.

>  	return config;
>  }
>  
>  void vkms_config_destroy(struct vkms_config *config)
>  {
>  	struct vkms_config_crtc *crtc_cfg, *crtc_tmp;
> +	struct vkms_config_encoder *encoder_cfg, *encoder_tmp;
>  
>  	list_for_each_entry_safe(crtc_cfg, crtc_tmp, &config->crtcs, list)
>  		vkms_config_destroy_crtc(config, crtc_cfg);
>  
> +	list_for_each_entry_safe(encoder_cfg, encoder_tmp, &config->encoders, list)
> +		vkms_config_destroy_encoder(config, encoder_cfg);
> +
>  	kfree(config);
>  }
>  
> @@ -59,6 +69,7 @@ static int vkms_config_show(struct seq_file *m, void *data)
>  	struct drm_device *dev = entry->dev;
>  	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
>  	struct vkms_config_crtc *crtc_cfg;
> +	struct vkms_config_encoder *encoder_cfg;
>  	int n;
>  
>  	seq_printf(m, "dev_name=%s\n", vkmsdev->config->dev_name);
> @@ -72,6 +83,13 @@ static int vkms_config_show(struct seq_file *m, void *data)
>  		n++;
>  	}
>  
> +	n = 0;
> +	list_for_each_entry(encoder_cfg, &vkmsdev->config->encoders, list) {
> +		seq_printf(m, "encoder(%d).possible_crtcs=%d\n", n,
> +			   encoder_cfg->possible_crtcs);
> +		n++;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -116,3 +134,35 @@ void vkms_config_destroy_crtc(struct vkms_config *config,
>  	list_del(&crtc_cfg->list);
>  	kfree(crtc_cfg);
>  }
> +
> +struct vkms_config_encoder *vkms_config_add_encoder(struct vkms_config *config,
> +						    uint32_t possible_crtcs)
> +{
> +	struct vkms_config_encoder *encoder_cfg;
> +
> +	encoder_cfg = kzalloc(sizeof(*encoder_cfg), GFP_KERNEL);
> +	if (!encoder_cfg)
> +		return ERR_PTR(-ENOMEM);
> +
> +	encoder_cfg->possible_crtcs = possible_crtcs;
> +
> +	encoder_cfg->index = 0;
> +	if (!list_empty(&config->encoders)) {
> +		struct vkms_config_encoder *last;
> +
> +		last = list_last_entry(&config->encoders,
> +				       struct vkms_config_encoder, list);
> +		encoder_cfg->index = last->index + 1;

Same here, drm core is already managing indexes, can we avoid 
reimplementing this logic?

There is the same issue as in CRTC, if you create 32 encoders, remove one 
and add one, the index will be invalid for drm.

> +	}
> +
> +	list_add_tail(&encoder_cfg->list, &config->encoders);
> +
> +	return encoder_cfg;
> +}
> +
> +void vkms_config_destroy_encoder(struct vkms_config *config,
> +				 struct vkms_config_encoder *encoder_cfg)
> +{
> +	list_del(&encoder_cfg->list);
> +	kfree(encoder_cfg);
> +}
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index bc40a0e3859a..b717b5c0d3d9 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -14,11 +14,18 @@ struct vkms_config_crtc {
>  	bool writeback;
>  };
>  
> +struct vkms_config_encoder {
> +	struct list_head list;
> +	unsigned int index;
> +	uint32_t possible_crtcs;
> +};
> +
>  struct vkms_config {
>  	char *dev_name;
>  	bool cursor;
>  	bool overlay;
>  	struct list_head crtcs;
> +	struct list_head encoders;
>  	/* only set when instantiated */
>  	struct vkms_device *dev;
>  };
> @@ -39,4 +46,10 @@ struct vkms_config_crtc *vkms_config_add_crtc(struct vkms_config *config,
>  void vkms_config_destroy_crtc(struct vkms_config *config,
>  			      struct vkms_config_crtc *crtc_cfg);
>  
> +/* Encoders */
> +struct vkms_config_encoder *vkms_config_add_encoder(struct vkms_config *config,
> +						    uint32_t possible_crtcs);
> +void vkms_config_destroy_encoder(struct vkms_config *config,
> +				 struct vkms_config_encoder *encoder_cfg);
> +
>  #endif /* _VKMS_CONFIG_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 15f0b72af325..7afe37aea52d 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -30,7 +30,8 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
>  };
>  
>  static struct drm_encoder *vkms_encoder_init(struct vkms_device *vkms_device,
> -					     uint32_t possible_crtcs)
> +					     uint32_t possible_crtcs,
> +					     unsigned int index)
>  {
>  	struct drm_encoder *encoder;
>  	int ret;
> @@ -49,6 +50,7 @@ static struct drm_encoder *vkms_encoder_init(struct vkms_device *vkms_device,
>  		return ERR_PTR(ret);
>  	}
>  
> +	encoder->index = index;
>  	encoder->possible_crtcs = possible_crtcs;
>  
>  	return encoder;
> @@ -74,6 +76,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  	struct drm_device *dev = &vkmsdev->drm;
>  	struct drm_connector *connector = &output->connector;
>  	struct drm_encoder *encoder;
> +	struct vkms_config_encoder *encoder_cfg;
>  	struct vkms_crtc *vkms_crtc;
>  	struct vkms_config_crtc *crtc_cfg;
>  	struct vkms_plane *primary, *cursor = NULL;
> @@ -123,9 +126,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  
>  	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
>  
> -	encoder = vkms_encoder_init(vkmsdev, BIT(0));
> -	if (IS_ERR(encoder))
> -		return PTR_ERR(encoder);
> +	list_for_each_entry(encoder_cfg, &vkmsdev->config->encoders, list) {
> +		encoder = vkms_encoder_init(vkmsdev, encoder_cfg->possible_crtcs,
> +					    encoder_cfg->index);
> +		if (IS_ERR(encoder))
> +			return PTR_ERR(encoder);
> +	}
>  
>  	ret = drm_connector_attach_encoder(connector, encoder);
>  	if (ret) {
> -- 
> 2.46.0
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index 3af750071f04..6a8dfebee24e 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -18,6 +18,7 @@  struct vkms_config *vkms_config_create(char *dev_name)
 
 	config->dev_name = dev_name;
 	config->crtcs = (struct list_head)LIST_HEAD_INIT(config->crtcs);
+	config->encoders = (struct list_head)LIST_HEAD_INIT(config->encoders);
 
 	return config;
 }
@@ -28,6 +29,7 @@  struct vkms_config *vkms_config_default_create(bool enable_cursor,
 {
 	struct vkms_config *config;
 	struct vkms_config_crtc *crtc_cfg;
+	struct vkms_config_encoder *encoder_cfg;
 
 	config = vkms_config_create(DEFAULT_DEVICE_NAME);
 	if (IS_ERR(config))
@@ -40,16 +42,24 @@  struct vkms_config *vkms_config_default_create(bool enable_cursor,
 	if (IS_ERR(crtc_cfg))
 		return ERR_CAST(crtc_cfg);
 
+	encoder_cfg = vkms_config_add_encoder(config, BIT(0));
+	if (IS_ERR(encoder_cfg))
+		return ERR_CAST(encoder_cfg);
+
 	return config;
 }
 
 void vkms_config_destroy(struct vkms_config *config)
 {
 	struct vkms_config_crtc *crtc_cfg, *crtc_tmp;
+	struct vkms_config_encoder *encoder_cfg, *encoder_tmp;
 
 	list_for_each_entry_safe(crtc_cfg, crtc_tmp, &config->crtcs, list)
 		vkms_config_destroy_crtc(config, crtc_cfg);
 
+	list_for_each_entry_safe(encoder_cfg, encoder_tmp, &config->encoders, list)
+		vkms_config_destroy_encoder(config, encoder_cfg);
+
 	kfree(config);
 }
 
@@ -59,6 +69,7 @@  static int vkms_config_show(struct seq_file *m, void *data)
 	struct drm_device *dev = entry->dev;
 	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
 	struct vkms_config_crtc *crtc_cfg;
+	struct vkms_config_encoder *encoder_cfg;
 	int n;
 
 	seq_printf(m, "dev_name=%s\n", vkmsdev->config->dev_name);
@@ -72,6 +83,13 @@  static int vkms_config_show(struct seq_file *m, void *data)
 		n++;
 	}
 
+	n = 0;
+	list_for_each_entry(encoder_cfg, &vkmsdev->config->encoders, list) {
+		seq_printf(m, "encoder(%d).possible_crtcs=%d\n", n,
+			   encoder_cfg->possible_crtcs);
+		n++;
+	}
+
 	return 0;
 }
 
@@ -116,3 +134,35 @@  void vkms_config_destroy_crtc(struct vkms_config *config,
 	list_del(&crtc_cfg->list);
 	kfree(crtc_cfg);
 }
+
+struct vkms_config_encoder *vkms_config_add_encoder(struct vkms_config *config,
+						    uint32_t possible_crtcs)
+{
+	struct vkms_config_encoder *encoder_cfg;
+
+	encoder_cfg = kzalloc(sizeof(*encoder_cfg), GFP_KERNEL);
+	if (!encoder_cfg)
+		return ERR_PTR(-ENOMEM);
+
+	encoder_cfg->possible_crtcs = possible_crtcs;
+
+	encoder_cfg->index = 0;
+	if (!list_empty(&config->encoders)) {
+		struct vkms_config_encoder *last;
+
+		last = list_last_entry(&config->encoders,
+				       struct vkms_config_encoder, list);
+		encoder_cfg->index = last->index + 1;
+	}
+
+	list_add_tail(&encoder_cfg->list, &config->encoders);
+
+	return encoder_cfg;
+}
+
+void vkms_config_destroy_encoder(struct vkms_config *config,
+				 struct vkms_config_encoder *encoder_cfg)
+{
+	list_del(&encoder_cfg->list);
+	kfree(encoder_cfg);
+}
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index bc40a0e3859a..b717b5c0d3d9 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -14,11 +14,18 @@  struct vkms_config_crtc {
 	bool writeback;
 };
 
+struct vkms_config_encoder {
+	struct list_head list;
+	unsigned int index;
+	uint32_t possible_crtcs;
+};
+
 struct vkms_config {
 	char *dev_name;
 	bool cursor;
 	bool overlay;
 	struct list_head crtcs;
+	struct list_head encoders;
 	/* only set when instantiated */
 	struct vkms_device *dev;
 };
@@ -39,4 +46,10 @@  struct vkms_config_crtc *vkms_config_add_crtc(struct vkms_config *config,
 void vkms_config_destroy_crtc(struct vkms_config *config,
 			      struct vkms_config_crtc *crtc_cfg);
 
+/* Encoders */
+struct vkms_config_encoder *vkms_config_add_encoder(struct vkms_config *config,
+						    uint32_t possible_crtcs);
+void vkms_config_destroy_encoder(struct vkms_config *config,
+				 struct vkms_config_encoder *encoder_cfg);
+
 #endif /* _VKMS_CONFIG_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 15f0b72af325..7afe37aea52d 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -30,7 +30,8 @@  static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
 };
 
 static struct drm_encoder *vkms_encoder_init(struct vkms_device *vkms_device,
-					     uint32_t possible_crtcs)
+					     uint32_t possible_crtcs,
+					     unsigned int index)
 {
 	struct drm_encoder *encoder;
 	int ret;
@@ -49,6 +50,7 @@  static struct drm_encoder *vkms_encoder_init(struct vkms_device *vkms_device,
 		return ERR_PTR(ret);
 	}
 
+	encoder->index = index;
 	encoder->possible_crtcs = possible_crtcs;
 
 	return encoder;
@@ -74,6 +76,7 @@  int vkms_output_init(struct vkms_device *vkmsdev, int index)
 	struct drm_device *dev = &vkmsdev->drm;
 	struct drm_connector *connector = &output->connector;
 	struct drm_encoder *encoder;
+	struct vkms_config_encoder *encoder_cfg;
 	struct vkms_crtc *vkms_crtc;
 	struct vkms_config_crtc *crtc_cfg;
 	struct vkms_plane *primary, *cursor = NULL;
@@ -123,9 +126,12 @@  int vkms_output_init(struct vkms_device *vkmsdev, int index)
 
 	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
 
-	encoder = vkms_encoder_init(vkmsdev, BIT(0));
-	if (IS_ERR(encoder))
-		return PTR_ERR(encoder);
+	list_for_each_entry(encoder_cfg, &vkmsdev->config->encoders, list) {
+		encoder = vkms_encoder_init(vkmsdev, encoder_cfg->possible_crtcs,
+					    encoder_cfg->index);
+		if (IS_ERR(encoder))
+			return PTR_ERR(encoder);
+	}
 
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret) {