diff mbox

[v3,6/7] drm/atomic: Convert get_existing_state callers to get_old/new_state, v2.

Message ID 1484559464-27107-7-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maarten Lankhorst Jan. 16, 2017, 9:37 a.m. UTC 
This is a straightforward conversion that converts all the users of
get_existing_state in atomic core to use get_old_state or get_new_state

Changes since v1:
- Fix using the wrong state in drm_atomic_helper_update_legacy_modeset_state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c        |  6 +++---
 drivers/gpu/drm/drm_atomic_helper.c | 39 +++++++++++++++++--------------------
 drivers/gpu/drm/drm_blend.c         |  3 +--
 3 files changed, 22 insertions(+), 26 deletions(-)

Comments

Laurent Pinchart Jan. 17, 2017, 1:12 a.m. UTC | #1 
Hi Maarten,

Thank you for the patch.

I don't think you need the "v2" at the end of the subject line.

On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
> This is a straightforward conversion that converts all the users of
> get_existing_state in atomic core to use get_old_state or get_new_state
> 
> Changes since v1:
> - Fix using the wrong state in
> drm_atomic_helper_update_legacy_modeset_state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        |  6 +++---
>  drivers/gpu/drm/drm_atomic_helper.c | 39 +++++++++++++++------------------
>  drivers/gpu/drm/drm_blend.c         |  3 +--
>  3 files changed, 22 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7b61e0da9ace..6428e9469607 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c

[snip]

> @@ -1835,7 +1832,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct
> drm_crtc_state *old_crtc_state)
> 
>  	drm_for_each_plane_mask(plane, crtc->dev, plane_mask) {
>  		struct drm_plane_state *old_plane_state =
> -			drm_atomic_get_existing_plane_state(old_state, plane);
> +			drm_atomic_get_old_plane_state(old_state, plane);

I believe this change to be correct, but given that the function is called 
after state swap, didn't it use the new state before this patch ?

>  		const struct drm_plane_helper_funcs *plane_funcs;
> 
>  		plane_funcs = plane->helper_private;
Laurent Pinchart Jan. 17, 2017, 1:27 a.m. UTC | #2 
Hi Maarten,

One more thing.

On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
> This is a straightforward conversion that converts all the users of
> get_existing_state in atomic core to use get_old_state or get_new_state
> 
> Changes since v1:
> - Fix using the wrong state in
> drm_atomic_helper_update_legacy_modeset_state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        |  6 +++---
>  drivers/gpu/drm/drm_atomic_helper.c | 39 ++++++++++++++--------------------
>  drivers/gpu/drm/drm_blend.c         |  3 +--
>  3 files changed, 22 insertions(+), 26 deletions(-)

[snip]

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index b26cf786ce12..1de8d5fbc8d3
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c

[snip]

> @@ -698,8 +695,7 @@ disable_outputs(struct drm_device *dev, struct
> drm_atomic_state *old_state) if (!old_conn_state->crtc)
>  			continue;
> 
> -		old_crtc_state = drm_atomic_get_existing_crtc_state(old_state,
> -								    
old_conn_state->crtc);
> +		old_crtc_state = drm_atomic_get_new_crtc_state(old_state,
> old_conn_state->crtc);

This looks wrong. I believe you should call drm_atomic_get_old_crtc_state() 
here. If I'm not mistaken drm_atomic_get_existing_crtc_state() did the right 
thing as old_state->crtcs[*].state was set to the old state by the state swap 
operation.

On the other hand, drm_atomic_helper_update_legacy_modeset_state() uses 
drm_atomic_get_new_plane_state() the same way you do here, even though it 
operates after state swap, and your changelog specifically mentions that 
you've fixed that in v2. It's getting too late to properly wrap my head around 
this, I'll let you check which option is correct (but I reserve the right to 
challenge it if your explanation isn't convincing enough ;-)).

>  		if (!old_crtc_state->active ||
>  		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc-
>state))
Maarten Lankhorst Feb. 16, 2017, 2:34 p.m. UTC | #3 
Op 17-01-17 om 02:27 schreef Laurent Pinchart:
> Hi Maarten,
>
> One more thing.
>
> On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
>> This is a straightforward conversion that converts all the users of
>> get_existing_state in atomic core to use get_old_state or get_new_state
>>
>> Changes since v1:
>> - Fix using the wrong state in
>> drm_atomic_helper_update_legacy_modeset_state.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c        |  6 +++---
>>  drivers/gpu/drm/drm_atomic_helper.c | 39 ++++++++++++++--------------------
>>  drivers/gpu/drm/drm_blend.c         |  3 +--
>>  3 files changed, 22 insertions(+), 26 deletions(-)
> [snip]
>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c index b26cf786ce12..1de8d5fbc8d3
>> 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> [snip]
>
>> @@ -698,8 +695,7 @@ disable_outputs(struct drm_device *dev, struct
>> drm_atomic_state *old_state) if (!old_conn_state->crtc)
>>  			continue;
>>
>> -		old_crtc_state = drm_atomic_get_existing_crtc_state(old_state,
>> -								    
> old_conn_state->crtc);
>> +		old_crtc_state = drm_atomic_get_new_crtc_state(old_state,
>> old_conn_state->crtc);
> This looks wrong. I believe you should call drm_atomic_get_old_crtc_state() 
> here. If I'm not mistaken drm_atomic_get_existing_crtc_state() did the right 
> thing as old_state->crtcs[*].state was set to the old state by the state swap 
> operation.
This is wrong, even. Fixed in next version. At least looking at the code made it obvious. :)
> On the other hand, drm_atomic_helper_update_legacy_modeset_state() uses 
> drm_atomic_get_new_plane_state() the same way you do here, even though it 
> operates after state swap, and your changelog specifically mentions that 
> you've fixed that in v2. It's getting too late to properly wrap my head around 
> this, I'll let you check which option is correct (but I reserve the right to 
> challenge it if your explanation isn't convincing enough ;-)).
update_legacy_modeset_state was looking at primary->state (new state) before,
but was using get_existing_state to check whether it's part of the commit, so it's
valid to look at plane->state.

This was fixed to get the new state, so it makes sense there.
>>  		if (!old_crtc_state->active ||
>>  		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc-
>> state))
Maarten Lankhorst Feb. 16, 2017, 2:37 p.m. UTC | #4 
Op 17-01-17 om 02:12 schreef Laurent Pinchart:
> Hi Maarten,
>
> Thank you for the patch.
>
> I don't think you need the "v2" at the end of the subject line.
>
> On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
>> This is a straightforward conversion that converts all the users of
>> get_existing_state in atomic core to use get_old_state or get_new_state
>>
>> Changes since v1:
>> - Fix using the wrong state in
>> drm_atomic_helper_update_legacy_modeset_state.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c        |  6 +++---
>>  drivers/gpu/drm/drm_atomic_helper.c | 39 +++++++++++++++------------------
>>  drivers/gpu/drm/drm_blend.c         |  3 +--
>>  3 files changed, 22 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 7b61e0da9ace..6428e9469607 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
> [snip]
>
>> @@ -1835,7 +1832,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct
>> drm_crtc_state *old_crtc_state)
>>
>>  	drm_for_each_plane_mask(plane, crtc->dev, plane_mask) {
>>  		struct drm_plane_state *old_plane_state =
>> -			drm_atomic_get_existing_plane_state(old_state, plane);
>> +			drm_atomic_get_old_plane_state(old_state, plane);
> I believe this change to be correct, but given that the function is called 
> after state swap, didn't it use the new state before this patch ?
get_existing_state returns the old state after swap.

But the call to drm_atomic_plane_disabling is confusing to me. I'm changing
the function to pass old and new plane state, which should make it easier to understand.

Cheers,
~Maarten
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7b61e0da9ace..6428e9469607 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1362,8 +1362,8 @@  drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 		return 0;
 
 	if (conn_state->crtc) {
-		crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,
-								conn_state->crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(conn_state->state,
+							   conn_state->crtc);
 
 		crtc_state->connector_mask &=
 			~(1 << drm_connector_index(conn_state->connector));
@@ -1480,7 +1480,7 @@  drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 {
 	struct drm_plane *plane;
 
-	WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
+	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));
 
 	drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
 		struct drm_plane_state *plane_state =
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index b26cf786ce12..1de8d5fbc8d3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -70,8 +70,7 @@  drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
 	struct drm_crtc_state *crtc_state;
 
 	if (old_plane_state->crtc) {
-		crtc_state = drm_atomic_get_existing_crtc_state(state,
-								old_plane_state->crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(state, old_plane_state->crtc);
 
 		if (WARN_ON(!crtc_state))
 			return;
@@ -80,8 +79,7 @@  drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
 	}
 
 	if (plane_state->crtc) {
-		crtc_state = drm_atomic_get_existing_crtc_state(state,
-								plane_state->crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
 
 		if (WARN_ON(!crtc_state))
 			return;
@@ -150,7 +148,7 @@  static int handle_conflicting_encoders(struct drm_atomic_state *state,
 	drm_for_each_connector_iter(connector, &conn_iter) {
 		struct drm_crtc_state *crtc_state;
 
-		if (drm_atomic_get_existing_connector_state(state, connector))
+		if (drm_atomic_get_new_connector_state(state, connector))
 			continue;
 
 		encoder = connector->state->best_encoder;
@@ -178,7 +176,7 @@  static int handle_conflicting_encoders(struct drm_atomic_state *state,
 				 conn_state->crtc->base.id, conn_state->crtc->name,
 				 connector->base.id, connector->name);
 
-		crtc_state = drm_atomic_get_existing_crtc_state(state, conn_state->crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
 
 		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
 		if (ret)
@@ -219,7 +217,7 @@  set_best_encoder(struct drm_atomic_state *state,
 		 */
 		WARN_ON(!crtc && encoder != conn_state->best_encoder);
 		if (crtc) {
-			crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+			crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 
 			crtc_state->encoder_mask &=
 				~(1 << drm_encoder_index(conn_state->best_encoder));
@@ -230,7 +228,7 @@  set_best_encoder(struct drm_atomic_state *state,
 		crtc = conn_state->crtc;
 		WARN_ON(!crtc);
 		if (crtc) {
-			crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+			crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 
 			crtc_state->encoder_mask |=
 				1 << drm_encoder_index(encoder);
@@ -263,7 +261,7 @@  steal_encoder(struct drm_atomic_state *state,
 
 		set_best_encoder(state, new_connector_state, NULL);
 
-		crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(state, encoder_crtc);
 		crtc_state->connectors_changed = true;
 
 		return;
@@ -286,12 +284,12 @@  update_connector_routing(struct drm_atomic_state *state,
 
 	if (old_connector_state->crtc != new_connector_state->crtc) {
 		if (old_connector_state->crtc) {
-			crtc_state = drm_atomic_get_existing_crtc_state(state, old_connector_state->crtc);
+			crtc_state = drm_atomic_get_new_crtc_state(state, old_connector_state->crtc);
 			crtc_state->connectors_changed = true;
 		}
 
 		if (new_connector_state->crtc) {
-			crtc_state = drm_atomic_get_existing_crtc_state(state, new_connector_state->crtc);
+			crtc_state = drm_atomic_get_new_crtc_state(state, new_connector_state->crtc);
 			crtc_state->connectors_changed = true;
 		}
 	}
@@ -349,7 +347,7 @@  update_connector_routing(struct drm_atomic_state *state,
 
 	set_best_encoder(state, new_connector_state, new_encoder);
 
-	crtc_state = drm_atomic_get_existing_crtc_state(state, new_connector_state->crtc);
+	crtc_state = drm_atomic_get_new_crtc_state(state, new_connector_state->crtc);
 	crtc_state->connectors_changed = true;
 
 	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
@@ -390,8 +388,7 @@  mode_fixup(struct drm_atomic_state *state)
 		if (!conn_state->crtc || !conn_state->best_encoder)
 			continue;
 
-		crtc_state = drm_atomic_get_existing_crtc_state(state,
-								conn_state->crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
 
 		/*
 		 * Each encoder has at most one connector (since we always steal
@@ -698,8 +695,7 @@  disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		if (!old_conn_state->crtc)
 			continue;
 
-		old_crtc_state = drm_atomic_get_existing_crtc_state(old_state,
-								    old_conn_state->crtc);
+		old_crtc_state = drm_atomic_get_new_crtc_state(old_state, old_conn_state->crtc);
 
 		if (!old_crtc_state->active ||
 		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
@@ -828,14 +824,15 @@  drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
 	/* set legacy state in the crtc structure */
 	for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
 		struct drm_plane *primary = crtc->primary;
+		struct drm_plane_state *plane_state;
 
 		crtc->mode = crtc_state->mode;
 		crtc->enabled = crtc_state->enable;
 
-		if (drm_atomic_get_existing_plane_state(old_state, primary) &&
-		    primary->state->crtc == crtc) {
-			crtc->x = primary->state->src_x >> 16;
-			crtc->y = primary->state->src_y >> 16;
+		plane_state = drm_atomic_get_new_plane_state(old_state, primary);
+		if (plane_state && plane_state->crtc == crtc) {
+			crtc->x = plane_state->src_x >> 16;
+			crtc->y = plane_state->src_y >> 16;
 		}
 
 		if (crtc_state->enable)
@@ -1835,7 +1832,7 @@  drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
 
 	drm_for_each_plane_mask(plane, crtc->dev, plane_mask) {
 		struct drm_plane_state *old_plane_state =
-			drm_atomic_get_existing_plane_state(old_state, plane);
+			drm_atomic_get_old_plane_state(old_state, plane);
 		const struct drm_plane_helper_funcs *plane_funcs;
 
 		plane_funcs = plane->helper_private;
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 1f2412c7ccfd..5c45d0852608 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -388,8 +388,7 @@  int drm_atomic_normalize_zpos(struct drm_device *dev,
 		if (!crtc)
 			continue;
 		if (plane->state->zpos != plane_state->zpos) {
-			crtc_state =
-				drm_atomic_get_existing_crtc_state(state, crtc);
+			crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 			crtc_state->zpos_changed = true;
 		}
 	}