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

[Tsuchiya Yuto <kitakar@gmail.com>]

<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