[PATCH 0/6] drm: Add struct drm_plane_helper_funcs.atomic_enable

[PATCH 0/6] drm: Add struct drm_plane_helper_funcs.atomic_enable

[Thomas Zimmermann]

Add atomic_enable, a plane helper that enables a plane. It is supposed
to be the inverse of atomic_disable.

While atomic_update can handle all of a plane's enable, disable and
update functionality, many drivers also implement atomic_disable to
do disabling separately. Some drivers also try to detect whether the
plane gets enabled and can benefit if DRM's atomic helpers already
do this. Give them the respective callback. At least in the case of
ast, avoiding to enable the primary plane improves performance with
some BMCs.

Tested on AST2100 hardware.

Thomas Zimmermann (6):
  drm/atomic-helper: Add atomic_enable plane-helper callback
  drm/ast: Implement struct drm_plane_helper_funcs.atomic_enable
  drm/mgag200: Remove disable handling from atomic_update
  drm/mgag200: Implement struct drm_plane_helper_funcs.atomic_enable
  drm/tidss: Remove return values from dispc_plane_{setup,enable}()
  drm/tidss: Implement struct drm_plane_helper_funcs.atomic_enable

 drivers/gpu/drm/ast/ast_mode.c           | 28 ++++++++++++++++-------
 drivers/gpu/drm/drm_atomic_helper.c      | 20 ++++++++++++----
 drivers/gpu/drm/mgag200/mgag200_drv.h    |  3 +++
 drivers/gpu/drm/mgag200/mgag200_mode.c   | 22 ++++++++++--------
 drivers/gpu/drm/tidss/tidss_dispc.c      | 12 ++++------
 drivers/gpu/drm/tidss/tidss_dispc.h      |  8 +++----
 drivers/gpu/drm/tidss/tidss_plane.c      | 20 ++++++++--------
 include/drm/drm_atomic_helper.h          | 26 +++++++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 29 +++++++++++++++++++++++-
 9 files changed, 124 insertions(+), 44 deletions(-)


base-commit: 1a019dd7a5d25f7c1c9b77931138290e28947e6a
-- 
2.39.1

[PATCH 1/6] drm/atomic-helper: Add atomic_enable plane-helper callback

[Thomas Zimmermann]

Add atomic_enable to struct drm_plane_helper_funcs. It enables a
plane independently from updating the plane's content. As such, it is
the inverse of the atomic_disable plane helper. Useful for hardware
where plane enable state is independent from plane content.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_atomic_helper.c      | 20 ++++++++++++----
 include/drm/drm_atomic_helper.h          | 26 +++++++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 29 +++++++++++++++++++++++-
 3 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d579fd8f7cb8..8606876f7233 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2702,6 +2702,11 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 			funcs->atomic_disable(plane, old_state);
 		} else if (new_plane_state->crtc || disabling) {
 			funcs->atomic_update(plane, old_state);
+
+			if (!disabling && funcs->atomic_enable) {
+				if (drm_atomic_plane_enabling(old_plane_state, new_plane_state))
+					funcs->atomic_enable(plane, old_state);
+			}
 		}
 	}
 
@@ -2762,6 +2767,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
 		struct drm_plane_state *new_plane_state =
 			drm_atomic_get_new_plane_state(old_state, plane);
 		const struct drm_plane_helper_funcs *plane_funcs;
+		bool disabling;
 
 		plane_funcs = plane->helper_private;
 
@@ -2771,12 +2777,18 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
 		WARN_ON(new_plane_state->crtc &&
 			new_plane_state->crtc != crtc);
 
-		if (drm_atomic_plane_disabling(old_plane_state, new_plane_state) &&
-		    plane_funcs->atomic_disable)
+		disabling = drm_atomic_plane_disabling(old_plane_state, new_plane_state);
+
+		if (disabling && plane_funcs->atomic_disable) {
 			plane_funcs->atomic_disable(plane, old_state);
-		else if (new_plane_state->crtc ||
-			 drm_atomic_plane_disabling(old_plane_state, new_plane_state))
+		} else if (new_plane_state->crtc || disabling) {
 			plane_funcs->atomic_update(plane, old_state);
+
+			if (!disabling && plane_funcs->atomic_enable) {
+				if (drm_atomic_plane_enabling(old_plane_state, new_plane_state))
+					plane_funcs->atomic_enable(plane, old_state);
+			}
+		}
 	}
 
 	if (crtc_funcs && crtc_funcs->atomic_flush)
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 33f982cd1a27..536a0b0091c3 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -209,6 +209,32 @@ int drm_atomic_helper_page_flip_target(
 			      __drm_atomic_get_current_plane_state((crtc_state)->state, \
 								   plane)))
 
