[PATCH 0/3] Add a drm_crtc_helper_atomic_check() helper

[PATCH 0/3] Add a drm_crtc_helper_atomic_check() helper

[Javier Martinez Canillas]

Add a helper function and make drivers that have the same logic in their
struct drm_crtc_helper_funcs .atomic_check handler to use this instead.

Patch #1 and #2 are just cleanups for the simpledrm and ssd130x drivers
respectively, so that these can be converted to use the helper added by
patch #3. The changes are inspired by a patch from Thomas Zimmermann for
the ast DRM driver:

https://patchwork.kernel.org/project/dri-devel/patch/20221010103625.19958-4-tzimmermann@suse.de/

Best regards,
Javier


Javier Martinez Canillas (3):
  drm/simpledrm: Do not call drm_atomic_add_affected_planes()
  drm/ssd130x: Do not call drm_atomic_add_affected_planes()
  drm/crtc-helper: Add a drm_crtc_helper_atomic_check() helper

 drivers/gpu/drm/drm_crtc_helper.c | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/solomon/ssd130x.c | 20 ++------------------
 drivers/gpu/drm/tiny/simpledrm.c  | 20 ++------------------
 include/drm/drm_crtc_helper.h     |  2 ++
 4 files changed, 30 insertions(+), 36 deletions(-)

-- 
2.37.3

[PATCH 1/3] drm/simpledrm: Do not call drm_atomic_add_affected_planes()

[Javier Martinez Canillas]

There's no need to add planes to the atomic state. Remove the call
to drm_atomic_add_affected_planes() from simpledrm.

On full modesets, the DRM helpers already add a CRTC's planes to the
atomic state; see drm_atomic_helper_check_modeset(). There's no reason
to call drm_atomic_add_affected_planes() unconditionally in the CRTC's
atomic_check() in simpledrm. It's also too late, as the atomic_check()
of the added planes will not be called before the commit.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/tiny/simpledrm.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index ecd49a8f3334..f03f17f62a56 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -549,17 +549,11 @@ static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
 					      struct drm_atomic_state *new_state)
 {
 	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
-	int ret;
 
 	if (!new_crtc_state->enable)
-		goto out;
-
-	ret = drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
-	if (ret)
-		return ret;
+		return 0;
 
-out:
-	return drm_atomic_add_affected_planes(new_state, crtc);
+	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
 }
 
 /*
-- 
2.37.3

Re: [PATCH 1/3] drm/simpledrm: Do not call drm_atomic_add_affected_planes()

[Thomas Zimmermann]

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

Hi

Am 10.10.22 um 19:02 schrieb Javier Martinez Canillas:
> There's no need to add planes to the atomic state. Remove the call
> to drm_atomic_add_affected_planes() from simpledrm.
> 
> On full modesets, the DRM helpers already add a CRTC's planes to the
> atomic state; see drm_atomic_helper_check_modeset(). There's no reason
> to call drm_atomic_add_affected_planes() unconditionally in the CRTC's
> atomic_check() in simpledrm. It's also too late, as the atomic_check()
> of the added planes will not be called before the commit.
> 
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

There's also drm_atomic_add_affected_planes() in mgag200. Since you're 
at it, I'd appreciate a patch.

Best regards
Thomas

> ---
> 
>   drivers/gpu/drm/tiny/simpledrm.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index ecd49a8f3334..f03f17f62a56 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -549,17 +549,11 @@ static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
>   					      struct drm_atomic_state *new_state)
>   {
>   	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
> -	int ret;
>   
>   	if (!new_crtc_state->enable)
> -		goto out;
> -
> -	ret = drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
> -	if (ret)
> -		return ret;
> +		return 0;
>   
> -out:
> -	return drm_atomic_add_affected_planes(new_state, crtc);
> +	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
>   }
>   
>   /*

-- 
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 1/3] drm/simpledrm: Do not call drm_atomic_add_affected_planes()

[Javier Martinez Canillas]

Hello Thomas,

On 10/11/22 15:06, Thomas Zimmermann wrote:
> Hi
> 
> Am 10.10.22 um 19:02 schrieb Javier Martinez Canillas:
>> There's no need to add planes to the atomic state. Remove the call
>> to drm_atomic_add_affected_planes() from simpledrm.
>>
>> On full modesets, the DRM helpers already add a CRTC's planes to the
>> atomic state; see drm_atomic_helper_check_modeset(). There's no reason
>> to call drm_atomic_add_affected_planes() unconditionally in the CRTC's
>> atomic_check() in simpledrm. It's also too late, as the atomic_check()
>> of the added planes will not be called before the commit.
>>
>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>

Thanks.
 
> There's also drm_atomic_add_affected_planes() in mgag200. Since you're 
> at it, I'd appreciate a patch.
>

Sure, I'll include in v2. I noticed that but didn't feel like posting
a patch because I'm not familiar with that device nor have HW to test.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 2/3] drm/ssd130x: Do not call drm_atomic_add_affected_planes()

[Javier Martinez Canillas]

There's no need to add planes to the atomic state. Remove the call
to drm_atomic_add_affected_planes() from ssd130x.

On full modesets, the DRM helpers already add a CRTC's planes to the
atomic state; see drm_atomic_helper_check_modeset(). There's no reason
to call drm_atomic_add_affected_planes() unconditionally in the CRTC's
atomic_check() in ssd130x. It's also too late, as the atomic_check()
of the added planes will not be called before the commit.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/solomon/ssd130x.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 57e48355c008..0d4ab65233db 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -649,17 +649,11 @@ static int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc,
 					    struct drm_atomic_state *new_state)
 {
 	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
-	int ret;
 
 	if (!new_crtc_state->enable)
-		goto out;
-
-	ret = drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
-	if (ret)
-		return ret;
+		return 0;
 
-out:
-	return drm_atomic_add_affected_planes(new_state, crtc);
+	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
 }
 
 /*
-- 
2.37.3

Re: [PATCH 2/3] drm/ssd130x: Do not call drm_atomic_add_affected_planes()

[Thomas Zimmermann]

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



Am 10.10.22 um 19:02 schrieb Javier Martinez Canillas:
> There's no need to add planes to the atomic state. Remove the call
> to drm_atomic_add_affected_planes() from ssd130x.
> 
> On full modesets, the DRM helpers already add a CRTC's planes to the
> atomic state; see drm_atomic_helper_check_modeset(). There's no reason
> to call drm_atomic_add_affected_planes() unconditionally in the CRTC's
> atomic_check() in ssd130x. It's also too late, as the atomic_check()
> of the added planes will not be called before the commit.
> 
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
> 
>   drivers/gpu/drm/solomon/ssd130x.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 57e48355c008..0d4ab65233db 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -649,17 +649,11 @@ static int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc,
>   					    struct drm_atomic_state *new_state)
>   {
>   	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
> -	int ret;
>   
>   	if (!new_crtc_state->enable)
> -		goto out;
> -
> -	ret = drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
> -	if (ret)
> -		return ret;
> +		return 0;
>   
> -out:
> -	return drm_atomic_add_affected_planes(new_state, crtc);
> +	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
>   }
>   
>   /*

-- 
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 --]

[PATCH 3/3] drm/crtc-helper: Add a drm_crtc_helper_atomic_check() helper

[Javier Martinez Canillas]

Provides a default CRTC state check handler for CRTCs that only have one
primary plane attached.

There are some drivers that duplicate this logic in their helpers, such as
simpledrm and ssd130x. Factor out this common code into a CRTC helper and
make drivers use it.

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

 drivers/gpu/drm/drm_crtc_helper.c | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/solomon/ssd130x.c | 14 ++------------
 drivers/gpu/drm/tiny/simpledrm.c  | 14 ++------------
 include/drm/drm_crtc_helper.h     |  2 ++
 4 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 457448cc60f7..4ad3abaa98f4 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -421,6 +421,30 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
 }
 EXPORT_SYMBOL(drm_crtc_helper_set_mode);
 
+/**
+ * drm_crtc_helper_atomic_check() - Helper to check CRTC atomic-state
+ * @crtc: CRTC to check
+ * @state: atomic state object
+ *
+ * Provides a default CRTC-state check handler for CRTCs that only have
+ * one primary plane attached to it.
+ *
+ * This is often the case for the CRTC of simple framebuffers.
+ *
+ * RETURNS:
+ * Zero on success, or an errno code otherwise.
+ */
+int drm_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+	if (!new_crtc_state->enable)
+		return 0;
+
+	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
+}
+EXPORT_SYMBOL(drm_crtc_helper_atomic_check);
+
 static void
 drm_crtc_helper_disable(struct drm_crtc *crtc)
 {
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 0d4ab65233db..f2795f90ea69 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -20,6 +20,7 @@
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
 #include <drm/drm_damage_helper.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fb_helper.h>
@@ -645,17 +646,6 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
 	return MODE_OK;
 }
 
-static int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc,
-					    struct drm_atomic_state *new_state)
-{
-	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
-
-	if (!new_crtc_state->enable)
-		return 0;
-
-	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
-}
-
 /*
  * The CRTC is always enabled. Screen updates are performed by
  * the primary plane's atomic_update function. Disabling clears
@@ -663,7 +653,7 @@ static int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc,
  */
 static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
 	.mode_valid = ssd130x_crtc_helper_mode_valid,
-	.atomic_check = ssd130x_crtc_helper_atomic_check,
+	.atomic_check = drm_crtc_helper_atomic_check,
 };
 
 static void ssd130x_crtc_reset(struct drm_crtc *crtc)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index f03f17f62a56..cbb100753154 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -11,6 +11,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_connector.h>
+#include <drm/drm_crtc_helper.h>
 #include <drm/drm_damage_helper.h>
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
@@ -545,17 +546,6 @@ static enum drm_mode_status simpledrm_crtc_helper_mode_valid(struct drm_crtc *cr
 	return drm_crtc_helper_mode_valid_fixed(crtc, mode, &sdev->mode);
 }
 
-static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
-					      struct drm_atomic_state *new_state)
-{
-	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
-
-	if (!new_crtc_state->enable)
-		return 0;
-
-	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
-}
-
 /*
  * The CRTC is always enabled. Screen updates are performed by
  * the primary plane's atomic_update function. Disabling clears
@@ -563,7 +553,7 @@ static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
  */
 static const struct drm_crtc_helper_funcs simpledrm_crtc_helper_funcs = {
 	.mode_valid = simpledrm_crtc_helper_mode_valid,
-	.atomic_check = simpledrm_crtc_helper_atomic_check,
+	.atomic_check = drm_crtc_helper_atomic_check,
 };
 
 static const struct drm_crtc_funcs simpledrm_crtc_funcs = {
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index a6d520d5b6ca..1840db247f69 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -50,6 +50,8 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
 			      struct drm_display_mode *mode,
 			      int x, int y,
 			      struct drm_framebuffer *old_fb);
+int drm_crtc_helper_atomic_check(struct drm_crtc *crtc,
+				 struct drm_atomic_state *state);
 bool drm_helper_crtc_in_use(struct drm_crtc *crtc);
 bool drm_helper_encoder_in_use(struct drm_encoder *encoder);
 
-- 
2.37.3

Re: [PATCH 3/3] drm/crtc-helper: Add a drm_crtc_helper_atomic_check() helper

[Thomas Zimmermann]

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

Hi

Am 10.10.22 um 19:02 schrieb Javier Martinez Canillas:
> Provides a default CRTC state check handler for CRTCs that only have one
> primary plane attached.
> 
> There are some drivers that duplicate this logic in their helpers, such as
> simpledrm and ssd130x. Factor out this common code into a CRTC helper and
> make drivers use it.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

There really isn't much here for now. I suspect that there are more 
drivers that could use this helper. If you merge this before ofdrm, I'll 
rebase ofdrm on top.

Please also see my comment below.


> ---
> 
>   drivers/gpu/drm/drm_crtc_helper.c | 24 ++++++++++++++++++++++++
>   drivers/gpu/drm/solomon/ssd130x.c | 14 ++------------
>   drivers/gpu/drm/tiny/simpledrm.c  | 14 ++------------
>   include/drm/drm_crtc_helper.h     |  2 ++
>   4 files changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 457448cc60f7..4ad3abaa98f4 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -421,6 +421,30 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>   }
>   EXPORT_SYMBOL(drm_crtc_helper_set_mode);
>   
> +/**
> + * drm_crtc_helper_atomic_check() - Helper to check CRTC atomic-state
> + * @crtc: CRTC to check
> + * @state: atomic state object
> + *
> + * Provides a default CRTC-state check handler for CRTCs that only have
> + * one primary plane attached to it.
> + *
> + * This is often the case for the CRTC of simple framebuffers.

I'd add a reference to drm_plane_helper_atomic_check() to this 
paragraph. Like

   See drm_plane_helper_atomic_check() for the respective plane helpers.

And also reference back from the plane-check helper to the CRTC-check 
helper.

Best regards
Thomas

> + *
> + * RETURNS:
> + * Zero on success, or an errno code otherwise.
> + */
> +int drm_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> +	if (!new_crtc_state->enable)
> +		return 0;
> +
> +	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
> +}
> +EXPORT_SYMBOL(drm_crtc_helper_atomic_check);
> +
>   static void
>   drm_crtc_helper_disable(struct drm_crtc *crtc)
>   {
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 0d4ab65233db..f2795f90ea69 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -20,6 +20,7 @@
>   
>   #include <drm/drm_atomic.h>
>   #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
>   #include <drm/drm_damage_helper.h>
>   #include <drm/drm_edid.h>
>   #include <drm/drm_fb_helper.h>
> @@ -645,17 +646,6 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
>   	return MODE_OK;
>   }
>   
> -static int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc,
> -					    struct drm_atomic_state *new_state)
> -{
> -	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
> -
> -	if (!new_crtc_state->enable)
> -		return 0;
> -
> -	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
> -}
> -
>   /*
>    * The CRTC is always enabled. Screen updates are performed by
>    * the primary plane's atomic_update function. Disabling clears
> @@ -663,7 +653,7 @@ static int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc,
>    */
>   static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
>   	.mode_valid = ssd130x_crtc_helper_mode_valid,
> -	.atomic_check = ssd130x_crtc_helper_atomic_check,
> +	.atomic_check = drm_crtc_helper_atomic_check,
>   };
>   
>   static void ssd130x_crtc_reset(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index f03f17f62a56..cbb100753154 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -11,6 +11,7 @@
>   #include <drm/drm_atomic.h>
>   #include <drm/drm_atomic_state_helper.h>
>   #include <drm/drm_connector.h>
> +#include <drm/drm_crtc_helper.h>
>   #include <drm/drm_damage_helper.h>
>   #include <drm/drm_device.h>
>   #include <drm/drm_drv.h>
> @@ -545,17 +546,6 @@ static enum drm_mode_status simpledrm_crtc_helper_mode_valid(struct drm_crtc *cr
>   	return drm_crtc_helper_mode_valid_fixed(crtc, mode, &sdev->mode);
>   }
>   
> -static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
> -					      struct drm_atomic_state *new_state)
> -{
> -	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
> -
> -	if (!new_crtc_state->enable)
> -		return 0;
> -
> -	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
> -}
> -
>   /*
>    * The CRTC is always enabled. Screen updates are performed by
>    * the primary plane's atomic_update function. Disabling clears
> @@ -563,7 +553,7 @@ static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
>    */
>   static const struct drm_crtc_helper_funcs simpledrm_crtc_helper_funcs = {
>   	.mode_valid = simpledrm_crtc_helper_mode_valid,
> -	.atomic_check = simpledrm_crtc_helper_atomic_check,
> +	.atomic_check = drm_crtc_helper_atomic_check,
>   };
>   
>   static const struct drm_crtc_funcs simpledrm_crtc_funcs = {
> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
> index a6d520d5b6ca..1840db247f69 100644
> --- a/include/drm/drm_crtc_helper.h
> +++ b/include/drm/drm_crtc_helper.h
> @@ -50,6 +50,8 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>   			      struct drm_display_mode *mode,
>   			      int x, int y,
>   			      struct drm_framebuffer *old_fb);
> +int drm_crtc_helper_atomic_check(struct drm_crtc *crtc,
> +				 struct drm_atomic_state *state);
>   bool drm_helper_crtc_in_use(struct drm_crtc *crtc);
>   bool drm_helper_encoder_in_use(struct drm_encoder *encoder);
>   

-- 
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/3] drm/crtc-helper: Add a drm_crtc_helper_atomic_check() helper

[Javier Martinez Canillas]

On 10/11/22 15:21, Thomas Zimmermann wrote:
> Hi
> 
> Am 10.10.22 um 19:02 schrieb Javier Martinez Canillas:
>> Provides a default CRTC state check handler for CRTCs that only have one
>> primary plane attached.
>>
>> There are some drivers that duplicate this logic in their helpers, such as
>> simpledrm and ssd130x. Factor out this common code into a CRTC helper and
>> make drivers use it.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> There really isn't much here for now. I suspect that there are more 
> drivers that could use this helper. If you merge this before ofdrm, I'll 
> rebase ofdrm on top.
>

Sure. I probably won't post a v2 until tomorrow and I believe ofdrm is
ready to be merged, so I'll just rebase this series on top of that once
just push it.

> Please also see my comment below.
> 
> 

[...]

>> +/**
>> + * drm_crtc_helper_atomic_check() - Helper to check CRTC atomic-state
>> + * @crtc: CRTC to check
>> + * @state: atomic state object
>> + *
>> + * Provides a default CRTC-state check handler for CRTCs that only have
>> + * one primary plane attached to it.
>> + *
>> + * This is often the case for the CRTC of simple framebuffers.
> 
> I'd add a reference to drm_plane_helper_atomic_check() to this 
> paragraph. Like
> 
>    See drm_plane_helper_atomic_check() for the respective plane helpers.
> 
> And also reference back from the plane-check helper to the CRTC-check 
> helper.
>

Good idea, I'll do that. Thanks for your review.
 
> Best regards
> Thomas
> 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 3/3] drm/crtc-helper: Add a drm_crtc_helper_atomic_check() helper

[Thomas Zimmermann]

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

Hi

Am 11.10.22 um 15:26 schrieb Javier Martinez Canillas:
> On 10/11/22 15:21, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 10.10.22 um 19:02 schrieb Javier Martinez Canillas:
>>> Provides a default CRTC state check handler for CRTCs that only have one
>>> primary plane attached.
>>>
>>> There are some drivers that duplicate this logic in their helpers, such as
>>> simpledrm and ssd130x. Factor out this common code into a CRTC helper and
>>> make drivers use it.
>>>
>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>> There really isn't much here for now. I suspect that there are more
>> drivers that could use this helper. If you merge this before ofdrm, I'll
>> rebase ofdrm on top.
>>
> 
> Sure. I probably won't post a v2 until tomorrow and I believe ofdrm is
> ready to be merged, so I'll just rebase this series on top of that once
> just push it.

I just realized that this function in ofdrm has additional code for 
color management. There won't be anything to convert.

Best regards
Thomas

> 
>> Please also see my comment below.
>>
>>
> 
> [...]
> 
>>> +/**
>>> + * drm_crtc_helper_atomic_check() - Helper to check CRTC atomic-state
>>> + * @crtc: CRTC to check
>>> + * @state: atomic state object
>>> + *
>>> + * Provides a default CRTC-state check handler for CRTCs that only have
>>> + * one primary plane attached to it.
>>> + *
>>> + * This is often the case for the CRTC of simple framebuffers.
>>
>> I'd add a reference to drm_plane_helper_atomic_check() to this
>> paragraph. Like
>>
>>     See drm_plane_helper_atomic_check() for the respective plane helpers.
>>
>> And also reference back from the plane-check helper to the CRTC-check
>> helper.
>>
> 
> Good idea, I'll do that. Thanks for your review.
>   
>> Best regards
>> Thomas
>>
> 

-- 
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 --]