Re: [PATCH v4 2/2] media: iris: add support for vbv delay

[Sachin Kumar Garg <sachin.garg@oss.qualcomm.com>]

On 9/29/2026 8:35 PM, Dmitry Baryshkov wrote:
> On Tue, 29 Sept 2026 at 14:54, Sachin Kumar Garg
> <sachin.garg@oss.qualcomm.com> wrote:
>>
>>
>>
>> On 9/27/2026 11:55 PM, Dmitry Baryshkov wrote:
>>> On Sun, Sep 27, 2026 at 09:27:46AM +0530, Sachin Kumar Garg wrote:
>>>> Add the VBV_DELAY instance capability for the HFI Gen2 encoder,
>>>> exposed to userspace via V4L2_CID_MPEG_VIDEO_VBV_DELAY and mapped to
>>>> HFI_PROP_VBV_DELAY through a new iris_set_vbv_delay(). The property
>>>> is only meaningful for CBR rate-control modes.
>>>>
>>>> Low VBV delay values (34-100 ms) require the encoder to run
>>>> with a single video pipe, so extend iris_set_pipe() to force
>>>> work_route to 1 whenever the configured VBV_DELAY falls in that
>>>> range.
>>>>
>>>> Reviewed-by: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
>>>> Signed-off-by: Sachin Kumar Garg <sachin.garg@oss.qualcomm.com>
>>>> ---
>>>>    drivers/media/platform/qcom/iris/iris_ctrls.c      | 37 +++++++++++++++++++++-
>>>>    drivers/media/platform/qcom/iris/iris_ctrls.h      |  1 +
>>>>    drivers/media/platform/qcom/iris/iris_hfi_gen2.c   | 12 ++++++-
>>>>    .../platform/qcom/iris/iris_hfi_gen2_defines.h     |  1 +
>>>>    .../platform/qcom/iris/iris_platform_common.h      |  1 +
>>>>    5 files changed, 50 insertions(+), 2 deletions(-)
>>>>
>>>> @@ -541,8 +545,19 @@ int iris_set_stage(struct iris_inst *inst, enum platform_inst_fw_cap_type cap_id
>>>>    int iris_set_pipe(struct iris_inst *inst, enum platform_inst_fw_cap_type cap_id)
>>>>    {
>>>>       const struct iris_hfi_session_ops *hfi_ops = inst->hfi_session_ops;
>>>> -    u32 work_route = inst->fw_caps[PIPE].value;
>>>> +    u32 work_route = inst->fw_caps[PIPE].max;
>>>
>>> Why is being set to the max value instead of using the .value?
>>>
>>
>> fw_caps[PIPE].min/.max/.value are all seeded from the same platform
>> constant (num_vpp_pipe) at init, so .max and .value were interchangeable
>> before this patch. This patch now writes the applied pipe count back
>> into .value (used elsewhere for power/cycle estimation), which makes
>> .value stateful. Since iris_set_pipe() is re-invoked on every resolution
>> change while streaming, seeding work_route from .value would let a
>> previous single-pipe override persist as the new baseline on the next
>> call, even after the VBV-delay/CBR condition no longer applies. Reading
>> from .max instead keeps the default pipe count stable and re-evaluates
>> the workaround fresh on every call.
> 
> With the .value being written, does it need a lock protection?
> 
No — inst->lock already serializes this. iris_set_pipe()'s write to 
fw_caps[PIPE].value only happens inside start_streaming 
(iris_venc/vdec_streamon_output(), both the iris_set_properties() cap 
loop and the DRC re-apply path), and start_streaming takes inst->lock 
before reaching either. The only reader outside that same call chain is 
the power/clock-scaling code (iris_vpu3x_vpu4x_calculate_frequency()), 
which is only invoked via iris_scale_power(), itself called from 
start_streaming and buf_queue — both of which also hold inst->lock. So 
writer and readers already can't run concurrently; adding a lock here 
would just be redundant with the existing vb2-op locking, and no other 
.set() callback in this file takes one either.

Regards,
Sachin>>
>> Regards,
>> Sachin>>        u32 hfi_id = inst->fw_caps[cap_id].hfi_id;
>>>> +    u32 vbv_delay;
>>>> +
>>>> +    if (inst->domain == ENCODER &&
>>>> +        inst->fw_caps[FRAME_RC_ENABLE].value &&
>>>> +        inst->fw_caps[BITRATE_MODE].value == V4L2_MPEG_VIDEO_BITRATE_MODE_CBR) {
>>>> +            vbv_delay = inst->fw_caps[VBV_DELAY].value;
>>>> +            if (vbv_delay >= 34 && vbv_delay <= 100)
>>>> +                    work_route = 1;
>>>> +    }
>>>> +
>>>> +    inst->fw_caps[PIPE].value = work_route;
>>>>
>>>>       return hfi_ops->session_set_property(inst, hfi_id,
>>>>                                            HFI_HOST_FLAGS_NONE,
>>
> 
>