| Message ID | f0c9b8ec25fedbfccac6d14ff1add2d40beaa088.1637140900.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: atomisp: reduce the gap for ISP2401 | expand |
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? > 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
Em Wed, 17 Nov 2021 13:02:38 +0300 Dan Carpenter <dan.carpenter@oracle.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
On Wed, Nov 17, 2021 at 11:19:56AM +0000, Mauro Carvalho Chehab wrote: > Em Wed, 17 Nov 2021 13:02:38 +0300 > Dan Carpenter <dan.carpenter@oracle.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 > Fine, but only removing the if (stage == 0) { is correct. The other two changes are harmless but wrong. regards, dan carpenter
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; sh_css_isp_stage.mem_initializers = *isp_mem_if; /* @@ -1013,16 +1014,14 @@ sh_css_sp_init_stage(struct ia_css_binary *binary, return err; #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) + ia_css_get_crop_offsets(pipe, &binary->in_frame_info); #else (void)pipe; /*avoid build warning*/ #endif
As we're using Intel Aero firmware, make the code closer to the driver for such device. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH 00/13] at: https://lore.kernel.org/all/cover.1637140900.git.mchehab+huawei@kernel.org/ drivers/staging/media/atomisp/pci/sh_css_sp.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)