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

Mon Jul 4 03:36:21 PDT 2022

Hello,

On Sun, Jun 26, 2022 at 02:07:22PM +0300, Laurent Pinchart wrote:
> 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 at 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.

Thinking out loud:

The most sure way is clearly to have feature flags for each feature.
Maybe one set of internal ones that only affect the driver, and another
set for userspace to see what the hardware is capable of. On the other
hand, from the top of my head, the ones that userspace cares about could
be probed in other ways, like the presence of the self path can be
obtained from the media graph, and available controls/formats from...
the list of available controls/formats.

Also if we have flags then... what's the point of the version number? Is
there any value in letting userspace know some version number if it
doesn't particularly correlate with the actual features that are
available? Plus, we run the risk of version numbers (from the version
register) colliding between VeriSilicon and Rockchip. Plus my
understanding is that the current version number that we have right now
(V10 ~ V13) is fairly arbitrary anyway? (but we have to keep it for
compatibility reasons.)

So...

1 - Do we need a precise version number for the purpose of probing
capabilities? Can it just be replaced with feature flags?
2 - Do we need feature flags? Can all information (that userspace cares
about be obtained by some other source? (I guess this needs more
investigation)
3 - Maybe userspace doesn't care about the precise version number at
all? We could just continue with an aribitrary version number (like
IMX_V10)?
4 - Or we could just expand the version number with what we get from the
version register? And have different... labels (?) based on the vendor?

Lots of thinking out loud with no clear conclusion :D

Paul

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