[PATCH v2 0/6] drm/vkms: Switch all vkms object to DRM managed objects

[PATCH v2 0/6] drm/vkms: Switch all vkms object to DRM managed objects

[Louis Chauvet]

To simplify the memory managment this series replace all allocation 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.

PATCH 1/6: Migrate connector managment to drm
PATCH 2/6: Migrate encoder managment to drm
PATCH 3/6: Rename vkms_output to vkms_crtc
PATCH 4/6: Rename to_vkms_crtc_state
PATCH 5/6: Migrate connector management to drm
PATCH 6/6: Add missing check in CRTC initialization

This series have conflicts with [1], which adds documentation for the 
vkms_output/vkms_crtc structure. when one of those series is merged, I 
will rebase the other.

[1]: https://lore.kernel.org/all/20240826-google-clarifications-v2-1-2574655b0b91@bootlin.com/

Signed-off-by: Louis Chauvet <louis.chauvet@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 (6):
      drm/vkms: Switch to managed for connector
      drm/vkms: Switch to managed for encoder
      drm/vkms: Rename vkms_output to vkms_crtc
      drm/vkms: rename to_vkms_crtc_state to drm_crtc_state_to_vkms_crtc_state  to avoid confusion
      drm/vkms: Switch to managed for CRTC
      drm/vkms: Add missing check for CRTC initialization

 drivers/gpu/drm/vkms/vkms_composer.c  |  30 +++++-----
 drivers/gpu/drm/vkms/vkms_crtc.c      |  97 ++++++++++++++++++--------------
 drivers/gpu/drm/vkms/vkms_drv.c       |  11 +---
 drivers/gpu/drm/vkms/vkms_drv.h       |  21 +++----
 drivers/gpu/drm/vkms/vkms_output.c    | 101 ++++++++++++++++------------------
 drivers/gpu/drm/vkms/vkms_writeback.c |  22 ++++----
 6 files changed, 138 insertions(+), 144 deletions(-)
---
base-commit: 071d583e01c88272f6ff216d4f867f8f35e94d7d
change-id: 20240521-google-vkms-managed-4aec99461a77

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

[PATCH v2 1/6] 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_drv.h    |  1 -
 drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++++----------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 5e46ea5b96dc..9a3c6c34d1f6 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -99,7 +99,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 hrtimer vblank_hrtimer;
 	ktime_t period_ns;
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 5ce70dd946aa..4fe6b88e8081 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,
@@ -50,7 +50,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 {
 	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, *cursor = NULL;
@@ -80,8 +80,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 	if (ret)
 		return ret;
 
-	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
-				 DRM_MODE_CONNECTOR_VIRTUAL);
+	connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
+	if (!connector) {
+		DRM_ERROR("Failed to allocate connector\n");
+		ret = -ENOMEM;
+		goto err_connector;
+	}
+
+	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_connector;
@@ -93,7 +100,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 			       DRM_MODE_ENCODER_VIRTUAL, NULL);
 	if (ret) {
 		DRM_ERROR("Failed to init encoder\n");
-		goto err_encoder;
+		return ret;
 	}
 	encoder->possible_crtcs = 1;
 
@@ -115,12 +122,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 
 err_attach:
 	drm_encoder_cleanup(encoder);
-
-err_encoder:
-	drm_connector_cleanup(connector);
-
 err_connector:
 	drm_crtc_cleanup(crtc);
-
 	return ret;
 }

-- 
2.44.2

Re: [PATCH v2 1/6] drm/vkms: Switch to managed for connector

[Maxime Ripard]

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

Hi,

On Tue, Aug 27, 2024 at 11:57:36AM GMT, 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_drv.h    |  1 -
>  drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++++----------
>  2 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 5e46ea5b96dc..9a3c6c34d1f6 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -99,7 +99,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 hrtimer vblank_hrtimer;
>  	ktime_t period_ns;
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 5ce70dd946aa..4fe6b88e8081 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,
> @@ -50,7 +50,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  {
>  	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, *cursor = NULL;
> @@ -80,8 +80,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  	if (ret)
>  		return ret;
>  
> -	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> -				 DRM_MODE_CONNECTOR_VIRTUAL);
> +	connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
> +	if (!connector) {
> +		DRM_ERROR("Failed to allocate connector\n");
> +		ret = -ENOMEM;
> +		goto err_connector;
> +	}
> +

I think it would be worth explaining why you need to move to a separate
allocation for the connector now.

Maxime

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

Re: [PATCH v2 1/6] drm/vkms: Switch to managed for connector

[Louis Chauvet]

Le 27/08/24 - 15:15, Maxime Ripard a écrit :
> Hi,
> 
> On Tue, Aug 27, 2024 at 11:57:36AM GMT, 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_drv.h    |  1 -
> >  drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++++----------
> >  2 files changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 5e46ea5b96dc..9a3c6c34d1f6 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -99,7 +99,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 hrtimer vblank_hrtimer;
> >  	ktime_t period_ns;
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 5ce70dd946aa..4fe6b88e8081 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,
> > @@ -50,7 +50,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> >  {
> >  	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, *cursor = NULL;
> > @@ -80,8 +80,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> > -				 DRM_MODE_CONNECTOR_VIRTUAL);
> > +	connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
> > +	if (!connector) {
> > +		DRM_ERROR("Failed to allocate connector\n");
> > +		ret = -ENOMEM;
> > +		goto err_connector;
> > +	}
> > +
> 
> I think it would be worth explaining why you need to move to a separate
> allocation for the connector now.
> 
> Maxime

Hi,

This is in preparation for ConfigFS implementation, as the number of 
connector/encoders/crtc/planes... will be dynamic, we need to have 
separate alloaction.

If I add this paragraph in the commit message, is it sufficient?

	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.

(same for encoder & CRTC)

Thanks,
Louis Chauvet

Re: [PATCH v2 1/6] drm/vkms: Switch to managed for connector

[Maxime Ripard]

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

