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

Daniel Vetter daniel at ffwll.ch
Mon Apr 30 14:55:24 UTC 2018


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).

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


More information about the dri-devel mailing list