[PATCH v4 0/5] drm/vkms: Switch all vkms object to DRM managed objects

[PATCH v4 0/5] drm/vkms: Switch all vkms object to DRM managed objects

[Louis Chauvet]

To simplify the memory managment this series replace all manual drm 
object managment by drm-managed one. This way the VKMS code don't have to 
manage it directly and the DRM core will handle the object destruction.

No functional changes are intended in this series. This series depends on 
[2] (for some code cleanup, which conflict with this series).

PATCH 1/5: Migrate connector managment to drmm
PATCH 2/5: Migrate encoder managment to drmm
PATCH 3/5: Migrate connector management to drm
PATCH 4/5: Introduce drmm_writeback helpers
PATCH 5/5: Migrate writeback connector management to drm

[2]:https://lore.kernel.org/all/20241010-vkms-remove-index-v2-1-6b8d6cfd5a15@bootlin.com/

For the drmm_writeback helpers, you can find some discussions here [3].

[3]:https://lore.kernel.org/all/20240906-writeback-drmm-v1-1-01ede328182c@bootlin.com/

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
Changes in v4:
- No changes for the managed part
- Add the patch to introduce drmm_writeback helpers
- Link to v3: https://lore.kernel.org/r/20240912-google-vkms-managed-v3-0-7708d6ad262d@bootlin.com

Changes in v3:
- As suggested by Maxime, split the managed and the dynamic allocation 
  parts in different series
- To reduce the diff in this series, extract the "remove crtc index" part, 
  see https://lore.kernel.org/all/20240906-vkms-remove-index-v1-1-3cfedd8ccb2f@bootlin.com/
- Link to v2: https://lore.kernel.org/r/20240827-google-vkms-managed-v2-0-f41104553aeb@bootlin.com

Changes in v2:
- Applied comments from José
- Extract the rename vkms_output -> vkms_crtc to avoid useless changes in 
  the last commit
- Extract the rename to_vkms_crtc_state to
  drm_crtc_state_to_vkms_crtc_state to avoid useless changes in last 
  commit
- Extract the drm_mode_crtc_set_gamma_size result check in its own commit
- Rebased on drm-misc/drm-misc-next
- Link to v1: https://lore.kernel.org/r/20240814-google-vkms-managed-v1-0-7ab8b8921103@bootlin.com

---
Louis Chauvet (5):
      drm/vkms: Switch to managed for connector
      drm/vkms: Switch to managed for encoder
      drm/vkms: Switch to managed for crtc
      drm: writeback: Introduce drm managed helpers
      drm/vkms: Switch to managed for writeback connector

 drivers/gpu/drm/drm_connector.c       |   4 +
 drivers/gpu/drm/drm_writeback.c       | 224 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_crtc.c      |  14 +++
 drivers/gpu/drm/vkms/vkms_drv.c       |   9 --
 drivers/gpu/drm/vkms/vkms_output.c    |  31 ++---
 drivers/gpu/drm/vkms/vkms_writeback.c |  13 +-
 include/drm/drm_writeback.h           |  10 ++
 7 files changed, 238 insertions(+), 67 deletions(-)
---
base-commit: 33c255312660653cf54f8019896b5dca28e3c580
change-id: 20240521-google-vkms-managed-4aec99461a77
prerequisite-message-id: <20241010-vkms-remove-index-v2-1-6b8d6cfd5a15@bootlin.com>
prerequisite-patch-id: 920c23497fc5bd2fdf1dded06ce198c227ea0ef9

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

[PATCH v4 1/5] drm/vkms: Switch to managed for connector

[Louis Chauvet]

The current VKMS driver uses non-managed function to create connectors. It
is not an issue yet, but in order to support multiple devices easily,
convert this code to use drm and device managed helpers.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_output.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 5128aa3b2eb6..8f7a05b73e1d 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -3,11 +3,11 @@
 #include "vkms_drv.h"
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_probe_helper.h>
 
 static const struct drm_connector_funcs vkms_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = drm_connector_cleanup,
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
@@ -70,17 +70,17 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 			if (IS_ERR(overlay)) {
 				DRM_DEV_ERROR(dev->dev, "Failed to init vkms plane\n");
 				ret = PTR_ERR(overlay);
-				goto err_crtc;
+				goto err_connector;
 			}
 			overlay->base.possible_crtcs = drm_crtc_mask(crtc);
 		}
 	}
 
-	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
-				 DRM_MODE_CONNECTOR_VIRTUAL);
+	ret = drmm_connector_init(dev, connector, &vkms_connector_funcs,
+				  DRM_MODE_CONNECTOR_VIRTUAL, NULL);
 	if (ret) {
 		DRM_ERROR("Failed to init connector\n");
-		goto err_crtc;
+		goto err_connector;
 	}
 
 	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
@@ -89,7 +89,7 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 			       DRM_MODE_ENCODER_VIRTUAL, NULL);
 	if (ret) {
 		DRM_ERROR("Failed to init encoder\n");
-		goto err_encoder;
+		return ret;
 	}
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
@@ -111,12 +111,7 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 
 err_attach:
 	drm_encoder_cleanup(encoder);
-
-err_encoder:
-	drm_connector_cleanup(connector);
-
-err_crtc:
+err_connector:
 	drm_crtc_cleanup(crtc);
-
 	return ret;
 }

-- 
2.46.2

Re: [PATCH v4 1/5] drm/vkms: Switch to managed for connector

[Maíra Canal]

Hi Louis,

On 10/10/24 14:39, Louis Chauvet wrote:
> The current VKMS driver uses non-managed function to create connectors. It
> is not an issue yet, but in order to support multiple devices easily,
> convert this code to use drm and device managed helpers.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>   drivers/gpu/drm/vkms/vkms_output.c | 19 +++++++------------
>   1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 5128aa3b2eb6..8f7a05b73e1d 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -3,11 +3,11 @@
>   #include "vkms_drv.h"
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_edid.h>
> +#include <drm/drm_managed.h>
>   #include <drm/drm_probe_helper.h>
>   
>   static const struct drm_connector_funcs vkms_connector_funcs = {
>   	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.destroy = drm_connector_cleanup,
>   	.reset = drm_atomic_helper_connector_reset,
>   	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>   	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> @@ -70,17 +70,17 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>   			if (IS_ERR(overlay)) {
>   				DRM_DEV_ERROR(dev->dev, "Failed to init vkms plane\n");
>   				ret = PTR_ERR(overlay);
> -				goto err_crtc;
> +				goto err_connector;

Why did you renamed err_crtc to err_connector? I think err_crtc looks
correct.

Best Regards,
- Maíra

>   			}
>   			overlay->base.possible_crtcs = drm_crtc_mask(crtc);
>   		}
>   	}
>   
> -	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> -				 DRM_MODE_CONNECTOR_VIRTUAL);
> +	ret = drmm_connector_init(dev, connector, &vkms_connector_funcs,
> +				  DRM_MODE_CONNECTOR_VIRTUAL, NULL);
>   	if (ret) {
>   		DRM_ERROR("Failed to init connector\n");
> -		goto err_crtc;
> +		goto err_connector;
>   	}
>   
>   	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
> @@ -89,7 +89,7 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>   			       DRM_MODE_ENCODER_VIRTUAL, NULL);
>   	if (ret) {
>   		DRM_ERROR("Failed to init encoder\n");
> -		goto err_encoder;
> +		return ret;
>   	}
>   	encoder->possible_crtcs = drm_crtc_mask(crtc);
>   
> @@ -111,12 +111,7 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>   
>   err_attach:
>   	drm_encoder_cleanup(encoder);
> -
> -err_encoder:
> -	drm_connector_cleanup(connector);
> -
> -err_crtc:
> +err_connector:
>   	drm_crtc_cleanup(crtc);
> -
>   	return ret;
>   }
> 

Re: [PATCH v4 1/5] drm/vkms: Switch to managed for connector

[Louis Chauvet]

On 26/10/24 - 12:29, Maíra Canal wrote:
> Hi Louis,
> 
> On 10/10/24 14:39, Louis Chauvet wrote:
> > The current VKMS driver uses non-managed function to create connectors. It
> > is not an issue yet, but in order to support multiple devices easily,
> > convert this code to use drm and device managed helpers.
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >   drivers/gpu/drm/vkms/vkms_output.c | 19 +++++++------------
> >   1 file changed, 7 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 5128aa3b2eb6..8f7a05b73e1d 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -3,11 +3,11 @@
> >   #include "vkms_drv.h"
> >   #include <drm/drm_atomic_helper.h>
> >   #include <drm/drm_edid.h>
> > +#include <drm/drm_managed.h>
> >   #include <drm/drm_probe_helper.h>
> >   static const struct drm_connector_funcs vkms_connector_funcs = {
> >   	.fill_modes = drm_helper_probe_single_connector_modes,
> > -	.destroy = drm_connector_cleanup,
> >   	.reset = drm_atomic_helper_connector_reset,
> >   	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> >   	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > @@ -70,17 +70,17 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> >   			if (IS_ERR(overlay)) {
> >   				DRM_DEV_ERROR(dev->dev, "Failed to init vkms plane\n");
> >   				ret = PTR_ERR(overlay);
> > -				goto err_crtc;
> > +				goto err_connector;
> 
> Why did you renamed err_crtc to err_connector? I think err_crtc looks
> correct.

I rename it many times during my work, it was never clear for me if 
"err_crtc" is about an error during the CRTC initialization or a label to 
clean the crtc.

If for you err_crtc is correct (ie err_<thing> means "cleanup <thing>"), I 
will switch to this pattern.

Thanks,
Louis Chauvet
 
> Best Regards,
> - Maíra
> 
> >   			}
> >   			overlay->base.possible_crtcs = drm_crtc_mask(crtc);
> >   		}
> >   	}
> > -	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> > -				 DRM_MODE_CONNECTOR_VIRTUAL);
> > +	ret = drmm_connector_init(dev, connector, &vkms_connector_funcs,
> > +				  DRM_MODE_CONNECTOR_VIRTUAL, NULL);
> >   	if (ret) {
> >   		DRM_ERROR("Failed to init connector\n");
> > -		goto err_crtc;
> > +		goto err_connector;
> >   	}
> >   	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
> > @@ -89,7 +89,7 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> >   			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> >   	if (ret) {
> >   		DRM_ERROR("Failed to init encoder\n");
> > -		goto err_encoder;
> > +		return ret;
> >   	}
> >   	encoder->possible_crtcs = drm_crtc_mask(crtc);
> > @@ -111,12 +111,7 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> >   err_attach:
> >   	drm_encoder_cleanup(encoder);
> > -
> > -err_encoder:
> > -	drm_connector_cleanup(connector);
> > -
> > -err_crtc:
> > +err_connector:
> >   	drm_crtc_cleanup(crtc);
> > -
> >   	return ret;
> >   }
> > 

