| Message ID | 20200312234722.23483-14-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: Miscellaneous fixes for i.MX7 CSI-2 receiver | expand |
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@ideasonboard.com> > --- > 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. 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 >
Hi Hans, On Wed, Mar 25, 2020 at 10:13:05AM +0100, 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@ideasonboard.com> > > --- > > 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. That's exactly why V4L2_COLORSPACE_SMPTE170M :-) It's what the driver uses today, I didn't want to change it in this patch. I agree it should be fixed on top. > 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. 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. > > > + 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
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@ideasonboard.com> >> --- >> 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 >>
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; + 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
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@ideasonboard.com> --- drivers/staging/media/imx/imx7-mipi-csis.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)