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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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?

It can. The V4L2 capture device is independently controlled, and doesn't
affect the DRM/KMS device. The only dependency is that the V4L2 device
will only provide frames on atomic commit, a blocking VIDIOC_DQBUF or a
select()/poll() call will block until the atomic commit.

> 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?

In theory yes, in practice possibly, but with a higher complexity. I
didn't go for V4L2 to avoid writeback connectors, but because the
infrastructure was there already in the VSP driver.

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

I get your point. There's no much use arguing, as I believe you're right
:-) I'll give this another shot using writeback connectors.

> > 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel