Re: [PATCH] vcodec: mediatek: Add g/s_selection support for V4L2 Encoder

[tiffany lin]

Hi Hans,

On Mon, 2016-07-18 at 14:44 +0200, Hans Verkuil wrote:
> On 07/18/2016 02:28 PM, tiffany lin wrote:
> > Understood now.
> > 
> > Now I am trying to figure out how to make this function right.
> > Our encoder only support crop range from (0, 0) to (width, height), so
> > if s->r.top and s->r.left not 0, I will return -EINVAL.
> > 
> > 
> > Another thing is that in v4l2-compliance test, it has testLegacyCrop.
> > It looks like we still need to support 
> >  V4L2_SEL_TGT_COMPOSE_DEFAULT:
> >  V4L2_SEL_TGT_COMPOSE_BOUNDS:
> >  V4L2_SEL_TGT_COMPOSE:
> > to pass v4l2 compliance test, Or it will fail in 
> > fail: v4l2-test-formats.cpp(1318): !doioctl(node, VIDIOC_G_SELECTION,
> > &sel)
> > fail: v4l2-test-formats.cpp(1336): testLegacyCrop(node)
> > test Cropping: FAIL
> 
> Against which kernel are you testing? In the current media_tree master
> there is a bug in drivers/media/v4l2-core/v4l2-ioctl.c, v4l_cropcap():
> 
> This code:
> 
> if (WARN_ON(!ops->vidioc_cropcap && !ops->vidioc_cropcap))
> 
> should be:
> 
> if (WARN_ON(!ops->vidioc_cropcap && !ops->vidioc_g_selection))
> 
> The fix is waiting for a pull from Linus.
> 
> Also update to the latest v4l2-compliance: I've made some changes that
> might affect this. And I added additional checks to verify if all the
> colorspace-related format fields are properly propagated from the
> output format to the capture format.
> 

Sorry, I miss this part.
After update to latest version include this fix, it can pass crop test
without supporting COMPOSE in output queue.
Appreciated for your help

best regards,
Tiffany



> Regards,
> 
> 	Hans
> 
> > 
> > I don't understand the following testing code.
> > 
> >         /*
> >          * If either CROPCAP or G_CROP works, then G_SELECTION should
> >          * work as well.
> >          * If neither CROPCAP nor G_CROP work, then G_SELECTION
> > shouldn't
> >          * work either.
> >          */
> >         if (!doioctl(node, VIDIOC_CROPCAP, &cap)) {
> >                 fail_on_test(doioctl(node, VIDIOC_G_SELECTION, &sel));
> > 
> >                 // Checks for invalid types
> >                 if (cap.type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >                         cap.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> >                 else
> >                         cap.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> >                 fail_on_test(doioctl(node, VIDIOC_CROPCAP, &cap) !=
> > EINVAL);
> >                 cap.type = 0xff;
> >                 fail_on_test(doioctl(node, VIDIOC_CROPCAP, &cap) !=
> > EINVAL);
> >         } else {
> >                 fail_on_test(!doioctl(node, VIDIOC_G_SELECTION, &sel));
> > -> fail here
> >         }
> > 
> > When test OUTPUT queue, it fail because v4l_cropcap will fail when
> > s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS.
> > If VIDIOC_CROPCAP ioctl fail, VIDIOC_G_SELECTION should fail.
> > But VIDIOC_G_SELECTION target on CROP not COMPOSE and it success.
> > 
> > 
> > best regards,
> > Tiffany
> > 
> > 
> > 
> >> Regards,
> >>
> >> 	Hans
> >>
> >>>
> >>>
> >>> static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
> >>> 				struct file *file, void *fh, void *arg)
> >>> {
> >>> 	struct v4l2_crop *p = arg;
> >>> 	struct v4l2_selection s = {
> >>> 		.type = p->type,
> >>> 	};
> >>> 	int ret;
> >>>
> >>> 	if (ops->vidioc_g_crop)
> >>> 		return ops->vidioc_g_crop(file, fh, p);
> >>> 	/* simulate capture crop using selection api */
> >>>
> >>> 	/* crop means compose for output devices */
> >>> 	if (V4L2_TYPE_IS_OUTPUT(p->type))
> >>> 		s.target = V4L2_SEL_TGT_COMPOSE_ACTIVE;
> >>> 	else
> >>> 		s.target = V4L2_SEL_TGT_CROP_ACTIVE;
> >>>
> >>> 	ret = ops->vidioc_g_selection(file, fh, &s);
> >>>
> >>> 	/* copying results to old structure on success */
> >>> 	if (!ret)
> >>> 		p->c = s.r;
> >>> 	return ret;
> >>> }
> >>>
> >>> static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
> >>> 				struct file *file, void *fh, void *arg)
> >>> {
> >>> 	struct v4l2_crop *p = arg;
> >>> 	struct v4l2_selection s = {
> >>> 		.type = p->type,
> >>> 		.r = p->c,
> >>> 	};
> >>>
> >>> 	if (ops->vidioc_s_crop)
> >>> 		return ops->vidioc_s_crop(file, fh, p);
> >>> 	/* simulate capture crop using selection api */
> >>>
> >>> 	/* crop means compose for output devices */
> >>> 	if (V4L2_TYPE_IS_OUTPUT(p->type))
> >>> 		s.target = V4L2_SEL_TGT_COMPOSE_ACTIVE;
> >>> 	else
> >>> 		s.target = V4L2_SEL_TGT_CROP_ACTIVE;
> >>>
> >>> 	return ops->vidioc_s_selection(file, fh, &s);
> >>> }
> >>>
> >>>
> >>> best regards,
> >>> Tiffany
> >>>
> >>>
> >>>
> >>>
> >>>> Regards,
> >>>>
> >>>> 	Hans
> >>>>
> >>>>> +
> >>>>> +	return 0;
> >>>>> +}
> >>>>> +
> >>>>>  static int vidioc_venc_qbuf(struct file *file, void *priv,
> >>>>>  			    struct v4l2_buffer *buf)
> >>>>>  {
> >>>>> @@ -688,6 +759,9 @@ const struct v4l2_ioctl_ops mtk_venc_ioctl_ops = {
> >>>>>  
> >>>>>  	.vidioc_create_bufs		= v4l2_m2m_ioctl_create_bufs,
> >>>>>  	.vidioc_prepare_buf		= v4l2_m2m_ioctl_prepare_buf,
> >>>>> +
> >>>>> +	.vidioc_g_selection		= vidioc_venc_g_selection,
> >>>>> +	.vidioc_s_selection		= vidioc_venc_s_selection,
> >>>>>  };
> >>>>>  
> >>>>>  static int vb2ops_venc_queue_setup(struct vb2_queue *vq,
> >>>>>
> >>>
> >>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> >>> the body of a message to majordomo@xxxxxxxxxxxxxxx
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>
> > 
> >