Message-ID: <20211117111956.215743f8@sal.lan>
Date:   Wed, 17 Nov 2021 11:19:56 +0000
From:   Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     linuxarm@...wei.com, mauro.chehab@...wei.com,
        Deepak R Varma <drv@...lo.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Tsuchiya Yuto <kitakar@...il.com>,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        linux-staging@...ts.linux.dev
Subject: Re: [PATCH 13/13] media: atomisp: sh_css_sp: better support the
 current firmware

Em Wed, 17 Nov 2021 13:02:38 +0300
Dan Carpenter <dan.carpenter@...cle.com> escreveu:

> On Wed, Nov 17, 2021 at 09:25:38AM +0000, Mauro Carvalho Chehab wrote:
> > As we're using Intel Aero firmware, make the code closer to the
> > driver for such device.  
> 
> I don't really understand this commit message.  Is there a out of tree
> Intel driver?

Yes. It is at:

	https://github.com/intel-aero/meta-intel-aero-base/tree/master/recipes-kernel/linux/linux-yocto

I guess I was too lazy to place the above on almost all patches in this 
series. There are some past patches for atomisp that already contains
such info.

The atomisp's TODO mentions it:


	- for CHT: /lib/firmware/shisp_2401a0_v21.bin

	  https://github.com/intel-aero/meta-intel-aero-base/blob/master/recipes-kernel/linux/linux-yocto/shisp_2401a0_v21.bin

-

Long answer:

Basically, as far as I'm aware, the original atomisp driver is produced by
some code generator, which not only dynamically add code to the driver's
source code output, but also sets several #ifdefs and dynamically change the
firmware API depending on the Atom CPU, features and on other options.

If you take a look at the original commit which added this driver:

	a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")

It basically had some efforts from Alan to support both ISP2400 (BYT) and
ISP2401 (CHT) versions at the same code.

If you look on its TODO, it says:

	"The ISP code depends on the exact FW version. The version defined in
	   BYT: 
	   drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
	   static const char *release_version = STR(irci_stable_candrpv_0415_20150521_0458);
	   CHT:
	   drivers/staging/media/atomisp/pci/atomisp2/css/sh_css_firmware.c
	   static const char *release_version = STR(irci_ecr-master_20150911_0724);"

So, the versions for BYT and CHT at the original patch were different, and
there are lots of #ifdefs to test between such differences.

There's another problem, through. The irci_ecr-master_20150911_0724 firmware
was never placed at linux-firmware, and got lost in time. I don't know
anyone whose has such exact version. It seems it got lost in time, no idea
why.

The only non-android Linux driver we know is for the Intel Aero hardware,
which is Cherrytail (CHT). For such hardware, the firmware is also provided
at the Intel Aero Yocto repositories, at:

	https://github.com/intel-aero/meta-intel-aero-base/blob/master/recipes-kernel/linux/linux-yocto/shisp_2401a0_v21.bin

The version of this firmware is:

	$ strings /lib/firmware/shisp_2401a0_v21.bin |grep irci_stable_
	irci_stable_candrpv_0415_20150521_0458

As we have both drivers and firmware for such version, we're actually
adjusting the upstream code to match this version for ISP2401 (CHT).

There's a side effect with this change: the code for ISP2400 and ISP2401
will be both based on the same version, which helps to reduce the
differences at the driver level - that's assuming that we can:

	- find a publicly-available ISP2400 
	  irci_stable_candrpv_0415_20150521_0458 binary;
	- have some BYT hardware for testing with upstream drivers[1];
	- have an Android driver for the same fix, in order to help
	  double-checking if the code is compatible with the firmware.

[1] On some BYT BIOS, the normal ACPI method of detecting the hardware
doesn't work, as the ISP is visible though the GPU. That's the case
of a commonly found hardware (Asus T100 with the production BIOS).

> 
> > diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c
> > index f6db1f4a0e1d..a11078acb072 100644
> > --- a/drivers/staging/media/atomisp/pci/sh_css_sp.c
> > +++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c
> > @@ -982,6 +982,7 @@ sh_css_sp_init_stage(struct ia_css_binary *binary,
> >  	/* Make sure binary name is smaller than allowed string size */
> >  	assert(strlen(binary_name) < SH_CSS_MAX_BINARY_NAME - 1);
> >  	strscpy(sh_css_isp_stage.binary_name, binary_name, SH_CSS_MAX_BINARY_NAME);
> > +	sh_css_isp_stage.binary_name[SH_CSS_MAX_BINARY_NAME - 1] = 0;  
> 
> No, need for this.  strscpy() already guarantees that the result is NULL
> terminated.  That's one of the main reasons Linus invented it instead of
> using strncpy().
> 
> >  #ifdef ISP2401
> > -	if (stage == 0) {
> > -		pipe = find_pipe_by_num(sh_css_sp_group.pipe[thread_id].pipe_num);
> > -		if (!pipe)
> > -			return -EINVAL;
> > +	pipe = find_pipe_by_num(sh_css_sp_group.pipe[thread_id].pipe_num);
> > +	if (!pipe)
> > +		return -EINVAL;
> >  
> > -		if (args->in_frame)
> > -			ia_css_get_crop_offsets(pipe, &args->in_frame->info);
> > -		else
> > -			ia_css_get_crop_offsets(pipe, &binary->in_frame_info);
> > -	}
> > +	if (args->in_frame)
> > +		ia_css_get_crop_offsets(pipe, &args->in_frame->info);
> > +	else if (&binary->in_frame_info)  
>                   ^^^^^^^^^^^^^^^^^^^^^
> 
> This condition is wrong.  This is the address of something in the middle
> of "binary" so it can't be NULL.
> 
> > +		ia_css_get_crop_offsets(pipe, &binary->in_frame_info);
> >  #else
> >  	(void)pipe; /*avoid build warning*/
> >  #endif  
> 
> regards,
> dan carpenter

Powered by blists - more mailing lists