diff mbox series

[v8,35/38] media: ov2740: Add support for embedded data

Message ID 20240313072516.241106-36-sakari.ailus@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Generic line based metadata support, internal pads | expand

Commit Message

Sakari Ailus March 13, 2024, 7:25 a.m. UTC 
Add support for embedded data. This introduces two internal pads for pixel
and embedded data streams. As the driver supports a single mode only,
there's no need for backward compatibility in mode selection.

The embedded data is configured to be placed before the image data whereas
after the image data is the default.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ov2740.c | 150 +++++++++++++++++++++++++++++++++----
 1 file changed, 137 insertions(+), 13 deletions(-)

Comments

Bingbu Cao March 14, 2024, 7 a.m. UTC | #1 
Sakari,

Thanks for the patch.

On 3/13/24 3:25 PM, Sakari Ailus wrote:
> Add support for embedded data. This introduces two internal pads for pixel
> and embedded data streams. As the driver supports a single mode only,
> there's no need for backward compatibility in mode selection.
> 
> The embedded data is configured to be placed before the image data whereas
> after the image data is the default.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/ov2740.c | 150 +++++++++++++++++++++++++++++++++----
>  1 file changed, 137 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index df57f0096e98..7488b2535071 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -11,6 +11,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/nvmem-provider.h>
>  #include <linux/regmap.h>
> +#include <media/mipi-csi2.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
> @@ -71,11 +72,31 @@
>  #define OV2740_REG_ISP_CTRL00		0x5000
>  /* ISP CTRL01 */
>  #define OV2740_REG_ISP_CTRL01		0x5001
> +
> +/* Embedded data line location control */
> +#define OV2740_REG_EMBEDDED_FLAG	0x5a08
> +#define OV2740_EMBEDDED_FLAG_FOOTER	BIT(2) /* otherwise it's in header */
> +#define OV2740_EMBEDDED_FLAG_MYSTERY	BIT(1)
>  /* Customer Addresses: 0x7010 - 0x710F */
>  #define CUSTOMER_USE_OTP_SIZE		0x100
>  /* OTP registers from sensor */
>  #define OV2740_REG_OTP_CUSTOMER		0x7010
>  
> +enum {
> +	OV2740_PAD_SOURCE,
> +	OV2740_PAD_PIXEL,
> +	OV2740_PAD_META,
> +	OV2740_NUM_PADS,
> +};
> +
> +enum {
> +	OV2740_STREAM_PIXEL,
> +	OV2740_STREAM_META,
> +};
> +
> +#define OV2740_META_WIDTH		100U /* 97 bytes of actual data */
> +#define OV2740_META_HEIGHT		1U
> +
>  struct nvm_data {
>  	struct nvmem_device *nvmem;
>  	struct regmap *regmap;
> @@ -513,7 +534,7 @@ static const struct ov2740_mode supported_modes_180mhz[] = {
>  
>  struct ov2740 {
>  	struct v4l2_subdev sd;
> -	struct media_pad pad;
> +	struct media_pad pads[OV2740_NUM_PADS];
>  	struct v4l2_ctrl_handler ctrl_handler;
>  
>  	/* V4L2 Controls */
> @@ -976,6 +997,11 @@ static int ov2740_enable_streams(struct v4l2_subdev *sd,
>  	if (ret)
>  		goto out_pm_put;
>  
> +	ret = ov2740_write_reg(ov2740, OV2740_REG_EMBEDDED_FLAG, 1,
> +			       OV2740_EMBEDDED_FLAG_MYSTERY);
> +	if (ret)
> +		return ret;
> +
>  	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
>  			       OV2740_MODE_STREAMING);
>  	if (ret) {
> @@ -1013,23 +1039,49 @@ static int ov2740_disable_streams(struct v4l2_subdev *sd,
>  	return ret;
>  }
>  
> -static int ov2740_set_format(struct v4l2_subdev *sd,
> -			     struct v4l2_subdev_state *sd_state,
> -			     struct v4l2_subdev_format *fmt)
> +static int __ov2740_set_format(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *sd_state,
> +			       struct v4l2_mbus_framefmt *format,
> +			       enum v4l2_subdev_format_whence which,
> +			       unsigned int pad, unsigned int stream)
>  {
> +	struct v4l2_mbus_framefmt *src_pix_fmt, *src_meta_fmt, *pix_fmt,
> +		*meta_fmt;
>  	struct ov2740 *ov2740 = to_ov2740(sd);
>  	const struct ov2740_mode *mode;
>  	s32 vblank_def, h_blank;
>  
> +	/*
> +	 * Allow setting format on internal pixel pad as well as the source
> +	 * pad's pixel stream (for compatibility).
> +	 */
> +	if (pad == OV2740_PAD_SOURCE || pad == OV2740_PAD_META ||
> +	    stream == OV2740_STREAM_META) {
> +		*format = *v4l2_subdev_state_get_format(sd_state, pad, stream);
> +		return 0;
> +	}
> +
> +	pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_PIXEL, 0);
> +	meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_META, 0);
> +	src_pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> +						   OV2740_STREAM_PIXEL);
> +	src_meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> +						    OV2740_STREAM_META);
> +
>  	mode = v4l2_find_nearest_size(ov2740->supported_modes,
>  				      ov2740->supported_modes_count,
>  				      width, height,
> -				      fmt->format.width, fmt->format.height);
> +				      format->width, format->height);
> +	ov2740_update_pad_format(mode, pix_fmt);
> +	*format = *src_pix_fmt = *pix_fmt;
>  
> -	ov2740_update_pad_format(mode, &fmt->format);
> -	*v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
> +	meta_fmt->code = MEDIA_BUS_FMT_OV2740_EMBEDDED;
> +	meta_fmt->width = OV2740_META_WIDTH;
> +	meta_fmt->height = OV2740_META_HEIGHT;
> +	*src_meta_fmt = *meta_fmt;
> +	src_meta_fmt->code = MEDIA_BUS_FMT_META_10;
>  
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
>  		return 0;
>  
>  	ov2740->cur_mode = mode;
> @@ -1049,6 +1101,14 @@ static int ov2740_set_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int ov2740_set_format(struct v4l2_subdev *sd,
> +			     struct v4l2_subdev_state *sd_state,
> +			     struct v4l2_subdev_format *fmt)
> +{
> +	return __ov2740_set_format(sd, sd_state, &fmt->format, fmt->which,
> +				   fmt->pad, fmt->stream);
> +}
> +
>  static int ov2740_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum *code)
> @@ -1085,10 +1145,68 @@ static int ov2740_enum_frame_size(struct v4l2_subdev *sd,
>  static int ov2740_init_state(struct v4l2_subdev *sd,
>  			     struct v4l2_subdev_state *sd_state)
>  {
> +	struct v4l2_subdev_route routes[] = {
> +		{
> +			.sink_pad = OV2740_PAD_PIXEL,
> +			.source_pad = OV2740_PAD_SOURCE,
> +			.source_stream = OV2740_STREAM_PIXEL,
> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> +		}, {
> +			.sink_pad = OV2740_PAD_META,
> +			.source_pad = OV2740_PAD_SOURCE,
> +			.source_stream = OV2740_STREAM_META,
> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> +		},
> +	};
> +	struct v4l2_subdev_krouting routing = {
> +		.routes = routes,
> +		.num_routes = ARRAY_SIZE(routes),
> +	};
> +	struct v4l2_subdev_state *active_state;
> +	struct v4l2_mbus_framefmt format = { 0 };
>  	struct ov2740 *ov2740 = to_ov2740(sd);
> +	int ret;
> +
> +	ret = v4l2_subdev_set_routing(sd, sd_state, &routing);
> +	if (ret)
> +		return ret;
> +
> +	active_state = v4l2_subdev_get_locked_active_state(sd);
> +
> +	ov2740_update_pad_format(&ov2740->supported_modes[0], &format);
> +
> +	return __ov2740_set_format(sd, sd_state, &format,
> +				   active_state == sd_state ?
> +				   V4L2_SUBDEV_FORMAT_ACTIVE :
> +				   V4L2_SUBDEV_FORMAT_TRY, OV2740_PAD_PIXEL, 0);
> +}
> +
> +static int ov2740_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				 struct v4l2_mbus_frame_desc *desc)
> +{
> +	struct v4l2_mbus_frame_desc_entry *entry = desc->entry;
> +	struct v4l2_subdev_state *sd_state;
> +	struct v4l2_mbus_framefmt *fmt;
> +
> +	desc->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +
> +	sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> +	fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> +					   OV2740_STREAM_PIXEL);
> +	entry->pixelcode = fmt->code;
> +	v4l2_subdev_unlock_state(sd_state);
> +
> +	entry->stream = OV2740_STREAM_PIXEL;
> +	entry->bus.csi2.dt = MIPI_CSI2_DT_RAW10;
> +	entry++;
> +	desc->num_entries++;
> +
> +	entry->pixelcode = MEDIA_BUS_FMT_META_8;

Should we change the pixelcode to META_10 also?

> +	entry->stream = OV2740_STREAM_META;
> +	entry->bus.csi2.dt = MIPI_CSI2_DT_GENERIC_LONG(1);

In the register setting array, the 0x4816 is set to 0x52 instead of
its default value 0x53, so the data type should be
MIPI_CSI2_DT_GENERIC_LONG(0) or MIPI_CSI2_DT_EMBEDDED_8B.

Or you can change back to the default settings. :)

> +	entry++;
> +	desc->num_entries++;
>  
> -	ov2740_update_pad_format(&ov2740->supported_modes[0],
> -				 v4l2_subdev_state_get_format(sd_state, 0));
>  	return 0;
>  }
>  
> @@ -1103,6 +1221,7 @@ static const struct v4l2_subdev_pad_ops ov2740_pad_ops = {
>  	.enum_frame_size = ov2740_enum_frame_size,
>  	.enable_streams = ov2740_enable_streams,
>  	.disable_streams = ov2740_disable_streams,
> +	.get_frame_desc = ov2740_get_frame_desc,
>  };
>  
>  static const struct v4l2_subdev_ops ov2740_subdev_ops = {
> @@ -1369,11 +1488,16 @@ static int ov2740_probe(struct i2c_client *client)
>  	}
>  
>  	ov2740->sd.state_lock = ov2740->ctrl_handler.lock;
> -	ov2740->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	ov2740->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
>  	ov2740->sd.entity.ops = &ov2740_subdev_entity_ops;
>  	ov2740->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> -	ov2740->pad.flags = MEDIA_PAD_FL_SOURCE;
> -	ret = media_entity_pads_init(&ov2740->sd.entity, 1, &ov2740->pad);
> +	ov2740->pads[OV2740_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +	ov2740->pads[OV2740_PAD_PIXEL].flags =
> +		MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
> +	ov2740->pads[OV2740_PAD_META].flags =
> +		MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
> +	ret = media_entity_pads_init(&ov2740->sd.entity,
> +				     ARRAY_SIZE(ov2740->pads), ov2740->pads);
>  	if (ret) {
>  		dev_err_probe(dev, ret, "failed to init entity pads\n");
>  		goto probe_error_v4l2_ctrl_handler_free;
>
Julien Massot March 14, 2024, 8:24 a.m. UTC | #2 
Hi Sakari,

On 3/13/24 08:25, Sakari Ailus wrote:
> Add support for embedded data. This introduces two internal pads for pixel
> and embedded data streams. As the driver supports a single mode only,
> there's no need for backward compatibility in mode selection.
> 
> The embedded data is configured to be placed before the image data whereas
> after the image data is the default.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>   drivers/media/i2c/ov2740.c | 150 +++++++++++++++++++++++++++++++++----
>   1 file changed, 137 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index df57f0096e98..7488b2535071 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -11,6 +11,7 @@
>   #include <linux/pm_runtime.h>
>   #include <linux/nvmem-provider.h>
>   #include <linux/regmap.h>
> +#include <media/mipi-csi2.h>
>   #include <media/v4l2-ctrls.h>
>   #include <media/v4l2-device.h>
>   #include <media/v4l2-fwnode.h>
> @@ -71,11 +72,31 @@
>   #define OV2740_REG_ISP_CTRL00		0x5000
>   /* ISP CTRL01 */
>   #define OV2740_REG_ISP_CTRL01		0x5001
> +
> +/* Embedded data line location control */
> +#define OV2740_REG_EMBEDDED_FLAG	0x5a08
> +#define OV2740_EMBEDDED_FLAG_FOOTER	BIT(2) /* otherwise it's in header */
> +#define OV2740_EMBEDDED_FLAG_MYSTERY	BIT(1)
>   /* Customer Addresses: 0x7010 - 0x710F */
>   #define CUSTOMER_USE_OTP_SIZE		0x100
>   /* OTP registers from sensor */
>   #define OV2740_REG_OTP_CUSTOMER		0x7010
>   
> +enum {
> +	OV2740_PAD_SOURCE,
> +	OV2740_PAD_PIXEL,
> +	OV2740_PAD_META,
> +	OV2740_NUM_PADS,
> +};
> +
> +enum {
> +	OV2740_STREAM_PIXEL,
> +	OV2740_STREAM_META,
> +};
> +
> +#define OV2740_META_WIDTH		100U /* 97 bytes of actual data */
> +#define OV2740_META_HEIGHT		1U
> +
>   struct nvm_data {
>   	struct nvmem_device *nvmem;
>   	struct regmap *regmap;
> @@ -513,7 +534,7 @@ static const struct ov2740_mode supported_modes_180mhz[] = {
>   
>   struct ov2740 {
>   	struct v4l2_subdev sd;
> -	struct media_pad pad;
> +	struct media_pad pads[OV2740_NUM_PADS];
>   	struct v4l2_ctrl_handler ctrl_handler;
>   
>   	/* V4L2 Controls */
> @@ -976,6 +997,11 @@ static int ov2740_enable_streams(struct v4l2_subdev *sd,
>   	if (ret)
>   		goto out_pm_put;
>   
> +	ret = ov2740_write_reg(ov2740, OV2740_REG_EMBEDDED_FLAG, 1,
> +			       OV2740_EMBEDDED_FLAG_MYSTERY);
> +	if (ret)
> +		return ret;
> +
>   	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
>   			       OV2740_MODE_STREAMING);
>   	if (ret) {
> @@ -1013,23 +1039,49 @@ static int ov2740_disable_streams(struct v4l2_subdev *sd,
>   	return ret;
>   }
>   
> -static int ov2740_set_format(struct v4l2_subdev *sd,
> -			     struct v4l2_subdev_state *sd_state,
> -			     struct v4l2_subdev_format *fmt)
> +static int __ov2740_set_format(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *sd_state,
> +			       struct v4l2_mbus_framefmt *format,
> +			       enum v4l2_subdev_format_whence which,
> +			       unsigned int pad, unsigned int stream)
>   {
> +	struct v4l2_mbus_framefmt *src_pix_fmt, *src_meta_fmt, *pix_fmt,
> +		*meta_fmt;
>   	struct ov2740 *ov2740 = to_ov2740(sd);
>   	const struct ov2740_mode *mode;
>   	s32 vblank_def, h_blank;
>   
> +	/*
> +	 * Allow setting format on internal pixel pad as well as the source
> +	 * pad's pixel stream (for compatibility).
> +	 */
> +	if (pad == OV2740_PAD_SOURCE || pad == OV2740_PAD_META ||
> +	    stream == OV2740_STREAM_META) {
This is equivalent to
if (pad != OV2740_PAD_PIXEL)
Correct me if I'm wrong but this code doesn't allow to set the format on 
the source pad.

Should it be:
if ((pad == OV2740_PAD_SOURCE && stream == OV2740_STREAM_META) ||
	pad == OV2740_PAD_META) {


> +		*format = *v4l2_subdev_state_get_format(sd_state, pad, stream);
> +		return 0;
> +	}
> +
> +	pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_PIXEL, 0);
> +	meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_META, 0);
> +	src_pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> +						   OV2740_STREAM_PIXEL);
> +	src_meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> +						    OV2740_STREAM_META);
> +
>   	mode = v4l2_find_nearest_size(ov2740->supported_modes,
>   				      ov2740->supported_modes_count,
>   				      width, height,
> -				      fmt->format.width, fmt->format.height);
> +				      format->width, format->height);
> +	ov2740_update_pad_format(mode, pix_fmt);
> +	*format = *src_pix_fmt = *pix_fmt;
>   
> -	ov2740_update_pad_format(mode, &fmt->format);
> -	*v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
> +	meta_fmt->code = MEDIA_BUS_FMT_OV2740_EMBEDDED;
> +	meta_fmt->width = OV2740_META_WIDTH;
> +	meta_fmt->height = OV2740_META_HEIGHT;
> +	*src_meta_fmt = *meta_fmt;
> +	src_meta_fmt->code = MEDIA_BUS_FMT_META_10;
>   
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
>   		return 0;
>   
>   	ov2740->cur_mode = mode;
Regards,
Sakari Ailus March 19, 2024, 1:13 p.m. UTC | #3 
Hi Bingbu,

On Thu, Mar 14, 2024 at 03:00:49PM +0800, Bingbu Cao wrote:
> > +	entry->stream = OV2740_STREAM_PIXEL;
> > +	entry->bus.csi2.dt = MIPI_CSI2_DT_RAW10;
> > +	entry++;
> > +	desc->num_entries++;
> > +
> > +	entry->pixelcode = MEDIA_BUS_FMT_META_8;
> 
> Should we change the pixelcode to META_10 also?

For others to know: Bingbu confirmed the sensor embedded data output is
actually using 10 bits per pixel packing.

> 
> > +	entry->stream = OV2740_STREAM_META;
> > +	entry->bus.csi2.dt = MIPI_CSI2_DT_GENERIC_LONG(1);
> 
> In the register setting array, the 0x4816 is set to 0x52 instead of
> its default value 0x53, so the data type should be
> MIPI_CSI2_DT_GENERIC_LONG(0) or MIPI_CSI2_DT_EMBEDDED_8B.
> 
> Or you can change back to the default settings. :)

I'll use MIPI_CSI2_DT_EMBEDDED_8B.
Sakari Ailus March 19, 2024, 1:18 p.m. UTC | #4 
Hi Julien,

Thanks for the review.

On Thu, Mar 14, 2024 at 09:24:48AM +0100, Julien Massot wrote:
> Hi Sakari,
> 
> On 3/13/24 08:25, Sakari Ailus wrote:
> > Add support for embedded data. This introduces two internal pads for pixel
> > and embedded data streams. As the driver supports a single mode only,
> > there's no need for backward compatibility in mode selection.
> > 
> > The embedded data is configured to be placed before the image data whereas
> > after the image data is the default.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >   drivers/media/i2c/ov2740.c | 150 +++++++++++++++++++++++++++++++++----
> >   1 file changed, 137 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> > index df57f0096e98..7488b2535071 100644
> > --- a/drivers/media/i2c/ov2740.c
> > +++ b/drivers/media/i2c/ov2740.c
> > @@ -11,6 +11,7 @@
> >   #include <linux/pm_runtime.h>
> >   #include <linux/nvmem-provider.h>
> >   #include <linux/regmap.h>
> > +#include <media/mipi-csi2.h>
> >   #include <media/v4l2-ctrls.h>
> >   #include <media/v4l2-device.h>
> >   #include <media/v4l2-fwnode.h>
> > @@ -71,11 +72,31 @@
> >   #define OV2740_REG_ISP_CTRL00		0x5000
> >   /* ISP CTRL01 */
> >   #define OV2740_REG_ISP_CTRL01		0x5001
> > +
> > +/* Embedded data line location control */
> > +#define OV2740_REG_EMBEDDED_FLAG	0x5a08
> > +#define OV2740_EMBEDDED_FLAG_FOOTER	BIT(2) /* otherwise it's in header */
> > +#define OV2740_EMBEDDED_FLAG_MYSTERY	BIT(1)
> >   /* Customer Addresses: 0x7010 - 0x710F */
> >   #define CUSTOMER_USE_OTP_SIZE		0x100
> >   /* OTP registers from sensor */
> >   #define OV2740_REG_OTP_CUSTOMER		0x7010
> > +enum {
> > +	OV2740_PAD_SOURCE,
> > +	OV2740_PAD_PIXEL,
> > +	OV2740_PAD_META,
> > +	OV2740_NUM_PADS,
> > +};
> > +
> > +enum {
> > +	OV2740_STREAM_PIXEL,
> > +	OV2740_STREAM_META,
> > +};
> > +
> > +#define OV2740_META_WIDTH		100U /* 97 bytes of actual data */
> > +#define OV2740_META_HEIGHT		1U
> > +
> >   struct nvm_data {
> >   	struct nvmem_device *nvmem;
> >   	struct regmap *regmap;
> > @@ -513,7 +534,7 @@ static const struct ov2740_mode supported_modes_180mhz[] = {
> >   struct ov2740 {
> >   	struct v4l2_subdev sd;
> > -	struct media_pad pad;
> > +	struct media_pad pads[OV2740_NUM_PADS];
> >   	struct v4l2_ctrl_handler ctrl_handler;
> >   	/* V4L2 Controls */
> > @@ -976,6 +997,11 @@ static int ov2740_enable_streams(struct v4l2_subdev *sd,
> >   	if (ret)
> >   		goto out_pm_put;
> > +	ret = ov2740_write_reg(ov2740, OV2740_REG_EMBEDDED_FLAG, 1,
> > +			       OV2740_EMBEDDED_FLAG_MYSTERY);
> > +	if (ret)
> > +		return ret;
> > +
> >   	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
> >   			       OV2740_MODE_STREAMING);
> >   	if (ret) {
> > @@ -1013,23 +1039,49 @@ static int ov2740_disable_streams(struct v4l2_subdev *sd,
> >   	return ret;
> >   }
> > -static int ov2740_set_format(struct v4l2_subdev *sd,
> > -			     struct v4l2_subdev_state *sd_state,
> > -			     struct v4l2_subdev_format *fmt)
> > +static int __ov2740_set_format(struct v4l2_subdev *sd,
> > +			       struct v4l2_subdev_state *sd_state,
> > +			       struct v4l2_mbus_framefmt *format,
> > +			       enum v4l2_subdev_format_whence which,
> > +			       unsigned int pad, unsigned int stream)
> >   {
> > +	struct v4l2_mbus_framefmt *src_pix_fmt, *src_meta_fmt, *pix_fmt,
> > +		*meta_fmt;
> >   	struct ov2740 *ov2740 = to_ov2740(sd);
> >   	const struct ov2740_mode *mode;
> >   	s32 vblank_def, h_blank;
> > +	/*
> > +	 * Allow setting format on internal pixel pad as well as the source
> > +	 * pad's pixel stream (for compatibility).
> > +	 */
> > +	if (pad == OV2740_PAD_SOURCE || pad == OV2740_PAD_META ||
> > +	    stream == OV2740_STREAM_META) {
> This is equivalent to
> if (pad != OV2740_PAD_PIXEL)
> Correct me if I'm wrong but this code doesn't allow to set the format on the
> source pad.

Good point. I put that to the comment but somehow failed to align the code
with the statement.

> 
> Should it be:
> if ((pad == OV2740_PAD_SOURCE && stream == OV2740_STREAM_META) ||
> 	pad == OV2740_PAD_META) {

This seems better indeed!

> 
> 
> > +		*format = *v4l2_subdev_state_get_format(sd_state, pad, stream);
> > +		return 0;
> > +	}
> > +
> > +	pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_PIXEL, 0);
> > +	meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_META, 0);
> > +	src_pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> > +						   OV2740_STREAM_PIXEL);
> > +	src_meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> > +						    OV2740_STREAM_META);
> > +
> >   	mode = v4l2_find_nearest_size(ov2740->supported_modes,
> >   				      ov2740->supported_modes_count,
> >   				      width, height,
> > -				      fmt->format.width, fmt->format.height);
> > +				      format->width, format->height);
> > +	ov2740_update_pad_format(mode, pix_fmt);
> > +	*format = *src_pix_fmt = *pix_fmt;
> > -	ov2740_update_pad_format(mode, &fmt->format);
> > -	*v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
> > +	meta_fmt->code = MEDIA_BUS_FMT_OV2740_EMBEDDED;
> > +	meta_fmt->width = OV2740_META_WIDTH;
> > +	meta_fmt->height = OV2740_META_HEIGHT;
> > +	*src_meta_fmt = *meta_fmt;
> > +	src_meta_fmt->code = MEDIA_BUS_FMT_META_10;
> > -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> > +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> >   		return 0;
> >   	ov2740->cur_mode = mode;
Laurent Pinchart March 21, 2024, 5:16 p.m. UTC | #5 
Hi Sakari,

Thank you for the patch.

On Wed, Mar 13, 2024 at 09:25:13AM +0200, Sakari Ailus wrote:
> Add support for embedded data. This introduces two internal pads for pixel
> and embedded data streams. As the driver supports a single mode only,
> there's no need for backward compatibility in mode selection.
> 
> The embedded data is configured to be placed before the image data whereas
> after the image data is the default.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/ov2740.c | 150 +++++++++++++++++++++++++++++++++----
>  1 file changed, 137 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index df57f0096e98..7488b2535071 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -11,6 +11,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/nvmem-provider.h>
>  #include <linux/regmap.h>
> +#include <media/mipi-csi2.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
> @@ -71,11 +72,31 @@
>  #define OV2740_REG_ISP_CTRL00		0x5000
>  /* ISP CTRL01 */
>  #define OV2740_REG_ISP_CTRL01		0x5001
> +
> +/* Embedded data line location control */
> +#define OV2740_REG_EMBEDDED_FLAG	0x5a08
> +#define OV2740_EMBEDDED_FLAG_FOOTER	BIT(2) /* otherwise it's in header */
> +#define OV2740_EMBEDDED_FLAG_MYSTERY	BIT(1)
>  /* Customer Addresses: 0x7010 - 0x710F */
>  #define CUSTOMER_USE_OTP_SIZE		0x100
>  /* OTP registers from sensor */
>  #define OV2740_REG_OTP_CUSTOMER		0x7010
>  
> +enum {
> +	OV2740_PAD_SOURCE,
> +	OV2740_PAD_PIXEL,
> +	OV2740_PAD_META,
> +	OV2740_NUM_PADS,
> +};
> +
> +enum {
> +	OV2740_STREAM_PIXEL,
> +	OV2740_STREAM_META,
> +};
> +
> +#define OV2740_META_WIDTH		100U /* 97 bytes of actual data */
> +#define OV2740_META_HEIGHT		1U
> +
>  struct nvm_data {
>  	struct nvmem_device *nvmem;
>  	struct regmap *regmap;
> @@ -513,7 +534,7 @@ static const struct ov2740_mode supported_modes_180mhz[] = {
>  
>  struct ov2740 {
>  	struct v4l2_subdev sd;
> -	struct media_pad pad;
> +	struct media_pad pads[OV2740_NUM_PADS];
>  	struct v4l2_ctrl_handler ctrl_handler;
>  
>  	/* V4L2 Controls */
> @@ -976,6 +997,11 @@ static int ov2740_enable_streams(struct v4l2_subdev *sd,
>  	if (ret)
>  		goto out_pm_put;
>  
> +	ret = ov2740_write_reg(ov2740, OV2740_REG_EMBEDDED_FLAG, 1,
> +			       OV2740_EMBEDDED_FLAG_MYSTERY);
> +	if (ret)
> +		return ret;
> +
>  	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
>  			       OV2740_MODE_STREAMING);
>  	if (ret) {
> @@ -1013,23 +1039,49 @@ static int ov2740_disable_streams(struct v4l2_subdev *sd,
>  	return ret;
>  }
>  
> -static int ov2740_set_format(struct v4l2_subdev *sd,
> -			     struct v4l2_subdev_state *sd_state,
> -			     struct v4l2_subdev_format *fmt)
> +static int __ov2740_set_format(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *sd_state,
> +			       struct v4l2_mbus_framefmt *format,
> +			       enum v4l2_subdev_format_whence which,
> +			       unsigned int pad, unsigned int stream)
>  {
> +	struct v4l2_mbus_framefmt *src_pix_fmt, *src_meta_fmt, *pix_fmt,
> +		*meta_fmt;
>  	struct ov2740 *ov2740 = to_ov2740(sd);
>  	const struct ov2740_mode *mode;
>  	s32 vblank_def, h_blank;
>  
> +	/*
> +	 * Allow setting format on internal pixel pad as well as the source
> +	 * pad's pixel stream (for compatibility).

The internal image pad represents the pixel array, it should thus report
the full pixel array size, and be unaffected by the selected mode.

> +	 */
> +	if (pad == OV2740_PAD_SOURCE || pad == OV2740_PAD_META ||
> +	    stream == OV2740_STREAM_META) {

As Julien pointed out, this isn't right.

> +		*format = *v4l2_subdev_state_get_format(sd_state, pad, stream);
> +		return 0;
> +	}
> +
> +	pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_PIXEL, 0);
> +	meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_META, 0);
> +	src_pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> +						   OV2740_STREAM_PIXEL);
> +	src_meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> +						    OV2740_STREAM_META);
> +
>  	mode = v4l2_find_nearest_size(ov2740->supported_modes,
>  				      ov2740->supported_modes_count,
>  				      width, height,
> -				      fmt->format.width, fmt->format.height);
> +				      format->width, format->height);
> +	ov2740_update_pad_format(mode, pix_fmt);
> +	*format = *src_pix_fmt = *pix_fmt;
>  
> -	ov2740_update_pad_format(mode, &fmt->format);
> -	*v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
> +	meta_fmt->code = MEDIA_BUS_FMT_OV2740_EMBEDDED;
> +	meta_fmt->width = OV2740_META_WIDTH;
> +	meta_fmt->height = OV2740_META_HEIGHT;
> +	*src_meta_fmt = *meta_fmt;
> +	src_meta_fmt->code = MEDIA_BUS_FMT_META_10;
>  
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
>  		return 0;
>  
>  	ov2740->cur_mode = mode;
> @@ -1049,6 +1101,14 @@ static int ov2740_set_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int ov2740_set_format(struct v4l2_subdev *sd,
> +			     struct v4l2_subdev_state *sd_state,
> +			     struct v4l2_subdev_format *fmt)
> +{
> +	return __ov2740_set_format(sd, sd_state, &fmt->format, fmt->which,
> +				   fmt->pad, fmt->stream);
> +}
> +
>  static int ov2740_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum *code)

You need to update this too, to enumerate the correct format on the
different pads and streams.

> @@ -1085,10 +1145,68 @@ static int ov2740_enum_frame_size(struct v4l2_subdev *sd,
>  static int ov2740_init_state(struct v4l2_subdev *sd,
>  			     struct v4l2_subdev_state *sd_state)
>  {
> +	struct v4l2_subdev_route routes[] = {
> +		{
> +			.sink_pad = OV2740_PAD_PIXEL,
> +			.source_pad = OV2740_PAD_SOURCE,
> +			.source_stream = OV2740_STREAM_PIXEL,
> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> +		}, {
> +			.sink_pad = OV2740_PAD_META,
> +			.source_pad = OV2740_PAD_SOURCE,
> +			.source_stream = OV2740_STREAM_META,
> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> +		},
> +	};
> +	struct v4l2_subdev_krouting routing = {
> +		.routes = routes,
> +		.num_routes = ARRAY_SIZE(routes),
> +	};
> +	struct v4l2_subdev_state *active_state;
> +	struct v4l2_mbus_framefmt format = { 0 };
>  	struct ov2740 *ov2740 = to_ov2740(sd);
> +	int ret;
> +
> +	ret = v4l2_subdev_set_routing(sd, sd_state, &routing);
> +	if (ret)
> +		return ret;
> +
> +	active_state = v4l2_subdev_get_locked_active_state(sd);

There's a lockdep assertion that will trip when initializing any try
state.

> +
> +	ov2740_update_pad_format(&ov2740->supported_modes[0], &format);
> +
> +	return __ov2740_set_format(sd, sd_state, &format,
> +				   active_state == sd_state ?
> +				   V4L2_SUBDEV_FORMAT_ACTIVE :
> +				   V4L2_SUBDEV_FORMAT_TRY, OV2740_PAD_PIXEL, 0);

Move the code specific to the active state from __ov2740_set_format() to
ov2740_set_format(), and drop the which parameter to
__ov2740_set_format(). You'll simplify the code here.

> +}
> +
> +static int ov2740_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				 struct v4l2_mbus_frame_desc *desc)
> +{
> +	struct v4l2_mbus_frame_desc_entry *entry = desc->entry;
> +	struct v4l2_subdev_state *sd_state;
> +	struct v4l2_mbus_framefmt *fmt;
> +
> +	desc->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +
> +	sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> +	fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> +					   OV2740_STREAM_PIXEL);
> +	entry->pixelcode = fmt->code;
> +	v4l2_subdev_unlock_state(sd_state);
> +
> +	entry->stream = OV2740_STREAM_PIXEL;
> +	entry->bus.csi2.dt = MIPI_CSI2_DT_RAW10;
> +	entry++;
> +	desc->num_entries++;

I think addressing entries explicitly would be clearer.

	entry[0].stream = ...;
	entry[0].bus.csi2.dt = ...;

	...

	desc->num_entries = 2;

> +
> +	entry->pixelcode = MEDIA_BUS_FMT_META_8;
> +	entry->stream = OV2740_STREAM_META;
> +	entry->bus.csi2.dt = MIPI_CSI2_DT_GENERIC_LONG(1);

As Bingbu mentioned, this is not correct.

> +	entry++;
> +	desc->num_entries++;
>  
> -	ov2740_update_pad_format(&ov2740->supported_modes[0],
> -				 v4l2_subdev_state_get_format(sd_state, 0));
>  	return 0;
>  }
>  
> @@ -1103,6 +1221,7 @@ static const struct v4l2_subdev_pad_ops ov2740_pad_ops = {
>  	.enum_frame_size = ov2740_enum_frame_size,
>  	.enable_streams = ov2740_enable_streams,
>  	.disable_streams = ov2740_disable_streams,
> +	.get_frame_desc = ov2740_get_frame_desc,
>  };
>  
>  static const struct v4l2_subdev_ops ov2740_subdev_ops = {
> @@ -1369,11 +1488,16 @@ static int ov2740_probe(struct i2c_client *client)
>  	}
>  
>  	ov2740->sd.state_lock = ov2740->ctrl_handler.lock;
> -	ov2740->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	ov2740->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
>  	ov2740->sd.entity.ops = &ov2740_subdev_entity_ops;
>  	ov2740->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> -	ov2740->pad.flags = MEDIA_PAD_FL_SOURCE;
> -	ret = media_entity_pads_init(&ov2740->sd.entity, 1, &ov2740->pad);
> +	ov2740->pads[OV2740_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +	ov2740->pads[OV2740_PAD_PIXEL].flags =
> +		MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
> +	ov2740->pads[OV2740_PAD_META].flags =
> +		MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
> +	ret = media_entity_pads_init(&ov2740->sd.entity,
> +				     ARRAY_SIZE(ov2740->pads), ov2740->pads);
>  	if (ret) {
>  		dev_err_probe(dev, ret, "failed to init entity pads\n");
>  		goto probe_error_v4l2_ctrl_handler_free;
Sakari Ailus April 10, 2024, 1:18 p.m. UTC | #6 
Hi Laurent,

Thanks for the review.

On Thu, Mar 21, 2024 at 07:16:08PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wed, Mar 13, 2024 at 09:25:13AM +0200, Sakari Ailus wrote:
> > Add support for embedded data. This introduces two internal pads for pixel
> > and embedded data streams. As the driver supports a single mode only,
> > there's no need for backward compatibility in mode selection.
> > 
> > The embedded data is configured to be placed before the image data whereas
> > after the image data is the default.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/i2c/ov2740.c | 150 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 137 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> > index df57f0096e98..7488b2535071 100644
> > --- a/drivers/media/i2c/ov2740.c
> > +++ b/drivers/media/i2c/ov2740.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/pm_runtime.h>
> >  #include <linux/nvmem-provider.h>
> >  #include <linux/regmap.h>
> > +#include <media/mipi-csi2.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-fwnode.h>
> > @@ -71,11 +72,31 @@
> >  #define OV2740_REG_ISP_CTRL00		0x5000
> >  /* ISP CTRL01 */
> >  #define OV2740_REG_ISP_CTRL01		0x5001
> > +
> > +/* Embedded data line location control */
> > +#define OV2740_REG_EMBEDDED_FLAG	0x5a08
> > +#define OV2740_EMBEDDED_FLAG_FOOTER	BIT(2) /* otherwise it's in header */
> > +#define OV2740_EMBEDDED_FLAG_MYSTERY	BIT(1)
> >  /* Customer Addresses: 0x7010 - 0x710F */
> >  #define CUSTOMER_USE_OTP_SIZE		0x100
> >  /* OTP registers from sensor */
> >  #define OV2740_REG_OTP_CUSTOMER		0x7010
> >  
> > +enum {
> > +	OV2740_PAD_SOURCE,
> > +	OV2740_PAD_PIXEL,
> > +	OV2740_PAD_META,
> > +	OV2740_NUM_PADS,
> > +};
> > +
> > +enum {
> > +	OV2740_STREAM_PIXEL,
> > +	OV2740_STREAM_META,
> > +};
> > +
> > +#define OV2740_META_WIDTH		100U /* 97 bytes of actual data */
> > +#define OV2740_META_HEIGHT		1U
> > +
> >  struct nvm_data {
> >  	struct nvmem_device *nvmem;
> >  	struct regmap *regmap;
> > @@ -513,7 +534,7 @@ static const struct ov2740_mode supported_modes_180mhz[] = {
> >  
> >  struct ov2740 {
> >  	struct v4l2_subdev sd;
> > -	struct media_pad pad;
> > +	struct media_pad pads[OV2740_NUM_PADS];
> >  	struct v4l2_ctrl_handler ctrl_handler;
> >  
> >  	/* V4L2 Controls */
> > @@ -976,6 +997,11 @@ static int ov2740_enable_streams(struct v4l2_subdev *sd,
> >  	if (ret)
> >  		goto out_pm_put;
> >  
> > +	ret = ov2740_write_reg(ov2740, OV2740_REG_EMBEDDED_FLAG, 1,
> > +			       OV2740_EMBEDDED_FLAG_MYSTERY);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
> >  			       OV2740_MODE_STREAMING);
> >  	if (ret) {
> > @@ -1013,23 +1039,49 @@ static int ov2740_disable_streams(struct v4l2_subdev *sd,
> >  	return ret;
> >  }
> >  
> > -static int ov2740_set_format(struct v4l2_subdev *sd,
> > -			     struct v4l2_subdev_state *sd_state,
> > -			     struct v4l2_subdev_format *fmt)
> > +static int __ov2740_set_format(struct v4l2_subdev *sd,
> > +			       struct v4l2_subdev_state *sd_state,
> > +			       struct v4l2_mbus_framefmt *format,
> > +			       enum v4l2_subdev_format_whence which,
> > +			       unsigned int pad, unsigned int stream)
> >  {
> > +	struct v4l2_mbus_framefmt *src_pix_fmt, *src_meta_fmt, *pix_fmt,
> > +		*meta_fmt;
> >  	struct ov2740 *ov2740 = to_ov2740(sd);
> >  	const struct ov2740_mode *mode;
> >  	s32 vblank_def, h_blank;
> >  
> > +	/*
> > +	 * Allow setting format on internal pixel pad as well as the source
> > +	 * pad's pixel stream (for compatibility).
> 
> The internal image pad represents the pixel array, it should thus report
> the full pixel array size, and be unaffected by the selected mode.

Yes, I also realised this later on. I think we'll need more time to define
a proper API for sensors and thus postpone the sensor patches. :-I But I
don't see a reason to delay the rest.

> 
> > +	 */
> > +	if (pad == OV2740_PAD_SOURCE || pad == OV2740_PAD_META ||
> > +	    stream == OV2740_STREAM_META) {
> 
> As Julien pointed out, this isn't right.

I've fixed that for the next version.

> 
> > +		*format = *v4l2_subdev_state_get_format(sd_state, pad, stream);
> > +		return 0;
> > +	}
> > +
> > +	pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_PIXEL, 0);
> > +	meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_META, 0);
> > +	src_pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> > +						   OV2740_STREAM_PIXEL);
> > +	src_meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> > +						    OV2740_STREAM_META);
> > +
> >  	mode = v4l2_find_nearest_size(ov2740->supported_modes,
> >  				      ov2740->supported_modes_count,
> >  				      width, height,
> > -				      fmt->format.width, fmt->format.height);
> > +				      format->width, format->height);
> > +	ov2740_update_pad_format(mode, pix_fmt);
> > +	*format = *src_pix_fmt = *pix_fmt;
> >  
> > -	ov2740_update_pad_format(mode, &fmt->format);
> > -	*v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
> > +	meta_fmt->code = MEDIA_BUS_FMT_OV2740_EMBEDDED;
> > +	meta_fmt->width = OV2740_META_WIDTH;
> > +	meta_fmt->height = OV2740_META_HEIGHT;
> > +	*src_meta_fmt = *meta_fmt;
> > +	src_meta_fmt->code = MEDIA_BUS_FMT_META_10;
> >  
> > -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> > +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> >  		return 0;
> >  
> >  	ov2740->cur_mode = mode;
> > @@ -1049,6 +1101,14 @@ static int ov2740_set_format(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > +static int ov2740_set_format(struct v4l2_subdev *sd,
> > +			     struct v4l2_subdev_state *sd_state,
> > +			     struct v4l2_subdev_format *fmt)
> > +{
> > +	return __ov2740_set_format(sd, sd_state, &fmt->format, fmt->which,
> > +				   fmt->pad, fmt->stream);
> > +}
> > +
> >  static int ov2740_enum_mbus_code(struct v4l2_subdev *sd,
> >  				 struct v4l2_subdev_state *sd_state,
> >  				 struct v4l2_subdev_mbus_code_enum *code)
> 
> You need to update this too, to enumerate the correct format on the
> different pads and streams.
> 
> > @@ -1085,10 +1145,68 @@ static int ov2740_enum_frame_size(struct v4l2_subdev *sd,
> >  static int ov2740_init_state(struct v4l2_subdev *sd,
> >  			     struct v4l2_subdev_state *sd_state)
> >  {
> > +	struct v4l2_subdev_route routes[] = {
> > +		{
> > +			.sink_pad = OV2740_PAD_PIXEL,
> > +			.source_pad = OV2740_PAD_SOURCE,
> > +			.source_stream = OV2740_STREAM_PIXEL,
> > +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> > +		}, {
> > +			.sink_pad = OV2740_PAD_META,
> > +			.source_pad = OV2740_PAD_SOURCE,
> > +			.source_stream = OV2740_STREAM_META,
> > +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> > +		},
> > +	};
> > +	struct v4l2_subdev_krouting routing = {
> > +		.routes = routes,
> > +		.num_routes = ARRAY_SIZE(routes),
> > +	};
> > +	struct v4l2_subdev_state *active_state;
> > +	struct v4l2_mbus_framefmt format = { 0 };
> >  	struct ov2740 *ov2740 = to_ov2740(sd);
> > +	int ret;
> > +
> > +	ret = v4l2_subdev_set_routing(sd, sd_state, &routing);
> > +	if (ret)
> > +		return ret;
> > +
> > +	active_state = v4l2_subdev_get_locked_active_state(sd);
> 
> There's a lockdep assertion that will trip when initializing any try
> state.

Uh-oh.

> 
> > +
> > +	ov2740_update_pad_format(&ov2740->supported_modes[0], &format);
> > +
> > +	return __ov2740_set_format(sd, sd_state, &format,
> > +				   active_state == sd_state ?
> > +				   V4L2_SUBDEV_FORMAT_ACTIVE :
> > +				   V4L2_SUBDEV_FORMAT_TRY, OV2740_PAD_PIXEL, 0);
> 
> Move the code specific to the active state from __ov2740_set_format() to
> ov2740_set_format(), and drop the which parameter to
> __ov2740_set_format(). You'll simplify the code here.

I'll see how this would work for v9.

> 
> > +}
> > +
> > +static int ov2740_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > +				 struct v4l2_mbus_frame_desc *desc)
> > +{
> > +	struct v4l2_mbus_frame_desc_entry *entry = desc->entry;
> > +	struct v4l2_subdev_state *sd_state;
> > +	struct v4l2_mbus_framefmt *fmt;
> > +
> > +	desc->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> > +
> > +	sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> > +	fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> > +					   OV2740_STREAM_PIXEL);
> > +	entry->pixelcode = fmt->code;
> > +	v4l2_subdev_unlock_state(sd_state);
> > +
> > +	entry->stream = OV2740_STREAM_PIXEL;
> > +	entry->bus.csi2.dt = MIPI_CSI2_DT_RAW10;
> > +	entry++;
> > +	desc->num_entries++;
> 
> I think addressing entries explicitly would be clearer.
> 
> 	entry[0].stream = ...;
> 	entry[0].bus.csi2.dt = ...;
> 
> 	...
> 
> 	desc->num_entries = 2;

I think we can disagree on this. I asked you to do the other way around in
the other patch. ;-)

> 
> > +
> > +	entry->pixelcode = MEDIA_BUS_FMT_META_8;
> > +	entry->stream = OV2740_STREAM_META;
> > +	entry->bus.csi2.dt = MIPI_CSI2_DT_GENERIC_LONG(1);
> 
> As Bingbu mentioned, this is not correct.

With default settings it would have seemed to be, but I think we can switch
to the embedded data type.

> 
> > +	entry++;
> > +	desc->num_entries++;
> >  
> > -	ov2740_update_pad_format(&ov2740->supported_modes[0],
> > -				 v4l2_subdev_state_get_format(sd_state, 0));
> >  	return 0;
> >  }
> >  
> > @@ -1103,6 +1221,7 @@ static const struct v4l2_subdev_pad_ops ov2740_pad_ops = {
> >  	.enum_frame_size = ov2740_enum_frame_size,
> >  	.enable_streams = ov2740_enable_streams,
> >  	.disable_streams = ov2740_disable_streams,
> > +	.get_frame_desc = ov2740_get_frame_desc,
> >  };
> >  
> >  static const struct v4l2_subdev_ops ov2740_subdev_ops = {
> > @@ -1369,11 +1488,16 @@ static int ov2740_probe(struct i2c_client *client)
> >  	}
> >  
> >  	ov2740->sd.state_lock = ov2740->ctrl_handler.lock;
> > -	ov2740->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +	ov2740->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
> >  	ov2740->sd.entity.ops = &ov2740_subdev_entity_ops;
> >  	ov2740->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > -	ov2740->pad.flags = MEDIA_PAD_FL_SOURCE;
> > -	ret = media_entity_pads_init(&ov2740->sd.entity, 1, &ov2740->pad);
> > +	ov2740->pads[OV2740_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> > +	ov2740->pads[OV2740_PAD_PIXEL].flags =
> > +		MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
> > +	ov2740->pads[OV2740_PAD_META].flags =
> > +		MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
> > +	ret = media_entity_pads_init(&ov2740->sd.entity,
> > +				     ARRAY_SIZE(ov2740->pads), ov2740->pads);
> >  	if (ret) {
> >  		dev_err_probe(dev, ret, "failed to init entity pads\n");
> >  		goto probe_error_v4l2_ctrl_handler_free;
>
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index df57f0096e98..7488b2535071 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -11,6 +11,7 @@ 
 #include <linux/pm_runtime.h>
 #include <linux/nvmem-provider.h>
 #include <linux/regmap.h>
+#include <media/mipi-csi2.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -71,11 +72,31 @@ 
 #define OV2740_REG_ISP_CTRL00		0x5000
 /* ISP CTRL01 */
 #define OV2740_REG_ISP_CTRL01		0x5001
+
+/* Embedded data line location control */
+#define OV2740_REG_EMBEDDED_FLAG	0x5a08
+#define OV2740_EMBEDDED_FLAG_FOOTER	BIT(2) /* otherwise it's in header */
+#define OV2740_EMBEDDED_FLAG_MYSTERY	BIT(1)
 /* Customer Addresses: 0x7010 - 0x710F */
 #define CUSTOMER_USE_OTP_SIZE		0x100
 /* OTP registers from sensor */
 #define OV2740_REG_OTP_CUSTOMER		0x7010
 
+enum {
+	OV2740_PAD_SOURCE,
+	OV2740_PAD_PIXEL,
+	OV2740_PAD_META,
+	OV2740_NUM_PADS,
+};
+
+enum {
+	OV2740_STREAM_PIXEL,
+	OV2740_STREAM_META,
+};
+
+#define OV2740_META_WIDTH		100U /* 97 bytes of actual data */
+#define OV2740_META_HEIGHT		1U
+
 struct nvm_data {
 	struct nvmem_device *nvmem;
 	struct regmap *regmap;
@@ -513,7 +534,7 @@  static const struct ov2740_mode supported_modes_180mhz[] = {
 
 struct ov2740 {
 	struct v4l2_subdev sd;
-	struct media_pad pad;
+	struct media_pad pads[OV2740_NUM_PADS];
 	struct v4l2_ctrl_handler ctrl_handler;
 
 	/* V4L2 Controls */
@@ -976,6 +997,11 @@  static int ov2740_enable_streams(struct v4l2_subdev *sd,
 	if (ret)
 		goto out_pm_put;
 
+	ret = ov2740_write_reg(ov2740, OV2740_REG_EMBEDDED_FLAG, 1,
+			       OV2740_EMBEDDED_FLAG_MYSTERY);
+	if (ret)
+		return ret;
+
 	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
 			       OV2740_MODE_STREAMING);
 	if (ret) {
@@ -1013,23 +1039,49 @@  static int ov2740_disable_streams(struct v4l2_subdev *sd,
 	return ret;
 }
 
-static int ov2740_set_format(struct v4l2_subdev *sd,
-			     struct v4l2_subdev_state *sd_state,
-			     struct v4l2_subdev_format *fmt)
+static int __ov2740_set_format(struct v4l2_subdev *sd,
+			       struct v4l2_subdev_state *sd_state,
+			       struct v4l2_mbus_framefmt *format,
+			       enum v4l2_subdev_format_whence which,
+			       unsigned int pad, unsigned int stream)
 {
+	struct v4l2_mbus_framefmt *src_pix_fmt, *src_meta_fmt, *pix_fmt,
+		*meta_fmt;
 	struct ov2740 *ov2740 = to_ov2740(sd);
 	const struct ov2740_mode *mode;
 	s32 vblank_def, h_blank;
 
+	/*
+	 * Allow setting format on internal pixel pad as well as the source
+	 * pad's pixel stream (for compatibility).
+	 */
+	if (pad == OV2740_PAD_SOURCE || pad == OV2740_PAD_META ||
+	    stream == OV2740_STREAM_META) {
+		*format = *v4l2_subdev_state_get_format(sd_state, pad, stream);
+		return 0;
+	}
+
+	pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_PIXEL, 0);
+	meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_META, 0);
+	src_pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
+						   OV2740_STREAM_PIXEL);
+	src_meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
+						    OV2740_STREAM_META);
+
 	mode = v4l2_find_nearest_size(ov2740->supported_modes,
 				      ov2740->supported_modes_count,
 				      width, height,
-				      fmt->format.width, fmt->format.height);
+				      format->width, format->height);
+	ov2740_update_pad_format(mode, pix_fmt);
+	*format = *src_pix_fmt = *pix_fmt;
 
-	ov2740_update_pad_format(mode, &fmt->format);
-	*v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
+	meta_fmt->code = MEDIA_BUS_FMT_OV2740_EMBEDDED;
+	meta_fmt->width = OV2740_META_WIDTH;
+	meta_fmt->height = OV2740_META_HEIGHT;
+	*src_meta_fmt = *meta_fmt;
+	src_meta_fmt->code = MEDIA_BUS_FMT_META_10;
 
-	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
 		return 0;
 
 	ov2740->cur_mode = mode;
@@ -1049,6 +1101,14 @@  static int ov2740_set_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int ov2740_set_format(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_state *sd_state,
+			     struct v4l2_subdev_format *fmt)
+{
+	return __ov2740_set_format(sd, sd_state, &fmt->format, fmt->which,
+				   fmt->pad, fmt->stream);
+}
+
 static int ov2740_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -1085,10 +1145,68 @@  static int ov2740_enum_frame_size(struct v4l2_subdev *sd,
 static int ov2740_init_state(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_state *sd_state)
 {
+	struct v4l2_subdev_route routes[] = {
+		{
+			.sink_pad = OV2740_PAD_PIXEL,
+			.source_pad = OV2740_PAD_SOURCE,
+			.source_stream = OV2740_STREAM_PIXEL,
+			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
+		}, {
+			.sink_pad = OV2740_PAD_META,
+			.source_pad = OV2740_PAD_SOURCE,
+			.source_stream = OV2740_STREAM_META,
+			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
+		},
+	};
+	struct v4l2_subdev_krouting routing = {
+		.routes = routes,
+		.num_routes = ARRAY_SIZE(routes),
+	};
+	struct v4l2_subdev_state *active_state;
+	struct v4l2_mbus_framefmt format = { 0 };
 	struct ov2740 *ov2740 = to_ov2740(sd);
+	int ret;
+
+	ret = v4l2_subdev_set_routing(sd, sd_state, &routing);
+	if (ret)
+		return ret;
+
+	active_state = v4l2_subdev_get_locked_active_state(sd);
+
+	ov2740_update_pad_format(&ov2740->supported_modes[0], &format);
+
+	return __ov2740_set_format(sd, sd_state, &format,
+				   active_state == sd_state ?
+				   V4L2_SUBDEV_FORMAT_ACTIVE :
+				   V4L2_SUBDEV_FORMAT_TRY, OV2740_PAD_PIXEL, 0);
+}
+
+static int ov2740_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				 struct v4l2_mbus_frame_desc *desc)
+{
+	struct v4l2_mbus_frame_desc_entry *entry = desc->entry;
+	struct v4l2_subdev_state *sd_state;
+	struct v4l2_mbus_framefmt *fmt;
+
+	desc->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
+
+	sd_state = v4l2_subdev_lock_and_get_active_state(sd);
+	fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
+					   OV2740_STREAM_PIXEL);
+	entry->pixelcode = fmt->code;
+	v4l2_subdev_unlock_state(sd_state);
+
+	entry->stream = OV2740_STREAM_PIXEL;
+	entry->bus.csi2.dt = MIPI_CSI2_DT_RAW10;
+	entry++;
+	desc->num_entries++;
+
+	entry->pixelcode = MEDIA_BUS_FMT_META_8;
+	entry->stream = OV2740_STREAM_META;
+	entry->bus.csi2.dt = MIPI_CSI2_DT_GENERIC_LONG(1);
+	entry++;
+	desc->num_entries++;
 
-	ov2740_update_pad_format(&ov2740->supported_modes[0],
-				 v4l2_subdev_state_get_format(sd_state, 0));
 	return 0;
 }
 
@@ -1103,6 +1221,7 @@  static const struct v4l2_subdev_pad_ops ov2740_pad_ops = {
 	.enum_frame_size = ov2740_enum_frame_size,
 	.enable_streams = ov2740_enable_streams,
 	.disable_streams = ov2740_disable_streams,
+	.get_frame_desc = ov2740_get_frame_desc,
 };
 
 static const struct v4l2_subdev_ops ov2740_subdev_ops = {
@@ -1369,11 +1488,16 @@  static int ov2740_probe(struct i2c_client *client)
 	}
 
 	ov2740->sd.state_lock = ov2740->ctrl_handler.lock;
-	ov2740->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	ov2740->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
 	ov2740->sd.entity.ops = &ov2740_subdev_entity_ops;
 	ov2740->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
-	ov2740->pad.flags = MEDIA_PAD_FL_SOURCE;
-	ret = media_entity_pads_init(&ov2740->sd.entity, 1, &ov2740->pad);
+	ov2740->pads[OV2740_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+	ov2740->pads[OV2740_PAD_PIXEL].flags =
+		MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
+	ov2740->pads[OV2740_PAD_META].flags =
+		MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
+	ret = media_entity_pads_init(&ov2740->sd.entity,
+				     ARRAY_SIZE(ov2740->pads), ov2740->pads);
 	if (ret) {
 		dev_err_probe(dev, ret, "failed to init entity pads\n");
 		goto probe_error_v4l2_ctrl_handler_free;