| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <328c6b00-787e-7372-11a2-5a95e9e7754a@quicinc.com>
Date: Thu, 6 Mar 2025 17:46:58 +0530
From: Dikshita Agarwal <quic_dikshita@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, <quic_vgarodia@...cinc.com>,
<quic_abhinavk@...cinc.com>, <mchehab@...nel.org>
CC: <hverkuil@...all.nl>, <linux-media@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 03/12] media: iris: Set mandatory properties for HEVC
and VP9 decoders.
On 3/6/2025 6:22 AM, Bryan O'Donoghue wrote:
> On 05/03/2025 10:43, Dikshita Agarwal wrote:
>> Subscribe and set mandatory properties to the firmware for HEVC and VP9
>> decoders.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
>> ---
>> .../platform/qcom/iris/iris_hfi_common.h | 1 +
>> .../qcom/iris/iris_hfi_gen1_command.c | 4 +-
>> .../qcom/iris/iris_hfi_gen2_command.c | 83 +++++++++++++++++--
>> .../qcom/iris/iris_hfi_gen2_response.c | 7 ++
>> .../platform/qcom/iris/iris_platform_common.h | 16 +++-
>> .../platform/qcom/iris/iris_platform_sm8250.c | 4 +-
>> .../platform/qcom/iris/iris_platform_sm8550.c | 61 ++++++++++++--
>> 7 files changed, 151 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_common.h
>> b/drivers/media/platform/qcom/iris/iris_hfi_common.h
>> index b2c541367fc6..9e6aadb83783 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_common.h
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_common.h
>> @@ -140,6 +140,7 @@ struct hfi_subscription_params {
>> u32 color_info;
>> u32 profile;
>> u32 level;
>> + u32 tier;
>> };
>> u32 iris_hfi_get_v4l2_color_primaries(u32 hfi_primaries);
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
>> index 1e774b058ab9..a160ae915886 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
>> @@ -784,8 +784,8 @@ static int
>> iris_hfi_gen1_session_set_config_params(struct iris_inst *inst, u32 p
>> iris_hfi_gen1_set_bufsize},
>> };
>> - config_params = core->iris_platform_data->input_config_params;
>> - config_params_size =
>> core->iris_platform_data->input_config_params_size;
>> + config_params = core->iris_platform_data->input_config_params_default;
>> + config_params_size =
>> core->iris_platform_data->input_config_params_default_size;
>> if (V4L2_TYPE_IS_OUTPUT(plane)) {
>> for (i = 0; i < config_params_size; i++) {
>> 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 a3ebcda9a2ba..5b4c89184297 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
>> @@ -192,7 +192,7 @@ static int iris_hfi_gen2_set_crop_offsets(struct
>> iris_inst *inst)
>> sizeof(u64));
>> }
>> -static int iris_hfi_gen2_set_bit_dpeth(struct iris_inst *inst)
>> +static int iris_hfi_gen2_set_bit_depth(struct iris_inst *inst)
>> {
>> struct iris_inst_hfi_gen2 *inst_hfi_gen2 =
>> to_iris_inst_hfi_gen2(inst);
>> u32 port = iris_hfi_gen2_get_port(V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
>> @@ -407,6 +407,23 @@ static int
>> iris_hfi_gen2_set_linear_stride_scanline(struct iris_inst *inst)
>> sizeof(u64));
>> }
>> +static int iris_hfi_gen2_set_tier(struct iris_inst *inst)
>> +{
>> + struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst);
>> + u32 port = iris_hfi_gen2_get_port(V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
>> + u32 tier = inst->fw_caps[TIER].value;
>> +
>> + inst_hfi_gen2->src_subcr_params.tier = tier;
>> +
>> + return iris_hfi_gen2_session_set_property(inst,
>> + HFI_PROP_TIER,
>> + HFI_HOST_FLAGS_NONE,
>> + port,
>> + HFI_PAYLOAD_U32_ENUM,
>> + &tier,
>> + sizeof(u32));
>> +}
>> +
>> static int iris_hfi_gen2_session_set_config_params(struct iris_inst
>> *inst, u32 plane)
>> {
>> struct iris_core *core = inst->core;
>> @@ -418,7 +435,7 @@ static int
>> iris_hfi_gen2_session_set_config_params(struct iris_inst *inst, u32 p
>> {HFI_PROP_BITSTREAM_RESOLUTION,
>> iris_hfi_gen2_set_bitstream_resolution },
>> {HFI_PROP_CROP_OFFSETS,
>> iris_hfi_gen2_set_crop_offsets },
>> {HFI_PROP_CODED_FRAMES,
>> iris_hfi_gen2_set_coded_frames },
>> - {HFI_PROP_LUMA_CHROMA_BIT_DEPTH,
>> iris_hfi_gen2_set_bit_dpeth },
>> + {HFI_PROP_LUMA_CHROMA_BIT_DEPTH,
>> iris_hfi_gen2_set_bit_depth },
>> {HFI_PROP_BUFFER_FW_MIN_OUTPUT_COUNT,
>> iris_hfi_gen2_set_min_output_count },
>> {HFI_PROP_PIC_ORDER_CNT_TYPE,
>> iris_hfi_gen2_set_picture_order_count },
>> {HFI_PROP_SIGNAL_COLOR_INFO,
>> iris_hfi_gen2_set_colorspace },
>> @@ -426,11 +443,25 @@ static int
>> iris_hfi_gen2_session_set_config_params(struct iris_inst *inst, u32 p
>> {HFI_PROP_LEVEL,
>> iris_hfi_gen2_set_level },
>> {HFI_PROP_COLOR_FORMAT,
>> iris_hfi_gen2_set_colorformat },
>> {HFI_PROP_LINEAR_STRIDE_SCANLINE,
>> iris_hfi_gen2_set_linear_stride_scanline },
>> + {HFI_PROP_TIER,
>> iris_hfi_gen2_set_tier },
>> };
>> if (V4L2_TYPE_IS_OUTPUT(plane)) {
>> - config_params = core->iris_platform_data->input_config_params;
>> - config_params_size =
>> core->iris_platform_data->input_config_params_size;
>> + if (inst->codec == V4L2_PIX_FMT_H264) {
>> + config_params =
>> core->iris_platform_data->input_config_params_default;
>> + config_params_size =
>> + core->iris_platform_data->input_config_params_default_size;
>> + } else if (inst->codec == V4L2_PIX_FMT_HEVC) {
>> + config_params =
>> core->iris_platform_data->input_config_params_hevc;
>> + config_params_size =
>> + core->iris_platform_data->input_config_params_hevc_size;
>> + } else if (inst->codec == V4L2_PIX_FMT_VP9) {
>> + config_params =
>> core->iris_platform_data->input_config_params_vp9;
>> + config_params_size =
>> + core->iris_platform_data->input_config_params_vp9_size;
>> + } else {
>> + return -EINVAL;
>> + }
>> } else {
>> config_params = core->iris_platform_data->output_config_params;
>> config_params_size =
>> core->iris_platform_data->output_config_params_size;
>> @@ -600,8 +631,21 @@ static int
>> iris_hfi_gen2_subscribe_change_param(struct iris_inst *inst, u32 plan
>> return 0;
>> }
>> - change_param = core->iris_platform_data->input_config_params;
>> - change_param_size = core->iris_platform_data->input_config_params_size;
>> + if (inst->codec == V4L2_PIX_FMT_H264) {
>> + change_param =
>> core->iris_platform_data->input_config_params_default;
>> + change_param_size =
>> + core->iris_platform_data->input_config_params_default_size;
>> + } else if (inst->codec == V4L2_PIX_FMT_HEVC) {
>> + change_param = core->iris_platform_data->input_config_params_hevc;
>> + change_param_size =
>> + core->iris_platform_data->input_config_params_hevc_size;
>> + } else if (inst->codec == V4L2_PIX_FMT_VP9) {
>> + change_param = core->iris_platform_data->input_config_params_vp9;
>> + change_param_size =
>> + core->iris_platform_data->input_config_params_vp9_size;
>> + } else {
>> + return -EINVAL;
>> + }
>
> I'm not a massive fan of multi-clause if/elses - in this particular case
> this will grow and grow into something very sprawling.
>
Sure, I can move to switch case if that is the preffered choice.
> Grateful if you'd switch() this.
>>> payload[0] = HFI_MODE_PORT_SETTINGS_CHANGE;
>> @@ -648,6 +692,11 @@ static int
>> iris_hfi_gen2_subscribe_change_param(struct iris_inst *inst, u32 plan
>> payload_size = sizeof(u32);
>> payload_type = HFI_PAYLOAD_U32;
>> break;
>> + case HFI_PROP_LUMA_CHROMA_BIT_DEPTH:
>> + payload[0] = subsc_params.bit_depth;
>> + payload_size = sizeof(u32);
>> + payload_type = HFI_PAYLOAD_U32;
>> + break;
>> case HFI_PROP_BUFFER_FW_MIN_OUTPUT_COUNT:
>> payload[0] = subsc_params.fw_min_count;
>> payload_size = sizeof(u32);
>> @@ -673,6 +722,11 @@ static int
>> iris_hfi_gen2_subscribe_change_param(struct iris_inst *inst, u32 plan
>> payload_size = sizeof(u32);
>> payload_type = HFI_PAYLOAD_U32;
>> break;
>> + case HFI_PROP_TIER:
>> + payload[0] = subsc_params.tier;
>> + payload_size = sizeof(u32);
>> + payload_type = HFI_PAYLOAD_U32;
>> + break;
>> default:
>> prop_type = 0;
>> ret = -EINVAL;
>> @@ -709,8 +763,21 @@ static int iris_hfi_gen2_subscribe_property(struct
>> iris_inst *inst, u32 plane)
>> subscribe_prop_size =
>> core->iris_platform_data->dec_input_prop_size;
>> subcribe_prop = core->iris_platform_data->dec_input_prop;
>> } else {
>> - subscribe_prop_size =
>> core->iris_platform_data->dec_output_prop_size;
>> - subcribe_prop = core->iris_platform_data->dec_output_prop;
>> + if (inst->codec == V4L2_PIX_FMT_H264) {
>> + subcribe_prop = core->iris_platform_data->dec_output_prop_avc;
>> + subscribe_prop_size =
>> + core->iris_platform_data->dec_output_prop_avc_size;
>> + } else if (inst->codec == V4L2_PIX_FMT_HEVC) {
>> + subcribe_prop = core->iris_platform_data->dec_output_prop_hevc;
>> + subscribe_prop_size =
>> + core->iris_platform_data->dec_output_prop_hevc_size;
>> + } else if (inst->codec == V4L2_PIX_FMT_VP9) {
>> + subcribe_prop = core->iris_platform_data->dec_output_prop_vp9;
>> + subscribe_prop_size =
>> + core->iris_platform_data->dec_output_prop_vp9_size;
>> + } else {
>> + return -EINVAL;
>> + }
>
> Same here, this code will be neater IMO as a swtich statement as it grows
> with new codecs.
>
>> }
>> for (i = 0; i < subscribe_prop_size; i++)
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
>> index 809bf0f238bd..6846311a26c3 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
>> @@ -580,6 +580,7 @@ static void
>> iris_hfi_gen2_read_input_subcr_params(struct iris_inst *inst)
>> }
>> inst->fw_caps[POC].value = subsc_params.pic_order_cnt;
>> + inst->fw_caps[TIER].value = subsc_params.tier;
>> if (subsc_params.bit_depth != BIT_DEPTH_8 ||
>> !(subsc_params.coded_frames & HFI_BITMASK_FRAME_MBS_ONLY_FLAG)) {
>> @@ -664,6 +665,9 @@ static int
>> iris_hfi_gen2_handle_session_property(struct iris_inst *inst,
>> inst_hfi_gen2->src_subcr_params.crop_offsets[0] = pkt->payload[0];
>> inst_hfi_gen2->src_subcr_params.crop_offsets[1] = pkt->payload[1];
>> break;
>> + case HFI_PROP_LUMA_CHROMA_BIT_DEPTH:
>> + inst_hfi_gen2->src_subcr_params.bit_depth = pkt->payload[0];
>> + break;
>> case HFI_PROP_CODED_FRAMES:
>> inst_hfi_gen2->src_subcr_params.coded_frames = pkt->payload[0];
>> break;
>> @@ -682,6 +686,9 @@ static int
>> iris_hfi_gen2_handle_session_property(struct iris_inst *inst,
>> case HFI_PROP_LEVEL:
>> inst_hfi_gen2->src_subcr_params.level = pkt->payload[0];
>> break;
>> + case HFI_PROP_TIER:
>> + inst_hfi_gen2->src_subcr_params.tier = pkt->payload[0];
>> + break;
>> case HFI_PROP_PICTURE_TYPE:
>> inst_hfi_gen2->hfi_frame_info.picture_type = pkt->payload[0];
>> break;
>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h
>> b/drivers/media/platform/qcom/iris/iris_platform_common.h
>> index 67204cddd44a..433ce9b00c68 100644
>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
>> @@ -174,14 +174,22 @@ struct iris_platform_data {
>> u32 num_vpp_pipe;
>> u32 max_session_count;
>> u32 max_core_mbpf;
>> - const u32 *input_config_params;
>> - unsigned int input_config_params_size;
>> + const u32 *input_config_params_default;
>> + unsigned int input_config_params_default_size;
>> + const u32 *input_config_params_hevc;
>> + unsigned int input_config_params_hevc_size;
>> + const u32 *input_config_params_vp9;
>> + unsigned int input_config_params_vp9_size;
>> const u32 *output_config_params;
>> unsigned int output_config_params_size;
>> const u32 *dec_input_prop;
>> unsigned int dec_input_prop_size;
>> - const u32 *dec_output_prop;
>> - unsigned int dec_output_prop_size;
>> + const u32 *dec_output_prop_avc;
>> + unsigned int dec_output_prop_avc_size;
>> + const u32 *dec_output_prop_hevc;
>> + unsigned int dec_output_prop_hevc_size;
>> + const u32 *dec_output_prop_vp9;
>> + unsigned int dec_output_prop_vp9_size;
>> const u32 *dec_ip_int_buf_tbl;
>> unsigned int dec_ip_int_buf_tbl_size;
>> const u32 *dec_op_int_buf_tbl;
>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8250.c
>> b/drivers/media/platform/qcom/iris/iris_platform_sm8250.c
>> index 5c86fd7b7b6f..5f74e57f04fc 100644
>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8250.c
>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8250.c
>> @@ -137,9 +137,9 @@ struct iris_platform_data sm8250_data = {
>> .num_vpp_pipe = 4,
>> .max_session_count = 16,
>> .max_core_mbpf = (8192 * 4352) / 256,
>> - .input_config_params =
>> + .input_config_params_default =
>> sm8250_vdec_input_config_param_default,
>> - .input_config_params_size =
>> + .input_config_params_default_size =
>> ARRAY_SIZE(sm8250_vdec_input_config_param_default),
>> .dec_ip_int_buf_tbl = sm8250_dec_ip_int_buf_tbl,
>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> index 29bc50785da5..779c71885f51 100644
>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> @@ -254,9 +254,10 @@ static struct tz_cp_config tz_cp_config_sm8550 = {
>> .cp_nonpixel_size = 0x24800000,
>> };
>> -static const u32 sm8550_vdec_input_config_params[] = {
>> +static const u32 sm8550_vdec_input_config_params_default[] = {
>> HFI_PROP_BITSTREAM_RESOLUTION,
>> HFI_PROP_CROP_OFFSETS,
>> + HFI_PROP_LUMA_CHROMA_BIT_DEPTH,
>> HFI_PROP_CODED_FRAMES,
>> HFI_PROP_BUFFER_FW_MIN_OUTPUT_COUNT,
>> HFI_PROP_PIC_ORDER_CNT_TYPE,
>> @@ -265,6 +266,26 @@ static const u32 sm8550_vdec_input_config_params[] = {
>> HFI_PROP_SIGNAL_COLOR_INFO,
>> };
>> +static const u32 sm8550_vdec_input_config_param_hevc[] = {
>> + HFI_PROP_BITSTREAM_RESOLUTION,
>> + HFI_PROP_CROP_OFFSETS,
>> + HFI_PROP_LUMA_CHROMA_BIT_DEPTH,
>> + HFI_PROP_BUFFER_FW_MIN_OUTPUT_COUNT,
>> + HFI_PROP_PROFILE,
>> + HFI_PROP_LEVEL,
>> + HFI_PROP_TIER,
>> + HFI_PROP_SIGNAL_COLOR_INFO,
>> +};
>> +
>> +static const u32 sm8550_vdec_input_config_param_vp9[] = {
>> + HFI_PROP_BITSTREAM_RESOLUTION,
>> + HFI_PROP_CROP_OFFSETS,
>> + HFI_PROP_LUMA_CHROMA_BIT_DEPTH,
>> + HFI_PROP_BUFFER_FW_MIN_OUTPUT_COUNT,
>> + HFI_PROP_PROFILE,
>> + HFI_PROP_LEVEL,
>> +};
>> +
>> static const u32 sm8550_vdec_output_config_params[] = {
>> HFI_PROP_COLOR_FORMAT,
>> HFI_PROP_LINEAR_STRIDE_SCANLINE,
>> @@ -274,11 +295,19 @@ static const u32
>> sm8550_vdec_subscribe_input_properties[] = {
>> HFI_PROP_NO_OUTPUT,
>> };
>> -static const u32 sm8550_vdec_subscribe_output_properties[] = {
>> +static const u32 sm8550_vdec_subscribe_output_properties_avc[] = {
>> HFI_PROP_PICTURE_TYPE,
>> HFI_PROP_CABAC_SESSION,
>> };
>> +static const u32 sm8550_vdec_subscribe_output_properties_hevc[] = {
>> + HFI_PROP_PICTURE_TYPE,
>> +};
>> +
>> +static const u32 sm8550_vdec_subscribe_output_properties_vp9[] = {
>> + HFI_PROP_PICTURE_TYPE,
>> +};
>> +
>> static const u32 sm8550_dec_ip_int_buf_tbl[] = {
>> BUF_BIN,
>> BUF_COMV,
>> @@ -322,19 +351,33 @@ struct iris_platform_data sm8550_data = {
>> .num_vpp_pipe = 4,
>> .max_session_count = 16,
>> .max_core_mbpf = ((8192 * 4352) / 256) * 2,
>
> I realise it's not in your changeset but....
>
> mbpf - mega bytes per frame ?
>
> What do these numbers capture and how do we validate them ?
>
> Defines with meaning would be better
>
> ((MAX_BYTES_PER_THING * NUMBER_OF_LINES_OF_PIXELS) / SOME_ALIGNMENT ) * 2
>
> or whatever it is those numbers ultimately capture.
>
mbpf is macroblocks per frame, 8192 * 4352 is 8k resolution
I can add a comment if that helps to understand this better.
Thanks,
Dikshita
>> - .input_config_params =
>> - sm8550_vdec_input_config_params,
>> - .input_config_params_size =
>> - ARRAY_SIZE(sm8550_vdec_input_config_params),
>> + .input_config_params_default =
>> + sm8550_vdec_input_config_params_default,
>> + .input_config_params_default_size =
>> + ARRAY_SIZE(sm8550_vdec_input_config_params_default),
>> + .input_config_params_hevc =
>> + sm8550_vdec_input_config_param_hevc,
>> + .input_config_params_hevc_size =
>> + ARRAY_SIZE(sm8550_vdec_input_config_param_hevc),
>> + .input_config_params_vp9 =
>> + sm8550_vdec_input_config_param_vp9,
>> + .input_config_params_vp9_size =
>> + ARRAY_SIZE(sm8550_vdec_input_config_param_vp9),
>> .output_config_params =
>> sm8550_vdec_output_config_params,
>> .output_config_params_size =
>> ARRAY_SIZE(sm8550_vdec_output_config_params),
>> .dec_input_prop = sm8550_vdec_subscribe_input_properties,
>> .dec_input_prop_size =
>> ARRAY_SIZE(sm8550_vdec_subscribe_input_properties),
>> - .dec_output_prop = sm8550_vdec_subscribe_output_properties,
>> - .dec_output_prop_size =
>> ARRAY_SIZE(sm8550_vdec_subscribe_output_properties),
>> -
>> + .dec_output_prop_avc = sm8550_vdec_subscribe_output_properties_avc,
>> + .dec_output_prop_avc_size =
>> + ARRAY_SIZE(sm8550_vdec_subscribe_output_properties_avc),
>> + .dec_output_prop_hevc = sm8550_vdec_subscribe_output_properties_hevc,
>> + .dec_output_prop_hevc_size =
>> + ARRAY_SIZE(sm8550_vdec_subscribe_output_properties_hevc),
>> + .dec_output_prop_vp9 = sm8550_vdec_subscribe_output_properties_vp9,
>> + .dec_output_prop_vp9_size =
>> + ARRAY_SIZE(sm8550_vdec_subscribe_output_properties_vp9),
>> .dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl,
>> .dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl),
>> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl,
>
> ---
> bod
Powered by blists - more mailing lists