diff mbox series

[04/17] media: atomisp: pci: do not use err var when checking port validity for ISP2400

Message ID 20211017161958.44351-5-kitakar@gmail.com (mailing list archive)
State Accepted, archived
Headers
Series various fixes for atomisp to make it work |

Commit Message

Tsuchiya Yuto Oct. 17, 2021, 4:19 p.m. UTC 
  Currently, the `port >= N_CSI_PORTS || err` checks for ISP2400 are always
evaluated as true because the err variable is set to `-EINVAL` on
declaration but the variable is never used until the evaluation.

Looking at the diff of commit 3c0538fbad9f ("media: atomisp: get rid of
most checks for ISP2401 version"), the `port >= N_CSI_PORTS` check is
for ISP2400 and the err variable check is for ISP2401. Fix this issue
by adding ISP version test there accordingly.

Yes, there are other better ways to fix this issue, like adding support
for ISP2400 to ia_css_mipi_is_source_port_valid(). In this way, we can
unify the following test:

	if (!IS_ISP2401)
		port = (unsigned int)pipe->stream->config.source.port.port;
	else
		err = ia_css_mipi_is_source_port_valid(pipe, &port);

However, the IS_ISP2401 test here (formerly `ifdef ISP2401`) is not
a result of real hardware difference, but just a result of the following
two different versions of driver merged by tools [1]:

  - ISP2400: irci_stable_candrpv_0415_20150521_0458
  - ISP2401: irci_ecr-master_20150911_0724

We should eventually remove (not unify) such tests caused by just a
driver version difference and use just one version of driver. So, for
now, let's avoid further unification.

[1] The function ia_css_mipi_is_source_port_valid() and its usage is
    added on updating css version to irci_master_20150701_0213
    https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
    ("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")

Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
 drivers/staging/media/atomisp/pci/sh_css_mipi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Tsuchiya Yuto Oct. 28, 2021, 4:12 a.m. UTC | #1 
<Adding back people/list to Cc>

On Tue, 2021-10-26 at 09:26 +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 18 Oct 2021 01:19:44 +0900
> Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> 
> > Currently, the `port >= N_CSI_PORTS || err` checks for ISP2400 are always
> > evaluated as true because the err variable is set to `-EINVAL` on
> > declaration but the variable is never used until the evaluation.
> > 
> > Looking at the diff of commit 3c0538fbad9f ("media: atomisp: get rid of
> > most checks for ISP2401 version"), the `port >= N_CSI_PORTS` check is
> > for ISP2400 and the err variable check is for ISP2401. Fix this issue
> > by adding ISP version test there accordingly.
> > 
> > Yes, there are other better ways to fix this issue, like adding support
> > for ISP2400 to ia_css_mipi_is_source_port_valid(). In this way, we can
> > unify the following test:
> > 
> > 	if (!IS_ISP2401)
> > 		port = (unsigned int)pipe->stream->config.source.port.port;
> > 	else
> > 		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > 
> > However, the IS_ISP2401 test here (formerly `ifdef ISP2401`) is not
> > a result of real hardware difference, but just a result of the following
> > two different versions of driver merged by tools [1]:
> > 
> >   - ISP2400: irci_stable_candrpv_0415_20150521_0458
> >   - ISP2401: irci_ecr-master_20150911_0724
> 
> No.
> 
> While I don't have any internal information from the hardware manufacturer,
> I guess you misinterpreted things here. 2400 and 2401 are different
> hardware versions. See atomisp_pci_probe() logic.
> 
> Basically, Cherrytail and Anniedale comes with 2401. Older Atom CPUs
> (Merrifield and Baytrail) comes with 2400.

Yes, indeed, 2400 and 2401 are different hardware. When they (I mean who
originally wrote atomisp driver non-upstream) needed to distinguish
between ISP2400 and ISP2401, they used the ifdefs like the following:

  - USE_INPUT_SYSTEM_VERSION_2    (for both ISP2400/ISP2401)
  - USE_INPUT_SYSTEM_VERSION_2401 (for ISP2401)
  ...

I think this is a sign that the atomisp driver supports both
ISP2400/ISP2401 in a single version.

Indeed, the upstreamed atomisp uses irci_stable_candrpv_0415_20150521_0458
for ISP2400 and IIUC it was working on Bay Trail. On the other hand,
intel-aero is a kernel for Cherry Trail and uses the same version
irci_stable_candrpv_0415_20150521_0458.

So, both ISP version ISP2400/ISP2401 can be supported by a single
driver version.

> > We should eventually remove (not unify) such tests caused by just a
> > driver version difference and use just one version of driver. So, for
> > now, let's avoid further unification.
> > 
> > [1] The function ia_css_mipi_is_source_port_valid() and its usage is
> >     added on updating css version to irci_master_20150701_0213
> >     https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
> >     ("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")
> 
> What happens is that there is a 2401 and a 2401 "legacy". It sounds
> that this due to some different software stacks that are reflected both
> at the firmware and at the driver.

Yeah, I'm not sure what the "legacy" is. It might be a reference of
`ISP2401_NEW_INPUT_SYSTEM` (css_2401_csi2p_system) and
non-`ISP2401_NEW_INPUT_SYSTEM` (css_2401_system).

> -
> 
> On other words, this patch requires some rework, as otherwise it will break
> support for Baytrail.

You mean "this patch"? then, I intended this patch is rather a fix for
ISP2400 case! The err variable for ISP2400 case is always true because
it is not used before the error check:

        int
        allocate_mipi_frames(struct ia_css_pipe *pipe,
        		     struct ia_css_stream_info *info)
        {
        	int err = -EINVAL;
[...]
        	if (!IS_ISP2401)
        		port = (unsigned int)pipe->stream->config.source.port.port;
        	else
        		err = ia_css_mipi_is_source_port_valid(pipe, &port);
        
        	assert(port < N_CSI_PORTS);
        
        	if (port >= N_CSI_PORTS || err) {
        		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
        				    "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
        				    pipe, port);
        		return -EINVAL;
        	}

The first usage of err variable is ia_css_mipi_is_source_port_valid()
for IS_ISP2401 case, but it's not used for ISP2400 case. This causes
the evaluation `port >= N_CSI_PORTS || err` always true for ISP2400 case,
meaning it will be always treated as a error.

> Also, patch 13 should be dropped, as the firmware versions for 2400 are
> different

The firmware version for 2400 on the upstreamed atomisp is
irci_stable_candrpv_0415_20150521_0458 :-)

        static const char *isp2400_release_version = STR(irci_stable_candrpv_0415_20150521_0458);
        static const char *isp2401_release_version = STR(irci_ecr - master_20150911_0724);

The intention of that patch is rather, it clarifies ISP2401 is now using
the same driver (css) version as ISP2400.

> - and maybe patches 8 to 12 may need more work in order to not
> touch 2400.

Those patches do not break ISP2400, because what they do for ISP2400
is that, they remove members from `struct`s which were initially inside
of `ifdef ISP2401`. And because these removed members were initially
inside of the ifdefs, the usage was also inside the ifdefs.

Regards,
Tsuchiya Yuto
Mauro Carvalho Chehab Oct. 28, 2021, 6:52 a.m. UTC | #2 
Em Thu, 28 Oct 2021 13:12:45 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:

> <Adding back people/list to Cc>
> 
> On Tue, 2021-10-26 at 09:26 +0100, Mauro Carvalho Chehab wrote:
> > Em Mon, 18 Oct 2021 01:19:44 +0900
> > Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> >   
> > > Currently, the `port >= N_CSI_PORTS || err` checks for ISP2400 are always
> > > evaluated as true because the err variable is set to `-EINVAL` on
> > > declaration but the variable is never used until the evaluation.
> > > 
> > > Looking at the diff of commit 3c0538fbad9f ("media: atomisp: get rid of
> > > most checks for ISP2401 version"), the `port >= N_CSI_PORTS` check is
> > > for ISP2400 and the err variable check is for ISP2401. Fix this issue
> > > by adding ISP version test there accordingly.
> > > 
> > > Yes, there are other better ways to fix this issue, like adding support
> > > for ISP2400 to ia_css_mipi_is_source_port_valid(). In this way, we can
> > > unify the following test:
> > > 
> > > 	if (!IS_ISP2401)
> > > 		port = (unsigned int)pipe->stream->config.source.port.port;
> > > 	else
> > > 		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > 
> > > However, the IS_ISP2401 test here (formerly `ifdef ISP2401`) is not
> > > a result of real hardware difference, but just a result of the following
> > > two different versions of driver merged by tools [1]:
> > > 
> > >   - ISP2400: irci_stable_candrpv_0415_20150521_0458
> > >   - ISP2401: irci_ecr-master_20150911_0724  
> > 
> > No.
> > 
> > While I don't have any internal information from the hardware manufacturer,
> > I guess you misinterpreted things here. 2400 and 2401 are different
> > hardware versions. See atomisp_pci_probe() logic.
> > 
> > Basically, Cherrytail and Anniedale comes with 2401. Older Atom CPUs
> > (Merrifield and Baytrail) comes with 2400.  
> 
> Yes, indeed, 2400 and 2401 are different hardware. When they (I mean who
> originally wrote atomisp driver non-upstream) needed to distinguish
> between ISP2400 and ISP2401, they used the ifdefs like the following:
> 
>   - USE_INPUT_SYSTEM_VERSION_2    (for both ISP2400/ISP2401)
>   - USE_INPUT_SYSTEM_VERSION_2401 (for ISP2401)
>   ...
> 
> I think this is a sign that the atomisp driver supports both
> ISP2400/ISP2401 in a single version.

Yes, the driver was written to support both, but it is not that simple.

As far as I heard, the downstream version used some code generator
to produce both the firmware and the driver. Those evaluate
internally some of the vars. What Alan seems to have done is to
generate both ISP2400 and ISP2401 versions and then merge them into
a single code, adding the ISP2400 version check.

If you look at the initial committed patch, you'll see things like:

	$ git show a49d25364dfb9f8a64037488a39ab1f56c5fa419
...
	+#ifdef ISP2401
	+
	+#endif

That clearly shows that some tool merged both CSP2400 and 2401 variants
into a single driver.

I did a large re-work of converting such ifdefs into runtime if's, as
the end goal is to be able have support for both BYT and CHT code
compiled at the time time, having the decision of enabling/disabling
the needed features in runtime.

This is not complete, though, as there's still a compile-time
variable to select the CHT variant: VIDEO_ATOMISP_ISP2401.

I would love to completely get rid of that, but the remaining bits
require to be able to test it on both BYT and CHT variants.

-

Now, if we look at the #ifdefs of the original upstreamed version,
clearly some of the changes are just improvements on the driver
(some are related to the addition of an error-check, for instance),
while others seem to be related to different hardware features
(like some related to the clock frequencies).

However, for most part, it is almost impossible to know if they're
due to a different firmware version or due to different hardware
features.

> Indeed, the upstreamed atomisp uses irci_stable_candrpv_0415_20150521_0458
> for ISP2400 and IIUC it was working on Bay Trail. 

Had you test it on BYT? On what hardware?

Btw, it would help a lot if you could describe exactly what tools are
you using to test. That would allow us to also test it on our hardware.

> On the other hand,
> intel-aero is a kernel for Cherry Trail and uses the same version
> irci_stable_candrpv_0415_20150521_0458.

See the Alan's original comment:

	+7. The ISP code depends on the exact FW version. The version defined in
	+   BYT: 
	+   drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
	+   static const char *release_version = STR(irci_stable_candrpv_0415_20150521_0458);
	+   CHT:
	+   drivers/staging/media/atomisp/pci/atomisp2/css/sh_css_firmware.c
	+   static const char *release_version = STR(irci_ecr-master_20150911_0724);
	+
	+   At some point we may need to round up a few driver versions and see if
	+   there are any specific things that can be done to fold in support for
	+   multiple firmware versions.

He clearly created it with a different firmware for CHT. Yet, nobody
were able to find the exact firmware he used. So, I ended changing
some parts of the driver to become closer to the one used by Intel Aero,
as the firmware for it is easy to retrieve.

Btw, even if BYT and CHT firmware were produced against the same version
of the generator toolset, it doesn't necessarily mean that they're
identical.

> 
> So, both ISP version ISP2400/ISP2401 can be supported by a single
> driver version.

Yes, but they needed to be compiled with a different option right
now, as the register addresses are different. That's basically the
main reason for VIDEO_ATOMISP_ISP2401 Kconfig option: to select
between ISP2400/candrpv register map and ISP2401/ecr-master register map.

Not clear yet if those differences are just due to different
firmware versions, just due to different hardware versions or both.

> > > We should eventually remove (not unify) such tests caused by just a
> > > driver version difference and use just one version of driver. So, for
> > > now, let's avoid further unification.
> > > 
> > > [1] The function ia_css_mipi_is_source_port_valid() and its usage is
> > >     added on updating css version to irci_master_20150701_0213
> > >     https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
> > >     ("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")  
> > 
> > What happens is that there is a 2401 and a 2401 "legacy". It sounds
> > that this due to some different software stacks that are reflected both
> > at the firmware and at the driver.  
> 
> Yeah, I'm not sure what the "legacy" is. It might be a reference of
> `ISP2401_NEW_INPUT_SYSTEM` (css_2401_csi2p_system) and
> non-`ISP2401_NEW_INPUT_SYSTEM` (css_2401_system).

Maybe.

> 
> > -
> > 
> > On other words, this patch requires some rework, as otherwise it will break
> > support for Baytrail.  
> 
> You mean "this patch"? then, I intended this patch is rather a fix for
> ISP2400 case! The err variable for ISP2400 case is always true because
> it is not used before the error check:
> 
>         int
>         allocate_mipi_frames(struct ia_css_pipe *pipe,
>         		     struct ia_css_stream_info *info)
>         {
>         	int err = -EINVAL;
> [...]
>         	if (!IS_ISP2401)
>         		port = (unsigned int)pipe->stream->config.source.port.port;
>         	else
>         		err = ia_css_mipi_is_source_port_valid(pipe, &port);
>         
>         	assert(port < N_CSI_PORTS);
>         
>         	if (port >= N_CSI_PORTS || err) {
>         		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
>         				    "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
>         				    pipe, port);
>         		return -EINVAL;
>         	}
> 
> The first usage of err variable is ia_css_mipi_is_source_port_valid()
> for IS_ISP2401 case, but it's not used for ISP2400 case. This causes
> the evaluation `port >= N_CSI_PORTS || err` always true for ISP2400 case,
> meaning it will be always treated as a error.

Ok. So, please make it clearer at the description.

> 
> > Also, patch 13 should be dropped, as the firmware versions for 2400 are
> > different  
> 
> The firmware version for 2400 on the upstreamed atomisp is
> irci_stable_candrpv_0415_20150521_0458 :-)
> 
>         static const char *isp2400_release_version = STR(irci_stable_candrpv_0415_20150521_0458);
>         static const char *isp2401_release_version = STR(irci_ecr - master_20150911_0724);
> 
> The intention of that patch is rather, it clarifies ISP2401 is now using
> the same driver (css) version as ISP2400.

Ok, I see the point. Yeah, using a single release version makes
things better.

> 
> > - and maybe patches 8 to 12 may need more work in order to not
> > touch 2400.  
> 
> Those patches do not break ISP2400, because what they do for ISP2400
> is that, they remove members from `struct`s which were initially inside
> of `ifdef ISP2401`. And because these removed members were initially
> inside of the ifdefs, the usage was also inside the ifdefs.

Had you test them with both ISP2400 and ISP2401 hardware?

Did you compile with both VIDEO_ATOMISP_ISP2401 enabled and
disabled?

Regards,
Mauro
Mauro Carvalho Chehab Oct. 28, 2021, 11:39 a.m. UTC | #3 
Em Thu, 28 Oct 2021 13:12:45 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:

> <Adding back people/list to Cc>
> 
> On Tue, 2021-10-26 at 09:26 +0100, Mauro Carvalho Chehab wrote:
> > Em Mon, 18 Oct 2021 01:19:44 +0900
> > Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> >   
> > > Currently, the `port >= N_CSI_PORTS || err` checks for ISP2400 are always
> > > evaluated as true because the err variable is set to `-EINVAL` on
> > > declaration but the variable is never used until the evaluation.
> > > 
> > > Looking at the diff of commit 3c0538fbad9f ("media: atomisp: get rid of
> > > most checks for ISP2401 version"), the `port >= N_CSI_PORTS` check is
> > > for ISP2400 and the err variable check is for ISP2401. Fix this issue
> > > by adding ISP version test there accordingly.
> > > 
> > > Yes, there are other better ways to fix this issue, like adding support
> > > for ISP2400 to ia_css_mipi_is_source_port_valid(). In this way, we can
> > > unify the following test:
> > > 
> > > 	if (!IS_ISP2401)
> > > 		port = (unsigned int)pipe->stream->config.source.port.port;
> > > 	else
> > > 		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > 
> > > However, the IS_ISP2401 test here (formerly `ifdef ISP2401`) is not
> > > a result of real hardware difference, but just a result of the following
> > > two different versions of driver merged by tools [1]:
> > > 
> > >   - ISP2400: irci_stable_candrpv_0415_20150521_0458
> > >   - ISP2401: irci_ecr-master_20150911_0724  
> > 
> > No.
> > 
> > While I don't have any internal information from the hardware manufacturer,
> > I guess you misinterpreted things here. 2400 and 2401 are different
> > hardware versions. See atomisp_pci_probe() logic.
> > 
> > Basically, Cherrytail and Anniedale comes with 2401. Older Atom CPUs
> > (Merrifield and Baytrail) comes with 2400.  
> 
> Yes, indeed, 2400 and 2401 are different hardware. When they (I mean who
> originally wrote atomisp driver non-upstream) needed to distinguish
> between ISP2400 and ISP2401, they used the ifdefs like the following:
> 
>   - USE_INPUT_SYSTEM_VERSION_2    (for both ISP2400/ISP2401)
>   - USE_INPUT_SYSTEM_VERSION_2401 (for ISP2401)
>   ...
> 
> I think this is a sign that the atomisp driver supports both
> ISP2400/ISP2401 in a single version.

Actually, supporting both on a single version is part of Alan's work.

It seems he used the generation tool to produce a version for 2400, and
then re-used it to generate for 2401. It then used some scripting tool
to convert the differences on #ifdef ISP2401. See:

	a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")

There are things there like:

	+#ifdef ISP2401
	+
	+#endif

I did a large cleanup work to get rid of those ifdefs, replacing them
by runtime logic.

The end goal is to have a single compile-time driver that works for
both 2400 and 2401.

This is not possible yet, as there are some registers that are mapped
on different addresses, depending on the hardware version, and making
it generic requires a lot of work and tests. So, for now, we need to 
have a compile-time option to select between both.

> Indeed, the upstreamed atomisp uses irci_stable_candrpv_0415_20150521_0458
> for ISP2400 and IIUC it was working on Bay Trail. On the other hand,
> intel-aero is a kernel for Cherry Trail and uses the same version
> irci_stable_candrpv_0415_20150521_0458.
> 
> So, both ISP version ISP2400/ISP2401 can be supported by a single
> driver version.

I See. OK!

> > > We should eventually remove (not unify) such tests caused by just a
> > > driver version difference and use just one version of driver. So, for
> > > now, let's avoid further unification.
> > > 
> > > [1] The function ia_css_mipi_is_source_port_valid() and its usage is
> > >     added on updating css version to irci_master_20150701_0213
> > >     https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
> > >     ("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")  
> > 
> > What happens is that there is a 2401 and a 2401 "legacy". It sounds
> > that this due to some different software stacks that are reflected both
> > at the firmware and at the driver.  
> 
> Yeah, I'm not sure what the "legacy" is. It might be a reference of
> `ISP2401_NEW_INPUT_SYSTEM` (css_2401_csi2p_system) and
> non-`ISP2401_NEW_INPUT_SYSTEM` (css_2401_system).
> 
> > -
> > 
> > On other words, this patch requires some rework, as otherwise it will break
> > support for Baytrail.  
> 
> You mean "this patch"? then, I intended this patch is rather a fix for
> ISP2400 case! The err variable for ISP2400 case is always true because
> it is not used before the error check:
> 
>         int
>         allocate_mipi_frames(struct ia_css_pipe *pipe,
>         		     struct ia_css_stream_info *info)
>         {
>         	int err = -EINVAL;
> [...]
>         	if (!IS_ISP2401)
>         		port = (unsigned int)pipe->stream->config.source.port.port;
>         	else
>         		err = ia_css_mipi_is_source_port_valid(pipe, &port);
>         
>         	assert(port < N_CSI_PORTS);
>         
>         	if (port >= N_CSI_PORTS || err) {
>         		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
>         				    "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
>         				    pipe, port);
>         		return -EINVAL;
>         	}
> 
> The first usage of err variable is ia_css_mipi_is_source_port_valid()
> for IS_ISP2401 case, but it's not used for ISP2400 case. This causes
> the evaluation `port >= N_CSI_PORTS || err` always true for ISP2400 case,
> meaning it will be always treated as a error.

Ok. Had you test the driver with Baytrail?

> 
> > Also, patch 13 should be dropped, as the firmware versions for 2400 are
> > different  
> 
> The firmware version for 2400 on the upstreamed atomisp is
> irci_stable_candrpv_0415_20150521_0458 :-)
> 
>         static const char *isp2400_release_version = STR(irci_stable_candrpv_0415_20150521_0458);
>         static const char *isp2401_release_version = STR(irci_ecr - master_20150911_0724);
> 
> The intention of that patch is rather, it clarifies ISP2401 is now using
> the same driver (css) version as ISP2400.

Ok.

> > - and maybe patches 8 to 12 may need more work in order to not
> > touch 2400.  
> 
> Those patches do not break ISP2400, because what they do for ISP2400
> is that, they remove members from `struct`s which were initially inside
> of `ifdef ISP2401`. And because these removed members were initially
> inside of the ifdefs, the usage was also inside the ifdefs.

Did you test on Baytrail (ISP2400), and with the compile-time option
enabled/disabled?

Regards,
Mauro
Tsuchiya Yuto Nov. 1, 2021, 1:38 p.m. UTC | #4 
On Thu, 2021-10-28 at 12:39 +0100, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Oct 2021 13:12:45 +0900
> Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> 
> > <Adding back people/list to Cc>
> > 
> > On Tue, 2021-10-26 at 09:26 +0100, Mauro Carvalho Chehab wrote:
> > > Em Mon, 18 Oct 2021 01:19:44 +0900
> > > Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> > >   
> > > > Currently, the `port >= N_CSI_PORTS || err` checks for ISP2400 are always
> > > > evaluated as true because the err variable is set to `-EINVAL` on
> > > > declaration but the variable is never used until the evaluation.
> > > > 
> > > > Looking at the diff of commit 3c0538fbad9f ("media: atomisp: get rid of
> > > > most checks for ISP2401 version"), the `port >= N_CSI_PORTS` check is
> > > > for ISP2400 and the err variable check is for ISP2401. Fix this issue
> > > > by adding ISP version test there accordingly.
> > > > 
> > > > Yes, there are other better ways to fix this issue, like adding support
> > > > for ISP2400 to ia_css_mipi_is_source_port_valid(). In this way, we can
> > > > unify the following test:
> > > > 
> > > > 	if (!IS_ISP2401)
> > > > 		port = (unsigned int)pipe->stream->config.source.port.port;
> > > > 	else
> > > > 		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > > 
> > > > However, the IS_ISP2401 test here (formerly `ifdef ISP2401`) is not
> > > > a result of real hardware difference, but just a result of the following
> > > > two different versions of driver merged by tools [1]:
> > > > 
> > > >   - ISP2400: irci_stable_candrpv_0415_20150521_0458
> > > >   - ISP2401: irci_ecr-master_20150911_0724  
> > > 
> > > No.
> > > 
> > > While I don't have any internal information from the hardware manufacturer,
> > > I guess you misinterpreted things here. 2400 and 2401 are different
> > > hardware versions. See atomisp_pci_probe() logic.
> > > 
> > > Basically, Cherrytail and Anniedale comes with 2401. Older Atom CPUs
> > > (Merrifield and Baytrail) comes with 2400.  
> > 
> > Yes, indeed, 2400 and 2401 are different hardware. When they (I mean who
> > originally wrote atomisp driver non-upstream) needed to distinguish
> > between ISP2400 and ISP2401, they used the ifdefs like the following:
> > 
> >   - USE_INPUT_SYSTEM_VERSION_2    (for both ISP2400/ISP2401)
> >   - USE_INPUT_SYSTEM_VERSION_2401 (for ISP2401)
> >   ...
> > 
> > I think this is a sign that the atomisp driver supports both
> > ISP2400/ISP2401 in a single version.
> 
> Actually, supporting both on a single version is part of Alan's work.
> 
> It seems he used the generation tool to produce a version for 2400, and
> then re-used it to generate for 2401. It then used some scripting tool
> to convert the differences on #ifdef ISP2401. See:
> 
> 	a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
> 
> There are things there like:
> 
> 	+#ifdef ISP2401
> 	+
> 	+#endif
> 
> I did a large cleanup work to get rid of those ifdefs, replacing them
> by runtime logic.
> 
> The end goal is to have a single compile-time driver that works for
> both 2400 and 2401.
> 
> This is not possible yet, as there are some registers that are mapped
> on different addresses, depending on the hardware version, and making
> it generic requires a lot of work and tests. So, for now, we need to 
> have a compile-time option to select between both.
> 
> > Indeed, the upstreamed atomisp uses irci_stable_candrpv_0415_20150521_0458
> > for ISP2400 and IIUC it was working on Bay Trail. On the other hand,
> > intel-aero is a kernel for Cherry Trail and uses the same version
> > irci_stable_candrpv_0415_20150521_0458.
> > 
> > So, both ISP version ISP2400/ISP2401 can be supported by a single
> > driver version.
> 
> I See. OK!
> 
> > > > We should eventually remove (not unify) such tests caused by just a
> > > > driver version difference and use just one version of driver. So, for
> > > > now, let's avoid further unification.
> > > > 
> > > > [1] The function ia_css_mipi_is_source_port_valid() and its usage is
> > > >     added on updating css version to irci_master_20150701_0213
> > > >     https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
> > > >     ("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")  
> > > 
> > > What happens is that there is a 2401 and a 2401 "legacy". It sounds
> > > that this due to some different software stacks that are reflected both
> > > at the firmware and at the driver.  
> > 
> > Yeah, I'm not sure what the "legacy" is. It might be a reference of
> > `ISP2401_NEW_INPUT_SYSTEM` (css_2401_csi2p_system) and
> > non-`ISP2401_NEW_INPUT_SYSTEM` (css_2401_system).
> > 
> > > -
> > > 
> > > On other words, this patch requires some rework, as otherwise it will break
> > > support for Baytrail.  
> > 
> > You mean "this patch"? then, I intended this patch is rather a fix for
> > ISP2400 case! The err variable for ISP2400 case is always true because
> > it is not used before the error check:
> > 
> >         int
> >         allocate_mipi_frames(struct ia_css_pipe *pipe,
> >         		     struct ia_css_stream_info *info)
> >         {
> >         	int err = -EINVAL;
> > [...]
> >         	if (!IS_ISP2401)
> >         		port = (unsigned int)pipe->stream->config.source.port.port;
> >         	else
> >         		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> >         
> >         	assert(port < N_CSI_PORTS);
> >         
> >         	if (port >= N_CSI_PORTS || err) {
> >         		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> >         				    "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
> >         				    pipe, port);
> >         		return -EINVAL;
> >         	}
> > 
> > The first usage of err variable is ia_css_mipi_is_source_port_valid()
> > for IS_ISP2401 case, but it's not used for ISP2400 case. This causes
> > the evaluation `port >= N_CSI_PORTS || err` always true for ISP2400 case,
> > meaning it will be always treated as a error.
> 
> Ok. Had you test the driver with Baytrail?

Unfortunately, no. I don't have a Baytrail device to test (yet?). I
noticed this issue anyway when I tried removing `#ifdef ISP2401`.

This is not directly related to this series, but how we should reduce
the ifdef usage in the future? Here are my two ideas:

  1. (my initial idea) remove `#ifdef ISP2401` part and make ISP2401
     part completely irci_stable_candrpv_0415_20150521_0458

this way does not require (relatively) much human work I think.

But as Mauro says, the `#ifdef ISP2401` part (irci_ecr-master_20150911_0724)
is basically an improved version. So, we may also:

  2. continue unifying `#ifdef ISP2401` and `#ifndef ISP2401` parts

but this way needs more human work I think though. And if we go this
way, I also need to rewrite this patch as mentioned in the commit
message.

> > 
> > > Also, patch 13 should be dropped, as the firmware versions for 2400 are
> > > different  
> > 
> > The firmware version for 2400 on the upstreamed atomisp is
> > irci_stable_candrpv_0415_20150521_0458 :-)
> > 
> >         static const char *isp2400_release_version = STR(irci_stable_candrpv_0415_20150521_0458);
> >         static const char *isp2401_release_version = STR(irci_ecr - master_20150911_0724);
> > 
> > The intention of that patch is rather, it clarifies ISP2401 is now using
> > the same driver (css) version as ISP2400.
> 
> Ok.
> 
> > > - and maybe patches 8 to 12 may need more work in order to not
> > > touch 2400.  
> > 
> > Those patches do not break ISP2400, because what they do for ISP2400
> > is that, they remove members from `struct`s which were initially inside
> > of `ifdef ISP2401`. And because these removed members were initially
> > inside of the ifdefs, the usage was also inside the ifdefs.
> 
> Did you test on Baytrail (ISP2400), and with the compile-time option
> enabled/disabled?

Sorry, I should have clarified on the cover later. For ISP2400, I did
compile test only (CONFIG_VIDEO_ATOMISP_ISP2401 enabled/disabled).

> Regards,
> Mauro
Mauro Carvalho Chehab Nov. 1, 2021, 2:10 p.m. UTC | #5 
Em Mon, 01 Nov 2021 22:38:55 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:

> On Thu, 2021-10-28 at 12:39 +0100, Mauro Carvalho Chehab wrote:
> > Em Thu, 28 Oct 2021 13:12:45 +0900
> > Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> >   
> > > <Adding back people/list to Cc>
> > > 
> > > On Tue, 2021-10-26 at 09:26 +0100, Mauro Carvalho Chehab wrote:  
> > > > Em Mon, 18 Oct 2021 01:19:44 +0900
> > > > Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> > > >     
> > > > > Currently, the `port >= N_CSI_PORTS || err` checks for ISP2400 are always
> > > > > evaluated as true because the err variable is set to `-EINVAL` on
> > > > > declaration but the variable is never used until the evaluation.
> > > > > 
> > > > > Looking at the diff of commit 3c0538fbad9f ("media: atomisp: get rid of
> > > > > most checks for ISP2401 version"), the `port >= N_CSI_PORTS` check is
> > > > > for ISP2400 and the err variable check is for ISP2401. Fix this issue
> > > > > by adding ISP version test there accordingly.
> > > > > 
> > > > > Yes, there are other better ways to fix this issue, like adding support
> > > > > for ISP2400 to ia_css_mipi_is_source_port_valid(). In this way, we can
> > > > > unify the following test:
> > > > > 
> > > > > 	if (!IS_ISP2401)
> > > > > 		port = (unsigned int)pipe->stream->config.source.port.port;
> > > > > 	else
> > > > > 		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > > > 
> > > > > However, the IS_ISP2401 test here (formerly `ifdef ISP2401`) is not
> > > > > a result of real hardware difference, but just a result of the following
> > > > > two different versions of driver merged by tools [1]:
> > > > > 
> > > > >   - ISP2400: irci_stable_candrpv_0415_20150521_0458
> > > > >   - ISP2401: irci_ecr-master_20150911_0724    
> > > > 
> > > > No.
> > > > 
> > > > While I don't have any internal information from the hardware manufacturer,
> > > > I guess you misinterpreted things here. 2400 and 2401 are different
> > > > hardware versions. See atomisp_pci_probe() logic.
> > > > 
> > > > Basically, Cherrytail and Anniedale comes with 2401. Older Atom CPUs
> > > > (Merrifield and Baytrail) comes with 2400.    
> > > 
> > > Yes, indeed, 2400 and 2401 are different hardware. When they (I mean who
> > > originally wrote atomisp driver non-upstream) needed to distinguish
> > > between ISP2400 and ISP2401, they used the ifdefs like the following:
> > > 
> > >   - USE_INPUT_SYSTEM_VERSION_2    (for both ISP2400/ISP2401)
> > >   - USE_INPUT_SYSTEM_VERSION_2401 (for ISP2401)
> > >   ...
> > > 
> > > I think this is a sign that the atomisp driver supports both
> > > ISP2400/ISP2401 in a single version.  
> > 
> > Actually, supporting both on a single version is part of Alan's work.
> > 
> > It seems he used the generation tool to produce a version for 2400, and
> > then re-used it to generate for 2401. It then used some scripting tool
> > to convert the differences on #ifdef ISP2401. See:
> > 
> > 	a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
> > 
> > There are things there like:
> > 
> > 	+#ifdef ISP2401
> > 	+
> > 	+#endif
> > 
> > I did a large cleanup work to get rid of those ifdefs, replacing them
> > by runtime logic.
> > 
> > The end goal is to have a single compile-time driver that works for
> > both 2400 and 2401.
> > 
> > This is not possible yet, as there are some registers that are mapped
> > on different addresses, depending on the hardware version, and making
> > it generic requires a lot of work and tests. So, for now, we need to 
> > have a compile-time option to select between both.
> >   
> > > Indeed, the upstreamed atomisp uses irci_stable_candrpv_0415_20150521_0458
> > > for ISP2400 and IIUC it was working on Bay Trail. On the other hand,
> > > intel-aero is a kernel for Cherry Trail and uses the same version
> > > irci_stable_candrpv_0415_20150521_0458.
> > > 
> > > So, both ISP version ISP2400/ISP2401 can be supported by a single
> > > driver version.  
> > 
> > I See. OK!
> >   
> > > > > We should eventually remove (not unify) such tests caused by just a
> > > > > driver version difference and use just one version of driver. So, for
> > > > > now, let's avoid further unification.
> > > > > 
> > > > > [1] The function ia_css_mipi_is_source_port_valid() and its usage is
> > > > >     added on updating css version to irci_master_20150701_0213
> > > > >     https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
> > > > >     ("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")    
> > > > 
> > > > What happens is that there is a 2401 and a 2401 "legacy". It sounds
> > > > that this due to some different software stacks that are reflected both
> > > > at the firmware and at the driver.    
> > > 
> > > Yeah, I'm not sure what the "legacy" is. It might be a reference of
> > > `ISP2401_NEW_INPUT_SYSTEM` (css_2401_csi2p_system) and
> > > non-`ISP2401_NEW_INPUT_SYSTEM` (css_2401_system).
> > >   
> > > > -
> > > > 
> > > > On other words, this patch requires some rework, as otherwise it will break
> > > > support for Baytrail.    
> > > 
> > > You mean "this patch"? then, I intended this patch is rather a fix for
> > > ISP2400 case! The err variable for ISP2400 case is always true because
> > > it is not used before the error check:
> > > 
> > >         int
> > >         allocate_mipi_frames(struct ia_css_pipe *pipe,
> > >         		     struct ia_css_stream_info *info)
> > >         {
> > >         	int err = -EINVAL;
> > > [...]
> > >         	if (!IS_ISP2401)
> > >         		port = (unsigned int)pipe->stream->config.source.port.port;
> > >         	else
> > >         		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > >         
> > >         	assert(port < N_CSI_PORTS);
> > >         
> > >         	if (port >= N_CSI_PORTS || err) {
> > >         		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> > >         				    "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
> > >         				    pipe, port);
> > >         		return -EINVAL;
> > >         	}
> > > 
> > > The first usage of err variable is ia_css_mipi_is_source_port_valid()
> > > for IS_ISP2401 case, but it's not used for ISP2400 case. This causes
> > > the evaluation `port >= N_CSI_PORTS || err` always true for ISP2400 case,
> > > meaning it will be always treated as a error.  
> > 
> > Ok. Had you test the driver with Baytrail?  
> 
> Unfortunately, no. I don't have a Baytrail device to test (yet?). I
> noticed this issue anyway when I tried removing `#ifdef ISP2401`.
> 
> This is not directly related to this series, but how we should reduce
> the ifdef usage in the future? Here are my two ideas:
> 
>   1. (my initial idea) remove `#ifdef ISP2401` part and make ISP2401
>      part completely irci_stable_candrpv_0415_20150521_0458
> 
> this way does not require (relatively) much human work I think.
> 
> But as Mauro says, the `#ifdef ISP2401` part (irci_ecr-master_20150911_0724)
> is basically an improved version.

No. What I said is that the if (ISP2401) and the remaining ifdefs are because
of BYT x CHT. The worse part of them are related to those files
(See Makefile):

obj-byt = \
	pci/css_2400_system/hive/ia_css_isp_configs.o \
	pci/css_2400_system/hive/ia_css_isp_params.o \
	pci/css_2400_system/hive/ia_css_isp_states.o \

obj-cht = \
	pci/css_2401_system/hive/ia_css_isp_configs.o \
	pci/css_2401_system/hive/ia_css_isp_params.o \
	pci/css_2401_system/hive/ia_css_isp_states.o \
	pci/css_2401_system/host/csi_rx.o \
	pci/css_2401_system/host/ibuf_ctrl.o \
	pci/css_2401_system/host/isys_dma.o \
	pci/css_2401_system/host/isys_irq.o \
	pci/css_2401_system/host/isys_stream2mmio.o

Those define regmaps for 2400 and 2401. I was able to remove a lot
of things from the old css_2400/css_2401 directories, but the ones
there at pci/*/css*/ia_css_isp_*.c are a lot more complex, and would
require some mapping functions to allow the same driver to work with
both BYT and CHT.

The better would be to test the driver first at BYT, fix issues (if any) and 
then write the mapping code.

> So, we may also:
> 
>   2. continue unifying `#ifdef ISP2401` and `#ifndef ISP2401` parts
> 
> but this way needs more human work I think though. And if we go this
> way, I also need to rewrite this patch as mentioned in the commit
> message.
> 
> > >   
> > > > Also, patch 13 should be dropped, as the firmware versions for 2400 are
> > > > different    
> > > 
> > > The firmware version for 2400 on the upstreamed atomisp is
> > > irci_stable_candrpv_0415_20150521_0458 :-)
> > > 
> > >         static const char *isp2400_release_version = STR(irci_stable_candrpv_0415_20150521_0458);
> > >         static const char *isp2401_release_version = STR(irci_ecr - master_20150911_0724);
> > > 
> > > The intention of that patch is rather, it clarifies ISP2401 is now using
> > > the same driver (css) version as ISP2400.  
> > 
> > Ok.
> >   
> > > > - and maybe patches 8 to 12 may need more work in order to not
> > > > touch 2400.    
> > > 
> > > Those patches do not break ISP2400, because what they do for ISP2400
> > > is that, they remove members from `struct`s which were initially inside
> > > of `ifdef ISP2401`. And because these removed members were initially
> > > inside of the ifdefs, the usage was also inside the ifdefs.  
> > 
> > Did you test on Baytrail (ISP2400), and with the compile-time option
> > enabled/disabled?  
> 
> Sorry, I should have clarified on the cover later. For ISP2400, I did
> compile test only (CONFIG_VIDEO_ATOMISP_ISP2401 enabled/disabled).

Maybe Hans could help us on that. I guess he has an Asus T100 device, 
which is BYT based.

Hans, if you're willing to do the tests, you could either use nvt
or v4l2grab to test it.

It seems that BYT has an additional issue, though: the ISP seems to
require 12 non-visible lines/columns (in addition to 16 ones required
by CHT?) for it to work.

So, you may need to tweak the resolution a bit, as otherwise the
driver will return an -EINVAL.

See:

	https://git.linuxtv.org/media_stage.git/commit/?id=dcbc4f570495dbd490975979471687cbe2246f99

For the workaround I had to add for CHT to properly report the
visible resolution.

Regards,
Mauro

Regards,
Mauro
Hans de Goede Nov. 1, 2021, 7:06 p.m. UTC | #6 
Hi,

On 11/1/21 15:10, Mauro Carvalho Chehab wrote:

<snip>

>>> Did you test on Baytrail (ISP2400), and with the compile-time option
>>> enabled/disabled?  
>>
>> Sorry, I should have clarified on the cover later. For ISP2400, I did
>> compile test only (CONFIG_VIDEO_ATOMISP_ISP2401 enabled/disabled).
> 
> Maybe Hans could help us on that. I guess he has an Asus T100 device, 
> which is BYT based.
> 
> Hans, if you're willing to do the tests, you could either use nvt
> or v4l2grab to test it.
> 
> It seems that BYT has an additional issue, though: the ISP seems to
> require 12 non-visible lines/columns (in addition to 16 ones required
> by CHT?) for it to work.
> 
> So, you may need to tweak the resolution a bit, as otherwise the
> driver will return an -EINVAL.
> 
> See:
> 
> 	https://git.linuxtv.org/media_stage.git/commit/?id=dcbc4f570495dbd490975979471687cbe2246f99
> 
> For the workaround I had to add for CHT to properly report the
> visible resolution.

Testing BYT support definitely is on my radar. Note that BYT
also has the additional issue that the atomisp2 on BYT can be
enumerated as either a PCI dev (which may work) or an ACPI/platform
dev which is unsupported in the original atomisp2 code-drop and
seems non trivial (because of pci config space writes) to get to
work.

On the T100TA the device is actually enumerated as an ACPI/platform
device and the BIOS option to change this is hidden. In the mean
time I've gained quite a lot of experience with changing hidden
BIOS options though, so I can easily put it in PCI mode for
testing. But eventually we also need to tackle ACPI enumeration
support...

Anyways I've let me self get distracted (hobby time wise) by
looking into PMIC/charger/fuel-gauge issues on the Xiaomi Mi Pad 2.
I've made a list of 3 (small-ish) loose ends which I want to
tie up there and then I plan to start looking into atomisp2
support in my hobby time. ATM my plan is:

   -Test on T101HA to reproduce Mauro's work
   -Try to get things to work on the T116
   -Patch to not load atomisp_foo sensor drivers on !BYT && !CHT
   
And I've just added:

   -Try out BYT support 

As 4th item. Eventually I want to look into writing a proper
regulator driver for the PMICs and then try to make the atomisp2
code work with the non "atomisp_xxx" versions of the sensor drivers.

Regards,

Hans
Andy Shevchenko Nov. 1, 2021, 7:27 p.m. UTC | #7 
On Mon, Nov 1, 2021 at 9:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 11/1/21 15:10, Mauro Carvalho Chehab wrote:

...

> Testing BYT support definitely is on my radar. Note that BYT
> also has the additional issue that the atomisp2 on BYT can be
> enumerated as either a PCI dev (which may work) or an ACPI/platform
> dev which is unsupported in the original atomisp2 code-drop and
> seems non trivial (because of pci config space writes) to get to
> work.
>
> On the T100TA the device is actually enumerated as an ACPI/platform
> device and the BIOS option to change this is hidden. In the mean
> time I've gained quite a lot of experience with changing hidden
> BIOS options though, so I can easily put it in PCI mode for
> testing. But eventually we also need to tackle ACPI enumeration
> support...

Not sure if you saw my very far from being even tested patch...
Share it here just in case. I believe you will have a better idea on
how to implement it.
Hans de Goede Nov. 1, 2021, 7:52 p.m. UTC | #8 
Hi,

On 11/1/21 20:27, Andy Shevchenko wrote:
> On Mon, Nov 1, 2021 at 9:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 11/1/21 15:10, Mauro Carvalho Chehab wrote:
> 
> ...
> 
>> Testing BYT support definitely is on my radar. Note that BYT
>> also has the additional issue that the atomisp2 on BYT can be
>> enumerated as either a PCI dev (which may work) or an ACPI/platform
>> dev which is unsupported in the original atomisp2 code-drop and
>> seems non trivial (because of pci config space writes) to get to
>> work.
>>
>> On the T100TA the device is actually enumerated as an ACPI/platform
>> device and the BIOS option to change this is hidden. In the mean
>> time I've gained quite a lot of experience with changing hidden
>> BIOS options though, so I can easily put it in PCI mode for
>> testing. But eventually we also need to tackle ACPI enumeration
>> support...
> 
> Not sure if you saw my very far from being even tested patch...
> Share it here just in case. I believe you will have a better idea on
> how to implement it.

I don't remember seeing that, it is great to at least have
a starting point when I get around to looking into this, thank you!

Regards,

Hans
Mauro Carvalho Chehab Nov. 1, 2021, 8:03 p.m. UTC | #9 
Em Mon, 1 Nov 2021 20:06:52 +0100
Hans de Goede <hdegoede@redhat.com> escreveu:

> Hi,
> 
> On 11/1/21 15:10, Mauro Carvalho Chehab wrote:
> 
> <snip>
> 
> >>> Did you test on Baytrail (ISP2400), and with the compile-time option
> >>> enabled/disabled?    
> >>
> >> Sorry, I should have clarified on the cover later. For ISP2400, I did
> >> compile test only (CONFIG_VIDEO_ATOMISP_ISP2401 enabled/disabled).  
> > 
> > Maybe Hans could help us on that. I guess he has an Asus T100 device, 
> > which is BYT based.
> > 
> > Hans, if you're willing to do the tests, you could either use nvt
> > or v4l2grab to test it.
> > 
> > It seems that BYT has an additional issue, though: the ISP seems to
> > require 12 non-visible lines/columns (in addition to 16 ones required
> > by CHT?) for it to work.
> > 
> > So, you may need to tweak the resolution a bit, as otherwise the
> > driver will return an -EINVAL.
> > 
> > See:
> > 
> > 	https://git.linuxtv.org/media_stage.git/commit/?id=dcbc4f570495dbd490975979471687cbe2246f99
> > 
> > For the workaround I had to add for CHT to properly report the
> > visible resolution.  
> 
> Testing BYT support definitely is on my radar. Note that BYT
> also has the additional issue that the atomisp2 on BYT can be
> enumerated as either a PCI dev (which may work) or an ACPI/platform
> dev which is unsupported in the original atomisp2 code-drop and
> seems non trivial (because of pci config space writes) to get to
> work.
> 
> On the T100TA the device is actually enumerated as an ACPI/platform
> device and the BIOS option to change this is hidden. In the mean
> time I've gained quite a lot of experience with changing hidden
> BIOS options though, so I can easily put it in PCI mode for
> testing. But eventually we also need to tackle ACPI enumeration
> support...
> 
> Anyways I've let me self get distracted (hobby time wise) by
> looking into PMIC/charger/fuel-gauge issues on the Xiaomi Mi Pad 2.
> I've made a list of 3 (small-ish) loose ends which I want to
> tie up there and then I plan to start looking into atomisp2
> support in my hobby time. ATM my plan is:
> 
>    -Test on T101HA to reproduce Mauro's work

Yeah, it would be great to have a second test on it. I suspect that it
should work just fine with USERPTR (with v4l2grab or nvt).

>    -Try to get things to work on the T116

Didn't test it here either, but won't be able to do in the next
couple of weeks.

>    -Patch to not load atomisp_foo sensor drivers on !BYT && !CHT

Not sure if it is worth doing it, as there are a lot more to be
done before being able to use a generic sensor driver.

> And I've just added:
> 
>    -Try out BYT support 

Thanks!

> 
> As 4th item. Eventually I want to look into writing a proper
> regulator driver for the PMICs

Yeah, a proper regulator driver would be a lot better than the
PMIC ones.

> and then try to make the atomisp2
> code work with the non "atomisp_xxx" versions of the sensor drivers.

With a regulator driver, part of the problem will be solved. However, 
as the ISP driver "eats" 16 lines and 16 columns. It means that the sensor 
needs to be adjusted for it to provide those extra data. So, the atomisp 
sensor resolutions are (X + 16) x (Y + 16), e. g. in the case of
ov2680, it is set to 1616x1216, while the upstream one is 1600x1200.

Not sure if the selection API currently allows changing that to
satisfy atomisp, or if something else would be needed.

Regards,
Mauro
Hans de Goede Nov. 1, 2021, 9:06 p.m. UTC | #10 
Hi,

On 11/1/21 21:03, Mauro Carvalho Chehab wrote:
> Em Mon, 1 Nov 2021 20:06:52 +0100
> Hans de Goede <hdegoede@redhat.com> escreveu:

<snip>

>>    -Patch to not load atomisp_foo sensor drivers on !BYT && !CHT
> 
> Not sure if it is worth doing it, as there are a lot more to be
> done before being able to use a generic sensor driver.

As you may know, I'm also working on IPU3 support for $dayjob atm
actually :)

