From: Philipp Zabel <p.zabel@pengutronix.de> To: Steve Longerbeam <slongerbeam@gmail.com>, linux-media@vger.kernel.org Cc: Mauro Carvalho Chehab <mchehab@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>, "open list:DRM DRIVERS FOR FREESCALE IMX" <dri-devel@lists.freedesktop.org>, open list <linux-kernel@vger.kernel.org>, "open list:STAGING SUBSYSTEM" <devel@driverdev.osuosl.org>, "open list:FRAMEBUFFER LAYER" <linux-fbdev@vger.kernel.org> Subject: Re: [PATCH v4 02/11] gpu: ipu-csi: Swap fields according to input/output field types Date: Fri, 05 Oct 2018 11:44:39 +0200 [thread overview] Message-ID: <1538732679.3545.5.camel@pengutronix.de> (raw) In-Reply-To: <20181004185401.15751-3-slongerbeam@gmail.com> Hi Steve, On Thu, 2018-10-04 at 11:53 -0700, Steve Longerbeam wrote: [...] > int ipu_csi_init_interface(struct ipu_csi *csi, > struct v4l2_mbus_config *mbus_cfg, > - struct v4l2_mbus_framefmt *mbus_fmt) > + struct v4l2_mbus_framefmt *infmt, > + struct v4l2_mbus_framefmt *outfmt) > { [...] > + std = V4L2_STD_UNKNOWN; > + if (width == 720) { > + switch (height) { > + case 480: > + std = V4L2_STD_NTSC; > + break; > + case 576: > + std = V4L2_STD_PAL; > + break; > + default: > + break; > + } > + } > + > + if (std == V4L2_STD_UNKNOWN) { > dev_err(csi->ipu->dev, [...] > + "Unsupported interlaced video mode\n"); > + ret = -EINVAL; > + goto out_unlock; > } [...] > + > + /* framelines for NTSC / PAL */ > + height = (std & V4L2_STD_525_60) ? 525 : 625; I think this is a bit convoluted. Instead of initializing std, then possibly changing it, and then comparing to the inital value, and then checking it again to determine the new height, why not just: if (width == 720 && height == 480) { std = V4L2_STD_NTSC; height = 525; } else if (width == 720 && height == 576) { std = V4L2_STD_PAL; height = 625; } else { dev_err(csi->ipu->dev, "Unsupported interlaced video mode\n"); ret = -EINVAL; goto out_unlock; } ? > break; > case IPU_CSI_CLK_MODE_CCIR1120_PROGRESSIVE_DDR: > case IPU_CSI_CLK_MODE_CCIR1120_PROGRESSIVE_SDR: > @@ -476,9 +529,10 @@ int ipu_csi_init_interface(struct ipu_csi *csi, > dev_dbg(csi->ipu->dev, "CSI_ACT_FRM_SIZE = 0x%08X\n", > ipu_csi_read(csi, CSI_ACT_FRM_SIZE)); > > +out_unlock: > spin_unlock_irqrestore(&csi->lock, flags); > > - return 0; > + return ret; > } > EXPORT_SYMBOL_GPL(ipu_csi_init_interface); > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > index 4acdd7ae612b..ad66f007d395 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -666,7 +666,6 @@ static int csi_setup(struct csi_priv *priv) > struct v4l2_mbus_framefmt *infmt, *outfmt; > const struct imx_media_pixfmt *incc; > struct v4l2_mbus_config mbus_cfg; > - struct v4l2_mbus_framefmt if_fmt; > struct v4l2_rect crop; > > infmt = &priv->format_mbus[CSI_SINK_PAD]; > @@ -679,22 +678,14 @@ static int csi_setup(struct csi_priv *priv) > priv->upstream_ep.bus.parallel.flags : > priv->upstream_ep.bus.mipi_csi2.flags; > > - /* > - * we need to pass input frame to CSI interface, but > - * with translated field type from output format > - */ > - if_fmt = *infmt; > - if_fmt.field = outfmt->field; > crop = priv->crop; > > /* > * if cycles is set, we need to handle this over multiple cycles as > * generic/bayer data > */ > - if (is_parallel_bus(&priv->upstream_ep) && incc->cycles) { > - if_fmt.width *= incc->cycles; If the input format width passed to ipu_csi_init_interface is not multiplied by the number of cycles per pixel anymore, width in the CSI_SENS_FRM_SIZE register will be set to the unmultiplied value from infmt. This breaks 779680e2e793 ("media: imx: add support for RGB565_2X8 on parallel bus"). > + if (is_parallel_bus(&priv->upstream_ep) && incc->cycles) > crop.width *= incc->cycles; > - } > > ipu_csi_set_window(priv->csi, &crop); > > @@ -702,7 +693,7 @@ static int csi_setup(struct csi_priv *priv) > priv->crop.width == 2 * priv->compose.width, > priv->crop.height == 2 * priv->compose.height); > > - ipu_csi_init_interface(priv->csi, &mbus_cfg, &if_fmt); > + ipu_csi_init_interface(priv->csi, &mbus_cfg, infmt, outfmt); regards Philipp
WARNING: multiple messages have this Message-ID (diff)
From: Philipp Zabel <p.zabel@pengutronix.de> To: Steve Longerbeam <slongerbeam@gmail.com>, linux-media@vger.kernel.org Cc: Mauro Carvalho Chehab <mchehab@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>, "open list:DRM DRIVERS FOR FREESCALE IMX" <dri-devel@lists.freedesktop.org>, open list <linux-kernel@vger.kernel.org>, "open list:STAGING SUBSYSTEM" <devel@driverdev.osuosl.org>, "open list:FRAMEBUFFER LAYER" <linux-fbdev@vger.kernel.org> Subject: Re: [PATCH v4 02/11] gpu: ipu-csi: Swap fields according to input/output field types Date: Fri, 05 Oct 2018 09:44:39 +0000 [thread overview] Message-ID: <1538732679.3545.5.camel@pengutronix.de> (raw) In-Reply-To: <20181004185401.15751-3-slongerbeam@gmail.com> Hi Steve, On Thu, 2018-10-04 at 11:53 -0700, Steve Longerbeam wrote: [...] > int ipu_csi_init_interface(struct ipu_csi *csi, > struct v4l2_mbus_config *mbus_cfg, > - struct v4l2_mbus_framefmt *mbus_fmt) > + struct v4l2_mbus_framefmt *infmt, > + struct v4l2_mbus_framefmt *outfmt) > { [...] > + std = V4L2_STD_UNKNOWN; > + if (width = 720) { > + switch (height) { > + case 480: > + std = V4L2_STD_NTSC; > + break; > + case 576: > + std = V4L2_STD_PAL; > + break; > + default: > + break; > + } > + } > + > + if (std = V4L2_STD_UNKNOWN) { > dev_err(csi->ipu->dev, [...] > + "Unsupported interlaced video mode\n"); > + ret = -EINVAL; > + goto out_unlock; > } [...] > + > + /* framelines for NTSC / PAL */ > + height = (std & V4L2_STD_525_60) ? 525 : 625; I think this is a bit convoluted. Instead of initializing std, then possibly changing it, and then comparing to the inital value, and then checking it again to determine the new height, why not just: if (width = 720 && height = 480) { std = V4L2_STD_NTSC; height = 525; } else if (width = 720 && height = 576) { std = V4L2_STD_PAL; height = 625; } else { dev_err(csi->ipu->dev, "Unsupported interlaced video mode\n"); ret = -EINVAL; goto out_unlock; } ? > break; > case IPU_CSI_CLK_MODE_CCIR1120_PROGRESSIVE_DDR: > case IPU_CSI_CLK_MODE_CCIR1120_PROGRESSIVE_SDR: > @@ -476,9 +529,10 @@ int ipu_csi_init_interface(struct ipu_csi *csi, > dev_dbg(csi->ipu->dev, "CSI_ACT_FRM_SIZE = 0x%08X\n", > ipu_csi_read(csi, CSI_ACT_FRM_SIZE)); > > +out_unlock: > spin_unlock_irqrestore(&csi->lock, flags); > > - return 0; > + return ret; > } > EXPORT_SYMBOL_GPL(ipu_csi_init_interface); > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > index 4acdd7ae612b..ad66f007d395 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -666,7 +666,6 @@ static int csi_setup(struct csi_priv *priv) > struct v4l2_mbus_framefmt *infmt, *outfmt; > const struct imx_media_pixfmt *incc; > struct v4l2_mbus_config mbus_cfg; > - struct v4l2_mbus_framefmt if_fmt; > struct v4l2_rect crop; > > infmt = &priv->format_mbus[CSI_SINK_PAD]; > @@ -679,22 +678,14 @@ static int csi_setup(struct csi_priv *priv) > priv->upstream_ep.bus.parallel.flags : > priv->upstream_ep.bus.mipi_csi2.flags; > > - /* > - * we need to pass input frame to CSI interface, but > - * with translated field type from output format > - */ > - if_fmt = *infmt; > - if_fmt.field = outfmt->field; > crop = priv->crop; > > /* > * if cycles is set, we need to handle this over multiple cycles as > * generic/bayer data > */ > - if (is_parallel_bus(&priv->upstream_ep) && incc->cycles) { > - if_fmt.width *= incc->cycles; If the input format width passed to ipu_csi_init_interface is not multiplied by the number of cycles per pixel anymore, width in the CSI_SENS_FRM_SIZE register will be set to the unmultiplied value from infmt. This breaks 779680e2e793 ("media: imx: add support for RGB565_2X8 on parallel bus"). > + if (is_parallel_bus(&priv->upstream_ep) && incc->cycles) > crop.width *= incc->cycles; > - } > > ipu_csi_set_window(priv->csi, &crop); > > @@ -702,7 +693,7 @@ static int csi_setup(struct csi_priv *priv) > priv->crop.width = 2 * priv->compose.width, > priv->crop.height = 2 * priv->compose.height); > > - ipu_csi_init_interface(priv->csi, &mbus_cfg, &if_fmt); > + ipu_csi_init_interface(priv->csi, &mbus_cfg, infmt, outfmt); regards Philipp
next prev parent reply other threads:[~2018-10-05 9:44 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-10-04 18:53 [PATCH v4 00/11] imx-media: Fixes for interlaced capture Steve Longerbeam 2018-10-04 18:53 ` [PATCH v4 01/11] media: videodev2.h: Add more field helper macros Steve Longerbeam 2018-10-04 18:53 ` [PATCH v4 02/11] gpu: ipu-csi: Swap fields according to input/output field types Steve Longerbeam 2018-10-04 18:53 ` Steve Longerbeam 2018-10-04 18:53 ` Steve Longerbeam 2018-10-05 9:44 ` Philipp Zabel [this message] 2018-10-05 9:44 ` Philipp Zabel 2018-10-05 9:44 ` Philipp Zabel 2018-10-08 21:59 ` Steve Longerbeam 2018-10-08 21:59 ` Steve Longerbeam 2018-10-08 21:59 ` Steve Longerbeam 2018-10-04 18:53 ` [PATCH v4 03/11] gpu: ipu-v3: Add planar support to interlaced scan Steve Longerbeam 2018-10-04 18:53 ` Steve Longerbeam 2018-10-04 18:53 ` Steve Longerbeam 2018-10-05 9:48 ` Philipp Zabel 2018-10-05 9:48 ` Philipp Zabel 2018-10-05 9:48 ` Philipp Zabel 2018-10-05 9:48 ` Philipp Zabel 2018-10-09 0:09 ` Steve Longerbeam 2018-10-09 0:09 ` Steve Longerbeam 2018-10-09 0:09 ` Steve Longerbeam 2018-10-04 18:53 ` [PATCH v4 04/11] media: imx: Fix field negotiation Steve Longerbeam 2018-10-05 10:17 ` Philipp Zabel 2018-10-04 18:53 ` [PATCH v4 05/11] media: imx-csi: Double crop height for alternate fields at sink Steve Longerbeam 2018-10-05 10:18 ` Philipp Zabel 2018-10-04 18:53 ` [PATCH v4 06/11] media: imx: interweave and odd-chroma-row skip are incompatible Steve Longerbeam 2018-10-05 10:20 ` Philipp Zabel 2018-10-04 18:53 ` [PATCH v4 07/11] media: imx-csi: Allow skipping odd chroma rows for YVU420 Steve Longerbeam 2018-10-05 10:20 ` Philipp Zabel 2018-10-04 18:53 ` [PATCH v4 08/11] media: imx: vdic: rely on VDIC for correct field order Steve Longerbeam 2018-10-04 18:53 ` [PATCH v4 09/11] media: imx-csi: Move crop/compose reset after filling default mbus fields Steve Longerbeam 2018-10-05 10:22 ` Philipp Zabel 2018-10-04 18:54 ` [PATCH v4 10/11] media: imx: Allow interweave with top/bottom lines swapped Steve Longerbeam 2018-10-05 10:43 ` Philipp Zabel 2018-10-09 1:07 ` Steve Longerbeam 2018-10-04 18:54 ` [PATCH v4 11/11] media: imx.rst: Update doc to reflect fixes to interlaced capture Steve Longerbeam 2018-10-05 10:52 ` Philipp Zabel 2018-10-09 1:09 ` Steve Longerbeam 2018-10-04 19:34 ` [PATCH v4 00/11] imx-media: Fixes for " Hans Verkuil 2018-10-04 20:16 ` Steve Longerbeam
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1538732679.3545.5.camel@pengutronix.de \ --to=p.zabel@pengutronix.de \ --cc=b.zolnierkie@samsung.com \ --cc=devel@driverdev.osuosl.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=gregkh@linuxfoundation.org \ --cc=linux-fbdev@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=mchehab@kernel.org \ --cc=slongerbeam@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.