On Tue, Aug 27, 2024 at 03:24:10PM GMT, Louis Chauvet wrote:
> Le 27/08/24 - 15:15, Maxime Ripard a écrit :
> > Hi,
> > 
> > On Tue, Aug 27, 2024 at 11:57:36AM GMT, 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_drv.h    |  1 -
> > >  drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++++----------
> > >  2 files changed, 12 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index 5e46ea5b96dc..9a3c6c34d1f6 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -99,7 +99,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 hrtimer vblank_hrtimer;
> > >  	ktime_t period_ns;
> > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > > index 5ce70dd946aa..4fe6b88e8081 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,
> > > @@ -50,7 +50,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > >  {
> > >  	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, *cursor = NULL;
> > > @@ -80,8 +80,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> > > -				 DRM_MODE_CONNECTOR_VIRTUAL);
> > > +	connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
> > > +	if (!connector) {
> > > +		DRM_ERROR("Failed to allocate connector\n");
> > > +		ret = -ENOMEM;
> > > +		goto err_connector;
> > > +	}
> > > +
> > 
> > I think it would be worth explaining why you need to move to a separate
> > allocation for the connector now.
> > 
> > Maxime
> 
> Hi,
> 
> This is in preparation for ConfigFS implementation, as the number of 
> connector/encoders/crtc/planes... will be dynamic, we need to have 
> separate alloaction.
> 
> If I add this paragraph in the commit message, is it sufficient?
> 
> 	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.
> 
> (same for encoder & CRTC)

Yeah, that's a good message, but it probably belongs in a separate patch
then.

Thanks!
Maxime

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

Re: [PATCH v2 1/6] drm/vkms: Switch to managed for connector

[Louis Chauvet]

Le 27/08/24 - 16:39, Maxime Ripard a écrit :
> On Tue, Aug 27, 2024 at 03:24:10PM GMT, Louis Chauvet wrote:
> > Le 27/08/24 - 15:15, Maxime Ripard a écrit :
> > > Hi,
> > > 
> > > On Tue, Aug 27, 2024 at 11:57:36AM GMT, 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_drv.h    |  1 -
> > > >  drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++++----------
> > > >  2 files changed, 12 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > index 5e46ea5b96dc..9a3c6c34d1f6 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > @@ -99,7 +99,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 hrtimer vblank_hrtimer;
> > > >  	ktime_t period_ns;
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > > > index 5ce70dd946aa..4fe6b88e8081 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,
> > > > @@ -50,7 +50,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > > >  {
> > > >  	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, *cursor = NULL;
> > > > @@ -80,8 +80,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> > > > -				 DRM_MODE_CONNECTOR_VIRTUAL);
> > > > +	connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
> > > > +	if (!connector) {
> > > > +		DRM_ERROR("Failed to allocate connector\n");
> > > > +		ret = -ENOMEM;
> > > > +		goto err_connector;
> > > > +	}
> > > > +
> > > 
> > > I think it would be worth explaining why you need to move to a separate
> > > allocation for the connector now.
> > > 
> > > Maxime
> > 
> > Hi,
> > 
> > This is in preparation for ConfigFS implementation, as the number of 
> > connector/encoders/crtc/planes... will be dynamic, we need to have 
> > separate alloaction.
> > 
> > If I add this paragraph in the commit message, is it sufficient?
> > 
> > 	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.
> > 
> > (same for encoder & CRTC)
> 
> Yeah, that's a good message, but it probably belongs in a separate patch
> then.

Can you explain what you mean by "in a separate patch"? I wanted to write 
this paragraph in the commit log.

Louis Chauvet

Re: [PATCH v2 1/6] drm/vkms: Switch to managed for connector

[Maxime Ripard]

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

On Tue, Aug 27, 2024 at 05:08:08PM GMT, Louis Chauvet wrote:
> Le 27/08/24 - 16:39, Maxime Ripard a écrit :
> > On Tue, Aug 27, 2024 at 03:24:10PM GMT, Louis Chauvet wrote:
> > > Le 27/08/24 - 15:15, Maxime Ripard a écrit :
> > > > Hi,
> > > > 
> > > > On Tue, Aug 27, 2024 at 11:57:36AM GMT, 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_drv.h    |  1 -
> > > > >  drivers/gpu/drm/vkms/vkms_output.c | 22 ++++++++++++----------
> > > > >  2 files changed, 12 insertions(+), 11 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > index 5e46ea5b96dc..9a3c6c34d1f6 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > @@ -99,7 +99,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 hrtimer vblank_hrtimer;
> > > > >  	ktime_t period_ns;
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > > > > index 5ce70dd946aa..4fe6b88e8081 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,
> > > > > @@ -50,7 +50,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > > > >  {
> > > > >  	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, *cursor = NULL;
> > > > > @@ -80,8 +80,15 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > > > >  	if (ret)
> > > > >  		return ret;
> > > > >  
> > > > > -	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> > > > > -				 DRM_MODE_CONNECTOR_VIRTUAL);
> > > > > +	connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
> > > > > +	if (!connector) {
> > > > > +		DRM_ERROR("Failed to allocate connector\n");
> > > > > +		ret = -ENOMEM;
> > > > > +		goto err_connector;
> > > > > +	}
> > > > > +
> > > > 
> > > > I think it would be worth explaining why you need to move to a separate
> > > > allocation for the connector now.
> > > > 
> > > > Maxime
> > > 
> > > Hi,
> > > 
> > > This is in preparation for ConfigFS implementation, as the number of 
> > > connector/encoders/crtc/planes... will be dynamic, we need to have 
> > > separate alloaction.
> > > 
> > > If I add this paragraph in the commit message, is it sufficient?
> > > 
> > > 	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.
> > > 
> > > (same for encoder & CRTC)
> > 
> > Yeah, that's a good message, but it probably belongs in a separate patch
> > then.
> 
> Can you explain what you mean by "in a separate patch"? I wanted to write 
> this paragraph in the commit log.

You're doing two things in that patch: converting to drmm like you
documented in your original commit log, and switching from having the
connector (encoder, and crtc) from vkms_output to being dynamically
allocated.

Ideally, you should have one patch to switch from being part of
vkms_output to a dynamic allocation (with the commit log you suggested
above), and one patch to switch to drmm with your original commit log.

Maxime

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

[PATCH v2 2/6] 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_drv.h    |  1 -
 drivers/gpu/drm/vkms/vkms_output.c | 22 +++++++++++-----------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 9a3c6c34d1f6..101993378b49 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -98,7 +98,6 @@ struct vkms_crtc_state {
 
 struct vkms_output {
 	struct drm_crtc crtc;
-	struct drm_encoder encoder;
 	struct drm_writeback_connector wb_connector;
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 4fe6b88e8081..bbec7c14229c 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;
@@ -51,7 +47,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 	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, *cursor = NULL;
 	int ret;
@@ -96,18 +92,24 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 
 	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
 
-	ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
-			       DRM_MODE_ENCODER_VIRTUAL, NULL);
+	encoder = drmm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
+	if (!encoder) {
+		DRM_ERROR("Failed to allocate encoder\n");
+		ret = -ENOMEM;
+		goto err_connector;
+	}
+
+	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 = 1;
 
 	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) {
@@ -120,8 +122,6 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 
 	return 0;
 
-err_attach:
-	drm_encoder_cleanup(encoder);
 err_connector:
 	drm_crtc_cleanup(crtc);
 	return ret;

-- 
2.44.2

Re: [PATCH v2 2/6] drm/vkms: Switch to managed for encoder

[Maxime Ripard]

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

On Tue, Aug 27, 2024 at 11:57:37AM GMT, Louis Chauvet wrote:
> 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_drv.h    |  1 -
>  drivers/gpu/drm/vkms/vkms_output.c | 22 +++++++++++-----------
>  2 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 9a3c6c34d1f6..101993378b49 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -98,7 +98,6 @@ struct vkms_crtc_state {
>  
>  struct vkms_output {
>  	struct drm_crtc crtc;
> -	struct drm_encoder encoder;
>  	struct drm_writeback_connector wb_connector;
>  	struct hrtimer vblank_hrtimer;
>  	ktime_t period_ns;
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 4fe6b88e8081..bbec7c14229c 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;
> @@ -51,7 +47,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  	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, *cursor = NULL;
>  	int ret;
> @@ -96,18 +92,24 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  
>  	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
>  
> -	ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
> -			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> +	encoder = drmm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
> +	if (!encoder) {
> +		DRM_ERROR("Failed to allocate encoder\n");
> +		ret = -ENOMEM;
> +		goto err_connector;
> +	}
> +

Ditto.

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

[PATCH v2 3/6] drm/vkms: Rename vkms_output to vkms_crtc

[Louis Chauvet]

The current vkms_output structure only contains crtc-related members. In
preparation of the migration to drmm for crtc and to avoid confusion,
rename this structure and all its usage to vkms_crtc.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c  | 30 +++++++++----------
 drivers/gpu/drm/vkms/vkms_crtc.c      | 54 +++++++++++++++++------------------
 drivers/gpu/drm/vkms/vkms_drv.c       |  4 +--
 drivers/gpu/drm/vkms/vkms_drv.h       | 12 ++++----
 drivers/gpu/drm/vkms/vkms_output.c    |  4 +--
 drivers/gpu/drm/vkms/vkms_writeback.c | 16 +++++------
 6 files changed, 59 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index e7441b227b3c..b1723cf02ed3 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -306,13 +306,13 @@ void vkms_composer_worker(struct work_struct *work)
 						composer_work);
 	struct drm_crtc *crtc = crtc_state->base.crtc;
 	struct vkms_writeback_job *active_wb = crtc_state->active_writeback;
-	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
 	bool crc_pending, wb_pending;
 	u64 frame_start, frame_end;
 	u32 crc32 = 0;
 	int ret;
 
-	spin_lock_irq(&out->composer_lock);
+	spin_lock_irq(&vkms_crtc->composer_lock);
 	frame_start = crtc_state->frame_start;
 	frame_end = crtc_state->frame_end;
 	crc_pending = crtc_state->crc_pending;
@@ -336,7 +336,7 @@ void vkms_composer_worker(struct work_struct *work)
 		crtc_state->gamma_lut.base = NULL;
 	}
 
-	spin_unlock_irq(&out->composer_lock);
+	spin_unlock_irq(&vkms_crtc->composer_lock);
 
 	/*
 	 * We raced with the vblank hrtimer and previous work already computed
@@ -354,10 +354,10 @@ void vkms_composer_worker(struct work_struct *work)
 		return;
 
 	if (wb_pending) {
-		drm_writeback_signal_completion(&out->wb_connector, 0);
-		spin_lock_irq(&out->composer_lock);
+		drm_writeback_signal_completion(&vkms_crtc->wb_connector, 0);
+		spin_lock_irq(&vkms_crtc->composer_lock);
 		crtc_state->wb_pending = false;
-		spin_unlock_irq(&out->composer_lock);
+		spin_unlock_irq(&vkms_crtc->composer_lock);
 	}
 
 	/*
@@ -407,31 +407,31 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
 	return 0;
 }
 
-void vkms_set_composer(struct vkms_output *out, bool enabled)
+void vkms_set_composer(struct vkms_crtc *vkms_crtc, bool enabled)
 {
 	bool old_enabled;
 
 	if (enabled)
-		drm_crtc_vblank_get(&out->crtc);
+		drm_crtc_vblank_get(&vkms_crtc->base);
 
-	spin_lock_irq(&out->lock);
-	old_enabled = out->composer_enabled;
-	out->composer_enabled = enabled;
-	spin_unlock_irq(&out->lock);
+	spin_lock_irq(&vkms_crtc->lock);
+	old_enabled = vkms_crtc->composer_enabled;
+	vkms_crtc->composer_enabled = enabled;
+	spin_unlock_irq(&vkms_crtc->lock);
 
 	if (old_enabled)
-		drm_crtc_vblank_put(&out->crtc);
+		drm_crtc_vblank_put(&vkms_crtc->base);
 }
 
 int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 {
-	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
 	bool enabled = false;
 	int ret = 0;
 
 	ret = vkms_crc_parse_source(src_name, &enabled);
 
-	vkms_set_composer(out, enabled);
+	vkms_set_composer(vkms_crtc, enabled);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 40b4d084e3ce..013bf8336b54 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -11,35 +11,35 @@
 
 static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 {
-	struct vkms_output *output = container_of(timer, struct vkms_output,
+	struct vkms_crtc *vkms_crtc = container_of(timer, struct vkms_crtc,
 						  vblank_hrtimer);
-	struct drm_crtc *crtc = &output->crtc;
+	struct drm_crtc *crtc = &vkms_crtc->base;
 	struct vkms_crtc_state *state;
 	u64 ret_overrun;
 	bool ret, fence_cookie;
 
 	fence_cookie = dma_fence_begin_signalling();
 
-	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
-					  output->period_ns);
+	ret_overrun = hrtimer_forward_now(&vkms_crtc->vblank_hrtimer,
+					  vkms_crtc->period_ns);
 	if (ret_overrun != 1)
 		pr_warn("%s: vblank timer overrun\n", __func__);
 
-	spin_lock(&output->lock);
+	spin_lock(&vkms_crtc->lock);
 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
-	state = output->composer_state;
-	spin_unlock(&output->lock);
+	state = vkms_crtc->composer_state;
+	spin_unlock(&vkms_crtc->lock);
 
-	if (state && output->composer_enabled) {
+	if (state && vkms_crtc->composer_enabled) {
 		u64 frame = drm_crtc_accurate_vblank_count(crtc);
 
 		/* update frame_start only if a queued vkms_composer_worker()
 		 * has read the data
 		 */
-		spin_lock(&output->composer_lock);
+		spin_lock(&vkms_crtc->composer_lock);
 		if (!state->crc_pending)
 			state->frame_start = frame;
 		else
@@ -47,9 +47,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 					 state->frame_start, frame);
 		state->frame_end = frame;
 		state->crc_pending = true;
