Hi Brian, On Thu, Feb 21, 2019 at 12:19:13PM +0000, Brian Starkey wrote: > On Thu, Feb 21, 2019 at 12:02:57PM +0200, Laurent Pinchart wrote: > > 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. I agree we should aim for a single API. My preference would have been for V4L2 universally, but now that we have writeback connectors upstream, the choice has been made, so we should stick to it. > 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? Now that I've sent an entirely untested patch series out, this is next on my list :-) > > 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. Any hope of sharing the sources ? > 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. If the developers who initially pushed the API upstream without an open-source test tool could spend a bit of time on this issue, I'm sure it would help too :-) > >> 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
