Op 01-05-18 om 10:58 schreef Maarten Lankhorst:
> Hey,
>
> Op 30-04-18 om 16:56 schreef Daniel Vetter:
>> On Mon, Apr 30, 2018 at 04:55:24PM +0200, Daniel Vetter wrote:
>>> On Sat, Apr 28, 2018 at 12:07:04AM +0300, Laurent Pinchart wrote:
>>>> Hi Daniel,
>>>>
>>>> (Removing the linux-media mailing list from CC as it is out of scope)
>>>>
>>>> You enquired on IRC whether this patch series passes the igt CRC tests.
>>>>
>>>> # ./kms_pipe_crc_basic --run-subtest read-crc-pipe-A
>>>> IGT-Version: 1.22-gf447f5fc531d (aarch64) (Linux: 4.17.0-rc1-00085-g56e849d93cc9 aarch64)
>>>> read-crc-pipe-A: Testing connector LVDS-1 using pipe A
>>>> (kms_pipe_crc_basic:1638) igt-debugfs-CRITICAL: Test assertion failure function igt_pipe_crc_start, file igt_debugfs.c:764:
>>>> (kms_pipe_crc_basic:1638) igt-debugfs-CRITICAL: Failed assertion: pipe_crc->crc_fd != -1
>>>> (kms_pipe_crc_basic:1638) igt-debugfs-CRITICAL: Last errno: 5, Input/output error
>>>> Stack trace:
>>>> Subtest read-crc-pipe-A failed.
>>>> **** DEBUG ****
>>>> (kms_pipe_crc_basic:1638) DEBUG: Test requirement passed: !(pipe >= data->display.n_pipes)
>>>> (kms_pipe_crc_basic:1638) INFO: read-crc-pipe-A: Testing connector LVDS-1 using pipe A
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: LVDS-1: set_pipe(A)
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: LVDS-1: Selecting pipe A
>>>> (kms_pipe_crc_basic:1638) DEBUG: Clearing the fb with color (0.00,1.00,0.00)
>>>> (kms_pipe_crc_basic:1638) igt-fb-DEBUG: igt_create_fb_with_bo_size(width=1024, height=768, format=0x34325258, tiling=0x0, size=0)
>>>> (kms_pipe_crc_basic:1638) igt-fb-DEBUG: igt_create_fb_with_bo_size(handle=1, pitch=4096)
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: Test requirement passed: plane_idx >= 0 && plane_idx < pipe->n_planes
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: A.0: plane_set_fb(140)
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: A.0: plane_set_size (1024x768)
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: A.0: fb_set_position(0,0)
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: A.0: fb_set_size(1024x768)
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: commit {
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: LVDS-1: SetCrtc pipe A, fb 140, src (0, 0), mode 1024x768
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetCrtc pipe A, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe A, plane 2, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe A, plane 3, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe A, plane 4, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetCrtc pipe B, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe B, plane 1, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe B, plane 2, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe B, plane 3, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe B, plane 4, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetCrtc pipe C, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe C, plane 1, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe C, plane 2, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe C, plane 3, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe C, plane 4, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetCrtc pipe D, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetCrtc pipe D, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe D, plane 2, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe D, plane 3, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: SetPlane pipe D, plane 4, disabling
>>>> (kms_pipe_crc_basic:1638) igt-kms-DEBUG: display: }
>>>> (kms_pipe_crc_basic:1638) igt-debugfs-DEBUG: Opening debugfs directory '/sys/kernel/debug/dri/0'
>>>> (kms_pipe_crc_basic:1638) igt-debugfs-DEBUG: Opening debugfs directory '/sys/kernel/debug/dri/0'
>>>> (kms_pipe_crc_basic:1638) igt-debugfs-CRITICAL: Test assertion failure function igt_pipe_crc_start, file igt_debugfs.c:764:
>>>> (kms_pipe_crc_basic:1638) igt-debugfs-CRITICAL: Failed assertion: pipe_crc->crc_fd != -1
>>>> (kms_pipe_crc_basic:1638) igt-debugfs-CRITICAL: Last errno: 5, Input/output error
>>>> (kms_pipe_crc_basic:1638) igt-core-INFO: Stack trace:
>>>> **** END ****
>>>> Subtest read-crc-pipe-A: FAIL (0.061s)
>>>>
>>>> I think the answer is no, but I don't think it's the fault of this patch
>>>> series. Opening the CRC data file returns -EIO because the CRTC is not active,
>>>> and I'm trying to find out why that is the case. The debug log shows a commit
>>>> that seems strange to me, enabling pipe A and immediately disabling right
>>>> afterwards. After some investigation I believe that this is caused by sharing
>>>> primary planes between CRTCs.
>>>>
>>>> The R-Car DU groups CRTCs by two and shares 5 planes between the two CRTCs of
>>>> the group. This specific SoC has two groups of two CRTCs, but that's not
>>>> relevant here, so we can ignore pipes C and D.
>>>>
>>>> Pipes A and B thus shared 5 planes that I will number 0 to 4 for simplicity.
>>>> The driver sets plane 0 as the primary plane for CRTC A and plane 1 as the
>>>> primary plane for CRTC B. Planes 2, 3 and 4 are created as overlay planes.
>>>>
>>>> When igt iterates over all planes for pipe A, it will first encounter plane 0
>>>> that has a framebuffer, and thus enables the pipe. It then iterates over
>>>> plane 1, recognizes it as a primary plane without a framebuffer, and thus
>>>> disables the pipe. Planes 2, 3 and 4 are recognized as overlay planes and thus
>>>> don't affect the pipe active state. Pipe B is handled the same way, and igt
>>>> disables it twice as planes 0 and 1 are primary.
>>>>
>>>> I don't know if the fault here is with igt that doesn't properly support this
>>>> architecture, or with the driver that shouldn't have two primary planes
>>>> available for a CRTC. In the former case, I'm not sure how to fix it, as I'm
>>>> not familiar enough with igt to rearchitecture the commit helpers. In the
>>>> latter case, how would you recommend fixing it on the driver side ?
>>> I guess thus far no one did run igt on a chip which did have reassignable
>>> primary planes. The problem here is that it's pretty hard to figure out
>>> which one is the real primary plane, since with possible CRTCs you could
>>> have multiple primary planes on 1 CRTC. There's no property or anything
>>> that explicitly tells you this. Two fixes:
>>>
>>> 1. Change drivers to limit primary planes to 1 crtc. Same for cursor
>>> overlays. There's probably other userspace than igt that gets confused
>>> by this, but this has the ugly downside that we artifically limit plane
>>> usage - if only 1 CRTC is on, we want to use all the available planes
>>> on that one.
>>>
>>> 2. Add some implicit or explicit uapi to allow userspace to figure out
>>> which primary plane is the primary plane for this crtc.
>>>
>>> a) Explicit option: We add a PRIMARY_ID property (and CURSOR_ID prop
>>> while at it) on the CRTC which points at the primary/cursor plane.
>>>
>>> b) Implicit option: We require primary planes are assigned to CRTC in the
>>> same order as they're created. So first primary plane you encouter
>>> is the one for the first CRTC, 2nd primary plane is the one for the
>>> 2nd CRTC and so on. If we go with this we probably should add a bit
>>> of code to the kernel to check that invariant (by making sure the
>>> primary plane has the corresponding CRTC included in its
>>> possible_crtc mask).
>>>
>>> Either way igt needs to be patched to not treat any primary plane that
>>> could work on a CRTC as the primary plane for that CRTC.
>>>
>>> Personally I'm leaning towards 2b).
>> Adding Maarten and igt-dev.
> We should first make the plane array global, instead of per crtc.
>
> That means removing plane->pipe and plane->index.
>
> And pipe_obj->planes and pipe_obj->n_planes need to be gone too. I think it's
> easiest to make
>
> There is one problem. Most of the tests are not aware of the limitations.
> If we make the plane array global it should be possible to make
> a for_each_possible_plane_on_pipe enumerate all planes that could be assigned
> to a certain pipe. We need to sort by z order somehow, but if we then add
> igt_plane_set_pipe() which updates the IGT_PLANE_CRTC_ID property, we should
> be good.
>
> Only thing we don't see is how the default crtc->primary and crtc->cursor are
> assigned. For the calls with COMMIT_LEGACY we need to know the mappings, but
> I don't think the kernel exposes them?
> I guess we could put it in a FIFO way, first enumerated plane with PRIMARY type
> is bound to the first crtc, same for cursor.
>
> This will hopefully allow legacy tests to keep working, while atomic plane aware
> tests will be able to use all planes.
>
> ~Maarten
>
https://cgit.freedesktop.org/~mlankhorst/intel-gpu-tools is what I was thinking
about, didn't test if it works, only compiles.
What's missing is that we should only create 1 igt_plane_t for each drm plane
instead of one for each pipe. Not sure how to get that working without
breaking a lot of assumptions in the tests, mainly the dual output tests. I think
that we should prevent for_each_plane_on_pipe until an output is set on the pipe,
and then automatically assign all planes to it and set plane->type to something matching.
When a second output is bound, steal unused sprite planes as required and divide them
equally. :)
~Maarten