-		spin_unlock(&output->composer_lock);
+		spin_unlock(&vkms_crtc->composer_lock);
 
-		ret = queue_work(output->composer_workq, &state->composer_work);
+		ret = queue_work(vkms_crtc->composer_workq, &state->composer_work);
 		if (!ret)
 			DRM_DEBUG_DRIVER("Composer worker already queued\n");
 	}
@@ -62,7 +62,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 static int vkms_enable_vblank(struct drm_crtc *crtc)
 {
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
-	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	struct vkms_crtc *out = drm_crtc_to_vkms_crtc(crtc);
 
 	drm_calc_timestamping_constants(crtc, &crtc->mode);
 
@@ -76,7 +76,7 @@ static int vkms_enable_vblank(struct drm_crtc *crtc)
 
 static void vkms_disable_vblank(struct drm_crtc *crtc)
 {
-	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	struct vkms_crtc *out = drm_crtc_to_vkms_crtc(crtc);
 
 	hrtimer_cancel(&out->vblank_hrtimer);
 }
@@ -85,9 +85,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_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
 
 	if (!READ_ONCE(vblank->enabled)) {
@@ -95,7 +93,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
 		return true;
 	}
 
-	*vblank_time = READ_ONCE(output->vblank_hrtimer.node.expires);
+	*vblank_time = READ_ONCE(vkms_crtc->vblank_hrtimer.node.expires);
 
 	if (WARN_ON(*vblank_time == vblank->time))
 		return true;
@@ -107,7 +105,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
 	 * the vblank core expects. Therefore we need to always correct the
 	 * timestampe by one frame.
 	 */
-	*vblank_time -= output->period_ns;
+	*vblank_time -= vkms_crtc->period_ns;
 
 	return true;
 }
@@ -233,18 +231,18 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
 static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
 				   struct drm_atomic_state *state)
 {
-	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+	struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
 
 	/* This lock is held across the atomic commit to block vblank timer
 	 * from scheduling vkms_composer_worker until the composer is updated
 	 */
-	spin_lock_irq(&vkms_output->lock);
+	spin_lock_irq(&vkms_crtc->lock);
 }
 
 static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 				   struct drm_atomic_state *state)
 {
-	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+	struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
 
 	if (crtc->state->event) {
 		spin_lock(&crtc->dev->event_lock);
@@ -259,9 +257,9 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 		crtc->state->event = NULL;
 	}
 
-	vkms_output->composer_state = to_vkms_crtc_state(crtc->state);
+	vkms_crtc->composer_state = to_vkms_crtc_state(crtc->state);
 
-	spin_unlock_irq(&vkms_output->lock);
+	spin_unlock_irq(&vkms_crtc->lock);
 }
 
 static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
@@ -275,7 +273,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor)
 {
-	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
+	struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
 	int ret;
 
 	ret = drmm_crtc_init_with_planes(dev, crtc, primary, cursor,
@@ -290,11 +288,11 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 	drm_mode_crtc_set_gamma_size(crtc, VKMS_LUT_SIZE);
 	drm_crtc_enable_color_mgmt(crtc, 0, false, VKMS_LUT_SIZE);
 
-	spin_lock_init(&vkms_out->lock);
-	spin_lock_init(&vkms_out->composer_lock);
+	spin_lock_init(&vkms_crtc->lock);
+	spin_lock_init(&vkms_crtc->composer_lock);
 
-	vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
-	if (!vkms_out->composer_workq)
+	vkms_crtc->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
+	if (!vkms_crtc->composer_workq)
 		return -ENOMEM;
 
 	return ret;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 0c1a713b7b7b..d1ed6bbe9559 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -57,8 +57,8 @@ 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);
+	if (vkms->crtc.composer_workq)
+		destroy_workqueue(vkms->crtc.composer_workq);
 }
 
 static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 101993378b49..c55ab45ccb20 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -96,8 +96,8 @@ struct vkms_crtc_state {
 	u64 frame_end;
 };
 
