[PATCH v4 0/3] drm/vkms: Switch to allocated for drm objects

[PATCH v4 0/3] drm/vkms: Switch to allocated for drm objects

[Louis Chauvet]

Specific allocations for each DRM object is not strictly needed in VKMS
right now, but in order to implement dynamic configuration of VKMS
(configFS), it will be easier to have one allocation per DRM object.

There is no need for a dynamic allocation for the writeback connector as
there can only be one per CRTC

No functionnal changes are intented in this series.

This series requires [1] to switch vkms objects to drm-managed objects.

[1]:https://lore.kernel.org/all/20250116-google-vkms-managed-v9-0-3e4ae1bd05a0@bootlin.com/

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
Changes in v4:
- Rebased
- Add R-by from Maxime
- Link to v3: https://lore.kernel.org/r/20241230-b4-vkms-allocated-v3-0-5dda4c1bd459@bootlin.com

Changes in v3:
- Add R-by
- Fix bug reported by José
- Remove patch to rename vkms_output to vkms_crtc
- Link to v2: https://lore.kernel.org/r/20241122-b4-vkms-allocated-v2-0-ff7bddbf0bfb@bootlin.com

Changes in v2:
- Rebased on drm-misc-next
- Apply comments from Jose
- Link to v1: https://lore.kernel.org/r/20240912-b4-vkms-allocated-v1-0-29abbaa14af9@bootlin.com

---
Louis Chauvet (3):
      drm/vkms: Switch to dynamic allocation for connector
      drm/vkms: Switch to dynamic allocation for encoder
      drm/vkms: Switch to dynamic allocation for CRTC

 drivers/gpu/drm/vkms/vkms_crtc.c      | 32 ++++++++++++++++--------------
 drivers/gpu/drm/vkms/vkms_drv.h       | 10 ++++------
 drivers/gpu/drm/vkms/vkms_output.c    | 37 +++++++++++++++++++++++------------
 drivers/gpu/drm/vkms/vkms_writeback.c | 23 +++++++++++-----------
 4 files changed, 58 insertions(+), 44 deletions(-)
---
base-commit: 6e11ce84c514f3ad8c8c766e1328bf49d80a0325
change-id: 20240909-b4-vkms-allocated-9f5508f4444a
prerequisite-message-id: 20250116-google-vkms-managed-v9-0-3e4ae1bd05a0@bootlin.com
prerequisite-patch-id: b608594ad493a41000ee703792eac4b23f9e35dc
prerequisite-patch-id: 5697aa87c44bbf3eda8a1ba424465dc792545d4c
prerequisite-patch-id: 274d0d4c603d6db7ced0b10153509c82b40ca274
prerequisite-patch-id: 5d35b764a11db0c22162acaf3524e0c6a40087b1
prerequisite-patch-id: ddaf2d9d6e6901923b3b84f0c32139e89a023132
prerequisite-patch-id: c309f210cafc91ec1dc91e4546cefe2b8d701b1f
prerequisite-patch-id: 1cc8ec3deb833570bf6fde605256bd9ddbf04083
prerequisite-patch-id: 52942106bbd11265737c0b41cd90c7b5ca04fd4d

Best regards,
-- 
Louis Chauvet <louis.chauvet@bootlin.com>

[PATCH v4 1/3] drm/vkms: Switch to dynamic allocation for connector

[Louis Chauvet]

A specific allocation for the connector is not strictly necessary
at this point, but in order to implement dynamic configuration of
VKMS (configFS), it will be easier to have one allocation per
connector.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_drv.h    | 1 -
 drivers/gpu/drm/vkms/vkms_output.c | 8 +++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 46ac36aebb27ce8d9018224735007c1b3fe7d0a5..afa625457b6156135d2b07cbd5b5e3f3c7f8f33a 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -177,7 +177,6 @@ struct vkms_crtc_state {
 struct vkms_output {
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
-	struct drm_connector connector;
 	struct drm_writeback_connector wb_connector;
 	struct drm_encoder wb_encoder;
 	struct hrtimer vblank_hrtimer;
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index de817e2794860f9071a71b3631460691e0d73a85..a41d7a29a377c14a3281968dfeb8f2b43000b120 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -32,7 +32,7 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 {
 	struct vkms_output *output = &vkmsdev->output;
 	struct drm_device *dev = &vkmsdev->drm;
-	struct drm_connector *connector = &output->connector;
+	struct drm_connector *connector;
 	struct drm_encoder *encoder = &output->encoder;
 	struct drm_crtc *crtc = &output->crtc;
 	struct vkms_plane *primary, *overlay, *cursor = NULL;
@@ -71,6 +71,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 		}
 	}
 
+	connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
+	if (!connector) {
+		DRM_ERROR("Failed to allocate connector\n");
+		return -ENOMEM;
+	}
+
 	ret = drmm_connector_init(dev, connector, &vkms_connector_funcs,
 				  DRM_MODE_CONNECTOR_VIRTUAL, NULL);
 	if (ret) {

-- 
2.47.1

[PATCH v4 2/3] drm/vkms: Switch to dynamic allocation for encoder

[Louis Chauvet]

A specific allocation for the encoder is not strictly necessary  at this
point, but in order to implement dynamic configuration of VKMS (configFS),
it will be easier to have one allocation per encoder.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_drv.h    | 1 -
 drivers/gpu/drm/vkms/vkms_output.c | 7 ++++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index afa625457b6156135d2b07cbd5b5e3f3c7f8f33a..333983bcf8d46ef85101e7c344e256df57551b78 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -176,7 +176,6 @@ struct vkms_crtc_state {
  */
 struct vkms_output {
 	struct drm_crtc crtc;
-	struct drm_encoder encoder;
 	struct drm_writeback_connector wb_connector;
 	struct drm_encoder wb_encoder;
 	struct hrtimer vblank_hrtimer;
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index a41d7a29a377c14a3281968dfeb8f2b43000b120..21ca975e424d148b0669b87784d86f5da2a8b333 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -33,7 +33,7 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 	struct vkms_output *output = &vkmsdev->output;
 	struct drm_device *dev = &vkmsdev->drm;
 	struct drm_connector *connector;
-	struct drm_encoder *encoder = &output->encoder;
+	struct drm_encoder *encoder;
 	struct drm_crtc *crtc = &output->crtc;
 	struct vkms_plane *primary, *overlay, *cursor = NULL;
 	int ret;
@@ -86,6 +86,11 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 
 	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
 
+	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) {

-- 
2.47.1

[PATCH v4 3/3] drm/vkms: Switch to dynamic allocation for CRTC

[Louis Chauvet]

A specific allocation for the CRTC is not strictly necessary at this
point, but in order to implement dynamic configuration of VKMS (configFS),
it will be easier to have one allocation per CRTC.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_crtc.c      | 32 +++++++++++++++++---------------
 drivers/gpu/drm/vkms/vkms_drv.h       |  8 ++++----
 drivers/gpu/drm/vkms/vkms_output.c    | 22 ++++++++++++----------
 drivers/gpu/drm/vkms/vkms_writeback.c | 23 ++++++++++++-----------
 4 files changed, 45 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 434c35d5e9477d2961826262591db8ab43838e09..cf229aec71c3f03bb1306095e8dd44d63f80cd2a 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -84,9 +84,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
 				      int *max_error, ktime_t *vblank_time,
 				      bool in_vblank_irq)
 {
-	struct drm_device *dev = crtc->dev;
-	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
-	struct vkms_output *output = &vkmsdev->output;
+	struct vkms_output *output = drm_crtc_to_vkms_output(crtc);
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
 
 	if (!READ_ONCE(vblank->enabled)) {
@@ -271,25 +269,29 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
 	.atomic_disable	= vkms_crtc_atomic_disable,
 };
 
-int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
-		   struct drm_plane *primary, struct drm_plane *cursor)
+struct vkms_output *vkms_crtc_init(struct drm_device *dev, struct drm_plane *primary,
+				   struct drm_plane *cursor)
 {
-	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
+	struct vkms_output *vkms_out;
+	struct drm_crtc *crtc;
 	int ret;
 
-	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;
+	vkms_out = drmm_crtc_alloc_with_planes(dev, struct vkms_output, crtc,
+					       primary, cursor,
+					       &vkms_crtc_funcs, NULL);
+	if (IS_ERR(vkms_out)) {
+		DRM_DEV_ERROR(dev->dev, "Failed to init CRTC\n");
+		return vkms_out;
 	}
 
+	crtc = &vkms_out->crtc;
+
 	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
 
 	ret = drm_mode_crtc_set_gamma_size(crtc, VKMS_LUT_SIZE);
 	if (ret) {
 		DRM_ERROR("Failed to set gamma size\n");
-		return ret;
+		return ERR_PTR(ret);
 	}
 
 	drm_crtc_enable_color_mgmt(crtc, 0, false, VKMS_LUT_SIZE);
@@ -299,9 +301,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 
 	vkms_out->composer_workq = drmm_alloc_ordered_workqueue(dev, "vkms_composer", 0);
 	if (IS_ERR(vkms_out->composer_workq))
-		return PTR_ERR(vkms_out->composer_workq);
+		return ERR_CAST(vkms_out->composer_workq);
 	if (!vkms_out->composer_workq)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
-	return ret;
+	return vkms_out;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 333983bcf8d46ef85101e7c344e256df57551b78..abbb652be2b5389f96cec78837117ceb9acef656 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -215,7 +215,6 @@ struct vkms_config {
 struct vkms_device {
 	struct drm_device drm;
 	struct platform_device *platform;
-	struct vkms_output output;
 	const struct vkms_config *config;
 };
 
@@ -242,8 +241,9 @@ struct vkms_device {
  * @primary: primary plane to attach to the CRTC
  * @cursor: plane to attach to the CRTC
  */
-int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
-		   struct drm_plane *primary, struct drm_plane *cursor);
+struct vkms_output *vkms_crtc_init(struct drm_device *dev,
+				   struct drm_plane *primary,
+				   struct drm_plane *cursor);
 
 /**
  * vkms_output_init() - Initialize all sub-components needed for a VKMS device.
@@ -274,6 +274,6 @@ void vkms_set_composer(struct vkms_output *out, bool enabled);
 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, struct drm_crtc *crtc);
+int vkms_enable_writeback_connector(struct vkms_device *vkmsdev, struct vkms_output *vkms_out);
 
 #endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 21ca975e424d148b0669b87784d86f5da2a8b333..22f0d678af3ac8177e43c4ea730af3f2109de5f3 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -30,11 +30,10 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
 
 int vkms_output_init(struct vkms_device *vkmsdev)
 {
-	struct vkms_output *output = &vkmsdev->output;
 	struct drm_device *dev = &vkmsdev->drm;
 	struct drm_connector *connector;
 	struct drm_encoder *encoder;
-	struct drm_crtc *crtc = &output->crtc;
+	struct vkms_output *output;
 	struct vkms_plane *primary, *overlay, *cursor = NULL;
 	int ret;
 	int writeback;
@@ -56,9 +55,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 			return PTR_ERR(cursor);
 	}
 
-	ret = vkms_crtc_init(dev, crtc, &primary->base, &cursor->base);
-	if (ret)
-		return ret;
+	output = vkms_crtc_init(dev, &primary->base,
+				cursor ? &cursor->base : NULL);
+	if (IS_ERR(output)) {
+		DRM_ERROR("Failed to allocate CRTC\n");
+		return PTR_ERR(output);
+	}
 
 	if (vkmsdev->config->overlay) {
 		for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
@@ -67,7 +69,7 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 				DRM_DEV_ERROR(dev->dev, "Failed to init vkms plane\n");
 				return PTR_ERR(overlay);
 			}
-			overlay->base.possible_crtcs = drm_crtc_mask(crtc);
+			overlay->base.possible_crtcs = drm_crtc_mask(&output->crtc);
 		}
 	}
 
@@ -97,23 +99,23 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 		DRM_ERROR("Failed to init encoder\n");
 		return ret;
 	}
-	encoder->possible_crtcs = drm_crtc_mask(crtc);
+	encoder->possible_crtcs = drm_crtc_mask(&output->crtc);
 
+	/* Attach the encoder and the connector */
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret) {
 		DRM_ERROR("Failed to attach connector to encoder\n");
 		return ret;
 	}
 
+	/* Initialize the writeback component */
 	if (vkmsdev->config->writeback) {
-		writeback = vkms_enable_writeback_connector(vkmsdev, crtc);
+		writeback = vkms_enable_writeback_connector(vkmsdev, output);
 		if (writeback)
 			DRM_ERROR("Failed to init writeback connector\n");
 	}
 
 	drm_mode_config_reset(dev);
 
-	return 0;
-
 	return ret;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 981975c2b0a0c75e4a3aceca2a965f5876ae0a8f..e9b5c74d7c58e7faed870a4368decbd67dab23d2 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -105,7 +105,9 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
 				struct drm_writeback_job *job)
 {
 	struct vkms_writeback_job *vkmsjob = job->priv;
-	struct vkms_device *vkmsdev;
+	struct vkms_output *vkms_output = container_of(connector,
+						       struct vkms_output,
+						       wb_connector);
 
 	if (!job->fb)
 		return;
@@ -114,8 +116,7 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
 
 	drm_framebuffer_put(vkmsjob->wb_frame_info.fb);
 
-	vkmsdev = drm_device_to_vkms_device(job->fb->dev);
-	vkms_set_composer(&vkmsdev->output, false);
+	vkms_set_composer(vkms_output, false);
 	kfree(vkmsjob);
 }
 
@@ -124,8 +125,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
 {
 	struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
 											 conn);
-	struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
-	struct vkms_output *output = &vkmsdev->output;
+	struct vkms_output *output = drm_crtc_to_vkms_output(connector_state->crtc);
 	struct drm_writeback_connector *wb_conn = &output->wb_connector;
 	struct drm_connector_state *conn_state = wb_conn->base.state;
 	struct vkms_crtc_state *crtc_state = output->composer_state;
@@ -139,7 +139,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
 	if (!conn_state)
 		return;
 
-	vkms_set_composer(&vkmsdev->output, true);
+	vkms_set_composer(output, true);
 
 	active_wb = conn_state->writeback_job->priv;
 	wb_frame_info = &active_wb->wb_frame_info;
@@ -162,22 +162,23 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
 	.atomic_check = vkms_wb_atomic_check,
 };
 
-int vkms_enable_writeback_connector(struct vkms_device *vkmsdev, struct drm_crtc *crtc)
+int vkms_enable_writeback_connector(struct vkms_device *vkmsdev,
+				    struct vkms_output *vkms_output)
 {
-	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
+	struct drm_writeback_connector *wb = &vkms_output->wb_connector;
 	int ret;
 
-	ret = drmm_encoder_init(&vkmsdev->drm, &vkmsdev->output.wb_encoder,
+	ret = drmm_encoder_init(&vkmsdev->drm, &vkms_output->wb_encoder,
 				NULL, DRM_MODE_ENCODER_VIRTUAL, NULL);
 	if (ret)
 		return ret;
-	vkmsdev->output.wb_encoder.possible_crtcs |= drm_crtc_mask(crtc);
+	vkms_output->wb_encoder.possible_crtcs |= drm_crtc_mask(&vkms_output->crtc);
 
 	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
 
 	return drmm_writeback_connector_init(&vkmsdev->drm, wb,
 					     &vkms_wb_connector_funcs,
-					     &vkmsdev->output.wb_encoder,
+					     &vkms_output->wb_encoder,
 					     vkms_wb_formats,
 					     ARRAY_SIZE(vkms_wb_formats));
 }

-- 
2.47.1

[PATCH v4 3/3] drm/vkms: Switch to dynamic allocation for CRTC

[José Expósito]

> A specific allocation for the CRTC is not strictly necessary at this
> point, but in order to implement dynamic configuration of VKMS (configFS),
> it will be easier to have one allocation per CRTC.
> 
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c      | 32 +++++++++++++++++---------------
>  drivers/gpu/drm/vkms/vkms_drv.h       |  8 ++++----
>  drivers/gpu/drm/vkms/vkms_output.c    | 22 ++++++++++++----------
>  drivers/gpu/drm/vkms/vkms_writeback.c | 23 ++++++++++++-----------
>  4 files changed, 45 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 434c35d5e9477d2961826262591db8ab43838e09..cf229aec71c3f03bb1306095e8dd44d63f80cd2a 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -84,9 +84,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
>  				      int *max_error, ktime_t *vblank_time,
>  				      bool in_vblank_irq)
>  {
> -	struct drm_device *dev = crtc->dev;
> -	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> -	struct vkms_output *output = &vkmsdev->output;
> +	struct vkms_output *output = drm_crtc_to_vkms_output(crtc);
>  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>  
>  	if (!READ_ONCE(vblank->enabled)) {
> @@ -271,25 +269,29 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
>  	.atomic_disable	= vkms_crtc_atomic_disable,
>  };
>  
> -int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> -		   struct drm_plane *primary, struct drm_plane *cursor)
> +struct vkms_output *vkms_crtc_init(struct drm_device *dev, struct drm_plane *primary,
> +				   struct drm_plane *cursor)
>  {
> -	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> +	struct vkms_output *vkms_out;
> +	struct drm_crtc *crtc;
>  	int ret;
>  
> -	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;
> +	vkms_out = drmm_crtc_alloc_with_planes(dev, struct vkms_output, crtc,
> +					       primary, cursor,
> +					       &vkms_crtc_funcs, NULL);
> +	if (IS_ERR(vkms_out)) {
> +		DRM_DEV_ERROR(dev->dev, "Failed to init CRTC\n");
> +		return vkms_out;
>  	}
>  
> +	crtc = &vkms_out->crtc;
> +
>  	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
>  
>  	ret = drm_mode_crtc_set_gamma_size(crtc, VKMS_LUT_SIZE);
>  	if (ret) {
>  		DRM_ERROR("Failed to set gamma size\n");
> -		return ret;
> +		return ERR_PTR(ret);
>  	}
>  
>  	drm_crtc_enable_color_mgmt(crtc, 0, false, VKMS_LUT_SIZE);
> @@ -299,9 +301,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  
>  	vkms_out->composer_workq = drmm_alloc_ordered_workqueue(dev, "vkms_composer", 0);
>  	if (IS_ERR(vkms_out->composer_workq))
> -		return PTR_ERR(vkms_out->composer_workq);
> +		return ERR_CAST(vkms_out->composer_workq);
>  	if (!vkms_out->composer_workq)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
> -	return ret;
> +	return vkms_out;
>  }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 333983bcf8d46ef85101e7c344e256df57551b78..abbb652be2b5389f96cec78837117ceb9acef656 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -215,7 +215,6 @@ struct vkms_config {
>  struct vkms_device {
>  	struct drm_device drm;
>  	struct platform_device *platform;
> -	struct vkms_output output;
>  	const struct vkms_config *config;
>  };
>  
> @@ -242,8 +241,9 @@ struct vkms_device {
>   * @primary: primary plane to attach to the CRTC
>   * @cursor: plane to attach to the CRTC
>   */
> -int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> -		   struct drm_plane *primary, struct drm_plane *cursor);
> +struct vkms_output *vkms_crtc_init(struct drm_device *dev,
> +				   struct drm_plane *primary,
> +				   struct drm_plane *cursor);
>  
>  /**
>   * vkms_output_init() - Initialize all sub-components needed for a VKMS device.
> @@ -274,6 +274,6 @@ void vkms_set_composer(struct vkms_output *out, bool enabled);
>  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, struct drm_crtc *crtc);
> +int vkms_enable_writeback_connector(struct vkms_device *vkmsdev, struct vkms_output *vkms_out);
>  
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 21ca975e424d148b0669b87784d86f5da2a8b333..22f0d678af3ac8177e43c4ea730af3f2109de5f3 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -30,11 +30,10 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
>  
>  int vkms_output_init(struct vkms_device *vkmsdev)
>  {
> -	struct vkms_output *output = &vkmsdev->output;
>  	struct drm_device *dev = &vkmsdev->drm;
>  	struct drm_connector *connector;
>  	struct drm_encoder *encoder;
> -	struct drm_crtc *crtc = &output->crtc;
> +	struct vkms_output *output;
>  	struct vkms_plane *primary, *overlay, *cursor = NULL;
>  	int ret;
>  	int writeback;
> @@ -56,9 +55,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>  			return PTR_ERR(cursor);
>  	}
>  
> -	ret = vkms_crtc_init(dev, crtc, &primary->base, &cursor->base);
> -	if (ret)
> -		return ret;
> +	output = vkms_crtc_init(dev, &primary->base,
> +				cursor ? &cursor->base : NULL);
> +	if (IS_ERR(output)) {
> +		DRM_ERROR("Failed to allocate CRTC\n");
> +		return PTR_ERR(output);
> +	}
>  
>  	if (vkmsdev->config->overlay) {
>  		for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
> @@ -67,7 +69,7 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>  				DRM_DEV_ERROR(dev->dev, "Failed to init vkms plane\n");
>  				return PTR_ERR(overlay);
>  			}
> -			overlay->base.possible_crtcs = drm_crtc_mask(crtc);
> +			overlay->base.possible_crtcs = drm_crtc_mask(&output->crtc);
>  		}
>  	}
>  
> @@ -97,23 +99,23 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>  		DRM_ERROR("Failed to init encoder\n");
>  		return ret;
>  	}
> -	encoder->possible_crtcs = drm_crtc_mask(crtc);
> +	encoder->possible_crtcs = drm_crtc_mask(&output->crtc);
>  
> +	/* Attach the encoder and the connector */
>  	ret = drm_connector_attach_encoder(connector, encoder);
>  	if (ret) {
>  		DRM_ERROR("Failed to attach connector to encoder\n");
>  		return ret;
>  	}
>  
> +	/* Initialize the writeback component */
>  	if (vkmsdev->config->writeback) {
> -		writeback = vkms_enable_writeback_connector(vkmsdev, crtc);
> +		writeback = vkms_enable_writeback_connector(vkmsdev, output);
>  		if (writeback)
>  			DRM_ERROR("Failed to init writeback connector\n");
>  	}

Hi Louis,

Thanks for fixing this error condition.

I have been working and running automated tests on top of this series and
I haven't found any other issue.

Reviewed-by: José Expósito <jose.exposito89@gmail.com>

>  
>  	drm_mode_config_reset(dev);
>  
> -	return 0;
> -
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 981975c2b0a0c75e4a3aceca2a965f5876ae0a8f..e9b5c74d7c58e7faed870a4368decbd67dab23d2 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -105,7 +105,9 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
>  				struct drm_writeback_job *job)
>  {
>  	struct vkms_writeback_job *vkmsjob = job->priv;
> -	struct vkms_device *vkmsdev;
> +	struct vkms_output *vkms_output = container_of(connector,
> +						       struct vkms_output,
> +						       wb_connector);
>  
>  	if (!job->fb)
>  		return;
> @@ -114,8 +116,7 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
>  
>  	drm_framebuffer_put(vkmsjob->wb_frame_info.fb);
>  
> -	vkmsdev = drm_device_to_vkms_device(job->fb->dev);
> -	vkms_set_composer(&vkmsdev->output, false);
> +	vkms_set_composer(vkms_output, false);
>  	kfree(vkmsjob);
>  }
>  
> @@ -124,8 +125,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
>  {
>  	struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
>  											 conn);
> -	struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> -	struct vkms_output *output = &vkmsdev->output;
> +	struct vkms_output *output = drm_crtc_to_vkms_output(connector_state->crtc);
>  	struct drm_writeback_connector *wb_conn = &output->wb_connector;
>  	struct drm_connector_state *conn_state = wb_conn->base.state;
>  	struct vkms_crtc_state *crtc_state = output->composer_state;
> @@ -139,7 +139,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
>  	if (!conn_state)
>  		return;
>  
> -	vkms_set_composer(&vkmsdev->output, true);
> +	vkms_set_composer(output, true);
>  
>  	active_wb = conn_state->writeback_job->priv;
>  	wb_frame_info = &active_wb->wb_frame_info;
> @@ -162,22 +162,23 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
>  	.atomic_check = vkms_wb_atomic_check,
>  };
>  
> -int vkms_enable_writeback_connector(struct vkms_device *vkmsdev, struct drm_crtc *crtc)
> +int vkms_enable_writeback_connector(struct vkms_device *vkmsdev,
> +				    struct vkms_output *vkms_output)
>  {
> -	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> +	struct drm_writeback_connector *wb = &vkms_output->wb_connector;
>  	int ret;
>  
> -	ret = drmm_encoder_init(&vkmsdev->drm, &vkmsdev->output.wb_encoder,
> +	ret = drmm_encoder_init(&vkmsdev->drm, &vkms_output->wb_encoder,
>  				NULL, DRM_MODE_ENCODER_VIRTUAL, NULL);
>  	if (ret)
>  		return ret;
> -	vkmsdev->output.wb_encoder.possible_crtcs |= drm_crtc_mask(crtc);
> +	vkms_output->wb_encoder.possible_crtcs |= drm_crtc_mask(&vkms_output->crtc);
>  
>  	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
>  
>  	return drmm_writeback_connector_init(&vkmsdev->drm, wb,
>  					     &vkms_wb_connector_funcs,
> -					     &vkmsdev->output.wb_encoder,
> +					     &vkms_output->wb_encoder,
>  					     vkms_wb_formats,
>  					     ARRAY_SIZE(vkms_wb_formats));
>  }
> 

Re: [PATCH v4 3/3] drm/vkms: Switch to dynamic allocation for CRTC

[Louis Chauvet]

On 20/01/25 - 17:23, José Expósito wrote:
> > A specific allocation for the CRTC is not strictly necessary at this
> > point, but in order to implement dynamic configuration of VKMS (configFS),
> > it will be easier to have one allocation per CRTC.
> > 
> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---

[...]

> > +	/* Initialize the writeback component */
> >  	if (vkmsdev->config->writeback) {
> > -		writeback = vkms_enable_writeback_connector(vkmsdev, crtc);
> > +		writeback = vkms_enable_writeback_connector(vkmsdev, output);
> >  		if (writeback)
> >  			DRM_ERROR("Failed to init writeback connector\n");
> >  	}
> 
> Hi Louis,
> 
> Thanks for fixing this error condition.
> 
> I have been working and running automated tests on top of this series and
> I haven't found any other issue.
> 
> Reviewed-by: José Expósito <jose.exposito89@gmail.com>

Thanks a lot! I will merge this tomorrow.

What is your automated tests series? 

I will also send tomorrow a new rebased iteration for:
- https://patchwork.freedesktop.org/series/140786/
- https://patchwork.freedesktop.org/series/133698/
- https://patchwork.freedesktop.org/patch/625883/

If someone can look on them and leave some reviews, I will be very happy 
to apply them!

I will also send a first version of the configFS work (two distincts 
series to make the review easier). 

Thanks a lot,
Louis Chauvet

Re: [PATCH v4 0/3] drm/vkms: Switch to allocated for drm objects

[Louis Chauvet]

On 17/01/25 - 10:04, Louis Chauvet wrote:
> Specific allocations for each DRM object is not strictly needed in VKMS
> right now, but in order to implement dynamic configuration of VKMS
> (configFS), it will be easier to have one allocation per DRM object.
> 
> There is no need for a dynamic allocation for the writeback connector as
> there can only be one per CRTC
> 
> No functionnal changes are intented in this series.
> 
> This series requires [1] to switch vkms objects to drm-managed objects.
> 
> [1]:https://lore.kernel.org/all/20250116-google-vkms-managed-v9-0-3e4ae1bd05a0@bootlin.com/
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>

Applied on drm-misc-next, thanks.

Louis Chauvet

Re: [PATCH v4 3/3] drm/vkms: Switch to dynamic allocation for CRTC

[José Expósito]

On Mon, Jan 20, 2025 at 06:26:07PM +0100, Louis Chauvet wrote:
> On 20/01/25 - 17:23, José Expósito wrote:
> > > A specific allocation for the CRTC is not strictly necessary at this
> > > point, but in order to implement dynamic configuration of VKMS (configFS),
> > > it will be easier to have one allocation per CRTC.
> > > 
> > > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > > ---
> 
> [...]
> 
> > > +	/* Initialize the writeback component */
> > >  	if (vkmsdev->config->writeback) {
> > > -		writeback = vkms_enable_writeback_connector(vkmsdev, crtc);
> > > +		writeback = vkms_enable_writeback_connector(vkmsdev, output);
> > >  		if (writeback)
> > >  			DRM_ERROR("Failed to init writeback connector\n");
> > >  	}
> > 
> > Hi Louis,
> > 
> > Thanks for fixing this error condition.
> > 
> > I have been working and running automated tests on top of this series and
> > I haven't found any other issue.
> > 
> > Reviewed-by: José Expósito <jose.exposito89@gmail.com>
> 
> Thanks a lot! I will merge this tomorrow.
> 
> What is your automated tests series? 

On the kernel side, I keep working on the ConfigFS patches here:
https://github.com/JoseExposito/linux/commits/patch-vkms-configfs/

It sits on top of your work to switch to managed memory. But now that
the code is merged, it needs to be rebased.
You'll notice that I kept your signed-off-by in many patches, as I
tried to reuse as much common code as possible.

About the automated testing, the series could be split in two:
- vkms_config.h/c, which is tested with KUnit
- ConfigFS, tested with IGT:
  https://gitlab.freedesktop.org/jexposit/igt-gpu-tools/-/commits/vkms-configfs

I made some wrong assumptions with connectors, for example, it is
possible to create a device without connectors and hot-add/remove
them later, and I'm still fixing them and writing tests.
Once that work is done I'll send the series to the ML.

Jose

> I will also send tomorrow a new rebased iteration for:
> - https://patchwork.freedesktop.org/series/140786/
> - https://patchwork.freedesktop.org/series/133698/
> - https://patchwork.freedesktop.org/patch/625883/
> 
> If someone can look on them and leave some reviews, I will be very happy 
> to apply them!
> 
> I will also send a first version of the configFS work (two distincts 
> series to make the review easier). 
> 
> Thanks a lot,
> Louis Chauvet
> 

Re: [PATCH v4 3/3] drm/vkms: Switch to dynamic allocation for CRTC

[Louis Chauvet]

On 21/01/25 - 11:45, José Expósito wrote:
> On Mon, Jan 20, 2025 at 06:26:07PM +0100, Louis Chauvet wrote:
> > On 20/01/25 - 17:23, José Expósito wrote:
> > > > A specific allocation for the CRTC is not strictly necessary at this
> > > > point, but in order to implement dynamic configuration of VKMS (configFS),
> > > > it will be easier to have one allocation per CRTC.
> > > > 
> > > > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > > > ---
> > 
> > [...]
> > 
> > > > +	/* Initialize the writeback component */
> > > >  	if (vkmsdev->config->writeback) {
> > > > -		writeback = vkms_enable_writeback_connector(vkmsdev, crtc);
> > > > +		writeback = vkms_enable_writeback_connector(vkmsdev, output);
> > > >  		if (writeback)
> > > >  			DRM_ERROR("Failed to init writeback connector\n");
> > > >  	}
> > > 
> > > Hi Louis,
> > > 
> > > Thanks for fixing this error condition.
> > > 
> > > I have been working and running automated tests on top of this series and
> > > I haven't found any other issue.
> > > 
> > > Reviewed-by: José Expósito <jose.exposito89@gmail.com>
> > 
> > Thanks a lot! I will merge this tomorrow.
> > 
> > What is your automated tests series? 
> 
> On the kernel side, I keep working on the ConfigFS patches here:
> https://github.com/JoseExposito/linux/commits/patch-vkms-configfs/

I saw your message just after sending my "vkms-config" series, I am 
currently looking at your commits to see what are the changes.

I also sent an RFC for the ConfigFS interface, I did not change a lot of 
thing. I think the two major changes are:
- Adding format configuration
- Removing YUV related configuration (encoding/range)

> It sits on top of your work to switch to managed memory. But now that
> the code is merged, it needs to be rebased.
>
> You'll notice that I kept your signed-off-by in many patches, as I
> tried to reuse as much common code as possible.

Yes, thank you. I will compare with my work and understand your change.

> About the automated testing, the series could be split in two:

I agree, and indeed that what I meant in my previous mail :-)
I did not sent them earlier because I wanted to merge 
vkms-managed/allocated before.

> - vkms_config.h/c, which is tested with KUnit

Yes! Thank you for this, I wrote only very basic tests, so thank you for 
your extended kunit tests. 

> - ConfigFS, tested with IGT:
>   https://gitlab.freedesktop.org/jexposit/igt-gpu-tools/-/commits/vkms-configfs

You did a really great job writting those tests! Please CC me 
when you will send them, I will be very happy to review them.

> I made some wrong assumptions with connectors, for example, it is
> possible to create a device without connectors and hot-add/remove
> them later, and I'm still fixing them and writing tests.
> Once that work is done I'll send the series to the ML.

I did the same asumption, so my series is also broken on this point. I 
don't want to duplicate the work, so I'm looking forward to your series!

Thanks for this amazing work,
Louis Chauvet

> Jose
> 
> > I will also send tomorrow a new rebased iteration for:
> > - https://patchwork.freedesktop.org/series/140786/
> > - https://patchwork.freedesktop.org/series/133698/
> > - https://patchwork.freedesktop.org/patch/625883/
> > 
> > If someone can look on them and leave some reviews, I will be very happy 
> > to apply them!
> > 
> > I will also send a first version of the configFS work (two distincts 
> > series to make the review easier). 
> > 
> > Thanks a lot,
> > Louis Chauvet
> >