| Message ID | 20190109110831.23395-3-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] media: imx: add capture compose rectangle | expand |
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; > }
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
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
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; }
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(-)