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

[Dmitry Baryshkov]

On Tue, Sep 29, 2026 at 09:37:38PM +0530, Sachin Kumar Garg wrote:
> 
> 
> On 9/29/2026 8:35 PM, Dmitry Baryshkov wrote:
> > On Tue, 29 Sept 2026 at 14:54, Sachin Kumar Garg
> > <sachin.garg@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > > > > Signed-off-by: Sachin Kumar Garg <sachin.garg@xxxxxxxxxxxxxxxx>
> > > > > ---
> > > > >    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.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>


-- 
With best wishes
Dmitry