[v3,4/5] include: linux: Add RKISP1_V_IMX8MP version
diff mbox series

Message ID 20240218164908.15921-5-laurent.pinchart@ideasonboard.com
State Accepted
Commit d2436c57d70d2e1f8985f0f421b086c31f581ddc
Headers show
Series
  • ipa: rkisp1: Support the i.MX8MP ISP
Related show

Commit Message

Laurent Pinchart Feb. 18, 2024, 4:49 p.m. UTC 
From: Paul Elder <paul.elder@ideasonboard.com>

Patches have been posted to the linux-media@vger.kernel.org mailing list
to add i.MX8MP support to the rkisp1 driver ([1]). As no changes are
expected to the userspace API in future versions of the series, add the
RKISP1_V_IMX8MP version manually to the rkisp1-config.h header already.
Once the patches get merged in the kernel, the changes will trickle down
to libcamera with the next kernel headers update.

[1] https://lore.kernel.org/linux-media/20240216095458.2919694-1-paul.elder@ideasonboard.com/

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/linux/rkisp1-config.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Stefan Klug Feb. 23, 2024, 1:15 p.m. UTC | #1 
Am 18.02.24 um 17:49 schrieb Laurent Pinchart:
> From: Paul Elder <paul.elder@ideasonboard.com>
> 
> Patches have been posted to the linux-media@vger.kernel.org mailing list
> to add i.MX8MP support to the rkisp1 driver ([1]). As no changes are
> expected to the userspace API in future versions of the series, add the
> RKISP1_V_IMX8MP version manually to the rkisp1-config.h header already.
> Once the patches get merged in the kernel, the changes will trickle down
> to libcamera with the next kernel headers update.
> 
> [1] https://lore.kernel.org/linux-media/20240216095458.2919694-1-paul.elder@ideasonboard.com/
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

> ---
>   include/linux/rkisp1-config.h | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/rkisp1-config.h b/include/linux/rkisp1-config.h
> index ec7cde8cd2e3..2d1c448a6ab8 100644
> --- a/include/linux/rkisp1-config.h
> +++ b/include/linux/rkisp1-config.h
> @@ -4,8 +4,8 @@
>    * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
>    */
>   
> -#ifndef _RKISP1_CONFIG_H
> -#define _RKISP1_CONFIG_H
> +#ifndef _UAPI_RKISP1_CONFIG_H
> +#define _UAPI_RKISP1_CONFIG_H
>   
>   #include <linux/types.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
> + * @RKISP1_V_IMX8MP: used in at least imx8mp
>    */
>   enum rkisp1_cif_isp_version {
>   	RKISP1_V10 = 10,
>   	RKISP1_V11,
>   	RKISP1_V12,
>   	RKISP1_V13,
> +	RKISP1_V_IMX8MP,

IMHO it might be more flexible in the future to start with a new number. 
eg. RKISP1_V_IMX8MP = 100

>   };
>   
>   enum rkisp1_cif_isp_histogram_mode {
> @@ -992,4 +994,4 @@ struct rkisp1_stat_buffer {
>   	struct rkisp1_cif_isp_stat params;
>   };
>   
> -#endif /* _RKISP1_CONFIG_H */
> +#endif /* _UAPI_RKISP1_CONFIG_H */
Laurent Pinchart Feb. 23, 2024, 1:20 p.m. UTC | #2 
Hi Stefan,

On Fri, Feb 23, 2024 at 02:15:14PM +0100, Stefan Klug wrote:
> Am 18.02.24 um 17:49 schrieb Laurent Pinchart:
> > From: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > Patches have been posted to the linux-media@vger.kernel.org mailing list
> > to add i.MX8MP support to the rkisp1 driver ([1]). As no changes are
> > expected to the userspace API in future versions of the series, add the
> > RKISP1_V_IMX8MP version manually to the rkisp1-config.h header already.
> > Once the patches get merged in the kernel, the changes will trickle down
> > to libcamera with the next kernel headers update.
> > 
> > [1] https://lore.kernel.org/linux-media/20240216095458.2919694-1-paul.elder@ideasonboard.com/
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> > ---
> >   include/linux/rkisp1-config.h | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/rkisp1-config.h b/include/linux/rkisp1-config.h
> > index ec7cde8cd2e3..2d1c448a6ab8 100644
> > --- a/include/linux/rkisp1-config.h
> > +++ b/include/linux/rkisp1-config.h
> > @@ -4,8 +4,8 @@
> >    * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> >    */
> >   
> > -#ifndef _RKISP1_CONFIG_H
> > -#define _RKISP1_CONFIG_H
> > +#ifndef _UAPI_RKISP1_CONFIG_H
> > +#define _UAPI_RKISP1_CONFIG_H
> >   
> >   #include <linux/types.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
> > + * @RKISP1_V_IMX8MP: used in at least imx8mp
> >    */
> >   enum rkisp1_cif_isp_version {
> >   	RKISP1_V10 = 10,
> >   	RKISP1_V11,
> >   	RKISP1_V12,
> >   	RKISP1_V13,
> > +	RKISP1_V_IMX8MP,
> 
> IMHO it might be more flexible in the future to start with a new number. 
> eg. RKISP1_V_IMX8MP = 100

I dislike the versioning scheme as well :-(

This being said, Rockchip has developed v2 and v3 versions of the ISP,
They are not compatible with v1, so I don't think this driver will ever
need to support new Rockchip versions.

> >   };
> >   
> >   enum rkisp1_cif_isp_histogram_mode {
> > @@ -992,4 +994,4 @@ struct rkisp1_stat_buffer {
> >   	struct rkisp1_cif_isp_stat params;
> >   };
> >   
> > -#endif /* _RKISP1_CONFIG_H */
> > +#endif /* _UAPI_RKISP1_CONFIG_H */
Kieran Bingham Feb. 23, 2024, 1:26 p.m. UTC | #3 
Quoting Laurent Pinchart (2024-02-23 13:20:40)
> Hi Stefan,
> 
> On Fri, Feb 23, 2024 at 02:15:14PM +0100, Stefan Klug wrote:
> > Am 18.02.24 um 17:49 schrieb Laurent Pinchart:
> > > From: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > Patches have been posted to the linux-media@vger.kernel.org mailing list
> > > to add i.MX8MP support to the rkisp1 driver ([1]). As no changes are
> > > expected to the userspace API in future versions of the series, add the
> > > RKISP1_V_IMX8MP version manually to the rkisp1-config.h header already.
> > > Once the patches get merged in the kernel, the changes will trickle down
> > > to libcamera with the next kernel headers update.
> > > 
> > > [1] https://lore.kernel.org/linux-media/20240216095458.2919694-1-paul.elder@ideasonboard.com/
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> > > ---
> > >   include/linux/rkisp1-config.h | 8 +++++---
> > >   1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/linux/rkisp1-config.h b/include/linux/rkisp1-config.h
> > > index ec7cde8cd2e3..2d1c448a6ab8 100644
> > > --- a/include/linux/rkisp1-config.h
> > > +++ b/include/linux/rkisp1-config.h
> > > @@ -4,8 +4,8 @@
> > >    * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> > >    */
> > >   
> > > -#ifndef _RKISP1_CONFIG_H
> > > -#define _RKISP1_CONFIG_H
> > > +#ifndef _UAPI_RKISP1_CONFIG_H
> > > +#define _UAPI_RKISP1_CONFIG_H
> > >   
> > >   #include <linux/types.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
> > > + * @RKISP1_V_IMX8MP: used in at least imx8mp
> > >    */
> > >   enum rkisp1_cif_isp_version {
> > >     RKISP1_V10 = 10,
> > >     RKISP1_V11,
> > >     RKISP1_V12,
> > >     RKISP1_V13,
> > > +   RKISP1_V_IMX8MP,
> > 
> > IMHO it might be more flexible in the future to start with a new number. 
> > eg. RKISP1_V_IMX8MP = 100
> 
> I dislike the versioning scheme as well :-(
> 
> This being said, Rockchip has developed v2 and v3 versions of the ISP,
> They are not compatible with v1, so I don't think this driver will ever
> need to support new Rockchip versions.

Do you /know/ that for a fact?

I thought Rockchip's BSP used a single driver for all of it's ISP range?
 (Not rkisp1)

--
Kieran


> 
> > >   };
> > >   
> > >   enum rkisp1_cif_isp_histogram_mode {
> > > @@ -992,4 +994,4 @@ struct rkisp1_stat_buffer {
> > >     struct rkisp1_cif_isp_stat params;
> > >   };
> > >   
> > > -#endif /* _RKISP1_CONFIG_H */
> > > +#endif /* _UAPI_RKISP1_CONFIG_H */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart Feb. 23, 2024, 1:37 p.m. UTC | #4 
On Fri, Feb 23, 2024 at 01:26:30PM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-02-23 13:20:40)
> > On Fri, Feb 23, 2024 at 02:15:14PM +0100, Stefan Klug wrote:
> > > Am 18.02.24 um 17:49 schrieb Laurent Pinchart:
> > > > From: Paul Elder <paul.elder@ideasonboard.com>
> > > > 
> > > > Patches have been posted to the linux-media@vger.kernel.org mailing list
> > > > to add i.MX8MP support to the rkisp1 driver ([1]). As no changes are
> > > > expected to the userspace API in future versions of the series, add the
> > > > RKISP1_V_IMX8MP version manually to the rkisp1-config.h header already.
> > > > Once the patches get merged in the kernel, the changes will trickle down
> > > > to libcamera with the next kernel headers update.
> > > > 
> > > > [1] https://lore.kernel.org/linux-media/20240216095458.2919694-1-paul.elder@ideasonboard.com/
> > > > 
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > 
> > > > ---
> > > >   include/linux/rkisp1-config.h | 8 +++++---
> > > >   1 file changed, 5 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/include/linux/rkisp1-config.h b/include/linux/rkisp1-config.h
> > > > index ec7cde8cd2e3..2d1c448a6ab8 100644
> > > > --- a/include/linux/rkisp1-config.h
> > > > +++ b/include/linux/rkisp1-config.h
> > > > @@ -4,8 +4,8 @@
> > > >    * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> > > >    */
> > > >   
> > > > -#ifndef _RKISP1_CONFIG_H
> > > > -#define _RKISP1_CONFIG_H
> > > > +#ifndef _UAPI_RKISP1_CONFIG_H
> > > > +#define _UAPI_RKISP1_CONFIG_H
> > > >   
> > > >   #include <linux/types.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
> > > > + * @RKISP1_V_IMX8MP: used in at least imx8mp
> > > >    */
> > > >   enum rkisp1_cif_isp_version {
> > > >     RKISP1_V10 = 10,
> > > >     RKISP1_V11,
> > > >     RKISP1_V12,
> > > >     RKISP1_V13,
> > > > +   RKISP1_V_IMX8MP,
> > > 
> > > IMHO it might be more flexible in the future to start with a new number. 
> > > eg. RKISP1_V_IMX8MP = 100
> > 
> > I dislike the versioning scheme as well :-(
> > 
> > This being said, Rockchip has developed v2 and v3 versions of the ISP,
> > They are not compatible with v1, so I don't think this driver will ever
> > need to support new Rockchip versions.
> 
> Do you /know/ that for a fact?
> 
> I thought Rockchip's BSP used a single driver for all of it's ISP range?
>  (Not rkisp1)

They do, and we could do so in mainline too, adding support for ISP v1
to a new driver that supports v2 and v3 as well. Doing it the other way
around without breaking any backward compatibility doesn't seem
realistic.

These are all speculations anyway. The bottom line is that without
information about the VSI versioning scheme for current and future ISP
versions, there's nothing we can really do.

> > > >   };
> > > >   
> > > >   enum rkisp1_cif_isp_histogram_mode {
> > > > @@ -992,4 +994,4 @@ struct rkisp1_stat_buffer {
> > > >     struct rkisp1_cif_isp_stat params;
> > > >   };
> > > >   
> > > > -#endif /* _RKISP1_CONFIG_H */
> > > > +#endif /* _UAPI_RKISP1_CONFIG_H */
Laurent Pinchart Feb. 23, 2024, 1:46 p.m. UTC | #5 
On Fri, Feb 23, 2024 at 03:37:20PM +0200, Laurent Pinchart wrote:
> On Fri, Feb 23, 2024 at 01:26:30PM +0000, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2024-02-23 13:20:40)
> > > On Fri, Feb 23, 2024 at 02:15:14PM +0100, Stefan Klug wrote:
> > > > Am 18.02.24 um 17:49 schrieb Laurent Pinchart:
> > > > > From: Paul Elder <paul.elder@ideasonboard.com>
> > > > > 
> > > > > Patches have been posted to the linux-media@vger.kernel.org mailing list
> > > > > to add i.MX8MP support to the rkisp1 driver ([1]). As no changes are
> > > > > expected to the userspace API in future versions of the series, add the
> > > > > RKISP1_V_IMX8MP version manually to the rkisp1-config.h header already.
> > > > > Once the patches get merged in the kernel, the changes will trickle down
> > > > > to libcamera with the next kernel headers update.
> > > > > 
> > > > > [1] https://lore.kernel.org/linux-media/20240216095458.2919694-1-paul.elder@ideasonboard.com/
> > > > > 
> > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > 
> > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > 
> > > > > ---
> > > > >   include/linux/rkisp1-config.h | 8 +++++---
> > > > >   1 file changed, 5 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/include/linux/rkisp1-config.h b/include/linux/rkisp1-config.h
> > > > > index ec7cde8cd2e3..2d1c448a6ab8 100644
> > > > > --- a/include/linux/rkisp1-config.h
> > > > > +++ b/include/linux/rkisp1-config.h
> > > > > @@ -4,8 +4,8 @@
> > > > >    * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> > > > >    */
> > > > >   
> > > > > -#ifndef _RKISP1_CONFIG_H
> > > > > -#define _RKISP1_CONFIG_H
> > > > > +#ifndef _UAPI_RKISP1_CONFIG_H
> > > > > +#define _UAPI_RKISP1_CONFIG_H
> > > > >   
> > > > >   #include <linux/types.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
> > > > > + * @RKISP1_V_IMX8MP: used in at least imx8mp
> > > > >    */
> > > > >   enum rkisp1_cif_isp_version {
> > > > >     RKISP1_V10 = 10,
> > > > >     RKISP1_V11,
> > > > >     RKISP1_V12,
> > > > >     RKISP1_V13,
> > > > > +   RKISP1_V_IMX8MP,
> > > > 
> > > > IMHO it might be more flexible in the future to start with a new number. 
> > > > eg. RKISP1_V_IMX8MP = 100
> > > 
> > > I dislike the versioning scheme as well :-(
> > > 
> > > This being said, Rockchip has developed v2 and v3 versions of the ISP,
> > > They are not compatible with v1, so I don't think this driver will ever
> > > need to support new Rockchip versions.
> > 
> > Do you /know/ that for a fact?
> > 
> > I thought Rockchip's BSP used a single driver for all of it's ISP range?
> >  (Not rkisp1)
> 
> They do, and we could do so in mainline too, adding support for ISP v1
> to a new driver that supports v2 and v3 as well. Doing it the other way
> around without breaking any backward compatibility doesn't seem
> realistic.
> 
> These are all speculations anyway. The bottom line is that without
> information about the VSI versioning scheme for current and future ISP
> versions, there's nothing we can really do.

Also, this discussion was about new v1.x versions of the ISP v1, and I'm
pretty sure we won't see any of those.

> > > > >   };
> > > > >   
> > > > >   enum rkisp1_cif_isp_histogram_mode {
> > > > > @@ -992,4 +994,4 @@ struct rkisp1_stat_buffer {
> > > > >     struct rkisp1_cif_isp_stat params;
> > > > >   };
> > > > >   
> > > > > -#endif /* _RKISP1_CONFIG_H */
> > > > > +#endif /* _UAPI_RKISP1_CONFIG_H */

Patch
diff mbox series 

diff --git a/include/linux/rkisp1-config.h b/include/linux/rkisp1-config.h
index ec7cde8cd2e3..2d1c448a6ab8 100644
--- a/include/linux/rkisp1-config.h
+++ b/include/linux/rkisp1-config.h
@@ -4,8 +4,8 @@ 
  * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
  */
 
-#ifndef _RKISP1_CONFIG_H
-#define _RKISP1_CONFIG_H
+#ifndef _UAPI_RKISP1_CONFIG_H
+#define _UAPI_RKISP1_CONFIG_H
 
 #include <linux/types.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
+ * @RKISP1_V_IMX8MP: used in at least imx8mp
  */
 enum rkisp1_cif_isp_version {
 	RKISP1_V10 = 10,
 	RKISP1_V11,
 	RKISP1_V12,
 	RKISP1_V13,
+	RKISP1_V_IMX8MP,
 };
 
 enum rkisp1_cif_isp_histogram_mode {
@@ -992,4 +994,4 @@  struct rkisp1_stat_buffer {
 	struct rkisp1_cif_isp_stat params;
 };
 
-#endif /* _RKISP1_CONFIG_H */
+#endif /* _UAPI_RKISP1_CONFIG_H */