Message ID | 20220217135645.1427466-1-eugen.hristev@microchip.com |
---|---|
Headers | show |
Series | media: atmel: atmel-isc: implement media controller | expand |
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
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 >
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
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 >
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 >
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 >
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 >
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 >
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 >
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 >
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 >>