Re: [PATCH 3/8] media: iris: stop copying r/o data

Dmitry Baryshkov
2025-10-09T00:16:09Z

On Thu, Oct 09, 2025 at 12:48:00AM +0100, Bryan O'Donoghue wrote:
> On 08/10/2025 05:33, Dmitry Baryshkov wrote:
> > Most of the platform_inst_caps data is read-only. In order to lower the
> > amount of memory consumed by the driver, store the value and the
> > corresponding indice in the read-write data and use the rest via the
> > pointer to r/o capability data.
>
> corresponding index

Ack

>
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > ---
> > drivers/media/platform/qcom/iris/iris_core.h | 4 +-
> > drivers/media/platform/qcom/iris/iris_ctrls.c | 238 ++++++++++-----------
> > drivers/media/platform/qcom/iris/iris_instance.h | 3 +-
> > .../platform/qcom/iris/iris_platform_common.h | 8 +-
> > drivers/media/platform/qcom/iris/iris_vdec.c | 5 +-
> > drivers/media/platform/qcom/iris/iris_venc.c | 5 +-
> > 6 files changed, 135 insertions(+), 128 deletions(-)
> >
> > diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h
> > index fb194c967ad4f9b5e00cd74f0d41e0b827ef14db..b5037ae8c71921753c165a86a277a4a4b5083b30 100644
> > --- a/drivers/media/platform/qcom/iris/iris_core.h
> > +++ b/drivers/media/platform/qcom/iris/iris_core.h
> > @@ -115,8 +115,8 @@ struct iris_core {
> > struct delayed_work sys_error_handler;
> > struct list_head instances;
> > /* encoder and decoder have overlapping caps, so two different arrays are required */
> > - struct platform_inst_fw_cap inst_fw_caps_dec[INST_FW_CAP_MAX];
> > - struct platform_inst_fw_cap inst_fw_caps_enc[INST_FW_CAP_MAX];
> > + struct platform_inst_fw_cap_value inst_fw_caps_dec[INST_FW_CAP_MAX];
> > + struct platform_inst_fw_cap_value inst_fw_caps_enc[INST_FW_CAP_MAX];
> > };
> >
> > int iris_core_init(struct iris_core *core);
> > diff --git a/drivers/media/platform/qcom/iris/iris_ctrls.c b/drivers/media/platform/qcom/iris/iris_ctrls.c
> > index 9da050aa1f7ce8152dfa46a706e2c27adfb5d6ce..0e9adb3982a49cfd7cbe5110cfd5f573f0f7bb38 100644
> > --- a/drivers/media/platform/qcom/iris/iris_ctrls.c
> > +++ b/drivers/media/platform/qcom/iris/iris_ctrls.c
> > @@ -194,26 +194,28 @@ static int iris_op_s_ctrl(struct v4l2_ctrl *ctrl)
> > {
> > struct iris_inst *inst = container_of(ctrl->handler, struct iris_inst, ctrl_handler);
> > enum platform_inst_fw_cap_type cap_id;
> > - struct platform_inst_fw_cap *cap;
> > + unsigned int cap_idx;
> > struct vb2_queue *q;
> >
> > - cap = &inst->fw_caps[0];
> > cap_id = iris_get_cap_id(ctrl->id);
> > if (!iris_valid_cap_id(cap_id))
> > return -EINVAL;
> >
> > + cap_idx = inst->fw_caps[cap_id].idx;
> > +
> > q = v4l2_m2m_get_src_vq(inst->m2m_ctx);
> > if (vb2_is_streaming(q) &&
> > - (!(inst->fw_caps[cap_id].flags & CAP_FLAG_DYNAMIC_ALLOWED)))
> > + (!(inst->inst_fw_caps[cap_id].flags & CAP_FLAG_DYNAMIC_ALLOWED)))
> > return -EINVAL;
> >
> > - cap[cap_id].flags |= CAP_FLAG_CLIENT_SET;
> > + inst->fw_caps[cap_id].client_set = true;
>
> Why drop just this one bit - CAP_FLAG_CLIENT_SET.
>
> Code seems neater with that bit retained in fw_caps to me, you have fewer
> LOC changed that way too.

The problem is that all other flags are read-only and they stay in
the constant struct platform_inst_fw_cap. The CAP_FLAG_CLIENT_SET is
dynamic and it is set for each instance. Initially I kept the flag and
copied the whole flag set, but it resulted in a bigger patch, because
all cap.flags lookups were now using inst->cap[id].flag (and there are
more than just CAP_FLAG_CLIENT_SET). In the end I decided that it's not
worth copying r/o flags, but instead we should separate r/o and r/w
data. Thus CAP_FLAG_CLIENT_SET ended up being a bool field. I think it's
more logical and easier to follow the code like this. If there are
multiple flags like this. we can always change the code to add
INST_FLAG_CLIENT_SET.

>
> >
> > inst->fw_caps[cap_id].value = ctrl->val;
> >
> > if (vb2_is_streaming(q)) {
> > - if (cap[cap_id].set)
> > - cap[cap_id].set(inst, cap_id);
> > +
> > + if (inst->inst_fw_caps[cap_idx].set)
> > + inst->inst_fw_caps[cap_idx].set(inst, cap_id);
> > }
> >
> > return 0;

--
With best wishes
Dmitry