Re: [igt-dev] igt trouble with planes shared between multiple CRTCs (Re: [PATCH v2 0/8] R-Car DU: Support CRC calculation)

Re: [igt-dev] igt trouble with planes shared between multiple CRTCs (Re: [PATCH v2 0/8] R-Car DU: Support CRC calculation)

[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.
-Daniel

> 
> Cheers, Daniel
> 
> > 
> > On Monday, 23 April 2018 01:34:22 EEST Laurent Pinchart wrote:
> > > Hello,
> > > 
> > > This patch series adds support for CRC calculation to the rcar-du-drm
> > > driver.
> > > 
> > > CRC calculation is supported starting at the Renesas R-Car Gen3 SoCs, as
> > > earlier versions don't have the necessary hardware. On Gen3 SoCs, the CRC is
> > > computed by the DISCOM module part of the VSP-D and VSP-DL.
> > > 
> > > The DISCOM is interfaced to the VSP through the UIF glue and appears as a
> > > VSP entity with a sink pad and a source pad.
> > > 
> > > The series starts with a switch to SPDX license headers in patch 1/8,
> > > prompted by a checkpatch.pl warning for a later patch that complained about
> > > missing SPDX license headers. It then continues with cleanup and
> > > refactoring. Patches 2/8 and 3/8 prepare for DISCOM and UIF support by
> > > extending generic code to make it usable for the UIF. Patch 4/8 documents a
> > > structure that will receive new fields.
> > > 
> > > Patch 5/8 then extends the API exposed by the VSP driver to the DU driver to
> > > support CRC computation configuration and reporting. The patch
> > > unfortunately needs to touch both the VSP and DU drivers, so the whole
> > > series will need to be merged through a single tree.
> > > 
> > > Patch 5/8 adds support for the DISCOM and UIF in the VSP driver, patch 7/8
> > > integrates it in the DRM pipeline, and patch 8/8 finally implements the CRC
> > > API in the DU driver to expose CRC computation to userspace.
> > > 
> > > The hardware supports computing the CRC at any arbitrary point in the
> > > pipeline on a configurable window of the frame. This patch series supports
> > > CRC computation on input planes or pipeline output, but on the full frame
> > > only. Support for CRC window configuration can be added later if needed but
> > > will require extending the userspace API, as the DRM/KMS CRC API doesn't
> > > support this feature.
> > > 
> > > Compared to v1, the CRC source names for plane inputs are now constructed
> > > from plane IDs instead of plane indices. This allows userspace to match CRC
> > > sources with planes.
> > > 
> > > Note that exposing the DISCOM and UIF though the V4L2 API isn't supported as
> > > the module is only found in VSP-D and VSP-DL instances that are not exposed
> > > through V4L2. It is possible to expose those instances through V4L2 with a
> > > small modification to the driver for testing purpose. If the need arises to
> > > test DISCOM and UIF with such an out-of-tree patch, support for CRC
> > > reporting through a V4L2 control can be added later without affecting how
> > > CRC is exposed through the DRM/KMS API.
> > > 
> > > The patches are based on top of the "[PATCH v2 00/15] R-Car VSP1:
> > > Dynamically assign blend units to display pipelines" patch series, itself
> > > based on top of the Linux media master branch and scheduled for merge in
> > > v4.18. The new base caused heavy conflicts, requiring this series to be
> > > merged through the V4L2 tree. Once the patches receive the necessary review
> > > I will ask Dave to ack the merge plan.
> > > 
> > > For convenience the patches are available at
> > > 
> > >         git://linuxtv.org/pinchartl/media.git vsp1-discom-v2-20180423
> > > 
> > > The code has been tested through the kms-test-crc.py script part of the DU
> > > test suite available at
> > > 
> > >         git://git.ideasonboard.com/renesas/kms-tests.git discom
> > > 
> > > Laurent Pinchart (8):
> > >   v4l: vsp1: Use SPDX license headers
> > >   v4l: vsp1: Share the CLU, LIF and LUT set_fmt pad operation code
> > >   v4l: vsp1: Reset the crop and compose rectangles in the set_fmt helper
> > >   v4l: vsp1: Document the vsp1_du_atomic_config structure
> > >   v4l: vsp1: Extend the DU API to support CRC computation
> > >   v4l: vsp1: Add support for the DISCOM entity
> > >   v4l: vsp1: Integrate DISCOM in display pipeline
> > >   drm: rcar-du: Add support for CRC computation
> > > 
> > >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c    | 156 ++++++++++++++++-
> > >  drivers/gpu/drm/rcar-du/rcar_du_crtc.h    |  19 +++
> > >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c     |  13 +-
> > >  drivers/media/platform/vsp1/Makefile      |   2 +-
> > >  drivers/media/platform/vsp1/vsp1.h        |  10 +-
> > >  drivers/media/platform/vsp1/vsp1_brx.c    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_brx.h    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_clu.c    |  71 ++------
> > >  drivers/media/platform/vsp1/vsp1_clu.h    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_dl.c     |   8 +-
> > >  drivers/media/platform/vsp1/vsp1_dl.h     |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_drm.c    | 127 ++++++++++++--
> > >  drivers/media/platform/vsp1/vsp1_drm.h    |  20 ++-
> > >  drivers/media/platform/vsp1/vsp1_drv.c    |  26 ++-
> > >  drivers/media/platform/vsp1/vsp1_entity.c | 103 +++++++++++-
> > >  drivers/media/platform/vsp1/vsp1_entity.h |  13 +-
> > >  drivers/media/platform/vsp1/vsp1_hgo.c    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_hgo.h    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_hgt.c    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_hgt.h    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_histo.c  |  65 +------
> > >  drivers/media/platform/vsp1/vsp1_histo.h  |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_hsit.c   |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_hsit.h   |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_lif.c    |  71 ++------
> > >  drivers/media/platform/vsp1/vsp1_lif.h    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_lut.c    |  71 ++------
> > >  drivers/media/platform/vsp1/vsp1_lut.h    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_pipe.c   |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_pipe.h   |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_regs.h   |  46 ++++-
> > >  drivers/media/platform/vsp1/vsp1_rpf.c    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_rwpf.c   |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_rwpf.h   |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_sru.c    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_sru.h    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_uds.c    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_uds.h    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_uif.c    | 271 +++++++++++++++++++++++++++
> > >  drivers/media/platform/vsp1/vsp1_uif.h    |  32 ++++
> > >  drivers/media/platform/vsp1/vsp1_video.c  |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_video.h  |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_wpf.c    |   6 +-
> > >  include/media/vsp1.h                      |  39 ++++-
> > >  44 files changed, 896 insertions(+), 417 deletions(-)
> > >  create mode 100644 drivers/media/platform/vsp1/vsp1_uif.c
> > >  create mode 100644 drivers/media/platform/vsp1/vsp1_uif.h
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
> > 
> > 
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] igt trouble with planes shared between multiple CRTCs (Re: [PATCH v2 0/8] R-Car DU: Support CRC calculation)

[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

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] igt trouble with planes shared between multiple CRTCs (Re: [PATCH v2 0/8] R-Car DU: Support CRC calculation)

[Maarten Lankhorst]

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

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: igt trouble with planes shared between multiple CRTCs (Re: [PATCH v2 0/8] R-Car DU: Support CRC calculation)

[Daniel Vetter]

On Tue, May 01, 2018 at 10:58:02AM +0200, Maarten Lankhorst wrote:
> 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.

Solutions for that problem is what I proposed in my mail, since atm the
kernel is indeed not exposing that in any fasion.

> This will hopefully allow legacy tests to keep working, while atomic plane aware
> tests will be able to use all planes.

Yeah ..
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [igt-dev] igt trouble with planes shared between multiple CRTCs (Re: [PATCH v2 0/8] R-Car DU: Support CRC calculation)

[Maarten Lankhorst]

Op 02-05-18 om 09:32 schreef Daniel Vetter:
> On Tue, May 01, 2018 at 10:58:02AM +0200, Maarten Lankhorst wrote:
>> 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.
> Solutions for that problem is what I proposed in my mail, since atm the
> kernel is indeed not exposing that in any fasion.
>
>> This will hopefully allow legacy tests to keep working, while atomic plane aware
>> tests will be able to use all planes.
> Yeah ..
> -Daniel

For reference: https://patchwork.freedesktop.org/series/42814/

The part where planes are reassigned isn't done yet, but that will depend on the final kernel api.

~Maarten

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev