Re: [PATCH 02/11] media: videobuf2: Add new uAPI for DVB streaming I/O

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Mon, 8 Jan 2018 15:26:50 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> A quick follow-up:
> 
> On 01/08/2018 02:54 PM, Hans Verkuil wrote:
> >> +/*
> >> + * Videobuf operations
> >> + */
> >> +int dvb_vb2_init(struct dvb_vb2_ctx *ctx, const char *name, int nonblocking)
> >> +{
> >> +	struct vb2_queue *q = &ctx->vb_q;
> >> +	int ret;
> >> +
> >> +	memset(ctx, 0, sizeof(struct dvb_vb2_ctx));
> >> +	q->type = DVB_BUF_TYPE_CAPTURE;  
> > 
> > We don't support DVB_BUF_TYPE_OUTPUT? Shouldn't this information come from the
> > driver?
> >   
> >> +	/**capture type*/  
> > 
> > Why /** ?
> >   
> >> +	q->is_output = 0;  
> > 
> > Can be dropped unless we support DVB_BUF_TYPE_OUTPUT.
> >   
> >> +	/**only mmap is supported currently*/  
> > 
> > /** ?
> >   
> >> +	q->io_modes = VB2_MMAP;
> >> +	q->drv_priv = ctx;
> >> +	q->buf_struct_size = sizeof(struct dvb_buffer);
> >> +	q->min_buffers_needed = 1;
> >> +	q->ops = &dvb_vb2_qops;
> >> +	q->mem_ops = &vb2_vmalloc_memops;
> >> +	q->buf_ops = &dvb_vb2_buf_ops;
> >> +	q->num_buffers = 0;  
> > 
> > Not needed, is zeroed in the memset above.
> > 
> > I'm also missing q->timestamp_flags: should be set to V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC.  
> 
> Ignore this, see my comments later on.
> 
> >   
> >> +	ret = vb2_core_queue_init(q);
> >> +	if (ret) {
> >> +		ctx->state = DVB_VB2_STATE_NONE;
> >> +		dprintk(1, "[%s] errno=%d\n", ctx->name, ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	mutex_init(&ctx->mutex);
> >> +	spin_lock_init(&ctx->slock);
> >> +	INIT_LIST_HEAD(&ctx->dvb_q);
> >> +
> >> +	strncpy(ctx->name, name, DVB_VB2_NAME_MAX);  
> > 
> > I believe strlcpy is recommended.
> >   
> >> +	ctx->nonblocking = nonblocking;
> >> +	ctx->state = DVB_VB2_STATE_INIT;
> >> +
> >> +	dprintk(3, "[%s]\n", ctx->name);
> >> +
> >> +	return 0;
> >> +}  
> 
> <snip>
> 
> >> diff --git a/include/uapi/linux/dvb/dmx.h b/include/uapi/linux/dvb/dmx.h
> >> index c10f1324b4ca..e212aa18ad78 100644
> >> --- a/include/uapi/linux/dvb/dmx.h
> >> +++ b/include/uapi/linux/dvb/dmx.h
> >> @@ -211,6 +211,64 @@ struct dmx_stc {
> >>  	__u64 stc;
> >>  };
> >>  
> >> +/**
> >> + * struct dmx_buffer - dmx buffer info
> >> + *
> >> + * @index:	id number of the buffer
> >> + * @bytesused:	number of bytes occupied by data in the buffer (payload);
> >> + * @offset:	for buffers with memory == DMX_MEMORY_MMAP;
> >> + *		offset from the start of the device memory for this plane,
> >> + *		(or a "cookie" that should be passed to mmap() as offset)  
> > 
> > Since we only support MMAP this is always a 'cookie' in practice. So I think this
> > should just be:
> > 
> > 	A "cookie" that should be passed to mmap() as offset.
> >   
> >> + * @length:	size in bytes of the buffer
> >> + *
> >> + * Contains data exchanged by application and driver using one of the streaming
> >> + * I/O methods.
> >> + */
> >> +struct dmx_buffer {
> >> +	__u32			index;
> >> +	__u32			bytesused;
> >> +	__u32			offset;  
> > 
> > I suggest making this a __u64: that way we can handle pointers as well in
> > the future if we need them (as we do for the USERPTR case for V4L2).
> > 
> > Should this also be wrapped in a union? Useful when adding dmabuf support in the
> > future.
> > 
> > Do you think there is any use-case for multiplanar formats in the future?
> > With perhaps meta data in a separate plane? Having to add support for this later
> > has proven to be very painful, so we need to be as certain as possible that
> > this isn't going to happen. Otherwise it's better to prepare for this right now.
> >   
> >> +	__u32			length;
> >> +	__u32			reserved[4];  
> > 
> > I do believe you need a memory field as well. It's only MMAP today, but in
> > the future DMABUF will most likely be supported as well and you need to be
> > able to tell what memory mode is being used.
> >   
> >> +};  
> 
> There is no 'flags' field here. But without that you cannot check the buffer
> states, esp. the ERROR state. Or can that never happen?

On DVB, there is a protocol stack there that allows checking errors,
either MPEG-TS (current standards) or IP/MMT - for the new, yet to be
implemented ATSC version 3 standard.

Even if we add an error state, userspace will just ignore, as it will
need to verify the packet CRC checks.

> Would a timestamp field be useful, if only for debugging?

I don't see how a timestamp will help here for debugging. Probably,
adding event trace points would work better, but we have those
already at vb2-core.

Anyway, as this is kAPI only, we can add it later as needed.

Thanks,
Mauro