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

[Brian Starkey]

Hi Laurent,

On Thu, Feb 21, 2019 at 12:02:57PM +0200, Laurent Pinchart wrote:
> Hi Brian,
> 
> On Thu, Feb 21, 2019 at 09:50:19AM +0000, Brian Starkey wrote:
> > On Thu, Feb 21, 2019 at 10:23:17AM +0200, Laurent Pinchart wrote:
> > > 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.
> > 
> > Sorry you didn't have a good time :-(
> 
> No worries. That was to be expected with such young code :-)
> 
> > > There writeback support in the DRM core leaks jobs,
> > 
> > Is this the cleanup on check fail, or something else?
> 
> Yes, that's the problem. I have patches for it that I will post soon.
> 
> > One possible pitfall is that you must set the job in the connector
> > state to NULL after you call drm_writeback_queue_job(). The API there
> > could easily be changed to pass in the connector_state and clear it in
> > drm_writeback_queue_job() instead of relying on drivers to do it.
> 
> I also have a patch for that :-)
> 
> > > 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.
> > 
> > Hm, yes that didn't occur to me; we don't have a prepare_fb callback.
> > 
> > > 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 ?
> > 
> > Indeed, there's igts on the list which I wrote and tested:
> > 
> > https://patchwork.kernel.org/patch/10764975/
> 
> Will you get these merged ? Pushing everybody to use the writeback
> connector API without any test is mainline isn't nice, it almost makes
> me want to go back to V4L2.

I wasn't trying to be pushy - I only shared my opinion that I didn't
think it was a good idea to introduce a second display writeback API,
when we already have one. You're entirely entitled to ignore my
opinion.

The tests have been available since the very early versions of the
writeback series. I don't know what's blocking them from merging, I
haven't been tracking it very closely.

If you'd be happy to provide your review and test on them, that may
help the process along?

> 
> igt test cases are nice to have, but what I need now is a tool to
> execise the API manually, similar to modetest, with command line
> parameters to configure the device, and the ability to capture frames to
> disk using writeback. How did you perform such tests when you developed
> writeback support ?
> 

I used a pre-existing internal tool which does exactly that.

I appreciate that we don't have upstream tooling for writeback. As you
say, it's a young API (well, not by date, but certainly by usage).

I also do appreciate you taking the time to consider it, identifying
issues which we did not, and for fixing them. The only way it stops
being a young API, with bugs and no tooling, is if people adopt it.

Thanks,
-Brian

> > And there's support in drm_hwcomposer (though I must admit I haven't
> > personally run the drm_hwc code):
> > 
> > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3
> 
> That won't help me much as I don't have an android port for the R-Car
> boards.
> 
> > I'm afraid I haven't really touched any of the writeback code for a
> > couple of years - Liviu picked that up. He's on holiday until Monday,
> > but he should be able to help with the status of the igts.
> > 
> > Hope that helps,
> > 
> > >>> 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