[PATCH v4 2/5] drm/vkms: Switch to managed for encoder

[Louis Chauvet]

The current VKMS driver uses non-managed function to create encoders. It
is not an issue yet, but in order to support multiple devices easily,
convert this code to use drm and device managed helpers.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_output.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 8f7a05b73e1d..c878b2843d05 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -13,10 +13,6 @@ static const struct drm_connector_funcs vkms_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static const struct drm_encoder_funcs vkms_encoder_funcs = {
-	.destroy = drm_encoder_cleanup,
-};
-
 static int vkms_conn_get_modes(struct drm_connector *connector)
 {
 	int count;
@@ -85,18 +81,18 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 
 	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
 
-	ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
-			       DRM_MODE_ENCODER_VIRTUAL, NULL);
+	ret = drmm_encoder_init(dev, encoder, NULL,
+				DRM_MODE_ENCODER_VIRTUAL, NULL);
 	if (ret) {
 		DRM_ERROR("Failed to init encoder\n");
-		return ret;
+		goto err_connector;
 	}
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret) {
 		DRM_ERROR("Failed to attach connector to encoder\n");
-		goto err_attach;
+		return ret;
 	}
 
 	if (vkmsdev->config->writeback) {
@@ -109,8 +105,6 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 
 	return 0;
 
-err_attach:
-	drm_encoder_cleanup(encoder);
 err_connector:
 	drm_crtc_cleanup(crtc);
 	return ret;

-- 
2.46.2

[PATCH v4 3/5] drm/vkms: Switch to managed for crtc

[Louis Chauvet]

The current VKMS driver uses managed function to create crtc, but
don't use it to properly clean the crtc workqueue. It is not an
issue yet, but in order to support multiple devices easily,
convert this code to use drm and device managed helpers.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_crtc.c | 14 ++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.c  |  9 ---------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index a40295c18b48..877877dc0af1 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -6,6 +6,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
+#include <drm/drm_managed.h>
 
 #include "vkms_drv.h"
 
@@ -274,6 +275,14 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
 	.atomic_disable	= vkms_crtc_atomic_disable,
 };
 
+static void vkms_crtc_destroy_workqueue(struct drm_device *dev,
+					void *raw_vkms_out)
+{
+	struct vkms_output *vkms_out = raw_vkms_out;
+
+	destroy_workqueue(vkms_out->composer_workq);
+}
+
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor)
 {
@@ -304,5 +313,10 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 	if (!vkms_out->composer_workq)
 		return -ENOMEM;
 
+	ret = drmm_add_action_or_reset(dev, vkms_crtc_destroy_workqueue,
+				       vkms_out);
+	if (ret)
+		return ret;
+
 	return ret;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 0f6805b9fe7b..eede3b8c70ef 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -54,14 +54,6 @@ MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
 
 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);
-
-	if (vkms->output.composer_workq)
-		destroy_workqueue(vkms->output.composer_workq);
-}
-
 static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
