Re: [PATCH] drm/plane-helper: Add a drm_plane_helper_atomic_check() helper

From: Thomas Zimmermann
Date: Mon Sep 12 2022 - 10:22:59 EST


Am 12.09.22 um 14:34 schrieb Ville Syrjälä:
On Mon, Sep 12, 2022 at 02:05:36PM +0200, Thomas Zimmermann wrote:

Am 12.09.22 um 13:18 schrieb Ville Syrjälä:
On Mon, Sep 12, 2022 at 01:05:45PM +0200, Thomas Zimmermann wrote:

Am 12.09.22 um 12:40 schrieb Ville Syrjälä:
On Mon, Sep 12, 2022 at 12:15:22PM +0200, Javier Martinez Canillas wrote:
Provides a default plane state check handler for primary planes that are a
fullscreen scanout buffer and whose state scale and position can't change.

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

Suggested-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>

drivers/gpu/drm/drm_plane_helper.c | 29 +++++++++++++++++++++++++++++
drivers/gpu/drm/solomon/ssd130x.c | 18 +-----------------
drivers/gpu/drm/tiny/simpledrm.c | 25 +------------------------
include/drm/drm_plane_helper.h | 2 ++
4 files changed, 33 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index c7785967f5bf..fb41eee74693 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -278,3 +278,32 @@ void drm_plane_helper_destroy(struct drm_plane *plane)
+ * drm_plane_helper_atomic_check() - Helper to check primary planes states
+ * @plane: plane to check
+ * @new_state: plane state to check

That is not a plane state. Also should s/new_// since it's just
the overall atomic state thing rather than some new or old state.

Using only 'state' is non-intuitive and has lead to bugs where sub-state
was retrieved from the wrong state information. So we've been using
'new_state' and 'old_state' explicitly in several places now.

There is no old or new drm_atomic_state. It contains both.

I (vaguely) remember a bug where a driver tried
drm_atomic_get_new_plane_state() with the (old) state that's passed to
atomic_update. It didn't return the expected results and modesetting
gave slightly wrong results.

As there is no wrong drm_atomic_state to pass I don't think it could
have been the case.

So we began to be more precise about new
and old. And whatever is stored in 'plane->state' is then just 'the state'.

There were certainly a lot of confusion before the explicit new/old
state stuff was added whether foo->state/etc. was the old or the
new state. And labeling things as explicitly old vs. new when passing
in individual object states certainly makes sense. But that doesn't
really have anything to do with mislabeling the overall drm_atomic_state.

I understand that the semantics of atomic_check are different from
atomic_update, but it still doesn't hurt to talk of new_state IMHO.

IMO it's just confusing. Makes the reader think there is somehow
different drm_atomic_states for old vs. new states when there isn't.
I also wouldn't call it new_state for .atomic_update() either.

In both cases you have the old and new states in there and how
exactly they get used in the hooks is more of an implementation
detail. The only rules you would have to follow is that at the
end of .atomic_update() the hardware state matches the new state,
and .atomic_check() makes sure the transition from the old to the
new state is possible.

From what I understand:

In atomic_check(), plane->state is the current state and the state argument is the state to be validated. Calling drm_atomic_get_new_plane_state() will return the plane's new state.

If you call drm_atomic_get_old_plane_state() from atomic_check(), what will it return?

In atomic_update() plane->state is the state to be committed and the state argument is the old state before the start of the atomic commit. And calling drm_atomic_get_new_plane_state() will *not* the return the plane's new state (i.e., the one in plane->state) IIRC. (As I mentioned, there was a related bug in one of the drivers.) So we began to call this 'old_state'.

My point is: the state passed to the check and commit functions are different things, even though they appear to be the same.

I've proposed renaming drm_atomic_state to eg. drm_atomic_transaction
a few times before but no one took the bait so far...

If you really don't like new_state, then let's call it state_tx.

Best regards

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