| Message ID | 20240214141906.245685-1-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Arm Mali-C55 Image Signal Processor Driver | expand |
Hi Sakari On Mon, Feb 26, 2024 at 11:03:47AM +0000, Sakari Ailus wrote: > Hi Daniel, > > Thanks for the set. > > How do you determine which buffers go together on the video buffer queues? > Or do you need a buffer on both for every frame if the nodes are set > streaming? > > This should be also documented, in the documentation patch. At least I > didn't find it there. > > On Wed, Feb 14, 2024 at 02:19:04PM +0000, 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> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > > --- > > 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 | 1021 +++++++++++++++++ > > .../platform/arm/mali-c55/mali-c55-common.h | 271 +++++ > > .../platform/arm/mali-c55/mali-c55-core.c | 767 +++++++++++++ > > .../platform/arm/mali-c55/mali-c55-isp.c | 682 +++++++++++ > > .../arm/mali-c55/mali-c55-registers.h | 180 +++ > > .../arm/mali-c55/mali-c55-resizer-coefs.h | 382 ++++++ > > .../platform/arm/mali-c55/mali-c55-resizer.c | 678 +++++++++++ > > .../platform/arm/mali-c55/mali-c55-tpg.c | 425 +++++++ > > 14 files changed, 4442 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 > > > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > > index 91e54215de3a..cd92c024e039 100644 > > --- a/drivers/media/platform/Kconfig > > +++ b/drivers/media/platform/Kconfig > > @@ -65,6 +65,7 @@ config VIDEO_MUX > > source "drivers/media/platform/allegro-dvt/Kconfig" > > source "drivers/media/platform/amlogic/Kconfig" > > source "drivers/media/platform/amphion/Kconfig" > > +source "drivers/media/platform/arm/Kconfig" > > source "drivers/media/platform/aspeed/Kconfig" > > source "drivers/media/platform/atmel/Kconfig" > > source "drivers/media/platform/cadence/Kconfig" > > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile > > index 3296ec1ebe16..ea6624c62559 100644 > > --- a/drivers/media/platform/Makefile > > +++ b/drivers/media/platform/Makefile > > @@ -8,6 +8,7 @@ > > obj-y += allegro-dvt/ > > obj-y += amlogic/ > > obj-y += amphion/ > > +obj-y += arm/ > > obj-y += aspeed/ > > obj-y += atmel/ > > obj-y += cadence/ > > diff --git a/drivers/media/platform/arm/Kconfig b/drivers/media/platform/arm/Kconfig > > new file mode 100644 > > index 000000000000..4f0764c329c7 > > --- /dev/null > > +++ b/drivers/media/platform/arm/Kconfig > > @@ -0,0 +1,5 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > + > > +comment "ARM media platform drivers" > > + > > +source "drivers/media/platform/arm/mali-c55/Kconfig" > > diff --git a/drivers/media/platform/arm/Makefile b/drivers/media/platform/arm/Makefile > > new file mode 100644 > > index 000000000000..8cc4918725ef > > --- /dev/null > > +++ b/drivers/media/platform/arm/Makefile > > @@ -0,0 +1,2 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +obj-y += mali-c55/ > > diff --git a/drivers/media/platform/arm/mali-c55/Kconfig b/drivers/media/platform/arm/mali-c55/Kconfig > > new file mode 100644 > > index 000000000000..602085e28b01 > > --- /dev/null > > +++ b/drivers/media/platform/arm/mali-c55/Kconfig > > @@ -0,0 +1,18 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +config VIDEO_MALI_C55 > > + tristate "ARM Mali-C55 Image Signal Processor driver" > > + depends on V4L_PLATFORM_DRIVERS > > + depends on VIDEO_DEV && OF > > + depends on ARCH_VEXPRESS || COMPILE_TEST > > + select MEDIA_CONTROLLER > > + select VIDEO_V4L2_SUBDEV_API > > + select VIDEOBUF2_DMA_CONTIG > > + select VIDEOBUF2_VMALLOC > > + select V4L2_FWNODE > > + select GENERIC_PHY_MIPI_DPHY > > + default n > > + help > > + Enable this to support Arm's Mali-C55 Image Signal Processor. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called mali-c55. > > diff --git a/drivers/media/platform/arm/mali-c55/Makefile b/drivers/media/platform/arm/mali-c55/Makefile > > new file mode 100644 > > index 000000000000..77dcb2fbf0f4 > > --- /dev/null > > +++ b/drivers/media/platform/arm/mali-c55/Makefile > > @@ -0,0 +1,9 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > + > > +mali-c55-y := mali-c55-capture.o \ > > + mali-c55-core.o \ > > + mali-c55-isp.o \ > > + mali-c55-tpg.o \ > > + mali-c55-resizer.o > > + > > +obj-$(CONFIG_VIDEO_MALI_C55) += mali-c55.o > > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-capture.c b/drivers/media/platform/arm/mali-c55/mali-c55-capture.c > > new file mode 100644 > > index 000000000000..98020b7ecb1e > > --- /dev/null > > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-capture.c > > @@ -0,0 +1,1021 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * ARM Mali-C55 ISP Driver - Video capture devices > > + * > > + * Copyright (C) 2023 Ideas on Board Oy > > 2024 > > > + */ > > + > > +#include <linux/minmax.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/string.h> > > +#include <linux/videodev2.h> > > + > > +#include <media/v4l2-dev.h> > > +#include <media/v4l2-event.h> > > +#include <media/v4l2-ioctl.h> > > +#include <media/v4l2-subdev.h> > > +#include <media/videobuf2-core.h> > > +#include <media/videobuf2-dma-contig.h> > > + > > +#include "mali-c55-common.h" > > +#include "mali-c55-registers.h" > > + > > +/* > > + * The Mali-C55 ISP has up to two output pipes; known as full resolution and > > + * down scaled. The register space for these is laid out identically, but offset > > + * by 372 bytes. > > + */ > > +#define MALI_C55_CAP_DEV_FR_REG_OFFSET 0x0 > > +#define MALI_C55_CAP_DEV_DS_REG_OFFSET 0x174 > > + > > +static const struct mali_c55_fmt mali_c55_fmts[] = { > > + /* > > + * This table is missing some entries which need further work or > > + * investigation: > > + * > > + * Base mode 1 is a backwards V4L2_PIX_FMT_XRGB32 with no V4L2 equivalent > > + * Base mode 5 is "Generic Data" > > + * Base mode 8 is a backwards V4L2_PIX_FMT_XYUV32 - no V4L2 equivalent > > + * Base mode 9 seems to have no V4L2 equivalent > > + * Base mode 17, 19 and 20 describe formats which seem to have no V4L2 > > + * equivalent > > + */ > > + { > > + .fourcc = V4L2_PIX_FMT_ARGB2101010, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_RGB121212_1X36, > > + MEDIA_BUS_FMT_RGB202020_1X60, > > + }, > > + .enumerate = true, > > + .is_raw = false, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_A2R10G10B10, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT0 > > + } > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_RGB565, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_RGB121212_1X36, > > + MEDIA_BUS_FMT_RGB202020_1X60, > > + }, > > + .enumerate = false, > > + .is_raw = false, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_RGB565, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT0 > > + } > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_BGR24, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_RGB121212_1X36, > > + MEDIA_BUS_FMT_RGB202020_1X60, > > + }, > > + .enumerate = false, > > + .is_raw = false, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_RGB24, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT0 > > + } > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_YUYV, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_YUV10_1X30, > > + }, > > + .enumerate = true, > > + .is_raw = false, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_YUY2, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT0 > > + } > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_UYVY, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_YUV10_1X30, > > + }, > > + .enumerate = false, > > + .is_raw = false, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_UYVY, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT0 > > + } > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_Y210, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_YUV10_1X30, > > + }, > > + .enumerate = false, > > + .is_raw = false, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_Y210, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT0 > > + } > > + }, > > + /* > > + * This is something of a hack, the ISP thinks it's running NV12M but > > + * by setting uv_plane = 0 we simply discard that planes and only output > > + * the Y-plane. > > + */ > > + { > > + .fourcc = V4L2_PIX_FMT_GREY, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_YUV10_1X30, > > + }, > > + .enumerate = false, > > + .is_raw = false, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_NV12_21, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT0 > > + } > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_NV12M, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_YUV10_1X30, > > + }, > > + .enumerate = false, > > + .is_raw = false, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_NV12_21, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT1 > > + } > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_NV21M, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_YUV10_1X30, > > + }, > > + .enumerate = false, > > + .is_raw = false, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_NV12_21, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT2 > > + } > > + }, > > + /* > > + * RAW uncompressed formats are all packed in 16 bpp. > > + * TODO: Expand this list to encompass all possible RAW formats. > > + */ > > + { > > + .fourcc = V4L2_PIX_FMT_SRGGB12, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_SRGGB12_1X12, > > + }, > > + .enumerate = true, > > + .is_raw = true, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_RAW16, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT0 > > + } > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_SBGGR12, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_SBGGR12_1X12, > > + }, > > + .enumerate = true, > > + .is_raw = true, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_RAW16, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT0 > > + } > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_SGBRG12, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_SGBRG12_1X12, > > + }, > > + .enumerate = true, > > + .is_raw = true, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_RAW16, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT0 > > + } > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_SGRBG12, > > + .mbus_codes = { > > + MEDIA_BUS_FMT_SGRBG12_1X12, > > + }, > > + .enumerate = true, > > + .is_raw = true, > > + .registers = { > > + .base_mode = MALI_C55_OUTPUT_RAW16, > > + .uv_plane = MALI_C55_OUTPUT_PLANE_ALT0 > > + } > > + }, > > +}; > > + > > +static bool mali_c55_mbus_code_can_produce_fmt(const struct mali_c55_fmt *fmt, > > + u32 code) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(fmt->mbus_codes); i++) { > > + if (fmt->mbus_codes[i] == code) > > + return true; > > + } > > + > > + return false; > > +} > > + > > +const struct mali_c55_fmt *mali_c55_cap_fmt_next(const struct mali_c55_fmt *fmt, > > + bool allow_raw, bool unique) > > +{ > > + if (!fmt) > > + fmt = &mali_c55_fmts[0]; > > + else > > + ++fmt; > > fmt++, please. > Can I ask why ? (here and in the next occurrences you have reported)
Hi Jacopo, On Wed, Feb 28, 2024 at 01:50:14PM +0100, Jacopo Mondi wrote: > > > +const struct mali_c55_fmt *mali_c55_cap_fmt_next(const struct mali_c55_fmt *fmt, > > > + bool allow_raw, bool unique) > > > +{ > > > + if (!fmt) > > > + fmt = &mali_c55_fmts[0]; > > > + else > > > + ++fmt; > > > > fmt++, please. > > > > Can I ask why ? (here and in the next occurrences you have reported) It's much, much more common and using that form makes the code easier to read. The rest of the driver primarily uses variable++, too, AFAIR. So you should use ++variable only when you need it.
Quoting Sakari Ailus (2024-02-28 13:11:39) > Hi Jacopo, > > On Wed, Feb 28, 2024 at 01:50:14PM +0100, Jacopo Mondi wrote: > > > > +const struct mali_c55_fmt *mali_c55_cap_fmt_next(const struct mali_c55_fmt *fmt, > > > > + bool allow_raw, bool unique) > > > > +{ > > > > + if (!fmt) > > > > + fmt = &mali_c55_fmts[0]; > > > > + else > > > > + ++fmt; > > > > > > fmt++, please. > > > > > > > Can I ask why ? (here and in the next occurrences you have reported) > > It's much, much more common and using that form makes the code easier to > read. The rest of the driver primarily uses variable++, too, AFAIR. > > So you should use ++variable only when you need it. I don't think this is a hot path, but I'll never forget my C tutor telling us how ++i is more efficient than i++ somewhere to do with the opcode ordering, and not having to make a copy [*1] Though I bet any clever optimising compiler could spot this anyway. [*1]. Whatever plausibility there is based on a 20 year old memory and should be verified elsewhere. -- Kieran > > -- > Sakari Ailus
On Wed, Feb 28, 2024 at 01:29:36PM +0000, Kieran Bingham wrote: > Quoting Sakari Ailus (2024-02-28 13:11:39) > > On Wed, Feb 28, 2024 at 01:50:14PM +0100, Jacopo Mondi wrote: > > > > > +const struct mali_c55_fmt *mali_c55_cap_fmt_next(const struct mali_c55_fmt *fmt, > > > > > + bool allow_raw, bool unique) > > > > > +{ > > > > > + if (!fmt) > > > > > + fmt = &mali_c55_fmts[0]; > > > > > + else > > > > > + ++fmt; > > > > > > > > fmt++, please. > > > > > > Can I ask why ? (here and in the next occurrences you have reported) > > > > It's much, much more common and using that form makes the code easier to > > read. The rest of the driver primarily uses variable++, too, AFAIR. > > > > So you should use ++variable only when you need it. > > I don't think this is a hot path, but I'll never forget my C tutor > telling us how ++i is more efficient than i++ somewhere to do with the > opcode ordering, and not having to make a copy [*1] > > Though I bet any clever optimising compiler could spot this anyway. > > [*1]. Whatever plausibility there is based on a 20 year old memory and > should be verified elsewhere. In C I wouldn't expect any practical difference. C++ is a different matter, as the prefix and postfix operators can have different implementations.
Hi Sakari On 26/02/2024 11:03, Sakari Ailus wrote: > < snip> >> +const struct mali_c55_fmt *mali_c55_cap_fmt_next(const struct mali_c55_fmt *fmt, >> + bool allow_raw, bool unique) >> +{ >> + if (!fmt) >> + fmt = &mali_c55_fmts[0]; >> + else >> + ++fmt; > fmt++, please. > >> + >> + for (; fmt < &mali_c55_fmts[ARRAY_SIZE(mali_c55_fmts)]; ++fmt) { > Ditto. > >> + if (!allow_raw && fmt->is_raw) { >> + fmt++; > Why? Sorry, I forgot to reply here...I think neither this nor the enumeration filter below is actually used - I'll double check and remove them for the v3. Thanks Dan > >> + continue; >> + } >> + >> + if (unique && !fmt->enumerate) { >> + fmt++; > Here, too. > >> + continue; >> + } >> + >> + return fmt; >> + } >> + >> + return NULL; >> +} >> +