| Message ID | 20230425074841.29063-1-hpa@redhat.com |
|---|---|
| Headers | show |
| Series | staging: media: atomisp: Remove #ifdef 2401 | expand |
Hi Kate, On 4/25/23 09:48, Kate Hsuan wrote: > The actions of ISP2401 and 2400 are determined at the runtime. > > Signed-off-by: Kate Hsuan <hpa@redhat.com> > --- > .../staging/media/atomisp/pci/sh_css_mipi.c | 101 +++++++++--------- > 1 file changed, 49 insertions(+), 52 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c > index bc6e8598a776..9c9d3b27ded4 100644 > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c <snip> > @@ -363,15 +360,15 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, > return -EINVAL; > } > > -#ifdef ISP2401 > - if (pipe->stream->config.online) { > - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > - "allocate_mipi_frames(%p) exit: no buffers needed for 2401 pipe mode.\n", > - pipe); > - return 0; > + if (IS_ISP2401) { > + if (pipe->stream->config.online) { > + ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > + "allocate_mipi_frames(%p) exit: no buffers needed for 2401 pipe mode.\n", > + pipe); > + return 0; > + } > } > > -#endif > if (pipe->stream->config.mode != IA_CSS_INPUT_MODE_BUFFERED_SENSOR) { > ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > "allocate_mipi_frames(%p) exit: no buffers needed for pipe mode.\n", Please combine the 2 conditions with && instead of using nested if-s: if (IS_ISP2401 && pipe->stream->config.online) { ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, "allocate_mipi_frames(%p) exit: no buffers needed for 2401 pipe mode.\n", pipe); return 0; } > @@ -386,30 +383,30 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, > return -EINVAL; > } > > -#ifdef ISP2401 > - err = calculate_mipi_buff_size(&pipe->stream->config, > - &my_css.mipi_frame_size[port]); > - /* > - * 2401 system allows multiple streams to use same physical port. This is not > - * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution. > - * TODO AM: Once that is changed (removed) this code should be removed as well. > - * In that case only 2400 related code should remain. > - */ > - if (ref_count_mipi_allocation[port] != 0) { > - ref_count_mipi_allocation[port]++; > - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > - "allocate_mipi_frames(%p) leave: nothing to do, already allocated for this port (port=%d).\n", > - pipe, port); > - return 0; > - } > -#else > - if (ref_count_mipi_allocation[port] != 0) { > - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > - "allocate_mipi_frames(%p) exit: already allocated for this port (port=%d).\n", > - pipe, port); > - return 0; > + if (IS_ISP2401) { > + err = calculate_mipi_buff_size(&pipe->stream->config, > + &my_css.mipi_frame_size[port]); > + /* > + * 2401 system allows multiple streams to use same physical port. This is not > + * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution. > + * TODO AM: Once that is changed (removed) this code should be removed as well. > + * In that case only 2400 related code should remain. > + */ > + if (ref_count_mipi_allocation[port] != 0) { > + ref_count_mipi_allocation[port]++; > + ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > + "allocate_mipi_frames(%p) leave: nothing to do, already allocated for this port (port=%d).\n", > + pipe, port); > + return 0; > + } > + } else { > + if (ref_count_mipi_allocation[port] != 0) { > + ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > + "allocate_mipi_frames(%p) exit: already allocated for this port (port=%d).\n", > + pipe, port); > + return 0; > + } > } > -#endif The "if (ref_count_mipi_allocation[port] != 0) { log-message; return 0; }" block is shared between the ISP2400 and ISP2401. Except for one debug log message using "leave" and the other using "exit" these 2 blocks are 100% the same, so please pick one of the 2 log messages and move this block out of the if (IS_ISP2401) {} block (and drop the else branch). > @@ -534,18 +531,18 @@ free_mipi_frames(struct ia_css_pipe *pipe) > ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > "free_mipi_frames(%p) exit (deallocated).\n", pipe); > } > -#if defined(ISP2401) > else { > - /* 2401 system allows multiple streams to use same physical port. This is not > - * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution. > - * TODO AM: Once that is changed (removed) this code should be removed as well. > - * In that case only 2400 related code should remain. > - */ > - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > - "free_mipi_frames(%p) leave: nothing to do, other streams still use this port (port=%d).\n", > - pipe, port); > + if (IS_ISP2401) { > + /* 2401 system allows multiple streams to use same physical port. This is not > + * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution. > + * TODO AM: Once that is changed (removed) this code should be removed as well. > + * In that case only 2400 related code should remain. > + */ > + ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > + "free_mipi_frames(%p) leave: nothing to do, other streams still use this port (port=%d).\n", > + pipe, port); > + } > } > -#endif > } > } else { /* pipe ==NULL */ > /* AM TEMP: free-ing all mipi buffers just like a legacy code. */ Hmm maybe just drop the entire ia_css_debug_dtrace() call here instead of replace the #ifdef with if (IS_ISP2401) { } ? I don't see much value in the ia_css_debug_dtrace() call, so to me just dropping it seems best. Regards, Hans
Since #ifdef ISP2401 is used to determine the action of both models in compiler time, we have to provide two binaries for both models. It is very unfriendly for the users and for the package management aspect. The proposed patch removed the #ifdef ISP2041 from the codes and made the path for both models can be determined at the runtime. Some of the #ifdef is highly integrated with functions and data structures. If we try to remove them, it will cause many issues, such as duplicated variable/function name and data length. Therefore, these patches focus on removing the #ifdef without affecting the change of structure. Kate Hsuan (5): staging: media: atomisp: sh_css: Remove #ifdef ISP2401 staging: media: atomisp: runtime: frame: remove #ifdef ISP2401 staging: media: atomisp: sh_css_sp: Remove #ifdef ISP2401 staging: media: atomisp: sh_css_firmware: determine firmware version at runtime staging: media: atomisp: sh_css_mipi: Remove #ifdef 2041 .../atomisp/pci/runtime/frame/src/frame.c | 15 +- drivers/staging/media/atomisp/pci/sh_css.c | 584 +++++++++--------- .../media/atomisp/pci/sh_css_firmware.c | 18 +- .../staging/media/atomisp/pci/sh_css_mipi.c | 101 ++- drivers/staging/media/atomisp/pci/sh_css_sp.c | 10 +- 5 files changed, 359 insertions(+), 369 deletions(-)