Re: [PATCH v5 05/16] media: mali-c55: Add Mali-C55 ISP driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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]

-- 
Regards,

Laurent Pinchart