[v2,5/5] staging: media: atomisp: sh_css_mipi: Remove #ifdef 2041
Commit Message
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(-)
Comments
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. :)
@@ -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. */