Re: [PATCH 1/1] cameraif: add ABI for para-virtual camera

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>]

On 09/10/2018 12:04 PM, Hans Verkuil wrote:
> On 09/10/2018 10:24 AM, Oleksandr Andrushchenko wrote:
>> On 09/10/2018 10:53 AM, Hans Verkuil wrote:
>>> Hi Oleksandr,
>>>
>>> On 09/10/2018 09:16 AM, Oleksandr Andrushchenko wrote:
> <snip>
>
>>>>> I suspect that you likely will want to support such sources eventually, so
>>>>> it pays to design this with that in mind.
>>>> Again, I think that this is the backend to hide these
>>>> use-cases from the frontend.
>>> I'm not sure you can: say you are playing a bluray connected to the system
>>> with HDMI, then if there is a resolution change, what do you do? You can tear
>>> everything down and build it up again, or you can just tell frontends that
>>> something changed and that they have to look at the new vcamera configuration.
>>>
>>> The latter seems to be more sensible to me. It is really not much that you
>>> need to do: all you really need is an event signalling that something changed.
>>> In V4L2 that's the V4L2_EVENT_SOURCE_CHANGE.
>> well, this complicates things a lot as I'll have to
>> re-allocate buffers - right?
> Right. Different resolutions means different sized buffers and usually lots of
> changes throughout the whole video pipeline, which in this case can even
> go into multiple VMs.
>
> One additional thing to keep in mind for the future: V4L2_EVENT_SOURCE_CHANGE
> has a flags field that tells userspace what changed. Right now that is just the
> resolution, but in the future you can expect flags for cases where just the
> colorspace information changes, but not the resolution.
>
> Which reminds me of two important missing pieces of information in your protocol:
>
> 1) You need to communicate the colorspace data:
>
> - colorspace
> - xfer_func
> - ycbcr_enc/hsv_enc (unlikely you ever want to support HSV pixelformats, so I
>    think you can ignore hsv_enc)
> - quantization
>
> See https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-v4l2.html#c.v4l2_pix_format
> and the links to the colorspace sections in the V4L2 spec for details).
>
> This information is part of the format, it is reported by the driver.
I'll take a look and think what can be put and how into the protocol,
do you think I'll have to implement all the above for
this stage?

>
> 2) If you support interlaced formats and V4L2_FIELD_ALTERNATE (i.e.
>     each buffer contains a single field), then you need to be able to tell
>     userspace whether the dequeued buffer contains a top or bottom field.
I think at the first stage we can assume that interlaced
formats are not supported and add such support later if need be.
>
> Also, what to do with dropped frames/fields: V4L2 has a sequence counter and
> timestamp that can help detecting that. You probably need something similar.
Ok, this can be reported as part of XENCAMERA_EVT_FRAME_AVAIL event
>
>> But anyways, I can add
>> #define XENCAMERA_EVT_CFG_CHANGE       0x01
>> in the protocol, so we can address this use-case
> <snip>
>
>>>> 1. set format command:
>>>>     * pixel_format - uint32_t, pixel format to be used, FOURCC code.
>>>>     * width - uint32_t, width in pixels.
>>>>     * height - uint32_t, height in pixels.
>>>>
>>>> 2. Set frame rate command:
>>>>     + * frame_rate_numer - uint32_t, numerator of the frame rate.
>>>>     + * frame_rate_denom - uint32_t, denominator of the frame rate.
>>>>
>>>> 3. Set/request num bufs:
>>>>     * num_bufs - uint8_t, desired number of buffers to be used.
>>> I like this much better. 1+2 could be combined, but 3 should definitely remain
>>> separate.
>> ok, then 1+2 combined + 3 separate.
>> Do you think we can still name 1+2 as "set_format" or "set_config"
>> will fit better?
> set_format is closer to S_FMT as used in V4L2, so I have a slight preference
> for that, but it is really up to you.
I'll probably stick to SET_CONFIG here
>
>>>>>> + *
>>>>>> + * See response format for this request.
>>>>>> + *
>>>>>> + * Notes:
>>>>>> + *  - frontend must check the corresponding response in order to see
>>>>>> + *    if the values reported back by the backend do match the desired ones
>>>>>> + *    and can be accepted.
>>>>>> + *  - frontend may send multiple XENCAMERA_OP_SET_CONFIG requests before
>>>>>> + *    sending XENCAMERA_OP_STREAM_START request to update or tune the
>>>>>> + *    configuration.
>>>>>> + */
>>>>>> +struct xencamera_config {
>>>>>> +    uint32_t pixel_format;
>>>>>> +    uint32_t width;
>>>>>> +    uint32_t height;
>>>>>> +    uint32_t frame_rate_nom;
>>>>>> +    uint32_t frame_rate_denom;
>>>>>> +    uint8_t num_bufs;
>>>>>> +};
>>>>>> +
>>>>>> +/*
>>>>>> + * Request buffer details - request camera buffer's memory layout.
>>>>>> + * detailed description:
>>>>>> + *         0                1                 2               3        octet
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + * |               id                |_GET_BUF_DETAILS|   reserved     | 4
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + * |                              reserved                             | 8
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + * |                              reserved                             | 64
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + *
>>>>>> + * See response format for this request.
>>>>>> + *
>>>>>> + *
>>>>>> + * Request camera buffer creation:
>>>>>> + *         0                1                 2               3        octet
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + * |               id                | _OP_BUF_CREATE |   reserved     | 4
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + * |                             reserved                              | 8
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + * |      index     |                     reserved                     | 12
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + * |                           gref_directory                          | 16
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + * |                             reserved                              | 20
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + * |                             reserved                              | 64
>>>>>> + * +----------------+----------------+----------------+----------------+
>>>>>> + *
>>>>>> + * An attempt to create multiple buffers with the same index is an error.
>>>>>> + * index can be re-used after destroying the corresponding camera buffer.
>>>>>> + *
>>>>>> + * index - uint8_t, index of the buffer to be created.
>>>>>> + * gref_directory - grant_ref_t, a reference to the first shared page
>>>>>> + *   describing shared buffer references. The size of the buffer is equal to
>>>>>> + *   XENCAMERA_OP_GET_BUF_DETAILS.size response. At least one page exists. If
>>>>>> + *   shared buffer size exceeds what can be addressed by this single page,
>>>>>> + *   then reference to the next shared page must be supplied (see
>>>>>> + *   gref_dir_next_page below).
>>>>> It might be better to allocate all buffers in one go, i.e. what VIDIOC_REQBUFS
>>>>> does.
>>>> Well, I still think it is better to have a per buffer interface
>>>> in the protocol as it is done for other Xen virtual devices.
>>>> So, I'll keep this as is for now: VIDIOC_REQBUFS can still do
>>>> what it does internally in the frontend driver
>>> I may have misunderstood the original API. The newly proposed XENCAMERA_OP_BUF_REQUEST
>>> maps to REQBUFS, right? And then BUF_CREATE/DESTROY just set up the shared buffer
>>> mappings for the buffers created by REQBUFS. If that's the sequence, then it makes
>>> sense. I'm not sure about the naming.
>>>
>>> You might want to make it clear that XENCAMERA_OP_BUF_REQUEST allocates the buffers
>>> on the backend, and so can fail. Also, the actual number of allocated buffers in
>>> case of success can be more or less than what was requested.
>> The buffers can be allocated and shared by either backend or frontend: see
>> "be-alloc" configuration option telling which domain (VM) shares
>> the Xen grant references to the pages of the buffer: either frontend
>> or backend.
> If you want to do zero-copy video capture,
this is the goal
>   then you need to know which
> device in your video pipeline (which now covers both actual hardware and
> multiple VMs) has the strictest memory layout requirements. Often the
> video HW requires contiguous physical memory for the buffers, which means
> you can't just give it a piece of non-contig memory allocated elsewhere.
We have already implemented zero copying use-cases for
virtual display, please see [1] and [2] which are dma-buf
based which can cope with real HW restrictions you mention.
And in that case we can implement zero-copying both ways,
e.g. when the Xen grant references are shared by either
backend or frontend. This is different from camera use-cases:
a single buffer needs to be shared with multiple frontends,
so zero-copying is only possible when backend allocates the references
and shares those with frontends. The way when frontend allocates
the buffers and still we can implement zero-copying is when
there is a single frontend in the system, otherwise we
need to copy the images from backend's buffers into frontend's
ones.
> In practice you have two possible memory models you can use with V4L2 drivers:
> MMAP (i.e. allocated by the driver and the buffers can, if needed, be exported
> as dmabuf handles with VIDIOC_EXPBUF), or DMABUF where buffers are allocated
> elsewhere and imported to V4L2, which may fail if it doesn't match the HW
> requirements.
For the frontend it is possible to work with both MMAP/DMABUF
and the rest is on the backend's side - this was proven by
virtual display implementation, so I see no problem here
for virtual camera.
>
>> So, I was more thinking that in case of V4L2 based frontend driver:
>> 1. Frontend serves REQBUFS ioctl and asks the backend with
>> XENCAMERA_OP_BUF_REQUEST
>> if it can handle that many buffers and gets number of buffers to be used
>> and buffer structure (number of planes, sizes, offsets etc.) as the reply
>> to that request
>> 2. Frontend creates n buffers with XENCAMERA_OP_BUF_CREATE
>> 3. Frontend returns from REQBUFS ioctl with actual number of buffers
>> allocated
> Regards,
>
> 	Hans
Thank you,
Oleksandr

[1] https://elixir.bootlin.com/linux/v4.19-rc3/source/drivers/gpu/drm/xen
[2] 
https://elixir.bootlin.com/linux/v4.19-rc3/source/drivers/xen/gntdev-dmabuf.c

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel