On Wed, 2016-07-06 at 11:55 +0200, Maarten Lankhorst wrote:
> None of the tests requires that a output bound to PIPE_ANY is assigned,
> so don't do it. Fix the display commit to iterate over crtc's instead
> of outputs to properly disable pipes without outputs.
>
> This also means that output->valid is only set after connecting a
> output to a pipe, so no longer depend on it in for_each_connected_output
> and similar macros.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> lib/igt_kms.c | 246 ++++++++++++++++++++++++++++-----------------------------
> -
> lib/igt_kms.h | 18 ++++-
> 2 files changed, 135 insertions(+), 129 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index ce8aa2455348..88cae7d51787 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -869,18 +869,20 @@ static bool _kmstest_connector_config(int drm_fd,
> uint32_t connector_id,
> */
> _kmstest_connector_config_crtc_mask(drm_fd, connector, config);
>
> + if (!kmstest_get_connector_default_mode(drm_fd, connector,
> + &config->default_mode))
> + goto err3;
> +
> + config->connector = connector;
> +
> crtc_idx_mask &= config->valid_crtc_idx_mask;
> if (!crtc_idx_mask)
> - goto err3;
> + /* Keep config->connector */
> + goto err2;
>
> config->pipe = ffs(crtc_idx_mask) - 1;
>
> - if (!kmstest_get_connector_default_mode(drm_fd, connector,
> - &config->default_mode))
> - goto err3;
> -
> config->encoder = _kmstest_connector_config_find_encoder(drm_fd,
> connector, config->pipe);
> - config->connector = connector;
> config->crtc = drmModeGetCrtc(drm_fd, resources->crtcs[config-
> >pipe]);
>
> drmModeFreeResources(resources);
> @@ -940,8 +942,13 @@ bool kmstest_probe_connector_config(int drm_fd, uint32_t
> connector_id,
> void kmstest_free_connector_config(struct kmstest_connector_config *config)
> {
> drmModeFreeCrtc(config->crtc);
> + config->crtc = NULL;
> +
> drmModeFreeEncoder(config->encoder);
> + config->encoder = NULL;
> +
> drmModeFreeConnector(config->connector);
> + config->connector = NULL;
> }
>
> /**
> @@ -1197,8 +1204,7 @@ static void igt_output_refresh(igt_output_t *output)
> /* we mask out the pipes already in use */
> crtc_idx_mask = output->pending_crtc_idx_mask & ~display-
> >pipes_in_use;
>
> - if (output->valid)
> - kmstest_free_connector_config(&output->config);
> + kmstest_free_connector_config(&output->config);
>
> ret = kmstest_get_connector_config(display->drm_fd,
> output->id,
> @@ -1209,19 +1215,19 @@ static void igt_output_refresh(igt_output_t *output)
> else
> output->valid = false;
>
> - if (!output->valid)
> - return;
> -
> - if (output->use_override_mode)
> - output->config.default_mode = output->override_mode;
> -
> - if (!output->name) {
> + if (!output->name && output->config.connector) {
> drmModeConnector *c = output->config.connector;
>
> igt_assert_neq(asprintf(&output->name, "%s-%d",
> kmstest_connector_type_str(c->connector_type), c->connector_type_id),
> -1);
> }
>
> + if (!output->valid)
> + return;
> +
> + if (output->use_override_mode)
> + output->config.default_mode = output->override_mode;
> +
> LOG(display, "%s: Selecting pipe %s\n", output->name,
> kmstest_pipe_name(output->config.pipe));
>
> @@ -1259,10 +1265,10 @@ get_crtc_property(int drm_fd, uint32_t crtc_id, const
> char *name,
> }
>
> static void
> -igt_crtc_set_property(igt_output_t *output, uint32_t prop_id, uint64_t value)
> +igt_crtc_set_property(igt_pipe_t *pipe, uint32_t prop_id, uint64_t value)
> {
> - drmModeObjectSetProperty(output->display->drm_fd,
> - output->config.crtc->crtc_id, DRM_MODE_OBJECT_CRTC, prop_id,
> value);
> + drmModeObjectSetProperty(pipe->display->drm_fd,
> + pipe->crtc_id, DRM_MODE_OBJECT_CRTC, prop_id, value);
> }
>
> /*
> @@ -1325,15 +1331,37 @@ void igt_display_init(igt_display_t *display, int
> drm_fd)
> int p = IGT_PLANE_2;
> int j, type;
> uint8_t n_planes = 0;
> + uint64_t prop_value;
>
> pipe->crtc_id = resources->crtcs[i];
> pipe->display = display;
> pipe->pipe = i;
>
> + get_crtc_property(display->drm_fd, pipe->crtc_id,
> + "background_color",
> + &pipe->background_property,
> + &prop_value,
> + NULL);
> + pipe->background = (uint32_t)prop_value;
> + get_crtc_property(display->drm_fd, pipe->crtc_id,
> + "DEGAMMA_LUT",
> + &pipe->degamma_property,
> + NULL,
> + NULL);
> + get_crtc_property(display->drm_fd, pipe->crtc_id,
> + "CTM",
> + &pipe->ctm_property,
> + NULL,
> + NULL);
> + get_crtc_property(display->drm_fd, pipe->crtc_id,
> + "GAMMA_LUT",
> + &pipe->gamma_property,
> + NULL,
> + NULL);
> +
I think it might be worth splitting the drm property related changes to a
separate patch. They seem fairly self contained and this is a somewhat long
patch.
> /* add the planes that can be used with that pipe */
> for (j = 0; j < plane_resources->count_planes; j++) {
> drmModePlane *drm_plane;
> - uint64_t prop_value;
>
> drm_plane = drmModeGetPlane(display->drm_fd,
> plane_resources-
> >planes[j]);
> @@ -1440,47 +1468,17 @@ void igt_display_init(igt_display_t *display, int
> drm_fd)
> igt_assert(display->outputs);
>
> for (i = 0; i < display->n_outputs; i++) {
> - int j;
> igt_output_t *output = &display->outputs[i];
>
> /*
> - * We're free to select any pipe to drive that output until
> - * a constraint is set with igt_output_set_pipe().
> + * We don't assign each output a pipe unless
> + * a pipe is set with igt_output_set_pipe().
> */
> - output->pending_crtc_idx_mask = -1UL;
> + output->pending_crtc_idx_mask = 0;
> output->id = resources->connectors[i];
> output->display = display;
>
> igt_output_refresh(output);
> -
> - for (j = 0; j < display->n_pipes; j++) {
> - uint64_t prop_value;
> - igt_pipe_t *pipe = &display->pipes[j];
> -
> - if (output->config.crtc) {
> - get_crtc_property(display->drm_fd, output-
> >config.crtc->crtc_id,
> - "background_color",
> - &pipe-
> >background_property,
> - &prop_value,
> - NULL);
> - pipe->background = (uint32_t)prop_value;
> - get_crtc_property(display->drm_fd, output-
> >config.crtc->crtc_id,
> - "DEGAMMA_LUT",
> - &pipe->degamma_property,
> - NULL,
> - NULL);
> - get_crtc_property(display->drm_fd, output-
> >config.crtc->crtc_id,
> - "CTM",
> - &pipe->ctm_property,
> - NULL,
> - NULL);
> - get_crtc_property(display->drm_fd, output-
> >config.crtc->crtc_id,
> - "GAMMA_LUT",
> - &pipe->gamma_property,
> - NULL,
> - NULL);
> - }
> - }
> }
>
> drmModeFreePlaneResources(plane_resources);
> @@ -1545,7 +1543,7 @@ static void igt_display_refresh(igt_display_t *display)
> for (i = 0; i < display->n_outputs; i++) {
> igt_output_t *a = &display->outputs[i];
>
> - if (a->pending_crtc_idx_mask == -1UL)
> + if (!a->pending_crtc_idx_mask)
> continue;
>
> for (j = 0; j < display->n_outputs; j++) {
> @@ -1554,9 +1552,6 @@ static void igt_display_refresh(igt_display_t *display)
> if (i == j)
> continue;
>
> - if (b->pending_crtc_idx_mask == -1UL)
> - continue;
> -
> igt_assert_f(a->pending_crtc_idx_mask !=
> b->pending_crtc_idx_mask,
> "%s and %s are both trying to use pipe
> %s\n",
> @@ -1565,25 +1560,9 @@ static void igt_display_refresh(igt_display_t *display)
> }
> }
>
> - /*
> - * The pipe allocation has to be done in two phases:
> - * - first, try to satisfy the outputs where a pipe has been
> specified
> - * - then, allocate the outputs with PIPE_ANY
> - */
> for (i = 0; i < display->n_outputs; i++) {
> igt_output_t *output = &display->outputs[i];
>
> - if (output->pending_crtc_idx_mask == -1UL)
> - continue;
> -
> - igt_output_refresh(output);
> - }
> - for (i = 0; i < display->n_outputs; i++) {
> - igt_output_t *output = &display->outputs[i];
> -
> - if (output->pending_crtc_idx_mask != -1UL)
> - continue;
> -
> igt_output_refresh(output);
> }
> }
> @@ -1593,12 +1572,11 @@ static igt_pipe_t
> *igt_output_get_driving_pipe(igt_output_t *output)
> igt_display_t *display = output->display;
> enum pipe pipe;
>
> - if (output->pending_crtc_idx_mask == -1UL) {
> + if (!output->pending_crtc_idx_mask) {
> /*
> - * The user hasn't specified a pipe to use, take the one
> - * configured by the last refresh()
> + * The user hasn't specified a pipe to use, return none.
> */
> - pipe = output->config.pipe;
> + return NULL;
> } else {
> /*
> * Otherwise, return the pending pipe (ie the pipe that
> should
> @@ -1628,6 +1606,21 @@ static igt_plane_t *igt_pipe_get_plane(igt_pipe_t
> *pipe, enum igt_plane plane)
> return &pipe->planes[idx];
> }
>
> +static igt_output_t *igt_pipe_get_output(igt_pipe_t *pipe)
> +{
> + igt_display_t *display = pipe->display;
> + int i;
> +
> + for (i = 0; i < display->n_outputs; i++) {
> + igt_output_t *output = &display->outputs[i];
> +
> + if (output->pending_crtc_idx_mask == (1 << pipe->pipe))
> + return output;
> + }
> +
> + return NULL;
> +}
> +
> bool igt_pipe_get_property(igt_pipe_t *pipe, const char *name,
> uint32_t *prop_id, uint64_t *value,
> drmModePropertyPtr *prop)
> @@ -1741,10 +1734,10 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane,
> igt_output_t *output,
> * tests that expect a specific error code).
> */
> static int igt_drm_plane_commit(igt_plane_t *plane,
> - igt_output_t *output,
> + igt_pipe_t *pipe,
> bool fail_on_error)
> {
> - igt_display_t *display = output->display;
> + igt_display_t *display = pipe->display;
> uint32_t fb_id, crtc_id;
> int ret;
> uint32_t src_x;
> @@ -1763,13 +1756,12 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
> !plane->rotation_changed);
>
> fb_id = igt_plane_get_fb_id(plane);
> - crtc_id = output->config.crtc->crtc_id;
> + crtc_id = pipe->crtc_id;
>
> if ((plane->fb_changed || plane->size_changed) && fb_id == 0) {
> LOG(display,
> - "%s: SetPlane pipe %s, plane %d, disabling\n",
> - igt_output_name(output),
> - kmstest_pipe_name(output->config.pipe),
> + "SetPlane pipe %s, plane %d, disabling\n",
> + kmstest_pipe_name(pipe->pipe),
> plane->index);
>
> ret = drmModeSetPlane(display->drm_fd,
> @@ -1797,10 +1789,9 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
> crtc_h = plane->crtc_h;
>
> LOG(display,
> - "%s: SetPlane %s.%d, fb %u, src = (%d, %d) "
> + "SetPlane %s.%d, fb %u, src = (%d, %d) "
> "%ux%u dst = (%u, %u) %ux%u\n",
> - igt_output_name(output),
> - kmstest_pipe_name(output->config.pipe),
> + kmstest_pipe_name(pipe->pipe),
> plane->index,
> fb_id,
> src_x >> 16, src_y >> 16, src_w >> 16, src_h >> 16,
> @@ -1841,11 +1832,11 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
> * code).
> */
> static int igt_cursor_commit_legacy(igt_plane_t *cursor,
> - igt_output_t *output,
> + igt_pipe_t *pipe,
> bool fail_on_error)
> {
> - igt_display_t *display = output->display;
> - uint32_t crtc_id = output->config.crtc->crtc_id;
> + igt_display_t *display = pipe->display;
> + uint32_t crtc_id = pipe->crtc_id;
> int ret;
>
> if (cursor->fb_changed) {
> @@ -1853,9 +1844,8 @@ static int igt_cursor_commit_legacy(igt_plane_t *cursor,
>
> if (gem_handle) {
> LOG(display,
> - "%s: SetCursor pipe %s, fb %u %dx%d\n",
> - igt_output_name(output),
> - kmstest_pipe_name(output->config.pipe),
> + "SetCursor pipe %s, fb %u %dx%d\n",
> + kmstest_pipe_name(pipe->pipe),
> gem_handle,
> cursor->crtc_w, cursor->crtc_h);
>
> @@ -1865,9 +1855,8 @@ static int igt_cursor_commit_legacy(igt_plane_t *cursor,
> cursor->crtc_h);
> } else {
> LOG(display,
> - "%s: SetCursor pipe %s, disabling\n",
> - igt_output_name(output),
> - kmstest_pipe_name(output->config.pipe));
> + "SetCursor pipe %s, disabling\n",
> + kmstest_pipe_name(pipe->pipe));
>
> ret = drmModeSetCursor(display->drm_fd, crtc_id,
> 0, 0, 0);
> @@ -1883,9 +1872,8 @@ static int igt_cursor_commit_legacy(igt_plane_t *cursor,
> int y = cursor->crtc_y;
>
> LOG(display,
> - "%s: MoveCursor pipe %s, (%d, %d)\n",
> - igt_output_name(output),
> - kmstest_pipe_name(output->config.pipe),
> + "MoveCursor pipe %s, (%d, %d)\n",
> + kmstest_pipe_name(pipe->pipe),
> x, y);
>
> ret = drmModeMoveCursor(display->drm_fd, crtc_id, x, y);
> @@ -1902,10 +1890,11 @@ static int igt_cursor_commit_legacy(igt_plane_t
> *cursor,
> * (setmode).
> */
> static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
> - igt_output_t *output,
> + igt_pipe_t *pipe,
> bool fail_on_error)
> {
> struct igt_display *display = primary->pipe->display;
> + igt_output_t *output = igt_pipe_get_output(pipe);
> drmModeModeInfo *mode;
> uint32_t fb_id, crtc_id;
> int ret;
> @@ -1920,7 +1909,7 @@ static int igt_primary_plane_commit_legacy(igt_plane_t
> *primary,
> !primary->size_changed && !primary->panning_changed)
> return 0;
>
> - crtc_id = output->config.crtc->crtc_id;
> + crtc_id = pipe->crtc_id;
> fb_id = igt_plane_get_fb_id(primary);
> if (fb_id)
> mode = igt_output_get_mode(output);
> @@ -1932,7 +1921,7 @@ static int igt_primary_plane_commit_legacy(igt_plane_t
> *primary,
> "%s: SetCrtc pipe %s, fb %u, panning (%d, %d), "
> "mode %dx%d\n",
> igt_output_name(output),
Here you kept the output name in the log while it was removed in all other LOG()
instances. Should it be removed from here too for consistency?
Looks good otherwise.
Ander
> - kmstest_pipe_name(output->config.pipe),
> + kmstest_pipe_name(pipe->pipe),
> fb_id,
> primary->pan_x, primary->pan_y,
> mode->hdisplay, mode->vdisplay);
> @@ -1946,9 +1935,8 @@ static int igt_primary_plane_commit_legacy(igt_plane_t
> *primary,
> mode);
> } else {
> LOG(display,
> - "%s: SetCrtc pipe %s, disabling\n",
> - igt_output_name(output),
> - kmstest_pipe_name(output->config.pipe));
> + "SetCrtc pipe %s, disabling\n",
> + kmstest_pipe_name(pipe->pipe));
>
> ret = drmModeSetCrtc(display->drm_fd,
> crtc_id,
> @@ -1976,17 +1964,17 @@ static int igt_primary_plane_commit_legacy(igt_plane_t
> *primary,
> * which API is used to do the programming.
> */
> static int igt_plane_commit(igt_plane_t *plane,
> - igt_output_t *output,
> + igt_pipe_t *pipe,
> enum igt_commit_style s,
> bool fail_on_error)
> {
> if (plane->is_cursor && s == COMMIT_LEGACY) {
> - return igt_cursor_commit_legacy(plane, output,
> fail_on_error);
> + return igt_cursor_commit_legacy(plane, pipe, fail_on_error);
> } else if (plane->is_primary && s == COMMIT_LEGACY) {
> - return igt_primary_plane_commit_legacy(plane, output,
> + return igt_primary_plane_commit_legacy(plane, pipe,
> fail_on_error);
> } else {
> - return igt_drm_plane_commit(plane, output,
> fail_on_error);
> + return igt_drm_plane_commit(plane, pipe,
> fail_on_error);
> }
> }
>
> @@ -2000,31 +1988,28 @@ static int igt_plane_commit(igt_plane_t *plane,
> * further programming will take place, which may result in some changes
> * taking effect and others not taking effect.
> */
> -static int igt_output_commit(igt_output_t *output,
> - enum igt_commit_style s,
> - bool fail_on_error)
> +static int igt_pipe_commit(igt_pipe_t *pipe,
> + enum igt_commit_style s,
> + bool fail_on_error)
> {
> - igt_display_t *display = output->display;
> - igt_pipe_t *pipe;
> + igt_display_t *display = pipe->display;
> int i;
> int ret;
> bool need_wait_for_vblank = false;
>
> - pipe = igt_output_get_driving_pipe(output);
> -
> if (pipe->background_changed) {
> - igt_crtc_set_property(output, pipe->background_property,
> + igt_crtc_set_property(pipe, pipe->background_property,
> pipe->background);
>
> pipe->background_changed = false;
> }
>
> if (pipe->color_mgmt_changed) {
> - igt_crtc_set_property(output, pipe->degamma_property,
> + igt_crtc_set_property(pipe, pipe->degamma_property,
> pipe->degamma_blob);
> - igt_crtc_set_property(output, pipe->ctm_property,
> + igt_crtc_set_property(pipe, pipe->ctm_property,
> pipe->ctm_blob);
> - igt_crtc_set_property(output, pipe->gamma_property,
> + igt_crtc_set_property(pipe, pipe->gamma_property,
> pipe->gamma_blob);
>
> pipe->color_mgmt_changed = false;
> @@ -2036,7 +2021,7 @@ static int igt_output_commit(igt_output_t *output,
> if (plane->fb_changed || plane->position_changed || plane-
> >size_changed)
> need_wait_for_vblank = true;
>
> - ret = igt_plane_commit(plane, output, s, fail_on_error);
> + ret = igt_plane_commit(plane, pipe, s, fail_on_error);
> CHECK_RETURN(ret, fail_on_error);
> }
>
> @@ -2060,6 +2045,9 @@ static void igt_atomic_prepare_crtc_commit(igt_output_t
> *output, drmModeAtomicRe
>
> igt_pipe_t *pipe_obj = igt_output_get_driving_pipe(output);
>
> + if (!pipe_obj)
> + return;
> +
> if (pipe_obj->background_changed)
> igt_atomic_populate_crtc_req(req, output,
> IGT_CRTC_BACKGROUND, pipe_obj->background);
>
> @@ -2125,6 +2113,8 @@ static int igt_atomic_commit(igt_display_t *display)
>
>
> pipe_obj = igt_output_get_driving_pipe(output);
> + if (!pipe_obj)
> + continue;
>
> pipe = pipe_obj->pipe;
>
> @@ -2167,14 +2157,14 @@ static int do_display_commit(igt_display_t *display,
>
> }
>
> - for (i = 0; i < display->n_outputs; i++) {
> - igt_output_t *output = &display->outputs[i];
> + 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->valid)
> - continue;
> + if (output && output->valid)
> + valid_outs++;
>
> - valid_outs++;
> - ret = igt_output_commit(output, s, fail_on_error);
> + ret = igt_pipe_commit(pipe_obj, s, fail_on_error);
> CHECK_RETURN(ret, fail_on_error);
> }
>
> @@ -2282,9 +2272,9 @@ void igt_output_set_pipe(igt_output_t *output, enum pipe
> pipe)
> {
> igt_display_t *display = output->display;
>
> - if (pipe == PIPE_ANY) {
> + if (pipe == PIPE_NONE) {
> LOG(display, "%s: set_pipe(any)\n", igt_output_name(output));
> - output->pending_crtc_idx_mask = -1UL;
> + output->pending_crtc_idx_mask = 0;
> } else {
> LOG(display, "%s: set_pipe(%s)\n", igt_output_name(output),
> kmstest_pipe_name(pipe));
> @@ -2297,6 +2287,8 @@ igt_plane_t *igt_output_get_plane(igt_output_t *output,
> enum igt_plane plane)
> igt_pipe_t *pipe;
>
> pipe = igt_output_get_driving_pipe(output);
> + igt_assert(pipe);
> +
> return igt_pipe_get_plane(pipe, plane);
> }
>
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index dc6be5e53b74..3531dc20b6e0 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -334,17 +334,31 @@ void igt_fb_set_size(struct igt_fb *fb, igt_plane_t
> *plane,
>
> void igt_wait_for_vblank(int drm_fd, enum pipe pipe);
>
> +static inline bool igt_output_is_connected(igt_output_t *output)
> +{
> + /* Something went wrong during probe? */
> + if (!output->config.connector)
> + return false;
> +
> + if (output->config.connector->connection == DRM_MODE_CONNECTED)
> + return true;
> +
> + return false;
> +}
> +
> static inline bool igt_pipe_connector_valid(enum pipe pipe,
> igt_output_t *output)
> {
> - return output->valid && (output->config.valid_crtc_idx_mask & (1 <<
> pipe));
> + return igt_output_is_connected(output) &&
> + (output->config.valid_crtc_idx_mask & (1 << pipe));
> }
>
> #define for_each_if(condition) if (!(condition)) {} else
>
> #define for_each_connected_output(display, output) \
> for (int i__ = 0; i__ < (display)->n_outputs; i__++) \
> - for_each_if (((output = &(display)->outputs[i__]), output-
> >valid))
> + for_each_if (((output = &(display)->outputs[i__]), \
> + igt_output_is_connected(output)))
>
> #define for_each_pipe(display, pipe) \
> for (pipe = 0; pipe < igt_display_get_n_pipes(display); pipe++)
> \
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
