* [PATCH v2 2/3] media: imx: set compose rectangle to mbus format
2019-01-09 11:08 [PATCH v2 1/3] media: imx: add capture compose rectangle Philipp Zabel
@ 2019-01-09 11:08 ` Philipp Zabel
2019-01-10 0:09 ` Steve Longerbeam
2019-01-09 11:08 ` [PATCH v2 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction Philipp Zabel
2019-01-10 0:08 ` [PATCH v2 1/3] media: imx: add capture compose rectangle Steve Longerbeam
2 siblings, 1 reply; 8+ messages in thread
From: Philipp Zabel @ 2019-01-09 11:08 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil, Steve Longerbeam, Sakari Ailus, kernel
Prepare for mbus format being smaller than the written rectangle
due to burst size.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
- Typo fixups, this time applied to the correct patch.
---
drivers/staging/media/imx/imx-media-capture.c | 56 +++++++++++++------
1 file changed, 38 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index fb985e68f9ab..614e335fb61c 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -203,21 +203,13 @@ 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_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 +223,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 +231,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_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 +264,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 +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 = f->fmt.pix.width;
- priv->vdev.compose.height = f->fmt.pix.height;
+ priv->vdev.compose.width = fmt_src.format.width;
+ priv->vdev.compose.height = fmt_src.format.height;
return 0;
}
@@ -306,9 +321,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.20.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 2/3] media: imx: set compose rectangle to mbus format
2019-01-09 11:08 ` [PATCH v2 2/3] media: imx: set compose rectangle to mbus format Philipp Zabel
@ 2019-01-10 0:09 ` Steve Longerbeam
0 siblings, 0 replies; 8+ messages in thread
From: Steve Longerbeam @ 2019-01-10 0:09 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: Hans Verkuil, Sakari Ailus, kernel
Reviewed-by: Steve Longerbeam <slongerbeam@gmail.com>
On 1/9/19 3:08 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>
> ---
> Changes since v1:
> - Typo fixups, this time applied to the correct patch.
> ---
> drivers/staging/media/imx/imx-media-capture.c | 56 +++++++++++++------
> 1 file changed, 38 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index fb985e68f9ab..614e335fb61c 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -203,21 +203,13 @@ 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_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 +223,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 +231,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_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 +264,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 +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 = f->fmt.pix.width;
> - priv->vdev.compose.height = f->fmt.pix.height;
> + priv->vdev.compose.width = fmt_src.format.width;
> + priv->vdev.compose.height = fmt_src.format.height;
>
> return 0;
> }
> @@ -306,9 +321,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] 8+ messages in thread
* [PATCH v2 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction
2019-01-09 11:08 [PATCH v2 1/3] media: imx: add capture compose rectangle Philipp Zabel
2019-01-09 11:08 ` [PATCH v2 2/3] media: imx: set compose rectangle to mbus format Philipp Zabel
@ 2019-01-09 11:08 ` Philipp Zabel
2019-01-09 19:21 ` Steve Longerbeam
2019-01-10 0:08 ` [PATCH v2 1/3] media: imx: add capture compose rectangle Steve Longerbeam
2 siblings, 1 reply; 8+ messages in thread
From: Philipp Zabel @ 2019-01-09 11:08 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil, Steve Longerbeam, Sakari Ailus, kernel
The CSI, PRP ENC, and PRP VF subdevices shouldn't have to care about
IDMAC line start address alignment. With compose rectangle support in
the capture driver, they don't have to anymore.
If the direct CSI -> IC path is enabled, the CSI output width must
still be aligned to 8 pixels (IC burst length).
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
- Relax PRP ENC and PRP VF source pad width alignment as well
- Relax CSI crop width alignment to 2 pixels if direct CSI -> IC path
is not enabled
---
drivers/staging/media/imx/imx-ic-prpencvf.c | 2 +-
drivers/staging/media/imx/imx-media-csi.c | 21 +++++++++++++++++++--
drivers/staging/media/imx/imx-media-utils.c | 15 ++++++++++++---
3 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index fe5a77baa592..7bb754cb703e 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -48,7 +48,7 @@
#define MAX_W_SRC 1024
#define MAX_H_SRC 1024
-#define W_ALIGN_SRC 4 /* multiple of 16 pixels */
+#define W_ALIGN_SRC 1 /* multiple of 2 pixels */
#define H_ALIGN_SRC 1 /* multiple of 2 lines */
#define S_ALIGN 1 /* multiple of 2 */
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index c4523afe7b48..1b4962b8b192 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 */
@@ -1130,6 +1130,20 @@ __csi_get_compose(struct csi_priv *priv, struct v4l2_subdev_pad_config *cfg,
return &priv->compose;
}
+static bool csi_src_pad_enabled(struct media_pad *pad)
+{
+ struct media_link *link;
+
+ list_for_each_entry(link, &pad->entity->links, list) {
+ if (link->source->entity == pad->entity &&
+ link->source->index == pad->index &&
+ link->flags & MEDIA_LNK_FL_ENABLED)
+ return true;
+ }
+
+ return false;
+}
+
static void csi_try_crop(struct csi_priv *priv,
struct v4l2_rect *crop,
struct v4l2_subdev_pad_config *cfg,
@@ -1141,7 +1155,10 @@ static void csi_try_crop(struct csi_priv *priv,
crop->left = infmt->width - crop->width;
/* adjust crop left/width to h/w alignment restrictions */
crop->left &= ~0x3;
- crop->width &= ~0x7;
+ if (csi_src_pad_enabled(&priv->pad[CSI_SRC_PAD_DIRECT]))
+ crop->width &= ~0x7; /* multiple of 8 pixels (IC burst) */
+ else
+ crop->width &= ~0x1; /* multiple of 2 pixels */
/*
* FIXME: not sure why yet, but on interlaced bt.656,
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.20.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction
2019-01-09 11:08 ` [PATCH v2 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction Philipp Zabel
@ 2019-01-09 19:21 ` Steve Longerbeam
2019-01-10 10:54 ` Philipp Zabel
0 siblings, 1 reply; 8+ messages in thread
From: Steve Longerbeam @ 2019-01-09 19:21 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: Hans Verkuil, Sakari Ailus, kernel
On 1/9/19 3:08 AM, Philipp Zabel wrote:
> The CSI, PRP ENC, and PRP VF subdevices shouldn't have to care about
> IDMAC line start address alignment. With compose rectangle support in
> the capture driver, they don't have to anymore.
> If the direct CSI -> IC path is enabled, the CSI output width must
> still be aligned to 8 pixels (IC burst length).
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v1:
> - Relax PRP ENC and PRP VF source pad width alignment as well
> - Relax CSI crop width alignment to 2 pixels if direct CSI -> IC path
> is not enabled
> ---
> drivers/staging/media/imx/imx-ic-prpencvf.c | 2 +-
> drivers/staging/media/imx/imx-media-csi.c | 21 +++++++++++++++++++--
> drivers/staging/media/imx/imx-media-utils.c | 15 ++++++++++++---
> 3 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index fe5a77baa592..7bb754cb703e 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -48,7 +48,7 @@
>
> #define MAX_W_SRC 1024
> #define MAX_H_SRC 1024
> -#define W_ALIGN_SRC 4 /* multiple of 16 pixels */
> +#define W_ALIGN_SRC 1 /* multiple of 2 pixels */
> #define H_ALIGN_SRC 1 /* multiple of 2 lines */
>
> #define S_ALIGN 1 /* multiple of 2 */
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index c4523afe7b48..1b4962b8b192 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 */
>
> @@ -1130,6 +1130,20 @@ __csi_get_compose(struct csi_priv *priv, struct v4l2_subdev_pad_config *cfg,
> return &priv->compose;
> }
>
> +static bool csi_src_pad_enabled(struct media_pad *pad)
> +{
> + struct media_link *link;
> +
> + list_for_each_entry(link, &pad->entity->links, list) {
> + if (link->source->entity == pad->entity &&
> + link->source->index == pad->index &&
> + link->flags & MEDIA_LNK_FL_ENABLED)
> + return true;
> + }
> +
> + return false;
> +}
I don't think this function is needed, first it is basically equivalent to
media_entity_remote_pad(), but also...
> +
> static void csi_try_crop(struct csi_priv *priv,
> struct v4l2_rect *crop,
> struct v4l2_subdev_pad_config *cfg,
> @@ -1141,7 +1155,10 @@ static void csi_try_crop(struct csi_priv *priv,
> crop->left = infmt->width - crop->width;
> /* adjust crop left/width to h/w alignment restrictions */
> crop->left &= ~0x3;
> - crop->width &= ~0x7;
> + if (csi_src_pad_enabled(&priv->pad[CSI_SRC_PAD_DIRECT]))
why not just use "if (priv->active_output_pad == CSI_SRC_PAD_DIRECT) ..." ?
Steve
> + crop->width &= ~0x7; /* multiple of 8 pixels (IC burst) */
> + else
> + crop->width &= ~0x1; /* multiple of 2 pixels */
>
> /*
> * FIXME: not sure why yet, but on interlaced bt.656,
> 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] 8+ messages in thread* Re: [PATCH v2 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction
2019-01-09 19:21 ` Steve Longerbeam
@ 2019-01-10 10:54 ` Philipp Zabel
2019-01-10 21:05 ` Steve Longerbeam
0 siblings, 1 reply; 8+ messages in thread
From: Philipp Zabel @ 2019-01-10 10:54 UTC (permalink / raw)
To: Steve Longerbeam, linux-media; +Cc: Hans Verkuil, Sakari Ailus, kernel
On Wed, 2019-01-09 at 11:21 -0800, Steve Longerbeam wrote:
>
> On 1/9/19 3:08 AM, Philipp Zabel wrote:
> > The CSI, PRP ENC, and PRP VF subdevices shouldn't have to care about
> > IDMAC line start address alignment. With compose rectangle support in
> > the capture driver, they don't have to anymore.
> > If the direct CSI -> IC path is enabled, the CSI output width must
> > still be aligned to 8 pixels (IC burst length).
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Changes since v1:
> > - Relax PRP ENC and PRP VF source pad width alignment as well
> > - Relax CSI crop width alignment to 2 pixels if direct CSI -> IC path
> > is not enabled
> > ---
> > drivers/staging/media/imx/imx-ic-prpencvf.c | 2 +-
> > drivers/staging/media/imx/imx-media-csi.c | 21 +++++++++++++++++++--
> > drivers/staging/media/imx/imx-media-utils.c | 15 ++++++++++++---
> > 3 files changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> > index fe5a77baa592..7bb754cb703e 100644
> > --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> > +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> > @@ -48,7 +48,7 @@
> >
> > #define MAX_W_SRC 1024
> > #define MAX_H_SRC 1024
> > -#define W_ALIGN_SRC 4 /* multiple of 16 pixels */
> > +#define W_ALIGN_SRC 1 /* multiple of 2 pixels */
> > #define H_ALIGN_SRC 1 /* multiple of 2 lines */
> >
> > #define S_ALIGN 1 /* multiple of 2 */
> > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> > index c4523afe7b48..1b4962b8b192 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 */
> >
> > @@ -1130,6 +1130,20 @@ __csi_get_compose(struct csi_priv *priv, struct v4l2_subdev_pad_config *cfg,
> > return &priv->compose;
> > }
> >
> > +static bool csi_src_pad_enabled(struct media_pad *pad)
> > +{
> > + struct media_link *link;
> > +
> > + list_for_each_entry(link, &pad->entity->links, list) {
> > + if (link->source->entity == pad->entity &&
> > + link->source->index == pad->index &&
> > + link->flags & MEDIA_LNK_FL_ENABLED)
> > + return true;
> > + }
> > +
> > + return false;
> > +}
>
> I don't think this function is needed, first it is basically equivalent to
> media_entity_remote_pad(), but also...
>
> > +
> > static void csi_try_crop(struct csi_priv *priv,
> > struct v4l2_rect *crop,
> > struct v4l2_subdev_pad_config *cfg,
> > @@ -1141,7 +1155,10 @@ static void csi_try_crop(struct csi_priv *priv,
> > crop->left = infmt->width - crop->width;
> > /* adjust crop left/width to h/w alignment restrictions */
> > crop->left &= ~0x3;
> > - crop->width &= ~0x7;
> > + if (csi_src_pad_enabled(&priv->pad[CSI_SRC_PAD_DIRECT]))
>
> why not just use "if (priv->active_output_pad == CSI_SRC_PAD_DIRECT) ..." ?
While both source pad links are disabled, whether or not IC burst
alignment is applied would depend on hidden state. This should be
consistent, regardless of previously enabled source pad links.
We could achieve that with your suggested change if csi_link_setup()
would always set active_output_pad = CSI_SRC_PAD_IDMAC when disabling
source pad links:
----------8<----------
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index dd911313fca2..e593fd7774ff 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1000,6 +1000,8 @@ static int csi_link_setup(struct media_entity *entity,
v4l2_ctrl_handler_free(&priv->ctrl_hdlr);
v4l2_ctrl_handler_init(&priv->ctrl_hdlr, 0);
priv->sink = NULL;
+ /* do not apply IC burst alignment in csi_try_crop */
+ priv->active_output_pad = CSI_SRC_PAD_IDMAC;
goto out;
}
---------->8----------
regards
Philipp
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction
2019-01-10 10:54 ` Philipp Zabel
@ 2019-01-10 21:05 ` Steve Longerbeam
0 siblings, 0 replies; 8+ messages in thread
From: Steve Longerbeam @ 2019-01-10 21:05 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: Hans Verkuil, Sakari Ailus, kernel
On 1/10/19 2:54 AM, Philipp Zabel wrote:
> On Wed, 2019-01-09 at 11:21 -0800, Steve Longerbeam wrote:
>> <snip>
>>
>> why not just use "if (priv->active_output_pad == CSI_SRC_PAD_DIRECT) ..." ?
> While both source pad links are disabled, whether or not IC burst
> alignment is applied would depend on hidden state. This should be
> consistent, regardless of previously enabled source pad links.
Yes good point.
>
> We could achieve that with your suggested change if csi_link_setup()
> would always set active_output_pad = CSI_SRC_PAD_IDMAC when disabling
> source pad links:
>
> ----------8<----------
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index dd911313fca2..e593fd7774ff 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1000,6 +1000,8 @@ static int csi_link_setup(struct media_entity *entity,
> v4l2_ctrl_handler_free(&priv->ctrl_hdlr);
> v4l2_ctrl_handler_init(&priv->ctrl_hdlr, 0);
> priv->sink = NULL;
> + /* do not apply IC burst alignment in csi_try_crop */
> + priv->active_output_pad = CSI_SRC_PAD_IDMAC;
Yes that will work. But I also notice priv->active_output_pad is not
even being initialized. Do you mind including that as well, e.g
initialize to CSI_SRC_PAD_IDMAC in imx_csi_probe().
Thanks Philipp.
Steve
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] media: imx: add capture compose rectangle
2019-01-09 11:08 [PATCH v2 1/3] media: imx: add capture compose rectangle Philipp Zabel
2019-01-09 11:08 ` [PATCH v2 2/3] media: imx: set compose rectangle to mbus format Philipp Zabel
2019-01-09 11:08 ` [PATCH v2 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction Philipp Zabel
@ 2019-01-10 0:08 ` Steve Longerbeam
2 siblings, 0 replies; 8+ messages in thread
From: Steve Longerbeam @ 2019-01-10 0:08 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: Hans Verkuil, Sakari Ailus, kernel
Reviewed-by: Steve Longerbeam <slongerbeam@gmail.com>
On 1/9/19 3:08 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>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Changes since v1:
> - remove NATIVE_SIZE selection target
> - fixed typo in capture_s_fmt_vid_cap
> ---
> drivers/staging/media/imx/imx-ic-prpencvf.c | 3 +-
> drivers/staging/media/imx/imx-media-capture.c | 37 +++++++++++++++++++
> 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, 43 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..fb985e68f9ab 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.pix.width;
> + priv->vdev.compose.height = f->fmt.pix.height;
>
> return 0;
> }
> @@ -290,6 +294,34 @@ 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_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 +382,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 +722,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] 8+ messages in thread