Re: [PATCH 1/3] media: imx: add capture compose rectangle
Hi Steve, thank you for the review. On Thu, 2018-11-08 at 21:33 -0800, Steve Longerbeam wrote: [...] > > --- a/drivers/staging/media/imx/imx-media-capture.c > > +++ b/drivers/staging/media/imx/imx-media-capture.c > > @@ -262,6 +262,10 @@ static int capture_s_fmt_vid_cap(struct file *file, > > void *fh, > > priv->vdev.fmt.fmt.pix = f->fmt.pix; > > priv->vdev.cc = imx_media_find_format(f->fmt.pix.pixelformat, > > CS_SEL_ANY, true); > > + priv->vdev.compose.left = 0; > > + priv->vdev.compose.top = 0; > > + priv->vdev.compose.width = f->fmt.fmt.pix.width; > > + priv->vdev.compose.height = f->fmt.fmt.pix.height; > > this should be: > > priv->vdev.compose.width = fmt_src.format.width; > priv->vdev.compose.height = fmt_src.format.height; > > (corrected in the next patches but needs to be corrected here). Thanks for catching this, it should be + priv->vdev.compose.width = f->fmt.pix.width; + priv->vdev.compose.height = f->fmt.pix.height; though, fmt_src is only introduced in patch 2. regards Philipp
Re: [PATCH 1/3] media: imx: add capture compose rectangle
Hi Philipp, On 11/5/18 7:20 AM, Philipp Zabel wrote: Allowing to compose captured images into larger memory buffers will let us lift alignment restrictions on CSI crop width. Signed-off-by: Philipp Zabel --- drivers/staging/media/imx/imx-ic-prpencvf.c | 3 +- drivers/staging/media/imx/imx-media-capture.c | 38 +++ drivers/staging/media/imx/imx-media-csi.c | 3 +- drivers/staging/media/imx/imx-media-vdic.c| 4 +- drivers/staging/media/imx/imx-media.h | 2 + 5 files changed, 44 insertions(+), 6 deletions(-) diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c index 28f41caba05d..fe5a77baa592 100644 --- a/drivers/staging/media/imx/imx-ic-prpencvf.c +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c @@ -366,8 +366,7 @@ static int prp_setup_channel(struct prp_priv *priv, memset(&image, 0, sizeof(image)); image.pix = vdev->fmt.fmt.pix; - image.rect.width = image.pix.width; - image.rect.height = image.pix.height; + image.rect = vdev->compose; if (rot_swap_width_height) { swap(image.pix.width, image.pix.height); diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c index b37e1186eb2f..cace8a51aca8 100644 --- a/drivers/staging/media/imx/imx-media-capture.c +++ b/drivers/staging/media/imx/imx-media-capture.c @@ -262,6 +262,10 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh, priv->vdev.fmt.fmt.pix = f->fmt.pix; priv->vdev.cc = imx_media_find_format(f->fmt.pix.pixelformat, CS_SEL_ANY, true); + priv->vdev.compose.left = 0; + priv->vdev.compose.top = 0; + priv->vdev.compose.width = f->fmt.fmt.pix.width; + priv->vdev.compose.height = f->fmt.fmt.pix.height; this should be: priv->vdev.compose.width = fmt_src.format.width; priv->vdev.compose.height = fmt_src.format.height; (corrected in the next patches but needs to be corrected here). return 0; } @@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh, v4l2_std_id std) return v4l2_subdev_call(priv->src_sd, video, s_std, std); } +static int capture_g_selection(struct file *file, void *fh, + struct v4l2_selection *s) +{ + struct capture_priv *priv = video_drvdata(file); + + switch (s->target) { + case V4L2_SEL_TGT_CROP: + case V4L2_SEL_TGT_CROP_DEFAULT: + case V4L2_SEL_TGT_CROP_BOUNDS: + case V4L2_SEL_TGT_NATIVE_SIZE: + case V4L2_SEL_TGT_COMPOSE: + case V4L2_SEL_TGT_COMPOSE_DEFAULT: + case V4L2_SEL_TGT_COMPOSE_BOUNDS: + case V4L2_SEL_TGT_COMPOSE_PADDED: + s->r = priv->vdev.compose; + break; + default: + return -EINVAL; + } + + return 0; +} + +static int capture_s_selection(struct file *file, void *fh, + struct v4l2_selection *s) +{ + return capture_g_selection(file, fh, s); +} + static int capture_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a) { @@ -350,6 +383,9 @@ static const struct v4l2_ioctl_ops capture_ioctl_ops = { .vidioc_g_std = capture_g_std, .vidioc_s_std = capture_s_std, + .vidioc_g_selection = capture_g_selection, + .vidioc_s_selection = capture_s_selection, + .vidioc_g_parm = capture_g_parm, .vidioc_s_parm = capture_s_parm, @@ -687,6 +723,8 @@ int imx_media_capture_device_register(struct imx_media_video_dev *vdev) vdev->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix, &fmt_src.format, NULL); + vdev->compose.width = fmt_src.format.width; + vdev->compose.height = fmt_src.format.height; vdev->cc = imx_media_find_format(vdev->fmt.fmt.pix.pixelformat, CS_SEL_ANY, false); diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c index 4223f8d418ae..c4523afe7b48 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -413,8 +413,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv) memset(&image, 0, sizeof(image)); image.pix = vdev->fmt.fmt.pix; - image.rect.width = image.pix.width; - image.rect.height = image.pix.height; + image.rect = vdev->compose; csi_idmac_setup_vb2_buf(priv, phys); diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c index 482250d47e7c..e08d296cf4eb 100644 --- a/drivers/staging/media/imx/imx-media-vdic.c +++ b/drivers/staging/media/imx/imx-media-vdic.c @@ -263,10 +263,10 @@ static int setup_vdi_channel(struct vdic_priv *priv, mem
Re: [PATCH 1/3] media: imx: add capture compose rectangle
Hi Philipp, On Tue, Nov 06, 2018 at 03:44:07PM +0100, Philipp Zabel wrote: > Hi Sakari, > > On Tue, 2018-11-06 at 16:01 +0200, Sakari Ailus wrote: > [...] > > @@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh, > > v4l2_std_id std) > > > return v4l2_subdev_call(priv->src_sd, video, s_std, std); > > > } > > > > > > +static int capture_g_selection(struct file *file, void *fh, > > > +struct v4l2_selection *s) > > > +{ > > > + struct capture_priv *priv = video_drvdata(file); > > > + > > > + switch (s->target) { > > > + case V4L2_SEL_TGT_CROP: > > > + case V4L2_SEL_TGT_CROP_DEFAULT: > > > + case V4L2_SEL_TGT_CROP_BOUNDS: > > > + case V4L2_SEL_TGT_NATIVE_SIZE: > > > > The NATIVE_SIZE is for devices such as sensors. It doesn't make sense here. > > Should this be documented in Documentation/media/uapi/v4l/v4l2- > selection-targets.rst ? There it only mentions when to make it > writeable. This seems to have originated from the documentation before the ReST conversion and I had hard time to figure out where the current text (apart from sensor pixel array) came from. There is also no driver using it in that meaning, and I doubt if the use is not already been covered by the compose rectangle. This indeed requires some follow-up, but that's out of scope of your set. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH 1/3] media: imx: add capture compose rectangle
Hi Sakari, On Tue, 2018-11-06 at 16:01 +0200, Sakari Ailus wrote: [...] > @@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh, > v4l2_std_id std) > > return v4l2_subdev_call(priv->src_sd, video, s_std, std); > > } > > > > +static int capture_g_selection(struct file *file, void *fh, > > + struct v4l2_selection *s) > > +{ > > + struct capture_priv *priv = video_drvdata(file); > > + > > + switch (s->target) { > > + case V4L2_SEL_TGT_CROP: > > + case V4L2_SEL_TGT_CROP_DEFAULT: > > + case V4L2_SEL_TGT_CROP_BOUNDS: > > + case V4L2_SEL_TGT_NATIVE_SIZE: > > The NATIVE_SIZE is for devices such as sensors. It doesn't make sense here. Should this be documented in Documentation/media/uapi/v4l/v4l2- selection-targets.rst ? There it only mentions when to make it writeable. > With that removed, > > Acked-by: Sakari Ailus Thank you, I'll remove that line. regards Philipp
Re: [PATCH 1/3] media: imx: add capture compose rectangle
Hi Philipp, On Mon, Nov 05, 2018 at 04:20:53PM +0100, Philipp Zabel wrote: > Allowing to compose captured images into larger memory buffers > will let us lift alignment restrictions on CSI crop width. > > Signed-off-by: Philipp Zabel > --- > drivers/staging/media/imx/imx-ic-prpencvf.c | 3 +- > drivers/staging/media/imx/imx-media-capture.c | 38 +++ > drivers/staging/media/imx/imx-media-csi.c | 3 +- > drivers/staging/media/imx/imx-media-vdic.c| 4 +- > drivers/staging/media/imx/imx-media.h | 2 + > 5 files changed, 44 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c > b/drivers/staging/media/imx/imx-ic-prpencvf.c > index 28f41caba05d..fe5a77baa592 100644 > --- a/drivers/staging/media/imx/imx-ic-prpencvf.c > +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c > @@ -366,8 +366,7 @@ static int prp_setup_channel(struct prp_priv *priv, > > memset(&image, 0, sizeof(image)); > image.pix = vdev->fmt.fmt.pix; > - image.rect.width = image.pix.width; > - image.rect.height = image.pix.height; > + image.rect = vdev->compose; > > if (rot_swap_width_height) { > swap(image.pix.width, image.pix.height); > diff --git a/drivers/staging/media/imx/imx-media-capture.c > b/drivers/staging/media/imx/imx-media-capture.c > index b37e1186eb2f..cace8a51aca8 100644 > --- a/drivers/staging/media/imx/imx-media-capture.c > +++ b/drivers/staging/media/imx/imx-media-capture.c > @@ -262,6 +262,10 @@ static int capture_s_fmt_vid_cap(struct file *file, void > *fh, > priv->vdev.fmt.fmt.pix = f->fmt.pix; > priv->vdev.cc = imx_media_find_format(f->fmt.pix.pixelformat, > CS_SEL_ANY, true); > + priv->vdev.compose.left = 0; > + priv->vdev.compose.top = 0; > + priv->vdev.compose.width = f->fmt.fmt.pix.width; > + priv->vdev.compose.height = f->fmt.fmt.pix.height; > > return 0; > } > @@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh, > v4l2_std_id std) > return v4l2_subdev_call(priv->src_sd, video, s_std, std); > } > > +static int capture_g_selection(struct file *file, void *fh, > +struct v4l2_selection *s) > +{ > + struct capture_priv *priv = video_drvdata(file); > + > + switch (s->target) { > + case V4L2_SEL_TGT_CROP: > + case V4L2_SEL_TGT_CROP_DEFAULT: > + case V4L2_SEL_TGT_CROP_BOUNDS: > + case V4L2_SEL_TGT_NATIVE_SIZE: The NATIVE_SIZE is for devices such as sensors. It doesn't make sense here. With that removed, Acked-by: Sakari Ailus > + case V4L2_SEL_TGT_COMPOSE: > + case V4L2_SEL_TGT_COMPOSE_DEFAULT: > + case V4L2_SEL_TGT_COMPOSE_BOUNDS: > + case V4L2_SEL_TGT_COMPOSE_PADDED: > + s->r = priv->vdev.compose; > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int capture_s_selection(struct file *file, void *fh, > +struct v4l2_selection *s) > +{ > + return capture_g_selection(file, fh, s); > +} > + > static int capture_g_parm(struct file *file, void *fh, > struct v4l2_streamparm *a) > { > @@ -350,6 +383,9 @@ static const struct v4l2_ioctl_ops capture_ioctl_ops = { > .vidioc_g_std = capture_g_std, > .vidioc_s_std = capture_s_std, > > + .vidioc_g_selection = capture_g_selection, > + .vidioc_s_selection = capture_s_selection, > + > .vidioc_g_parm = capture_g_parm, > .vidioc_s_parm = capture_s_parm, > > @@ -687,6 +723,8 @@ int imx_media_capture_device_register(struct > imx_media_video_dev *vdev) > vdev->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix, > &fmt_src.format, NULL); > + vdev->compose.width = fmt_src.format.width; > + vdev->compose.height = fmt_src.format.height; > vdev->cc = imx_media_find_format(vdev->fmt.fmt.pix.pixelformat, >CS_SEL_ANY, false); > > diff --git a/drivers/staging/media/imx/imx-media-csi.c > b/drivers/staging/media/imx/imx-media-csi.c > index 4223f8d418ae..c4523afe7b48 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -413,8 +413,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv) > > memset(&image, 0, sizeof(image)); > image.pix = vdev->fmt.fmt.pix; > - image.rect.width = image.pix.width; > - image.rect.height = image.pix.height; > + image.rect = vdev->compose; > > csi_idmac_setup_vb2_buf(priv, phys); > > diff --git a/drivers/staging/media/imx/imx-media-vdic.c > b/drivers/staging/media/imx/imx-media-vdic.c > index 482250d47e7c..e08d296cf4eb 100644 > --- a/drivers/staging/media/imx/imx-media-vdic.c > +++ b/drivers/staging/media/imx/im
[PATCH 1/3] media: imx: add capture compose rectangle
Allowing to compose captured images into larger memory buffers will let us lift alignment restrictions on CSI crop width. Signed-off-by: Philipp Zabel --- drivers/staging/media/imx/imx-ic-prpencvf.c | 3 +- drivers/staging/media/imx/imx-media-capture.c | 38 +++ drivers/staging/media/imx/imx-media-csi.c | 3 +- drivers/staging/media/imx/imx-media-vdic.c| 4 +- drivers/staging/media/imx/imx-media.h | 2 + 5 files changed, 44 insertions(+), 6 deletions(-) diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c index 28f41caba05d..fe5a77baa592 100644 --- a/drivers/staging/media/imx/imx-ic-prpencvf.c +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c @@ -366,8 +366,7 @@ static int prp_setup_channel(struct prp_priv *priv, memset(&image, 0, sizeof(image)); image.pix = vdev->fmt.fmt.pix; - image.rect.width = image.pix.width; - image.rect.height = image.pix.height; + image.rect = vdev->compose; if (rot_swap_width_height) { swap(image.pix.width, image.pix.height); diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c index b37e1186eb2f..cace8a51aca8 100644 --- a/drivers/staging/media/imx/imx-media-capture.c +++ b/drivers/staging/media/imx/imx-media-capture.c @@ -262,6 +262,10 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh, priv->vdev.fmt.fmt.pix = f->fmt.pix; priv->vdev.cc = imx_media_find_format(f->fmt.pix.pixelformat, CS_SEL_ANY, true); + priv->vdev.compose.left = 0; + priv->vdev.compose.top = 0; + priv->vdev.compose.width = f->fmt.fmt.pix.width; + priv->vdev.compose.height = f->fmt.fmt.pix.height; return 0; } @@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh, v4l2_std_id std) return v4l2_subdev_call(priv->src_sd, video, s_std, std); } +static int capture_g_selection(struct file *file, void *fh, + struct v4l2_selection *s) +{ + struct capture_priv *priv = video_drvdata(file); + + switch (s->target) { + case V4L2_SEL_TGT_CROP: + case V4L2_SEL_TGT_CROP_DEFAULT: + case V4L2_SEL_TGT_CROP_BOUNDS: + case V4L2_SEL_TGT_NATIVE_SIZE: + case V4L2_SEL_TGT_COMPOSE: + case V4L2_SEL_TGT_COMPOSE_DEFAULT: + case V4L2_SEL_TGT_COMPOSE_BOUNDS: + case V4L2_SEL_TGT_COMPOSE_PADDED: + s->r = priv->vdev.compose; + break; + default: + return -EINVAL; + } + + return 0; +} + +static int capture_s_selection(struct file *file, void *fh, + struct v4l2_selection *s) +{ + return capture_g_selection(file, fh, s); +} + static int capture_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a) { @@ -350,6 +383,9 @@ static const struct v4l2_ioctl_ops capture_ioctl_ops = { .vidioc_g_std = capture_g_std, .vidioc_s_std = capture_s_std, + .vidioc_g_selection = capture_g_selection, + .vidioc_s_selection = capture_s_selection, + .vidioc_g_parm = capture_g_parm, .vidioc_s_parm = capture_s_parm, @@ -687,6 +723,8 @@ int imx_media_capture_device_register(struct imx_media_video_dev *vdev) vdev->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix, &fmt_src.format, NULL); + vdev->compose.width = fmt_src.format.width; + vdev->compose.height = fmt_src.format.height; vdev->cc = imx_media_find_format(vdev->fmt.fmt.pix.pixelformat, CS_SEL_ANY, false); diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c index 4223f8d418ae..c4523afe7b48 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -413,8 +413,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv) memset(&image, 0, sizeof(image)); image.pix = vdev->fmt.fmt.pix; - image.rect.width = image.pix.width; - image.rect.height = image.pix.height; + image.rect = vdev->compose; csi_idmac_setup_vb2_buf(priv, phys); diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c index 482250d47e7c..e08d296cf4eb 100644 --- a/drivers/staging/media/imx/imx-media-vdic.c +++ b/drivers/staging/media/imx/imx-media-vdic.c @@ -263,10 +263,10 @@ static int setup_vdi_channel(struct vdic_priv *priv, memset(&image, 0, sizeof(image)); image.pix = vdev->fmt.fmt.pix; + image.rect = vdev->compose; /* one field to VDIC channels */ image.pix.height /= 2; - image.rect.width = image.pix.width; -