So the drivers for e.g. the ov5693 sensor conflict, by adding
a small (one line) check to atomisp_ov5693.c to not register
the driver at all when not on BYT/CHT we can avoid the conflict
on most devices for now. And when actually on BYT/CHT the user
will need to blacklist the non atomisp sensor-modules which, well
sucks, but atomisp is in staging for a reason ...

So the idea here is that with some small added ugliness to the
atomisp_foo.c sensor drivers we can make the 2 drivers co-exist
a bit more, allowing e.g. generic distro kernels to (maybe) enable
the atomisp2 stuff without regressing the IPU3 support.

###

Since we are discussing this now anyways, the atomisp_foo.c
patches would look like this:

#include <linux/platform_data/x86/soc.h>

        if (!soc_intel_is_byt() && !soc_intel_is_cht())
                return -ENODEV;

In the probe() function and change driver.name from
e.g. "ov5693" to "atom_ov5693".

Before I spend time on writing patches for this, would patches doing
this for conflicting drivers be acceptable ?

Regards,

Hans
Mauro Carvalho Chehab Nov. 1, 2021, 9:33 p.m. UTC | #11 
Em Mon, 1 Nov 2021 22:06:12 +0100
Hans de Goede <hdegoede@redhat.com> escreveu:

> Hi,
> 
> On 11/1/21 21:03, Mauro Carvalho Chehab wrote:
> > Em Mon, 1 Nov 2021 20:06:52 +0100
> > Hans de Goede <hdegoede@redhat.com> escreveu:  
> 
> <snip>
> 
> >>    -Patch to not load atomisp_foo sensor drivers on !BYT && !CHT  
> > 
> > Not sure if it is worth doing it, as there are a lot more to be
> > done before being able to use a generic sensor driver.  
> 
> As you may know, I'm also working on IPU3 support for $dayjob atm
> actually :)

Didn't know that... Btw, I have one Dell device with IPU3. It would 
be great to have it working there ;-)

> So the drivers for e.g. the ov5693 sensor conflict, by adding
> a small (one line) check to atomisp_ov5693.c to not register
> the driver at all when not on BYT/CHT we can avoid the conflict
> on most devices for now. And when actually on BYT/CHT the user
> will need to blacklist the non atomisp sensor-modules which, well
> sucks, but atomisp is in staging for a reason ...
> 
> So the idea here is that with some small added ugliness to the
> atomisp_foo.c sensor drivers we can make the 2 drivers co-exist
> a bit more, allowing e.g. generic distro kernels to (maybe) enable
> the atomisp2 stuff without regressing the IPU3 support.
> 
> ###
> 
> Since we are discussing this now anyways, the atomisp_foo.c
> patches would look like this:
> 
> #include <linux/platform_data/x86/soc.h>
> 
>         if (!soc_intel_is_byt() && !soc_intel_is_cht())
>                 return -ENODEV;
> 
> In the probe() function and change driver.name from
> e.g. "ov5693" to "atom_ov5693".
> 
> Before I spend time on writing patches for this, would patches doing
> this for conflicting drivers be acceptable ?

Surely. Yeah, it makes sense to me. Feel free to submit such
patches.

Regards,
Mauro
Tsuchiya Yuto Nov. 11, 2021, 2:34 p.m. UTC | #12 
Sorry for a little late reply. This is hard to explain...

On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:
> Em Mon, 01 Nov 2021 22:38:55 +0900
> Tsuchiya Yuto <kitakar@gmail.com> escreveu:

[...]
> 

> > This is not directly related to this series, but how we should reduce
> > the ifdef usage in the future? Here are my two ideas:
> > 
> >   1. (my initial idea) remove `#ifdef ISP2401` part and make ISP2401
> >      part completely irci_stable_candrpv_0415_20150521_0458
> > 
> > this way does not require (relatively) much human work I think.
> > 
> > But as Mauro says, the `#ifdef ISP2401` part (irci_ecr-master_20150911_0724)
> > is basically an improved version.
> 
> No. What I said is that the if (ISP2401) and the remaining ifdefs are because
> of BYT x CHT.

I need to elaborate on this. Indeed some of them are really because of
BYT x CHT, but others are stuff from irci_ecr-master_20150911_0724.

What I meant when I mentioned "remove `#ifdef ISP2401` part" is that,
removing things which was _initially_ inside the `#ifdef ISP2401` on the
initial commit of atomisp.

Also I believe we can remove more `if (IS_ISP2401)` and `/* ISP2401 */`
things as well as the some remaining `#ifdef ISP2401` things.

I added about this below and hope it clarifies things...

> The worse part of them are related to those files
> (See Makefile):
> 
> obj-byt = \
> 	pci/css_2400_system/hive/ia_css_isp_configs.o \
> 	pci/css_2400_system/hive/ia_css_isp_params.o \
> 	pci/css_2400_system/hive/ia_css_isp_states.o \
> 
> obj-cht = \
> 	pci/css_2401_system/hive/ia_css_isp_configs.o \
> 	pci/css_2401_system/hive/ia_css_isp_params.o \
> 	pci/css_2401_system/hive/ia_css_isp_states.o \
> 	pci/css_2401_system/host/csi_rx.o \
> 	pci/css_2401_system/host/ibuf_ctrl.o \
> 	pci/css_2401_system/host/isys_dma.o \
> 	pci/css_2401_system/host/isys_irq.o \
> 	pci/css_2401_system/host/isys_stream2mmio.o
> 
> Those define regmaps for 2400 and 2401. I was able to remove a lot
> of things from the old css_2400/css_2401 directories, but the ones
> there at pci/*/css*/ia_css_isp_*.c are a lot more complex, and would
> require some mapping functions to allow the same driver to work with
> both BYT and CHT.
> 
> The better would be to test the driver first at BYT, fix issues (if any) and 
> then write the mapping code.
> 
> > So, we may also:
> > 
> >   2. continue unifying `#ifdef ISP2401` and `#ifndef ISP2401` parts
> > 
> > but this way needs more human work I think though. And if we go this
> > way, I also need to rewrite this patch as mentioned in the commit
> > message.

What the idea #1 want to say is, let's remove things _initially_ within
`ifdef ISP2401` (so, except things which were added inside it later
upstream) including formerly within `ifdef ISP2401` things, i.e.,
`if (IS_ISP2401)` and things commented with `/* ISP2401 */`.

However, I don't say we can remove all the ifdefs like things formerly
within USE_INPUT_SYSTEM_VERSION_2, USE_INPUT_SYSTEM_VERSION_2401, etc.,
which later removed/integrated into `ifdef ISP2401` on some commits [1].
We may temporarily revert those commits when we want to distinguish
between what were formerly within there and what were not.

Such ifdefs were added by them as a real hardware difference. Thus, I
agree that we still need the CONFIG_VIDEO_ATOMISP_ISP2401 stuff to support
both ISP2400/ISP2401 at the same time.

This is what I meant "reduce the ifdef usage" in a previous mail. So,
I'm not talking about if dropping CONFIG_VIDEO_ATOMISP_ISP2401 is doable,
but talking about just how to reduce the code.

[1] 641c2292bf19 ("media: atomisp: get rid of version-dependent globals")
    bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")

Anyway, if you agree or not on what I'm saying, can I send this patch
without code changes in v2, i.e., looks OK for you regarding the code?
I'll remove the commit message about
irci_stable_candrpv_0415_20150521_0458 vs irci_ecr-master_20150911_0724
in v2 anyway, which needs to be discussed further later.



The following notes are about what I have done until now for removing
such tests. (More elaborated version than cover letter). You don't have
to see them, but I hope it might clarify things...

  ## `ifdef ISP2401` added in the initial commit of atomisp

The `ifdef ISP2401` was the result of merging two different version of
driver, added on the initial commit of upstreamed atomisp. And for the
`ifdef ISP2401`, I confirmed I can remove (almost [1]) all of them against
the initial commit of atomisp [2][3]

[1] here are the three exceptions:
    ("NOTE: ifdef ISP2400/ISP2401 usage in aero-atomisp")
    https://github.com/kitakar5525/linux-kernel/commit/1a8488cdd31ad38a3805824700b29d1e5213d3f2

[2] ("atomisp: pci: css2400: remove ISP2401 ifdefs")
    https://github.com/kitakar5525/linux-kernel/commit/dd6723fc5b9fe040e33b227b509a7e004243edce
[3] ("atomisp: pci: remove ISP2401 ifdefs for main pci driver")
    https://github.com/kitakar5525/linux-kernel/commit/1734341f84a96945af7635f6fff061db910f746f

Here is the kernel tree if someone is interested:

        https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@a49d25364dfb_first

Especially, here is one of the part where this patch is touching
for example:

        --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
        +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
        @@ -416,26 +362,16 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, struct ia_css_stream_info *info)
[...]
        -#ifndef ISP2401
         	port = (unsigned int) pipe->stream->config.source.port.port;
         	assert(port < N_CSI_PORTS);
         	if (port >= N_CSI_PORTS) {
        -#else
        -	if (!ia_css_mipi_is_source_port_valid(pipe, &port)) {
        -#endif
         		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
         			"allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
         			pipe, port);

By removing (almost) all of the `#ifdef ISP2401` things, (although we
still can't remove like former USE_INPUT_SYSTEM_VERSION_2,
USE_INPUT_SYSTEM_VERSION_2401) we can reduce the number of ifdefs.



  ## `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */` in the recent
      atomisp

That is for the initial commit of atomisp. For the recent version of
atomisp, we can still remove `ifdef ISP2401` things (again, except things
which were added inside it later upstream) as well as the former
`ifdef ISP2401` things, i.e., `if (IS_ISP2401)` [1] and things commented
with [2] `/* ISP2401 */`.

[1] ("atomisp: pci: remove IS_ISP2401 test")
    https://github.com/kitakar5525/linux-kernel/commit/397e543e493dfd60d91e2b5cc164da342b26906c
[2] ("atomisp: pci: remove `/* ISP2401 */` comments and its contents")
    https://github.com/kitakar5525/linux-kernel/commit/b3928e3c1a709853971715ce35459b9b79e708f2
    These commits were made against
    bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
    where I randomely picked.

Here is the kernel tree if someone is interested:

    https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@bd674b5a413c_before_get_rid_ver_globals

I confirmed capture is still working here on surface3 (ISP2401). Compile
tested for ISP2400. As you can see, there are some WIP and FIXME commits
on top of removing such tests though. (The others are backports).

Especially, here is one of the part where this patch is touching
for example:

        --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
        +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
        @@ -553,10 +548,7 @@ free_mipi_frames(struct ia_css_pipe *pipe) {
         			return err;
         		}
        
        -		if (!IS_ISP2401)
        -			port = (unsigned int)pipe->stream->config.source.port.port;
        -		else
        -			err = ia_css_mipi_is_source_port_valid(pipe, &port);
        +		port = (unsigned int)pipe->stream->config.source.port.port;
        
         		assert(port < N_CSI_PORTS);
        

So, we can also remove a lot of `if (IS_ISP2401)` and `/* ISP2401 */`
things as well as the remaining `ifdef ISP2401`.



  ## WIP: removing `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */`
     against the latest atomisp

And here is the branch where I'm working on removing such tests against
the latest atomisp:

        https://github.com/kitakar5525/linux-kernel/commits/mainline+upst_atomisp+remove_unneeded_tests

It'd be the best if I can show you working one, but it currently has
seemingly include issues on ISP2400 vs ISP2401 (as well as many WIP
commits I added):

	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c: In function ‘ia_css_isys_init’:
	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:29:9: error: unknown type name ‘backend_channel_cfg_t’
	   29 |         backend_channel_cfg_t backend_ch0;
	      |         ^~~~~~~~~~~~~~~~~~~~~
	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:30:9: error: unknown type name ‘backend_channel_cfg_t’
	   30 |         backend_channel_cfg_t backend_ch1;
	      |         ^~~~~~~~~~~~~~~~~~~~~
	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:31:9: error: unknown type name ‘target_cfg2400_t’
	   31 |         target_cfg2400_t targetB;
	      |         ^~~~~~~~~~~~~~~~
	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:32:9: error: unknown type name ‘target_cfg2400_t’
	   32 |         target_cfg2400_t targetC;
	      |         ^~~~~~~~~~~~~~~~
	[...]

The full output of the make error is here:

        ("NOTE: issue: some undeclared errors")
        https://github.com/kitakar5525/linux-kernel/commit/a932d16681f941161385659b9d0316a3a4975e86



Regards,
Tsuchiya Yuto
> > >
Andy Shevchenko Nov. 11, 2021, 4:09 p.m. UTC | #13 
On Thu, Nov 11, 2021 at 11:34:23PM +0900, Tsuchiya Yuto wrote:
> On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:
> > Em Mon, 01 Nov 2021 22:38:55 +0900
> > Tsuchiya Yuto <kitakar@gmail.com> escreveu:

...

> The full output of the make error is here:
> 
>         ("NOTE: issue: some undeclared errors")
>         https://github.com/kitakar5525/linux-kernel/commit/a932d16681f941161385659b9d0316a3a4975e86

I just realize that we may do at some point

cflags-y += -Werror

to avoid changes that breaks build (with warnings). And also I would suggest
to run build with `make W=1 C=1 CF=-D__CHECK_ENDIAN__ ...`
Mauro Carvalho Chehab Nov. 11, 2021, 6:38 p.m. UTC | #14 
Em Thu, 11 Nov 2021 23:34:23 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:

> Sorry for a little late reply. This is hard to explain...
> 
> On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:
> > Em Mon, 01 Nov 2021 22:38:55 +0900
> > Tsuchiya Yuto <kitakar@gmail.com> escreveu:  
> 
> [...]
> >   
> 
> > > This is not directly related to this series, but how we should reduce
> > > the ifdef usage in the future? Here are my two ideas:
> > > 
> > >   1. (my initial idea) remove `#ifdef ISP2401` part and make ISP2401
> > >      part completely irci_stable_candrpv_0415_20150521_0458
> > > 
> > > this way does not require (relatively) much human work I think.
> > > 
> > > But as Mauro says, the `#ifdef ISP2401` part (irci_ecr-master_20150911_0724)
> > > is basically an improved version.  
> > 
> > No. What I said is that the if (ISP2401) and the remaining ifdefs are because
> > of BYT x CHT.  
> 
> I need to elaborate on this. Indeed some of them are really because of
> BYT x CHT, but others are stuff from irci_ecr-master_20150911_0724.
> 
> What I meant when I mentioned "remove `#ifdef ISP2401` part" is that,
> removing things which was _initially_ inside the `#ifdef ISP2401` on the
> initial commit of atomisp.
> 
> Also I believe we can remove more `if (IS_ISP2401)` and `/* ISP2401 */`
> things as well as the some remaining `#ifdef ISP2401` things.
> 
> I added about this below and hope it clarifies things...

It is clearer now. Yeah, we can touch on whatever is inside the
ISP2401 ifs, as we can now test them. Touching things for ISP2400
is harder, as we depend on a test platform.

> > The worse part of them are related to those files
> > (See Makefile):
> > 
> > obj-byt = \
> > 	pci/css_2400_system/hive/ia_css_isp_configs.o \
> > 	pci/css_2400_system/hive/ia_css_isp_params.o \
> > 	pci/css_2400_system/hive/ia_css_isp_states.o \
> > 
> > obj-cht = \
> > 	pci/css_2401_system/hive/ia_css_isp_configs.o \
> > 	pci/css_2401_system/hive/ia_css_isp_params.o \
> > 	pci/css_2401_system/hive/ia_css_isp_states.o \
> > 	pci/css_2401_system/host/csi_rx.o \
> > 	pci/css_2401_system/host/ibuf_ctrl.o \
> > 	pci/css_2401_system/host/isys_dma.o \
> > 	pci/css_2401_system/host/isys_irq.o \
> > 	pci/css_2401_system/host/isys_stream2mmio.o
> > 
> > Those define regmaps for 2400 and 2401. I was able to remove a lot
> > of things from the old css_2400/css_2401 directories, but the ones
> > there at pci/*/css*/ia_css_isp_*.c are a lot more complex, and would
> > require some mapping functions to allow the same driver to work with
> > both BYT and CHT.
> > 
> > The better would be to test the driver first at BYT, fix issues (if any) and 
> > then write the mapping code.
> >   
> > > So, we may also:
> > > 
> > >   2. continue unifying `#ifdef ISP2401` and `#ifndef ISP2401` parts
> > > 
> > > but this way needs more human work I think though. And if we go this
> > > way, I also need to rewrite this patch as mentioned in the commit
> > > message.  
> 
> What the idea #1 want to say is, let's remove things _initially_ within
> `ifdef ISP2401` (so, except things which were added inside it later
> upstream) including formerly within `ifdef ISP2401` things, i.e.,
> `if (IS_ISP2401)` and things commented with `/* ISP2401 */`.
> 
> However, I don't say we can remove all the ifdefs like things formerly
> within USE_INPUT_SYSTEM_VERSION_2, USE_INPUT_SYSTEM_VERSION_2401, etc.,
> which later removed/integrated into `ifdef ISP2401` on some commits [1].
> We may temporarily revert those commits when we want to distinguish
> between what were formerly within there and what were not.
> 
> Such ifdefs were added by them as a real hardware difference. Thus, I
> agree that we still need the CONFIG_VIDEO_ATOMISP_ISP2401 stuff to support
> both ISP2400/ISP2401 at the same time.
> 
> This is what I meant "reduce the ifdef usage" in a previous mail. So,
> I'm not talking about if dropping CONFIG_VIDEO_ATOMISP_ISP2401 is doable,
> but talking about just how to reduce the code.
> 
> [1] 641c2292bf19 ("media: atomisp: get rid of version-dependent globals")
>     bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
> 
> Anyway, if you agree or not on what I'm saying, can I send this patch
> without code changes in v2, i.e., looks OK for you regarding the code?
> I'll remove the commit message about
> irci_stable_candrpv_0415_20150521_0458 vs irci_ecr-master_20150911_0724
> in v2 anyway, which needs to be discussed further later.

No need for a v2. The /17 patch series was merged already, plus some
patches from the /5 that made sense to apply.

Ok, there are some followup patches that could be added, but please
send those in separate.

> The following notes are about what I have done until now for removing
> such tests. (More elaborated version than cover letter). You don't have
> to see them, but I hope it might clarify things...
> 
>   ## `ifdef ISP2401` added in the initial commit of atomisp
> 
> The `ifdef ISP2401` was the result of merging two different version of
> driver, added on the initial commit of upstreamed atomisp. And for the
> `ifdef ISP2401`, I confirmed I can remove (almost [1]) all of them against
> the initial commit of atomisp [2][3]
> 
> [1] here are the three exceptions:
>     ("NOTE: ifdef ISP2400/ISP2401 usage in aero-atomisp")
>     https://github.com/kitakar5525/linux-kernel/commit/1a8488cdd31ad38a3805824700b29d1e5213d3f2
> 
> [2] ("atomisp: pci: css2400: remove ISP2401 ifdefs")
>     https://github.com/kitakar5525/linux-kernel/commit/dd6723fc5b9fe040e33b227b509a7e004243edce
> [3] ("atomisp: pci: remove ISP2401 ifdefs for main pci driver")
>     https://github.com/kitakar5525/linux-kernel/commit/1734341f84a96945af7635f6fff061db910f746f

Ok, if there are more if/ifdef ISP2401 that, if reverted will keep the
driver running with the firmware we're using, I'm all for it. Just send
the patches ;-)

> 
> Here is the kernel tree if someone is interested:
> 
>         https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@a49d25364dfb_first
> 
> Especially, here is one of the part where this patch is touching
> for example:
> 
>         --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
>         +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
>         @@ -416,26 +362,16 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, struct ia_css_stream_info *info)
> [...]
>         -#ifndef ISP2401
>          	port = (unsigned int) pipe->stream->config.source.port.port;
>          	assert(port < N_CSI_PORTS);
>          	if (port >= N_CSI_PORTS) {
>         -#else
>         -	if (!ia_css_mipi_is_source_port_valid(pipe, &port)) {
>         -#endif
>          		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
>          			"allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
>          			pipe, port);
> 
> By removing (almost) all of the `#ifdef ISP2401` things, (although we
> still can't remove like former USE_INPUT_SYSTEM_VERSION_2,
> USE_INPUT_SYSTEM_VERSION_2401) we can reduce the number of ifdefs.

Sounds good to me.

> 
> 
>   ## `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */` in the recent
>       atomisp
> 
> That is for the initial commit of atomisp. For the recent version of
> atomisp, we can still remove `ifdef ISP2401` things (again, except things
> which were added inside it later upstream) as well as the former
> `ifdef ISP2401` things, i.e., `if (IS_ISP2401)` [1] and things commented
> with [2] `/* ISP2401 */`.
> 
> [1] ("atomisp: pci: remove IS_ISP2401 test")
>     https://github.com/kitakar5525/linux-kernel/commit/397e543e493dfd60d91e2b5cc164da342b26906c
> [2] ("atomisp: pci: remove `/* ISP2401 */` comments and its contents")
>     https://github.com/kitakar5525/linux-kernel/commit/b3928e3c1a709853971715ce35459b9b79e708f2
>     These commits were made against
>     bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
>     where I randomely picked.
> 
> Here is the kernel tree if someone is interested:
> 
>     https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@bd674b5a413c_before_get_rid_ver_globals
> 
> I confirmed capture is still working here on surface3 (ISP2401). Compile
> tested for ISP2400. As you can see, there are some WIP and FIXME commits
> on top of removing such tests though. (The others are backports).
> 
> Especially, here is one of the part where this patch is touching
> for example:
> 
>         --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
>         +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
>         @@ -553,10 +548,7 @@ free_mipi_frames(struct ia_css_pipe *pipe) {
>          			return err;
>          		}
>         
>         -		if (!IS_ISP2401)
>         -			port = (unsigned int)pipe->stream->config.source.port.port;
>         -		else
>         -			err = ia_css_mipi_is_source_port_valid(pipe, &port);
>         +		port = (unsigned int)pipe->stream->config.source.port.port;
>         
>          		assert(port < N_CSI_PORTS);
>         
> 
> So, we can also remove a lot of `if (IS_ISP2401)` and `/* ISP2401 */`
> things as well as the remaining `ifdef ISP2401`.
> 
> 
> 
>   ## WIP: removing `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */`
>      against the latest atomisp
> 
> And here is the branch where I'm working on removing such tests against
> the latest atomisp:
> 
>         https://github.com/kitakar5525/linux-kernel/commits/mainline+upst_atomisp+remove_unneeded_tests
> 
> It'd be the best if I can show you working one, 

Well, send the ones that were already tested, and won't cause
regressions to v4l2grab and camorama (e. g. it shouldn't cause
generic V4L2 generic apps to break).

It would be nice to also not break nvt and other original apps for
this device, as it could be useful later, in order to be able to 
test the other pipelines, as currently only the preview one seems
to be working properly, at least with generic apps.

> but it currently has
> seemingly include issues on ISP2400 vs ISP2401 (as well as many WIP
> commits I added):
> 
> 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c: In function ‘ia_css_isys_init’:
> 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:29:9: error: unknown type name ‘backend_channel_cfg_t’
> 	   29 |         backend_channel_cfg_t backend_ch0;
> 	      |         ^~~~~~~~~~~~~~~~~~~~~
> 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:30:9: error: unknown type name ‘backend_channel_cfg_t’
> 	   30 |         backend_channel_cfg_t backend_ch1;
> 	      |         ^~~~~~~~~~~~~~~~~~~~~
> 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:31:9: error: unknown type name ‘target_cfg2400_t’
> 	   31 |         target_cfg2400_t targetB;
> 	      |         ^~~~~~~~~~~~~~~~
> 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:32:9: error: unknown type name ‘target_cfg2400_t’
> 	   32 |         target_cfg2400_t targetC;
> 	      |         ^~~~~~~~~~~~~~~~
> 	[...]
> 
> The full output of the make error is here:
> 
>         ("NOTE: issue: some undeclared errors")
>         https://github.com/kitakar5525/linux-kernel/commit/a932d16681f941161385659b9d0316a3a4975e86
> 
> 
> 
> Regards,
> Tsuchiya Yuto
> > > >
Mauro Carvalho Chehab Nov. 11, 2021, 7:37 p.m. UTC | #15 
Em Thu, 11 Nov 2021 18:09:49 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> escreveu:

> On Thu, Nov 11, 2021 at 11:34:23PM +0900, Tsuchiya Yuto wrote:
> > On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:  
> > > Em Mon, 01 Nov 2021 22:38:55 +0900
> > > Tsuchiya Yuto <kitakar@gmail.com> escreveu:  
> 
> ...
> 
> > The full output of the make error is here:
> > 
> >         ("NOTE: issue: some undeclared errors")
> >         https://github.com/kitakar5525/linux-kernel/commit/a932d16681f941161385659b9d0316a3a4975e86  
> 
> I just realize that we may do at some point
> 
> cflags-y += -Werror
>
> to avoid changes that breaks build (with warnings). 

No need. Upstream already added Werror. It is just a matter of
adding:

	CONFIG_WERROR=y

to .config.

> And also I would suggest
> to run build with `make W=1 C=1 CF=-D__CHECK_ENDIAN__ ...`

Yeah, that's good. On media, we also require no (unjustified) sparse
and smatch warnings.

Regards,
Mauro
Andy Shevchenko Nov. 11, 2021, 8:39 p.m. UTC | #16 
On Thu, Nov 11, 2021 at 9:41 PM Mauro Carvalho Chehab
<mchehab@kernel.org> wrote:
> Em Thu, 11 Nov 2021 18:09:49 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> escreveu:
> > On Thu, Nov 11, 2021 at 11:34:23PM +0900, Tsuchiya Yuto wrote:

...

> > I just realize that we may do at some point
> >
> > cflags-y += -Werror
> >
> > to avoid changes that breaks build (with warnings).
>
> No need. Upstream already added Werror. It is just a matter of
> adding:
>
>         CONFIG_WERROR=y
>
> to .config.

Unfortunately this is not gonna work with `make W=1`.
https://lore.kernel.org/lkml/CAHp75Vef8QW3Y0yA702KUqPDHNRLN0kCv06=cgPpgPbUeAb-dw@mail.gmail.com/T/#u

> > And also I would suggest
> > to run build with `make W=1 C=1 CF=-D__CHECK_ENDIAN__ ...`
>
> Yeah, that's good. On media, we also require no (unjustified) sparse
> and smatch warnings.
Mauro Carvalho Chehab Nov. 17, 2021, 10:24 p.m. UTC | #17 
Hi 
Em Thu, 11 Nov 2021 18:38:12 +0000
Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:

> > The `ifdef ISP2401` was the result of merging two different version of
> > driver, added on the initial commit of upstreamed atomisp. And for the
> > `ifdef ISP2401`, I confirmed I can remove (almost [1]) all of them against
> > the initial commit of atomisp [2][3]
> > 
> > [1] here are the three exceptions:
> >     ("NOTE: ifdef ISP2400/ISP2401 usage in aero-atomisp")
> >     https://github.com/kitakar5525/linux-kernel/commit/1a8488cdd31ad38a3805824700b29d1e5213d3f2
> > 
> > [2] ("atomisp: pci: css2400: remove ISP2401 ifdefs")
> >     https://github.com/kitakar5525/linux-kernel/commit/dd6723fc5b9fe040e33b227b509a7e004243edce
> > [3] ("atomisp: pci: remove ISP2401 ifdefs for main pci driver")
> >     https://github.com/kitakar5525/linux-kernel/commit/1734341f84a96945af7635f6fff061db910f746f  
> 
> Ok, if there are more if/ifdef ISP2401 that, if reverted will keep the
> driver running with the firmware we're using, I'm all for it. Just send
> the patches ;-)

I went ahead and solved several INPUT_SYSTEM related ifdefs on a way
that it is compatible with Intel Aero firmware for the sh_css* files.
Except if I made any mistake, the ifdefs that are related to the
input system were already addressed.

I didn't notice any changes when running camorama on the PREVIEW
node. 

Please test. Feel free to submit fixup patches if needed.

Regards,
Mauro
Tsuchiya Yuto Dec. 1, 2021, 11:30 a.m. UTC | #18 
I'm really sorry about this delay recently. I can't spare much time now...

On Wed, 2021-11-17 at 22:24 +0000, Mauro Carvalho Chehab wrote:
> Hi 
> Em Thu, 11 Nov 2021 18:38:12 +0000
> Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:
> 
> > > The `ifdef ISP2401` was the result of merging two different version of
> > > driver, added on the initial commit of upstreamed atomisp. And for the
> > > `ifdef ISP2401`, I confirmed I can remove (almost [1]) all of them against
> > > the initial commit of atomisp [2][3]
> > > 
> > > [1] here are the three exceptions:
> > >     ("NOTE: ifdef ISP2400/ISP2401 usage in aero-atomisp")
> > >     https://github.com/kitakar5525/linux-kernel/commit/1a8488cdd31ad38a3805824700b29d1e5213d3f2
> > > 
> > > [2] ("atomisp: pci: css2400: remove ISP2401 ifdefs")
> > >     https://github.com/kitakar5525/linux-kernel/commit/dd6723fc5b9fe040e33b227b509a7e004243edce
> > > [3] ("atomisp: pci: remove ISP2401 ifdefs for main pci driver")
> > >     https://github.com/kitakar5525/linux-kernel/commit/1734341f84a96945af7635f6fff061db910f746f  
> > 
> > Ok, if there are more if/ifdef ISP2401 that, if reverted will keep the
> > driver running with the firmware we're using, I'm all for it. Just send
> > the patches ;-)
> 
> I went ahead and solved several INPUT_SYSTEM related ifdefs on a way
> that it is compatible with Intel Aero firmware for the sh_css* files.
> Except if I made any mistake, the ifdefs that are related to the
> input system were already addressed.
> 
> I didn't notice any changes when running camorama on the PREVIEW
> node. 
> 
> Please test. Feel free to submit fixup patches if needed.

Thank you for your work. Just tried now and I also don't notice any
issues so far with the latest media_stage tree.

Regards,
Tsuchiya Yuto
Tsuchiya Yuto Dec. 1, 2021, 11:35 a.m. UTC | #19 
On Thu, 2021-11-11 at 18:38 +0000, Mauro Carvalho Chehab wrote:
> Em Thu, 11 Nov 2021 23:34:23 +0900
> Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> 
> > Sorry for a little late reply. This is hard to explain...
> > 
> > On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:
> > > Em Mon, 01 Nov 2021 22:38:55 +0900
> > > Tsuchiya Yuto <kitakar@gmail.com> escreveu:  
> > 
> > [...]
> > >   
> > 
> > > > This is not directly related to this series, but how we should reduce
> > > > the ifdef usage in the future? Here are my two ideas:
> > > > 
> > > >   1. (my initial idea) remove `#ifdef ISP2401` part and make ISP2401
> > > >      part completely irci_stable_candrpv_0415_20150521_0458
> > > > 
> > > > this way does not require (relatively) much human work I think.
> > > > 
> > > > But as Mauro says, the `#ifdef ISP2401` part (irci_ecr-master_20150911_0724)
> > > > is basically an improved version.  
> > > 
> > > No. What I said is that the if (ISP2401) and the remaining ifdefs are because
> > > of BYT x CHT.  
> > 
> > I need to elaborate on this. Indeed some of them are really because of
> > BYT x CHT, but others are stuff from irci_ecr-master_20150911_0724.
> > 
> > What I meant when I mentioned "remove `#ifdef ISP2401` part" is that,
> > removing things which was _initially_ inside the `#ifdef ISP2401` on the
> > initial commit of atomisp.
> > 
> > Also I believe we can remove more `if (IS_ISP2401)` and `/* ISP2401 */`
> > things as well as the some remaining `#ifdef ISP2401` things.
> > 
> > I added about this below and hope it clarifies things...
> 
> It is clearer now. Yeah, we can touch on whatever is inside the
> ISP2401 ifs, as we can now test them. Touching things for ISP2400
> is harder, as we depend on a test platform.
> 
> > > The worse part of them are related to those files
> > > (See Makefile):
> > > 
> > > obj-byt = \
> > > 	pci/css_2400_system/hive/ia_css_isp_configs.o \
> > > 	pci/css_2400_system/hive/ia_css_isp_params.o \
> > > 	pci/css_2400_system/hive/ia_css_isp_states.o \
> > > 
> > > obj-cht = \
> > > 	pci/css_2401_system/hive/ia_css_isp_configs.o \
> > > 	pci/css_2401_system/hive/ia_css_isp_params.o \
> > > 	pci/css_2401_system/hive/ia_css_isp_states.o \
> > > 	pci/css_2401_system/host/csi_rx.o \
> > > 	pci/css_2401_system/host/ibuf_ctrl.o \
> > > 	pci/css_2401_system/host/isys_dma.o \
> > > 	pci/css_2401_system/host/isys_irq.o \
> > > 	pci/css_2401_system/host/isys_stream2mmio.o
> > > 
> > > Those define regmaps for 2400 and 2401. I was able to remove a lot
> > > of things from the old css_2400/css_2401 directories, but the ones
> > > there at pci/*/css*/ia_css_isp_*.c are a lot more complex, and would
> > > require some mapping functions to allow the same driver to work with
> > > both BYT and CHT.
> > > 
> > > The better would be to test the driver first at BYT, fix issues (if any) and 
> > > then write the mapping code.
> > >   
> > > > So, we may also:
> > > > 
> > > >   2. continue unifying `#ifdef ISP2401` and `#ifndef ISP2401` parts
> > > > 
> > > > but this way needs more human work I think though. And if we go this
> > > > way, I also need to rewrite this patch as mentioned in the commit
> > > > message.  
> > 
> > What the idea #1 want to say is, let's remove things _initially_ within
> > `ifdef ISP2401` (so, except things which were added inside it later
> > upstream) including formerly within `ifdef ISP2401` things, i.e.,
> > `if (IS_ISP2401)` and things commented with `/* ISP2401 */`.
> > 
> > However, I don't say we can remove all the ifdefs like things formerly
> > within USE_INPUT_SYSTEM_VERSION_2, USE_INPUT_SYSTEM_VERSION_2401, etc.,
> > which later removed/integrated into `ifdef ISP2401` on some commits [1].
> > We may temporarily revert those commits when we want to distinguish
> > between what were formerly within there and what were not.
> > 
> > Such ifdefs were added by them as a real hardware difference. Thus, I
> > agree that we still need the CONFIG_VIDEO_ATOMISP_ISP2401 stuff to support
> > both ISP2400/ISP2401 at the same time.
> > 
> > This is what I meant "reduce the ifdef usage" in a previous mail. So,
> > I'm not talking about if dropping CONFIG_VIDEO_ATOMISP_ISP2401 is doable,
> > but talking about just how to reduce the code.
> > 
> > [1] 641c2292bf19 ("media: atomisp: get rid of version-dependent globals")
> >     bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
> > 
> > Anyway, if you agree or not on what I'm saying, can I send this patch
> > without code changes in v2, i.e., looks OK for you regarding the code?
> > I'll remove the commit message about
> > irci_stable_candrpv_0415_20150521_0458 vs irci_ecr-master_20150911_0724
> > in v2 anyway, which needs to be discussed further later.
> 
> No need for a v2. The /17 patch series was merged already, plus some
> patches from the /5 that made sense to apply.
> 
> Ok, there are some followup patches that could be added, but please
> send those in separate.

OK, thanks. I'll prepare the followup patches as soon as I can.

> > The following notes are about what I have done until now for removing
> > such tests. (More elaborated version than cover letter). You don't have
> > to see them, but I hope it might clarify things...
> > 
> >   ## `ifdef ISP2401` added in the initial commit of atomisp
> > 
> > The `ifdef ISP2401` was the result of merging two different version of
> > driver, added on the initial commit of upstreamed atomisp. And for the
> > `ifdef ISP2401`, I confirmed I can remove (almost [1]) all of them against
> > the initial commit of atomisp [2][3]
> > 
> > [1] here are the three exceptions:
> >     ("NOTE: ifdef ISP2400/ISP2401 usage in aero-atomisp")
> >     https://github.com/kitakar5525/linux-kernel/commit/1a8488cdd31ad38a3805824700b29d1e5213d3f2
> > 
> > [2] ("atomisp: pci: css2400: remove ISP2401 ifdefs")
> >     https://github.com/kitakar5525/linux-kernel/commit/dd6723fc5b9fe040e33b227b509a7e004243edce
> > [3] ("atomisp: pci: remove ISP2401 ifdefs for main pci driver")
> >     https://github.com/kitakar5525/linux-kernel/commit/1734341f84a96945af7635f6fff061db910f746f
> 
> Ok, if there are more if/ifdef ISP2401 that, if reverted will keep the
> driver running with the firmware we're using, I'm all for it. Just send
> the patches ;-)
> 
> > 
> > Here is the kernel tree if someone is interested:
> > 
> >         https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@a49d25364dfb_first
> > 
> > Especially, here is one of the part where this patch is touching
> > for example:
> > 
> >         --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
> >         +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
> >         @@ -416,26 +362,16 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, struct ia_css_stream_info *info)
> > [...]
> >         -#ifndef ISP2401
> >          	port = (unsigned int) pipe->stream->config.source.port.port;
> >          	assert(port < N_CSI_PORTS);
> >          	if (port >= N_CSI_PORTS) {
> >         -#else
> >         -	if (!ia_css_mipi_is_source_port_valid(pipe, &port)) {
> >         -#endif
> >          		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> >          			"allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
> >          			pipe, port);
> > 
> > By removing (almost) all of the `#ifdef ISP2401` things, (although we
> > still can't remove like former USE_INPUT_SYSTEM_VERSION_2,
> > USE_INPUT_SYSTEM_VERSION_2401) we can reduce the number of ifdefs.
> 
> Sounds good to me.
> 
> > 
> > 
> >   ## `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */` in the recent
> >       atomisp
> > 
> > That is for the initial commit of atomisp. For the recent version of
> > atomisp, we can still remove `ifdef ISP2401` things (again, except things
> > which were added inside it later upstream) as well as the former
> > `ifdef ISP2401` things, i.e., `if (IS_ISP2401)` [1] and things commented
> > with [2] `/* ISP2401 */`.
> > 
> > [1] ("atomisp: pci: remove IS_ISP2401 test")
> >     https://github.com/kitakar5525/linux-kernel/commit/397e543e493dfd60d91e2b5cc164da342b26906c
> > [2] ("atomisp: pci: remove `/* ISP2401 */` comments and its contents")
> >     https://github.com/kitakar5525/linux-kernel/commit/b3928e3c1a709853971715ce35459b9b79e708f2
> >     These commits were made against
> >     bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
> >     where I randomely picked.
> > 
> > Here is the kernel tree if someone is interested:
> > 
> >     https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@bd674b5a413c_before_get_rid_ver_globals
> > 
> > I confirmed capture is still working here on surface3 (ISP2401). Compile
> > tested for ISP2400. As you can see, there are some WIP and FIXME commits
> > on top of removing such tests though. (The others are backports).
> > 
> > Especially, here is one of the part where this patch is touching
> > for example:
> > 
> >         --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> >         +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> >         @@ -553,10 +548,7 @@ free_mipi_frames(struct ia_css_pipe *pipe) {
> >          			return err;
> >          		}
> >         
> >         -		if (!IS_ISP2401)
> >         -			port = (unsigned int)pipe->stream->config.source.port.port;
> >         -		else
> >         -			err = ia_css_mipi_is_source_port_valid(pipe, &port);
> >         +		port = (unsigned int)pipe->stream->config.source.port.port;
> >         
> >          		assert(port < N_CSI_PORTS);
> >         
> > 
> > So, we can also remove a lot of `if (IS_ISP2401)` and `/* ISP2401 */`
> > things as well as the remaining `ifdef ISP2401`.
> > 
> > 
> > 
> >   ## WIP: removing `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */`
> >      against the latest atomisp
> > 
> > And here is the branch where I'm working on removing such tests against
> > the latest atomisp:
> > 
> >         https://github.com/kitakar5525/linux-kernel/commits/mainline+upst_atomisp+remove_unneeded_tests
> > 
> > It'd be the best if I can show you working one, 
> 
> Well, send the ones that were already tested, and won't cause
> regressions to v4l2grab and camorama (e. g. it shouldn't cause
> generic V4L2 generic apps to break).
> 
> It would be nice to also not break nvt and other original apps for
> this device, as it could be useful later, in order to be able to 
> test the other pipelines, as currently only the preview one seems
> to be working properly, at least with generic apps.

Got it :-)

> > but it currently has
> > seemingly include issues on ISP2400 vs ISP2401 (as well as many WIP
> > commits I added):
> > 
> > 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c: In function ‘ia_css_isys_init’:
> > 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:29:9: error: unknown type name ‘backend_channel_cfg_t’
> > 	   29 |         backend_channel_cfg_t backend_ch0;
> > 	      |         ^~~~~~~~~~~~~~~~~~~~~
> > 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:30:9: error: unknown type name ‘backend_channel_cfg_t’
> > 	   30 |         backend_channel_cfg_t backend_ch1;
> > 	      |         ^~~~~~~~~~~~~~~~~~~~~
> > 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:31:9: error: unknown type name ‘target_cfg2400_t’
> > 	   31 |         target_cfg2400_t targetB;
> > 	      |         ^~~~~~~~~~~~~~~~
> > 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:32:9: error: unknown type name ‘target_cfg2400_t’
> > 	   32 |         target_cfg2400_t targetC;
> > 	      |         ^~~~~~~~~~~~~~~~
> > 	[...]
> > 
> > The full output of the make error is here:
> > 
> >         ("NOTE: issue: some undeclared errors")
> >         https://github.com/kitakar5525/linux-kernel/commit/a932d16681f941161385659b9d0316a3a4975e86
> > 
> > 
> > 
> > Regards,
> > Tsuchiya Yuto
> > > > >
Tsuchiya Yuto Dec. 1, 2021, noon UTC | #20 
On Wed, 2021-12-01 at 20:35 +0900, Tsuchiya Yuto wrote:
> On Thu, 2021-11-11 at 18:38 +0000, Mauro Carvalho Chehab wrote:
> > Em Thu, 11 Nov 2021 23:34:23 +0900
> > Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> > 
> > > Sorry for a little late reply. This is hard to explain...
> > > 
> > > On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:
> > > > Em Mon, 01 Nov 2021 22:38:55 +0900
> > > > Tsuchiya Yuto <kitakar@gmail.com> escreveu:  
> > > 
> > > [...]
> > > >   
> > > 
> > > > > This is not directly related to this series, but how we should reduce
> > > > > the ifdef usage in the future? Here are my two ideas:
> > > > > 
> > > > >   1. (my initial idea) remove `#ifdef ISP2401` part and make ISP2401
> > > > >      part completely irci_stable_candrpv_0415_20150521_0458
> > > > > 
> > > > > this way does not require (relatively) much human work I think.
> > > > > 
> > > > > But as Mauro says, the `#ifdef ISP2401` part (irci_ecr-master_20150911_0724)
> > > > > is basically an improved version.  
> > > > 
> > > > No. What I said is that the if (ISP2401) and the remaining ifdefs are because
> > > > of BYT x CHT.  
> > > 
> > > I need to elaborate on this. Indeed some of them are really because of
> > > BYT x CHT, but others are stuff from irci_ecr-master_20150911_0724.
> > > 
> > > What I meant when I mentioned "remove `#ifdef ISP2401` part" is that,
> > > removing things which was _initially_ inside the `#ifdef ISP2401` on the
> > > initial commit of atomisp.
> > > 
> > > Also I believe we can remove more `if (IS_ISP2401)` and `/* ISP2401 */`
> > > things as well as the some remaining `#ifdef ISP2401` things.
> > > 
> > > I added about this below and hope it clarifies things...
> > 
> > It is clearer now. Yeah, we can touch on whatever is inside the
> > ISP2401 ifs, as we can now test them. Touching things for ISP2400
> > is harder, as we depend on a test platform.
> > 
> > > > The worse part of them are related to those files
> > > > (See Makefile):
> > > > 
> > > > obj-byt = \
> > > > 	pci/css_2400_system/hive/ia_css_isp_configs.o \
> > > > 	pci/css_2400_system/hive/ia_css_isp_params.o \
> > > > 	pci/css_2400_system/hive/ia_css_isp_states.o \
> > > > 
> > > > obj-cht = \
> > > > 	pci/css_2401_system/hive/ia_css_isp_configs.o \
> > > > 	pci/css_2401_system/hive/ia_css_isp_params.o \
> > > > 	pci/css_2401_system/hive/ia_css_isp_states.o \
> > > > 	pci/css_2401_system/host/csi_rx.o \
> > > > 	pci/css_2401_system/host/ibuf_ctrl.o \
> > > > 	pci/css_2401_system/host/isys_dma.o \
> > > > 	pci/css_2401_system/host/isys_irq.o \
> > > > 	pci/css_2401_system/host/isys_stream2mmio.o
> > > > 
> > > > Those define regmaps for 2400 and 2401. I was able to remove a lot
> > > > of things from the old css_2400/css_2401 directories, but the ones
> > > > there at pci/*/css*/ia_css_isp_*.c are a lot more complex, and would
> > > > require some mapping functions to allow the same driver to work with
> > > > both BYT and CHT.
> > > > 
> > > > The better would be to test the driver first at BYT, fix issues (if any) and 
> > > > then write the mapping code.
> > > >   
> > > > > So, we may also:
> > > > > 
> > > > >   2. continue unifying `#ifdef ISP2401` and `#ifndef ISP2401` parts
> > > > > 
> > > > > but this way needs more human work I think though. And if we go this
> > > > > way, I also need to rewrite this patch as mentioned in the commit
> > > > > message.  
> > > 
> > > What the idea #1 want to say is, let's remove things _initially_ within
> > > `ifdef ISP2401` (so, except things which were added inside it later
> > > upstream) including formerly within `ifdef ISP2401` things, i.e.,
> > > `if (IS_ISP2401)` and things commented with `/* ISP2401 */`.
> > > 
> > > However, I don't say we can remove all the ifdefs like things formerly
> > > within USE_INPUT_SYSTEM_VERSION_2, USE_INPUT_SYSTEM_VERSION_2401, etc.,
> > > which later removed/integrated into `ifdef ISP2401` on some commits [1].
> > > We may temporarily revert those commits when we want to distinguish
> > > between what were formerly within there and what were not.
> > > 
> > > Such ifdefs were added by them as a real hardware difference. Thus, I
> > > agree that we still need the CONFIG_VIDEO_ATOMISP_ISP2401 stuff to support
> > > both ISP2400/ISP2401 at the same time.
> > > 
> > > This is what I meant "reduce the ifdef usage" in a previous mail. So,
> > > I'm not talking about if dropping CONFIG_VIDEO_ATOMISP_ISP2401 is doable,
> > > but talking about just how to reduce the code.
> > > 
> > > [1] 641c2292bf19 ("media: atomisp: get rid of version-dependent globals")
> > >     bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
> > > 
> > > Anyway, if you agree or not on what I'm saying, can I send this patch
> > > without code changes in v2, i.e., looks OK for you regarding the code?
> > > I'll remove the commit message about
> > > irci_stable_candrpv_0415_20150521_0458 vs irci_ecr-master_20150911_0724
> > > in v2 anyway, which needs to be discussed further later.
> > 
> > No need for a v2. The /17 patch series was merged already, plus some
> > patches from the /5 that made sense to apply.
> > 
> > Ok, there are some followup patches that could be added, but please
> > send those in separate.
> 
> OK, thanks. I'll prepare the followup patches as soon as I can.

Oh, I spoke too soon. The issues pointed out by code review are already
fixed/gone by rebase and later patches. Thanks!

> > > The following notes are about what I have done until now for removing
> > > such tests. (More elaborated version than cover letter). You don't have
> > > to see them, but I hope it might clarify things...
> > > 
> > >   ## `ifdef ISP2401` added in the initial commit of atomisp
> > > 
> > > The `ifdef ISP2401` was the result of merging two different version of
> > > driver, added on the initial commit of upstreamed atomisp. And for the
> > > `ifdef ISP2401`, I confirmed I can remove (almost [1]) all of them against
> > > the initial commit of atomisp [2][3]
> > > 
> > > [1] here are the three exceptions:
> > >     ("NOTE: ifdef ISP2400/ISP2401 usage in aero-atomisp")
> > >     https://github.com/kitakar5525/linux-kernel/commit/1a8488cdd31ad38a3805824700b29d1e5213d3f2
> > > 
> > > [2] ("atomisp: pci: css2400: remove ISP2401 ifdefs")
> > >     https://github.com/kitakar5525/linux-kernel/commit/dd6723fc5b9fe040e33b227b509a7e004243edce
> > > [3] ("atomisp: pci: remove ISP2401 ifdefs for main pci driver")
> > >     https://github.com/kitakar5525/linux-kernel/commit/1734341f84a96945af7635f6fff061db910f746f
> > 
> > Ok, if there are more if/ifdef ISP2401 that, if reverted will keep the
> > driver running with the firmware we're using, I'm all for it. Just send
> > the patches ;-)
> > 
> > > 
> > > Here is the kernel tree if someone is interested:
> > > 
> > >         https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@a49d25364dfb_first
> > > 
> > > Especially, here is one of the part where this patch is touching
> > > for example:
> > > 
> > >         --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
> > >         +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
> > >         @@ -416,26 +362,16 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, struct ia_css_stream_info *info)
> > > [...]
> > >         -#ifndef ISP2401
> > >          	port = (unsigned int) pipe->stream->config.source.port.port;
> > >          	assert(port < N_CSI_PORTS);
> > >          	if (port >= N_CSI_PORTS) {
> > >         -#else
> > >         -	if (!ia_css_mipi_is_source_port_valid(pipe, &port)) {
> > >         -#endif
> > >          		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> > >          			"allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
> > >          			pipe, port);
> > > 
> > > By removing (almost) all of the `#ifdef ISP2401` things, (although we
> > > still can't remove like former USE_INPUT_SYSTEM_VERSION_2,
> > > USE_INPUT_SYSTEM_VERSION_2401) we can reduce the number of ifdefs.
> > 
> > Sounds good to me.
> > 
> > > 
> > > 
> > >   ## `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */` in the recent
> > >       atomisp
> > > 
> > > That is for the initial commit of atomisp. For the recent version of
> > > atomisp, we can still remove `ifdef ISP2401` things (again, except things
> > > which were added inside it later upstream) as well as the former
> > > `ifdef ISP2401` things, i.e., `if (IS_ISP2401)` [1] and things commented
> > > with [2] `/* ISP2401 */`.
> > > 
> > > [1] ("atomisp: pci: remove IS_ISP2401 test")
> > >     https://github.com/kitakar5525/linux-kernel/commit/397e543e493dfd60d91e2b5cc164da342b26906c
> > > [2] ("atomisp: pci: remove `/* ISP2401 */` comments and its contents")
> > >     https://github.com/kitakar5525/linux-kernel/commit/b3928e3c1a709853971715ce35459b9b79e708f2
> > >     These commits were made against
> > >     bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
> > >     where I randomely picked.
> > > 
> > > Here is the kernel tree if someone is interested:
> > > 
> > >     https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@bd674b5a413c_before_get_rid_ver_globals
> > > 
> > > I confirmed capture is still working here on surface3 (ISP2401). Compile
> > > tested for ISP2400. As you can see, there are some WIP and FIXME commits
> > > on top of removing such tests though. (The others are backports).
> > > 
> > > Especially, here is one of the part where this patch is touching
> > > for example:
> > > 
> > >         --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > >         +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > >         @@ -553,10 +548,7 @@ free_mipi_frames(struct ia_css_pipe *pipe) {
> > >          			return err;
> > >          		}
> > >         
> > >         -		if (!IS_ISP2401)
> > >         -			port = (unsigned int)pipe->stream->config.source.port.port;
> > >         -		else
> > >         -			err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > >         +		port = (unsigned int)pipe->stream->config.source.port.port;
> > >         
> > >          		assert(port < N_CSI_PORTS);
> > >         
> > > 
> > > So, we can also remove a lot of `if (IS_ISP2401)` and `/* ISP2401 */`
> > > things as well as the remaining `ifdef ISP2401`.
> > > 
> > > 
> > > 
> > >   ## WIP: removing `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */`
> > >      against the latest atomisp
> > > 
> > > And here is the branch where I'm working on removing such tests against
> > > the latest atomisp:
> > > 
> > >         https://github.com/kitakar5525/linux-kernel/commits/mainline+upst_atomisp+remove_unneeded_tests
> > > 
> > > It'd be the best if I can show you working one, 
> > 
> > Well, send the ones that were already tested, and won't cause
> > regressions to v4l2grab and camorama (e. g. it shouldn't cause
> > generic V4L2 generic apps to break).
> > 
> > It would be nice to also not break nvt and other original apps for
> > this device, as it could be useful later, in order to be able to 
> > test the other pipelines, as currently only the preview one seems
> > to be working properly, at least with generic apps.
> 
> Got it :-)
> 
> > > but it currently has
> > > seemingly include issues on ISP2400 vs ISP2401 (as well as many WIP
> > > commits I added):
> > > 
> > > 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c: In function ‘ia_css_isys_init’:
> > > 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:29:9: error: unknown type name ‘backend_channel_cfg_t’
> > > 	   29 |         backend_channel_cfg_t backend_ch0;
> > > 	      |         ^~~~~~~~~~~~~~~~~~~~~
> > > 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:30:9: error: unknown type name ‘backend_channel_cfg_t’
> > > 	   30 |         backend_channel_cfg_t backend_ch1;
> > > 	      |         ^~~~~~~~~~~~~~~~~~~~~
> > > 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:31:9: error: unknown type name ‘target_cfg2400_t’
> > > 	   31 |         target_cfg2400_t targetB;
> > > 	      |         ^~~~~~~~~~~~~~~~
> > > 	drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:32:9: error: unknown type name ‘target_cfg2400_t’
> > > 	   32 |         target_cfg2400_t targetC;
> > > 	      |         ^~~~~~~~~~~~~~~~
> > > 	[...]
> > > 
> > > The full output of the make error is here:
> > > 
> > >         ("NOTE: issue: some undeclared errors")
> > >         https://github.com/kitakar5525/linux-kernel/commit/a932d16681f941161385659b9d0316a3a4975e86
> > > 
> > > 
> > > 
> > > Regards,
> > > Tsuchiya Yuto
> > > > > >   
>
diff mbox series

Patch

diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
index 483d40a467c7..65fc93c5d56b 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
@@ -430,7 +430,8 @@  allocate_mipi_frames(struct ia_css_pipe *pipe,
 
 	assert(port < N_CSI_PORTS);
 
-	if (port >= N_CSI_PORTS || err) {
+	if ((!IS_ISP2401 && port >= N_CSI_PORTS) ||
+	    (IS_ISP2401 && err)) {
 		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
 				    "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
 				    pipe, port);
@@ -559,7 +560,8 @@  free_mipi_frames(struct ia_css_pipe *pipe)
 
 		assert(port < N_CSI_PORTS);
 
-		if (port >= N_CSI_PORTS || err) {
+		if ((!IS_ISP2401 && port >= N_CSI_PORTS) ||
+		    (IS_ISP2401 && err)) {
 			ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
 					    "free_mipi_frames(%p, %d) exit: error: pipe port is not correct.\n",
 					    pipe, port);
@@ -670,7 +672,8 @@  send_mipi_frames(struct ia_css_pipe *pipe)
 
 	assert(port < N_CSI_PORTS);
 
-	if (port >= N_CSI_PORTS || err) {
+	if ((!IS_ISP2401 && port >= N_CSI_PORTS) ||
+	    (IS_ISP2401 && err)) {
 		IA_CSS_ERROR("send_mipi_frames(%p) exit: invalid port specified (port=%d).\n",
 			     pipe, port);
 		return err;