@@ -110,7 +102,6 @@ static const struct drm_debugfs_info vkms_config_debugfs_list[] = {
 
 static const struct drm_driver vkms_driver = {
 	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
-	.release		= vkms_release,
 	.fops			= &vkms_driver_fops,
 	DRM_GEM_SHMEM_DRIVER_OPS,
 	DRM_FBDEV_SHMEM_DRIVER_OPS,

-- 
2.46.2

[PATCH v4 4/5] drm: writeback: Introduce drm managed helpers

[Louis Chauvet]

Currently drm_writeback_connector are created by
drm_writeback_connector_init or drm_writeback_connector_init_with_encoder.
Both of the function uses drm_connector_init and drm_encoder_init, but
there is no way to properly clean those structure from outside. By using
drm managed variants, we can ensure that the writeback connector is
properly cleaned.

This patch introduce drmm_writeback_connector_init, an helper to initialize
a writeback connector using drm managed helpers. This function allows the
caller to use its own encoder.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/drm_connector.c |   4 +
 drivers/gpu/drm/drm_writeback.c | 224 ++++++++++++++++++++++++++++++++++------
 include/drm/drm_writeback.h     |  10 ++
 3 files changed, 208 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index fc35f47e2849..fe4c1967860a 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -613,6 +613,7 @@ static void drm_mode_remove(struct drm_connector *connector,
 	drm_mode_destroy(connector->dev, mode);
 }
 
+void drm_writeback_connector_cleanup(struct drm_device *dev, void *data);
 /**
  * drm_connector_cleanup - cleans up an initialised connector
  * @connector: connector to cleanup
@@ -631,6 +632,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
 		    DRM_CONNECTOR_REGISTERED))
 		drm_connector_unregister(connector);
 
+	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
+		drm_writeback_connector_cleanup(dev, connector);
+
 	if (connector->privacy_screen) {
 		drm_privacy_screen_put(connector->privacy_screen);
 		connector->privacy_screen = NULL;
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 33a3c98a962d..28f299ce8e10 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -15,6 +15,7 @@
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_framebuffer.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_property.h>
 #include <drm/drm_writeback.h>
@@ -196,13 +197,12 @@ int drm_writeback_connector_init(struct drm_device *dev,
 EXPORT_SYMBOL(drm_writeback_connector_init);
 
 /**
- * drm_writeback_connector_init_with_encoder - Initialize a writeback connector with
- * a custom encoder
+ * __drm_writeback_connector_init - Common initialization code for writeback
+ * connector
  *
  * @dev: DRM device
  * @wb_connector: Writeback connector to initialize
  * @enc: handle to the already initialized drm encoder
- * @con_funcs: Connector funcs vtable
  * @formats: Array of supported pixel formats for the writeback engine
  * @n_formats: Length of the formats array
  *
@@ -218,41 +218,32 @@ EXPORT_SYMBOL(drm_writeback_connector_init);
  * assigning the encoder helper functions, possible_crtcs and any other encoder
  * specific operation.
  *
- * Drivers should always use this function instead of drm_connector_init() to
- * set up writeback connectors if they want to manage themselves the lifetime of the
- * associated encoder.
- *
  * Returns: 0 on success, or a negative error code
  */
-int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
-		struct drm_writeback_connector *wb_connector, struct drm_encoder *enc,
-		const struct drm_connector_funcs *con_funcs, const u32 *formats,
-		int n_formats)
+static int __drm_writeback_connector_init(struct drm_device *dev,
+					  struct drm_writeback_connector *wb_connector,
+					  struct drm_encoder *enc,
+					  const u32 *formats, int n_formats)
 {
-	struct drm_property_blob *blob;
 	struct drm_connector *connector = &wb_connector->base;
 	struct drm_mode_config *config = &dev->mode_config;
-	int ret = create_writeback_properties(dev);
-
-	if (ret != 0)
-		return ret;
-
-	blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
-					formats);
-	if (IS_ERR(blob))
-		return PTR_ERR(blob);
-
+	struct drm_property_blob *blob;
+	int ret;
 
 	connector->interlace_allowed = 0;
 
-	ret = drm_connector_init(dev, connector, con_funcs,
-				 DRM_MODE_CONNECTOR_WRITEBACK);
+	ret = create_writeback_properties(dev);
 	if (ret)
-		goto connector_fail;
+		return ret;
 
 	ret = drm_connector_attach_encoder(connector, enc);
 	if (ret)
-		goto attach_fail;
+		return ret;
+
+	blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
+					formats);
+	if (IS_ERR(blob))
+		return PTR_ERR(blob);
 
 	INIT_LIST_HEAD(&wb_connector->job_queue);
 	spin_lock_init(&wb_connector->job_lock);
@@ -275,15 +266,188 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
 	wb_connector->pixel_formats_blob_ptr = blob;
 
 	return 0;
