Re: [PATCH i-g-t v2 09/15] igt_kms: Clear all _changed members centrally

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Wed, 2016-07-06 at 11:55 +0200, Maarten Lankhorst wrote:
> This will make it easier to support TEST_ONLY commits.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  lib/igt_kms.c | 84 ++++++++++++++++++++++++++++++++++----------------------
> ---
>  1 file changed, 48 insertions(+), 36 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 9fc5559a5df4..d0d0dcff8193 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1727,11 +1727,6 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane,
> igt_pipe_t *pipe,
>  	if (plane->rotation_changed)
>  		igt_atomic_populate_plane_req(req, plane,
>  			IGT_PLANE_ROTATION, plane->rotation);
> -
> -	plane->fb_changed = false;
> -	plane->position_changed = false;
> -	plane->size_changed = false;
> -	plane->rotation_changed = false;
>  }
>  
>  
> @@ -1819,15 +1814,10 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
>  		CHECK_RETURN(ret, fail_on_error);
>  	}
>  
> -	plane->fb_changed = false;
> -	plane->position_changed = false;
> -	plane->size_changed = false;
> -
>  	if (plane->rotation_changed) {
>  		ret = igt_plane_set_property(plane, plane->rotation_property,
>  				       plane->rotation);
>  
> -		plane->rotation_changed = false;
>  		CHECK_RETURN(ret, fail_on_error);
>  	}
>  
> @@ -1872,8 +1862,6 @@ static int igt_cursor_commit_legacy(igt_plane_t *cursor,
>  		}
>  
>  		CHECK_RETURN(ret, fail_on_error);
> -
> -		cursor->fb_changed = false;
>  	}
>  
>  	if (cursor->position_changed) {
> @@ -1887,8 +1875,6 @@ static int igt_cursor_commit_legacy(igt_plane_t *cursor,
>  
>  		ret = drmModeMoveCursor(display->drm_fd, crtc_id, x, y);
>  		CHECK_RETURN(ret, fail_on_error);
> -
> -		cursor->position_changed = false;
>  	}
>  
>  	return 0;
> @@ -1915,7 +1901,7 @@ static int igt_primary_plane_commit_legacy(igt_plane_t
> *primary,
>  	igt_assert(!primary->rotation_changed);
>  
>  	if (!primary->fb_changed && !primary->position_changed &&
> -		!primary->size_changed)
> +	    !primary->size_changed)
>  		return 0;
>  
>  	crtc_id = pipe->crtc_id;
> @@ -1959,9 +1945,6 @@ static int igt_primary_plane_commit_legacy(igt_plane_t
> *primary,
>  	CHECK_RETURN(ret, fail_on_error);
>  
>  	primary->pipe->enabled = (fb_id != 0);
> -	primary->fb_changed = false;
> -	primary->position_changed = false;
> -	primary->size_changed = false;
>  
>  	return 0;
>  }
> @@ -1982,7 +1965,7 @@ static int igt_plane_commit(igt_plane_t *plane,
>  		return igt_primary_plane_commit_legacy(plane, pipe,
>  						       fail_on_error);
>  	} else {
> -			return igt_drm_plane_commit(plane, pipe,
> fail_on_error);
> +		return igt_drm_plane_commit(plane, pipe, fail_on_error);
>  	}
>  }
>  
> @@ -2008,8 +1991,6 @@ static int igt_pipe_commit(igt_pipe_t *pipe,
>  	if (pipe->background_changed) {
>  		igt_crtc_set_property(pipe, pipe->background_property,
>  			pipe->background);
> -
> -		pipe->background_changed = false;
>  	}
>  
>  	if (pipe->color_mgmt_changed) {
> @@ -2019,8 +2000,6 @@ static int igt_pipe_commit(igt_pipe_t *pipe,
>  				      pipe->ctm_blob);
>  		igt_crtc_set_property(pipe, pipe->gamma_property,
>  				      pipe->gamma_blob);
> -
> -		pipe->color_mgmt_changed = false;
>  	}
>  
>  	for (i = 0; i < pipe->n_planes; i++) {
> @@ -2140,35 +2119,68 @@ static int do_display_commit(igt_display_t *display,
>  			     bool fail_on_error)
>  {
>  	int i, ret;
> -	int valid_outs = 0;
> -
> +	enum pipe pipe;
>  	LOG_INDENT(display, "commit");
>  
>  	igt_display_refresh(display);
>  
>  	if (s == COMMIT_ATOMIC) {
> -
>  		ret = igt_atomic_commit(display);
> +
>  		CHECK_RETURN(ret, fail_on_error);
> -		return 0;
> +	} else {
> +		int valid_outs = 0;
>  
> -	}
> +		for_each_pipe(display, pipe) {
> +			igt_pipe_t *pipe_obj = &display->pipes[pipe];
> +			igt_output_t *output = igt_pipe_get_output(pipe_obj);
>  
> -	for_each_pipe(display, i) {
> -		igt_pipe_t *pipe_obj = &display->pipes[i];
> -		igt_output_t *output = igt_pipe_get_output(pipe_obj);
> +			if (output && output->valid)
> +				valid_outs++;
>  
> -		if (output && output->valid)
> -			valid_outs++;
> +			ret = igt_pipe_commit(pipe_obj, s, fail_on_error);
> +			CHECK_RETURN(ret, fail_on_error);
> +		}
>  
> -		ret = igt_pipe_commit(pipe_obj, s, fail_on_error);
>  		CHECK_RETURN(ret, fail_on_error);
> +
> +		if (valid_outs == 0) {
> +			LOG_UNINDENT(display);
> +
> +			return -1;
> +		}
>  	}
>  
>  	LOG_UNINDENT(display);
>  
> -	if (valid_outs == 0)
> -		return -1;
> +	if (ret)
> +		return ret;
> +
> +	for_each_pipe(display, pipe) {
> +		igt_pipe_t *pipe_obj = &display->pipes[pipe];
> +		igt_plane_t *plane;
> +
> +		if (s == COMMIT_ATOMIC) {
> +			pipe_obj->color_mgmt_changed = false;
> +			pipe_obj->background_changed = false;
> +		}

If this is supposed to match the previous behavior, shouldn't the condition be
"!= COMMIT_ATOMIC". Both flags are set from igt_pipe_commit() which is not
called in the atomic case. However, the flags aren't set to false in
igt_atomic_prepare_crtc_commit(). That actually sounds like a bug?

> +
> +		for_each_plane_on_pipe(display, pipe, plane) {
> +			plane->fb_changed = false;
> +			plane->position_changed = false;
> +			plane->size_changed = false;
> +
> +			if (s != COMMIT_LEGACY || !(plane->is_primary || 
> plane->is_cursor))
> +				plane->rotation_changed = false;

Can't plane->rotation_changed be set unconditionally here? The legacy primary
plane commit has an assert for rotation_changed == false and perhaps the cursor
version should have the same.

> +		}
> +	}
> +
> +	for (i = 0; i < display->n_outputs && s == COMMIT_ATOMIC; i++) {
> +		igt_output_t *output = &display->outputs[i];
> +
> +		output->config.connector_dpms_changed = false;
> +		output->config.connector_scaling_mode_changed = false;
> +	}
>  
>  	igt_debug_wait_for_keypress("modeset");
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx