On Thu, 2021-11-11 at 18:38 +0000, Mauro Carvalho Chehab wrote:

Em Thu, 11 Nov 2021 23:34:23 +0900
Tsuchiya Yuto [off-list ref] escreveu:

quoted

Sorry for a little late reply. This is hard to explain...

On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:

quoted

Em Mon, 01 Nov 2021 22:38:55 +0900
Tsuchiya Yuto [off-list ref] escreveu:  

[...]

quoted

quoted

quoted

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.

quoted

quoted

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.
  

quoted

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.

quoted

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 ;-)

quoted

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.

quoted

  ## `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 :-)

quoted

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

quoted

quoted

quoted