Message-ID: <gehwgofhviqcnopaughxfcpsqmbbiaayid2scgat4xnd5ngwmo@ylawfiup2tqc>
Date: Thu, 29 Aug 2024 12:33:23 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: quic_dikshita@...cinc.com
Cc: Vikash Garodia <quic_vgarodia@...cinc.com>, 
	Abhinav Kumar <quic_abhinavk@...cinc.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>, 
	linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Vedang Nagar <quic_vnagar@...cinc.com>
Subject: Re: [PATCH v3 16/29] media: iris: implement iris v4l2_ctrl_ops and
 prepare capabilities

On Tue, Aug 27, 2024 at 03:35:41PM GMT, Dikshita Agarwal via B4 Relay wrote:
> From: Vedang Nagar <quic_vnagar@...cinc.com>
> 
> Implement s_ctrl and g_volatile_ctrl ctrl ops.
> Introduce platform specific driver and firmware capabilities.
> Capabilities are set of video specifications
> and features supported by a specific platform (SOC).
> Each capability is defined with min, max, range, default
> value and corresponding HFI.
> 
> Signed-off-by: Vedang Nagar <quic_vnagar@...cinc.com>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
> ---
>  drivers/media/platform/qcom/iris/Makefile          |   1 +
>  drivers/media/platform/qcom/iris/iris_buffer.c     |   3 +-
>  drivers/media/platform/qcom/iris/iris_core.h       |   2 +
>  drivers/media/platform/qcom/iris/iris_ctrls.c      | 194 +++++++++++++++++++++
>  drivers/media/platform/qcom/iris/iris_ctrls.h      |  15 ++
>  .../platform/qcom/iris/iris_hfi_gen1_defines.h     |   4 +
>  .../platform/qcom/iris/iris_hfi_gen2_command.c     |   1 +
>  .../platform/qcom/iris/iris_hfi_gen2_defines.h     |   9 +
>  drivers/media/platform/qcom/iris/iris_instance.h   |   6 +
>  .../platform/qcom/iris/iris_platform_common.h      |  71 ++++++++
>  .../platform/qcom/iris/iris_platform_sm8250.c      |  56 ++++++
>  .../platform/qcom/iris/iris_platform_sm8550.c      | 138 +++++++++++++++
>  drivers/media/platform/qcom/iris/iris_probe.c      |   7 +
>  drivers/media/platform/qcom/iris/iris_vdec.c       |  24 ++-
>  drivers/media/platform/qcom/iris/iris_vdec.h       |   2 +-
>  drivers/media/platform/qcom/iris/iris_vidc.c       |  16 +-
>  16 files changed, 536 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/Makefile b/drivers/media/platform/qcom/iris/Makefile
> index 9c50e29db41e..a746681e03cd 100644
> --- a/drivers/media/platform/qcom/iris/Makefile
> +++ b/drivers/media/platform/qcom/iris/Makefile
> @@ -1,5 +1,6 @@
>  iris-objs += iris_buffer.o \
>               iris_core.o \
> +             iris_ctrls.o \
>               iris_firmware.o \
>               iris_hfi_common.o \
>               iris_hfi_gen1_command.o \
> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c
> index a1017ceede7d..652117a19b45 100644
> --- a/drivers/media/platform/qcom/iris/iris_buffer.c
> +++ b/drivers/media/platform/qcom/iris/iris_buffer.c
> @@ -12,7 +12,6 @@
>  #define MAX_WIDTH 4096
>  #define MAX_HEIGHT 2304
>  #define NUM_MBS_4K (DIV_ROUND_UP(MAX_WIDTH, 16) * DIV_ROUND_UP(MAX_HEIGHT, 16))
> -#define BASE_RES_MB_MAX 138240
>  
>  /*
>   * NV12:
> @@ -74,7 +73,7 @@ static u32 iris_input_buffer_size(struct iris_inst *inst)
>  	num_mbs = iris_get_mbpf(inst);
>  	if (num_mbs > NUM_MBS_4K) {
>  		div_factor = 4;
> -		base_res_mbs = BASE_RES_MB_MAX;
> +		base_res_mbs = inst->driver_cap[MBPF].value;
>  	} else {
>  		base_res_mbs = NUM_MBS_4K;
>  		div_factor = 2;
> diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h
> index 1f6eca31928d..657d26a0fa2e 100644
> --- a/drivers/media/platform/qcom/iris/iris_core.h
> +++ b/drivers/media/platform/qcom/iris/iris_core.h
> @@ -58,6 +58,7 @@
>   * @intr_status: interrupt status
>   * @sys_error_handler: a delayed work for handling system fatal error
>   * @instances: a list_head of all instances
> + * @inst_fw_cap: an array of supported instance capabilities
>   */
>  
>  struct iris_core {
> @@ -97,6 +98,7 @@ struct iris_core {
>  	u32					intr_status;
>  	struct delayed_work			sys_error_handler;
>  	struct list_head			instances;
> +	struct platform_inst_fw_cap		inst_fw_cap[INST_FW_CAP_MAX];
>  };
>  
>  int iris_core_init(struct iris_core *core);
> diff --git a/drivers/media/platform/qcom/iris/iris_ctrls.c b/drivers/media/platform/qcom/iris/iris_ctrls.c
> new file mode 100644
> index 000000000000..868306d68a87
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_ctrls.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include "iris_ctrls.h"
> +#include "iris_instance.h"
> +
> +static bool iris_valid_cap_id(enum platform_inst_fw_cap_type cap_id)
> +{
> +	return cap_id >= 1 && cap_id < INST_FW_CAP_MAX;
> +}
> +
> +static enum platform_inst_fw_cap_type iris_get_cap_id(u32 id)
> +{
> +	switch (id) {
> +	case V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER:
> +		return DEBLOCK;
> +	case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
> +		return PROFILE;
> +	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
> +		return LEVEL;
> +	default:
> +		return INST_FW_CAP_MAX;
> +	}
> +}
> +
> +static u32 iris_get_v4l2_id(enum platform_inst_fw_cap_type cap_id)
> +{
> +	if (!iris_valid_cap_id(cap_id))
> +		return 0;
> +
> +	switch (cap_id) {
> +	case DEBLOCK:
> +		return V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER;
> +	case PROFILE:
> +		return V4L2_CID_MPEG_VIDEO_H264_PROFILE;
> +	case LEVEL:
> +		return V4L2_CID_MPEG_VIDEO_H264_LEVEL;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int iris_vdec_op_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	enum platform_inst_fw_cap_type cap_id;
> +	struct iris_inst *inst = NULL;
> +
> +	inst = container_of(ctrl->handler, struct iris_inst, ctrl_handler);
> +	switch (ctrl->id) {
> +	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
> +		ctrl->val = inst->buffers[BUF_OUTPUT].min_count;
> +		break;
> +	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
> +		ctrl->val = inst->buffers[BUF_INPUT].min_count;
> +		break;
> +	default:
> +		cap_id = iris_get_cap_id(ctrl->id);
> +		if (iris_valid_cap_id(cap_id))
> +			ctrl->val = inst->fw_cap[cap_id].value;
> +		else
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int iris_vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	enum platform_inst_fw_cap_type cap_id;
> +	struct platform_inst_fw_cap *cap;
> +	struct iris_inst *inst;
> +
> +	inst = container_of(ctrl->handler, struct iris_inst, ctrl_handler);
> +	cap = &inst->fw_cap[0];
> +
> +	cap_id = iris_get_cap_id(ctrl->id);
> +	if (!iris_valid_cap_id(cap_id))
> +		return -EINVAL;
> +
> +	cap[cap_id].flags |= CAP_FLAG_CLIENT_SET;
> +
> +	inst->fw_cap[cap_id].value = ctrl->val;
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops iris_ctrl_ops = {
> +	.s_ctrl = iris_vdec_op_s_ctrl,
> +	.g_volatile_ctrl = iris_vdec_op_g_volatile_ctrl,
> +};
> +
> +int iris_ctrls_init(struct iris_inst *inst)
> +{
> +	struct platform_inst_fw_cap *cap;
> +	int num_ctrls = 0, ctrl_idx = 0;
> +	int idx = 0, ret;
> +	u32 v4l2_id;
> +
> +	cap = &inst->fw_cap[0];
> +
> +	for (idx = 1; idx < INST_FW_CAP_MAX; idx++) {
> +		if (iris_get_v4l2_id(cap[idx].cap_id))
> +			num_ctrls++;
> +	}
> +	if (!num_ctrls)
> +		return -EINVAL;
> +
> +	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, num_ctrls);
> +	if (ret)
> +		return ret;
> +
> +	for (idx = 1; idx < INST_FW_CAP_MAX; idx++) {
> +		struct v4l2_ctrl *ctrl;
> +
> +		v4l2_id = iris_get_v4l2_id(cap[idx].cap_id);
> +		if (!v4l2_id)
> +			continue;
> +
> +		if (ctrl_idx >= num_ctrls) {
> +			ret = -EINVAL;
> +			goto error;
> +		}
> +
> +		if (cap[idx].flags & CAP_FLAG_MENU) {
> +			ctrl = v4l2_ctrl_new_std_menu(&inst->ctrl_handler,
> +						      &iris_ctrl_ops,
> +						      v4l2_id,
> +						      cap[idx].max,
> +						      ~(cap[idx].step_or_mask),
> +						      cap[idx].value);
> +		} else {
> +			ctrl = v4l2_ctrl_new_std(&inst->ctrl_handler,
> +						 &iris_ctrl_ops,
> +						 v4l2_id,
> +						 cap[idx].min,
> +						 cap[idx].max,
> +						 cap[idx].step_or_mask,
> +						 cap[idx].value);
> +		}
> +		if (!ctrl) {
> +			ret = -EINVAL;
> +			goto error;
> +		}
> +
> +		ret = inst->ctrl_handler.error;
> +		if (ret)
> +			goto error;
> +
> +		if ((cap[idx].flags & CAP_FLAG_VOLATILE) ||
> +		    (ctrl->id == V4L2_CID_MIN_BUFFERS_FOR_CAPTURE ||
> +		     ctrl->id == V4L2_CID_MIN_BUFFERS_FOR_OUTPUT))
> +			ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
> +
> +		ctrl->flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> +		ctrl_idx++;
> +	}
> +
> +	return 0;
> +error:
> +	v4l2_ctrl_handler_free(&inst->ctrl_handler);
> +
> +	return ret;
> +}
> +
> +int iris_session_init_caps(struct iris_core *core)
> +{
> +	struct platform_inst_fw_cap *inst_plat_cap_data;
> +	int i, num_inst_cap;
> +	u32 cap_id;
> +
> +	inst_plat_cap_data = core->iris_platform_data->inst_fw_cap_data;
> +	if (!inst_plat_cap_data)
> +		return -EINVAL;
> +
> +	num_inst_cap = core->iris_platform_data->inst_fw_cap_data_size;
> +
> +	for (i = 0; i < num_inst_cap && i < INST_FW_CAP_MAX - 1; i++) {

Drop the second condition

> +		cap_id = inst_plat_cap_data[i].cap_id;
> +		if (!iris_valid_cap_id(cap_id))
> +			continue;
> +
> +		core->inst_fw_cap[cap_id].cap_id = inst_plat_cap_data[i].cap_id;
> +		core->inst_fw_cap[cap_id].min = inst_plat_cap_data[i].min;
> +		core->inst_fw_cap[cap_id].max = inst_plat_cap_data[i].max;
> +		core->inst_fw_cap[cap_id].step_or_mask = inst_plat_cap_data[i].step_or_mask;
> +		core->inst_fw_cap[cap_id].value = inst_plat_cap_data[i].value;
> +		core->inst_fw_cap[cap_id].flags = inst_plat_cap_data[i].flags;
> +		core->inst_fw_cap[cap_id].hfi_id = inst_plat_cap_data[i].hfi_id;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/media/platform/qcom/iris/iris_ctrls.h b/drivers/media/platform/qcom/iris/iris_ctrls.h
> new file mode 100644
> index 000000000000..46e1da847aa8
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_ctrls.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _IRIS_CTRLS_H_
> +#define _IRIS_CTRLS_H_
> +
> +struct iris_core;
> +struct iris_inst;
> +
> +int iris_ctrls_init(struct iris_inst *inst);
> +int iris_session_init_caps(struct iris_core *core);
> +
> +#endif
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_defines.h b/drivers/media/platform/qcom/iris/iris_hfi_gen1_defines.h
> index da52e497b74a..9dc050063924 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_defines.h
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_defines.h
> @@ -31,9 +31,13 @@
>  #define HFI_EVENT_SYS_ERROR				0x1
>  #define HFI_EVENT_SESSION_ERROR				0x2
>  
> +#define HFI_PROPERTY_CONFIG_VDEC_POST_LOOP_DEBLOCKER	0x1200001
> +
>  #define HFI_PROPERTY_SYS_CODEC_POWER_PLANE_CTRL		0x5
>  #define HFI_PROPERTY_SYS_IMAGE_VERSION			0x6
>  
> +#define HFI_PROPERTY_PARAM_WORK_MODE			0x1015
> +#define HFI_PROPERTY_PARAM_WORK_ROUTE			0x1017
>  #define HFI_MSG_SYS_INIT				0x20001
>  #define HFI_MSG_SYS_SESSION_INIT			0x20006
>  #define HFI_MSG_SYS_SESSION_END				0x20007
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> index a74114b0761a..6ad2ca7be0f0 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> @@ -108,6 +108,7 @@ static int iris_hfi_gen2_session_set_default_header(struct iris_inst *inst)
>  	struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst);
>  	u32 default_header = false;
>  
> +	default_header = inst->fw_cap[DEFAULT_HEADER].value;

This isn't related to the s_ctrl and g_volatile_ctrl. Please split this
commit into the chunk that is actually related to that API and the rest
of the changes.

>  	iris_hfi_gen2_packet_session_property(inst,
>  					      HFI_PROP_DEC_DEFAULT_HEADER,
>  					      HFI_HOST_FLAGS_NONE,
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h b/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h
> index 18cc9365ab75..401df7b4e976 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h
> @@ -28,7 +28,16 @@
>  #define HFI_PROP_UBWC_BANK_SWZL_LEVEL3		0x03000008
>  #define HFI_PROP_UBWC_BANK_SPREADING		0x03000009
>  #define HFI_PROP_CODEC				0x03000100
> +#define HFI_PROP_PROFILE			0x03000107
> +#define HFI_PROP_LEVEL				0x03000108
> +#define HFI_PROP_STAGE				0x0300010a
> +#define HFI_PROP_PIPE				0x0300010b
> +#define HFI_PROP_LUMA_CHROMA_BIT_DEPTH		0x0300010f
> +#define HFI_PROP_CODED_FRAMES			0x03000120
> +#define HFI_PROP_BUFFER_HOST_MAX_COUNT		0x03000123
> +#define HFI_PROP_PIC_ORDER_CNT_TYPE		0x03000128
>  #define HFI_PROP_DEC_DEFAULT_HEADER		0x03000168
> +#define HFI_PROP_DEC_START_FROM_RAP_FRAME	0x03000169
>  #define HFI_PROP_END				0x03FFFFFF
>  
>  #define HFI_SESSION_ERROR_BEGIN			0x04000000
> diff --git a/drivers/media/platform/qcom/iris/iris_instance.h b/drivers/media/platform/qcom/iris/iris_instance.h
> index d28b8fd7ec2f..2429b9860789 100644
> --- a/drivers/media/platform/qcom/iris/iris_instance.h
> +++ b/drivers/media/platform/qcom/iris/iris_instance.h
> @@ -23,8 +23,11 @@
>   * @fh: reference of v4l2 file handler
>   * @fmt_src: structure of v4l2_format for source
>   * @fmt_dst: structure of v4l2_format for destination
> + * @ctrl_handler: reference of v4l2 ctrl handler
>   * @crop: structure of crop info
>   * @completions: structure of signal completions
> + * @driver_cap: array of supported instance driver capabilities
> + * @fw_cap: array of supported instance firmware capabilities
>   * @buffers: array of different iris buffers
>   * @fw_min_count: minimnum count of buffers needed by fw
>   * @once_per_session_set: boolean to set once per session property
> @@ -42,8 +45,11 @@ struct iris_inst {
>  	struct v4l2_fh			fh;
>  	struct v4l2_format		*fmt_src;
>  	struct v4l2_format		*fmt_dst;
> +	struct v4l2_ctrl_handler	ctrl_handler;
>  	struct iris_hfi_rect_desc	crop;
>  	struct completion		completion;
> +	struct platform_inst_driver_cap	driver_cap[INST_DRIVER_CAP_MAX];
> +	struct platform_inst_fw_cap	fw_cap[INST_FW_CAP_MAX];
>  	struct iris_buffers		buffers[BUF_TYPE_MAX];
>  	u32				fw_min_count;
>  	bool				once_per_session_set;
> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
> index 754cccc638a5..2935b769abb7 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
> @@ -10,6 +10,23 @@
>  #define HW_RESPONSE_TIMEOUT_VALUE               (1000) /* milliseconds */
>  #define AUTOSUSPEND_DELAY_VALUE			(HW_RESPONSE_TIMEOUT_VALUE + 500) /* milliseconds */
>  
> +#define REGISTER_BIT_DEPTH(luma, chroma)	((luma) << 16 | (chroma))
> +#define BIT_DEPTH_8				REGISTER_BIT_DEPTH(8, 8)
> +#define CODED_FRAMES_PROGRESSIVE		0x0
> +#define DEFAULT_MAX_HOST_BUF_COUNT		64
> +#define DEFAULT_MAX_HOST_BURST_BUF_COUNT	256
> +
> +enum stage_type {
> +	STAGE_1 = 1,
> +	STAGE_2 = 2,
> +};
> +
> +enum pipe_type {
> +	PIPE_1 = 1,
> +	PIPE_2 = 2,
> +	PIPE_4 = 4,
> +};
> +
>  extern struct iris_platform_data sm8550_data;
>  extern struct iris_platform_data sm8250_data;
>  
> @@ -41,6 +58,56 @@ struct ubwc_config_data {
>  	u32	bank_spreading;
>  };
>  
> +enum platform_inst_driver_cap_type {
> +	FRAME_WIDTH = 1,
> +	FRAME_HEIGHT,
> +	MBPF,
> +	INST_DRIVER_CAP_MAX,
> +};

Please use C structures for platform caps. You have introduced a
wrapping that 1:1 maps to C code, which is not iterated or otherwise
accessed via a generic ID aside from the driver code.

> +
> +enum platform_inst_fw_cap_type {
> +	PROFILE = 1,
> +	LEVEL,
> +	INPUT_BUF_HOST_MAX_COUNT,
> +	STAGE,
> +	PIPE,
> +	POC,
> +	CODED_FRAMES,
> +	BIT_DEPTH,
> +	DEFAULT_HEADER,
> +	RAP_FRAME,
> +	DEBLOCK,
> +	INST_FW_CAP_MAX,
> +};

I have mixed feelings towards fw caps. Let's see how the code evolves
after you split the commit into V4L2 CTRL code and the rest of the
changes.

> +
> +enum platform_inst_cap_flags {
> +	CAP_FLAG_NONE			= 0,

No need to define NONE, just skip the setting.

> +	CAP_FLAG_DYNAMIC_ALLOWED	= BIT(0),
> +	CAP_FLAG_MENU			= BIT(1),
> +	CAP_FLAG_INPUT_PORT		= BIT(2),
> +	CAP_FLAG_OUTPUT_PORT		= BIT(3),
> +	CAP_FLAG_CLIENT_SET		= BIT(4),
> +	CAP_FLAG_BITMASK		= BIT(5),
> +	CAP_FLAG_VOLATILE		= BIT(6),
> +};
> +
> +struct platform_inst_driver_cap {
> +	enum platform_inst_driver_cap_type cap_id;
> +	u32 min;
> +	u32 max;
> +	u32 value;
> +};
> +
> +struct platform_inst_fw_cap {
> +	enum platform_inst_fw_cap_type cap_id;
> +	s64 min;
> +	s64 max;
> +	s64 step_or_mask;
> +	s64 value;
> +	u32 hfi_id;
> +	enum platform_inst_cap_flags flags;
> +};
> +


-- 
With best wishes
Dmitry

Powered by blists - more mailing lists