Hi Hans,
On 3/25/20 2:13 AM, Hans Verkuil wrote:
On 3/13/20 12:47 AM, Laurent Pinchart wrote:
The imx7-mipi-csis only uses the imx_media_init_mbus_fmt() function from
the imx-media-utils helpers. The helpers don't support all the media bus
formats used by this driver, and are thus a bad fit. As the MIPI CSIS is
a standalone IP core that could be integrated in other SoCs, let's not
use the helper.
Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
---
drivers/staging/media/imx/imx7-mipi-csis.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 0829980d7af5..66ff73919371 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -28,8 +28,6 @@
#include <media/v4l2-fwnode.h>
#include <media/v4l2-subdev.h>
-#include "imx-media.h"
-
#define CSIS_DRIVER_NAME "imx7-mipi-csis"
#define CSIS_SUBDEV_NAME CSIS_DRIVER_NAME
@@ -709,15 +707,21 @@ static int mipi_csis_init_cfg(struct v4l2_subdev *mipi_sd,
struct v4l2_mbus_framefmt *fmt_sink;
struct v4l2_mbus_framefmt *fmt_source;
enum v4l2_subdev_format_whence which;
- int ret;
which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
fmt_sink = mipi_csis_get_format(state, cfg, which, CSIS_PAD_SINK);
- ret = imx_media_init_mbus_fmt(fmt_sink, MIPI_CSIS_DEF_PIX_WIDTH,
- MIPI_CSIS_DEF_PIX_HEIGHT, 0,
- V4L2_FIELD_NONE, NULL);
- if (ret < 0)
- return ret;
+
+ fmt_sink->code = MEDIA_BUS_FMT_UYVY8_2X8;
+ fmt_sink->width = MIPI_CSIS_DEF_PIX_WIDTH;
+ fmt_sink->height = MIPI_CSIS_DEF_PIX_HEIGHT;
+ fmt_sink->field = V4L2_FIELD_NONE;
+
+ fmt_sink->colorspace = V4L2_COLORSPACE_SMPTE170M;
Why V4L2_COLORSPACE_SMPTE170M?
After grepping a bit more in the imx code I see that the colorspace
handling is wrong in any case, so I will just accept this patch, but
this really should be fixed driver-wide.
It looks like the driver makes the typical mistake of thinking that
YUV formats use SMPTE170M colorspace and RGB formats use SRGB, but that's
not true.
The imx6 driver mostly just propagates the colorspace from sink to
source pads that comes originally from the sensor, and computes the
other colorimetry parameters based on the propagated colorspace, see
imx_media_try_colorimetry(). But I found one location where the
colorspace is set to V4L2_COLORSPACE_SRGB if the pixel format is RGB,
and V4L2_COLORSPACE_SMPTE170M if the pixel format is YUV, in the
function init_mbus_colorimetry(). That's not necessarily a good choice
for a default colorspace, since SMPTE170M could have a RGB encoding, or
SRGB could have a YUV encoding. I'll send a patch that just defaults to
IPUV3_COLORSPACE_RGB in init_mbus_colorimetry().
Steve
For drivers like this that typically are used with sensors
initializing the colorspace to SRGB is a good default. But the actual
colorspace comes from the sensor or the video receiver, the imx driver can't
know. And YUV vs RGB memory formats is just a different pixel encoding and
is unrelated to the colorspace.
Regards,
Hans
+ fmt_sink->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt_sink->colorspace);
+ fmt_sink->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt_sink->colorspace);
+ fmt_sink->quantization =
+ V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink->colorspace,
+ fmt_sink->ycbcr_enc);
/*
* When called from mipi_csis_subdev_init() to initialize the active
