[PATCH v3 0/2] drm/atomic-helper: rename drm_atomic_helper_check_wb_encoder_state

[PATCH v3 0/2] drm/atomic-helper: rename drm_atomic_helper_check_wb_encoder_state

[Dmitry Baryshkov]

The function drm_atomic_helper_check_wb_encoder_state() doesn't use
drm_encoder for anything sensible. Internally it checks
drm_writeback_connector's state. Thus it makes sense to let this
function accept drm_connector object and the drm_atomic_state
and rename it to drm_atomic_helper_check_wb_connector_state().

Changes since v2:
- Make the function accept drm_connector instead of
  drm_writeback_connector (Maxime)

Changes since v1:
- Make the function accept drm_writeback_connector and drm_atomic_state
  (Maxime)
- Added a patch for VKMS to move atomic_check of WB path from encoder to
  connector helpers.

Dmitry Baryshkov (2):
  drm/atomic-helper: rename drm_atomic_helper_check_wb_encoder_state
  drm/vkms: move wb's atomic_check from encoder to connector

 drivers/gpu/drm/drm_atomic_helper.c   | 16 +++++++++-------
 drivers/gpu/drm/vkms/vkms_writeback.c | 25 +++++++++++++++----------
 include/drm/drm_atomic_helper.h       |  5 ++---
 3 files changed, 26 insertions(+), 20 deletions(-)

-- 
2.39.2

[PATCH v3 1/2] drm/atomic-helper: rename drm_atomic_helper_check_wb_encoder_state

[Dmitry Baryshkov]

The drm_atomic_helper_check_wb_encoder_state() function doesn't use
encoder for anything other than getting the drm_device instance. The
function's description talks about checking the writeback connector
state, not the encoder state. Moreover, there is no such thing as an
encoder state, encoders generally do not have a state on their own.

Rename the function to drm_atomic_helper_check_wb_connector_state()
and change arguments to drm_writeback_connector and drm_atomic_state.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_atomic_helper.c   | 16 +++++++++-------
 drivers/gpu/drm/vkms/vkms_writeback.c |  5 ++++-
 include/drm/drm_atomic_helper.h       |  5 ++---
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index c3f677130def..c98a766ca3bd 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -795,9 +795,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
 
 /**
- * drm_atomic_helper_check_wb_encoder_state() - Check writeback encoder state
- * @encoder: encoder state to check
- * @conn_state: connector state to check
+ * drm_atomic_helper_check_wb_connector_state() - Check writeback connector state
+ * @connector: corresponding connector
+ * @state: the driver state object
  *
  * Checks if the writeback connector state is valid, and returns an error if it
  * isn't.
@@ -806,9 +806,11 @@ EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
  * Zero for success or -errno
  */
 int
-drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
-					 struct drm_connector_state *conn_state)
+drm_atomic_helper_check_wb_connector_state(struct drm_connector *connector,
+					   struct drm_atomic_state *state)
 {
+	struct drm_connector_state *conn_state =
+		drm_atomic_get_new_connector_state(state, connector);
 	struct drm_writeback_job *wb_job = conn_state->writeback_job;
 	struct drm_property_blob *pixel_format_blob;
 	struct drm_framebuffer *fb;
@@ -827,11 +829,11 @@ drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
 		if (fb->format->format == formats[i])
 			return 0;
 
-	drm_dbg_kms(encoder->dev, "Invalid pixel format %p4cc\n", &fb->format->format);
+	drm_dbg_kms(connector->dev, "Invalid pixel format %p4cc\n", &fb->format->format);
 
 	return -EINVAL;
 }
-EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
+EXPORT_SYMBOL(drm_atomic_helper_check_wb_connector_state);
 
 /**
  * drm_atomic_helper_check_plane_state() - Check plane state for validity
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index d7e63aa14663..23c4f7b61cb6 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -34,6 +34,9 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
 					struct drm_crtc_state *crtc_state,
 					struct drm_connector_state *conn_state)
 {
+	struct drm_connector *connector = conn_state->connector;
+	struct drm_writeback_connector *wb_conn =
+		drm_connector_to_writeback(connector);
 	struct drm_framebuffer *fb;
 	const struct drm_display_mode *mode = &crtc_state->mode;
 	int ret;
@@ -48,7 +51,7 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
 		return -EINVAL;
 	}
 
-	ret = drm_atomic_helper_check_wb_encoder_state(encoder, conn_state);
+	ret = drm_atomic_helper_check_wb_connector_state(wb_conn, conn_state->state);
 	if (ret < 0)
 		return ret;
 
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 536a0b0091c3..fea528aacfe2 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -49,9 +49,8 @@ struct drm_private_state;
 
 int drm_atomic_helper_check_modeset(struct drm_device *dev,
 				struct drm_atomic_state *state);
-int
-drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
-					 struct drm_connector_state *conn_state);
+int drm_atomic_helper_check_wb_connector_state(struct drm_connector *connector,
+					       struct drm_atomic_state *state);
 int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
 					const struct drm_crtc_state *crtc_state,
 					int min_scale,
-- 
2.39.2

[PATCH v3 2/2] drm/vkms: move wb's atomic_check from encoder to connector

[Dmitry Baryshkov]

As the renamed drm_atomic_helper_check_wb_connector_state() now accepts
drm_writeback_connector as the first argument (instead of drm_encoder),
move the VKMS writeback atomic_check from drm_encoder_helper_funcs to
drm_connector_helper_funcs. Also drop the vkms_wb_encoder_helper_funcs,
which have become empty now.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/vkms/vkms_writeback.c | 28 ++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 23c4f7b61cb6..bc724cbd5e3a 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -30,20 +30,25 @@ static const struct drm_connector_funcs vkms_wb_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
-					struct drm_crtc_state *crtc_state,
-					struct drm_connector_state *conn_state)
+static int vkms_wb_atomic_check(struct drm_connector *connector,
+				struct drm_atomic_state *state)
 {
-	struct drm_connector *connector = conn_state->connector;
-	struct drm_writeback_connector *wb_conn =
-		drm_connector_to_writeback(connector);
+	struct drm_connector_state *conn_state =
+		drm_atomic_get_new_connector_state(state, connector);
+	struct drm_crtc_state *crtc_state;
 	struct drm_framebuffer *fb;
-	const struct drm_display_mode *mode = &crtc_state->mode;
+	const struct drm_display_mode *mode;
 	int ret;
 
 	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
 		return 0;
 
+	if (!conn_state->crtc)
+		return 0;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+	mode = &crtc_state->mode;
+
 	fb = conn_state->writeback_job->fb;
 	if (fb->width != mode->hdisplay || fb->height != mode->vdisplay) {
 		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
@@ -51,17 +56,13 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
 		return -EINVAL;
 	}
 
-	ret = drm_atomic_helper_check_wb_connector_state(wb_conn, conn_state->state);
+	ret = drm_atomic_helper_check_wb_connector_state(connector, state);
 	if (ret < 0)
 		return ret;
 
 	return 0;
 }
 
-static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
-	.atomic_check = vkms_wb_encoder_atomic_check,
-};
-
 static int vkms_wb_connector_get_modes(struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
@@ -164,6 +165,7 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
 	.prepare_writeback_job = vkms_wb_prepare_job,
 	.cleanup_writeback_job = vkms_wb_cleanup_job,
 	.atomic_commit = vkms_wb_atomic_commit,
+	.atomic_check = vkms_wb_atomic_check,
 };
 
 int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
@@ -174,7 +176,7 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
 
 	return drm_writeback_connector_init(&vkmsdev->drm, wb,
 					    &vkms_wb_connector_funcs,
-					    &vkms_wb_encoder_helper_funcs,
+					    NULL,
 					    vkms_wb_formats,
 					    ARRAY_SIZE(vkms_wb_formats),
 					    1);
-- 
2.39.2

Re: [PATCH v3 1/2] drm/atomic-helper: rename drm_atomic_helper_check_wb_encoder_state

[Maxime Ripard]

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

Hi,

On Wed, Dec 06, 2023 at 01:14:54PM +0300, Dmitry Baryshkov wrote:
> The drm_atomic_helper_check_wb_encoder_state() function doesn't use
> encoder for anything other than getting the drm_device instance. The
> function's description talks about checking the writeback connector
> state, not the encoder state. Moreover, there is no such thing as an
> encoder state, encoders generally do not have a state on their own.
> 
> Rename the function to drm_atomic_helper_check_wb_connector_state()
> and change arguments to drm_writeback_connector and drm_atomic_state.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c   | 16 +++++++++-------
>  drivers/gpu/drm/vkms/vkms_writeback.c |  5 ++++-
>  include/drm/drm_atomic_helper.h       |  5 ++---
>  3 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c3f677130def..c98a766ca3bd 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -795,9 +795,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
>  
>  /**
> - * drm_atomic_helper_check_wb_encoder_state() - Check writeback encoder state
> - * @encoder: encoder state to check
> - * @conn_state: connector state to check
> + * drm_atomic_helper_check_wb_connector_state() - Check writeback connector state
> + * @connector: corresponding connector
> + * @state: the driver state object
>   *
>   * Checks if the writeback connector state is valid, and returns an error if it
>   * isn't.
> @@ -806,9 +806,11 @@ EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
>   * Zero for success or -errno
>   */
>  int
> -drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
> -					 struct drm_connector_state *conn_state)
> +drm_atomic_helper_check_wb_connector_state(struct drm_connector *connector,
> +					   struct drm_atomic_state *state)
>  {
> +	struct drm_connector_state *conn_state =
> +		drm_atomic_get_new_connector_state(state, connector);
>  	struct drm_writeback_job *wb_job = conn_state->writeback_job;
>  	struct drm_property_blob *pixel_format_blob;
>  	struct drm_framebuffer *fb;
> @@ -827,11 +829,11 @@ drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
>  		if (fb->format->format == formats[i])
>  			return 0;
>  
> -	drm_dbg_kms(encoder->dev, "Invalid pixel format %p4cc\n", &fb->format->format);
> +	drm_dbg_kms(connector->dev, "Invalid pixel format %p4cc\n", &fb->format->format);
>  
>  	return -EINVAL;
>  }
> -EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
> +EXPORT_SYMBOL(drm_atomic_helper_check_wb_connector_state);

Thanks for updating the prototype ...

>  /**
>   * drm_atomic_helper_check_plane_state() - Check plane state for validity
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index d7e63aa14663..23c4f7b61cb6 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -34,6 +34,9 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
>  					struct drm_crtc_state *crtc_state,
>  					struct drm_connector_state *conn_state)
>  {
> +	struct drm_connector *connector = conn_state->connector;
> +	struct drm_writeback_connector *wb_conn =
> +		drm_connector_to_writeback(connector);
>  	struct drm_framebuffer *fb;
>  	const struct drm_display_mode *mode = &crtc_state->mode;
>  	int ret;
> @@ -48,7 +51,7 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
>  		return -EINVAL;
>  	}
>  
> -	ret = drm_atomic_helper_check_wb_encoder_state(encoder, conn_state);
> +	ret = drm_atomic_helper_check_wb_connector_state(wb_conn, conn_state->state);

... but it looks like you forgot to update it here

Maxime

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

Re: [PATCH v3 1/2] drm/atomic-helper: rename drm_atomic_helper_check_wb_encoder_state

[Dmitry Baryshkov]

On Thu, 7 Dec 2023 at 12:10, Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Wed, Dec 06, 2023 at 01:14:54PM +0300, Dmitry Baryshkov wrote:
> > The drm_atomic_helper_check_wb_encoder_state() function doesn't use
> > encoder for anything other than getting the drm_device instance. The
> > function's description talks about checking the writeback connector
> > state, not the encoder state. Moreover, there is no such thing as an
> > encoder state, encoders generally do not have a state on their own.
> >
> > Rename the function to drm_atomic_helper_check_wb_connector_state()
> > and change arguments to drm_writeback_connector and drm_atomic_state.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c   | 16 +++++++++-------
> >  drivers/gpu/drm/vkms/vkms_writeback.c |  5 ++++-
> >  include/drm/drm_atomic_helper.h       |  5 ++---
> >  3 files changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index c3f677130def..c98a766ca3bd 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -795,9 +795,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >  EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
> >
> >  /**
> > - * drm_atomic_helper_check_wb_encoder_state() - Check writeback encoder state
> > - * @encoder: encoder state to check
> > - * @conn_state: connector state to check
> > + * drm_atomic_helper_check_wb_connector_state() - Check writeback connector state
> > + * @connector: corresponding connector
> > + * @state: the driver state object
> >   *
> >   * Checks if the writeback connector state is valid, and returns an error if it
> >   * isn't.
> > @@ -806,9 +806,11 @@ EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
> >   * Zero for success or -errno
> >   */
> >  int
> > -drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
> > -                                      struct drm_connector_state *conn_state)
> > +drm_atomic_helper_check_wb_connector_state(struct drm_connector *connector,
> > +                                        struct drm_atomic_state *state)
> >  {
> > +     struct drm_connector_state *conn_state =
> > +             drm_atomic_get_new_connector_state(state, connector);
> >       struct drm_writeback_job *wb_job = conn_state->writeback_job;
> >       struct drm_property_blob *pixel_format_blob;
> >       struct drm_framebuffer *fb;
> > @@ -827,11 +829,11 @@ drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
> >               if (fb->format->format == formats[i])
> >                       return 0;
> >
> > -     drm_dbg_kms(encoder->dev, "Invalid pixel format %p4cc\n", &fb->format->format);
> > +     drm_dbg_kms(connector->dev, "Invalid pixel format %p4cc\n", &fb->format->format);
> >
> >       return -EINVAL;
> >  }
> > -EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
> > +EXPORT_SYMBOL(drm_atomic_helper_check_wb_connector_state);
>
> Thanks for updating the prototype ...
>
> >  /**
> >   * drm_atomic_helper_check_plane_state() - Check plane state for validity
> > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > index d7e63aa14663..23c4f7b61cb6 100644
> > --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -34,6 +34,9 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> >                                       struct drm_crtc_state *crtc_state,
> >                                       struct drm_connector_state *conn_state)
> >  {
> > +     struct drm_connector *connector = conn_state->connector;
> > +     struct drm_writeback_connector *wb_conn =
> > +             drm_connector_to_writeback(connector);
> >       struct drm_framebuffer *fb;
> >       const struct drm_display_mode *mode = &crtc_state->mode;
> >       int ret;
> > @@ -48,7 +51,7 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> >               return -EINVAL;
> >       }
> >
> > -     ret = drm_atomic_helper_check_wb_encoder_state(encoder, conn_state);
> > +     ret = drm_atomic_helper_check_wb_connector_state(wb_conn, conn_state->state);
>
> ... but it looks like you forgot to update it here

Indeed, I fixed it in the second patch, but forgot to update the first one.

>
> Maxime



-- 
With best wishes
Dmitry