| Message ID | 1401376157-24940-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 29, 2014 at 08:09:13AM -0700, Matt Roper wrote: > Add support for universal planes. This involves revamping the existing > plane handling a bit to allow primary & cursor planes to come from the > DRM plane list, rather than always being manually added. Also, > eliminate the hard-coded position of primary & cursor in the plane list > since the DRM could return them in any order. > > To minimize impact for existing tests, we add a new > igt_display_use_universal_commits() call to toggle between universal and > legacy API's for programming the primary and cursor planes. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> With the tiny change below, that patch looks reasonable. So: Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> One thing we may want to clean up when we bring in even more ways to do commits is how we store the states to flush. What I mean by that is: - In the *_set_*() functions (eg. igt_plane_set_fb()), we store the states that change (eg. in set_fb() we only set fb_changed to true and don't touch need_set_crtc/need_set_cursor) - In the commit, we resolve what we need to do depending on the states changed and the commit method. > --- > lib/igt_kms.c | 132 +++++++++++++++++++++++++++++++++++++++++++++------------- > lib/igt_kms.h | 5 +++ > 2 files changed, 107 insertions(+), 30 deletions(-) > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > index d00250d..97e329b 100644 > --- a/lib/igt_kms.c > +++ b/lib/igt_kms.c > @@ -500,27 +500,24 @@ void igt_display_init(igt_display_t *display, int drm_fd) > */ > display->n_pipes = resources->count_crtcs; > > + drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1); Can we have: #ifndef DRM_CLIENT_CAP_UNIVERSAL_PLANES #define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2 #endif As an easy way to make sure that's always defined, even if we don't have the (unreleased) libdrm with this support? > plane_resources = drmModeGetPlaneResources(display->drm_fd); > igt_assert(plane_resources); > > for (i = 0; i < display->n_pipes; i++) { > igt_pipe_t *pipe = &display->pipes[i]; > - igt_plane_t *plane; > - int p, j; > + igt_plane_t *plane = NULL; > + int p = 0, j; > > pipe->display = display; > pipe->pipe = i; > > - /* primary plane */ > - p = IGT_PLANE_PRIMARY; > - plane = &pipe->planes[p]; > - plane->pipe = pipe; > - plane->index = p; > - plane->is_primary = true; > - > /* add the planes that can be used with that pipe */ > for (j = 0; j < plane_resources->count_planes; j++) { > drmModePlane *drm_plane; > + drmModeObjectPropertiesPtr proplist; > + drmModePropertyPtr prop; > + int k; > > drm_plane = drmModeGetPlane(display->drm_fd, > plane_resources->planes[j]); > @@ -531,21 +528,67 @@ void igt_display_init(igt_display_t *display, int drm_fd) > continue; > } > > - p++; > plane = &pipe->planes[p]; > plane->pipe = pipe; > - plane->index = p; > + plane->index = p++; > plane->drm_plane = drm_plane; > + > + prop = NULL; > + proplist = drmModeObjectGetProperties(display->drm_fd, > + plane_resources->planes[j], > + DRM_MODE_OBJECT_PLANE); > + for (k = 0; k < proplist->count_props; k++) { > + prop = drmModeGetProperty(display->drm_fd, proplist->props[k]); > + if (!prop) > + continue; > + > + if (strcmp(prop->name, "type") != 0) { > + drmModeFreeProperty(prop); > + continue; > + } > + > + switch (proplist->prop_values[k]) { > + case DRM_PLANE_TYPE_PRIMARY: > + plane->is_primary = 1; > + display->has_universal_planes = 1; > + break; > + case DRM_PLANE_TYPE_CURSOR: > + plane->is_cursor = 1; > + pipe->has_cursor = 1; > + display->has_universal_planes = 1; > + break; > + } > + > + drmModeFreeProperty(prop); > + break; > + } > + > } > > - /* cursor plane */ > - p++; > - plane = &pipe->planes[p]; > - plane->pipe = pipe; > - plane->index = p; > - plane->is_cursor = true; > + /* > + * Add a drm_plane-less primary plane for kernels without > + * universal plane support. > + */ > + if (!display->has_universal_planes) { > + plane = &pipe->planes[p]; > + plane->pipe = pipe; > + plane->index = p++; > + plane->is_primary = true; > + } > > - pipe->n_planes = ++p; > + /* > + * Add drm_plane-less cursor plane for kernels that don't > + * expose a universal cursor plane. > + */ > + if (!pipe->has_cursor) { > + /* cursor plane */ > + plane = &pipe->planes[p]; > + plane->pipe = pipe; > + plane->index = p++; > + plane->is_cursor = true; > + } > + > + pipe->n_planes = p; > > /* make sure we don't overflow the plane array */ > igt_assert(pipe->n_planes <= IGT_MAX_PLANES); > @@ -700,16 +743,29 @@ static igt_plane_t *igt_pipe_get_plane(igt_pipe_t *pipe, enum igt_plane plane) > { > int idx; > > - /* Cursor plane is always the upper plane */ > - if (plane == IGT_PLANE_CURSOR) > - idx = pipe->n_planes - 1; > - else { > - igt_assert_f(plane >= 0 && plane < (pipe->n_planes), > - "plane=%d\n", plane); > - idx = plane; > + for (idx = 0; idx < pipe->n_planes; idx++) { > + switch (plane) { > + case IGT_PLANE_PRIMARY: > + if (pipe->planes[idx].is_primary) > + return &pipe->planes[idx]; > + break; > + case IGT_PLANE_CURSOR: > + if (pipe->planes[idx].is_cursor) > + return &pipe->planes[idx]; > + break; > + case IGT_PLANE_2: > + if (!pipe->planes[idx].is_primary && > + !pipe->planes[idx].is_cursor) > + return &pipe->planes[idx]; > + default: > + if (!pipe->planes[idx].is_primary && > + !pipe->planes[idx].is_cursor) > + /* Consume this overlay and keep searching for another */ > + plane--; > + } > } > > - return &pipe->planes[idx]; > + return NULL; > } > > static uint32_t igt_plane_get_fb_id(igt_plane_t *plane) > @@ -761,6 +817,8 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output) > uint32_t fb_id, crtc_id; > int ret; > > + igt_assert(plane->drm_plane); > + > fb_id = igt_plane_get_fb_id(plane); > crtc_id = output->config.crtc->crtc_id; > pipe = igt_output_get_driving_pipe(output); > @@ -820,7 +878,11 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output) > > static int igt_plane_commit(igt_plane_t *plane, igt_output_t *output) > { > - if (plane->is_cursor) { > + struct igt_display *display = plane->pipe->display; > + > + if (display->commit_universal && plane->drm_plane) { > + igt_drm_plane_commit(plane, output); > + } else if (plane->is_cursor) { > igt_cursor_commit(plane, output); > } else if (plane->is_primary) { > /* state updated by SetCrtc */ > @@ -838,8 +900,8 @@ static int igt_output_commit(igt_output_t *output) > int i; > > pipe = igt_output_get_driving_pipe(output); > - if (pipe->need_set_crtc) { > - igt_plane_t *primary = &pipe->planes[0]; > + if (pipe->need_set_crtc && !display->commit_universal) { > + igt_plane_t *primary = igt_pipe_get_plane(pipe, IGT_PLANE_PRIMARY); > drmModeModeInfo *mode; > uint32_t fb_id, crtc_id; > int ret; > @@ -889,7 +951,7 @@ static int igt_output_commit(igt_output_t *output) > primary->fb_changed = false; > } > > - if (pipe->need_set_cursor) { > + if (pipe->need_set_cursor && !display->commit_universal) { > igt_plane_t *cursor; > uint32_t gem_handle, crtc_id; > int ret; > @@ -940,6 +1002,16 @@ static int igt_output_commit(igt_output_t *output) > return 0; > } > > +/* > + * Indicate whether future display commits should use universal plane API's > + * or legacy API's. > + */ > +void igt_display_use_universal_commits(igt_display_t *display, > + bool use_universal) > +{ > + display->commit_universal = use_universal; > +} > + > int igt_display_commit(igt_display_t *display) > { > int i; > diff --git a/lib/igt_kms.h b/lib/igt_kms.h > index 8e80d4b..4955fc8 100644 > --- a/lib/igt_kms.h > +++ b/lib/igt_kms.h > @@ -121,6 +121,7 @@ struct igt_pipe { > unsigned int need_set_crtc : 1; > unsigned int need_set_cursor : 1; > unsigned int need_wait_for_vblank : 1; > + unsigned int has_cursor : 1; > #define IGT_MAX_PLANES 4 > int n_planes; > igt_plane_t planes[IGT_MAX_PLANES]; > @@ -143,6 +144,8 @@ struct igt_display { > unsigned long pipes_in_use; > igt_output_t *outputs; > igt_pipe_t pipes[I915_MAX_PIPES]; > + bool has_universal_planes; > + bool commit_universal; > }; > > /* set vt into graphics mode, required to prevent fbcon from interfering */ > @@ -150,6 +153,8 @@ void igt_set_vt_graphics_mode(void); > > void igt_display_init(igt_display_t *display, int drm_fd); > void igt_display_fini(igt_display_t *display); > +void igt_display_use_universal_commits(igt_display_t *display, > + bool use_universal); > int igt_display_commit(igt_display_t *display); > int igt_display_get_n_pipes(igt_display_t *display); > > -- > 1.8.5.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
As a small summary, I don't think we should be exposing the plane_commit() function and always go through igt_display_commit() to make hooking the atomic ioctl() easier. Loosing the plane ordering is a bit meh but should be able to live with it. The rest looks good.
diff --git a/lib/igt_kms.c b/lib/igt_kms.c index d00250d..97e329b 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -500,27 +500,24 @@ void igt_display_init(igt_display_t *display, int drm_fd) */ display->n_pipes = resources->count_crtcs; + drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1); plane_resources = drmModeGetPlaneResources(display->drm_fd); igt_assert(plane_resources); for (i = 0; i < display->n_pipes; i++) { igt_pipe_t *pipe = &display->pipes[i]; - igt_plane_t *plane; - int p, j; + igt_plane_t *plane = NULL; + int p = 0, j; pipe->display = display; pipe->pipe = i; - /* primary plane */ - p = IGT_PLANE_PRIMARY; - plane = &pipe->planes[p]; - plane->pipe = pipe; - plane->index = p; - plane->is_primary = true; - /* add the planes that can be used with that pipe */ for (j = 0; j < plane_resources->count_planes; j++) { drmModePlane *drm_plane; + drmModeObjectPropertiesPtr proplist; + drmModePropertyPtr prop; + int k; drm_plane = drmModeGetPlane(display->drm_fd, plane_resources->planes[j]); @@ -531,21 +528,67 @@ void igt_display_init(igt_display_t *display, int drm_fd) continue; } - p++; plane = &pipe->planes[p]; plane->pipe = pipe; - plane->index = p; + plane->index = p++; plane->drm_plane = drm_plane; + + prop = NULL; + proplist = drmModeObjectGetProperties(display->drm_fd, + plane_resources->planes[j], + DRM_MODE_OBJECT_PLANE); + for (k = 0; k < proplist->count_props; k++) { + prop = drmModeGetProperty(display->drm_fd, proplist->props[k]); + if (!prop) + continue; + + if (strcmp(prop->name, "type") != 0) { + drmModeFreeProperty(prop); + continue; + } + + switch (proplist->prop_values[k]) { + case DRM_PLANE_TYPE_PRIMARY: + plane->is_primary = 1; + display->has_universal_planes = 1; + break; + case DRM_PLANE_TYPE_CURSOR: + plane->is_cursor = 1; + pipe->has_cursor = 1; + display->has_universal_planes = 1; + break; + } + + drmModeFreeProperty(prop); + break; + } + } - /* cursor plane */ - p++; - plane = &pipe->planes[p]; - plane->pipe = pipe; - plane->index = p; - plane->is_cursor = true; + /* + * Add a drm_plane-less primary plane for kernels without + * universal plane support. + */ + if (!display->has_universal_planes) { + plane = &pipe->planes[p]; + plane->pipe = pipe; + plane->index = p++; + plane->is_primary = true; + } - pipe->n_planes = ++p; + /* + * Add drm_plane-less cursor plane for kernels that don't + * expose a universal cursor plane. + */ + if (!pipe->has_cursor) { + /* cursor plane */ + plane = &pipe->planes[p]; + plane->pipe = pipe; + plane->index = p++; + plane->is_cursor = true; + } + + pipe->n_planes = p; /* make sure we don't overflow the plane array */ igt_assert(pipe->n_planes <= IGT_MAX_PLANES); @@ -700,16 +743,29 @@ static igt_plane_t *igt_pipe_get_plane(igt_pipe_t *pipe, enum igt_plane plane) { int idx; - /* Cursor plane is always the upper plane */ - if (plane == IGT_PLANE_CURSOR) - idx = pipe->n_planes - 1; - else { - igt_assert_f(plane >= 0 && plane < (pipe->n_planes), - "plane=%d\n", plane); - idx = plane; + for (idx = 0; idx < pipe->n_planes; idx++) { + switch (plane) { + case IGT_PLANE_PRIMARY: + if (pipe->planes[idx].is_primary) + return &pipe->planes[idx]; + break; + case IGT_PLANE_CURSOR: + if (pipe->planes[idx].is_cursor) + return &pipe->planes[idx]; + break; + case IGT_PLANE_2: + if (!pipe->planes[idx].is_primary && + !pipe->planes[idx].is_cursor) + return &pipe->planes[idx]; + default: + if (!pipe->planes[idx].is_primary && + !pipe->planes[idx].is_cursor) + /* Consume this overlay and keep searching for another */ + plane--; + } } - return &pipe->planes[idx]; + return NULL; } static uint32_t igt_plane_get_fb_id(igt_plane_t *plane) @@ -761,6 +817,8 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output) uint32_t fb_id, crtc_id; int ret; + igt_assert(plane->drm_plane); + fb_id = igt_plane_get_fb_id(plane); crtc_id = output->config.crtc->crtc_id; pipe = igt_output_get_driving_pipe(output); @@ -820,7 +878,11 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output) static int igt_plane_commit(igt_plane_t *plane, igt_output_t *output) { - if (plane->is_cursor) { + struct igt_display *display = plane->pipe->display; + + if (display->commit_universal && plane->drm_plane) { + igt_drm_plane_commit(plane, output); + } else if (plane->is_cursor) { igt_cursor_commit(plane, output); } else if (plane->is_primary) { /* state updated by SetCrtc */ @@ -838,8 +900,8 @@ static int igt_output_commit(igt_output_t *output) int i; pipe = igt_output_get_driving_pipe(output); - if (pipe->need_set_crtc) { - igt_plane_t *primary = &pipe->planes[0]; + if (pipe->need_set_crtc && !display->commit_universal) { + igt_plane_t *primary = igt_pipe_get_plane(pipe, IGT_PLANE_PRIMARY); drmModeModeInfo *mode; uint32_t fb_id, crtc_id; int ret; @@ -889,7 +951,7 @@ static int igt_output_commit(igt_output_t *output) primary->fb_changed = false; } - if (pipe->need_set_cursor) { + if (pipe->need_set_cursor && !display->commit_universal) { igt_plane_t *cursor; uint32_t gem_handle, crtc_id; int ret; @@ -940,6 +1002,16 @@ static int igt_output_commit(igt_output_t *output) return 0; } +/* + * Indicate whether future display commits should use universal plane API's + * or legacy API's. + */ +void igt_display_use_universal_commits(igt_display_t *display, + bool use_universal) +{ + display->commit_universal = use_universal; +} + int igt_display_commit(igt_display_t *display) { int i; diff --git a/lib/igt_kms.h b/lib/igt_kms.h index 8e80d4b..4955fc8 100644 --- a/lib/igt_kms.h +++ b/lib/igt_kms.h @@ -121,6 +121,7 @@ struct igt_pipe { unsigned int need_set_crtc : 1; unsigned int need_set_cursor : 1; unsigned int need_wait_for_vblank : 1; + unsigned int has_cursor : 1; #define IGT_MAX_PLANES 4 int n_planes; igt_plane_t planes[IGT_MAX_PLANES]; @@ -143,6 +144,8 @@ struct igt_display { unsigned long pipes_in_use; igt_output_t *outputs; igt_pipe_t pipes[I915_MAX_PIPES]; + bool has_universal_planes; + bool commit_universal; }; /* set vt into graphics mode, required to prevent fbcon from interfering */ @@ -150,6 +153,8 @@ void igt_set_vt_graphics_mode(void); void igt_display_init(igt_display_t *display, int drm_fd); void igt_display_fini(igt_display_t *display); +void igt_display_use_universal_commits(igt_display_t *display, + bool use_universal); int igt_display_commit(igt_display_t *display); int igt_display_get_n_pipes(igt_display_t *display);
Add support for universal planes. This involves revamping the existing plane handling a bit to allow primary & cursor planes to come from the DRM plane list, rather than always being manually added. Also, eliminate the hard-coded position of primary & cursor in the plane list since the DRM could return them in any order. To minimize impact for existing tests, we add a new igt_display_use_universal_commits() call to toggle between universal and legacy API's for programming the primary and cursor planes. Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- lib/igt_kms.c | 132 +++++++++++++++++++++++++++++++++++++++++++++------------- lib/igt_kms.h | 5 +++ 2 files changed, 107 insertions(+), 30 deletions(-)