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 its media device, and add the video entity to this
media device. The subdevices are registered to the same media device.
The ISC will have a base entity which is auto-detected as atmel_isc_base.
It will also register a subdevice that allows cropping of the incoming frame
to the maximum frame size supported by the ISC.
The ISC will create a link between the subdevice that is asynchronously
registered and the atmel_isc_scaler entity.
Then, the atmel_isc_scaler and atmel_isc_base are connected through another
link.
Signed-off-by: Eugen Hristev <[email protected]>
---
Changes in v6:
- reworked a bit as suggested by Jacopo
- add try crops
Changes in v5:
- reworked s_fmt to pass the same format from sink to source
- simplified enum_mbus_code
- separated tgt and bounds to report correctly in g_sel
Changes in v4:
As suggested by Jacopo:
- renamed atmel_isc_mc to atmel_isc_scaler.c
- moved init_mc/clean_mc to isc_base file
Changes in v2:
- implement try formats
drivers/media/platform/atmel/Makefile | 2 +-
drivers/media/platform/atmel/atmel-isc-base.c | 75 ++++-
.../media/platform/atmel/atmel-isc-scaler.c | 271 ++++++++++++++++++
drivers/media/platform/atmel/atmel-isc.h | 37 +++
.../media/platform/atmel/atmel-sama5d2-isc.c | 14 +-
.../media/platform/atmel/atmel-sama7g5-isc.c | 12 +-
6 files changed, 403 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..448bf281c61a 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,19 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
goto isc_async_complete_err;
}
+ ret = isc_scaler_link(isc);
+ if (ret < 0)
+ goto isc_async_complete_unregister_device;
+
+ ret = media_device_register(&isc->mdev);
+ if (ret < 0)
+ goto isc_async_complete_unregister_device;
+
return 0;
+isc_async_complete_unregister_device:
+ video_unregister_device(vdev);
+
isc_async_complete_err:
mutex_destroy(&isc->lock);
return ret;
@@ -1971,6 +1995,49 @@ int isc_pipeline_init(struct isc_device *isc)
}
EXPORT_SYMBOL_GPL(isc_pipeline_init);
+int isc_mc_init(struct isc_device *isc, u32 ver)
+{
+ const struct of_device_id *match;
+ int ret;
+
+ isc->video_dev.entity.function = MEDIA_ENT_F_IO_V4L;
+ isc->video_dev.entity.flags = MEDIA_ENT_FL_DEFAULT;
+ isc->pads[ISC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+
+ ret = media_entity_pads_init(&isc->video_dev.entity, ISC_PADS_NUM,
+ isc->pads);
+ if (ret < 0) {
+ dev_err(isc->dev, "media entity init failed\n");
+ return ret;
+ }
+
+ isc->mdev.dev = isc->dev;
+
+ match = of_match_node(isc->dev->driver->of_match_table,
+ isc->dev->of_node);
+
+ strscpy(isc->mdev.driver_name, KBUILD_MODNAME,
+ sizeof(isc->mdev.driver_name));
+ strscpy(isc->mdev.model, match->compatible, sizeof(isc->mdev.model));
+ snprintf(isc->mdev.bus_info, sizeof(isc->mdev.bus_info), "platform:%s",
+ isc->v4l2_dev.name);
+ isc->mdev.hw_revision = ver;
+
+ media_device_init(&isc->mdev);
+
+ isc->v4l2_dev.mdev = &isc->mdev;
+
+ return isc_scaler_init(isc);
+}
+EXPORT_SYMBOL_GPL(isc_mc_init);
+
+void isc_mc_cleanup(struct isc_device *isc)
+{
+ media_entity_cleanup(&isc->video_dev.entity);
+ media_device_cleanup(&isc->mdev);
+}
+EXPORT_SYMBOL_GPL(isc_mc_cleanup);
+
/* regmap configuration */
#define ATMEL_ISC_REG_MAX 0xd5c
const struct regmap_config isc_regmap_config = {
diff --git a/drivers/media/platform/atmel/atmel-isc-scaler.c b/drivers/media/platform/atmel/atmel-isc-scaler.c
new file mode 100644
index 000000000000..305524c1123e
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc-scaler.c
@@ -0,0 +1,271 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Microchip Image Sensor Controller (ISC) Scaler entity support
+ *
+ * Copyright (C) 2022 Microchip Technology, Inc.
+ *
+ * Author: Eugen Hristev <[email protected]>
+ *
+ */
+
+#include <media/media-device.h>
+#include <media/media-entity.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+
+#include "atmel-isc-regs.h"
+#include "atmel-isc.h"
+
+static void isc_scaler_prepare_fmt(struct v4l2_mbus_framefmt *framefmt)
+{
+ framefmt->colorspace = V4L2_COLORSPACE_SRGB;
+ framefmt->field = V4L2_FIELD_NONE;
+ framefmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+ framefmt->quantization = V4L2_QUANTIZATION_DEFAULT;
+ framefmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
+};
+
+static int isc_scaler_get_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_format *format)
+{
+ struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
+ struct v4l2_mbus_framefmt *v4l2_try_fmt;
+
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+ v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
+ format->pad);
+ format->format = *v4l2_try_fmt;
+
+ return 0;
+ }
+
+ format->format = isc->scaler_format[format->pad];
+
+ return 0;
+}
+
+static int isc_scaler_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_format *req_fmt)
+{
+ struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
+ struct v4l2_mbus_framefmt *v4l2_try_fmt;
+ struct isc_format *fmt;
+ unsigned int i;
+
+ /* Source format is fixed, we cannot change it */
+ if (req_fmt->pad == ISC_SCALER_PAD_SOURCE) {
+ req_fmt->format = isc->scaler_format[ISC_SCALER_PAD_SOURCE];
+ return 0;
+ }
+
+ /* There is no limit on the frame size on the sink pad */
+ v4l_bound_align_image(&req_fmt->format.width, 16, UINT_MAX, 0,
+ &req_fmt->format.height, 16, UINT_MAX, 0, 0);
+
+ isc_scaler_prepare_fmt(&req_fmt->format);
+
+ fmt = isc_find_format_by_code(isc, req_fmt->format.code, &i);
+
+ if (!fmt)
+ fmt = &isc->formats_list[0];
+
+ req_fmt->format.code = fmt->mbus_code;
+
+ if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+ v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
+ req_fmt->pad);
+ *v4l2_try_fmt = req_fmt->format;
+ /* Trying on the sink pad makes the source pad change too */
+ v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
+ ISC_SCALER_PAD_SOURCE);
+ *v4l2_try_fmt = req_fmt->format;
+
+ v4l_bound_align_image(&v4l2_try_fmt->width,
+ 16, isc->max_width, 0,
+ &v4l2_try_fmt->height,
+ 16, isc->max_height, 0, 0);
+ /* if we are just trying, we are done */
+ return 0;
+ }
+
+ isc->scaler_format[ISC_SCALER_PAD_SINK] = req_fmt->format;
+
+ /* The source pad is the same as the sink, but we have to crop it */
+ isc->scaler_format[ISC_SCALER_PAD_SOURCE] =
+ isc->scaler_format[ISC_SCALER_PAD_SINK];
+ v4l_bound_align_image
+ (&isc->scaler_format[ISC_SCALER_PAD_SOURCE].width, 16,
+ isc->max_width, 0,
+ &isc->scaler_format[ISC_SCALER_PAD_SOURCE].height, 16,
+ isc->max_height, 0, 0);
+
+ return 0;
+}
+
+static int isc_scaler_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
+
+ /*
+ * All formats supported by the ISC are supported by the scaler.
+ * Advertise the formats which the ISC can take as input, as the scaler
+ * entity cropping is part of the PFE module (parallel front end)
+ */
+ if (code->index < isc->formats_list_size) {
+ code->code = isc->formats_list[code->index].mbus_code;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static int isc_scaler_g_sel(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_selection *sel)
+{
+ struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
+
+ if (sel->pad == ISC_SCALER_PAD_SOURCE)
+ return -EINVAL;
+
+ if (sel->target == V4L2_SEL_TGT_CROP_BOUNDS) {
+ /* bounds are the input format received */
+ sel->r.height = isc->scaler_format[ISC_SCALER_PAD_SINK].height;
+ sel->r.width = isc->scaler_format[ISC_SCALER_PAD_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[ISC_SCALER_PAD_SOURCE].height;
+ sel->r.width = isc->scaler_format[ISC_SCALER_PAD_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 v4l2_rect *try_crop;
+ struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
+
+ *v4l2_try_fmt = isc->scaler_format[ISC_SCALER_PAD_SOURCE];
+
+ try_crop = v4l2_subdev_get_try_crop(sd, sd_state, 0);
+
+ try_crop->top = 0;
+ try_crop->left = 0;
+ try_crop->width = v4l2_try_fmt->width;
+ try_crop->height = v4l2_try_fmt->height;
+
+ return 0;
+}
+
+static const struct v4l2_subdev_pad_ops isc_scaler_pad_ops = {
+ .enum_mbus_code = isc_scaler_enum_mbus_code,
+ .set_fmt = isc_scaler_set_fmt,
+ .get_fmt = isc_scaler_get_fmt,
+ .get_selection = isc_scaler_g_sel,
+ .init_cfg = isc_scaler_init_cfg,
+};
+
+static const struct v4l2_subdev_ops xisc_scaler_subdev_ops = {
+ .pad = &isc_scaler_pad_ops,
+};
+
+int isc_scaler_init(struct isc_device *isc)
+{
+ int ret;
+
+ v4l2_subdev_init(&isc->scaler_sd, &xisc_scaler_subdev_ops);
+
+ isc->scaler_sd.owner = THIS_MODULE;
+ isc->scaler_sd.dev = isc->dev;
+ snprintf(isc->scaler_sd.name, sizeof(isc->scaler_sd.name),
+ "atmel_isc_scaler");
+
+ isc->scaler_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ isc->scaler_sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
+ isc->scaler_pads[ISC_SCALER_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+ isc->scaler_pads[ISC_SCALER_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+
+ isc_scaler_prepare_fmt(&isc->scaler_format[ISC_SCALER_PAD_SOURCE]);
+ isc->scaler_format[ISC_SCALER_PAD_SOURCE].height = isc->max_height;
+ isc->scaler_format[ISC_SCALER_PAD_SOURCE].width = isc->max_width;
+ isc->scaler_format[ISC_SCALER_PAD_SOURCE].code =
+ isc->formats_list[0].mbus_code;
+
+ isc->scaler_format[ISC_SCALER_PAD_SINK] =
+ isc->scaler_format[ISC_SCALER_PAD_SOURCE];
+
+ ret = media_entity_pads_init(&isc->scaler_sd.entity,
+ ISC_SCALER_PADS_NUM,
+ isc->scaler_pads);
+ if (ret < 0) {
+ dev_err(isc->dev, "scaler sd media entity init failed\n");
+ return ret;
+ }
+
+ ret = v4l2_device_register_subdev(&isc->v4l2_dev, &isc->scaler_sd);
+ if (ret < 0) {
+ dev_err(isc->dev, "scaler sd failed to register subdev\n");
+ return ret;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(isc_scaler_init);
+
+int isc_scaler_link(struct isc_device *isc)
+{
+ int ret;
+
+ ret = media_create_pad_link(&isc->current_subdev->sd->entity,
+ isc->remote_pad, &isc->scaler_sd.entity,
+ ISC_SCALER_PAD_SINK,
+ MEDIA_LNK_FL_ENABLED |
+ MEDIA_LNK_FL_IMMUTABLE);
+
+ if (ret < 0) {
+ dev_err(isc->dev, "Failed to create pad link: %s to %s\n",
+ isc->current_subdev->sd->entity.name,
+ isc->scaler_sd.entity.name);
+ return ret;
+ }
+
+ dev_dbg(isc->dev, "link with %s pad: %d\n",
+ isc->current_subdev->sd->name, isc->remote_pad);
+
+ ret = media_create_pad_link(&isc->scaler_sd.entity,
+ ISC_SCALER_PAD_SOURCE,
+ &isc->video_dev.entity, ISC_PAD_SINK,
+ MEDIA_LNK_FL_ENABLED |
+ MEDIA_LNK_FL_IMMUTABLE);
+
+ if (ret < 0) {
+ dev_err(isc->dev, "Failed to create pad link: %s to %s\n",
+ isc->scaler_sd.entity.name,
+ isc->video_dev.entity.name);
+ return ret;
+ }
+
+ dev_dbg(isc->dev, "link with %s pad: %d\n", isc->scaler_sd.name,
+ ISC_SCALER_PAD_SOURCE);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(isc_scaler_link);
+
diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
index f9ad7ec6bd13..9cc69c3ae26d 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,12 @@ struct isc_reg_offsets {
* be used as an input to the controller
* @controller_formats_size: size of controller_formats array
* @formats_list_size: size of formats_list array
+ * @pads: media controller pads for isc video entity
+ * @mdev: media device that is registered by the isc
+ * @remote_pad: remote pad on the connected subdevice
+ * @scaler_sd: subdevice for the scaler that isc registers
+ * @scaler_pads: media controller pads for the scaler subdevice
+ * @scaler_format: current format for the scaler subdevice
*/
struct isc_device {
struct regmap *regmap;
@@ -346,6 +363,19 @@ struct isc_device {
struct isc_format *formats_list;
u32 controller_formats_size;
u32 formats_list_size;
+
+ struct {
+ struct media_pad pads[ISC_PADS_NUM];
+ struct media_device mdev;
+
+ u32 remote_pad;
+ };
+
+ struct {
+ struct v4l2_subdev scaler_sd;
+ struct media_pad scaler_pads[ISC_SCALER_PADS_NUM];
+ struct v4l2_mbus_framefmt scaler_format[ISC_SCALER_PADS_NUM];
+ };
};
extern const struct regmap_config isc_regmap_config;
@@ -357,4 +387,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
Hi Eugen
On Thu, Mar 03, 2022 at 05:36:09PM +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 its media device, and add the video entity to this
> media device. The subdevices are registered to the same media device.
> The ISC will have a base entity which is auto-detected as atmel_isc_base.
> It will also register a subdevice that allows cropping of the incoming frame
> to the maximum frame size supported by the ISC.
> The ISC will create a link between the subdevice that is asynchronously
> registered and the atmel_isc_scaler entity.
> Then, the atmel_isc_scaler and atmel_isc_base are connected through another
> link.
>
> Signed-off-by: Eugen Hristev <[email protected]>
> ---
> Changes in v6:
> - reworked a bit as suggested by Jacopo
> - add try crops
>
> Changes in v5:
> - reworked s_fmt to pass the same format from sink to source
> - simplified enum_mbus_code
> - separated tgt and bounds to report correctly in g_sel
>
> Changes in v4:
> As suggested by Jacopo:
> - renamed atmel_isc_mc to atmel_isc_scaler.c
> - moved init_mc/clean_mc to isc_base file
>
> Changes in v2:
> - implement try formats
>
> drivers/media/platform/atmel/Makefile | 2 +-
> drivers/media/platform/atmel/atmel-isc-base.c | 75 ++++-
> .../media/platform/atmel/atmel-isc-scaler.c | 271 ++++++++++++++++++
> drivers/media/platform/atmel/atmel-isc.h | 37 +++
> .../media/platform/atmel/atmel-sama5d2-isc.c | 14 +-
> .../media/platform/atmel/atmel-sama7g5-isc.c | 12 +-
> 6 files changed, 403 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..448bf281c61a 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,19 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
> goto isc_async_complete_err;
> }
>
> + ret = isc_scaler_link(isc);
> + if (ret < 0)
> + goto isc_async_complete_unregister_device;
> +
> + ret = media_device_register(&isc->mdev);
> + if (ret < 0)
> + goto isc_async_complete_unregister_device;
> +
> return 0;
>
> +isc_async_complete_unregister_device:
> + video_unregister_device(vdev);
> +
> isc_async_complete_err:
> mutex_destroy(&isc->lock);
> return ret;
> @@ -1971,6 +1995,49 @@ int isc_pipeline_init(struct isc_device *isc)
> }
> EXPORT_SYMBOL_GPL(isc_pipeline_init);
>
> +int isc_mc_init(struct isc_device *isc, u32 ver)
> +{
> + const struct of_device_id *match;
> + int ret;
> +
> + isc->video_dev.entity.function = MEDIA_ENT_F_IO_V4L;
> + isc->video_dev.entity.flags = MEDIA_ENT_FL_DEFAULT;
> + isc->pads[ISC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> +
> + ret = media_entity_pads_init(&isc->video_dev.entity, ISC_PADS_NUM,
> + isc->pads);
> + if (ret < 0) {
> + dev_err(isc->dev, "media entity init failed\n");
> + return ret;
> + }
> +
> + isc->mdev.dev = isc->dev;
> +
> + match = of_match_node(isc->dev->driver->of_match_table,
> + isc->dev->of_node);
> +
> + strscpy(isc->mdev.driver_name, KBUILD_MODNAME,
> + sizeof(isc->mdev.driver_name));
> + strscpy(isc->mdev.model, match->compatible, sizeof(isc->mdev.model));
> + snprintf(isc->mdev.bus_info, sizeof(isc->mdev.bus_info), "platform:%s",
> + isc->v4l2_dev.name);
> + isc->mdev.hw_revision = ver;
> +
> + media_device_init(&isc->mdev);
> +
> + isc->v4l2_dev.mdev = &isc->mdev;
> +
> + return isc_scaler_init(isc);
> +}
> +EXPORT_SYMBOL_GPL(isc_mc_init);
> +
> +void isc_mc_cleanup(struct isc_device *isc)
> +{
> + media_entity_cleanup(&isc->video_dev.entity);
> + media_device_cleanup(&isc->mdev);
> +}
> +EXPORT_SYMBOL_GPL(isc_mc_cleanup);
> +
> /* regmap configuration */
> #define ATMEL_ISC_REG_MAX 0xd5c
> const struct regmap_config isc_regmap_config = {
> diff --git a/drivers/media/platform/atmel/atmel-isc-scaler.c b/drivers/media/platform/atmel/atmel-isc-scaler.c
> new file mode 100644
> index 000000000000..305524c1123e
> --- /dev/null
> +++ b/drivers/media/platform/atmel/atmel-isc-scaler.c
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Microchip Image Sensor Controller (ISC) Scaler entity support
> + *
> + * Copyright (C) 2022 Microchip Technology, Inc.
> + *
> + * Author: Eugen Hristev <[email protected]>
> + *
> + */
> +
> +#include <media/media-device.h>
> +#include <media/media-entity.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +
> +#include "atmel-isc-regs.h"
> +#include "atmel-isc.h"
> +
> +static void isc_scaler_prepare_fmt(struct v4l2_mbus_framefmt *framefmt)
> +{
> + framefmt->colorspace = V4L2_COLORSPACE_SRGB;
> + framefmt->field = V4L2_FIELD_NONE;
> + framefmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + framefmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> + framefmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +};
> +
> +static int isc_scaler_get_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *format)
> +{
> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
> + struct v4l2_mbus_framefmt *v4l2_try_fmt;
> +
> + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> + v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
> + format->pad);
> + format->format = *v4l2_try_fmt;
> +
> + return 0;
> + }
> +
> + format->format = isc->scaler_format[format->pad];
> +
> + return 0;
> +}
> +
> +static int isc_scaler_set_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *req_fmt)
> +{
> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
> + struct v4l2_mbus_framefmt *v4l2_try_fmt;
> + struct isc_format *fmt;
> + unsigned int i;
> +
> + /* Source format is fixed, we cannot change it */
> + if (req_fmt->pad == ISC_SCALER_PAD_SOURCE) {
> + req_fmt->format = isc->scaler_format[ISC_SCALER_PAD_SOURCE];
> + return 0;
> + }
> +
> + /* There is no limit on the frame size on the sink pad */
> + v4l_bound_align_image(&req_fmt->format.width, 16, UINT_MAX, 0,
> + &req_fmt->format.height, 16, UINT_MAX, 0, 0);
> +
> + isc_scaler_prepare_fmt(&req_fmt->format);
> +
> + fmt = isc_find_format_by_code(isc, req_fmt->format.code, &i);
> +
> + if (!fmt)
> + fmt = &isc->formats_list[0];
> +
> + req_fmt->format.code = fmt->mbus_code;
> +
> + if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> + v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
> + req_fmt->pad);
> + *v4l2_try_fmt = req_fmt->format;
> + /* Trying on the sink pad makes the source pad change too */
> + v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
> + ISC_SCALER_PAD_SOURCE);
> + *v4l2_try_fmt = req_fmt->format;
> +
> + v4l_bound_align_image(&v4l2_try_fmt->width,
> + 16, isc->max_width, 0,
> + &v4l2_try_fmt->height,
> + 16, isc->max_height, 0, 0);
> + /* if we are just trying, we are done */
> + return 0;
> + }
> +
> + isc->scaler_format[ISC_SCALER_PAD_SINK] = req_fmt->format;
> +
> + /* The source pad is the same as the sink, but we have to crop it */
> + isc->scaler_format[ISC_SCALER_PAD_SOURCE] =
> + isc->scaler_format[ISC_SCALER_PAD_SINK];
> + v4l_bound_align_image
> + (&isc->scaler_format[ISC_SCALER_PAD_SOURCE].width, 16,
> + isc->max_width, 0,
> + &isc->scaler_format[ISC_SCALER_PAD_SOURCE].height, 16,
> + isc->max_height, 0, 0);
> +
> + return 0;
> +}
> +
> +static int isc_scaler_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
> +
> + /*
> + * All formats supported by the ISC are supported by the scaler.
> + * Advertise the formats which the ISC can take as input, as the scaler
> + * entity cropping is part of the PFE module (parallel front end)
> + */
> + if (code->index < isc->formats_list_size) {
> + code->code = isc->formats_list[code->index].mbus_code;
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int isc_scaler_g_sel(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_selection *sel)
> +{
> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
> +
> + if (sel->pad == ISC_SCALER_PAD_SOURCE)
> + return -EINVAL;
> +
> + if (sel->target == V4L2_SEL_TGT_CROP_BOUNDS) {
> + /* bounds are the input format received */
> + sel->r.height = isc->scaler_format[ISC_SCALER_PAD_SINK].height;
> + sel->r.width = isc->scaler_format[ISC_SCALER_PAD_SINK].width;
I'll re-paste our discussion on v4 to make sure we're on the same page
-------------------------------------------------------------------------------
> >> + if (sel->pad == ISC_SCALER_PAD_SOURCE)
> >> + return -EINVAL;
> >> +
> >> + if (sel->target != V4L2_SEL_TGT_CROP_BOUNDS &&
> >> + sel->target != V4L2_SEL_TGT_CROP)
> >> + return -EINVAL;
> >> +
> >> + sel->r.height = isc->max_height;
> >> + sel->r.width = isc->max_width;
> >
> > The CROP_BOUNDS should be set to the same size as the sink pad image format,
> > as it represents the maximum valid crop rectangle.
> >
> > TGT_CROP should report the configured crop rectangle which can be
> > intiialized to the same size as CROP_BOUNDS, if my understanding of
> > the spec is correct
>
> So you would like to have this differentiated, and report the
> CROP_BOUNDS to whatever is on the sink pad, and the TGT_CROP to what is
> reported now, the maximum size of the ISC frame .
> My understanding is correct ?
>
I didn't know you have an HW limitation, so your _BOUNDS is not the
input image size but rather 3264x2464 ( == max_width x max_height).
What I meant is that _BOUNDS should report the maximum rectangle size
that can be applied to the _CROP target. In you case you have an HW
limitation 3264x2464 and that's the largest rectangle you can apply.
TGT_CROP can be initialized to the same as _BOUND, but if you
implement s_selection it should report what has been there applied.
But as you don't implement s_selection yet, I think this is fine for
now. Maybe a little comment ?
-------------------------------------------------------------------------------
I think then that _BOUNDS should be fixed to 3264x2464 and CROP to
src_fmt.
The rest looks good! Thanks for pushing up to this version and thanks
for addressings comments!
Reviewed-by: Jacopo Mondi <[email protected]>
Thanks
j
> + 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[ISC_SCALER_PAD_SOURCE].height;
> + sel->r.width = isc->scaler_format[ISC_SCALER_PAD_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 v4l2_rect *try_crop;
> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
> +
> + *v4l2_try_fmt = isc->scaler_format[ISC_SCALER_PAD_SOURCE];
> +
> + try_crop = v4l2_subdev_get_try_crop(sd, sd_state, 0);
> +
> + try_crop->top = 0;
> + try_crop->left = 0;
> + try_crop->width = v4l2_try_fmt->width;
> + try_crop->height = v4l2_try_fmt->height;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_pad_ops isc_scaler_pad_ops = {
> + .enum_mbus_code = isc_scaler_enum_mbus_code,
> + .set_fmt = isc_scaler_set_fmt,
> + .get_fmt = isc_scaler_get_fmt,
> + .get_selection = isc_scaler_g_sel,
> + .init_cfg = isc_scaler_init_cfg,
> +};
> +
> +static const struct v4l2_subdev_ops xisc_scaler_subdev_ops = {
> + .pad = &isc_scaler_pad_ops,
> +};
> +
> +int isc_scaler_init(struct isc_device *isc)
> +{
> + int ret;
> +
> + v4l2_subdev_init(&isc->scaler_sd, &xisc_scaler_subdev_ops);
> +
> + isc->scaler_sd.owner = THIS_MODULE;
> + isc->scaler_sd.dev = isc->dev;
> + snprintf(isc->scaler_sd.name, sizeof(isc->scaler_sd.name),
> + "atmel_isc_scaler");
> +
> + isc->scaler_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + isc->scaler_sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
> + isc->scaler_pads[ISC_SCALER_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> + isc->scaler_pads[ISC_SCALER_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +
> + isc_scaler_prepare_fmt(&isc->scaler_format[ISC_SCALER_PAD_SOURCE]);
> + isc->scaler_format[ISC_SCALER_PAD_SOURCE].height = isc->max_height;
> + isc->scaler_format[ISC_SCALER_PAD_SOURCE].width = isc->max_width;
> + isc->scaler_format[ISC_SCALER_PAD_SOURCE].code =
> + isc->formats_list[0].mbus_code;
> +
> + isc->scaler_format[ISC_SCALER_PAD_SINK] =
> + isc->scaler_format[ISC_SCALER_PAD_SOURCE];
> +
> + ret = media_entity_pads_init(&isc->scaler_sd.entity,
> + ISC_SCALER_PADS_NUM,
> + isc->scaler_pads);
> + if (ret < 0) {
> + dev_err(isc->dev, "scaler sd media entity init failed\n");
> + return ret;
> + }
> +
> + ret = v4l2_device_register_subdev(&isc->v4l2_dev, &isc->scaler_sd);
> + if (ret < 0) {
> + dev_err(isc->dev, "scaler sd failed to register subdev\n");
> + return ret;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(isc_scaler_init);
> +
> +int isc_scaler_link(struct isc_device *isc)
> +{
> + int ret;
> +
> + ret = media_create_pad_link(&isc->current_subdev->sd->entity,
> + isc->remote_pad, &isc->scaler_sd.entity,
> + ISC_SCALER_PAD_SINK,
> + MEDIA_LNK_FL_ENABLED |
> + MEDIA_LNK_FL_IMMUTABLE);
> +
> + if (ret < 0) {
> + dev_err(isc->dev, "Failed to create pad link: %s to %s\n",
> + isc->current_subdev->sd->entity.name,
> + isc->scaler_sd.entity.name);
> + return ret;
> + }
> +
> + dev_dbg(isc->dev, "link with %s pad: %d\n",
> + isc->current_subdev->sd->name, isc->remote_pad);
> +
> + ret = media_create_pad_link(&isc->scaler_sd.entity,
> + ISC_SCALER_PAD_SOURCE,
> + &isc->video_dev.entity, ISC_PAD_SINK,
> + MEDIA_LNK_FL_ENABLED |
> + MEDIA_LNK_FL_IMMUTABLE);
> +
> + if (ret < 0) {
> + dev_err(isc->dev, "Failed to create pad link: %s to %s\n",
> + isc->scaler_sd.entity.name,
> + isc->video_dev.entity.name);
> + return ret;
> + }
> +
> + dev_dbg(isc->dev, "link with %s pad: %d\n", isc->scaler_sd.name,
> + ISC_SCALER_PAD_SOURCE);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(isc_scaler_link);
> +
> diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
> index f9ad7ec6bd13..9cc69c3ae26d 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,12 @@ struct isc_reg_offsets {
> * be used as an input to the controller
> * @controller_formats_size: size of controller_formats array
> * @formats_list_size: size of formats_list array
> + * @pads: media controller pads for isc video entity
> + * @mdev: media device that is registered by the isc
> + * @remote_pad: remote pad on the connected subdevice
> + * @scaler_sd: subdevice for the scaler that isc registers
> + * @scaler_pads: media controller pads for the scaler subdevice
> + * @scaler_format: current format for the scaler subdevice
> */
> struct isc_device {
> struct regmap *regmap;
> @@ -346,6 +363,19 @@ struct isc_device {
> struct isc_format *formats_list;
> u32 controller_formats_size;
> u32 formats_list_size;
> +
> + struct {
> + struct media_pad pads[ISC_PADS_NUM];
> + struct media_device mdev;
> +
> + u32 remote_pad;
> + };
> +
> + struct {
> + struct v4l2_subdev scaler_sd;
> + struct media_pad scaler_pads[ISC_SCALER_PADS_NUM];
> + struct v4l2_mbus_framefmt scaler_format[ISC_SCALER_PADS_NUM];
> + };
> };
>
> extern const struct regmap_config isc_regmap_config;
> @@ -357,4 +387,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
>
[snip]
>>> +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[ISC_SCALER_PAD_SINK].height;
>>> + sel->r.width = isc->scaler_format[ISC_SCALER_PAD_SINK].width;
>>
>> I'll re-paste our discussion on v4 to make sure we're on the same page
>>
>> -------------------------------------------------------------------------------
>>>>> + if (sel->pad == ISC_SCALER_PAD_SOURCE)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + if (sel->target != V4L2_SEL_TGT_CROP_BOUNDS &&
>>>>> + sel->target != V4L2_SEL_TGT_CROP)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + sel->r.height = isc->max_height;
>>>>> + sel->r.width = isc->max_width;
>>>>
>>>> The CROP_BOUNDS should be set to the same size as the sink pad image format,
>>>> as it represents the maximum valid crop rectangle.
>>>>
>>>> TGT_CROP should report the configured crop rectangle which can be
>>>> intiialized to the same size as CROP_BOUNDS, if my understanding of
>>>> the spec is correct
>>>
>>> So you would like to have this differentiated, and report the
>>> CROP_BOUNDS to whatever is on the sink pad, and the TGT_CROP to what is
>>> reported now, the maximum size of the ISC frame .
>>> My understanding is correct ?
>>>
>>
>> I didn't know you have an HW limitation, so your _BOUNDS is not the
>> input image size but rather 3264x2464 ( == max_width x max_height).
>>
>> What I meant is that _BOUNDS should report the maximum rectangle size
>> that can be applied to the _CROP target. In you case you have an HW
>> limitation 3264x2464 and that's the largest rectangle you can apply.
>> TGT_CROP can be initialized to the same as _BOUND, but if you
>> implement s_selection it should report what has been there applied.
>> But as you don't implement s_selection yet, I think this is fine for
>> now. Maybe a little comment ?
>> -------------------------------------------------------------------------------
>>
>> I think then that _BOUNDS should be fixed to 3264x2464 and CROP to
>> src_fmt.
>>
>> The rest looks good! Thanks for pushing up to this version and thanks
>> for addressings comments!
>>
>> Reviewed-by: Jacopo Mondi <[email protected]>
>>
>> Thanks
>> j
>
> Hello Jacopo,
>
> So BOUNDS goes to isc max H / max V, and CROP goes to format of the
> source pad.
> Got it.
>
> I will update with a v7 as soon as you give me your feedback on the
> patch 8/13 which still required some changes from v6.
>
> Thanks again for reviewing this !
With the incoming v7, media-ctl -p now looks like this:
Device topology
- entity 1: atmel_isc_scaler (2 pads, 2 links)
type V4L2 subdev subtype Unknown flags 0
device node name /dev/v4l-subdev0
pad0: Sink
[fmt:SRGGB10_1X10/3280x2464 field:none colorspace:srgb
crop.bounds:(0,0)/3264x2464
crop:(0,0)/3264x2464]
<- "csi2dc":1 [ENABLED,IMMUTABLE]
pad1: Source
[fmt:SRGGB10_1X10/3264x2464 field:none colorspace:srgb]
-> "atmel_isc_common":0 [ENABLED,IMMUTABLE]
Bounds as maximum isc size: 3264x2464, crop as maximum isc size 3264x2464
In case e.g. the sensor is sending a stream at lower res, like
1920x1080, it will keep the same bounds:
Device topology
- entity 1: atmel_isc_scaler (2 pads, 2 links)
type V4L2 subdev subtype Unknown flags 0
device node name /dev/v4l-subdev0
pad0: Sink
[fmt:SRGGB10_1X10/1920x1080 field:none colorspace:srgb
crop.bounds:(0,0)/3264x2464
crop:(0,0)/1920x1080]
<- "csi2dc":1 [ENABLED,IMMUTABLE]
pad1: Source
[fmt:SRGGB10_1X10/1920x1080 field:none colorspace:srgb]
-> "atmel_isc_common":0 [ENABLED,IMMUTABLE]
>
> Eugen
>
[snip]
On 3/7/22 10:46 AM, Jacopo Mondi wrote:
> Hi Eugen
>
> On Thu, Mar 03, 2022 at 05:36:09PM +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 its media device, and add the video entity to this
>> media device. The subdevices are registered to the same media device.
>> The ISC will have a base entity which is auto-detected as atmel_isc_base.
>> It will also register a subdevice that allows cropping of the incoming frame
>> to the maximum frame size supported by the ISC.
>> The ISC will create a link between the subdevice that is asynchronously
>> registered and the atmel_isc_scaler entity.
>> Then, the atmel_isc_scaler and atmel_isc_base are connected through another
>> link.
>>
>> Signed-off-by: Eugen Hristev <[email protected]>
>> ---
>> Changes in v6:
>> - reworked a bit as suggested by Jacopo
>> - add try crops
>>
>> Changes in v5:
>> - reworked s_fmt to pass the same format from sink to source
>> - simplified enum_mbus_code
>> - separated tgt and bounds to report correctly in g_sel
>>
>> Changes in v4:
>> As suggested by Jacopo:
>> - renamed atmel_isc_mc to atmel_isc_scaler.c
>> - moved init_mc/clean_mc to isc_base file
>>
>> Changes in v2:
>> - implement try formats
>>
>> drivers/media/platform/atmel/Makefile | 2 +-
>> drivers/media/platform/atmel/atmel-isc-base.c | 75 ++++-
>> .../media/platform/atmel/atmel-isc-scaler.c | 271 ++++++++++++++++++
>> drivers/media/platform/atmel/atmel-isc.h | 37 +++
>> .../media/platform/atmel/atmel-sama5d2-isc.c | 14 +-
>> .../media/platform/atmel/atmel-sama7g5-isc.c | 12 +-
>> 6 files changed, 403 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..448bf281c61a 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,19 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>> goto isc_async_complete_err;
>> }
>>
>> + ret = isc_scaler_link(isc);
>> + if (ret < 0)
>> + goto isc_async_complete_unregister_device;
>> +
>> + ret = media_device_register(&isc->mdev);
>> + if (ret < 0)
>> + goto isc_async_complete_unregister_device;
>> +
>> return 0;
>>
>> +isc_async_complete_unregister_device:
>> + video_unregister_device(vdev);
>> +
>> isc_async_complete_err:
>> mutex_destroy(&isc->lock);
>> return ret;
>> @@ -1971,6 +1995,49 @@ int isc_pipeline_init(struct isc_device *isc)
>> }
>> EXPORT_SYMBOL_GPL(isc_pipeline_init);
>>
>> +int isc_mc_init(struct isc_device *isc, u32 ver)
>> +{
>> + const struct of_device_id *match;
>> + int ret;
>> +
>> + isc->video_dev.entity.function = MEDIA_ENT_F_IO_V4L;
>> + isc->video_dev.entity.flags = MEDIA_ENT_FL_DEFAULT;
>> + isc->pads[ISC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>> +
>> + ret = media_entity_pads_init(&isc->video_dev.entity, ISC_PADS_NUM,
>> + isc->pads);
>> + if (ret < 0) {
>> + dev_err(isc->dev, "media entity init failed\n");
>> + return ret;
>> + }
>> +
>> + isc->mdev.dev = isc->dev;
>> +
>> + match = of_match_node(isc->dev->driver->of_match_table,
>> + isc->dev->of_node);
>> +
>> + strscpy(isc->mdev.driver_name, KBUILD_MODNAME,
>> + sizeof(isc->mdev.driver_name));
>> + strscpy(isc->mdev.model, match->compatible, sizeof(isc->mdev.model));
>> + snprintf(isc->mdev.bus_info, sizeof(isc->mdev.bus_info), "platform:%s",
>> + isc->v4l2_dev.name);
>> + isc->mdev.hw_revision = ver;
>> +
>> + media_device_init(&isc->mdev);
>> +
>> + isc->v4l2_dev.mdev = &isc->mdev;
>> +
>> + return isc_scaler_init(isc);
>> +}
>> +EXPORT_SYMBOL_GPL(isc_mc_init);
>> +
>> +void isc_mc_cleanup(struct isc_device *isc)
>> +{
>> + media_entity_cleanup(&isc->video_dev.entity);
>> + media_device_cleanup(&isc->mdev);
>> +}
>> +EXPORT_SYMBOL_GPL(isc_mc_cleanup);
>> +
>> /* regmap configuration */
>> #define ATMEL_ISC_REG_MAX 0xd5c
>> const struct regmap_config isc_regmap_config = {
>> diff --git a/drivers/media/platform/atmel/atmel-isc-scaler.c b/drivers/media/platform/atmel/atmel-isc-scaler.c
>> new file mode 100644
>> index 000000000000..305524c1123e
>> --- /dev/null
>> +++ b/drivers/media/platform/atmel/atmel-isc-scaler.c
>> @@ -0,0 +1,271 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Microchip Image Sensor Controller (ISC) Scaler entity support
>> + *
>> + * Copyright (C) 2022 Microchip Technology, Inc.
>> + *
>> + * Author: Eugen Hristev <[email protected]>
>> + *
>> + */
>> +
>> +#include <media/media-device.h>
>> +#include <media/media-entity.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-subdev.h>
>> +
>> +#include "atmel-isc-regs.h"
>> +#include "atmel-isc.h"
>> +
>> +static void isc_scaler_prepare_fmt(struct v4l2_mbus_framefmt *framefmt)
>> +{
>> + framefmt->colorspace = V4L2_COLORSPACE_SRGB;
>> + framefmt->field = V4L2_FIELD_NONE;
>> + framefmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>> + framefmt->quantization = V4L2_QUANTIZATION_DEFAULT;
>> + framefmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
>> +};
>> +
>> +static int isc_scaler_get_fmt(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_format *format)
>> +{
>> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
>> + struct v4l2_mbus_framefmt *v4l2_try_fmt;
>> +
>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>> + v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
>> + format->pad);
>> + format->format = *v4l2_try_fmt;
>> +
>> + return 0;
>> + }
>> +
>> + format->format = isc->scaler_format[format->pad];
>> +
>> + return 0;
>> +}
>> +
>> +static int isc_scaler_set_fmt(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_format *req_fmt)
>> +{
>> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
>> + struct v4l2_mbus_framefmt *v4l2_try_fmt;
>> + struct isc_format *fmt;
>> + unsigned int i;
>> +
>> + /* Source format is fixed, we cannot change it */
>> + if (req_fmt->pad == ISC_SCALER_PAD_SOURCE) {
>> + req_fmt->format = isc->scaler_format[ISC_SCALER_PAD_SOURCE];
>> + return 0;
>> + }
>> +
>> + /* There is no limit on the frame size on the sink pad */
>> + v4l_bound_align_image(&req_fmt->format.width, 16, UINT_MAX, 0,
>> + &req_fmt->format.height, 16, UINT_MAX, 0, 0);
>> +
>> + isc_scaler_prepare_fmt(&req_fmt->format);
>> +
>> + fmt = isc_find_format_by_code(isc, req_fmt->format.code, &i);
>> +
>> + if (!fmt)
>> + fmt = &isc->formats_list[0];
>> +
>> + req_fmt->format.code = fmt->mbus_code;
>> +
>> + if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>> + v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
>> + req_fmt->pad);
>> + *v4l2_try_fmt = req_fmt->format;
>> + /* Trying on the sink pad makes the source pad change too */
>> + v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
>> + ISC_SCALER_PAD_SOURCE);
>> + *v4l2_try_fmt = req_fmt->format;
>> +
>> + v4l_bound_align_image(&v4l2_try_fmt->width,
>> + 16, isc->max_width, 0,
>> + &v4l2_try_fmt->height,
>> + 16, isc->max_height, 0, 0);
>> + /* if we are just trying, we are done */
>> + return 0;
>> + }
>> +
>> + isc->scaler_format[ISC_SCALER_PAD_SINK] = req_fmt->format;
>> +
>> + /* The source pad is the same as the sink, but we have to crop it */
>> + isc->scaler_format[ISC_SCALER_PAD_SOURCE] =
>> + isc->scaler_format[ISC_SCALER_PAD_SINK];
>> + v4l_bound_align_image
>> + (&isc->scaler_format[ISC_SCALER_PAD_SOURCE].width, 16,
>> + isc->max_width, 0,
>> + &isc->scaler_format[ISC_SCALER_PAD_SOURCE].height, 16,
>> + isc->max_height, 0, 0);
>> +
>> + return 0;
>> +}
>> +
>> +static int isc_scaler_enum_mbus_code(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
>> +
>> + /*
>> + * All formats supported by the ISC are supported by the scaler.
>> + * Advertise the formats which the ISC can take as input, as the scaler
>> + * entity cropping is part of the PFE module (parallel front end)
>> + */
>> + if (code->index < isc->formats_list_size) {
>> + code->code = isc->formats_list[code->index].mbus_code;
>> + return 0;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int isc_scaler_g_sel(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_selection *sel)
>> +{
>> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
>> +
>> + if (sel->pad == ISC_SCALER_PAD_SOURCE)
>> + return -EINVAL;
>> +
>> + if (sel->target == V4L2_SEL_TGT_CROP_BOUNDS) {
>> + /* bounds are the input format received */
>> + sel->r.height = isc->scaler_format[ISC_SCALER_PAD_SINK].height;
>> + sel->r.width = isc->scaler_format[ISC_SCALER_PAD_SINK].width;
>
> I'll re-paste our discussion on v4 to make sure we're on the same page
>
> -------------------------------------------------------------------------------
>>>> + if (sel->pad == ISC_SCALER_PAD_SOURCE)
>>>> + return -EINVAL;
>>>> +
>>>> + if (sel->target != V4L2_SEL_TGT_CROP_BOUNDS &&
>>>> + sel->target != V4L2_SEL_TGT_CROP)
>>>> + return -EINVAL;
>>>> +
>>>> + sel->r.height = isc->max_height;
>>>> + sel->r.width = isc->max_width;
>>>
>>> The CROP_BOUNDS should be set to the same size as the sink pad image format,
>>> as it represents the maximum valid crop rectangle.
>>>
>>> TGT_CROP should report the configured crop rectangle which can be
>>> intiialized to the same size as CROP_BOUNDS, if my understanding of
>>> the spec is correct
>>
>> So you would like to have this differentiated, and report the
>> CROP_BOUNDS to whatever is on the sink pad, and the TGT_CROP to what is
>> reported now, the maximum size of the ISC frame .
>> My understanding is correct ?
>>
>
> I didn't know you have an HW limitation, so your _BOUNDS is not the
> input image size but rather 3264x2464 ( == max_width x max_height).
>
> What I meant is that _BOUNDS should report the maximum rectangle size
> that can be applied to the _CROP target. In you case you have an HW
> limitation 3264x2464 and that's the largest rectangle you can apply.
> TGT_CROP can be initialized to the same as _BOUND, but if you
> implement s_selection it should report what has been there applied.
> But as you don't implement s_selection yet, I think this is fine for
> now. Maybe a little comment ?
> -------------------------------------------------------------------------------
>
> I think then that _BOUNDS should be fixed to 3264x2464 and CROP to
> src_fmt.
>
> The rest looks good! Thanks for pushing up to this version and thanks
> for addressings comments!
>
> Reviewed-by: Jacopo Mondi <[email protected]>
>
> Thanks
> j
Hello Jacopo,
So BOUNDS goes to isc max H / max V, and CROP goes to format of the
source pad.
Got it.
I will update with a v7 as soon as you give me your feedback on the
patch 8/13 which still required some changes from v6.
Thanks again for reviewing this !
Eugen
>
>> + 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[ISC_SCALER_PAD_SOURCE].height;
>> + sel->r.width = isc->scaler_format[ISC_SCALER_PAD_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 v4l2_rect *try_crop;
>> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
>> +
>> + *v4l2_try_fmt = isc->scaler_format[ISC_SCALER_PAD_SOURCE];
>> +
>> + try_crop = v4l2_subdev_get_try_crop(sd, sd_state, 0);
>> +
>> + try_crop->top = 0;
>> + try_crop->left = 0;
>> + try_crop->width = v4l2_try_fmt->width;
>> + try_crop->height = v4l2_try_fmt->height;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct v4l2_subdev_pad_ops isc_scaler_pad_ops = {
>> + .enum_mbus_code = isc_scaler_enum_mbus_code,
>> + .set_fmt = isc_scaler_set_fmt,
>> + .get_fmt = isc_scaler_get_fmt,
>> + .get_selection = isc_scaler_g_sel,
>> + .init_cfg = isc_scaler_init_cfg,
>> +};
>> +
>> +static const struct v4l2_subdev_ops xisc_scaler_subdev_ops = {
>> + .pad = &isc_scaler_pad_ops,
>> +};
>> +
>> +int isc_scaler_init(struct isc_device *isc)
>> +{
>> + int ret;
>> +
>> + v4l2_subdev_init(&isc->scaler_sd, &xisc_scaler_subdev_ops);
>> +
>> + isc->scaler_sd.owner = THIS_MODULE;
>> + isc->scaler_sd.dev = isc->dev;
>> + snprintf(isc->scaler_sd.name, sizeof(isc->scaler_sd.name),
>> + "atmel_isc_scaler");
>> +
>> + isc->scaler_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> + isc->scaler_sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
>> + isc->scaler_pads[ISC_SCALER_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>> + isc->scaler_pads[ISC_SCALER_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
>> +
>> + isc_scaler_prepare_fmt(&isc->scaler_format[ISC_SCALER_PAD_SOURCE]);
>> + isc->scaler_format[ISC_SCALER_PAD_SOURCE].height = isc->max_height;
>> + isc->scaler_format[ISC_SCALER_PAD_SOURCE].width = isc->max_width;
>> + isc->scaler_format[ISC_SCALER_PAD_SOURCE].code =
>> + isc->formats_list[0].mbus_code;
>> +
>> + isc->scaler_format[ISC_SCALER_PAD_SINK] =
>> + isc->scaler_format[ISC_SCALER_PAD_SOURCE];
>> +
>> + ret = media_entity_pads_init(&isc->scaler_sd.entity,
>> + ISC_SCALER_PADS_NUM,
>> + isc->scaler_pads);
>> + if (ret < 0) {
>> + dev_err(isc->dev, "scaler sd media entity init failed\n");
>> + return ret;
>> + }
>> +
>> + ret = v4l2_device_register_subdev(&isc->v4l2_dev, &isc->scaler_sd);
>> + if (ret < 0) {
>> + dev_err(isc->dev, "scaler sd failed to register subdev\n");
>> + return ret;
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(isc_scaler_init);
>> +
>> +int isc_scaler_link(struct isc_device *isc)
>> +{
>> + int ret;
>> +
>> + ret = media_create_pad_link(&isc->current_subdev->sd->entity,
>> + isc->remote_pad, &isc->scaler_sd.entity,
>> + ISC_SCALER_PAD_SINK,
>> + MEDIA_LNK_FL_ENABLED |
>> + MEDIA_LNK_FL_IMMUTABLE);
>> +
>> + if (ret < 0) {
>> + dev_err(isc->dev, "Failed to create pad link: %s to %s\n",
>> + isc->current_subdev->sd->entity.name,
>> + isc->scaler_sd.entity.name);
>> + return ret;
>> + }
>> +
>> + dev_dbg(isc->dev, "link with %s pad: %d\n",
>> + isc->current_subdev->sd->name, isc->remote_pad);
>> +
>> + ret = media_create_pad_link(&isc->scaler_sd.entity,
>> + ISC_SCALER_PAD_SOURCE,
>> + &isc->video_dev.entity, ISC_PAD_SINK,
>> + MEDIA_LNK_FL_ENABLED |
>> + MEDIA_LNK_FL_IMMUTABLE);
>> +
>> + if (ret < 0) {
>> + dev_err(isc->dev, "Failed to create pad link: %s to %s\n",
>> + isc->scaler_sd.entity.name,
>> + isc->video_dev.entity.name);
>> + return ret;
>> + }
>> +
>> + dev_dbg(isc->dev, "link with %s pad: %d\n", isc->scaler_sd.name,
>> + ISC_SCALER_PAD_SOURCE);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(isc_scaler_link);
>> +
>> diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
>> index f9ad7ec6bd13..9cc69c3ae26d 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,12 @@ struct isc_reg_offsets {
>> * be used as an input to the controller
>> * @controller_formats_size: size of controller_formats array
>> * @formats_list_size: size of formats_list array
>> + * @pads: media controller pads for isc video entity
>> + * @mdev: media device that is registered by the isc
>> + * @remote_pad: remote pad on the connected subdevice
>> + * @scaler_sd: subdevice for the scaler that isc registers
>> + * @scaler_pads: media controller pads for the scaler subdevice
>> + * @scaler_format: current format for the scaler subdevice
>> */
>> struct isc_device {
>> struct regmap *regmap;
>> @@ -346,6 +363,19 @@ struct isc_device {
>> struct isc_format *formats_list;
>> u32 controller_formats_size;
>> u32 formats_list_size;
>> +
>> + struct {
>> + struct media_pad pads[ISC_PADS_NUM];
>> + struct media_device mdev;
>> +
>> + u32 remote_pad;
>> + };
>> +
>> + struct {
>> + struct v4l2_subdev scaler_sd;
>> + struct media_pad scaler_pads[ISC_SCALER_PADS_NUM];
>> + struct v4l2_mbus_framefmt scaler_format[ISC_SCALER_PADS_NUM];
>> + };
>> };
>>
>> extern const struct regmap_config isc_regmap_config;
>> @@ -357,4 +387,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
>>