| Message ID | 20220614191127.3420492-49-paul.elder@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: rkisp1: Cleanups and add support for i.MX8MP | expand |
Hi Paul, Thank you for the patch. 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 ? > }; > > enum rkisp1_cif_isp_histogram_mode {
On 18.06.2022 02:26, Laurent Pinchart wrote: >Hi Paul, > >Thank you for the patch. > >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/ thanks, Dafna > >> }; >> >> enum rkisp1_cif_isp_histogram_mode { > >-- >Regards, > >Laurent Pinchart
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 {
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@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 {
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, }; enum rkisp1_cif_isp_histogram_mode {
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(+)