+/**
+ * drm_atomic_plane_enabling - check whether a plane is being enabled
+ * @old_plane_state: old atomic plane state
+ * @new_plane_state: new atomic plane state
+ *
+ * Checks the atomic state of a plane to determine whether it's being enabled
+ * or not. This also WARNs if it detects an invalid state (both CRTC and FB
+ * need to either both be NULL or both be non-NULL).
+ *
+ * RETURNS:
+ * True if the plane is being enabled, false otherwise.
+ */
+static inline bool drm_atomic_plane_enabling(struct drm_plane_state *old_plane_state,
+					     struct drm_plane_state *new_plane_state)
+{
+	/*
+	 * When enabling a plane, CRTC and FB should always be set together.
+	 * Anything else should be considered a bug in the atomic core, so we
+	 * gently warn about it.
+	 */
+	WARN_ON((!new_plane_state->crtc && new_plane_state->fb) ||
+		(new_plane_state->crtc && !new_plane_state->fb));
+
+	return !old_plane_state->crtc && new_plane_state->crtc;
+}
+
 /**
  * drm_atomic_plane_disabling - check whether a plane is being disabled
  * @old_plane_state: old atomic plane state
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 206f495bbf06..965faf082a6d 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1331,6 +1331,32 @@ struct drm_plane_helper_funcs {
 	 */
 	void (*atomic_update)(struct drm_plane *plane,
 			      struct drm_atomic_state *state);
+
+	/**
+	 * @atomic_enable:
+	 *
+	 * Drivers should use this function to unconditionally enable a plane.
+	 * This hook is called in-between the &drm_crtc_helper_funcs.atomic_begin
+	 * and drm_crtc_helper_funcs.atomic_flush callbacks. It is called after
+	 * @atomic_update, which will be called for all enabled planes. Drivers
+	 * that use @atomic_enable should set up a plane in @atomic_update and
+	 * afterwards enable the plane in @atomic_enable. If a plane needs to be
+	 * enabled before installing the scanout buffer, drivers can still do
+	 * so in @atomic_update.
+	 *
+	 * Note that the power state of the display pipe when this function is
+	 * called depends upon the exact helpers and calling sequence the driver
+	 * has picked. See drm_atomic_helper_commit_planes() for a discussion of
+	 * the tradeoffs and variants of plane commit helpers.
+	 *
+	 * This callback is used by the atomic modeset helpers, but it is
+	 * optional. If implemented, @atomic_enable should be the inverse of
+	 * @atomic_disable. Drivers that don't want to use either can still
+	 * implement the complete plane update in @atomic_update.
+	 */
+	void (*atomic_enable)(struct drm_plane *plane,
+			      struct drm_atomic_state *state);
+
 	/**
 	 * @atomic_disable:
 	 *
@@ -1351,7 +1377,8 @@ struct drm_plane_helper_funcs {
 	 * the tradeoffs and variants of plane commit helpers.
 	 *
 	 * This callback is used by the atomic modeset helpers and by the
-	 * transitional plane helpers, but it is optional.
+	 * transitional plane helpers, but it is optional. It's intended to
+	 * reverse the effects of @atomic_enable.
 	 */
 	void (*atomic_disable)(struct drm_plane *plane,
 			       struct drm_atomic_state *state);
-- 
2.39.1

Re: [PATCH 1/6] drm/atomic-helper: Add atomic_enable plane-helper callback

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Add atomic_enable to struct drm_plane_helper_funcs. It enables a
> plane independently from updating the plane's content. As such, it is
> the inverse of the atomic_disable plane helper. Useful for hardware
> where plane enable state is independent from plane content.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Thanks a lot for adding this. I remember wanting a plane .atomic_enable in
the past, and due not having one ended adding the enable/disable logic in
the encoder .atomic_{en,dis}able callbacks instead.

From my understanding of the atomic state helpers the patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

[PATCH 2/6] drm/ast: Implement struct drm_plane_helper_funcs.atomic_enable

[Thomas Zimmermann]

Enable the primary plane for ast hardware via atomic_enable. Atomic
helpers invoke this callback only when the plane becomes active.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index dcb8ced4ce75..7e8a6a39de35 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -672,17 +672,28 @@ static void ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
 
 	/*
 	 * Some BMCs stop scanning out the video signal after the driver
-	 * reprogrammed the offset or scanout address. This stalls display
-	 * output for several seconds and makes the display unusable.
-	 * Therefore only update the offset if it changes and reprogram the
-	 * address after enabling the plane.
+	 * reprogrammed the offset. This stalls display output for several
+	 * seconds and makes the display unusable. Therefore only update
+	 * the offset if it changes.
 	 */
 	if (!old_fb || old_fb->pitches[0] != fb->pitches[0])
 		ast_set_offset_reg(ast, fb);
-	if (!old_fb) {
-		ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
-		ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
-	}
+}
+
+static void ast_primary_plane_helper_atomic_enable(struct drm_plane *plane,
+						   struct drm_atomic_state *state)
+{
+	struct ast_private *ast = to_ast_private(plane->dev);
+	struct ast_plane *ast_plane = to_ast_plane(plane);
+
+	/*
+	 * Some BMCs stop scanning out the video signal after the driver
+	 * reprogrammed the scanout address. This stalls display
+	 * output for several seconds and makes the display unusable.
+	 * Therefore only reprogram the address after enabling the plane.
+	 */
+	ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
+	ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
 }
 
 static void ast_primary_plane_helper_atomic_disable(struct drm_plane *plane,
@@ -697,6 +708,7 @@ static const struct drm_plane_helper_funcs ast_primary_plane_helper_funcs = {
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
 	.atomic_check = ast_primary_plane_helper_atomic_check,
 	.atomic_update = ast_primary_plane_helper_atomic_update,
+	.atomic_enable = ast_primary_plane_helper_atomic_enable,
 	.atomic_disable = ast_primary_plane_helper_atomic_disable,
 };
 
-- 
2.39.1

Re: [PATCH 2/6] drm/ast: Implement struct drm_plane_helper_funcs.atomic_enable

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Enable the primary plane for ast hardware via atomic_enable. Atomic
> helpers invoke this callback only when the plane becomes active.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

[PATCH 3/6] drm/mgag200: Remove disable handling from atomic_update

[Thomas Zimmermann]

The primary plane has the atomic_disable helper set, so atomic_update
won't be called if the plane gets disabled. Remove the respective branch
from the helper.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 0a5aaf78172a..47e86eadb239 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -503,9 +503,6 @@ void mgag200_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	struct drm_rect damage;
 	u8 seq1;
 
-	if (!fb)
-		return;
-
 	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
 	drm_atomic_for_each_plane_damage(&iter, &damage) {
 		mgag200_handle_damage(mdev, shadow_plane_state->data, fb, &damage);
-- 
2.39.1

Re: [PATCH 3/6] drm/mgag200: Remove disable handling from atomic_update

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 685 bytes --]

This mail never made it to dri-devel.

Am 17.02.23 um 14:26 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
>> The primary plane has the atomic_disable helper set, so atomic_update
>> won't be called if the plane gets disabled. Remove the respective branch
>> from the helper.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Best regards,
> Javier
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/6] drm/mgag200: Remove disable handling from atomic_update

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> This mail never made it to dri-devel.
>

Oh, sorry about that. It seems that your response wasn't enough for
patchwork to pick the tag, so:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 4/6] drm/mgag200: Implement struct drm_plane_helper_funcs.atomic_enable

[Thomas Zimmermann]

Enable the primary plane for mgag200 hardware via atomic_enable.
Atomic helpers invoke this callback only when the plane becomes
active.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  3 +++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 19 ++++++++++++-------
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 9e604dbb8e44..57c7edcab602 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -375,12 +375,15 @@ int mgag200_primary_plane_helper_atomic_check(struct drm_plane *plane,
 					      struct drm_atomic_state *new_state);
 void mgag200_primary_plane_helper_atomic_update(struct drm_plane *plane,
 						struct drm_atomic_state *old_state);
+void mgag200_primary_plane_helper_atomic_enable(struct drm_plane *plane,
+						struct drm_atomic_state *state);
 void mgag200_primary_plane_helper_atomic_disable(struct drm_plane *plane,
 						 struct drm_atomic_state *old_state);
 #define MGAG200_PRIMARY_PLANE_HELPER_FUNCS \
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, \
 	.atomic_check = mgag200_primary_plane_helper_atomic_check, \
 	.atomic_update = mgag200_primary_plane_helper_atomic_update, \
