Re: [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver
On Tuesday 11 March 2014 06:19 PM, Hans Verkuil wrote: On 03/11/14 13:46, Archit Taneja wrote: 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: Yes. If for no other reason that I plan on adding crop/compose/scaling support to v4l2-compliance soon, and this will probably be one of the tests. Thanks, will update. Thanks for reviewing of the series. Archit Thanks, Archit -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver
On 03/11/14 13:46, Archit Taneja wrote: > 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 >>> --- >>> 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. It's so easy to get confused here. > >> >>> + >>> +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;' Yes, that makes more sense. > >>> + >>> +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 */ >>> +c
Re: [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver
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 --- 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->
Re: [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver
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 > --- > 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? > + > + s->r = q_data->c_rect; > + return 0; > + > + 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. > + return 0; > + > + /* > + * CROP target holds for the output buffer type, and COMPOSE target > + * holds for the capture buffer type. We still r
[PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver
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 --- 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; + + s->r = q_data->c_rect; + return 0; + + 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; + + 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; + return 0; + + /* +* CROP target holds for the output buffer type, and COMPOSE target +* holds for the capture buffer type. We still return the c_rect params +* for both the target types. +*/ + case V4L2_SEL_TGT_COMPOSE: + case V4L2_SEL_TGT_CROP: + s->r.left = q_data->c_rect.left; + s->r.top = q_data->c_rect.top; + s->r.width = q_data->c_rect.width; + s->r.height = q_data->c_rect.height; +