[PATCH v1 3/6] media: rkisp1-isp: Add target_format

Isaac Scott posted 6 patches 4 days, 21 hours ago
There is a newer version of this series
[PATCH v1 3/6] media: rkisp1-isp: Add target_format
Posted by Isaac Scott 4 days, 21 hours ago
In passthough mode, we want to make sure the output format of the ISP is
identical to the input format. Add logic to ensure the format we
configure the output of the ISP with is the sink format when we are in
bypass, and the source format if we are not.

Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index f636d738b7e8..716bd7f3c66d 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -175,9 +175,10 @@ static void rkisp1_gasket_disable(struct rkisp1_device *rkisp1)
 static void rkisp1_config_ism(struct rkisp1_isp *isp,
 			      const struct v4l2_subdev_state *sd_state)
 {
-	const struct v4l2_rect *src_crop =
+	const struct v4l2_rect *target_crop =
 		v4l2_subdev_state_get_crop(sd_state,
-					   RKISP1_ISP_PAD_SOURCE_VIDEO);
+			(isp->rkisp1->in_bypass ?
+				RKISP1_ISP_PAD_SINK_VIDEO : RKISP1_ISP_PAD_SOURCE_VIDEO));
 	struct rkisp1_device *rkisp1 = isp->rkisp1;
 	u32 val;
 
@@ -185,10 +186,10 @@ static void rkisp1_config_ism(struct rkisp1_isp *isp,
 	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_MAX_DX, 0);
 	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_MAX_DY, 0);
 	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_DISPLACE, 0);
-	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_H_OFFS, src_crop->left);
-	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_V_OFFS, src_crop->top);
-	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_H_SIZE, src_crop->width);
-	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_V_SIZE, src_crop->height);
+	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_H_OFFS, target_crop->left);
+	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_V_OFFS, target_crop->top);
+	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_H_SIZE, target_crop->width);
+	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_V_SIZE, target_crop->height);
 
 	/* IS(Image Stabilization) is always on, working as output crop */
 	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_CTRL, 1);
-- 
2.43.0
Re: [PATCH v1 3/6] media: rkisp1-isp: Add target_format
Posted by Laurent Pinchart 4 days, 21 hours ago
On Wed, Feb 04, 2026 at 11:25:03AM +0000, Isaac Scott wrote:
> In passthough mode, we want to make sure the output format of the ISP is
> identical to the input format. Add logic to ensure the format we
> configure the output of the ISP with is the sink format when we are in
> bypass, and the source format if we are not.

I don't think that's right. In bypass mode, userspace needs to configure
the same format on the image sink and image source pads of the ISP, and
not set any cropping. The crop rectangle on the source pad should
contain the information you need.

> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> ---
>  drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> index f636d738b7e8..716bd7f3c66d 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> @@ -175,9 +175,10 @@ static void rkisp1_gasket_disable(struct rkisp1_device *rkisp1)
>  static void rkisp1_config_ism(struct rkisp1_isp *isp,
>  			      const struct v4l2_subdev_state *sd_state)
>  {
> -	const struct v4l2_rect *src_crop =
> +	const struct v4l2_rect *target_crop =
>  		v4l2_subdev_state_get_crop(sd_state,
> -					   RKISP1_ISP_PAD_SOURCE_VIDEO);
> +			(isp->rkisp1->in_bypass ?
> +				RKISP1_ISP_PAD_SINK_VIDEO : RKISP1_ISP_PAD_SOURCE_VIDEO));
>  	struct rkisp1_device *rkisp1 = isp->rkisp1;
>  	u32 val;
>  
> @@ -185,10 +186,10 @@ static void rkisp1_config_ism(struct rkisp1_isp *isp,
>  	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_MAX_DX, 0);
>  	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_MAX_DY, 0);
>  	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_DISPLACE, 0);
> -	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_H_OFFS, src_crop->left);
> -	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_V_OFFS, src_crop->top);
> -	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_H_SIZE, src_crop->width);
> -	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_V_SIZE, src_crop->height);
> +	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_H_OFFS, target_crop->left);
> +	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_V_OFFS, target_crop->top);
> +	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_H_SIZE, target_crop->width);
> +	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_V_SIZE, target_crop->height);
>  
>  	/* IS(Image Stabilization) is always on, working as output crop */
>  	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_CTRL, 1);

-- 
Regards,

Laurent Pinchart
Re: [PATCH v1 3/6] media: rkisp1-isp: Add target_format
Posted by Isaac Scott 4 days, 19 hours ago
Hi Laurent,

Thank you for the review!

Quoting Laurent Pinchart (2026-02-04 11:37:10)
> On Wed, Feb 04, 2026 at 11:25:03AM +0000, Isaac Scott wrote:
> > In passthough mode, we want to make sure the output format of the ISP is
> > identical to the input format. Add logic to ensure the format we
> > configure the output of the ISP with is the sink format when we are in
> > bypass, and the source format if we are not.
> 
> I don't think that's right. In bypass mode, userspace needs to configure
> the same format on the image sink and image source pads of the ISP, and
> not set any cropping. The crop rectangle on the source pad should
> contain the information you need.
> 

Yes, that makes sense. I suppose the idea here was to "enforce" it, but
perhaps we shouldn't be potentially overwriting a format userspace is
setting. I'll take another look for v2.

Best wishes,
Isaac

> > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> > ---
> >  drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > index f636d738b7e8..716bd7f3c66d 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > @@ -175,9 +175,10 @@ static void rkisp1_gasket_disable(struct rkisp1_device *rkisp1)
> >  static void rkisp1_config_ism(struct rkisp1_isp *isp,
> >                             const struct v4l2_subdev_state *sd_state)
> >  {
> > -     const struct v4l2_rect *src_crop =
> > +     const struct v4l2_rect *target_crop =
> >               v4l2_subdev_state_get_crop(sd_state,
> > -                                        RKISP1_ISP_PAD_SOURCE_VIDEO);
> > +                     (isp->rkisp1->in_bypass ?
> > +                             RKISP1_ISP_PAD_SINK_VIDEO : RKISP1_ISP_PAD_SOURCE_VIDEO));
> >       struct rkisp1_device *rkisp1 = isp->rkisp1;
> >       u32 val;
> >  
> > @@ -185,10 +186,10 @@ static void rkisp1_config_ism(struct rkisp1_isp *isp,
> >       rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_MAX_DX, 0);
> >       rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_MAX_DY, 0);
> >       rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_DISPLACE, 0);
> > -     rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_H_OFFS, src_crop->left);
> > -     rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_V_OFFS, src_crop->top);
> > -     rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_H_SIZE, src_crop->width);
> > -     rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_V_SIZE, src_crop->height);
> > +     rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_H_OFFS, target_crop->left);
> > +     rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_V_OFFS, target_crop->top);
> > +     rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_H_SIZE, target_crop->width);
> > +     rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_V_SIZE, target_crop->height);
> >  
> >       /* IS(Image Stabilization) is always on, working as output crop */
> >       rkisp1_write(rkisp1, RKISP1_CIF_ISP_IS_CTRL, 1);
> 
> -- 
> Regards,
> 
> Laurent Pinchart