+}
+
+/**
+ * drm_writeback_connector_init_with_encoder - Initialize a writeback connector with
+ * a custom encoder
+ *
+ * @dev: DRM device
+ * @wb_connector: Writeback connector to initialize
+ * @enc: handle to the already initialized drm encoder
+ * @con_funcs: Connector funcs vtable
+ * @formats: Array of supported pixel formats for the writeback engine
+ * @n_formats: Length of the formats array
+ *
+ * This function creates the writeback-connector-specific properties if they
+ * have not been already created, initializes the connector as
+ * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
+ * values.
+ *
+ * This function assumes that the drm_writeback_connector's encoder has already been
+ * created and initialized before invoking this function.
+ *
+ * In addition, this function also assumes that callers of this API will manage
+ * assigning the encoder helper functions, possible_crtcs and any other encoder
+ * specific operation.
+ *
+ * Drivers should always use this function instead of drm_connector_init() to
+ * set up writeback connectors if they want to manage themselves the lifetime of the
+ * associated encoder.
+ *
+ * Returns: 0 on success, or a negative error code
+ */
+int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
+					      struct drm_writeback_connector *wb_connector,
+					      struct drm_encoder *enc,
+					      const struct drm_connector_funcs *con_funcs,
+					      const u32 *formats, int n_formats)
+{
+	struct drm_connector *connector = &wb_connector->base;
+	int ret;
+
+	ret = drm_connector_init(dev, connector, con_funcs,
+				 DRM_MODE_CONNECTOR_WRITEBACK);
+	if (ret)
+		return ret;
+
+	ret = __drm_writeback_connector_init(dev, wb_connector, enc, formats,
+					     n_formats);
+	if (ret)
+		drm_connector_cleanup(connector);
 
-attach_fail:
-	drm_connector_cleanup(connector);
-connector_fail:
-	drm_property_blob_put(blob);
 	return ret;
 }
 EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);
 
+/**
+ * drm_writeback_connector_cleanup - Cleanup the writeback connector
+ * @dev: DRM device
+ * @data: Opaque pointer to the connector
+ *
+ * This will decrement the reference counter of blobs and it will clean the
+ * remaining jobs in this writeback connector.
+ */
+void drm_writeback_connector_cleanup(struct drm_device *dev, void *data)
+{
+	struct drm_connector *connector = data;
+	struct drm_writeback_connector *wb_connector = container_of(connector,
+								    struct drm_writeback_connector,
+								    base);
+	unsigned long flags;
+	struct drm_writeback_job *pos, *n;
+
+	drm_property_blob_put(wb_connector->pixel_formats_blob_ptr);
+
+	spin_lock_irqsave(&wb_connector->job_lock, flags);
+	list_for_each_entry_safe(pos, n, &wb_connector->job_queue, list_entry) {
+		drm_writeback_cleanup_job(pos);
+		list_del(&pos->list_entry);
+	}
+	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
+}
+
+/**
+ * __drmm_writeback_connector_init - Initialize a writeback connector with
+ * a custom encoder
+ *
+ * @dev: DRM device
+ * @wb_connector: Writeback connector to initialize
+ * @con_funcs: Connector funcs vtable
+ * @enc: handle to the already initialized drm encoder
+ * @formats: Array of supported pixel formats for the writeback engine
+ * @n_formats: Length of the formats array
+ *
+ * This function initialize a writeback connector and register its cleanup.
+ * It uses the common helper @__drm_writeback_connector_init to do the
+ * general initialization.
+ *
+ * This function assumes that @enc has already been created and initialized
+ * before invoking this function.
+ *
+ * In addition, this function also assumes that callers of this API will manage
+ * assigning the encoder helper functions, possible_crtcs and any other encoder
+ * specific operation.
+ *
+ * Returns: 0 on success, or a negative error code
+ */
+static int __drmm_writeback_connector_init(struct drm_device *dev,
+					   struct drm_writeback_connector *wb_connector,
+					   const struct drm_connector_funcs *con_funcs,
+					   struct drm_encoder *enc,
+					   const u32 *formats, int n_formats)
+{
+	struct drm_connector *connector = &wb_connector->base;
+	int ret;
+
+	ret = drmm_connector_init(dev, connector, con_funcs,
+				  DRM_MODE_CONNECTOR_WRITEBACK, NULL);
+	if (ret)
+		return ret;
+
+	ret = __drm_writeback_connector_init(dev, wb_connector, enc, formats,
+					     n_formats);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/**
+ * drmm_writeback_connector_init - Initialize a writeback connector with
+ * a custom encoder
+ *
+ * @dev: DRM device
+ * @wb_connector: Writeback connector to initialize
+ * @con_funcs: Connector funcs vtable
+ * @enc: handle to the already initialized drm encoder, optional
+ * @enc_funcs: Encoder funcs vtable, optional, only used when @enc is NULL
+ * @formats: Array of supported pixel formats for the writeback engine
+ * @n_formats: Length of the formats array
+ * @possible_crtcs: if @enc is NULL, this will set the possible_crtc for the
+ *		    newly created encoder
+ *
+ * This function initialize a writeback connector and register its cleanup.
+ *
+ * This function creates the writeback-connector-specific properties if they
+ * have not been already created, initializes the connector as
+ * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
+ * values.
+ *
+ * If @enc is NULL, this function will create a drm-managed encoder and will
+ * attach @enc_funcs on it. It will also attach the CRTC passed in
+ * @possible_crtcs
+ *
+ * Returns: 0 on success, or a negative error code
+ */
+int drmm_writeback_connector_init(struct drm_device *dev,
+				  struct drm_writeback_connector *wb_connector,
+				  const struct drm_connector_funcs *con_funcs,
+				  struct drm_encoder *enc,
+				  const struct drm_encoder_helper_funcs *enc_funcs,
+				  const u32 *formats, int n_formats,
+				  u32 possible_crtcs)
+{
+	int ret;
+
+	if (!enc) {
+		ret = drmm_encoder_init(dev, &wb_connector->encoder,
+					NULL, DRM_MODE_ENCODER_VIRTUAL, NULL);
+		if (ret)
+			return ret;
+
+		enc = &wb_connector->encoder;
+		enc->possible_crtcs |= possible_crtcs;
+		if (enc_funcs)
+			drm_encoder_helper_add(enc, enc_funcs);
+	}
+
+	return __drmm_writeback_connector_init(dev, wb_connector, con_funcs,
+					       &wb_connector->encoder, formats,
+					       n_formats);
+}
+EXPORT_SYMBOL(drmm_writeback_connector_init);
+
 int drm_writeback_set_fb(struct drm_connector_state *conn_state,
 			 struct drm_framebuffer *fb)
 {
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 17e576c80169..5e5ff8dd9d9d 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -161,6 +161,14 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
 				const struct drm_connector_funcs *con_funcs, const u32 *formats,
 				int n_formats);
 
+int drmm_writeback_connector_init(struct drm_device *dev,
+				  struct drm_writeback_connector *wb_connector,
+				  const struct drm_connector_funcs *con_funcs,
+				  struct drm_encoder *enc,
+				  const struct drm_encoder_helper_funcs *enc_funcs,
+				  const u32 *formats, int n_formats,
+				  u32 possible_crtcs);
+
 int drm_writeback_set_fb(struct drm_connector_state *conn_state,
 			 struct drm_framebuffer *fb);
 
@@ -175,6 +183,8 @@ void
 drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector,
 				int status);
 
+void drm_writeback_connector_cleanup(struct drm_device *dev, void *data);
+
 struct dma_fence *
 drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector);
 #endif

-- 
2.46.2

Re: [PATCH v4 4/5] drm: writeback: Introduce drm managed helpers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2849 bytes --]

Hi,

On Thu, Oct 10, 2024 at 07:39:06PM +0200, Louis Chauvet wrote:
> Currently drm_writeback_connector are created by
> drm_writeback_connector_init or drm_writeback_connector_init_with_encoder.
> Both of the function uses drm_connector_init and drm_encoder_init, but
> there is no way to properly clean those structure from outside. By using
> drm managed variants, we can ensure that the writeback connector is
> properly cleaned.
> 
> This patch introduce drmm_writeback_connector_init, an helper to initialize
> a writeback connector using drm managed helpers. This function allows the
> caller to use its own encoder.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>  drivers/gpu/drm/drm_connector.c |   4 +
>  drivers/gpu/drm/drm_writeback.c | 224 ++++++++++++++++++++++++++++++++++------
>  include/drm/drm_writeback.h     |  10 ++
>  3 files changed, 208 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index fc35f47e2849..fe4c1967860a 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -613,6 +613,7 @@ static void drm_mode_remove(struct drm_connector *connector,
>  	drm_mode_destroy(connector->dev, mode);
>  }
>  
> +void drm_writeback_connector_cleanup(struct drm_device *dev, void *data);
>  /**
>   * drm_connector_cleanup - cleans up an initialised connector
>   * @connector: connector to cleanup
> @@ -631,6 +632,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  		    DRM_CONNECTOR_REGISTERED))
>  		drm_connector_unregister(connector);
>  
> +	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
> +		drm_writeback_connector_cleanup(dev, connector);
> +

So I think it should live in its own patch.

You're doing multiple things here. There's a) the bug that writeback
connectors aren't built properly, b) the discussion about how it's best to
clean it up, and c) how to make every driver clean up properly.

AFAIU, you're trying to address a and c here.

I think putting that call in drm_connector_cleanup is backward compared
to the pattern we're using in the rest of DRM.

drm_connector_cleanup should just clean what was allocated by
drm_connector_init, and that's it.

So we should create a drm_writeback_connector_cleanup function to
address a). That should be your first patch.

Now, it would indeed be best if drm_writeback_connector_cleanup didn't
need to be called at all. That's the second part of your patch, and
should be in its own patch as well. It would address b).

And finally, addressing c will require some driver changes, to either a
call to drmm_writeback_connector_init_* or by using
drm_writeback_connector_cleanup, but we'll have to make that change in
every driver.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[PATCH v4 5/5] drm/vkms: Switch to managed for writeback connector

[Louis Chauvet]

The current VKMS driver uses non-managed function to create
writeback connectors. It is not an issue yet, but in order
to support multiple devices easily, convert this code to
use drm and device managed helpers.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_writeback.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index bc724cbd5e3a..a948f4598764 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -24,7 +24,6 @@ static const u32 vkms_wb_formats[] = {
 
 static const struct drm_connector_funcs vkms_wb_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = drm_connector_cleanup,
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
@@ -174,10 +173,10 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
 
 	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
 
-	return drm_writeback_connector_init(&vkmsdev->drm, wb,
-					    &vkms_wb_connector_funcs,
-					    NULL,
-					    vkms_wb_formats,
-					    ARRAY_SIZE(vkms_wb_formats),
-					    1);
+	return drmm_writeback_connector_init(&vkmsdev->drm, wb,
+					     &vkms_wb_connector_funcs,
+					     NULL, NULL,
+					     vkms_wb_formats,
+					     ARRAY_SIZE(vkms_wb_formats),
+					     1);
 }

-- 
2.46.2

Re: [PATCH v4 0/5] drm/vkms: Switch all vkms object to DRM managed objects

[Maíra Canal]

Hi Louis,

How do you feel about adding the patch [1] to this series? This will
avoid issues when reviewing and pushing the series?

[1] 
https://lore.kernel.org/all/20241010-vkms-remove-index-v2-1-6b8d6cfd5a15@bootlin.com/

Best Regards,
- Maíra

On 10/10/24 14:39, Louis Chauvet wrote:
> To simplify the memory managment this series replace all manual drm
> object managment by drm-managed one. This way the VKMS code don't have to
> manage it directly and the DRM core will handle the object destruction.
> 
> No functional changes are intended in this series. This series depends on
> [2] (for some code cleanup, which conflict with this series).
> 
> PATCH 1/5: Migrate connector managment to drmm
> PATCH 2/5: Migrate encoder managment to drmm
> PATCH 3/5: Migrate connector management to drm
> PATCH 4/5: Introduce drmm_writeback helpers
> PATCH 5/5: Migrate writeback connector management to drm
> 
> [2]:https://lore.kernel.org/all/20241010-vkms-remove-index-v2-1-6b8d6cfd5a15@bootlin.com/
> 
> For the drmm_writeback helpers, you can find some discussions here [3].
> 
> [3]:https://lore.kernel.org/all/20240906-writeback-drmm-v1-1-01ede328182c@bootlin.com/
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> Changes in v4:
> - No changes for the managed part
> - Add the patch to introduce drmm_writeback helpers
> - Link to v3: https://lore.kernel.org/r/20240912-google-vkms-managed-v3-0-7708d6ad262d@bootlin.com
> 
> Changes in v3:
> - As suggested by Maxime, split the managed and the dynamic allocation
>    parts in different series
> - To reduce the diff in this series, extract the "remove crtc index" part,
>    see https://lore.kernel.org/all/20240906-vkms-remove-index-v1-1-3cfedd8ccb2f@bootlin.com/
> - Link to v2: https://lore.kernel.org/r/20240827-google-vkms-managed-v2-0-f41104553aeb@bootlin.com
> 
> Changes in v2:
> - Applied comments from José
> - Extract the rename vkms_output -> vkms_crtc to avoid useless changes in
>    the last commit
> - Extract the rename to_vkms_crtc_state to
>    drm_crtc_state_to_vkms_crtc_state to avoid useless changes in last
>    commit
> - Extract the drm_mode_crtc_set_gamma_size result check in its own commit
> - Rebased on drm-misc/drm-misc-next
> - Link to v1: https://lore.kernel.org/r/20240814-google-vkms-managed-v1-0-7ab8b8921103@bootlin.com
> 
> ---
> Louis Chauvet (5):
>        drm/vkms: Switch to managed for connector
>        drm/vkms: Switch to managed for encoder
>        drm/vkms: Switch to managed for crtc
>        drm: writeback: Introduce drm managed helpers
>        drm/vkms: Switch to managed for writeback connector
> 
>   drivers/gpu/drm/drm_connector.c       |   4 +
>   drivers/gpu/drm/drm_writeback.c       | 224 +++++++++++++++++++++++++++++-----
>   drivers/gpu/drm/vkms/vkms_crtc.c      |  14 +++
>   drivers/gpu/drm/vkms/vkms_drv.c       |   9 --
>   drivers/gpu/drm/vkms/vkms_output.c    |  31 ++---
>   drivers/gpu/drm/vkms/vkms_writeback.c |  13 +-
>   include/drm/drm_writeback.h           |  10 ++
>   7 files changed, 238 insertions(+), 67 deletions(-)
> ---
> base-commit: 33c255312660653cf54f8019896b5dca28e3c580
> change-id: 20240521-google-vkms-managed-4aec99461a77
> prerequisite-message-id: <20241010-vkms-remove-index-v2-1-6b8d6cfd5a15@bootlin.com>
> prerequisite-patch-id: 920c23497fc5bd2fdf1dded06ce198c227ea0ef9
> 
> Best regards,

Re: [PATCH v4 0/5] drm/vkms: Switch all vkms object to DRM managed objects

[Louis Chauvet]

On 26/10/24 - 12:33, Maíra Canal wrote:
> Hi Louis,
> 
> How do you feel about adding the patch [1] to this series? This will
> avoid issues when reviewing and pushing the series?

Ack!

Thanks,
Louis Chauvet
 
> [1] https://lore.kernel.org/all/20241010-vkms-remove-index-v2-1-6b8d6cfd5a15@bootlin.com/
> 
> Best Regards,
> - Maíra
> 
> On 10/10/24 14:39, Louis Chauvet wrote:
> > To simplify the memory managment this series replace all manual drm
> > object managment by drm-managed one. This way the VKMS code don't have to
> > manage it directly and the DRM core will handle the object destruction.
> > 
> > No functional changes are intended in this series. This series depends on
> > [2] (for some code cleanup, which conflict with this series).
> > 
> > PATCH 1/5: Migrate connector managment to drmm
> > PATCH 2/5: Migrate encoder managment to drmm
> > PATCH 3/5: Migrate connector management to drm
> > PATCH 4/5: Introduce drmm_writeback helpers
> > PATCH 5/5: Migrate writeback connector management to drm
> > 
> > [2]:https://lore.kernel.org/all/20241010-vkms-remove-index-v2-1-6b8d6cfd5a15@bootlin.com/
> > 
> > For the drmm_writeback helpers, you can find some discussions here [3].
> > 
> > [3]:https://lore.kernel.org/all/20240906-writeback-drmm-v1-1-01ede328182c@bootlin.com/
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> > Changes in v4:
> > - No changes for the managed part
> > - Add the patch to introduce drmm_writeback helpers
> > - Link to v3: https://lore.kernel.org/r/20240912-google-vkms-managed-v3-0-7708d6ad262d@bootlin.com
> > 
> > Changes in v3:
> > - As suggested by Maxime, split the managed and the dynamic allocation
> >    parts in different series
> > - To reduce the diff in this series, extract the "remove crtc index" part,
> >    see https://lore.kernel.org/all/20240906-vkms-remove-index-v1-1-3cfedd8ccb2f@bootlin.com/
> > - Link to v2: https://lore.kernel.org/r/20240827-google-vkms-managed-v2-0-f41104553aeb@bootlin.com
> > 
> > Changes in v2:
> > - Applied comments from José
> > - Extract the rename vkms_output -> vkms_crtc to avoid useless changes in
> >    the last commit
> > - Extract the rename to_vkms_crtc_state to
> >    drm_crtc_state_to_vkms_crtc_state to avoid useless changes in last
> >    commit
> > - Extract the drm_mode_crtc_set_gamma_size result check in its own commit
> > - Rebased on drm-misc/drm-misc-next
> > - Link to v1: https://lore.kernel.org/r/20240814-google-vkms-managed-v1-0-7ab8b8921103@bootlin.com
> > 
> > ---
> > Louis Chauvet (5):
> >        drm/vkms: Switch to managed for connector
> >        drm/vkms: Switch to managed for encoder
> >        drm/vkms: Switch to managed for crtc
> >        drm: writeback: Introduce drm managed helpers
> >        drm/vkms: Switch to managed for writeback connector
> > 
> >   drivers/gpu/drm/drm_connector.c       |   4 +
> >   drivers/gpu/drm/drm_writeback.c       | 224 +++++++++++++++++++++++++++++-----
> >   drivers/gpu/drm/vkms/vkms_crtc.c      |  14 +++
> >   drivers/gpu/drm/vkms/vkms_drv.c       |   9 --
> >   drivers/gpu/drm/vkms/vkms_output.c    |  31 ++---
> >   drivers/gpu/drm/vkms/vkms_writeback.c |  13 +-
> >   include/drm/drm_writeback.h           |  10 ++
> >   7 files changed, 238 insertions(+), 67 deletions(-)
> > ---
> > base-commit: 33c255312660653cf54f8019896b5dca28e3c580
> > change-id: 20240521-google-vkms-managed-4aec99461a77
> > prerequisite-message-id: <20241010-vkms-remove-index-v2-1-6b8d6cfd5a15@bootlin.com>
> > prerequisite-patch-id: 920c23497fc5bd2fdf1dded06ce198c227ea0ef9
> > 
> > Best regards,
>