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

[Dan Scally]

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 at arm.com>
>>>> Co-developed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>>>> Signed-off-by: Daniel Scally <dan.scally at 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]
>