Re: [PATCH v3 0/5] Introduce "non-pixel" sub node within iris video node

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 03-Jul-25 14:54, Krzysztof Kozlowski wrote:
> On 03/07/2025 14:38, Konrad Dybcio wrote:
>>
>>
>> On 03-Jul-25 09:27, Krzysztof Kozlowski wrote:
>>> On 03/07/2025 00:26, Dmitry Baryshkov wrote:
>>>> On Wed, Jul 02, 2025 at 01:06:17PM +0100, Bryan O'Donoghue wrote:
>>>>> On 02/07/2025 13:01, Vikash Garodia wrote:
>>>>>>> Anyway, in other messages I explained what is missing. You are changing
>>>>>>> existing hardware and you clearly must explain how existing hardware is
>>>>>>> affected, how can we reproduce it, how users are affected.
>>>>>> Exactly all of these i have explained in the commit message. The limitation with
>>>>>> existing hardware binding usage and how my new approach mitigates that limition.
>>>>>>
>>>>>> Coming to usecase, i made a generic comment saying usecases which needs higher
>>>>>> IOVA, i can add the explicit detail about usecase like 8k or higher
>>>>>> concurrencies like 32 or higher concurrent sessions.
>>>>>
>>>>> Why not make this change for a new SoC, instead of an existing ?
>>>>
>>>> Because we definitely want to improve support for older SoCs too.
>>>
>>> Older SoCs came with completely new drivers and bindings, instead of
>>> evolving existing Venus, so they for sure came with correct code and
>>> correct binding.
>>
>> No, this is a terrible assumption to make, and we've been
>> through this time and time again - a huge portion of the code
>> submitted in the early days of linux-arm-msm did the bare minimum
> 
> We do not talk about early days of linux-arm-msm, but latest where they
> rejected existing venus drivers and instead insisted on completely new
> driver iris. This is a new code, so how early days are applicable?
> 
>> to present a feature, without giving much thought to the sanity of
>> hw description, be it on a block or platform level.
> 
> You are saying that iris driver was again shoved without any sanity? It
> should have never been merged then. Better to grow existing insanity
> than allow to have two insanities - old venus and new iris.

Iris was created with the hard requirement of being compatible with the
bindings previously consumed by Venus. I think the logical consequences
of that are rather clear.


Perhaps you're saying that the binding for "newer" ("not previously
supported by venus") platforms should have included that from the start,
and I agree, that would have been better, but hindsight's always 20/20.

On a flip side, any additional requirements we talk about here, also
apply (in reality/hw, not talking about current bindings/driver state) to
every single "older" platform as well, and skipping them is pushing your
luck every time you access the hardware.


I also don't think it's fair to leave them in a permanently-suboptimal
state just because the initial submission wasn't forward-looking to
these previously-unimplemented requirements. I'd even say that if we
want to fix it, we should do it sooner than later, before the bindings
age and get more users that would care about breakage.

Comparing against downstream, I only really see IOMMU specifics (binding
SIDs to a PA range, which this series touches upon plus ensuring secure
buffers are associated with a specific SID, which is done basically the
same way) and (on some targets) an nvmem-cell for speedbinning.
Everything else (PDs, clks, icc, irq, etc.), we've already covered.

>> That's why we're still adding clocks to mdss, regulators to camera
>> etc. etc. to this day. And it's only going to get worse when there
>> will be a need or will to add S2disk support with register
> 
> We speak about iris here only.

Sure, I'm using the other ones as an example to show you the actual
root cause of the problem. It's the same "the initial bindings
submission was not perfect and to make better use of the hardware, we
need to describe more resources / describe them more precisely"
problem that pops up every now and then and is actually difficult
to prevent.

Maybe we can have some sort of a broader conversation about whether
bindings from before SOME_DATE or e.g. not older than N kernel releases
back should be deemed volatile, but that is a story for another day.

Back to the point, I actually think what this patchset does is
resonable, especially given the address range and SMMU SID requirements
that the OS *must* be aware of (or the device will crash after a
translation fault / security violation).

Konrad