Date:   Wed, 01 Dec 2021 20:35:39 +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 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.

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