* [PATCH 2/3] media: imx: set compose rectangle to mbus format
2018-11-05 15:20 [PATCH 1/3] media: imx: add capture compose rectangle Philipp Zabel
@ 2018-11-05 15:20 ` Philipp Zabel
2018-11-09 5:33 ` Steve Longerbeam
2018-11-05 15:20 ` [PATCH 3/3] media: imx: lift CSI width alignment restriction Philipp Zabel
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Philipp Zabel @ 2018-11-05 15:20 UTC (permalink / raw)
To: linux-media; +Cc: Steve Longerbeam, Hans Verkuil
Prepare for mbus format being smaller than the written rectangle
due to burst size.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/staging/media/imx/imx-media-capture.c | 55 +++++++++++++------
1 file changed, 38 insertions(+), 17 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index cace8a51aca8..2d49d9573056 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -203,21 +203,14 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
return 0;
}
-static int capture_try_fmt_vid_cap(struct file *file, void *fh,
- struct v4l2_format *f)
+static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
+ struct v4l2_subev_format *fmt_src,
+ struct v4l2_format *f)
{
struct capture_priv *priv = video_drvdata(file);
- struct v4l2_subdev_format fmt_src;
const struct imx_media_pixfmt *cc, *cc_src;
- int ret;
-
- fmt_src.pad = priv->src_sd_pad;
- fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
- ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
- if (ret)
- return ret;
- cc_src = imx_media_find_ipu_format(fmt_src.format.code, CS_SEL_ANY);
+ cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
if (cc_src) {
u32 fourcc, cs_sel;
@@ -231,7 +224,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
cc = imx_media_find_format(fourcc, cs_sel, false);
}
} else {
- cc_src = imx_media_find_mbus_format(fmt_src.format.code,
+ cc_src = imx_media_find_mbus_format(fmt_src->format.code,
CS_SEL_ANY, true);
if (WARN_ON(!cc_src))
return -EINVAL;
@@ -239,15 +232,32 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
cc = cc_src;
}
- imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, &fmt_src.format, cc);
+ imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, &fmt_src->format, cc);
return 0;
}
+static int capture_try_fmt_vid_cap(struct file *file, void *fh,
+ struct v4l2_format *f)
+{
+ struct capture_priv *priv = video_drvdata(file);
+ struct v4l2_subdev_format fmt_src;
+ int ret;
+
+ fmt_src.pad = priv->src_sd_pad;
+ fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
+ if (ret)
+ return ret;
+
+ return __capture_try_fmt(priv, &fmt_src, f);
+}
+
static int capture_s_fmt_vid_cap(struct file *file, void *fh,
struct v4l2_format *f)
{
struct capture_priv *priv = video_drvdata(file);
+ struct v4l2_subdev_format fmt_src;
int ret;
if (vb2_is_busy(&priv->q)) {
@@ -255,7 +265,13 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
return -EBUSY;
}
- ret = capture_try_fmt_vid_cap(file, priv, f);
+ fmt_src.pad = priv->src_sd_pad;
+ fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
+ if (ret)
+ return ret;
+
+ ret = __capture_try_fmt_vid_cap(priv, &fmt_src, f);
if (ret)
return ret;
@@ -264,8 +280,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
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;
+ priv->vdev.compose.width = fmt_src.width;
+ priv->vdev.compose.height = fmt_src.height;
return 0;
}
@@ -307,9 +323,14 @@ static int capture_g_selection(struct file *file, void *fh,
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;
+ case V4L2_SEL_TGT_COMPOSE_PADDED:
+ s->r.left = 0;
+ s->r.top = 0;
+ s->r.width = priv->vdev.fmt.fmt.pix.width;
+ s->r.height = priv->vdev.fmt.fmt.pix.height;
+ break;
default:
return -EINVAL;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] media: imx: set compose rectangle to mbus format
2018-11-05 15:20 ` [PATCH 2/3] media: imx: set compose rectangle to mbus format Philipp Zabel
@ 2018-11-09 5:33 ` Steve Longerbeam
2018-11-09 13:09 ` Philipp Zabel
0 siblings, 1 reply; 14+ messages in thread
From: Steve Longerbeam @ 2018-11-09 5:33 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: Hans Verkuil
Hi Philipp,
On 11/5/18 7:20 AM, Philipp Zabel wrote:
> Prepare for mbus format being smaller than the written rectangle
> due to burst size.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> drivers/staging/media/imx/imx-media-capture.c | 55 +++++++++++++------
> 1 file changed, 38 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index cace8a51aca8..2d49d9573056 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -203,21 +203,14 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
> return 0;
> }
>
> -static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> - struct v4l2_format *f)
> +static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
> + struct v4l2_subev_format *fmt_src,
typo: struct v4l2_subdev_format *fmt_src,
> + struct v4l2_format *f)
> {
> struct capture_priv *priv = video_drvdata(file);
> - struct v4l2_subdev_format fmt_src;
> const struct imx_media_pixfmt *cc, *cc_src;
> - int ret;
> -
> - fmt_src.pad = priv->src_sd_pad;
> - fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> - ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
> - if (ret)
> - return ret;
>
> - cc_src = imx_media_find_ipu_format(fmt_src.format.code, CS_SEL_ANY);
> + cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
> if (cc_src) {
> u32 fourcc, cs_sel;
>
> @@ -231,7 +224,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> cc = imx_media_find_format(fourcc, cs_sel, false);
> }
> } else {
> - cc_src = imx_media_find_mbus_format(fmt_src.format.code,
> + cc_src = imx_media_find_mbus_format(fmt_src->format.code,
> CS_SEL_ANY, true);
> if (WARN_ON(!cc_src))
> return -EINVAL;
> @@ -239,15 +232,32 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> cc = cc_src;
> }
>
> - imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, &fmt_src.format, cc);
> + imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, &fmt_src->format, cc);
>
> return 0;
> }
>
> +static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct capture_priv *priv = video_drvdata(file);
> + struct v4l2_subdev_format fmt_src;
> + int ret;
> +
> + fmt_src.pad = priv->src_sd_pad;
> + fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
> + if (ret)
> + return ret;
> +
> + return __capture_try_fmt(priv, &fmt_src, f);
typo: return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
> +}
> +
> static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> struct v4l2_format *f)
> {
> struct capture_priv *priv = video_drvdata(file);
> + struct v4l2_subdev_format fmt_src;
> int ret;
>
> if (vb2_is_busy(&priv->q)) {
> @@ -255,7 +265,13 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> return -EBUSY;
> }
>
> - ret = capture_try_fmt_vid_cap(file, priv, f);
> + fmt_src.pad = priv->src_sd_pad;
> + fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
> + if (ret)
> + return ret;
> +
> + ret = __capture_try_fmt_vid_cap(priv, &fmt_src, f);
> if (ret)
> return ret;
>
> @@ -264,8 +280,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> 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;
> + priv->vdev.compose.width = fmt_src.width;
> + priv->vdev.compose.height = fmt_src.height;
>
> return 0;
> }
> @@ -307,9 +323,14 @@ static int capture_g_selection(struct file *file, void *fh,
> 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;
> + case V4L2_SEL_TGT_COMPOSE_PADDED:
> + s->r.left = 0;
> + s->r.top = 0;
> + s->r.width = priv->vdev.fmt.fmt.pix.width;
> + s->r.height = priv->vdev.fmt.fmt.pix.height;
> + break;
> default:
> return -EINVAL;
> }
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] media: imx: set compose rectangle to mbus format
2018-11-09 5:33 ` Steve Longerbeam
@ 2018-11-09 13:09 ` Philipp Zabel
0 siblings, 0 replies; 14+ messages in thread
From: Philipp Zabel @ 2018-11-09 13:09 UTC (permalink / raw)
To: Steve Longerbeam, linux-media; +Cc: Hans Verkuil
On Thu, 2018-11-08 at 21:33 -0800, Steve Longerbeam wrote:
> Hi Philipp,
>
> On 11/5/18 7:20 AM, Philipp Zabel wrote:
> > Prepare for mbus format being smaller than the written rectangle
> > due to burst size.
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > drivers/staging/media/imx/imx-media-capture.c | 55 +++++++++++++------
> > 1 file changed, 38 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> > index cace8a51aca8..2d49d9573056 100644
> > --- a/drivers/staging/media/imx/imx-media-capture.c
> > +++ b/drivers/staging/media/imx/imx-media-capture.c
> > @@ -203,21 +203,14 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
> > return 0;
> > }
> >
> > -static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> > - struct v4l2_format *f)
> > +static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
> > + struct v4l2_subev_format *fmt_src,
>
>
> typo: struct v4l2_subdev_format *fmt_src,
Fixed, thanks.
[...]
> >
> > + return __capture_try_fmt(priv, &fmt_src, f);
>
>
> typo: return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
And thanks. Looks like I've misplaced a fixup! patch.
regards
Philipp
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] media: imx: lift CSI width alignment restriction
2018-11-05 15:20 [PATCH 1/3] media: imx: add capture compose rectangle Philipp Zabel
2018-11-05 15:20 ` [PATCH 2/3] media: imx: set compose rectangle to mbus format Philipp Zabel
@ 2018-11-05 15:20 ` Philipp Zabel
2018-11-09 5:46 ` Steve Longerbeam
2018-11-06 14:01 ` [PATCH 1/3] media: imx: add capture compose rectangle Sakari Ailus
2018-11-09 5:33 ` Steve Longerbeam
3 siblings, 1 reply; 14+ messages in thread
From: Philipp Zabel @ 2018-11-05 15:20 UTC (permalink / raw)
To: linux-media; +Cc: Steve Longerbeam, Hans Verkuil
The CSI subdevice shouldn't have to care about IDMAC line start
address alignment. With compose rectangle support in the capture
driver, it doesn't have to anymore.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/staging/media/imx/imx-media-capture.c | 9 ++++-----
drivers/staging/media/imx/imx-media-csi.c | 2 +-
drivers/staging/media/imx/imx-media-utils.c | 15 ++++++++++++---
3 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index 2d49d9573056..f87d6e8019e5 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -204,10 +204,9 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
}
static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
- struct v4l2_subev_format *fmt_src,
+ struct v4l2_subdev_format *fmt_src,
struct v4l2_format *f)
{
- struct capture_priv *priv = video_drvdata(file);
const struct imx_media_pixfmt *cc, *cc_src;
cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
@@ -250,7 +249,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
if (ret)
return ret;
- return __capture_try_fmt(priv, &fmt_src, f);
+ return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
}
static int capture_s_fmt_vid_cap(struct file *file, void *fh,
@@ -280,8 +279,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
CS_SEL_ANY, true);
priv->vdev.compose.left = 0;
priv->vdev.compose.top = 0;
- priv->vdev.compose.width = fmt_src.width;
- priv->vdev.compose.height = fmt_src.height;
+ priv->vdev.compose.width = fmt_src.format.width;
+ priv->vdev.compose.height = fmt_src.format.height;
return 0;
}
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index c4523afe7b48..d39682192a67 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -41,7 +41,7 @@
#define MIN_H 144
#define MAX_W 4096
#define MAX_H 4096
-#define W_ALIGN 4 /* multiple of 16 pixels */
+#define W_ALIGN 1 /* multiple of 2 pixels */
#define H_ALIGN 1 /* multiple of 2 lines */
#define S_ALIGN 1 /* multiple of 2 */
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 0eaa353d5cb3..5f110d90a4ef 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -580,6 +580,7 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
struct v4l2_mbus_framefmt *mbus,
const struct imx_media_pixfmt *cc)
{
+ u32 width;
u32 stride;
if (!cc) {
@@ -602,9 +603,16 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
cc = imx_media_find_mbus_format(code, CS_SEL_YUV, false);
}
- stride = cc->planar ? mbus->width : (mbus->width * cc->bpp) >> 3;
+ /* Round up width for minimum burst size */
+ width = round_up(mbus->width, 8);
- pix->width = mbus->width;
+ /* Round up stride for IDMAC line start address alignment */
+ if (cc->planar)
+ stride = round_up(width, 16);
+ else
+ stride = round_up((width * cc->bpp) >> 3, 8);
+
+ pix->width = width;
pix->height = mbus->height;
pix->pixelformat = cc->fourcc;
pix->colorspace = mbus->colorspace;
@@ -613,7 +621,8 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
pix->quantization = mbus->quantization;
pix->field = mbus->field;
pix->bytesperline = stride;
- pix->sizeimage = (pix->width * pix->height * cc->bpp) >> 3;
+ pix->sizeimage = cc->planar ? ((stride * pix->height * cc->bpp) >> 3) :
+ stride * pix->height;
return 0;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] media: imx: lift CSI width alignment restriction
2018-11-05 15:20 ` [PATCH 3/3] media: imx: lift CSI width alignment restriction Philipp Zabel
@ 2018-11-09 5:46 ` Steve Longerbeam
2018-11-09 13:30 ` Philipp Zabel
2018-11-09 14:50 ` Philipp Zabel
0 siblings, 2 replies; 14+ messages in thread
From: Steve Longerbeam @ 2018-11-09 5:46 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: Hans Verkuil
On 11/5/18 7:20 AM, Philipp Zabel wrote:
> The CSI subdevice shouldn't have to care about IDMAC line start
> address alignment. With compose rectangle support in the capture
> driver, it doesn't have to anymore.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> drivers/staging/media/imx/imx-media-capture.c | 9 ++++-----
> drivers/staging/media/imx/imx-media-csi.c | 2 +-
> drivers/staging/media/imx/imx-media-utils.c | 15 ++++++++++++---
> 3 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index 2d49d9573056..f87d6e8019e5 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -204,10 +204,9 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
> }
>
> static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
> - struct v4l2_subev_format *fmt_src,
> + struct v4l2_subdev_format *fmt_src,
> struct v4l2_format *f)
> {
> - struct capture_priv *priv = video_drvdata(file);
> const struct imx_media_pixfmt *cc, *cc_src;
>
> cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
> @@ -250,7 +249,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> if (ret)
> return ret;
>
> - return __capture_try_fmt(priv, &fmt_src, f);
> + return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
> }
>
> static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> @@ -280,8 +279,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> CS_SEL_ANY, true);
> priv->vdev.compose.left = 0;
> priv->vdev.compose.top = 0;
> - priv->vdev.compose.width = fmt_src.width;
> - priv->vdev.compose.height = fmt_src.height;
> + priv->vdev.compose.width = fmt_src.format.width;
> + priv->vdev.compose.height = fmt_src.format.height;
>
> return 0;
> }
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index c4523afe7b48..d39682192a67 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -41,7 +41,7 @@
> #define MIN_H 144
> #define MAX_W 4096
> #define MAX_H 4096
> -#define W_ALIGN 4 /* multiple of 16 pixels */
> +#define W_ALIGN 1 /* multiple of 2 pixels */
This works for the IDMAC output pad because the channel's cpmem width
and stride can be rounded up, but width align at the CSI sink still
needs to be 8 pixels when directed to the IC via the CSI_SRC_PAD_DIRECT
pad, in order to support the 8x8 block rotator in the IC PRP, and
there's no way AFAIK to do the same trick of rounding up width and
stride for non-IDMAC direct paths through the IPU.
Also, the imx-ic-prpencvf.c W_ALIGN_SRC can be relaxed to 2 pixels as well.
Steve
> #define H_ALIGN 1 /* multiple of 2 lines */
> #define S_ALIGN 1 /* multiple of 2 */
>
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 0eaa353d5cb3..5f110d90a4ef 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -580,6 +580,7 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> struct v4l2_mbus_framefmt *mbus,
> const struct imx_media_pixfmt *cc)
> {
> + u32 width;
> u32 stride;
>
> if (!cc) {
> @@ -602,9 +603,16 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> cc = imx_media_find_mbus_format(code, CS_SEL_YUV, false);
> }
>
> - stride = cc->planar ? mbus->width : (mbus->width * cc->bpp) >> 3;
> + /* Round up width for minimum burst size */
> + width = round_up(mbus->width, 8);
>
> - pix->width = mbus->width;
> + /* Round up stride for IDMAC line start address alignment */
> + if (cc->planar)
> + stride = round_up(width, 16);
> + else
> + stride = round_up((width * cc->bpp) >> 3, 8);
> +
> + pix->width = width;
> pix->height = mbus->height;
> pix->pixelformat = cc->fourcc;
> pix->colorspace = mbus->colorspace;
> @@ -613,7 +621,8 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> pix->quantization = mbus->quantization;
> pix->field = mbus->field;
> pix->bytesperline = stride;
> - pix->sizeimage = (pix->width * pix->height * cc->bpp) >> 3;
> + pix->sizeimage = cc->planar ? ((stride * pix->height * cc->bpp) >> 3) :
> + stride * pix->height;
>
> return 0;
> }
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] media: imx: lift CSI width alignment restriction
2018-11-09 5:46 ` Steve Longerbeam
@ 2018-11-09 13:30 ` Philipp Zabel
2018-11-09 14:50 ` Philipp Zabel
1 sibling, 0 replies; 14+ messages in thread
From: Philipp Zabel @ 2018-11-09 13:30 UTC (permalink / raw)
To: Steve Longerbeam, linux-media; +Cc: Hans Verkuil
Hi Steve,
On Thu, 2018-11-08 at 21:46 -0800, Steve Longerbeam wrote:
> > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> > index c4523afe7b48..d39682192a67 100644
> > --- a/drivers/staging/media/imx/imx-media-csi.c
> > +++ b/drivers/staging/media/imx/imx-media-csi.c
> > @@ -41,7 +41,7 @@
> > #define MIN_H 144
> > #define MAX_W 4096
> > #define MAX_H 4096
> > -#define W_ALIGN 4 /* multiple of 16 pixels */
> > +#define W_ALIGN 1 /* multiple of 2 pixels */
>
> This works for the IDMAC output pad because the channel's cpmem width
> and stride can be rounded up, but width align at the CSI sink still
> needs to be 8 pixels when directed to the IC via the CSI_SRC_PAD_DIRECT
> pad, in order to support the 8x8 block rotator in the IC PRP, and
> there's no way AFAIK to do the same trick of rounding up width andq
> stride for non-IDMAC direct paths through the IPU.
Can't we just disallow rotation on prp subdevs if sink format is not
aligned to 2^3? Another possibility would be to align sink pad format
width to 2^3 only if the PAD_DIRECT link is enabled.
> Also, the imx-ic-prpencvf.c W_ALIGN_SRC can be relaxed to 2 pixels as
> well.
True, added for v2.
regards
Philipp
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] media: imx: lift CSI width alignment restriction
2018-11-09 5:46 ` Steve Longerbeam
2018-11-09 13:30 ` Philipp Zabel
@ 2018-11-09 14:50 ` Philipp Zabel
2018-11-09 17:45 ` Steve Longerbeam
1 sibling, 1 reply; 14+ messages in thread
From: Philipp Zabel @ 2018-11-09 14:50 UTC (permalink / raw)
To: Steve Longerbeam, linux-media; +Cc: Hans Verkuil
On Thu, 2018-11-08 at 21:46 -0800, Steve Longerbeam wrote:
> On 11/5/18 7:20 AM, Philipp Zabel wrote:
> > The CSI subdevice shouldn't have to care about IDMAC line start
> > address alignment. With compose rectangle support in the capture
> > driver, it doesn't have to anymore.
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > drivers/staging/media/imx/imx-media-capture.c | 9 ++++-----
> > drivers/staging/media/imx/imx-media-csi.c | 2 +-
> > drivers/staging/media/imx/imx-media-utils.c | 15 ++++++++++++---
> > 3 files changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> > index 2d49d9573056..f87d6e8019e5 100644
> > --- a/drivers/staging/media/imx/imx-media-capture.c
> > +++ b/drivers/staging/media/imx/imx-media-capture.c
> > @@ -204,10 +204,9 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
> > }
> >
> > static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
> > - struct v4l2_subev_format *fmt_src,
> > + struct v4l2_subdev_format *fmt_src,
> > struct v4l2_format *f)
> > {
> > - struct capture_priv *priv = video_drvdata(file);
> > const struct imx_media_pixfmt *cc, *cc_src;
> >
> > cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
> > @@ -250,7 +249,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> > if (ret)
> > return ret;
> >
> > - return __capture_try_fmt(priv, &fmt_src, f);
> > + return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
> > }
> >
> > static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> > @@ -280,8 +279,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> > CS_SEL_ANY, true);
> > priv->vdev.compose.left = 0;
> > priv->vdev.compose.top = 0;
> > - priv->vdev.compose.width = fmt_src.width;
> > - priv->vdev.compose.height = fmt_src.height;
> > + priv->vdev.compose.width = fmt_src.format.width;
> > + priv->vdev.compose.height = fmt_src.format.height;
> >
> > return 0;
> > }
> > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> > index c4523afe7b48..d39682192a67 100644
> > --- a/drivers/staging/media/imx/imx-media-csi.c
> > +++ b/drivers/staging/media/imx/imx-media-csi.c
> > @@ -41,7 +41,7 @@
> > #define MIN_H 144
> > #define MAX_W 4096
> > #define MAX_H 4096
> > -#define W_ALIGN 4 /* multiple of 16 pixels */
> > +#define W_ALIGN 1 /* multiple of 2 pixels */
>
>
> This works for the IDMAC output pad because the channel's cpmem width
> and stride can be rounded up, but width align at the CSI sink still
> needs to be 8 pixels when directed to the IC via the CSI_SRC_PAD_DIRECT
> pad, in order to support the 8x8 block rotator in the IC PRP, and
> there's no way AFAIK to do the same trick of rounding up width and
> stride for non-IDMAC direct paths through the IPU.
Actually, this is not necessary at all. csi_try_crop takes care of this
by setting:
crop->width &= ~0x7;
Which is then used to set compose rectangle and source pad formats.
So this should be relaxed as well, if the SRC_DIRECT pad is not enabled.
And further, I think there is no reason to align crop->left to multiples
of 4 pixels?
regards
Philipp
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] media: imx: lift CSI width alignment restriction
2018-11-09 14:50 ` Philipp Zabel
@ 2018-11-09 17:45 ` Steve Longerbeam
0 siblings, 0 replies; 14+ messages in thread
From: Steve Longerbeam @ 2018-11-09 17:45 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: Hans Verkuil
On 11/9/18 6:50 AM, Philipp Zabel wrote:
> On Thu, 2018-11-08 at 21:46 -0800, Steve Longerbeam wrote:
>> On 11/5/18 7:20 AM, Philipp Zabel wrote:
>>> The CSI subdevice shouldn't have to care about IDMAC line start
>>> address alignment. With compose rectangle support in the capture
>>> driver, it doesn't have to anymore.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>> drivers/staging/media/imx/imx-media-capture.c | 9 ++++-----
>>> drivers/staging/media/imx/imx-media-csi.c | 2 +-
>>> drivers/staging/media/imx/imx-media-utils.c | 15 ++++++++++++---
>>> 3 files changed, 17 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
>>> index 2d49d9573056..f87d6e8019e5 100644
>>> --- a/drivers/staging/media/imx/imx-media-capture.c
>>> +++ b/drivers/staging/media/imx/imx-media-capture.c
>>> @@ -204,10 +204,9 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
>>> }
>>>
>>> static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
>>> - struct v4l2_subev_format *fmt_src,
>>> + struct v4l2_subdev_format *fmt_src,
>>> struct v4l2_format *f)
>>> {
>>> - struct capture_priv *priv = video_drvdata(file);
>>> const struct imx_media_pixfmt *cc, *cc_src;
>>>
>>> cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
>>> @@ -250,7 +249,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
>>> if (ret)
>>> return ret;
>>>
>>> - return __capture_try_fmt(priv, &fmt_src, f);
>>> + return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
>>> }
>>>
>>> static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>>> @@ -280,8 +279,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>>> CS_SEL_ANY, true);
>>> priv->vdev.compose.left = 0;
>>> priv->vdev.compose.top = 0;
>>> - priv->vdev.compose.width = fmt_src.width;
>>> - priv->vdev.compose.height = fmt_src.height;
>>> + priv->vdev.compose.width = fmt_src.format.width;
>>> + priv->vdev.compose.height = fmt_src.format.height;
>>>
>>> return 0;
>>> }
>>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
>>> index c4523afe7b48..d39682192a67 100644
>>> --- a/drivers/staging/media/imx/imx-media-csi.c
>>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>>> @@ -41,7 +41,7 @@
>>> #define MIN_H 144
>>> #define MAX_W 4096
>>> #define MAX_H 4096
>>> -#define W_ALIGN 4 /* multiple of 16 pixels */
>>> +#define W_ALIGN 1 /* multiple of 2 pixels */
>>
>> This works for the IDMAC output pad because the channel's cpmem width
>> and stride can be rounded up, but width align at the CSI sink still
>> needs to be 8 pixels when directed to the IC via the CSI_SRC_PAD_DIRECT
>> pad, in order to support the 8x8 block rotator in the IC PRP, and
>> there's no way AFAIK to do the same trick of rounding up width and
>> stride for non-IDMAC direct paths through the IPU.
> Actually, this is not necessary at all. csi_try_crop takes care of this
> by setting:
> crop->width &= ~0x7;
> Which is then used to set compose rectangle and source pad formats.
Ah you are right, I had forgotten about that line.
>
> So this should be relaxed as well, if the SRC_DIRECT pad is not enabled.
Agreed, crop->width align can be relaxed but only if SRC_DIRECT pad
not enabled.
> And further, I think there is no reason to align crop->left to multiples
> of 4 pixels?
Honestly, I don't know the reason for that either. Along with crop->top
this defines the CSI active image frame position (HSC/VSC fields in
IPU_CSI_OUT_FRM_CTRL register), and I don't see any h/w restrictions on
HSC in the ref manual. I do remember that this alignment exists in
FSL/NXP BSP's though, so maybe it is an undocumented restriction.
Steve
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] media: imx: add capture compose rectangle
2018-11-05 15:20 [PATCH 1/3] media: imx: add capture compose rectangle Philipp Zabel
2018-11-05 15:20 ` [PATCH 2/3] media: imx: set compose rectangle to mbus format Philipp Zabel
2018-11-05 15:20 ` [PATCH 3/3] media: imx: lift CSI width alignment restriction Philipp Zabel
@ 2018-11-06 14:01 ` Sakari Ailus
2018-11-06 14:44 ` Philipp Zabel
2018-11-09 5:33 ` Steve Longerbeam
3 siblings, 1 reply; 14+ messages in thread
From: Sakari Ailus @ 2018-11-06 14:01 UTC (permalink / raw)
To: Philipp Zabel; +Cc: linux-media, Steve Longerbeam, Hans Verkuil
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 <p.zabel@pengutronix.de>
> ---
> 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 <sakari.ailus@linux.intel.com>
> + 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;
> - image.rect.height = image.pix.height;
> + image.rect.height /= 2;
> image.phys0 = phys0;
> image.phys1 = phys1;
>
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index bc7feb81937c..7a0e658753f0 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -80,6 +80,8 @@ struct imx_media_video_dev {
>
> /* the user format */
> struct v4l2_format fmt;
> + /* the compose rectangle */
> + struct v4l2_rect compose;
> const struct imx_media_pixfmt *cc;
>
> /* links this vdev to master list */
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] media: imx: add capture compose rectangle
2018-11-06 14:01 ` [PATCH 1/3] media: imx: add capture compose rectangle Sakari Ailus
@ 2018-11-06 14:44 ` Philipp Zabel
2018-11-07 11:45 ` Sakari Ailus
0 siblings, 1 reply; 14+ messages in thread
From: Philipp Zabel @ 2018-11-06 14:44 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, Steve Longerbeam, Hans Verkuil
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 <sakari.ailus@linux.intel.com>
Thank you, I'll remove that line.
regards
Philipp
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] media: imx: add capture compose rectangle
2018-11-06 14:44 ` Philipp Zabel
@ 2018-11-07 11:45 ` Sakari Ailus
0 siblings, 0 replies; 14+ messages in thread
From: Sakari Ailus @ 2018-11-07 11:45 UTC (permalink / raw)
To: Philipp Zabel; +Cc: linux-media, Steve Longerbeam, Hans Verkuil
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.ailus@iki.fi
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] media: imx: add capture compose rectangle
2018-11-05 15:20 [PATCH 1/3] media: imx: add capture compose rectangle Philipp Zabel
` (2 preceding siblings ...)
2018-11-06 14:01 ` [PATCH 1/3] media: imx: add capture compose rectangle Sakari Ailus
@ 2018-11-09 5:33 ` Steve Longerbeam
2018-11-09 13:00 ` Philipp Zabel
3 siblings, 1 reply; 14+ messages in thread
From: Steve Longerbeam @ 2018-11-09 5:33 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: Hans Verkuil
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 <p.zabel@pengutronix.de>
> ---
> 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,
>
> 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;
> - image.rect.height = image.pix.height;
> + image.rect.height /= 2;
> image.phys0 = phys0;
> image.phys1 = phys1;
>
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index bc7feb81937c..7a0e658753f0 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -80,6 +80,8 @@ struct imx_media_video_dev {
>
> /* the user format */
> struct v4l2_format fmt;
> + /* the compose rectangle */
> + struct v4l2_rect compose;
> const struct imx_media_pixfmt *cc;
>
> /* links this vdev to master list */
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] media: imx: add capture compose rectangle
2018-11-09 5:33 ` Steve Longerbeam
@ 2018-11-09 13:00 ` Philipp Zabel
0 siblings, 0 replies; 14+ messages in thread
From: Philipp Zabel @ 2018-11-09 13:00 UTC (permalink / raw)
To: Steve Longerbeam, linux-media; +Cc: Hans Verkuil
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
^ permalink raw reply [flat|nested] 14+ messages in thread