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@xxxxxxx>
Co-developed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
---
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]
