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@xxxxxxxxxx>
> ---
> .../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
