Message-ID: <418bc2d2ccac70c9d634d0d5cad44078c988eabf.camel@gmail.com>
Date:   Wed, 01 Dec 2021 21:00:09 +0900
From:   Tsuchiya Yuto <kitakar@...il.com>
To:     Mauro Carvalho Chehab <mchehab@...nel.org>
Cc:     Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-media@...r.kernel.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org,
        Nable <nable.maininbox@...glemail.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Fabio Aiuto <fabioaiuto83@...il.com>,
        "andrey.i.trufanov" <andrey.i.trufanov@...il.com>,
        Patrik Gfeller <patrik.gfeller@...il.com>,
        Hans de Goede <hdegoede@...hat.com>
Subject: Re: [PATCH 04/17] media: atomisp: pci: do not use err var when
 checking port validity for ISP2400

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@...il.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@...il.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
> > > > > >   
> 

Powered by blists - more mailing lists