[Intel-gfx] [PATCH v3 4/7] drm/atomic: Fix atomic helpers to use the new iterator macros.

[Maarten Lankhorst]

Op 17-01-17 om 02:01 schreef Laurent Pinchart:
> Hi Maarten,
>
> (CC'ing Daniel)
>
> Thank you for the patch.
>
> On Monday 16 Jan 2017 10:37:41 Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++++---------------
>>  1 file changed, 152 insertions(+), 141 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c index d153e8a72921..b26cf786ce12
>> 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> [snip]
>
>> @@ -245,22 +246,22 @@ steal_encoder(struct drm_atomic_state *state,
>>  {
>>  	struct drm_crtc_state *crtc_state;
>>  	struct drm_connector *connector;
>> -	struct drm_connector_state *connector_state;
>> +	struct drm_connector_state *old_connector_state, *new_connector_state;
> The kernel favours one variable declaration per line, and I think this file 
> does as well, at least mostly (this comment holds for the rest of this patch 
> and the other patches in the series).
This is the first I've heard of it..

~/nfs/linux$ git grep 'int i,'  | wc -l
8490
~/nfs/linux$ git grep 'int i;'  | wc -l
23636

Based on this, I don't think it's uncommon to have multiple declarations per line.

>>  	int i;
>>
>> -	for_each_connector_in_state(state, connector, connector_state, i) {
>> +	for_each_oldnew_connector_in_state(state, connector,
>> old_connector_state, new_connector_state, i) {
> This is getting quite long, you could wrap the line.
Sounds good.
>> 		struct drm_crtc *encoder_crtc;
>>
>> -		if (connector_state->best_encoder != encoder)
>> +		if (new_connector_state->best_encoder != encoder)
>>  			continue;
>>
>> -		encoder_crtc = connector->state->crtc;
>> +		encoder_crtc = old_connector_state->crtc;
>>
>>  		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s], 
> stealing it\n",
>> encoder->base.id, encoder->name,
>>  				 encoder_crtc->base.id, encoder_crtc->name);
>>
>> -		set_best_encoder(state, connector_state, NULL);
>> +		set_best_encoder(state, new_connector_state, NULL);
>>
>>  		crtc_state = drm_atomic_get_existing_crtc_state(state, 
> encoder_crtc);
>>  		crtc_state->connectors_changed = true;
> [snip]
>
>> @@ -516,14 +518,15 @@ drm_atomic_helper_check_modeset(struct drm_device
>> *dev, if (ret)
>>  		return ret;
>>
>> -	for_each_connector_in_state(state, connector, connector_state, i) {
>> +	for_each_oldnew_connector_in_state(state, connector,
>> old_connector_state, new_connector_state, i) {
> Line wrap here too ?
>
>> 		/*
>>  		 * This only sets crtc->connectors_changed for routing 
> changes,
>>  		 * drivers must set crtc->connectors_changed themselves when
>>  		 * connector properties need to be updated.
>>  		 */
>>  		ret = update_connector_routing(state, connector,
>> -					       connector_state);
>> +					       old_connector_state,
>> +					       new_connector_state);
>>  		if (ret)
>>  			return ret;
>>  	}
> [snip]
>
>> @@ -599,15 +602,15 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>> struct drm_crtc *crtc;
>>  	struct drm_crtc_state *crtc_state;
>>  	struct drm_plane *plane;
>> -	struct drm_plane_state *plane_state;
>> +	struct drm_plane_state *plane_state, *old_plane_state;
> In some location you use new_*_state and in others *_state. I believe this 
> should this be standardized to avoid confusion (the code is hard enough to 
> read as it is :-)).
>
> Similarly, you sometimes use *_conn_state and sometimes *_connector_state. 
> That should be standardized as well.
Indeed, at least for those that use both.
>>  	int i, ret = 0;
>>
>> -	for_each_plane_in_state(state, plane, plane_state, i) {
>> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state,
>> plane_state, i) {
>> 		const struct drm_plane_helper_funcs *funcs;
>>
>>  		funcs = plane->helper_private;
>>
>> -		drm_atomic_helper_plane_changed(state, plane_state, plane);
>> +		drm_atomic_helper_plane_changed(state, old_plane_state,
>> plane_state, plane);
>>
>>  		if (!funcs || !funcs->atomic_check)
>>  			continue;
> [snip]
>
>> @@ -974,18 +977,18 @@ void drm_atomic_helper_commit_modeset_enables(struct
>> drm_device *dev, }
>>  	}
>>
>> -	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
>> +	for_each_new_connector_in_state(old_state, connector, conn_state, i) {
> Not strictly related to this patch, but I've been wondering how this works, 
> given that we need to loop over connectors in the new state, not the old 
> state. After some investigation I've realized that both the old and new states 
> contain the same list of objects, as we don't keep a copy of the old global 
> atomic state. old_state was the new state before the state swap, and the swap 
> operation sets state->/objects/[*].state to the old state for all objects, but 
> doesn't modify the object pointers. This is possibly more of a question for 
> Daniel, should this be captured in the documentation somewhere (and is my 
> understanding correct) ?
Yes. But that will go away soon after this patch + all drivers converted.

This is why get_state should not be called during atomic commit, and get_existing_state will be removed.
>>  		const struct drm_encoder_helper_funcs *funcs;
>>  		struct drm_encoder *encoder;
>>
>> -		if (!connector->state->best_encoder)
>> +		if (!conn_state->best_encoder)
>>  			continue;
>>
>> -		if (!connector->state->crtc->state->active ||
>> -		    !drm_atomic_crtc_needs_modeset(connector->state->crtc-
>> state))
>> +		if (!conn_state->crtc->state->active ||
>> +		    !drm_atomic_crtc_needs_modeset(conn_state->crtc->state))
>>  			continue;
>>
>> -		encoder = connector->state->best_encoder;
>> +		encoder = conn_state->best_encoder;
>>  		funcs = encoder->helper_private;
>>
>>  		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
> [snip]
>
>> @@ -1921,12 +1919,19 @@ void drm_atomic_helper_cleanup_planes(struct
>> drm_device *dev, struct drm_atomic_state *old_state)
>>  {
>>  	struct drm_plane *plane;
>> -	struct drm_plane_state *plane_state;
>> +	struct drm_plane_state *plane_state, *new_plane_state;
>>  	int i;
>>
>> -	for_each_plane_in_state(old_state, plane, plane_state, i) {
>> +	for_each_oldnew_plane_in_state(old_state, plane, plane_state,
>> new_plane_state, i) {
>> 		const struct drm_plane_helper_funcs *funcs;
>>
>> +		/*
>> +		 * This might be called before swapping when commit is 
> aborted,
>> +		 * in which case we have to free the new state.
> Should this be "cleanup the new state" instead of "free the new state" ?
Indeed.
>> +		 */
>> +		if (plane_state == plane->state)
>> +			plane_state = new_plane_state;
>> +
>>  		funcs = plane->helper_private;
>>
>>  		if (funcs->cleanup_fb)
> [snip]
>