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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <rmeerm4hqcdxqjmylbfep7enswzrypqfyhl2yka554m7mo4y3s@4ycpb6eeojpe>
Date: Thu, 9 Oct 2025 03:16:02 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
Cc: Vikash Garodia <vikash.garodia@....qualcomm.com>,
        Dikshita Agarwal <dikshita.agarwal@....qualcomm.com>,
        Abhinav Kumar <abhinav.kumar@...ux.dev>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/8] media: iris: stop copying r/o data

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@....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