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

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

[Tiffany Lin]

This patch add g/s_selection support for MT8173 v4l2 encoder

Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
---
v3:
- add v4l2_s_selection to check constraint flags
- remove visible_height modification in s_fmt_out
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c |   81 +++++++++++++++++---
 1 file changed, 71 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
index 3ed3f2d..c4b8e00 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
@@ -487,7 +487,6 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv,
 	struct mtk_q_data *q_data;
 	int ret, i;
 	struct mtk_video_fmt *fmt;
-	unsigned int pitch_w_div16;
 	struct v4l2_pix_format_mplane *pix_fmt_mp = &f->fmt.pix_mp;
 
 	vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type);
@@ -530,15 +529,6 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv,
 	q_data->coded_width = f->fmt.pix_mp.width;
 	q_data->coded_height = f->fmt.pix_mp.height;
 
-	pitch_w_div16 = DIV_ROUND_UP(q_data->visible_width, 16);
-	if (pitch_w_div16 % 8 != 0) {
-		/* Adjust returned width/height, so application could correctly
-		 * allocate hw required memory
-		 */
-		q_data->visible_height += 32;
-		vidioc_try_fmt(f, q_data->fmt);
-	}
-
 	q_data->field = f->fmt.pix_mp.field;
 	ctx->colorspace = f->fmt.pix_mp.colorspace;
 	ctx->ycbcr_enc = f->fmt.pix_mp.ycbcr_enc;
@@ -631,6 +621,74 @@ static int vidioc_try_fmt_vid_out_mplane(struct file *file, void *priv,
 	return vidioc_try_fmt(f, fmt);
 }
 
+static int vidioc_venc_g_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)
+		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_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:
+		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;
+	struct v4l2_rect r = s->r;
+	int err;
+
+	if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
+		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) */
+		s->r.top = 0;
+		s->r.left = 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;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int vidioc_venc_qbuf(struct file *file, void *priv,
 			    struct v4l2_buffer *buf)
 {
@@ -689,6 +747,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,
-- 
1.7.9.5

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1603 bytes --]

Hi Tiffany,

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20160804]
[cannot apply to v4.7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tiffany-Lin/vcodec-mediatek-Add-g-s_selection-support-for-V4L2-Encoder/20160804-181402
base:   git://linuxtv.org/media_tree.git master
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c: In function 'vidioc_venc_s_selection':
>> drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c:680:9: error: implicit declaration of function 'v4l2_s_selection' [-Werror=implicit-function-declaration]
      err = v4l2_s_selection(s, &r);
            ^~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/v4l2_s_selection +680 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c

   674		case V4L2_SEL_TGT_CROP:
   675			/* Only support crop from (0,0) */
   676			s->r.top = 0;
   677			s->r.left = 0;
   678			r.width = min(r.width, q_data->coded_width);
   679			r.height = min(r.height, q_data->coded_height);
 > 680			err = v4l2_s_selection(s, &r);
   681			if (err)
   682				return err;
   683			q_data->visible_width = s->r.width;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 55451 bytes --]

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

[Hans Verkuil]

On 08/04/2016 12:08 PM, Tiffany Lin wrote:
> This patch add g/s_selection support for MT8173 v4l2 encoder
> 
> Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
> ---
> v3:
> - add v4l2_s_selection to check constraint flags
> - remove visible_height modification in s_fmt_out

Is this selection-related, or is this an unrelated bug fix?
If it is the latter, then please split it up as a separate patch.

In both cases the commit log should explain a bit more why
this s_fmt_out change is needed.

> ---
>  drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c |   81 +++++++++++++++++---
>  1 file changed, 71 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> index 3ed3f2d..c4b8e00 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> @@ -487,7 +487,6 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv,
>  	struct mtk_q_data *q_data;
>  	int ret, i;
>  	struct mtk_video_fmt *fmt;
> -	unsigned int pitch_w_div16;
>  	struct v4l2_pix_format_mplane *pix_fmt_mp = &f->fmt.pix_mp;
>  
>  	vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type);
> @@ -530,15 +529,6 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv,
>  	q_data->coded_width = f->fmt.pix_mp.width;
>  	q_data->coded_height = f->fmt.pix_mp.height;
>  
> -	pitch_w_div16 = DIV_ROUND_UP(q_data->visible_width, 16);
> -	if (pitch_w_div16 % 8 != 0) {
> -		/* Adjust returned width/height, so application could correctly
> -		 * allocate hw required memory
> -		 */
> -		q_data->visible_height += 32;
> -		vidioc_try_fmt(f, q_data->fmt);
> -	}
> -
>  	q_data->field = f->fmt.pix_mp.field;
>  	ctx->colorspace = f->fmt.pix_mp.colorspace;
>  	ctx->ycbcr_enc = f->fmt.pix_mp.ycbcr_enc;
> @@ -631,6 +621,74 @@ static int vidioc_try_fmt_vid_out_mplane(struct file *file, void *priv,
>  	return vidioc_try_fmt(f, fmt);
>  }
>  
> +static int vidioc_venc_g_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)
> +		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_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:
> +		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;
> +	struct v4l2_rect r = s->r;
> +	int err;
> +
> +	if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> +		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) */
> +		s->r.top = 0;
> +		s->r.left = 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;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  static int vidioc_venc_qbuf(struct file *file, void *priv,
>  			    struct v4l2_buffer *buf)
>  {
> @@ -689,6 +747,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,
> 

The selection code looks good!

	Hans

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

[Tiffany Lin]

Hi Hans,

On Thu, 2016-08-04 at 15:00 +0200, Hans Verkuil wrote:
> 
> On 08/04/2016 12:08 PM, Tiffany Lin wrote:
> > This patch add g/s_selection support for MT8173 v4l2 encoder
> > 
> > Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
> > ---
> > v3:
> > - add v4l2_s_selection to check constraint flags
> > - remove visible_height modification in s_fmt_out
> 
> Is this selection-related, or is this an unrelated bug fix?
> If it is the latter, then please split it up as a separate patch.
> 
> In both cases the commit log should explain a bit more why
> this s_fmt_out change is needed.
> 
This is a bug I found when I add g_selection.
It will make visible_height out of coded_height and compliance test will
fail if not include this part of modification.

The original code here is for that user space could get buffer size HW
required by using coded_width * coded_height.
But the original code here adjust visible_height and not returned as
f->fmt.pix_mp.height, it's a bug.
But if it update coded_height here, q_data->coded_height += 32;, it will
fail in s_fmt compliance test that s_fmt(g_fmt) != g_fmt.
So I decided to drop this.
User space should use sizeimage to get buffer size HW required and
coded_width/coded_height just alignment width/height that HW required.

best regards,
Tiffany


> > ---
> >  drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c |   81 +++++++++++++++++---
> >  1 file changed, 71 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> > index 3ed3f2d..c4b8e00 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> > @@ -487,7 +487,6 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv,
> >  	struct mtk_q_data *q_data;
> >  	int ret, i;
> >  	struct mtk_video_fmt *fmt;
> > -	unsigned int pitch_w_div16;
> >  	struct v4l2_pix_format_mplane *pix_fmt_mp = &f->fmt.pix_mp;
> >  
> >  	vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type);
> > @@ -530,15 +529,6 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv,
> >  	q_data->coded_width = f->fmt.pix_mp.width;
> >  	q_data->coded_height = f->fmt.pix_mp.height;
> >  
> > -	pitch_w_div16 = DIV_ROUND_UP(q_data->visible_width, 16);
> > -	if (pitch_w_div16 % 8 != 0) {
> > -		/* Adjust returned width/height, so application could correctly
> > -		 * allocate hw required memory
> > -		 */
> > -		q_data->visible_height += 32;
> > -		vidioc_try_fmt(f, q_data->fmt);
> > -	}
> > -
> >  	q_data->field = f->fmt.pix_mp.field;
> >  	ctx->colorspace = f->fmt.pix_mp.colorspace;
> >  	ctx->ycbcr_enc = f->fmt.pix_mp.ycbcr_enc;
> > @@ -631,6 +621,74 @@ static int vidioc_try_fmt_vid_out_mplane(struct file *file, void *priv,
> >  	return vidioc_try_fmt(f, fmt);
> >  }
> >  
> > +static int vidioc_venc_g_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)
> > +		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_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:
> > +		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;
> > +	struct v4l2_rect r = s->r;
> > +	int err;
> > +
> > +	if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > +		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) */
> > +		s->r.top = 0;
> > +		s->r.left = 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;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> > +
> >  static int vidioc_venc_qbuf(struct file *file, void *priv,
> >  			    struct v4l2_buffer *buf)
> >  {
> > @@ -689,6 +747,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,
> > 
> 
> The selection code looks good!
> 
> 	Hans