Hi Tiffany,

On 07/29/2016 11:50 AM, Tiffany Lin wrote:

This is not an error you want to log, just drop this. You can always get detailed
debugging by running 'echo 2 >/sys/class/video4linux/videoX/dev_debug'.

+		return -EINVAL;
+	}
+
+	q_data = mtk_venc_get_q_data(ctx, s->type);
+	if (!q_data)
+		return -EINVAL;
+
+	/* crop means compose for output devices */

Drop this comment. That's only true for the old G/S_CROP ioctls.
For the selection API crop is really crop.

+	switch (s->target) {
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		s->r.top = 0;
+		s->r.left = 0;
+		s->r.width = q_data->coded_width;
+		s->r.height = q_data->coded_height;
+		break;
+	case V4L2_SEL_TGT_CROP:
+		s->r.top = 0;
+		s->r.left = 0;
+		s->r.width = q_data->visible_width;
+		s->r.height = q_data->visible_height;
+		break;
+	default:
+		mtk_v4l2_err("Invalid s->target = %d", s->target);

Same here: just drop this line.

+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int vidioc_venc_s_selection(struct file *file, void *priv,
+				     struct v4l2_selection *s)
+{
+	struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
+	struct mtk_q_data *q_data;
+
+	if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) {
+		mtk_v4l2_err("Invalid s->type = %d", s->type);

Ditto.

+		return -EINVAL;
+	}
+
+	q_data = mtk_venc_get_q_data(ctx, s->type);
+	if (!q_data)
+		return -EINVAL;
+
+	switch (s->target) {
+	case V4L2_SEL_TGT_CROP:
+		/* Only support crop from (0,0) */
+		if ((s->r.width > q_data->coded_width) ||
+			(s->r.height > q_data->coded_height)) {
+			return -ERANGE;
+		}

This isn't correct, instead just correct the width and height to the
max possible value.

+		s->r.top = 0;
+		s->r.left = 0;
+		q_data->visible_width = s->r.width;
+		q_data->visible_height = s->r.height;
+		break;
+	default:
+		mtk_v4l2_err("Invalid s->target = %d", s->target);

Ditto.

+		return -EINVAL;
+	}
+	return 0;
+}

Note that this function doesn't check the constraint flags in s->r. However, this
is a generic problem that I see often. I've written a patch that adds a helper
function that can check the new rectangle against the original rectangle and
constraint flags and returns -ERANGE if it doesn't fit.

That should simplify this code to:

struct v4l2_rect r = s->r;
int err;

...

case V4L2_SEL_TGT_CROP:
	r.left = 0;
	r.top = 0;
	r.width = min(r.width, q_data->coded_width);
	r.height = min(r.height, q_data->coded_height);
	err = v4l2_s_selection(s, &r);
	if (err)
		return err;
	q_data->visible_width = s->r.width;
	q_data->visible_height = s->r.height;
	break;

Please test that patch and fold it into your v3 patch series.
	

Regards,

	Hans