diff mbox series

MEDIA: i.MX6: Support 16-bit BT.1120 video input

Message ID m3tv0ivm5k.fsf@t19.piap.pl (mailing list archive)
State New, archived
Headers show
Series MEDIA: i.MX6: Support 16-bit BT.1120 video input | expand

Commit Message

Krzysztof Hałasa May 14, 2020, 10:36 a.m. UTC 
This patch adds support for BT.1120 mode (16-bit version of BT.656)
on i.MX6.

Basically, BT.1120 sends Y data over one set of 8 lines, and at the same
time sends multiplexed U/V data over another set of 8 lines. Everything
uses the same single clock input. DE signal may optionally be used,
H and V syncs are not used. The start and stop codes are the same as in
BT.656 mode, and are transmitted in both Y and U/V channels.

It appears the proper format designation for this mode is
MEDIA_BUS_FMT_YUYV8_1X16. It could be extended to support 10-bit data as
well (YUYV10_1X20), but I don't have necessary hardware to easily test
it.

Note that both these 8-bit (tested) and 10-bit modes, according to the
docs, can be used directly, without the so called "passthrough".
Also note the hardware bus width should not be used when determining the
exact transfer mode - data format should be used for this (we can have
a "narrow" device connected to a "wide" bus).

This patch assumes BT.1120 uses SDR signaling. It will need to be
modified if/when there is hw using DDR.

Signed-off-by: Krzysztof Halasa <khalasa@piap.pl>

Comments

Philipp Zabel June 26, 2020, 9:11 a.m. UTC | #1 
Hi Krzysztof,

I have some doubts about this patch, see below:

On Thu, 2020-05-14 at 12:36 +0200, Krzysztof Hałasa wrote:
> This patch adds support for BT.1120 mode (16-bit version of BT.656)
> on i.MX6.
> 
> Basically, BT.1120 sends Y data over one set of 8 lines, and at the same
> time sends multiplexed U/V data over another set of 8 lines. Everything
> uses the same single clock input. DE signal may optionally be used,
> H and V syncs are not used. The start and stop codes are the same as in
> BT.656 mode, and are transmitted in both Y and U/V channels.
> 
> It appears the proper format designation for this mode is
> MEDIA_BUS_FMT_YUYV8_1X16. It could be extended to support 10-bit data as
> well (YUYV10_1X20), but I don't have necessary hardware to easily test
> it.
> 
> Note that both these 8-bit (tested) and 10-bit modes, according to the
> docs, can be used directly, without the so called "passthrough".
> Also note the hardware bus width should not be used when determining the
> exact transfer mode - data format should be used for this (we can have
> a "narrow" device connected to a "wide" bus).
> 
> This patch assumes BT.1120 uses SDR signaling. It will need to be
> modified if/when there is hw using DDR.

How could DDR signaling be configured?

> Signed-off-by: Krzysztof Halasa <khalasa@piap.pl>
> 
> diff --git a/drivers/gpu/ipu-v3/ipu-csi.c b/drivers/gpu/ipu-v3/ipu-csi.c
> index 658c173bebdf..e352757f3f0f 100644
> --- a/drivers/gpu/ipu-v3/ipu-csi.c
> +++ b/drivers/gpu/ipu-v3/ipu-csi.c
> @@ -250,22 +250,18 @@ static int mbus_code_to_bus_cfg(struct ipu_csi_bus_config *cfg, u32 mbus_code,
>  		cfg->mipi_dt = MIPI_DT_RGB888;
>  		cfg->data_width = IPU_CSI_DATA_WIDTH_8;
>  		break;
> +	case MEDIA_BUS_FMT_UYVY8_1X16:
>  	case MEDIA_BUS_FMT_UYVY8_2X8:
>  		cfg->data_fmt = CSI_SENS_CONF_DATA_FMT_YUV422_UYVY;
>  		cfg->mipi_dt = MIPI_DT_YUV422;
>  		cfg->data_width = IPU_CSI_DATA_WIDTH_8;
>  		break;
> +	case MEDIA_BUS_FMT_YUYV8_1X16:
>  	case MEDIA_BUS_FMT_YUYV8_2X8:
>  		cfg->data_fmt = CSI_SENS_CONF_DATA_FMT_YUV422_YUYV;
>  		cfg->mipi_dt = MIPI_DT_YUV422;
>  		cfg->data_width = IPU_CSI_DATA_WIDTH_8;
>  		break;
> -	case MEDIA_BUS_FMT_UYVY8_1X16:
> -	case MEDIA_BUS_FMT_YUYV8_1X16:
> -		cfg->data_fmt = CSI_SENS_CONF_DATA_FMT_BAYER;
> -		cfg->mipi_dt = MIPI_DT_YUV422;
> -		cfg->data_width = IPU_CSI_DATA_WIDTH_16;
> -		break;

I think this will break support for devices that send 16-bit YUYV but
that use HSYNC/VSYNC instead of BT.1120 signaling. For those we can only
use passthrough mode.

> 	case MEDIA_BUS_FMT_SBGGR8_1X8:
>  	case MEDIA_BUS_FMT_SGBRG8_1X8:
>  	case MEDIA_BUS_FMT_SGRBG8_1X8:
> @@ -352,7 +348,7 @@ static int fill_csi_bus_cfg(struct ipu_csi_bus_config *csicfg,
>  			    const struct v4l2_mbus_config *mbus_cfg,
>  			    const struct v4l2_mbus_framefmt *mbus_fmt)
>  {
> -	int ret;
> +	int ret, is_bt1120;
>  
>  	memset(csicfg, 0, sizeof(*csicfg));
>  
> @@ -373,11 +369,18 @@ static int fill_csi_bus_cfg(struct ipu_csi_bus_config *csicfg,
>  		break;
>  	case V4L2_MBUS_BT656:
>  		csicfg->ext_vsync = 0;
> +		/* UYVY10_1X20 etc. could be supported as well */
> +		is_bt1120 = mbus_fmt->code == MEDIA_BUS_FMT_UYVY8_1X16 ||
> +			mbus_fmt->code == MEDIA_BUS_FMT_YUYV8_1X16;
>  		if (V4L2_FIELD_HAS_BOTH(mbus_fmt->field) ||
>  		    mbus_fmt->field == V4L2_FIELD_ALTERNATE)
> -			csicfg->clk_mode = IPU_CSI_CLK_MODE_CCIR656_INTERLACED;
> +			csicfg->clk_mode = is_bt1120 ?
> +				IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_SDR :
> +				IPU_CSI_CLK_MODE_CCIR656_INTERLACED;
>  		else
> -			csicfg->clk_mode = IPU_CSI_CLK_MODE_CCIR656_PROGRESSIVE;
> +			csicfg->clk_mode = is_bt1120 ?
> +				IPU_CSI_CLK_MODE_CCIR1120_PROGRESSIVE_SDR :
> +				IPU_CSI_CLK_MODE_CCIR656_PROGRESSIVE;
>  		break;
>  	case V4L2_MBUS_CSI2_DPHY:
>  		/*
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index d9e5388ffeb5..3d1d184a0ba7 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -123,27 +123,21 @@ static inline bool is_parallel_bus(struct v4l2_fwnode_endpoint *ep)
>  	return ep->bus_type != V4L2_MBUS_CSI2_DPHY;
>  }
>  
> -static inline bool is_parallel_16bit_bus(struct v4l2_fwnode_endpoint *ep)
> -{
> -	return is_parallel_bus(ep) && ep->bus.parallel.bus_width >= 16;
> -}
> -
>  /*
>   * Check for conditions that require the IPU to handle the
>   * data internally as generic data, aka passthrough mode:
>   * - raw bayer media bus formats, or
> - * - the CSI is receiving from a 16-bit parallel bus, or

This is still true for HSYNC/VSYNC signaling.

> - * - the CSI is receiving from an 8-bit parallel bus and the incoming
> - *   media bus format is other than UYVY8_2X8/YUYV8_2X8.
> + * - other unsupported modes such as JPEG, Y8 etc.
>   */
>  static inline bool requires_passthrough(struct v4l2_fwnode_endpoint *ep,
>  					struct v4l2_mbus_framefmt *infmt,
>  					const struct imx_media_pixfmt *incc)
>  {
> -	return incc->bayer || is_parallel_16bit_bus(ep) ||
> -		(is_parallel_bus(ep) &&
> +	return incc->bayer || (is_parallel_bus(ep) &&
>  		 infmt->code != MEDIA_BUS_FMT_UYVY8_2X8 &&
> -		 infmt->code != MEDIA_BUS_FMT_YUYV8_2X8);
> +		 infmt->code != MEDIA_BUS_FMT_UYVY8_1X16 &&
> +		 infmt->code != MEDIA_BUS_FMT_YUYV8_2X8 &&
> +		 infmt->code != MEDIA_BUS_FMT_YUYV8_1X16);

So I don't think it is that easy, we have to take into account
v4l2_mbus_config::type here.

regards
Philipp
diff mbox series

Patch

diff --git a/drivers/gpu/ipu-v3/ipu-csi.c b/drivers/gpu/ipu-v3/ipu-csi.c
index 658c173bebdf..e352757f3f0f 100644
--- a/drivers/gpu/ipu-v3/ipu-csi.c
+++ b/drivers/gpu/ipu-v3/ipu-csi.c
@@ -250,22 +250,18 @@  static int mbus_code_to_bus_cfg(struct ipu_csi_bus_config *cfg, u32 mbus_code,
 		cfg->mipi_dt = MIPI_DT_RGB888;
 		cfg->data_width = IPU_CSI_DATA_WIDTH_8;
 		break;
+	case MEDIA_BUS_FMT_UYVY8_1X16:
 	case MEDIA_BUS_FMT_UYVY8_2X8:
 		cfg->data_fmt = CSI_SENS_CONF_DATA_FMT_YUV422_UYVY;
 		cfg->mipi_dt = MIPI_DT_YUV422;
 		cfg->data_width = IPU_CSI_DATA_WIDTH_8;
 		break;
+	case MEDIA_BUS_FMT_YUYV8_1X16:
 	case MEDIA_BUS_FMT_YUYV8_2X8:
 		cfg->data_fmt = CSI_SENS_CONF_DATA_FMT_YUV422_YUYV;
 		cfg->mipi_dt = MIPI_DT_YUV422;
 		cfg->data_width = IPU_CSI_DATA_WIDTH_8;
 		break;
-	case MEDIA_BUS_FMT_UYVY8_1X16:
-	case MEDIA_BUS_FMT_YUYV8_1X16:
-		cfg->data_fmt = CSI_SENS_CONF_DATA_FMT_BAYER;
-		cfg->mipi_dt = MIPI_DT_YUV422;
-		cfg->data_width = IPU_CSI_DATA_WIDTH_16;
-		break;
 	case MEDIA_BUS_FMT_SBGGR8_1X8:
 	case MEDIA_BUS_FMT_SGBRG8_1X8:
 	case MEDIA_BUS_FMT_SGRBG8_1X8:
@@ -352,7 +348,7 @@  static int fill_csi_bus_cfg(struct ipu_csi_bus_config *csicfg,
 			    const struct v4l2_mbus_config *mbus_cfg,
 			    const struct v4l2_mbus_framefmt *mbus_fmt)
 {
-	int ret;
+	int ret, is_bt1120;
 
 	memset(csicfg, 0, sizeof(*csicfg));
 
@@ -373,11 +369,18 @@  static int fill_csi_bus_cfg(struct ipu_csi_bus_config *csicfg,
 		break;
 	case V4L2_MBUS_BT656:
 		csicfg->ext_vsync = 0;
+		/* UYVY10_1X20 etc. could be supported as well */
+		is_bt1120 = mbus_fmt->code == MEDIA_BUS_FMT_UYVY8_1X16 ||
+			mbus_fmt->code == MEDIA_BUS_FMT_YUYV8_1X16;
 		if (V4L2_FIELD_HAS_BOTH(mbus_fmt->field) ||
 		    mbus_fmt->field == V4L2_FIELD_ALTERNATE)
-			csicfg->clk_mode = IPU_CSI_CLK_MODE_CCIR656_INTERLACED;
+			csicfg->clk_mode = is_bt1120 ?
+				IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_SDR :
+				IPU_CSI_CLK_MODE_CCIR656_INTERLACED;
 		else
-			csicfg->clk_mode = IPU_CSI_CLK_MODE_CCIR656_PROGRESSIVE;
+			csicfg->clk_mode = is_bt1120 ?
+				IPU_CSI_CLK_MODE_CCIR1120_PROGRESSIVE_SDR :
+				IPU_CSI_CLK_MODE_CCIR656_PROGRESSIVE;
 		break;
 	case V4L2_MBUS_CSI2_DPHY:
 		/*
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index d9e5388ffeb5..3d1d184a0ba7 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -123,27 +123,21 @@  static inline bool is_parallel_bus(struct v4l2_fwnode_endpoint *ep)
 	return ep->bus_type != V4L2_MBUS_CSI2_DPHY;
 }
 
-static inline bool is_parallel_16bit_bus(struct v4l2_fwnode_endpoint *ep)
-{
-	return is_parallel_bus(ep) && ep->bus.parallel.bus_width >= 16;
-}
-
 /*
  * Check for conditions that require the IPU to handle the
  * data internally as generic data, aka passthrough mode:
  * - raw bayer media bus formats, or
- * - the CSI is receiving from a 16-bit parallel bus, or
- * - the CSI is receiving from an 8-bit parallel bus and the incoming
- *   media bus format is other than UYVY8_2X8/YUYV8_2X8.
+ * - other unsupported modes such as JPEG, Y8 etc.
  */
 static inline bool requires_passthrough(struct v4l2_fwnode_endpoint *ep,
 					struct v4l2_mbus_framefmt *infmt,
 					const struct imx_media_pixfmt *incc)
 {
-	return incc->bayer || is_parallel_16bit_bus(ep) ||
-		(is_parallel_bus(ep) &&
+	return incc->bayer || (is_parallel_bus(ep) &&
 		 infmt->code != MEDIA_BUS_FMT_UYVY8_2X8 &&
-		 infmt->code != MEDIA_BUS_FMT_YUYV8_2X8);
+		 infmt->code != MEDIA_BUS_FMT_UYVY8_1X16 &&
+		 infmt->code != MEDIA_BUS_FMT_YUYV8_2X8 &&
+		 infmt->code != MEDIA_BUS_FMT_YUYV8_1X16);
 }
 
 /*