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

[tiffany lin <tiffany.lin@mediatek.com>]

Hi Hans,

On Mon, 2016-07-11 at 06:32 +0200, Hans Verkuil wrote:
> Hi Tiffany,
> 
> My apologies for the delay, but here is my review at last:
> 
> On 05/30/2016 09:52 AM, Tiffany Lin wrote:
> > This patch add g/s_selection support for MT8173
> > 
> > Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
> > ---
> >  drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c |   74 ++++++++++++++++++++
> >  1 file changed, 74 insertions(+)
> > 
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> > index 6e72d73..23ef9a1 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> > @@ -630,6 +630,77 @@ 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;
> > +
> > +	/* crop means compose for output devices */
> > +	switch (s->target) {
> > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > +	case V4L2_SEL_TGT_CROP:
> > +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> > +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > +	case V4L2_SEL_TGT_COMPOSE:
> > +		if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> > +			mtk_v4l2_err("Invalid s->type = %d", s->type);
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	default:
> > +		mtk_v4l2_err("Invalid s->target = %d", s->target);
> > +		return -EINVAL;
> > +	}
> > +
> > +	q_data = mtk_venc_get_q_data(ctx, s->type);
> > +	if (!q_data)
> > +		return -EINVAL;
> > +
> > +	s->r.top = 0;
> > +	s->r.left = 0;
> > +	s->r.width = q_data->visible_width;
> > +	s->r.height = q_data->visible_height;
> > +
> > +	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;
> > +
> > +	switch (s->target) {
> > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > +	case V4L2_SEL_TGT_CROP:
> > +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> > +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > +	case V4L2_SEL_TGT_COMPOSE:
> > +		if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> > +			mtk_v4l2_err("Invalid s->type = %d", s->type);
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	default:
> > +		mtk_v4l2_err("Invalid s->target = %d", s->target);
> > +		return -EINVAL;
> > +	}
> > +
> > +	q_data = mtk_venc_get_q_data(ctx, s->type);
> > +	if (!q_data)
> > +		return -EINVAL;
> > +
> > +	s->r.top = 0;
> > +	s->r.left = 0;
> > +	q_data->visible_width = s->r.width;
> > +	q_data->visible_height = s->r.height;
> 
> This makes no sense.
> 
> See this page:
> 
> https://hverkuil.home.xs4all.nl/spec/media.html#selection-api
> 
> For the video output direction (memory -> HW encoder) the data source is
> the memory, the data sink is the HW encoder. For the video capture direction
> (HW encoder -> memory) the data source is the HW encoder and the data sink
> is the memory.
> 
> Usually for m2m devices the video output direction may support cropping and
> the video capture direction may support composing.
> 
> It's not clear what you intend here, especially since you set left and right
> to 0. That's not what the selection operation is supposed to do.
> 

We only support simple crop, but as you mentioned we need to use
g/s_selection not g/s_crop.
This provide our user space application could get/set encode region to
driver and encode region should always start from (0,0) to (width,
height).
That's why we always set top and left to 0.


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