-struct vkms_output {
-	struct drm_crtc crtc;
+struct vkms_crtc {
+	struct drm_crtc base;
 	struct drm_writeback_connector wb_connector;
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
@@ -126,12 +126,12 @@ struct vkms_config {
 struct vkms_device {
 	struct drm_device drm;
 	struct platform_device *platform;
-	struct vkms_output output;
+	struct vkms_crtc crtc;
 	const struct vkms_config *config;
 };
 
-#define drm_crtc_to_vkms_output(target) \
-	container_of(target, struct vkms_output, crtc)
+#define drm_crtc_to_vkms_crtc(target) \
+	container_of(target, struct vkms_crtc, base)
 
 #define drm_device_to_vkms_device(target) \
 	container_of(target, struct vkms_device, drm)
@@ -160,7 +160,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 
 /* Composer Support */
 void vkms_composer_worker(struct work_struct *work);
-void vkms_set_composer(struct vkms_output *out, bool enabled);
+void vkms_set_composer(struct vkms_crtc *out, bool enabled);
 void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state *plane, int y);
 void vkms_writeback_row(struct vkms_writeback_job *wb, const struct line_buffer *src_buffer, int y);
 
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index bbec7c14229c..cd506dcfd50f 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -44,11 +44,11 @@ static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
 
 int vkms_output_init(struct vkms_device *vkmsdev, int index)
 {
-	struct vkms_output *output = &vkmsdev->output;
+	struct vkms_crtc *vkms_crtc = &vkmsdev->crtc;
 	struct drm_device *dev = &vkmsdev->drm;
 	struct drm_connector *connector;
 	struct drm_encoder *encoder;
-	struct drm_crtc *crtc = &output->crtc;
+	struct drm_crtc *crtc = &vkms_crtc->base;
 	struct vkms_plane *primary, *cursor = NULL;
 	int ret;
 	int writeback;
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index bc724cbd5e3a..e055ad41241b 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -116,7 +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(&vkmsdev->crtc, false);
 	kfree(vkmsjob);
 }
 
@@ -126,10 +126,10 @@ 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 drm_writeback_connector *wb_conn = &output->wb_connector;
+	struct vkms_crtc *vkms_crtc = &vkmsdev->crtc;
+	struct drm_writeback_connector *wb_conn = &vkms_crtc->wb_connector;
 	struct drm_connector_state *conn_state = wb_conn->base.state;
-	struct vkms_crtc_state *crtc_state = output->composer_state;
+	struct vkms_crtc_state *crtc_state = vkms_crtc->composer_state;
 	struct drm_framebuffer *fb = connector_state->writeback_job->fb;
 	u16 crtc_height = crtc_state->base.crtc->mode.vdisplay;
 	u16 crtc_width = crtc_state->base.crtc->mode.hdisplay;
@@ -140,15 +140,15 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
 	if (!conn_state)
 		return;
 
-	vkms_set_composer(&vkmsdev->output, true);
+	vkms_set_composer(&vkmsdev->crtc, true);
 
 	active_wb = conn_state->writeback_job->priv;
 	wb_frame_info = &active_wb->wb_frame_info;
 
-	spin_lock_irq(&output->composer_lock);
+	spin_lock_irq(&vkms_crtc->composer_lock);
 	crtc_state->active_writeback = active_wb;
 	crtc_state->wb_pending = true;
-	spin_unlock_irq(&output->composer_lock);
+	spin_unlock_irq(&vkms_crtc->composer_lock);
 
 	wb_frame_info->offset = fb->offsets[0];
 	wb_frame_info->pitch = fb->pitches[0];
@@ -170,7 +170,7 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
 
 int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
 {
-	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
+	struct drm_writeback_connector *wb = &vkmsdev->crtc.wb_connector;
 
 	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
 

-- 
2.44.2

[PATCH v2 4/6] drm/vkms: rename to_vkms_crtc_state to drm_crtc_state_to_vkms_crtc_state to avoid confusion

[Louis Chauvet]

To avoid confusion in macro, rename to_vkms_crtc_state to a more explicit
name drm_crtc_state_to_vkms_crtc_state

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

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 013bf8336b54..2bf733a1b9f0 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -132,7 +132,7 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
 static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {
-	struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(state);
+	struct vkms_crtc_state *vkms_state = drm_crtc_state_to_vkms_crtc_state(state);
 
 	__drm_atomic_helper_crtc_destroy_state(state);
 
@@ -173,7 +173,7 @@ static int vkms_crtc_atomic_check(struct drm_crtc *crtc,
 {
 	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
 									  crtc);
-	struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(crtc_state);
+	struct vkms_crtc_state *vkms_state = drm_crtc_state_to_vkms_crtc_state(crtc_state);
 	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
 	int i = 0, ret;
@@ -257,7 +257,7 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 		crtc->state->event = NULL;
 	}
 
-	vkms_crtc->composer_state = to_vkms_crtc_state(crtc->state);
+	vkms_crtc->composer_state = drm_crtc_state_to_vkms_crtc_state(crtc->state);
 
 	spin_unlock_irq(&vkms_crtc->lock);
 }
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index d1ed6bbe9559..f2818374f5c9 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -82,7 +82,7 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
 
 	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
 		struct vkms_crtc_state *vkms_state =
-			to_vkms_crtc_state(old_crtc_state);
+					       drm_crtc_state_to_vkms_crtc_state(old_crtc_state);
 
 		flush_work(&vkms_state->composer_work);
 	}
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index c55ab45ccb20..3501cd9401d5 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -136,7 +136,7 @@ struct vkms_device {
 #define drm_device_to_vkms_device(target) \
 	container_of(target, struct vkms_device, drm)
 
-#define to_vkms_crtc_state(target)\
+#define drm_crtc_state_to_vkms_crtc_state(target)\
 	container_of(target, struct vkms_crtc_state, base)
 
 #define to_vkms_plane_state(target)\

-- 
2.44.2

[PATCH v2 5/6] drm/vkms: Switch to managed for CRTC

[Louis Chauvet]

The current VKMS driver uses non-managed function to create CRTCs. 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      | 40 +++++++++++++-------
 drivers/gpu/drm/vkms/vkms_drv.c       |  9 -----
 drivers/gpu/drm/vkms/vkms_drv.h       |  7 ++--
 drivers/gpu/drm/vkms/vkms_output.c    | 69 +++++++++++++++--------------------
 drivers/gpu/drm/vkms/vkms_writeback.c | 14 +++----
 5 files changed, 65 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 2bf733a1b9f0..ff55099c86ce 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"
 
@@ -270,30 +271,41 @@ 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)
+static void vkms_crtc_destroy_workqueue(struct drm_device *dev, void *raw_vkms_crtc)
 {
-	struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
+	struct vkms_crtc *vkms_crtc = raw_vkms_crtc;
+
+	destroy_workqueue(vkms_crtc->composer_workq);
+}
+
+struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev, struct drm_plane *primary,
+				 struct drm_plane *cursor)
+{
+	struct drm_device *dev = &vkmsdev->drm;
+	struct vkms_crtc *vkms_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_crtc = drmm_crtc_alloc_with_planes(dev, struct vkms_crtc, base, primary, cursor,
+						&vkms_crtc_funcs, NULL);
+	if (IS_ERR(vkms_crtc)) {
+		DRM_DEV_ERROR(vkmsdev->drm.dev, "Failed to init CRTC\n");
+		return vkms_crtc;
 	}
+	drm_crtc_helper_add(&vkms_crtc->base, &vkms_crtc_helper_funcs);
 
-	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
-
-	drm_mode_crtc_set_gamma_size(crtc, VKMS_LUT_SIZE);
-	drm_crtc_enable_color_mgmt(crtc, 0, false, VKMS_LUT_SIZE);
+	drm_mode_crtc_set_gamma_size(&vkms_crtc->base, VKMS_LUT_SIZE);
+	drm_crtc_enable_color_mgmt(&vkms_crtc->base, 0, false, VKMS_LUT_SIZE);
 
 	spin_lock_init(&vkms_crtc->lock);
 	spin_lock_init(&vkms_crtc->composer_lock);
 
 	vkms_crtc->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
 	if (!vkms_crtc->composer_workq)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
+
+	ret = drmm_add_action_or_reset(dev, vkms_crtc_destroy_workqueue, vkms_crtc);
+	if (ret)
+		return ERR_PTR(ret);
 
-	return ret;
+	return vkms_crtc;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index f2818374f5c9..5282f71b50a6 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -53,14 +53,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->crtc.composer_workq)
-		destroy_workqueue(vkms->crtc.composer_workq);
-}
-
 static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
@@ -109,7 +101,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,
 
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 3501cd9401d5..cff2e0686c04 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -126,7 +126,6 @@ struct vkms_config {
 struct vkms_device {
 	struct drm_device drm;
 	struct platform_device *platform;
-	struct vkms_crtc crtc;
 	const struct vkms_config *config;
 };
 
@@ -143,8 +142,8 @@ struct vkms_device {
 	container_of(target, struct vkms_plane_state, base.base)
 
 /* CRTC */
-int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
-		   struct drm_plane *primary, struct drm_plane *cursor);
+struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev, struct drm_plane *primary,
+				 struct drm_plane *cursor);
 
 int vkms_output_init(struct vkms_device *vkmsdev, int index);
 
@@ -165,6 +164,6 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
 void vkms_writeback_row(struct vkms_writeback_job *wb, const struct line_buffer *src_buffer, int y);
 
 /* Writeback */
-int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
+int vkms_enable_writeback_connector(struct vkms_device *vkmsdev, struct vkms_crtc *vkms_crtc);
 
 #endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index cd506dcfd50f..0c585e48fa01 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -27,42 +27,31 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
 	.get_modes    = vkms_conn_get_modes,
 };
 
-static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
-				  struct drm_crtc *crtc)
-{
-	struct vkms_plane *overlay;
-
-	overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index);
-	if (IS_ERR(overlay))
-		return PTR_ERR(overlay);
-
-	if (!overlay->base.possible_crtcs)
-		overlay->base.possible_crtcs = drm_crtc_mask(crtc);
-
-	return 0;
-}
-
 int vkms_output_init(struct vkms_device *vkmsdev, int index)
 {
-	struct vkms_crtc *vkms_crtc = &vkmsdev->crtc;
+	struct vkms_plane *primary, *cursor, *overlay = NULL;
 	struct drm_device *dev = &vkmsdev->drm;
 	struct drm_connector *connector;
 	struct drm_encoder *encoder;
-	struct drm_crtc *crtc = &vkms_crtc->base;
-	struct vkms_plane *primary, *cursor = NULL;
-	int ret;
-	int writeback;
+	struct vkms_crtc *crtc;
 	unsigned int n;
+	int ret;
 
+	/*
+	 * Initialize used plane. One primary plane is required to perform the composition.
+	 *
+	 * The overlay and cursor planes are not mandatory, but can be used to perform complex
+	 * composition.
+	 */
 	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
 	if (IS_ERR(primary))
 		return PTR_ERR(primary);
 
 	if (vkmsdev->config->overlay) {
 		for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
-			ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
-			if (ret)
-				return ret;
+			overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index);
+			if (IS_ERR(overlay))
+				return PTR_ERR(overlay);
 		}
 	}
 
@@ -72,22 +61,24 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 			return PTR_ERR(cursor);
 	}
 
-	ret = vkms_crtc_init(dev, crtc, &primary->base, &cursor->base);
-	if (ret)
-		return ret;
+	crtc = vkms_crtc_init(vkmsdev, &primary->base,
+			      cursor ? &cursor->base : NULL);
+	if (IS_ERR(crtc)) {
+		DRM_ERROR("Failed to allocate CRTC\n");
+		return PTR_ERR(crtc);
+	}
 
 	connector = drmm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
 	if (!connector) {
 		DRM_ERROR("Failed to allocate connector\n");
-		ret = -ENOMEM;
-		goto err_connector;
+		return -ENOMEM;
 	}
 
 	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_connector;
+		return ret;
 	}
 
 	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
@@ -95,34 +86,34 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 	encoder = drmm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
 	if (!encoder) {
 		DRM_ERROR("Failed to allocate encoder\n");
-		ret = -ENOMEM;
-		goto err_connector;
+		return -ENOMEM;
 	}
 
 	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_VIRTUAL, NULL);
 	if (ret) {
 		DRM_ERROR("Failed to init encoder\n");
-		goto err_connector;
+		return ret;
 	}
-	encoder->possible_crtcs = 1;
 
+	encoder->possible_crtcs = drm_crtc_mask(&crtc->base);
+
+	/* 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);
-		if (writeback)
+		ret = vkms_enable_writeback_connector(vkmsdev, crtc);
+		if (ret) {
 			DRM_ERROR("Failed to init writeback connector\n");
+			return ret;
+		}
 	}
 
 	drm_mode_config_reset(dev);
 
 	return 0;
-
-err_connector:
-	drm_crtc_cleanup(crtc);
-	return ret;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index e055ad41241b..18aed20cd733 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -106,7 +106,7 @@ 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_crtc *vkms_crtc = container_of(connector, struct vkms_crtc, wb_connector);
 
 	if (!job->fb)
 		return;
@@ -115,8 +115,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->crtc, false);
+	vkms_set_composer(vkms_crtc, false);
 	kfree(vkmsjob);
 }
 
@@ -125,8 +124,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_crtc *vkms_crtc = &vkmsdev->crtc;
+	struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(connector_state->crtc);
 	struct drm_writeback_connector *wb_conn = &vkms_crtc->wb_connector;
 	struct drm_connector_state *conn_state = wb_conn->base.state;
 	struct vkms_crtc_state *crtc_state = vkms_crtc->composer_state;
@@ -140,7 +138,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
 	if (!conn_state)
 		return;
 
-	vkms_set_composer(&vkmsdev->crtc, true);
+	vkms_set_composer(vkms_crtc, true);
 
 	active_wb = conn_state->writeback_job->priv;
 	wb_frame_info = &active_wb->wb_frame_info;
@@ -168,9 +166,9 @@ 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)
+int vkms_enable_writeback_connector(struct vkms_device *vkmsdev, struct vkms_crtc *vkms_crtc)
 {
-	struct drm_writeback_connector *wb = &vkmsdev->crtc.wb_connector;
+	struct drm_writeback_connector *wb = &vkms_crtc->wb_connector;
 
 	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
 

-- 
2.44.2

Re: [PATCH v2 5/6] drm/vkms: Switch to managed for CRTC

[kernel test robot]

Hi Louis,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 071d583e01c88272f6ff216d4f867f8f35e94d7d]

url:    https://github.com/intel-lab-lkp/linux/commits/Louis-Chauvet/drm-vkms-Switch-to-managed-for-connector/20240827-180427
base:   071d583e01c88272f6ff216d4f867f8f35e94d7d
patch link:    https://lore.kernel.org/r/20240827-google-vkms-managed-v2-5-f41104553aeb%40bootlin.com
patch subject: [PATCH v2 5/6] drm/vkms: Switch to managed for CRTC
config: i386-buildonly-randconfig-004-20240831 (https://download.01.org/0day-ci/archive/20240831/202408310524.JPBkb81E-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240831/202408310524.JPBkb81E-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408310524.JPBkb81E-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/vkms/vkms_output.c:58:6: warning: variable 'cursor' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
      58 |         if (vkmsdev->config->cursor) {
         |             ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/vkms/vkms_output.c:65:10: note: uninitialized use occurs here
      65 |                               cursor ? &cursor->base : NULL);
         |                               ^~~~~~
   drivers/gpu/drm/vkms/vkms_output.c:58:2: note: remove the 'if' if its condition is always true
      58 |         if (vkmsdev->config->cursor) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/vkms/vkms_output.c:32:37: note: initialize the variable 'cursor' to silence this warning
      32 |         struct vkms_plane *primary, *cursor, *overlay = NULL;
         |                                            ^
         |                                             = NULL
   1 warning generated.


vim +58 drivers/gpu/drm/vkms/vkms_output.c

d16489307a52f0 Rodrigo Siqueira    2018-07-11  29  
e9d85f731de06a Rodrigo Siqueira    2019-06-25  30  int vkms_output_init(struct vkms_device *vkmsdev, int index)
854502fa0a38dc Rodrigo Siqueira    2018-05-16  31  {
2abd7e59001123 Louis Chauvet       2024-08-27  32  	struct vkms_plane *primary, *cursor, *overlay = NULL;
854502fa0a38dc Rodrigo Siqueira    2018-05-16  33  	struct drm_device *dev = &vkmsdev->drm;
a12934d3402c04 Louis Chauvet       2024-08-27  34  	struct drm_connector *connector;
e3b4c152118a04 Louis Chauvet       2024-08-27  35  	struct drm_encoder *encoder;
2abd7e59001123 Louis Chauvet       2024-08-27  36  	struct vkms_crtc *crtc;
df2d385cb4132e José Expósito       2022-01-07  37  	unsigned int n;
2abd7e59001123 Louis Chauvet       2024-08-27  38  	int ret;
854502fa0a38dc Rodrigo Siqueira    2018-05-16  39  
2abd7e59001123 Louis Chauvet       2024-08-27  40  	/*
2abd7e59001123 Louis Chauvet       2024-08-27  41  	 * Initialize used plane. One primary plane is required to perform the composition.
2abd7e59001123 Louis Chauvet       2024-08-27  42  	 *
2abd7e59001123 Louis Chauvet       2024-08-27  43  	 * The overlay and cursor planes are not mandatory, but can be used to perform complex
2abd7e59001123 Louis Chauvet       2024-08-27  44  	 * composition.
2abd7e59001123 Louis Chauvet       2024-08-27  45  	 */
e9d85f731de06a Rodrigo Siqueira    2019-06-25  46  	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
854502fa0a38dc Rodrigo Siqueira    2018-05-16  47  	if (IS_ERR(primary))
854502fa0a38dc Rodrigo Siqueira    2018-05-16  48  		return PTR_ERR(primary);
854502fa0a38dc Rodrigo Siqueira    2018-05-16  49  
310e506c06e495 Melissa Wen         2021-04-24  50  	if (vkmsdev->config->overlay) {
df2d385cb4132e José Expósito       2022-01-07  51  		for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
2abd7e59001123 Louis Chauvet       2024-08-27  52  			overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index);
2abd7e59001123 Louis Chauvet       2024-08-27  53  			if (IS_ERR(overlay))
2abd7e59001123 Louis Chauvet       2024-08-27  54  				return PTR_ERR(overlay);
310e506c06e495 Melissa Wen         2021-04-24  55  		}
df2d385cb4132e José Expósito       2022-01-07  56  	}
310e506c06e495 Melissa Wen         2021-04-24  57  
2df7af93fdadb9 Sumera Priyadarsini 2021-01-12 @58  	if (vkmsdev->config->cursor) {

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 5/6] drm/vkms: Switch to managed for CRTC

[kernel test robot]

Hi Louis,

kernel test robot noticed the following build errors:

[auto build test ERROR on 071d583e01c88272f6ff216d4f867f8f35e94d7d]

url:    https://github.com/intel-lab-lkp/linux/commits/Louis-Chauvet/drm-vkms-Switch-to-managed-for-connector/20240827-180427
base:   071d583e01c88272f6ff216d4f867f8f35e94d7d
patch link:    https://lore.kernel.org/r/20240827-google-vkms-managed-v2-5-f41104553aeb%40bootlin.com
patch subject: [PATCH v2 5/6] drm/vkms: Switch to managed for CRTC
config: i386-randconfig-141-20240831 (https://download.01.org/0day-ci/archive/20240831/202408310628.jJbxaMOR-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240831/202408310628.jJbxaMOR-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408310628.jJbxaMOR-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/vkms/vkms_output.c:58:6: error: variable 'cursor' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
      58 |         if (vkmsdev->config->cursor) {
         |             ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/vkms/vkms_output.c:65:10: note: uninitialized use occurs here
      65 |                               cursor ? &cursor->base : NULL);
         |                               ^~~~~~
   drivers/gpu/drm/vkms/vkms_output.c:58:2: note: remove the 'if' if its condition is always true
      58 |         if (vkmsdev->config->cursor) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/vkms/vkms_output.c:32:37: note: initialize the variable 'cursor' to silence this warning
      32 |         struct vkms_plane *primary, *cursor, *overlay = NULL;
         |                                            ^
         |                                             = NULL
   1 error generated.


vim +58 drivers/gpu/drm/vkms/vkms_output.c

d16489307a52f0 Rodrigo Siqueira    2018-07-11  29  
e9d85f731de06a Rodrigo Siqueira    2019-06-25  30  int vkms_output_init(struct vkms_device *vkmsdev, int index)
854502fa0a38dc Rodrigo Siqueira    2018-05-16  31  {
2abd7e59001123 Louis Chauvet       2024-08-27  32  	struct vkms_plane *primary, *cursor, *overlay = NULL;
854502fa0a38dc Rodrigo Siqueira    2018-05-16  33  	struct drm_device *dev = &vkmsdev->drm;
a12934d3402c04 Louis Chauvet       2024-08-27  34  	struct drm_connector *connector;
e3b4c152118a04 Louis Chauvet       2024-08-27  35  	struct drm_encoder *encoder;
2abd7e59001123 Louis Chauvet       2024-08-27  36  	struct vkms_crtc *crtc;
df2d385cb4132e José Expósito       2022-01-07  37  	unsigned int n;
2abd7e59001123 Louis Chauvet       2024-08-27  38  	int ret;
854502fa0a38dc Rodrigo Siqueira    2018-05-16  39  
2abd7e59001123 Louis Chauvet       2024-08-27  40  	/*
2abd7e59001123 Louis Chauvet       2024-08-27  41  	 * Initialize used plane. One primary plane is required to perform the composition.
2abd7e59001123 Louis Chauvet       2024-08-27  42  	 *
2abd7e59001123 Louis Chauvet       2024-08-27  43  	 * The overlay and cursor planes are not mandatory, but can be used to perform complex
2abd7e59001123 Louis Chauvet       2024-08-27  44  	 * composition.
2abd7e59001123 Louis Chauvet       2024-08-27  45  	 */
e9d85f731de06a Rodrigo Siqueira    2019-06-25  46  	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
854502fa0a38dc Rodrigo Siqueira    2018-05-16  47  	if (IS_ERR(primary))
854502fa0a38dc Rodrigo Siqueira    2018-05-16  48  		return PTR_ERR(primary);
854502fa0a38dc Rodrigo Siqueira    2018-05-16  49  
310e506c06e495 Melissa Wen         2021-04-24  50  	if (vkmsdev->config->overlay) {
df2d385cb4132e José Expósito       2022-01-07  51  		for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
2abd7e59001123 Louis Chauvet       2024-08-27  52  			overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index);
2abd7e59001123 Louis Chauvet       2024-08-27  53  			if (IS_ERR(overlay))
2abd7e59001123 Louis Chauvet       2024-08-27  54  				return PTR_ERR(overlay);
310e506c06e495 Melissa Wen         2021-04-24  55  		}
df2d385cb4132e José Expósito       2022-01-07  56  	}
310e506c06e495 Melissa Wen         2021-04-24  57  
2df7af93fdadb9 Sumera Priyadarsini 2021-01-12 @58  	if (vkmsdev->config->cursor) {

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 6/6] drm/vkms: Add missing check for CRTC initialization

[Louis Chauvet]

CRTC initialization call drm_mode_crtc_set_gamma_size without the proper
checks, introduce this check.

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

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index ff55099c86ce..39802c928bdf 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -293,7 +293,12 @@ struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev, struct drm_plane *
 	}
 	drm_crtc_helper_add(&vkms_crtc->base, &vkms_crtc_helper_funcs);
 
-	drm_mode_crtc_set_gamma_size(&vkms_crtc->base, VKMS_LUT_SIZE);
+	ret = drm_mode_crtc_set_gamma_size(&vkms_crtc->base, VKMS_LUT_SIZE);
+	if (ret) {
+		DRM_DEV_ERROR(vkmsdev->drm.dev, "Failed to set gamma size\n");
+		return ERR_PTR(ret);
+	}
+
 	drm_crtc_enable_color_mgmt(&vkms_crtc->base, 0, false, VKMS_LUT_SIZE);
 
 	spin_lock_init(&vkms_crtc->lock);

-- 
2.44.2