+	.atomic_enable = mgag200_primary_plane_helper_atomic_enable, \
 	.atomic_disable = mgag200_primary_plane_helper_atomic_disable
 
 #define MGAG200_PRIMARY_PLANE_FUNCS \
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 47e86eadb239..0f2dd26755df 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -501,7 +501,6 @@ void mgag200_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct drm_atomic_helper_damage_iter iter;
 	struct drm_rect damage;
-	u8 seq1;
 
 	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
 	drm_atomic_for_each_plane_damage(&iter, &damage) {
@@ -511,13 +510,19 @@ void mgag200_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	/* Always scanout image at VRAM offset 0 */
 	mgag200_set_startadd(mdev, (u32)0);
 	mgag200_set_offset(mdev, fb);
+}
 
-	if (!old_plane_state->crtc && plane_state->crtc) { // enabling
-		RREG_SEQ(0x01, seq1);
-		seq1 &= ~MGAREG_SEQ1_SCROFF;
-		WREG_SEQ(0x01, seq1);
-		msleep(20);
-	}
+void mgag200_primary_plane_helper_atomic_enable(struct drm_plane *plane,
+						struct drm_atomic_state *state)
+{
+	struct drm_device *dev = plane->dev;
+	struct mga_device *mdev = to_mga_device(dev);
+	u8 seq1;
+
+	RREG_SEQ(0x01, seq1);
+	seq1 &= ~MGAREG_SEQ1_SCROFF;
+	WREG_SEQ(0x01, seq1);
+	msleep(20);
 }
 
 void mgag200_primary_plane_helper_atomic_disable(struct drm_plane *plane,
-- 
2.39.1

Re: [PATCH 4/6] drm/mgag200: Implement struct drm_plane_helper_funcs.atomic_enable

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Enable the primary plane for mgag200 hardware via atomic_enable.
> Atomic helpers invoke this callback only when the plane becomes
> active.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

[PATCH 5/6] drm/tidss: Remove return values from dispc_plane_{setup, enable}()

[Thomas Zimmermann]

Calls to dispc_plane_setup() and dispc_plane_enable() cannot fail.
Remove the return value.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++++--------
 drivers/gpu/drm/tidss/tidss_dispc.h |  8 ++++----
 drivers/gpu/drm/tidss/tidss_plane.c | 11 +----------
 3 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 165365b515e1..dca077411f77 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1985,9 +1985,9 @@ dma_addr_t dispc_plane_state_p_uv_addr(const struct drm_plane_state *state)
 		(y * fb->pitches[1] / fb->format->vsub);
 }
 
-int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
-		      const struct drm_plane_state *state,
-		      u32 hw_videoport)
+void dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
+		       const struct drm_plane_state *state,
+		       u32 hw_videoport)
 {
 	bool lite = dispc->feat->vid_lite[hw_plane];
 	u32 fourcc = state->fb->format->format;
@@ -2066,15 +2066,11 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
 	else
 		VID_REG_FLD_MOD(dispc, hw_plane, DISPC_VID_ATTRIBUTES, 0,
 				28, 28);
-
-	return 0;
 }
 
-int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable)
+void dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable)
 {
 	VID_REG_FLD_MOD(dispc, hw_plane, DISPC_VID_ATTRIBUTES, !!enable, 0, 0);
-
-	return 0;
 }
 
 static u32 dispc_vid_get_fifo_size(struct dispc_device *dispc, u32 hw_plane)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index e49432f0abf5..946ed769caaf 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -123,10 +123,10 @@ int dispc_runtime_resume(struct dispc_device *dispc);
 int dispc_plane_check(struct dispc_device *dispc, u32 hw_plane,
 		      const struct drm_plane_state *state,
 		      u32 hw_videoport);
-int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
-		      const struct drm_plane_state *state,
-		      u32 hw_videoport);
-int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
+void dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
+		       const struct drm_plane_state *state,
+		       u32 hw_videoport);
+void dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
 const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len);
 
 int dispc_init(struct tidss_device *tidss);
diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
index fe2c41f0cd4f..0b12405edb47 100644
--- a/drivers/gpu/drm/tidss/tidss_plane.c
+++ b/drivers/gpu/drm/tidss/tidss_plane.c
@@ -113,7 +113,6 @@ static void tidss_plane_atomic_update(struct drm_plane *plane,
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
 									   plane);
 	u32 hw_videoport;
