Re: [PATCH v1 09/11] media: platform: Add NXP Neoisp Image Signal Processor

Re: [PATCH v1 09/11] media: platform: Add NXP Neoisp Image Signal Processor

[Geert Uytterhoeven]

Hi Antoine,

On Mon, 13 Apr 2026 at 18:10, Antoine Bouyer <antoine.bouyer@nxp.com> wrote:
> First NXP neoisp driver version with the following contents:
>
> This driver was initially inspired from raspberrypi pisp_be driver. It
> reuses same approach for ISP job scheduling.
>
> The Neoisp driver supports:
> * 8, 10, 12, 14 and 16-bits RAW Bayer images input.
> * Monochrome sensors input.
> * RGB/YUV, IR and Greyscale output formats.
>
> The neoisp features are:
> * Provides single context to limit amount of v4l2 devices.
> * Supports M2M operations.
> * Support SDR and HDR modes.
> * Supports generic v4l2-isp framework for extensible Parameters and
> Statistics buffers.
> * Provides a `core_media_register` API to register neoisp's media entities
> into another media graph.
> * A module parameter to run in standalone mode with its own media device.
>
> Co-developed-by: Alexi Birlinger <alexi.birlinger@nxp.com>
> Signed-off-by: Alexi Birlinger <alexi.birlinger@nxp.com>
> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/media/platform/nxp/neoisp/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config VIDEO_NXP_NEOISP
> +       tristate "NXP NEOISP v4l2 hardware driver"
> +       depends on VIDEO_DEV

depends on ARCH_MXC || COMPILE_TEST

> +       select MEDIA_CONTROLLER
> +       select VIDEOBUF2_DMA_CONTIG
> +       select V4L2_ISP
> +       help
> +         Enable this to support the NXP NEO Image Signal Processing (ISP)
> +         module present in various NXP SoCs. This module offers multiple
> +         functions for processing RAW images and generating RGB or YUV images.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called neoisp.


> +#define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2_MASK GENMASK(23, 16)
> +#define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2_SET(x) (((x) & GENMASK(7, 0)) << 16)
> +#define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2_GET(x) (((x) >> 16) &  GENMASK(7, 0))

What about just:

    #define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2 GENMASK(23, 16)

and using the helpers from include/linux/bitfield.h in the driver code?

    FIELD_PREP(NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2, val)
    FIELD_GET(NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2, reg)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 00/11] media: Add iMX95 neoisp driver

[Antoine Bouyer]

Le 16/04/2026 à 11:20, Krzysztof Kozlowski a écrit :
> 
> 
> On Mon, Apr 13, 2026 at 06:03:20PM +0200, Antoine Bouyer wrote:
>> Hello
>>
>> This patch series introduces the NXP Neo Image Signal Processor (ISP)
>> driver, used in the NXP i.MX95 SoC and future devices in the i.MX9 family.
>> The series also includes updates to the generic v4l2-isp interface to
>> support extended statistics required by the Neo ISP.
>>
>> The Neo ISP processes one or more camera streams, converting RAW formats
>> into YUV or RGB outputs. Its architecture is largely influenced by the
>> PISP driver. To limit the number of v4l2 devices, the driver supports only
>> one context, with three sink pads (main input, second input for HDR, and
>> parameter buffers) and three source pads (RGB output, IR output, and
>> statistics metadata).
>>
>> The driver supports the generic extensible v4l2-isp framework for
>> parameters, similar to rkisp1 and mali-c55, and applies the same approach
>> to statistics buffers. The generic v4l2-isp framework is modified to
>> factorize structure definitions and versioning, so that both parameters
>> and statistics buffers share the same mechanisms.
> 
> That's a v2, not v1.

Hi Krzysztof

Thanks for your review

Sorry, my mistake. My previous submission was an RFC, so I reset version 
counter to 1 by mistake when submitting this patch series.

> 
> Does this work:
> b4 diff 20260413160331.2611829-1-antoine.bouyer@nxp.com
> 
> ? No.
> 
> Should it work? Yes.

I hope v2 will do then ...

> 
> Also, implement previous feedback - I see no changelog, so I assume you
> just ignored everything. Judging by build process errors, it's even
> worse.

Oh no, don't worry, I did not ignore your comments: using maxItems for 
clocks, removing unused configs, and renaming filename to match the 
compatible used in imx95 SoC.

I tried to summarize these changes in this cover letter, but probably 
missing details.

They will be included in v2, along with the fix for the obvious error 
reported by the bot.

Best regards
Antoine

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 09/11] media: platform: Add NXP Neoisp Image Signal Processor

[Antoine Bouyer]

Hi Geert

Le 06/05/2026 à 16:26, Geert Uytterhoeven a écrit :
> 
> Hi Antoine,
> 
> On Mon, 13 Apr 2026 at 18:10, Antoine Bouyer <antoine.bouyer@nxp.com> wrote:
>> First NXP neoisp driver version with the following contents:
>>
>> This driver was initially inspired from raspberrypi pisp_be driver. It
>> reuses same approach for ISP job scheduling.
>>
>> The Neoisp driver supports:
>> * 8, 10, 12, 14 and 16-bits RAW Bayer images input.
>> * Monochrome sensors input.
>> * RGB/YUV, IR and Greyscale output formats.
>>
>> The neoisp features are:
>> * Provides single context to limit amount of v4l2 devices.
>> * Supports M2M operations.
>> * Support SDR and HDR modes.
>> * Supports generic v4l2-isp framework for extensible Parameters and
>> Statistics buffers.
>> * Provides a `core_media_register` API to register neoisp's media entities
>> into another media graph.
>> * A module parameter to run in standalone mode with its own media device.
>>
>> Co-developed-by: Alexi Birlinger <alexi.birlinger@nxp.com>
>> Signed-off-by: Alexi Birlinger <alexi.birlinger@nxp.com>
>> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
> 
> Thanks for your patch!
> 
>> --- /dev/null
>> +++ b/drivers/media/platform/nxp/neoisp/Kconfig
>> @@ -0,0 +1,15 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +
>> +config VIDEO_NXP_NEOISP
>> +       tristate "NXP NEOISP v4l2 hardware driver"
>> +       depends on VIDEO_DEV
> 
> depends on ARCH_MXC || COMPILE_TEST

ack

Will integrate in v2. Thanks for proposal.

> 
>> +       select MEDIA_CONTROLLER
>> +       select VIDEOBUF2_DMA_CONTIG
>> +       select V4L2_ISP
>> +       help
>> +         Enable this to support the NXP NEO Image Signal Processing (ISP)
>> +         module present in various NXP SoCs. This module offers multiple
>> +         functions for processing RAW images and generating RGB or YUV images.
>> +
>> +         To compile this driver as a module, choose M here: the module
>> +         will be called neoisp.
> 
> 
>> +#define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2_MASK GENMASK(23, 16)
>> +#define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2_SET(x) (((x) & GENMASK(7, 0)) << 16)
>> +#define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2_GET(x) (((x) >> 16) &  GENMASK(7, 0))
> 
> What about just:
> 
>      #define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2 GENMASK(23, 16)
> 
> and using the helpers from include/linux/bitfield.h in the driver code?
> 
>      FIELD_PREP(NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2, val)
>      FIELD_GET(NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2, reg)

That file was auto generated, so I did not want to change it.

But I agree, these macro would help to save a couple of lines, and 
probably ease readability too. I need to double check the impact, making 
sure there is no regression with such update.

Could that wait for a v3 ? as I was about to send a v2 with other 
changes first.

Thanks
Antoine

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds

Re: [PATCH v1 09/11] media: platform: Add NXP Neoisp Image Signal Processor

[Geert Uytterhoeven]

Hi Antoine,

On Thu, 7 May 2026 at 15:48, Antoine Bouyer <antoine.bouyer@nxp.com> wrote:
> Le 06/05/2026 à 16:26, Geert Uytterhoeven a écrit :
> > On Mon, 13 Apr 2026 at 18:10, Antoine Bouyer <antoine.bouyer@nxp.com> wrote:
> >> First NXP neoisp driver version with the following contents:
> >>
> >> This driver was initially inspired from raspberrypi pisp_be driver. It
> >> reuses same approach for ISP job scheduling.
> >>
> >> The Neoisp driver supports:
> >> * 8, 10, 12, 14 and 16-bits RAW Bayer images input.
> >> * Monochrome sensors input.
> >> * RGB/YUV, IR and Greyscale output formats.
> >>
> >> The neoisp features are:
> >> * Provides single context to limit amount of v4l2 devices.
> >> * Supports M2M operations.
> >> * Support SDR and HDR modes.
> >> * Supports generic v4l2-isp framework for extensible Parameters and
> >> Statistics buffers.
> >> * Provides a `core_media_register` API to register neoisp's media entities
> >> into another media graph.
> >> * A module parameter to run in standalone mode with its own media device.
> >>
> >> Co-developed-by: Alexi Birlinger <alexi.birlinger@nxp.com>
> >> Signed-off-by: Alexi Birlinger <alexi.birlinger@nxp.com>
> >> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>

> >> +#define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2_MASK GENMASK(23, 16)
> >> +#define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2_SET(x) (((x) & GENMASK(7, 0)) << 16)
> >> +#define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2_GET(x) (((x) >> 16) &  GENMASK(7, 0))
> >
> > What about just:
> >
> >      #define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2 GENMASK(23, 16)
> >
> > and using the helpers from include/linux/bitfield.h in the driver code?
> >
> >      FIELD_PREP(NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2, val)
> >      FIELD_GET(NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2, reg)
>
> That file was auto generated, so I did not want to change it.

Oh, the pleasure of auto-generated files ;-)
Perhaps you can fix the generator?

Here you have 3 definitions per field, which is worse than the
auto-generated AMD header files, which have only two (MASK + SHIFT), and
have been dominating the changed-lines-of-code statistics recently...

> But I agree, these macro would help to save a couple of lines, and
> probably ease readability too. I need to double check the impact, making
> sure there is no regression with such update.

You can compare the generated assembler code, it should be more or
less the same before/after.

> Could that wait for a v3 ? as I was about to send a v2 with other
> changes first.

Sure, thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds