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
