| Message ID | 1393832008-22174-8-git-send-email-archit@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Archit! On 03/03/2014 08:33 AM, Archit Taneja wrote: > Add crop ioctl ops. For VPE, cropping only makes sense with the input to VPE, or > the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type. > > For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the > whole image itself, hence making crop dimensions same as the pix dimensions. > > Setting the crop successfully should result in re-configuration of those > registers which are affected when either source or destination dimensions > change, set_srcdst_params() is called for this purpose. > > Some standard crop parameter checks are done in __vpe_try_crop(). Please use the selection ops instead: if you implement cropping with those then you'll support both the selection API and the old cropping API will be implemented by the v4l2 core using the selection ops. Two for the price of one... Regards, Hans > > Signed-off-by: Archit Taneja <archit@ti.com> > --- > drivers/media/platform/ti-vpe/vpe.c | 96 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 96 insertions(+) > > diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c > index 6acdcd8..c6778f4 100644 > --- a/drivers/media/platform/ti-vpe/vpe.c > +++ b/drivers/media/platform/ti-vpe/vpe.c > @@ -1585,6 +1585,98 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f) > return set_srcdst_params(ctx); > } > > +static int __vpe_try_crop(struct vpe_ctx *ctx, struct v4l2_crop *cr) > +{ > + struct vpe_q_data *q_data; > + > + q_data = get_q_data(ctx, cr->type); > + if (!q_data) > + return -EINVAL; > + > + /* we don't support crop on capture plane */ > + if (cr->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { > + cr->c.top = cr->c.left = 0; > + cr->c.width = q_data->width; > + cr->c.height = q_data->height; > + return 0; > + } > + > + if (cr->c.top < 0 || cr->c.left < 0) { > + vpe_err(ctx->dev, "negative values for top and left\n"); > + cr->c.top = cr->c.left = 0; > + } > + > + v4l_bound_align_image(&cr->c.width, MIN_W, q_data->width, 1, > + &cr->c.height, MIN_H, q_data->height, H_ALIGN, S_ALIGN); > + > + /* adjust left/top if cropping rectangle is out of bounds */ > + if (cr->c.left + cr->c.width > q_data->width) > + cr->c.left = q_data->width - cr->c.width; > + if (cr->c.top + cr->c.height > q_data->height) > + cr->c.top = q_data->height - cr->c.height; > + > + return 0; > +} > + > +static int vpe_cropcap(struct file *file, void *priv, struct v4l2_cropcap *cr) > +{ > + struct vpe_ctx *ctx = file2ctx(file); > + struct vpe_q_data *q_data; > + > + q_data = get_q_data(ctx, cr->type); > + if (!q_data) > + return -EINVAL; > + > + cr->bounds.left = 0; > + cr->bounds.top = 0; > + cr->bounds.width = q_data->width; > + cr->bounds.height = q_data->height; > + cr->defrect = cr->bounds; > + > + return 0; > +} > + > +static int vpe_g_crop(struct file *file, void *fh, struct v4l2_crop *cr) > +{ > + struct vpe_ctx *ctx = file2ctx(file); > + struct vpe_q_data *q_data; > + > + q_data = get_q_data(ctx, cr->type); > + if (!q_data) > + return -EINVAL; > + > + cr->c = q_data->c_rect; > + > + return 0; > +} > + > +static int vpe_s_crop(struct file *file, void *priv, > + const struct v4l2_crop *crop) > +{ > + struct vpe_ctx *ctx = file2ctx(file); > + struct vpe_q_data *q_data; > + struct v4l2_crop cr = *crop; > + int ret; > + > + ret = __vpe_try_crop(ctx, &cr); > + if (ret) > + return ret; > + > + q_data = get_q_data(ctx, cr.type); > + > + if ((q_data->c_rect.left == cr.c.left) && > + (q_data->c_rect.top == cr.c.top) && > + (q_data->c_rect.width == cr.c.width) && > + (q_data->c_rect.height == cr.c.height)) { > + vpe_dbg(ctx->dev, "requested crop values are already set\n"); > + return 0; > + } > + > + q_data->c_rect = cr.c; > + > + return set_srcdst_params(ctx); > +} > + > static int vpe_reqbufs(struct file *file, void *priv, > struct v4l2_requestbuffers *reqbufs) > { > @@ -1672,6 +1764,10 @@ static const struct v4l2_ioctl_ops vpe_ioctl_ops = { > .vidioc_try_fmt_vid_out_mplane = vpe_try_fmt, > .vidioc_s_fmt_vid_out_mplane = vpe_s_fmt, > > + .vidioc_cropcap = vpe_cropcap, > + .vidioc_g_crop = vpe_g_crop, > + .vidioc_s_crop = vpe_s_crop, > + > .vidioc_reqbufs = vpe_reqbufs, > .vidioc_querybuf = vpe_querybuf, > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Monday 03 March 2014 01:20 PM, Hans Verkuil wrote: > Hi Archit! > > On 03/03/2014 08:33 AM, Archit Taneja wrote: >> Add crop ioctl ops. For VPE, cropping only makes sense with the input to VPE, or >> the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type. >> >> For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the >> whole image itself, hence making crop dimensions same as the pix dimensions. >> >> Setting the crop successfully should result in re-configuration of those >> registers which are affected when either source or destination dimensions >> change, set_srcdst_params() is called for this purpose. >> >> Some standard crop parameter checks are done in __vpe_try_crop(). > > Please use the selection ops instead: if you implement cropping with those then you'll > support both the selection API and the old cropping API will be implemented by the v4l2 > core using the selection ops. Two for the price of one... <snip> Thanks for the feedback. I'll use selection ops here. Archit -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Archit, > From: Archit Taneja [mailto:archit@ti.com] > Sent: Monday, March 03, 2014 9:26 AM > > Hi, > > On Monday 03 March 2014 01:20 PM, Hans Verkuil wrote: > > Hi Archit! > > > > On 03/03/2014 08:33 AM, Archit Taneja wrote: > >> Add crop ioctl ops. For VPE, cropping only makes sense with the > input > >> to VPE, or the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type. > >> > >> For the CAPTURE type, a S_CROP ioctl results in setting the crop > >> region as the whole image itself, hence making crop dimensions same > as the pix dimensions. > >> > >> Setting the crop successfully should result in re-configuration of > >> those registers which are affected when either source or destination > >> dimensions change, set_srcdst_params() is called for this purpose. > >> > >> Some standard crop parameter checks are done in __vpe_try_crop(). > > > > Please use the selection ops instead: if you implement cropping with > > those then you'll support both the selection API and the old cropping > > API will be implemented by the v4l2 core using the selection ops. Two > for the price of one... > > <snip> > > Thanks for the feedback. I'll use selection ops here. From your reply I understand that you will send a v2 of these patches, right? If so, please correct the typos I mentioned in the patch 5/7. Also, it is quite late for v3.15. I did already send a pull request to Mauro, However I have one patch pending. Could you tell me when are you planning to post v2 of these patches? I want to decide whether should I wait for your patchset or should I send the second pull request with the pending patch as soon as possible. Best wishes,
Hi, On Monday 03 March 2014 05:51 PM, Kamil Debski wrote: > Hi Archit, > >> From: Archit Taneja [mailto:archit@ti.com] >> Sent: Monday, March 03, 2014 9:26 AM >> >> Hi, >> >> On Monday 03 March 2014 01:20 PM, Hans Verkuil wrote: >>> Hi Archit! >>> >>> On 03/03/2014 08:33 AM, Archit Taneja wrote: >>>> Add crop ioctl ops. For VPE, cropping only makes sense with the >> input >>>> to VPE, or the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type. >>>> >>>> For the CAPTURE type, a S_CROP ioctl results in setting the crop >>>> region as the whole image itself, hence making crop dimensions same >> as the pix dimensions. >>>> >>>> Setting the crop successfully should result in re-configuration of >>>> those registers which are affected when either source or destination >>>> dimensions change, set_srcdst_params() is called for this purpose. >>>> >>>> Some standard crop parameter checks are done in __vpe_try_crop(). >>> >>> Please use the selection ops instead: if you implement cropping with >>> those then you'll support both the selection API and the old cropping >>> API will be implemented by the v4l2 core using the selection ops. Two >> for the price of one... >> >> <snip> >> >> Thanks for the feedback. I'll use selection ops here. > > From your reply I understand that you will send a v2 of these patches, > right? > If so, please correct the typos I mentioned in the patch 5/7. > > Also, it is quite late for v3.15. I did already send a pull request to > Mauro, > However I have one patch pending. Could you tell me when are you planning to > post v2 of these patches? I want to decide whether should I wait for your > patchset or should I send the second pull request with the pending patch > as soon as possible. I'll post a v2 of this set by tomorrow(India time). I have worked on a patch which converts it to selection ioctls, but I need to test it, and get some comments on whether I have implemented the selection ops correctly. Thanks, Archit -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Monday 03 March 2014 01:20 PM, Hans Verkuil wrote: > Hi Archit! > > On 03/03/2014 08:33 AM, Archit Taneja wrote: >> Add crop ioctl ops. For VPE, cropping only makes sense with the input to VPE, or >> the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type. >> >> For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the >> whole image itself, hence making crop dimensions same as the pix dimensions. >> >> Setting the crop successfully should result in re-configuration of those >> registers which are affected when either source or destination dimensions >> change, set_srcdst_params() is called for this purpose. >> >> Some standard crop parameter checks are done in __vpe_try_crop(). > > Please use the selection ops instead: if you implement cropping with those then you'll > support both the selection API and the old cropping API will be implemented by the v4l2 > core using the selection ops. Two for the price of one... When using selection API, I was finding issues using the older cropping API. The v4l_s_crop() ioctl func assumes that "crop means compose for output devices". However, for a m2m device. It probably makes sense to provide the following configuration: for V4L2_BUF_TYPE_VIDEO_OUTPUT (input to the mem to mem HW), use CROP target(to crop the input buffer) and, for V4L2_BUF_TYPE_VIDEO_CAPTURE(output of the mem to mem HW), use COMPOSE target(to place the HW output into a larger region) Don't you think forcing OUTPUT devices to 'COMPOSE' for older cropping API is a bit limiting? Thanks, Archit -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/04/2014 08:38 AM, Archit Taneja wrote: > Hi Hans, > > On Monday 03 March 2014 01:20 PM, Hans Verkuil wrote: >> Hi Archit! >> >> On 03/03/2014 08:33 AM, Archit Taneja wrote: >>> Add crop ioctl ops. For VPE, cropping only makes sense with the input to VPE, or >>> the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type. >>> >>> For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the >>> whole image itself, hence making crop dimensions same as the pix dimensions. >>> >>> Setting the crop successfully should result in re-configuration of those >>> registers which are affected when either source or destination dimensions >>> change, set_srcdst_params() is called for this purpose. >>> >>> Some standard crop parameter checks are done in __vpe_try_crop(). >> >> Please use the selection ops instead: if you implement cropping with those then you'll >> support both the selection API and the old cropping API will be implemented by the v4l2 >> core using the selection ops. Two for the price of one... > > > When using selection API, I was finding issues using the older cropping > API. The v4l_s_crop() ioctl func assumes that "crop means compose for > output devices". However, for a m2m device. It probably makes sense to > provide the following configuration: > > for V4L2_BUF_TYPE_VIDEO_OUTPUT (input to the mem to mem HW), use CROP > target(to crop the input buffer) > > and, for V4L2_BUF_TYPE_VIDEO_CAPTURE(output of the mem to mem HW), use > COMPOSE target(to place the HW output into a larger region) > > Don't you think forcing OUTPUT devices to 'COMPOSE' for older cropping > API is a bit limiting? Yes, and that's why the selection API was created to work around that limitation :-) The old cropping API was insufficiently flexible for modern devices, so we came up with this replacement. Another reason why you have to implement the selection API: it's the only way to implement your functionality. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tuesday 04 March 2014 01:13 PM, Hans Verkuil wrote: > On 03/04/2014 08:38 AM, Archit Taneja wrote: >> Hi Hans, >> >> On Monday 03 March 2014 01:20 PM, Hans Verkuil wrote: >>> Hi Archit! >>> >>> On 03/03/2014 08:33 AM, Archit Taneja wrote: >>>> Add crop ioctl ops. For VPE, cropping only makes sense with the input to VPE, or >>>> the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type. >>>> >>>> For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the >>>> whole image itself, hence making crop dimensions same as the pix dimensions. >>>> >>>> Setting the crop successfully should result in re-configuration of those >>>> registers which are affected when either source or destination dimensions >>>> change, set_srcdst_params() is called for this purpose. >>>> >>>> Some standard crop parameter checks are done in __vpe_try_crop(). >>> >>> Please use the selection ops instead: if you implement cropping with those then you'll >>> support both the selection API and the old cropping API will be implemented by the v4l2 >>> core using the selection ops. Two for the price of one... >> >> >> When using selection API, I was finding issues using the older cropping >> API. The v4l_s_crop() ioctl func assumes that "crop means compose for >> output devices". However, for a m2m device. It probably makes sense to >> provide the following configuration: >> >> for V4L2_BUF_TYPE_VIDEO_OUTPUT (input to the mem to mem HW), use CROP >> target(to crop the input buffer) >> >> and, for V4L2_BUF_TYPE_VIDEO_CAPTURE(output of the mem to mem HW), use >> COMPOSE target(to place the HW output into a larger region) >> >> Don't you think forcing OUTPUT devices to 'COMPOSE' for older cropping >> API is a bit limiting? > > Yes, and that's why the selection API was created to work around that > limitation :-) > > The old cropping API was insufficiently flexible for modern devices, so > we came up with this replacement. > > Another reason why you have to implement the selection API: it's the only > way to implement your functionality. Okay, I'll go ahead with the selection API then :) Archit -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c index 6acdcd8..c6778f4 100644 --- a/drivers/media/platform/ti-vpe/vpe.c +++ b/drivers/media/platform/ti-vpe/vpe.c @@ -1585,6 +1585,98 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f) return set_srcdst_params(ctx); } +static int __vpe_try_crop(struct vpe_ctx *ctx, struct v4l2_crop *cr) +{ + struct vpe_q_data *q_data; + + q_data = get_q_data(ctx, cr->type); + if (!q_data) + return -EINVAL; + + /* we don't support crop on capture plane */ + if (cr->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { + cr->c.top = cr->c.left = 0; + cr->c.width = q_data->width; + cr->c.height = q_data->height; + return 0; + } + + if (cr->c.top < 0 || cr->c.left < 0) { + vpe_err(ctx->dev, "negative values for top and left\n"); + cr->c.top = cr->c.left = 0; + } + + v4l_bound_align_image(&cr->c.width, MIN_W, q_data->width, 1, + &cr->c.height, MIN_H, q_data->height, H_ALIGN, S_ALIGN); + + /* adjust left/top if cropping rectangle is out of bounds */ + if (cr->c.left + cr->c.width > q_data->width) + cr->c.left = q_data->width - cr->c.width; + if (cr->c.top + cr->c.height > q_data->height) + cr->c.top = q_data->height - cr->c.height; + + return 0; +} + +static int vpe_cropcap(struct file *file, void *priv, struct v4l2_cropcap *cr) +{ + struct vpe_ctx *ctx = file2ctx(file); + struct vpe_q_data *q_data; + + q_data = get_q_data(ctx, cr->type); + if (!q_data) + return -EINVAL; + + cr->bounds.left = 0; + cr->bounds.top = 0; + cr->bounds.width = q_data->width; + cr->bounds.height = q_data->height; + cr->defrect = cr->bounds; + + return 0; +} + +static int vpe_g_crop(struct file *file, void *fh, struct v4l2_crop *cr) +{ + struct vpe_ctx *ctx = file2ctx(file); + struct vpe_q_data *q_data; + + q_data = get_q_data(ctx, cr->type); + if (!q_data) + return -EINVAL; + + cr->c = q_data->c_rect; + + return 0; +} + +static int vpe_s_crop(struct file *file, void *priv, + const struct v4l2_crop *crop) +{ + struct vpe_ctx *ctx = file2ctx(file); + struct vpe_q_data *q_data; + struct v4l2_crop cr = *crop; + int ret; + + ret = __vpe_try_crop(ctx, &cr); + if (ret) + return ret; + + q_data = get_q_data(ctx, cr.type); + + if ((q_data->c_rect.left == cr.c.left) && + (q_data->c_rect.top == cr.c.top) && + (q_data->c_rect.width == cr.c.width) && + (q_data->c_rect.height == cr.c.height)) { + vpe_dbg(ctx->dev, "requested crop values are already set\n"); + return 0; + } + + q_data->c_rect = cr.c; + + return set_srcdst_params(ctx); +} + static int vpe_reqbufs(struct file *file, void *priv, struct v4l2_requestbuffers *reqbufs) { @@ -1672,6 +1764,10 @@ static const struct v4l2_ioctl_ops vpe_ioctl_ops = { .vidioc_try_fmt_vid_out_mplane = vpe_try_fmt, .vidioc_s_fmt_vid_out_mplane = vpe_s_fmt, + .vidioc_cropcap = vpe_cropcap, + .vidioc_g_crop = vpe_g_crop, + .vidioc_s_crop = vpe_s_crop, + .vidioc_reqbufs = vpe_reqbufs, .vidioc_querybuf = vpe_querybuf,
Add crop ioctl ops. For VPE, cropping only makes sense with the input to VPE, or the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type. For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the whole image itself, hence making crop dimensions same as the pix dimensions. Setting the crop successfully should result in re-configuration of those registers which are affected when either source or destination dimensions change, set_srcdst_params() is called for this purpose. Some standard crop parameter checks are done in __vpe_try_crop(). Signed-off-by: Archit Taneja <archit@ti.com> --- drivers/media/platform/ti-vpe/vpe.c | 96 +++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+)