On 06/10/21 11:28PM, Sakari Ailus wrote:

Hi Pratyush,

On Wed, Sep 15, 2021 at 05:32:37PM +0530, Pratyush Yadav wrote:
...

quoted

+/*
+ * Find the input format. This is done by finding the first device in the
+ * pipeline which can tell us the current format. This could be the sensor, or
+ * this could be another device in the middle which is capable of format
+ * conversions.
+ */
+static int ti_csi2rx_validate_pipeline(struct ti_csi2rx_dev *csi)
+{
+	struct media_pipeline *pipe = &csi->pipe;
+	struct media_entity *entity;
+	struct v4l2_subdev *sd;
+	struct v4l2_subdev_format fmt;
+	struct v4l2_pix_format *pix = &csi->v_fmt.fmt.pix;
+	struct media_device *mdev = &csi->mdev;
+	const struct ti_csi2rx_fmt *ti_fmt;
+	int ret;
+
+	mutex_lock(&mdev->graph_mutex);
+	ret = media_graph_walk_init(&pipe->graph, mdev);
+	if (ret) {
+		mutex_unlock(&mdev->graph_mutex);
+		return ret;
+	}
+
+	media_graph_walk_start(&pipe->graph, &csi->vdev.entity);
+
+	while ((entity = media_graph_walk_next(&pipe->graph))) {
+		if (!is_media_entity_v4l2_subdev(entity))
+			continue;

You shouldn't rely on media_graph_walk_next() to return entities in a
particular order.

Ah, right. Need to drop this.

I'd suggest approach taken in isp_video_check_external_subdevs() (in
drivers/media/platform/omap3isp/ispvideo.c).

quoted

+
+		sd = media_entity_to_v4l2_subdev(entity);
+
+		fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+		fmt.pad = media_get_pad_index(entity, 0, PAD_SIGNAL_DEFAULT);
+
+		ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
+		if (ret && ret != -ENOIOCTLCMD) {
+			media_graph_walk_cleanup(&pipe->graph);
+			mutex_unlock(&mdev->graph_mutex);
+			return ret;
+		}
+
+		if (!ret)
+			break;
+	}
+
+	media_graph_walk_cleanup(&pipe->graph);
+	mutex_unlock(&mdev->graph_mutex);
+
+	/* Could not find input format. */
+	if (!entity)
+		return -EPIPE;
+
+	if (fmt.format.width != pix->width)
+		return -EPIPE;
+	if (fmt.format.height != pix->height)
+		return -EPIPE;

Pipeline validation should take place during media_pipeline_start(). Why
are you doing it here?

How would be do that? Via the link_validate callback?

quoted

+
+	ti_fmt = find_format_by_pix(pix->pixelformat);
+	if (WARN_ON(!ti_fmt))
+		return -EINVAL;
+
+	if (fmt.format.code == MEDIA_BUS_FMT_YUYV8_2X8 ||
+	    fmt.format.code == MEDIA_BUS_FMT_VYUY8_2X8 ||
+	    fmt.format.code == MEDIA_BUS_FMT_YVYU8_2X8) {
+		dev_err(csi->dev,
+			"Only UYVY input allowed for YUV422 8-bit. Output format can be configured.\n");
+		return -EPIPE;
+	}
+
+	if (fmt.format.code == MEDIA_BUS_FMT_UYVY8_2X8) {
+		/* Format conversion between YUV422 formats can be done. */
+		if (ti_fmt->code != MEDIA_BUS_FMT_UYVY8_2X8 &&
+		    ti_fmt->code != MEDIA_BUS_FMT_YUYV8_2X8 &&
+		    ti_fmt->code != MEDIA_BUS_FMT_VYUY8_2X8 &&
+		    ti_fmt->code != MEDIA_BUS_FMT_YVYU8_2X8)
+			return -EPIPE;
+	} else if (fmt.format.code != ti_fmt->code) {
+		return -EPIPE;
+	}
+
+	if (fmt.format.field != V4L2_FIELD_NONE &&
+	    fmt.format.field != V4L2_FIELD_ANY)
+		return -EPIPE;
+
+	return 0;
+}
+
+static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
+{
+	struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vq);
+	struct ti_csi2rx_dma *dma = &csi->dma;
+	struct ti_csi2rx_buffer *buf, *tmp;
+	unsigned long flags = 0;

No need to assign flags here.

Ok.

quoted

+	int ret = 0;
+

-- 
Kind regards,

Sakari Ailus

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.