From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A9FD43890E8; Mon, 16 Mar 2026 09:34:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773653688; cv=none; b=heSTDAhmLyNa4LQ4PbXwrccHId6CNZScrNWVwpm7MDcSjsVM5LAdqyk/slAGO4bVn/cRxyQgdtavGSeiHX6+Ob/C19hxnW5RvhvXwENZTEf5GTsdgBC57vRXnlsdSjTzIYNAtPsiluDEbqSiLUHNLdrcX/HELv7SfyFnqhi6piw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773653688; c=relaxed/simple; bh=smKU7vgLEK28vakLCnFnMCy3X76eoGX0KAjuReHQaWQ=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=PjjzRG7y0gALUdN9O/do2PC78UWLbp1FpKPLpDcosDGbFulKY3HsTH2PU168Prp4NobVYwawH/3Z5fLqwdsyQ0FJTIRpW3jrcMowqTLVKjl/X2fWqGj9yVxxUhRqTGL/g1/e0TSerETCMOsj93L2PaeFSst1ZVRF9R5QcdzfZvg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=hCp1NwGR; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="hCp1NwGR" Received: from [192.168.0.43] (cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id CBA47B1A; Mon, 16 Mar 2026 10:33:30 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1773653611; bh=smKU7vgLEK28vakLCnFnMCy3X76eoGX0KAjuReHQaWQ=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=hCp1NwGRMhKGbyJ9V9AFpokbUikqM3O7lniwMzek3puleVV8h1yasTVoD7bIbtyWl +8wDnqDST9L5np6745HXoxpMSXCzZhF5J1jdcBFchS8Q8hI8RGcsVEuohgJzDB+Ahh MV3PvfO1djn3WyHWLIVrKG/Bf5XE09T3B0EAcqV8= Message-ID: <45348d8b-3c64-4716-ae8e-6211d001fb55@ideasonboard.com> Date: Mon, 16 Mar 2026 09:34:39 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 2/6] media: mali-c55: Initialize the ISP in enable_streams() To: Jacopo Mondi Cc: Anthony McGivern , Vincenzo Frascino , Nayden Kanchev , Konstantin Babin , =?UTF-8?Q?Barnab=C3=A1s_P=C5=91cze?= , Mauro Carvalho Chehab , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org References: <20260313-mali-c55-fixes-v7-0-v2-0-885c07961f30@ideasonboard.com> <20260313-mali-c55-fixes-v7-0-v2-2-885c07961f30@ideasonboard.com> <5c0f9a45-4726-4d42-b209-9d80108147ae@ideasonboard.com> Content-Language: en-US From: Dan Scally In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Morning Jacopo, happy Monday. On 13/03/2026 17:00, Jacopo Mondi wrote: > 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 ? Yes; and indeed when we add uAPI blocks that allow users to configure these particular registers they would need resetting at stream-start too, so: Reviewed-by: Daniel Scally > > >> >> Thanks >> Dan >> >>> >>> Thanks >>> j >>> >>>> >>>> Thanks >>>> Dan >>>> >>>>> Cc: stable@vger.kernel.org >>>>> Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver") >>>>> Signed-off-by: Jacopo Mondi >>>>> --- >>>>> .../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; >>>>> >>>> >>