Re: [PATCH] media: imx: Use get_mbus_config instead of parsing upstream DT endpoints

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

 



Hi Philipp

On Thu, Aug 18, 2022 at 02:42:53PM +0200, Philipp Zabel wrote:
> Stop parsing upstream neighbors' device-tree endpoints to retrieve the
> media bus configuration. Instead use the get_mbus_config op and throw an
> error if the upstream subdevice does not implement it.
>
> Also drop the corresponding TODO entry and the now unused
> imx_media_get_pad_fwnode() function.
>
> Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> ---
>  drivers/staging/media/imx/TODO              |  12 --
>  drivers/staging/media/imx/imx-media-csi.c   | 122 +++++++++-----------
>  drivers/staging/media/imx/imx-media-utils.c |  33 ------
>  drivers/staging/media/imx/imx-media.h       |   1 -
>  4 files changed, 56 insertions(+), 112 deletions(-)
>
> diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
> index 5d3a337c8702..62d0426445c2 100644
> --- a/drivers/staging/media/imx/TODO
> +++ b/drivers/staging/media/imx/TODO
> @@ -2,18 +2,6 @@
>  - The Frame Interval Monitor could be exported to v4l2-core for
>    general use.
>
> -- The CSI subdevice parses its nearest upstream neighbor's device-tree
> -  bus config in order to setup the CSI. Laurent Pinchart argues that
> -  instead the CSI subdev should call its neighbor's g_mbus_config op
> -  (which should be propagated if necessary) to get this info. However
> -  Hans Verkuil is planning to remove the g_mbus_config op. For now this
> -  driver uses the parsed DT bus config method until this issue is
> -  resolved.
> -
> -  2020-06: g_mbus has been removed in favour of the get_mbus_config pad
> -  operation which should be used to avoid parsing the remote endpoint
> -  configuration.
> -
>  - This media driver supports inheriting V4L2 controls to the
>    video capture devices, from the subdevices in the capture device's
>    pipeline. The controls for each capture device are updated in the
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index a9377ea8379b..8b5682228d77 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -97,8 +97,8 @@ struct csi_priv {
>  	/* the mipi virtual channel number at link validate */
>  	int vc_num;
>
> -	/* the upstream endpoint CSI is receiving from */
> -	struct v4l2_fwnode_endpoint upstream_ep;
> +	/* media bus config of the upstream subdevice CSI is receiving from */
> +	struct v4l2_mbus_config mbus_cfg;
>
>  	spinlock_t irqlock; /* protect eof_irq handler */
>  	struct timer_list eof_timeout_timer;
> @@ -125,14 +125,14 @@ static inline struct csi_priv *notifier_to_dev(struct v4l2_async_notifier *n)
>  	return container_of(n, struct csi_priv, notifier);
>  }
>
> -static inline bool is_parallel_bus(struct v4l2_fwnode_endpoint *ep)
> +static inline bool is_parallel_bus(struct v4l2_mbus_config *mbus_cfg)
>  {
> -	return ep->bus_type != V4L2_MBUS_CSI2_DPHY;
> +	return mbus_cfg->type != V4L2_MBUS_CSI2_DPHY;
>  }
>
> -static inline bool is_parallel_16bit_bus(struct v4l2_fwnode_endpoint *ep)
> +static inline bool is_parallel_16bit_bus(struct v4l2_mbus_config *mbus_cfg)
>  {
> -	return is_parallel_bus(ep) && ep->bus.parallel.bus_width >= 16;
> +	return is_parallel_bus(mbus_cfg) && mbus_cfg->bus.parallel.bus_width >= 16;
>  }
>
>  /*
> @@ -145,33 +145,31 @@ static inline bool is_parallel_16bit_bus(struct v4l2_fwnode_endpoint *ep)
>   * - the CSI is receiving from an 8-bit parallel bus and the incoming
>   *   media bus format is other than UYVY8_2X8/YUYV8_2X8.
>   */
> -static inline bool requires_passthrough(struct v4l2_fwnode_endpoint *ep,
> +static inline bool requires_passthrough(struct v4l2_mbus_config *mbus_cfg,
>  					struct v4l2_mbus_framefmt *infmt,
>  					const struct imx_media_pixfmt *incc)
>  {
> -	if (ep->bus_type == V4L2_MBUS_BT656) // including BT.1120
> +	if (mbus_cfg->type == V4L2_MBUS_BT656) // including BT.1120
>  		return false;
>
> -	return incc->bayer || is_parallel_16bit_bus(ep) ||
> -		(is_parallel_bus(ep) &&
> +	return incc->bayer || is_parallel_16bit_bus(mbus_cfg) ||
> +		(is_parallel_bus(mbus_cfg) &&
>  		 infmt->code != MEDIA_BUS_FMT_UYVY8_2X8 &&
>  		 infmt->code != MEDIA_BUS_FMT_YUYV8_2X8);
>  }
>
>  /*
> - * Parses the fwnode endpoint from the source pad of the entity
> - * connected to this CSI. This will either be the entity directly
> - * upstream from the CSI-2 receiver, directly upstream from the
> - * video mux, or directly upstream from the CSI itself. The endpoint
> - * is needed to determine the bus type and bus config coming into
> - * the CSI.
> + * Queries the media bus config of the upstream entity that provides data to
> + * the CSI. This will either be the entity directly upstream from the CSI-2
> + * receiver, directly upstream from a video mux, or directly upstream from
> + * the CSI itself.
>   */
> -static int csi_get_upstream_endpoint(struct csi_priv *priv,
> -				     struct v4l2_fwnode_endpoint *ep)
> +static int csi_get_upstream_mbus_config(struct csi_priv *priv,
> +					struct v4l2_mbus_config *mbus_cfg)
>  {
> -	struct fwnode_handle *endpoint;
> -	struct v4l2_subdev *sd;
> -	struct media_pad *pad;
> +	struct v4l2_subdev *sd, *remote_sd;
> +	struct media_pad *remote_pad;
> +	int ret;
>
>  	if (!IS_ENABLED(CONFIG_OF))
>  		return -ENXIO;

Is this still necessary ?

> @@ -206,19 +204,21 @@ static int csi_get_upstream_endpoint(struct csi_priv *priv,
>  	}
>
>  	/* get source pad of entity directly upstream from sd */
> -	pad = imx_media_pipeline_pad(&sd->entity, 0, 0, true);
> -	if (!pad)
> -		return -ENODEV;
> -
> -	endpoint = imx_media_get_pad_fwnode(pad);
> -	if (IS_ERR(endpoint))
> -		return PTR_ERR(endpoint);
> +	remote_pad = media_entity_remote_pad_unique(&sd->entity,
> +						    MEDIA_PAD_FL_SOURCE);
> +	if (IS_ERR(remote_pad))
> +		return PTR_ERR(remote_pad);
>
> -	v4l2_fwnode_endpoint_parse(endpoint, ep);
> +	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
>
> -	fwnode_handle_put(endpoint);
> +	ret = v4l2_subdev_call(remote_sd, pad, get_mbus_config,
> +			       remote_pad->index, mbus_cfg);
> +	if (ret == -ENOIOCTLCMD)
> +		v4l2_err(&priv->sd,
> +			 "upstream entity %s does not implement get_mbus_config()\n",
> +			 remote_pad->entity->name);

This makes mandatory for the remote to implement .get_mbus_config().
I think it is fine, even more this is staging, and I don't think you
can use any meaningful default in case the op is not implemented...

>
> -	return 0;
> +	return ret;
>  }
>
>  static void csi_idmac_put_ipu_resources(struct csi_priv *priv)
> @@ -435,7 +435,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
>  	image.phys0 = phys[0];
>  	image.phys1 = phys[1];
>
> -	passthrough = requires_passthrough(&priv->upstream_ep, infmt, incc);
> +	passthrough = requires_passthrough(&priv->mbus_cfg, infmt, incc);
>  	passthrough_cycles = 1;
>
>  	/*
> @@ -708,7 +708,6 @@ static int csi_setup(struct csi_priv *priv)
>  {
>  	struct v4l2_mbus_framefmt *infmt, *outfmt;
>  	const struct imx_media_pixfmt *incc;
> -	struct v4l2_mbus_config mbus_cfg;
>  	struct v4l2_mbus_framefmt if_fmt;
>  	struct v4l2_rect crop;
>
> @@ -716,13 +715,6 @@ static int csi_setup(struct csi_priv *priv)
>  	incc = priv->cc[CSI_SINK_PAD];
>  	outfmt = &priv->format_mbus[priv->active_output_pad];
>
> -	/* compose mbus_config from the upstream endpoint */
> -	mbus_cfg.type = priv->upstream_ep.bus_type;
> -	if (is_parallel_bus(&priv->upstream_ep))
> -		mbus_cfg.bus.parallel = priv->upstream_ep.bus.parallel;
> -	else
> -		mbus_cfg.bus.mipi_csi2 = priv->upstream_ep.bus.mipi_csi2;
> -
>  	if_fmt = *infmt;
>  	crop = priv->crop;
>
> @@ -730,7 +722,7 @@ static int csi_setup(struct csi_priv *priv)
>  	 * if cycles is set, we need to handle this over multiple cycles as
>  	 * generic/bayer data
>  	 */
> -	if (is_parallel_bus(&priv->upstream_ep) && incc->cycles) {
> +	if (is_parallel_bus(&priv->mbus_cfg) && incc->cycles) {
>  		if_fmt.width *= incc->cycles;
>  		crop.width *= incc->cycles;
>  	}
> @@ -741,7 +733,7 @@ static int csi_setup(struct csi_priv *priv)
>  			     priv->crop.width == 2 * priv->compose.width,
>  			     priv->crop.height == 2 * priv->compose.height);
>
> -	ipu_csi_init_interface(priv->csi, &mbus_cfg, &if_fmt, outfmt);
> +	ipu_csi_init_interface(priv->csi, &priv->mbus_cfg, &if_fmt, outfmt);
>
>  	ipu_csi_set_dest(priv->csi, priv->dest);
>
> @@ -769,7 +761,7 @@ static int csi_start(struct csi_priv *priv)
>  		return ret;
>
>  	/* Skip first few frames from a BT.656 source */
> -	if (priv->upstream_ep.bus_type == V4L2_MBUS_BT656) {
> +	if (priv->mbus_cfg.type == V4L2_MBUS_BT656) {
>  		u32 delay_usec, bad_frames = 20;
>
>  		delay_usec = DIV_ROUND_UP_ULL((u64)USEC_PER_SEC *
> @@ -1118,7 +1110,7 @@ static int csi_link_validate(struct v4l2_subdev *sd,
>  			     struct v4l2_subdev_format *sink_fmt)
>  {
>  	struct csi_priv *priv = v4l2_get_subdevdata(sd);
> -	struct v4l2_fwnode_endpoint upstream_ep = { .bus_type = 0 };
> +	struct v4l2_mbus_config mbus_cfg = { .type = 0 };
>  	bool is_csi2;
>  	int ret;
>
> @@ -1127,16 +1119,16 @@ static int csi_link_validate(struct v4l2_subdev *sd,
>  	if (ret)
>  		return ret;
>
> -	ret = csi_get_upstream_endpoint(priv, &upstream_ep);
> +	ret = csi_get_upstream_mbus_config(priv, &mbus_cfg);
>  	if (ret) {
> -		v4l2_err(&priv->sd, "failed to find upstream endpoint\n");
> +		v4l2_err(&priv->sd, "failed to find upstream media bus configuration\n");
>  		return ret;
>  	}
>
>  	mutex_lock(&priv->lock);
>
> -	priv->upstream_ep = upstream_ep;
> -	is_csi2 = !is_parallel_bus(&upstream_ep);
> +	priv->mbus_cfg = mbus_cfg;
> +	is_csi2 = !is_parallel_bus(&mbus_cfg);
>  	if (is_csi2) {
>  		/*
>  		 * NOTE! It seems the virtual channels from the mipi csi-2
> @@ -1192,7 +1184,7 @@ static void csi_try_crop(struct csi_priv *priv,
>  			 struct v4l2_rect *crop,
>  			 struct v4l2_subdev_state *sd_state,
>  			 struct v4l2_mbus_framefmt *infmt,
> -			 struct v4l2_fwnode_endpoint *upstream_ep)
> +			 struct v4l2_mbus_config *mbus_cfg)
>  {
>  	u32 in_height;
>
> @@ -1216,7 +1208,7 @@ static void csi_try_crop(struct csi_priv *priv,
>  	 * sync, so fix it to NTSC/PAL active lines. NTSC contains
>  	 * 2 extra lines of active video that need to be cropped.
>  	 */
> -	if (upstream_ep->bus_type == V4L2_MBUS_BT656 &&
> +	if (mbus_cfg->type == V4L2_MBUS_BT656 &&
>  	    (V4L2_FIELD_HAS_BOTH(infmt->field) ||
>  	     infmt->field == V4L2_FIELD_ALTERNATE)) {
>  		crop->height = in_height;
> @@ -1233,7 +1225,7 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd,
>  			      struct v4l2_subdev_mbus_code_enum *code)
>  {
>  	struct csi_priv *priv = v4l2_get_subdevdata(sd);
> -	struct v4l2_fwnode_endpoint upstream_ep = { .bus_type = 0 };
> +	struct v4l2_mbus_config mbus_cfg = { .type = 0 };
>  	const struct imx_media_pixfmt *incc;
>  	struct v4l2_mbus_framefmt *infmt;
>  	int ret = 0;
> @@ -1254,13 +1246,13 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd,
>  		break;
>  	case CSI_SRC_PAD_DIRECT:
>  	case CSI_SRC_PAD_IDMAC:
> -		ret = csi_get_upstream_endpoint(priv, &upstream_ep);
> +		ret = csi_get_upstream_mbus_config(priv, &mbus_cfg);
>  		if (ret) {
>  			v4l2_err(&priv->sd, "failed to find upstream endpoint\n");

Should the error message be updated ?

>  			goto out;
>  		}
>
> -		if (requires_passthrough(&upstream_ep, infmt, incc)) {
> +		if (requires_passthrough(&mbus_cfg, infmt, incc)) {
>  			if (code->index != 0) {
>  				ret = -EINVAL;
>  				goto out;
> @@ -1430,7 +1422,7 @@ static void csi_try_field(struct csi_priv *priv,
>  }
>
>  static void csi_try_fmt(struct csi_priv *priv,
> -			struct v4l2_fwnode_endpoint *upstream_ep,
> +			struct v4l2_mbus_config *mbus_cfg,
>  			struct v4l2_subdev_state *sd_state,
>  			struct v4l2_subdev_format *sdformat,
>  			struct v4l2_rect *crop,
> @@ -1451,7 +1443,7 @@ static void csi_try_fmt(struct csi_priv *priv,
>  		sdformat->format.width = compose->width;
>  		sdformat->format.height = compose->height;
>
> -		if (requires_passthrough(upstream_ep, infmt, incc)) {
> +		if (requires_passthrough(mbus_cfg, infmt, incc)) {
>  			sdformat->format.code = infmt->code;
>  			*cc = incc;
>  		} else {
> @@ -1501,8 +1493,7 @@ static void csi_try_fmt(struct csi_priv *priv,
>  		crop->height = sdformat->format.height;
>  		if (sdformat->format.field == V4L2_FIELD_ALTERNATE)
>  			crop->height *= 2;
> -		csi_try_crop(priv, crop, sd_state, &sdformat->format,
> -			     upstream_ep);
> +		csi_try_crop(priv, crop, sd_state, &sdformat->format, mbus_cfg);
>  		compose->left = 0;
>  		compose->top = 0;
>  		compose->width = crop->width;
> @@ -1520,7 +1511,7 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
>  		       struct v4l2_subdev_format *sdformat)
>  {
>  	struct csi_priv *priv = v4l2_get_subdevdata(sd);
> -	struct v4l2_fwnode_endpoint upstream_ep = { .bus_type = 0 };
> +	struct v4l2_mbus_config mbus_cfg = { .type = 0 };
>  	const struct imx_media_pixfmt *cc;
>  	struct v4l2_mbus_framefmt *fmt;
>  	struct v4l2_rect *crop, *compose;
> @@ -1529,7 +1520,7 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
>  	if (sdformat->pad >= CSI_NUM_PADS)
>  		return -EINVAL;
>
> -	ret = csi_get_upstream_endpoint(priv, &upstream_ep);
> +	ret = csi_get_upstream_mbus_config(priv, &mbus_cfg);
>  	if (ret) {
>  		v4l2_err(&priv->sd, "failed to find upstream endpoint\n");

Same here

>  		return ret;
> @@ -1545,8 +1536,7 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
>  	crop = __csi_get_crop(priv, sd_state, sdformat->which);
>  	compose = __csi_get_compose(priv, sd_state, sdformat->which);
>
> -	csi_try_fmt(priv, &upstream_ep, sd_state, sdformat, crop, compose,
> -		    &cc);
> +	csi_try_fmt(priv, &mbus_cfg, sd_state, sdformat, crop, compose, &cc);
>
>  	fmt = __csi_get_fmt(priv, sd_state, sdformat->pad, sdformat->which);
>  	*fmt = sdformat->format;
> @@ -1563,8 +1553,8 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
>  			format.pad = pad;
>  			format.which = sdformat->which;
>  			format.format = sdformat->format;
> -			csi_try_fmt(priv, &upstream_ep, sd_state, &format,
> -				    NULL, compose, &outcc);
> +			csi_try_fmt(priv, &mbus_cfg, sd_state, &format, NULL,
> +				    compose, &outcc);
>
>  			outfmt = __csi_get_fmt(priv, sd_state, pad,
>  					       sdformat->which);
> @@ -1652,7 +1642,7 @@ static int csi_set_selection(struct v4l2_subdev *sd,
>  			     struct v4l2_subdev_selection *sel)
>  {
>  	struct csi_priv *priv = v4l2_get_subdevdata(sd);
> -	struct v4l2_fwnode_endpoint upstream_ep = { .bus_type = 0 };
> +	struct v4l2_mbus_config mbus_cfg = { .type = 0 };
>  	struct v4l2_mbus_framefmt *infmt;
>  	struct v4l2_rect *crop, *compose;
>  	int pad, ret;
> @@ -1660,7 +1650,7 @@ static int csi_set_selection(struct v4l2_subdev *sd,
>  	if (sel->pad != CSI_SINK_PAD)
>  		return -EINVAL;
>
> -	ret = csi_get_upstream_endpoint(priv, &upstream_ep);
> +	ret = csi_get_upstream_mbus_config(priv, &mbus_cfg);
>  	if (ret) {
>  		v4l2_err(&priv->sd, "failed to find upstream endpoint\n");

And same here!

>  		return ret;
> @@ -1691,7 +1681,7 @@ static int csi_set_selection(struct v4l2_subdev *sd,
>  			goto out;
>  		}
>
> -		csi_try_crop(priv, &sel->r, sd_state, infmt, &upstream_ep);
> +		csi_try_crop(priv, &sel->r, sd_state, infmt, &mbus_cfg);
>
>  		*crop = sel->r;
>
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 294c808b2ebe..70dc89d6503b 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -813,39 +813,6 @@ imx_media_pipeline_video_device(struct media_entity *start_entity,
>  }
>  EXPORT_SYMBOL_GPL(imx_media_pipeline_video_device);
>
> -/*
> - * Find a fwnode endpoint that maps to the given subdevice's pad.
> - * If there are multiple endpoints that map to the pad, only the
> - * first endpoint encountered is returned.
> - *
> - * On success the refcount of the returned fwnode endpoint is
> - * incremented.
> - */
> -struct fwnode_handle *imx_media_get_pad_fwnode(struct media_pad *pad)
> -{
> -	struct fwnode_handle *endpoint;
> -	struct v4l2_subdev *sd;
> -
> -	if (!is_media_entity_v4l2_subdev(pad->entity))
> -		return ERR_PTR(-ENODEV);
> -
> -	sd = media_entity_to_v4l2_subdev(pad->entity);
> -
> -	fwnode_graph_for_each_endpoint(dev_fwnode(sd->dev), endpoint) {
> -		int pad_idx = media_entity_get_fwnode_pad(&sd->entity,
> -							  endpoint,
> -							  pad->flags);
> -		if (pad_idx < 0)
> -			continue;
> -
> -		if (pad_idx == pad->index)
> -			return endpoint;
> -	}
> -
> -	return ERR_PTR(-ENODEV);
> -}
> -EXPORT_SYMBOL_GPL(imx_media_get_pad_fwnode);
> -
>  /*
>   * Turn current pipeline streaming on/off starting from entity.
>   */
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index 1aefbfde3486..573c97b7827a 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -219,7 +219,6 @@ imx_media_pipeline_subdev(struct media_entity *start_entity, u32 grp_id,
>  struct video_device *
>  imx_media_pipeline_video_device(struct media_entity *start_entity,
>  				enum v4l2_buf_type buftype, bool upstream);
> -struct fwnode_handle *imx_media_get_pad_fwnode(struct media_pad *pad);
>

All minors, with the error messages fixed
Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx>

Thanks
   j

>  struct imx_media_dma_buf {
>  	void          *virt;
> --
> 2.30.2
>




[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux