Re: [PATCH 48/55] media: rkisp1: Add match data for i.MX8MP ISP

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

On Sun, Jun 26, 2022 at 07:05:43AM +0300, Dafna Hirschfeld wrote:
> On 18.06.2022 02:26, Laurent Pinchart wrote:
> > On Wed, Jun 15, 2022 at 04:11:20AM +0900, Paul Elder wrote:
> >> Add match data to the rkisp1 driver to match the i.MX8MP ISP.
> >>
> >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >> ---
> >>  .../platform/rockchip/rkisp1/rkisp1-dev.c     | 22 +++++++++++++++++++
> >>  include/uapi/linux/rkisp1-config.h            |  3 +++
> >>  2 files changed, 25 insertions(+)
> >>
> >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> >> index 62fa2bd275fe..3a0115bdcee5 100644
> >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> >> @@ -486,6 +486,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",
> >> @@ -495,6 +513,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 583ca0d9a79d..40677d47825c 100644
> >> --- a/include/uapi/linux/rkisp1-config.h
> >> +++ b/include/uapi/linux/rkisp1-config.h
> >> @@ -140,12 +140,15 @@
> >>   * @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,
> >> +	/* TODO Choose a better version for this */
> >> +	IMX8MP_V10,
> >
> > Time to address this ? :-)
> >
> > Does anyone know where the current versioning scheme come from ?
> 
> It was added by Heiko Stübner, basically trying to figure out the
> versions from the vedor code,
> see https://lore.kernel.org/all/20210121144407.9045-6-dafna.hirschfeld@collabora.com/

Is that public code ? Heiko, do you have any pointer to it ?

As far as I understand, this ISP IP has been forked, and is now
developed in parallel by Rockchip (for their own SoCs) and by
VeriSilicon (under the name of ISP8000Nano). The versioning schemes on
the two sides are different, and may have common roots. The fact that
the ISP8000Nano can be customized at synthesis time also complicates
this.

The question at hand is how to expose a single coherent versioning
scheme to userspace in this driver. The hardware has a version
identification register that we could use, but I don't know if it gives
us enough information (as in guaranteeing that the same version value
won't be used for different IP versions that would need to be
distinguished from each other in userspace). The fact that Rockchip and
VeriSilicon probably develop their own new versions without cooperating
increases the risk of collision.

We will also have to tackle the question of UABI. Newer ISP versions
will require extensions to the ISP parameters structure. The i.MX8MP has
extra processing blocks that are not supported by the driver today. One
option would be to duplicate the top-level rkisp1_params_cfg structure
per ISP version, as well as a set of lower-level structure where
appropriate (how to handle that easily and efficiently on the driver
side will be interesting to figure out). Another option is to only add
parameters to the end of the rkisp1_params_cfg structure, but I fear
that won't scale. We'll end up with a large structure where lots of data
will be irrelevant for any particular ISP version, and that will be
error-prone.

Comments and idea will be appreciated.

> >>  };
> >>
> >>  enum rkisp1_cif_isp_histogram_mode {

-- 
Regards,

Laurent Pinchart