Hi Dan,
Thank you for the patch.
On Tue, Jul 09, 2024 at 02:29:01PM +0100, Daniel Scally wrote:
> On ISP_START, fill the stats buffer by reading out the metering space
> in the ISP's memory. This is done for the non-active config just as
> the dma transfer of the registers is. To acheive that, move the
s/acheive/achieve/
> checking of the current config outside of mali_c55_swap_next_config()
> so we can use it for both functions.
>
> Acked-by: Nayden Kanchev <nayden.kanchev@xxxxxxx>
> Co-developed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> ---
> Changes in v6:
>
> - None
>
> Changes in v5:
>
> - New patch
>
> .../platform/arm/mali-c55/mali-c55-core.c | 34 ++++++++++++++-----
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> 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 eedc8f450184..ed0db34767a4 100644
> --- a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> @@ -567,15 +567,9 @@ static int mali_c55_check_hwcfg(struct mali_c55 *mali_c55)
> return 0;
> }
>
> -static void mali_c55_swap_next_config(struct mali_c55 *mali_c55)
> +static void mali_c55_swap_next_config(struct mali_c55 *mali_c55, u32 next_config)
> {
> struct mali_c55_context *ctx = mali_c55_get_active_context(mali_c55);
> - u32 curr_config, next_config;
> -
> - curr_config = mali_c55_read(mali_c55, MALI_C55_REG_PING_PONG_READ);
> - curr_config = (curr_config & MALI_C55_REG_PING_PONG_READ_MASK)
> - >> (ffs(MALI_C55_REG_PING_PONG_READ_MASK) - 1);
> - next_config = curr_config ^ 1;
>
> mali_c55_update_bits(mali_c55, MALI_C55_REG_MCU_CONFIG,
> MALI_C55_REG_MCU_CONFIG_WRITE_MASK,
> @@ -588,6 +582,7 @@ static irqreturn_t mali_c55_isr(int irq, void *context)
> {
> struct device *dev = context;
> struct mali_c55 *mali_c55 = dev_get_drvdata(dev);
> + u32 curr_config, next_config;
> u32 interrupt_status;
> unsigned int i, j;
>
> @@ -612,7 +607,30 @@ static irqreturn_t mali_c55_isr(int irq, void *context)
> for (j = i; j < MALI_C55_NUM_CAP_DEVS; j++)
> mali_c55_set_next_buffer(&mali_c55->cap_devs[j]);
>
> - mali_c55_swap_next_config(mali_c55);
> + /*
> + * When the ISP starts a frame we have some work to do:
> + *
> + * 1. Copy over the config for the **next** frame
> + * 2. Read out the metering stats for the **last** frame
> + */
> +
> + curr_config = mali_c55_read(mali_c55,
> + MALI_C55_REG_PING_PONG_READ);
> + curr_config &= MALI_C55_REG_PING_PONG_READ_MASK;
> + curr_config >>= ffs(MALI_C55_REG_PING_PONG_READ_MASK) - 1;
> + next_config = curr_config ^ 1;
> +
> + /*
> + * The ordering of these two is currently important as
> + * mali_c55_stats_fill_buffer() is asynchronous whereas
> + * mali_c55_swap_next_config() is not.
> + *
> + * TODO: Should mali_c55_swap_next_config() be async?
Isn't it in this version of the series, at least when using DMA ?
As I wrote in the review of 07/18, I think the reconfiguration probably
needs more careful consideration.
> + */
> + mali_c55_stats_fill_buffer(mali_c55,
> + next_config ? MALI_C55_CONFIG_PING :
> + MALI_C55_CONFIG_PONG);
> + mali_c55_swap_next_config(mali_c55, next_config);
>
> break;
> case MALI_C55_IRQ_ISP_DONE:
--
Regards,
Laurent Pinchart
