[igt-dev] [PATCH i-g-t] lib/igt_kms: Fix commits for planes with multiple possible CRTCs
Kazlauskas, Nicholas
Nicholas.Kazlauskas at amd.com
Fri Feb 22 15:01:48 UTC 2019
On 2/22/19 9:51 AM, Daniel Vetter wrote:
> On Thu, Feb 21, 2019 at 04:23:36PM -0500, Nicholas Kazlauskas wrote:
>> An igt_plane_t is defined per igt_pipe_t. It is treated as its
>> own independent resource but DRM planes can be exposed to being used on
>> a number of possible CRTCs - making it a shared resource.
>>
>> In IGT planes with multiple possible CRTCs are added to the plane list
>> for each pipe that the plane supports. The internal state remains
>> independent in IGT so when the same plane is modified for multiple
>> pipes in a single commit the last pipe to modify it is the one whose
>> state gets fully applied.
>>
>> This situation happens fairly often in practice - resetting the display
>> at the start of the test before a commit will reset the CRTC ID and FB
>> ID for each plane.
>>
>> For an example, consider the
>> igt at kms_plane_alpha_blend@pipe-a-constant-alpha-max test.
>>
>> This test will fail for any overlay plane exposed for multiple CRTCs.
>>
>> The test tries to set a framebuffer for pipe A but has all the other
>> pipes reset the plane state in the same commit. If there are multiple
>> pipes on the hardware then the last pipe will be the one to set all the
>> plane properties. The driver will receive a commit with no overlay
>> plane enabled since the last pipe set CRTC ID and FB ID to 0, disabling
>> the plane. The reference CRC capture will be incorrect since not all
>> the planes have been enabled and the subsequent CRC captures will
>> not match, failing the test.
>>
>> The simplest (but hacky) fix to this problem is to only set the
>> properties for the plane for the pipe that last had it bound.
>>
>> This patch introduces a global plane list on igt_display_t that keeps
>> track of the pipe that pipe that last called igt_plane_set_fb. The
>> properties for the plane will only be applied from that single pipe
>> when commiting the state to DRM.
>>
>> No behavioral changes should be introduced by this patch for hardware
>> whose planes are only ever exposed one CRTC.
>>
>> It would likely be best to eventually modify the igt_pipe_t plane list
>> to be a list of pointers to planes instead (igt_plane_t**)
>> instead of being the actual plane objects, but that can come later.
>>
>> Many areas of the code like to make use of the backpointer to the pipe
>> on the plane which makes refactoring the code in that manner a little
>> trickier.
>
> Thanks a lot for tackling this intel-ism in igt, which indeed is totally
> not how kms works (outside of recent i915 hw).
>
> I think your plan to first build up a global plane list which aliases to
> the current per-pipe planes as a first step is sound.
>
> The next step we can probably do gradually, i.e. add a new
> igt_pipe->plane_res or so which is the list of pointers for the global
> plane_res array. And then step-by-step fade out usage of igt_pipe_planes.
>
> Needs a bit of book-keeping for the transition, but should be doable.
Yeah, I saw this as happening in two phases as well.
Though I think it would be easier to just convert the existing planes
list to an array of pointers and audit everywhere that tried using it
directly. Most tests actually do make use of the iteration helpers
already so it's mostly only igt_kms and 3 or 4 tests that want to access
it directly.
>
>>
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
>> ---
>> lib/igt_kms.c | 76 +++++++++++++++++++++++++++++++--------------------
>> lib/igt_kms.h | 10 +++++++
>> 2 files changed, 57 insertions(+), 29 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 080f90ae..f133a5cd 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -1905,6 +1905,20 @@ void igt_display_require(igt_display_t *display, int drm_fd)
>> plane_resources = drmModeGetPlaneResources(display->drm_fd);
>> igt_assert(plane_resources);
>>
>> + display->n_planes = plane_resources->count_planes;
>> + display->planes = calloc(sizeof(igt_plane_res_t), display->n_planes);
>> + igt_assert_f(display->planes, "Failed to allocate memory for %d planes\n", display->n_planes);
>> +
>> + for (i = 0; i < plane_resources->count_planes; ++i) {
>> + igt_plane_res_t *plane = &display->planes[i];
>> + uint32_t id = plane_resources->planes[i];
>> +
>> + plane->drm_plane = drmModeGetPlane(display->drm_fd, id);
>> + igt_assert(plane->drm_plane);
>> +
>> + plane->type = get_drm_plane_type(display->drm_fd, id);
>> + }
>> +
>> for_each_pipe(display, i) {
>> igt_pipe_t *pipe = &display->pipes[i];
>> igt_plane_t *plane;
>> @@ -1922,17 +1936,12 @@ void igt_display_require(igt_display_t *display, int drm_fd)
>> igt_fill_pipe_props(display, pipe, IGT_NUM_CRTC_PROPS, igt_crtc_prop_names);
>>
>> /* count number of valid planes */
>> - for (j = 0; j < plane_resources->count_planes; j++) {
>> - drmModePlane *drm_plane;
>> -
>> - drm_plane = drmModeGetPlane(display->drm_fd,
>> - plane_resources->planes[j]);
>> + for (j = 0; j < display->n_planes; j++) {
>> + drmModePlane *drm_plane = display->planes[j].drm_plane;
>> igt_assert(drm_plane);
>>
>> if (drm_plane->possible_crtcs & (1 << i))
>> n_planes++;
>> -
>> - drmModeFreePlane(drm_plane);
>> }
>>
>> igt_assert_lt(0, n_planes);
>> @@ -1941,20 +1950,14 @@ void igt_display_require(igt_display_t *display, int drm_fd)
>> last_plane = n_planes - 1;
>>
>> /* add the planes that can be used with that pipe */
>> - for (j = 0; j < plane_resources->count_planes; j++) {
>> - drmModePlane *drm_plane;
>> -
>> - drm_plane = drmModeGetPlane(display->drm_fd,
>> - plane_resources->planes[j]);
>> - igt_assert(drm_plane);
>> + for (j = 0; j < display->n_planes; j++) {
>> + igt_plane_res_t *res = &display->planes[j];
>> + drmModePlane *drm_plane = res->drm_plane;
>>
>> - if (!(drm_plane->possible_crtcs & (1 << i))) {
>> - drmModeFreePlane(drm_plane);
>> + if (!(drm_plane->possible_crtcs & (1 << i)))
>> continue;
>> - }
>>
>> - type = get_drm_plane_type(display->drm_fd,
>> - plane_resources->planes[j]);
>> + type = res->type;
>>
>> if (type == DRM_PLANE_TYPE_PRIMARY && pipe->plane_primary == -1) {
>> plane = &pipe->planes[0];
>> @@ -1975,6 +1978,10 @@ void igt_display_require(igt_display_t *display, int drm_fd)
>> plane->pipe = pipe;
>> plane->drm_plane = drm_plane;
>> plane->values[IGT_PLANE_IN_FENCE_FD] = ~0ULL;
>> + plane->res = res;
>> +
>> + if (!res->state)
>> + res->state = plane;
>>
>> igt_fill_plane_props(display, plane, IGT_NUM_PLANE_PROPS, igt_plane_prop_names);
>>
>> @@ -2127,17 +2134,6 @@ igt_output_t *igt_output_from_connector(igt_display_t *display,
>>
>> static void igt_pipe_fini(igt_pipe_t *pipe)
>> {
>> - int i;
>> -
>> - for (i = 0; i < pipe->n_planes; i++) {
>> - igt_plane_t *plane = &pipe->planes[i];
>> -
>> - if (plane->drm_plane) {
>> - drmModeFreePlane(plane->drm_plane);
>> - plane->drm_plane = NULL;
>> - }
>> - }
>> -
>> free(pipe->planes);
>> pipe->planes = NULL;
>>
>> @@ -2163,6 +2159,15 @@ void igt_display_fini(igt_display_t *display)
>> {
>> int i;
>>
>> + for (i = 0; i < display->n_planes; ++i) {
>> + igt_plane_res_t *plane = &display->planes[i];
>> +
>> + if (plane->drm_plane) {
>> + drmModeFreePlane(plane->drm_plane);
>> + plane->drm_plane = NULL;
>> + }
>> + }
>> +
>> for (i = 0; i < display->n_pipes; i++)
>> igt_pipe_fini(&display->pipes[i]);
>>
>> @@ -2172,6 +2177,8 @@ void igt_display_fini(igt_display_t *display)
>> display->outputs = NULL;
>> free(display->pipes);
>> display->pipes = NULL;
>> + free(display->planes);
>> + display->planes = NULL;
>> }
>>
>> static void igt_display_refresh(igt_display_t *display)
>> @@ -2789,6 +2796,10 @@ static int igt_pipe_commit(igt_pipe_t *pipe,
>> for (i = 0; i < pipe->n_planes; i++) {
>> igt_plane_t *plane = &pipe->planes[i];
>>
>> + /* skip planes that are handled by another pipe */
>> + if (plane->res->state != plane)
>> + continue;
>> +
>> ret = igt_plane_commit(plane, pipe, s, fail_on_error);
>> CHECK_RETURN(ret, fail_on_error);
>> }
>> @@ -3177,6 +3188,10 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
>> igt_atomic_prepare_crtc_commit(pipe_obj, req);
>>
>> for_each_plane_on_pipe(display, pipe, plane) {
>> + /* skip planes that are handled by another pipe */
>> + if (plane->res->state != plane)
>> + continue;
>> +
>> if (plane->changed)
>> igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
>> }
>> @@ -3709,6 +3724,9 @@ void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb)
>> if (igt_plane_has_prop(plane, IGT_PLANE_COLOR_RANGE))
>> igt_plane_set_prop_enum(plane, IGT_PLANE_COLOR_RANGE,
>> igt_color_range_to_str(fb->color_range));
>> +
>> + /* hack to prioritize plane on crtc it was last bound to */
>> + plane->res->state = plane;
>
> Hm, I'd creata a new igt_plane_set_pipe and put this in there. That would
> give you a nice place to update all the other transition pointers we might
> eventually need. Plus would allow us to gradually switch tests over to the
> new world, but maybe better to keep that for the next step.
That sounds reasonable to me, I can add that in a v2.
>
>> } else {
>> igt_plane_set_size(plane, 0, 0);
>>
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> index 679d4e84..32d840ce 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -285,6 +285,7 @@ extern const char * const igt_plane_prop_names[];
>>
>> typedef struct igt_display igt_display_t;
>> typedef struct igt_pipe igt_pipe_t;
>> +typedef struct igt_plane_res igt_plane_res_t;
>> typedef uint32_t igt_fixed_t; /* 16.16 fixed point */
>>
>> typedef enum {
>> @@ -303,6 +304,7 @@ typedef enum {
>> typedef struct {
>> /*< private >*/
>> igt_pipe_t *pipe;
>> + igt_plane_res_t *res;
>> int index;
>> /* capabilities */
>> int type;
>> @@ -332,6 +334,12 @@ typedef struct {
>> int format_mod_count;
>> } igt_plane_t;
>>
>> +typedef struct igt_plane_res {
>> + igt_plane_t *state;
>> + drmModePlane *drm_plane;
>> + int type;
>> +} igt_plane_res_t;
>
> Why not just reuse igt_plane_t for this? This means some duplication and
> syncing (which you could do in the igt_plane_set_pipe for the transition),
> but I think cleaner path forward. And easier to transition tests I think.
> -Daniel
Yeah, that's probably cleaner. The one problem with this is that I'd
still need a way to map the global plane to the pipe plane, but I think
I could always using the drm_plane pointer itself and iterate over the
list every time or I could just add a new member into igt_plane_t for
this purpose during the transition.
Thanks!
Nicholas Kazlauskas
>
>> +
>> struct igt_pipe {
>> igt_display_t *display;
>> enum pipe pipe;
>> @@ -372,8 +380,10 @@ struct igt_display {
>> int drm_fd;
>> int log_shift;
>> int n_pipes;
>> + int n_planes;
>> int n_outputs;
>> igt_output_t *outputs;
>> + igt_plane_res_t *planes;
>> igt_pipe_t *pipes;
>> bool has_cursor_plane;
>> bool is_atomic;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> igt-dev mailing list
>> igt-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/igt-dev
>
More information about the igt-dev
mailing list