Re: [RFC PATCH 02/12] media: iris: Add platform capabilities for HEVC and VP9 decoders

[Dikshita Agarwal <quic_dikshita@quicinc.com>]

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@quicinc.com>
>> ---
>>   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@linaro.org>

Thanks,
Dikshita