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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Feb 21, 2019 at 12:19:13PM +0000, Brian Starkey wrote:
> 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.

Mentioned on irc already to Laurent, but here for completeness: igt has
pretty decent support for combining that into one utility. We support
piles of standard stuff like --interactive-debug already, plus you can add
whatever additional things you need and feel like. There's quite a few
such igt tests/tools combinations already.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel