by Laurent Pinchart

[permalink] [raw]

Subject: Re: [PATCH v3 00/14] media: rkisp1: Add support for i.MX8MP

Hi Adam,

On Wed, Feb 15, 2023 at 07:57:53AM -0600, Adam Ford wrote:
> On Fri, Nov 18, 2022 at 3:44 AM Paul Elder wrote:
>
> > This series depends on v3 of "dt-bindings: media: Add macros for video
> > interface bus types" [1].
> >
> > This series extends the rkisp1 driver to support the ISP found in the
> > NXP i.MX8MP SoC.
>
> I'm going to spend some time testing this over the weekend. Is there a V4
> pending, or should I just test whatever is in Laurent's repo?

I've updated all the v6.2-based branches on
https://gitlab.com/ideasonboard/nxp/linux.git (and renamed them to
v6.2/*). Beside a rebase, the v6.2/isp branch contains (at the bottom) 6
additional patches that I've previously posted to the linux-media
mailing list (feel free to review them ;-)).

My only concern with this series is with patch "media: rkisp1: Add match
data for i.MX8MP ISP", and in particular with the following hunk:

enum rkisp1_cif_isp_version {
RKISP1_V10 = 10,
RKISP1_V11,
RKISP1_V12,
RKISP1_V13,
+ IMX8MP_V10,
};

It's not a very nice versioning scheme :-S I'll see if I can find
something better, but regardless of that, I'll post v4 with the goal of
merging it in v6.4.

> I have an IMX219 camera with 4-lane support and an i.MX8M Plus kit from
> Beacon, and I want to test the RGGB bayer conversion to see how well it
> works.
>
> > The ISP IP cores in the Rockchip RK3399 (known as the "Rockchip ISP1")
> > and in the NXP i.MX8MP have the same origin, and have slightly diverged
> > over time as they are now independently developed (afaik) by Rockchip
> > and VeriSilicon. The latter is marketed under the name "ISP8000Nano",
> > and is close enough to the RK3399 ISP that it can easily be supported by
> > the same driver.
>
> Is there a reason the driver cannot be renamed to a more generic name than
> rkisp1 if the Rockchip and VeriSilicon had similar origins? Having the
> name Rockchip referenced from an NXP i.MX8M Plus seems odd to me.

The common roots of the IP core predate both Rockchip and VeriSilicon.
Those two implementations have now diverged (as with all forks), so
either name would be wrong in some cases :-S

> > The last two patches add support for UYVY output format, which can be
> > implemented on the ISP version in the i.MX8MP but not in the one in the
> > RK3399.
> >
> > This version of the series specifically has been tested on a Polyhex
> > Debix model A with an imx219 (Raspberry Pi cam v2).
> >
> > [1] https://lore.kernel.org/linux-media/[email protected]/
> >
> > Laurent Pinchart (3):
> > dt-bindings: media: rkisp1: Add i.MX8MP ISP example
> > media: rkisp1: Add and use rkisp1_has_feature() macro
> > media: rkisp1: Configure gasket on i.MX8MP
> >
> > Paul Elder (11):
> > dt-bindings: media: rkisp1: Add i.MX8MP ISP to compatible
> > media: rkisp1: Add match data for i.MX8MP ISP
> > media: rkisp1: Add and set registers for crop for i.MX8MP
> > media: rkisp1: Add and set registers for output size config on i.MX8MP
> > media: rkisp1: Add i.MX8MP-specific registers for MI and resizer
> > media: rkisp1: Shift DMA buffer addresses on i.MX8MP
> > media: rkisp1: Add register definitions for the test pattern generator
> > media: rkisp1: Fix RSZ_CTRL bits for i.MX8MP
> > media: rkisp1: Support devices without self path
> > media: rkisp1: Add YC swap capability
> > media: rkisp1: Add UYVY as an output format
> >
> > .../bindings/media/rockchip-isp1.yaml | 79 ++++++++++-
> > .../platform/rockchip/rkisp1/rkisp1-capture.c | 102 +++++++++++---
> > .../platform/rockchip/rkisp1/rkisp1-common.h | 32 +++++
> > .../platform/rockchip/rkisp1/rkisp1-debug.c | 14 +-
> > .../platform/rockchip/rkisp1/rkisp1-dev.c | 67 +++++++--
> > .../platform/rockchip/rkisp1/rkisp1-isp.c | 128 +++++++++++++++++-
> > .../platform/rockchip/rkisp1/rkisp1-regs.h | 90 ++++++++++++
> > .../platform/rockchip/rkisp1/rkisp1-resizer.c | 35 ++++-
> > include/uapi/linux/rkisp1-config.h | 2 +
> > 9 files changed, 509 insertions(+), 40 deletions(-)

--
Regards,

Laurent Pinchart

2023-02-18 16:14:26

On Fri, Nov 18, 2022 at 3:44 AM Paul Elder <[email protected]> wrote:
>
> Add match data to the rkisp1 driver to match the i.MX8MP ISP.
>
> Although the new version number isn't very precise, it ought to be fine
> as the other version numbers aren't precise either, and we have separate
> feature flags for important version-specific features. Despite this
> version number being seemingly unimportant, it is added to distinguish
> it from the ISP versions integrated in rockchip SoCs.
>
> Signed-off-by: Paul Elder <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
>

Paul,

It's been nearly a year since this commit was sent. I noticed it
hasn't been applied, and I was curious to know if there is any
movement here? I'm happy to test on my 8MP if necessary.

Thanks!

adam

> ---
> Changes in v3:
> - Remove todo for improving the version number
> - Expand the commit message to address the version number
> ---
> .../platform/rockchip/rkisp1/rkisp1-dev.c | 22 +++++++++++++++++++
> include/uapi/linux/rkisp1-config.h | 2 ++
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index e348d8c86861..69464ce91d59 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -496,6 +496,24 @@ static const struct rkisp1_info rk3399_isp_info = {
> .features = RKISP1_FEATURE_MIPI_CSI2,
> };
>
> +static const char * const imx8mp_isp_clks[] = {
> + "isp",
> + "hclk",
> + "aclk",
> +};
> +
> +static const struct rkisp1_isr_data imx8mp_isp_isrs[] = {
> + { NULL, rkisp1_isr },
> +};
> +
> +static const struct rkisp1_info imx8mp_isp_info = {
> + .clks = imx8mp_isp_clks,
> + .clk_size = ARRAY_SIZE(imx8mp_isp_clks),
> + .isrs = imx8mp_isp_isrs,
> + .isr_size = ARRAY_SIZE(imx8mp_isp_isrs),
> + .isp_ver = IMX8MP_V10,
> +};
> +
> static const struct of_device_id rkisp1_of_match[] = {
> {
> .compatible = "rockchip,px30-cif-isp",
> @@ -505,6 +523,10 @@ static const struct of_device_id rkisp1_of_match[] = {
> .compatible = "rockchip,rk3399-cif-isp",
> .data = &rk3399_isp_info,
> },
> + {
> + .compatible = "fsl,imx8mp-isp",
> + .data = &imx8mp_isp_info,
> + },
> {},
> };
> MODULE_DEVICE_TABLE(of, rkisp1_of_match);
> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> index 730673ecc63d..f602442c2018 100644
> --- a/include/uapi/linux/rkisp1-config.h
> +++ b/include/uapi/linux/rkisp1-config.h
> @@ -179,12 +179,14 @@
> * @RKISP1_V11: declared in the original vendor code, but not used
> * @RKISP1_V12: used at least in rk3326 and px30
> * @RKISP1_V13: used at least in rk1808
> + * @IMX8MP_V10: used in at least imx8mp
> */
> enum rkisp1_cif_isp_version {
> RKISP1_V10 = 10,
> RKISP1_V11,
> RKISP1_V12,
> RKISP1_V13,
> + IMX8MP_V10,
> };
>
> enum rkisp1_cif_isp_histogram_mode {
> --
> 2.35.1
>