Re: [RFC PATCH v2 2/7] media: v4l2: Extend pixel formats to unify single/multi-planar handling (and more)

[Boris Brezillon <boris.brezillon@collabora.com>]

On Thu, 11 Apr 2019 10:24:16 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:


> >  static void v4l_print_framebuffer(const void *arg, bool write_only)
> >  {
> >  	const struct v4l2_framebuffer *p = arg;
> > @@ -951,11 +1027,15 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
> >  	switch (type) {
> >  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >  		if ((is_vid || is_tch) && is_rx &&
> > -		    (ops->vidioc_g_fmt_vid_cap || ops->vidioc_g_fmt_vid_cap_mplane))
> > +		    (ops->vidioc_g_fmt_vid_cap ||
> > +		     ops->vidioc_g_ext_fmt_vid_cap ||
> > +		     ops->vidioc_g_fmt_vid_cap_mplane))
> >  			return 0;
> >  		break;
> >  	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > -		if (is_vid && is_rx && ops->vidioc_g_fmt_vid_cap_mplane)
> > +		if (is_vid && is_rx &&
> > +		    (ops->vidioc_g_fmt_vid_cap_mplane ||
> > +		     ops->vidioc_g_ext_fmt_vid_cap))  
> 
> Is this right? I thought that V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE was to be refused
> when used with the new ioctls? Perhaps check_fmt needs an extra argument that
> tells it whether it is called from the new ioctls or not.

Or maybe we should do the format conversion (or just a type conversion)
before calling check_fmt().

> 
> >  			return 0;
> >  		break;
> >  	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> > @@ -964,11 +1044,15 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
> >  		break;
> >  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> >  		if (is_vid && is_tx &&
> > -		    (ops->vidioc_g_fmt_vid_out || ops->vidioc_g_fmt_vid_out_mplane))
> > +		    (ops->vidioc_g_fmt_vid_out ||
> > +		     ops->vidioc_g_ext_fmt_vid_out ||
> > +		     ops->vidioc_g_fmt_vid_out_mplane))
> >  			return 0;
> >  		break;
> >  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > -		if (is_vid && is_tx && ops->vidioc_g_fmt_vid_out_mplane)
> > +		if (is_vid && is_tx &&
> > +		    (ops->vidioc_g_ext_fmt_vid_out ||
> > +		     ops->vidioc_g_fmt_vid_out_mplane))
> >  			return 0;
> >  		break;
> >  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
> > @@ -1048,6 +1132,197 @@ static void v4l_sanitize_format(struct v4l2_format *fmt)
> >  	       sizeof(fmt->fmt.pix) - offset);
> >  }


