linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Cc: dave.stevenson@raspberrypi.com, devicetree@vger.kernel.org,
	kernel-list@raspberrypi.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
	linux-rpi-kernel@lists.infradead.org, lukasz@jany.st,
	mchehab@kernel.org, naush@raspberrypi.com, robh@kernel.org,
	tomi.valkeinen@ideasonboard.com,
	bcm-kernel-feedback-list@broadcom.com, stefan.wahren@i2se.com
Subject: Re: [PATCH v5 11/11] media: imx219: Add support for the V4L2 subdev active state
Date: Mon, 21 Feb 2022 09:25:02 +0200	[thread overview]
Message-ID: <YhM+TgcOgbyXgs/x@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20220208155027.891055-12-jeanmichel.hautbois@ideasonboard.com>

Hi Jean-Michel,

Thank you for the patch.

On Tue, Feb 08, 2022 at 04:50:27PM +0100, Jean-Michel Hautbois wrote:
> Now that we have multiplexed streams support in the V4L2 API, add
> support for the subdev active state in the sensor.
> 
> In order to use state to manage the active configuration, we need to
> initialize the subdevice state with a call to
> v4l2_subdev_init_finalize() before registering it. The call to
> v4l2_subdev_cleanup() is also required to release the resources before
> unregistering.
> 
> Accessing the configuration is then done after a call to
> v4l2_subdev_lock_state() and the set_fmt operation can then call
> v4l2_state_get_stream_format() directly.
> 
> The get_fmt operation does not need to be complex, and a simple call to
> v4l2_subdev_get_fmt will do the trick, as it will grab the configured
> format based on the state.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  drivers/media/i2c/imx219.c | 151 +++++++++++++++++++------------------
>  1 file changed, 77 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index f005ad8d2124..f6322f39c3f1 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -887,78 +887,43 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_state *sd_state,
>  				  struct v4l2_subdev_frame_size_enum *fse)
>  {
> -	struct imx219 *imx219 = to_imx219(sd);
> -	u32 code;
> -
> -	if (fse->index >= ARRAY_SIZE(supported_modes))
> -		return -EINVAL;
> -
> -	mutex_lock(&imx219->mutex);
> -	code = imx219_get_format_code(imx219, fse->code);
> -	mutex_unlock(&imx219->mutex);
> -	if (fse->code != code)
> -		return -EINVAL;
> -
> -	fse->min_width = supported_modes[fse->index].width;
> -	fse->max_width = fse->min_width;
> -	fse->min_height = supported_modes[fse->index].height;
> -	fse->max_height = fse->min_height;
> +	unsigned int i;
>  
> -	return 0;
> -}
> +	if (fse->stream == 0) {
> +		for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); ++i) {
> +			if (imx219_mbus_formats[i] == fse->code)
> +				break;
> +		}
>  
> -static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
> -{
> -	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> -	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> -	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> -							  fmt->colorspace,
> -							  fmt->ycbcr_enc);
> -	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> -}
> +		if (i == ARRAY_SIZE(imx219_mbus_formats))
> +			return -EINVAL;
>  
> -static void imx219_update_pad_format(struct imx219 *imx219,
> -				     const struct imx219_mode *mode,
> -				     struct v4l2_subdev_format *fmt)
> -{
> -	fmt->format.width = mode->width;
> -	fmt->format.height = mode->height;
> -	fmt->format.field = V4L2_FIELD_NONE;
> -	imx219_reset_colorspace(&fmt->format);
> -}
> +		if (fse->index >= ARRAY_SIZE(supported_modes))
> +			return -EINVAL;
>  
> -static int __imx219_get_pad_format(struct imx219 *imx219,
> -				   struct v4l2_subdev_state *sd_state,
> -				   struct v4l2_subdev_format *fmt)
> -{
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		struct v4l2_mbus_framefmt *try_fmt =
> -			v4l2_subdev_get_try_format(&imx219->sd, sd_state,
> -						   fmt->pad);
> -		/* update the code which could change due to vflip or hflip: */
> -		try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> -		fmt->format = *try_fmt;
> +		fse->min_width  = supported_modes[fse->index].width;
> +		fse->max_width  = fse->min_width;
> +		fse->max_height = supported_modes[fse->index].height;
> +		fse->min_height = fse->max_height;
>  	} else {
> -		imx219_update_pad_format(imx219, imx219->mode, fmt);
> -		fmt->format.code = imx219_get_format_code(imx219,
> -							  imx219->fmt.code);
> +		if (fse->code != MEDIA_BUS_FMT_METADATA_8)
> +			return -EINVAL;
> +
> +		fse->min_width = IMX219_EMBEDDED_LINE_WIDTH;
> +		fse->max_width = fse->min_width;
> +		fse->min_height = IMX219_NUM_EMBEDDED_LINES;
> +		fse->max_height = fse->min_height;
>  	}
>  
>  	return 0;
>  }
>  
> -static int imx219_get_pad_format(struct v4l2_subdev *sd,
> -				 struct v4l2_subdev_state *sd_state,
> -				 struct v4l2_subdev_format *fmt)
> +static void imx219_update_metadata_pad_format(struct v4l2_subdev_format *fmt)
>  {
> -	struct imx219 *imx219 = to_imx219(sd);
> -	int ret;
> -
> -	mutex_lock(&imx219->mutex);
> -	ret = __imx219_get_pad_format(imx219, sd_state, fmt);
> -	mutex_unlock(&imx219->mutex);
> -
> -	return ret;
> +	fmt->format.width = IMX219_EMBEDDED_LINE_WIDTH;
> +	fmt->format.height = IMX219_NUM_EMBEDDED_LINES;
> +	fmt->format.code = MEDIA_BUS_FMT_METADATA_8;
> +	fmt->format.field = V4L2_FIELD_NONE;
>  }
>  
>  static int imx219_set_pad_format(struct v4l2_subdev *sd,
> @@ -966,32 +931,58 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_format *fmt)
>  {
>  	struct imx219 *imx219 = to_imx219(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
>  	const struct imx219_mode *mode;
> -	struct v4l2_mbus_framefmt *framefmt;
> +	struct v4l2_mbus_framefmt *format;
>  	int exposure_max, exposure_def, hblank;
>  	unsigned int i;
> +	int ret = 0;
>  
>  	mutex_lock(&imx219->mutex);
>  
> +	if (fmt->pad != 0) {
> +		dev_err(&client->dev, "%s Could not get pad %d\n", __func__,
> +			fmt->pad);

Drop this message.

> +		ret = -EINVAL;
> +		goto done;
> +	}
> +
> +	if (fmt->stream == 1) {
> +		/* Only one embedded data mode is supported */
> +		imx219_update_metadata_pad_format(fmt);

		return v4l2_subdev_get_fmt(sd, sd_state, fmt);

will be simpler. You can drop the imx219_update_metadata_pad_format()
function. And move the mutex_lock() lower, after all the code that
doesn't touch fields of the imx219 structure.

> +		ret = 0;

Not needed.

> +		goto done;
> +	}
> +
>  	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
>  		if (imx219_mbus_formats[i] == fmt->format.code)
>  			break;
>  	if (i >= ARRAY_SIZE(imx219_mbus_formats))
>  		i = 0;
>  
> -	/* Bayer order varies with flips */
> -	fmt->format.code = imx219_get_format_code(imx219, imx219_mbus_formats[i]);
> -
>  	mode = v4l2_find_nearest_size(supported_modes,
>  				      ARRAY_SIZE(supported_modes),
>  				      width, height,
>  				      fmt->format.width, fmt->format.height);
> -	imx219_update_pad_format(imx219, mode, fmt);
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> -		*framefmt = fmt->format;
> -	} else if (imx219->mode != mode ||
> -		   imx219->fmt.code != fmt->format.code) {
> +
> +	v4l2_subdev_lock_state(sd_state);
> +
> +	/* Update the stored format and return it. */
> +	format = v4l2_state_get_stream_format(sd_state, fmt->pad, fmt->stream);
> +
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && imx219->streaming) {
> +		ret = -EBUSY;
> +		goto err_state;
> +	}
> +
> +	/* Bayer order varies with flips */
> +	format->code = imx219_get_format_code(imx219, imx219_mbus_formats[i]);
> +	format->width = mode->width;
> +	format->height = mode->height;
> +	/* Bayer order varies with flips */
> +	fmt->format = *format;
> +
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>  		imx219->fmt = fmt->format;

The whole point of using the active state is that you don't need to
store the format in the imx219 structure anymore.

This patch does too much. I'm tempted to ask you to first switch to
usage of the active state, even before adding routing support, then add
routing support without the embedded data stream, and finally add
embedded data support.

>  		imx219->mode = mode;
>  		/* Update limits and set FPS to default */
> @@ -1018,9 +1009,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  					 hblank);
>  	}
>  
> +err_state:
> +	v4l2_subdev_unlock_state(sd_state);
> +done:
>  	mutex_unlock(&imx219->mutex);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int imx219_set_framefmt(struct imx219 *imx219)
> @@ -1322,7 +1316,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
>  static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
>  	.init_cfg		= imx219_init_cfg,
>  	.enum_mbus_code		= imx219_enum_mbus_code,
> -	.get_fmt		= imx219_get_pad_format,
> +	.get_fmt		= v4l2_subdev_get_fmt,
>  	.set_fmt		= imx219_set_pad_format,
>  	.get_selection		= imx219_get_selection,
>  	.set_routing		= imx219_set_routing,
> @@ -1602,10 +1596,16 @@ static int imx219_probe(struct i2c_client *client)
>  		goto error_handler_free;
>  	}
>  
> +	ret = v4l2_subdev_init_finalize(sd);
> +	if (ret) {
> +		dev_err(dev, "failed to finalize sensor init: %d\n", ret);
> +		goto error_media_entity;
> +	}
> +
>  	ret = v4l2_async_register_subdev_sensor(sd);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> -		goto error_media_entity;
> +		goto error_free_state;
>  	}
>  
>  	/* Enable runtime PM and turn off the device */
> @@ -1615,6 +1615,8 @@ static int imx219_probe(struct i2c_client *client)
>  
>  	return 0;
>  
> +error_free_state:

error_subdev_cleanup:

> +	v4l2_subdev_cleanup(sd);
>  error_media_entity:
>  	media_entity_cleanup(&sd->entity);
>  
> @@ -1633,6 +1635,7 @@ static int imx219_remove(struct i2c_client *client)
>  	struct imx219 *imx219 = to_imx219(sd);
>  
>  	v4l2_async_unregister_subdev(sd);
> +	v4l2_subdev_cleanup(sd);
>  	media_entity_cleanup(&sd->entity);
>  	imx219_free_controls(imx219);
>  

-- 
Regards,

Laurent Pinchart

  reply	other threads:[~2022-02-21  7:26 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-08 15:50 [PATCH v5 00/11] Add support for BCM2835 camera interface (unicam) Jean-Michel Hautbois
2022-02-08 15:50 ` [PATCH v5 01/11] media: v4l: Add V4L2-PIX-FMT-Y12P format Jean-Michel Hautbois
2022-02-08 15:50 ` [PATCH v5 02/11] media: v4l: Add V4L2-PIX-FMT-Y14P format Jean-Michel Hautbois
2022-02-08 15:50 ` [PATCH v5 03/11] dt-bindings: media: Add bindings for bcm2835-unicam Jean-Michel Hautbois
2022-02-09 18:56   ` Rob Herring
2022-02-13 15:48   ` Stefan Wahren
2022-02-14  9:39     ` Maxime Ripard
2022-02-14  9:54       ` Laurent Pinchart
2022-02-14 11:32         ` Stefan Wahren
2022-02-21  7:10           ` Laurent Pinchart
2022-02-21 10:03             ` Maxime Ripard
2022-02-21 12:45             ` Stefan Wahren
2022-02-21 12:52               ` Laurent Pinchart
2022-02-25  8:19   ` Sakari Ailus
2022-02-08 15:50 ` [PATCH v5 04/11] media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface Jean-Michel Hautbois
2022-02-08 21:00   ` Stefan Wahren
2022-02-13 12:52     ` Laurent Pinchart
2022-02-13 11:17   ` Stefan Wahren
2022-02-13 12:49     ` Laurent Pinchart
2022-02-20 10:01   ` Stefan Wahren
2022-02-20 10:08     ` Laurent Pinchart
2022-02-21  9:55   ` Laurent Pinchart
2022-02-25  9:29   ` Sakari Ailus
2023-07-02 15:23     ` Laurent Pinchart
2023-07-02 18:18       ` Sakari Ailus
2023-07-02 21:45         ` Laurent Pinchart
2023-07-02 21:47           ` Laurent Pinchart
2023-07-02 21:56             ` Sakari Ailus
2023-07-02 22:01               ` Laurent Pinchart
2023-07-02 22:20                 ` Sakari Ailus
2023-07-02 22:28                   ` Laurent Pinchart
2023-07-02 22:33                     ` Sakari Ailus
2023-07-02 21:53           ` Sakari Ailus
2023-07-02 21:58             ` Laurent Pinchart
2022-02-08 15:50 ` [PATCH v5 05/11] media: MAINTAINERS: add bcm2835 unicam driver Jean-Michel Hautbois
2022-02-08 15:58   ` Laurent Pinchart
2022-02-08 15:50 ` [PATCH v5 06/11] ARM: dts: bcm2711: Add unicam CSI nodes Jean-Michel Hautbois
2022-02-13 10:35   ` Stefan Wahren
2022-02-13 13:51     ` Stefan Wahren
2022-02-08 15:50 ` [PATCH v5 07/11] media: imx219: Rename mbus codes array Jean-Michel Hautbois
2022-02-08 15:50 ` [PATCH v5 08/11] media: imx219: Switch from open to init_cfg Jean-Michel Hautbois
2022-02-08 16:02   ` Laurent Pinchart
2022-02-08 16:05     ` Laurent Pinchart
2022-02-08 15:50 ` [PATCH v5 09/11] media: imx219: Introduce the set_routing operation Jean-Michel Hautbois
2022-02-21  7:17   ` Laurent Pinchart
2022-02-08 15:50 ` [PATCH v5 10/11] media: imx219: use a local v4l2_subdev to simplify reading Jean-Michel Hautbois
2022-02-08 15:50 ` [PATCH v5 11/11] media: imx219: Add support for the V4L2 subdev active state Jean-Michel Hautbois
2022-02-21  7:25   ` Laurent Pinchart [this message]
2022-02-16 20:57 ` [PATCH v5 00/11] Add support for BCM2835 camera interface (unicam) Stefan Wahren
2022-02-20 14:30   ` Jean-Michel Hautbois
2022-02-26 17:18     ` Stefan Wahren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YhM+TgcOgbyXgs/x@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=bcm-kernel-feedback-list@broadcom.com \
    --cc=dave.stevenson@raspberrypi.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jeanmichel.hautbois@ideasonboard.com \
    --cc=kernel-list@raspberrypi.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=lukasz@jany.st \
    --cc=mchehab@kernel.org \
    --cc=naush@raspberrypi.com \
    --cc=robh@kernel.org \
    --cc=stefan.wahren@i2se.com \
    --cc=tomi.valkeinen@ideasonboard.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).