Re: [PATCH 13/14] media: imx: imx7-mipi-csis: Don't use imx-media-utils helpers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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.

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

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux