[PATCH v2 0/2] staging: media: atomisp: clean up ISP configuration path

[PATCH v2 0/2] staging: media: atomisp: clean up ISP configuration path

[Jose A. Perez de Azpillaga]

NOT TESTED, REVIEW CAREFULLY.

This series cleans up technical debt in the ISP configuration path of
the AtomISP driver.

Resolves a long-standing FIXME by gating ref and TNR frame configuration
behind the ISP feature flags that already govern their allocation,
rather than unconditionally attempting to use frames that may not have
been built into the pipeline, and removes a duplicate call that
overwrites the same cached state with identical values.

v2:
- Replaced NULL checks with feature flag guards to address the root
  cause rather than the symptom.
- Updated subject line and commit message accordingly in patch 1/2.
- Expanded commit message to explain why the duplicate call is safe to
  remove in patch 2/2.
- Updated subject line in patch 2/2.

Jose A. Perez de Azpillaga (2):
  staging: media: atomisp: gate ref and tnr frame config behind ISP
    enable flags
  media: atomisp: remove redundant call to ia_css_output0_configure()

 drivers/staging/media/atomisp/pci/sh_css_sp.c | 41 +++++++++----------
 1 file changed, 19 insertions(+), 22 deletions(-)

--
2.53.0

[PATCH v2 1/2] staging: media: atomisp: gate ref and tnr frame config behind ISP enable flags

[Jose A. Perez de Azpillaga]

The FIXME comment noted that delay_frames can be NULL for certain
pipeline configurations, without knowing why. The reason is that when a
binary does not enable ref_frame, delay frame allocation is
intentionally skipped to save memory, leaving the pointers NULL by
design.

The ISP feature flags in binary->info->sp.enable accurately reflect
which features are active for a given binary. Using enable.ref_frame and
enable.tnr as the predicate for their respective configuration steps
ensures the configuration path stays in sync with what was actually
built into the pipeline.

Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 drivers/staging/media/atomisp/pci/sh_css_sp.c | 38 +++++++++----------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c
index 6da151e7a873..abdffff41ae2 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_sp.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c
@@ -775,9 +775,13 @@ static int configure_isp_from_args(const struct sh_css_sp_pipeline *pipeline,
 	ret = ia_css_fpn_configure(binary,  &binary->in_frame_info);
 	if (ret)
 		return ret;
-	ret = ia_css_crop_configure(binary, ia_css_frame_get_info(args->delay_frames[0]));
-	if (ret)
-		return ret;
+
+	if (binary->info->sp.enable.ref_frame) {
+		ret = ia_css_crop_configure(binary, ia_css_frame_get_info(args->delay_frames[0]));
+		if (ret)
+			return ret;
+	}
+
 	ret = ia_css_qplane_configure(pipeline, binary, &binary->in_frame_info);
 	if (ret)
 		return ret;
@@ -807,22 +811,18 @@ static int configure_isp_from_args(const struct sh_css_sp_pipeline *pipeline,
 	if (ret)
 		return ret;

-	/*
-	 * FIXME: args->delay_frames can be NULL here
-	 *
-	 * Somehow, the driver at the Intel Atom Yocto tree doesn't seem to
-	 * suffer from the same issue.
-	 *
-	 * Anyway, the function below should now handle a NULL delay_frames
-	 * without crashing, but the pipeline should likely be built without
-	 * adding it at the first place (or there are a hidden bug somewhere)
-	 */
-	ret = ia_css_ref_configure(binary, args->delay_frames, pipeline->dvs_frame_delay);
-	if (ret)
-		return ret;
-	ret = ia_css_tnr_configure(binary, args->tnr_frames);
-	if (ret)
-		return ret;
+	if (binary->info->sp.enable.ref_frame) {
+		ret = ia_css_ref_configure(binary, args->delay_frames, pipeline->dvs_frame_delay);
+		if (ret)
+			return ret;
+	}
+
+	if (binary->info->sp.enable.tnr) {
+		ret = ia_css_tnr_configure(binary, args->tnr_frames);
+		if (ret)
+			return ret;
+	}
+
 	return ia_css_bayer_io_config(binary, args);
 }

--
2.53.0

[PATCH v2 2/2] staging: media: atomisp: remove redundant call to ia_css_output0_configure()

[Jose A. Perez de Azpillaga]

The function configure_isp_from_args() contained a duplicate call to
ia_css_output0_configure() using the same output frame index. Remove the
redundant call to simplify the configuration path.

The ia_css_output0_configure() function acts as a configuration setter.
It populates a struct ia_css_output0_configuration from the frame info
and caches it in the binary parameters. Calling it twice with the same
out_frame[0] pointer merely overwrites the exact same state with
identical values. It has no cumulative state, either does its order
matter relative to ia_css_copy_output_configure().

Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 drivers/staging/media/atomisp/pci/sh_css_sp.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c
index abdffff41ae2..2beb7168517f 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_sp.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c
@@ -792,9 +792,6 @@ static int configure_isp_from_args(const struct sh_css_sp_pipeline *pipeline,
 	if (ret)
 		return ret;
 	ret = ia_css_copy_output_configure(binary, args->copy_output);
-	if (ret)
-		return ret;
-	ret = ia_css_output0_configure(binary, ia_css_frame_get_info(args->out_frame[0]));
 	if (ret)
 		return ret;
 	ret = ia_css_iterator_configure(binary, ia_css_frame_get_info(args->in_frame));
--
2.53.0

Re: [PATCH v2 1/2] staging: media: atomisp: gate ref and tnr frame config behind ISP enable flags

[Andy Shevchenko]

On Wed, Apr 1, 2026 at 12:17 AM Jose A. Perez de Azpillaga
<azpijr@gmail.com> wrote:
>
> The FIXME comment noted that delay_frames can be NULL for certain
> pipeline configurations, without knowing why. The reason is that when a
> binary does not enable ref_frame, delay frame allocation is
> intentionally skipped to save memory, leaving the pointers NULL by
> design.
>
> The ISP feature flags in binary->info->sp.enable accurately reflect
> which features are active for a given binary. Using enable.ref_frame and
> enable.tnr as the predicate for their respective configuration steps
> ensures the configuration path stays in sync with what was actually
> built into the pipeline.

This sounds legit and being thought through, thanks! Also the code looks better.
Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] staging: media: atomisp: clean up ISP configuration path

[Andy Shevchenko]

On Wed, Apr 1, 2026 at 12:17 AM Jose A. Perez de Azpillaga
<azpijr@gmail.com> wrote:
>
> NOT TESTED, REVIEW CAREFULLY.
>
> This series cleans up technical debt in the ISP configuration path of
> the AtomISP driver.
>
> Resolves a long-standing FIXME by gating ref and TNR frame configuration
> behind the ISP feature flags that already govern their allocation,
> rather than unconditionally attempting to use frames that may not have
> been built into the pipeline, and removes a duplicate call that
> overwrites the same cached state with identical values.
>
> v2:
> - Replaced NULL checks with feature flag guards to address the root
>   cause rather than the symptom.
> - Updated subject line and commit message accordingly in patch 1/2.
> - Expanded commit message to explain why the duplicate call is safe to
>   remove in patch 2/2.
> - Updated subject line in patch 2/2.

Actually we don't need "staging:" in the Subject, it makes the lines
unnecessary longer. AtomISPv2 is a unique driver and I'm sure staging
or not we always identify it right. Also check `git log --no-merges
--oneline -- drivers/staging/media/atomisp/`, it shows the common way
of doing that.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] staging: media: atomisp: remove redundant call to ia_css_output0_configure()

[Andy Shevchenko]

On Wed, Apr 1, 2026 at 12:17 AM Jose A. Perez de Azpillaga
<azpijr@gmail.com> wrote:
>
> The function configure_isp_from_args() contained a duplicate call to
> ia_css_output0_configure() using the same output frame index. Remove the
> redundant call to simplify the configuration path.
>
> The ia_css_output0_configure() function acts as a configuration setter.
> It populates a struct ia_css_output0_configuration from the frame info
> and caches it in the binary parameters. Calling it twice with the same
> out_frame[0] pointer merely overwrites the exact same state with
> identical values. It has no cumulative state, either does its order

neither

> matter relative to ia_css_copy_output_configure().

With fixed subject and detail in the commit message above,
Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko