[PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Eugen Hristev]

This series is a split from the series :
[PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
and it includes the media controller part.
previous fixes were sent on a different patch series.

As discussed on the ML, moving forward with having the media link validate at
start/stop streaming call.
I will test the patch :
[RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
afterwards, but that patch requires moving my logic to the new vb2 callbacks.

Full series history:

Changes in v10:
-> split the series into this first fixes part.
-> moved IO_MC addition from first patch to the second patch on the driver changes
-> edited commit messages
-> DT nodes now disabled by default.

Changes in v9:
-> kernel robot reported isc_link_validate is not static, changed to static.

Changes in v8:
-> scaler: modified crop bounds to have the exact source size

Changes in v7:
-> scaler: modified crop bounds to have maximum isc size
-> format propagation: did small changes as per Jacopo review


Changes in v6:
-> worked a bit on scaler, added try crop and other changes as per Jacopo review
-> worked on isc-base enum_fmt , reworked as per Jacopo review

Changes in v5:
-> removed patch that removed the 'stop' variable as it was still required
-> added two new trivial patches
-> reworked some parts of the scaler and format propagation after discussions with Jacopo


Changes in v4:
-> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
one patch that was using it
-> as reviewed by Jacopo, reworked some parts of the media controller implementation


Changes in v3:
- change in bindings, small fixes in csi2dc driver and conversion to mc
for the isc-base.
- removed some MAINTAINERS patches and used patterns in MAINTAINERS

Changes in v2:
- integrated many changes suggested by Jacopo in the review of the v1 series.
- add a few new patches

Eugen Hristev (5):
  media: atmel: atmel-isc: prepare for media controller support
  media: atmel: atmel-isc: implement media controller
  ARM: dts: at91: sama7g5: add nodes for video capture
  ARM: configs: at91: sama7: add xisc and csi2dc
  ARM: multi_v7_defconfig: add atmel video pipeline modules

 arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
 arch/arm/configs/multi_v7_defconfig           |   3 +
 arch/arm/configs/sama7_defconfig              |   2 +
 drivers/media/platform/atmel/Makefile         |   2 +-
 drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
 .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
 drivers/media/platform/atmel/atmel-isc.h      |  50 +-
 .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
 .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
 9 files changed, 685 insertions(+), 241 deletions(-)
 create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c

-- 
2.25.1

[PATCH v10 1/5] media: atmel: atmel-isc: prepare for media controller support

[Eugen Hristev]

Prepare the support for media-controller.
This means that the capabilities of the driver have changed and now it's
capable of media controller operations.
The driver will register its media device, and add the video entity to this
media device. The subdevices are registered to the same media device.
The ISC will have a base entity which is auto-detected as atmel_isc_base.
It will also register a subdevice that allows cropping of the incoming
frame to the maximum frame size supported by the ISC.
The ISC will create a link between the subdevice that is asynchronously
registered and the atmel_isc_scaler entity.
Then, the atmel_isc_scaler and atmel_isc_base are connected through another
link.
This patch does not change the previous capability of the driver, the
fact that the format is still being propagated from the top video node
down to the sensor.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
Changes in v10:
- edited commit message, and patch name
- moved IO_MC to another patch

Changes in v8:
- use source format size as bounds always

Changes in v7:
- use maximum isc frame size as bounds always

Changes in v6:
- reworked a bit as suggested by Jacopo
- add try crops

Changes in v5:
- reworked s_fmt to pass the same format from sink to source
- simplified enum_mbus_code
- separated tgt and bounds to report correctly in g_sel

Changes in v4:
As suggested by Jacopo:
- renamed atmel_isc_mc to atmel_isc_scaler.c
- moved init_mc/clean_mc to isc_base file

Changes in v2:
- implement try formats

 drivers/media/platform/atmel/Makefile         |   2 +-
 drivers/media/platform/atmel/atmel-isc-base.c |  72 ++++-
 .../media/platform/atmel/atmel-isc-scaler.c   | 262 ++++++++++++++++++
 drivers/media/platform/atmel/atmel-isc.h      |  37 +++
 .../media/platform/atmel/atmel-sama5d2-isc.c  |  14 +-
 .../media/platform/atmel/atmel-sama7g5-isc.c  |  12 +-
 6 files changed, 392 insertions(+), 7 deletions(-)
 create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c

diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile
index 794e8f739287..f02d03df89d6 100644
--- a/drivers/media/platform/atmel/Makefile
+++ b/drivers/media/platform/atmel/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 atmel-isc-objs = atmel-sama5d2-isc.o
 atmel-xisc-objs = atmel-sama7g5-isc.o
-atmel-isc-common-objs = atmel-isc-base.o atmel-isc-clk.o
+atmel-isc-common-objs = atmel-isc-base.o atmel-isc-clk.o atmel-isc-scaler.o
 
 obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
 obj-$(CONFIG_VIDEO_ATMEL_ISC_BASE) += atmel-isc-common.o
diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
index 2f07a50035c8..749a41b1763e 100644
--- a/drivers/media/platform/atmel/atmel-isc-base.c
+++ b/drivers/media/platform/atmel/atmel-isc-base.c
@@ -1730,6 +1730,7 @@ static int isc_async_bound(struct v4l2_async_notifier *notifier,
 					      struct isc_device, v4l2_dev);
 	struct isc_subdev_entity *subdev_entity =
 		container_of(notifier, struct isc_subdev_entity, notifier);
+	int pad;
 
 	if (video_is_registered(&isc->video_dev)) {
 		v4l2_err(&isc->v4l2_dev, "only supports one sub-device.\n");
@@ -1738,6 +1739,16 @@ static int isc_async_bound(struct v4l2_async_notifier *notifier,
 
 	subdev_entity->sd = subdev;
 
+	pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode,
+					  MEDIA_PAD_FL_SOURCE);
+	if (pad < 0) {
+		v4l2_err(&isc->v4l2_dev, "failed to find pad for %s\n",
+			 subdev->name);
+		return pad;
+	}
+
+	isc->remote_pad = pad;
+
 	return 0;
 }
 
@@ -1753,8 +1764,8 @@ static void isc_async_unbind(struct v4l2_async_notifier *notifier,
 	v4l2_ctrl_handler_free(&isc->ctrls.handler);
 }
 
-static struct isc_format *find_format_by_code(struct isc_device *isc,
-					      unsigned int code, int *index)
+struct isc_format *isc_find_format_by_code(struct isc_device *isc,
+					   unsigned int code, int *index)
 {
 	struct isc_format *fmt = &isc->formats_list[0];
 	unsigned int i;
@@ -1770,6 +1781,7 @@ static struct isc_format *find_format_by_code(struct isc_device *isc,
 
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(isc_find_format_by_code);
 
 static int isc_formats_init(struct isc_device *isc)
 {
@@ -1786,7 +1798,7 @@ static int isc_formats_init(struct isc_device *isc)
 	       NULL, &mbus_code)) {
 		mbus_code.index++;
 
-		fmt = find_format_by_code(isc, mbus_code.code, &i);
+		fmt = isc_find_format_by_code(isc, mbus_code.code, &i);
 		if (!fmt) {
 			v4l2_warn(&isc->v4l2_dev, "Mbus code %x not supported\n",
 				  mbus_code.code);
@@ -1924,8 +1936,19 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
 		goto isc_async_complete_err;
 	}
 
+	ret = isc_scaler_link(isc);
+	if (ret < 0)
+		goto isc_async_complete_unregister_device;
+
+	ret = media_device_register(&isc->mdev);
+	if (ret < 0)
+		goto isc_async_complete_unregister_device;
+
 	return 0;
 
+isc_async_complete_unregister_device:
+	video_unregister_device(vdev);
+
 isc_async_complete_err:
 	mutex_destroy(&isc->awb_mutex);
 	mutex_destroy(&isc->lock);
@@ -1993,6 +2016,49 @@ int isc_pipeline_init(struct isc_device *isc)
 }
 EXPORT_SYMBOL_GPL(isc_pipeline_init);
 
+int isc_mc_init(struct isc_device *isc, u32 ver)
+{
+	const struct of_device_id *match;
+	int ret;
+
+	isc->video_dev.entity.function = MEDIA_ENT_F_IO_V4L;
+	isc->video_dev.entity.flags = MEDIA_ENT_FL_DEFAULT;
+	isc->pads[ISC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+
+	ret = media_entity_pads_init(&isc->video_dev.entity, ISC_PADS_NUM,
+				     isc->pads);
+	if (ret < 0) {
+		dev_err(isc->dev, "media entity init failed\n");
+		return ret;
+	}
+
+	isc->mdev.dev = isc->dev;
+
+	match = of_match_node(isc->dev->driver->of_match_table,
+			      isc->dev->of_node);
+
+	strscpy(isc->mdev.driver_name, KBUILD_MODNAME,
+		sizeof(isc->mdev.driver_name));
+	strscpy(isc->mdev.model, match->compatible, sizeof(isc->mdev.model));
+	snprintf(isc->mdev.bus_info, sizeof(isc->mdev.bus_info), "platform:%s",
+		 isc->v4l2_dev.name);
+	isc->mdev.hw_revision = ver;
+
+	media_device_init(&isc->mdev);
+
+	isc->v4l2_dev.mdev = &isc->mdev;
+
+	return isc_scaler_init(isc);
+}
+EXPORT_SYMBOL_GPL(isc_mc_init);
+
+void isc_mc_cleanup(struct isc_device *isc)
+{
+	media_entity_cleanup(&isc->video_dev.entity);
+	media_device_cleanup(&isc->mdev);
+}
+EXPORT_SYMBOL_GPL(isc_mc_cleanup);
+
 /* regmap configuration */
 #define ATMEL_ISC_REG_MAX    0xd5c
 const struct regmap_config isc_regmap_config = {
diff --git a/drivers/media/platform/atmel/atmel-isc-scaler.c b/drivers/media/platform/atmel/atmel-isc-scaler.c
new file mode 100644
index 000000000000..a1ca4633787c
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc-scaler.c
@@ -0,0 +1,262 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Microchip Image Sensor Controller (ISC) Scaler entity support
+ *
+ * Copyright (C) 2022 Microchip Technology, Inc.
+ *
+ * Author: Eugen Hristev <eugen.hristev@microchip.com>
+ *
+ */
+
+#include <media/media-device.h>
+#include <media/media-entity.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+
+#include "atmel-isc-regs.h"
+#include "atmel-isc.h"
+
+static void isc_scaler_prepare_fmt(struct v4l2_mbus_framefmt *framefmt)
+{
+	framefmt->colorspace = V4L2_COLORSPACE_SRGB;
+	framefmt->field = V4L2_FIELD_NONE;
+	framefmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+	framefmt->quantization = V4L2_QUANTIZATION_DEFAULT;
+	framefmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
+};
+
+static int isc_scaler_get_fmt(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_state *sd_state,
+			      struct v4l2_subdev_format *format)
+{
+	struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
+	struct v4l2_mbus_framefmt *v4l2_try_fmt;
+
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+		v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
+							  format->pad);
+		format->format = *v4l2_try_fmt;
+
+		return 0;
+	}
+
+	format->format = isc->scaler_format[format->pad];
+
+	return 0;
+}
+
+static int isc_scaler_set_fmt(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_state *sd_state,
+			      struct v4l2_subdev_format *req_fmt)
+{
+	struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
+	struct v4l2_mbus_framefmt *v4l2_try_fmt;
+	struct isc_format *fmt;
+	unsigned int i;
+
+	/* Source format is fixed, we cannot change it */
+	if (req_fmt->pad == ISC_SCALER_PAD_SOURCE) {
+		req_fmt->format = isc->scaler_format[ISC_SCALER_PAD_SOURCE];
+		return 0;
+	}
+
+	/* There is no limit on the frame size on the sink pad */
+	v4l_bound_align_image(&req_fmt->format.width, 16, UINT_MAX, 0,
+			      &req_fmt->format.height, 16, UINT_MAX, 0, 0);
+
+	isc_scaler_prepare_fmt(&req_fmt->format);
+
+	fmt = isc_find_format_by_code(isc, req_fmt->format.code, &i);
+
+	if (!fmt)
+		fmt = &isc->formats_list[0];
+
+	req_fmt->format.code = fmt->mbus_code;
+
+	if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
+							  req_fmt->pad);
+		*v4l2_try_fmt = req_fmt->format;
+		/* Trying on the sink pad makes the source pad change too */
+		v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
+							  ISC_SCALER_PAD_SOURCE);
+		*v4l2_try_fmt = req_fmt->format;
+
+		v4l_bound_align_image(&v4l2_try_fmt->width,
+				      16, isc->max_width, 0,
+				      &v4l2_try_fmt->height,
+				      16, isc->max_height, 0, 0);
+		/* if we are just trying, we are done */
+		return 0;
+	}
+
+	isc->scaler_format[ISC_SCALER_PAD_SINK] = req_fmt->format;
+
+	/* The source pad is the same as the sink, but we have to crop it */
+	isc->scaler_format[ISC_SCALER_PAD_SOURCE] =
+		isc->scaler_format[ISC_SCALER_PAD_SINK];
+	v4l_bound_align_image
+		(&isc->scaler_format[ISC_SCALER_PAD_SOURCE].width, 16,
+		 isc->max_width, 0,
+		 &isc->scaler_format[ISC_SCALER_PAD_SOURCE].height, 16,
+		 isc->max_height, 0, 0);
+
+	return 0;
+}
+
+static int isc_scaler_enum_mbus_code(struct v4l2_subdev *sd,
+				     struct v4l2_subdev_state *sd_state,
+				     struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
+
+	/*
+	 * All formats supported by the ISC are supported by the scaler.
+	 * Advertise the formats which the ISC can take as input, as the scaler
+	 * entity cropping is part of the PFE module (parallel front end)
+	 */
+	if (code->index < isc->formats_list_size) {
+		code->code = isc->formats_list[code->index].mbus_code;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int isc_scaler_g_sel(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_state *sd_state,
+			    struct v4l2_subdev_selection *sel)
+{
+	struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
+
+	if (sel->pad == ISC_SCALER_PAD_SOURCE)
+		return -EINVAL;
+
+	if (sel->target != V4L2_SEL_TGT_CROP_BOUNDS &&
+	    sel->target != V4L2_SEL_TGT_CROP)
+		return -EINVAL;
+
+	sel->r.height = isc->scaler_format[ISC_SCALER_PAD_SOURCE].height;
+	sel->r.width = isc->scaler_format[ISC_SCALER_PAD_SOURCE].width;
+
+	sel->r.left = 0;
+	sel->r.top = 0;
+
+	return 0;
+}
+
+static int isc_scaler_init_cfg(struct v4l2_subdev *sd,
+			       struct v4l2_subdev_state *sd_state)
+{
+	struct v4l2_mbus_framefmt *v4l2_try_fmt =
+		v4l2_subdev_get_try_format(sd, sd_state, 0);
+	struct v4l2_rect *try_crop;
+	struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
+
+	*v4l2_try_fmt = isc->scaler_format[ISC_SCALER_PAD_SOURCE];
+
+	try_crop = v4l2_subdev_get_try_crop(sd, sd_state, 0);
+
+	try_crop->top = 0;
+	try_crop->left = 0;
+	try_crop->width = v4l2_try_fmt->width;
+	try_crop->height = v4l2_try_fmt->height;
+
+	return 0;
+}
+
+static const struct v4l2_subdev_pad_ops isc_scaler_pad_ops = {
+	.enum_mbus_code = isc_scaler_enum_mbus_code,
+	.set_fmt = isc_scaler_set_fmt,
+	.get_fmt = isc_scaler_get_fmt,
+	.get_selection = isc_scaler_g_sel,
+	.init_cfg = isc_scaler_init_cfg,
+};
+
+static const struct v4l2_subdev_ops xisc_scaler_subdev_ops = {
+	.pad = &isc_scaler_pad_ops,
+};
+
+int isc_scaler_init(struct isc_device *isc)
+{
+	int ret;
+
+	v4l2_subdev_init(&isc->scaler_sd, &xisc_scaler_subdev_ops);
+
+	isc->scaler_sd.owner = THIS_MODULE;
+	isc->scaler_sd.dev = isc->dev;
+	snprintf(isc->scaler_sd.name, sizeof(isc->scaler_sd.name),
+		 "atmel_isc_scaler");
+
+	isc->scaler_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	isc->scaler_sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
+	isc->scaler_pads[ISC_SCALER_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+	isc->scaler_pads[ISC_SCALER_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+
+	isc_scaler_prepare_fmt(&isc->scaler_format[ISC_SCALER_PAD_SOURCE]);
+	isc->scaler_format[ISC_SCALER_PAD_SOURCE].height = isc->max_height;
+	isc->scaler_format[ISC_SCALER_PAD_SOURCE].width = isc->max_width;
+	isc->scaler_format[ISC_SCALER_PAD_SOURCE].code =
+		 isc->formats_list[0].mbus_code;
+
+	isc->scaler_format[ISC_SCALER_PAD_SINK] =
+		 isc->scaler_format[ISC_SCALER_PAD_SOURCE];
+
+	ret = media_entity_pads_init(&isc->scaler_sd.entity,
+				     ISC_SCALER_PADS_NUM,
+				     isc->scaler_pads);
+	if (ret < 0) {
+		dev_err(isc->dev, "scaler sd media entity init failed\n");
+		return ret;
+	}
+
+	ret = v4l2_device_register_subdev(&isc->v4l2_dev, &isc->scaler_sd);
+	if (ret < 0) {
+		dev_err(isc->dev, "scaler sd failed to register subdev\n");
+		return ret;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(isc_scaler_init);
+
+int isc_scaler_link(struct isc_device *isc)
+{
+	int ret;
+
+	ret = media_create_pad_link(&isc->current_subdev->sd->entity,
+				    isc->remote_pad, &isc->scaler_sd.entity,
+				    ISC_SCALER_PAD_SINK,
+				    MEDIA_LNK_FL_ENABLED |
+				    MEDIA_LNK_FL_IMMUTABLE);
+
+	if (ret < 0) {
+		dev_err(isc->dev, "Failed to create pad link: %s to %s\n",
+			isc->current_subdev->sd->entity.name,
+			isc->scaler_sd.entity.name);
+		return ret;
+	}
+
+	dev_dbg(isc->dev, "link with %s pad: %d\n",
+		isc->current_subdev->sd->name, isc->remote_pad);
+
+	ret = media_create_pad_link(&isc->scaler_sd.entity,
+				    ISC_SCALER_PAD_SOURCE,
+				    &isc->video_dev.entity, ISC_PAD_SINK,
+				    MEDIA_LNK_FL_ENABLED |
+				    MEDIA_LNK_FL_IMMUTABLE);
+
+	if (ret < 0) {
+		dev_err(isc->dev, "Failed to create pad link: %s to %s\n",
+			isc->scaler_sd.entity.name,
+			isc->video_dev.entity.name);
+		return ret;
+	}
+
+	dev_dbg(isc->dev, "link with %s pad: %d\n", isc->scaler_sd.name,
+		ISC_SCALER_PAD_SOURCE);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(isc_scaler_link);
+
diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
index ff60ba020cb9..f98f25a55e73 100644
--- a/drivers/media/platform/atmel/atmel-isc.h
+++ b/drivers/media/platform/atmel/atmel-isc.h
@@ -183,6 +183,17 @@ struct isc_reg_offsets {
 	u32 his_entry;
 };
 
+enum isc_mc_pads {
+	ISC_PAD_SINK	= 0,
+	ISC_PADS_NUM	= 1,
+};
+
+enum isc_scaler_pads {
+	ISC_SCALER_PAD_SINK	= 0,
+	ISC_SCALER_PAD_SOURCE	= 1,
+	ISC_SCALER_PADS_NUM	= 2,
+};
+
 /*
  * struct isc_device - ISC device driver data/config struct
  * @regmap:		Register map
@@ -259,6 +270,12 @@ struct isc_reg_offsets {
  *			be used as an input to the controller
  * @controller_formats_size:	size of controller_formats array
  * @formats_list_size:	size of formats_list array
+ * @pads:		media controller pads for isc video entity
+ * @mdev:		media device that is registered by the isc
+ * @remote_pad:		remote pad on the connected subdevice
+ * @scaler_sd:		subdevice for the scaler that isc registers
+ * @scaler_pads:	media controller pads for the scaler subdevice
+ * @scaler_format:	current format for the scaler subdevice
  */
 struct isc_device {
 	struct regmap		*regmap;
@@ -348,6 +365,19 @@ struct isc_device {
 	struct isc_format		*formats_list;
 	u32				controller_formats_size;
 	u32				formats_list_size;
+
+	struct {
+		struct media_pad		pads[ISC_PADS_NUM];
+		struct media_device		mdev;
+
+		u32				remote_pad;
+	};
+
+	struct {
+		struct v4l2_subdev		scaler_sd;
+		struct media_pad		scaler_pads[ISC_SCALER_PADS_NUM];
+		struct v4l2_mbus_framefmt	scaler_format[ISC_SCALER_PADS_NUM];
+	};
 };
 
 extern const struct regmap_config isc_regmap_config;
@@ -359,4 +389,11 @@ int isc_clk_init(struct isc_device *isc);
 void isc_subdev_cleanup(struct isc_device *isc);
 void isc_clk_cleanup(struct isc_device *isc);
 
+int isc_scaler_link(struct isc_device *isc);
+int isc_scaler_init(struct isc_device *isc);
+int isc_mc_init(struct isc_device *isc, u32 ver);
+void isc_mc_cleanup(struct isc_device *isc);
+
+struct isc_format *isc_find_format_by_code(struct isc_device *isc,
+					   unsigned int code, int *index);
 #endif
diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
index e236319935ce..d96ee3373889 100644
--- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
@@ -536,6 +536,12 @@ static int atmel_isc_probe(struct platform_device *pdev)
 			break;
 	}
 
+	regmap_read(isc->regmap, ISC_VERSION + isc->offsets.version, &ver);
+
+	ret = isc_mc_init(isc, ver);
+	if (ret < 0)
+		goto isc_probe_mc_init_err;
+
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 	pm_request_idle(dev);
@@ -545,7 +551,7 @@ static int atmel_isc_probe(struct platform_device *pdev)
 	ret = clk_prepare_enable(isc->ispck);
 	if (ret) {
 		dev_err(dev, "failed to enable ispck: %d\n", ret);
-		goto cleanup_subdev;
+		goto isc_probe_mc_init_err;
 	}
 
 	/* ispck should be greater or equal to hclock */
@@ -555,7 +561,6 @@ static int atmel_isc_probe(struct platform_device *pdev)
 		goto unprepare_clk;
 	}
 
-	regmap_read(isc->regmap, ISC_VERSION + isc->offsets.version, &ver);
 	dev_info(dev, "Microchip ISC version %x\n", ver);
 
 	return 0;
@@ -563,6 +568,9 @@ static int atmel_isc_probe(struct platform_device *pdev)
 unprepare_clk:
 	clk_disable_unprepare(isc->ispck);
 
+isc_probe_mc_init_err:
+	isc_mc_cleanup(isc);
+
 cleanup_subdev:
 	isc_subdev_cleanup(isc);
 
@@ -583,6 +591,8 @@ static int atmel_isc_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 
+	isc_mc_cleanup(isc);
+
 	isc_subdev_cleanup(isc);
 
 	v4l2_device_unregister(&isc->v4l2_dev);
diff --git a/drivers/media/platform/atmel/atmel-sama7g5-isc.c b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
index 83b175070c06..462a3b8b67ab 100644
--- a/drivers/media/platform/atmel/atmel-sama7g5-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
@@ -526,15 +526,23 @@ static int microchip_xisc_probe(struct platform_device *pdev)
 			break;
 	}
 
+	regmap_read(isc->regmap, ISC_VERSION + isc->offsets.version, &ver);
+
+	ret = isc_mc_init(isc, ver);
+	if (ret < 0)
+		goto isc_probe_mc_init_err;
+
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 	pm_request_idle(dev);
 
-	regmap_read(isc->regmap, ISC_VERSION + isc->offsets.version, &ver);
 	dev_info(dev, "Microchip XISC version %x\n", ver);
 
 	return 0;
 
+isc_probe_mc_init_err:
+	isc_mc_cleanup(isc);
+
 cleanup_subdev:
 	isc_subdev_cleanup(isc);
 
@@ -555,6 +563,8 @@ static int microchip_xisc_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 
+	isc_mc_cleanup(isc);
+
 	isc_subdev_cleanup(isc);
 
 	v4l2_device_unregister(&isc->v4l2_dev);
-- 
2.25.1

[PATCH v10 2/5] media: atmel: atmel-isc: implement media controller

[Eugen Hristev]

As a top MC video driver, the atmel-isc should not propagate the format to
the subdevice, it should rather check at start_streaming() time if the
subdev is properly configured with a compatible format.
Removed the whole format finding logic, and reworked the format
verification at start_streaming time, such that the ISC will return an
error if the subdevice is not properly configured.
To achieve this, media_pipeline_start is called and a link_validate
callback is created to check the formats.
With this being done, the module parameter 'sensor_preferred' makes no
sense anymore. The ISC should not decide which format the sensor is using.
The ISC should only cope with the situation and inform userspace if the
streaming is possible in the current configuration.
The redesign of the format propagation has also risen the question of the
enumfmt callback. If enumfmt is called with an mbus_code, the enumfmt
handler should only return the formats that are supported for this
mbus_code. Otherwise, the enumfmt will report all the formats that the ISC
could output.
With this rework, the dynamic list of user formats is removed. It makes no
more sense to identify at complete time which formats the sensor could
emit, and add those into a separate dynamic list.
The ISC will start with a simple preconfigured default format, and at
link validate time, decide whether it can use the format that is
configured on the sink or not.
From now on, the driver also advertises the IO_MC capability.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
Changes in v10:
- moved IO_MC to this patch
- edited commit message

Changes in v9:
- isc_link_validate now static

Changes in v7:
- minor typos as suggested by Jacopo
- small changes, reduce some indentation, modified an index, as suggested by
Jacopo

Changes in v6:
- reworked a bit enum_fmt as suggested by Jacopo

Changes in v5:
- removed user_formats dynamic list as it is now pointless
- greatly simplified the enum_fmt function
- removed some init code that was useless now

Changes in v4:
- moved validation code into link_validate and used media_pipeline_start
- merged this patch with the enum_fmt patch which was previously in v3 of
the series

Changes in v3:
- clamp to maximum resolution once the frame size from the subdev is found

 drivers/media/platform/atmel/atmel-isc-base.c | 415 ++++++++----------
 .../media/platform/atmel/atmel-isc-scaler.c   |   5 +
 drivers/media/platform/atmel/atmel-isc.h      |  13 +-
 .../media/platform/atmel/atmel-sama5d2-isc.c  |  20 +
 .../media/platform/atmel/atmel-sama7g5-isc.c  |  20 +
 5 files changed, 238 insertions(+), 235 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
index 749a41b1763e..4d71e92aa5ea 100644
--- a/drivers/media/platform/atmel/atmel-isc-base.c
+++ b/drivers/media/platform/atmel/atmel-isc-base.c
@@ -36,11 +36,6 @@ static unsigned int debug;
 module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "debug level (0-2)");
 
-static unsigned int sensor_preferred = 1;
-module_param(sensor_preferred, uint, 0644);
-MODULE_PARM_DESC(sensor_preferred,
-		 "Sensor is preferred to output the specified format (1-on 0-off), default 1");
-
 #define ISC_IS_FORMAT_RAW(mbus_code) \
 	(((mbus_code) & 0xf000) == 0x3000)
 
@@ -337,6 +332,10 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 	unsigned long flags;
 	int ret;
 
+	ret = media_pipeline_start(&isc->video_dev.entity, &isc->mpipe);
+	if (ret)
+		goto err_pipeline_start;
+
 	/* Enable stream on the sub device */
 	ret = v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 1);
 	if (ret && ret != -ENOIOCTLCMD) {
@@ -385,6 +384,9 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 	v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);
 
 err_start_stream:
+	media_pipeline_stop(&isc->video_dev.entity);
+
+err_pipeline_start:
 	spin_lock_irqsave(&isc->dma_queue_lock, flags);
 	list_for_each_entry(buf, &isc->dma_queue, list)
 		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
@@ -423,6 +425,9 @@ static void isc_stop_streaming(struct vb2_queue *vq)
 	if (ret && ret != -ENOIOCTLCMD)
 		v4l2_err(&isc->v4l2_dev, "stream off failed in subdev\n");
 
+	/* Stop media pipeline */
+	media_pipeline_stop(&isc->video_dev.entity);
+
 	/* Release all active buffers */
 	spin_lock_irqsave(&isc->dma_queue_lock, flags);
 	if (unlikely(isc->cur_frm)) {
@@ -453,22 +458,6 @@ static void isc_buffer_queue(struct vb2_buffer *vb)
 	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
 }
 
-static struct isc_format *find_format_by_fourcc(struct isc_device *isc,
-						 unsigned int fourcc)
-{
-	unsigned int num_formats = isc->num_user_formats;
-	struct isc_format *fmt;
-	unsigned int i;
-
-	for (i = 0; i < num_formats; i++) {
-		fmt = isc->user_formats[i];
-		if (fmt->fourcc == fourcc)
-			return fmt;
-	}
-
-	return NULL;
-}
-
 static const struct vb2_ops isc_vb2_ops = {
 	.queue_setup		= isc_queue_setup,
 	.wait_prepare		= vb2_ops_wait_prepare,
@@ -497,23 +486,57 @@ static int isc_enum_fmt_vid_cap(struct file *file, void *priv,
 {
 	struct isc_device *isc = video_drvdata(file);
 	u32 index = f->index;
-	u32 i, supported_index;
+	u32 i, supported_index = 0;
+	struct isc_format *fmt;
+
+	/*
+	 * If we are not asked a specific mbus_code, we have to report all
+	 * the formats that we can output.
+	 */
+	if (!f->mbus_code) {
+		if (index >= isc->controller_formats_size)
+			return -EINVAL;
 
-	if (index < isc->controller_formats_size) {
 		f->pixelformat = isc->controller_formats[index].fourcc;
+
+		return 0;
+	}
+
+	/*
+	 * If a specific mbus_code is requested, check if we support
+	 * this mbus_code as input for the ISC.
+	 * If it's supported, then we report the corresponding pixelformat
+	 * as first possible option for the ISC.
+	 * E.g. mbus MEDIA_BUS_FMT_YUYV8_2X8 and report
+	 * 'YUYV' (YUYV 4:2:2)
+	 */
+	fmt = isc_find_format_by_code(isc, f->mbus_code, &i);
+	if (!fmt)
+		return -EINVAL;
+
+	if (!index) {
+		f->pixelformat = fmt->fourcc;
+
 		return 0;
 	}
 
-	index -= isc->controller_formats_size;
+	supported_index++;
 
-	supported_index = 0;
+	/* If the index is not raw, we don't have anymore formats to report */
+	if (!ISC_IS_FORMAT_RAW(f->mbus_code))
+		return -EINVAL;
 
-	for (i = 0; i < isc->formats_list_size; i++) {
-		if (!ISC_IS_FORMAT_RAW(isc->formats_list[i].mbus_code) ||
-		    !isc->formats_list[i].sd_support)
+	/*
+	 * We are asked for a specific mbus code, which is raw.
+	 * We have to search through the formats we can convert to.
+	 * We have to skip the raw formats, we cannot convert to raw.
+	 * E.g. 'AR12' (16-bit ARGB 4-4-4-4), 'AR15' (16-bit ARGB 1-5-5-5), etc.
+	 */
+	for (i = 0; i < isc->controller_formats_size; i++) {
+		if (isc->controller_formats[i].raw)
 			continue;
-		if (supported_index == index) {
-			f->pixelformat = isc->formats_list[i].fourcc;
+		if (index == supported_index) {
+			f->pixelformat = isc->controller_formats[i].fourcc;
 			return 0;
 		}
 		supported_index++;
@@ -584,20 +607,30 @@ static int isc_try_validate_formats(struct isc_device *isc)
 		break;
 	default:
 	/* any other different formats are not supported */
+		v4l2_err(&isc->v4l2_dev, "Requested unsupported format.\n");
 		ret = -EINVAL;
 	}
 	v4l2_dbg(1, debug, &isc->v4l2_dev,
 		 "Format validation, requested rgb=%u, yuv=%u, grey=%u, bayer=%u\n",
 		 rgb, yuv, grey, bayer);
 
-	/* we cannot output RAW if we do not receive RAW */
-	if ((bayer) && !ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code))
+	if (bayer &&
+	    !ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code)) {
+		v4l2_err(&isc->v4l2_dev, "Cannot output RAW if we do not receive RAW.\n");
 		return -EINVAL;
+	}
 
-	/* we cannot output GREY if we do not receive RAW/GREY */
 	if (grey && !ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code) &&
-	    !ISC_IS_FORMAT_GREY(isc->try_config.sd_format->mbus_code))
+	    !ISC_IS_FORMAT_GREY(isc->try_config.sd_format->mbus_code)) {
+		v4l2_err(&isc->v4l2_dev, "Cannot output GREY if we do not receive RAW/GREY.\n");
 		return -EINVAL;
+	}
+
+	if ((rgb || bayer || yuv) &&
+	    ISC_IS_FORMAT_GREY(isc->try_config.sd_format->mbus_code)) {
+		v4l2_err(&isc->v4l2_dev, "Cannot convert GREY to another format.\n");
+		return -EINVAL;
+	}
 
 	return ret;
 }
@@ -825,7 +858,7 @@ static void isc_try_fse(struct isc_device *isc,
 	 * If we do not know yet which format the subdev is using, we cannot
 	 * do anything.
 	 */
-	if (!isc->try_config.sd_format)
+	if (!isc->config.sd_format)
 		return;
 
 	fse.code = isc->try_config.sd_format->mbus_code;
@@ -846,180 +879,140 @@ static void isc_try_fse(struct isc_device *isc,
 	}
 }
 
-static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f,
-			u32 *code)
+static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f)
 {
-	int i;
-	struct isc_format *sd_fmt = NULL, *direct_fmt = NULL;
 	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
-	struct v4l2_subdev_pad_config pad_cfg = {};
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
-	struct v4l2_subdev_format format = {
-		.which = V4L2_SUBDEV_FORMAT_TRY,
-	};
-	u32 mbus_code;
-	int ret;
-	bool rlp_dma_direct_dump = false;
+	unsigned int i;
 
 	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return -EINVAL;
 
-	/* Step 1: find a RAW format that is supported */
-	for (i = 0; i < isc->num_user_formats; i++) {
-		if (ISC_IS_FORMAT_RAW(isc->user_formats[i]->mbus_code)) {
-			sd_fmt = isc->user_formats[i];
+	isc->try_config.fourcc = isc->controller_formats[0].fourcc;
+
+	/* find if the format requested is supported */
+	for (i = 0; i < isc->controller_formats_size; i++)
+		if (isc->controller_formats[i].fourcc == pixfmt->pixelformat) {
+			isc->try_config.fourcc = pixfmt->pixelformat;
 			break;
 		}
-	}
-	/* Step 2: We can continue with this RAW format, or we can look
-	 * for better: maybe sensor supports directly what we need.
-	 */
-	direct_fmt = find_format_by_fourcc(isc, pixfmt->pixelformat);
-
-	/* Step 3: We have both. We decide given the module parameter which
-	 * one to use.
-	 */
-	if (direct_fmt && sd_fmt && sensor_preferred)
-		sd_fmt = direct_fmt;
-
-	/* Step 4: we do not have RAW but we have a direct format. Use it. */
-	if (direct_fmt && !sd_fmt)
-		sd_fmt = direct_fmt;
-
-	/* Step 5: if we are using a direct format, we need to package
-	 * everything as 8 bit data and just dump it
-	 */
-	if (sd_fmt == direct_fmt)
-		rlp_dma_direct_dump = true;
-
-	/* Step 6: We have no format. This can happen if the userspace
-	 * requests some weird/invalid format.
-	 * In this case, default to whatever we have
-	 */
-	if (!sd_fmt && !direct_fmt) {
-		sd_fmt = isc->user_formats[isc->num_user_formats - 1];
-		v4l2_dbg(1, debug, &isc->v4l2_dev,
-			 "Sensor not supporting %.4s, using %.4s\n",
-			 (char *)&pixfmt->pixelformat, (char *)&sd_fmt->fourcc);
-	}
-
-	if (!sd_fmt) {
-		ret = -EINVAL;
-		goto isc_try_fmt_err;
-	}
-
-	/* Step 7: Print out what we decided for debugging */
-	v4l2_dbg(1, debug, &isc->v4l2_dev,
-		 "Preferring to have sensor using format %.4s\n",
-		 (char *)&sd_fmt->fourcc);
 
-	/* Step 8: at this moment we decided which format the subdev will use */
-	isc->try_config.sd_format = sd_fmt;
+	isc_try_configure_rlp_dma(isc, false);
 
 	/* Limit to Atmel ISC hardware capabilities */
-	if (pixfmt->width > isc->max_width)
-		pixfmt->width = isc->max_width;
-	if (pixfmt->height > isc->max_height)
-		pixfmt->height = isc->max_height;
-
-	/*
-	 * The mbus format is the one the subdev outputs.
-	 * The pixels will be transferred in this format Sensor -> ISC
-	 */
-	mbus_code = sd_fmt->mbus_code;
-
-	/*
-	 * Validate formats. If the required format is not OK, default to raw.
-	 */
-
-	isc->try_config.fourcc = pixfmt->pixelformat;
-
-	if (isc_try_validate_formats(isc)) {
-		pixfmt->pixelformat = isc->try_config.fourcc = sd_fmt->fourcc;
-		/* Re-try to validate the new format */
-		ret = isc_try_validate_formats(isc);
-		if (ret)
-			goto isc_try_fmt_err;
-	}
-
-	ret = isc_try_configure_rlp_dma(isc, rlp_dma_direct_dump);
-	if (ret)
-		goto isc_try_fmt_err;
-
-	ret = isc_try_configure_pipeline(isc);
-	if (ret)
-		goto isc_try_fmt_err;
-
-	/* Obtain frame sizes if possible to have crop requirements ready */
-	isc_try_fse(isc, &pad_state);
-
-	v4l2_fill_mbus_format(&format.format, pixfmt, mbus_code);
-	ret = v4l2_subdev_call(isc->current_subdev->sd, pad, set_fmt,
-			       &pad_state, &format);
-	if (ret < 0)
-		goto isc_try_fmt_subdev_err;
-
-	v4l2_fill_pix_format(pixfmt, &format.format);
+	v4l_bound_align_image(&pixfmt->width, 16, isc->max_width, 0,
+			      &pixfmt->height, 16, isc->max_height, 0, 0);
+	/* If we did not find the requested format, we will fallback here */
+	pixfmt->pixelformat = isc->try_config.fourcc;
+	pixfmt->colorspace = V4L2_COLORSPACE_SRGB;
+	pixfmt->field = V4L2_FIELD_NONE;
 
-	/* Limit to Atmel ISC hardware capabilities */
-	if (pixfmt->width > isc->max_width)
-		pixfmt->width = isc->max_width;
-	if (pixfmt->height > isc->max_height)
-		pixfmt->height = isc->max_height;
 
-	pixfmt->field = V4L2_FIELD_NONE;
 	pixfmt->bytesperline = (pixfmt->width * isc->try_config.bpp_v4l2) >> 3;
 	pixfmt->sizeimage = ((pixfmt->width * isc->try_config.bpp) >> 3) *
 			     pixfmt->height;
 
-	if (code)
-		*code = mbus_code;
+	isc->try_fmt = *f;
 
 	return 0;
+}
 
-isc_try_fmt_err:
-	v4l2_err(&isc->v4l2_dev, "Could not find any possible format for a working pipeline\n");
-isc_try_fmt_subdev_err:
-	memset(&isc->try_config, 0, sizeof(isc->try_config));
+static int isc_set_fmt(struct isc_device *isc, struct v4l2_format *f)
+{
+	isc_try_fmt(isc, f);
 
-	return ret;
+	/* make the try configuration active */
+	isc->config = isc->try_config;
+	isc->fmt = isc->try_fmt;
+
+	v4l2_dbg(1, debug, &isc->v4l2_dev, "ISC set_fmt to %.4s @%dx%d\n",
+		 (char *)&f->fmt.pix.pixelformat,
+		 f->fmt.pix.width, f->fmt.pix.height);
+
+	return 0;
 }
 
-static int isc_set_fmt(struct isc_device *isc, struct v4l2_format *f)
+static int isc_validate(struct isc_device *isc)
 {
+	int ret;
+	int i;
+	struct isc_format *sd_fmt = NULL;
+	struct v4l2_pix_format *pixfmt = &isc->fmt.fmt.pix;
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		.pad = isc->remote_pad,
+	};
+	struct v4l2_subdev_pad_config pad_cfg = {};
+	struct v4l2_subdev_state pad_state = {
+		.pads = &pad_cfg,
 	};
-	u32 mbus_code = 0;
-	int ret;
 
-	ret = isc_try_fmt(isc, f, &mbus_code);
+	/* Get current format from subdev */
+	ret = v4l2_subdev_call(isc->current_subdev->sd, pad, get_fmt, NULL,
+			       &format);
 	if (ret)
 		return ret;
 
-	v4l2_fill_mbus_format(&format.format, &f->fmt.pix, mbus_code);
-	ret = v4l2_subdev_call(isc->current_subdev->sd, pad,
-			       set_fmt, NULL, &format);
-	if (ret < 0)
-		return ret;
+	/* Identify the subdev's format configuration */
+	for (i = 0; i < isc->formats_list_size; i++)
+		if (isc->formats_list[i].mbus_code == format.format.code) {
+			sd_fmt = &isc->formats_list[i];
+			break;
+		}
+
+	/* Check if the format is not supported */
+	if (!sd_fmt) {
+		v4l2_err(&isc->v4l2_dev,
+			 "Current subdevice is streaming a media bus code that is not supported 0x%x\n",
+			 format.format.code);
+		return -EPIPE;
+	}
+
+	/* At this moment we know which format the subdev will use */
+	isc->try_config.sd_format = sd_fmt;
+
+	/* If the sensor is not RAW, we can only do a direct dump */
+	if (!ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code))
+		isc_try_configure_rlp_dma(isc, true);
 
 	/* Limit to Atmel ISC hardware capabilities */
-	if (f->fmt.pix.width > isc->max_width)
-		f->fmt.pix.width = isc->max_width;
-	if (f->fmt.pix.height > isc->max_height)
-		f->fmt.pix.height = isc->max_height;
+	v4l_bound_align_image(&format.format.width, 16, isc->max_width, 0,
+			      &format.format.height, 16, isc->max_height, 0, 0);
 
-	isc->fmt = *f;
+	/* Check if the frame size is the same. Otherwise we may overflow */
+	if (pixfmt->height != format.format.height ||
+	    pixfmt->width != format.format.width) {
+		v4l2_err(&isc->v4l2_dev,
+			 "ISC not configured with the proper frame size: %dx%d\n",
+			 format.format.width, format.format.height);
+		return -EPIPE;
+	}
 
+	v4l2_dbg(1, debug, &isc->v4l2_dev,
+		 "Identified subdev using format %.4s with %dx%d %d bpp\n",
+		 (char *)&sd_fmt->fourcc, pixfmt->width, pixfmt->height,
+		 isc->try_config.bpp);
+
+	/* Reset and restart AWB if the subdevice changed the format */
 	if (isc->try_config.sd_format && isc->config.sd_format &&
 	    isc->try_config.sd_format != isc->config.sd_format) {
 		isc->ctrls.hist_stat = HIST_INIT;
 		isc_reset_awb_ctrls(isc);
 		isc_update_v4l2_ctrls(isc);
 	}
-	/* make the try configuration active */
+
+	/* Validate formats */
+	ret = isc_try_validate_formats(isc);
+	if (ret)
+		return ret;
+
+	/* Obtain frame sizes if possible to have crop requirements ready */
+	isc_try_fse(isc, &pad_state);
+
+	/* Configure ISC pipeline for the config */
+	ret = isc_try_configure_pipeline(isc);
+	if (ret)
+		return ret;
+
 	isc->config = isc->try_config;
 
 	v4l2_dbg(1, debug, &isc->v4l2_dev, "New ISC configuration in place\n");
@@ -1043,7 +1036,7 @@ static int isc_try_fmt_vid_cap(struct file *file, void *priv,
 {
 	struct isc_device *isc = video_drvdata(file);
 
-	return isc_try_fmt(isc, f, NULL);
+	return isc_try_fmt(isc, f);
 }
 
 static int isc_enum_input(struct file *file, void *priv,
@@ -1098,10 +1091,6 @@ static int isc_enum_framesizes(struct file *file, void *fh,
 	if (fsize->index)
 		return -EINVAL;
 
-	for (i = 0; i < isc->num_user_formats; i++)
-		if (isc->user_formats[i]->fourcc == fsize->pixel_format)
-			ret = 0;
-
 	for (i = 0; i < isc->controller_formats_size; i++)
 		if (isc->controller_formats[i].fourcc == fsize->pixel_format)
 			ret = 0;
@@ -1783,52 +1772,6 @@ struct isc_format *isc_find_format_by_code(struct isc_device *isc,
 }
 EXPORT_SYMBOL_GPL(isc_find_format_by_code);
 
-static int isc_formats_init(struct isc_device *isc)
-{
-	struct isc_format *fmt;
-	struct v4l2_subdev *subdev = isc->current_subdev->sd;
-	unsigned int num_fmts, i, j;
-	u32 list_size = isc->formats_list_size;
-	struct v4l2_subdev_mbus_code_enum mbus_code = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-	};
-
-	num_fmts = 0;
-	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
-	       NULL, &mbus_code)) {
-		mbus_code.index++;
-
-		fmt = isc_find_format_by_code(isc, mbus_code.code, &i);
-		if (!fmt) {
-			v4l2_warn(&isc->v4l2_dev, "Mbus code %x not supported\n",
-				  mbus_code.code);
-			continue;
-		}
-
-		fmt->sd_support = true;
-		num_fmts++;
-	}
-
-	if (!num_fmts)
-		return -ENXIO;
-
-	isc->num_user_formats = num_fmts;
-	isc->user_formats = devm_kcalloc(isc->dev,
-					 num_fmts, sizeof(*isc->user_formats),
-					 GFP_KERNEL);
-	if (!isc->user_formats)
-		return -ENOMEM;
-
-	fmt = &isc->formats_list[0];
-	for (i = 0, j = 0; i < list_size; i++) {
-		if (fmt->sd_support)
-			isc->user_formats[j++] = fmt;
-		fmt++;
-	}
-
-	return 0;
-}
-
 static int isc_set_default_fmt(struct isc_device *isc)
 {
 	struct v4l2_format f = {
@@ -1837,12 +1780,12 @@ static int isc_set_default_fmt(struct isc_device *isc)
 			.width		= VGA_WIDTH,
 			.height		= VGA_HEIGHT,
 			.field		= V4L2_FIELD_NONE,
-			.pixelformat	= isc->user_formats[0]->fourcc,
+			.pixelformat	= isc->controller_formats[0].fourcc,
 		},
 	};
 	int ret;
 
-	ret = isc_try_fmt(isc, &f, NULL);
+	ret = isc_try_fmt(isc, &f);
 	if (ret)
 		return ret;
 
@@ -1897,13 +1840,6 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
 	spin_lock_init(&isc->dma_queue_lock);
 	spin_lock_init(&isc->awb_lock);
 
-	ret = isc_formats_init(isc);
-	if (ret < 0) {
-		v4l2_err(&isc->v4l2_dev,
-			 "Init format failed: %d\n", ret);
-		goto isc_async_complete_err;
-	}
-
 	ret = isc_set_default_fmt(isc);
 	if (ret) {
 		v4l2_err(&isc->v4l2_dev, "Could not set default format\n");
@@ -1926,7 +1862,8 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
 	vdev->queue		= q;
 	vdev->lock		= &isc->lock;
 	vdev->ctrl_handler	= &isc->ctrls.handler;
-	vdev->device_caps	= V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_CAPTURE;
+	vdev->device_caps	= V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_CAPTURE |
+				  V4L2_CAP_IO_MC;
 	video_set_drvdata(vdev, isc);
 
 	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
@@ -2016,6 +1953,24 @@ int isc_pipeline_init(struct isc_device *isc)
 }
 EXPORT_SYMBOL_GPL(isc_pipeline_init);
 
+static int isc_link_validate(struct media_link *link)
+{
+	struct video_device *vdev =
+		media_entity_to_video_device(link->sink->entity);
+	struct isc_device *isc = video_get_drvdata(vdev);
+	int ret;
+
+	ret = v4l2_subdev_link_validate(link);
+	if (ret)
+		return ret;
+
+	return isc_validate(isc);
+}
+
+static const struct media_entity_operations isc_entity_operations = {
+	.link_validate = isc_link_validate,
+};
+
 int isc_mc_init(struct isc_device *isc, u32 ver)
 {
 	const struct of_device_id *match;
@@ -2023,6 +1978,8 @@ int isc_mc_init(struct isc_device *isc, u32 ver)
 
 	isc->video_dev.entity.function = MEDIA_ENT_F_IO_V4L;
 	isc->video_dev.entity.flags = MEDIA_ENT_FL_DEFAULT;
+	isc->video_dev.entity.ops = &isc_entity_operations;
+
 	isc->pads[ISC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
 
 	ret = media_entity_pads_init(&isc->video_dev.entity, ISC_PADS_NUM,
diff --git a/drivers/media/platform/atmel/atmel-isc-scaler.c b/drivers/media/platform/atmel/atmel-isc-scaler.c
index a1ca4633787c..2b0621297dfe 100644
--- a/drivers/media/platform/atmel/atmel-isc-scaler.c
+++ b/drivers/media/platform/atmel/atmel-isc-scaler.c
@@ -173,6 +173,10 @@ static const struct v4l2_subdev_pad_ops isc_scaler_pad_ops = {
 	.init_cfg = isc_scaler_init_cfg,
 };
 
+static const struct media_entity_operations isc_scaler_entity_ops = {
+	.link_validate = v4l2_subdev_link_validate,
+};
+
 static const struct v4l2_subdev_ops xisc_scaler_subdev_ops = {
 	.pad = &isc_scaler_pad_ops,
 };
@@ -190,6 +194,7 @@ int isc_scaler_init(struct isc_device *isc)
 
 	isc->scaler_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	isc->scaler_sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
+	isc->scaler_sd.entity.ops = &isc_scaler_entity_ops;
 	isc->scaler_pads[ISC_SCALER_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
 	isc->scaler_pads[ISC_SCALER_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
 
diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
index f98f25a55e73..5b0100c8c245 100644
--- a/drivers/media/platform/atmel/atmel-isc.h
+++ b/drivers/media/platform/atmel/atmel-isc.h
@@ -63,15 +63,16 @@ struct isc_subdev_entity {
  * @cfa_baycfg:		If this format is RAW BAYER, indicate the type of bayer.
 			this is either BGBG, RGRG, etc.
  * @pfe_cfg0_bps:	Number of hardware data lines connected to the ISC
+ * @raw:		If the format is raw bayer.
  */
 
 struct isc_format {
 	u32	fourcc;
 	u32	mbus_code;
 	u32	cfa_baycfg;
-
-	bool	sd_support;
 	u32	pfe_cfg0_bps;
+
+	bool	raw;
 };
 
 /* Pipeline bitmap */
@@ -216,8 +217,7 @@ enum isc_scaler_pads {
  * @comp:		completion reference that signals frame completion
  *
  * @fmt:		current v42l format
- * @user_formats:	list of formats that are supported and agreed with sd
- * @num_user_formats:	how many formats are in user_formats
+ * @try_fmt:		current v4l2 try format
  *
  * @config:		current ISC format configuration
  * @try_config:		the current ISC try format , not yet activated
@@ -272,6 +272,7 @@ enum isc_scaler_pads {
  * @formats_list_size:	size of formats_list array
  * @pads:		media controller pads for isc video entity
  * @mdev:		media device that is registered by the isc
+ * @mpipe:		media device pipeline used by the isc
  * @remote_pad:		remote pad on the connected subdevice
  * @scaler_sd:		subdevice for the scaler that isc registers
  * @scaler_pads:	media controller pads for the scaler subdevice
@@ -298,8 +299,7 @@ struct isc_device {
 	struct completion	comp;
 
 	struct v4l2_format	fmt;
-	struct isc_format	**user_formats;
-	unsigned int		num_user_formats;
+	struct v4l2_format	try_fmt;
 
 	struct fmt_config	config;
 	struct fmt_config	try_config;
@@ -369,6 +369,7 @@ struct isc_device {
 	struct {
 		struct media_pad		pads[ISC_PADS_NUM];
 		struct media_device		mdev;
+		struct media_pipeline		mpipe;
 
 		u32				remote_pad;
 	};
diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
index d96ee3373889..2a651e237193 100644
--- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
@@ -80,20 +80,40 @@ static const struct isc_format sama5d2_controller_formats[] = {
 		.fourcc		= V4L2_PIX_FMT_Y10,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SBGGR8,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SGBRG8,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SGRBG8,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SRGGB8,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SBGGR10,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SGBRG10,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SGRBG10,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SRGGB10,
+		.raw		= true,
+	}, {
+		.fourcc		= V4L2_PIX_FMT_SBGGR12,
+		.raw		= true,
+	}, {
+		.fourcc		= V4L2_PIX_FMT_SGBRG12,
+		.raw		= true,
+	}, {
+		.fourcc		= V4L2_PIX_FMT_SGRBG12,
+		.raw		= true,
+	}, {
+		.fourcc		= V4L2_PIX_FMT_SRGGB12,
+		.raw		= true,
 	},
 };
 
diff --git a/drivers/media/platform/atmel/atmel-sama7g5-isc.c b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
index 462a3b8b67ab..638af8da2694 100644
--- a/drivers/media/platform/atmel/atmel-sama7g5-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
@@ -89,20 +89,40 @@ static const struct isc_format sama7g5_controller_formats[] = {
 		.fourcc		= V4L2_PIX_FMT_Y16,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SBGGR8,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SGBRG8,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SGRBG8,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SRGGB8,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SBGGR10,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SGBRG10,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SGRBG10,
+		.raw		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SRGGB10,
+		.raw		= true,
+	}, {
+		.fourcc		= V4L2_PIX_FMT_SBGGR12,
+		.raw		= true,
+	}, {
+		.fourcc		= V4L2_PIX_FMT_SGBRG12,
+		.raw		= true,
+	}, {
+		.fourcc		= V4L2_PIX_FMT_SGRBG12,
+		.raw		= true,
+	}, {
+		.fourcc		= V4L2_PIX_FMT_SRGGB12,
+		.raw		= true,
 	},
 };
 
-- 
2.25.1

[PATCH v10 3/5] ARM: dts: at91: sama7g5: add nodes for video capture

[Eugen Hristev]

Add node for the XISC (eXtended Image Sensor Controller) and CSI2DC
(csi2 demux controller).
These nodes represent the top level of the video capture hardware pipeline
and are directly connected in hardware.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
Changes in v10:
- nodes disabled by default

 arch/arm/boot/dts/sama7g5.dtsi | 51 ++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/arch/arm/boot/dts/sama7g5.dtsi b/arch/arm/boot/dts/sama7g5.dtsi
index 4decd3a91a76..fe9c6df9819b 100644
--- a/arch/arm/boot/dts/sama7g5.dtsi
+++ b/arch/arm/boot/dts/sama7g5.dtsi
@@ -454,6 +454,57 @@ sdmmc2: mmc@e120c000 {
 			status = "disabled";
 		};
 
+		csi2dc: csi2dc@e1404000 {
+			compatible = "microchip,sama7g5-csi2dc";
+			reg = <0xe1404000 0x500>;
+			clocks = <&pmc PMC_TYPE_PERIPHERAL 34>, <&xisc>;
+			clock-names = "pclk", "scck";
+			assigned-clocks = <&xisc>;
+			assigned-clock-rates = <266000000>;
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				port@0 {
+					reg = <0>;
+					csi2dc_in: endpoint {
+					};
+				};
+
+				port@1 {
+					reg = <1>;
+					csi2dc_out: endpoint {
+						bus-width = <14>;
+						hsync-active = <1>;
+						vsync-active = <1>;
+						remote-endpoint = <&xisc_in>;
+					};
+				};
+			};
+		};
+
+		xisc: xisc@e1408000 {
+			compatible = "microchip,sama7g5-isc";
+			reg = <0xe1408000 0x2000>;
+			interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&pmc PMC_TYPE_PERIPHERAL 56>;
+			clock-names = "hclock";
+			#clock-cells = <0>;
+			clock-output-names = "isc-mck";
+			status = "disabled";
+
+			port {
+				xisc_in: endpoint {
+					bus-type = <5>; /* Parallel */
+					bus-width = <14>;
+					hsync-active = <1>;
+					vsync-active = <1>;
+					remote-endpoint = <&csi2dc_out>;
+				};
+			};
+		};
+
 		pwm: pwm@e1604000 {
 			compatible = "microchip,sama7g5-pwm", "atmel,sama5d2-pwm";
 			reg = <0xe1604000 0x4000>;
-- 
2.25.1

Re: [PATCH v10 3/5] ARM: dts: at91: sama7g5: add nodes for video capture

[Eugen.Hristev]

On 5/3/22 12:51, Eugen Hristev wrote:
> Add node for the XISC (eXtended Image Sensor Controller) and CSI2DC
> (csi2 demux controller).
> These nodes represent the top level of the video capture hardware pipeline
> and are directly connected in hardware.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
> Changes in v10:
> - nodes disabled by default
> 
>   arch/arm/boot/dts/sama7g5.dtsi | 51 ++++++++++++++++++++++++++++++++++
>   1 file changed, 51 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sama7g5.dtsi b/arch/arm/boot/dts/sama7g5.dtsi
> index 4decd3a91a76..fe9c6df9819b 100644
> --- a/arch/arm/boot/dts/sama7g5.dtsi
> +++ b/arch/arm/boot/dts/sama7g5.dtsi
> @@ -454,6 +454,57 @@ sdmmc2: mmc@e120c000 {
>   			status = "disabled";
>   		};
>   
> +		csi2dc: csi2dc@e1404000 {
> +			compatible = "microchip,sama7g5-csi2dc";
> +			reg = <0xe1404000 0x500>;
> +			clocks = <&pmc PMC_TYPE_PERIPHERAL 34>, <&xisc>;
> +			clock-names = "pclk", "scck";
> +			assigned-clocks = <&xisc>;
> +			assigned-clock-rates = <266000000>;
> +			status = "disabled";
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				port@0 {
> +					reg = <0>;
> +					csi2dc_in: endpoint {
> +					};
> +				};
> +
> +				port@1 {
> +					reg = <1>;
> +					csi2dc_out: endpoint {
> +						bus-width = <14>;
> +						hsync-active = <1>;
> +						vsync-active = <1>;
> +						remote-endpoint = <&xisc_in>;
> +					};
> +				};
> +			};
> +		};
> +
> +		xisc: xisc@e1408000 {
> +			compatible = "microchip,sama7g5-isc";
> +			reg = <0xe1408000 0x2000>;
> +			interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&pmc PMC_TYPE_PERIPHERAL 56>;
> +			clock-names = "hclock";
> +			#clock-cells = <0>;
> +			clock-output-names = "isc-mck";
> +			status = "disabled";
> +
> +			port {
> +				xisc_in: endpoint {
> +					bus-type = <5>; /* Parallel */
> +					bus-width = <14>;
> +					hsync-active = <1>;
> +					vsync-active = <1>;
> +					remote-endpoint = <&csi2dc_out>;
> +				};
> +			};
> +		};
> +
>   		pwm: pwm@e1604000 {
>   			compatible = "microchip,sama7g5-pwm", "atmel,sama5d2-pwm";
>   			reg = <0xe1604000 0x4000>;

Hello Claudiu, Nicolas,

This patch is ready to go now , as the media controller support for XISC 
driver is in tree.

Let me know if you need this to be resent.

Thanks,
Eugen

Re: [PATCH v10 3/5] ARM: dts: at91: sama7g5: add nodes for video capture

[Claudiu.Beznea]

On 14.12.2022 14:55, Eugen Hristev - M18282 wrote:
> On 5/3/22 12:51, Eugen Hristev wrote:
>> Add node for the XISC (eXtended Image Sensor Controller) and CSI2DC
>> (csi2 demux controller).
>> These nodes represent the top level of the video capture hardware pipeline
>> and are directly connected in hardware.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>> Changes in v10:
>> - nodes disabled by default
>>
>>   arch/arm/boot/dts/sama7g5.dtsi | 51 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 51 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sama7g5.dtsi b/arch/arm/boot/dts/sama7g5.dtsi
>> index 4decd3a91a76..fe9c6df9819b 100644
>> --- a/arch/arm/boot/dts/sama7g5.dtsi
>> +++ b/arch/arm/boot/dts/sama7g5.dtsi
>> @@ -454,6 +454,57 @@ sdmmc2: mmc@e120c000 {
>>   			status = "disabled";
>>   		};
>>   
>> +		csi2dc: csi2dc@e1404000 {
>> +			compatible = "microchip,sama7g5-csi2dc";
>> +			reg = <0xe1404000 0x500>;
>> +			clocks = <&pmc PMC_TYPE_PERIPHERAL 34>, <&xisc>;
>> +			clock-names = "pclk", "scck";
>> +			assigned-clocks = <&xisc>;
>> +			assigned-clock-rates = <266000000>;
>> +			status = "disabled";
>> +
>> +			ports {
>> +				#address-cells = <1>;
>> +				#size-cells = <0>;
>> +				port@0 {
>> +					reg = <0>;
>> +					csi2dc_in: endpoint {
>> +					};
>> +				};
>> +
>> +				port@1 {
>> +					reg = <1>;
>> +					csi2dc_out: endpoint {
>> +						bus-width = <14>;
>> +						hsync-active = <1>;
>> +						vsync-active = <1>;
>> +						remote-endpoint = <&xisc_in>;
>> +					};
>> +				};
>> +			};
>> +		};
>> +
>> +		xisc: xisc@e1408000 {
>> +			compatible = "microchip,sama7g5-isc";
>> +			reg = <0xe1408000 0x2000>;
>> +			interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&pmc PMC_TYPE_PERIPHERAL 56>;
>> +			clock-names = "hclock";
>> +			#clock-cells = <0>;
>> +			clock-output-names = "isc-mck";
>> +			status = "disabled";
>> +
>> +			port {
>> +				xisc_in: endpoint {
>> +					bus-type = <5>; /* Parallel */
>> +					bus-width = <14>;
>> +					hsync-active = <1>;
>> +					vsync-active = <1>;
>> +					remote-endpoint = <&csi2dc_out>;
>> +				};
>> +			};
>> +		};
>> +
>>   		pwm: pwm@e1604000 {
>>   			compatible = "microchip,sama7g5-pwm", "atmel,sama5d2-pwm";
>>   			reg = <0xe1604000 0x4000>;
> 
> Hello Claudiu, Nicolas,

Hi, Eugen,

> 
> This patch is ready to go now , as the media controller support for XISC 
> driver is in tree.
> 
> Let me know if you need this to be resent.

No need. I'll take this one for the next PR.

Thank you,
Claudiu Beznea

> 
> Thanks,
> Eugen
> 

Re: [PATCH v10 3/5] ARM: dts: at91: sama7g5: add nodes for video capture

[Claudiu.Beznea]

On 03.05.2022 12:51, Eugen Hristev wrote:
> Add node for the XISC (eXtended Image Sensor Controller) and CSI2DC
> (csi2 demux controller).
> These nodes represent the top level of the video capture hardware pipeline
> and are directly connected in hardware.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

Applied to at91-dt, thanks!

> ---
> Changes in v10:
> - nodes disabled by default
> 
>  arch/arm/boot/dts/sama7g5.dtsi | 51 ++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sama7g5.dtsi b/arch/arm/boot/dts/sama7g5.dtsi
> index 4decd3a91a76..fe9c6df9819b 100644
> --- a/arch/arm/boot/dts/sama7g5.dtsi
> +++ b/arch/arm/boot/dts/sama7g5.dtsi
> @@ -454,6 +454,57 @@ sdmmc2: mmc@e120c000 {
>  			status = "disabled";
>  		};
>  
> +		csi2dc: csi2dc@e1404000 {
> +			compatible = "microchip,sama7g5-csi2dc";
> +			reg = <0xe1404000 0x500>;
> +			clocks = <&pmc PMC_TYPE_PERIPHERAL 34>, <&xisc>;
> +			clock-names = "pclk", "scck";
> +			assigned-clocks = <&xisc>;
> +			assigned-clock-rates = <266000000>;
> +			status = "disabled";
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				port@0 {
> +					reg = <0>;
> +					csi2dc_in: endpoint {
> +					};
> +				};
> +
> +				port@1 {
> +					reg = <1>;
> +					csi2dc_out: endpoint {
> +						bus-width = <14>;
> +						hsync-active = <1>;
> +						vsync-active = <1>;
> +						remote-endpoint = <&xisc_in>;
> +					};
> +				};
> +			};
> +		};
> +
> +		xisc: xisc@e1408000 {
> +			compatible = "microchip,sama7g5-isc";
> +			reg = <0xe1408000 0x2000>;
> +			interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&pmc PMC_TYPE_PERIPHERAL 56>;
> +			clock-names = "hclock";
> +			#clock-cells = <0>;
> +			clock-output-names = "isc-mck";
> +			status = "disabled";
> +
> +			port {
> +				xisc_in: endpoint {
> +					bus-type = <5>; /* Parallel */
> +					bus-width = <14>;
> +					hsync-active = <1>;
> +					vsync-active = <1>;
> +					remote-endpoint = <&csi2dc_out>;
> +				};
> +			};
> +		};
> +
>  		pwm: pwm@e1604000 {
>  			compatible = "microchip,sama7g5-pwm", "atmel,sama5d2-pwm";
>  			reg = <0xe1604000 0x4000>;

[PATCH v10 4/5] ARM: configs: at91: sama7: add xisc and csi2dc

[Eugen Hristev]

Enable XISC and CSI2DC drivers.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 arch/arm/configs/sama7_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/sama7_defconfig b/arch/arm/configs/sama7_defconfig
index 07b0494ef743..b375bf5f924c 100644
--- a/arch/arm/configs/sama7_defconfig
+++ b/arch/arm/configs/sama7_defconfig
@@ -138,6 +138,8 @@ CONFIG_MEDIA_SUPPORT_FILTER=y
 CONFIG_MEDIA_CAMERA_SUPPORT=y
 CONFIG_MEDIA_PLATFORM_SUPPORT=y
 CONFIG_V4L_PLATFORM_DRIVERS=y
+CONFIG_VIDEO_ATMEL_XISC=y
+CONFIG_VIDEO_MICROCHIP_CSI2DC=y
 CONFIG_VIDEO_IMX219=m
 CONFIG_VIDEO_IMX274=m
 CONFIG_VIDEO_OV5647=m
-- 
2.25.1

Re: [PATCH v10 4/5] ARM: configs: at91: sama7: add xisc and csi2dc

[Nicolas Ferre]

On 03/05/2022 at 11:51, Eugen Hristev wrote:
> Enable XISC and CSI2DC drivers.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
and queued in at91-defconfig branch for 5.19, as the drivers and 
defconfig options are already there in 5.18-rc1.

Thanks, best regards,
   Nicolas

> ---
>   arch/arm/configs/sama7_defconfig | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/configs/sama7_defconfig b/arch/arm/configs/sama7_defconfig
> index 07b0494ef743..b375bf5f924c 100644
> --- a/arch/arm/configs/sama7_defconfig
> +++ b/arch/arm/configs/sama7_defconfig
> @@ -138,6 +138,8 @@ CONFIG_MEDIA_SUPPORT_FILTER=y
>   CONFIG_MEDIA_CAMERA_SUPPORT=y
>   CONFIG_MEDIA_PLATFORM_SUPPORT=y
>   CONFIG_V4L_PLATFORM_DRIVERS=y
> +CONFIG_VIDEO_ATMEL_XISC=y
> +CONFIG_VIDEO_MICROCHIP_CSI2DC=y
>   CONFIG_VIDEO_IMX219=m
>   CONFIG_VIDEO_IMX274=m
>   CONFIG_VIDEO_OV5647=m


-- 
Nicolas Ferre

[PATCH v10 5/5] ARM: multi_v7_defconfig: add atmel video pipeline modules

[Eugen Hristev]

Add drivers for the atmel video capture pipeline: atmel isc, xisc and
microchip csi2dc.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 arch/arm/configs/multi_v7_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 6e0c8c19b35c..621bd1cbaf7c 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -662,7 +662,10 @@ CONFIG_VIDEO_S5P_MIPI_CSIS=m
 CONFIG_VIDEO_EXYNOS_FIMC_LITE=m
 CONFIG_VIDEO_EXYNOS4_FIMC_IS=m
 CONFIG_VIDEO_RCAR_VIN=m
+CONFIG_VIDEO_ATMEL_ISC=m
+CONFIG_VIDEO_ATMEL_XISC=m
 CONFIG_VIDEO_ATMEL_ISI=m
+CONFIG_VIDEO_MICROCHIP_CSI2DC=m
 CONFIG_V4L_MEM2MEM_DRIVERS=y
 CONFIG_VIDEO_SAMSUNG_S5P_JPEG=m
 CONFIG_VIDEO_SAMSUNG_S5P_MFC=m
-- 
2.25.1

Re: [PATCH v10 5/5] ARM: multi_v7_defconfig: add atmel video pipeline modules

[Nicolas Ferre]

On 03/05/2022 at 11:51, Eugen Hristev wrote:
> Add drivers for the atmel video capture pipeline: atmel isc, xisc and
> microchip csi2dc.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

Queued in at91-defconfig for 5.19.
Best regards,
   Nicolas

> ---
>   arch/arm/configs/multi_v7_defconfig | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
> index 6e0c8c19b35c..621bd1cbaf7c 100644
> --- a/arch/arm/configs/multi_v7_defconfig
> +++ b/arch/arm/configs/multi_v7_defconfig
> @@ -662,7 +662,10 @@ CONFIG_VIDEO_S5P_MIPI_CSIS=m
>   CONFIG_VIDEO_EXYNOS_FIMC_LITE=m
>   CONFIG_VIDEO_EXYNOS4_FIMC_IS=m
>   CONFIG_VIDEO_RCAR_VIN=m
> +CONFIG_VIDEO_ATMEL_ISC=m
> +CONFIG_VIDEO_ATMEL_XISC=m
>   CONFIG_VIDEO_ATMEL_ISI=m
> +CONFIG_VIDEO_MICROCHIP_CSI2DC=m
>   CONFIG_V4L_MEM2MEM_DRIVERS=y
>   CONFIG_VIDEO_SAMSUNG_S5P_JPEG=m
>   CONFIG_VIDEO_SAMSUNG_S5P_MFC=m


-- 
Nicolas Ferre

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Eugen.Hristev]

On 5/3/22 12:51 PM, Eugen Hristev wrote:
> This series is a split from the series :
> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
> and it includes the media controller part.
> previous fixes were sent on a different patch series.
> 
> As discussed on the ML, moving forward with having the media link validate at
> start/stop streaming call.
> I will test the patch :
> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
> 
> Full series history:
> 
> Changes in v10:
> -> split the series into this first fixes part.
> -> moved IO_MC addition from first patch to the second patch on the driver changes
> -> edited commit messages
> -> DT nodes now disabled by default.
> 
> Changes in v9:
> -> kernel robot reported isc_link_validate is not static, changed to static.
> 
> Changes in v8:
> -> scaler: modified crop bounds to have the exact source size
> 
> Changes in v7:
> -> scaler: modified crop bounds to have maximum isc size
> -> format propagation: did small changes as per Jacopo review
> 
> 
> Changes in v6:
> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
> -> worked on isc-base enum_fmt , reworked as per Jacopo review
> 
> Changes in v5:
> -> removed patch that removed the 'stop' variable as it was still required
> -> added two new trivial patches
> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
> 
> 
> Changes in v4:
> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
> one patch that was using it
> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
> 
> 
> Changes in v3:
> - change in bindings, small fixes in csi2dc driver and conversion to mc
> for the isc-base.
> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
> 
> Changes in v2:
> - integrated many changes suggested by Jacopo in the review of the v1 series.
> - add a few new patches
> 
> Eugen Hristev (5):
>    media: atmel: atmel-isc: prepare for media controller support
>    media: atmel: atmel-isc: implement media controller
>    ARM: dts: at91: sama7g5: add nodes for video capture
>    ARM: configs: at91: sama7: add xisc and csi2dc
>    ARM: multi_v7_defconfig: add atmel video pipeline modules
> 
>   arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>   arch/arm/configs/multi_v7_defconfig           |   3 +
>   arch/arm/configs/sama7_defconfig              |   2 +
>   drivers/media/platform/atmel/Makefile         |   2 +-
>   drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>   .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>   drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>   .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>   .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>   9 files changed, 685 insertions(+), 241 deletions(-)
>   create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
> 


Hello Hans,

What do you think about this series, does it require more work or 
changes until it could move further ? Anything in particular you would 
like me to try or test out ?

Thanks,
Eugen

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Hans Verkuil]

Hi Eugen,

On 6/15/22 13:06, Eugen.Hristev@microchip.com wrote:
> On 5/3/22 12:51 PM, Eugen Hristev wrote:
>> This series is a split from the series :
>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
>> and it includes the media controller part.
>> previous fixes were sent on a different patch series.
>>
>> As discussed on the ML, moving forward with having the media link validate at
>> start/stop streaming call.
>> I will test the patch :
>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
>>
>> Full series history:
>>
>> Changes in v10:
>> -> split the series into this first fixes part.
>> -> moved IO_MC addition from first patch to the second patch on the driver changes
>> -> edited commit messages
>> -> DT nodes now disabled by default.
>>
>> Changes in v9:
>> -> kernel robot reported isc_link_validate is not static, changed to static.
>>
>> Changes in v8:
>> -> scaler: modified crop bounds to have the exact source size
>>
>> Changes in v7:
>> -> scaler: modified crop bounds to have maximum isc size
>> -> format propagation: did small changes as per Jacopo review
>>
>>
>> Changes in v6:
>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
>>
>> Changes in v5:
>> -> removed patch that removed the 'stop' variable as it was still required
>> -> added two new trivial patches
>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
>>
>>
>> Changes in v4:
>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
>> one patch that was using it
>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
>>
>>
>> Changes in v3:
>> - change in bindings, small fixes in csi2dc driver and conversion to mc
>> for the isc-base.
>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
>>
>> Changes in v2:
>> - integrated many changes suggested by Jacopo in the review of the v1 series.
>> - add a few new patches
>>
>> Eugen Hristev (5):
>>    media: atmel: atmel-isc: prepare for media controller support
>>    media: atmel: atmel-isc: implement media controller
>>    ARM: dts: at91: sama7g5: add nodes for video capture
>>    ARM: configs: at91: sama7: add xisc and csi2dc
>>    ARM: multi_v7_defconfig: add atmel video pipeline modules
>>
>>   arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>>   arch/arm/configs/multi_v7_defconfig           |   3 +
>>   arch/arm/configs/sama7_defconfig              |   2 +
>>   drivers/media/platform/atmel/Makefile         |   2 +-
>>   drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>>   .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>>   drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>>   .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>>   .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>>   9 files changed, 685 insertions(+), 241 deletions(-)
>>   create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
>>
> 
> 
> Hello Hans,
> 
> What do you think about this series, does it require more work or 
> changes until it could move further ? Anything in particular you would 
> like me to try or test out ?

It's high on my todo list to look at this and see if anything else needs to
be done. I hope to get to this this week.

Regards,

	Hans

> 
> Thanks,
> Eugen

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Hans Verkuil]

Hi Eugen,

On 03/05/2022 11:51, Eugen Hristev wrote:
> This series is a split from the series :
> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
> and it includes the media controller part.
> previous fixes were sent on a different patch series.
> 
> As discussed on the ML, moving forward with having the media link validate at
> start/stop streaming call.
> I will test the patch :
> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
> afterwards, but that patch requires moving my logic to the new vb2 callbacks.

I'm looking at merging this series, but I would like to have the output of
'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
correct.

And one more question which may have been answered already in the past:

Changing to the MC will break existing applications, doesn't it? Or did I
miss something?

Regards,

	Hans

> 
> Full series history:
> 
> Changes in v10:
> -> split the series into this first fixes part.
> -> moved IO_MC addition from first patch to the second patch on the driver changes
> -> edited commit messages
> -> DT nodes now disabled by default.
> 
> Changes in v9:
> -> kernel robot reported isc_link_validate is not static, changed to static.
> 
> Changes in v8:
> -> scaler: modified crop bounds to have the exact source size
> 
> Changes in v7:
> -> scaler: modified crop bounds to have maximum isc size
> -> format propagation: did small changes as per Jacopo review
> 
> 
> Changes in v6:
> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
> -> worked on isc-base enum_fmt , reworked as per Jacopo review
> 
> Changes in v5:
> -> removed patch that removed the 'stop' variable as it was still required
> -> added two new trivial patches
> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
> 
> 
> Changes in v4:
> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
> one patch that was using it
> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
> 
> 
> Changes in v3:
> - change in bindings, small fixes in csi2dc driver and conversion to mc
> for the isc-base.
> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
> 
> Changes in v2:
> - integrated many changes suggested by Jacopo in the review of the v1 series.
> - add a few new patches
> 
> Eugen Hristev (5):
>   media: atmel: atmel-isc: prepare for media controller support
>   media: atmel: atmel-isc: implement media controller
>   ARM: dts: at91: sama7g5: add nodes for video capture
>   ARM: configs: at91: sama7: add xisc and csi2dc
>   ARM: multi_v7_defconfig: add atmel video pipeline modules
> 
>  arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>  arch/arm/configs/multi_v7_defconfig           |   3 +
>  arch/arm/configs/sama7_defconfig              |   2 +
>  drivers/media/platform/atmel/Makefile         |   2 +-
>  drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>  .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>  drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>  .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>  .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>  9 files changed, 685 insertions(+), 241 deletions(-)
>  create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
> 

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Eugen.Hristev]

[-- Attachment #1: Type: text/plain, Size: 4785 bytes --]

On 6/22/22 2:53 PM, Hans Verkuil wrote:
> Hi Eugen,
> 
> On 03/05/2022 11:51, Eugen Hristev wrote:
>> This series is a split from the series :
>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
>> and it includes the media controller part.
>> previous fixes were sent on a different patch series.
>>
>> As discussed on the ML, moving forward with having the media link validate at
>> start/stop streaming call.
>> I will test the patch :
>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
> 
> I'm looking at merging this series, but I would like to have the output of
> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
> correct.

Hello Hans,

Please have a look at attached file . Unless you want me to add the 
whole output to the e-mail ?

I also added output of media-ctl -p for your convenience.
the subdev2 is a device and driver that is not upstream and has some 
compliance issues, they are reported by the v4l2-compliance tool, but 
they should not affect this series, it's a synopsys driver that was 
rejected on mainline a few years ago, I took it for internal usage, but 
it's not cleaned up nor worked a lot upon.

> 
> And one more question which may have been answered already in the past:
> 
> Changing to the MC will break existing applications, doesn't it? Or did I
> miss something?
> 

The existing applications will have to configure the pipeline now. It 
will no longer work by configuring just the top video node /dev/video0 .
They would have to use media-ctl for it, something similar with this set 
of commands:

media-ctl -d /dev/media0 --set-v4l2 '"imx219 
1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
media-ctl -d /dev/media0 --set-v4l2 
'"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
media-ctl -d /dev/media0 --set-v4l2 
'"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'

Thank you for taking care of this !

Eugen

> Regards,
> 
>          Hans
> 
>>
>> Full series history:
>>
>> Changes in v10:
>> -> split the series into this first fixes part.
>> -> moved IO_MC addition from first patch to the second patch on the driver changes
>> -> edited commit messages
>> -> DT nodes now disabled by default.
>>
>> Changes in v9:
>> -> kernel robot reported isc_link_validate is not static, changed to static.
>>
>> Changes in v8:
>> -> scaler: modified crop bounds to have the exact source size
>>
>> Changes in v7:
>> -> scaler: modified crop bounds to have maximum isc size
>> -> format propagation: did small changes as per Jacopo review
>>
>>
>> Changes in v6:
>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
>>
>> Changes in v5:
>> -> removed patch that removed the 'stop' variable as it was still required
>> -> added two new trivial patches
>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
>>
>>
>> Changes in v4:
>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
>> one patch that was using it
>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
>>
>>
>> Changes in v3:
>> - change in bindings, small fixes in csi2dc driver and conversion to mc
>> for the isc-base.
>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
>>
>> Changes in v2:
>> - integrated many changes suggested by Jacopo in the review of the v1 series.
>> - add a few new patches
>>
>> Eugen Hristev (5):
>>    media: atmel: atmel-isc: prepare for media controller support
>>    media: atmel: atmel-isc: implement media controller
>>    ARM: dts: at91: sama7g5: add nodes for video capture
>>    ARM: configs: at91: sama7: add xisc and csi2dc
>>    ARM: multi_v7_defconfig: add atmel video pipeline modules
>>
>>   arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>>   arch/arm/configs/multi_v7_defconfig           |   3 +
>>   arch/arm/configs/sama7_defconfig              |   2 +
>>   drivers/media/platform/atmel/Makefile         |   2 +-
>>   drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>>   .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>>   drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>>   .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>>   .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>>   9 files changed, 685 insertions(+), 241 deletions(-)
>>   create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
>>
> 


[-- Attachment #2: v4l2-compl-atmel --]
[-- Type: text/plain, Size: 20788 bytes --]

# media-ctl -p
Media controller API version 5.15.32

Media device information
------------------------
driver          atmel_isc_commo
model           microchip,sama7g5-isc
serial
bus info        platform:microchip-sama7g5-xisc
hw revision     0x220
driver version  5.15.32

Device topology
- entity 1: atmel_isc_scaler (2 pads, 2 links)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
        pad0: Sink
                [fmt:SBGGR8_1X8/3264x2464 field:none colorspace:srgb
                 crop.bounds:(0,0)/3264x2464
                 crop:(0,0)/3264x2464]
                <- "csi2dc":1 [ENABLED,IMMUTABLE]
        pad1: Source
                [fmt:SBGGR8_1X8/3264x2464 field:none colorspace:srgb]
                -> "atmel_isc_common":0 [ENABLED,IMMUTABLE]

- entity 4: csi2dc (2 pads, 2 links)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev1
        pad0: Sink
                [fmt:SRGGB8_1X8/640x480 field:none colorspace:srgb]
                <- "dw-csi.0":1 [ENABLED]
        pad1: Source
                [fmt:SRGGB8_1X8/640x480 field:none colorspace:srgb]
                -> "atmel_isc_scaler":0 [ENABLED,IMMUTABLE]

- entity 7: dw-csi.0 (2 pads, 2 links)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev2
        pad0: Sink
                [fmt:SBGGR8_1X8/0x0]
                <- "imx219 1-0010":0 [ENABLED]
        pad1: Source
                [fmt:SBGGR8_1X8/0x0]
                -> "csi2dc":0 [ENABLED]

- entity 12: imx219 1-0010 (1 pad, 1 link)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev3
        pad0: Source
                [fmt:SRGGB10_1X10/3280x2464 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range
                 crop.bounds:(8,8)/3280x2464
                 crop:(8,8)/3280x2464]
                -> "dw-csi.0":0 [ENABLED]

- entity 24: atmel_isc_common (1 pad, 1 link)
             type Node subtype V4L flags 1
             device node name /dev/video0
        pad0: Sink
                <- "atmel_isc_scaler":1 [ENABLED,IMMUTABLE]




v4l2-compliance 1.22.1, 32 bits, 32-bit time_t

Compliance test for atmel_isc_commo device /dev/media0:

Media Driver Info:
	Driver name      : atmel_isc_commo
	Model            : microchip,sama7g5-isc
	Serial           : 
	Bus info         : platform:microchip-sama7g5-xisc
	Media version    : 5.15.32
	Hardware revision: 0x00000220 (544)
	Driver version   : 5.15.32

Required ioctls:
	test MEDIA_IOC_DEVICE_INFO: OK
	test invalid ioctls: OK

Allow for multiple opens:
	test second /dev/media0 open: OK
	test MEDIA_IOC_DEVICE_INFO: OK
	test for unlimited opens: OK

Media Controller ioctls:
	test MEDIA_IOC_G_TOPOLOGY: OK
	Entities: 5 Interfaces: 5 Pads: 8 Links: 9
	test MEDIA_IOC_ENUM_ENTITIES/LINKS: OK
	test MEDIA_IOC_SETUP_LINK: OK

Total for atmel_isc_commo device /dev/media0: 8, Succeeded: 8, Failed: 0, Warnings: 0
--------------------------------------------------------------------------------
Compliance test for atmel_isc_commo device /dev/v4l-subdev0:

Driver Info:
	Driver version   : 5.15.32
	Capabilities     : 0x00000000
Media Driver Info:
	Driver name      : atmel_isc_commo
	Model            : microchip,sama7g5-isc
	Serial           : 
	Bus info         : platform:microchip-sama7g5-xisc
	Media version    : 5.15.32
	Hardware revision: 0x00000220 (544)
	Driver version   : 5.15.32
Interface Info:
	ID               : 0x03000010
	Type             : V4L Sub-Device
Entity Info:
	ID               : 0x00000001 (1)
	Name             : atmel_isc_scaler
	Function         : Video Scaler
	Pad 0x01000002   : 0: Sink
	  Link 0x0200001c: from remote pad 0x1000006 of entity 'csi2dc' (Video Interface Bridge): Data, Enabled, Immutable
	Pad 0x01000003   : 1: Source
	  Link 0x0200001e: to remote pad 0x1000019 of entity 'atmel_isc_common' (V4L2 I/O): Data, Enabled, Immutable

Required ioctls:
	test MC information (see 'Media Driver Info' above): OK
	test VIDIOC_SUDBEV_QUERYCAP: OK
	test invalid ioctls: OK

Allow for multiple opens:
	test second /dev/v4l-subdev0 open: OK
	test VIDIOC_SUBDEV_QUERYCAP: OK
	test for unlimited opens: OK

Debug ioctls:
	test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Sub-Device ioctls (Sink Pad 0):
	test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
	test Try VIDIOC_SUBDEV_G/S_FMT: OK
		warn: v4l2-test-subdevs.cpp(507): VIDIOC_SUBDEV_G_SELECTION is supported for target 0 but not VIDIOC_SUBDEV_S_SELECTION
	test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK
	test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
	test Active VIDIOC_SUBDEV_G/S_FMT: OK
		warn: v4l2-test-subdevs.cpp(507): VIDIOC_SUBDEV_G_SELECTION is supported for target 0 but not VIDIOC_SUBDEV_S_SELECTION
	test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK
	test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported)

Sub-Device ioctls (Source Pad 1):
	test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
	test Try VIDIOC_SUBDEV_G/S_FMT: OK
	test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported)
	test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
	test Active VIDIOC_SUBDEV_G/S_FMT: OK
	test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported)
	test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported)

Control ioctls:
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
	test VIDIOC_QUERYCTRL: OK (Not Supported)
	test VIDIOC_G/S_CTRL: OK (Not Supported)
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 0 Private Controls: 0

Format ioctls:
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported)
	test VIDIOC_G/S_PARM: OK (Not Supported)
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK (Not Supported)
	test VIDIOC_TRY_FMT: OK (Not Supported)
	test VIDIOC_S_FMT: OK (Not Supported)
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK (Not Supported)

Codec ioctls:
	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported)
	test VIDIOC_EXPBUF: OK (Not Supported)
	test Requests: OK (Not Supported)
	test TIME32/64: OK

Total for atmel_isc_commo device /dev/v4l-subdev0: 59, Succeeded: 59, Failed: 0, Warnings: 2
--------------------------------------------------------------------------------
Compliance test for device /dev/v4l-subdev1:

Driver Info:
	Driver version   : 5.15.32
	Capabilities     : 0x00000000

Required ioctls:
	test VIDIOC_SUDBEV_QUERYCAP: OK
	test invalid ioctls: OK

Allow for multiple opens:
	test second /dev/v4l-subdev1 open: OK
	test VIDIOC_SUBDEV_QUERYCAP: OK
	test for unlimited opens: OK

Debug ioctls:
	test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls:
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
	test VIDIOC_QUERYCTRL: OK (Not Supported)
	test VIDIOC_G/S_CTRL: OK (Not Supported)
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 0 Private Controls: 0

Format ioctls:
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported)
	test VIDIOC_G/S_PARM: OK (Not Supported)
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK (Not Supported)
	test VIDIOC_TRY_FMT: OK (Not Supported)
	test VIDIOC_S_FMT: OK (Not Supported)
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK (Not Supported)

Codec ioctls:
	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported)
	test VIDIOC_EXPBUF: OK (Not Supported)
	test Requests: OK (Not Supported)
	test TIME32/64: OK

Total for device /dev/v4l-subdev1: 44, Succeeded: 44, Failed: 0, Warnings: 0
--------------------------------------------------------------------------------
Compliance test for atmel_isc_commo device /dev/v4l-subdev2:

Driver Info:
	Driver version   : 5.15.32
	Capabilities     : 0x00000000
Media Driver Info:
	Driver name      : atmel_isc_commo
	Model            : microchip,sama7g5-isc
	Serial           : 
	Bus info         : platform:microchip-sama7g5-xisc
	Media version    : 5.15.32
	Hardware revision: 0x00000220 (544)
	Driver version   : 5.15.32
Interface Info:
	ID               : 0x03000014
	Type             : V4L Sub-Device
Entity Info:
	ID               : 0x00000007 (7)
	Name             : dw-csi.0
	Function         : Video Interface Bridge
	Pad 0x01000008   : 0: Sink
	  Link 0x0200000e: from remote pad 0x100000d of entity 'imx219 1-0010' (Camera Sensor): Data, Enabled
	Pad 0x01000009   : 1: Source
	  Link 0x0200000a: to remote pad 0x1000005 of entity 'csi2dc' (Video Interface Bridge): Data, Enabled

Required ioctls:
	test MC information (see 'Media Driver Info' above): OK
	test VIDIOC_SUDBEV_QUERYCAP: OK
	test invalid ioctls: OK

Allow for multiple opens:
	test second /dev/v4l-subdev2 open: OK
	test VIDIOC_SUBDEV_QUERYCAP: OK
	test for unlimited opens: OK

Debug ioctls:
	test VIDIOC_LOG_STATUS: OK

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Sub-Device ioctls (Sink Pad 0):
	test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
		fail: v4l2-test-subdevs.cpp(324): fmt.width == 0 || fmt.width > 65536
		fail: v4l2-test-subdevs.cpp(369): checkMBusFrameFmt(node, fmt.format)
	test Try VIDIOC_SUBDEV_G/S_FMT: FAIL
	test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported)
	test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
		fail: v4l2-test-subdevs.cpp(324): fmt.width == 0 || fmt.width > 65536
		fail: v4l2-test-subdevs.cpp(369): checkMBusFrameFmt(node, fmt.format)
	test Active VIDIOC_SUBDEV_G/S_FMT: FAIL
	test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported)
	test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported)

Sub-Device ioctls (Source Pad 1):
	test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
		fail: v4l2-test-subdevs.cpp(324): fmt.width == 0 || fmt.width > 65536
		fail: v4l2-test-subdevs.cpp(369): checkMBusFrameFmt(node, fmt.format)
	test Try VIDIOC_SUBDEV_G/S_FMT: FAIL
	test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported)
	test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
		fail: v4l2-test-subdevs.cpp(324): fmt.width == 0 || fmt.width > 65536
		fail: v4l2-test-subdevs.cpp(369): checkMBusFrameFmt(node, fmt.format)
	test Active VIDIOC_SUBDEV_G/S_FMT: FAIL
	test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported)
	test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported)

Control ioctls:
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
	test VIDIOC_QUERYCTRL: OK (Not Supported)
	test VIDIOC_G/S_CTRL: OK (Not Supported)
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 0 Private Controls: 0

Format ioctls:
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported)
	test VIDIOC_G/S_PARM: OK (Not Supported)
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK (Not Supported)
	test VIDIOC_TRY_FMT: OK (Not Supported)
	test VIDIOC_S_FMT: OK (Not Supported)
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK (Not Supported)

Codec ioctls:
	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported)
	test VIDIOC_EXPBUF: OK (Not Supported)
	test Requests: OK (Not Supported)
	test TIME32/64: OK

Total for atmel_isc_commo device /dev/v4l-subdev2: 59, Succeeded: 55, Failed: 4, Warnings: 0
--------------------------------------------------------------------------------
Compliance test for device /dev/v4l-subdev3:

Driver Info:
	Driver version   : 5.15.32
	Capabilities     : 0x00000000

Required ioctls:
	test VIDIOC_SUDBEV_QUERYCAP: OK
	test invalid ioctls: OK

Allow for multiple opens:
	test second /dev/v4l-subdev3 open: OK
	test VIDIOC_SUBDEV_QUERYCAP: OK
	test for unlimited opens: OK

Debug ioctls:
	test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls:
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
	test VIDIOC_QUERYCTRL: OK
	test VIDIOC_G/S_CTRL: OK
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 17 Private Controls: 0

Format ioctls:
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported)
	test VIDIOC_G/S_PARM: OK (Not Supported)
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK (Not Supported)
	test VIDIOC_TRY_FMT: OK (Not Supported)
	test VIDIOC_S_FMT: OK (Not Supported)
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK (Not Supported)

Codec ioctls:
	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported)
	test VIDIOC_EXPBUF: OK (Not Supported)
	test Requests: OK (Not Supported)
	test TIME32/64: OK

Total for device /dev/v4l-subdev3: 44, Succeeded: 44, Failed: 0, Warnings: 0
--------------------------------------------------------------------------------
Compliance test for microchip-isc device /dev/video0:

Driver Info:
	Driver name      : microchip-isc
	Card type        : Atmel Image Sensor Controller
	Bus info         : platform:microchip-sama7g5-xisc
	Driver version   : 5.15.32
	Capabilities     : 0xa4200001
		Video Capture
		Streaming
		Extended Pix Format
		Device Capabilities
	Device Caps      : 0x24200001
		Video Capture
		Streaming
		Extended Pix Format
Media Driver Info:
	Driver name      : atmel_isc_commo
	Model            : microchip,sama7g5-isc
	Serial           : 
	Bus info         : platform:microchip-sama7g5-xisc
	Media version    : 5.15.32
	Hardware revision: 0x00000220 (544)
	Driver version   : 5.15.32
Interface Info:
	ID               : 0x0300001a
	Type             : V4L Video
Entity Info:
	ID               : 0x00000018 (24)
	Name             : atmel_isc_common
	Function         : V4L2 I/O
	Flags            : default
	Pad 0x01000019   : 0: Sink
	  Link 0x0200001e: from remote pad 0x1000003 of entity 'atmel_isc_scaler' (Video Scaler): Data, Enabled, Immutable

Required ioctls:
	test MC information (see 'Media Driver Info' above): OK
	test VIDIOC_QUERYCAP: OK
	test invalid ioctls: OK

Allow for multiple opens:
	test second /dev/video0 open: OK
	test VIDIOC_QUERYCAP: OK
	test VIDIOC_G/S_PRIORITY: OK
	test for unlimited opens: OK

Debug ioctls:
	test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
	test VIDIOC_LOG_STATUS: OK

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
	test VIDIOC_QUERYCTRL: OK
	test VIDIOC_G/S_CTRL: OK
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 6 Private Controls: 8

Format ioctls (Input 0):
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
	test VIDIOC_G/S_PARM: OK (Not Supported)
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK
	test VIDIOC_TRY_FMT: OK
	test VIDIOC_S_FMT: OK
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK

Codec ioctls (Input 0):
	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
	test VIDIOC_EXPBUF: OK
	test Requests: OK (Not Supported)
	test TIME32/64: OK

Total for microchip-isc device /dev/video0: 47, Succeeded: 47, Failed: 0, Warnings: 0

Grand Total for atmel_isc_commo device /dev/media0: 261, Succeeded: 257, Failed: 4, Warnings: 2

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Eugen.Hristev]

On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote:
> On 6/22/22 2:53 PM, Hans Verkuil wrote:
>> Hi Eugen,
>>
>> On 03/05/2022 11:51, Eugen Hristev wrote:
>>> This series is a split from the series :
>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
>>> and it includes the media controller part.
>>> previous fixes were sent on a different patch series.
>>>
>>> As discussed on the ML, moving forward with having the media link validate at
>>> start/stop streaming call.
>>> I will test the patch :
>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
>>
>> I'm looking at merging this series, but I would like to have the output of
>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
>> correct.
> 
> Hello Hans,
> 
> Please have a look at attached file . Unless you want me to add the
> whole output to the e-mail ?
> 
> I also added output of media-ctl -p for your convenience.
> the subdev2 is a device and driver that is not upstream and has some
> compliance issues, they are reported by the v4l2-compliance tool, but
> they should not affect this series, it's a synopsys driver that was
> rejected on mainline a few years ago, I took it for internal usage, but
> it's not cleaned up nor worked a lot upon.
> 
>>
>> And one more question which may have been answered already in the past:
>>
>> Changing to the MC will break existing applications, doesn't it? Or did I
>> miss something?
>>
> 
> The existing applications will have to configure the pipeline now. It
> will no longer work by configuring just the top video node /dev/video0 .
> They would have to use media-ctl for it, something similar with this set
> of commands:

To add on top of that, actually, the reality is that without the MC 
support in atmel-isc , some of our platforms do not work at all, because 
the csi2dc driver which is in the middle of the pipeline, is a MC 
driver. So it will not work without configuring it with MC anyway. It 
used to work in a very preliminary version of the csi2dc driver which I 
sent a few years ago, but that way of handling things was rejected. 
Hence I changed the csi2dc to being full-MC driver (requested for new 
drivers) and now I am completing the conversion for the whole pipeline.
We are using this MC-centric approach in production for our products to 
be as close as possible to mainline, and backported it to our 5.15 
internal releases, which people are using right now.

> 
> media-ctl -d /dev/media0 --set-v4l2 '"imx219
> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
> media-ctl -d /dev/media0 --set-v4l2
> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
> media-ctl -d /dev/media0 --set-v4l2
> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'
> 
> Thank you for taking care of this !
> 
> Eugen
> 
>> Regards,
>>
>>           Hans
>>
>>>
>>> Full series history:
>>>
>>> Changes in v10:
>>> -> split the series into this first fixes part.
>>> -> moved IO_MC addition from first patch to the second patch on the driver changes
>>> -> edited commit messages
>>> -> DT nodes now disabled by default.
>>>
>>> Changes in v9:
>>> -> kernel robot reported isc_link_validate is not static, changed to static.
>>>
>>> Changes in v8:
>>> -> scaler: modified crop bounds to have the exact source size
>>>
>>> Changes in v7:
>>> -> scaler: modified crop bounds to have maximum isc size
>>> -> format propagation: did small changes as per Jacopo review
>>>
>>>
>>> Changes in v6:
>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
>>>
>>> Changes in v5:
>>> -> removed patch that removed the 'stop' variable as it was still required
>>> -> added two new trivial patches
>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
>>>
>>>
>>> Changes in v4:
>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
>>> one patch that was using it
>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
>>>
>>>
>>> Changes in v3:
>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
>>> for the isc-base.
>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
>>>
>>> Changes in v2:
>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
>>> - add a few new patches
>>>
>>> Eugen Hristev (5):
>>>     media: atmel: atmel-isc: prepare for media controller support
>>>     media: atmel: atmel-isc: implement media controller
>>>     ARM: dts: at91: sama7g5: add nodes for video capture
>>>     ARM: configs: at91: sama7: add xisc and csi2dc
>>>     ARM: multi_v7_defconfig: add atmel video pipeline modules
>>>
>>>    arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>>>    arch/arm/configs/multi_v7_defconfig           |   3 +
>>>    arch/arm/configs/sama7_defconfig              |   2 +
>>>    drivers/media/platform/atmel/Makefile         |   2 +-
>>>    drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>>>    .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>>>    drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>>>    .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>>>    .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>>>    9 files changed, 685 insertions(+), 241 deletions(-)
>>>    create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
>>>
>>
> 

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Hans Verkuil]

On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote:
> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote:
>> On 6/22/22 2:53 PM, Hans Verkuil wrote:
>>> Hi Eugen,
>>>
>>> On 03/05/2022 11:51, Eugen Hristev wrote:
>>>> This series is a split from the series :
>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
>>>> and it includes the media controller part.
>>>> previous fixes were sent on a different patch series.
>>>>
>>>> As discussed on the ML, moving forward with having the media link validate at
>>>> start/stop streaming call.
>>>> I will test the patch :
>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
>>>
>>> I'm looking at merging this series, but I would like to have the output of
>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
>>> correct.
>>
>> Hello Hans,
>>
>> Please have a look at attached file . Unless you want me to add the
>> whole output to the e-mail ?
>>
>> I also added output of media-ctl -p for your convenience.
>> the subdev2 is a device and driver that is not upstream and has some
>> compliance issues, they are reported by the v4l2-compliance tool, but
>> they should not affect this series, it's a synopsys driver that was
>> rejected on mainline a few years ago, I took it for internal usage, but
>> it's not cleaned up nor worked a lot upon.
>>
>>>
>>> And one more question which may have been answered already in the past:
>>>
>>> Changing to the MC will break existing applications, doesn't it? Or did I
>>> miss something?
>>>
>>
>> The existing applications will have to configure the pipeline now. It
>> will no longer work by configuring just the top video node /dev/video0 .
>> They would have to use media-ctl for it, something similar with this set
>> of commands:
> 
> To add on top of that, actually, the reality is that without the MC 
> support in atmel-isc , some of our platforms do not work at all, because 
> the csi2dc driver which is in the middle of the pipeline, is a MC 
> driver. So it will not work without configuring it with MC anyway. It 
> used to work in a very preliminary version of the csi2dc driver which I 
> sent a few years ago, but that way of handling things was rejected. 
> Hence I changed the csi2dc to being full-MC driver (requested for new 
> drivers) and now I am completing the conversion for the whole pipeline.
> We are using this MC-centric approach in production for our products to 
> be as close as possible to mainline, and backported it to our 5.15 
> internal releases, which people are using right now.

I'm not all that keen on breaking userspace for those who do NOT use the
Atmel BSP. Basically some platforms are currently broken, and with this patch
series some other platforms are broken, but at least can be fixed by changing
userspace.

How feasible is it to do something similar that TI did for the cal driver?
(drivers/media/platform/ti/cal)

I.e., based on a module option the MC is enabled or disabled. And if a
csi2dc is present, then the MC API is always enabled.

Regards,

	Hans

> 
>>
>> media-ctl -d /dev/media0 --set-v4l2 '"imx219
>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
>> media-ctl -d /dev/media0 --set-v4l2
>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
>> media-ctl -d /dev/media0 --set-v4l2
>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'
>>
>> Thank you for taking care of this !
>>
>> Eugen
>>
>>> Regards,
>>>
>>>           Hans
>>>
>>>>
>>>> Full series history:
>>>>
>>>> Changes in v10:
>>>> -> split the series into this first fixes part.
>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes
>>>> -> edited commit messages
>>>> -> DT nodes now disabled by default.
>>>>
>>>> Changes in v9:
>>>> -> kernel robot reported isc_link_validate is not static, changed to static.
>>>>
>>>> Changes in v8:
>>>> -> scaler: modified crop bounds to have the exact source size
>>>>
>>>> Changes in v7:
>>>> -> scaler: modified crop bounds to have maximum isc size
>>>> -> format propagation: did small changes as per Jacopo review
>>>>
>>>>
>>>> Changes in v6:
>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
>>>>
>>>> Changes in v5:
>>>> -> removed patch that removed the 'stop' variable as it was still required
>>>> -> added two new trivial patches
>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
>>>>
>>>>
>>>> Changes in v4:
>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
>>>> one patch that was using it
>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
>>>>
>>>>
>>>> Changes in v3:
>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
>>>> for the isc-base.
>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
>>>>
>>>> Changes in v2:
>>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
>>>> - add a few new patches
>>>>
>>>> Eugen Hristev (5):
>>>>     media: atmel: atmel-isc: prepare for media controller support
>>>>     media: atmel: atmel-isc: implement media controller
>>>>     ARM: dts: at91: sama7g5: add nodes for video capture
>>>>     ARM: configs: at91: sama7: add xisc and csi2dc
>>>>     ARM: multi_v7_defconfig: add atmel video pipeline modules
>>>>
>>>>    arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>>>>    arch/arm/configs/multi_v7_defconfig           |   3 +
>>>>    arch/arm/configs/sama7_defconfig              |   2 +
>>>>    drivers/media/platform/atmel/Makefile         |   2 +-
>>>>    drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>>>>    .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>>>>    drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>>>>    .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>>>>    .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>>>>    9 files changed, 685 insertions(+), 241 deletions(-)
>>>>    create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
>>>>
>>>
>>
> 

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Eugen.Hristev]

On 6/22/22 4:47 PM, Hans Verkuil wrote:
> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote:
>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote:
>>> On 6/22/22 2:53 PM, Hans Verkuil wrote:
>>>> Hi Eugen,
>>>>
>>>> On 03/05/2022 11:51, Eugen Hristev wrote:
>>>>> This series is a split from the series :
>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
>>>>> and it includes the media controller part.
>>>>> previous fixes were sent on a different patch series.
>>>>>
>>>>> As discussed on the ML, moving forward with having the media link validate at
>>>>> start/stop streaming call.
>>>>> I will test the patch :
>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
>>>>
>>>> I'm looking at merging this series, but I would like to have the output of
>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
>>>> correct.
>>>
>>> Hello Hans,
>>>
>>> Please have a look at attached file . Unless you want me to add the
>>> whole output to the e-mail ?
>>>
>>> I also added output of media-ctl -p for your convenience.
>>> the subdev2 is a device and driver that is not upstream and has some
>>> compliance issues, they are reported by the v4l2-compliance tool, but
>>> they should not affect this series, it's a synopsys driver that was
>>> rejected on mainline a few years ago, I took it for internal usage, but
>>> it's not cleaned up nor worked a lot upon.
>>>
>>>>
>>>> And one more question which may have been answered already in the past:
>>>>
>>>> Changing to the MC will break existing applications, doesn't it? Or did I
>>>> miss something?
>>>>
>>>
>>> The existing applications will have to configure the pipeline now. It
>>> will no longer work by configuring just the top video node /dev/video0 .
>>> They would have to use media-ctl for it, something similar with this set
>>> of commands:
>>
>> To add on top of that, actually, the reality is that without the MC
>> support in atmel-isc , some of our platforms do not work at all, because
>> the csi2dc driver which is in the middle of the pipeline, is a MC
>> driver. So it will not work without configuring it with MC anyway. It
>> used to work in a very preliminary version of the csi2dc driver which I
>> sent a few years ago, but that way of handling things was rejected.
>> Hence I changed the csi2dc to being full-MC driver (requested for new
>> drivers) and now I am completing the conversion for the whole pipeline.
>> We are using this MC-centric approach in production for our products to
>> be as close as possible to mainline, and backported it to our 5.15
>> internal releases, which people are using right now.
> 
> I'm not all that keen on breaking userspace for those who do NOT use the
> Atmel BSP. Basically some platforms are currently broken, and with this patch
> series some other platforms are broken, but at least can be fixed by changing
> userspace.
> 
> How feasible is it to do something similar that TI did for the cal driver?
> (drivers/media/platform/ti/cal)
> 
> I.e., based on a module option the MC is enabled or disabled. And if a
> csi2dc is present, then the MC API is always enabled.

Some platforms that are now broken, never worked, because they would 
need all the modules to be configured with MC anyway.
The other platforms, that work now by just configuring the top video 
node, work, but in a quite limited way, as sensor drivers can be 
MC-capable and configurable from userspace, but the top video driver 
will overwrite their configuration based on it's own decision algorithm.
With the MC approach, all the platforms work, the drawback is, as you 
said, that the userspace has to configure the pipeline from head to toe; 
but , we knew that: moving to a MC approach, makes the old way of 
configuring the image capture simply 'not enough'.

It would be difficult to maintain a driver that would use the MC API and 
handle the things for itself and just check the pipeline; and in the 
same time if a module parameter is different, pass configuration down 
the pipeline and have an algorithm implemented that would interact with 
the subdev, ask for it's capabilities, and then decide on its own what 
the subdev would use. The driver would be a bit big and it would have a 
lot of code. That is one of the advantages of these patches, to simplify 
the driver.

I would prefer to not have to keep that code, and move to MC approach, 
but in the end you have arguments and you are in charge.

Eugen


> 
> Regards,
> 
>          Hans
> 
>>
>>>
>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219
>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
>>> media-ctl -d /dev/media0 --set-v4l2
>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
>>> media-ctl -d /dev/media0 --set-v4l2
>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'
>>>
>>> Thank you for taking care of this !
>>>
>>> Eugen
>>>
>>>> Regards,
>>>>
>>>>            Hans
>>>>
>>>>>
>>>>> Full series history:
>>>>>
>>>>> Changes in v10:
>>>>> -> split the series into this first fixes part.
>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes
>>>>> -> edited commit messages
>>>>> -> DT nodes now disabled by default.
>>>>>
>>>>> Changes in v9:
>>>>> -> kernel robot reported isc_link_validate is not static, changed to static.
>>>>>
>>>>> Changes in v8:
>>>>> -> scaler: modified crop bounds to have the exact source size
>>>>>
>>>>> Changes in v7:
>>>>> -> scaler: modified crop bounds to have maximum isc size
>>>>> -> format propagation: did small changes as per Jacopo review
>>>>>
>>>>>
>>>>> Changes in v6:
>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
>>>>>
>>>>> Changes in v5:
>>>>> -> removed patch that removed the 'stop' variable as it was still required
>>>>> -> added two new trivial patches
>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
>>>>>
>>>>>
>>>>> Changes in v4:
>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
>>>>> one patch that was using it
>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
>>>>>
>>>>>
>>>>> Changes in v3:
>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
>>>>> for the isc-base.
>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
>>>>>
>>>>> Changes in v2:
>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
>>>>> - add a few new patches
>>>>>
>>>>> Eugen Hristev (5):
>>>>>      media: atmel: atmel-isc: prepare for media controller support
>>>>>      media: atmel: atmel-isc: implement media controller
>>>>>      ARM: dts: at91: sama7g5: add nodes for video capture
>>>>>      ARM: configs: at91: sama7: add xisc and csi2dc
>>>>>      ARM: multi_v7_defconfig: add atmel video pipeline modules
>>>>>
>>>>>     arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>>>>>     arch/arm/configs/multi_v7_defconfig           |   3 +
>>>>>     arch/arm/configs/sama7_defconfig              |   2 +
>>>>>     drivers/media/platform/atmel/Makefile         |   2 +-
>>>>>     drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>>>>>     .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>>>>>     drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>>>>>     .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>>>>>     .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>>>>>     9 files changed, 685 insertions(+), 241 deletions(-)
>>>>>     create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
>>>>>
>>>>
>>>
>>
> 

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Jacopo Mondi]

Hi Hans, Eugen

On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote:
> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote:
> > On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote:
> >> On 6/22/22 2:53 PM, Hans Verkuil wrote:
> >>> Hi Eugen,
> >>>
> >>> On 03/05/2022 11:51, Eugen Hristev wrote:
> >>>> This series is a split from the series :
> >>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
> >>>> and it includes the media controller part.
> >>>> previous fixes were sent on a different patch series.
> >>>>
> >>>> As discussed on the ML, moving forward with having the media link validate at
> >>>> start/stop streaming call.
> >>>> I will test the patch :
> >>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
> >>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
> >>>
> >>> I'm looking at merging this series, but I would like to have the output of
> >>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
> >>> correct.
> >>
> >> Hello Hans,
> >>
> >> Please have a look at attached file . Unless you want me to add the
> >> whole output to the e-mail ?
> >>
> >> I also added output of media-ctl -p for your convenience.
> >> the subdev2 is a device and driver that is not upstream and has some
> >> compliance issues, they are reported by the v4l2-compliance tool, but
> >> they should not affect this series, it's a synopsys driver that was
> >> rejected on mainline a few years ago, I took it for internal usage, but
> >> it's not cleaned up nor worked a lot upon.
> >>
> >>>
> >>> And one more question which may have been answered already in the past:
> >>>
> >>> Changing to the MC will break existing applications, doesn't it? Or did I
> >>> miss something?
> >>>
> >>
> >> The existing applications will have to configure the pipeline now. It
> >> will no longer work by configuring just the top video node /dev/video0 .
> >> They would have to use media-ctl for it, something similar with this set
> >> of commands:
> >
> > To add on top of that, actually, the reality is that without the MC
> > support in atmel-isc , some of our platforms do not work at all, because
> > the csi2dc driver which is in the middle of the pipeline, is a MC
> > driver. So it will not work without configuring it with MC anyway. It
> > used to work in a very preliminary version of the csi2dc driver which I
> > sent a few years ago, but that way of handling things was rejected.
> > Hence I changed the csi2dc to being full-MC driver (requested for new
> > drivers) and now I am completing the conversion for the whole pipeline.
> > We are using this MC-centric approach in production for our products to
> > be as close as possible to mainline, and backported it to our 5.15
> > internal releases, which people are using right now.
>
> I'm not all that keen on breaking userspace for those who do NOT use the
> Atmel BSP. Basically some platforms are currently broken, and with this patch
> series some other platforms are broken, but at least can be fixed by changing
> userspace.
>
> How feasible is it to do something similar that TI did for the cal driver?
> (drivers/media/platform/ti/cal)
>
> I.e., based on a module option the MC is enabled or disabled. And if a
> csi2dc is present, then the MC API is always enabled.
>

I think I have suggested Eugen to move to MC when he
started looking in libcamera, so sorry for the intrusion but I feel
a bit bad for not rising the point earlier and get him to v10

I understand your point Hans, and when a vendor upstreaming code or a
user requires to maintain compatibility, the burden of keeping more
code in to handle the MC and non-MC cases is worth the complications.

But if even the vendor wants to move to MC to allow more use-cases I
think we have to acknolege that if you're running mainline on an
embedded system you could expect to adjust your setup between kernel
updates. The idea to document the media-ctl commands required to setup
the pipeline it's helpful, and might help in the interim period until
the platform is not supported by libcamera.

That said, if Eugen wants to give the flag a try I won't
oppose :)


> Regards,
>
> 	Hans
>
> >
> >>
> >> media-ctl -d /dev/media0 --set-v4l2 '"imx219
> >> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
> >> media-ctl -d /dev/media0 --set-v4l2
> >> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
> >> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
> >> media-ctl -d /dev/media0 --set-v4l2
> >> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'
> >>
> >> Thank you for taking care of this !
> >>
> >> Eugen
> >>
> >>> Regards,
> >>>
> >>>           Hans
> >>>
> >>>>
> >>>> Full series history:
> >>>>
> >>>> Changes in v10:
> >>>> -> split the series into this first fixes part.
> >>>> -> moved IO_MC addition from first patch to the second patch on the driver changes
> >>>> -> edited commit messages
> >>>> -> DT nodes now disabled by default.
> >>>>
> >>>> Changes in v9:
> >>>> -> kernel robot reported isc_link_validate is not static, changed to static.
> >>>>
> >>>> Changes in v8:
> >>>> -> scaler: modified crop bounds to have the exact source size
> >>>>
> >>>> Changes in v7:
> >>>> -> scaler: modified crop bounds to have maximum isc size
> >>>> -> format propagation: did small changes as per Jacopo review
> >>>>
> >>>>
> >>>> Changes in v6:
> >>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
> >>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
> >>>>
> >>>> Changes in v5:
> >>>> -> removed patch that removed the 'stop' variable as it was still required
> >>>> -> added two new trivial patches
> >>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
> >>>>
> >>>>
> >>>> Changes in v4:
> >>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
> >>>> one patch that was using it
> >>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
> >>>>
> >>>>
> >>>> Changes in v3:
> >>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
> >>>> for the isc-base.
> >>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
> >>>>
> >>>> Changes in v2:
> >>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
> >>>> - add a few new patches
> >>>>
> >>>> Eugen Hristev (5):
> >>>>     media: atmel: atmel-isc: prepare for media controller support
> >>>>     media: atmel: atmel-isc: implement media controller
> >>>>     ARM: dts: at91: sama7g5: add nodes for video capture
> >>>>     ARM: configs: at91: sama7: add xisc and csi2dc
> >>>>     ARM: multi_v7_defconfig: add atmel video pipeline modules
> >>>>
> >>>>    arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
> >>>>    arch/arm/configs/multi_v7_defconfig           |   3 +
> >>>>    arch/arm/configs/sama7_defconfig              |   2 +
> >>>>    drivers/media/platform/atmel/Makefile         |   2 +-
> >>>>    drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
> >>>>    .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
> >>>>    drivers/media/platform/atmel/atmel-isc.h      |  50 +-
> >>>>    .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
> >>>>    .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
> >>>>    9 files changed, 685 insertions(+), 241 deletions(-)
> >>>>    create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
> >>>>
> >>>
> >>
> >
>

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Eugen.Hristev]

On 6/22/22 5:14 PM, Jacopo Mondi wrote:
> Hi Hans, Eugen
> 
> On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote:
>> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote:
>>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote:
>>>> On 6/22/22 2:53 PM, Hans Verkuil wrote:
>>>>> Hi Eugen,
>>>>>
>>>>> On 03/05/2022 11:51, Eugen Hristev wrote:
>>>>>> This series is a split from the series :
>>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
>>>>>> and it includes the media controller part.
>>>>>> previous fixes were sent on a different patch series.
>>>>>>
>>>>>> As discussed on the ML, moving forward with having the media link validate at
>>>>>> start/stop streaming call.
>>>>>> I will test the patch :
>>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
>>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
>>>>>
>>>>> I'm looking at merging this series, but I would like to have the output of
>>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
>>>>> correct.
>>>>
>>>> Hello Hans,
>>>>
>>>> Please have a look at attached file . Unless you want me to add the
>>>> whole output to the e-mail ?
>>>>
>>>> I also added output of media-ctl -p for your convenience.
>>>> the subdev2 is a device and driver that is not upstream and has some
>>>> compliance issues, they are reported by the v4l2-compliance tool, but
>>>> they should not affect this series, it's a synopsys driver that was
>>>> rejected on mainline a few years ago, I took it for internal usage, but
>>>> it's not cleaned up nor worked a lot upon.
>>>>
>>>>>
>>>>> And one more question which may have been answered already in the past:
>>>>>
>>>>> Changing to the MC will break existing applications, doesn't it? Or did I
>>>>> miss something?
>>>>>
>>>>
>>>> The existing applications will have to configure the pipeline now. It
>>>> will no longer work by configuring just the top video node /dev/video0 .
>>>> They would have to use media-ctl for it, something similar with this set
>>>> of commands:
>>>
>>> To add on top of that, actually, the reality is that without the MC
>>> support in atmel-isc , some of our platforms do not work at all, because
>>> the csi2dc driver which is in the middle of the pipeline, is a MC
>>> driver. So it will not work without configuring it with MC anyway. It
>>> used to work in a very preliminary version of the csi2dc driver which I
>>> sent a few years ago, but that way of handling things was rejected.
>>> Hence I changed the csi2dc to being full-MC driver (requested for new
>>> drivers) and now I am completing the conversion for the whole pipeline.
>>> We are using this MC-centric approach in production for our products to
>>> be as close as possible to mainline, and backported it to our 5.15
>>> internal releases, which people are using right now.
>>
>> I'm not all that keen on breaking userspace for those who do NOT use the
>> Atmel BSP. Basically some platforms are currently broken, and with this patch
>> series some other platforms are broken, but at least can be fixed by changing
>> userspace.
>>
>> How feasible is it to do something similar that TI did for the cal driver?
>> (drivers/media/platform/ti/cal)
>>
>> I.e., based on a module option the MC is enabled or disabled. And if a
>> csi2dc is present, then the MC API is always enabled.
>>
> 
> I think I have suggested Eugen to move to MC when he
> started looking in libcamera, so sorry for the intrusion but I feel
> a bit bad for not rising the point earlier and get him to v10

Don't get me wrong, I like the MC approach, after implementing it, I am 
happy how the driver turned out. It's much simpler and covering a 
plethora of new use cases which were previously not available, as it was 
much more rigid. So I agree with what you suggested, and I support the 
idea as well.

> 
> I understand your point Hans, and when a vendor upstreaming code or a
> user requires to maintain compatibility, the burden of keeping more
> code in to handle the MC and non-MC cases is worth the complications.

We are pretty much convinced to use the MC-only approach and are moving 
in that direction. But, if we have to keep the old code to maintain 
backwards compatibility , we have no choice.
However, we will move forward, and only use the MC approach from now on, 
and will no longer use this driver without MC. That use case will be out 
of our scope. If there are people using it and it works, all the better 
then.



> 
> But if even the vendor wants to move to MC to allow more use-cases I
> think we have to acknolege that if you're running mainline on an
> embedded system you could expect to adjust your setup between kernel
> updates. The idea to document the media-ctl commands required to setup
> the pipeline it's helpful, and might help in the interim period until
> the platform is not supported by libcamera.
> 
> That said, if Eugen wants to give the flag a try I won't
> oppose :)
> 
> 
>> Regards,
>>
>>        Hans
>>
>>>
>>>>
>>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219
>>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
>>>> media-ctl -d /dev/media0 --set-v4l2
>>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
>>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
>>>> media-ctl -d /dev/media0 --set-v4l2
>>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'
>>>>
>>>> Thank you for taking care of this !
>>>>
>>>> Eugen
>>>>
>>>>> Regards,
>>>>>
>>>>>            Hans
>>>>>
>>>>>>
>>>>>> Full series history:
>>>>>>
>>>>>> Changes in v10:
>>>>>> -> split the series into this first fixes part.
>>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes
>>>>>> -> edited commit messages
>>>>>> -> DT nodes now disabled by default.
>>>>>>
>>>>>> Changes in v9:
>>>>>> -> kernel robot reported isc_link_validate is not static, changed to static.
>>>>>>
>>>>>> Changes in v8:
>>>>>> -> scaler: modified crop bounds to have the exact source size
>>>>>>
>>>>>> Changes in v7:
>>>>>> -> scaler: modified crop bounds to have maximum isc size
>>>>>> -> format propagation: did small changes as per Jacopo review
>>>>>>
>>>>>>
>>>>>> Changes in v6:
>>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
>>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
>>>>>>
>>>>>> Changes in v5:
>>>>>> -> removed patch that removed the 'stop' variable as it was still required
>>>>>> -> added two new trivial patches
>>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
>>>>>>
>>>>>>
>>>>>> Changes in v4:
>>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
>>>>>> one patch that was using it
>>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
>>>>>>
>>>>>>
>>>>>> Changes in v3:
>>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
>>>>>> for the isc-base.
>>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
>>>>>>
>>>>>> Changes in v2:
>>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
>>>>>> - add a few new patches
>>>>>>
>>>>>> Eugen Hristev (5):
>>>>>>      media: atmel: atmel-isc: prepare for media controller support
>>>>>>      media: atmel: atmel-isc: implement media controller
>>>>>>      ARM: dts: at91: sama7g5: add nodes for video capture
>>>>>>      ARM: configs: at91: sama7: add xisc and csi2dc
>>>>>>      ARM: multi_v7_defconfig: add atmel video pipeline modules
>>>>>>
>>>>>>     arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>>>>>>     arch/arm/configs/multi_v7_defconfig           |   3 +
>>>>>>     arch/arm/configs/sama7_defconfig              |   2 +
>>>>>>     drivers/media/platform/atmel/Makefile         |   2 +-
>>>>>>     drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>>>>>>     .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>>>>>>     drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>>>>>>     .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>>>>>>     .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>>>>>>     9 files changed, 685 insertions(+), 241 deletions(-)
>>>>>>     create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
>>>>>>
>>>>>
>>>>
>>>
>>

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Hans Verkuil]

Hi Eugen, Jacopo,

On 22/06/2022 16:14, Jacopo Mondi wrote:
> Hi Hans, Eugen
> 
> On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote:
>> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote:
>>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote:
>>>> On 6/22/22 2:53 PM, Hans Verkuil wrote:
>>>>> Hi Eugen,
>>>>>
>>>>> On 03/05/2022 11:51, Eugen Hristev wrote:
>>>>>> This series is a split from the series :
>>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
>>>>>> and it includes the media controller part.
>>>>>> previous fixes were sent on a different patch series.
>>>>>>
>>>>>> As discussed on the ML, moving forward with having the media link validate at
>>>>>> start/stop streaming call.
>>>>>> I will test the patch :
>>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
>>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
>>>>>
>>>>> I'm looking at merging this series, but I would like to have the output of
>>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
>>>>> correct.
>>>>
>>>> Hello Hans,
>>>>
>>>> Please have a look at attached file . Unless you want me to add the
>>>> whole output to the e-mail ?
>>>>
>>>> I also added output of media-ctl -p for your convenience.
>>>> the subdev2 is a device and driver that is not upstream and has some
>>>> compliance issues, they are reported by the v4l2-compliance tool, but
>>>> they should not affect this series, it's a synopsys driver that was
>>>> rejected on mainline a few years ago, I took it for internal usage, but
>>>> it's not cleaned up nor worked a lot upon.
>>>>
>>>>>
>>>>> And one more question which may have been answered already in the past:
>>>>>
>>>>> Changing to the MC will break existing applications, doesn't it? Or did I
>>>>> miss something?
>>>>>
>>>>
>>>> The existing applications will have to configure the pipeline now. It
>>>> will no longer work by configuring just the top video node /dev/video0 .
>>>> They would have to use media-ctl for it, something similar with this set
>>>> of commands:
>>>
>>> To add on top of that, actually, the reality is that without the MC
>>> support in atmel-isc , some of our platforms do not work at all, because
>>> the csi2dc driver which is in the middle of the pipeline, is a MC
>>> driver. So it will not work without configuring it with MC anyway. It
>>> used to work in a very preliminary version of the csi2dc driver which I
>>> sent a few years ago, but that way of handling things was rejected.
>>> Hence I changed the csi2dc to being full-MC driver (requested for new
>>> drivers) and now I am completing the conversion for the whole pipeline.
>>> We are using this MC-centric approach in production for our products to
>>> be as close as possible to mainline, and backported it to our 5.15
>>> internal releases, which people are using right now.
>>
>> I'm not all that keen on breaking userspace for those who do NOT use the
>> Atmel BSP. Basically some platforms are currently broken, and with this patch
>> series some other platforms are broken, but at least can be fixed by changing
>> userspace.
>>
>> How feasible is it to do something similar that TI did for the cal driver?
>> (drivers/media/platform/ti/cal)
>>
>> I.e., based on a module option the MC is enabled or disabled. And if a
>> csi2dc is present, then the MC API is always enabled.
>>
> 
> I think I have suggested Eugen to move to MC when he
> started looking in libcamera, so sorry for the intrusion but I feel
> a bit bad for not rising the point earlier and get him to v10
> 
> I understand your point Hans, and when a vendor upstreaming code or a
> user requires to maintain compatibility, the burden of keeping more
> code in to handle the MC and non-MC cases is worth the complications.

Eugen, can you provide a list of platforms that will break with this
change and which platforms are currently broken without this series?

I'm trying to get a bit of a feel of the potential problems this change
will introduce.

> 
> But if even the vendor wants to move to MC to allow more use-cases I
> think we have to acknolege that if you're running mainline on an
> embedded system you could expect to adjust your setup between kernel
> updates. The idea to document the media-ctl commands required to setup
> the pipeline it's helpful, and might help in the interim period until
> the platform is not supported by libcamera.

Well, I don't want Linus to start yelling at me for breaking userspace :-)

We have broken userspace API (intentionally) in the past, but only with
good reasons. And sometimes a driver is used so rarely that it is not worth
the effort to try and keep compatible.

As a developer I'd love to just forget about the old API, but as subsystem
maintainer I need good arguments.

Another option might be to take the TI cal approach, but have warnings that
it will be removed in, say, 2 years time. Or even make a copy of the driver
for the old platforms, and perhaps move that to staging to be removed eventually.

The idea of a sudden breakage when going from kernel K to K+1 doesn't sit
well with me, if there was a transition period of 1-2 years then that would be
better.

Regards,

	Hans

> 
> That said, if Eugen wants to give the flag a try I won't
> oppose :)
> 
> 
>> Regards,
>>
>> 	Hans
>>
>>>
>>>>
>>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219
>>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
>>>> media-ctl -d /dev/media0 --set-v4l2
>>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
>>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
>>>> media-ctl -d /dev/media0 --set-v4l2
>>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'
>>>>
>>>> Thank you for taking care of this !
>>>>
>>>> Eugen
>>>>
>>>>> Regards,
>>>>>
>>>>>           Hans
>>>>>
>>>>>>
>>>>>> Full series history:
>>>>>>
>>>>>> Changes in v10:
>>>>>> -> split the series into this first fixes part.
>>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes
>>>>>> -> edited commit messages
>>>>>> -> DT nodes now disabled by default.
>>>>>>
>>>>>> Changes in v9:
>>>>>> -> kernel robot reported isc_link_validate is not static, changed to static.
>>>>>>
>>>>>> Changes in v8:
>>>>>> -> scaler: modified crop bounds to have the exact source size
>>>>>>
>>>>>> Changes in v7:
>>>>>> -> scaler: modified crop bounds to have maximum isc size
>>>>>> -> format propagation: did small changes as per Jacopo review
>>>>>>
>>>>>>
>>>>>> Changes in v6:
>>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
>>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
>>>>>>
>>>>>> Changes in v5:
>>>>>> -> removed patch that removed the 'stop' variable as it was still required
>>>>>> -> added two new trivial patches
>>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
>>>>>>
>>>>>>
>>>>>> Changes in v4:
>>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
>>>>>> one patch that was using it
>>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
>>>>>>
>>>>>>
>>>>>> Changes in v3:
>>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
>>>>>> for the isc-base.
>>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
>>>>>>
>>>>>> Changes in v2:
>>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
>>>>>> - add a few new patches
>>>>>>
>>>>>> Eugen Hristev (5):
>>>>>>     media: atmel: atmel-isc: prepare for media controller support
>>>>>>     media: atmel: atmel-isc: implement media controller
>>>>>>     ARM: dts: at91: sama7g5: add nodes for video capture
>>>>>>     ARM: configs: at91: sama7: add xisc and csi2dc
>>>>>>     ARM: multi_v7_defconfig: add atmel video pipeline modules
>>>>>>
>>>>>>    arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>>>>>>    arch/arm/configs/multi_v7_defconfig           |   3 +
>>>>>>    arch/arm/configs/sama7_defconfig              |   2 +
>>>>>>    drivers/media/platform/atmel/Makefile         |   2 +-
>>>>>>    drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>>>>>>    .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>>>>>>    drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>>>>>>    .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>>>>>>    .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>>>>>>    9 files changed, 685 insertions(+), 241 deletions(-)
>>>>>>    create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
>>>>>>
>>>>>
>>>>
>>>
>>

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Jacopo Mondi]

Hi Hans,

On Wed, Jun 22, 2022 at 04:55:27PM +0200, Hans Verkuil wrote:
> Hi Eugen, Jacopo,
>
> On 22/06/2022 16:14, Jacopo Mondi wrote:
> > Hi Hans, Eugen
> >
> > On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote:
> >> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote:
> >>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote:
> >>>> On 6/22/22 2:53 PM, Hans Verkuil wrote:
> >>>>> Hi Eugen,
> >>>>>
> >>>>> On 03/05/2022 11:51, Eugen Hristev wrote:
> >>>>>> This series is a split from the series :
> >>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
> >>>>>> and it includes the media controller part.
> >>>>>> previous fixes were sent on a different patch series.
> >>>>>>
> >>>>>> As discussed on the ML, moving forward with having the media link validate at
> >>>>>> start/stop streaming call.
> >>>>>> I will test the patch :
> >>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
> >>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
> >>>>>
> >>>>> I'm looking at merging this series, but I would like to have the output of
> >>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
> >>>>> correct.
> >>>>
> >>>> Hello Hans,
> >>>>
> >>>> Please have a look at attached file . Unless you want me to add the
> >>>> whole output to the e-mail ?
> >>>>
> >>>> I also added output of media-ctl -p for your convenience.
> >>>> the subdev2 is a device and driver that is not upstream and has some
> >>>> compliance issues, they are reported by the v4l2-compliance tool, but
> >>>> they should not affect this series, it's a synopsys driver that was
> >>>> rejected on mainline a few years ago, I took it for internal usage, but
> >>>> it's not cleaned up nor worked a lot upon.
> >>>>
> >>>>>
> >>>>> And one more question which may have been answered already in the past:
> >>>>>
> >>>>> Changing to the MC will break existing applications, doesn't it? Or did I
> >>>>> miss something?
> >>>>>
> >>>>
> >>>> The existing applications will have to configure the pipeline now. It
> >>>> will no longer work by configuring just the top video node /dev/video0 .
> >>>> They would have to use media-ctl for it, something similar with this set
> >>>> of commands:
> >>>
> >>> To add on top of that, actually, the reality is that without the MC
> >>> support in atmel-isc , some of our platforms do not work at all, because
> >>> the csi2dc driver which is in the middle of the pipeline, is a MC
> >>> driver. So it will not work without configuring it with MC anyway. It
> >>> used to work in a very preliminary version of the csi2dc driver which I
> >>> sent a few years ago, but that way of handling things was rejected.
> >>> Hence I changed the csi2dc to being full-MC driver (requested for new
> >>> drivers) and now I am completing the conversion for the whole pipeline.
> >>> We are using this MC-centric approach in production for our products to
> >>> be as close as possible to mainline, and backported it to our 5.15
> >>> internal releases, which people are using right now.
> >>
> >> I'm not all that keen on breaking userspace for those who do NOT use the
> >> Atmel BSP. Basically some platforms are currently broken, and with this patch
> >> series some other platforms are broken, but at least can be fixed by changing
> >> userspace.
> >>
> >> How feasible is it to do something similar that TI did for the cal driver?
> >> (drivers/media/platform/ti/cal)
> >>
> >> I.e., based on a module option the MC is enabled or disabled. And if a
> >> csi2dc is present, then the MC API is always enabled.
> >>
> >
> > I think I have suggested Eugen to move to MC when he
> > started looking in libcamera, so sorry for the intrusion but I feel
> > a bit bad for not rising the point earlier and get him to v10
> >
> > I understand your point Hans, and when a vendor upstreaming code or a
> > user requires to maintain compatibility, the burden of keeping more
> > code in to handle the MC and non-MC cases is worth the complications.
>
> Eugen, can you provide a list of platforms that will break with this
> change and which platforms are currently broken without this series?
>
> I'm trying to get a bit of a feel of the potential problems this change
> will introduce.
>
> >
> > But if even the vendor wants to move to MC to allow more use-cases I
> > think we have to acknolege that if you're running mainline on an
> > embedded system you could expect to adjust your setup between kernel
> > updates. The idea to document the media-ctl commands required to setup
> > the pipeline it's helpful, and might help in the interim period until
> > the platform is not supported by libcamera.
>
> Well, I don't want Linus to start yelling at me for breaking userspace :-)
>
> We have broken userspace API (intentionally) in the past, but only with
> good reasons. And sometimes a driver is used so rarely that it is not worth
> the effort to try and keep compatible.
>
> As a developer I'd love to just forget about the old API, but as subsystem
> maintainer I need good arguments.

I understand and I think these are all valid concerns. Finding a
balance between new features and legacy is not easy.

>
> Another option might be to take the TI cal approach, but have warnings that
> it will be removed in, say, 2 years time. Or even make a copy of the driver
> for the old platforms, and perhaps move that to staging to be removed eventually.
>
> The idea of a sudden breakage when going from kernel K to K+1 doesn't sit
> well with me, if there was a transition period of 1-2 years then that would be
> better.
>

If staging works for you that's probably the easiest option. Let's see
what Eugen prefers!

> Regards,
>
> 	Hans
>
> >
> > That said, if Eugen wants to give the flag a try I won't
> > oppose :)
> >
> >
> >> Regards,
> >>
> >> 	Hans
> >>
> >>>
> >>>>
> >>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219
> >>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
> >>>> media-ctl -d /dev/media0 --set-v4l2
> >>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
> >>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
> >>>> media-ctl -d /dev/media0 --set-v4l2
> >>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'
> >>>>
> >>>> Thank you for taking care of this !
> >>>>
> >>>> Eugen
> >>>>
> >>>>> Regards,
> >>>>>
> >>>>>           Hans
> >>>>>
> >>>>>>
> >>>>>> Full series history:
> >>>>>>
> >>>>>> Changes in v10:
> >>>>>> -> split the series into this first fixes part.
> >>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes
> >>>>>> -> edited commit messages
> >>>>>> -> DT nodes now disabled by default.
> >>>>>>
> >>>>>> Changes in v9:
> >>>>>> -> kernel robot reported isc_link_validate is not static, changed to static.
> >>>>>>
> >>>>>> Changes in v8:
> >>>>>> -> scaler: modified crop bounds to have the exact source size
> >>>>>>
> >>>>>> Changes in v7:
> >>>>>> -> scaler: modified crop bounds to have maximum isc size
> >>>>>> -> format propagation: did small changes as per Jacopo review
> >>>>>>
> >>>>>>
> >>>>>> Changes in v6:
> >>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
> >>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
> >>>>>>
> >>>>>> Changes in v5:
> >>>>>> -> removed patch that removed the 'stop' variable as it was still required
> >>>>>> -> added two new trivial patches
> >>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
> >>>>>>
> >>>>>>
> >>>>>> Changes in v4:
> >>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
> >>>>>> one patch that was using it
> >>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
> >>>>>>
> >>>>>>
> >>>>>> Changes in v3:
> >>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
> >>>>>> for the isc-base.
> >>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
> >>>>>>
> >>>>>> Changes in v2:
> >>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
> >>>>>> - add a few new patches
> >>>>>>
> >>>>>> Eugen Hristev (5):
> >>>>>>     media: atmel: atmel-isc: prepare for media controller support
> >>>>>>     media: atmel: atmel-isc: implement media controller
> >>>>>>     ARM: dts: at91: sama7g5: add nodes for video capture
> >>>>>>     ARM: configs: at91: sama7: add xisc and csi2dc
> >>>>>>     ARM: multi_v7_defconfig: add atmel video pipeline modules
> >>>>>>
> >>>>>>    arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
> >>>>>>    arch/arm/configs/multi_v7_defconfig           |   3 +
> >>>>>>    arch/arm/configs/sama7_defconfig              |   2 +
> >>>>>>    drivers/media/platform/atmel/Makefile         |   2 +-
> >>>>>>    drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
> >>>>>>    .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
> >>>>>>    drivers/media/platform/atmel/atmel-isc.h      |  50 +-
> >>>>>>    .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
> >>>>>>    .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
> >>>>>>    9 files changed, 685 insertions(+), 241 deletions(-)
> >>>>>>    create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
>

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Eugen.Hristev]

On 6/22/22 6:46 PM, Jacopo Mondi wrote:
> Hi Hans,
> 
> On Wed, Jun 22, 2022 at 04:55:27PM +0200, Hans Verkuil wrote:
>> Hi Eugen, Jacopo,
>>
>> On 22/06/2022 16:14, Jacopo Mondi wrote:
>>> Hi Hans, Eugen
>>>
>>> On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote:
>>>> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote:
>>>>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote:
>>>>>> On 6/22/22 2:53 PM, Hans Verkuil wrote:
>>>>>>> Hi Eugen,
>>>>>>>
>>>>>>> On 03/05/2022 11:51, Eugen Hristev wrote:
>>>>>>>> This series is a split from the series :
>>>>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
>>>>>>>> and it includes the media controller part.
>>>>>>>> previous fixes were sent on a different patch series.
>>>>>>>>
>>>>>>>> As discussed on the ML, moving forward with having the media link validate at
>>>>>>>> start/stop streaming call.
>>>>>>>> I will test the patch :
>>>>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
>>>>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
>>>>>>>
>>>>>>> I'm looking at merging this series, but I would like to have the output of
>>>>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
>>>>>>> correct.
>>>>>>
>>>>>> Hello Hans,
>>>>>>
>>>>>> Please have a look at attached file . Unless you want me to add the
>>>>>> whole output to the e-mail ?
>>>>>>
>>>>>> I also added output of media-ctl -p for your convenience.
>>>>>> the subdev2 is a device and driver that is not upstream and has some
>>>>>> compliance issues, they are reported by the v4l2-compliance tool, but
>>>>>> they should not affect this series, it's a synopsys driver that was
>>>>>> rejected on mainline a few years ago, I took it for internal usage, but
>>>>>> it's not cleaned up nor worked a lot upon.
>>>>>>
>>>>>>>
>>>>>>> And one more question which may have been answered already in the past:
>>>>>>>
>>>>>>> Changing to the MC will break existing applications, doesn't it? Or did I
>>>>>>> miss something?
>>>>>>>
>>>>>>
>>>>>> The existing applications will have to configure the pipeline now. It
>>>>>> will no longer work by configuring just the top video node /dev/video0 .
>>>>>> They would have to use media-ctl for it, something similar with this set
>>>>>> of commands:
>>>>>
>>>>> To add on top of that, actually, the reality is that without the MC
>>>>> support in atmel-isc , some of our platforms do not work at all, because
>>>>> the csi2dc driver which is in the middle of the pipeline, is a MC
>>>>> driver. So it will not work without configuring it with MC anyway. It
>>>>> used to work in a very preliminary version of the csi2dc driver which I
>>>>> sent a few years ago, but that way of handling things was rejected.
>>>>> Hence I changed the csi2dc to being full-MC driver (requested for new
>>>>> drivers) and now I am completing the conversion for the whole pipeline.
>>>>> We are using this MC-centric approach in production for our products to
>>>>> be as close as possible to mainline, and backported it to our 5.15
>>>>> internal releases, which people are using right now.
>>>>
>>>> I'm not all that keen on breaking userspace for those who do NOT use the
>>>> Atmel BSP. Basically some platforms are currently broken, and with this patch
>>>> series some other platforms are broken, but at least can be fixed by changing
>>>> userspace.
>>>>
>>>> How feasible is it to do something similar that TI did for the cal driver?
>>>> (drivers/media/platform/ti/cal)
>>>>
>>>> I.e., based on a module option the MC is enabled or disabled. And if a
>>>> csi2dc is present, then the MC API is always enabled.
>>>>
>>>
>>> I think I have suggested Eugen to move to MC when he
>>> started looking in libcamera, so sorry for the intrusion but I feel
>>> a bit bad for not rising the point earlier and get him to v10
>>>
>>> I understand your point Hans, and when a vendor upstreaming code or a
>>> user requires to maintain compatibility, the burden of keeping more
>>> code in to handle the MC and non-MC cases is worth the complications.
>>
>> Eugen, can you provide a list of platforms that will break with this
>> change and which platforms are currently broken without this series?

Hi Hans,

Basically the sama5d2 platform (we have several versions of the chip : 
sama5d21, sama5d27, sama5d29, in various packages, SIP, SoMs, on many 
different boards ) would be broken. this is the old platform.
It would be broken if the sensor default format is a mismatch with the 
default format of the ISC .  Basically the old code currently is 
propagating all the frame information down to the sensor, thing that no 
longer happens with this patch series.

The platform that needs MC is mainly sama7g5 , which has a longer 
pipeline, supports CSI2 bus, and has more drivers (the csi2dc is one of 
them ), some are not mainlined.
Future platforms , which are currently in prototyping, have a similar 
pipeline with sama7g5, some have more complicated pipelines, but they 
include the ISC and we plan to use the same driver.

>>
>> I'm trying to get a bit of a feel of the potential problems this change
>> will introduce.
>>
>>>
>>> But if even the vendor wants to move to MC to allow more use-cases I
>>> think we have to acknolege that if you're running mainline on an
>>> embedded system you could expect to adjust your setup between kernel
>>> updates. The idea to document the media-ctl commands required to setup
>>> the pipeline it's helpful, and might help in the interim period until
>>> the platform is not supported by libcamera.
>>
>> Well, I don't want Linus to start yelling at me for breaking userspace :-)
>>
>> We have broken userspace API (intentionally) in the past, but only with
>> good reasons. And sometimes a driver is used so rarely that it is not worth
>> the effort to try and keep compatible.
>>
>> As a developer I'd love to just forget about the old API, but as subsystem
>> maintainer I need good arguments.
> 
> I understand and I think these are all valid concerns. Finding a
> balance between new features and legacy is not easy.
> 
>>
>> Another option might be to take the TI cal approach, but have warnings that
>> it will be removed in, say, 2 years time. Or even make a copy of the driver
>> for the old platforms, and perhaps move that to staging to be removed eventually.
>>
>> The idea of a sudden breakage when going from kernel K to K+1 doesn't sit
>> well with me, if there was a transition period of 1-2 years then that would be
>> better.
>>
> 
> If staging works for you that's probably the easiest option. Let's see
> what Eugen prefers!

Hi Jacopo,

How does the staging solution work ? I do not fully understand the 
options here to make an educated choice

Thanks for helping out,

Eugen


> 
>> Regards,
>>
>>        Hans
>>
>>>
>>> That said, if Eugen wants to give the flag a try I won't
>>> oppose :)
>>>
>>>
>>>> Regards,
>>>>
>>>>     Hans
>>>>
>>>>>
>>>>>>
>>>>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219
>>>>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
>>>>>> media-ctl -d /dev/media0 --set-v4l2
>>>>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
>>>>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
>>>>>> media-ctl -d /dev/media0 --set-v4l2
>>>>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'
>>>>>>
>>>>>> Thank you for taking care of this !
>>>>>>
>>>>>> Eugen
>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>>            Hans
>>>>>>>
>>>>>>>>
>>>>>>>> Full series history:
>>>>>>>>
>>>>>>>> Changes in v10:
>>>>>>>> -> split the series into this first fixes part.
>>>>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes
>>>>>>>> -> edited commit messages
>>>>>>>> -> DT nodes now disabled by default.
>>>>>>>>
>>>>>>>> Changes in v9:
>>>>>>>> -> kernel robot reported isc_link_validate is not static, changed to static.
>>>>>>>>
>>>>>>>> Changes in v8:
>>>>>>>> -> scaler: modified crop bounds to have the exact source size
>>>>>>>>
>>>>>>>> Changes in v7:
>>>>>>>> -> scaler: modified crop bounds to have maximum isc size
>>>>>>>> -> format propagation: did small changes as per Jacopo review
>>>>>>>>
>>>>>>>>
>>>>>>>> Changes in v6:
>>>>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
>>>>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
>>>>>>>>
>>>>>>>> Changes in v5:
>>>>>>>> -> removed patch that removed the 'stop' variable as it was still required
>>>>>>>> -> added two new trivial patches
>>>>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
>>>>>>>>
>>>>>>>>
>>>>>>>> Changes in v4:
>>>>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
>>>>>>>> one patch that was using it
>>>>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
>>>>>>>>
>>>>>>>>
>>>>>>>> Changes in v3:
>>>>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
>>>>>>>> for the isc-base.
>>>>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
>>>>>>>>
>>>>>>>> Changes in v2:
>>>>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
>>>>>>>> - add a few new patches
>>>>>>>>
>>>>>>>> Eugen Hristev (5):
>>>>>>>>      media: atmel: atmel-isc: prepare for media controller support
>>>>>>>>      media: atmel: atmel-isc: implement media controller
>>>>>>>>      ARM: dts: at91: sama7g5: add nodes for video capture
>>>>>>>>      ARM: configs: at91: sama7: add xisc and csi2dc
>>>>>>>>      ARM: multi_v7_defconfig: add atmel video pipeline modules
>>>>>>>>
>>>>>>>>     arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>>>>>>>>     arch/arm/configs/multi_v7_defconfig           |   3 +
>>>>>>>>     arch/arm/configs/sama7_defconfig              |   2 +
>>>>>>>>     drivers/media/platform/atmel/Makefile         |   2 +-
>>>>>>>>     drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>>>>>>>>     .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>>>>>>>>     drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>>>>>>>>     .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>>>>>>>>     .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>>>>>>>>     9 files changed, 685 insertions(+), 241 deletions(-)
>>>>>>>>     create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Jacopo Mondi]

Hi Eugen,

On Thu, Jun 23, 2022 at 08:39:48AM +0000, Eugen.Hristev@microchip.com wrote:
> On 6/22/22 6:46 PM, Jacopo Mondi wrote:
> > Hi Hans,
> >
> > On Wed, Jun 22, 2022 at 04:55:27PM +0200, Hans Verkuil wrote:
> >> Hi Eugen, Jacopo,
> >>
> >> On 22/06/2022 16:14, Jacopo Mondi wrote:
> >>> Hi Hans, Eugen
> >>>
> >>> On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote:
> >>>> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote:
> >>>>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote:
> >>>>>> On 6/22/22 2:53 PM, Hans Verkuil wrote:
> >>>>>>> Hi Eugen,
> >>>>>>>
> >>>>>>> On 03/05/2022 11:51, Eugen Hristev wrote:
> >>>>>>>> This series is a split from the series :
> >>>>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
> >>>>>>>> and it includes the media controller part.
> >>>>>>>> previous fixes were sent on a different patch series.
> >>>>>>>>
> >>>>>>>> As discussed on the ML, moving forward with having the media link validate at
> >>>>>>>> start/stop streaming call.
> >>>>>>>> I will test the patch :
> >>>>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
> >>>>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
> >>>>>>>
> >>>>>>> I'm looking at merging this series, but I would like to have the output of
> >>>>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
> >>>>>>> correct.
> >>>>>>
> >>>>>> Hello Hans,
> >>>>>>
> >>>>>> Please have a look at attached file . Unless you want me to add the
> >>>>>> whole output to the e-mail ?
> >>>>>>
> >>>>>> I also added output of media-ctl -p for your convenience.
> >>>>>> the subdev2 is a device and driver that is not upstream and has some
> >>>>>> compliance issues, they are reported by the v4l2-compliance tool, but
> >>>>>> they should not affect this series, it's a synopsys driver that was
> >>>>>> rejected on mainline a few years ago, I took it for internal usage, but
> >>>>>> it's not cleaned up nor worked a lot upon.
> >>>>>>
> >>>>>>>
> >>>>>>> And one more question which may have been answered already in the past:
> >>>>>>>
> >>>>>>> Changing to the MC will break existing applications, doesn't it? Or did I
> >>>>>>> miss something?
> >>>>>>>
> >>>>>>
> >>>>>> The existing applications will have to configure the pipeline now. It
> >>>>>> will no longer work by configuring just the top video node /dev/video0 .
> >>>>>> They would have to use media-ctl for it, something similar with this set
> >>>>>> of commands:
> >>>>>
> >>>>> To add on top of that, actually, the reality is that without the MC
> >>>>> support in atmel-isc , some of our platforms do not work at all, because
> >>>>> the csi2dc driver which is in the middle of the pipeline, is a MC
> >>>>> driver. So it will not work without configuring it with MC anyway. It
> >>>>> used to work in a very preliminary version of the csi2dc driver which I
> >>>>> sent a few years ago, but that way of handling things was rejected.
> >>>>> Hence I changed the csi2dc to being full-MC driver (requested for new
> >>>>> drivers) and now I am completing the conversion for the whole pipeline.
> >>>>> We are using this MC-centric approach in production for our products to
> >>>>> be as close as possible to mainline, and backported it to our 5.15
> >>>>> internal releases, which people are using right now.
> >>>>
> >>>> I'm not all that keen on breaking userspace for those who do NOT use the
> >>>> Atmel BSP. Basically some platforms are currently broken, and with this patch
> >>>> series some other platforms are broken, but at least can be fixed by changing
> >>>> userspace.
> >>>>
> >>>> How feasible is it to do something similar that TI did for the cal driver?
> >>>> (drivers/media/platform/ti/cal)
> >>>>
> >>>> I.e., based on a module option the MC is enabled or disabled. And if a
> >>>> csi2dc is present, then the MC API is always enabled.
> >>>>
> >>>
> >>> I think I have suggested Eugen to move to MC when he
> >>> started looking in libcamera, so sorry for the intrusion but I feel
> >>> a bit bad for not rising the point earlier and get him to v10
> >>>
> >>> I understand your point Hans, and when a vendor upstreaming code or a
> >>> user requires to maintain compatibility, the burden of keeping more
> >>> code in to handle the MC and non-MC cases is worth the complications.
> >>
> >> Eugen, can you provide a list of platforms that will break with this
> >> change and which platforms are currently broken without this series?
>
> Hi Hans,
>
> Basically the sama5d2 platform (we have several versions of the chip :
> sama5d21, sama5d27, sama5d29, in various packages, SIP, SoMs, on many
> different boards ) would be broken. this is the old platform.
> It would be broken if the sensor default format is a mismatch with the
> default format of the ISC .  Basically the old code currently is
> propagating all the frame information down to the sensor, thing that no
> longer happens with this patch series.
>
> The platform that needs MC is mainly sama7g5 , which has a longer
> pipeline, supports CSI2 bus, and has more drivers (the csi2dc is one of
> them ), some are not mainlined.
> Future platforms , which are currently in prototyping, have a similar
> pipeline with sama7g5, some have more complicated pipelines, but they
> include the ISC and we plan to use the same driver.
>
> >>
> >> I'm trying to get a bit of a feel of the potential problems this change
> >> will introduce.
> >>
> >>>
> >>> But if even the vendor wants to move to MC to allow more use-cases I
> >>> think we have to acknolege that if you're running mainline on an
> >>> embedded system you could expect to adjust your setup between kernel
> >>> updates. The idea to document the media-ctl commands required to setup
> >>> the pipeline it's helpful, and might help in the interim period until
> >>> the platform is not supported by libcamera.
> >>
> >> Well, I don't want Linus to start yelling at me for breaking userspace :-)
> >>
> >> We have broken userspace API (intentionally) in the past, but only with
> >> good reasons. And sometimes a driver is used so rarely that it is not worth
> >> the effort to try and keep compatible.
> >>
> >> As a developer I'd love to just forget about the old API, but as subsystem
> >> maintainer I need good arguments.
> >
> > I understand and I think these are all valid concerns. Finding a
> > balance between new features and legacy is not easy.
> >
> >>
> >> Another option might be to take the TI cal approach, but have warnings that
> >> it will be removed in, say, 2 years time. Or even make a copy of the driver
> >> for the old platforms, and perhaps move that to staging to be removed eventually.
> >>
> >> The idea of a sudden breakage when going from kernel K to K+1 doesn't sit
> >> well with me, if there was a transition period of 1-2 years then that would be
> >> better.
> >>
> >
> > If staging works for you that's probably the easiest option. Let's see
> > what Eugen prefers!
>
> Hi Jacopo,
>
> How does the staging solution work ? I do not fully understand the
> options here to make an educated choice

Hans should probably tell, but my interepetation would be to move the
existing driver (before this series) to drivers/staging/ and advance
the existing one in drivers/media/ to MC support.

Users of the old driver interface could keep using the one in (de)staging
for a little longer.

Would changing the driver KConfig symbol name help making the change
more evident maybe ? Users that upgrade to a new kernel will be
notified about the new symbol instead of being silently moved to the new
interface.

Thanks
  j

>
> Thanks for helping out,
>
> Eugen
>
>
> >
> >> Regards,
> >>
> >>        Hans
> >>
> >>>
> >>> That said, if Eugen wants to give the flag a try I won't
> >>> oppose :)
> >>>
> >>>
> >>>> Regards,
> >>>>
> >>>>     Hans
> >>>>
> >>>>>
> >>>>>>
> >>>>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219
> >>>>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
> >>>>>> media-ctl -d /dev/media0 --set-v4l2
> >>>>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
> >>>>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
> >>>>>> media-ctl -d /dev/media0 --set-v4l2
> >>>>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'
> >>>>>>
> >>>>>> Thank you for taking care of this !
> >>>>>>
> >>>>>> Eugen
> >>>>>>
> >>>>>>> Regards,
> >>>>>>>
> >>>>>>>            Hans
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Full series history:
> >>>>>>>>
> >>>>>>>> Changes in v10:
> >>>>>>>> -> split the series into this first fixes part.
> >>>>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes
> >>>>>>>> -> edited commit messages
> >>>>>>>> -> DT nodes now disabled by default.
> >>>>>>>>
> >>>>>>>> Changes in v9:
> >>>>>>>> -> kernel robot reported isc_link_validate is not static, changed to static.
> >>>>>>>>
> >>>>>>>> Changes in v8:
> >>>>>>>> -> scaler: modified crop bounds to have the exact source size
> >>>>>>>>
> >>>>>>>> Changes in v7:
> >>>>>>>> -> scaler: modified crop bounds to have maximum isc size
> >>>>>>>> -> format propagation: did small changes as per Jacopo review
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Changes in v6:
> >>>>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
> >>>>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
> >>>>>>>>
> >>>>>>>> Changes in v5:
> >>>>>>>> -> removed patch that removed the 'stop' variable as it was still required
> >>>>>>>> -> added two new trivial patches
> >>>>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Changes in v4:
> >>>>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
> >>>>>>>> one patch that was using it
> >>>>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Changes in v3:
> >>>>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
> >>>>>>>> for the isc-base.
> >>>>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
> >>>>>>>>
> >>>>>>>> Changes in v2:
> >>>>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
> >>>>>>>> - add a few new patches
> >>>>>>>>
> >>>>>>>> Eugen Hristev (5):
> >>>>>>>>      media: atmel: atmel-isc: prepare for media controller support
> >>>>>>>>      media: atmel: atmel-isc: implement media controller
> >>>>>>>>      ARM: dts: at91: sama7g5: add nodes for video capture
> >>>>>>>>      ARM: configs: at91: sama7: add xisc and csi2dc
> >>>>>>>>      ARM: multi_v7_defconfig: add atmel video pipeline modules
> >>>>>>>>
> >>>>>>>>     arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
> >>>>>>>>     arch/arm/configs/multi_v7_defconfig           |   3 +
> >>>>>>>>     arch/arm/configs/sama7_defconfig              |   2 +
> >>>>>>>>     drivers/media/platform/atmel/Makefile         |   2 +-
> >>>>>>>>     drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
> >>>>>>>>     .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
> >>>>>>>>     drivers/media/platform/atmel/atmel-isc.h      |  50 +-
> >>>>>>>>     .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
> >>>>>>>>     .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
> >>>>>>>>     9 files changed, 685 insertions(+), 241 deletions(-)
> >>>>>>>>     create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
>

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Hans Verkuil]

