Re: [PATCH] MEDIA: i.MX6: Support 16-bit BT.1120 video input

[Philipp Zabel <p.zabel@pengutronix.de>]

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