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. 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. 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. > 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. >> >>>>> + * >>>>> + * 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, 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. 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. > > 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