On 23/06/2022 11:19, Jacopo Mondi wrote:
> Hi Eugen,
> 
> On Thu, Jun 23, 2022 at 08:39:48AM +0000, Eugen.Hristev@microchip.com wrote:
>> On 6/22/22 6:46 PM, Jacopo Mondi wrote:
>>> Hi Hans,
>>>
>>> On Wed, Jun 22, 2022 at 04:55:27PM +0200, Hans Verkuil wrote:
>>>> Hi Eugen, Jacopo,
>>>>
>>>> On 22/06/2022 16:14, Jacopo Mondi wrote:
>>>>> Hi Hans, Eugen
>>>>>
>>>>> On Wed, Jun 22, 2022 at 03:47:33PM +0200, Hans Verkuil wrote:
>>>>>> On 22/06/2022 14:42, Eugen.Hristev@microchip.com wrote:
>>>>>>> On 6/22/22 3:25 PM, Eugen Hristev - M18282 wrote:
>>>>>>>> On 6/22/22 2:53 PM, Hans Verkuil wrote:
>>>>>>>>> Hi Eugen,
>>>>>>>>>
>>>>>>>>> On 03/05/2022 11:51, Eugen Hristev wrote:
>>>>>>>>>> This series is a split from the series :
>>>>>>>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
>>>>>>>>>> and it includes the media controller part.
>>>>>>>>>> previous fixes were sent on a different patch series.
>>>>>>>>>>
>>>>>>>>>> As discussed on the ML, moving forward with having the media link validate at
>>>>>>>>>> start/stop streaming call.
>>>>>>>>>> I will test the patch :
>>>>>>>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
>>>>>>>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
>>>>>>>>>
>>>>>>>>> I'm looking at merging this series, but I would like to have the output of
>>>>>>>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
>>>>>>>>> correct.
>>>>>>>>
>>>>>>>> Hello Hans,
>>>>>>>>
>>>>>>>> Please have a look at attached file . Unless you want me to add the
>>>>>>>> whole output to the e-mail ?
>>>>>>>>
>>>>>>>> I also added output of media-ctl -p for your convenience.
>>>>>>>> the subdev2 is a device and driver that is not upstream and has some
>>>>>>>> compliance issues, they are reported by the v4l2-compliance tool, but
>>>>>>>> they should not affect this series, it's a synopsys driver that was
>>>>>>>> rejected on mainline a few years ago, I took it for internal usage, but
>>>>>>>> it's not cleaned up nor worked a lot upon.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> And one more question which may have been answered already in the past:
>>>>>>>>>
>>>>>>>>> Changing to the MC will break existing applications, doesn't it? Or did I
>>>>>>>>> miss something?
>>>>>>>>>
>>>>>>>>
>>>>>>>> The existing applications will have to configure the pipeline now. It
>>>>>>>> will no longer work by configuring just the top video node /dev/video0 .
>>>>>>>> They would have to use media-ctl for it, something similar with this set
>>>>>>>> of commands:
>>>>>>>
>>>>>>> To add on top of that, actually, the reality is that without the MC
>>>>>>> support in atmel-isc , some of our platforms do not work at all, because
>>>>>>> the csi2dc driver which is in the middle of the pipeline, is a MC
>>>>>>> driver. So it will not work without configuring it with MC anyway. It
>>>>>>> used to work in a very preliminary version of the csi2dc driver which I
>>>>>>> sent a few years ago, but that way of handling things was rejected.
>>>>>>> Hence I changed the csi2dc to being full-MC driver (requested for new
>>>>>>> drivers) and now I am completing the conversion for the whole pipeline.
>>>>>>> We are using this MC-centric approach in production for our products to
>>>>>>> be as close as possible to mainline, and backported it to our 5.15
>>>>>>> internal releases, which people are using right now.
>>>>>>
>>>>>> I'm not all that keen on breaking userspace for those who do NOT use the
>>>>>> Atmel BSP. Basically some platforms are currently broken, and with this patch
>>>>>> series some other platforms are broken, but at least can be fixed by changing
>>>>>> userspace.
>>>>>>
>>>>>> How feasible is it to do something similar that TI did for the cal driver?
>>>>>> (drivers/media/platform/ti/cal)
>>>>>>
>>>>>> I.e., based on a module option the MC is enabled or disabled. And if a
>>>>>> csi2dc is present, then the MC API is always enabled.
>>>>>>
>>>>>
>>>>> I think I have suggested Eugen to move to MC when he
>>>>> started looking in libcamera, so sorry for the intrusion but I feel
>>>>> a bit bad for not rising the point earlier and get him to v10
>>>>>
>>>>> I understand your point Hans, and when a vendor upstreaming code or a
>>>>> user requires to maintain compatibility, the burden of keeping more
>>>>> code in to handle the MC and non-MC cases is worth the complications.
>>>>
>>>> Eugen, can you provide a list of platforms that will break with this
>>>> change and which platforms are currently broken without this series?
>>
>> Hi Hans,
>>
>> Basically the sama5d2 platform (we have several versions of the chip :
>> sama5d21, sama5d27, sama5d29, in various packages, SIP, SoMs, on many
>> different boards ) would be broken. this is the old platform.
>> It would be broken if the sensor default format is a mismatch with the
>> default format of the ISC .  Basically the old code currently is
>> propagating all the frame information down to the sensor, thing that no
>> longer happens with this patch series.
>>
>> The platform that needs MC is mainly sama7g5 , which has a longer
>> pipeline, supports CSI2 bus, and has more drivers (the csi2dc is one of
>> them ), some are not mainlined.
>> Future platforms , which are currently in prototyping, have a similar
>> pipeline with sama7g5, some have more complicated pipelines, but they
>> include the ISC and we plan to use the same driver.
>>
>>>>
>>>> I'm trying to get a bit of a feel of the potential problems this change
>>>> will introduce.
>>>>
>>>>>
>>>>> But if even the vendor wants to move to MC to allow more use-cases I
>>>>> think we have to acknolege that if you're running mainline on an
>>>>> embedded system you could expect to adjust your setup between kernel
>>>>> updates. The idea to document the media-ctl commands required to setup
>>>>> the pipeline it's helpful, and might help in the interim period until
>>>>> the platform is not supported by libcamera.
>>>>
>>>> Well, I don't want Linus to start yelling at me for breaking userspace :-)
>>>>
>>>> We have broken userspace API (intentionally) in the past, but only with
>>>> good reasons. And sometimes a driver is used so rarely that it is not worth
>>>> the effort to try and keep compatible.
>>>>
>>>> As a developer I'd love to just forget about the old API, but as subsystem
>>>> maintainer I need good arguments.
>>>
>>> I understand and I think these are all valid concerns. Finding a
>>> balance between new features and legacy is not easy.
>>>
>>>>
>>>> Another option might be to take the TI cal approach, but have warnings that
>>>> it will be removed in, say, 2 years time. Or even make a copy of the driver
>>>> for the old platforms, and perhaps move that to staging to be removed eventually.
>>>>
>>>> The idea of a sudden breakage when going from kernel K to K+1 doesn't sit
>>>> well with me, if there was a transition period of 1-2 years then that would be
>>>> better.
>>>>
>>>
>>> If staging works for you that's probably the easiest option. Let's see
>>> what Eugen prefers!
>>
>> Hi Jacopo,
>>
>> How does the staging solution work ? I do not fully understand the
>> options here to make an educated choice
> 
> Hans should probably tell, but my interepetation would be to move the
> existing driver (before this series) to drivers/staging/ and advance
> the existing one in drivers/media/ to MC support.

Right. And strip the support for the newer platforms from the staging driver.
So it is just for sama5d2.

> 
> Users of the old driver interface could keep using the one in (de)staging
> for a little longer.
> 
> Would changing the driver KConfig symbol name help making the change
> more evident maybe ? Users that upgrade to a new kernel will be
> notified about the new symbol instead of being silently moved to the new
> interface.

I'm inclined to change the Kconfig symbol for both old and new drivers
if we decide to go in this direction: in both cases you need to be aware
that there are major changes: the new uses the MC API, the old is marked
deprecated and users should be aware that it will be removed eventually
and they should work to switch to the 'new' driver.

Regards,

	Hans

> 
> Thanks
>   j
> 
>>
>> Thanks for helping out,
>>
>> Eugen
>>
>>
>>>
>>>> Regards,
>>>>
>>>>        Hans
>>>>
>>>>>
>>>>> That said, if Eugen wants to give the flag a try I won't
>>>>> oppose :)
>>>>>
>>>>>
>>>>>> Regards,
>>>>>>
>>>>>>     Hans
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> media-ctl -d /dev/media0 --set-v4l2 '"imx219
>>>>>>>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
>>>>>>>> media-ctl -d /dev/media0 --set-v4l2
>>>>>>>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
>>>>>>>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
>>>>>>>> media-ctl -d /dev/media0 --set-v4l2
>>>>>>>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'
>>>>>>>>
>>>>>>>> Thank you for taking care of this !
>>>>>>>>
>>>>>>>> Eugen
>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>>
>>>>>>>>>            Hans
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Full series history:
>>>>>>>>>>
>>>>>>>>>> Changes in v10:
>>>>>>>>>> -> split the series into this first fixes part.
>>>>>>>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes
>>>>>>>>>> -> edited commit messages
>>>>>>>>>> -> DT nodes now disabled by default.
>>>>>>>>>>
>>>>>>>>>> Changes in v9:
>>>>>>>>>> -> kernel robot reported isc_link_validate is not static, changed to static.
>>>>>>>>>>
>>>>>>>>>> Changes in v8:
>>>>>>>>>> -> scaler: modified crop bounds to have the exact source size
>>>>>>>>>>
>>>>>>>>>> Changes in v7:
>>>>>>>>>> -> scaler: modified crop bounds to have maximum isc size
>>>>>>>>>> -> format propagation: did small changes as per Jacopo review
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Changes in v6:
>>>>>>>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
>>>>>>>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
>>>>>>>>>>
>>>>>>>>>> Changes in v5:
>>>>>>>>>> -> removed patch that removed the 'stop' variable as it was still required
>>>>>>>>>> -> added two new trivial patches
>>>>>>>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Changes in v4:
>>>>>>>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
>>>>>>>>>> one patch that was using it
>>>>>>>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Changes in v3:
>>>>>>>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
>>>>>>>>>> for the isc-base.
>>>>>>>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
>>>>>>>>>>
>>>>>>>>>> Changes in v2:
>>>>>>>>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
>>>>>>>>>> - add a few new patches
>>>>>>>>>>
>>>>>>>>>> Eugen Hristev (5):
>>>>>>>>>>      media: atmel: atmel-isc: prepare for media controller support
>>>>>>>>>>      media: atmel: atmel-isc: implement media controller
>>>>>>>>>>      ARM: dts: at91: sama7g5: add nodes for video capture
>>>>>>>>>>      ARM: configs: at91: sama7: add xisc and csi2dc
>>>>>>>>>>      ARM: multi_v7_defconfig: add atmel video pipeline modules
>>>>>>>>>>
>>>>>>>>>>     arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>>>>>>>>>>     arch/arm/configs/multi_v7_defconfig           |   3 +
>>>>>>>>>>     arch/arm/configs/sama7_defconfig              |   2 +
>>>>>>>>>>     drivers/media/platform/atmel/Makefile         |   2 +-
>>>>>>>>>>     drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>>>>>>>>>>     .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>>>>>>>>>>     drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>>>>>>>>>>     .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>>>>>>>>>>     .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>>>>>>>>>>     9 files changed, 685 insertions(+), 241 deletions(-)
>>>>>>>>>>     create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Hans Verkuil]

Hi Eugen,

On 22/06/2022 14:25, Eugen.Hristev@microchip.com wrote:
> On 6/22/22 2:53 PM, Hans Verkuil wrote:
>> Hi Eugen,
>>
>> On 03/05/2022 11:51, Eugen Hristev wrote:
>>> This series is a split from the series :
>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
>>> and it includes the media controller part.
>>> previous fixes were sent on a different patch series.
>>>
>>> As discussed on the ML, moving forward with having the media link validate at
>>> start/stop streaming call.
>>> I will test the patch :
>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
>>
>> I'm looking at merging this series, but I would like to have the output of
>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
>> correct.
> 
> Hello Hans,
> 
> Please have a look at attached file . Unless you want me to add the 
> whole output to the e-mail ?

No, this is fine, thank you!

> 
> I also added output of media-ctl -p for your convenience.
> the subdev2 is a device and driver that is not upstream and has some 
> compliance issues, they are reported by the v4l2-compliance tool, but 
> they should not affect this series, it's a synopsys driver that was 
> rejected on mainline a few years ago, I took it for internal usage, but 
> it's not cleaned up nor worked a lot upon.

OK, good to know.

From the compliance output:

	v4l2-compliance 1.22.1, 32 bits, 32-bit time_t

This is an old v4l2-compliance version. Compile it directly from the
v4l-utils git repo and check the output again.

	Compliance test for atmel_isc_commo device /dev/media0:

As you can see, the driver name is cut off. Isn't 'atmel-isc'
a better name?

> 
>>
>> And one more question which may have been answered already in the past:
>>
>> Changing to the MC will break existing applications, doesn't it? Or did I
>> miss something?
>>
> 
> The existing applications will have to configure the pipeline now. It 
> will no longer work by configuring just the top video node /dev/video0 .
> They would have to use media-ctl for it, something similar with this set 
> of commands:
> 
> media-ctl -d /dev/media0 --set-v4l2 '"imx219 
> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
> media-ctl -d /dev/media0 --set-v4l2 
> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
> media-ctl -d /dev/media0 --set-v4l2 
> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'

I'd like to see this documented in a new
Documentation/admin-guide/media/atmel-isc.rst file. That can be a new patch.

Regards,

	Hans

> 
> Thank you for taking care of this !
> 
> Eugen
> 
>> Regards,
>>
>>          Hans
>>
>>>
>>> Full series history:
>>>
>>> Changes in v10:
>>> -> split the series into this first fixes part.
>>> -> moved IO_MC addition from first patch to the second patch on the driver changes
>>> -> edited commit messages
>>> -> DT nodes now disabled by default.
>>>
>>> Changes in v9:
>>> -> kernel robot reported isc_link_validate is not static, changed to static.
>>>
>>> Changes in v8:
>>> -> scaler: modified crop bounds to have the exact source size
>>>
>>> Changes in v7:
>>> -> scaler: modified crop bounds to have maximum isc size
>>> -> format propagation: did small changes as per Jacopo review
>>>
>>>
>>> Changes in v6:
>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
>>>
>>> Changes in v5:
>>> -> removed patch that removed the 'stop' variable as it was still required
>>> -> added two new trivial patches
>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
>>>
>>>
>>> Changes in v4:
>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
>>> one patch that was using it
>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
>>>
>>>
>>> Changes in v3:
>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
>>> for the isc-base.
>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
>>>
>>> Changes in v2:
>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
>>> - add a few new patches
>>>
>>> Eugen Hristev (5):
>>>    media: atmel: atmel-isc: prepare for media controller support
>>>    media: atmel: atmel-isc: implement media controller
>>>    ARM: dts: at91: sama7g5: add nodes for video capture
>>>    ARM: configs: at91: sama7: add xisc and csi2dc
>>>    ARM: multi_v7_defconfig: add atmel video pipeline modules
>>>
>>>   arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>>>   arch/arm/configs/multi_v7_defconfig           |   3 +
>>>   arch/arm/configs/sama7_defconfig              |   2 +
>>>   drivers/media/platform/atmel/Makefile         |   2 +-
>>>   drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>>>   .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>>>   drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>>>   .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>>>   .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>>>   9 files changed, 685 insertions(+), 241 deletions(-)
>>>   create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
>>>
>>
> 

Re: [PATCH v10 0/5] media: atmel: atmel-isc: implement media controller

[Eugen.Hristev]

On 6/22/22 4:35 PM, Hans Verkuil wrote:
> Hi Eugen,
> 
> On 22/06/2022 14:25, Eugen.Hristev@microchip.com wrote:
>> On 6/22/22 2:53 PM, Hans Verkuil wrote:
>>> Hi Eugen,
>>>
>>> On 03/05/2022 11:51, Eugen Hristev wrote:
>>>> This series is a split from the series :
>>>> [PATCH v9 00/13] media: atmel: atmel-isc: implement media controller
>>>> and it includes the media controller part.
>>>> previous fixes were sent on a different patch series.
>>>>
>>>> As discussed on the ML, moving forward with having the media link validate at
>>>> start/stop streaming call.
>>>> I will test the patch :
>>>> [RFC PATCHv2] vb2: add support for (un)prepare_streaming queue ops
>>>> afterwards, but that patch requires moving my logic to the new vb2 callbacks.
>>>
>>> I'm looking at merging this series, but I would like to have the output of
>>> 'v4l2-compliance -m /dev/mediaX' to verify that the MC links etc. is all
>>> correct.
>>
>> Hello Hans,
>>
>> Please have a look at attached file . Unless you want me to add the
>> whole output to the e-mail ?
> 
> No, this is fine, thank you!
> 
>>
>> I also added output of media-ctl -p for your convenience.
>> the subdev2 is a device and driver that is not upstream and has some
>> compliance issues, they are reported by the v4l2-compliance tool, but
>> they should not affect this series, it's a synopsys driver that was
>> rejected on mainline a few years ago, I took it for internal usage, but
>> it's not cleaned up nor worked a lot upon.
> 
> OK, good to know.
> 
>  From the compliance output:
> 
>          v4l2-compliance 1.22.1, 32 bits, 32-bit time_t
> 
> This is an old v4l2-compliance version. Compile it directly from the
> v4l-utils git repo and check the output again.

Okay, I will start a build. I have to get my full environment ready, 
fire up a Buildroot. It will be ready tomorrow.

> 
>          Compliance test for atmel_isc_commo device /dev/media0:
> 
> As you can see, the driver name is cut off. Isn't 'atmel-isc'
> a better name?

Maybe, but this name is the name of the entity, and is taken from the 
name of the kernel module. Since the split of the isc driver into 
atmel_isc_common and atmel-sama5d2-isc , atmel-sama7g5-isc , this name 
has been set for the module here:

https://git.linuxtv.org/hverkuil/media_tree.git/tree/drivers/media/platform/atmel/Makefile?h=for-v5.20e#n7

And the ISC driver takes the module name and uses it for video name here:

https://git.linuxtv.org/hverkuil/media_tree.git/tree/drivers/media/platform/atmel/atmel-isc-base.c?h=for-v5.20e#n1999



> 
>>
>>>
>>> And one more question which may have been answered already in the past:
>>>
>>> Changing to the MC will break existing applications, doesn't it? Or did I
>>> miss something?
>>>
>>
>> The existing applications will have to configure the pipeline now. It
>> will no longer work by configuring just the top video node /dev/video0 .
>> They would have to use media-ctl for it, something similar with this set
>> of commands:
>>
>> media-ctl -d /dev/media0 --set-v4l2 '"imx219
>> 1-0010":0[fmt:SRGGB10_1X10/1920x1080]'
>> media-ctl -d /dev/media0 --set-v4l2
>> '"dw-csi.0":0[fmt:SRGGB10_1X10/1920x1080]'
>> media-ctl -d /dev/media0 --set-v4l2 '"csi2dc":0[fmt:SRGGB10_1X10/1920x1080]'
>> media-ctl -d /dev/media0 --set-v4l2
>> '"atmel_isc_scaler":0[fmt:SRGGB10_1X10/1920x1080]'
> 
> I'd like to see this documented in a new
> Documentation/admin-guide/media/atmel-isc.rst file. That can be a new patch.

Allright, I can do that.

> 
> Regards,
> 
>          Hans
> 
>>
>> Thank you for taking care of this !
>>
>> Eugen
>>
>>> Regards,
>>>
>>>           Hans
>>>
>>>>
>>>> Full series history:
>>>>
>>>> Changes in v10:
>>>> -> split the series into this first fixes part.
>>>> -> moved IO_MC addition from first patch to the second patch on the driver changes
>>>> -> edited commit messages
>>>> -> DT nodes now disabled by default.
>>>>
>>>> Changes in v9:
>>>> -> kernel robot reported isc_link_validate is not static, changed to static.
>>>>
>>>> Changes in v8:
>>>> -> scaler: modified crop bounds to have the exact source size
>>>>
>>>> Changes in v7:
>>>> -> scaler: modified crop bounds to have maximum isc size
>>>> -> format propagation: did small changes as per Jacopo review
>>>>
>>>>
>>>> Changes in v6:
>>>> -> worked a bit on scaler, added try crop and other changes as per Jacopo review
>>>> -> worked on isc-base enum_fmt , reworked as per Jacopo review
>>>>
>>>> Changes in v5:
>>>> -> removed patch that removed the 'stop' variable as it was still required
>>>> -> added two new trivial patches
>>>> -> reworked some parts of the scaler and format propagation after discussions with Jacopo
>>>>
>>>>
>>>> Changes in v4:
>>>> -> as reviewed by Hans, added new patch to remove the 'stop' variable and reworked
>>>> one patch that was using it
>>>> -> as reviewed by Jacopo, reworked some parts of the media controller implementation
>>>>
>>>>
>>>> Changes in v3:
>>>> - change in bindings, small fixes in csi2dc driver and conversion to mc
>>>> for the isc-base.
>>>> - removed some MAINTAINERS patches and used patterns in MAINTAINERS
>>>>
>>>> Changes in v2:
>>>> - integrated many changes suggested by Jacopo in the review of the v1 series.
>>>> - add a few new patches
>>>>
>>>> Eugen Hristev (5):
>>>>     media: atmel: atmel-isc: prepare for media controller support
>>>>     media: atmel: atmel-isc: implement media controller
>>>>     ARM: dts: at91: sama7g5: add nodes for video capture
>>>>     ARM: configs: at91: sama7: add xisc and csi2dc
>>>>     ARM: multi_v7_defconfig: add atmel video pipeline modules
>>>>
>>>>    arch/arm/boot/dts/sama7g5.dtsi                |  51 ++
>>>>    arch/arm/configs/multi_v7_defconfig           |   3 +
>>>>    arch/arm/configs/sama7_defconfig              |   2 +
>>>>    drivers/media/platform/atmel/Makefile         |   2 +-
>>>>    drivers/media/platform/atmel/atmel-isc-base.c | 485 +++++++++---------
>>>>    .../media/platform/atmel/atmel-isc-scaler.c   | 267 ++++++++++
>>>>    drivers/media/platform/atmel/atmel-isc.h      |  50 +-
>>>>    .../media/platform/atmel/atmel-sama5d2-isc.c  |  34 +-
>>>>    .../media/platform/atmel/atmel-sama7g5-isc.c  |  32 +-
>>>>    9 files changed, 685 insertions(+), 241 deletions(-)
>>>>    create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
>>>>
>>>
>>
>