Received-SPF: pass (google.com: domain of linux-kernel+bounces-5121-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <6b2ce264-3cc6-4c00-85c8-c885b2707787@linaro.org>
Date: Tue, 19 Dec 2023 11:57:03 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 14/34] media: iris: implement iris v4l2 file ops
Content-Language: en-US
To: Dikshita Agarwal <quic_dikshita@quicinc.com>,
 linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
 stanimir.k.varbanov@gmail.com, quic_vgarodia@quicinc.com, agross@kernel.org,
 andersson@kernel.org, konrad.dybcio@linaro.org, mchehab@kernel.org
Cc: linux-arm-msm@vger.kernel.org, quic_abhinavk@quicinc.com
References: <1702899149-21321-1-git-send-email-quic_dikshita@quicinc.com>
 <1702899149-21321-15-git-send-email-quic_dikshita@quicinc.com>
From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
In-Reply-To: <1702899149-21321-15-git-send-email-quic_dikshita@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 18/12/2023 11:32, Dikshita Agarwal wrote:
> From: Vikash Garodia <quic_vgarodia@quicinc.com>
> 
> Implements iris v4l2 file ops - open, close and poll.
> 
> Open:
> Configure the vb2 queue and v4l2 file handler. Initialize
> a video instance and add the instance to core instance list.
> Open a session in video firmware via HFI_CMD_OPEN.
> 
> Close:
> De-initialize the instance and remove it from core
> instance list. Close the session in firmware via HFI_CMD_CLOSE.
> 
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
>   drivers/media/platform/qcom/vcodec/iris/Makefile   |   2 +
>   .../media/platform/qcom/vcodec/iris/hfi_defines.h  |   2 +
>   .../media/platform/qcom/vcodec/iris/iris_common.h  |  26 ++
>   .../media/platform/qcom/vcodec/iris/iris_core.c    |  42 ++-
>   .../media/platform/qcom/vcodec/iris/iris_core.h    |   8 +
>   .../media/platform/qcom/vcodec/iris/iris_helpers.c |  62 +++++
>   .../media/platform/qcom/vcodec/iris/iris_helpers.h |   2 +
>   drivers/media/platform/qcom/vcodec/iris/iris_hfi.c | 115 +++++++-
>   drivers/media/platform/qcom/vcodec/iris/iris_hfi.h |   3 +
>   .../platform/qcom/vcodec/iris/iris_hfi_packet.c    |  28 ++
>   .../platform/qcom/vcodec/iris/iris_hfi_packet.h    |   7 +
>   .../platform/qcom/vcodec/iris/iris_instance.h      |  48 ++++
>   .../media/platform/qcom/vcodec/iris/iris_probe.c   |  10 +
>   .../media/platform/qcom/vcodec/iris/iris_vdec.c    |  47 ++++
>   .../media/platform/qcom/vcodec/iris/iris_vdec.h    |  14 +
>   .../media/platform/qcom/vcodec/iris/iris_vidc.c    | 298 +++++++++++++++++++++
>   .../media/platform/qcom/vcodec/iris/iris_vidc.h    |  15 ++
>   17 files changed, 719 insertions(+), 10 deletions(-)
>   create mode 100644 drivers/media/platform/qcom/vcodec/iris/iris_common.h
>   create mode 100644 drivers/media/platform/qcom/vcodec/iris/iris_instance.h
>   create mode 100644 drivers/media/platform/qcom/vcodec/iris/iris_vdec.c
>   create mode 100644 drivers/media/platform/qcom/vcodec/iris/iris_vdec.h
>   create mode 100644 drivers/media/platform/qcom/vcodec/iris/iris_vidc.c
>   create mode 100644 drivers/media/platform/qcom/vcodec/iris/iris_vidc.h
> 
> diff --git a/drivers/media/platform/qcom/vcodec/iris/Makefile b/drivers/media/platform/qcom/vcodec/iris/Makefile
> index c50e3241..3c076d0 100644
> --- a/drivers/media/platform/qcom/vcodec/iris/Makefile
> +++ b/drivers/media/platform/qcom/vcodec/iris/Makefile
> @@ -3,6 +3,8 @@ iris-objs += ../hfi_queue.o ../firmware.o
>   iris-objs += iris_probe.o \
>                iris_state.o \
>                iris_core.o \
> +             iris_vidc.o \
> +             iris_vdec.o \
>                iris_state.o \
>                iris_helpers.o \
>                iris_hfi.o \
> diff --git a/drivers/media/platform/qcom/vcodec/iris/hfi_defines.h b/drivers/media/platform/qcom/vcodec/iris/hfi_defines.h
> index fb383b2..423ba1a 100644
> --- a/drivers/media/platform/qcom/vcodec/iris/hfi_defines.h
> +++ b/drivers/media/platform/qcom/vcodec/iris/hfi_defines.h
> @@ -9,6 +9,8 @@
>   #define HFI_VIDEO_ARCH_LX			0x1
>   
>   #define HFI_CMD_INIT				0x01000001
> +#define HFI_CMD_OPEN				0x01000003
> +#define HFI_CMD_CLOSE				0x01000004
>   
>   #define HFI_PROP_IMAGE_VERSION			0x03000001
>   
> diff --git a/drivers/media/platform/qcom/vcodec/iris/iris_common.h b/drivers/media/platform/qcom/vcodec/iris/iris_common.h
> new file mode 100644
> index 0000000..3e4dd71
> --- /dev/null
> +++ b/drivers/media/platform/qcom/vcodec/iris/iris_common.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#ifndef _IRIS_COMMON_H_
> +#define _IRIS_COMMON_H_
> +
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +#define INPUT_MPLANE V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> +#define OUTPUT_MPLANE V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> +#define DEFAULT_WIDTH 320
> +#define DEFAULT_HEIGHT 240
> +#define DEFAULT_BUF_SIZE 115200
> +
> +enum signal_session_response {
> +	SIGNAL_CMD_STOP_INPUT = 0,
> +	SIGNAL_CMD_STOP_OUTPUT,
> +	SIGNAL_CMD_CLOSE,
> +	MAX_SIGNAL,
> +};
> +
> +#endif
> diff --git a/drivers/media/platform/qcom/vcodec/iris/iris_core.c b/drivers/media/platform/qcom/vcodec/iris/iris_core.c
> index ba8960d..174fad9 100644
> --- a/drivers/media/platform/qcom/vcodec/iris/iris_core.c
> +++ b/drivers/media/platform/qcom/vcodec/iris/iris_core.c
> @@ -3,12 +3,14 @@
>    * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
>    */
>   
> +#include <linux/delay.h>
> +
>   #include "iris_core.h"
>   #include "iris_helpers.h"
>   #include "iris_hfi.h"
>   #include "iris_state.h"
>   
> -static int iris_core_deinit_locked(struct iris_core *core)
> +int iris_core_deinit_locked(struct iris_core *core)
>   {
>   	int ret;
>   
> @@ -68,3 +70,41 @@ int iris_core_init(struct iris_core *core)
>   
>   	return ret;
>   }
> +
> +int iris_core_init_wait(struct iris_core *core)
> +{
> +	const int interval = 10;
> +	int max_tries, count = 0, ret = 0;
> +
> +	mutex_lock(&core->lock);
> +	if (!core_in_valid_state(core)) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	if (core->state == IRIS_CORE_INIT)
> +		goto unlock;
> +
> +	max_tries = core->cap[HW_RESPONSE_TIMEOUT].value / interval;
> +	while (count < max_tries) {
> +		if (core->state != IRIS_CORE_INIT_WAIT)
> +			break;
> +		msleep(interval);
> +		count++;
> +	}

Is the intention here really to poll and sleep for 10 milliseconds and 
if so why ?

Shouldn't there be some sort of bootup IRQ or event that we can trap ?

That's how it works in the venus driver right ? Just kick a workqueue 
when the IRQ fires and have the iris_core_init_wait() do a 
wait_for_completion_timeout().


> +int iris_hfi_session_open(struct iris_inst *inst)
> +{
> +	struct iris_core *core;
> +	int ret;
> +
> +	inst->packet_size = 4096;
> +	inst->packet = kzalloc(inst->packet_size, GFP_KERNEL);
> +	if (!inst->packet)
> +		return -ENOMEM;

Who is responsible for cleaning up this allocation ?

> +
> +fail_free_packet:
> +	kfree(inst->packet);

freed here.

> +int iris_hfi_session_close(struct iris_inst *inst)
> +{
>  }

but not here ?

Still finding this alot of code to go through.

Please try to break this up into smaller series.

---
bod