Message-ID: <Zrue3y6Lr6D4s1Tn@louis-chauvet-laptop>
Date: Tue, 13 Aug 2024 19:58:55 +0200
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: José Expósito <jose.exposito89@...il.com>
Cc: rodrigosiqueiramelo@...il.com, melissa.srw@...il.com,
	mairacanal@...eup.net, hamohammed.sa@...il.com, daniel@...ll.ch,
	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: [RFC PATCH 04/17] drm/vkms: Allow to configure multiple CRTCs

Le 13/08/24 - 12:44, José Expósito a écrit :
> Add a list of CRTC configurations to vkms_config and add as many CRTCs
> as configured during output initialization.
> 
> For backwards compatibility, create a single CRTC in the default
> configuration.
> 
> Since writeback support, vblank and composer states are now per CRTC,
> extract all the fields to the vkms_crtc structure and allow each
> vkms_device to have a list of CRTCs.
> 
> Signed-off-by: José Expósito <jose.exposito89@...il.com>


If we choose your implementation, can you split this commit in two? I 
think you did two interesting stuff here:
- Creating the vkms_crtc structure
- Creating the vkms_crtc_config

> ---
>  drivers/gpu/drm/vkms/vkms_composer.c  | 30 ++++----
>  drivers/gpu/drm/vkms/vkms_config.c    | 54 ++++++++++++++-
>  drivers/gpu/drm/vkms/vkms_config.h    | 15 +++-
>  drivers/gpu/drm/vkms/vkms_crtc.c      | 99 ++++++++++++++-------------
>  drivers/gpu/drm/vkms/vkms_drv.c       | 10 ++-
>  drivers/gpu/drm/vkms/vkms_drv.h       | 35 +++++++---
>  drivers/gpu/drm/vkms/vkms_output.c    | 38 +++++-----
>  drivers/gpu/drm/vkms/vkms_writeback.c | 27 ++++----
>  8 files changed, 197 insertions(+), 111 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index 6ab8091bf72f..3af750071f04 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -26,20 +27,29 @@ struct vkms_config *vkms_config_default_create(bool enable_cursor,
>  					       bool enable_overlay)
>  {
>  	struct vkms_config *config;
> +	struct vkms_config_crtc *crtc_cfg;
>  
>  	config = vkms_config_create(DEFAULT_DEVICE_NAME);
>  	if (IS_ERR(config))
>  		return config;
>  
>  	config->cursor = enable_cursor;
> -	config->writeback = enable_writeback;
>  	config->overlay = enable_overlay;
>  
> +	crtc_cfg = vkms_config_add_crtc(config, enable_writeback);
> +	if (IS_ERR(crtc_cfg))
> +		return ERR_CAST(crtc_cfg);
> +
>  	return config;
>  }
>  
>  void vkms_config_destroy(struct vkms_config *config)
>  {
> +	struct vkms_config_crtc *crtc_cfg, *crtc_tmp;
> +
> +	list_for_each_entry_safe(crtc_cfg, crtc_tmp, &config->crtcs, list)
> +		vkms_config_destroy_crtc(config, crtc_cfg);
> +
>  	kfree(config);
>  }
>  
> @@ -48,12 +58,20 @@ static int vkms_config_show(struct seq_file *m, void *data)
>  	struct drm_debugfs_entry *entry = m->private;
>  	struct drm_device *dev = entry->dev;
>  	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> +	struct vkms_config_crtc *crtc_cfg;
> +	int n;
>  
>  	seq_printf(m, "dev_name=%s\n", vkmsdev->config->dev_name);
> -	seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
>  	seq_printf(m, "cursor=%d\n", vkmsdev->config->cursor);
>  	seq_printf(m, "overlay=%d\n", vkmsdev->config->overlay);
>  
> +	n = 0;
> +	list_for_each_entry(crtc_cfg, &vkmsdev->config->crtcs, list) {
> +		seq_printf(m, "crtc(%d).writeback=%d\n", n,
> +			   crtc_cfg->writeback);
> +		n++;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -66,3 +84,35 @@ void vkms_config_debugfs_init(struct vkms_device *vkms_device)
>  	drm_debugfs_add_files(&vkms_device->drm, vkms_config_debugfs_list,
>  			      ARRAY_SIZE(vkms_config_debugfs_list));
>  }
> +
> +struct vkms_config_crtc *vkms_config_add_crtc(struct vkms_config *config,
> +					      bool enable_writeback)
> +{
> +	struct vkms_config_crtc *crtc_cfg;
> +
> +	crtc_cfg = kzalloc(sizeof(*crtc_cfg), GFP_KERNEL);
> +	if (!crtc_cfg)
> +		return ERR_PTR(-ENOMEM);
> +
> +	crtc_cfg->writeback = enable_writeback;
> +
> +	crtc_cfg->index = 0;
> +	if (!list_empty(&config->crtcs)) {
> +		struct vkms_config_crtc *last;
> +
> +		last = list_last_entry(&config->crtcs, struct vkms_config_crtc,
> +				       list);
> +		crtc_cfg->index = last->index + 1;
> +	}

This code may break if you create 32 crtc, delete the 31 first, and then 
recreate 31 crtc. The indexes will be 31..63, which is wrong I think for 
the `possible_crtc` mask.

That why in my series I choose to use drm_crtc_mask with the created crtc 
instance, so I don't have to manage in my code this case, I just rely on 
drm core implementation to provide indexes.

> +
> +	list_add_tail(&crtc_cfg->list, &config->crtcs);
> +
> +	return crtc_cfg;
> +}
> +
> +void vkms_config_destroy_crtc(struct vkms_config *config,
> +			      struct vkms_config_crtc *crtc_cfg)
> +{
> +	list_del(&crtc_cfg->list);
> +	kfree(crtc_cfg);
> +}
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index ba06aad32799..bc40a0e3859a 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -3,15 +3,22 @@
>  #ifndef _VKMS_CONFIG_H_
>  #define _VKMS_CONFIG_H_
>  
> +#include <linux/list.h>
>  #include <linux/types.h>
>  
>  struct vkms_device;
>  
> +struct vkms_config_crtc {
> +	struct list_head list;
> +	unsigned int index;
> +	bool writeback;
> +};
> +
>  struct vkms_config {
>  	char *dev_name;
> -	bool writeback;
>  	bool cursor;
>  	bool overlay;
> +	struct list_head crtcs;
>  	/* only set when instantiated */
>  	struct vkms_device *dev;
>  };
> @@ -26,4 +33,10 @@ void vkms_config_destroy(struct vkms_config *config);
>  /* DebugFS */
>  void vkms_config_debugfs_init(struct vkms_device *vkms_device);
>  
> +/* CRTCs */
> +struct vkms_config_crtc *vkms_config_add_crtc(struct vkms_config *config,
> +					      bool enable_writeback);
> +void vkms_config_destroy_crtc(struct vkms_config *config,
> +			      struct vkms_config_crtc *crtc_cfg);
> +
>  #endif /* _VKMS_CONFIG_H_ */



> -int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> -		   struct drm_plane *primary, struct drm_plane *cursor)
> +struct vkms_crtc *vkms_crtc_init(struct drm_device *dev,
> +				 struct drm_plane *primary,
> +				 struct drm_plane *cursor,
> +				 unsigned int index)
>  {
> -	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> -	int ret;
> +	struct vkms_crtc *crtc;
>  
> -	ret = drmm_crtc_init_with_planes(dev, crtc, primary, cursor,
> -					 &vkms_crtc_funcs, NULL);
> -	if (ret) {
> -		DRM_ERROR("Failed to init CRTC\n");
> -		return ret;
> +	crtc = drmm_crtc_alloc_with_planes(dev, struct vkms_crtc, base, primary,
> +					   cursor, &vkms_crtc_funcs, NULL);
> +	if (IS_ERR(crtc)) {
> +		DRM_ERROR("Failed to alloc CRTC\n");
> +		return crtc;
>  	}
>  
> -	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
> +	crtc->base.index = index;
> +	primary->possible_crtcs = drm_crtc_mask(&crtc->base);

It feel strange to configure the primary plane possible_crtc inside the 
vkms_crtc_init function, espicially when it is already configured in
vkms_plane_init.

The previous comment apply for allocating index to crtc, it is already 
managed by the drm core (in drmm_crtc_alloc_with_plane). I don't think 
this is an issue for the core to have it changed, but I would prefer to 
avoid duplicating this management in vkms.

> +	if (cursor)
> +		cursor->possible_crtcs = drm_crtc_mask(&crtc->base);

(same for cursor)

> -	drm_mode_crtc_set_gamma_size(crtc, VKMS_LUT_SIZE);
> -	drm_crtc_enable_color_mgmt(crtc, 0, false, VKMS_LUT_SIZE);
> +	INIT_LIST_HEAD(&crtc->list);
>  
> -	spin_lock_init(&vkms_out->lock);
> -	spin_lock_init(&vkms_out->composer_lock);
> +	drm_crtc_helper_add(&crtc->base, &vkms_crtc_helper_funcs);
>  
> -	vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
> -	if (!vkms_out->composer_workq)
> -		return -ENOMEM;
> +	drm_mode_crtc_set_gamma_size(&crtc->base, VKMS_LUT_SIZE);
> +	drm_crtc_enable_color_mgmt(&crtc->base, 0, false, VKMS_LUT_SIZE);
> +
> +	spin_lock_init(&crtc->lock);
> +	spin_lock_init(&crtc->composer_lock);
> +
> +	crtc->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
> +	if (!crtc->composer_workq)
> +		return ERR_PTR(-ENOMEM);
>
> -	return ret;
> +	return crtc;
>  }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 2f9d1db0cfae..15a2ba26d190 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -54,9 +54,12 @@ DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>  static void vkms_release(struct drm_device *dev)
>  {
>  	struct vkms_device *vkms = drm_device_to_vkms_device(dev);
> +	struct vkms_crtc *vkms_crtc;
>  
> -	if (vkms->output.composer_workq)
> -		destroy_workqueue(vkms->output.composer_workq);
> +	list_for_each_entry(vkms_crtc, &vkms->crtcs, list) {
> +		if (vkms_crtc->composer_workq)
> +			destroy_workqueue(vkms_crtc->composer_workq);
> +	}
>  }

To avoid managing this per-crtc, you can maybe use 
drmm_add_action_or_reset [1] and let drm core manage the call to 
destroy_workqueue.

[1]: https://github.com/Fomys/linux/commit/c4d665599798265aedd131f93cff8c7263cacff8#diff-08d2420ad9a7a48e3c7dafa0f61e0a7c860a53de2f41659d03735875cffebea5R318
  
>  static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> @@ -177,6 +180,7 @@ static int vkms_create(struct vkms_config *config)
>  	}
>  	vkms_device->platform = pdev;
>  	vkms_device->config = config;
> +	vkms_device->crtcs = (struct list_head)LIST_HEAD_INIT(vkms_device->crtcs);

Why do you need a cast here?

>  	config->dev = vkms_device;
>  
>  	ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
> @@ -187,7 +191,7 @@ static int vkms_create(struct vkms_config *config)
>  		goto out_devres;
>  	}
>  
> -	ret = drm_vblank_init(&vkms_device->drm, 1);
> +	ret = drm_vblank_init(&vkms_device->drm, list_count_nodes(&config->crtcs));

Thanks, I think I forgot this change in my implementation!

>  	if (ret) {
>  		DRM_ERROR("Failed to vblank\n");
>  		goto out_devres;
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 87e44b51a03f..3156ff896c33 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -4,6 +4,7 @@
>  #define _VKMS_DRV_H_
>  
>  #include <linux/hrtimer.h>
> +#include <linux/list.h>
>  
>  #include <drm/drm.h>
>  #include <drm/drm_framebuffer.h>
> @@ -98,10 +99,11 @@ struct vkms_crtc_state {
>  	u64 frame_end;
>  };
>  
> -struct vkms_output {
> -	struct drm_crtc crtc;
> -	struct drm_encoder encoder;
> -	struct drm_connector connector;
> +struct vkms_crtc {
> +	struct list_head list;
> +
> +	struct drm_crtc base;
> +
>  	struct drm_writeback_connector wb_connector;
>  	struct hrtimer vblank_hrtimer;
>  	ktime_t period_ns;
> @@ -119,19 +121,28 @@ struct vkms_output {
>  
>  struct vkms_config;
>  
> +struct vkms_output {
> +	struct drm_encoder encoder;
> +	struct drm_connector connector;
> +};
> +
>  struct vkms_device {
>  	struct drm_device drm;
>  	struct platform_device *platform;
>  	struct vkms_output output;
> +	struct list_head crtcs;
>  	const struct vkms_config *config;
>  };

I don't think you need to store the list of crtcs. You can use 
drm_for_each_crtc and container_of macro to get the vkms_crtc structure. 

>  
> -#define drm_crtc_to_vkms_output(target) \
> -	container_of(target, struct vkms_output, crtc)
> -
>  #define drm_device_to_vkms_device(target) \
>  	container_of(target, struct vkms_device, drm)
>  
> +#define drm_crtc_to_vkms_crtc(crtc) \
> +	container_of(crtc, struct vkms_crtc, base)
> +
> +#define timer_to_vkms_crtc(timer) \
> +	container_of(timer, struct vkms_crtc, vblank_hrtimer)
> +
>  #define to_vkms_crtc_state(target)\
>  	container_of(target, struct vkms_crtc_state, base)
>  
> @@ -139,8 +150,10 @@ struct vkms_device {
>  	container_of(target, struct vkms_plane_state, base.base)
>  
>  /* CRTC */
> -int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> -		   struct drm_plane *primary, struct drm_plane *cursor);
> +struct vkms_crtc *vkms_crtc_init(struct drm_device *dev,
> +				 struct drm_plane *primary,
> +				 struct drm_plane *cursor,
> +				 unsigned int index);
>  
>  int vkms_output_init(struct vkms_device *vkmsdev, int index);
>  
> @@ -156,11 +169,11 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>  
>  /* Composer Support */
>  void vkms_composer_worker(struct work_struct *work);
> -void vkms_set_composer(struct vkms_output *out, bool enabled);
> +void vkms_set_composer(struct vkms_crtc *vkms_crtc, bool enabled);
>  void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state *plane, int y);
>  void vkms_writeback_row(struct vkms_writeback_job *wb, const struct line_buffer *src_buffer, int y);
>  
>  /* Writeback */
> -int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
> +int vkms_enable_writeback_connector(struct vkms_crtc *vkms_crtc);
>  
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index afe3945c1962..dcd32bc30e17 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -32,8 +32,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
>  	.get_modes    = vkms_conn_get_modes,
>  };
>  
> -static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
> -				  struct drm_crtc *crtc)
> +static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index)
>  {
>  	struct vkms_plane *overlay;
>  
> @@ -42,7 +41,7 @@ static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
>  		return PTR_ERR(overlay);
>  
>  	if (!overlay->base.possible_crtcs)
> -		overlay->base.possible_crtcs = drm_crtc_mask(crtc);
> +		overlay->base.possible_crtcs = BIT(index);

Again, I prefer to see `drm_crtc_mask` to compute possible_crtcs bitmask.
  
>  	return 0;
>  }
> @@ -53,7 +52,8 @@ 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 = &output->encoder;
> -	struct drm_crtc *crtc = &output->crtc;
> +	struct vkms_crtc *vkms_crtc;
> +	struct vkms_config_crtc *crtc_cfg;
>  	struct vkms_plane *primary, *cursor = NULL;
>  	int ret;
>  	int writeback;
> @@ -65,7 +65,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  
>  	if (vkmsdev->config->overlay) {
>  		for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
> -			ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
> +			ret = vkms_add_overlay_plane(vkmsdev, index);
>  			if (ret)
>  				return ret;
>  		}
> @@ -77,15 +77,26 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  			return PTR_ERR(cursor);
>  	}
>  
> -	ret = vkms_crtc_init(dev, crtc, &primary->base, &cursor->base);
> -	if (ret)
> -		return ret;
> +	list_for_each_entry(crtc_cfg, &vkmsdev->config->crtcs, list) {
> +		vkms_crtc = vkms_crtc_init(dev, &primary->base, &cursor->base,
> +					   crtc_cfg->index);
> +		if (IS_ERR(vkms_crtc))
> +			return PTR_ERR(vkms_crtc);
> +
> +		list_add_tail(&vkms_crtc->list, &vkmsdev->crtcs);
> +
> +		if (crtc_cfg->writeback) {
> +			writeback = vkms_enable_writeback_connector(vkms_crtc);
> +			if (writeback)
> +				DRM_ERROR("Failed to init writeback connector\n");
> +		}
> +	}

You are creating multiple CRTC, but only link planes to the `index` one. 
Is this intentional? DRM is not happy if a CRTC don't have any primary 
plane.

>  
>  	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
>  				 DRM_MODE_CONNECTOR_VIRTUAL);
>  	if (ret) {
>  		DRM_ERROR("Failed to init connector\n");
> -		goto err_connector;
> +		return ret;
>  	}
>  
>  	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
> @@ -104,12 +115,6 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  		goto err_attach;
>  	}
>  
> -	if (vkmsdev->config->writeback) {
> -		writeback = vkms_enable_writeback_connector(vkmsdev);
> -		if (writeback)
> -			DRM_ERROR("Failed to init writeback connector\n");
> -	}
> -
>  	drm_mode_config_reset(dev);
>  
>  	return 0;
> @@ -120,8 +125,5 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  err_encoder:
>  	drm_connector_cleanup(connector);
>  
> -err_connector:
> -	drm_crtc_cleanup(crtc);
> -
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index bc724cbd5e3a..b317cb291586 100644

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists