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

Ander Conselvan De Oliveira
2016-07-21T12:13:49Z

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");
>