Message-ID: <Z636vXoYgNVwa8Qt@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 15/15] drm/vkms: Allow to attach connectors and
 encoders

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

The compilation is broken as module:

diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index 7f4e21e8ee0c..76f24bd463f6 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -610,6 +610,7 @@ int __must_check vkms_config_connector_attach_encoder(struct vkms_config_connect
        return xa_alloc(&connector_cfg->possible_encoders, &encoder_idx,
                        encoder_cfg, xa_limit_32b, GFP_KERNEL);
 }
+EXPORT_SYMBOL_IF_KUNIT(vkms_config_connector_attach_encoder);

 void vkms_config_connector_detach_encoder(struct vkms_config_connector *connector_cfg,
                                          struct vkms_config_encoder *encoder_cfg)
@@ -623,3 +624,4 @@ void vkms_config_connector_detach_encoder(struct vkms_config_connector *connecto
                        xa_erase(&connector_cfg->possible_encoders, idx);
        }
 }
+EXPORT_SYMBOL_IF_KUNIT(vkms_config_connector_detach_encoder);


> ---
>  .clang-format                                 |  1 +
>  drivers/gpu/drm/vkms/tests/vkms_config_test.c | 94 +++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_config.c            | 60 ++++++++++++
>  drivers/gpu/drm/vkms/vkms_config.h            | 29 ++++++
>  drivers/gpu/drm/vkms/vkms_output.c            | 33 ++++---
>  5 files changed, 204 insertions(+), 13 deletions(-)
> 
> diff --git a/.clang-format b/.clang-format
> index ca49832993c5..7630990aa07a 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -694,6 +694,7 @@ ForEachMacros:
>    - 'vkms_config_for_each_crtc'
>    - 'vkms_config_for_each_encoder'
>    - 'vkms_config_for_each_plane'
> +  - 'vkms_config_connector_for_each_possible_encoder'
>    - 'vkms_config_encoder_for_each_possible_crtc'
>    - 'vkms_config_plane_for_each_possible_crtc'
>    - 'while_for_each_ftrace_op'
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_config_test.c b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> index cba7e9d2fcad..2d104ecfde3b 100644
> --- a/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> @@ -511,6 +511,27 @@ static void vkms_config_test_invalid_connector_number(struct kunit *test)
>  	vkms_config_destroy(config);
>  }
>  
> +static void vkms_config_test_valid_connector_possible_encoders(struct kunit *test)
> +{
> +	struct vkms_config *config;
> +	struct vkms_config_encoder *encoder_cfg;
> +	struct vkms_config_connector *connector_cfg;
> +
> +	config = vkms_config_default_create(false, false, false);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, config);
> +
> +	encoder_cfg = list_first_entry(&config->encoders,
> +				       typeof(*encoder_cfg), link);
> +	connector_cfg = list_first_entry(&config->connectors,
> +					 typeof(*connector_cfg), link);
> +
> +	/* Invalid: Connector without a possible encoder */
> +	vkms_config_connector_detach_encoder(connector_cfg, encoder_cfg);
> +	KUNIT_EXPECT_FALSE(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;
> @@ -700,6 +721,77 @@ static void vkms_config_test_encoder_get_possible_crtcs(struct kunit *test)
>  	vkms_config_destroy(config);
>  }
>  
> +static void vkms_config_test_connector_get_possible_encoders(struct kunit *test)
> +{
> +	struct vkms_config *config;
> +	struct vkms_config_connector *connector_cfg1, *connector_cfg2;
> +	struct vkms_config_encoder *encoder_cfg1, *encoder_cfg2;
> +	struct vkms_config_encoder *possible_encoder;
> +	unsigned long idx = 0;
> +	int n_encoders = 0;
> +	int err;
> +
> +	config = vkms_config_create("test");
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, config);
> +
> +	connector_cfg1 = vkms_config_create_connector(config);
> +	connector_cfg2 = vkms_config_create_connector(config);
> +	encoder_cfg1 = vkms_config_create_encoder(config);
> +	encoder_cfg2 = vkms_config_create_encoder(config);
> +
> +	/* No possible encoders */
> +	vkms_config_connector_for_each_possible_encoder(connector_cfg1, idx,
> +							possible_encoder)
> +		KUNIT_FAIL(test, "Unexpected possible encoder");
> +
> +	vkms_config_connector_for_each_possible_encoder(connector_cfg2, idx,
> +							possible_encoder)
> +		KUNIT_FAIL(test, "Unexpected possible encoder");
> +
> +	/* Connector 1 attached to encoders 1 and 2 */
> +	err = vkms_config_connector_attach_encoder(connector_cfg1, encoder_cfg1);
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +	err = vkms_config_connector_attach_encoder(connector_cfg1, encoder_cfg2);
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +
> +	vkms_config_connector_for_each_possible_encoder(connector_cfg1, idx,
> +							possible_encoder) {
> +		n_encoders++;
> +		if (possible_encoder != encoder_cfg1 &&
> +		    possible_encoder != encoder_cfg2)
> +			KUNIT_FAIL(test, "Unexpected possible encoder");
> +	}
> +	KUNIT_ASSERT_EQ(test, n_encoders, 2);
> +	n_encoders = 0;
> +
> +	vkms_config_connector_for_each_possible_encoder(connector_cfg2, idx,
> +							possible_encoder)
> +		KUNIT_FAIL(test, "Unexpected possible encoder");
> +
> +	/* Connector 1 attached to encoder 1 and connector 2 to encoder 2 */
> +	vkms_config_connector_detach_encoder(connector_cfg1, encoder_cfg2);
> +	vkms_config_connector_for_each_possible_encoder(connector_cfg1, idx,
> +							possible_encoder) {
> +		n_encoders++;
> +		if (possible_encoder != encoder_cfg1)
> +			KUNIT_FAIL(test, "Unexpected possible encoder");
> +	}
> +	KUNIT_ASSERT_EQ(test, n_encoders, 1);
> +	n_encoders = 0;
> +
> +	err = vkms_config_connector_attach_encoder(connector_cfg2, encoder_cfg2);
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +	vkms_config_connector_for_each_possible_encoder(connector_cfg2, idx,
> +							possible_encoder) {
> +		n_encoders++;
> +		if (possible_encoder != encoder_cfg2)
> +			KUNIT_FAIL(test, "Unexpected possible encoder");
> +	}
> +	KUNIT_ASSERT_EQ(test, n_encoders, 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,
> @@ -715,9 +807,11 @@ static struct kunit_case vkms_config_test_cases[] = {
>  	KUNIT_CASE(vkms_config_test_invalid_encoder_number),
>  	KUNIT_CASE(vkms_config_test_valid_encoder_possible_crtcs),
>  	KUNIT_CASE(vkms_config_test_invalid_connector_number),
> +	KUNIT_CASE(vkms_config_test_valid_connector_possible_encoders),
>  	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),
> +	KUNIT_CASE(vkms_config_test_connector_get_possible_encoders),
>  	{}
>  };
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index d52280d3bbee..3d95dc713151 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -93,6 +93,9 @@ struct vkms_config *vkms_config_default_create(bool enable_cursor,
>  	if (IS_ERR(connector_cfg))
>  		goto err_alloc;
>  
> +	if (vkms_config_connector_attach_encoder(connector_cfg, encoder_cfg))
> +		goto err_alloc;
> +
>  	return config;
>  
>  err_alloc:
> @@ -275,6 +278,22 @@ static bool valid_connector_number(struct vkms_config *config)
>  	return true;
>  }
>  
> +static bool valid_connector_possible_encoders(struct vkms_config *config)
> +{
> +	struct drm_device *dev = &config->dev->drm;
> +	struct vkms_config_connector *connector_cfg;
> +
> +	vkms_config_for_each_connector(config, connector_cfg) {
> +		if (xa_empty(&connector_cfg->possible_encoders)) {
> +			drm_info(dev,
> +				 "All connectors 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;
> @@ -302,6 +321,9 @@ bool vkms_config_is_valid(struct vkms_config *config)
>  	if (!valid_encoder_possible_crtcs(config))
>  		return false;
>  
> +	if (!valid_connector_possible_encoders(config))
> +		return false;
> +
>  	return true;
>  }
>  
> @@ -493,6 +515,11 @@ 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)
>  {
> +	struct vkms_config_connector *connector_cfg;
> +
> +	vkms_config_for_each_connector(config, connector_cfg)
> +		vkms_config_connector_detach_encoder(connector_cfg, encoder_cfg);
> +
>  	xa_destroy(&encoder_cfg->possible_crtcs);
>  	list_del(&encoder_cfg->link);
>  	kfree(encoder_cfg);
> @@ -534,6 +561,8 @@ struct vkms_config_connector *vkms_config_create_connector(struct vkms_config *c
>  	if (!connector_cfg)
>  		return ERR_PTR(-ENOMEM);
>  
> +	xa_init_flags(&connector_cfg->possible_encoders, XA_FLAGS_ALLOC);
> +
>  	list_add_tail(&connector_cfg->link, &config->connectors);
>  
>  	return connector_cfg;
> @@ -541,6 +570,37 @@ struct vkms_config_connector *vkms_config_create_connector(struct vkms_config *c
>  
>  void vkms_config_destroy_connector(struct vkms_config_connector *connector_cfg)
>  {
> +	xa_destroy(&connector_cfg->possible_encoders);
>  	list_del(&connector_cfg->link);
>  	kfree(connector_cfg);
>  }
> +
> +int __must_check vkms_config_connector_attach_encoder(struct vkms_config_connector *connector_cfg,
> +						      struct vkms_config_encoder *encoder_cfg)
> +{
> +	struct vkms_config_encoder *possible_encoder;
> +	unsigned long idx = 0;
> +	u32 encoder_idx = 0;
> +
> +	vkms_config_connector_for_each_possible_encoder(connector_cfg, idx,
> +							possible_encoder) {
> +		if (possible_encoder == encoder_cfg)
> +			return -EINVAL;
> +	}
> +
> +	return xa_alloc(&connector_cfg->possible_encoders, &encoder_idx,
> +			encoder_cfg, xa_limit_32b, GFP_KERNEL);
> +}
> +
> +void vkms_config_connector_detach_encoder(struct vkms_config_connector *connector_cfg,
> +					  struct vkms_config_encoder *encoder_cfg)
> +{
> +	struct vkms_config_encoder *possible_encoder;
> +	unsigned long idx = 0;
> +
> +	vkms_config_connector_for_each_possible_encoder(connector_cfg, idx,
> +							possible_encoder) {
> +		if (possible_encoder == encoder_cfg)
> +			xa_erase(&connector_cfg->possible_encoders, idx);
> +	}
> +}
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index 8451c2f127b6..c87513d174f2 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -92,6 +92,7 @@ struct vkms_config_encoder {
>   * struct vkms_config_connector
>   *
>   * @link: Link to the others connector in vkms_config
> + * @possible_encoders: Array of encoders that can be used with this connector
>   * @connector: Internal usage. This pointer should never be considered as valid.
>   *             It can be used to store a temporary reference to a VKMS connector
>   *             during device creation. This pointer is not managed by the
> @@ -100,6 +101,8 @@ struct vkms_config_encoder {
>  struct vkms_config_connector {
>  	struct list_head link;
>  
> +	struct xarray possible_encoders;
> +
>  	/* Internal usage */
>  	struct vkms_connector *connector;
>  };
> @@ -156,6 +159,16 @@ struct vkms_config_connector {
>  #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_connector_for_each_possible_encoder - Iterate over the
> + * vkms_config_connector possible encoders
> + * @connector_cfg: &struct vkms_config_connector pointer
> + * @idx: Index of the cursor
> + * @possible_encoder: &struct vkms_config_encoder pointer used as cursor
> + */
> +#define vkms_config_connector_for_each_possible_encoder(connector_cfg, idx, possible_encoder) \
> +	xa_for_each(&(connector_cfg)->possible_encoders, idx, (possible_encoder))
> +
>  /**
>   * vkms_config_create() - Create a new VKMS configuration
>   * @dev_name: Name of the device
> @@ -397,4 +410,20 @@ struct vkms_config_connector *vkms_config_create_connector(struct vkms_config *c
>   */
>  void vkms_config_destroy_connector(struct vkms_config_connector *connector_cfg);
>  
> +/**
> + * vkms_config_connector_attach_encoder - Attach a connector to an encoder
> + * @connector_cfg: Connector to attach
> + * @encoder_cfg: Encoder to attach @connector_cfg to
> + */
> +int __must_check vkms_config_connector_attach_encoder(struct vkms_config_connector *connector_cfg,
> +						      struct vkms_config_encoder *encoder_cfg);
> +
> +/**
> + * vkms_config_connector_detach_encoder - Detach a connector from an encoder
> + * @connector_cfg: Connector to detach
> + * @encoder_cfg: Encoder to detach @connector_cfg from
> + */
> +void vkms_config_connector_detach_encoder(struct vkms_config_connector *connector_cfg,
> +					  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 8920d6b5d105..8d7ca0cdd79f 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -8,10 +8,10 @@
>  int vkms_output_init(struct vkms_device *vkmsdev)
>  {
>  	struct drm_device *dev = &vkmsdev->drm;
> -	struct vkms_connector *connector;
>  	struct vkms_config_plane *plane_cfg;
>  	struct vkms_config_crtc *crtc_cfg;
>  	struct vkms_config_encoder *encoder_cfg;
> +	struct vkms_config_connector *connector_cfg;
>  	int ret;
>  	int writeback;
>  
> @@ -83,22 +83,29 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>  		}
>  	}
>  
> -	connector = vkms_connector_init(vkmsdev);
> -	if (IS_ERR(connector)) {
> -		DRM_ERROR("Failed to init connector\n");
> -		return PTR_ERR(connector);
> -	}
> +	vkms_config_for_each_connector(vkmsdev->config, connector_cfg) {
> +		struct vkms_config_encoder *possible_encoder;
> +		unsigned long idx = 0;
>  
> -	/* Attach the encoder and the connector */
> -	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;
> +		connector_cfg->connector = vkms_connector_init(vkmsdev);
> +		if (IS_ERR(connector_cfg->connector)) {
> +			DRM_ERROR("Failed to init connector\n");
> +			return PTR_ERR(connector_cfg->connector);
> +		}
> +
> +		vkms_config_connector_for_each_possible_encoder(connector_cfg,
> +								idx,
> +								possible_encoder) {
> +			ret = drm_connector_attach_encoder(&connector_cfg->connector->base,
> +							   possible_encoder->encoder);
> +			if (ret) {
> +				DRM_ERROR("Failed to attach connector to encoder\n");
> +				return ret;
> +			}
>  		}
>  	}
>  
>  	drm_mode_config_reset(dev);
>  
> -	return ret;
> +	return 0;
>  }
> -- 
> 2.48.1
> 

Powered by blists - more mailing lists