[PATCH v4 0/2] drm/atomic-helpers: Fix CRTC primary-plane test

[PATCH v4 0/2] drm/atomic-helpers: Fix CRTC primary-plane test

[Thomas Zimmermann]

(was: drm/atomic-helper: Don't allocated plane state in CRTC check)

Fix the test for a CRTC's primary plane and clean up the test function
to only do the test. Up to v3, this patchset was a single patch, but
the cleanup turns it into a series.

v4:
	* clean up the helper function (Ville)

Thomas Zimmermann (2):
  drm/atomic-helper: Don't allocated plane state in CRTC check
  drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()

 drivers/gpu/drm/ast/ast_mode.c          |  8 ++--
 drivers/gpu/drm/drm_atomic_helper.c     | 60 ++++++++-----------------
 drivers/gpu/drm/drm_simple_kms_helper.c |  6 ++-
 drivers/gpu/drm/mgag200/mgag200_mode.c  |  8 ++--
 drivers/gpu/drm/solomon/ssd130x.c       |  6 ++-
 drivers/gpu/drm/tiny/simpledrm.c        |  6 ++-
 include/drm/drm_atomic_helper.h         |  3 +-
 7 files changed, 42 insertions(+), 55 deletions(-)

-- 
2.37.3

[PATCH v4 1/2] drm/atomic-helper: Don't allocated plane state in CRTC check

[Thomas Zimmermann]

In drm_atomic_helper_check_crtc_state(), do not add a new plane state
to the global state if it does not exist already. Adding a new plane
state will result in overhead for the plane during the atomic-commit
step.

For the test in drm_atomic_helper_check_crtc_state() to succeed, it
is important that the CRTC has an enabled primary plane after the
commit. Simply testing the CRTC state's plane_mask for a primary plane
is sufficient.

Note that the helper still only tests for an attached primary plane.
Drivers have to ensure that the plane contains valid pixel information.

v3:
	* test for a primary plane in plane_mask (Ville)
v2:
	* remove unnecessary test for plane->crtc (Ville)
	* inline drm_atomic_get_next_plane_state() (Ville)
	* acquire plane lock before accessing plane->state (Ville)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Jocelyn Falempe <jfalempe@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_atomic_helper.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 98cc3137c062..02b4a7dc92f5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -945,7 +945,6 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
 				       bool can_disable_primary_planes)
 {
 	struct drm_device *dev = crtc_state->crtc->dev;
-	struct drm_atomic_state *state = crtc_state->state;
 
 	if (!crtc_state->enable)
 		return 0;
@@ -956,14 +955,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
 		struct drm_plane *plane;
 
 		drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
-			struct drm_plane_state *plane_state;
-
-			if (plane->type != DRM_PLANE_TYPE_PRIMARY)
-				continue;
-			plane_state = drm_atomic_get_plane_state(state, plane);
-			if (IS_ERR(plane_state))
-				return PTR_ERR(plane_state);
-			if (plane_state->fb && plane_state->crtc) {
+			if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
 				has_primary_plane = true;
 				break;
 			}
-- 
2.37.3

Re: [PATCH v4 1/2] drm/atomic-helper: Don't allocated plane state in CRTC check

[Javier Martinez Canillas]

Hello Thomas,

On 10/5/22 13:40, Thomas Zimmermann wrote:
> In drm_atomic_helper_check_crtc_state(), do not add a new plane state
> to the global state if it does not exist already. Adding a new plane
> state will result in overhead for the plane during the atomic-commit
> step.
> 
> For the test in drm_atomic_helper_check_crtc_state() to succeed, it
> is important that the CRTC has an enabled primary plane after the
> commit. Simply testing the CRTC state's plane_mask for a primary plane
> is sufficient.
> 
> Note that the helper still only tests for an attached primary plane.
> Drivers have to ensure that the plane contains valid pixel information.
> 
> v3:
> 	* test for a primary plane in plane_mask (Ville)
> v2:
> 	* remove unnecessary test for plane->crtc (Ville)
> 	* inline drm_atomic_get_next_plane_state() (Ville)
> 	* acquire plane lock before accessing plane->state (Ville)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")

This patch makes sense to me.

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

but I've a hard time parsing the subject line. Did you mean instead:

"drm/atomic-helper: Don't allocate new plane state in CRTC check" ?

or "drm/atomic-helper: Don't add a new plane state in CRTC check" ?

In any case you can fix that while applying so no need to resend IMO.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 1/2] drm/atomic-helper: Don't allocated plane state in CRTC check

[Thomas Zimmermann]

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

Hi

