* [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller
@ 2022-02-17 13:56 Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 01/13] media: atmel: atmel-isc-base: use streaming status when queueing buffers Eugen Hristev
` (12 more replies)
0 siblings, 13 replies; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
Eugen Hristev
This series is the v5 series that attempts to support media controller in the
atmel ISC and XISC drivers.
The CSI2DC driver was accepted thus removed from the patch series, together with
other patches.
Important note: this series applies on top of current media_staging tree, as it
relies on previous patches in the series which were accepted.
Thanks to everyone who reviewed my work !
Eugen
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 (13):
media: atmel: atmel-isc-base: use streaming status when queueing
buffers
media: atmel: atmel-isc-base: replace is_streaming call in
s_fmt_vid_cap
media: atmel: atmel-isc: remove redundant comments
media: atmel: atmel-isc: implement media controller
media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check
media: atmel: atmel-isc-base: use mutex to lock awb workqueue from
streaming
media: atmel: atmel-isc: compact the controller formats list
media: atmel: atmel-isc: change format propagation to subdev into only
verification
media: atmel: atmel-sama7g5-isc: remove stray line
dt-bindings: media: microchip,xisc: add bus-width of 14
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
.../bindings/media/microchip,xisc.yaml | 2 +-
arch/arm/boot/dts/sama7g5.dtsi | 49 ++
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 | 531 ++++++++++--------
.../media/platform/atmel/atmel-isc-scaler.c | 266 +++++++++
drivers/media/platform/atmel/atmel-isc.h | 61 +-
.../media/platform/atmel/atmel-sama5d2-isc.c | 87 +--
.../media/platform/atmel/atmel-sama7g5-isc.c | 93 +--
10 files changed, 762 insertions(+), 334 deletions(-)
create mode 100644 drivers/media/platform/atmel/atmel-isc-scaler.c
--
2.25.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 01/13] media: atmel: atmel-isc-base: use streaming status when queueing buffers
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
2022-02-23 17:05 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 02/13] media: atmel: atmel-isc-base: replace is_streaming call in s_fmt_vid_cap Eugen Hristev
` (11 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
Eugen Hristev
During experiments with libcamera, it looks like vb2_is_streaming returns
true before our start streaming is called.
Order of operations is streamon -> queue -> start_streaming
ISC would have started the DMA immediately when a buffer is being added
to the vbqueue if the queue is streaming.
It is more safe to start the DMA after the start streaming of the driver is
called.
Thus, even if vb2queue is streaming, add the buffer to the dma queue of the
driver instead of actually starting the DMA process, if the start streaming
has not been called yet.
Tho achieve this, we have to use vb2_start_streaming_called instead of
vb2_is_streaming.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/media/platform/atmel/atmel-isc-base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
index db15770d5b88..d2cc6c99984f 100644
--- a/drivers/media/platform/atmel/atmel-isc-base.c
+++ b/drivers/media/platform/atmel/atmel-isc-base.c
@@ -442,7 +442,7 @@ static void isc_buffer_queue(struct vb2_buffer *vb)
spin_lock_irqsave(&isc->dma_queue_lock, flags);
if (!isc->cur_frm && list_empty(&isc->dma_queue) &&
- vb2_is_streaming(vb->vb2_queue)) {
+ vb2_start_streaming_called(vb->vb2_queue)) {
isc->cur_frm = buf;
isc_start_dma(isc);
} else
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 02/13] media: atmel: atmel-isc-base: replace is_streaming call in s_fmt_vid_cap
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 01/13] media: atmel: atmel-isc-base: use streaming status when queueing buffers Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
2022-02-23 17:05 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 03/13] media: atmel: atmel-isc: remove redundant comments Eugen Hristev
` (10 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
Eugen Hristev, Hans Verkuil
In s_fmt_vid_cap, we should check if vb2_is_busy and return EBUSY,
not check if it's streaming to return the busy state.
Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/media/platform/atmel/atmel-isc-base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
index d2cc6c99984f..67b4a2323fed 100644
--- a/drivers/media/platform/atmel/atmel-isc-base.c
+++ b/drivers/media/platform/atmel/atmel-isc-base.c
@@ -1029,7 +1029,7 @@ static int isc_s_fmt_vid_cap(struct file *file, void *priv,
{
struct isc_device *isc = video_drvdata(file);
- if (vb2_is_streaming(&isc->vb2_vidq))
+ if (vb2_is_busy(&isc->vb2_vidq))
return -EBUSY;
return isc_set_fmt(isc, f);
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 03/13] media: atmel: atmel-isc: remove redundant comments
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 01/13] media: atmel: atmel-isc-base: use streaming status when queueing buffers Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 02/13] media: atmel: atmel-isc-base: replace is_streaming call in s_fmt_vid_cap Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
2022-02-23 17:06 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 04/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
` (9 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
Eugen Hristev
Remove duplicate comments which are already in place before the struct
definition.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/media/platform/atmel/atmel-isc.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
index 07fa6dbf8460..f9ad7ec6bd13 100644
--- a/drivers/media/platform/atmel/atmel-isc.h
+++ b/drivers/media/platform/atmel/atmel-isc.h
@@ -272,7 +272,7 @@ struct isc_device {
struct video_device video_dev;
struct vb2_queue vb2_vidq;
- spinlock_t dma_queue_lock; /* serialize access to dma queue */
+ spinlock_t dma_queue_lock;
struct list_head dma_queue;
struct isc_buffer *cur_frm;
unsigned int sequence;
@@ -289,8 +289,8 @@ struct isc_device {
struct isc_ctrls ctrls;
struct work_struct awb_work;
- struct mutex lock; /* serialize access to file operations */
- spinlock_t awb_lock; /* serialize access to DMA buffers from awb work queue */
+ struct mutex lock;
+ spinlock_t awb_lock;
struct regmap_field *pipeline[ISC_PIPE_LINE_NODE_NUM];
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 04/13] media: atmel: atmel-isc: implement media controller
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
` (2 preceding siblings ...)
2022-02-17 13:56 ` [PATCH v5 03/13] media: atmel: atmel-isc: remove redundant comments Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
2022-02-17 14:59 ` Eugen.Hristev
2022-02-23 16:32 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 05/13] media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check Eugen Hristev
` (8 subsequent siblings)
12 siblings, 2 replies; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
Eugen Hristev
Implement the support for media-controller.
This means that the capabilities of the driver have changed and now
it also advertises the IO_MC .
The driver will register it's 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.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
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 | 73 ++++-
.../media/platform/atmel/atmel-isc-scaler.c | 261 ++++++++++++++++++
drivers/media/platform/atmel/atmel-isc.h | 40 +++
.../media/platform/atmel/atmel-sama5d2-isc.c | 14 +-
.../media/platform/atmel/atmel-sama7g5-isc.c | 12 +-
6 files changed, 394 insertions(+), 8 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 67b4a2323fed..74f575544e09 100644
--- a/drivers/media/platform/atmel/atmel-isc-base.c
+++ b/drivers/media/platform/atmel/atmel-isc-base.c
@@ -1712,6 +1712,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");
@@ -1720,6 +1721,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;
}
@@ -1734,8 +1745,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;
@@ -1751,6 +1762,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)
{
@@ -1767,7 +1779,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);
@@ -1893,7 +1905,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);
@@ -1903,8 +1916,18 @@ 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->lock);
return ret;
@@ -1971,6 +1994,48 @@ 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);
+}
+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..0c1f49db47b4
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc-scaler.c
@@ -0,0 +1,261 @@
+// 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 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;
+ }
+
+ if (format->pad == ISC_SCALER_PAD_SOURCE)
+ format->format = isc->scaler_format_source;
+ else
+ format->format = isc->scaler_format_sink;
+
+ 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_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);
+
+ req_fmt->format.colorspace = V4L2_COLORSPACE_SRGB;
+ req_fmt->format.field = V4L2_FIELD_NONE;
+ req_fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+ req_fmt->format.quantization = V4L2_QUANTIZATION_DEFAULT;
+ req_fmt->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
+
+ 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_sink = req_fmt->format;
+
+ /* The source pad is the same as the sink, but we have to crop it */
+ isc->scaler_format_source = isc->scaler_format_sink;
+ v4l_bound_align_image
+ (&isc->scaler_format_source.width, 16, isc->max_width, 0,
+ &isc->scaler_format_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) {
+ /* bounds are the input format received */
+ sel->r.height = isc->scaler_format_sink.height;
+ sel->r.width = isc->scaler_format_sink.width;
+ sel->r.left = 0;
+ sel->r.top = 0;
+ } else if (sel->target == V4L2_SEL_TGT_CROP) {
+ /*
+ * crop is done to the output format,
+ * limited by ISC maximum size
+ */
+ sel->r.height = isc->scaler_format_source.height;
+ sel->r.width = isc->scaler_format_source.width;
+ sel->r.left = 0;
+ sel->r.top = 0;
+ } else {
+ return -EINVAL;
+ }
+
+ 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 isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
+
+ *v4l2_try_fmt = isc->scaler_format_source;
+
+ 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_format_source.height = isc->max_height;
+ isc->scaler_format_source.width = isc->max_width;
+ isc->scaler_format_source.code = isc->formats_list[0].mbus_code;
+ isc->scaler_format_source.colorspace = V4L2_COLORSPACE_SRGB;
+ isc->scaler_format_source.field = V4L2_FIELD_NONE;
+ isc->scaler_format_source.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+ isc->scaler_format_source.quantization = V4L2_QUANTIZATION_DEFAULT;
+ isc->scaler_format_source.xfer_func = V4L2_XFER_FUNC_DEFAULT;
+
+ isc->scaler_format_sink = isc->scaler_format_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 f9ad7ec6bd13..c1ca3916700e 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
@@ -258,6 +269,14 @@ 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_sink: current format for the scaler subdevice on the sink pad
+ * @scaler_format_source: current format for the scaler subdevice on the
+ * source pad
*/
struct isc_device {
struct regmap *regmap;
@@ -346,6 +365,20 @@ 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_sink;
+ struct v4l2_mbus_framefmt scaler_format_source;
+ };
};
extern const struct regmap_config isc_regmap_config;
@@ -357,4 +390,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 c5b9563e36cb..c244682ea22f 100644
--- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
@@ -553,6 +553,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);
@@ -562,7 +568,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 */
@@ -572,7 +578,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;
@@ -580,6 +585,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);
@@ -600,6 +608,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 07a80b08bc54..9dc75eed0098 100644
--- a/drivers/media/platform/atmel/atmel-sama7g5-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
@@ -547,15 +547,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);
@@ -576,6 +584,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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 05/13] media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
` (3 preceding siblings ...)
2022-02-17 13:56 ` [PATCH v5 04/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
2022-02-23 17:06 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 06/13] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming Eugen Hristev
` (7 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
Eugen Hristev
While this does not happen in production, this check should be done
versus the mask, as checking with the YCYC value may not include
some bits that may be set.
Is it correct and safe to check the whole mask.
Fixes: 123aaf816b95 ("media: atmel: atmel-sama5d2-isc: fix YUYV format")
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/media/platform/atmel/atmel-sama5d2-isc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
index c244682ea22f..025c3e8a7e95 100644
--- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
@@ -291,7 +291,7 @@ static void isc_sama5d2_config_rlp(struct isc_device *isc)
* Thus, if the YCYC mode is selected, replace it with the
* sama5d2-compliant mode which is YYCC .
*/
- if ((rlp_mode & ISC_RLP_CFG_MODE_YCYC) == ISC_RLP_CFG_MODE_YCYC) {
+ if ((rlp_mode & ISC_RLP_CFG_MODE_MASK) == ISC_RLP_CFG_MODE_YCYC) {
rlp_mode &= ~ISC_RLP_CFG_MODE_MASK;
rlp_mode |= ISC_RLP_CFG_MODE_YYCC;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 06/13] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
` (4 preceding siblings ...)
2022-02-17 13:56 ` [PATCH v5 05/13] media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
2022-02-23 17:07 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 07/13] media: atmel: atmel-isc: compact the controller formats list Eugen Hristev
` (6 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
Eugen Hristev
The AWB workqueue runs in a kernel thread and needs to be synchronized
w.r.t. the streaming status.
It is possible that streaming is stopped while the AWB workq is running.
In this case it is likely that the check for vb2_start_streaming_called is done
at one point in time, but the AWB computations are done later, including a call
to isc_update_profile, which requires streaming to be started.
Thus , isc_update_profile will fail if during this operation sequence the
streaming was stopped.
To solve this issue, a mutex is added, that will serialize the awb work and
streaming stopping, with the mention that either streaming is stopped
completely including termination of the last frame is done, and after that
the AWB work can check stream status and stop; either first AWB work is
completed and after that the streaming can stop correctly.
The awb spin lock cannot be used since this spinlock is taken in the same
context and using it in the stop streaming will result in a recursion BUG.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/media/platform/atmel/atmel-isc-base.c | 29 ++++++++++++++++---
drivers/media/platform/atmel/atmel-isc.h | 2 ++
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
index 74f575544e09..37c59bb4dc18 100644
--- a/drivers/media/platform/atmel/atmel-isc-base.c
+++ b/drivers/media/platform/atmel/atmel-isc-base.c
@@ -401,6 +401,7 @@ static void isc_stop_streaming(struct vb2_queue *vq)
struct isc_buffer *buf;
int ret;
+ mutex_lock(&isc->awb_mutex);
v4l2_ctrl_activate(isc->do_wb_ctrl, false);
isc->stop = true;
@@ -410,6 +411,8 @@ static void isc_stop_streaming(struct vb2_queue *vq)
v4l2_err(&isc->v4l2_dev,
"Timeout waiting for end of the capture\n");
+ mutex_unlock(&isc->awb_mutex);
+
/* Disable DMA interrupt */
regmap_write(isc->regmap, ISC_INTDIS, ISC_INT_DDONE);
@@ -1397,10 +1400,6 @@ static void isc_awb_work(struct work_struct *w)
u32 min, max;
int ret;
- /* streaming is not active anymore */
- if (isc->stop)
- return;
-
if (ctrls->hist_stat != HIST_ENABLED)
return;
@@ -1455,7 +1454,24 @@ static void isc_awb_work(struct work_struct *w)
}
regmap_write(regmap, ISC_HIS_CFG + isc->offsets.his,
hist_id | baysel | ISC_HIS_CFG_RAR);
+
+ /*
+ * We have to make sure the streaming has not stopped meanwhile.
+ * ISC requires a frame to clock the internal profile update.
+ * To avoid issues, lock the sequence with a mutex
+ */
+ mutex_lock(&isc->awb_mutex);
+
+ /* streaming is not active anymore */
+ if (isc->stop) {
+ mutex_unlock(&isc->awb_mutex);
+ return;
+ };
+
isc_update_profile(isc);
+
+ mutex_unlock(&isc->awb_mutex);
+
/* if awb has been disabled, we don't need to start another histogram */
if (ctrls->awb)
regmap_write(regmap, ISC_CTRLEN, ISC_CTRL_HISREQ);
@@ -1548,6 +1564,7 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl)
*/
v4l2_ctrl_activate(isc->do_wb_ctrl, false);
}
+ mutex_unlock(&isc->awb_mutex);
/* if we have autowhitebalance on, start histogram procedure */
if (ctrls->awb == ISC_WB_AUTO &&
@@ -1740,6 +1757,7 @@ static void isc_async_unbind(struct v4l2_async_notifier *notifier,
{
struct isc_device *isc = container_of(notifier->v4l2_dev,
struct isc_device, v4l2_dev);
+ mutex_destroy(&isc->awb_mutex);
cancel_work_sync(&isc->awb_work);
video_unregister_device(&isc->video_dev);
v4l2_ctrl_handler_free(&isc->ctrls.handler);
@@ -1850,6 +1868,8 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
isc->current_subdev = container_of(notifier,
struct isc_subdev_entity, notifier);
mutex_init(&isc->lock);
+ mutex_init(&isc->awb_mutex);
+
init_completion(&isc->comp);
/* Initialize videobuf2 queue */
@@ -1929,6 +1949,7 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
video_unregister_device(vdev);
isc_async_complete_err:
+ mutex_destroy(&isc->awb_mutex);
mutex_destroy(&isc->lock);
return ret;
}
diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
index c1ca3916700e..ac8c8679dd53 100644
--- a/drivers/media/platform/atmel/atmel-isc.h
+++ b/drivers/media/platform/atmel/atmel-isc.h
@@ -229,6 +229,7 @@ enum isc_scaler_pads {
*
* @lock: lock for serializing userspace file operations
* with ISC operations
+ * @awb_mutex: serialize access to streaming status from awb work queue
* @awb_lock: lock for serializing awb work queue operations
* with DMA/buffer operations
*
@@ -309,6 +310,7 @@ struct isc_device {
struct work_struct awb_work;
struct mutex lock;
+ struct mutex awb_mutex;
spinlock_t awb_lock;
struct regmap_field *pipeline[ISC_PIPE_LINE_NODE_NUM];
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 07/13] media: atmel: atmel-isc: compact the controller formats list
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
` (5 preceding siblings ...)
2022-02-17 13:56 ` [PATCH v5 06/13] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 08/13] media: atmel: atmel-isc: change format propagation to subdev into only verification Eugen Hristev
` (5 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
Eugen Hristev, Jacopo Mondi
Compact the list array to be more readable.
No other changes, only cosmetic.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
.../media/platform/atmel/atmel-sama5d2-isc.c | 51 ++++++----------
.../media/platform/atmel/atmel-sama7g5-isc.c | 60 +++++++------------
2 files changed, 37 insertions(+), 74 deletions(-)
diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
index 025c3e8a7e95..d96ee3373889 100644
--- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
@@ -60,56 +60,39 @@
static const struct isc_format sama5d2_controller_formats[] = {
{
.fourcc = V4L2_PIX_FMT_ARGB444,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_ARGB555,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_RGB565,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_ABGR32,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_XBGR32,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_YUV420,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_YUYV,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_YUV422P,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_GREY,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_Y10,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SBGGR8,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SGBRG8,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SGRBG8,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SRGGB8,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SBGGR10,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SGBRG10,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SGRBG10,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SRGGB10,
},
};
diff --git a/drivers/media/platform/atmel/atmel-sama7g5-isc.c b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
index 9dc75eed0098..e07ae188c15f 100644
--- a/drivers/media/platform/atmel/atmel-sama7g5-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
@@ -63,65 +63,45 @@
static const struct isc_format sama7g5_controller_formats[] = {
{
.fourcc = V4L2_PIX_FMT_ARGB444,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_ARGB555,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_RGB565,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_ABGR32,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_XBGR32,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_YUV420,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_UYVY,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_VYUY,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_YUYV,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_YUV422P,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_GREY,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_Y10,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_Y16,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SBGGR8,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SGBRG8,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SGRBG8,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SRGGB8,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SBGGR10,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SGBRG10,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SGRBG10,
- },
- {
+ }, {
.fourcc = V4L2_PIX_FMT_SRGGB10,
},
};
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 08/13] media: atmel: atmel-isc: change format propagation to subdev into only verification
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
` (6 preceding siblings ...)
2022-02-17 13:56 ` [PATCH v5 07/13] media: atmel: atmel-isc: compact the controller formats list Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
2022-02-23 17:03 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 09/13] media: atmel: atmel-sama7g5-isc: remove stray line Eugen Hristev
` (4 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
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.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
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 | 427 ++++++++----------
.../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, 244 insertions(+), 241 deletions(-)
diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
index 37c59bb4dc18..1a079ed9608d 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,
@@ -498,28 +487,63 @@ 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;
+ struct isc_format *fmt;
- if (index < isc->controller_formats_size) {
- f->pixelformat = isc->controller_formats[index].fourcc;
- return 0;
- }
-
- index -= isc->controller_formats_size;
-
- supported_index = 0;
-
- 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)
- continue;
- if (supported_index == index) {
- f->pixelformat = isc->formats_list[i].fourcc;
+ /*
+ * If a specific mbus_code is requested, check if we support
+ * this mbus_code as input for the ISC first.
+ * If it's supported, then we report the corresponding pixelformat
+ * as first possible option for the ISC.
+ * E.g. mbus MEDIA_BUS_FMT_SGBRG12_1X12 and report
+ * 'GB12' (12-bit Bayer GBGB/RGRG)
+ */
+ if (f->mbus_code) {
+ fmt = isc_find_format_by_code(isc, f->mbus_code, &i);
+ if (!fmt)
+ return -EINVAL;
+ if (index == supported_index) {
+ f->pixelformat = fmt->fourcc;
return 0;
}
supported_index++;
}
- return -EINVAL;
+ /*
+ * 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.
+ */
+ if (f->mbus_code && ISC_IS_FORMAT_RAW(f->mbus_code)) {
+ for (i = 0; i < isc->controller_formats_size; i++)
+ if (!isc->controller_formats[i].raw) {
+ if (index == supported_index) {
+ f->pixelformat =
+ isc->controller_formats[i].fourcc;
+ return 0;
+ }
+ supported_index++;
+ }
+ }
+
+ /*
+ * If we are asked a specific mbus_code, we have no more formats to
+ * report
+ * e.g. if the format is not raw, we cannot do any more processing.
+ */
+ if (f->mbus_code)
+ return -EINVAL;
+
+ /*
+ * And finally, without a specific mbus_code,
+ * we have to report all our formats.
+ */
+ if (index >= isc->controller_formats_size)
+ return -EINVAL;
+
+ f->pixelformat = isc->controller_formats[index].fourcc;
+
+ return 0;
}
static int isc_g_fmt_vid_cap(struct file *file, void *priv,
@@ -584,20 +608,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 +859,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 +880,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 +1037,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 +1092,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;
@@ -1782,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 = {
@@ -1836,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;
@@ -1896,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");
@@ -2015,6 +1952,24 @@ int isc_pipeline_init(struct isc_device *isc)
}
EXPORT_SYMBOL_GPL(isc_pipeline_init);
+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;
@@ -2022,6 +1977,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 0c1f49db47b4..b0200bb44f8a 100644
--- a/drivers/media/platform/atmel/atmel-isc-scaler.c
+++ b/drivers/media/platform/atmel/atmel-isc-scaler.c
@@ -171,6 +171,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,
};
@@ -188,6 +192,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 ac8c8679dd53..e2261ce99f4f 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
@@ -300,8 +301,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;
@@ -371,6 +371,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 e07ae188c15f..a0d60cfdba7b 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 09/13] media: atmel: atmel-sama7g5-isc: remove stray line
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
` (7 preceding siblings ...)
2022-02-17 13:56 ` [PATCH v5 08/13] media: atmel: atmel-isc: change format propagation to subdev into only verification Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
2022-02-23 17:07 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 10/13] dt-bindings: media: microchip,xisc: add bus-width of 14 Eugen Hristev
` (3 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
Eugen Hristev
Remove stray line from formats struct.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/media/platform/atmel/atmel-sama7g5-isc.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/media/platform/atmel/atmel-sama7g5-isc.c b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
index a0d60cfdba7b..638af8da2694 100644
--- a/drivers/media/platform/atmel/atmel-sama7g5-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
@@ -225,7 +225,6 @@ static struct isc_format sama7g5_formats_list[] = {
.mbus_code = MEDIA_BUS_FMT_Y10_1X10,
.pfe_cfg0_bps = ISC_PFG_CFG0_BPS_TEN,
},
-
};
static void isc_sama7g5_config_csc(struct isc_device *isc)
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 10/13] dt-bindings: media: microchip,xisc: add bus-width of 14
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
` (8 preceding siblings ...)
2022-02-17 13:56 ` [PATCH v5 09/13] media: atmel: atmel-sama7g5-isc: remove stray line Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 11/13] ARM: dts: at91: sama7g5: add nodes for video capture Eugen Hristev
` (2 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
Eugen Hristev, Rob Herring
The Microchip XISC supports a bus width of 14 bits.
Add it to the supported bus widths.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/media/microchip,xisc.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/media/microchip,xisc.yaml b/Documentation/devicetree/bindings/media/microchip,xisc.yaml
index 086e1430af4f..3be8f64c3e21 100644
--- a/Documentation/devicetree/bindings/media/microchip,xisc.yaml
+++ b/Documentation/devicetree/bindings/media/microchip,xisc.yaml
@@ -67,7 +67,7 @@ properties:
remote-endpoint: true
bus-width:
- enum: [8, 9, 10, 11, 12]
+ enum: [8, 9, 10, 11, 12, 14]
default: 12
hsync-active:
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 11/13] ARM: dts: at91: sama7g5: add nodes for video capture
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
` (9 preceding siblings ...)
2022-02-17 13:56 ` [PATCH v5 10/13] dt-bindings: media: microchip,xisc: add bus-width of 14 Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 12/13] ARM: configs: at91: sama7: add xisc and csi2dc Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 13/13] ARM: multi_v7_defconfig: add atmel video pipeline modules Eugen Hristev
12 siblings, 0 replies; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
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>
---
arch/arm/boot/dts/sama7g5.dtsi | 49 ++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/arch/arm/boot/dts/sama7g5.dtsi b/arch/arm/boot/dts/sama7g5.dtsi
index eddcfbf4d223..de43f854ce47 100644
--- a/arch/arm/boot/dts/sama7g5.dtsi
+++ b/arch/arm/boot/dts/sama7g5.dtsi
@@ -266,6 +266,55 @@ 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>;
+
+ 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";
+
+ 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 12/13] ARM: configs: at91: sama7: add xisc and csi2dc
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
` (10 preceding siblings ...)
2022-02-17 13:56 ` [PATCH v5 11/13] ARM: dts: at91: sama7g5: add nodes for video capture Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 13/13] ARM: multi_v7_defconfig: add atmel video pipeline modules Eugen Hristev
12 siblings, 0 replies; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
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 0368068e04d9..9918cff93e5b 100644
--- a/arch/arm/configs/sama7_defconfig
+++ b/arch/arm/configs/sama7_defconfig
@@ -127,6 +127,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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 13/13] ARM: multi_v7_defconfig: add atmel video pipeline modules
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
` (11 preceding siblings ...)
2022-02-17 13:56 ` [PATCH v5 12/13] ARM: configs: at91: sama7: add xisc and csi2dc Eugen Hristev
@ 2022-02-17 13:56 ` Eugen Hristev
12 siblings, 0 replies; 26+ messages in thread
From: Eugen Hristev @ 2022-02-17 13:56 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, nicolas.ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, claudiu.beznea,
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 8863fa969ede..b768abad8df0 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -639,7 +639,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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v5 04/13] media: atmel: atmel-isc: implement media controller
2022-02-17 13:56 ` [PATCH v5 04/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
@ 2022-02-17 14:59 ` Eugen.Hristev
2022-02-23 16:34 ` Jacopo Mondi
2022-02-23 16:32 ` Jacopo Mondi
1 sibling, 1 reply; 26+ messages in thread
From: Eugen.Hristev @ 2022-02-17 14:59 UTC (permalink / raw)
To: linux-media, jacopo, hverkuil-cisco, Nicolas.Ferre
Cc: devicetree, linux-kernel, linux-arm-kernel, Claudiu.Beznea
On 2/17/22 3:56 PM, Eugen Hristev wrote:
> Implement the support for media-controller.
> This means that the capabilities of the driver have changed and now
> it also advertises the IO_MC .
> The driver will register it's 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.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
Hello Jacopo,
I will add to this patch a little update about how the scaler is seen
now by the media-ctl :
for imx219 sensor, which generates 3280x2464:
- 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:SRGGB10_1X10/3280x2464 field:none colorspace:srgb
crop.bounds:(0,0)/3280x2464
crop:(0,0)/3264x2464]
<- "csi2dc":1 [ENABLED,IMMUTABLE]
pad1: Source
[fmt:SRGGB10_1X10/3264x2464 field:none colorspace:srgb]
-> "atmel_isc_common":0 [ENABLED,IMMUTABLE]
The source pad of the scaler looks good now.
For the imx274 which I am using (and it generates 3840x2160 ):
- 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:SRGGB10_1X10/3840x2160 field:none colorspace:srgb
crop.bounds:(0,0)/3840x2160
crop:(0,0)/3264x2160]
<- "csi2dc":1 [ENABLED,IMMUTABLE]
pad1: Source
[fmt:SRGGB10_1X10/3264x2160 field:none colorspace:srgb]
-> "atmel_isc_common":0 [ENABLED,IMMUTABLE]
So in both cases, each line is cropped down to 3264 as the maximum width.
If we select a lower frame size, the scaler will automatically update
the source pad to reflect this, e.g.:
- 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:SRGGB10_1X10/1920x1080 field:none colorspace:srgb
crop.bounds:(0,0)/1920x1080
crop:(0,0)/1920x1080]
<- "csi2dc":1 [ENABLED,IMMUTABLE]
pad1: Source
[fmt:SRGGB10_1X10/1920x1080 field:none colorspace:srgb]
-> "atmel_isc_common":0 [ENABLED,IMMUTABLE]
does it look good now ?
Thanks for checking this out !
Eugen
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 04/13] media: atmel: atmel-isc: implement media controller
2022-02-17 13:56 ` [PATCH v5 04/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
2022-02-17 14:59 ` Eugen.Hristev
@ 2022-02-23 16:32 ` Jacopo Mondi
2022-03-01 8:56 ` Eugen.Hristev
1 sibling, 1 reply; 26+ messages in thread
From: Jacopo Mondi @ 2022-02-23 16:32 UTC (permalink / raw)
To: Eugen Hristev
Cc: linux-media, hverkuil-cisco, nicolas.ferre, devicetree,
linux-kernel, linux-arm-kernel, claudiu.beznea
Hi Eugen
On Thu, Feb 17, 2022 at 03:56:36PM +0200, Eugen Hristev wrote:
> Implement the support for media-controller.
> This means that the capabilities of the driver have changed and now
> it also advertises the IO_MC .
> The driver will register it's media device, and add the video entity to this
s/it's/its
> 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.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
> 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 | 73 ++++-
> .../media/platform/atmel/atmel-isc-scaler.c | 261 ++++++++++++++++++
> drivers/media/platform/atmel/atmel-isc.h | 40 +++
> .../media/platform/atmel/atmel-sama5d2-isc.c | 14 +-
> .../media/platform/atmel/atmel-sama7g5-isc.c | 12 +-
> 6 files changed, 394 insertions(+), 8 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 67b4a2323fed..74f575544e09 100644
> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> @@ -1712,6 +1712,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");
> @@ -1720,6 +1721,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;
> }
>
> @@ -1734,8 +1745,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;
> @@ -1751,6 +1762,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)
> {
> @@ -1767,7 +1779,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);
> @@ -1893,7 +1905,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);
> @@ -1903,8 +1916,18 @@ 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;
Empty line ?
Just one clarification, the media device is initialize by mc_init()
which is called by the soc-specific code. Do all you platforms call
it ?
> return 0;
>
> +isc_async_complete_unregister_device:
> + video_unregister_device(vdev);
> +
> isc_async_complete_err:
> mutex_destroy(&isc->lock);
> return ret;
> @@ -1971,6 +1994,48 @@ 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;
It's weird very few drivers use this. It seems correct to me..
> + 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);
do you need to media_device_cleanup() ?
> +}
> +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..0c1f49db47b4
> --- /dev/null
> +++ b/drivers/media/platform/atmel/atmel-isc-scaler.c
> @@ -0,0 +1,261 @@
> +// 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 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;
> + }
> +
> + if (format->pad == ISC_SCALER_PAD_SOURCE)
> + format->format = isc->scaler_format_source;
> + else
> + format->format = isc->scaler_format_sink;
Having an [] might make this nicer..
> +
> + 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_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,
Fits on the previous line
> + &req_fmt->format.height, 16, UINT_MAX, 0, 0);
> +
> + req_fmt->format.colorspace = V4L2_COLORSPACE_SRGB;
> + req_fmt->format.field = V4L2_FIELD_NONE;
> + req_fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + req_fmt->format.quantization = V4L2_QUANTIZATION_DEFAULT;
> + req_fmt->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +
> + fmt = isc_find_format_by_code(isc, req_fmt->format.code, &i);
> +
> + if (!fmt)
> + fmt = &isc->formats_list[0];
The
fmt = isc_find_format_by_code();
if (!fmt)
fmt = &isc->formats_list[0];
pattern is repeated in a few places. Maybe isc_find_format_by_code()
could default back to format_list[0] instead of returning NULL ?
> +
> + 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,
Fits on previous line ?
> + 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_sink = req_fmt->format;
> +
> + /* The source pad is the same as the sink, but we have to crop it */
> + isc->scaler_format_source = isc->scaler_format_sink;
> + v4l_bound_align_image
> + (&isc->scaler_format_source.width, 16, isc->max_width, 0,
> + &isc->scaler_format_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) {
> + /* bounds are the input format received */
> + sel->r.height = isc->scaler_format_sink.height;
> + sel->r.width = isc->scaler_format_sink.width;
> + sel->r.left = 0;
> + sel->r.top = 0;
> + } else if (sel->target == V4L2_SEL_TGT_CROP) {
> + /*
> + * crop is done to the output format,
> + * limited by ISC maximum size
> + */
> + sel->r.height = isc->scaler_format_source.height;
> + sel->r.width = isc->scaler_format_source.width;
> + sel->r.left = 0;
> + sel->r.top = 0;
> + } else {
> + return -EINVAL;
> + }
> +
> + 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 isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
> +
> + *v4l2_try_fmt = isc->scaler_format_source;
Try crop rectangles should be initialized too ?
> +
> + 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_format_source.height = isc->max_height;
> + isc->scaler_format_source.width = isc->max_width;
> + isc->scaler_format_source.code = isc->formats_list[0].mbus_code;
> + isc->scaler_format_source.colorspace = V4L2_COLORSPACE_SRGB;
> + isc->scaler_format_source.field = V4L2_FIELD_NONE;
> + isc->scaler_format_source.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + isc->scaler_format_source.quantization = V4L2_QUANTIZATION_DEFAULT;
> + isc->scaler_format_source.xfer_func = V4L2_XFER_FUNC_DEFAULT;
Minor detail: you assign the field from colorspace on at s_ftm time
too.
Maybe if you define a static const default_fmt you can re-use it
instead of assigning fields one by one ?
> +
> + isc->scaler_format_sink = isc->scaler_format_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;
> + }
empty line ?
> + 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 f9ad7ec6bd13..c1ca3916700e 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
> @@ -258,6 +269,14 @@ 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_sink: current format for the scaler subdevice on the sink pad
> + * @scaler_format_source: current format for the scaler subdevice on the
I'm still not super happy with the scaler configuration being part of
the larger ISC base structure. But I understand this is fine with you.
Please consider having an array for the scaler pads formats, but it's a minor
detail.
> + * source pad
> */
> struct isc_device {
> struct regmap *regmap;
> @@ -346,6 +365,20 @@ 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_sink;
> + struct v4l2_mbus_framefmt scaler_format_source;
> + };
> };
>
> extern const struct regmap_config isc_regmap_config;
> @@ -357,4 +390,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 c5b9563e36cb..c244682ea22f 100644
> --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> @@ -553,6 +553,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);
> @@ -562,7 +568,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 */
> @@ -572,7 +578,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;
> @@ -580,6 +585,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);
>
> @@ -600,6 +608,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 07a80b08bc54..9dc75eed0098 100644
> --- a/drivers/media/platform/atmel/atmel-sama7g5-isc.c
> +++ b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
> @@ -547,15 +547,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);
>
> @@ -576,6 +584,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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 04/13] media: atmel: atmel-isc: implement media controller
2022-02-17 14:59 ` Eugen.Hristev
@ 2022-02-23 16:34 ` Jacopo Mondi
0 siblings, 0 replies; 26+ messages in thread
From: Jacopo Mondi @ 2022-02-23 16:34 UTC (permalink / raw)
To: Eugen.Hristev
Cc: linux-media, hverkuil-cisco, Nicolas.Ferre, devicetree,
linux-kernel, linux-arm-kernel, Claudiu.Beznea
Hi Eugen,
On Thu, Feb 17, 2022 at 02:59:19PM +0000, Eugen.Hristev@microchip.com wrote:
> On 2/17/22 3:56 PM, Eugen Hristev wrote:
> > Implement the support for media-controller.
> > This means that the capabilities of the driver have changed and now
> > it also advertises the IO_MC .
> > The driver will register it's 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.
> >
> > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > ---
>
>
> Hello Jacopo,
>
> I will add to this patch a little update about how the scaler is seen
> now by the media-ctl :
>
> for imx219 sensor, which generates 3280x2464:
>
>
> - 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:SRGGB10_1X10/3280x2464 field:none colorspace:srgb
> crop.bounds:(0,0)/3280x2464
> crop:(0,0)/3264x2464]
> <- "csi2dc":1 [ENABLED,IMMUTABLE]
> pad1: Source
> [fmt:SRGGB10_1X10/3264x2464 field:none colorspace:srgb]
> -> "atmel_isc_common":0 [ENABLED,IMMUTABLE]
>
>
> The source pad of the scaler looks good now.
>
> For the imx274 which I am using (and it generates 3840x2160 ):
>
> - 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:SRGGB10_1X10/3840x2160 field:none colorspace:srgb
> crop.bounds:(0,0)/3840x2160
> crop:(0,0)/3264x2160]
> <- "csi2dc":1 [ENABLED,IMMUTABLE]
> pad1: Source
> [fmt:SRGGB10_1X10/3264x2160 field:none colorspace:srgb]
> -> "atmel_isc_common":0 [ENABLED,IMMUTABLE]
>
> So in both cases, each line is cropped down to 3264 as the maximum width.
>
> If we select a lower frame size, the scaler will automatically update
> the source pad to reflect this, e.g.:
>
>
> - 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:SRGGB10_1X10/1920x1080 field:none colorspace:srgb
> crop.bounds:(0,0)/1920x1080
> crop:(0,0)/1920x1080]
> <- "csi2dc":1 [ENABLED,IMMUTABLE]
> pad1: Source
> [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:srgb]
> -> "atmel_isc_common":0 [ENABLED,IMMUTABLE]
>
>
> does it look good now ?
great! I think it now works as intended, thanks!
I only have one question. BOUND target is the larger rectangle that
contains all crop rectangles. As your max crop is limited to 3264x2160
I wonder if BOUND should be limited by the same size. Does
v4l2-compliance check for this.
Otherwise it looks good!
Thanks
j
>
> Thanks for checking this out !
> Eugen
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 08/13] media: atmel: atmel-isc: change format propagation to subdev into only verification
2022-02-17 13:56 ` [PATCH v5 08/13] media: atmel: atmel-isc: change format propagation to subdev into only verification Eugen Hristev
@ 2022-02-23 17:03 ` Jacopo Mondi
2022-03-03 15:21 ` Eugen.Hristev
0 siblings, 1 reply; 26+ messages in thread
From: Jacopo Mondi @ 2022-02-23 17:03 UTC (permalink / raw)
To: Eugen Hristev
Cc: linux-media, hverkuil-cisco, nicolas.ferre, devicetree,
linux-kernel, linux-arm-kernel, claudiu.beznea
Hi Eugen
On Thu, Feb 17, 2022 at 03:56:40PM +0200, Eugen Hristev wrote:
> 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.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
> 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 | 427 ++++++++----------
> .../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, 244 insertions(+), 241 deletions(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> index 37c59bb4dc18..1a079ed9608d 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,
> @@ -498,28 +487,63 @@ 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;
> + struct isc_format *fmt;
>
> - if (index < isc->controller_formats_size) {
> - f->pixelformat = isc->controller_formats[index].fourcc;
> - return 0;
> - }
> -
> - index -= isc->controller_formats_size;
> -
> - supported_index = 0;
You seem to have lost this one.
You can initialize while declaring it
> -
> - 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)
> - continue;
> - if (supported_index == index) {
> - f->pixelformat = isc->formats_list[i].fourcc;
> + /*
> + * If a specific mbus_code is requested, check if we support
> + * this mbus_code as input for the ISC first.
> + * If it's supported, then we report the corresponding pixelformat
> + * as first possible option for the ISC.
> + * E.g. mbus MEDIA_BUS_FMT_SGBRG12_1X12 and report
Is this a good example, since you check for RAW in a separate loop ?
> + * 'GB12' (12-bit Bayer GBGB/RGRG)
> + */
> + if (f->mbus_code) {
> + fmt = isc_find_format_by_code(isc, f->mbus_code, &i);
> + if (!fmt)
> + return -EINVAL;
> + if (index == supported_index) {
> + f->pixelformat = fmt->fourcc;
> return 0;
> }
Not sure I get this. Are there multiple pixel formats associated with
a non-RAW mbus_code ? If not, your isc_find_format_by_code() call will always
return a single pixelformat per mbus_code, so there is no need to
check with supported index. If the ISC cannot convert between
colorspaces (ie not YUYV->RGB) I think it would be easier to check for
RAW first and enumerate all the non-RAW pixfmt the ISC can generate
from RAW, then if mbus_code is non RAW just check if it's in the list
of supported formats and return the corresponding pixfmt. Or can the
ISC conver between different colorspaces and I didn't get this ?
> supported_index++;
> }
>
> - return -EINVAL;
> + /*
> + * 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.
> + */
> + if (f->mbus_code && ISC_IS_FORMAT_RAW(f->mbus_code)) {
Won't this be hijacked by the previous loop ?
if (f->mbus_code) {
}
if (f->mbus_code && ISC_IS_FORMAT_RAW(f->mbus_code)) {
}
> + for (i = 0; i < isc->controller_formats_size; i++)
> + if (!isc->controller_formats[i].raw) {
if (isc->controller_format[i].raw)
continue;
To save an indentation level
> + if (index == supported_index) {
Should supported_index be re-initialized before this loop ?
> + f->pixelformat =
> + isc->controller_formats[i].fourcc;
You are enumerating -all- formats except RAW, right ?
This mean you can convert RAW to all other formats ?
> + return 0;
> + }
> + supported_index++;
> + }
> + }
> +
> + /*
> + * If we are asked a specific mbus_code, we have no more formats to
> + * report
> + * e.g. if the format is not raw, we cannot do any more processing.
> + */
> + if (f->mbus_code)
> + return -EINVAL;
As suggested, if my understanding is correct, I would enumerate all
formats generated from a RAW mbus_code. If the mbus_code is not RAW
then look if it's supported with isc_find_format_by_code() (and only
index==0 is valid, unless there will be multiple entries with the same
mbus_code in your format list, but isc_find_format_by_code() doesn't
seem to support that).
> +
> + /*
> + * And finally, without a specific mbus_code,
> + * we have to report all our formats.
> + */
> + if (index >= isc->controller_formats_size)
> + return -EINVAL;
> +
> + f->pixelformat = isc->controller_formats[index].fourcc;
> +
> + return 0;
> }
>
> static int isc_g_fmt_vid_cap(struct file *file, void *priv,
> @@ -584,20 +608,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 +859,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 +880,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);
I think when v4l2_subdev_call_state_active() will land it will
probably simplify calling the subdev driver operations.
> 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);
This is done at try_format(). Is it intentionally repeated ?
>
> /* 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 +1037,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 +1092,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;
> @@ -1782,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 = {
> @@ -1836,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;
>
> @@ -1896,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");
> @@ -2015,6 +1952,24 @@ int isc_pipeline_init(struct isc_device *isc)
> }
> EXPORT_SYMBOL_GPL(isc_pipeline_init);
>
> +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;
> @@ -2022,6 +1977,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 0c1f49db47b4..b0200bb44f8a 100644
> --- a/drivers/media/platform/atmel/atmel-isc-scaler.c
> +++ b/drivers/media/platform/atmel/atmel-isc-scaler.c
> @@ -171,6 +171,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,
> };
> @@ -188,6 +192,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 ac8c8679dd53..e2261ce99f4f 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
> @@ -300,8 +301,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;
> @@ -371,6 +371,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,
Is the list zero-initialized ? Begin a static const I assume so (I
should look at the C spec instead of asking :)
> },
> };
>
> diff --git a/drivers/media/platform/atmel/atmel-sama7g5-isc.c b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
> index e07ae188c15f..a0d60cfdba7b 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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 01/13] media: atmel: atmel-isc-base: use streaming status when queueing buffers
2022-02-17 13:56 ` [PATCH v5 01/13] media: atmel: atmel-isc-base: use streaming status when queueing buffers Eugen Hristev
@ 2022-02-23 17:05 ` Jacopo Mondi
0 siblings, 0 replies; 26+ messages in thread
From: Jacopo Mondi @ 2022-02-23 17:05 UTC (permalink / raw)
To: Eugen Hristev
Cc: linux-media, hverkuil-cisco, nicolas.ferre, devicetree,
linux-kernel, linux-arm-kernel, claudiu.beznea
Hi Eugen
On Thu, Feb 17, 2022 at 03:56:33PM +0200, Eugen Hristev wrote:
> During experiments with libcamera, it looks like vb2_is_streaming returns
> true before our start streaming is called.
> Order of operations is streamon -> queue -> start_streaming
> ISC would have started the DMA immediately when a buffer is being added
> to the vbqueue if the queue is streaming.
> It is more safe to start the DMA after the start streaming of the driver is
> called.
> Thus, even if vb2queue is streaming, add the buffer to the dma queue of the
> driver instead of actually starting the DMA process, if the start streaming
> has not been called yet.
> Tho achieve this, we have to use vb2_start_streaming_called instead of
> vb2_is_streaming.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Thanks
j
> ---
> drivers/media/platform/atmel/atmel-isc-base.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> index db15770d5b88..d2cc6c99984f 100644
> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> @@ -442,7 +442,7 @@ static void isc_buffer_queue(struct vb2_buffer *vb)
>
> spin_lock_irqsave(&isc->dma_queue_lock, flags);
> if (!isc->cur_frm && list_empty(&isc->dma_queue) &&
> - vb2_is_streaming(vb->vb2_queue)) {
> + vb2_start_streaming_called(vb->vb2_queue)) {
> isc->cur_frm = buf;
> isc_start_dma(isc);
> } else
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 02/13] media: atmel: atmel-isc-base: replace is_streaming call in s_fmt_vid_cap
2022-02-17 13:56 ` [PATCH v5 02/13] media: atmel: atmel-isc-base: replace is_streaming call in s_fmt_vid_cap Eugen Hristev
@ 2022-02-23 17:05 ` Jacopo Mondi
0 siblings, 0 replies; 26+ messages in thread
From: Jacopo Mondi @ 2022-02-23 17:05 UTC (permalink / raw)
To: Eugen Hristev
Cc: linux-media, hverkuil-cisco, nicolas.ferre, devicetree,
linux-kernel, linux-arm-kernel, claudiu.beznea, Hans Verkuil
Hi Eugen
On Thu, Feb 17, 2022 at 03:56:34PM +0200, Eugen Hristev wrote:
> In s_fmt_vid_cap, we should check if vb2_is_busy and return EBUSY,
> not check if it's streaming to return the busy state.
>
> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Thanks
j
> ---
> drivers/media/platform/atmel/atmel-isc-base.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> index d2cc6c99984f..67b4a2323fed 100644
> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> @@ -1029,7 +1029,7 @@ static int isc_s_fmt_vid_cap(struct file *file, void *priv,
> {
> struct isc_device *isc = video_drvdata(file);
>
> - if (vb2_is_streaming(&isc->vb2_vidq))
> + if (vb2_is_busy(&isc->vb2_vidq))
> return -EBUSY;
>
> return isc_set_fmt(isc, f);
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 03/13] media: atmel: atmel-isc: remove redundant comments
2022-02-17 13:56 ` [PATCH v5 03/13] media: atmel: atmel-isc: remove redundant comments Eugen Hristev
@ 2022-02-23 17:06 ` Jacopo Mondi
0 siblings, 0 replies; 26+ messages in thread
From: Jacopo Mondi @ 2022-02-23 17:06 UTC (permalink / raw)
To: Eugen Hristev
Cc: linux-media, hverkuil-cisco, nicolas.ferre, devicetree,
linux-kernel, linux-arm-kernel, claudiu.beznea
Hi Eugen
On Thu, Feb 17, 2022 at 03:56:35PM +0200, Eugen Hristev wrote:
> Remove duplicate comments which are already in place before the struct
> definition.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Thanks
j
> ---
> drivers/media/platform/atmel/atmel-isc.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
> index 07fa6dbf8460..f9ad7ec6bd13 100644
> --- a/drivers/media/platform/atmel/atmel-isc.h
> +++ b/drivers/media/platform/atmel/atmel-isc.h
> @@ -272,7 +272,7 @@ struct isc_device {
> struct video_device video_dev;
>
> struct vb2_queue vb2_vidq;
> - spinlock_t dma_queue_lock; /* serialize access to dma queue */
> + spinlock_t dma_queue_lock;
> struct list_head dma_queue;
> struct isc_buffer *cur_frm;
> unsigned int sequence;
> @@ -289,8 +289,8 @@ struct isc_device {
> struct isc_ctrls ctrls;
> struct work_struct awb_work;
>
> - struct mutex lock; /* serialize access to file operations */
> - spinlock_t awb_lock; /* serialize access to DMA buffers from awb work queue */
> + struct mutex lock;
> + spinlock_t awb_lock;
>
> struct regmap_field *pipeline[ISC_PIPE_LINE_NODE_NUM];
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 05/13] media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check
2022-02-17 13:56 ` [PATCH v5 05/13] media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check Eugen Hristev
@ 2022-02-23 17:06 ` Jacopo Mondi
0 siblings, 0 replies; 26+ messages in thread
From: Jacopo Mondi @ 2022-02-23 17:06 UTC (permalink / raw)
To: Eugen Hristev
Cc: linux-media, hverkuil-cisco, nicolas.ferre, devicetree,
linux-kernel, linux-arm-kernel, claudiu.beznea
Hi Eugen
On Thu, Feb 17, 2022 at 03:56:37PM +0200, Eugen Hristev wrote:
> While this does not happen in production, this check should be done
> versus the mask, as checking with the YCYC value may not include
> some bits that may be set.
> Is it correct and safe to check the whole mask.
>
> Fixes: 123aaf816b95 ("media: atmel: atmel-sama5d2-isc: fix YUYV format")
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
I trust this
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Thanks
j
> ---
> drivers/media/platform/atmel/atmel-sama5d2-isc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> index c244682ea22f..025c3e8a7e95 100644
> --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> @@ -291,7 +291,7 @@ static void isc_sama5d2_config_rlp(struct isc_device *isc)
> * Thus, if the YCYC mode is selected, replace it with the
> * sama5d2-compliant mode which is YYCC .
> */
> - if ((rlp_mode & ISC_RLP_CFG_MODE_YCYC) == ISC_RLP_CFG_MODE_YCYC) {
> + if ((rlp_mode & ISC_RLP_CFG_MODE_MASK) == ISC_RLP_CFG_MODE_YCYC) {
> rlp_mode &= ~ISC_RLP_CFG_MODE_MASK;
> rlp_mode |= ISC_RLP_CFG_MODE_YYCC;
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 06/13] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming
2022-02-17 13:56 ` [PATCH v5 06/13] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming Eugen Hristev
@ 2022-02-23 17:07 ` Jacopo Mondi
0 siblings, 0 replies; 26+ messages in thread
From: Jacopo Mondi @ 2022-02-23 17:07 UTC (permalink / raw)
To: Eugen Hristev
Cc: linux-media, hverkuil-cisco, nicolas.ferre, devicetree,
linux-kernel, linux-arm-kernel, claudiu.beznea
Hi Eugen
On Thu, Feb 17, 2022 at 03:56:38PM +0200, Eugen Hristev wrote:
> The AWB workqueue runs in a kernel thread and needs to be synchronized
> w.r.t. the streaming status.
> It is possible that streaming is stopped while the AWB workq is running.
> In this case it is likely that the check for vb2_start_streaming_called is done
> at one point in time, but the AWB computations are done later, including a call
> to isc_update_profile, which requires streaming to be started.
> Thus , isc_update_profile will fail if during this operation sequence the
> streaming was stopped.
> To solve this issue, a mutex is added, that will serialize the awb work and
> streaming stopping, with the mention that either streaming is stopped
> completely including termination of the last frame is done, and after that
> the AWB work can check stream status and stop; either first AWB work is
> completed and after that the streaming can stop correctly.
> The awb spin lock cannot be used since this spinlock is taken in the same
> context and using it in the stop streaming will result in a recursion BUG.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
I trust this doesn't deadlock :)
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Thanks
j
> ---
> drivers/media/platform/atmel/atmel-isc-base.c | 29 ++++++++++++++++---
> drivers/media/platform/atmel/atmel-isc.h | 2 ++
> 2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> index 74f575544e09..37c59bb4dc18 100644
> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> @@ -401,6 +401,7 @@ static void isc_stop_streaming(struct vb2_queue *vq)
> struct isc_buffer *buf;
> int ret;
>
> + mutex_lock(&isc->awb_mutex);
> v4l2_ctrl_activate(isc->do_wb_ctrl, false);
>
> isc->stop = true;
> @@ -410,6 +411,8 @@ static void isc_stop_streaming(struct vb2_queue *vq)
> v4l2_err(&isc->v4l2_dev,
> "Timeout waiting for end of the capture\n");
>
> + mutex_unlock(&isc->awb_mutex);
> +
> /* Disable DMA interrupt */
> regmap_write(isc->regmap, ISC_INTDIS, ISC_INT_DDONE);
>
> @@ -1397,10 +1400,6 @@ static void isc_awb_work(struct work_struct *w)
> u32 min, max;
> int ret;
>
> - /* streaming is not active anymore */
> - if (isc->stop)
> - return;
> -
> if (ctrls->hist_stat != HIST_ENABLED)
> return;
>
> @@ -1455,7 +1454,24 @@ static void isc_awb_work(struct work_struct *w)
> }
> regmap_write(regmap, ISC_HIS_CFG + isc->offsets.his,
> hist_id | baysel | ISC_HIS_CFG_RAR);
> +
> + /*
> + * We have to make sure the streaming has not stopped meanwhile.
> + * ISC requires a frame to clock the internal profile update.
> + * To avoid issues, lock the sequence with a mutex
> + */
> + mutex_lock(&isc->awb_mutex);
> +
> + /* streaming is not active anymore */
> + if (isc->stop) {
> + mutex_unlock(&isc->awb_mutex);
> + return;
> + };
> +
> isc_update_profile(isc);
> +
> + mutex_unlock(&isc->awb_mutex);
> +
> /* if awb has been disabled, we don't need to start another histogram */
> if (ctrls->awb)
> regmap_write(regmap, ISC_CTRLEN, ISC_CTRL_HISREQ);
> @@ -1548,6 +1564,7 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl)
> */
> v4l2_ctrl_activate(isc->do_wb_ctrl, false);
> }
> + mutex_unlock(&isc->awb_mutex);
>
> /* if we have autowhitebalance on, start histogram procedure */
> if (ctrls->awb == ISC_WB_AUTO &&
> @@ -1740,6 +1757,7 @@ static void isc_async_unbind(struct v4l2_async_notifier *notifier,
> {
> struct isc_device *isc = container_of(notifier->v4l2_dev,
> struct isc_device, v4l2_dev);
> + mutex_destroy(&isc->awb_mutex);
> cancel_work_sync(&isc->awb_work);
> video_unregister_device(&isc->video_dev);
> v4l2_ctrl_handler_free(&isc->ctrls.handler);
> @@ -1850,6 +1868,8 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
> isc->current_subdev = container_of(notifier,
> struct isc_subdev_entity, notifier);
> mutex_init(&isc->lock);
> + mutex_init(&isc->awb_mutex);
> +
> init_completion(&isc->comp);
>
> /* Initialize videobuf2 queue */
> @@ -1929,6 +1949,7 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
> video_unregister_device(vdev);
>
> isc_async_complete_err:
> + mutex_destroy(&isc->awb_mutex);
> mutex_destroy(&isc->lock);
> return ret;
> }
> diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
> index c1ca3916700e..ac8c8679dd53 100644
> --- a/drivers/media/platform/atmel/atmel-isc.h
> +++ b/drivers/media/platform/atmel/atmel-isc.h
> @@ -229,6 +229,7 @@ enum isc_scaler_pads {
> *
> * @lock: lock for serializing userspace file operations
> * with ISC operations
> + * @awb_mutex: serialize access to streaming status from awb work queue
> * @awb_lock: lock for serializing awb work queue operations
> * with DMA/buffer operations
> *
> @@ -309,6 +310,7 @@ struct isc_device {
> struct work_struct awb_work;
>
> struct mutex lock;
> + struct mutex awb_mutex;
> spinlock_t awb_lock;
>
> struct regmap_field *pipeline[ISC_PIPE_LINE_NODE_NUM];
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 09/13] media: atmel: atmel-sama7g5-isc: remove stray line
2022-02-17 13:56 ` [PATCH v5 09/13] media: atmel: atmel-sama7g5-isc: remove stray line Eugen Hristev
@ 2022-02-23 17:07 ` Jacopo Mondi
0 siblings, 0 replies; 26+ messages in thread
From: Jacopo Mondi @ 2022-02-23 17:07 UTC (permalink / raw)
To: Eugen Hristev
Cc: linux-media, hverkuil-cisco, nicolas.ferre, devicetree,
linux-kernel, linux-arm-kernel, claudiu.beznea
Hi Eugen
On Thu, Feb 17, 2022 at 03:56:41PM +0200, Eugen Hristev wrote:
> Remove stray line from formats struct.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Thanks
j
> ---
> drivers/media/platform/atmel/atmel-sama7g5-isc.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-sama7g5-isc.c b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
> index a0d60cfdba7b..638af8da2694 100644
> --- a/drivers/media/platform/atmel/atmel-sama7g5-isc.c
> +++ b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
> @@ -225,7 +225,6 @@ static struct isc_format sama7g5_formats_list[] = {
> .mbus_code = MEDIA_BUS_FMT_Y10_1X10,
> .pfe_cfg0_bps = ISC_PFG_CFG0_BPS_TEN,
> },
> -
> };
>
> static void isc_sama7g5_config_csc(struct isc_device *isc)
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 04/13] media: atmel: atmel-isc: implement media controller
2022-02-23 16:32 ` Jacopo Mondi
@ 2022-03-01 8:56 ` Eugen.Hristev
0 siblings, 0 replies; 26+ messages in thread
From: Eugen.Hristev @ 2022-03-01 8:56 UTC (permalink / raw)
To: jacopo
Cc: linux-media, hverkuil-cisco, Nicolas.Ferre, devicetree,
linux-kernel, linux-arm-kernel, Claudiu.Beznea
On 2/23/22 6:32 PM, Jacopo Mondi wrote:
> Hi Eugen
>
> On Thu, Feb 17, 2022 at 03:56:36PM +0200, Eugen Hristev wrote:
>> Implement the support for media-controller.
>> This means that the capabilities of the driver have changed and now
>> it also advertises the IO_MC .
>> The driver will register it's media device, and add the video entity to this
>
> s/it's/its
>
>> 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.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>> 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 | 73 ++++-
>> .../media/platform/atmel/atmel-isc-scaler.c | 261 ++++++++++++++++++
>> drivers/media/platform/atmel/atmel-isc.h | 40 +++
>> .../media/platform/atmel/atmel-sama5d2-isc.c | 14 +-
>> .../media/platform/atmel/atmel-sama7g5-isc.c | 12 +-
>> 6 files changed, 394 insertions(+), 8 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 67b4a2323fed..74f575544e09 100644
>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>> @@ -1712,6 +1712,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");
>> @@ -1720,6 +1721,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;
>> }
>>
>> @@ -1734,8 +1745,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;
>> @@ -1751,6 +1762,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)
>> {
>> @@ -1767,7 +1779,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);
>> @@ -1893,7 +1905,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);
>> @@ -1903,8 +1916,18 @@ 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;
>
> Empty line ?
>
> Just one clarification, the media device is initialize by mc_init()
> which is called by the soc-specific code. Do all you platforms call
> it ?
Hi Jacopo,
Yes. There are two platforms right now using this driver, the sama5d2
and the sama7g5 and both are converted to media controller.
>
>> return 0;
>>
>> +isc_async_complete_unregister_device:
>> + video_unregister_device(vdev);
>> +
>> isc_async_complete_err:
>> mutex_destroy(&isc->lock);
>> return ret;
>> @@ -1971,6 +1994,48 @@ 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;
>
> It's weird very few drivers use this. It seems correct to me..
>
>> + 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);
>
> do you need to media_device_cleanup() ?
>
>> +}
>> +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..0c1f49db47b4
>> --- /dev/null
>> +++ b/drivers/media/platform/atmel/atmel-isc-scaler.c
>> @@ -0,0 +1,261 @@
>> +// 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 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;
>> + }
>> +
>> + if (format->pad == ISC_SCALER_PAD_SOURCE)
>> + format->format = isc->scaler_format_source;
>> + else
>> + format->format = isc->scaler_format_sink;
>
> Having an [] might make this nicer..
>
>> +
>> + 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_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,
>
> Fits on the previous line
>
>> + &req_fmt->format.height, 16, UINT_MAX, 0, 0);
>> +
>> + req_fmt->format.colorspace = V4L2_COLORSPACE_SRGB;
>> + req_fmt->format.field = V4L2_FIELD_NONE;
>> + req_fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>> + req_fmt->format.quantization = V4L2_QUANTIZATION_DEFAULT;
>> + req_fmt->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
>> +
>> + fmt = isc_find_format_by_code(isc, req_fmt->format.code, &i);
>> +
>> + if (!fmt)
>> + fmt = &isc->formats_list[0];
>
> The
> fmt = isc_find_format_by_code();
> if (!fmt)
> fmt = &isc->formats_list[0];
>
> pattern is repeated in a few places. Maybe isc_find_format_by_code()
> could default back to format_list[0] instead of returning NULL ?
>
>> +
>> + 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,
>
> Fits on previous line ?
>
>> + 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_sink = req_fmt->format;
>> +
>> + /* The source pad is the same as the sink, but we have to crop it */
>> + isc->scaler_format_source = isc->scaler_format_sink;
>> + v4l_bound_align_image
>> + (&isc->scaler_format_source.width, 16, isc->max_width, 0,
>> + &isc->scaler_format_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) {
>> + /* bounds are the input format received */
>> + sel->r.height = isc->scaler_format_sink.height;
>> + sel->r.width = isc->scaler_format_sink.width;
>> + sel->r.left = 0;
>> + sel->r.top = 0;
>> + } else if (sel->target == V4L2_SEL_TGT_CROP) {
>> + /*
>> + * crop is done to the output format,
>> + * limited by ISC maximum size
>> + */
>> + sel->r.height = isc->scaler_format_source.height;
>> + sel->r.width = isc->scaler_format_source.width;
>> + sel->r.left = 0;
>> + sel->r.top = 0;
>> + } else {
>> + return -EINVAL;
>> + }
>> +
>> + 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 isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
>> +
>> + *v4l2_try_fmt = isc->scaler_format_source;
>
> Try crop rectangles should be initialized too ?
>
>> +
>> + 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_format_source.height = isc->max_height;
>> + isc->scaler_format_source.width = isc->max_width;
>> + isc->scaler_format_source.code = isc->formats_list[0].mbus_code;
>> + isc->scaler_format_source.colorspace = V4L2_COLORSPACE_SRGB;
>> + isc->scaler_format_source.field = V4L2_FIELD_NONE;
>> + isc->scaler_format_source.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>> + isc->scaler_format_source.quantization = V4L2_QUANTIZATION_DEFAULT;
>> + isc->scaler_format_source.xfer_func = V4L2_XFER_FUNC_DEFAULT;
>
> Minor detail: you assign the field from colorspace on at s_ftm time
> too.
>
> Maybe if you define a static const default_fmt you can re-use it
> instead of assigning fields one by one ?
In s_fmt I have to alter the req_fmt to have it set with all the
required fields to the proper value. So I cannot assign the req_fmt in
s_fmt to this const, because it will erase everything else that I really
need (format, frame size, ...).
I will add a small function to set all the other fields to a given frame
fmt.
Thanks for reviewing, I will come up with the next version soon.
Eugen
>
>> +
>> + isc->scaler_format_sink = isc->scaler_format_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;
>> + }
>
> empty line ?
>
>> + 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 f9ad7ec6bd13..c1ca3916700e 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
>> @@ -258,6 +269,14 @@ 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_sink: current format for the scaler subdevice on the sink pad
>> + * @scaler_format_source: current format for the scaler subdevice on the
>
> I'm still not super happy with the scaler configuration being part of
> the larger ISC base structure. But I understand this is fine with you.
> Please consider having an array for the scaler pads formats, but it's a minor
> detail.
>
>> + * source pad
>> */
>> struct isc_device {
>> struct regmap *regmap;
>> @@ -346,6 +365,20 @@ 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_sink;
>> + struct v4l2_mbus_framefmt scaler_format_source;
>> + };
>> };
>>
>> extern const struct regmap_config isc_regmap_config;
>> @@ -357,4 +390,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 c5b9563e36cb..c244682ea22f 100644
>> --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
>> +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
>> @@ -553,6 +553,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);
>> @@ -562,7 +568,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 */
>> @@ -572,7 +578,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;
>> @@ -580,6 +585,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);
>>
>> @@ -600,6 +608,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 07a80b08bc54..9dc75eed0098 100644
>> --- a/drivers/media/platform/atmel/atmel-sama7g5-isc.c
>> +++ b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
>> @@ -547,15 +547,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);
>>
>> @@ -576,6 +584,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
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 08/13] media: atmel: atmel-isc: change format propagation to subdev into only verification
2022-02-23 17:03 ` Jacopo Mondi
@ 2022-03-03 15:21 ` Eugen.Hristev
0 siblings, 0 replies; 26+ messages in thread
From: Eugen.Hristev @ 2022-03-03 15:21 UTC (permalink / raw)
To: jacopo
Cc: linux-media, hverkuil-cisco, Nicolas.Ferre, devicetree,
linux-kernel, linux-arm-kernel, Claudiu.Beznea
On 2/23/22 7:03 PM, Jacopo Mondi wrote:
> Hi Eugen
>
> On Thu, Feb 17, 2022 at 03:56:40PM +0200, Eugen Hristev wrote:
>> 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.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>> 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 | 427 ++++++++----------
>> .../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, 244 insertions(+), 241 deletions(-)
>>
>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>> index 37c59bb4dc18..1a079ed9608d 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,
>> @@ -498,28 +487,63 @@ 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;
>> + struct isc_format *fmt;
>>
>> - if (index < isc->controller_formats_size) {
>> - f->pixelformat = isc->controller_formats[index].fourcc;
>> - return 0;
>> - }
>> -
>> - index -= isc->controller_formats_size;
>> -
>> - supported_index = 0;
>
> You seem to have lost this one.
> You can initialize while declaring it
>
>> -
>> - 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)
>> - continue;
>> - if (supported_index == index) {
>> - f->pixelformat = isc->formats_list[i].fourcc;
>> + /*
>> + * If a specific mbus_code is requested, check if we support
>> + * this mbus_code as input for the ISC first.
>> + * If it's supported, then we report the corresponding pixelformat
>> + * as first possible option for the ISC.
>> + * E.g. mbus MEDIA_BUS_FMT_SGBRG12_1X12 and report
>
> Is this a good example, since you check for RAW in a separate loop ?
>
>> + * 'GB12' (12-bit Bayer GBGB/RGRG)
>> + */
>> + if (f->mbus_code) {
>> + fmt = isc_find_format_by_code(isc, f->mbus_code, &i);
>> + if (!fmt)
>> + return -EINVAL;
>> + if (index == supported_index) {
>> + f->pixelformat = fmt->fourcc;
>> return 0;
>> }
>
> Not sure I get this. Are there multiple pixel formats associated with
> a non-RAW mbus_code ? If not, your isc_find_format_by_code() call will always
> return a single pixelformat per mbus_code, so there is no need to
> check with supported index. If the ISC cannot convert between
> colorspaces (ie not YUYV->RGB) I think it would be easier to check for
> RAW first and enumerate all the non-RAW pixfmt the ISC can generate
> from RAW, then if mbus_code is non RAW just check if it's in the list
> of supported formats and return the corresponding pixfmt. Or can the
> ISC conver between different colorspaces and I didn't get this ?
>
>> supported_index++;
>> }
>>
>> - return -EINVAL;
>> + /*
>> + * 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.
>> + */
>> + if (f->mbus_code && ISC_IS_FORMAT_RAW(f->mbus_code)) {
>
> Won't this be hijacked by the previous loop ?
>
> if (f->mbus_code) {
>
> }
>
> if (f->mbus_code && ISC_IS_FORMAT_RAW(f->mbus_code)) {
>
> }
>
>> + for (i = 0; i < isc->controller_formats_size; i++)
>> + if (!isc->controller_formats[i].raw) {
>
> if (isc->controller_format[i].raw)
> continue;
>
> To save an indentation level
>
>> + if (index == supported_index) {
>
> Should supported_index be re-initialized before this loop ?
>
>> + f->pixelformat =
>> + isc->controller_formats[i].fourcc;
>
> You are enumerating -all- formats except RAW, right ?
> This mean you can convert RAW to all other formats ?
>
>> + return 0;
>> + }
>> + supported_index++;
>> + }
>> + }
>> +
>> + /*
>> + * If we are asked a specific mbus_code, we have no more formats to
>> + * report
>> + * e.g. if the format is not raw, we cannot do any more processing.
>> + */
>> + if (f->mbus_code)
>> + return -EINVAL;
>
> As suggested, if my understanding is correct, I would enumerate all
> formats generated from a RAW mbus_code. If the mbus_code is not RAW
> then look if it's supported with isc_find_format_by_code() (and only
> index==0 is valid, unless there will be multiple entries with the same
> mbus_code in your format list, but isc_find_format_by_code() doesn't
> seem to support that).
Hi Jacopo,
I reworked this code using your suggestions.
Basically, I want to have three cases:
1/ input non-RAW (YUYV, RGB565) -> can output the same format, and be
done with it.
2/ input Raw -> enum all possible formats, but not all the Raw formats,
only the corresponding one.
3/ no input -> output all formats that we can output
You can have a look in v6 that I will send soon to cover these cases,
basically in the following form:
case 1: if we are not asking a specific mbus code, list everything we
support as output:
# v4l2-ctl -d /dev/video0 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
Type: Video Capture
[0]: 'AR12' (16-bit ARGB 4-4-4-4)
Size: Continuous 16x16 - 3264x2464
[1]: 'AR15' (16-bit ARGB 1-5-5-5)
Size: Continuous 16x16 - 3264x2464
[2]: 'RGBP' (16-bit RGB 5-6-5)
Size: Continuous 16x16 - 3264x2464
[3]: 'AR24' (32-bit BGRA 8-8-8-8)
Size: Continuous 16x16 - 3264x2464
[4]: 'XR24' (32-bit BGRX 8-8-8-8)
Size: Continuous 16x16 - 3264x2464
[5]: 'YU12' (Planar YUV 4:2:0)
Size: Continuous 16x16 - 3264x2464
[6]: 'UYVY' (UYVY 4:2:2)
Size: Continuous 16x16 - 3264x2464
[7]: 'VYUY' (VYUY 4:2:2)
Size: Continuous 16x16 - 3264x2464
[8]: 'YUYV' (YUYV 4:2:2)
Size: Continuous 16x16 - 3264x2464
[9]: '422P' (Planar YUV 4:2:2)
Size: Continuous 16x16 - 3264x2464
[10]: 'GREY' (8-bit Greyscale)
Size: Continuous 16x16 - 3264x2464
[11]: 'Y10 ' (10-bit Greyscale)
Size: Continuous 16x16 - 3264x2464
[12]: 'Y16 ' (16-bit Greyscale)
Size: Continuous 16x16 - 3264x2464
[13]: 'BA81' (8-bit Bayer BGBG/GRGR)
Size: Continuous 16x16 - 3264x2464
[14]: 'GBRG' (8-bit Bayer GBGB/RGRG)
Size: Continuous 16x16 - 3264x2464
[15]: 'GRBG' (8-bit Bayer GRGR/BGBG)
Size: Continuous 16x16 - 3264x2464
[16]: 'RGGB' (8-bit Bayer RGRG/GBGB)
Size: Continuous 16x16 - 3264x2464
[17]: 'BG10' (10-bit Bayer BGBG/GRGR)
Size: Continuous 16x16 - 3264x2464
[18]: 'GB10' (10-bit Bayer GBGB/RGRG)
Size: Continuous 16x16 - 3264x2464
[19]: 'BA10' (10-bit Bayer GRGR/BGBG)
Size: Continuous 16x16 - 3264x2464
[20]: 'RG10' (10-bit Bayer RGRG/GBGB)
Size: Continuous 16x16 - 3264x2464
[21]: 'BG12' (12-bit Bayer BGBG/GRGR)
Size: Continuous 16x16 - 3264x2464
[22]: 'GB12' (12-bit Bayer GBGB/RGRG)
Size: Continuous 16x16 - 3264x2464
[23]: 'BA12' (12-bit Bayer GRGR/BGBG)
Size: Continuous 16x16 - 3264x2464
[24]: 'RG12' (12-bit Bayer RGRG/GBGB)
Size: Continuous 16x16 - 3264x2464
case 2: if we are asked a specific mbus, but non-raw: search the
corresponding format with isc_find_format_by_code (which still has to
return NULL in case it's not supported, such that we can advertise
nothing ): (e.g. 0x2007 is not supported at all )
# v4l2-ctl -d /dev/video0 --list-formats-ext 0x2008
ioctl: VIDIOC_ENUM_FMT
Type: Video Capture
[0]: 'YUYV' (YUYV 4:2:2)
Size: Continuous 16x16 - 3264x2464
# v4l2-ctl -d /dev/video0 --list-formats-ext 0x2007
ioctl: VIDIOC_ENUM_FMT
Type: Video Capture
#
case 3: if we are asked a specific mbus, but raw: search the
corresponding format with isc_find_format_by_code , to find exactly
which corresponding raw format we have to advertise, and then, advertise
all the non-raw formats , because we can convert from RAW to everything
else:
# v4l2-ctl -d /dev/video0 --list-formats-ext 0x3001
ioctl: VIDIOC_ENUM_FMT
Type: Video Capture
[0]: 'BA81' (8-bit Bayer BGBG/GRGR)
Size: Continuous 16x16 - 3264x2464
[1]: 'AR12' (16-bit ARGB 4-4-4-4)
Size: Continuous 16x16 - 3264x2464
[2]: 'AR15' (16-bit ARGB 1-5-5-5)
Size: Continuous 16x16 - 3264x2464
[3]: 'RGBP' (16-bit RGB 5-6-5)
Size: Continuous 16x16 - 3264x2464
[4]: 'AR24' (32-bit BGRA 8-8-8-8)
Size: Continuous 16x16 - 3264x2464
[5]: 'XR24' (32-bit BGRX 8-8-8-8)
Size: Continuous 16x16 - 3264x2464
[6]: 'YU12' (Planar YUV 4:2:0)
Size: Continuous 16x16 - 3264x2464
[7]: 'UYVY' (UYVY 4:2:2)
Size: Continuous 16x16 - 3264x2464
[8]: 'VYUY' (VYUY 4:2:2)
Size: Continuous 16x16 - 3264x2464
[9]: 'YUYV' (YUYV 4:2:2)
Size: Continuous 16x16 - 3264x2464
[10]: '422P' (Planar YUV 4:2:2)
Size: Continuous 16x16 - 3264x2464
[11]: 'GREY' (8-bit Greyscale)
Size: Continuous 16x16 - 3264x2464
[12]: 'Y10 ' (10-bit Greyscale)
Size: Continuous 16x16 - 3264x2464
[13]: 'Y16 ' (16-bit Greyscale)
Size: Continuous 16x16 - 3264x2464
>
>> +
>> + /*
>> + * And finally, without a specific mbus_code,
>> + * we have to report all our formats.
>> + */
>> + if (index >= isc->controller_formats_size)
>> + return -EINVAL;
>> +
>> + f->pixelformat = isc->controller_formats[index].fourcc;
>> +
>> + return 0;
>> }
>>
>> static int isc_g_fmt_vid_cap(struct file *file, void *priv,
>> @@ -584,20 +608,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 +859,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 +880,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);
>
> I think when v4l2_subdev_call_state_active() will land it will
> probably simplify calling the subdev driver operations.
>
>> 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);
>
> This is done at try_format(). Is it intentionally repeated ?
Yes. It's reuse of that code, this time the second parameter is true,
because it's what I call a 'direct dump', e.g. YUYV directly to YUYV, no
processing.
>
>>
>> /* 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 +1037,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 +1092,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;
>> @@ -1782,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 = {
>> @@ -1836,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;
>>
>> @@ -1896,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");
>> @@ -2015,6 +1952,24 @@ int isc_pipeline_init(struct isc_device *isc)
>> }
>> EXPORT_SYMBOL_GPL(isc_pipeline_init);
>>
>> +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;
>> @@ -2022,6 +1977,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 0c1f49db47b4..b0200bb44f8a 100644
>> --- a/drivers/media/platform/atmel/atmel-isc-scaler.c
>> +++ b/drivers/media/platform/atmel/atmel-isc-scaler.c
>> @@ -171,6 +171,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,
>> };
>> @@ -188,6 +192,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 ac8c8679dd53..e2261ce99f4f 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
>> @@ -300,8 +301,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;
>> @@ -371,6 +371,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,
>
> Is the list zero-initialized ? Begin a static const I assume so (I
> should look at the C spec instead of asking :)
Yes this should be the case.
Thank you for your detailed review.
I will come up with v6 soon and it should behave like I described above.
Eugen
>
>> },
>> };
>>
>> diff --git a/drivers/media/platform/atmel/atmel-sama7g5-isc.c b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
>> index e07ae188c15f..a0d60cfdba7b 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
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2022-03-03 15:21 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-17 13:56 [PATCH v5 00/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 01/13] media: atmel: atmel-isc-base: use streaming status when queueing buffers Eugen Hristev
2022-02-23 17:05 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 02/13] media: atmel: atmel-isc-base: replace is_streaming call in s_fmt_vid_cap Eugen Hristev
2022-02-23 17:05 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 03/13] media: atmel: atmel-isc: remove redundant comments Eugen Hristev
2022-02-23 17:06 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 04/13] media: atmel: atmel-isc: implement media controller Eugen Hristev
2022-02-17 14:59 ` Eugen.Hristev
2022-02-23 16:34 ` Jacopo Mondi
2022-02-23 16:32 ` Jacopo Mondi
2022-03-01 8:56 ` Eugen.Hristev
2022-02-17 13:56 ` [PATCH v5 05/13] media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check Eugen Hristev
2022-02-23 17:06 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 06/13] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming Eugen Hristev
2022-02-23 17:07 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 07/13] media: atmel: atmel-isc: compact the controller formats list Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 08/13] media: atmel: atmel-isc: change format propagation to subdev into only verification Eugen Hristev
2022-02-23 17:03 ` Jacopo Mondi
2022-03-03 15:21 ` Eugen.Hristev
2022-02-17 13:56 ` [PATCH v5 09/13] media: atmel: atmel-sama7g5-isc: remove stray line Eugen Hristev
2022-02-23 17:07 ` Jacopo Mondi
2022-02-17 13:56 ` [PATCH v5 10/13] dt-bindings: media: microchip,xisc: add bus-width of 14 Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 11/13] ARM: dts: at91: sama7g5: add nodes for video capture Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 12/13] ARM: configs: at91: sama7: add xisc and csi2dc Eugen Hristev
2022-02-17 13:56 ` [PATCH v5 13/13] ARM: multi_v7_defconfig: add atmel video pipeline modules Eugen Hristev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).