<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 [off-list ref] escreveu:

quoted

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.

quoted

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