[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aJCWSAfxBzLYqlQL@trex>
Date: Mon, 4 Aug 2025 13:15:20 +0200
From: Jorge Ramirez <jorge.ramirez@....qualcomm.com>
To: Jorge Ramirez <jorge.ramirez@....qualcomm.com>
Cc: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, quic_vgarodia@...cinc.com,
quic_dikshita@...cinc.com, krzk+dt@...nel.org, konradybcio@...nel.org,
mchehab@...nel.org, andersson@...nel.org, conor+dt@...nel.org,
amit.kucheria@....qualcomm.com, linux-media@...r.kernel.org,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 3/7] media: venus: Add support for AR50_LITE video core
On 17/07/25 14:33:04, Jorge Ramirez wrote:
> On 17/07/25 10:29:44, Bryan O'Donoghue wrote:
> > On 17/07/2025 08:19, Jorge Ramirez wrote:
> > > > > --- a/drivers/media/platform/qcom/venus/helpers.c
> > > > > +++ b/drivers/media/platform/qcom/venus/helpers.c
> > > > > @@ -230,6 +230,24 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(venus_helper_alloc_dpb_bufs);
> > > > > +void venus_helper_prepare_eos_data(struct venus_inst *inst,
> > > > > + struct hfi_frame_data *data)
> > > > > +{
> > > > > + struct venus_core *core = inst->core;
> > > > > +
> > > > > + data->buffer_type = HFI_BUFFER_INPUT;
> > > > > + data->flags = HFI_BUFFERFLAG_EOS;
> > > > > +
> > > > > + if (IS_V6(core) && is_fw_rev_or_older(core, 1, 0, 87))
> > > > > + return;
> > > > > +
> > > > > + if (IS_V4(core) && is_lite(core) && is_fw_rev_or_older(core, 6, 0, 53))
> > > > > + data->alloc_len = 1;
> > > > > +
> > > > > + data->device_addr = 0xdeadb000;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(venus_helper_prepare_eos_data);
> > > > This function doesn't appear to have alot to do with AR50_LITE as it
> > > > pertains to IS_V6() and IS_V4().
> > > >
> > > > This I think should be a separate patch with its own commit log to describe
> > > > the quite complex logic of version numbers going on here.
> > > Let me give it some background:
> > >
> > > According to the HFI specification, EOS (End-of-Stream) buffers must
> > > have 'valid' addresses. While the firmware currently appears to make no
> > > use of the EOS buffer contents, allocating and mapping them would have
> > > been a better driver choice IMO. Hoever this one has better performance
> > > which is probably the reason why it has stayed.
> > >
> > > The firmware then does perform operations involving the buffer's size
> > > and length fields, and enforces boundary checks accordingly. On the
> > > AR50_LITE platform, an earlier firmware version lacked a check on
> > > alloc_len, leading to a division-by-zero scenario.
> > >
> > > This has been addressed, and we plan to release firmware version 6.0.54,
> > > which includes the necessary boundary check for alloc_len.
> > >
> > > I should probaly replace IS_V4(core) && is_lite(core) with
> > > IS_AR50_LITE() instead of trying to give it the appearence of a design
> > > feature.
> > >
> > > seems the sensible thing to do, right?
> >
> > I'll stipulate to all of that.
> >
> > I know I'm being pedantic but, the title and subject of this patch is
> > "AR50_LITE" does stuff.
> >
> > As traveler from a mirror-universe - I would read the commit log here, look
> > at this function and be none the wiser what was going on.
>
> right this is because I think you saw through my inconsistency.
>
> The truth is I should have written the function as below; and I think
> now there is a case for it being in this patch since it is addressing a
> firmware issue on the firmware release supporting this core which the
> patch is adding the functionality for.
>
> void venus_helper_prepare_eos_data(struct venus_inst *inst,
> struct hfi_frame_data *data)
> {
> struct venus_core *core = inst->core;
>
> data->buffer_type = HFI_BUFFER_INPUT;
> data->flags = HFI_BUFFERFLAG_EOS;
>
> if (IS_V6(core) && is_fw_rev_or_older(core, 1, 0, 87))
> return;
>
> if (IS_AR50_LITE(core) && is_fw_rev_or_older(core, 6, 0, 53))
> data->alloc_len = 1;
>
> data->device_addr = 0xdeadb000;
> }
> EXPORT_SYMBOL_GPL(venus_helper_prepare_eos_data);
>
> what do you think? this is really just adding a the AR50_LITE line to
> the existing implementation.
>
> >
> > The EOS check is a fundamental HFI capability which is why I again reiterate
> > it deserves its own commit log with the above explanation - word-for-word
> > would be fine from my POV, to explain what is going on.
> >
>
> umm not sure yet but if the above does not make you change your mind
> I'll do it.
>
> > Long live the Empire!
>
> :)
>
> >
> > ---
> > bod
will drop this - not needed since the decoder will not work except with
the incoming 6.0.55 release (see below)
https://gitlab.com/kernel-firmware/linux-firmware/-/merge_requests/630
This firmware - on top of supporting the encoding functionality- removes
the need for the dummy length.
Powered by blists - more mailing lists