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

[PATCH v3 0/2] 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.

v3:
- Picked up RB tag from Andy Shevchenko.
- Fixed typo. (either -> neither)
- Removed 'staging:' from subject lines.
- Added links to previous versions.

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.

Link [v2]: https://lore.kernel.org/linux-staging/20260331211649.421777-1-azpijr@gmail.com/
Link [v1]: https://lore.kernel.org/linux-staging/20260328192721.255493-1-azpijr@gmail.com/

Jose A. Perez de Azpillaga (2):
  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 v3 1/2] 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

Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
Reviewed-by: Andy Shevchenko <andy@kernel.org>
---
 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 v3 2/2] 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, neither does its order
matter relative to ia_css_copy_output_configure().

Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
Reviewed-by: Andy Shevchenko <andy@kernel.org>
---
 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 v3 2/2] media: atomisp: remove redundant call to ia_css_output0_configure()

[Greg Kroah-Hartman]

On Thu, Apr 02, 2026 at 08:33:45PM +0200, Jose A. Perez de Azpillaga 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.

Are you sure the hardware doesn't actually need this called twice?  Lots
of devices need to be told multiple times what to do in order for it to
"stick", hardware is "fun" that way :(

Have you tested this?

thanks,

greg k-h

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

[Jose A. Perez de Azpillaga]

On Fri, Apr 03, 2026 at 08:42:11AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 02, 2026 at 08:33:45PM +0200, Jose A. Perez de Azpillaga 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.
>
> Are you sure the hardware doesn't actually need this called twice?  Lots
> of devices need to be told multiple times what to do in order for it to
> "stick", hardware is "fun" that way :(
>

The concern is valid in general, but ia_css_output0_configure() does not
write to a hardware register.

ia_css_configure_output0() writes into binary->mem_params.params[], a
software-side DMEM parameter buffer in kernel memory. the ISP firmware
receives these parameters later as a batch, not at the time of the call.
calling a pure memory write twice with the same pointer and same value
simply overwrites the same location with identical data, there is no
hardware interaction that could require repetition.

> Have you tested this?
>

as noted in the cover letter, I don't have the hardware to test this.

--
regards,
jose a. p-a

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

[Greg Kroah-Hartman]

On Fri, Apr 03, 2026 at 11:02:17AM +0200, Jose A. Perez de Azpillaga wrote:
> On Fri, Apr 03, 2026 at 08:42:11AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Apr 02, 2026 at 08:33:45PM +0200, Jose A. Perez de Azpillaga 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.
> >
> > Are you sure the hardware doesn't actually need this called twice?  Lots
> > of devices need to be told multiple times what to do in order for it to
> > "stick", hardware is "fun" that way :(
> >
> 
> The concern is valid in general, but ia_css_output0_configure() does not
> write to a hardware register.
> 
> ia_css_configure_output0() writes into binary->mem_params.params[], a
> software-side DMEM parameter buffer in kernel memory. the ISP firmware
> receives these parameters later as a batch, not at the time of the call.
> calling a pure memory write twice with the same pointer and same value
> simply overwrites the same location with identical data, there is no
> hardware interaction that could require repetition.

Ok, great, perhaps put that in the changelog text?

> > Have you tested this?
> >
> 
> as noted in the cover letter, I don't have the hardware to test this.

That's going to make doing code logic changes a bit hard for this
driver, you might want to rethink this :)

thanks,

greg k-h

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

[Jose A. Perez de Azpillaga]

On Fri, Apr 03, 2026 at 11:53:59AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Apr 03, 2026 at 11:02:17AM +0200, Jose A. Perez de Azpillaga wrote:
> > On Fri, Apr 03, 2026 at 08:42:11AM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Apr 02, 2026 at 08:33:45PM +0200, Jose A. Perez de Azpillaga 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.
> > >
> > > Are you sure the hardware doesn't actually need this called twice?  Lots
> > > of devices need to be told multiple times what to do in order for it to
> > > "stick", hardware is "fun" that way :(
> > >
> >
> > The concern is valid in general, but ia_css_output0_configure() does not
> > write to a hardware register.
> >
> > ia_css_configure_output0() writes into binary->mem_params.params[], a
> > software-side DMEM parameter buffer in kernel memory. the ISP firmware
> > receives these parameters later as a batch, not at the time of the call.
> > calling a pure memory write twice with the same pointer and same value
> > simply overwrites the same location with identical data, there is no
> > hardware interaction that could require repetition.
>
> Ok, great, perhaps put that in the changelog text?
>

ok, I'll amend it to the commit message and send a v4.

> > > Have you tested this?
> > >
> >
> > as noted in the cover letter, I don't have the hardware to test this.
>
> That's going to make doing code logic changes a bit hard for this
> driver, you might want to rethink this :)

mhm... okay, I'll keep that in mind. thanks :)

--
regards,
jose a. p-a