> > +static int v4l_g_fmt_ext_pix(const struct v4l2_ioctl_ops *ops,
> > +			     struct file *file, void *fh,
> > +			     struct v4l2_format *f)
> > +{
> > +	struct v4l2_ext_format ef = {
> > +		.type = f->type,
> > +	};
> > +	int ret;
> > +
> > +	switch (f->type) {
> > +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > +		ret = ops->vidioc_g_ext_fmt_vid_cap(file, fh, &ef.fmt.pix);  
> 
> This will call vidioc_g_ext_fmt_vid_cap with type _MPLANE, which we said we
> wouldn't support for the extended ioctls.

That's okay because we don't pass ef around just the ef.fmt.pix
which does not contain the format type. This being said, we should
definitely have

		ef.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;

for the v4l2_ext_format_to_format() conversion that happens at the end
of the function.

> 
> > +		break;
> > +
> > +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> > +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:

Same here:

		ef.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;

> > +		ret = ops->vidioc_g_ext_fmt_vid_out(file, fh, &ef.fmt.pix);
> > +		break;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	return v4l2_ext_format_to_format(&ef, f,
> > +					 V4L2_TYPE_IS_MULTIPLANAR(f->type),
> > +					 true);
> > +}
> > +
> >  static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
> >  				struct file *file, void *fh, void *arg)
> >  {
> > @@ -1475,15 +1782,22 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
> >  
> >  	switch (p->type) {
> >  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > -		if (unlikely(!ops->vidioc_g_fmt_vid_cap))
> > -			break;
> > -		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> > -		ret = ops->vidioc_g_fmt_vid_cap(file, fh, arg);
> > -		/* just in case the driver zeroed it again */
> > -		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> > -		return ret;
> > +		if (ops->vidioc_g_fmt_vid_cap) {
> > +			p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> > +			ret = ops->vidioc_g_fmt_vid_cap(file, fh, arg);
> > +			/* just in case the driver zeroed it again */
> > +			p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> > +			return ret;
> > +		} else if (ops->vidioc_g_ext_fmt_vid_cap) {
> > +			return v4l_g_fmt_ext_pix(ops, file, fh, p);  
> 
> Test for the extended op first. Same elsewhere below. E.g.:
> 
> 		if (ops->vidioc_g_ext_fmt_vid_cap)
> 			return v4l_g_fmt_ext_pix(ops, file, fh, p);
> 		if (unlikely(!ops->vidioc_g_fmt_vid_cap))
> 			break;
> 		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> 		...

Sure, I'll change the order.

> 
> > +		}
> > +		break;
> >  	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > -		return ops->vidioc_g_fmt_vid_cap_mplane(file, fh, arg);
> > +		if (ops->vidioc_g_fmt_vid_cap_mplane)
> > +			return ops->vidioc_g_fmt_vid_cap_mplane(file, fh, arg);
> > +		else if (ops->vidioc_g_ext_fmt_vid_cap)
> > +			return v4l_g_fmt_ext_pix(ops, file, fh, p);
> > +		break;
> >  	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> >  		return ops->vidioc_g_fmt_vid_overlay(file, fh, arg);
> >  	case V4L2_BUF_TYPE_VBI_CAPTURE:
> > @@ -1491,15 +1805,22 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
> >  	case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE:
> >  		return ops->vidioc_g_fmt_sliced_vbi_cap(file, fh, arg);
> >  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> > -		if (unlikely(!ops->vidioc_g_fmt_vid_out))
> > -			break;
> > -		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> > -		ret = ops->vidioc_g_fmt_vid_out(file, fh, arg);
> > -		/* just in case the driver zeroed it again */
> > -		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> > -		return ret;
> > +		if (ops->vidioc_g_fmt_vid_out) {
> > +			p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> > +			ret = ops->vidioc_g_fmt_vid_out(file, fh, arg);
> > +			/* just in case the driver zeroed it again */
> > +			p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> > +			return ret;
> > +		} else if (ops->vidioc_g_ext_fmt_vid_out) {
> > +			return v4l_g_fmt_ext_pix(ops, file, fh, p);
> > +		}
> > +		break;
> >  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > -		return ops->vidioc_g_fmt_vid_out_mplane(file, fh, arg);
> > +		if (ops->vidioc_g_fmt_vid_out_mplane)
> > +			return ops->vidioc_g_fmt_vid_out_mplane(file, fh, arg);
> > +		else if (ops->vidioc_g_ext_fmt_vid_out)
> > +			return v4l_g_fmt_ext_pix(ops, file, fh, p);
> > +		break;
> >  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
> >  		return ops->vidioc_g_fmt_vid_out_overlay(file, fh, arg);
> >  	case V4L2_BUF_TYPE_VBI_OUTPUT:


> >  static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
> >  				struct file *file, void *fh, void *arg)
> >  {
> > @@ -1551,23 +1943,31 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
> >  
> >  	switch (p->type) {
> >  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > -		if (unlikely(!ops->vidioc_s_fmt_vid_cap))
> > -			break;
> > -		CLEAR_AFTER_FIELD(p, fmt.pix);
> > -		ret = ops->vidioc_s_fmt_vid_cap(file, fh, arg);
> > -		/* just in case the driver zeroed it again */
> > -		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> > +		if (ops->vidioc_s_fmt_vid_cap) {
> > +			CLEAR_AFTER_FIELD(p, fmt.pix);
> > +			ret = ops->vidioc_s_fmt_vid_cap(file, fh, arg);
> > +			/* just in case the driver zeroed it again */
> > +			p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> > +		} else if (ops->vidioc_s_ext_fmt_vid_cap) {
> > +			ret = v4l_s_fmt_ext_pix(ops, file, fh, arg);  
> 
> The helper functions v4l_[g/s/try]_fmt_ext_pix are confusingly named.
> How about: v4l_[g/s/try]_fmt_using_ext_fmt.

Yep, that's definitely a better name.