Message-ID: <Z636vUrKlPtPP3yq@louis-chauvet-laptop>
Date: Thu, 13 Feb 2025 14:59:25 +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 v2 13/15] drm/vkms: Allow to attach encoders and CRTCs

On 11/02/25 - 12:09, José Expósito wrote:
> Add a list of possible CRTCs to the encoder configuration and helpers to
> attach and detach them.
> 
> Now that the default configuration has its encoder and CRTC correctly
> attached, configure the output following the configuration.
> 
> Co-developed-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>

Build is broken when compiling VKMS as a module:

diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index fdb535be912a..a5e77c752da3 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -497,6 +497,7 @@ int __must_check vkms_config_encoder_attach_crtc(struct vkms_config_encoder *enc
        return xa_alloc(&encoder_cfg->possible_crtcs, &crtc_idx, crtc_cfg,
                        xa_limit_32b, GFP_KERNEL);
 }
+EXPORT_SYMBOL_IF_KUNIT(vkms_config_encoder_attach_crtc);

 void vkms_config_encoder_detach_crtc(struct vkms_config_encoder *encoder_cfg,
                                     struct vkms_config_crtc *crtc_cfg)
@@ -509,3 +510,4 @@ void vkms_config_encoder_detach_crtc(struct vkms_config_encoder *encoder_cfg,
                        xa_erase(&encoder_cfg->possible_crtcs, idx);
        }
 }
+EXPORT_SYMBOL_IF_KUNIT(vkms_config_encoder_detach_crtc);


> ---
>  .clang-format                                 |   1 +
>  drivers/gpu/drm/vkms/tests/vkms_config_test.c | 115 ++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_config.c            |  77 ++++++++++++
>  drivers/gpu/drm/vkms/vkms_config.h            |  29 +++++
>  drivers/gpu/drm/vkms/vkms_output.c            |  49 +++++---
>  5 files changed, 251 insertions(+), 20 deletions(-)
> 
> diff --git a/.clang-format b/.clang-format
> index c355a2f58eed..5d21c0e4edbd 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -693,6 +693,7 @@ ForEachMacros:
>    - 'vkms_config_for_each_crtc'
>    - 'vkms_config_for_each_encoder'
>    - 'vkms_config_for_each_plane'
> +  - 'vkms_config_encoder_for_each_possible_crtc'
>    - 'vkms_config_plane_for_each_possible_crtc'
>    - 'while_for_each_ftrace_op'
>    - 'xa_for_each'
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_config_test.c b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> index 0bb76a1e6c79..7458d175acb6 100644
> --- a/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> @@ -260,6 +260,7 @@ static void vkms_config_test_valid_plane_type(struct kunit *test)
>  	struct vkms_config *config;
>  	struct vkms_config_plane *plane_cfg;
>  	struct vkms_config_crtc *crtc_cfg;
> +	struct vkms_config_encoder *encoder_cfg;
>  	int err;
>  
>  	config = vkms_config_default_create(false, false, false);
> @@ -313,6 +314,9 @@ static void vkms_config_test_valid_plane_type(struct kunit *test)
>  
>  	/* Invalid: Second CRTC without primary plane */
>  	crtc_cfg = vkms_config_create_crtc(config);
> +	encoder_cfg = vkms_config_create_encoder(config);
> +	err = vkms_config_encoder_attach_crtc(encoder_cfg, crtc_cfg);
> +	KUNIT_EXPECT_EQ(test, err, 0);
>  	KUNIT_EXPECT_FALSE(test, vkms_config_is_valid(config));
>  
>  	/* Valid: Second CRTC with a primary plane */
> @@ -390,6 +394,51 @@ static void vkms_config_test_invalid_encoder_number(struct kunit *test)
>  	vkms_config_destroy(config);
>  }
>  
> +static void vkms_config_test_valid_encoder_possible_crtcs(struct kunit *test)
> +{
> +	struct vkms_config *config;
> +	struct vkms_config_plane *plane_cfg;
> +	struct vkms_config_crtc *crtc_cfg1, *crtc_cfg2;
> +	struct vkms_config_encoder *encoder_cfg;
> +	int err;
> +
> +	config = vkms_config_default_create(false, false, false);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, config);
> +
> +	crtc_cfg1 = list_first_entry(&config->crtcs, typeof(*crtc_cfg1), link);
> +
> +	/* Invalid: Encoder without a possible CRTC */
> +	encoder_cfg = vkms_config_create_encoder(config);
> +	KUNIT_EXPECT_FALSE(test, vkms_config_is_valid(config));
> +
> +	/* Valid: Second CRTC with shared encoder */
> +	crtc_cfg2 = vkms_config_create_crtc(config);
> +
> +	plane_cfg = vkms_config_create_plane(config);
> +	vkms_config_plane_set_type(plane_cfg, DRM_PLANE_TYPE_PRIMARY);
> +	err = vkms_config_plane_attach_crtc(plane_cfg, crtc_cfg2);
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +
> +	err = vkms_config_encoder_attach_crtc(encoder_cfg, crtc_cfg1);
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +
> +	err = vkms_config_encoder_attach_crtc(encoder_cfg, crtc_cfg2);
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +
> +	KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
> +
> +	/* Invalid: Second CRTC without encoders */
> +	vkms_config_encoder_detach_crtc(encoder_cfg, crtc_cfg2);
> +	KUNIT_EXPECT_FALSE(test, vkms_config_is_valid(config));
> +
> +	/* Valid: First CRTC with 2 possible encoder */
> +	vkms_config_destroy_plane(plane_cfg);
> +	vkms_config_destroy_crtc(config, crtc_cfg2);
> +	KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
> +
> +	vkms_config_destroy(config);
> +}
> +
>  static void vkms_config_test_plane_attach_crtc(struct kunit *test)
>  {
>  	struct vkms_config *config;
> @@ -515,6 +564,70 @@ static void vkms_config_test_plane_get_possible_crtcs(struct kunit *test)
>  	vkms_config_destroy(config);
>  }
>  
> +static void vkms_config_test_encoder_get_possible_crtcs(struct kunit *test)
> +{
> +	struct vkms_config *config;
> +	struct vkms_config_encoder *encoder_cfg1, *encoder_cfg2;
> +	struct vkms_config_crtc *crtc_cfg1, *crtc_cfg2;
> +	struct vkms_config_crtc *possible_crtc;
> +	unsigned long idx = 0;
> +	int n_crtcs = 0;
> +	int err;
> +
> +	config = vkms_config_create("test");
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, config);
> +
> +	encoder_cfg1 = vkms_config_create_encoder(config);
> +	encoder_cfg2 = vkms_config_create_encoder(config);
> +	crtc_cfg1 = vkms_config_create_crtc(config);
> +	crtc_cfg2 = vkms_config_create_crtc(config);
> +
> +	/* No possible CRTCs */
> +	vkms_config_encoder_for_each_possible_crtc(encoder_cfg1, idx, possible_crtc)
> +		KUNIT_FAIL(test, "Unexpected possible CRTC");
> +
> +	vkms_config_encoder_for_each_possible_crtc(encoder_cfg2, idx, possible_crtc)
> +		KUNIT_FAIL(test, "Unexpected possible CRTC");
> +
> +	/* Encoder 1 attached to CRTC 1 and 2 */
> +	err = vkms_config_encoder_attach_crtc(encoder_cfg1, crtc_cfg1);
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +	err = vkms_config_encoder_attach_crtc(encoder_cfg1, crtc_cfg2);
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +
> +	vkms_config_encoder_for_each_possible_crtc(encoder_cfg1, idx, possible_crtc) {
> +		n_crtcs++;
> +		if (possible_crtc != crtc_cfg1 && possible_crtc != crtc_cfg2)
> +			KUNIT_FAIL(test, "Unexpected possible CRTC");
> +	}
> +	KUNIT_ASSERT_EQ(test, n_crtcs, 2);
> +	n_crtcs = 0;
> +
> +	vkms_config_encoder_for_each_possible_crtc(encoder_cfg2, idx, possible_crtc)
> +		KUNIT_FAIL(test, "Unexpected possible CRTC");
> +
> +	/* Encoder 1 attached to CRTC 1 and encoder 2 to CRTC 2 */
> +	vkms_config_encoder_detach_crtc(encoder_cfg1, crtc_cfg2);
> +	vkms_config_encoder_for_each_possible_crtc(encoder_cfg1, idx, possible_crtc) {
> +		n_crtcs++;
> +		if (possible_crtc != crtc_cfg1)
> +			KUNIT_FAIL(test, "Unexpected possible CRTC");
> +	}
> +	KUNIT_ASSERT_EQ(test, n_crtcs, 1);
> +	n_crtcs = 0;
> +
> +	err = vkms_config_encoder_attach_crtc(encoder_cfg2, crtc_cfg2);
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +	vkms_config_encoder_for_each_possible_crtc(encoder_cfg2, idx, possible_crtc) {
> +		n_crtcs++;
> +		if (possible_crtc != crtc_cfg2)
> +			KUNIT_FAIL(test, "Unexpected possible CRTC");
> +	}
> +	KUNIT_ASSERT_EQ(test, n_crtcs, 1);
> +
> +	vkms_config_destroy(config);
> +}
> +
>  static struct kunit_case vkms_config_test_cases[] = {
>  	KUNIT_CASE(vkms_config_test_empty_config),
>  	KUNIT_CASE_PARAM(vkms_config_test_default_config,
> @@ -527,8 +640,10 @@ static struct kunit_case vkms_config_test_cases[] = {
>  	KUNIT_CASE(vkms_config_test_valid_plane_possible_crtcs),
>  	KUNIT_CASE(vkms_config_test_invalid_crtc_number),
>  	KUNIT_CASE(vkms_config_test_invalid_encoder_number),
> +	KUNIT_CASE(vkms_config_test_valid_encoder_possible_crtcs),
>  	KUNIT_CASE(vkms_config_test_plane_attach_crtc),
>  	KUNIT_CASE(vkms_config_test_plane_get_possible_crtcs),
> +	KUNIT_CASE(vkms_config_test_encoder_get_possible_crtcs),
>  	{}
>  };
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index 0cf6105fe743..f727c0009489 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -84,6 +84,9 @@ struct vkms_config *vkms_config_default_create(bool enable_cursor,
>  	if (IS_ERR(encoder_cfg))
>  		goto err_alloc;
>  
> +	if (vkms_config_encoder_attach_crtc(encoder_cfg, crtc_cfg))
> +		goto err_alloc;
> +
>  	return config;
>  
>  err_alloc:
> @@ -212,6 +215,42 @@ static bool valid_encoder_number(struct vkms_config *config)
>  	return true;
>  }
>  
> +static bool valid_encoder_possible_crtcs(struct vkms_config *config)
> +{
> +	struct drm_device *dev = &config->dev->drm;

Ditto

> +	struct vkms_config_crtc *crtc_cfg;
> +	struct vkms_config_encoder *encoder_cfg;
> +
> +	vkms_config_for_each_encoder(config, encoder_cfg) {
> +		if (xa_empty(&encoder_cfg->possible_crtcs)) {
> +			drm_info(dev, "All encoders must have at least one possible CRTC\n");
> +			return false;
> +		}
> +	}
> +
> +	vkms_config_for_each_crtc(config, crtc_cfg) {
> +		bool crtc_has_encoder = false;
> +
> +		vkms_config_for_each_encoder(config, encoder_cfg) {
> +			struct vkms_config_crtc *possible_crtc;
> +			unsigned long idx = 0;
> +
> +			vkms_config_encoder_for_each_possible_crtc(encoder_cfg,
> +								   idx, possible_crtc) {
> +				if (possible_crtc == crtc_cfg)
> +					crtc_has_encoder = true;
> +			}
> +		}
> +
> +		if (!crtc_has_encoder) {
> +			drm_info(dev, "All CRTCs must have at least one possible encoder\n");
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  bool vkms_config_is_valid(struct vkms_config *config)
>  {
>  	struct vkms_config_crtc *crtc_cfg;
> @@ -233,6 +272,9 @@ bool vkms_config_is_valid(struct vkms_config *config)
>  			return false;
>  	}
>  
> +	if (!valid_encoder_possible_crtcs(config))
> +		return false;
> +
>  	return true;
>  }
>  
> @@ -347,10 +389,14 @@ void vkms_config_destroy_crtc(struct vkms_config *config,
>  			      struct vkms_config_crtc *crtc_cfg)
>  {
>  	struct vkms_config_plane *plane_cfg;
> +	struct vkms_config_encoder *encoder_cfg;
>  
>  	vkms_config_for_each_plane(config, plane_cfg)
>  		vkms_config_plane_detach_crtc(plane_cfg, crtc_cfg);
>  
> +	vkms_config_for_each_encoder(config, encoder_cfg)
> +		vkms_config_encoder_detach_crtc(encoder_cfg, crtc_cfg);
> +
>  	list_del(&crtc_cfg->link);
>  	kfree(crtc_cfg);
>  }
> @@ -406,6 +452,8 @@ struct vkms_config_encoder *vkms_config_create_encoder(struct vkms_config *confi
>  	if (!encoder_cfg)
>  		return ERR_PTR(-ENOMEM);
>  
> +	xa_init_flags(&encoder_cfg->possible_crtcs, XA_FLAGS_ALLOC);
> +
>  	list_add_tail(&encoder_cfg->link, &config->encoders);
>  
>  	return encoder_cfg;
> @@ -414,6 +462,35 @@ struct vkms_config_encoder *vkms_config_create_encoder(struct vkms_config *confi
>  void vkms_config_destroy_encoder(struct vkms_config *config,
>  				 struct vkms_config_encoder *encoder_cfg)
>  {
> +	xa_destroy(&encoder_cfg->possible_crtcs);
>  	list_del(&encoder_cfg->link);
>  	kfree(encoder_cfg);
>  }
> +
> +int __must_check vkms_config_encoder_attach_crtc(struct vkms_config_encoder *encoder_cfg,
> +						 struct vkms_config_crtc *crtc_cfg)
> +{
> +	struct vkms_config_crtc *possible_crtc;
> +	unsigned long idx = 0;
> +	u32 crtc_idx = 0;
> +
> +	vkms_config_encoder_for_each_possible_crtc(encoder_cfg, idx, possible_crtc) {
> +		if (possible_crtc == crtc_cfg)
> +			return -EINVAL;
> +	}
> +
> +	return xa_alloc(&encoder_cfg->possible_crtcs, &crtc_idx, crtc_cfg,
> +			xa_limit_32b, GFP_KERNEL);
> +}
> +
> +void vkms_config_encoder_detach_crtc(struct vkms_config_encoder *encoder_cfg,
> +				     struct vkms_config_crtc *crtc_cfg)
> +{
> +	struct vkms_config_crtc *possible_crtc;
> +	unsigned long idx = 0;
> +
> +	vkms_config_encoder_for_each_possible_crtc(encoder_cfg, idx, possible_crtc) {
> +		if (possible_crtc == crtc_cfg)
> +			xa_erase(&encoder_cfg->possible_crtcs, idx);
> +	}
> +}
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index 2ba80c4c9ce5..28c24afebe1e 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -71,6 +71,7 @@ struct vkms_config_crtc {
>   * struct vkms_config_encoder
>   *
>   * @link: Link to the others encoders in vkms_config
> + * @possible_crtcs: Array of CRTCs that can be used with this encoder
>   * @encoder: Internal usage. This pointer should never be considered as valid.
>   *           It can be used to store a temporary reference to a VKMS encoder
>   *           during device creation. This pointer is not managed by the
> @@ -79,6 +80,8 @@ struct vkms_config_crtc {
>  struct vkms_config_encoder {
>  	struct list_head link;
>  
> +	struct xarray possible_crtcs;
> +
>  	/* Internal usage */
>  	struct drm_encoder *encoder;
>  };
> @@ -117,6 +120,16 @@ struct vkms_config_encoder {
>  #define vkms_config_plane_for_each_possible_crtc(plane_cfg, idx, possible_crtc) \
>  	xa_for_each(&(plane_cfg)->possible_crtcs, idx, (possible_crtc))
>  
> +/**
> + * vkms_config_encoder_for_each_possible_crtc - Iterate over the
> + * vkms_config_encoder possible CRTCs
> + * @encoder_cfg: &struct vkms_config_encoder pointer
> + * @idx: Index of the cursor
> + * @possible_crtc: &struct vkms_config_crtc pointer used as cursor
> + */
> +#define vkms_config_encoder_for_each_possible_crtc(encoder_cfg, idx, possible_crtc) \
> +	xa_for_each(&(encoder_cfg)->possible_crtcs, idx, (possible_crtc))
> +
>  /**
>   * vkms_config_create() - Create a new VKMS configuration
>   * @dev_name: Name of the device
> @@ -326,4 +339,20 @@ struct vkms_config_encoder *vkms_config_create_encoder(struct vkms_config *confi
>  void vkms_config_destroy_encoder(struct vkms_config *config,
>  				 struct vkms_config_encoder *encoder_cfg);
>  
> +/**
> + * vkms_config_encoder_attach_crtc - Attach a encoder to a CRTC
> + * @encoder_cfg: Encoder to attach
> + * @crtc_cfg: CRTC to attach @encoder_cfg to
> + */
> +int __must_check vkms_config_encoder_attach_crtc(struct vkms_config_encoder *encoder_cfg,
> +						 struct vkms_config_crtc *crtc_cfg);
> +
> +/**
> + * vkms_config_encoder_detach_crtc - Detach a encoder from a CRTC
> + * @encoder_cfg: Encoder to detach
> + * @crtc_cfg: CRTC to detach @encoder_cfg from
> + */
> +void vkms_config_encoder_detach_crtc(struct vkms_config_encoder *encoder_cfg,
> +				     struct vkms_config_crtc *crtc_cfg);
> +
>  #endif /* _VKMS_CONFIG_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index f63bc8e3014b..8920d6b5d105 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -9,9 +9,9 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>  {
>  	struct drm_device *dev = &vkmsdev->drm;
>  	struct vkms_connector *connector;
> -	struct drm_encoder *encoder;
>  	struct vkms_config_plane *plane_cfg;
>  	struct vkms_config_crtc *crtc_cfg;
> +	struct vkms_config_encoder *encoder_cfg;
>  	int ret;
>  	int writeback;
>  
> @@ -61,32 +61,41 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>  		}
>  	}
>  
> +	vkms_config_for_each_encoder(vkmsdev->config, encoder_cfg) {
> +		struct vkms_config_crtc *possible_crtc;
> +		unsigned long idx = 0;
> +
> +		encoder_cfg->encoder = drmm_kzalloc(dev, sizeof(*encoder_cfg->encoder), GFP_KERNEL);
> +		if (!encoder_cfg->encoder) {
> +			DRM_ERROR("Failed to allocate encoder\n");
> +			return -ENOMEM;
> +		}
> +		ret = drmm_encoder_init(dev, encoder_cfg->encoder, NULL,
> +					DRM_MODE_ENCODER_VIRTUAL, NULL);
> +		if (ret) {
> +			DRM_ERROR("Failed to init encoder\n");
> +			return ret;
> +		}
> +
> +		vkms_config_encoder_for_each_possible_crtc(encoder_cfg, idx, possible_crtc) {
> +			encoder_cfg->encoder->possible_crtcs |=
> +				drm_crtc_mask(&possible_crtc->crtc->crtc);
> +		}
> +	}
> +
>  	connector = vkms_connector_init(vkmsdev);
>  	if (IS_ERR(connector)) {
>  		DRM_ERROR("Failed to init connector\n");
>  		return PTR_ERR(connector);
>  	}
>  
> -	encoder = drmm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
> -	if (!encoder) {
> -		DRM_ERROR("Failed to allocate encoder\n");
> -		return -ENOMEM;
> -	}
> -	ret = drmm_encoder_init(dev, encoder, NULL,
> -				DRM_MODE_ENCODER_VIRTUAL, NULL);
> -	if (ret) {
> -		DRM_ERROR("Failed to init encoder\n");
> -		return ret;
> -	}
> -
> -	vkms_config_for_each_crtc(vkmsdev->config, crtc_cfg)
> -		encoder->possible_crtcs = drm_crtc_mask(&crtc_cfg->crtc->crtc);
> -
>  	/* Attach the encoder and the connector */
> -	ret = drm_connector_attach_encoder(&connector->base, encoder);
> -	if (ret) {
> -		DRM_ERROR("Failed to attach connector to encoder\n");
> -		return ret;
> +	vkms_config_for_each_encoder(vkmsdev->config, encoder_cfg) {
> +		ret = drm_connector_attach_encoder(&connector->base, encoder_cfg->encoder);
> +		if (ret) {
> +			DRM_ERROR("Failed to attach connector to encoder\n");
> +			return ret;
> +		}
>  	}
>  
>  	drm_mode_config_reset(dev);
> -- 
> 2.48.1
> 

Powered by blists - more mailing lists