[igt-dev] [PATCH i-g-t v3] lib/igt_kms: Fix commits for planes with multiple possible CRTCs
Kazlauskas, Nicholas
Nicholas.Kazlauskas at amd.com
Thu Feb 28 14:29:59 UTC 2019
On 2/28/19 5:23 AM, Daniel Vetter wrote:
> On Wed, Feb 27, 2019 at 08:13:37PM +0000, Wentland, Harry wrote:
>> On 2019-02-25 10:42 a.m., 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.
>>>
>>> v2: Add igt_plane_set_fb, use igt_plane_t for global plane list (Daniel)
>>> v3: Leave TODO for filling in all state/props on global planes
>>>
>>> Cc: Daniel Vetter <daniel at ffwll.ch>
>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
>>
>> Acked-by: Harry Wentland <harry.wentland at amd.com>
>>
>> Daniel, ping.
>
> Sry, drowning and behind mails. I acked the general plan already, as long
> as you don't upset CI all fine with me. Once it's all done would be good
> to make a fine combed pass over docs and make sure all the fixed
> pipe/plane association language is also gone.
> -Daniel
Sounds good to me, thanks for the tips.
Rev3 of this patch doesn't seem to break anything since it passes both
fast and full CI runs. Just needs a review from anyone willing.
Like mentioned in previous emails, when I get some time I'd like to
update the plane creation to be done on global DRM plane list entirely.
Pipes could then reference the list with its own internal list pointing
back to the global one.
Then the plane iteration macro (per pipe) can be updated to not take the
address of the plane but just return the reference to the global list.
There are still ~5 or so remaining tests that use this list directly
from what I can tell but they can probably be updated accordingly.
Then the last bit is updating the documentation where appropriate. It'd
probably be good to note that the pipes are shared objects, so any state
modifications from "different" pipes will affect each other. Not that
anything uses planes like that in its current state from what I can tell.
I think it's fine to leave setting the pipe implicitly on when setting
the FB at this point because that's generally why you'd be setting an FB
in the first place. There's the new helper as part of this patch that
lets you reassign the plane to a different pipe even when the FB is
already bound in case that functionality is needed somewhere.
Nicholas Kazlauskas
>
>>
>> Harry
>>
>>> ---
>>> lib/igt_kms.c | 103 ++++++++++++++++++++++++++++++++++++--------------
>>> lib/igt_kms.h | 6 ++-
>>> 2 files changed, 79 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>> index 761bb193..a68a9087 100644
>>> --- a/lib/igt_kms.c
>>> +++ b/lib/igt_kms.c
>>> @@ -1905,6 +1905,26 @@ 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_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_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);
>>> +
>>> + /*
>>> + * TODO: Fill in the rest of the plane properties here and
>>> + * move away from the plane per pipe model to align closer
>>> + * to the DRM KMS model.
>>> + */
>>> + }
>>> +
>>> for_each_pipe(display, i) {
>>> igt_pipe_t *pipe = &display->pipes[i];
>>> igt_plane_t *plane;
>>> @@ -1922,17 +1942,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 +1956,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_t *global_plane = &display->planes[j];
>>> + drmModePlane *drm_plane = global_plane->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 = global_plane->type;
>>>
>>> if (type == DRM_PLANE_TYPE_PRIMARY && pipe->plane_primary == -1) {
>>> plane = &pipe->planes[0];
>>> @@ -1975,6 +1984,14 @@ 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->ref = global_plane;
>>> +
>>> + /*
>>> + * HACK: point the global plane to the first pipe that
>>> + * it can go on.
>>> + */
>>> + if (!global_plane->ref)
>>> + igt_plane_set_pipe(plane, pipe);
>>>
>>> igt_fill_plane_props(display, plane, IGT_NUM_PLANE_PROPS, igt_plane_prop_names);
>>>
>>> @@ -2127,17 +2144,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 +2169,15 @@ void igt_display_fini(igt_display_t *display)
>>> {
>>> int i;
>>>
>>> + for (i = 0; i < display->n_planes; ++i) {
>>> + igt_plane_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 +2187,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 +2806,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->ref->pipe != pipe)
>>> + continue;
>>> +
>>> ret = igt_plane_commit(plane, pipe, s, fail_on_error);
>>> CHECK_RETURN(ret, fail_on_error);
>>> }
>>> @@ -3177,6 +3198,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->ref->pipe != pipe_obj)
>>> + continue;
>>> +
>>> if (plane->changed)
>>> igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
>>> }
>>> @@ -3709,6 +3734,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 the plane on the pipe that last set fb */
>>> + igt_plane_set_pipe(plane, pipe);
>>> } else {
>>> igt_plane_set_size(plane, 0, 0);
>>>
>>> @@ -3743,6 +3771,23 @@ void igt_plane_set_fence_fd(igt_plane_t *plane, int fence_fd)
>>> igt_plane_set_prop_value(plane, IGT_PLANE_IN_FENCE_FD, fd);
>>> }
>>>
>>> +/**
>>> + * igt_plane_set_pipe:
>>> + * @plane: Target plane pointer
>>> + * @pipe: The pipe to assign the plane to
>>> + *
>>> + */
>>> +void igt_plane_set_pipe(igt_plane_t *plane, igt_pipe_t *pipe)
>>> +{
>>> + /*
>>> + * HACK: Point the global plane back to the local plane.
>>> + * This is used to help apply the correct atomic state while
>>> + * we're moving away from the single pipe per plane model.
>>> + */
>>> + plane->ref->ref = plane;
>>> + plane->ref->pipe = pipe;
>>> +}
>>> +
>>> /**
>>> * igt_plane_set_position:
>>> * @plane: Plane pointer for which position is to be set
>>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>>> index 5755b160..e1c5c967 100644
>>> --- a/lib/igt_kms.h
>>> +++ b/lib/igt_kms.h
>>> @@ -300,9 +300,10 @@ typedef enum {
>>> #define IGT_ROTATION_MASK \
>>> (IGT_ROTATION_0 | IGT_ROTATION_90 | IGT_ROTATION_180 | IGT_ROTATION_270)
>>>
>>> -typedef struct {
>>> +typedef struct igt_plane {
>>> /*< private >*/
>>> igt_pipe_t *pipe;
>>> + struct igt_plane *ref;
>>> int index;
>>> /* capabilities */
>>> int type;
>>> @@ -372,8 +373,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_t *planes;
>>> igt_pipe_t *pipes;
>>> bool has_cursor_plane;
>>> bool is_atomic;
>>> @@ -413,6 +416,7 @@ void igt_pipe_request_out_fence(igt_pipe_t *pipe);
>>>
>>> void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
>>> void igt_plane_set_fence_fd(igt_plane_t *plane, int fence_fd);
>>> +void igt_plane_set_pipe(igt_plane_t *plane, igt_pipe_t *pipe);
>>> void igt_plane_set_position(igt_plane_t *plane, int x, int y);
>>> void igt_plane_set_size(igt_plane_t *plane, int w, int h);
>>> void igt_plane_set_rotation(igt_plane_t *plane, igt_rotation_t rotation);
>>>
>
More information about the igt-dev
mailing list