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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 30.09.22 um 11:12 schrieb Ville Syrjälä:
On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote:
Hi,

thanks for reviewing.

Am 29.09.22 um 21:03 schrieb Ville Syrjälä:
On Thu, Sep 29, 2022 at 04:07:14PM +0200, 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 results 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.
This can be a newly enabled plane or an already enabled plane. So if a
plane is not part of the commit, use the plane's existing state. The new
helper drm_atomic_get_next_plane_state() returns the correct instance.

Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
Cc: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
Cc: Maxime Ripard <mripard@xxxxxxxxxx>
Cc: David Airlie <airlied@xxxxxxxx>
Cc: Daniel Vetter <daniel@xxxxxxxx>
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
---
   drivers/gpu/drm/drm_atomic_helper.c |  4 +---
   include/drm/drm_atomic.h            | 20 ++++++++++++++++++++
   2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 98cc3137c062..463d4f3fa443 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_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);
+			plane_state = drm_atomic_get_next_plane_state(state, plane);
   			if (plane_state->fb && plane_state->crtc) {

Hmm. Why this is even looking at these. If the plane is in the crtc's
plane_mask then surely plane_state->crtc must already point to this
crtc? And I don't think ->fb and ->crtc are allowed to disagree, so
if one is set then surely the other one must be as well or we'd
return an error at some point somewhere?

Yeah, the crtc test is done for keeping consistency. Other places also
sometimes validate that these fields don't disagree. I'll remove the
crtc test in the next version. The fb test is the important one.

What I'm asking how can you have crtc!=NULL && fb==NULL at all here?
Some other plane state check function (can't remember which one
specifically) should have rejected that. So either you're checking
for impossible things, or there is a bug somewhere else.

So to me it looks like it should be enough to just check the plane_mask
and not even bother looking at the plane state at all.

Indeed. Somehow I confused plane_mask with 'mask of all planes possible'.

Best regards
Thomas





   				has_primary_plane = true;
   				break;
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 10b1990bc1f6..4006f2d459e3 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
   	return state->planes[drm_plane_index(plane)].new_state;
   }
+/**
+ * drm_atomic_get_next_plane_state - get plane state after atomic commit
+ * @state: global atomic state object
+ * @plane: plane to grab
+ *
+ * This function returns the plane state that the given plane will have
+ * after the atomic commit. This will be the new plane state if the plane
+ * is part of the global atomic state, or the current state otherwise.
+ */
+static inline struct drm_plane_state *
+drm_atomic_get_next_plane_state(struct drm_atomic_state *state,
+				struct drm_plane *plane)
+{
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+
+	if (plane_state)
+		return plane_state;
+	return plane->state;

You're not holding the appropriate lock so that does not look safe. Even
if you know somewhere, somehow this sort of thing to be safe I don't
think it's a good idea to promote this with any kind of official
function because someone will shoot themselves in the foot with it.

OK. It'll be inlined with locking added.

Best regards
Thomas


+}
+
   /**
    * drm_atomic_get_existing_connector_state - get connector state, if it exists
    * @state: global atomic state object
--
2.37.3


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





--
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: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux