Re: [PATCH v2 2/6] media: mali-c55: Initialize the ISP in enable_streams()

[Jacopo Mondi]

Hi Dan

On Fri, Mar 13, 2026 at 04:14:09PM +0000, Dan Scally wrote:
> Hi Jacopo
>
> On 13/03/2026 16:10, Jacopo Mondi wrote:
> > Hi Dan
> >
> > On Fri, Mar 13, 2026 at 03:59:22PM +0000, Dan Scally wrote:
> > > Hi Jacopo - thanks for the patches
> > >
> > > On 13/03/2026 14:53, Jacopo Mondi wrote:
> > > > The Mali C55 driver initializes the ISP in two points:
> > > >
> > > > 1) At probe time it disables ISP blocks by configuring them in bypass
> > > >      mode
> > > > 2) At enable_streams() it initializes the crop rectangles and the image
> > > >      processing pipeline using the current image format
> > > >
> > > > However, as ISP blocks are configured by userspace, if their
> > > > configuration is not reset, from the second enable_streams() call
> > > > onwards the ISP configuration will depend on the previous streaming
> > > > session configuration.
> > > >
> > > > To re-initialize the ISP completely at enable_strems() time consolidate
> > >
> > > s/enable_strems/enable_streams
> > >
> > > > the ISP block bypass configuration and the image processing path
> > > > configuration in a single function to be called at enabled_streams()
> > > > time.
> > >
> > > I'm slightly confused; the change seems fine, but as far as I can see it's
> > > non-functional...or is this just preliminary reorganisation to make the next
> > > patch easier?
> >
> > Isn't mali_c55_init_context() only called at probe() time ?
> >
> > IOW all the bypass configuration that were only performed at probe
> > time are now performed at every streaming start.
> >
> > Or have I missed something ?
>
> But they're context rather than hardware writes; they're stored in the
> context's registers buffer and should persist past power off / on and then
> be re-written to the hardware when mali_c55_config_write() is called in
> mali_c55_isp_start() the next time streaming started...at least that's
> what's supposed to happen.

I'm not so concerned about power on/off but rather about configs from a
streaming session being applied to the following one.

In the case of the bypasses that are configured in
mali_c55_init_context(), at the moment there is no uAPI to enable
them.

However, rather soon we'll introduce blocks in the uAPI to enable
them, and I think there still is value in resetting the ISP
configuration in full at start streaming time. Do you agree ?


>
> Thanks
> Dan
>
> >
> > Thanks
> >    j
> >
> > >
> > > Thanks
> > > Dan
> > >
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > > > ---
> > > >    .../media/platform/arm/mali-c55/mali-c55-common.h  |  2 +
> > > >    .../media/platform/arm/mali-c55/mali-c55-core.c    | 35 -----------
> > > >    drivers/media/platform/arm/mali-c55/mali-c55-isp.c | 37 ++---------
> > > >    .../media/platform/arm/mali-c55/mali-c55-params.c  | 72 ++++++++++++++++++++++
> > > >    4 files changed, 79 insertions(+), 67 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-common.h b/drivers/media/platform/arm/mali-c55/mali-c55-common.h
> > > > index 31c1deaca146..13a3e9dc4243 100644
> > > > --- a/drivers/media/platform/arm/mali-c55/mali-c55-common.h
> > > > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-common.h
> > > > @@ -306,5 +306,7 @@ bool mali_c55_pipeline_ready(struct mali_c55 *mali_c55);
> > > >    void mali_c55_stats_fill_buffer(struct mali_c55 *mali_c55,
> > > >    				enum mali_c55_config_spaces cfg_space);
> > > >    void mali_c55_params_write_config(struct mali_c55 *mali_c55);
> > > > +void mali_c55_params_init_isp_config(struct mali_c55 *mali_c55,
> > > > +				     const struct v4l2_subdev_state *state);
> > > >    #endif /* _MALI_C55_COMMON_H */
> > > > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-core.c b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> > > > index 43b834459ccf..c1a562cd214e 100644
> > > > --- a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> > > > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> > > > @@ -663,41 +663,6 @@ static int mali_c55_init_context(struct mali_c55 *mali_c55,
> > > >    		      mali_c55->base + config_space_addrs[MALI_C55_CONFIG_PING],
> > > >    		      MALI_C55_CONFIG_SPACE_SIZE);
> > > > -	/*
> > > > -	 * Some features of the ISP need to be disabled by default and only
> > > > -	 * enabled at the same time as they're configured by a parameters buffer
> > > > -	 */
> > > > -
> > > > -	/* Bypass the sqrt and square compression and expansion modules */
> > > > -	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_BYPASS_1,
> > > > -				 MALI_C55_REG_BYPASS_1_FE_SQRT,
> > > > -				 MALI_C55_REG_BYPASS_1_FE_SQRT);
> > > > -	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_BYPASS_3,
> > > > -				 MALI_C55_REG_BYPASS_3_SQUARE_BE,
> > > > -				 MALI_C55_REG_BYPASS_3_SQUARE_BE);
> > > > -
> > > > -	/* Bypass the temper module */
> > > > -	mali_c55_ctx_write(mali_c55, MALI_C55_REG_BYPASS_2,
> > > > -			   MALI_C55_REG_BYPASS_2_TEMPER);
> > > > -
> > > > -	/* Disable the temper module's DMA read/write */
> > > > -	mali_c55_ctx_write(mali_c55, MALI_C55_REG_TEMPER_DMA_IO, 0x0);
> > > > -
> > > > -	/* Bypass the colour noise reduction  */
> > > > -	mali_c55_ctx_write(mali_c55, MALI_C55_REG_BYPASS_4,
> > > > -			   MALI_C55_REG_BYPASS_4_CNR);
> > > > -
> > > > -	/* Disable the sinter module */
> > > > -	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_SINTER_CONFIG,
> > > > -				 MALI_C55_SINTER_ENABLE_MASK, 0);
> > > > -
> > > > -	/* Disable the RGB Gamma module for each output */
> > > > -	mali_c55_ctx_write(mali_c55, MALI_C55_REG_FR_GAMMA_RGB_ENABLE, 0);
> > > > -	mali_c55_ctx_write(mali_c55, MALI_C55_REG_DS_GAMMA_RGB_ENABLE, 0);
> > > > -
> > > > -	/* Disable the colour correction matrix */
> > > > -	mali_c55_ctx_write(mali_c55, MALI_C55_REG_CCM_ENABLE, 0);
> > > > -
> > > >    	return 0;
> > > >    }
> > > > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-isp.c b/drivers/media/platform/arm/mali-c55/mali-c55-isp.c
> > > > index 497f25fbdd13..4c0fd1ec741c 100644
> > > > --- a/drivers/media/platform/arm/mali-c55/mali-c55-isp.c
> > > > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-isp.c
> > > > @@ -112,9 +112,6 @@ static int mali_c55_isp_start(struct mali_c55 *mali_c55,
> > > >    			      const struct v4l2_subdev_state *state)
> > > >    {
> > > >    	struct mali_c55_context *ctx = mali_c55_get_active_context(mali_c55);
> > > > -	const struct mali_c55_isp_format_info *cfg;
> > > > -	const struct v4l2_mbus_framefmt *format;
> > > > -	const struct v4l2_rect *crop;
> > > >    	u32 val;
> > > >    	int ret;
> > > > @@ -122,35 +119,11 @@ static int mali_c55_isp_start(struct mali_c55 *mali_c55,
> > > >    			     MALI_C55_REG_MCU_CONFIG_WRITE_MASK,
> > > >    			     MALI_C55_REG_MCU_CONFIG_WRITE_PING);
> > > > -	/* Apply input windowing */
> > > > -	crop = v4l2_subdev_state_get_crop(state, MALI_C55_ISP_PAD_SINK_VIDEO);
> > > > -	format = v4l2_subdev_state_get_format(state,
> > > > -					      MALI_C55_ISP_PAD_SINK_VIDEO);
> > > > -	cfg = mali_c55_isp_get_mbus_config_by_code(format->code);
> > > > -
> > > > -	mali_c55_write(mali_c55, MALI_C55_REG_HC_START,
> > > > -		       MALI_C55_HC_START(crop->left));
> > > > -	mali_c55_write(mali_c55, MALI_C55_REG_HC_SIZE,
> > > > -		       MALI_C55_HC_SIZE(crop->width));
> > > > -	mali_c55_write(mali_c55, MALI_C55_REG_VC_START_SIZE,
> > > > -		       MALI_C55_VC_START(crop->top) |
> > > > -		       MALI_C55_VC_SIZE(crop->height));
> > > > -	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_BASE_ADDR,
> > > > -				 MALI_C55_REG_ACTIVE_WIDTH_MASK, format->width);
> > > > -	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_BASE_ADDR,
> > > > -				 MALI_C55_REG_ACTIVE_HEIGHT_MASK,
> > > > -				 format->height << 16);
> > > > -	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_BAYER_ORDER,
> > > > -				 MALI_C55_BAYER_ORDER_MASK, cfg->order);
> > > > -	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_INPUT_WIDTH,
> > > > -				 MALI_C55_INPUT_WIDTH_MASK,
> > > > -				 MALI_C55_INPUT_WIDTH_20BIT);
> > > > -
> > > > -	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_ISP_RAW_BYPASS,
> > > > -				 MALI_C55_ISP_RAW_BYPASS_BYPASS_MASK,
> > > > -				 cfg->bypass ? MALI_C55_ISP_RAW_BYPASS_BYPASS_MASK :
> > > > -					     0x00);
> > > > -
> > > > +	/*
> > > > +	 * Apply default ISP configuration and the apply configurations from
> > > > +	 * the first available parameters buffer.
> > > > +	 */
> > > > +	mali_c55_params_init_isp_config(mali_c55, state);
> > > >    	mali_c55_params_write_config(mali_c55);
> > > >    	ret = mali_c55_config_write(ctx, MALI_C55_CONFIG_PING, true);
> > > >    	if (ret) {
> > > > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > > index c03a6120ddbf..c84a6047a570 100644
> > > > --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > > @@ -732,6 +732,78 @@ void mali_c55_params_write_config(struct mali_c55 *mali_c55)
> > > >    	vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> > > >    }
> > > > +void mali_c55_params_init_isp_config(struct mali_c55 *mali_c55,
> > > > +				     const struct v4l2_subdev_state *state)
> > > > +{
> > > > +	const struct mali_c55_isp_format_info *cfg;
> > > > +	const struct v4l2_mbus_framefmt *format;
> > > > +	const struct v4l2_rect *crop;
> > > > +
> > > > +	/* Apply input windowing */
> > > > +	crop = v4l2_subdev_state_get_crop(state, MALI_C55_ISP_PAD_SINK_VIDEO);
> > > > +	format = v4l2_subdev_state_get_format(state,
> > > > +					      MALI_C55_ISP_PAD_SINK_VIDEO);
> > > > +	cfg = mali_c55_isp_get_mbus_config_by_code(format->code);
> > > > +
> > > > +	mali_c55_write(mali_c55, MALI_C55_REG_HC_START,
> > > > +		       MALI_C55_HC_START(crop->left));
> > > > +	mali_c55_write(mali_c55, MALI_C55_REG_HC_SIZE,
> > > > +		       MALI_C55_HC_SIZE(crop->width));
> > > > +	mali_c55_write(mali_c55, MALI_C55_REG_VC_START_SIZE,
> > > > +		       MALI_C55_VC_START(crop->top) |
> > > > +		       MALI_C55_VC_SIZE(crop->height));
> > > > +	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_BASE_ADDR,
> > > > +				 MALI_C55_REG_ACTIVE_WIDTH_MASK, format->width);
> > > > +	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_BASE_ADDR,
> > > > +				 MALI_C55_REG_ACTIVE_HEIGHT_MASK,
> > > > +				 format->height << 16);
> > > > +	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_BAYER_ORDER,
> > > > +				 MALI_C55_BAYER_ORDER_MASK, cfg->order);
> > > > +	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_INPUT_WIDTH,
> > > > +				 MALI_C55_INPUT_WIDTH_MASK,
> > > > +				 MALI_C55_INPUT_WIDTH_20BIT);
> > > > +
> > > > +	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_ISP_RAW_BYPASS,
> > > > +				 MALI_C55_ISP_RAW_BYPASS_BYPASS_MASK,
> > > > +				 cfg->bypass ? MALI_C55_ISP_RAW_BYPASS_BYPASS_MASK :
> > > > +					     0x00);
> > > > +
> > > > +	/*
> > > > +	 * Some features of the ISP need to be disabled by default and only
> > > > +	 * enabled at the same time as they're configured by a parameters buffer
> > > > +	 */
> > > > +
> > > > +	/* Bypass the sqrt and square compression and expansion modules */
> > > > +	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_BYPASS_1,
> > > > +				 MALI_C55_REG_BYPASS_1_FE_SQRT,
> > > > +				 MALI_C55_REG_BYPASS_1_FE_SQRT);
> > > > +	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_BYPASS_3,
> > > > +				 MALI_C55_REG_BYPASS_3_SQUARE_BE,
> > > > +				 MALI_C55_REG_BYPASS_3_SQUARE_BE);
> > > > +
> > > > +	/* Bypass the temper module */
> > > > +	mali_c55_ctx_write(mali_c55, MALI_C55_REG_BYPASS_2,
> > > > +			   MALI_C55_REG_BYPASS_2_TEMPER);
> > > > +
> > > > +	/* Disable the temper module's DMA read/write */
> > > > +	mali_c55_ctx_write(mali_c55, MALI_C55_REG_TEMPER_DMA_IO, 0x0);
> > > > +
> > > > +	/* Bypass the colour noise reduction  */
> > > > +	mali_c55_ctx_write(mali_c55, MALI_C55_REG_BYPASS_4,
> > > > +			   MALI_C55_REG_BYPASS_4_CNR);
> > > > +
> > > > +	/* Disable the sinter module */
> > > > +	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_SINTER_CONFIG,
> > > > +				 MALI_C55_SINTER_ENABLE_MASK, 0);
> > > > +
> > > > +	/* Disable the RGB Gamma module for each output */
> > > > +	mali_c55_ctx_write(mali_c55, MALI_C55_REG_FR_GAMMA_RGB_ENABLE, 0);
> > > > +	mali_c55_ctx_write(mali_c55, MALI_C55_REG_DS_GAMMA_RGB_ENABLE, 0);
> > > > +
> > > > +	/* Disable the colour correction matrix */
> > > > +	mali_c55_ctx_write(mali_c55, MALI_C55_REG_CCM_ENABLE, 0);
> > > > +}
> > > > +
> > > >    void mali_c55_unregister_params(struct mali_c55 *mali_c55)
> > > >    {
> > > >    	struct mali_c55_params *params = &mali_c55->params;
> > > >
> > >
>