Re: [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver

[Archit Taneja <a0393947@ti.com>]

On Tuesday 11 March 2014 05:51 PM, Hans Verkuil wrote:
> Hi Archit,
>
> A few small comments below...
>
> On 03/11/14 09:33, Archit Taneja wrote:
>> Add selection ioctl ops. For VPE, cropping makes sense only for the input to
>> VPE(or V4L2_BUF_TYPE_VIDEO_OUTPUT/MPLANE buffers) and composing makes sense
>> only for the output of VPE(or V4L2_BUF_TYPE_VIDEO_CAPTURE/MPLANE buffers).
>>
>> For the CAPTURE type, V4L2_SEL_TGT_COMPOSE results in VPE writing the output
>> in a rectangle within the capture buffer. For the OUTPUT type, V4L2_SEL_TGT_CROP
>> results in selecting a rectangle region within the source buffer.
>>
>> Setting the crop/compose rectangles should successfully result in
>> re-configuration of registers which are affected when either source or
>> destination dimensions change, set_srcdst_params() is called for this purpose.
>>
>> Signed-off-by: Archit Taneja <archit@ti.com>
>> ---
>>   drivers/media/platform/ti-vpe/vpe.c | 141 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 141 insertions(+)
>>
>> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
>> index ece9b96..4abb85c 100644
>> --- a/drivers/media/platform/ti-vpe/vpe.c
>> +++ b/drivers/media/platform/ti-vpe/vpe.c
>> @@ -410,8 +410,10 @@ static struct vpe_q_data *get_q_data(struct vpe_ctx *ctx,
>>   {
>>   	switch (type) {
>>   	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>>   		return &ctx->q_data[Q_DATA_SRC];
>>   	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>   		return &ctx->q_data[Q_DATA_DST];
>>   	default:
>>   		BUG();
>> @@ -1587,6 +1589,142 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>   	return set_srcdst_params(ctx);
>>   }
>>
>> +static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
>> +{
>> +	struct vpe_q_data *q_data;
>> +
>> +	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
>> +		return -EINVAL;
>> +
>> +	q_data = get_q_data(ctx, s->type);
>> +	if (!q_data)
>> +		return -EINVAL;
>> +
>> +	switch (s->target) {
>> +	case V4L2_SEL_TGT_COMPOSE:
>> +		/*
>> +		 * COMPOSE target is only valid for capture buffer type, for
>> +		 * output buffer type, assign existing crop parameters to the
>> +		 * selection rectangle
>> +		 */
>> +		if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> +			break;
>
> Shouldn't this return -EINVAL?

compose only makes sense for CAPTURE. So, it breaks and performs the 
calculations after the switch statement.

>
>> +
>> +		s->r = q_data->c_rect;
>> +		return 0;

The above 2 lines are called for if we try to set compose for OUTPUT. I 
don't return an error here, just keep the size as the original rect 
size, and return 0.

I'll replace these 2 lines with 'return -EINVAL;'

>> +
>> +	case V4L2_SEL_TGT_CROP:
>> +		/*
>> +		 * CROP target is only valid for output buffer type, for capture
>> +		 * buffer type, assign existing compose parameters to the
>> +		 * selection rectangle
>> +		 */
>> +		if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
>> +			break;
>
> Ditto.
>
>> +
>> +		s->r = q_data->c_rect;
>> +		return 0;
>> +
>> +	/*
>> +	 * bound and default crop/compose targets are invalid targets to
>> +	 * try/set
>> +	 */
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (s->r.top < 0 || s->r.left < 0) {
>> +		vpe_err(ctx->dev, "negative values for top and left\n");
>> +		s->r.top = s->r.left = 0;
>> +	}
>> +
>> +	v4l_bound_align_image(&s->r.width, MIN_W, q_data->width, 1,
>> +		&s->r.height, MIN_H, q_data->height, H_ALIGN, S_ALIGN);
>> +
>> +	/* adjust left/top if cropping rectangle is out of bounds */
>> +	if (s->r.left + s->r.width > q_data->width)
>> +		s->r.left = q_data->width - s->r.width;
>> +	if (s->r.top + s->r.height > q_data->height)
>> +		s->r.top = q_data->height - s->r.height;
>> +
>> +	return 0;
>> +}
>> +
>> +static int vpe_g_selection(struct file *file, void *fh,
>> +		struct v4l2_selection *s)
>> +{
>> +	struct vpe_ctx *ctx = file2ctx(file);
>> +	struct vpe_q_data *q_data;
>> +
>> +	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
>> +		return -EINVAL;
>> +
>> +	q_data = get_q_data(ctx, s->type);
>> +	if (!q_data)
>> +		return -EINVAL;
>> +
>> +	switch (s->target) {
>> +	/* return width and height from S_FMT of the respective buffer type */
>> +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
>> +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>> +	case V4L2_SEL_TGT_CROP_DEFAULT:
>> +		s->r.left = 0;
>> +		s->r.top = 0;
>> +		s->r.width = q_data->width;
>> +		s->r.height = q_data->height;
>
> The crop targets only make sense for type OUTPUT and the compose only for
> type CAPTURE. Add some checks for that.

I return the image size for G_FMT irrespective of what combination of 
crop/compose CAPTURE/OUTPUT it is. I suppose it is better to return an 
error if the user tries to configure a wrong combination.

Thanks,
Archit