| Message ID | 20230425074841.29063-6-hpa@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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
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 @@ -67,13 +67,12 @@ ia_css_mipi_frame_calculate_size(const unsigned int width, unsigned int mem_words = 0; unsigned int width_padded = width; -#if defined(ISP2401) /* The changes will be reverted as soon as RAW * Buffers are deployed by the 2401 Input System * in the non-continuous use scenario. */ - width_padded += (2 * ISP_VEC_NELEMS); -#endif + if (IS_ISP2401) + width_padded += (2 * ISP_VEC_NELEMS); IA_CSS_ENTER("padded_width=%d, height=%d, format=%d, hasSOLandEOL=%d, embedded_data_size_words=%d\n", width_padded, height, format, hasSOLandEOL, embedded_data_size_words); @@ -235,7 +234,6 @@ bool mipi_is_free(void) return true; } -#if defined(ISP2401) /* * @brief Calculate the required MIPI buffer sizes. * Based on the stream configuration, calculate the @@ -342,7 +340,6 @@ static int calculate_mipi_buff_size(struct ia_css_stream_config *stream_cfg, IA_CSS_LEAVE_ERR(err); return err; } -#endif int allocate_mipi_frames(struct ia_css_pipe *pipe, @@ -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", @@ -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 ref_count_mipi_allocation[port]++; @@ -503,14 +500,14 @@ free_mipi_frames(struct ia_css_pipe *pipe) } if (ref_count_mipi_allocation[port] > 0) { -#if !defined(ISP2401) - assert(ref_count_mipi_allocation[port] == 1); - if (ref_count_mipi_allocation[port] != 1) { - IA_CSS_ERROR("free_mipi_frames(%p) exit: wrong ref_count (ref_count=%d).", - pipe, ref_count_mipi_allocation[port]); - return err; + if (!IS_ISP2401) { + assert(ref_count_mipi_allocation[port] == 1); + if (ref_count_mipi_allocation[port] != 1) { + IA_CSS_ERROR("free_mipi_frames(%p) exit: wrong ref_count (ref_count=%d).", + pipe, ref_count_mipi_allocation[port]); + return err; + } } -#endif ref_count_mipi_allocation[port]--; @@ -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. */
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(-)