-	int ret;
 
 	dev_dbg(ddev->dev, "%s\n", __func__);
 
@@ -124,15 +123,7 @@ static void tidss_plane_atomic_update(struct drm_plane *plane,
 
 	hw_videoport = to_tidss_crtc(new_state->crtc)->hw_videoport;
 
-	ret = dispc_plane_setup(tidss->dispc, tplane->hw_plane_id,
-				new_state, hw_videoport);
-
-	if (ret) {
-		dev_err(plane->dev->dev, "%s: Failed to setup plane %d\n",
-			__func__, tplane->hw_plane_id);
-		dispc_plane_enable(tidss->dispc, tplane->hw_plane_id, false);
-		return;
-	}
+	dispc_plane_setup(tidss->dispc, tplane->hw_plane_id, new_state, hw_videoport);
 
 	dispc_plane_enable(tidss->dispc, tplane->hw_plane_id, true);
 }
-- 
2.39.1

Re: [PATCH 5/6] drm/tidss: Remove return values from dispc_plane_{setup, enable}()

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Calls to dispc_plane_setup() and dispc_plane_enable() cannot fail.
> Remove the return value.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH 5/6] drm/tidss: Remove return values from dispc_plane_{setup,enable}()

[Tomi Valkeinen]

On 09/02/2023 17:41, Thomas Zimmermann wrote:
> Calls to dispc_plane_setup() and dispc_plane_enable() cannot fail.
> Remove the return value.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++++--------
>   drivers/gpu/drm/tidss/tidss_dispc.h |  8 ++++----
>   drivers/gpu/drm/tidss/tidss_plane.c | 11 +----------
>   3 files changed, 9 insertions(+), 22 deletions(-)
> 

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

[PATCH 6/6] drm/tidss: Implement struct drm_plane_helper_funcs.atomic_enable

[Thomas Zimmermann]

Enable the primary plane for tidss hardware via atomic_enable.
Atomic helpers invoke this callback only when the plane becomes
active.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tidss/tidss_plane.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
index 0b12405edb47..6bdd6e4a955a 100644
--- a/drivers/gpu/drm/tidss/tidss_plane.c
+++ b/drivers/gpu/drm/tidss/tidss_plane.c
@@ -124,6 +124,16 @@ static void tidss_plane_atomic_update(struct drm_plane *plane,
 	hw_videoport = to_tidss_crtc(new_state->crtc)->hw_videoport;
 
 	dispc_plane_setup(tidss->dispc, tplane->hw_plane_id, new_state, hw_videoport);
+}
+
+static void tidss_plane_atomic_enable(struct drm_plane *plane,
+				      struct drm_atomic_state *state)
+{
+	struct drm_device *ddev = plane->dev;
+	struct tidss_device *tidss = to_tidss(ddev);
+	struct tidss_plane *tplane = to_tidss_plane(plane);
+
+	dev_dbg(ddev->dev, "%s\n", __func__);
 
 	dispc_plane_enable(tidss->dispc, tplane->hw_plane_id, true);
 }
@@ -151,6 +161,7 @@ static void drm_plane_destroy(struct drm_plane *plane)
 static const struct drm_plane_helper_funcs tidss_plane_helper_funcs = {
 	.atomic_check = tidss_plane_atomic_check,
 	.atomic_update = tidss_plane_atomic_update,
+	.atomic_enable = tidss_plane_atomic_enable,
 	.atomic_disable = tidss_plane_atomic_disable,
 };
 
-- 
2.39.1

Re: [PATCH 6/6] drm/tidss: Implement struct drm_plane_helper_funcs.atomic_enable

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Enable the primary plane for tidss hardware via atomic_enable.
> Atomic helpers invoke this callback only when the plane becomes
> active.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH 6/6] drm/tidss: Implement struct drm_plane_helper_funcs.atomic_enable

[Tomi Valkeinen]

On 09/02/2023 17:41, Thomas Zimmermann wrote:
> Enable the primary plane for tidss hardware via atomic_enable.
> Atomic helpers invoke this callback only when the plane becomes
> active.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/tidss/tidss_plane.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
> index 0b12405edb47..6bdd6e4a955a 100644
> --- a/drivers/gpu/drm/tidss/tidss_plane.c
> +++ b/drivers/gpu/drm/tidss/tidss_plane.c
> @@ -124,6 +124,16 @@ static void tidss_plane_atomic_update(struct drm_plane *plane,
>   	hw_videoport = to_tidss_crtc(new_state->crtc)->hw_videoport;
>   
>   	dispc_plane_setup(tidss->dispc, tplane->hw_plane_id, new_state, hw_videoport);
> +}
> +
> +static void tidss_plane_atomic_enable(struct drm_plane *plane,
> +				      struct drm_atomic_state *state)
> +{
> +	struct drm_device *ddev = plane->dev;
> +	struct tidss_device *tidss = to_tidss(ddev);
> +	struct tidss_plane *tplane = to_tidss_plane(plane);
> +
> +	dev_dbg(ddev->dev, "%s\n", __func__);
>   
>   	dispc_plane_enable(tidss->dispc, tplane->hw_plane_id, true);
>   }
> @@ -151,6 +161,7 @@ static void drm_plane_destroy(struct drm_plane *plane)
>   static const struct drm_plane_helper_funcs tidss_plane_helper_funcs = {
>   	.atomic_check = tidss_plane_atomic_check,
>   	.atomic_update = tidss_plane_atomic_update,
> +	.atomic_enable = tidss_plane_atomic_enable,
>   	.atomic_disable = tidss_plane_atomic_disable,
>   };
>   

I haven't tested this, but looks fine to me.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

One thought, though, is that we still do dispc_plane_enable(false) in 
tidss_plane_atomic_update() when the plane is not visible. Not a 
problem, but it would be nice to only enable/disable the plane inside 
atomic_enable/disable.

Or maybe in cases like this the driver should only use atomic_update, 
and do all the enabling and disabling there...

  Tomi

Re: [PATCH 6/6] drm/tidss: Implement struct drm_plane_helper_funcs.atomic_enable

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2936 bytes --]

Hi

Am 17.02.23 um 15:42 schrieb Tomi Valkeinen:
> On 09/02/2023 17:41, Thomas Zimmermann wrote:
>> Enable the primary plane for tidss hardware via atomic_enable.
>> Atomic helpers invoke this callback only when the plane becomes
>> active.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/tidss/tidss_plane.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_plane.c 
>> b/drivers/gpu/drm/tidss/tidss_plane.c
>> index 0b12405edb47..6bdd6e4a955a 100644
>> --- a/drivers/gpu/drm/tidss/tidss_plane.c
>> +++ b/drivers/gpu/drm/tidss/tidss_plane.c
>> @@ -124,6 +124,16 @@ static void tidss_plane_atomic_update(struct 
>> drm_plane *plane,
>>       hw_videoport = to_tidss_crtc(new_state->crtc)->hw_videoport;
>>       dispc_plane_setup(tidss->dispc, tplane->hw_plane_id, new_state, 
>> hw_videoport);
>> +}
>> +
>> +static void tidss_plane_atomic_enable(struct drm_plane *plane,
>> +                      struct drm_atomic_state *state)
>> +{
>> +    struct drm_device *ddev = plane->dev;
>> +    struct tidss_device *tidss = to_tidss(ddev);
>> +    struct tidss_plane *tplane = to_tidss_plane(plane);
>> +
>> +    dev_dbg(ddev->dev, "%s\n", __func__);
>>       dispc_plane_enable(tidss->dispc, tplane->hw_plane_id, true);
>>   }
>> @@ -151,6 +161,7 @@ static void drm_plane_destroy(struct drm_plane 
>> *plane)
>>   static const struct drm_plane_helper_funcs tidss_plane_helper_funcs = {
>>       .atomic_check = tidss_plane_atomic_check,
>>       .atomic_update = tidss_plane_atomic_update,
>> +    .atomic_enable = tidss_plane_atomic_enable,
>>       .atomic_disable = tidss_plane_atomic_disable,
>>   };
> 
> I haven't tested this, but looks fine to me.
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
> One thought, though, is that we still do dispc_plane_enable(false) in 
> tidss_plane_atomic_update() when the plane is not visible. Not a 
> problem, but it would be nice to only enable/disable the plane inside 
> atomic_enable/disable.
> 
> Or maybe in cases like this the driver should only use atomic_update, 
> and do all the enabling and disabling there...

I agree. Drivers that have complex enable/disable semantics should 
probably handle everything in atomic_update.

Enabling/disabling is currently connected to the plane's framebuffer. As 
you said, it would be nice if this could be tied to visibility instead. 
The patch would be trivial, but some drivers might not like the change. 
I guess we could do an RFC patch and gather opinions.

Best regards
Thomas

> 
>   Tomi
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]