| Message ID | 20240529152858.183799-1-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Arm Mali-C55 Image Signal Processor Driver | expand |
Hi Dan, Thank you for the patches. On Wed, May 29, 2024 at 04:28:42PM +0100, Daniel Scally wrote: > Hello all > > This patchset introduces a driver for Arm's Mali-C55 Image Signal Processor. > The driver uses the V4L2 / media controller API and implements both of the ISP's > capture pipelines allowing a range of output formats plus downscaling and > cropping. The capture pipelines are named "Full resolution" and "Downscale" and > so abbreviated FR and DS throughout the driver. > > The driver exposes 4 V4L2 subdevices: > > - mali-c55 isp: input data formatting > - mali-c55 tpg: test pattern generator (modeled as a camera sensor entity) > - mali-c55 resizer fr: downscale / crop and format setting for the FR pipe > - mali-c55 resizer ds: downscale / crop and format setting for the DS pipe > > Along with 4 V4L2 Video devices: > > - mali-c55 fr: Capture device for the full resolution pipe > - mali-c55 ds: Capture device for the downscale pipe > - mali-c55 3a stats: Capture device for statistics to support 3A algorithms > - mali-c55 3a params: Output device for parameter buffers to configure the ISP > > Support is implemented in the parameters video device code for many of the ISP'S > hardware blocks, but not yet all of them. The buffer format is (as far as I am > aware anyway) a novel design that we intend to be extensible so that support for > the C55's remaining hardware blocks can be added later. > > Patches 1, 4, 5, 6 and 7 have already had 4 versions on the mailing list...I > decided to post the additional work on the driver as extra patches rather than > merge them all into the existing series as it's already a lot of code to review > and I hoped that that might make it a little easier...if I'm wrong and that's > not liked I can just squash them into a much smaller series. Could you please include the v4l2-compliance report in v6 ? Make sure to use the very latest version of v4l-utils, compiled from the master branch. > Daniel Scally (15): > media: uapi: Add MEDIA_BUS_FMT_RGB202020_1X60 format code > media: uapi: Add 20-bit bayer formats > media: v4l2-common: Add RAW16 format info > dt-bindings: media: Add bindings for ARM mali-c55 > media: mali-c55: Add Mali-C55 ISP driver > media: Documentation: Add Mali-C55 ISP Documentation > MAINTAINERS: Add entry for mali-c55 driver > media: Add MALI_C55_3A_STATS meta format > media: uapi: Add 3a stats buffer for mali-c55 > media: platform: Add mali-c55 3a stats devnode > media: platform: Fill stats buffer on ISP_START > Documentation: mali-c55: Add Statistics documentation > media: uapi: Add parameters structs to mali-c55-config.h > media: platform: Add mali-c55 parameters video node > Documentation: mali-c55: Document the mali-c55 parameter setting > > Jacopo Mondi (1): > media: mali-c55: Add image formats for Mali-C55 parameters buffer > > .../admin-guide/media/mali-c55-graph.dot | 19 + > Documentation/admin-guide/media/mali-c55.rst | 406 ++++++++ > .../admin-guide/media/v4l-drivers.rst | 1 + > .../bindings/media/arm,mali-c55.yaml | 66 ++ > .../userspace-api/media/v4l/meta-formats.rst | 1 + > .../media/v4l/metafmt-arm-mali-c55.rst | 87 ++ > .../media/v4l/subdev-formats.rst | 268 +++++ > MAINTAINERS | 10 + > drivers/media/platform/Kconfig | 1 + > drivers/media/platform/Makefile | 1 + > drivers/media/platform/arm/Kconfig | 5 + > drivers/media/platform/arm/Makefile | 2 + > drivers/media/platform/arm/mali-c55/Kconfig | 18 + > drivers/media/platform/arm/mali-c55/Makefile | 11 + > .../platform/arm/mali-c55/mali-c55-capture.c | 951 ++++++++++++++++++ > .../platform/arm/mali-c55/mali-c55-common.h | 312 ++++++ > .../platform/arm/mali-c55/mali-c55-core.c | 825 +++++++++++++++ > .../platform/arm/mali-c55/mali-c55-isp.c | 626 ++++++++++++ > .../platform/arm/mali-c55/mali-c55-params.c | 615 +++++++++++ > .../arm/mali-c55/mali-c55-registers.h | 365 +++++++ > .../arm/mali-c55/mali-c55-resizer-coefs.h | 382 +++++++ > .../platform/arm/mali-c55/mali-c55-resizer.c | 779 ++++++++++++++ > .../platform/arm/mali-c55/mali-c55-stats.c | 350 +++++++ > .../platform/arm/mali-c55/mali-c55-tpg.c | 402 ++++++++ > drivers/media/v4l2-core/v4l2-common.c | 4 + > drivers/media/v4l2-core/v4l2-ioctl.c | 2 + > include/uapi/linux/media-bus-format.h | 9 +- > .../uapi/linux/media/arm/mali-c55-config.h | 851 ++++++++++++++++ > include/uapi/linux/videodev2.h | 3 + > 29 files changed, 7370 insertions(+), 2 deletions(-) > create mode 100644 Documentation/admin-guide/media/mali-c55-graph.dot > create mode 100644 Documentation/admin-guide/media/mali-c55.rst > create mode 100644 Documentation/devicetree/bindings/media/arm,mali-c55.yaml > create mode 100644 Documentation/userspace-api/media/v4l/metafmt-arm-mali-c55.rst > create mode 100644 drivers/media/platform/arm/Kconfig > create mode 100644 drivers/media/platform/arm/Makefile > create mode 100644 drivers/media/platform/arm/mali-c55/Kconfig > create mode 100644 drivers/media/platform/arm/mali-c55/Makefile > create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-capture.c > create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-common.h > create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-core.c > create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-isp.c > create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-params.c > create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-registers.h > create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-resizer-coefs.h > create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-resizer.c > create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-stats.c > create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-tpg.c > create mode 100644 include/uapi/linux/media/arm/mali-c55-config.h
Hi Laurent On Fri, May 31, 2024 at 12:43:48AM GMT, Laurent Pinchart wrote: > And now the second part of the review, addressing mali-c55-capture.c and > mali-c55-resizer.c. I've reviewed the code from the bottom up, so some > messages may be repeated in an order that seems weird. Sorry about that. > [snip] A few replies/questions on the resizer module > > > > > + > > > +#endif /* _MALI_C55_RESIZER_COEFS_H */ > > > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c > > > new file mode 100644 > > > index 000000000000..0a5a2969d3ce > > > --- /dev/null > > > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c > > > @@ -0,0 +1,779 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * ARM Mali-C55 ISP Driver - Image signal processor > > > + * > > > + * Copyright (C) 2024 Ideas on Board Oy > > > + */ > > > + > > > +#include <linux/math.h> > > > +#include <linux/minmax.h> > > > + > > > +#include <media/media-entity.h> > > > +#include <media/v4l2-subdev.h> > > > + > > > +#include "mali-c55-common.h" > > > +#include "mali-c55-registers.h" > > > +#include "mali-c55-resizer-coefs.h" > > > + > > > +/* Scaling factor in Q4.20 format. */ > > > +#define MALI_C55_RZR_SCALER_FACTOR (1U << 20) > > > + > > > +static const u32 rzr_non_bypass_src_fmts[] = { > > > + MEDIA_BUS_FMT_RGB121212_1X36, > > > + MEDIA_BUS_FMT_YUV10_1X30 > > > +}; > > > + > > > +static const char * const mali_c55_resizer_names[] = { > > > + [MALI_C55_RZR_FR] = "resizer fr", > > > + [MALI_C55_RZR_DS] = "resizer ds", > > > +}; > > > + > > > +static int mali_c55_rzr_program_crop(struct mali_c55_resizer *rzr, > > > + struct v4l2_subdev_state *state) > > > +{ > > > + unsigned int reg_offset = rzr->cap_dev->reg_offset; > > > + struct mali_c55 *mali_c55 = rzr->mali_c55; > > > + struct v4l2_mbus_framefmt *fmt; > > > + struct v4l2_rect *crop; > > const > > > > + > > > + /* Verify if crop should be enabled. */ > > > + fmt = v4l2_subdev_state_get_format(state, MALI_C55_RZR_SINK_PAD, 0); > > > + crop = v4l2_subdev_state_get_crop(state, MALI_C55_RZR_SINK_PAD, 0); > > > + > > > + if (fmt->width == crop->width && fmt->height == crop->height) > > > + return MALI_C55_BYPASS_CROP; > > > + > > > + mali_c55_write(mali_c55, MALI_C55_REG_CROP_X_START(reg_offset), > > > + crop->left); > > > + mali_c55_write(mali_c55, MALI_C55_REG_CROP_Y_START(reg_offset), > > > + crop->top); > > > + mali_c55_write(mali_c55, MALI_C55_REG_CROP_X_SIZE(reg_offset), > > > + crop->width); > > > + mali_c55_write(mali_c55, MALI_C55_REG_CROP_Y_SIZE(reg_offset), > > > + crop->height); > > > + > > > + mali_c55_write(mali_c55, MALI_C55_REG_CROP_EN(reg_offset), > > > + MALI_C55_CROP_ENABLE); > > > + > > > + return 0; > > > +} > > > + > > > +static int mali_c55_rzr_program_resizer(struct mali_c55_resizer *rzr, > > > + struct v4l2_subdev_state *state) > > > +{ > > > + unsigned int reg_offset = rzr->cap_dev->reg_offset; > > > + struct mali_c55 *mali_c55 = rzr->mali_c55; > > > + struct v4l2_rect *crop, *scale; > > const > > Once "[PATCH v4 0/3] media: v4l2-subdev: Support const-awareness in > state accessors" gets merged, the state argument to this function can be > made const too. Same for other functions, as applicable. > > > > + unsigned int h_bank, v_bank; > > > + u64 h_scale, v_scale; > > > + > > > + /* Verify if scaling should be enabled. */ > > > + crop = v4l2_subdev_state_get_crop(state, MALI_C55_RZR_SINK_PAD, 0); > > > + scale = v4l2_subdev_state_get_compose(state, MALI_C55_RZR_SINK_PAD, 0); > > > + > > > + if (crop->width == scale->width && crop->height == scale->height) > > > + return MALI_C55_BYPASS_SCALER; > > > + > > > + /* Program the V/H scaling factor in Q4.20 format. */ > > > + h_scale = crop->width * MALI_C55_RZR_SCALER_FACTOR; > > > + v_scale = crop->height * MALI_C55_RZR_SCALER_FACTOR; > > > + > > > + do_div(h_scale, scale->width); > > > + do_div(v_scale, scale->height); > > > + > > > + mali_c55_write(mali_c55, > > > + MALI_C55_REG_SCALER_IN_WIDTH(reg_offset), > > > + crop->width); > > > + mali_c55_write(mali_c55, > > > + MALI_C55_REG_SCALER_IN_HEIGHT(reg_offset), > > > + crop->height); > > > + > > > + mali_c55_write(mali_c55, > > > + MALI_C55_REG_SCALER_OUT_WIDTH(reg_offset), > > > + scale->width); > > > + mali_c55_write(mali_c55, > > > + MALI_C55_REG_SCALER_OUT_HEIGHT(reg_offset), > > > + scale->height); > > > + > > > + mali_c55_write(mali_c55, > > > + MALI_C55_REG_SCALER_HFILT_TINC(reg_offset), > > > + h_scale); > > > + mali_c55_write(mali_c55, > > > + MALI_C55_REG_SCALER_VFILT_TINC(reg_offset), > > > + v_scale); > > > + > > > + h_bank = mali_c55_calculate_bank_num(mali_c55, crop->width, > > > + scale->width); > > > + mali_c55_write(mali_c55, > > > + MALI_C55_REG_SCALER_HFILT_COEF(reg_offset), > > > + h_bank); > > > + > > > + v_bank = mali_c55_calculate_bank_num(mali_c55, crop->height, > > > + scale->height); > > > + mali_c55_write(mali_c55, > > > + MALI_C55_REG_SCALER_VFILT_COEF(reg_offset), > > > + v_bank); > > > + > > > + return 0; > > > +} > > > + > > > +static void mali_c55_rzr_program(struct mali_c55_resizer *rzr, > > > + struct v4l2_subdev_state *state) > > > +{ > > > + struct mali_c55 *mali_c55 = rzr->mali_c55; > > > + u32 bypass = 0; > > > + > > > + /* Verify if cropping and scaling should be enabled. */ > > > + bypass |= mali_c55_rzr_program_crop(rzr, state); > > > + bypass |= mali_c55_rzr_program_resizer(rzr, state); > > > + > > > + mali_c55_update_bits(mali_c55, rzr->id == MALI_C55_RZR_FR ? > > > + MALI_C55_REG_FR_BYPASS : MALI_C55_REG_DS_BYPASS, > > > + MALI_C55_BYPASS_CROP | MALI_C55_BYPASS_SCALER, > > > + bypass); > > > +} > > > + > > > +/* > > > + * Inspect the routing table to know which of the two (mutually exclusive) > > > + * routes is enabled and return the sink pad id of the active route. > > > + */ > > > +static unsigned int mali_c55_rzr_get_active_sink(struct v4l2_subdev_state *state) > > > +{ > > > + struct v4l2_subdev_krouting *routing = &state->routing; > > > + struct v4l2_subdev_route *route; > > > + > > > + /* A single route is enabled at a time. */ > > > + for_each_active_route(routing, route) > > > + return route->sink_pad; > > > + > > > + return MALI_C55_RZR_SINK_PAD; > > > +} > > > + > > > +static u32 mali_c55_rzr_shift_mbus_code(u32 mbus_code) > > > +{ > > > + u32 corrected_code = 0; > > > + > > > + /* > > > + * The ISP takes input in a 20-bit format, but can only output 16-bit > > > + * RAW bayer data (with the 4 least significant bits from the input > > > + * being lost). Return the 16-bit version of the 20-bit input formats. > > > + */ > > > + switch (mbus_code) { > > > + case MEDIA_BUS_FMT_SBGGR20_1X20: > > > + corrected_code = MEDIA_BUS_FMT_SBGGR16_1X16; > > > + break; > > > + case MEDIA_BUS_FMT_SGBRG20_1X20: > > > + corrected_code = MEDIA_BUS_FMT_SGBRG16_1X16; > > > + break; > > > + case MEDIA_BUS_FMT_SGRBG20_1X20: > > > + corrected_code = MEDIA_BUS_FMT_SGRBG16_1X16; > > > + break; > > > + case MEDIA_BUS_FMT_SRGGB20_1X20: > > > + corrected_code = MEDIA_BUS_FMT_SRGGB16_1X16; > > > + break; > > Would it make sense to add the shifted code to mali_c55_isp_fmt ? > > > > + } > > > + > > > + return corrected_code; > > > +} > > > + > > > +static int __mali_c55_rzr_set_routing(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state, > > > + struct v4l2_subdev_krouting *routing) > > I think the last argument can be const. If I have to adjust the routing table instead of refusing it, it can't > > > > +{ > > > + struct mali_c55_resizer *rzr = container_of(sd, struct mali_c55_resizer, > > > + sd); > > A to_mali_c55_resizer() static inline function would be useful. Same for > other components, where applicable. > > > > + unsigned int active_sink = UINT_MAX; > > > + struct v4l2_mbus_framefmt *src_fmt; > > > + struct v4l2_rect *crop, *compose; > > > + struct v4l2_subdev_route *route; > > > + unsigned int active_routes = 0; > > > + struct v4l2_mbus_framefmt *fmt; > > > + int ret; > > > + > > > + ret = v4l2_subdev_routing_validate(sd, routing, 0); > > > + if (ret) > > > + return ret; > > > + > > > + /* Only a single route can be enabled at a time. */ > > > + for_each_active_route(routing, route) { > > > + if (++active_routes > 1) { > > > + dev_err(rzr->mali_c55->dev, > > > + "Only one route can be active"); > > No kernel log message with a level higher than dev_dbg() from > user-controlled paths please, here and where applicable. This is to > avoid giving applications an easy way to flood the kernel log. > > > > + return -EINVAL; > > > + } > > > + > > > + active_sink = route->sink_pad; > > > + } > > > + if (active_sink == UINT_MAX) { > > > + dev_err(rzr->mali_c55->dev, "One route has to be active"); > > > + return -EINVAL; > > > + } > > The recommended handling of invalid routing is to adjust the routing > table, not to return errors. > How should I adjust it ? The error here is due to the fact multiple routes are set as active, which one should I make active ? the first one ? Should I go and reset the flags in the subdev_route for the one that has to be made non-active ? > > > + > > > + ret = v4l2_subdev_set_routing(sd, state, routing); > > > + if (ret) { > > > + dev_err(rzr->mali_c55->dev, "Failed to set routing\n"); > > > + return ret; > > > + } > > > + > > > + fmt = v4l2_subdev_state_get_format(state, active_sink, 0); > > > + crop = v4l2_subdev_state_get_crop(state, active_sink, 0); > > > + compose = v4l2_subdev_state_get_compose(state, active_sink, 0); > > > + > > > + fmt->width = MALI_C55_DEFAULT_WIDTH; > > > + fmt->height = MALI_C55_DEFAULT_HEIGHT; > > > + fmt->colorspace = V4L2_COLORSPACE_SRGB; > > There are other colorspace-related fields. > > > > + fmt->field = V4L2_FIELD_NONE; > > I wonder if we should really update the sink pad format, or just > propagate it. If we update it, I think it should be set to defaults on > both sink pads, not just the active sink pad. > If only one route can be active, there will only be one state.stream_config entry for the active sink, not for the other one (see v4l2_subdev_init_stream_configs()), this mean I can't reset both sink formats ? > > > + > > > + if (active_sink == MALI_C55_RZR_SINK_PAD) { > > > + fmt->code = MEDIA_BUS_FMT_RGB121212_1X36; > > > + > > > + crop->left = crop->top = 0; > > crop->left = 0; > crop->top = 0; > > > > + crop->width = MALI_C55_DEFAULT_WIDTH; > > > + crop->height = MALI_C55_DEFAULT_HEIGHT; > > > + > > > + *compose = *crop; > > > + } else { > > > + fmt->code = MEDIA_BUS_FMT_SRGGB20_1X20; > > > + } > > > + > > > + /* Propagate the format to the source pad */ > > > + src_fmt = v4l2_subdev_state_get_format(state, MALI_C55_RZR_SOURCE_PAD, > > > + 0); > > > + *src_fmt = *fmt; > > > + > > > + /* In the event this is the bypass pad the mbus code needs correcting */ > > > + if (active_sink == MALI_C55_RZR_SINK_BYPASS_PAD) > > > + src_fmt->code = mali_c55_rzr_shift_mbus_code(src_fmt->code); > > > + > > > + return 0; > > > +} > > > + > > > +static int mali_c55_rzr_enum_mbus_code(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state, > > > + struct v4l2_subdev_mbus_code_enum *code) > > > +{ > > > + struct v4l2_mbus_framefmt *sink_fmt; > > > + const struct mali_c55_isp_fmt *fmt; > > > + unsigned int index = 0; > > > + u32 sink_pad; > > > + > > > + switch (code->pad) { > > > + case MALI_C55_RZR_SINK_PAD: > > > + if (code->index) > > > + return -EINVAL; > > > + > > > + code->code = MEDIA_BUS_FMT_RGB121212_1X36; > > > + > > > + return 0; > > > + case MALI_C55_RZR_SOURCE_PAD: > > > + sink_pad = mali_c55_rzr_get_active_sink(state); > > > + sink_fmt = v4l2_subdev_state_get_format(state, sink_pad, 0); > > > + > > > + /* > > > + * If the active route is from the Bypass sink pad, then the > > > + * source pad is a simple passthrough of the sink format, > > > + * downshifted to 16-bits. > > > + */ > > > + > > > + if (sink_pad == MALI_C55_RZR_SINK_BYPASS_PAD) { > > > + if (code->index) > > > + return -EINVAL; > > > + > > > + code->code = mali_c55_rzr_shift_mbus_code(sink_fmt->code); > > > + if (!code->code) > > > + return -EINVAL; > > > + > > > + return 0; > > > + } > > > + > > > + /* > > > + * If the active route is from the non-bypass sink then we can > > > + * select either RGB or conversion to YUV. > > > + */ > > > + > > > + if (code->index >= ARRAY_SIZE(rzr_non_bypass_src_fmts)) > > > + return -EINVAL; > > > + > > > + code->code = rzr_non_bypass_src_fmts[code->index]; > > > + > > > + return 0; > > > + case MALI_C55_RZR_SINK_BYPASS_PAD: > > > + for_each_mali_isp_fmt(fmt) { > > > + if (index++ == code->index) { > > > + code->code = fmt->code; > > > + return 0; > > > + } > > > + } > > > + > > > + break; > > > + } > > > + > > > + return -EINVAL; > > > +} > > > + > > > +static int mali_c55_rzr_enum_frame_size(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state, > > > + struct v4l2_subdev_frame_size_enum *fse) > > > +{ > > > + if (fse->index) > > > + return -EINVAL; > > > + > > > + fse->max_width = MALI_C55_MAX_WIDTH; > > > + fse->max_height = MALI_C55_MAX_HEIGHT; > > > + fse->min_width = MALI_C55_MIN_WIDTH; > > > + fse->min_height = MALI_C55_MIN_HEIGHT; > > > + > > > + return 0; > > > +} > > > + > > > +static int mali_c55_rzr_set_sink_fmt(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state, > > > + struct v4l2_subdev_format *format) > > > +{ > > > + struct v4l2_mbus_framefmt *fmt = &format->format; > > > + struct v4l2_rect *rect; > > > + unsigned int sink_pad; > > > + > > > + /* > > > + * Clamp to min/max and then reset crop and compose rectangles to the > > > + * newly applied size. > > > + */ > > > + clamp_t(unsigned int, fmt->width, > > > + MALI_C55_MIN_WIDTH, MALI_C55_MAX_WIDTH); also, clamp_t doens't clamp in place fmt->width = clamp_t... > > > + clamp_t(unsigned int, fmt->height, > > > + MALI_C55_MIN_HEIGHT, MALI_C55_MAX_HEIGHT); > > Please check comments for other components related to the colorspace > fields, to decide how to handle them here. > > > > + > > > + sink_pad = mali_c55_rzr_get_active_sink(state); > > > + if (sink_pad == MALI_C55_RZR_SINK_PAD) { > > The selection here should depend on format->pad, not the active sink > pad. > > > > + fmt->code = MEDIA_BUS_FMT_RGB121212_1X36; > > > + > > > + rect = v4l2_subdev_state_get_crop(state, MALI_C55_RZR_SINK_PAD); > > > + rect->left = 0; > > > + rect->top = 0; > > > + rect->width = fmt->width; > > > + rect->height = fmt->height; > > > + > > > + rect = v4l2_subdev_state_get_compose(state, > > > + MALI_C55_RZR_SINK_PAD); > > > + rect->left = 0; > > > + rect->top = 0; > > > + rect->width = fmt->width; > > > + rect->height = fmt->height; > > > + } else { > > > + /* > > > + * Make sure the media bus code is one of the supported > > > + * ISP input media bus codes. > > > + */ > > > + if (!mali_c55_isp_is_format_supported(fmt->code)) > > > + fmt->code = MALI_C55_DEFAULT_MEDIA_BUS_FMT; And DEFAULT_MEDIA_BUS_FMT is not one of the supported input media bus codes > > > + } > > > + > > > + *v4l2_subdev_state_get_format(state, sink_pad, 0) = *fmt; > > > + *v4l2_subdev_state_get_format(state, MALI_C55_RZR_SOURCE_PAD, 0) = *fmt; > > Propagation to the source pad, however, should depend on the active > route. If format->pad is routed to the source pad, you should propagate, > otherwise, you shouldn't. > > > > + > > > + return 0; I ended up with static int mali_c55_rsz_set_sink_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, struct v4l2_subdev_format *format) { struct v4l2_mbus_framefmt *fmt = &format->format; unsigned int active_sink; struct v4l2_rect *rect; /* * Clamp to min/max and then reset crop and compose rectangles to the * newly applied size. */ fmt->width = clamp_t(unsigned int, fmt->width, MALI_C55_MIN_WIDTH, MALI_C55_MAX_WIDTH); fmt->height = clamp_t(unsigned int, fmt->height, MALI_C55_MIN_HEIGHT, MALI_C55_MAX_HEIGHT); rect = v4l2_subdev_state_get_crop(state, format->pad); rect->left = 0; rect->top = 0; rect->width = fmt->width; rect->height = fmt->height; rect = v4l2_subdev_state_get_compose(state, format->pad); rect->left = 0; rect->top = 0; rect->width = fmt->width; rect->height = fmt->height; if (format->pad == MALI_C55_RSZ_SINK_BYPASS_PAD) { /* * Make sure the media bus code is one of the supported * ISP input media bus codes. Default it to SRGGB otherwise. */ if (!mali_c55_isp_is_format_supported(fmt->code)) fmt->code = MEDIA_BUS_FMT_SRGGB20_1X20; } else { fmt->code = MEDIA_BUS_FMT_RGB121212_1X36; } *v4l2_subdev_state_get_format(state, format->pad, 0) = *fmt; /* If format->pad is routed to the source pad, propagate the format. */ active_sink = mali_c55_rsz_get_active_sink(state); if (active_sink == format->pad) { /* If the bypass route is used, downshift the code to 16bpp. */ if (active_sink == MALI_C55_RSZ_SINK_BYPASS_PAD) fmt->code = mali_c55_rsz_shift_mbus_code(fmt->code); *v4l2_subdev_state_get_format(state, MALI_C55_RSZ_SOURCE_PAD, 0) = *fmt; } return 0; } > > > +} > > > + > > > +static int mali_c55_rzr_set_source_fmt(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state, > > > + struct v4l2_subdev_format *format) > > > +{ > > > + struct mali_c55_resizer *rzr = container_of(sd, struct mali_c55_resizer, > > > + sd); > > > + struct v4l2_mbus_framefmt *fmt = &format->format; > > > + struct v4l2_mbus_framefmt *sink_fmt; > > > + struct v4l2_rect *crop, *compose; > > > + unsigned int sink_pad; > > > + unsigned int i; > > > + > > > + sink_pad = mali_c55_rzr_get_active_sink(state); > > > + sink_fmt = v4l2_subdev_state_get_format(state, sink_pad, 0); > > > + crop = v4l2_subdev_state_get_crop(state, sink_pad, 0); > > > + compose = v4l2_subdev_state_get_compose(state, sink_pad, 0); > > > + > > > + /* FR Bypass pipe. */ > > > + > > > + if (sink_pad == MALI_C55_RZR_SINK_BYPASS_PAD) { > > > + /* > > > + * Format on the source pad is the same as the one on the > > > + * sink pad, downshifted to 16-bits. > > > + */ > > > + fmt->code = mali_c55_rzr_shift_mbus_code(sink_fmt->code); > > > + if (!fmt->code) > > > + return -EINVAL; > > > + > > > + /* RAW bypass disables scaling and cropping. */ > > > + crop->top = compose->top = 0; > > > + crop->left = compose->left = 0; > > > + fmt->width = crop->width = compose->width = sink_fmt->width; > > > + fmt->height = crop->height = compose->height = sink_fmt->height; > > I don't think this is right. This function sets the format on the source > pad. Subdevs should propagate formats from the sink to the source, not > the other way around. > > The only parameter that can be modified on the source pad (as far as I > understand) is the media bus code. In the bypass path, I understand it's > fixed, while in the other path, you can select between RGB and YUV. I > think the following code is what you need to implement this function. > > static int mali_c55_rzr_set_source_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_state *state, > struct v4l2_subdev_format *format) > { > struct mali_c55_resizer *rzr = container_of(sd, struct mali_c55_resizer, > sd); > struct v4l2_mbus_framefmt *fmt; > > fmt = v4l2_subdev_state_get_format(state, format->pad); > > /* In the non-bypass path the output format can be selected. */ > if (mali_c55_rzr_get_active_sink(state) == MALI_C55_RZR_SINK_PAD) { > unsigned int i; > > fmt->code = format->format.code; > > for (i = 0; i < ARRAY_SIZE(rzr_non_bypass_src_fmts); i++) { > if (fmt->code == rzr_non_bypass_src_fmts[i]) > break; > } > > if (i == ARRAY_SIZE(rzr_non_bypass_src_fmts)) > fmt->code = rzr_non_bypass_src_fmts[0]; > } > > format->format = *fmt; > > return 0; > } Almost. Your proposal doesn't adjust format->format.width/height I think the following is more appropriate static int mali_c55_rsz_set_source_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, struct v4l2_subdev_format *format) { struct v4l2_mbus_framefmt *fmt = &format->format; struct v4l2_mbus_framefmt *sink_fmt; struct v4l2_rect *sink_compose; unsigned int active_sink; active_sink = mali_c55_rsz_get_active_sink(state); sink_fmt = v4l2_subdev_state_get_format(state, active_sink, 0); sink_compose = v4l2_subdev_state_get_compose(state, active_sink, 0); /* * The source pad format sizes come directly from the active sink pad * compose rectangle. */ fmt->width = sink_compose->width; fmt->height = sink_compose->height; if (active_sink == MALI_C55_RSZ_SINK_PAD) { /* * Regular processing pipe: RGB121212 can be color-space * converted to YUV101010. */ unsigned int i; for (i = 0; i < ARRAY_SIZE(rsz_non_bypass_src_fmts); i++) { if (fmt->code == rsz_non_bypass_src_fmts[i]) break; } if (i == ARRAY_SIZE(rsz_non_bypass_src_fmts)) fmt->code = MEDIA_BUS_FMT_RGB121212_1X36; } else { /* * Bypass pipe: the source format is the same as the bypass * sink pad downshifted to 16bpp. */ fmt->code = mali_c55_rsz_shift_mbus_code(sink_fmt->code); } *v4l2_subdev_state_get_format(state, MALI_C55_RSZ_SOURCE_PAD) = *fmt; return 0; } I'll handle the colorspace fields as well > > > > + > > > + *v4l2_subdev_state_get_format(state, > > > + MALI_C55_RZR_SOURCE_PAD) = *fmt; > > > + > > > + return 0; > > > + } > > > + > > > + /* Regular processing pipe. */ > > > + > > > + for (i = 0; i < ARRAY_SIZE(rzr_non_bypass_src_fmts); i++) { > > > + if (fmt->code == rzr_non_bypass_src_fmts[i]) > > > + break; > > > + } > > > + > > > + if (i == ARRAY_SIZE(rzr_non_bypass_src_fmts)) { > > > + dev_dbg(rzr->mali_c55->dev, > > > + "Unsupported mbus code 0x%x: using default\n", > > > + fmt->code); > > I think you can drop this message. > > > > + fmt->code = MEDIA_BUS_FMT_RGB121212_1X36; > > > + } > > > + > > > + /* > > > + * The source pad format size comes directly from the sink pad > > > + * compose rectangle. > > > + */ > > > + fmt->width = compose->width; > > > + fmt->height = compose->height; > > > + > > > + *v4l2_subdev_state_get_format(state, MALI_C55_RZR_SOURCE_PAD) = *fmt; > > > + > > > + return 0; > > > +} > > > + > > > +static int mali_c55_rzr_set_fmt(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state, > > > + struct v4l2_subdev_format *format) > > > +{ > > > + /* > > > + * On sink pads fmt is either fixed for the 'regular' processing > > > + * pad or a RAW format or 20-bit wide RGB/YUV format for the FR bypass > > > + * pad. > > > + * > > > + * On source pad sizes are the result of crop+compose on the sink > > > + * pad sizes, while the format depends on the active route. > > > + */ > > > + > > > + if (format->pad != MALI_C55_RZR_SOURCE_PAD) > > > + return mali_c55_rzr_set_sink_fmt(sd, state, format); > > > + > > > + return mali_c55_rzr_set_source_fmt(sd, state, format); > > Nitpicking, > > if (format->pad == MALI_C55_RZR_SOURCE_PAD) > return mali_c55_rzr_set_source_fmt(sd, state, format); > > return mali_c55_rzr_set_sink_fmt(sd, state, format); > > to match SOURCE_PAD and source_fmt. > Done at the expense a bit more verbose check if (format->pad == MALI_C55_RSZ_SINK_PAD || format->pad == MALI_C55_RSZ_SINK_BYPASS_PAD) return mali_c55_rsz_set_sink_fmt(sd, state, format); > > > +} > > > + > > > +static int mali_c55_rzr_get_selection(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state, > > > + struct v4l2_subdev_selection *sel) > > > +{ > > > + if (sel->pad != MALI_C55_RZR_SINK_PAD) > > > + return -EINVAL; > > > + > > > + if (sel->target != V4L2_SEL_TGT_CROP && > > > + sel->target != V4L2_SEL_TGT_COMPOSE) > > > + return -EINVAL; > > > + > > > + sel->r = sel->target == V4L2_SEL_TGT_CROP > > > + ? *v4l2_subdev_state_get_crop(state, MALI_C55_RZR_SINK_PAD) > > > + : *v4l2_subdev_state_get_compose(state, MALI_C55_RZR_SINK_PAD); > > > + > > > + return 0; > > > +} > > > + > > > +static int mali_c55_rzr_set_selection(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state, > > > + struct v4l2_subdev_selection *sel) > > > +{ > > > + struct mali_c55_resizer *rzr = container_of(sd, struct mali_c55_resizer, > > > + sd); > > > + struct v4l2_mbus_framefmt *source_fmt; > > > + struct v4l2_mbus_framefmt *sink_fmt; > > > + struct v4l2_rect *crop, *compose; > > > + > > > + if (sel->pad != MALI_C55_RZR_SINK_PAD) > > > + return -EINVAL; > > > + > > > + if (sel->target != V4L2_SEL_TGT_CROP && > > > + sel->target != V4L2_SEL_TGT_COMPOSE) > > > + return -EINVAL; > > > + > > > + source_fmt = v4l2_subdev_state_get_format(state, > > > + MALI_C55_RZR_SOURCE_PAD); > > > + sink_fmt = v4l2_subdev_state_get_format(state, MALI_C55_RZR_SINK_PAD); > > > + crop = v4l2_subdev_state_get_crop(state, MALI_C55_RZR_SINK_PAD); > > > + compose = v4l2_subdev_state_get_compose(state, MALI_C55_RZR_SINK_PAD); > > > + > > > + /* RAW bypass disables crop/scaling. */ > > > + if (mali_c55_format_is_raw(source_fmt->code)) { > > > + crop->top = compose->top = 0; > > > + crop->left = compose->left = 0; > > > + crop->width = compose->width = sink_fmt->width; > > > + crop->height = compose->height = sink_fmt->height; > > > + > > > + sel->r = sel->target == V4L2_SEL_TGT_CROP ? *crop : *compose; > > > + > > > + return 0; > > > + } > > > + > > > + /* During streaming, it is allowed to only change the crop rectangle. */ > > > + if (rzr->streaming && sel->target != V4L2_SEL_TGT_CROP) > > > + return -EINVAL; > > > + > > > + /* > > > + * Update the desired target and then clamp the crop rectangle to the > > > + * sink format sizes and the compose size to the crop sizes. > > > + */ > > > + if (sel->target == V4L2_SEL_TGT_CROP) > > > + *crop = sel->r; > > > + else > > > + *compose = sel->r; > > > + > > > + clamp_t(unsigned int, crop->left, 0, sink_fmt->width); > > > + clamp_t(unsigned int, crop->top, 0, sink_fmt->height); > > > + clamp_t(unsigned int, crop->width, MALI_C55_MIN_WIDTH, > > > + sink_fmt->width - crop->left); > > > + clamp_t(unsigned int, crop->height, MALI_C55_MIN_HEIGHT, > > > + sink_fmt->height - crop->top); > > > + > > > + if (rzr->streaming) { > > > + /* > > > + * Apply at runtime a crop rectangle on the resizer's sink only > > > + * if it doesn't require re-programming the scaler output sizes > > > + * as it would require changing the output buffer sizes as well. > > > + */ > > > + if (sel->r.width < compose->width || > > > + sel->r.height < compose->height) > > > + return -EINVAL; > > > + > > > + *crop = sel->r; > > > + mali_c55_rzr_program(rzr, state); > > > + > > > + return 0; > > > + } > > > + > > > + compose->left = 0; > > > + compose->top = 0; > > > + clamp_t(unsigned int, compose->left, 0, sink_fmt->width); > > > + clamp_t(unsigned int, compose->top, 0, sink_fmt->height); > > > + clamp_t(unsigned int, compose->width, crop->width / 8, crop->width); > > > + clamp_t(unsigned int, compose->height, crop->height / 8, crop->height); > > > + > > > + sel->r = sel->target == V4L2_SEL_TGT_CROP ? *crop : *compose; > > > + > > > + return 0; > > > +} > > > + > > > +static int mali_c55_rzr_set_routing(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state, > > > + enum v4l2_subdev_format_whence which, > > > + struct v4l2_subdev_krouting *routing) > > > +{ > > > + if (which == V4L2_SUBDEV_FORMAT_ACTIVE && > > > + media_entity_is_streaming(&sd->entity)) > > > + return -EBUSY; > > > + > > > + return __mali_c55_rzr_set_routing(sd, state, routing); > > > +} > > > + > > > +static const struct v4l2_subdev_pad_ops mali_c55_resizer_pad_ops = { > > > + .enum_mbus_code = mali_c55_rzr_enum_mbus_code, > > > + .enum_frame_size = mali_c55_rzr_enum_frame_size, > > > + .get_fmt = v4l2_subdev_get_fmt, > > > + .set_fmt = mali_c55_rzr_set_fmt, > > > + .get_selection = mali_c55_rzr_get_selection, > > > + .set_selection = mali_c55_rzr_set_selection, > > > + .set_routing = mali_c55_rzr_set_routing, > > > +}; > > > + > > > +void mali_c55_rzr_start_stream(struct mali_c55_resizer *rzr) > > Could this be handled through the .enable_streams() and > .disable_streams() operations ? They ensure that the stream state stored > internal is correct. That may not matter much today, but I think it will > become increasingly important in the future for the V4L2 core. > > > > +{ > > > + struct mali_c55 *mali_c55 = rzr->mali_c55; > > > + struct v4l2_subdev *sd = &rzr->sd; > > > + struct v4l2_subdev_state *state; > > > + unsigned int sink_pad; > > > + > > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > > + > > > + sink_pad = mali_c55_rzr_get_active_sink(state); > > > + if (sink_pad == MALI_C55_RZR_SINK_BYPASS_PAD) { > > > + /* Bypass FR pipe processing if the bypass route is active. */ > > > + mali_c55_update_bits(mali_c55, MALI_C55_REG_ISP_RAW_BYPASS, > > > + MALI_C55_ISP_RAW_BYPASS_FR_BYPASS_MASK, > > > + MALI_C55_ISP_RAW_BYPASS_RAW_FR_BYPASS); > > > + goto unlock_state; > > > + } > > > + > > > + /* Disable bypass and use regular processing. */ > > > + mali_c55_update_bits(mali_c55, MALI_C55_REG_ISP_RAW_BYPASS, > > > + MALI_C55_ISP_RAW_BYPASS_FR_BYPASS_MASK, 0); > > > + mali_c55_rzr_program(rzr, state); > > > + > > > +unlock_state: > > > + rzr->streaming = true; > > And hopefully you'll be able to replace this with > v4l2_subdev_is_streaming(), introduced in "[PATCH v6 00/11] media: > subdev: Improve stream enable/disable machinery" (Sakari has sent a pull > request for v6.11 yesterday). > > > > + v4l2_subdev_unlock_state(state); > > > +} > > > + > > > +void mali_c55_rzr_stop_stream(struct mali_c55_resizer *rzr) > > > +{ > > > + struct v4l2_subdev *sd = &rzr->sd; > > > + struct v4l2_subdev_state *state; > > > + > > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > > + rzr->streaming = false; > > > + v4l2_subdev_unlock_state(state); > > > +} > > > + > > > +static const struct v4l2_subdev_ops mali_c55_resizer_ops = { > > > + .pad = &mali_c55_resizer_pad_ops, > > > +}; > > > + > > > +static int mali_c55_rzr_init_state(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state) > > > +{ > > > + struct mali_c55_resizer *rzr = container_of(sd, struct mali_c55_resizer, > > > + sd); > > > + struct v4l2_subdev_krouting routing = { }; > > > + struct v4l2_subdev_route *routes; > > > + unsigned int i; > > > + int ret; > > > + > > > + routes = kcalloc(rzr->num_routes, sizeof(*routes), GFP_KERNEL); > > > + if (!routes) > > > + return -ENOMEM; > > > + > > > + for (i = 0; i < rzr->num_routes; ++i) { > > > + struct v4l2_subdev_route *route = &routes[i]; > > > + > > > + route->sink_pad = i > > > + ? MALI_C55_RZR_SINK_BYPASS_PAD > > > + : MALI_C55_RZR_SINK_PAD; > > > + route->source_pad = MALI_C55_RZR_SOURCE_PAD; > > > + if (route->sink_pad != MALI_C55_RZR_SINK_BYPASS_PAD) > > > + route->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE; > > > + } > > > + > > > + routing.num_routes = rzr->num_routes; > > > + routing.routes = routes; > > > + > > > + ret = __mali_c55_rzr_set_routing(sd, state, &routing); > > > + kfree(routes); > > > + > > > + return ret; > > I think this could be simplified. > > struct v4l2_subdev_route routes[2] = { > { > .sink_pad = MALI_C55_RZR_SINK_PAD, > .source_pad = MALI_C55_RZR_SOURCE_PAD, > .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > }, { > .sink_pad = MALI_C55_RZR_SINK_BYPASS_PAD, > .source_pad = MALI_C55_RZR_SOURCE_PAD, > }, > }; > struct v4l2_subdev_krouting routing = { > .num_routes = rzr->num_routes, > .routes = routes, > }; > > return __mali_c55_rzr_set_routing(sd, state, &routing); > > > > +} > > > + > > > +static const struct v4l2_subdev_internal_ops mali_c55_resizer_internal_ops = { > > > + .init_state = mali_c55_rzr_init_state, > > > +}; > > > + > > > +static void mali_c55_resizer_program_coefficients(struct mali_c55 *mali_c55, > > > + unsigned int index) > > > +{ > > > + const unsigned int scaler_filt_coefmem_addrs[][2] = { > > > + [MALI_C55_RZR_FR] = { > > > + 0x034A8, /* hfilt */ > > > + 0x044A8 /* vfilt */ > > > > Lowercase hex constants. > > And addresses belong to the mali-c55-registers.h file. > > > > + }, > > > + [MALI_C55_RZR_DS] = { > > > + 0x014A8, /* hfilt */ > > > + 0x024A8 /* vfilt */ > > > + }, > > > + }; > > > + unsigned int haddr = scaler_filt_coefmem_addrs[index][0]; > > > + unsigned int vaddr = scaler_filt_coefmem_addrs[index][1]; > > > + unsigned int i, j; > > > + > > > + for (i = 0; i < MALI_C55_RESIZER_COEFS_NUM_BANKS; i++) { > > > + for (j = 0; j < MALI_C55_RESIZER_COEFS_NUM_ENTRIES; j++) { > > > + mali_c55_write(mali_c55, haddr, > > > + mali_c55_scaler_h_filter_coefficients[i][j]); > > > + mali_c55_write(mali_c55, vaddr, > > > + mali_c55_scaler_v_filter_coefficients[i][j]); > > > + > > > + haddr += sizeof(u32); > > > + vaddr += sizeof(u32); > > > + } > > > + } > > How about memcpy_toio() ? I suppose this function isn't > performance sensitive, so maybe usage of mali_c55_write() is better from > a consistency point of view. > > > > +} > > > + > > > +int mali_c55_register_resizers(struct mali_c55 *mali_c55) > > > +{ > > > + unsigned int i; > > > + int ret; > > > + > > > + for (i = 0; i < MALI_C55_NUM_RZRS; ++i) { > > Moving the inner content to a separate mali_c55_register_resizer() > function would increase readability I think, and remove usage of gotos. > I would probably do the same for unregistration too, for consistency. > > > > + struct mali_c55_resizer *rzr = &mali_c55->resizers[i]; > > > + struct v4l2_subdev *sd = &rzr->sd; > > > + unsigned int num_pads = MALI_C55_RZR_NUM_PADS; > > > + > > > + rzr->id = i; > > > + rzr->streaming = false; > > > + > > > + if (rzr->id == MALI_C55_RZR_FR) > > > + rzr->cap_dev = &mali_c55->cap_devs[MALI_C55_CAP_DEV_FR]; > > > + else > > > + rzr->cap_dev = &mali_c55->cap_devs[MALI_C55_CAP_DEV_DS]; > > > + > > > + mali_c55_resizer_program_coefficients(mali_c55, i); > > Should this be done at stream start, given that power may be cut off > between streaming sessions ? > > > > + > > > + v4l2_subdev_init(sd, &mali_c55_resizer_ops); > > > + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE > > > + | V4L2_SUBDEV_FL_STREAMS; > > > + sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER; > > > + sd->internal_ops = &mali_c55_resizer_internal_ops; > > > + snprintf(sd->name, ARRAY_SIZE(sd->name), "%s %s", > > snprintf(sd->name, ARRAY_SIZE(sd->name), "%s resizer %s", > > and drop the "resizer " prefix from mali_c55_resizer_names. You can also > make mali_c55_resizer_names a local static const variable. > > > > + MALI_C55_DRIVER_NAME, mali_c55_resizer_names[i]); > > > + > > > + rzr->pads[MALI_C55_RZR_SINK_PAD].flags = MEDIA_PAD_FL_SINK; > > > + rzr->pads[MALI_C55_RZR_SOURCE_PAD].flags = MEDIA_PAD_FL_SOURCE; > > > + > > > + /* Only the FR pipe has a bypass pad. */ > > > + if (rzr->id == MALI_C55_RZR_FR) { > > > + rzr->pads[MALI_C55_RZR_SINK_BYPASS_PAD].flags = > > > + MEDIA_PAD_FL_SINK; > > > + rzr->num_routes = 2; > > > + } else { > > > + num_pads -= 1; > > > + rzr->num_routes = 1; > > > + } > > > + > > > + ret = media_entity_pads_init(&sd->entity, num_pads, rzr->pads); > > > + if (ret) > > > + return ret; > > > + > > > + ret = v4l2_subdev_init_finalize(sd); > > > + if (ret) > > > + goto err_cleanup; > > > + > > > + ret = v4l2_device_register_subdev(&mali_c55->v4l2_dev, sd); > > > + if (ret) > > > + goto err_cleanup; > > > + > > > + rzr->mali_c55 = mali_c55; > > > + } > > > + > > > + return 0; > > > + > > > +err_cleanup: > > > + for (; i >= 0; --i) { > > > + struct mali_c55_resizer *rzr = &mali_c55->resizers[i]; > > > + struct v4l2_subdev *sd = &rzr->sd; > > > + > > > + v4l2_subdev_cleanup(sd); > > > + media_entity_cleanup(&sd->entity); > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +void mali_c55_unregister_resizers(struct mali_c55 *mali_c55) > > > +{ > > > + unsigned int i; > > > + > > > + for (i = 0; i < MALI_C55_NUM_RZRS; i++) { > > > + struct mali_c55_resizer *resizer = &mali_c55->resizers[i]; > > > + > > > + if (!resizer->mali_c55) > > > + continue; > > > + > > > + v4l2_device_unregister_subdev(&resizer->sd); > > > + v4l2_subdev_cleanup(&resizer->sd); > > > + media_entity_cleanup(&resizer->sd.entity); > > > + } > > > +}
On Thu, Jun 06, 2024 at 02:47:08PM +0200, Jacopo Mondi wrote: > Hi Laurent > > On Fri, May 31, 2024 at 12:43:48AM GMT, Laurent Pinchart wrote: > > And now the second part of the review, addressing mali-c55-capture.c and > > mali-c55-resizer.c. I've reviewed the code from the bottom up, so some > > messages may be repeated in an order that seems weird. Sorry about that. > > [snip] > > A few replies/questions on the resizer module > > > > > + > > > > +#endif /* _MALI_C55_RESIZER_COEFS_H */ > > > > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c > > > > new file mode 100644 > > > > index 000000000000..0a5a2969d3ce > > > > --- /dev/null > > > > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c > > > > @@ -0,0 +1,779 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * ARM Mali-C55 ISP Driver - Image signal processor > > > > + * > > > > + * Copyright (C) 2024 Ideas on Board Oy > > > > + */ > > > > + > > > > +#include <linux/math.h> > > > > +#include <linux/minmax.h> > > > > + > > > > +#include <media/media-entity.h> > > > > +#include <media/v4l2-subdev.h> > > > > + > > > > +#include "mali-c55-common.h" > > > > +#include "mali-c55-registers.h" > > > > +#include "mali-c55-resizer-coefs.h" > > > > + > > > > +/* Scaling factor in Q4.20 format. */ > > > > +#define MALI_C55_RZR_SCALER_FACTOR (1U << 20) > > > > + > > > > +static const u32 rzr_non_bypass_src_fmts[] = { > > > > + MEDIA_BUS_FMT_RGB121212_1X36, > > > > + MEDIA_BUS_FMT_YUV10_1X30 > > > > +}; > > > > + > > > > +static const char * const mali_c55_resizer_names[] = { > > > > + [MALI_C55_RZR_FR] = "resizer fr", > > > > + [MALI_C55_RZR_DS] = "resizer ds", > > > > +}; > > > > + > > > > +static int mali_c55_rzr_program_crop(struct mali_c55_resizer *rzr, > > > > + struct v4l2_subdev_state *state) > > > > +{ > > > > + unsigned int reg_offset = rzr->cap_dev->reg_offset; > > > > + struct mali_c55 *mali_c55 = rzr->mali_c55; > > > > + struct v4l2_mbus_framefmt *fmt; > > > > + struct v4l2_rect *crop; > > > > const > > > > > > + > > > > + /* Verify if crop should be enabled. */ > > > > + fmt = v4l2_subdev_state_get_format(state, MALI_C55_RZR_SINK_PAD, 0); > > > > + crop = v4l2_subdev_state_get_crop(state, MALI_C55_RZR_SINK_PAD, 0); > > > > + > > > > + if (fmt->width == crop->width && fmt->height == crop->height) > > > > + return MALI_C55_BYPASS_CROP; > > > > + > > > > + mali_c55_write(mali_c55, MALI_C55_REG_CROP_X_START(reg_offset), > > > > + crop->left); > > > > + mali_c55_write(mali_c55, MALI_C55_REG_CROP_Y_START(reg_offset), > > > > + crop->top); > > > > + mali_c55_write(mali_c55, MALI_C55_REG_CROP_X_SIZE(reg_offset), > > > > + crop->width); > > > > + mali_c55_write(mali_c55, MALI_C55_REG_CROP_Y_SIZE(reg_offset), > > > > + crop->height); > > > > + > > > > + mali_c55_write(mali_c55, MALI_C55_REG_CROP_EN(reg_offset), > > > > + MALI_C55_CROP_ENABLE); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int mali_c55_rzr_program_resizer(struct mali_c55_resizer *rzr, > > > > + struct v4l2_subdev_state *state) > > > > +{ > > > > + unsigned int reg_offset = rzr->cap_dev->reg_offset; > > > > + struct mali_c55 *mali_c55 = rzr->mali_c55; > > > > + struct v4l2_rect *crop, *scale; > > > > const > > > > Once "[PATCH v4 0/3] media: v4l2-subdev: Support const-awareness in > > state accessors" gets merged, the state argument to this function can be > > made const too. Same for other functions, as applicable. > > > > > > + unsigned int h_bank, v_bank; > > > > + u64 h_scale, v_scale; > > > > + > > > > + /* Verify if scaling should be enabled. */ > > > > + crop = v4l2_subdev_state_get_crop(state, MALI_C55_RZR_SINK_PAD, 0); > > > > + scale = v4l2_subdev_state_get_compose(state, MALI_C55_RZR_SINK_PAD, 0); > > > > + > > > > + if (crop->width == scale->width && crop->height == scale->height) > > > > + return MALI_C55_BYPASS_SCALER; > > > > + > > > > + /* Program the V/H scaling factor in Q4.20 format. */ > > > > + h_scale = crop->width * MALI_C55_RZR_SCALER_FACTOR; > > > > + v_scale = crop->height * MALI_C55_RZR_SCALER_FACTOR; > > > > + > > > > + do_div(h_scale, scale->width); > > > > + do_div(v_scale, scale->height); > > > > + > > > > + mali_c55_write(mali_c55, > > > > + MALI_C55_REG_SCALER_IN_WIDTH(reg_offset), > > > > + crop->width); > > > > + mali_c55_write(mali_c55, > > > > + MALI_C55_REG_SCALER_IN_HEIGHT(reg_offset), > > > > + crop->height); > > > > + > > > > + mali_c55_write(mali_c55, > > > > + MALI_C55_REG_SCALER_OUT_WIDTH(reg_offset), > > > > + scale->width); > > > > + mali_c55_write(mali_c55, > > > > + MALI_C55_REG_SCALER_OUT_HEIGHT(reg_offset), > > > > + scale->height); > > > > + > > > > + mali_c55_write(mali_c55, > > > > + MALI_C55_REG_SCALER_HFILT_TINC(reg_offset), > > > > + h_scale); > > > > + mali_c55_write(mali_c55, > > > > + MALI_C55_REG_SCALER_VFILT_TINC(reg_offset), > > > > + v_scale); > > > > + > > > > + h_bank = mali_c55_calculate_bank_num(mali_c55, crop->width, > > > > + scale->width); > > > > + mali_c55_write(mali_c55, > > > > + MALI_C55_REG_SCALER_HFILT_COEF(reg_offset), > > > > + h_bank); > > > > + > > > > + v_bank = mali_c55_calculate_bank_num(mali_c55, crop->height, > > > > + scale->height); > > > > + mali_c55_write(mali_c55, > > > > + MALI_C55_REG_SCALER_VFILT_COEF(reg_offset), > > > > + v_bank); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static void mali_c55_rzr_program(struct mali_c55_resizer *rzr, > > > > + struct v4l2_subdev_state *state) > > > > +{ > > > > + struct mali_c55 *mali_c55 = rzr->mali_c55; > > > > + u32 bypass = 0; > > > > + > > > > + /* Verify if cropping and scaling should be enabled. */ > > > > + bypass |= mali_c55_rzr_program_crop(rzr, state); > > > > + bypass |= mali_c55_rzr_program_resizer(rzr, state); > > > > + > > > > + mali_c55_update_bits(mali_c55, rzr->id == MALI_C55_RZR_FR ? > > > > + MALI_C55_REG_FR_BYPASS : MALI_C55_REG_DS_BYPASS, > > > > + MALI_C55_BYPASS_CROP | MALI_C55_BYPASS_SCALER, > > > > + bypass); > > > > +} > > > > + > > > > +/* > > > > + * Inspect the routing table to know which of the two (mutually exclusive) > > > > + * routes is enabled and return the sink pad id of the active route. > > > > + */ > > > > +static unsigned int mali_c55_rzr_get_active_sink(struct v4l2_subdev_state *state) > > > > +{ > > > > + struct v4l2_subdev_krouting *routing = &state->routing; > > > > + struct v4l2_subdev_route *route; > > > > + > > > > + /* A single route is enabled at a time. */ > > > > + for_each_active_route(routing, route) > > > > + return route->sink_pad; > > > > + > > > > + return MALI_C55_RZR_SINK_PAD; > > > > +} > > > > + > > > > +static u32 mali_c55_rzr_shift_mbus_code(u32 mbus_code) > > > > +{ > > > > + u32 corrected_code = 0; > > > > + > > > > + /* > > > > + * The ISP takes input in a 20-bit format, but can only output 16-bit > > > > + * RAW bayer data (with the 4 least significant bits from the input > > > > + * being lost). Return the 16-bit version of the 20-bit input formats. > > > > + */ > > > > + switch (mbus_code) { > > > > + case MEDIA_BUS_FMT_SBGGR20_1X20: > > > > + corrected_code = MEDIA_BUS_FMT_SBGGR16_1X16; > > > > + break; > > > > + case MEDIA_BUS_FMT_SGBRG20_1X20: > > > > + corrected_code = MEDIA_BUS_FMT_SGBRG16_1X16; > > > > + break; > > > > + case MEDIA_BUS_FMT_SGRBG20_1X20: > > > > + corrected_code = MEDIA_BUS_FMT_SGRBG16_1X16; > > > > + break; > > > > + case MEDIA_BUS_FMT_SRGGB20_1X20: > > > > + corrected_code = MEDIA_BUS_FMT_SRGGB16_1X16; > > > > + break; > > > > Would it make sense to add the shifted code to mali_c55_isp_fmt ? > > > > > > + } > > > > + > > > > + return corrected_code; > > > > +} > > > > + > > > > +static int __mali_c55_rzr_set_routing(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *state, > > > > + struct v4l2_subdev_krouting *routing) > > > > I think the last argument can be const. > > If I have to adjust the routing table instead of refusing it, it can't Indeed, my bad. I wrote that based on the current implementation and forgot to modify the comment after. > > > > +{ > > > > + struct mali_c55_resizer *rzr = container_of(sd, struct mali_c55_resizer, > > > > + sd); > > > > A to_mali_c55_resizer() static inline function would be useful. Same for > > other components, where applicable. > > > > > > + unsigned int active_sink = UINT_MAX; > > > > + struct v4l2_mbus_framefmt *src_fmt; > > > > + struct v4l2_rect *crop, *compose; > > > > + struct v4l2_subdev_route *route; > > > > + unsigned int active_routes = 0; > > > > + struct v4l2_mbus_framefmt *fmt; > > > > + int ret; > > > > + > > > > + ret = v4l2_subdev_routing_validate(sd, routing, 0); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* Only a single route can be enabled at a time. */ > > > > + for_each_active_route(routing, route) { > > > > + if (++active_routes > 1) { > > > > + dev_err(rzr->mali_c55->dev, > > > > + "Only one route can be active"); > > > > No kernel log message with a level higher than dev_dbg() from > > user-controlled paths please, here and where applicable. This is to > > avoid giving applications an easy way to flood the kernel log. > > > > > > + return -EINVAL; > > > > + } > > > > + > > > > + active_sink = route->sink_pad; > > > > + } > > > > + if (active_sink == UINT_MAX) { > > > > + dev_err(rzr->mali_c55->dev, "One route has to be active"); > > > > + return -EINVAL; > > > > + } > > > > The recommended handling of invalid routing is to adjust the routing > > table, not to return errors. > > How should I adjust it ? The error here is due to the fact multiple > routes are set as active, which one should I make active ? the first > one ? Should I go and reset the flags in the subdev_route for the one > that has to be made non-active ? The same way you would adjust an invalid format, you can pick the route you consider should be the default. I'd like Sakari's and Tomi's opinions on this, as it's a new API and the behaviour is still a bit in flux. > > > > + > > > > + ret = v4l2_subdev_set_routing(sd, state, routing); > > > > + if (ret) { > > > > + dev_err(rzr->mali_c55->dev, "Failed to set routing\n"); > > > > + return ret; > > > > + } > > > > + > > > > + fmt = v4l2_subdev_state_get_format(state, active_sink, 0); > > > > + crop = v4l2_subdev_state_get_crop(state, active_sink, 0); > > > > + compose = v4l2_subdev_state_get_compose(state, active_sink, 0); > > > > + > > > > + fmt->width = MALI_C55_DEFAULT_WIDTH; > > > > + fmt->height = MALI_C55_DEFAULT_HEIGHT; > > > > + fmt->colorspace = V4L2_COLORSPACE_SRGB; > > > > There are other colorspace-related fields. > > > > > > + fmt->field = V4L2_FIELD_NONE; > > > > I wonder if we should really update the sink pad format, or just > > propagate it. If we update it, I think it should be set to defaults on > > both sink pads, not just the active sink pad. > > If only one route can be active, there will only be one state.stream_config > entry for the active sink, not for the other one (see > v4l2_subdev_init_stream_configs()), this mean I can't reset both sink > formats ? I think you can change the format on pads not included in active routes. Again, new API, so thoughts are appreciated :-) > > > > + > > > > + if (active_sink == MALI_C55_RZR_SINK_PAD) { > > > > + fmt->code = MEDIA_BUS_FMT_RGB121212_1X36; > > > > + > > > > + crop->left = crop->top = 0; > > > > crop->left = 0; > > crop->top = 0; > > > > > > + crop->width = MALI_C55_DEFAULT_WIDTH; > > > > + crop->height = MALI_C55_DEFAULT_HEIGHT; > > > > + > > > > + *compose = *crop; > > > > + } else { > > > > + fmt->code = MEDIA_BUS_FMT_SRGGB20_1X20; > > > > + } > > > > + > > > > + /* Propagate the format to the source pad */ > > > > + src_fmt = v4l2_subdev_state_get_format(state, MALI_C55_RZR_SOURCE_PAD, > > > > + 0); > > > > + *src_fmt = *fmt; > > > > + > > > > + /* In the event this is the bypass pad the mbus code needs correcting */ > > > > + if (active_sink == MALI_C55_RZR_SINK_BYPASS_PAD) > > > > + src_fmt->code = mali_c55_rzr_shift_mbus_code(src_fmt->code); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int mali_c55_rzr_enum_mbus_code(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *state, > > > > + struct v4l2_subdev_mbus_code_enum *code) > > > > +{ > > > > + struct v4l2_mbus_framefmt *sink_fmt; > > > > + const struct mali_c55_isp_fmt *fmt; > > > > + unsigned int index = 0; > > > > + u32 sink_pad; > > > > + > > > > + switch (code->pad) { > > > > + case MALI_C55_RZR_SINK_PAD: > > > > + if (code->index) > > > > + return -EINVAL; > > > > + > > > > + code->code = MEDIA_BUS_FMT_RGB121212_1X36; > > > > + > > > > + return 0; > > > > + case MALI_C55_RZR_SOURCE_PAD: > > > > + sink_pad = mali_c55_rzr_get_active_sink(state); > > > > + sink_fmt = v4l2_subdev_state_get_format(state, sink_pad, 0); > > > > + > > > > + /* > > > > + * If the active route is from the Bypass sink pad, then the > > > > + * source pad is a simple passthrough of the sink format, > > > > + * downshifted to 16-bits. > > > > + */ > > > > + > > > > + if (sink_pad == MALI_C55_RZR_SINK_BYPASS_PAD) { > > > > + if (code->index) > > > > + return -EINVAL; > > > > + > > > > + code->code = mali_c55_rzr_shift_mbus_code(sink_fmt->code); > > > > + if (!code->code) > > > > + return -EINVAL; > > > > + > > > > + return 0; > > > > + } > > > > + > > > > + /* > > > > + * If the active route is from the non-bypass sink then we can > > > > + * select either RGB or conversion to YUV. > > > > + */ > > > > + > > > > + if (code->index >= ARRAY_SIZE(rzr_non_bypass_src_fmts)) > > > > + return -EINVAL; > > > > + > > > > + code->code = rzr_non_bypass_src_fmts[code->index]; > > > > + > > > > + return 0; > > > > + case MALI_C55_RZR_SINK_BYPASS_PAD: > > > > + for_each_mali_isp_fmt(fmt) { > > > > + if (index++ == code->index) { > > > > + code->code = fmt->code; > > > > + return 0; > > > > + } > > > > + } > > > > + > > > > + break; > > > > + } > > > > + > > > > + return -EINVAL; > > > > +} > > > > + > > > > +static int mali_c55_rzr_enum_frame_size(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *state, > > > > + struct v4l2_subdev_frame_size_enum *fse) > > > > +{ > > > > + if (fse->index) > > > > + return -EINVAL; > > > > + > > > > + fse->max_width = MALI_C55_MAX_WIDTH; > > > > + fse->max_height = MALI_C55_MAX_HEIGHT; > > > > + fse->min_width = MALI_C55_MIN_WIDTH; > > > > + fse->min_height = MALI_C55_MIN_HEIGHT; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int mali_c55_rzr_set_sink_fmt(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *state, > > > > + struct v4l2_subdev_format *format) > > > > +{ > > > > + struct v4l2_mbus_framefmt *fmt = &format->format; > > > > + struct v4l2_rect *rect; > > > > + unsigned int sink_pad; > > > > + > > > > + /* > > > > + * Clamp to min/max and then reset crop and compose rectangles to the > > > > + * newly applied size. > > > > + */ > > > > + clamp_t(unsigned int, fmt->width, > > > > + MALI_C55_MIN_WIDTH, MALI_C55_MAX_WIDTH); > > also, clamp_t doens't clamp in place > > fmt->width = clamp_t... Correct. I've commented on that in other places. > > > > + clamp_t(unsigned int, fmt->height, > > > > + MALI_C55_MIN_HEIGHT, MALI_C55_MAX_HEIGHT); > > > > Please check comments for other components related to the colorspace > > fields, to decide how to handle them here. > > > > > > + > > > > + sink_pad = mali_c55_rzr_get_active_sink(state); > > > > + if (sink_pad == MALI_C55_RZR_SINK_PAD) { > > > > The selection here should depend on format->pad, not the active sink > > pad. > > > > > > + fmt->code = MEDIA_BUS_FMT_RGB121212_1X36; > > > > + > > > > + rect = v4l2_subdev_state_get_crop(state, MALI_C55_RZR_SINK_PAD); > > > > + rect->left = 0; > > > > + rect->top = 0; > > > > + rect->width = fmt->width; > > > > + rect->height = fmt->height; > > > > + > > > > + rect = v4l2_subdev_state_get_compose(state, > > > > + MALI_C55_RZR_SINK_PAD); > > > > + rect->left = 0; > > > > + rect->top = 0; > > > > + rect->width = fmt->width; > > > > + rect->height = fmt->height; > > > > + } else { > > > > + /* > > > > + * Make sure the media bus code is one of the supported > > > > + * ISP input media bus codes. > > > > + */ > > > > + if (!mali_c55_isp_is_format_supported(fmt->code)) > > > > + fmt->code = MALI_C55_DEFAULT_MEDIA_BUS_FMT; > > And DEFAULT_MEDIA_BUS_FMT is not one of the supported input media bus > codes That's not good :-) > > > > + } > > > > + > > > > + *v4l2_subdev_state_get_format(state, sink_pad, 0) = *fmt; > > > > + *v4l2_subdev_state_get_format(state, MALI_C55_RZR_SOURCE_PAD, 0) = *fmt; > > > > Propagation to the source pad, however, should depend on the active > > route. If format->pad is routed to the source pad, you should propagate, > > otherwise, you shouldn't. > > > > > > + > > > > + return 0; > > I ended up with > > static int mali_c55_rsz_set_sink_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_state *state, > struct v4l2_subdev_format *format) > { > struct v4l2_mbus_framefmt *fmt = &format->format; > unsigned int active_sink; > struct v4l2_rect *rect; > > /* > * Clamp to min/max and then reset crop and compose rectangles to the > * newly applied size. > */ > fmt->width = clamp_t(unsigned int, fmt->width, MALI_C55_MIN_WIDTH, > MALI_C55_MAX_WIDTH); > fmt->height = clamp_t(unsigned int, fmt->height, MALI_C55_MIN_HEIGHT, > MALI_C55_MAX_HEIGHT); > > rect = v4l2_subdev_state_get_crop(state, format->pad); > rect->left = 0; > rect->top = 0; > rect->width = fmt->width; > rect->height = fmt->height; > > rect = v4l2_subdev_state_get_compose(state, format->pad); > rect->left = 0; > rect->top = 0; > rect->width = fmt->width; > rect->height = fmt->height; > > if (format->pad == MALI_C55_RSZ_SINK_BYPASS_PAD) { > /* > * Make sure the media bus code is one of the supported > * ISP input media bus codes. Default it to SRGGB otherwise. > */ > if (!mali_c55_isp_is_format_supported(fmt->code)) > fmt->code = MEDIA_BUS_FMT_SRGGB20_1X20; > } else { > fmt->code = MEDIA_BUS_FMT_RGB121212_1X36; > } I think you need to handle colour spaces. > > *v4l2_subdev_state_get_format(state, format->pad, 0) = *fmt; You can drop the last argument to the function, it's optional and defaults to 0. The recommendation is to not include it when the device doesn't support streams. Same elsewhere in the file. > > /* If format->pad is routed to the source pad, propagate the format. */ > active_sink = mali_c55_rsz_get_active_sink(state); > if (active_sink == format->pad) { You can also if (active_sink != format->pad) return 0; if you want to reduce indentation. Up to you. > > /* If the bypass route is used, downshift the code to 16bpp. */ > if (active_sink == MALI_C55_RSZ_SINK_BYPASS_PAD) > fmt->code = mali_c55_rsz_shift_mbus_code(fmt->code); > > *v4l2_subdev_state_get_format(state, > MALI_C55_RSZ_SOURCE_PAD, 0) = *fmt; > } > > return 0; > } > > > > > +} > > > > + > > > > +static int mali_c55_rzr_set_source_fmt(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *state, > > > > + struct v4l2_subdev_format *format) > > > > +{ > > > > + struct mali_c55_resizer *rzr = container_of(sd, struct mali_c55_resizer, > > > > + sd); > > > > + struct v4l2_mbus_framefmt *fmt = &format->format; > > > > + struct v4l2_mbus_framefmt *sink_fmt; > > > > + struct v4l2_rect *crop, *compose; > > > > + unsigned int sink_pad; > > > > + unsigned int i; > > > > + > > > > + sink_pad = mali_c55_rzr_get_active_sink(state); > > > > + sink_fmt = v4l2_subdev_state_get_format(state, sink_pad, 0); > > > > + crop = v4l2_subdev_state_get_crop(state, sink_pad, 0); > > > > + compose = v4l2_subdev_state_get_compose(state, sink_pad, 0); > > > > + > > > > + /* FR Bypass pipe. */ > > > > + > > > > + if (sink_pad == MALI_C55_RZR_SINK_BYPASS_PAD) { > > > > + /* > > > > + * Format on the source pad is the same as the one on the > > > > + * sink pad, downshifted to 16-bits. > > > > + */ > > > > + fmt->code = mali_c55_rzr_shift_mbus_code(sink_fmt->code); > > > > + if (!fmt->code) > > > > + return -EINVAL; > > > > + > > > > + /* RAW bypass disables scaling and cropping. */ > > > > + crop->top = compose->top = 0; > > > > + crop->left = compose->left = 0; > > > > + fmt->width = crop->width = compose->width = sink_fmt->width; > > > > + fmt->height = crop->height = compose->height = sink_fmt->height; > > > > I don't think this is right. This function sets the format on the source > > pad. Subdevs should propagate formats from the sink to the source, not > > the other way around. > > > > The only parameter that can be modified on the source pad (as far as I > > understand) is the media bus code. In the bypass path, I understand it's > > fixed, while in the other path, you can select between RGB and YUV. I > > think the following code is what you need to implement this function. > > > > static int mali_c55_rzr_set_source_fmt(struct v4l2_subdev *sd, > > struct v4l2_subdev_state *state, > > struct v4l2_subdev_format *format) > > { > > struct mali_c55_resizer *rzr = container_of(sd, struct mali_c55_resizer, > > sd); > > struct v4l2_mbus_framefmt *fmt; > > > > fmt = v4l2_subdev_state_get_format(state, format->pad); > > > > /* In the non-bypass path the output format can be selected. */ > > if (mali_c55_rzr_get_active_sink(state) == MALI_C55_RZR_SINK_PAD) { > > unsigned int i; > > > > fmt->code = format->format.code; > > > > for (i = 0; i < ARRAY_SIZE(rzr_non_bypass_src_fmts); i++) { > > if (fmt->code == rzr_non_bypass_src_fmts[i]) > > break; > > } > > > > if (i == ARRAY_SIZE(rzr_non_bypass_src_fmts)) > > fmt->code = rzr_non_bypass_src_fmts[0]; > > } > > > > format->format = *fmt; > > > > return 0; > > } > > Almost. Your proposal doesn't adjust format->format.width/height It does, on the last line: format->format = *fmt; *fmt is the format taken from the subdev state, so it has been initialized by .init_cfg() and updated by format propagation when setting a format on the sink pad, or when setting a selection rectangle. I like this structure better, where we take the format from the state, update the fields that can be updated, and then copy the result to the function argument to provide it to userspace. The alternative below updates the fields from the userspace-provided format instead, and then copies it to the state. That's more error-prone, as you need to explicitly update everything that can't be changed by userspace, while in the code above you update the fields that can be changed. A white list approach is safer than a black list. > I think the following is more appropriate > > static int mali_c55_rsz_set_source_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_state *state, > struct v4l2_subdev_format *format) > { > struct v4l2_mbus_framefmt *fmt = &format->format; > struct v4l2_mbus_framefmt *sink_fmt; > struct v4l2_rect *sink_compose; > unsigned int active_sink; > > active_sink = mali_c55_rsz_get_active_sink(state); > sink_fmt = v4l2_subdev_state_get_format(state, active_sink, 0); > sink_compose = v4l2_subdev_state_get_compose(state, active_sink, 0); > > /* > * The source pad format sizes come directly from the active sink pad > * compose rectangle. > */ > fmt->width = sink_compose->width; > fmt->height = sink_compose->height; > > if (active_sink == MALI_C55_RSZ_SINK_PAD) { > /* > * Regular processing pipe: RGB121212 can be color-space > * converted to YUV101010. > */ > unsigned int i; > > for (i = 0; i < ARRAY_SIZE(rsz_non_bypass_src_fmts); i++) { > if (fmt->code == rsz_non_bypass_src_fmts[i]) > break; > } > > if (i == ARRAY_SIZE(rsz_non_bypass_src_fmts)) > fmt->code = MEDIA_BUS_FMT_RGB121212_1X36; > } else { > /* > * Bypass pipe: the source format is the same as the bypass > * sink pad downshifted to 16bpp. > */ > fmt->code = mali_c55_rsz_shift_mbus_code(sink_fmt->code); > } > > *v4l2_subdev_state_get_format(state, MALI_C55_RSZ_SOURCE_PAD) = *fmt; > > return 0; > } > > I'll handle the colorspace fields as well Ah, yes :-) > > > > + > > > > + *v4l2_subdev_state_get_format(state, > > > > + MALI_C55_RZR_SOURCE_PAD) = *fmt; > > > > + > > > > + return 0; > > > > + } > > > > + > > > > + /* Regular processing pipe. */ > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(rzr_non_bypass_src_fmts); i++) { > > > > + if (fmt->code == rzr_non_bypass_src_fmts[i]) > > > > + break; > > > > + } > > > > + > > > > + if (i == ARRAY_SIZE(rzr_non_bypass_src_fmts)) { > > > > + dev_dbg(rzr->mali_c55->dev, > > > > + "Unsupported mbus code 0x%x: using default\n", > > > > + fmt->code); > > > > I think you can drop this message. > > > > > > + fmt->code = MEDIA_BUS_FMT_RGB121212_1X36; > > > > + } > > > > + > > > > + /* > > > > + * The source pad format size comes directly from the sink pad > > > > + * compose rectangle. > > > > + */ > > > > + fmt->width = compose->width; > > > > + fmt->height = compose->height; > > > > + > > > > + *v4l2_subdev_state_get_format(state, MALI_C55_RZR_SOURCE_PAD) = *fmt; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int mali_c55_rzr_set_fmt(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *state, > > > > + struct v4l2_subdev_format *format) > > > > +{ > > > > + /* > > > > + * On sink pads fmt is either fixed for the 'regular' processing > > > > + * pad or a RAW format or 20-bit wide RGB/YUV format for the FR bypass > > > > + * pad. > > > > + * > > > > + * On source pad sizes are the result of crop+compose on the sink > > > > + * pad sizes, while the format depends on the active route. > > > > + */ > > > > + > > > > + if (format->pad != MALI_C55_RZR_SOURCE_PAD) > > > > + return mali_c55_rzr_set_sink_fmt(sd, state, format); > > > > + > > > > + return mali_c55_rzr_set_source_fmt(sd, state, format); > > > > Nitpicking, > > > > if (format->pad == MALI_C55_RZR_SOURCE_PAD) > > return mali_c55_rzr_set_source_fmt(sd, state, format); > > > > return mali_c55_rzr_set_sink_fmt(sd, state, format); > > > > to match SOURCE_PAD and source_fmt. > > > > Done at the expense a bit more verbose check > > if (format->pad == MALI_C55_RSZ_SINK_PAD || > format->pad == MALI_C55_RSZ_SINK_BYPASS_PAD) > return mali_c55_rsz_set_sink_fmt(sd, state, format); > > > > > +} > > > > + > > > > +static int mali_c55_rzr_get_selection(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *state, > > > > + struct v4l2_subdev_selection *sel) > > > > +{ > > > > + if (sel->pad != MALI_C55_RZR_SINK_PAD) > > > > + return -EINVAL; > > > > + > > > > + if (sel->target != V4L2_SEL_TGT_CROP && > > > > + sel->target != V4L2_SEL_TGT_COMPOSE) > > > > + return -EINVAL; > > > > + > > > > + sel->r = sel->target == V4L2_SEL_TGT_CROP > > > > + ? *v4l2_subdev_state_get_crop(state, MALI_C55_RZR_SINK_PAD) > > > > + : *v4l2_subdev_state_get_compose(state, MALI_C55_RZR_SINK_PAD); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int mali_c55_rzr_set_selection(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *state, > > > > + struct v4l2_subdev_selection *sel) > > > > +{ > > > > + struct mali_c55_resizer *rzr = container_of(sd, struct mali_c55_resizer, > > > > + sd); > > > > + struct v4l2_mbus_framefmt *source_fmt; > > > > + struct v4l2_mbus_framefmt *sink_fmt; > > > > + struct v4l2_rect *crop, *compose; > > > > + > > > > + if (sel->pad != MALI_C55_RZR_SINK_PAD) > > > > + return -EINVAL; > > > > + > > > > + if (sel->target != V4L2_SEL_TGT_CROP && > > > > + sel->target != V4L2_SEL_TGT_COMPOSE) > > > > + return -EINVAL; > > > > + > > > > + source_fmt = v4l2_subdev_state_get_format(state, > > > > + MALI_C55_RZR_SOURCE_PAD); > > > > + sink_fmt = v4l2_subdev_state_get_format(state, MALI_C55_RZR_SINK_PAD); > > > > + crop = v4l2_subdev_state_get_crop(state, MALI_C55_RZR_SINK_PAD); > > > > + compose = v4l2_subdev_state_get_compose(state, MALI_C55_RZR_SINK_PAD); > > > > + > > > > + /* RAW bypass disables crop/scaling. */ > > > > + if (mali_c55_format_is_raw(source_fmt->code)) { > > > > + crop->top = compose->top = 0; > > > > + crop->left = compose->left = 0; > > > > + crop->width = compose->width = sink_fmt->width; > > > > + crop->height = compose->height = sink_fmt->height; > > > > + > > > > + sel->r = sel->target == V4L2_SEL_TGT_CROP ? *crop : *compose; > > > > + > > > > + return 0; > > > > + } > > > > + > > > > + /* During streaming, it is allowed to only change the crop rectangle. */ > > > > + if (rzr->streaming && sel->target != V4L2_SEL_TGT_CROP) > > > > + return -EINVAL; > > > > + > > > > + /* > > > > + * Update the desired target and then clamp the crop rectangle to the > > > > + * sink format sizes and the compose size to the crop sizes. > > > > + */ > > > > + if (sel->target == V4L2_SEL_TGT_CROP) > > > > + *crop = sel->r; > > > > + else > > > > + *compose = sel->r; > > > > + > > > > + clamp_t(unsigned int, crop->left, 0, sink_fmt->width); > > > > + clamp_t(unsigned int, crop->top, 0, sink_fmt->height); > > > > + clamp_t(unsigned int, crop->width, MALI_C55_MIN_WIDTH, > > > > + sink_fmt->width - crop->left); > > > > + clamp_t(unsigned int, crop->height, MALI_C55_MIN_HEIGHT, > > > > + sink_fmt->height - crop->top); > > > > + > > > > + if (rzr->streaming) { > > > > + /* > > > > + * Apply at runtime a crop rectangle on the resizer's sink only > > > > + * if it doesn't require re-programming the scaler output sizes > > > > + * as it would require changing the output buffer sizes as well. > > > > + */ > > > > + if (sel->r.width < compose->width || > > > > + sel->r.height < compose->height) > > > > + return -EINVAL; > > > > + > > > > + *crop = sel->r; > > > > + mali_c55_rzr_program(rzr, state); > > > > + > > > > + return 0; > > > > + } > > > > + > > > > + compose->left = 0; > > > > + compose->top = 0; > > > > + clamp_t(unsigned int, compose->left, 0, sink_fmt->width); > > > > + clamp_t(unsigned int, compose->top, 0, sink_fmt->height); > > > > + clamp_t(unsigned int, compose->width, crop->width / 8, crop->width); > > > > + clamp_t(unsigned int, compose->height, crop->height / 8, crop->height); > > > > + > > > > + sel->r = sel->target == V4L2_SEL_TGT_CROP ? *crop : *compose; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int mali_c55_rzr_set_routing(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *state, > > > > + enum v4l2_subdev_format_whence which, > > > > + struct v4l2_subdev_krouting *routing) > > > > +{ > > > > + if (which == V4L2_SUBDEV_FORMAT_ACTIVE && > > > > + media_entity_is_streaming(&sd->entity)) > > > > + return -EBUSY; > > > > + > > > > + return __mali_c55_rzr_set_routing(sd, state, routing); > > > > +} > > > > + > > > > +static const struct v4l2_subdev_pad_ops mali_c55_resizer_pad_ops = { > > > > + .enum_mbus_code = mali_c55_rzr_enum_mbus_code, > > > > + .enum_frame_size = mali_c55_rzr_enum_frame_size, > > > > + .get_fmt = v4l2_subdev_get_fmt, > > > > + .set_fmt = mali_c55_rzr_set_fmt, > > > > + .get_selection = mali_c55_rzr_get_selection, > > > > + .set_selection = mali_c55_rzr_set_selection, > > > > + .set_routing = mali_c55_rzr_set_routing, > > > > +}; > > > > + > > > > +void mali_c55_rzr_start_stream(struct mali_c55_resizer *rzr) > > > > Could this be handled through the .enable_streams() and > > .disable_streams() operations ? They ensure that the stream state stored > > internal is correct. That may not matter much today, but I think it will > > become increasingly important in the future for the V4L2 core. > > > > > > +{ > > > > + struct mali_c55 *mali_c55 = rzr->mali_c55; > > > > + struct v4l2_subdev *sd = &rzr->sd; > > > > + struct v4l2_subdev_state *state; > > > > + unsigned int sink_pad; > > > > + > > > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > > > + > > > > + sink_pad = mali_c55_rzr_get_active_sink(state); > > > > + if (sink_pad == MALI_C55_RZR_SINK_BYPASS_PAD) { > > > > + /* Bypass FR pipe processing if the bypass route is active. */ > > > > + mali_c55_update_bits(mali_c55, MALI_C55_REG_ISP_RAW_BYPASS, > > > > + MALI_C55_ISP_RAW_BYPASS_FR_BYPASS_MASK, > > > > + MALI_C55_ISP_RAW_BYPASS_RAW_FR_BYPASS); > > > > + goto unlock_state; > > > > + } > > > > + > > > > + /* Disable bypass and use regular processing. */ > > > > + mali_c55_update_bits(mali_c55, MALI_C55_REG_ISP_RAW_BYPASS, > > > > + MALI_C55_ISP_RAW_BYPASS_FR_BYPASS_MASK, 0); > > > > + mali_c55_rzr_program(rzr, state); > > > > + > > > > +unlock_state: > > > > + rzr->streaming = true; > > > > And hopefully you'll be able to replace this with > > v4l2_subdev_is_streaming(), introduced in "[PATCH v6 00/11] media: > > subdev: Improve stream enable/disable machinery" (Sakari has sent a pull > > request for v6.11 yesterday). > > > > > > + v4l2_subdev_unlock_state(state); > > > > +} > > > > + > > > > +void mali_c55_rzr_stop_stream(struct mali_c55_resizer *rzr) > > > > +{ > > > > + struct v4l2_subdev *sd = &rzr->sd; > > > > + struct v4l2_subdev_state *state; > > > > + > > > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > > > + rzr->streaming = false; > > > > + v4l2_subdev_unlock_state(state); > > > > +} > > > > + > > > > +static const struct v4l2_subdev_ops mali_c55_resizer_ops = { > > > > + .pad = &mali_c55_resizer_pad_ops, > > > > +}; > > > > + > > > > +static int mali_c55_rzr_init_state(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *state) > > > > +{ > > > > + struct mali_c55_resizer *rzr = container_of(sd, struct mali_c55_resizer, > > > > + sd); > > > > + struct v4l2_subdev_krouting routing = { }; > > > > + struct v4l2_subdev_route *routes; > > > > + unsigned int i; > > > > + int ret; > > > > + > > > > + routes = kcalloc(rzr->num_routes, sizeof(*routes), GFP_KERNEL); > > > > + if (!routes) > > > > + return -ENOMEM; > > > > + > > > > + for (i = 0; i < rzr->num_routes; ++i) { > > > > + struct v4l2_subdev_route *route = &routes[i]; > > > > + > > > > + route->sink_pad = i > > > > + ? MALI_C55_RZR_SINK_BYPASS_PAD > > > > + : MALI_C55_RZR_SINK_PAD; > > > > + route->source_pad = MALI_C55_RZR_SOURCE_PAD; > > > > + if (route->sink_pad != MALI_C55_RZR_SINK_BYPASS_PAD) > > > > + route->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE; > > > > + } > > > > + > > > > + routing.num_routes = rzr->num_routes; > > > > + routing.routes = routes; > > > > + > > > > + ret = __mali_c55_rzr_set_routing(sd, state, &routing); > > > > + kfree(routes); > > > > + > > > > + return ret; > > > > I think this could be simplified. > > > > struct v4l2_subdev_route routes[2] = { > > { > > .sink_pad = MALI_C55_RZR_SINK_PAD, > > .source_pad = MALI_C55_RZR_SOURCE_PAD, > > .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > > }, { > > .sink_pad = MALI_C55_RZR_SINK_BYPASS_PAD, > > .source_pad = MALI_C55_RZR_SOURCE_PAD, > > }, > > }; > > struct v4l2_subdev_krouting routing = { > > .num_routes = rzr->num_routes, > > .routes = routes, > > }; > > > > return __mali_c55_rzr_set_routing(sd, state, &routing); > > > > > > +} > > > > + > > > > +static const struct v4l2_subdev_internal_ops mali_c55_resizer_internal_ops = { > > > > + .init_state = mali_c55_rzr_init_state, > > > > +}; > > > > + > > > > +static void mali_c55_resizer_program_coefficients(struct mali_c55 *mali_c55, > > > > + unsigned int index) > > > > +{ > > > > + const unsigned int scaler_filt_coefmem_addrs[][2] = { > > > > + [MALI_C55_RZR_FR] = { > > > > + 0x034A8, /* hfilt */ > > > > + 0x044A8 /* vfilt */ > > > > > > Lowercase hex constants. > > > > And addresses belong to the mali-c55-registers.h file. > > > > > > + }, > > > > + [MALI_C55_RZR_DS] = { > > > > + 0x014A8, /* hfilt */ > > > > + 0x024A8 /* vfilt */ > > > > + }, > > > > + }; > > > > + unsigned int haddr = scaler_filt_coefmem_addrs[index][0]; > > > > + unsigned int vaddr = scaler_filt_coefmem_addrs[index][1]; > > > > + unsigned int i, j; > > > > + > > > > + for (i = 0; i < MALI_C55_RESIZER_COEFS_NUM_BANKS; i++) { > > > > + for (j = 0; j < MALI_C55_RESIZER_COEFS_NUM_ENTRIES; j++) { > > > > + mali_c55_write(mali_c55, haddr, > > > > + mali_c55_scaler_h_filter_coefficients[i][j]); > > > > + mali_c55_write(mali_c55, vaddr, > > > > + mali_c55_scaler_v_filter_coefficients[i][j]); > > > > + > > > > + haddr += sizeof(u32); > > > > + vaddr += sizeof(u32); > > > > + } > > > > + } > > > > How about memcpy_toio() ? I suppose this function isn't > > performance sensitive, so maybe usage of mali_c55_write() is better from > > a consistency point of view. > > > > > > +} > > > > + > > > > +int mali_c55_register_resizers(struct mali_c55 *mali_c55) > > > > +{ > > > > + unsigned int i; > > > > + int ret; > > > > + > > > > + for (i = 0; i < MALI_C55_NUM_RZRS; ++i) { > > > > Moving the inner content to a separate mali_c55_register_resizer() > > function would increase readability I think, and remove usage of gotos. > > I would probably do the same for unregistration too, for consistency. > > > > > > + struct mali_c55_resizer *rzr = &mali_c55->resizers[i]; > > > > + struct v4l2_subdev *sd = &rzr->sd; > > > > + unsigned int num_pads = MALI_C55_RZR_NUM_PADS; > > > > + > > > > + rzr->id = i; > > > > + rzr->streaming = false; > > > > + > > > > + if (rzr->id == MALI_C55_RZR_FR) > > > > + rzr->cap_dev = &mali_c55->cap_devs[MALI_C55_CAP_DEV_FR]; > > > > + else > > > > + rzr->cap_dev = &mali_c55->cap_devs[MALI_C55_CAP_DEV_DS]; > > > > + > > > > + mali_c55_resizer_program_coefficients(mali_c55, i); > > > > Should this be done at stream start, given that power may be cut off > > between streaming sessions ? > > > > > > + > > > > + v4l2_subdev_init(sd, &mali_c55_resizer_ops); > > > > + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE > > > > + | V4L2_SUBDEV_FL_STREAMS; > > > > + sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER; > > > > + sd->internal_ops = &mali_c55_resizer_internal_ops; > > > > + snprintf(sd->name, ARRAY_SIZE(sd->name), "%s %s", > > > > snprintf(sd->name, ARRAY_SIZE(sd->name), "%s resizer %s", > > > > and drop the "resizer " prefix from mali_c55_resizer_names. You can also > > make mali_c55_resizer_names a local static const variable. > > > > > > + MALI_C55_DRIVER_NAME, mali_c55_resizer_names[i]); > > > > + > > > > + rzr->pads[MALI_C55_RZR_SINK_PAD].flags = MEDIA_PAD_FL_SINK; > > > > + rzr->pads[MALI_C55_RZR_SOURCE_PAD].flags = MEDIA_PAD_FL_SOURCE; > > > > + > > > > + /* Only the FR pipe has a bypass pad. */ > > > > + if (rzr->id == MALI_C55_RZR_FR) { > > > > + rzr->pads[MALI_C55_RZR_SINK_BYPASS_PAD].flags = > > > > + MEDIA_PAD_FL_SINK; > > > > + rzr->num_routes = 2; > > > > + } else { > > > > + num_pads -= 1; > > > > + rzr->num_routes = 1; > > > > + } > > > > + > > > > + ret = media_entity_pads_init(&sd->entity, num_pads, rzr->pads); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = v4l2_subdev_init_finalize(sd); > > > > + if (ret) > > > > + goto err_cleanup; > > > > + > > > > + ret = v4l2_device_register_subdev(&mali_c55->v4l2_dev, sd); > > > > + if (ret) > > > > + goto err_cleanup; > > > > + > > > > + rzr->mali_c55 = mali_c55; > > > > + } > > > > + > > > > + return 0; > > > > + > > > > +err_cleanup: > > > > + for (; i >= 0; --i) { > > > > + struct mali_c55_resizer *rzr = &mali_c55->resizers[i]; > > > > + struct v4l2_subdev *sd = &rzr->sd; > > > > + > > > > + v4l2_subdev_cleanup(sd); > > > > + media_entity_cleanup(&sd->entity); > > > > + } > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +void mali_c55_unregister_resizers(struct mali_c55 *mali_c55) > > > > +{ > > > > + unsigned int i; > > > > + > > > > + for (i = 0; i < MALI_C55_NUM_RZRS; i++) { > > > > + struct mali_c55_resizer *resizer = &mali_c55->resizers[i]; > > > > + > > > > + if (!resizer->mali_c55) > > > > + continue; > > > > + > > > > + v4l2_device_unregister_subdev(&resizer->sd); > > > > + v4l2_subdev_cleanup(&resizer->sd); > > > > + media_entity_cleanup(&resizer->sd.entity); > > > > + } > > > > +}
On 06/06/2024 20:53, Laurent Pinchart wrote: >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + active_sink = route->sink_pad; >>>>> + } >>>>> + if (active_sink == UINT_MAX) { >>>>> + dev_err(rzr->mali_c55->dev, "One route has to be active"); >>>>> + return -EINVAL; >>>>> + } >>> The recommended handling of invalid routing is to adjust the routing >>> table, not to return errors. >> How should I adjust it ? The error here is due to the fact multiple >> routes are set as active, which one should I make active ? the first >> one ? Should I go and reset the flags in the subdev_route for the one >> that has to be made non-active ? > The same way you would adjust an invalid format, you can pick the route > you consider should be the default. > > I'd like Sakari's and Tomi's opinions on this, as it's a new API and the > behaviour is still a bit in flux. Well... My opinion is that the driver adjusting the given config parameters (for any ioctl) is awful and should be deprecated. If the user asks for X, and the driver adjusts it and returns Y, then the user has two options: fail, because it didn't get X (after possibly laborious field by field checks), or shrug it's virtual shoulders and accept Y and hope that things still work even though it wanted X. But maybe that was an answer to a question you didn't really ask =). I think setting it to default routing in case of an error is as fine as any other "fix" for the routing. It won't work anyway. But if the function sets default routing and returns 0 here, why would it return an error from v4l2_subdev_routing_validate()? Should it just set default routing in that case too? So should set_routing() ever return an error, if we can just set the default routing? In the VIDIOC_SUBDEV_S_ROUTING doc we do list some cases where EINVAL or E2BIG is returned. But only a few, and I think v4l2_subdev_routing_validate() will return errors for many other cases too. For what it's worth, the drivers I have written just return an error. It's simple for the driver and the user and works. If the consensus is that the drivers should instead set the default routing, or somehow mangle the given routing to an acceptable form, I can update those drivers accordingly. But we probably need to update the docs too to be a bit more clear what VIDIOC_SUBDEV_S_ROUTING will do (although are the other ioctls any clearer?). All that said, I think it's still a bit case-by-case. I don't think the drivers should always return an error if they get a routing table that's not 100% perfect. E.g. if a device supports two static routes, but the second one can be enabled or disabled, the driver should still accept a routing table from the user with only the first route present. Etc. For the specific case in this patch... I'd prefer returning an error, or if that's not ok, set default routing. Tomi
Moi, On Thu, Jun 06, 2024 at 10:10:14PM +0300, Tomi Valkeinen wrote: > On 06/06/2024 20:53, Laurent Pinchart wrote: > > > > > > + return -EINVAL; > > > > > > + } > > > > > > + > > > > > > + active_sink = route->sink_pad; > > > > > > + } > > > > > > + if (active_sink == UINT_MAX) { > > > > > > + dev_err(rzr->mali_c55->dev, "One route has to be active"); > > > > > > + return -EINVAL; > > > > > > + } > > > > The recommended handling of invalid routing is to adjust the routing > > > > table, not to return errors. > > > How should I adjust it ? The error here is due to the fact multiple > > > routes are set as active, which one should I make active ? the first > > > one ? Should I go and reset the flags in the subdev_route for the one > > > that has to be made non-active ? > > The same way you would adjust an invalid format, you can pick the route > > you consider should be the default. > > > > I'd like Sakari's and Tomi's opinions on this, as it's a new API and the > > behaviour is still a bit in flux. > > Well... My opinion is that the driver adjusting the given config parameters > (for any ioctl) is awful and should be deprecated. If the user asks for X, > and the driver adjusts it and returns Y, then the user has two options: > fail, because it didn't get X (after possibly laborious field by field > checks), or shrug it's virtual shoulders and accept Y and hope that things > still work even though it wanted X. This is still often the only way to tell what the hardware can do as the limitations in different cases (cropping and scaling for instance) can be arbitrary. The other option is that the user space has to know the hardware capabilities without them being available from the kernel. There could be cases of IOCTLs where returning an error if what was requested can't be performed exactly is workable in general, but then again having consistency across IOCTL behaviour is very beneficial as well. If you need something exactly, then I think you should check after the IOCTL that this is what you also got, beyond the IOCTL succeeding. > > But maybe that was an answer to a question you didn't really ask =). > > I think setting it to default routing in case of an error is as fine as any > other "fix" for the routing. It won't work anyway. > > But if the function sets default routing and returns 0 here, why would it > return an error from v4l2_subdev_routing_validate()? Should it just set > default routing in that case too? So should set_routing() ever return an > error, if we can just set the default routing? S_ROUTING is a bit special as it deals with multiple routes and the user space does have a way to add them incrementally. Perhaps we should document better what the driver is expected to to correct the routes? I'd think routes may be added by the driver (as some of them cannot be disabled for instance) but if a requested route cannot be created, that should probably be an error. I've copied my current (with all the pending patches) documentation here <URL:https://www.retiisi.eu/~sailus/v4l2/tmp/streams-doc/userspace-api/media/v4l/dev-subdev.html#streams-multiplexed-media-pads-and-internal-routing>. The text does not elaborate what exactly a driver could or should do, apart from specifying the condition for EINVAL. I think we should specify this in greater detail. My original thought wws the adjustment would be done by adding static routes omitted by the caller, not trying to come up with e.g. valid pad/stream pairs when user provided invalid ones. Could this correction functionality be limited to returning static routes? > > In the VIDIOC_SUBDEV_S_ROUTING doc we do list some cases where EINVAL or > E2BIG is returned. But only a few, and I think > v4l2_subdev_routing_validate() will return errors for many other cases too. > > For what it's worth, the drivers I have written just return an error. It's > simple for the driver and the user and works. If the consensus is that the > drivers should instead set the default routing, or somehow mangle the given > routing to an acceptable form, I can update those drivers accordingly. > > But we probably need to update the docs too to be a bit more clear what > VIDIOC_SUBDEV_S_ROUTING will do (although are the other ioctls any > clearer?). > > All that said, I think it's still a bit case-by-case. I don't think the > drivers should always return an error if they get a routing table that's not > 100% perfect. E.g. if a device supports two static routes, but the second > one can be enabled or disabled, the driver should still accept a routing > table from the user with only the first route present. Etc. > > For the specific case in this patch... I'd prefer returning an error, or if > that's not ok, set default routing. Not modifying the routing table is another option as well but it may require separating validating user-provided routes and applying the routes to the sub-device state. The default could be useful in principle, too, for routing-unaware applications but they won't be calling S_ROUTING anyway.
Hi Dan, On Fri, Jun 14, 2024 at 11:13:29AM +0100, Daniel Scally wrote: > On 30/05/2024 01:15, Laurent Pinchart wrote: > > On Wed, May 29, 2024 at 04:28:47PM +0100, Daniel Scally wrote: > >> Add a driver for Arm's Mali-C55 Image Signal Processor. The driver is > >> V4L2 and Media Controller compliant and creates subdevices to manage > >> the ISP itself, its internal test pattern generator as well as the > >> crop, scaler and output format functionality for each of its two > >> output devices. > >> > >> Acked-by: Nayden Kanchev <nayden.kanchev@arm.com> > >> Co-developed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > >> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > >> --- > >> Changes in v5: > >> > >> - Reworked input formats - previously we allowed representing input data > >> as any 8-16 bit format. Now we only allow input data to be represented > >> by the new 20-bit bayer formats, which is corrected to the equivalent > >> 16-bit format in RAW bypass mode. > >> - Stopped bypassing blocks that we haven't added supporting parameters > >> for yet. > >> - Addressed most of Sakari's comments from the list > >> > >> Changes not yet made in v5: > >> > >> - The output pipelines can still be started and stopped independently of > >> one another - I'd like to discuss that more. > >> - the TPG subdev still uses .s_stream() - I need to rebase onto a tree > >> with working .enable_streams() for a single-source-pad subdevice. > >> > >> Changes in v4: > >> > >> - Reworked mali_c55_update_bits() to internally perform the bit-shift > > > > I really don't like that, it makes the code very confusing, even more so > > as it differs from regmap_update_bits(). > > > > Look at this for instance: > > > > mali_c55_update_bits(mali_c55, MALI_C55_REG_MCU_CONFIG, > > MALI_C55_REG_MCU_CONFIG_OVERRIDE_MASK, > > MALI_C55_REG_MCU_CONFIG_OVERRIDE_MASK); > > > > It only works by change because MALI_C55_REG_MCU_CONFIG_OVERRIDE_MASK is > > BIT(0). > > > > Sorry, I know it will be painful, but this change needs to be reverted. > > I'd like to argue for keeping this, on the grounds that it's better. I got lazy in the change you > reference there, and because BIT(0) is the same as 0x01 didn't bother changing it. I agree that > that's confusing but I think it would be better to keep the change and just update all the call > sites properly. The benefits as I see them are two: > > > 1. This method ensures sane consistent calling of the function. Without the internal shift you have > to shift the values at the call site, but there's no reason to do that if the value you're setting > is 0x00 or if the field you're targeting in the register starts at bit 0, so I think writing code > naturally we'd have a mix of situations like so: > > > #define REG_1 0xfc00 > > #define REG_2 0xff > > mali_c55_update_bits(mali_c55, 0x1234, REG_1, 0x02 << 10); > > mali_c55_update_bits(mali_c55, 0x1234, REG_1, 0x00); > > mali_c55_update_bits(mali_c55, 0x1234, REG_2, 0x02); > > And I think that the mixture is more confusing than the difference with regmap_update_bits(). We > could include the bitshifting for consistencies sake despite it being unecessary, but it's extra > work for no real reason and itself "looks wrong" if the field starts at bit(0)...it would look less > wrong with an offset macro that defines the number of bits to shift as 0 but then we're on to > advantage #2... > > 2. It makes the driver far cleaner. Without it we either have magic numbers scattered throughout > (and sometimes have to calculate them with extra variables where the write can target different > places conditionally) or have macros defining the number of bits to shift, or have to do (ffs(mask) > - 1) everywhere, and that tends to make the call sites a lot messier - this was the original reason > I moved it internal actually. > > What do you think? All register values (possibly with the exception of 0) should have macros. The callers will thus not need to perform shifts manually, they will all be handled in the register fields macros. From a caller point of view, not handling the shifts inside mali_c55_update_bits() will not make a difference. It's the first time I notice a driver trying to shift internally in its update_bits function. I think that's really confusing, especially given that it departs from how regmap operates. I still strongly favour handling the shifts in the register macros. > >> - Reworked the resizer to allow cropping during streaming > >> - Fixed a bug in NV12 output > >> > >> Changes in v3: > >> > >> - Mostly minor fixes suggested by Sakari > >> - Fixed the sequencing of vb2 buffers to be synchronised across the two > >> capture devices. > >> > >> Changes in v2: > >> > >> - Clock handling > >> - Fixed the warnings raised by the kernel test robot > >> > >> drivers/media/platform/Kconfig | 1 + > >> drivers/media/platform/Makefile | 1 + > >> drivers/media/platform/arm/Kconfig | 5 + > >> drivers/media/platform/arm/Makefile | 2 + > >> drivers/media/platform/arm/mali-c55/Kconfig | 18 + > >> drivers/media/platform/arm/mali-c55/Makefile | 9 + > >> .../platform/arm/mali-c55/mali-c55-capture.c | 951 ++++++++++++++++++ > >> .../platform/arm/mali-c55/mali-c55-common.h | 266 +++++ > >> .../platform/arm/mali-c55/mali-c55-core.c | 767 ++++++++++++++ > >> .../platform/arm/mali-c55/mali-c55-isp.c | 611 +++++++++++ > >> .../arm/mali-c55/mali-c55-registers.h | 258 +++++ > >> .../arm/mali-c55/mali-c55-resizer-coefs.h | 382 +++++++ > >> .../platform/arm/mali-c55/mali-c55-resizer.c | 779 ++++++++++++++ > >> .../platform/arm/mali-c55/mali-c55-tpg.c | 402 ++++++++ > >> 14 files changed, 4452 insertions(+) > >> create mode 100644 drivers/media/platform/arm/Kconfig > >> create mode 100644 drivers/media/platform/arm/Makefile > >> create mode 100644 drivers/media/platform/arm/mali-c55/Kconfig > >> create mode 100644 drivers/media/platform/arm/mali-c55/Makefile > >> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-capture.c > >> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-common.h > >> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-core.c > >> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-isp.c > >> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-registers.h > >> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-resizer-coefs.h > >> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-resizer.c > >> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-tpg.c [snip]
On Sun, Jun 09, 2024 at 06:21:52AM +0000, Sakari Ailus wrote: > On Thu, Jun 06, 2024 at 10:10:14PM +0300, Tomi Valkeinen wrote: > > On 06/06/2024 20:53, Laurent Pinchart wrote: > > > > > > > + return -EINVAL; > > > > > > > + } > > > > > > > + > > > > > > > + active_sink = route->sink_pad; > > > > > > > + } > > > > > > > + if (active_sink == UINT_MAX) { > > > > > > > + dev_err(rzr->mali_c55->dev, "One route has to be active"); > > > > > > > + return -EINVAL; > > > > > > > + } > > > > > > > > > > The recommended handling of invalid routing is to adjust the routing > > > > > table, not to return errors. > > > > > > > > How should I adjust it ? The error here is due to the fact multiple > > > > routes are set as active, which one should I make active ? the first > > > > one ? Should I go and reset the flags in the subdev_route for the one > > > > that has to be made non-active ? > > > > > > The same way you would adjust an invalid format, you can pick the route > > > you consider should be the default. > > > > > > I'd like Sakari's and Tomi's opinions on this, as it's a new API and the > > > behaviour is still a bit in flux. > > > > Well... My opinion is that the driver adjusting the given config parameters > > (for any ioctl) is awful and should be deprecated. If the user asks for X, > > and the driver adjusts it and returns Y, then the user has two options: > > fail, because it didn't get X (after possibly laborious field by field > > checks), or shrug it's virtual shoulders and accept Y and hope that things > > still work even though it wanted X. > > This is still often the only way to tell what the hardware can do as the > limitations in different cases (cropping and scaling for instance) can be > arbitrary. The other option is that the user space has to know the hardware > capabilities without them being available from the kernel. For some parameters that make sense (we don't have a try mechanism for ISP parameters buffers for instance), but when it comes to configuring a pipeline, I think a parameters adjustment model is needed when we don't have means to expose constraints in a generic way to userspace. The question is in which category routing falls. > There could be cases of IOCTLs where returning an error if what was > requested can't be performed exactly is workable in general, but then again > having consistency across IOCTL behaviour is very beneficial as well. > > If you need something exactly, then I think you should check after the > IOCTL that this is what you also got, beyond the IOCTL succeeding. I do agree with Tomi that this kind of check can be annoying for applications. In cases where checking the result would be complex, and where there is very little use case for receiving anything but the exact configuration you asked for, adjusting the parameters could increase the implementation complexity on both the kernel side and userspace side for no or very little benefit. > > But maybe that was an answer to a question you didn't really ask =). > > > > I think setting it to default routing in case of an error is as fine as any > > other "fix" for the routing. It won't work anyway. > > > > But if the function sets default routing and returns 0 here, why would it > > return an error from v4l2_subdev_routing_validate()? Should it just set > > default routing in that case too? So should set_routing() ever return an > > error, if we can just set the default routing? That's a good point. I asked myself the same question after sending my previous e-mail, and wondered if anyone else would notice too :-) > S_ROUTING is a bit special as it deals with multiple routes and the user > space does have a way to add them incrementally. > > Perhaps we should document better what the driver is expected to to correct > the routes? We should document the expected behaviour clearly. After agreeing on the expected behaviour, that is. > I'd think routes may be added by the driver (as some of them cannot be > disabled for instance) but if a requested route cannot be created, that > should probably be an error. > > I've copied my current (with all the pending patches) documentation here > <URL:https://www.retiisi.eu/~sailus/v4l2/tmp/streams-doc/userspace-api/media/v4l/dev-subdev.html#streams-multiplexed-media-pads-and-internal-routing>. > > The text does not elaborate what exactly a driver could or should do, apart > from specifying the condition for EINVAL. I think we should specify this in I don't see mentions of EINVAL related to streams there, am I missing something ? > greater detail. My original thought wws the adjustment would be done by > adding static routes omitted by the caller, not trying to come up with e.g. > valid pad/stream pairs when user provided invalid ones. > > Could this correction functionality be limited to returning static routes? That would make userspace a tad simpler, and wouldn't be hard to do in the kernel, but I wonder if departing from the rule that invalid routing tables result in an error is worth it for such a small gain. > > In the VIDIOC_SUBDEV_S_ROUTING doc we do list some cases where EINVAL or > > E2BIG is returned. But only a few, and I think > > v4l2_subdev_routing_validate() will return errors for many other cases too. > > > > For what it's worth, the drivers I have written just return an error. It's > > simple for the driver and the user and works. If the consensus is that the > > drivers should instead set the default routing, or somehow mangle the given > > routing to an acceptable form, I can update those drivers accordingly. > > > > But we probably need to update the docs too to be a bit more clear what > > VIDIOC_SUBDEV_S_ROUTING will do (although are the other ioctls any > > clearer?). > > > > All that said, I think it's still a bit case-by-case. I don't think the > > drivers should always return an error if they get a routing table that's not > > 100% perfect. E.g. if a device supports two static routes, but the second > > one can be enabled or disabled, the driver should still accept a routing > > table from the user with only the first route present. Etc. > > > > For the specific case in this patch... I'd prefer returning an error, or if > > that's not ok, set default routing. > > Not modifying the routing table is another option as well but it may > require separating validating user-provided routes and applying the routes I'm not sure to follow you here. By not modifying the routing table, do you mean returning an error ? Why would that require separation of validation and configuration ? > to the sub-device state. The default could be useful in principle, too, for > routing-unaware applications but they won't be calling S_ROUTING anyway.
Morning Laurent On 16/06/2024 20:39, Laurent Pinchart wrote: > Hi Dan, > > On Fri, Jun 14, 2024 at 11:13:29AM +0100, Daniel Scally wrote: >> On 30/05/2024 01:15, Laurent Pinchart wrote: >>> On Wed, May 29, 2024 at 04:28:47PM +0100, Daniel Scally wrote: >>>> Add a driver for Arm's Mali-C55 Image Signal Processor. The driver is >>>> V4L2 and Media Controller compliant and creates subdevices to manage >>>> the ISP itself, its internal test pattern generator as well as the >>>> crop, scaler and output format functionality for each of its two >>>> output devices. >>>> >>>> Acked-by: Nayden Kanchev <nayden.kanchev@arm.com> >>>> Co-developed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>>> --- >>>> Changes in v5: >>>> >>>> - Reworked input formats - previously we allowed representing input data >>>> as any 8-16 bit format. Now we only allow input data to be represented >>>> by the new 20-bit bayer formats, which is corrected to the equivalent >>>> 16-bit format in RAW bypass mode. >>>> - Stopped bypassing blocks that we haven't added supporting parameters >>>> for yet. >>>> - Addressed most of Sakari's comments from the list >>>> >>>> Changes not yet made in v5: >>>> >>>> - The output pipelines can still be started and stopped independently of >>>> one another - I'd like to discuss that more. >>>> - the TPG subdev still uses .s_stream() - I need to rebase onto a tree >>>> with working .enable_streams() for a single-source-pad subdevice. >>>> >>>> Changes in v4: >>>> >>>> - Reworked mali_c55_update_bits() to internally perform the bit-shift >>> I really don't like that, it makes the code very confusing, even more so >>> as it differs from regmap_update_bits(). >>> >>> Look at this for instance: >>> >>> mali_c55_update_bits(mali_c55, MALI_C55_REG_MCU_CONFIG, >>> MALI_C55_REG_MCU_CONFIG_OVERRIDE_MASK, >>> MALI_C55_REG_MCU_CONFIG_OVERRIDE_MASK); >>> >>> It only works by change because MALI_C55_REG_MCU_CONFIG_OVERRIDE_MASK is >>> BIT(0). >>> >>> Sorry, I know it will be painful, but this change needs to be reverted. >> I'd like to argue for keeping this, on the grounds that it's better. I got lazy in the change you >> reference there, and because BIT(0) is the same as 0x01 didn't bother changing it. I agree that >> that's confusing but I think it would be better to keep the change and just update all the call >> sites properly. The benefits as I see them are two: >> >> >> 1. This method ensures sane consistent calling of the function. Without the internal shift you have >> to shift the values at the call site, but there's no reason to do that if the value you're setting >> is 0x00 or if the field you're targeting in the register starts at bit 0, so I think writing code >> naturally we'd have a mix of situations like so: >> >> >> #define REG_1 0xfc00 >> >> #define REG_2 0xff >> >> mali_c55_update_bits(mali_c55, 0x1234, REG_1, 0x02 << 10); >> >> mali_c55_update_bits(mali_c55, 0x1234, REG_1, 0x00); >> >> mali_c55_update_bits(mali_c55, 0x1234, REG_2, 0x02); >> >> And I think that the mixture is more confusing than the difference with regmap_update_bits(). We >> could include the bitshifting for consistencies sake despite it being unecessary, but it's extra >> work for no real reason and itself "looks wrong" if the field starts at bit(0)...it would look less >> wrong with an offset macro that defines the number of bits to shift as 0 but then we're on to >> advantage #2... >> >> 2. It makes the driver far cleaner. Without it we either have magic numbers scattered throughout >> (and sometimes have to calculate them with extra variables where the write can target different >> places conditionally) or have macros defining the number of bits to shift, or have to do (ffs(mask) >> - 1) everywhere, and that tends to make the call sites a lot messier - this was the original reason >> I moved it internal actually. >> >> What do you think? > All register values (possibly with the exception of 0) should have > macros. The callers will thus not need to perform shifts manually, they > will all be handled in the register fields macros. From a caller point > of view, not handling the shifts inside mali_c55_update_bits() will not > make a difference. > > It's the first time I notice a driver trying to shift internally in its > update_bits function. I think that's really confusing, especially given > that it departs from how regmap operates. I still strongly favour > handling the shifts in the register macros. Alright - I will handle it that way (in fact I already did). Thanks! > >>>> - Reworked the resizer to allow cropping during streaming >>>> - Fixed a bug in NV12 output >>>> >>>> Changes in v3: >>>> >>>> - Mostly minor fixes suggested by Sakari >>>> - Fixed the sequencing of vb2 buffers to be synchronised across the two >>>> capture devices. >>>> >>>> Changes in v2: >>>> >>>> - Clock handling >>>> - Fixed the warnings raised by the kernel test robot >>>> >>>> drivers/media/platform/Kconfig | 1 + >>>> drivers/media/platform/Makefile | 1 + >>>> drivers/media/platform/arm/Kconfig | 5 + >>>> drivers/media/platform/arm/Makefile | 2 + >>>> drivers/media/platform/arm/mali-c55/Kconfig | 18 + >>>> drivers/media/platform/arm/mali-c55/Makefile | 9 + >>>> .../platform/arm/mali-c55/mali-c55-capture.c | 951 ++++++++++++++++++ >>>> .../platform/arm/mali-c55/mali-c55-common.h | 266 +++++ >>>> .../platform/arm/mali-c55/mali-c55-core.c | 767 ++++++++++++++ >>>> .../platform/arm/mali-c55/mali-c55-isp.c | 611 +++++++++++ >>>> .../arm/mali-c55/mali-c55-registers.h | 258 +++++ >>>> .../arm/mali-c55/mali-c55-resizer-coefs.h | 382 +++++++ >>>> .../platform/arm/mali-c55/mali-c55-resizer.c | 779 ++++++++++++++ >>>> .../platform/arm/mali-c55/mali-c55-tpg.c | 402 ++++++++ >>>> 14 files changed, 4452 insertions(+) >>>> create mode 100644 drivers/media/platform/arm/Kconfig >>>> create mode 100644 drivers/media/platform/arm/Makefile >>>> create mode 100644 drivers/media/platform/arm/mali-c55/Kconfig >>>> create mode 100644 drivers/media/platform/arm/mali-c55/Makefile >>>> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-capture.c >>>> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-common.h >>>> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-core.c >>>> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-isp.c >>>> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-registers.h >>>> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-resizer-coefs.h >>>> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-resizer.c >>>> create mode 100644 drivers/media/platform/arm/mali-c55/mali-c55-tpg.c > [snip] >
Hi Laurent, On Sun, Jun 16, 2024 at 11:38:07PM +0300, Laurent Pinchart wrote: > On Sun, Jun 09, 2024 at 06:21:52AM +0000, Sakari Ailus wrote: > > On Thu, Jun 06, 2024 at 10:10:14PM +0300, Tomi Valkeinen wrote: > > > On 06/06/2024 20:53, Laurent Pinchart wrote: > > > > > > > > + return -EINVAL; > > > > > > > > + } > > > > > > > > + > > > > > > > > + active_sink = route->sink_pad; > > > > > > > > + } > > > > > > > > + if (active_sink == UINT_MAX) { > > > > > > > > + dev_err(rzr->mali_c55->dev, "One route has to be active"); > > > > > > > > + return -EINVAL; > > > > > > > > + } > > > > > > > > > > > > The recommended handling of invalid routing is to adjust the routing > > > > > > table, not to return errors. > > > > > > > > > > How should I adjust it ? The error here is due to the fact multiple > > > > > routes are set as active, which one should I make active ? the first > > > > > one ? Should I go and reset the flags in the subdev_route for the one > > > > > that has to be made non-active ? > > > > > > > > The same way you would adjust an invalid format, you can pick the route > > > > you consider should be the default. > > > > > > > > I'd like Sakari's and Tomi's opinions on this, as it's a new API and the > > > > behaviour is still a bit in flux. > > > > > > Well... My opinion is that the driver adjusting the given config parameters > > > (for any ioctl) is awful and should be deprecated. If the user asks for X, > > > and the driver adjusts it and returns Y, then the user has two options: > > > fail, because it didn't get X (after possibly laborious field by field > > > checks), or shrug it's virtual shoulders and accept Y and hope that things > > > still work even though it wanted X. > > > > This is still often the only way to tell what the hardware can do as the > > limitations in different cases (cropping and scaling for instance) can be > > arbitrary. The other option is that the user space has to know the hardware > > capabilities without them being available from the kernel. > > For some parameters that make sense (we don't have a try mechanism for > ISP parameters buffers for instance), but when it comes to configuring a > pipeline, I think a parameters adjustment model is needed when we don't > have means to expose constraints in a generic way to userspace. The > question is in which category routing falls. > > > There could be cases of IOCTLs where returning an error if what was > > requested can't be performed exactly is workable in general, but then again > > having consistency across IOCTL behaviour is very beneficial as well. > > > > If you need something exactly, then I think you should check after the > > IOCTL that this is what you also got, beyond the IOCTL succeeding. > > I do agree with Tomi that this kind of check can be annoying for > applications. In cases where checking the result would be complex, and > where there is very little use case for receiving anything but the exact > configuration you asked for, adjusting the parameters could increase the > implementation complexity on both the kernel side and userspace side for > no or very little benefit. > > > > But maybe that was an answer to a question you didn't really ask =). > > > > > > I think setting it to default routing in case of an error is as fine as any > > > other "fix" for the routing. It won't work anyway. > > > > > > But if the function sets default routing and returns 0 here, why would it > > > return an error from v4l2_subdev_routing_validate()? Should it just set > > > default routing in that case too? So should set_routing() ever return an > > > error, if we can just set the default routing? > > That's a good point. I asked myself the same question after sending my > previous e-mail, and wondered if anyone else would notice too :-) > > > S_ROUTING is a bit special as it deals with multiple routes and the user > > space does have a way to add them incrementally. > > > > Perhaps we should document better what the driver is expected to to correct > > the routes? > > We should document the expected behaviour clearly. After agreeing on the > expected behaviour, that is. > > > I'd think routes may be added by the driver (as some of them cannot be > > disabled for instance) but if a requested route cannot be created, that > > should probably be an error. > > > > I've copied my current (with all the pending patches) documentation here > > <URL:https://www.retiisi.eu/~sailus/v4l2/tmp/streams-doc/userspace-api/media/v4l/dev-subdev.html#streams-multiplexed-media-pads-and-internal-routing>. > > > > The text does not elaborate what exactly a driver could or should do, apart > > from specifying the condition for EINVAL. I think we should specify this in > > I don't see mentions of EINVAL related to streams there, am I missing > something ? > > > greater detail. My original thought wws the adjustment would be done by > > adding static routes omitted by the caller, not trying to come up with e.g. > > valid pad/stream pairs when user provided invalid ones. > > > > Could this correction functionality be limited to returning static routes? > > That would make userspace a tad simpler, and wouldn't be hard to do in > the kernel, but I wonder if departing from the rule that invalid routing > tables result in an error is worth it for such a small gain. I'm just referring to our previous decision on the matter. :-) Of course an application can do G_ROUTING, toggle the routes it needs to and call S_ROUTING again, in order to be (fairly) certain it'll succeed. Say, if an application wants to enable an embedded data route, then it'll be required to supply the route for the image data as well, even if there's no configuration that could be made for that route. I'm thinking of fairly generic code here, if a device requires special routing setup, it'll need the user space to be aware of it. > > > > In the VIDIOC_SUBDEV_S_ROUTING doc we do list some cases where EINVAL or > > > E2BIG is returned. But only a few, and I think > > > v4l2_subdev_routing_validate() will return errors for many other cases too. > > > > > > For what it's worth, the drivers I have written just return an error. It's > > > simple for the driver and the user and works. If the consensus is that the > > > drivers should instead set the default routing, or somehow mangle the given > > > routing to an acceptable form, I can update those drivers accordingly. > > > > > > But we probably need to update the docs too to be a bit more clear what > > > VIDIOC_SUBDEV_S_ROUTING will do (although are the other ioctls any > > > clearer?). > > > > > > All that said, I think it's still a bit case-by-case. I don't think the > > > drivers should always return an error if they get a routing table that's not > > > 100% perfect. E.g. if a device supports two static routes, but the second > > > one can be enabled or disabled, the driver should still accept a routing > > > table from the user with only the first route present. Etc. > > > > > > For the specific case in this patch... I'd prefer returning an error, or if > > > that's not ok, set default routing. > > > > Not modifying the routing table is another option as well but it may > > require separating validating user-provided routes and applying the routes > > I'm not sure to follow you here. By not modifying the routing table, do > you mean returning an error ? Why would that require separation of > validation and configuration ? If a driver has already made changes to its routing table, it's a bad idea to return an error to the user. In this case changes shouldn't be made. > > > to the sub-device state. The default could be useful in principle, too, for > > routing-unaware applications but they won't be calling S_ROUTING anyway. >
On Mon, Jun 17, 2024 at 06:53:07AM +0000, Sakari Ailus wrote: > Hi Laurent, > > On Sun, Jun 16, 2024 at 11:38:07PM +0300, Laurent Pinchart wrote: > > On Sun, Jun 09, 2024 at 06:21:52AM +0000, Sakari Ailus wrote: > > > On Thu, Jun 06, 2024 at 10:10:14PM +0300, Tomi Valkeinen wrote: > > > > On 06/06/2024 20:53, Laurent Pinchart wrote: > > > > > > > > > + return -EINVAL; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + active_sink = route->sink_pad; > > > > > > > > > + } > > > > > > > > > + if (active_sink == UINT_MAX) { > > > > > > > > > + dev_err(rzr->mali_c55->dev, "One route has to be active"); > > > > > > > > > + return -EINVAL; > > > > > > > > > + } > > > > > > > > > > > > > > The recommended handling of invalid routing is to adjust the routing > > > > > > > table, not to return errors. > > > > > > > > > > > > How should I adjust it ? The error here is due to the fact multiple > > > > > > routes are set as active, which one should I make active ? the first > > > > > > one ? Should I go and reset the flags in the subdev_route for the one > > > > > > that has to be made non-active ? > > > > > > > > > > The same way you would adjust an invalid format, you can pick the route > > > > > you consider should be the default. > > > > > > > > > > I'd like Sakari's and Tomi's opinions on this, as it's a new API and the > > > > > behaviour is still a bit in flux. > > > > > > > > Well... My opinion is that the driver adjusting the given config parameters > > > > (for any ioctl) is awful and should be deprecated. If the user asks for X, > > > > and the driver adjusts it and returns Y, then the user has two options: > > > > fail, because it didn't get X (after possibly laborious field by field > > > > checks), or shrug it's virtual shoulders and accept Y and hope that things > > > > still work even though it wanted X. > > > > > > This is still often the only way to tell what the hardware can do as the > > > limitations in different cases (cropping and scaling for instance) can be > > > arbitrary. The other option is that the user space has to know the hardware > > > capabilities without them being available from the kernel. > > > > For some parameters that make sense (we don't have a try mechanism for > > ISP parameters buffers for instance), but when it comes to configuring a > > pipeline, I think a parameters adjustment model is needed when we don't > > have means to expose constraints in a generic way to userspace. The > > question is in which category routing falls. > > > > > There could be cases of IOCTLs where returning an error if what was > > > requested can't be performed exactly is workable in general, but then again > > > having consistency across IOCTL behaviour is very beneficial as well. > > > > > > If you need something exactly, then I think you should check after the > > > IOCTL that this is what you also got, beyond the IOCTL succeeding. > > > > I do agree with Tomi that this kind of check can be annoying for > > applications. In cases where checking the result would be complex, and > > where there is very little use case for receiving anything but the exact > > configuration you asked for, adjusting the parameters could increase the > > implementation complexity on both the kernel side and userspace side for > > no or very little benefit. > > > > > > But maybe that was an answer to a question you didn't really ask =). > > > > > > > > I think setting it to default routing in case of an error is as fine as any > > > > other "fix" for the routing. It won't work anyway. > > > > > > > > But if the function sets default routing and returns 0 here, why would it > > > > return an error from v4l2_subdev_routing_validate()? Should it just set > > > > default routing in that case too? So should set_routing() ever return an > > > > error, if we can just set the default routing? > > > > That's a good point. I asked myself the same question after sending my > > previous e-mail, and wondered if anyone else would notice too :-) > > > > > S_ROUTING is a bit special as it deals with multiple routes and the user > > > space does have a way to add them incrementally. > > > > > > Perhaps we should document better what the driver is expected to to correct > > > the routes? > > > > We should document the expected behaviour clearly. After agreeing on the > > expected behaviour, that is. > > > > > I'd think routes may be added by the driver (as some of them cannot be > > > disabled for instance) but if a requested route cannot be created, that > > > should probably be an error. > > > > > > I've copied my current (with all the pending patches) documentation here > > > <URL:https://www.retiisi.eu/~sailus/v4l2/tmp/streams-doc/userspace-api/media/v4l/dev-subdev.html#streams-multiplexed-media-pads-and-internal-routing>. > > > > > > The text does not elaborate what exactly a driver could or should do, apart > > > from specifying the condition for EINVAL. I think we should specify this in > > > > I don't see mentions of EINVAL related to streams there, am I missing > > something ? > > > > > greater detail. My original thought wws the adjustment would be done by > > > adding static routes omitted by the caller, not trying to come up with e.g. > > > valid pad/stream pairs when user provided invalid ones. > > > > > > Could this correction functionality be limited to returning static routes? > > > > That would make userspace a tad simpler, and wouldn't be hard to do in > > the kernel, but I wonder if departing from the rule that invalid routing > > tables result in an error is worth it for such a small gain. > > I'm just referring to our previous decision on the matter. :-) > > Of course an application can do G_ROUTING, toggle the routes it needs to > and call S_ROUTING again, in order to be (fairly) certain it'll succeed. > > Say, if an application wants to enable an embedded data route, then it'll > be required to supply the route for the image data as well, even if there's > no configuration that could be made for that route. > > I'm thinking of fairly generic code here, if a device requires special > routing setup, it'll need the user space to be aware of it. I hope the libcamera implementation of the routing API will help us figuring this out. We can leave the point open for now. > > > > In the VIDIOC_SUBDEV_S_ROUTING doc we do list some cases where EINVAL or > > > > E2BIG is returned. But only a few, and I think > > > > v4l2_subdev_routing_validate() will return errors for many other cases too. > > > > > > > > For what it's worth, the drivers I have written just return an error. It's > > > > simple for the driver and the user and works. If the consensus is that the > > > > drivers should instead set the default routing, or somehow mangle the given > > > > routing to an acceptable form, I can update those drivers accordingly. > > > > > > > > But we probably need to update the docs too to be a bit more clear what > > > > VIDIOC_SUBDEV_S_ROUTING will do (although are the other ioctls any > > > > clearer?). > > > > > > > > All that said, I think it's still a bit case-by-case. I don't think the > > > > drivers should always return an error if they get a routing table that's not > > > > 100% perfect. E.g. if a device supports two static routes, but the second > > > > one can be enabled or disabled, the driver should still accept a routing > > > > table from the user with only the first route present. Etc. > > > > > > > > For the specific case in this patch... I'd prefer returning an error, or if > > > > that's not ok, set default routing. > > > > > > Not modifying the routing table is another option as well but it may > > > require separating validating user-provided routes and applying the routes > > > > I'm not sure to follow you here. By not modifying the routing table, do > > you mean returning an error ? Why would that require separation of > > validation and configuration ? > > If a driver has already made changes to its routing table, it's a bad idea > to return an error to the user. In this case changes shouldn't be made. Ah yes, that should be clearly documented. If an error is returned, no change to the state shall occur. > > > to the sub-device state. The default could be useful in principle, too, for > > > routing-unaware applications but they won't be calling S_ROUTING anyway.