Re: [PATCH v4 0/7] VSP1: Display writeback support

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Brian Starkey <Brian.Starkey@arm.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
	nd <nd@arm.com>, Kieran Bingham <kieran.bingham@ideasonboard.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>
Subject: Re: [PATCH v4 0/7] VSP1: Display writeback support
Date: Thu, 21 Feb 2019 10:23:17 +0200	[thread overview]
Message-ID: <20190221082317.GB3451@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20190218122257.o5blxfs7jo5xu7ge@DESKTOP-E1NTVVP.localdomain>

Hi Brian,

On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote:
> On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote:
> > Hello,
> > 
> > This patch series implements display writeback support for the R-Car
> > Gen3 platforms in the VSP1 driver.
> > 
> > DRM/KMS provides a writeback API through a special type of writeback
> > connectors. This series takes a different approach by exposing writeback
> > as a V4L2 device. While there is nothing fundamentally wrong with
> > writeback connectors, display for R-Car Gen3 platforms relies on the
> > VSP1 driver behind the scene, which already implements V4L2 support.
> > Enabling writeback through V4L2 is thus significantly easier in this
> > case.
> 
> How does this look to an application? (I'm entirely ignorant about
> R-Car). They are interacting with the DRM device, and then need to
> open and configure a v4l2 device to get the writeback? Can the process
> which isn't controlling the DRM device independently capture the
> screen output?
> 
> I didn't see any major complication to implementing this as a
> writeback connector. If you want/need to use the vb2_queue, couldn't
> you just do that entirely from within the kernel?
> 
> Honestly (predictably?), to me it seems like a bad idea to introduce a
> second, non-compatible interface for display writeback. Any
> application interested in display writeback (e.g. compositors) will
> need to implement both in order to support all HW. drm_hwcomposer
> already supports writeback via DRM writeback connectors.
> 
> While I can see the advantages of having writeback exposed via v4l2
> for streaming use-cases, I think it would be better to have it done in
> such a way that it works for all writeback connectors, rather than
> being VSP1-specific. That would allow any application to choose
> whichever method is most appropriate for their use-case, without
> limiting themselves to a subset of hardware.

So I gave writeback connectors a go, and it wasn't very pretty. There
writeback support in the DRM core leaks jobs, and is missing support for
the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job
driver-specific data. I'm working on these issues and will submit
patches.

In the meantime, I need to test my implementation, so I need a command
line application that will let me exercise the API. I assume you've
tested your code, haven't you ? :-) Could you tell me how I can test
writeback ?

> > The writeback pixel format is restricted to RGB, due to the VSP1
> > outputting RGB to the display and lacking a separate colour space
> > conversion unit for writeback. The resolution can be freely picked by
> > will result in cropping or composing, not scaling.
> > 
> > Writeback requests are queued to the hardware on page flip (atomic
> > flush), and complete at the next vblank. This means that a queued
> > writeback buffer will not be processed until the next page flip, but
> > once it starts being written to by the VSP, it will complete at the next
> > vblank regardless of whether another page flip occurs at that time.
> 
> This sounds the same as mali-dp, and so fits directly with the
> semantics of writeback connectors.
> 
> > The code is based on a merge of the media master branch, the drm-next
> > branch and the R-Car DT next branch. For convenience patches can be
> > found at
> > 
> > 	git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback
> > 
> > Kieran Bingham (2):
> >   Revert "[media] v4l: vsp1: Supply frames to the DU continuously"
> >   media: vsp1: Provide a writeback video device
> > 
> > Laurent Pinchart (5):
> >   media: vsp1: wpf: Fix partition configuration for display pipelines
> >   media: vsp1: Replace leftover occurrence of fragment with body
> >   media: vsp1: Fix addresses of display-related registers for VSP-DL
> >   media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse
> >   media: vsp1: Replace the display list internal flag with a flags field
> > 
> >  drivers/media/platform/vsp1/vsp1_dl.c    | 118 ++++++++++++--
> >  drivers/media/platform/vsp1/vsp1_dl.h    |   6 +-
> >  drivers/media/platform/vsp1/vsp1_drm.c   |  24 ++-
> >  drivers/media/platform/vsp1/vsp1_drv.c   |  17 +-
> >  drivers/media/platform/vsp1/vsp1_pipe.c  |   5 +
> >  drivers/media/platform/vsp1/vsp1_pipe.h  |   6 +
> >  drivers/media/platform/vsp1/vsp1_regs.h  |   6 +-
> >  drivers/media/platform/vsp1/vsp1_rwpf.h  |   2 +
> >  drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++----
> >  drivers/media/platform/vsp1/vsp1_video.h |   6 +
> >  drivers/media/platform/vsp1/vsp1_wpf.c   |  65 ++++++--
> >  11 files changed, 378 insertions(+), 75 deletions(-)

-- 
Regards,

Laurent Pinchart