DRM device memory writeback (Mali-DP)

[Brian Starkey]

Hi,

On Fri, Jul 15, 2016 at 10:42:01AM -0700, Eric Anholt wrote:
>Ville Syrjälä <ville.syrjala at linux.intel.com> writes:
>
>> On Fri, Jul 15, 2016 at 10:09:19AM +0100, Brian Starkey wrote:
>>> Hi Daniel,
>>>
>>> Thanks for taking a look.
>>>
>>> (+Cc Laurent)
>>>
>>> On Fri, Jul 15, 2016 at 09:33:34AM +0200, Daniel Vetter wrote:
>>> >On Thu, Jul 14, 2016 at 06:03:40PM +0100, Brian Starkey wrote:
>>> >> Hi,
>>> >>
>>> >> The Mali-DP display processors have a memory-writeback engine which
>>> >> can write the result of the composition (CRTC output) to a memory
>>> >> buffer in a variety of formats.
>>> >>
>>> >> We're looking for feedback/suggestions on how to expose this in the
>>> >> mali-dp DRM kernel driver - possibly via V4L2.
>>> >>
>>> >> We've got a few use cases where writeback is useful:
>>> >>    - testing, to check the displayed image
>>> >
>>> >This might or might not need a separate interface. There are efforts to
>>> >make the intel kms validation tests in i-g-t generic (well under way
>>> >already), and part of that is creating a generic infrastructure to capture
>>> >display CRCs for functional tests (still in progress).
>>> >
>>> >But it might be better if userspace abstracts between full readback and
>>> >display CRC, assuming we can make full writeback cross-vendor enough for
>>> >that use-case.
>>> >
>>>
>>> I'd lean towards the userspace abstraction.
>>> Encumbering a simple CRC interface with all the complexity of
>>> full-writeback (size, scaling, pixel format, multi-planar etc.) sounds
>>> a bit unnecessary.
>>>
>>> Of course, if v4l2 isn't going to be the cross-vendor full-writeback
>>> implementation, then we need to be aiming to use whatever _is_ in
>>> the mali-dp driver.
>>>
>>> >>    - screen recording
>>> >>    - wireless display (e.g. Miracast)
>>> >>    - dual-display clone mode
>>> >>    - memory-to-memory composition
>>> >> Note that the HW is capable of writing one of the input planes instead
>>> >> of the CRTC output, but we've no good use-case for wanting to expose
>>> >> that.
>>> >>
>>> >> In our Android ADF driver[1] we exposed the memory write engine as
>>> >> part of the ADF device using ADF's "MEMORY" interface type. DRM/KMS
>>> >> doesn't have any similar support for memory output from CRTCs, but we
>>> >> want to expose the functionality in the mainline Mali-DP DRM driver.
>>> >>
>>> >> A previous discussion on the topic went towards exposing the
>>> >> memory-write engine via V4L2[2].
>>> >>
>>> >> I'm thinking to userspace this would look like two distinct devices:
>>> >>    - A DRM KMS display controller.
>>> >>    - A V4L2 video source.
>>> >> They'd both exist in the same kernel driver.
>>> >> A V4L2 client can queue up (CAPTURE) buffers in the normal way, and
>>> >> the DRM driver would see if there's a buffer to dequeue every time a
>>> >> new modeset is received via the DRM API - if so, it can configure the
>>> >> HW to dump into it (one-shot operation).
>>> >>
>>> >> An implication of this is that if the screen is actively displaying a
>>> >> static scene and the V4L2 client queues up a buffer, it won't get
>>> >> filled until the DRM scene changes. This seems best, otherwise the
>>> >> V4L2 driver has to change the HW configuration out-of-band from the
>>> >> DRM device which sounds horribly racy.
>>> >>
>>> >> One further complication is scaling. Our HW has a scaler which can
>>> >> tasked with either scaling an input plane or the buffer being written
>>> >> to memory, but not both at the same time. This means we need to
>>> >> arbitrate the scaler between the DRM device (scaling input planes) and
>>> >> the V4L2 device (scaling output buffers).
>>> >>
>>> >> I think the simplest approach here is to allow V4L2 to "claim" the
>>> >> scaler by setting the image size (VIDIOC_S_FMT) to something other
>>> >> than the CRTC's current resolution. After that, any attempt to use the
>>> >> scaler on an input plane via DRM should fail atomic_check().
>>> >
>>> >That's perfectly fine atomic_check behaviour. Only trouble is that the v4l
>>> >locking must integrate into the drm locking, but that should be doable.
>>> >Worst case you must shadow all v4l locks with a wait/wound
>>> >drm_modeset_lock to avoid deadlocks (since you could try to grab locks
>>> >from either end).
>>> >
>>>
>>> Yes, I haven't looked at the details of the locking but I'm hoping
>>> it's manageable.
>>>
>>> >> If the V4L2 client goes away or sets the image size to the CRTC's
>>> >> native resolution, then the DRM device is allowed to use the scaler.
>>> >> I don't know if/how the DRM device should communicate to userspace
>>> >> that the scaler is or isn't available for use.
>>> >>
>>> >> Any thoughts on this approach?
>>> >> Is it acceptable to both V4L2 and DRM folks?
>>> >
>>> >For streaming a V4L2 capture device seems like the right interface. But if
>>> >you want to use writeback in your compositor you must know which atomic
>>> >kms update results in which frame, since if you don't you can't use that
>>> >composited buffer for the next frame reliable.
>>> >
>>> >For that case I think a drm-only solution would be better, to make sure
>>> >you can do an atomic update and writeback in one step. v4l seems to grow
>>> >an atomic api of its own, but I don't think we'll have one spanning
>>> >subsystems anytime soon.
>>> >
>>>
>>> I've been thinking about this from the point of view of a HWComposer
>>> implementation and I think the hybrid DRM-V4L2 device would work OK.
>>> However it depends on the behaviour I mentioned above:
>>>
>>> >> if the screen is actively displaying a
>>> >> static scene and the V4L2 client queues up a buffer, it won't get
>>> >> filled until the DRM scene changes.
>>>
>>> V4L2 buffer queues are FIFO, so as long as the compositor queues only
>>> one V4L2 buffer per atomic update, there's no ambiguity.
>>> In the most simplistic case the compositor would alternate between:
>>>   - Queue V4L2 buffer
>>>   - DRM atomic update
>>> ... and dequeue either in the same thread or a different one. As long
>>> as the compositor keeps track of how many buffers it has queued and
>>> how many atomic updates it's made, it doesn't really matter.
>>>
>>> We'd probably be looking to add in V4L2 asynchronous dequeue using
>>> fences for synchronisation, but that's a separate issue.
>>>
>>> >For the kms-only interface the idea was to add a property on the crtc
>>> >where you can attach a writeback drm_framebuffer. Extending that idea to
>>> >the drm->v4l case we could create special drm_framebuffer objects
>>> >representing a v4l sink, and attach them to the same property. That would
>>> >also solve the problem of getting some agreement on buffer metadata
>>> >between v4l and drm side.
>>> >
>>>
>>> I think a drm_framebuffer on its own wouldn't be enough to handle our
>>> scaling case - at that point it starts to look more like a plane.
>>> However, if "special" CRTC sinks became a thing it could allow us to
>>> chain our writeback output to another CRTC's input (via memory)
>>> without a trip through userspace, which would be nice.
>>
>> My thinking has been that we could represent the writeback sink
>> as a connector. Combine that with my other idea of exposing a
>> fixed mode property for each connector (to make output scaling
>> user configurable), and we should end up with a way to control
>> the scaling of the writeback path.
>>
>> Also it would mean there's always a connector attached to the
>> crtc, even for the pure writeback only case, which I think should
>> result in less problems in general as I'm sure there are a lot of
>> assumptions in the code that there must be at least one connector
>> for an active crtc.
>
>This is what I've been thinking for vc4's writeback as well.  I
>definitely want my writeback to be using the same drivers/gpu/drm/vc4
>code for setting up the hardware.  And, since I expect writeback to be
>mostly used by compositors for the case where a frame exceeds bandwidth
>constraints, I want userspace to be able to use the same DRM APIs for
>setting up the frame to be written back as it was trying to use for
>setting up the frame before.

OK, so let's talk about using connectors instead then :-)

We can attach a generic fixed_mode property which can represent panel
fitters or Mali-DP's writeback scaling, that sounds good.

The DRM driver can create a new "virtual" encoder/connector pair, I
guess with a new connector type specifically for memory-writeback?
On this connector we allow the fb_id property to attach a framebuffer
for writing into.
We'd probably want an additional property to enable writeback, perhaps
an enum: "disabled", "streaming", "oneshot".

I think it makes sense to put the output buffer format, size etc.
validation in the virtual encoder's atomic_check. It has access to
both CRTC and connector state, and the encoder is supposed to
represent the format/stream conversion element anyway.

What I'm not so clear on is how to manage the API with userspace.

Changing connectors is a "full modeset required" operation. I expect
this means an application needs to know up-front if it will ever make
use of memory-writeback, and if so do its initial modeset on both the
writeback and the normal connector, even if it doesn't attach a
writeback framebuffer until much later. Otherwise, enabling writeback
will force a full disable-modeset-enable cycle.

The writeback connector would have to be hidden from legacy clients
which would expect it to work like a normal connector - I suppose
that's somewhat similar to universal planes.

For the case that Ville mentioned where only the writeback connector
is connected, would userspace be expected to "make up" a
drm_mode_modeinfo to set? I can't think of any meaningful list of
available modes for the writeback connector to report.

Thanks for all the input so far!

-Brian