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