| Message ID | 20230508062632.34537-5-hpa@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/5] staging: media: atomisp: sh_css: Remove #ifdef ISP2401 | expand |
Hi Kate, On 5/8/23 08:26, 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 | 65 ++++++------------- > 1 file changed, 20 insertions(+), 45 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..52a1ed63e9a5 100644 > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c > @@ -386,30 +381,22 @@ 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]); Before you changes this code always run ISP2401, now it only runs when (ref_count_mipi_allocation[port] != 0) is not true. So this statement should stay here in the code, just prefixed with a if (IS_ISP2401) condition. > - /* > - * 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; > + } 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. > + */ This comment block actually belongs to the if (ref_count_mipi_allocation[port] != 0) check, the code executed if that check passes was actually different between the ISP2400 and ISP2401 (my bad, sorry). The ISP2401 case did an extra: ref_count_mipi_allocation[port]++; when (ref_count_mipi_allocation[port] != 0), so we need to add: if (IS_ISP2401) ref_count_mipi_allocation[port]++; above the return 0 above. > + if (IS_ISP2401) > + err = calculate_mipi_buff_size(&pipe->stream->config, > + &my_css.mipi_frame_size[port]); I have fixed this all up while merging your series and the new diff for this code-block now looks like this: @@ -386,9 +381,10 @@ 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]); + 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. @@ -396,20 +392,14 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, * In that case only 2400 related code should remain. */ if (ref_count_mipi_allocation[port] != 0) { - ref_count_mipi_allocation[port]++; + if (IS_ISP2401) + 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]++; Regards, Hans
Hi Hans, On Thu, May 11, 2023 at 2:34 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Kate, > > On 5/8/23 08:26, 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 | 65 ++++++------------- > > 1 file changed, 20 insertions(+), 45 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..52a1ed63e9a5 100644 > > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c > > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c > > @@ -386,30 +381,22 @@ 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]); > > Before you changes this code always run ISP2401, now it only runs > when (ref_count_mipi_allocation[port] != 0) is not true. > > So this statement should stay here in the code, just prefixed > with a if (IS_ISP2401) condition. > > > - /* > > - * 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; > > + } 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. > > + */ > > This comment block actually belongs to the if (ref_count_mipi_allocation[port] != 0) > check, the code executed if that check passes was actually different between > the ISP2400 and ISP2401 (my bad, sorry). The ISP2401 case did an extra: > > ref_count_mipi_allocation[port]++; > > when (ref_count_mipi_allocation[port] != 0), so we need to add: > > if (IS_ISP2401) > ref_count_mipi_allocation[port]++; > > above the return 0 above. > > > + if (IS_ISP2401) > > + err = calculate_mipi_buff_size(&pipe->stream->config, > > + &my_css.mipi_frame_size[port]); > > I have fixed this all up while merging your series and the new > diff for this code-block now looks like this: > > @@ -386,9 +381,10 @@ 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]); > + 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. > @@ -396,20 +392,14 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, > * In that case only 2400 related code should remain. > */ > if (ref_count_mipi_allocation[port] != 0) { > - ref_count_mipi_allocation[port]++; > + if (IS_ISP2401) > + 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]++; > > > > Regards, > > Hans > It's my bad when trying to simplify the if expressions. Thank you for fixing this. :)
diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c index bc6e8598a776..52a1ed63e9a5 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,13 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, return -EINVAL; } -#ifdef ISP2401 - if (pipe->stream->config.online) { + 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; } -#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 +381,22 @@ 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; + } 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. + */ + if (IS_ISP2401) + err = calculate_mipi_buff_size(&pipe->stream->config, + &my_css.mipi_frame_size[port]); } -#endif ref_count_mipi_allocation[port]++; @@ -503,14 +490,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 +521,6 @@ 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); - } -#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 | 65 ++++++------------- 1 file changed, 20 insertions(+), 45 deletions(-)