[PATCH AUTOSEL 6.19-6.1] media: rkisp1: Fix filter mode register configuration

[PATCH AUTOSEL 6.19-6.1] media: rkisp1: Fix filter mode register configuration

[Sasha Levin]

From: Rui Wang <rui.wang@ideasonboard.com>

[ Upstream commit 5a50f2b61104d0d351b59ec179f67abab7870453 ]

The rkisp1_flt_config() function performs an initial direct write to
RKISP1_CIF_ISP_FILT_MODE without including the RKISP1_CIF_ISP_FLT_ENA
bit, which clears the filter enable bit in the hardware.

The subsequent read/modify/write sequence then reads back the register
with the enable bit already cleared and cannot restore it, resulting in
the filter being inadvertently disabled.

Remove the redundant direct write. The read/modify/write sequence alone
correctly preserves the existing enable bit state while updating the
DNR mode and filter configuration bits.

Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Link: https://patch.msgid.link/20260105171142.147792-2-rui.wang@ideasonboard.com
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Analysis

### Commit Message Analysis

The commit message clearly describes a **bug fix**: the
`rkisp1_flt_config()` function performs an initial direct write to the
`RKISP1_CIF_ISP_FILT_MODE` register that **clears the filter enable
bit** (`RKISP1_CIF_ISP_FLT_ENA`). The subsequent read/modify/write
sequence reads back the register with the enable bit already cleared and
cannot restore it. This results in the **ISP filter being inadvertently
disabled** whenever its configuration is updated.

The fix removes the redundant direct write, leaving only the
read/modify/write sequence that correctly preserves the enable bit.

### Code Change Analysis

The diff is straightforward:

**Removed code (the bug):**
```c
rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_FILT_MODE,
     (arg->mode ? RKISP1_CIF_ISP_FLT_MODE_DNR : 0) |
     RKISP1_CIF_ISP_FLT_CHROMA_V_MODE(arg->chr_v_mode) |
     RKISP1_CIF_ISP_FLT_CHROMA_H_MODE(arg->chr_h_mode) |
     RKISP1_CIF_ISP_FLT_GREEN_STAGE1(arg->grn_stage1));
```

This write sets the mode register but does **not** include the
`RKISP1_CIF_ISP_FLT_ENA` bit, effectively disabling the filter in
hardware.

**Preserved code (the correct path):**
```c
filt_mode = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_FILT_MODE);
filt_mode &= RKISP1_CIF_ISP_FLT_ENA;
// ... build up new mode value preserving enable bit ...
rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_FILT_MODE, filt_mode);
```

The read/modify/write sequence correctly reads the current register
value, preserves only the enable bit, then OR's in the new
configuration. But because the direct write above already cleared the
enable bit, reading it back gets a 0 for the enable bit - defeating the
entire purpose of the read/modify/write pattern.

### Bug Mechanism

1. Filter is enabled (FLT_ENA bit = 1 in hardware register)
2. User updates filter configuration parameters
3. `rkisp1_flt_config()` is called
4. Direct write clears FLT_ENA bit (writes mode without enable)
5. Read/modify/write reads back register with FLT_ENA = 0
6. Final write does not have FLT_ENA set
7. **Filter is now disabled** even though user only wanted to change
   configuration

This is a real functional bug that affects image processing quality on
Rockchip platforms using the ISP (Image Signal Processor).

### Scope and Risk

- **Size**: Removal of 5 lines of code. Extremely small and surgical.
- **Files**: Single file change (`rkisp1-params.c`)
- **Risk**: Very low. The removed code is entirely redundant - the
  read/modify/write sequence that follows does everything the direct
  write did, plus correctly preserves the enable bit. Removing the
  direct write can only improve behavior.
- **Subsystem**: Media/camera driver for Rockchip ISP - well-defined
  scope, won't affect other subsystems.

### Review Quality

The commit has been reviewed by three experienced media subsystem
developers:
- Stefan Klug (Reviewed-by)
- Kieran Bingham (Reviewed-by)
- Laurent Pinchart (Reviewed-by, also signed off as maintainer)

Merged by Hans Verkuil, the V4L2 subsystem maintainer.

### User Impact

This bug affects anyone using the Rockchip ISP filter (noise reduction,
sharpening) on platforms like RK3399 and similar SoCs. When the filter
configuration is updated, the filter gets inadvertently disabled,
leading to degraded image quality. This is particularly relevant for
embedded systems and cameras that use stable kernels.

### Stable Criteria Assessment

- **Obviously correct**: Yes - removing a redundant write that defeats
  the read/modify/write pattern is clearly correct
- **Fixes a real bug**: Yes - filter being disabled when reconfigured is
  a real functional bug
- **Small and contained**: Yes - 5 lines removed from a single function
  in a single file
- **No new features**: Correct - pure bug fix
- **Tested**: Multiple reviews from domain experts

**YES**

 drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index c9f88635224cc..6442436a5e428 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -411,12 +411,6 @@ static void rkisp1_flt_config(struct rkisp1_params *params,
 	rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_FILT_LUM_WEIGHT,
 		     arg->lum_weight);
 
-	rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_FILT_MODE,
-		     (arg->mode ? RKISP1_CIF_ISP_FLT_MODE_DNR : 0) |
-		     RKISP1_CIF_ISP_FLT_CHROMA_V_MODE(arg->chr_v_mode) |
-		     RKISP1_CIF_ISP_FLT_CHROMA_H_MODE(arg->chr_h_mode) |
-		     RKISP1_CIF_ISP_FLT_GREEN_STAGE1(arg->grn_stage1));
-
 	/* avoid to override the old enable value */
 	filt_mode = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_FILT_MODE);
 	filt_mode &= RKISP1_CIF_ISP_FLT_ENA;
-- 
2.51.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip