.../bindings/media/qcom,sm8550-iris.yaml | 40 ++++++++++++++++- drivers/media/platform/qcom/iris/iris_buffer.c | 15 ++++++- drivers/media/platform/qcom/iris/iris_core.h | 2 + drivers/media/platform/qcom/iris/iris_hfi_queue.c | 20 ++++++--- drivers/media/platform/qcom/iris/iris_probe.c | 50 +++++++++++++++++++++- drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++ 6 files changed, 119 insertions(+), 12 deletions(-)
This series introduces a sub node "non-pixel" within iris video node.
Video driver registers this sub node as a platform device and configure
it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues
and internal buffers related to bitstream processing, would be managed
by this non_pixel device.
Purpose to add this sub-node:
Iris device limits the IOVA to an addressable range of 4GiB, and even
within that range, some of the space is used by IO registers, thereby
limiting the available IOVA to even lesser. For certain video usecase,
this limited range in not sufficient enough, hence it brings the need to
extend the possibility of higher IOVA range.
Video hardware is designed to emit different stream-ID for pixel and
non-pixel buffers, thereby introduce a non-pixel sub node to handle
non-pixel stream-ID into a separate platform device.
With this, both iris and non-pixel device can have IOVA range of
approximately 0-4GiB individually for each device, thereby doubling the
range of addressable IOVA.
Tested on SM8550 and SA8775p hardwares.
Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
Changes in v3:
- Add info about change in iommus binding (Thanks Krzysztof)
- Link to v2: https://lore.kernel.org/r/20250627-video_cb-v2-0-3931c3f49361@quicinc.com
Changes in v2:
- Add ref to reserve-memory schema and drop it from redefining it in
iris schema (Thanks Krzysztof)
- Drop underscores and add info about non pixel buffers (Thanks Dmitry)
- Link to v1: https://lore.kernel.org/r/20250620-video_cb-v1-0-9bcac1c8800c@quicinc.com
---
Vikash Garodia (5):
media: dt-bindings: add non-pixel property in iris schema
media: iris: register and configure non-pixel node as platform device
media: iris: use np_dev as preferred DMA device in HFI queue management
media: iris: select appropriate DMA device for internal buffers
media: iris: configure DMA device for vb2 queue on OUTPUT plane
.../bindings/media/qcom,sm8550-iris.yaml | 40 ++++++++++++++++-
drivers/media/platform/qcom/iris/iris_buffer.c | 15 ++++++-
drivers/media/platform/qcom/iris/iris_core.h | 2 +
drivers/media/platform/qcom/iris/iris_hfi_queue.c | 20 ++++++---
drivers/media/platform/qcom/iris/iris_probe.c | 50 +++++++++++++++++++++-
drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++
6 files changed, 119 insertions(+), 12 deletions(-)
---
base-commit: 8d2b7fde56597ca912f5daaf3ab58915458ba1fc
change-id: 20250619-video_cb-ea872d6e6627
Best regards,
--
Vikash Garodia <quic_vgarodia@quicinc.com>
On 27/06/2025 17:48, Vikash Garodia wrote: > This series introduces a sub node "non-pixel" within iris video node. > Video driver registers this sub node as a platform device and configure > it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues > and internal buffers related to bitstream processing, would be managed > by this non_pixel device. > > Purpose to add this sub-node: > Iris device limits the IOVA to an addressable range of 4GiB, and even > within that range, some of the space is used by IO registers, thereby > limiting the available IOVA to even lesser. For certain video usecase, > this limited range in not sufficient enough, hence it brings the need to > extend the possibility of higher IOVA range. > > Video hardware is designed to emit different stream-ID for pixel and > non-pixel buffers, thereby introduce a non-pixel sub node to handle > non-pixel stream-ID into a separate platform device. > With this, both iris and non-pixel device can have IOVA range of > approximately 0-4GiB individually for each device, thereby doubling the > range of addressable IOVA. > > Tested on SM8550 and SA8775p hardwares. > > Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> > --- > Changes in v3: > - Add info about change in iommus binding (Thanks Krzysztof) Nothing improved in commit msg. You are changing existing device and the reason for that change is not communicated at all. There was big feedback from qualcomm saying that some commit in the past received review, so future commits can repeat the same stuff. If qcom approaches that way, sorry, no you need to come with proper commit description. Please align internally how to solve it, because my response that past imperfect review is not justification for whatever future issues was not enough. Best regards, Krzysztof
On 7/2/2025 4:48 PM, Krzysztof Kozlowski wrote: > On 27/06/2025 17:48, Vikash Garodia wrote: >> This series introduces a sub node "non-pixel" within iris video node. >> Video driver registers this sub node as a platform device and configure >> it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues >> and internal buffers related to bitstream processing, would be managed >> by this non_pixel device. >> >> Purpose to add this sub-node: >> Iris device limits the IOVA to an addressable range of 4GiB, and even >> within that range, some of the space is used by IO registers, thereby >> limiting the available IOVA to even lesser. For certain video usecase, >> this limited range in not sufficient enough, hence it brings the need to >> extend the possibility of higher IOVA range. >> >> Video hardware is designed to emit different stream-ID for pixel and >> non-pixel buffers, thereby introduce a non-pixel sub node to handle >> non-pixel stream-ID into a separate platform device. >> With this, both iris and non-pixel device can have IOVA range of >> approximately 0-4GiB individually for each device, thereby doubling the >> range of addressable IOVA. >> >> Tested on SM8550 and SA8775p hardwares. >> >> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >> --- >> Changes in v3: >> - Add info about change in iommus binding (Thanks Krzysztof) > > Nothing improved in commit msg. You are changing existing device and the > reason for that change is not communicated at all. > > There was big feedback from qualcomm saying that some commit in the past > received review, so future commits can repeat the same stuff. If qcom > approaches that way, sorry, no you need to come with proper commit > description. > > Please align internally how to solve it, because my response that past > imperfect review is not justification for whatever future issues was not > enough. Sure, lets take this as an example and you can suggest to provide a better commit message for this case, it would help me to compare where is the gap. I have tried my best to capture and explain the limitations and how the changes address those limitations. If that is not sufficient, we might have the perfect message from you and compare to find the gaps and improve, I am sorry, but thats how i feel at the moment. > > Best regards, > Krzysztof
On 02/07/2025 13:37, Vikash Garodia wrote: > > On 7/2/2025 4:48 PM, Krzysztof Kozlowski wrote: >> On 27/06/2025 17:48, Vikash Garodia wrote: >>> This series introduces a sub node "non-pixel" within iris video node. >>> Video driver registers this sub node as a platform device and configure >>> it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues >>> and internal buffers related to bitstream processing, would be managed >>> by this non_pixel device. >>> >>> Purpose to add this sub-node: >>> Iris device limits the IOVA to an addressable range of 4GiB, and even >>> within that range, some of the space is used by IO registers, thereby >>> limiting the available IOVA to even lesser. For certain video usecase, >>> this limited range in not sufficient enough, hence it brings the need to >>> extend the possibility of higher IOVA range. >>> >>> Video hardware is designed to emit different stream-ID for pixel and >>> non-pixel buffers, thereby introduce a non-pixel sub node to handle >>> non-pixel stream-ID into a separate platform device. >>> With this, both iris and non-pixel device can have IOVA range of >>> approximately 0-4GiB individually for each device, thereby doubling the >>> range of addressable IOVA. >>> >>> Tested on SM8550 and SA8775p hardwares. >>> >>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >>> --- >>> Changes in v3: >>> - Add info about change in iommus binding (Thanks Krzysztof) >> >> Nothing improved in commit msg. You are changing existing device and the >> reason for that change is not communicated at all. >> >> There was big feedback from qualcomm saying that some commit in the past >> received review, so future commits can repeat the same stuff. If qcom >> approaches that way, sorry, no you need to come with proper commit >> description. >> >> Please align internally how to solve it, because my response that past >> imperfect review is not justification for whatever future issues was not >> enough. > Sure, lets take this as an example and you can suggest to provide a better > commit message for this case, it would help me to compare where is the gap. I > have tried my best to capture and explain the limitations and how the changes > address those limitations. If that is not sufficient, we might have the perfect > message from you and compare to find the gaps and improve, I am sorry, but thats It is not question to me: I did not want imperfectness. Qualcomm engineer used issues in existing commits or imperfect commit in discussion, so that's my solution. I don't need that perfect commit, but it seems if I agree to that, then I will have to defend it later. Well, no, I don't want it. > how i feel at the moment. Sure, I feel confused now as well. 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. All these answers. Best regards, Krzysztof
On 7/2/2025 5:22 PM, Krzysztof Kozlowski wrote: > On 02/07/2025 13:37, Vikash Garodia wrote: >> >> On 7/2/2025 4:48 PM, Krzysztof Kozlowski wrote: >>> On 27/06/2025 17:48, Vikash Garodia wrote: >>>> This series introduces a sub node "non-pixel" within iris video node. >>>> Video driver registers this sub node as a platform device and configure >>>> it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues >>>> and internal buffers related to bitstream processing, would be managed >>>> by this non_pixel device. >>>> >>>> Purpose to add this sub-node: >>>> Iris device limits the IOVA to an addressable range of 4GiB, and even >>>> within that range, some of the space is used by IO registers, thereby >>>> limiting the available IOVA to even lesser. For certain video usecase, >>>> this limited range in not sufficient enough, hence it brings the need to >>>> extend the possibility of higher IOVA range. >>>> >>>> Video hardware is designed to emit different stream-ID for pixel and >>>> non-pixel buffers, thereby introduce a non-pixel sub node to handle >>>> non-pixel stream-ID into a separate platform device. >>>> With this, both iris and non-pixel device can have IOVA range of >>>> approximately 0-4GiB individually for each device, thereby doubling the >>>> range of addressable IOVA. >>>> >>>> Tested on SM8550 and SA8775p hardwares. >>>> >>>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >>>> --- >>>> Changes in v3: >>>> - Add info about change in iommus binding (Thanks Krzysztof) >>> >>> Nothing improved in commit msg. You are changing existing device and the >>> reason for that change is not communicated at all. >>> >>> There was big feedback from qualcomm saying that some commit in the past >>> received review, so future commits can repeat the same stuff. If qcom >>> approaches that way, sorry, no you need to come with proper commit >>> description. >>> >>> Please align internally how to solve it, because my response that past >>> imperfect review is not justification for whatever future issues was not >>> enough. >> Sure, lets take this as an example and you can suggest to provide a better >> commit message for this case, it would help me to compare where is the gap. I >> have tried my best to capture and explain the limitations and how the changes >> address those limitations. If that is not sufficient, we might have the perfect >> message from you and compare to find the gaps and improve, I am sorry, but thats > > It is not question to me: I did not want imperfectness. Qualcomm > engineer used issues in existing commits or imperfect commit in > discussion, so that's my solution. I don't need that perfect commit, but > it seems if I agree to that, then I will have to defend it later. Well, > no, I don't want it. > >> how i feel at the moment. > Sure, I feel confused now as well. > > 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. > > All these answers. > > Best regards, > Krzysztof
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 ? That way you don't have to make the argument for retrospective ABI changes. --- bod
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. > > That way you don't have to make the argument for retrospective ABI changes. > > --- > bod -- With best wishes Dmitry
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. That was one of the reasons why duplicating venus was accepted: to get things right, so obviously your argument cannot be true, right? Best regards, Krzysztof
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 to present a feature, without giving much thought to the sanity of hw description, be it on a block or platform level. 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 save/restore.. Konrad > > That was one of the reasons why duplicating venus was accepted: to get > things right, so obviously your argument cannot be true, right? > > Best regards, > Krzysztof >
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. > > 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. Best regards, Krzysztof
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
On 03/07/2025 16:28, Konrad Dybcio wrote:
> 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).
I still give my RB for the series.
To me the only question is should it be applied to sm8550 or to new SoCs
from now on, sa8775p, x1e and derivatives.
I take the point on ABI breakage on 8550, so to me it seems fully
consistent with Krzysztof's statements on ABI maintenance and indeed the
need to expand the features of this driver to do so from the next
submission onwards.
That can be as simple as
schema.yaml
if compatible newsoc
minItems:1
maxItems:2
8550's ABI is stable and new SoC submissions will support the
secure/non-pixel method.
---
bod
On Thu, Jul 03, 2025 at 09:28:12PM +0100, Bryan O'Donoghue wrote: > On 03/07/2025 16:28, Konrad Dybcio wrote: > > 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). > > I still give my RB for the series. > > To me the only question is should it be applied to sm8550 or to new SoCs > from now on, sa8775p, x1e and derivatives. I think we need to apply it to _all_ SoCs, maybe starting from MSM8x26 and MSM8x16. Likewise, we need to think bout secure buffers usecase. And once we do that, we will realize that it also changes the ABI for all SoCs that support either Venus or Iris. > I take the point on ABI breakage on 8550, so to me it seems fully consistent > with Krzysztof's statements on ABI maintenance and indeed the need to expand > the features of this driver to do so from the next submission onwards. > > That can be as simple as > > schema.yaml > > if compatible newsoc > minItems:1 > maxItems:2 > > 8550's ABI is stable and new SoC submissions will support the > secure/non-pixel method. -- With best wishes Dmitry
On 03/07/2025 22:23, Dmitry Baryshkov wrote: >> I still give my RB for the series. >> >> To me the only question is should it be applied to sm8550 or to new SoCs >> from now on, sa8775p, x1e and derivatives. > I think we need to apply it to_all_ SoCs, maybe starting from MSM8x26 > and MSM8x16. Likewise, we need to think bout secure buffers usecase. And > once we do that, we will realize that it also changes the ABI for all > SoCs that support either Venus or Iris. I think a dts change is a non-starter as its an ABI break. So if ABI break is out and reworking future dts to allow for a new device is out, then some API change is needed to allow the driver to stop the kernel automatically setting up the IOMMUs, create the new device as a platform device not dependent on DT change and have the probe() of the relevant drivers setup their own IOMMU extents based on - probably indexes we have in the driver configuration parameters. --- bod
On 7/4/2025 1:53 PM, Bryan O'Donoghue wrote: > On 03/07/2025 22:23, Dmitry Baryshkov wrote: >>> I still give my RB for the series. >>> >>> To me the only question is should it be applied to sm8550 or to new SoCs >>> from now on, sa8775p, x1e and derivatives. >> I think we need to apply it to_all_ SoCs, maybe starting from MSM8x26 >> and MSM8x16. Likewise, we need to think bout secure buffers usecase. And >> once we do that, we will realize that it also changes the ABI for all >> SoCs that support either Venus or Iris. > > I think a dts change is a non-starter as its an ABI break. > > So if ABI break is out and reworking future dts to allow for a new device is > out, then some API change is needed to allow the driver to stop the kernel > automatically setting up the IOMMUs, create the new device as a platform device > not dependent on DT change and have the probe() of the relevant drivers setup > their own IOMMU extents based on - probably indexes we have in the driver > configuration parameters. The concept of sub device is nothing new, it has been there for firmware iommus on venus bindings [1] and i am just extending it for non-pixel on iris. So instead of complicating the driver, pls relook into the existing solution which looks much simpler. The proposal to dis-associate from DT would certainly bite us in future when we have more and more iommu configurations, lets say "qcom,iommu-vmid" [2], comes in for managing secure stream-ids. [1] https://elixir.bootlin.com/linux/v6.16-rc4/source/Documentation/devicetree/bindings/media/qcom,venus-common.yaml#L50 [2] https://lore.kernel.org/all/20231101071144.16309-2-quic_gkohli@quicinc.com/
On Sat, Jul 05, 2025 at 12:45:10AM +0530, Vikash Garodia wrote: > > On 7/4/2025 1:53 PM, Bryan O'Donoghue wrote: > > On 03/07/2025 22:23, Dmitry Baryshkov wrote: > >>> I still give my RB for the series. > >>> > >>> To me the only question is should it be applied to sm8550 or to new SoCs > >>> from now on, sa8775p, x1e and derivatives. > >> I think we need to apply it to_all_ SoCs, maybe starting from MSM8x26 > >> and MSM8x16. Likewise, we need to think bout secure buffers usecase. And > >> once we do that, we will realize that it also changes the ABI for all > >> SoCs that support either Venus or Iris. > > > > I think a dts change is a non-starter as its an ABI break. > > > > So if ABI break is out and reworking future dts to allow for a new device is > > out, then some API change is needed to allow the driver to stop the kernel > > automatically setting up the IOMMUs, create the new device as a platform device > > not dependent on DT change and have the probe() of the relevant drivers setup > > their own IOMMU extents based on - probably indexes we have in the driver > > configuration parameters. > > The concept of sub device is nothing new, it has been there for firmware iommus > on venus bindings [1] and i am just extending it for non-pixel on iris. So > instead of complicating the driver, pls relook into the existing solution which > looks much simpler. We should not be using sub-devices. They don't describe any actual block or device in the hardware, as such they are frowned upon by DT bindings maintainers. > > The proposal to dis-associate from DT would certainly bite us in future when we > have more and more iommu configurations, lets say "qcom,iommu-vmid" [2], comes > in for managing secure stream-ids. > > [1] > https://elixir.bootlin.com/linux/v6.16-rc4/source/Documentation/devicetree/bindings/media/qcom,venus-common.yaml#L50 > > [2] https://lore.kernel.org/all/20231101071144.16309-2-quic_gkohli@quicinc.com/ -- With best wishes Dmitry
On Fri, Jul 04, 2025 at 09:23:06AM +0100, Bryan O'Donoghue wrote: > On 03/07/2025 22:23, Dmitry Baryshkov wrote: > > > I still give my RB for the series. > > > > > > To me the only question is should it be applied to sm8550 or to new SoCs > > > from now on, sa8775p, x1e and derivatives. > > I think we need to apply it to_all_ SoCs, maybe starting from MSM8x26 > > and MSM8x16. Likewise, we need to think bout secure buffers usecase. And > > once we do that, we will realize that it also changes the ABI for all > > SoCs that support either Venus or Iris. > > I think a dts change is a non-starter as its an ABI break. > > So if ABI break is out and reworking future dts to allow for a new device is > out, then some API change is needed to allow the driver to stop the kernel > automatically setting up the IOMMUs, create the new device as a platform > device not dependent on DT change and have the probe() of the relevant > drivers setup their own IOMMU extents based on - probably indexes we have in > the driver configuration parameters. What about instead: - keep IOMMU entries as is - Add iommu-maps, mapping the non-pixel SID - In future expand iommu-maps, describing the secure contexts? > > --- > bod -- With best wishes Dmitry
On 7/4/2025 10:15 PM, Dmitry Baryshkov wrote:
> On Fri, Jul 04, 2025 at 09:23:06AM +0100, Bryan O'Donoghue wrote:
>> On 03/07/2025 22:23, Dmitry Baryshkov wrote:
>>>> I still give my RB for the series.
>>>>
>>>> To me the only question is should it be applied to sm8550 or to new SoCs
>>>> from now on, sa8775p, x1e and derivatives.
>>> I think we need to apply it to_all_ SoCs, maybe starting from MSM8x26
>>> and MSM8x16. Likewise, we need to think bout secure buffers usecase. And
>>> once we do that, we will realize that it also changes the ABI for all
>>> SoCs that support either Venus or Iris.
>> I think a dts change is a non-starter as its an ABI break.
>>
>> So if ABI break is out and reworking future dts to allow for a new device is
>> out, then some API change is needed to allow the driver to stop the kernel
>> automatically setting up the IOMMUs, create the new device as a platform
>> device not dependent on DT change and have the probe() of the relevant
>> drivers setup their own IOMMU extents based on - probably indexes we have in
>> the driver configuration parameters.
>
> What about instead:
>
> - keep IOMMU entries as is
> - Add iommu-maps, mapping the non-pixel SID
Otherways to avoid the ABI breakage:
a) Keep iommus max items as 2, which is unchanged.
b) Repeat the same sid for both entries, i.e.,
iommus = <&apps_smmu 0x1940 0x0000>,
- <&apps_smmu 0x1947 0x0000>;
+ <&apps_smmu 0x1940 0x0000>;
and then create the new device as a platform device independent of dt.
RFC for this is posted[1].
However, Dmitry(in offline forums) termed creating 2 same sid entries
as -- "ridiculous band-aid just to fulfill the "ABI" requirements and
really doesn't make sense". Looks Fair.
OTOH, To exactly implement the idea mentioned here, I have the below
questions, can you please help me with:
1) By keeping the entries in 'iommu=' as is, then add non-pixel sid in
iommu-maps actually creates the overlapping entries.
So, IIUC -- this requires changes to the iommu driver to ignore the
setting up the non-pixel sid(or whatever is mentioned in iommu-maps) and
then use newly created platform device to program the entries mentioned
in iommu-maps. Please CMIW.
If the above understanding correct -- Doesn't it look like we are trying
to fix in the iommu driver for the problem statement related to dt bindings?
2) However, I still think that problem statement of __non-eligibility
for video IP to create the sub devices in the dt__ is still valid and
can be taken separately and actually [1] is targetting this. I believe
this is atleast required for secure contexts. please CMIW.
[1]
https://lore.kernel.org/all/20250928171718.436440-1-charan.kalla@oss.qualcomm.com/
Thanks,
Charan> - In future expand iommu-maps, describing the secure contexts?
On 07/10/2025 15:11, Charan Teja Kalla wrote: > a) Keep iommus max items as 2, which is unchanged. > b) Repeat the same sid for both entries, i.e., > iommus = <&apps_smmu 0x1940 0x0000>, > - <&apps_smmu 0x1947 0x0000>; > + <&apps_smmu 0x1940 0x0000>; > > and then create the new device as a platform device independent of dt. > RFC for this is posted[1]. If you are going to bury iommu data into the platform code in the driver, why do you need to modify this array at all ? The upstream DT should describe what is correct for the APSS - Linux. There's nothing to stop having an exotic set of platform code in drivers to setup SMMU entries for non-APSS system agents though. Seems a shame though - in the ideal the DT should describe the whole hardware and the FUNCTION_ID would be included into the iommu entries. Rob suggested using an implicit index for function id https://lore.kernel.org/all/CAL_JsqK9waZK=i+ov0jV-PonWSfddwHvE94Q+pks4zAEtKc+yg@mail.gmail.com/ Couldn't we list the entire set of iommus - then detach - subsequently re-attaching in our platform code with FUNCTION_IDs we keep listed in our drivers ? That way the DT is complete and correct, we have a compliant upstream DT but we also find a way to make the FUNCTION_ID specific setup we need. --- bod
On 07/10/2025 15:25, Bryan O'Donoghue wrote: > Rob suggested using an implicit index for function id > > https://lore.kernel.org/all/CAL_JsqK9waZK=i+ov0jV- > PonWSfddwHvE94Q+pks4zAEtKc+yg@mail.gmail.com/ > > Couldn't we list the entire set of iommus - then detach - subsequently > re-attaching in our platform code with FUNCTION_IDs we keep listed in > our drivers ? > > That way the DT is complete and correct, we have a compliant upstream DT > but we also find a way to make the FUNCTION_ID specific setup we need. i.e. you can keep the FUNCTION_ID "metadata" in the driver and associate specific iommu indexes with the FUNCTION_ID you want in there. That way you could have multiple FUNCTION_ID smmu entries in the DT and just associate the DT indexes locally in drivers/platform/qcom/iris_metadata_goes_here.c --- bod
On 07/10/2025 15:29, Bryan O'Donoghue wrote:
> On 07/10/2025 15:25, Bryan O'Donoghue wrote:
>> Rob suggested using an implicit index for function id
>>
>> https://lore.kernel.org/all/CAL_JsqK9waZK=i+ov0jV-
>> PonWSfddwHvE94Q+pks4zAEtKc+yg@mail.gmail.com/
>>
>> Couldn't we list the entire set of iommus - then detach - subsequently
>> re-attaching in our platform code with FUNCTION_IDs we keep listed in
>> our drivers ?
>>
>> That way the DT is complete and correct, we have a compliant upstream DT
>> but we also find a way to make the FUNCTION_ID specific setup we need.
>
> i.e. you can keep the FUNCTION_ID "metadata" in the driver and associate
> specific iommu indexes with the FUNCTION_ID you want in there.
>
> That way you could have multiple FUNCTION_ID smmu entries in the DT and
> just associate the DT indexes locally in drivers/platform/qcom/
> iris_metadata_goes_here.c
>
> ---
> bod
Actually why can't we specify FUNCTION_ID in the iommus = <entries>
Surely we could do
#iommu-cells = <4>;
iommus = <&apps_smmu 0x420 0x2 FUNCTION_ID>;
and encode the real data we need directly in the iommus list...
---
bod
On Tue, 7 Oct 2025 at 17:44, Bryan O'Donoghue <bod@kernel.org> wrote: > > On 07/10/2025 15:29, Bryan O'Donoghue wrote: > > On 07/10/2025 15:25, Bryan O'Donoghue wrote: > >> Rob suggested using an implicit index for function id > >> > >> https://lore.kernel.org/all/CAL_JsqK9waZK=i+ov0jV- > >> PonWSfddwHvE94Q+pks4zAEtKc+yg@mail.gmail.com/ > >> > >> Couldn't we list the entire set of iommus - then detach - subsequently > >> re-attaching in our platform code with FUNCTION_IDs we keep listed in > >> our drivers ? > >> > >> That way the DT is complete and correct, we have a compliant upstream DT > >> but we also find a way to make the FUNCTION_ID specific setup we need. > > > > i.e. you can keep the FUNCTION_ID "metadata" in the driver and associate > > specific iommu indexes with the FUNCTION_ID you want in there. > > > > That way you could have multiple FUNCTION_ID smmu entries in the DT and > > just associate the DT indexes locally in drivers/platform/qcom/ > > iris_metadata_goes_here.c > > > > --- > > bod > > Actually why can't we specify FUNCTION_ID in the iommus = <entries> > > Surely we could do > > #iommu-cells = <4>; Because #iommu-cells is a part of the apps_smmu: device rather than a part ofthe iris/venus/GPU/display/etc. > iommus = <&apps_smmu 0x420 0x2 FUNCTION_ID>; > > and encode the real data we need directly in the iommus list... > > --- > bod -- With best wishes Dmitry
On 07/10/2025 20:40, Dmitry Baryshkov wrote: >> Surely we could do >> >> #iommu-cells = <4>; > Because #iommu-cells is a part of the apps_smmu: device rather than a > part ofthe iris/venus/GPU/display/etc. What's to stop us extending the definition for qcom platforms ? Rather than jumping through hoops in drivers, we can just encode the data in the DT. --- bod
On Tue, Oct 07, 2025 at 11:10:28PM +0100, Bryan O'Donoghue wrote: > On 07/10/2025 20:40, Dmitry Baryshkov wrote: > > > Surely we could do > > > > > > #iommu-cells = <4>; > > Because #iommu-cells is a part of the apps_smmu: device rather than a > > part ofthe iris/venus/GPU/display/etc. > > What's to stop us extending the definition for qcom platforms ? > > Rather than jumping through hoops in drivers, we can just encode the data in > the DT. The IOMMU driver or a ARM SMMU device itself has nothing to do with the way individual devices use it. As such, I don't think we should be extending #iommu-cells in the SMMU device. -- With best wishes Dmitry
On Wed, Oct 08, 2025 at 06:10:26AM +0300, Dmitry Baryshkov wrote: > On Tue, Oct 07, 2025 at 11:10:28PM +0100, Bryan O'Donoghue wrote: > > On 07/10/2025 20:40, Dmitry Baryshkov wrote: > > > > Surely we could do > > > > > > > > #iommu-cells = <4>; > > > Because #iommu-cells is a part of the apps_smmu: device rather than a > > > part ofthe iris/venus/GPU/display/etc. > > > > What's to stop us extending the definition for qcom platforms ? > > > > Rather than jumping through hoops in drivers, we can just encode the data in > > the DT. > > The IOMMU driver or a ARM SMMU device itself has nothing to do with the > way individual devices use it. As such, I don't think we should be > extending #iommu-cells in the SMMU device. ... and I don't think that we should be encoding IOMMUs or other definitions inside the driver, if we can express them clearly in DT. -- With best wishes Dmitry
On 04/07/2025 17:45, Dmitry Baryshkov wrote: > What about instead: > > - keep IOMMU entries as is ack > - Add iommu-maps, mapping the non-pixel SID > - In future expand iommu-maps, describing the secure contexts? Interesting, we are _adding_ so that's not an ABI break and if I'm reading the documentation right, there's no hard rule on what iommu-map defines i.e. nothing to preclude us from encoding the information we want here. This might work. drivers/pci/controller/dwc/pcie-qcom.c::qcom_pcie_config_sid_1_9_0() You can register your platform device to the SID map you parse. Worth an experiment. --- bod
On 7/5/2025 4:14 AM, Bryan O'Donoghue wrote: > On 04/07/2025 17:45, Dmitry Baryshkov wrote: >> What about instead: >> >> - keep IOMMU entries as is > ack > >> - Add iommu-maps, mapping the non-pixel SID >> - In future expand iommu-maps, describing the secure contexts? > > Interesting, we are _adding_ so that's not an ABI break and if I'm > reading the documentation right, there's no hard rule on what iommu-map > defines i.e. nothing to preclude us from encoding the information we > want here. > > This might work. > > drivers/pci/controller/dwc/pcie-qcom.c::qcom_pcie_config_sid_1_9_0() > > You can register your platform device to the SID map you parse. I see few limitations with using iommu-map here, some of these are listed in [1] 1. We can't specify SMR mask with iommu-map. 2. We can't specify different iommu-addresses range for specific contexts. 3. The secure CB support [2] would require vmid information associated with per context. I think this can't be provided with iommu-map. [1] https://lore.kernel.org/all/85137a8e-45be-3bb2-d094-79754fa2a8be@quicinc.com/ [2] https://lore.kernel.org/all/20231101071144.16309-2-quic_gkohli@quicinc.com/ Thanks, Prakash
On 7/10/25 8:18 PM, Prakash Gupta wrote:
>
>
> On 7/5/2025 4:14 AM, Bryan O'Donoghue wrote:
>> On 04/07/2025 17:45, Dmitry Baryshkov wrote:
>>> What about instead:
>>>
>>> - keep IOMMU entries as is
>> ack
>>
>>> - Add iommu-maps, mapping the non-pixel SID
>>> - In future expand iommu-maps, describing the secure contexts?
>>
>> Interesting, we are _adding_ so that's not an ABI break and if I'm
>> reading the documentation right, there's no hard rule on what iommu-map
>> defines i.e. nothing to preclude us from encoding the information we
>> want here.
>>
>> This might work.
>>
>> drivers/pci/controller/dwc/pcie-qcom.c::qcom_pcie_config_sid_1_9_0()
>>
>> You can register your platform device to the SID map you parse.
>
> I see few limitations with using iommu-map here, some of these are
> listed in [1]
>
> 1. We can't specify SMR mask with iommu-map.
> 2. We can't specify different iommu-addresses range for specific contexts.
> 3. The secure CB support [2] would require vmid information associated
> with per context. I think this can't be provided with iommu-map.
FWIW iommu-map should probably be evolved to support passing more
than one cell of iommu_spec in general.. For us specifically, it's
only a matter of time before some platform's PCIe controller
requires* we pass a non-zero SMR mask (unless we should be doing
that already somewhere..)
(* we can obviously do the masking manually and put the effective
values in dt, but "eeh")
Perhaps that can even be done without messing up backwards
compatiblity (treat it as pseudocode, def incomplete):
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 7043acd971a0..bca54035f90e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2068,9 +2068,12 @@ int of_map_id(const struct device_node *np, u32 id,
const char *map_name, const char *map_mask_name,
struct device_node **target, u32 *id_out)
{
+ const char *cells_prop_name __free(kfree);
u32 map_mask, masked_id;
+ u32 cell_count;
int map_len;
const __be32 *map = NULL;
+ int ret;
if (!np || !map_name || (!target && !id_out))
return -EINVAL;
@@ -2084,7 +2087,15 @@ int of_map_id(const struct device_node *np, u32 id,
return 0;
}
- if (!map_len || map_len % (4 * sizeof(*map))) {
+ cells_prop_name = kasprintf(GFP_KERNEL, "#%s-cells", map_name);
+ if (!cells_prop_name)
+ return -EINVAL;
+
+ ret = of_property_read_u32(np, cells_prop_name, &cell_count);
+ if (ret)
+ return ret;
+
+ if (!map_len || map_len % ((2 + cell_count + 1) * sizeof(*map))) {
pr_err("%pOF: Error: Bad %s length: %d\n", np,
map_name, map_len);
return -EINVAL;
@@ -2106,7 +2117,7 @@ int of_map_id(const struct device_node *np, u32 id,
u32 id_base = be32_to_cpup(map + 0);
u32 phandle = be32_to_cpup(map + 1);
u32 out_base = be32_to_cpup(map + 2);
- u32 id_len = be32_to_cpup(map + 3);
+ u32 id_len = be32_to_cpup(map + cell_count - 1);
if (id_base & ~map_mask) {
pr_err("%pOF: Invalid %s translation - %s-mask (0x%x) ignores id-base (0x%x)\n",
Konrad
On 04-Jul-25 10:23, Bryan O'Donoghue wrote: > On 03/07/2025 22:23, Dmitry Baryshkov wrote: >>> I still give my RB for the series. >>> >>> To me the only question is should it be applied to sm8550 or to new SoCs >>> from now on, sa8775p, x1e and derivatives. >> I think we need to apply it to_all_ SoCs, maybe starting from MSM8x26 >> and MSM8x16. Likewise, we need to think bout secure buffers usecase. And >> once we do that, we will realize that it also changes the ABI for all >> SoCs that support either Venus or Iris. > > I think a dts change is a non-starter as its an ABI break. > > So if ABI break is out and reworking future dts to allow for a new device is out, then some API change is needed to allow the driver to stop the kernel automatically setting up the IOMMUs, create the new device as a platform device not dependent on DT change and have the probe() of the relevant drivers setup their own IOMMU extents based on - probably indexes we have in the driver configuration parameters. FWIW not even counting the address space limitations, no video hw binding today is ""complete"" (with all related SIDs bound, secure or nonsecure) Konrad
On 02/07/2025 14:01, Vikash Garodia wrote: > > > On 7/2/2025 5:22 PM, Krzysztof Kozlowski wrote: >> On 02/07/2025 13:37, Vikash Garodia wrote: >>> >>> On 7/2/2025 4:48 PM, Krzysztof Kozlowski wrote: >>>> On 27/06/2025 17:48, Vikash Garodia wrote: >>>>> This series introduces a sub node "non-pixel" within iris video node. >>>>> Video driver registers this sub node as a platform device and configure >>>>> it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues >>>>> and internal buffers related to bitstream processing, would be managed >>>>> by this non_pixel device. >>>>> >>>>> Purpose to add this sub-node: >>>>> Iris device limits the IOVA to an addressable range of 4GiB, and even >>>>> within that range, some of the space is used by IO registers, thereby >>>>> limiting the available IOVA to even lesser. For certain video usecase, >>>>> this limited range in not sufficient enough, hence it brings the need to >>>>> extend the possibility of higher IOVA range. >>>>> >>>>> Video hardware is designed to emit different stream-ID for pixel and >>>>> non-pixel buffers, thereby introduce a non-pixel sub node to handle >>>>> non-pixel stream-ID into a separate platform device. >>>>> With this, both iris and non-pixel device can have IOVA range of >>>>> approximately 0-4GiB individually for each device, thereby doubling the >>>>> range of addressable IOVA. >>>>> >>>>> Tested on SM8550 and SA8775p hardwares. >>>>> >>>>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >>>>> --- >>>>> Changes in v3: >>>>> - Add info about change in iommus binding (Thanks Krzysztof) >>>> >>>> Nothing improved in commit msg. You are changing existing device and the >>>> reason for that change is not communicated at all. >>>> >>>> There was big feedback from qualcomm saying that some commit in the past >>>> received review, so future commits can repeat the same stuff. If qcom >>>> approaches that way, sorry, no you need to come with proper commit >>>> description. >>>> >>>> Please align internally how to solve it, because my response that past >>>> imperfect review is not justification for whatever future issues was not >>>> enough. >>> Sure, lets take this as an example and you can suggest to provide a better >>> commit message for this case, it would help me to compare where is the gap. I >>> have tried my best to capture and explain the limitations and how the changes >>> address those limitations. If that is not sufficient, we might have the perfect >>> message from you and compare to find the gaps and improve, I am sorry, but thats >> >> It is not question to me: I did not want imperfectness. Qualcomm >> engineer used issues in existing commits or imperfect commit in >> discussion, so that's my solution. I don't need that perfect commit, but >> it seems if I agree to that, then I will have to defend it later. Well, >> no, I don't want it. >> >>> how i feel at the moment. >> Sure, I feel confused now as well. >> >> 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 Well, no. I did not see reproduce steps, users affected, which boards, nothing like that. Your commit says "certain video usecases"... how is this specific? You are deflecting now questions. Point me then part of commit msg which answers to: "explain how existing hardware is affected" "how can we reproduce it" "how users are affected." Best regards, Krzysztof
On 7/2/2025 5:35 PM, Krzysztof Kozlowski wrote: > On 02/07/2025 14:01, Vikash Garodia wrote: >> >> >> On 7/2/2025 5:22 PM, Krzysztof Kozlowski wrote: >>> On 02/07/2025 13:37, Vikash Garodia wrote: >>>> >>>> On 7/2/2025 4:48 PM, Krzysztof Kozlowski wrote: >>>>> On 27/06/2025 17:48, Vikash Garodia wrote: >>>>>> This series introduces a sub node "non-pixel" within iris video node. >>>>>> Video driver registers this sub node as a platform device and configure >>>>>> it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues >>>>>> and internal buffers related to bitstream processing, would be managed >>>>>> by this non_pixel device. >>>>>> >>>>>> Purpose to add this sub-node: >>>>>> Iris device limits the IOVA to an addressable range of 4GiB, and even >>>>>> within that range, some of the space is used by IO registers, thereby >>>>>> limiting the available IOVA to even lesser. For certain video usecase, >>>>>> this limited range in not sufficient enough, hence it brings the need to >>>>>> extend the possibility of higher IOVA range. >>>>>> >>>>>> Video hardware is designed to emit different stream-ID for pixel and >>>>>> non-pixel buffers, thereby introduce a non-pixel sub node to handle >>>>>> non-pixel stream-ID into a separate platform device. >>>>>> With this, both iris and non-pixel device can have IOVA range of >>>>>> approximately 0-4GiB individually for each device, thereby doubling the >>>>>> range of addressable IOVA. >>>>>> >>>>>> Tested on SM8550 and SA8775p hardwares. >>>>>> >>>>>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >>>>>> --- >>>>>> Changes in v3: >>>>>> - Add info about change in iommus binding (Thanks Krzysztof) >>>>> >>>>> Nothing improved in commit msg. You are changing existing device and the >>>>> reason for that change is not communicated at all. >>>>> >>>>> There was big feedback from qualcomm saying that some commit in the past >>>>> received review, so future commits can repeat the same stuff. If qcom >>>>> approaches that way, sorry, no you need to come with proper commit >>>>> description. >>>>> >>>>> Please align internally how to solve it, because my response that past >>>>> imperfect review is not justification for whatever future issues was not >>>>> enough. >>>> Sure, lets take this as an example and you can suggest to provide a better >>>> commit message for this case, it would help me to compare where is the gap. I >>>> have tried my best to capture and explain the limitations and how the changes >>>> address those limitations. If that is not sufficient, we might have the perfect >>>> message from you and compare to find the gaps and improve, I am sorry, but thats >>> >>> It is not question to me: I did not want imperfectness. Qualcomm >>> engineer used issues in existing commits or imperfect commit in >>> discussion, so that's my solution. I don't need that perfect commit, but >>> it seems if I agree to that, then I will have to defend it later. Well, >>> no, I don't want it. >>> >>>> how i feel at the moment. >>> Sure, I feel confused now as well. >>> >>> 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 > > Well, no. > > I did not see reproduce steps, users affected, which boards, nothing > like that. > > Your commit says "certain video usecases"... how is this specific? > > You are deflecting now questions. Point me then part of commit msg which > answers to: > > "explain how existing hardware is affected" > > "how can we reproduce it" > > "how users are affected." Ack, will add further specifics covering above aspects. > > > > Best regards, > Krzysztof
On 02/07/2025 13:52, Krzysztof Kozlowski wrote: > On 02/07/2025 13:37, Vikash Garodia wrote: >> >> On 7/2/2025 4:48 PM, Krzysztof Kozlowski wrote: >>> On 27/06/2025 17:48, Vikash Garodia wrote: >>>> This series introduces a sub node "non-pixel" within iris video node. >>>> Video driver registers this sub node as a platform device and configure >>>> it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues >>>> and internal buffers related to bitstream processing, would be managed >>>> by this non_pixel device. >>>> >>>> Purpose to add this sub-node: >>>> Iris device limits the IOVA to an addressable range of 4GiB, and even >>>> within that range, some of the space is used by IO registers, thereby >>>> limiting the available IOVA to even lesser. For certain video usecase, >>>> this limited range in not sufficient enough, hence it brings the need to >>>> extend the possibility of higher IOVA range. >>>> >>>> Video hardware is designed to emit different stream-ID for pixel and >>>> non-pixel buffers, thereby introduce a non-pixel sub node to handle >>>> non-pixel stream-ID into a separate platform device. >>>> With this, both iris and non-pixel device can have IOVA range of >>>> approximately 0-4GiB individually for each device, thereby doubling the >>>> range of addressable IOVA. >>>> >>>> Tested on SM8550 and SA8775p hardwares. >>>> >>>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >>>> --- >>>> Changes in v3: >>>> - Add info about change in iommus binding (Thanks Krzysztof) >>> >>> Nothing improved in commit msg. You are changing existing device and the >>> reason for that change is not communicated at all. >>> >>> There was big feedback from qualcomm saying that some commit in the past >>> received review, so future commits can repeat the same stuff. If qcom >>> approaches that way, sorry, no you need to come with proper commit >>> description. >>> >>> Please align internally how to solve it, because my response that past >>> imperfect review is not justification for whatever future issues was not >>> enough. >> Sure, lets take this as an example and you can suggest to provide a better >> commit message for this case, it would help me to compare where is the gap. I >> have tried my best to capture and explain the limitations and how the changes >> address those limitations. If that is not sufficient, we might have the perfect >> message from you and compare to find the gaps and improve, I am sorry, but thats > > It is not question to me: I did not want imperfectness. Qualcomm Double negation... should be: "It is not question to me: I did not want perfectness. Qualcomm" > engineer used issues in existing commits or imperfect commit in > discussion, so that's my solution. I don't need that perfect commit, but > it seems if I agree to that, then I will have to defend it later. Well, > no, I don't want it. > >> how i feel at the moment. > Sure, I feel confused now as well. > > 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. > > All these answers. Best regards, Krzysztof
On 27/06/2025 17:48, Vikash Garodia wrote: > This series introduces a sub node "non-pixel" within iris video node. > Video driver registers this sub node as a platform device and configure > it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues > and internal buffers related to bitstream processing, would be managed > by this non_pixel device. > > Purpose to add this sub-node: > Iris device limits the IOVA to an addressable range of 4GiB, and even > within that range, some of the space is used by IO registers, thereby > limiting the available IOVA to even lesser. For certain video usecase, > this limited range in not sufficient enough, hence it brings the need to > extend the possibility of higher IOVA range. > > Video hardware is designed to emit different stream-ID for pixel and > non-pixel buffers, thereby introduce a non-pixel sub node to handle > non-pixel stream-ID into a separate platform device. > With this, both iris and non-pixel device can have IOVA range of > approximately 0-4GiB individually for each device, thereby doubling the > range of addressable IOVA. > > Tested on SM8550 and SA8775p hardwares. > > Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> > --- > Changes in v3: > - Add info about change in iommus binding (Thanks Krzysztof) > - Link to v2: https://lore.kernel.org/r/20250627-video_cb-v2-0-3931c3f49361@quicinc.com > > Changes in v2: > - Add ref to reserve-memory schema and drop it from redefining it in > iris schema (Thanks Krzysztof) > - Drop underscores and add info about non pixel buffers (Thanks Dmitry) > - Link to v1: https://lore.kernel.org/r/20250620-video_cb-v1-0-9bcac1c8800c@quicinc.com > > --- > Vikash Garodia (5): > media: dt-bindings: add non-pixel property in iris schema > media: iris: register and configure non-pixel node as platform device > media: iris: use np_dev as preferred DMA device in HFI queue management > media: iris: select appropriate DMA device for internal buffers > media: iris: configure DMA device for vb2 queue on OUTPUT plane > > .../bindings/media/qcom,sm8550-iris.yaml | 40 ++++++++++++++++- > drivers/media/platform/qcom/iris/iris_buffer.c | 15 ++++++- > drivers/media/platform/qcom/iris/iris_core.h | 2 + > drivers/media/platform/qcom/iris/iris_hfi_queue.c | 20 ++++++--- > drivers/media/platform/qcom/iris/iris_probe.c | 50 +++++++++++++++++++++- > drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++ > 6 files changed, 119 insertions(+), 12 deletions(-) > --- > base-commit: 8d2b7fde56597ca912f5daaf3ab58915458ba1fc > change-id: 20250619-video_cb-ea872d6e6627 > > Best regards, I tried the patchset on SM8550 QRD and SM8650 QRD/HDK and the system just reboots a few millisecond after probing iris, no error messages nor reboot to sahara mode. The DT changeset for reference: https://git.codelinaro.org/neil.armstrong/linux/-/commit/e1b3628469c038559a60d310386f006f353e3d59 Neil
On 6/30/2025 11:34 PM, neil.armstrong@linaro.org wrote: > On 27/06/2025 17:48, Vikash Garodia wrote: >> This series introduces a sub node "non-pixel" within iris video node. >> Video driver registers this sub node as a platform device and configure >> it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues >> and internal buffers related to bitstream processing, would be managed >> by this non_pixel device. >> >> Purpose to add this sub-node: >> Iris device limits the IOVA to an addressable range of 4GiB, and even >> within that range, some of the space is used by IO registers, thereby >> limiting the available IOVA to even lesser. For certain video usecase, >> this limited range in not sufficient enough, hence it brings the need to >> extend the possibility of higher IOVA range. >> >> Video hardware is designed to emit different stream-ID for pixel and >> non-pixel buffers, thereby introduce a non-pixel sub node to handle >> non-pixel stream-ID into a separate platform device. >> With this, both iris and non-pixel device can have IOVA range of >> approximately 0-4GiB individually for each device, thereby doubling the >> range of addressable IOVA. >> >> Tested on SM8550 and SA8775p hardwares. >> >> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >> --- >> Changes in v3: >> - Add info about change in iommus binding (Thanks Krzysztof) >> - Link to v2: >> https://lore.kernel.org/r/20250627-video_cb-v2-0-3931c3f49361@quicinc.com >> >> Changes in v2: >> - Add ref to reserve-memory schema and drop it from redefining it in >> iris schema (Thanks Krzysztof) >> - Drop underscores and add info about non pixel buffers (Thanks Dmitry) >> - Link to v1: >> https://lore.kernel.org/r/20250620-video_cb-v1-0-9bcac1c8800c@quicinc.com >> >> --- >> Vikash Garodia (5): >> media: dt-bindings: add non-pixel property in iris schema >> media: iris: register and configure non-pixel node as platform device >> media: iris: use np_dev as preferred DMA device in HFI queue management >> media: iris: select appropriate DMA device for internal buffers >> media: iris: configure DMA device for vb2 queue on OUTPUT plane >> >> .../bindings/media/qcom,sm8550-iris.yaml | 40 ++++++++++++++++- >> drivers/media/platform/qcom/iris/iris_buffer.c | 15 ++++++- >> drivers/media/platform/qcom/iris/iris_core.h | 2 + >> drivers/media/platform/qcom/iris/iris_hfi_queue.c | 20 ++++++--- >> drivers/media/platform/qcom/iris/iris_probe.c | 50 +++++++++++++++++++++- >> drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++ >> 6 files changed, 119 insertions(+), 12 deletions(-) >> --- >> base-commit: 8d2b7fde56597ca912f5daaf3ab58915458ba1fc >> change-id: 20250619-video_cb-ea872d6e6627 >> >> Best regards, > > I tried the patchset on SM8550 QRD and SM8650 QRD/HDK and the system just reboots > a few millisecond after probing iris, no error messages nor reboot to sahara mode. > > The DT changeset for reference: > https://git.codelinaro.org/neil.armstrong/linux/-/commit/e1b3628469c038559a60d310386f006f353e3d59 I was able to repro this case, the issue was due to a incorrect node name in driver. Fixing the name as per binding, fixes the issue for me. I have made the comment in your code branch [1], which should fix it for you as well. Please share your observations. Regards, Vikash [1] https://git.codelinaro.org/neil.armstrong/linux/-/commit/e1b3628469c038559a60d310386f006f353e3d59#note_23930047 > > Neil
On 01/07/2025 12:23, Vikash Garodia wrote: > > On 6/30/2025 11:34 PM, neil.armstrong@linaro.org wrote: >> On 27/06/2025 17:48, Vikash Garodia wrote: >>> This series introduces a sub node "non-pixel" within iris video node. >>> Video driver registers this sub node as a platform device and configure >>> it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues >>> and internal buffers related to bitstream processing, would be managed >>> by this non_pixel device. >>> >>> Purpose to add this sub-node: >>> Iris device limits the IOVA to an addressable range of 4GiB, and even >>> within that range, some of the space is used by IO registers, thereby >>> limiting the available IOVA to even lesser. For certain video usecase, >>> this limited range in not sufficient enough, hence it brings the need to >>> extend the possibility of higher IOVA range. >>> >>> Video hardware is designed to emit different stream-ID for pixel and >>> non-pixel buffers, thereby introduce a non-pixel sub node to handle >>> non-pixel stream-ID into a separate platform device. >>> With this, both iris and non-pixel device can have IOVA range of >>> approximately 0-4GiB individually for each device, thereby doubling the >>> range of addressable IOVA. >>> >>> Tested on SM8550 and SA8775p hardwares. >>> >>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >>> --- >>> Changes in v3: >>> - Add info about change in iommus binding (Thanks Krzysztof) >>> - Link to v2: >>> https://lore.kernel.org/r/20250627-video_cb-v2-0-3931c3f49361@quicinc.com >>> >>> Changes in v2: >>> - Add ref to reserve-memory schema and drop it from redefining it in >>> iris schema (Thanks Krzysztof) >>> - Drop underscores and add info about non pixel buffers (Thanks Dmitry) >>> - Link to v1: >>> https://lore.kernel.org/r/20250620-video_cb-v1-0-9bcac1c8800c@quicinc.com >>> >>> --- >>> Vikash Garodia (5): >>> media: dt-bindings: add non-pixel property in iris schema >>> media: iris: register and configure non-pixel node as platform device >>> media: iris: use np_dev as preferred DMA device in HFI queue management >>> media: iris: select appropriate DMA device for internal buffers >>> media: iris: configure DMA device for vb2 queue on OUTPUT plane >>> >>> .../bindings/media/qcom,sm8550-iris.yaml | 40 ++++++++++++++++- >>> drivers/media/platform/qcom/iris/iris_buffer.c | 15 ++++++- >>> drivers/media/platform/qcom/iris/iris_core.h | 2 + >>> drivers/media/platform/qcom/iris/iris_hfi_queue.c | 20 ++++++--- >>> drivers/media/platform/qcom/iris/iris_probe.c | 50 +++++++++++++++++++++- >>> drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++ >>> 6 files changed, 119 insertions(+), 12 deletions(-) >>> --- >>> base-commit: 8d2b7fde56597ca912f5daaf3ab58915458ba1fc >>> change-id: 20250619-video_cb-ea872d6e6627 >>> >>> Best regards, >> >> I tried the patchset on SM8550 QRD and SM8650 QRD/HDK and the system just reboots >> a few millisecond after probing iris, no error messages nor reboot to sahara mode. >> >> The DT changeset for reference: >> https://git.codelinaro.org/neil.armstrong/linux/-/commit/e1b3628469c038559a60d310386f006f353e3d59 > > I was able to repro this case, the issue was due to a incorrect node name in > driver. Fixing the name as per binding, fixes the issue for me. I have made the Because this code was never tested... I found this response now, after I briefly look at your code. Best regards, Krzysztof
Hi,
On 01/07/2025 12:23, Vikash Garodia wrote:
>
> On 6/30/2025 11:34 PM, neil.armstrong@linaro.org wrote:
>> On 27/06/2025 17:48, Vikash Garodia wrote:
>>> This series introduces a sub node "non-pixel" within iris video node.
>>> Video driver registers this sub node as a platform device and configure
>>> it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues
>>> and internal buffers related to bitstream processing, would be managed
>>> by this non_pixel device.
>>>
>>> Purpose to add this sub-node:
>>> Iris device limits the IOVA to an addressable range of 4GiB, and even
>>> within that range, some of the space is used by IO registers, thereby
>>> limiting the available IOVA to even lesser. For certain video usecase,
>>> this limited range in not sufficient enough, hence it brings the need to
>>> extend the possibility of higher IOVA range.
>>>
>>> Video hardware is designed to emit different stream-ID for pixel and
>>> non-pixel buffers, thereby introduce a non-pixel sub node to handle
>>> non-pixel stream-ID into a separate platform device.
>>> With this, both iris and non-pixel device can have IOVA range of
>>> approximately 0-4GiB individually for each device, thereby doubling the
>>> range of addressable IOVA.
>>>
>>> Tested on SM8550 and SA8775p hardwares.
>>>
>>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>>> ---
>>> Changes in v3:
>>> - Add info about change in iommus binding (Thanks Krzysztof)
>>> - Link to v2:
>>> https://lore.kernel.org/r/20250627-video_cb-v2-0-3931c3f49361@quicinc.com
>>>
>>> Changes in v2:
>>> - Add ref to reserve-memory schema and drop it from redefining it in
>>> iris schema (Thanks Krzysztof)
>>> - Drop underscores and add info about non pixel buffers (Thanks Dmitry)
>>> - Link to v1:
>>> https://lore.kernel.org/r/20250620-video_cb-v1-0-9bcac1c8800c@quicinc.com
>>>
>>> ---
>>> Vikash Garodia (5):
>>> media: dt-bindings: add non-pixel property in iris schema
>>> media: iris: register and configure non-pixel node as platform device
>>> media: iris: use np_dev as preferred DMA device in HFI queue management
>>> media: iris: select appropriate DMA device for internal buffers
>>> media: iris: configure DMA device for vb2 queue on OUTPUT plane
>>>
>>> .../bindings/media/qcom,sm8550-iris.yaml | 40 ++++++++++++++++-
>>> drivers/media/platform/qcom/iris/iris_buffer.c | 15 ++++++-
>>> drivers/media/platform/qcom/iris/iris_core.h | 2 +
>>> drivers/media/platform/qcom/iris/iris_hfi_queue.c | 20 ++++++---
>>> drivers/media/platform/qcom/iris/iris_probe.c | 50 +++++++++++++++++++++-
>>> drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++
>>> 6 files changed, 119 insertions(+), 12 deletions(-)
>>> ---
>>> base-commit: 8d2b7fde56597ca912f5daaf3ab58915458ba1fc
>>> change-id: 20250619-video_cb-ea872d6e6627
>>>
>>> Best regards,
>>
>> I tried the patchset on SM8550 QRD and SM8650 QRD/HDK and the system just reboots
>> a few millisecond after probing iris, no error messages nor reboot to sahara mode.
>>
>> The DT changeset for reference:
>> https://git.codelinaro.org/neil.armstrong/linux/-/commit/e1b3628469c038559a60d310386f006f353e3d59
>
> I was able to repro this case, the issue was due to a incorrect node name in
> driver. Fixing the name as per binding, fixes the issue for me. I have made the
> comment in your code branch [1], which should fix it for you as well. Please
> share your observations.
Indeed, with:
============><========================================
diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index 8da2b780395d..06657b42da49 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -3264,6 +3264,9 @@ &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
iommus = <&apps_smmu 0x1947 0>;
dma-coherent;
+ #address-cells = <2>;
+ #size-cells = <2>;
+
/*
* IRIS firmware is signed by vendors, only
* enable in boards where the proper signed firmware
diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index b53a9aa5adbf..7ada62ee411e 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -5011,6 +5011,9 @@ &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
dma-coherent;
+ #address-cells = <2>;
+ #size-cells = <2>;
+
/*
* IRIS firmware is signed by vendors, only
* enable in boards where the proper signed firmware
diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
index 8fe87e30bd40..c57645a60bc4 100644
--- a/drivers/media/platform/qcom/iris/iris_probe.c
+++ b/drivers/media/platform/qcom/iris/iris_probe.c
@@ -136,7 +136,7 @@ static int iris_init_non_pixel_node(struct iris_core *core)
struct device_node *np_node;
int ret;
- np_node = of_get_child_by_name(core->dev->of_node, "non_pixel");
+ np_node = of_get_child_by_name(core->dev->of_node, "non-pixel");
if (!np_node)
return 0;
============><========================================
It boots again and I can run some decodes on 8550 and 8650.
Thanks,
Neil
>
> Regards,
> Vikash
>
> [1]
> https://git.codelinaro.org/neil.armstrong/linux/-/commit/e1b3628469c038559a60d310386f006f353e3d59#note_23930047
>
>>
>> Neil
On 7/1/2025 6:49 PM, Neil Armstrong wrote: > Hi, > > On 01/07/2025 12:23, Vikash Garodia wrote: >> >> On 6/30/2025 11:34 PM, neil.armstrong@linaro.org wrote: >>> On 27/06/2025 17:48, Vikash Garodia wrote: >>>> This series introduces a sub node "non-pixel" within iris video node. >>>> Video driver registers this sub node as a platform device and configure >>>> it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues >>>> and internal buffers related to bitstream processing, would be managed >>>> by this non_pixel device. >>>> >>>> Purpose to add this sub-node: >>>> Iris device limits the IOVA to an addressable range of 4GiB, and even >>>> within that range, some of the space is used by IO registers, thereby >>>> limiting the available IOVA to even lesser. For certain video usecase, >>>> this limited range in not sufficient enough, hence it brings the need to >>>> extend the possibility of higher IOVA range. >>>> >>>> Video hardware is designed to emit different stream-ID for pixel and >>>> non-pixel buffers, thereby introduce a non-pixel sub node to handle >>>> non-pixel stream-ID into a separate platform device. >>>> With this, both iris and non-pixel device can have IOVA range of >>>> approximately 0-4GiB individually for each device, thereby doubling the >>>> range of addressable IOVA. >>>> >>>> Tested on SM8550 and SA8775p hardwares. >>>> >>>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >>>> --- >>>> Changes in v3: >>>> - Add info about change in iommus binding (Thanks Krzysztof) >>>> - Link to v2: >>>> https://lore.kernel.org/r/20250627-video_cb-v2-0-3931c3f49361@quicinc.com >>>> >>>> Changes in v2: >>>> - Add ref to reserve-memory schema and drop it from redefining it in >>>> iris schema (Thanks Krzysztof) >>>> - Drop underscores and add info about non pixel buffers (Thanks Dmitry) >>>> - Link to v1: >>>> https://lore.kernel.org/r/20250620-video_cb-v1-0-9bcac1c8800c@quicinc.com >>>> >>>> --- >>>> Vikash Garodia (5): >>>> media: dt-bindings: add non-pixel property in iris schema >>>> media: iris: register and configure non-pixel node as platform device >>>> media: iris: use np_dev as preferred DMA device in HFI queue management >>>> media: iris: select appropriate DMA device for internal buffers >>>> media: iris: configure DMA device for vb2 queue on OUTPUT plane >>>> >>>> .../bindings/media/qcom,sm8550-iris.yaml | 40 ++++++++++++++++- >>>> drivers/media/platform/qcom/iris/iris_buffer.c | 15 ++++++- >>>> drivers/media/platform/qcom/iris/iris_core.h | 2 + >>>> drivers/media/platform/qcom/iris/iris_hfi_queue.c | 20 ++++++--- >>>> drivers/media/platform/qcom/iris/iris_probe.c | 50 >>>> +++++++++++++++++++++- >>>> drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++ >>>> 6 files changed, 119 insertions(+), 12 deletions(-) >>>> --- >>>> base-commit: 8d2b7fde56597ca912f5daaf3ab58915458ba1fc >>>> change-id: 20250619-video_cb-ea872d6e6627 >>>> >>>> Best regards, >>> >>> I tried the patchset on SM8550 QRD and SM8650 QRD/HDK and the system just >>> reboots >>> a few millisecond after probing iris, no error messages nor reboot to sahara >>> mode. >>> >>> The DT changeset for reference: >>> https://git.codelinaro.org/neil.armstrong/linux/-/commit/e1b3628469c038559a60d310386f006f353e3d59 >> >> I was able to repro this case, the issue was due to a incorrect node name in >> driver. Fixing the name as per binding, fixes the issue for me. I have made the >> comment in your code branch [1], which should fix it for you as well. Please >> share your observations. > > Indeed, with: > ============><======================================== > diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi > b/arch/arm64/boot/dts/qcom/sm8550.dtsi > index 8da2b780395d..06657b42da49 100644 > --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi > @@ -3264,6 +3264,9 @@ &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>, > iommus = <&apps_smmu 0x1947 0>; > dma-coherent; > > + #address-cells = <2>; > + #size-cells = <2>; > + > /* > * IRIS firmware is signed by vendors, only > * enable in boards where the proper signed firmware > diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi > b/arch/arm64/boot/dts/qcom/sm8650.dtsi > index b53a9aa5adbf..7ada62ee411e 100644 > --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi > @@ -5011,6 +5011,9 @@ &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>, > > dma-coherent; > > + #address-cells = <2>; > + #size-cells = <2>; > + > /* > * IRIS firmware is signed by vendors, only > * enable in boards where the proper signed firmware > diff --git a/drivers/media/platform/qcom/iris/iris_probe.c > b/drivers/media/platform/qcom/iris/iris_probe.c > index 8fe87e30bd40..c57645a60bc4 100644 > --- a/drivers/media/platform/qcom/iris/iris_probe.c > +++ b/drivers/media/platform/qcom/iris/iris_probe.c > @@ -136,7 +136,7 @@ static int iris_init_non_pixel_node(struct iris_core *core) > struct device_node *np_node; > int ret; > > - np_node = of_get_child_by_name(core->dev->of_node, "non_pixel"); > + np_node = of_get_child_by_name(core->dev->of_node, "non-pixel"); > if (!np_node) > return 0; > > ============><======================================== > > It boots again and I can run some decodes on 8550 and 8650. Nice. Thank you for your efforts in trying out the patches. Would that be ok with you if i can put the tested-by tags in next revision with the amendments ? Regards, Vikash > > Thanks, > Neil > >> >> Regards, >> Vikash >> >> [1] >> https://git.codelinaro.org/neil.armstrong/linux/-/commit/e1b3628469c038559a60d310386f006f353e3d59#note_23930047 >> >>> >>> Neil >
On 01/07/2025 18:11, Vikash Garodia wrote: > > On 7/1/2025 6:49 PM, Neil Armstrong wrote: >> Hi, >> >> On 01/07/2025 12:23, Vikash Garodia wrote: >>> >>> On 6/30/2025 11:34 PM, neil.armstrong@linaro.org wrote: >>>> On 27/06/2025 17:48, Vikash Garodia wrote: >>>>> This series introduces a sub node "non-pixel" within iris video node. >>>>> Video driver registers this sub node as a platform device and configure >>>>> it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues >>>>> and internal buffers related to bitstream processing, would be managed >>>>> by this non_pixel device. >>>>> >>>>> Purpose to add this sub-node: >>>>> Iris device limits the IOVA to an addressable range of 4GiB, and even >>>>> within that range, some of the space is used by IO registers, thereby >>>>> limiting the available IOVA to even lesser. For certain video usecase, >>>>> this limited range in not sufficient enough, hence it brings the need to >>>>> extend the possibility of higher IOVA range. >>>>> >>>>> Video hardware is designed to emit different stream-ID for pixel and >>>>> non-pixel buffers, thereby introduce a non-pixel sub node to handle >>>>> non-pixel stream-ID into a separate platform device. >>>>> With this, both iris and non-pixel device can have IOVA range of >>>>> approximately 0-4GiB individually for each device, thereby doubling the >>>>> range of addressable IOVA. >>>>> >>>>> Tested on SM8550 and SA8775p hardwares. >>>>> >>>>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >>>>> --- >>>>> Changes in v3: >>>>> - Add info about change in iommus binding (Thanks Krzysztof) >>>>> - Link to v2: >>>>> https://lore.kernel.org/r/20250627-video_cb-v2-0-3931c3f49361@quicinc.com >>>>> >>>>> Changes in v2: >>>>> - Add ref to reserve-memory schema and drop it from redefining it in >>>>> iris schema (Thanks Krzysztof) >>>>> - Drop underscores and add info about non pixel buffers (Thanks Dmitry) >>>>> - Link to v1: >>>>> https://lore.kernel.org/r/20250620-video_cb-v1-0-9bcac1c8800c@quicinc.com >>>>> >>>>> --- >>>>> Vikash Garodia (5): >>>>> media: dt-bindings: add non-pixel property in iris schema >>>>> media: iris: register and configure non-pixel node as platform device >>>>> media: iris: use np_dev as preferred DMA device in HFI queue management >>>>> media: iris: select appropriate DMA device for internal buffers >>>>> media: iris: configure DMA device for vb2 queue on OUTPUT plane >>>>> >>>>> .../bindings/media/qcom,sm8550-iris.yaml | 40 ++++++++++++++++- >>>>> drivers/media/platform/qcom/iris/iris_buffer.c | 15 ++++++- >>>>> drivers/media/platform/qcom/iris/iris_core.h | 2 + >>>>> drivers/media/platform/qcom/iris/iris_hfi_queue.c | 20 ++++++--- >>>>> drivers/media/platform/qcom/iris/iris_probe.c | 50 >>>>> +++++++++++++++++++++- >>>>> drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++ >>>>> 6 files changed, 119 insertions(+), 12 deletions(-) >>>>> --- >>>>> base-commit: 8d2b7fde56597ca912f5daaf3ab58915458ba1fc >>>>> change-id: 20250619-video_cb-ea872d6e6627 >>>>> >>>>> Best regards, >>>> >>>> I tried the patchset on SM8550 QRD and SM8650 QRD/HDK and the system just >>>> reboots >>>> a few millisecond after probing iris, no error messages nor reboot to sahara >>>> mode. >>>> >>>> The DT changeset for reference: >>>> https://git.codelinaro.org/neil.armstrong/linux/-/commit/e1b3628469c038559a60d310386f006f353e3d59 >>> >>> I was able to repro this case, the issue was due to a incorrect node name in >>> driver. Fixing the name as per binding, fixes the issue for me. I have made the >>> comment in your code branch [1], which should fix it for you as well. Please >>> share your observations. >> >> Indeed, with: >> ============><======================================== >> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi >> b/arch/arm64/boot/dts/qcom/sm8550.dtsi >> index 8da2b780395d..06657b42da49 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi >> @@ -3264,6 +3264,9 @@ &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>, >> iommus = <&apps_smmu 0x1947 0>; >> dma-coherent; >> >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> /* >> * IRIS firmware is signed by vendors, only >> * enable in boards where the proper signed firmware >> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi >> b/arch/arm64/boot/dts/qcom/sm8650.dtsi >> index b53a9aa5adbf..7ada62ee411e 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi >> @@ -5011,6 +5011,9 @@ &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>, >> >> dma-coherent; >> >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> /* >> * IRIS firmware is signed by vendors, only >> * enable in boards where the proper signed firmware >> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c >> b/drivers/media/platform/qcom/iris/iris_probe.c >> index 8fe87e30bd40..c57645a60bc4 100644 >> --- a/drivers/media/platform/qcom/iris/iris_probe.c >> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >> @@ -136,7 +136,7 @@ static int iris_init_non_pixel_node(struct iris_core *core) >> struct device_node *np_node; >> int ret; >> >> - np_node = of_get_child_by_name(core->dev->of_node, "non_pixel"); >> + np_node = of_get_child_by_name(core->dev->of_node, "non-pixel"); >> if (!np_node) >> return 0; >> >> ============><======================================== >> >> It boots again and I can run some decodes on 8550 and 8650. > Nice. Thank you for your efforts in trying out the patches. Would that be ok > with you if i can put the tested-by tags in next revision with the amendments ? Sure please add: Tested-by: Neil Armstrong <neil.armstrong@linaro.org> with those changes Neil > > Regards, > Vikash >> >> Thanks, >> Neil >> >>> >>> Regards, >>> Vikash >>> >>> [1] >>> https://git.codelinaro.org/neil.armstrong/linux/-/commit/e1b3628469c038559a60d310386f006f353e3d59#note_23930047 >>> >>>> >>>> Neil >>
On 30-Jun-25 20:04, neil.armstrong@linaro.org wrote: > On 27/06/2025 17:48, Vikash Garodia wrote: >> This series introduces a sub node "non-pixel" within iris video node. >> Video driver registers this sub node as a platform device and configure >> it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues >> and internal buffers related to bitstream processing, would be managed >> by this non_pixel device. [...] > I tried the patchset on SM8550 QRD and SM8650 QRD/HDK and the system just reboots > a few millisecond after probing iris, no error messages nor reboot to sahara mode. > > The DT changeset for reference: > https://git.codelinaro.org/neil.armstrong/linux/-/commit/e1b3628469c038559a60d310386f006f353e3d59 I think that's because of the majorly suboptimal 'define disallowed ranges' approach with the iommu-addresses bindings.. 8550 (and most 64-bit QC SoCs) also have DRAM mapped above 32bits, meaning you'd have to add a whole lot of boilerplate to disallow these ranges as well Konrad
Hi, On 27/06/2025 17:48, Vikash Garodia wrote: > This series introduces a sub node "non-pixel" within iris video node. > Video driver registers this sub node as a platform device and configure > it for DMA operations. All non pixel buffers, i.e bitstream, HFI queues > and internal buffers related to bitstream processing, would be managed > by this non_pixel device. > > Purpose to add this sub-node: > Iris device limits the IOVA to an addressable range of 4GiB, and even > within that range, some of the space is used by IO registers, thereby > limiting the available IOVA to even lesser. For certain video usecase, > this limited range in not sufficient enough, hence it brings the need to > extend the possibility of higher IOVA range. > > Video hardware is designed to emit different stream-ID for pixel and > non-pixel buffers, thereby introduce a non-pixel sub node to handle > non-pixel stream-ID into a separate platform device. > With this, both iris and non-pixel device can have IOVA range of > approximately 0-4GiB individually for each device, thereby doubling the > range of addressable IOVA. > > Tested on SM8550 and SA8775p hardwares. Is there any test to validate this works correctly? Neil > > Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> > --- > Changes in v3: > - Add info about change in iommus binding (Thanks Krzysztof) > - Link to v2: https://lore.kernel.org/r/20250627-video_cb-v2-0-3931c3f49361@quicinc.com > > Changes in v2: > - Add ref to reserve-memory schema and drop it from redefining it in > iris schema (Thanks Krzysztof) > - Drop underscores and add info about non pixel buffers (Thanks Dmitry) > - Link to v1: https://lore.kernel.org/r/20250620-video_cb-v1-0-9bcac1c8800c@quicinc.com > > --- > Vikash Garodia (5): > media: dt-bindings: add non-pixel property in iris schema > media: iris: register and configure non-pixel node as platform device > media: iris: use np_dev as preferred DMA device in HFI queue management > media: iris: select appropriate DMA device for internal buffers > media: iris: configure DMA device for vb2 queue on OUTPUT plane > > .../bindings/media/qcom,sm8550-iris.yaml | 40 ++++++++++++++++- > drivers/media/platform/qcom/iris/iris_buffer.c | 15 ++++++- > drivers/media/platform/qcom/iris/iris_core.h | 2 + > drivers/media/platform/qcom/iris/iris_hfi_queue.c | 20 ++++++--- > drivers/media/platform/qcom/iris/iris_probe.c | 50 +++++++++++++++++++++- > drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++ > 6 files changed, 119 insertions(+), 12 deletions(-) > --- > base-commit: 8d2b7fde56597ca912f5daaf3ab58915458ba1fc > change-id: 20250619-video_cb-ea872d6e6627 > > Best regards,
On 27/06/2025 16:48, Vikash Garodia wrote: > Changes in v3: > - Add info about change in iommus binding (Thanks Krzysztof) > - Link to v2:https://lore.kernel.org/r/20250627-video_cb-v2-0-3931c3f49361@quicinc.com Hmm. I would be nice to see what also _wasn't_ done i.e. why you didn't do what Dmitry was suggesting, IMO that's as important as stating what you did change. Not a huge deal you explained in a response email your logic already but just as suggestion for future, I think its good practice to go through each point and say what you did do, what you didn't do, perhaps what you tried and then decided to do in a different way. --- bod
On 27/06/2025 18:30, Bryan O'Donoghue wrote: > On 27/06/2025 16:48, Vikash Garodia wrote: >> Changes in v3: >> - Add info about change in iommus binding (Thanks Krzysztof) >> - Link to v2:https://lore.kernel.org/r/20250627-video_cb-v2-0-3931c3f49361@quicinc.com > > Hmm. > > I would be nice to see what also _wasn't_ done i.e. why you didn't do > what Dmitry was suggesting, IMO that's as important as stating what you > did change. > > Not a huge deal you explained in a response email your logic already but It is a deal. Not doing what reviewers are asking is not the standard practice. > just as suggestion for future, I think its good practice to go through > each point and say what you did do, what you didn't do, perhaps what you > tried and then decided to do in a different way. Best regards, Krzysztof
On 6/27/2025 10:00 PM, Bryan O'Donoghue wrote: > On 27/06/2025 16:48, Vikash Garodia wrote: >> Changes in v3: >> - Add info about change in iommus binding (Thanks Krzysztof) >> - Link to >> v2:https://lore.kernel.org/r/20250627-video_cb-v2-0-3931c3f49361@quicinc.com > > Hmm. > > I would be nice to see what also _wasn't_ done i.e. why you didn't do what > Dmitry was suggesting, IMO that's as important as stating what you did change. > > Not a huge deal you explained in a response email your logic already but just as > suggestion for future, I think its good practice to go through each point and > say what you did do, what you didn't do, perhaps what you tried and then decided > to do in a different way. I see your point here, and it would be certainly good to document (as a summary in cover letter) the design approaches which was not considered stating the limitations as well to drop them out. Regards, Vikash > > --- > bod
© 2016 - 2025 Red Hat, Inc.