On 3/6/2025 5:58 AM, Bryan O'Donoghue wrote:
> On 05/03/2025 10:43, Dikshita Agarwal wrote:
>> Add platform capabilities for HEVC and VP9 codecs in decoder driver
>> with related hooks.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
>> ---
>> drivers/media/platform/qcom/iris/iris_ctrls.c | 28 ++++++-
>> .../qcom/iris/iris_hfi_gen2_command.c | 30 ++++++-
>> .../qcom/iris/iris_hfi_gen2_defines.h | 1 +
>> .../qcom/iris/iris_hfi_gen2_response.c | 36 ++++++++-
>> .../platform/qcom/iris/iris_platform_common.h | 9 ++-
>> .../platform/qcom/iris/iris_platform_sm8550.c | 80 ++++++++++++++++++-
>> 6 files changed, 170 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_ctrls.c
>> b/drivers/media/platform/qcom/iris/iris_ctrls.c
>> index b690578256d5..fb2b818c7c5c 100644
>> --- a/drivers/media/platform/qcom/iris/iris_ctrls.c
>> +++ b/drivers/media/platform/qcom/iris/iris_ctrls.c
>> @@ -20,9 +20,19 @@ static enum platform_inst_fw_cap_type
>> iris_get_cap_id(u32 id)
>> case V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER:
>> return DEBLOCK;
>> case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
>> - return PROFILE;
>> + return PROFILE_H264;
>> + case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
>> + return PROFILE_HEVC;
>> + case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
>> + return PROFILE_VP9;
>> case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
>> - return LEVEL;
>> + return LEVEL_H264;
>> + case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
>> + return LEVEL_HEVC;
>> + case V4L2_CID_MPEG_VIDEO_VP9_LEVEL:
>> + return LEVEL_VP9;
>> + case V4L2_CID_MPEG_VIDEO_HEVC_TIER:
>> + return TIER;
>> default:
>> return INST_FW_CAP_MAX;
>> }
>> @@ -36,10 +46,20 @@ static u32 iris_get_v4l2_id(enum
>> platform_inst_fw_cap_type cap_id)
>> switch (cap_id) {
>> case DEBLOCK:
>> return V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER;
>> - case PROFILE:
>> + case PROFILE_H264:
>> return V4L2_CID_MPEG_VIDEO_H264_PROFILE;
>> - case LEVEL:
>> + case PROFILE_HEVC:
>> + return V4L2_CID_MPEG_VIDEO_HEVC_PROFILE;
>> + case PROFILE_VP9:
>> + return V4L2_CID_MPEG_VIDEO_VP9_PROFILE;
>> + case LEVEL_H264:
>> return V4L2_CID_MPEG_VIDEO_H264_LEVEL;
>> + case LEVEL_HEVC:
>> + return V4L2_CID_MPEG_VIDEO_HEVC_LEVEL;
>> + case LEVEL_VP9:
>> + return V4L2_CID_MPEG_VIDEO_VP9_LEVEL;
>> + case TIER:
>> + return V4L2_CID_MPEG_VIDEO_HEVC_TIER;
>> default:
>> return 0;
>> }
>> 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 beaf3a051d7c..a3ebcda9a2ba 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
>> @@ -309,7 +309,20 @@ static int iris_hfi_gen2_set_profile(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 profile = inst->fw_caps[PROFILE].value;
>> + u32 profile;
>> +
>> + switch (inst->codec) {
>> + case V4L2_PIX_FMT_HEVC:
>> + profile = inst->fw_caps[PROFILE_HEVC].value;
>> + break;
>> + case V4L2_PIX_FMT_VP9:
>> + profile = inst->fw_caps[PROFILE_VP9].value;
>> + break;
>> + case V4L2_PIX_FMT_H264:
>> + default:
>> + profile = inst->fw_caps[PROFILE_H264].value;
>> + break;
>
> Following up on my previous comment about returning a 0 default and running
> with it instead of erroring it - you then treat default == 0 @ inst->codec
> assigned in iris_hfi_gen[1|2]_sys_init as H264.
>
> In fact why have a default by the time you get this this point in the code
> anyway ?
>
> Just chuck out parameters which aren't expected as errors and then don't
> bother with these defaults.
>
Ack, as mentioned in previous comment, will remove the default case.
>> + }
>> inst_hfi_gen2->src_subcr_params.profile = profile;
>> @@ -326,7 +339,20 @@ static int iris_hfi_gen2_set_level(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 level = inst->fw_caps[LEVEL].value;
>> + u32 level;
>> +
>> + switch (inst->codec) {
>> + case V4L2_PIX_FMT_HEVC:
>> + level = inst->fw_caps[LEVEL_HEVC].value;
>> + break;
>> + case V4L2_PIX_FMT_VP9:
>> + level = inst->fw_caps[LEVEL_VP9].value;
>> + break;
>> + case V4L2_PIX_FMT_H264:
>> + default:
>> + level = inst->fw_caps[LEVEL_H264].value;
>> + break;
>> + }
>> inst_hfi_gen2->src_subcr_params.level = level;
>> 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 2fcf7914b70f..48c507a1ec27 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h
>> @@ -46,6 +46,7 @@
>> #define HFI_PROP_CROP_OFFSETS 0x03000105
>> #define HFI_PROP_PROFILE 0x03000107
>> #define HFI_PROP_LEVEL 0x03000108
>> +#define HFI_PROP_TIER 0x03000109
>> #define HFI_PROP_STAGE 0x0300010a
>> #define HFI_PROP_PIPE 0x0300010b
>> #define HFI_PROP_LUMA_CHROMA_BIT_DEPTH 0x0300010f
>
> These seem like - probably bitfields ?
>
> Could we get the bits in a follow on patch/series ?
>
I didn't understand this comment.
These are not bit fields, but HFI macros, which we use to communicate with
firmware.
>> 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 b75a01641d5d..809bf0f238bd 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
>> @@ -563,8 +563,22 @@ static void
>> iris_hfi_gen2_read_input_subcr_params(struct iris_inst *inst)
>> inst->crop.width = pixmp_ip->width -
>> ((subsc_params.crop_offsets[1] >> 16) & 0xFFFF) - inst->crop.left;
>> - inst->fw_caps[PROFILE].value = subsc_params.profile;
>> - inst->fw_caps[LEVEL].value = subsc_params.level;
>> + switch (inst->codec) {
>> + case V4L2_PIX_FMT_HEVC:
>> + inst->fw_caps[PROFILE_HEVC].value = subsc_params.profile;
>> + inst->fw_caps[LEVEL_HEVC].value = subsc_params.level;
>> + break;
>> + case V4L2_PIX_FMT_VP9:
>> + inst->fw_caps[PROFILE_VP9].value = subsc_params.profile;
>> + inst->fw_caps[LEVEL_VP9].value = subsc_params.level;
>> + break;
>> + case V4L2_PIX_FMT_H264:
>> + default:
>> + inst->fw_caps[PROFILE_H264].value = subsc_params.profile;
>> + inst->fw_caps[LEVEL_H264].value = subsc_params.level;
>> + break;
>> + }
>> +
>> inst->fw_caps[POC].value = subsc_params.pic_order_cnt;
>> if (subsc_params.bit_depth != BIT_DEPTH_8 ||
>> @@ -791,8 +805,22 @@ static void
>> iris_hfi_gen2_init_src_change_param(struct iris_inst *inst)
>> full_range, video_format,
>> video_signal_type_present_flag);
>> - subsc_params->profile = inst->fw_caps[PROFILE].value;
>> - subsc_params->level = inst->fw_caps[LEVEL].value;
>> + switch (inst->codec) {
>> + case V4L2_PIX_FMT_HEVC:
>> + subsc_params->profile = inst->fw_caps[PROFILE_HEVC].value;
>> + subsc_params->level = inst->fw_caps[LEVEL_HEVC].value;
>> + break;
>> + case V4L2_PIX_FMT_VP9:
>> + subsc_params->profile = inst->fw_caps[PROFILE_VP9].value;
>> + subsc_params->level = inst->fw_caps[LEVEL_VP9].value;
>> + break;
>> + case V4L2_PIX_FMT_H264:
>> + default:
>> + subsc_params->profile = inst->fw_caps[PROFILE_H264].value;
>> + subsc_params->level = inst->fw_caps[LEVEL_H264].value;
>> + break;
>> + }
>> +
>> subsc_params->pic_order_cnt = inst->fw_caps[POC].value;
>> subsc_params->bit_depth = inst->fw_caps[BIT_DEPTH].value;
>> if (inst->fw_caps[CODED_FRAMES].value ==
>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h
>> b/drivers/media/platform/qcom/iris/iris_platform_common.h
>> index f6b15d2805fb..67204cddd44a 100644
>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
>> @@ -78,8 +78,12 @@ struct platform_inst_caps {
>> };
>> enum platform_inst_fw_cap_type {
>> - PROFILE = 1,
>> - LEVEL,
>> + PROFILE_H264 = 1,
>> + PROFILE_HEVC,
>> + PROFILE_VP9,
>> + LEVEL_H264,
>> + LEVEL_HEVC,
>> + LEVEL_VP9,
>> INPUT_BUF_HOST_MAX_COUNT,
>> STAGE,
>> PIPE,
>> @@ -88,6 +92,7 @@ enum platform_inst_fw_cap_type {
>> BIT_DEPTH,
>> RAP_FRAME,
>> DEBLOCK,
>> + TIER,
>> INST_FW_CAP_MAX,
>> };
>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> index 35d278996c43..29bc50785da5 100644
>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> @@ -14,7 +14,7 @@
>> static struct platform_inst_fw_cap inst_fw_cap_sm8550[] = {
>> {
>> - .cap_id = PROFILE,
>> + .cap_id = PROFILE_H264,
>> .min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,
>> .max = V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_HIGH,
>> .step_or_mask = BIT(V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) |
>> @@ -28,7 +28,29 @@ static struct platform_inst_fw_cap
>> inst_fw_cap_sm8550[] = {
>> .set = iris_set_u32_enum,
>> },
>> {
>> - .cap_id = LEVEL,
>> + .cap_id = PROFILE_HEVC,
>> + .min = V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN,
>> + .max = V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN_STILL_PICTURE,
>> + .step_or_mask = BIT(V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN_STILL_PICTURE),
>> + .value = V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN,
>> + .hfi_id = HFI_PROP_PROFILE,
>> + .flags = CAP_FLAG_OUTPUT_PORT | CAP_FLAG_MENU,
>> + .set = iris_set_u32_enum,
>> + },
>> + {
>> + .cap_id = PROFILE_VP9,
>> + .min = V4L2_MPEG_VIDEO_VP9_PROFILE_0,
>> + .max = V4L2_MPEG_VIDEO_VP9_PROFILE_2,
>> + .step_or_mask = BIT(V4L2_MPEG_VIDEO_VP9_PROFILE_0) |
>> + BIT(V4L2_MPEG_VIDEO_VP9_PROFILE_2),
>> + .value = V4L2_MPEG_VIDEO_VP9_PROFILE_0,
>> + .hfi_id = HFI_PROP_PROFILE,
>> + .flags = CAP_FLAG_OUTPUT_PORT | CAP_FLAG_MENU,
>> + .set = iris_set_u32_enum,
>> + },
>> + {
>> + .cap_id = LEVEL_H264,
>> .min = V4L2_MPEG_VIDEO_H264_LEVEL_1_0,
>> .max = V4L2_MPEG_VIDEO_H264_LEVEL_6_2,
>> .step_or_mask = BIT(V4L2_MPEG_VIDEO_H264_LEVEL_1_0) |
>> @@ -56,6 +78,60 @@ static struct platform_inst_fw_cap
>> inst_fw_cap_sm8550[] = {
>> .flags = CAP_FLAG_OUTPUT_PORT | CAP_FLAG_MENU,
>> .set = iris_set_u32_enum,
>> },
>> + {
>> + .cap_id = LEVEL_HEVC,
>> + .min = V4L2_MPEG_VIDEO_HEVC_LEVEL_1,
>> + .max = V4L2_MPEG_VIDEO_HEVC_LEVEL_6_2,
>> + .step_or_mask = BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_1) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_2) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_2_1) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_3) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_3_1) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_4) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_4_1) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_5) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_5_1) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_5_2) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_6) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_6_1) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_LEVEL_6_2),
>> + .value = V4L2_MPEG_VIDEO_HEVC_LEVEL_6_1,
>> + .hfi_id = HFI_PROP_LEVEL,
>> + .flags = CAP_FLAG_OUTPUT_PORT | CAP_FLAG_MENU,
>> + .set = iris_set_u32_enum,
>> + },
>> + {
>> + .cap_id = LEVEL_VP9,
>> + .min = V4L2_MPEG_VIDEO_VP9_LEVEL_1_0,
>> + .max = V4L2_MPEG_VIDEO_VP9_LEVEL_6_0,
>> + .step_or_mask = BIT(V4L2_MPEG_VIDEO_VP9_LEVEL_1_0) |
>> + BIT(V4L2_MPEG_VIDEO_VP9_LEVEL_1_1) |
>> + BIT(V4L2_MPEG_VIDEO_VP9_LEVEL_2_0) |
>> + BIT(V4L2_MPEG_VIDEO_VP9_LEVEL_2_1) |
>> + BIT(V4L2_MPEG_VIDEO_VP9_LEVEL_3_0) |
>> + BIT(V4L2_MPEG_VIDEO_VP9_LEVEL_3_1) |
>> + BIT(V4L2_MPEG_VIDEO_VP9_LEVEL_4_0) |
>> + BIT(V4L2_MPEG_VIDEO_VP9_LEVEL_4_1) |
>> + BIT(V4L2_MPEG_VIDEO_VP9_LEVEL_5_0) |
>> + BIT(V4L2_MPEG_VIDEO_VP9_LEVEL_5_1) |
>> + BIT(V4L2_MPEG_VIDEO_VP9_LEVEL_5_2) |
>> + BIT(V4L2_MPEG_VIDEO_VP9_LEVEL_6_0),
>> + .value = V4L2_MPEG_VIDEO_VP9_LEVEL_6_0,
>> + .hfi_id = HFI_PROP_LEVEL,
>> + .flags = CAP_FLAG_OUTPUT_PORT | CAP_FLAG_MENU,
>> + .set = iris_set_u32_enum,
>> + },
>> + {
>> + .cap_id = TIER,
>> + .min = V4L2_MPEG_VIDEO_HEVC_TIER_MAIN,
>> + .max = V4L2_MPEG_VIDEO_HEVC_TIER_HIGH,
>> + .step_or_mask = BIT(V4L2_MPEG_VIDEO_HEVC_TIER_MAIN) |
>> + BIT(V4L2_MPEG_VIDEO_HEVC_TIER_HIGH),
>> + .value = V4L2_MPEG_VIDEO_HEVC_TIER_HIGH,
>> + .hfi_id = HFI_PROP_TIER,
>> + .flags = CAP_FLAG_OUTPUT_PORT | CAP_FLAG_MENU,
>> + .set = iris_set_u32_enum,
>> + },
>> {
>> .cap_id = INPUT_BUF_HOST_MAX_COUNT,
>> .min = DEFAULT_MAX_HOST_BUF_COUNT,
>
> Other than those nits
>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
Thanks,
Dikshita
