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

[Hans Verkuil]

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.

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
>>>
> 
>