Am 06.10.22 um 22:18 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 10/5/22 13:40, Thomas Zimmermann wrote:
>> In drm_atomic_helper_check_crtc_state(), do not add a new plane state
>> to the global state if it does not exist already. Adding a new plane
>> state will result in overhead for the plane during the atomic-commit
>> step.
>>
>> For the test in drm_atomic_helper_check_crtc_state() to succeed, it
>> is important that the CRTC has an enabled primary plane after the
>> commit. Simply testing the CRTC state's plane_mask for a primary plane
>> is sufficient.
>>
>> Note that the helper still only tests for an attached primary plane.
>> Drivers have to ensure that the plane contains valid pixel information.
>>
>> v3:
>> 	* test for a primary plane in plane_mask (Ville)
>> v2:
>> 	* remove unnecessary test for plane->crtc (Ville)
>> 	* inline drm_atomic_get_next_plane_state() (Ville)
>> 	* acquire plane lock before accessing plane->state (Ville)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
> 
> This patch makes sense to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> but I've a hard time parsing the subject line. Did you mean instead:
> 
> "drm/atomic-helper: Don't allocate new plane state in CRTC check" ?

Ok, I'll do that.

Best regard
Thomas

> 
> or "drm/atomic-helper: Don't add a new plane state in CRTC check" ?
> 
> In any case you can fix that while applying so no need to resend IMO.
> 

-- 
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 v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()

[Thomas Zimmermann]

Rename the atomic helper function drm_atomic_helper_check_crtc_state()
to drm_atomic_helper_check_crtc_primary_plane() and only check for an
attached primary plane. Adapt callers.

Instead of having one big function to check for various CRTC state
conditions, we rather want smaller functions that drivers can pick
individually.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c          |  8 ++--
 drivers/gpu/drm/drm_atomic_helper.c     | 52 +++++++++----------------
 drivers/gpu/drm/drm_simple_kms_helper.c |  6 ++-
 drivers/gpu/drm/mgag200/mgag200_mode.c  |  8 ++--
 drivers/gpu/drm/solomon/ssd130x.c       |  6 ++-
 drivers/gpu/drm/tiny/simpledrm.c        |  6 ++-
 include/drm/drm_atomic_helper.h         |  3 +-
 7 files changed, 42 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 1bc0220e6783..d598b1e1447f 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1129,13 +1129,13 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
 	bool succ;
 	int ret;
 
-	ret = drm_atomic_helper_check_crtc_state(crtc_state, false);
-	if (ret)
-		return ret;
-
 	if (!crtc_state->enable)
 		goto out;
 
+	ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
+	if (ret)
+		return ret;
+
 	ast_state = to_ast_crtc_state(crtc_state);
 
 	format = ast_state->format;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 02b4a7dc92f5..1a586b3c454b 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -924,51 +924,35 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
 EXPORT_SYMBOL(drm_atomic_helper_check_plane_state);
 
 /**
- * drm_atomic_helper_check_crtc_state() - Check CRTC state for validity
+ * drm_atomic_helper_check_crtc_primary_plane() - Check CRTC state for primary plane
  * @crtc_state: CRTC state to check
- * @can_disable_primary_planes: can the CRTC be enabled without a primary plane?
  *
- * Checks that a desired CRTC update is valid. Drivers that provide
- * their own CRTC handling rather than helper-provided implementations may
- * still wish to call this function to avoid duplication of error checking
- * code.
- *
- * Note that @can_disable_primary_planes only tests if the CRTC can be
- * enabled without a primary plane. To test if a primary plane can be updated
- * without a CRTC, use drm_atomic_helper_check_plane_state() in the plane's
- * atomic check.
+ * Checks that a CRTC has at least one primary plane attached to it, which is
+ * a requirement on some hardware. Note that this only involves the CRTC side
+ * of the test. To test if the primary plane is visible or if it can be updated
+ * without the CRTC being enabled, use drm_atomic_helper_check_plane_state() in
+ * the plane's atomic check.
  *
  * RETURNS:
- * Zero if update appears valid, error code on failure
+ * 0 if a primary plane is attached to the CRTC, or an error code otherwise
  */
-int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
-				       bool can_disable_primary_planes)
+int drm_atomic_helper_check_crtc_primary_plane(struct drm_crtc_state *crtc_state)
 {
-	struct drm_device *dev = crtc_state->crtc->dev;
-
-	if (!crtc_state->enable)
-		return 0;
+	struct drm_crtc *crtc = crtc_state->crtc;
+	struct drm_device *dev = crtc->dev;
+	struct drm_plane *plane;
 
 	/* needs at least one primary plane to be enabled */
-	if (!can_disable_primary_planes) {
-		bool has_primary_plane = false;
-		struct drm_plane *plane;
-
-		drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
-			if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
-				has_primary_plane = true;
-				break;
-			}
-		}
-		if (!has_primary_plane) {
-			drm_dbg_kms(dev, "Cannot enable CRTC without a primary plane.\n");
-			return -EINVAL;
-		}
+	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
+		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+			return 0;
 	}
 
-	return 0;
+	drm_dbg_atomic(dev, "[CRTC:%d:%s] primary plane missing\n", crtc->base.id, crtc->name);
+
+	return -EINVAL;
 }
-EXPORT_SYMBOL(drm_atomic_helper_check_crtc_state);
+EXPORT_SYMBOL(drm_atomic_helper_check_crtc_primary_plane);
 
 /**
  * drm_atomic_helper_check_planes - validate state object for planes changes
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index e9f782119d3d..31233c6ae3c4 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -102,10 +102,14 @@ static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
 	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	int ret;
 
-	ret = drm_atomic_helper_check_crtc_state(crtc_state, false);
+	if (!crtc_state->enable)
+		goto out;
+
+	ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
 	if (ret)
 		return ret;
 
+out:
 	return drm_atomic_add_affected_planes(state, crtc);
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index bbab2549243a..5f7eb642f0c6 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -579,13 +579,13 @@ int mgag200_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_st
 	struct drm_property_blob *new_gamma_lut = new_crtc_state->gamma_lut;
 	int ret;
 
-	ret = drm_atomic_helper_check_crtc_state(new_crtc_state, false);
-	if (ret)
-		return ret;
-
 	if (!new_crtc_state->enable)
 		return 0;
 
+	ret = drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
+	if (ret)
+		return ret;
+
 	if (new_crtc_state->mode_changed) {
 		if (funcs->pixpllc_atomic_check) {
 			ret = funcs->pixpllc_atomic_check(crtc, new_state);
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index bc41a5ae810a..473db3dc0d55 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -648,10 +648,14 @@ static int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc,
 	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
 	int ret;
 
-	ret = drm_atomic_helper_check_crtc_state(new_crtc_state, false);
+	if (!new_crtc_state->enable)
+		goto out;
+
+	ret = drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
 	if (ret)
 		return ret;
 
+out:
 	return drm_atomic_add_affected_planes(new_state, crtc);
 }
 
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 18489779fb8a..ecd49a8f3334 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -551,10 +551,14 @@ static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
 	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
 	int ret;
 
-	ret = drm_atomic_helper_check_crtc_state(new_crtc_state, false);
+	if (!new_crtc_state->enable)
+		goto out;
+
+	ret = drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
 	if (ret)
 		return ret;
 
+out:
 	return drm_atomic_add_affected_planes(new_state, crtc);
 }
 
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 06d8902a8097..33f982cd1a27 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -58,10 +58,9 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
 					int max_scale,
 					bool can_position,
 					bool can_update_disabled);
-int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
-				       bool can_disable_primary_plane);
 int drm_atomic_helper_check_planes(struct drm_device *dev,
 			       struct drm_atomic_state *state);
+int drm_atomic_helper_check_crtc_primary_plane(struct drm_crtc_state *crtc_state);
 int drm_atomic_helper_check(struct drm_device *dev,
 			    struct drm_atomic_state *state);
 void drm_atomic_helper_commit_tail(struct drm_atomic_state *state);
-- 
2.37.3

Re: [PATCH v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()

[Javier Martinez Canillas]

On 10/5/22 13:40, Thomas Zimmermann wrote:
> Rename the atomic helper function drm_atomic_helper_check_crtc_state()
> to drm_atomic_helper_check_crtc_primary_plane() and only check for an
> attached primary plane. Adapt callers.
> 
> Instead of having one big function to check for various CRTC state
> conditions, we rather want smaller functions that drivers can pick
> individually.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

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

[...]

> +	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> +			return 0;
>  	}

I believe the code convention is to drop the curly braces when you
have a single statement inside the a loop ?

Feel free to ignore it though. I particularly don't agree with that
convention anyways, because I think that makes the code more error
prone. But still thought that was worth to point that out.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()

[Ville Syrjälä]

On Thu, Oct 06, 2022 at 10:28:12PM +0200, Javier Martinez Canillas wrote:
> On 10/5/22 13:40, Thomas Zimmermann wrote:
> > Rename the atomic helper function drm_atomic_helper_check_crtc_state()
> > to drm_atomic_helper_check_crtc_primary_plane() and only check for an
> > attached primary plane. Adapt callers.
> > 
> > Instead of having one big function to check for various CRTC state
> > conditions, we rather want smaller functions that drivers can pick
> > individually.
> > 
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > ---
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> [...]
> 
> > +	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
> > +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > +			return 0;
> >  	}
> 
> I believe the code convention is to drop the curly braces when you
> have a single statement inside the a loop ?

This has two.

> 
> Feel free to ignore it though. I particularly don't agree with that
> convention anyways, because I think that makes the code more error
> prone. But still thought that was worth to point that out.
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()

[Javier Martinez Canillas]

On 10/7/22 09:07, Ville Syrjälä wrote:
> On Thu, Oct 06, 2022 at 10:28:12PM +0200, Javier Martinez Canillas wrote:
>> On 10/5/22 13:40, Thomas Zimmermann wrote:
>>> Rename the atomic helper function drm_atomic_helper_check_crtc_state()
>>> to drm_atomic_helper_check_crtc_primary_plane() and only check for an
>>> attached primary plane. Adapt callers.
>>>
>>> Instead of having one big function to check for various CRTC state
>>> conditions, we rather want smaller functions that drivers can pick
>>> individually.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>>
>> [...]
>>
>>> +	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
>>> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>>> +			return 0;
>>>  	}
>>
>> I believe the code convention is to drop the curly braces when you
>> have a single statement inside the a loop ?
> 
> This has two.
> 

No, it has only one that is the if statement. So according to the Linux
kernel coding style AFAIU it should be written as:

	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask)
		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
			return 0;


-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()

[Ville Syrjälä]

On Fri, Oct 07, 2022 at 09:17:50AM +0200, Javier Martinez Canillas wrote:
> On 10/7/22 09:07, Ville Syrjälä wrote:
> > On Thu, Oct 06, 2022 at 10:28:12PM +0200, Javier Martinez Canillas wrote:
> >> On 10/5/22 13:40, Thomas Zimmermann wrote:
> >>> Rename the atomic helper function drm_atomic_helper_check_crtc_state()
> >>> to drm_atomic_helper_check_crtc_primary_plane() and only check for an
> >>> attached primary plane. Adapt callers.
> >>>
> >>> Instead of having one big function to check for various CRTC state
> >>> conditions, we rather want smaller functions that drivers can pick
> >>> individually.
> >>>
> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>> ---
> >>
> >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> >>
> >> [...]
> >>
> >>> +	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
> >>> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> >>> +			return 0;
> >>>  	}
> >>
> >> I believe the code convention is to drop the curly braces when you
> >> have a single statement inside the a loop ?
> > 
> > This has two.
> > 
> 
> No, it has only one that is the if statement. So according to the Linux
> kernel coding style AFAIU it should be written as:
> 
> 	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask)
> 		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> 			return 0;

That is exactly what it says not to do.

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()

[Javier Martinez Canillas]

On 10/7/22 09:29, Ville Syrjälä wrote:

[...]

>> 	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask)
>> 		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>> 			return 0;
> 
> That is exactly what it says not to do.
> 

Ah, good to know. I re-read the kernel coding style and see now
that it only applies to single simple statements.

Better then, since not using braces even for single statements
is something that's error prone IMO, as mentioned before.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()

[Thomas Zimmermann]

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

Hi

Am 07.10.22 um 09:29 schrieb Ville Syrjälä:
> On Fri, Oct 07, 2022 at 09:17:50AM +0200, Javier Martinez Canillas wrote:
>> On 10/7/22 09:07, Ville Syrjälä wrote:
>>> On Thu, Oct 06, 2022 at 10:28:12PM +0200, Javier Martinez Canillas wrote:
>>>> On 10/5/22 13:40, Thomas Zimmermann wrote:
>>>>> Rename the atomic helper function drm_atomic_helper_check_crtc_state()
>>>>> to drm_atomic_helper_check_crtc_primary_plane() and only check for an
>>>>> attached primary plane. Adapt callers.
>>>>>
>>>>> Instead of having one big function to check for various CRTC state
>>>>> conditions, we rather want smaller functions that drivers can pick
>>>>> individually.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> ---
>>>>
>>>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>>>>
>>>> [...]
>>>>
>>>>> +	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
>>>>> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>>>>> +			return 0;
>>>>>   	}
>>>>
>>>> I believe the code convention is to drop the curly braces when you
>>>> have a single statement inside the a loop ?
>>>
>>> This has two.
>>>
>>
>> No, it has only one that is the if statement. So according to the Linux
>> kernel coding style AFAIU it should be written as:
>>
>> 	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask)
>> 		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>> 			return 0;
> 
> That is exactly what it says not to do.

Hey, no need to be so upfront about it. Without the outer braces, I'd 
find it hard to parse anyway.

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