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

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

[Vikash Garodia]

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>

[PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Vikash Garodia]

Existing definition 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. 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.

With this, both iris and non-pixel device can have IOVA range of 0-4GiB
individually. Certain video usecases like higher video concurrency needs
IOVA higher than 4GiB.

Add reference to the reserve-memory schema, which defines reserved IOVA
regions that are *excluded* from addressable range. Video hardware
generates different stream IDs based on the predefined range of IOVA
addresses. Thereby IOVA addresses for firmware and data buffers need to
be non overlapping. For ex. 0x0-0x25800000 address range is reserved for
firmware stream-ID, while non-pixel (bitstream) stream-ID can be
generated by hardware only when bitstream buffers IOVA address is from
0x25800000-0xe0000000.
Non-pixel stream-ID can now be part of the new sub-node, hence iommus in
iris node can have either 1 entry for pixel stream-id or 2 entries for
pixel and non-pixel stream-ids.

Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
 .../bindings/media/qcom,sm8550-iris.yaml           | 40 ++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
index c79bf2101812d83b99704f38b7348a9f728dff44..4dda2c9ca1293baa7aee3b9ee10aff38d280fe05 100644
--- a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
@@ -65,10 +65,31 @@ properties:
       - const: core
 
   iommus:
+    minItems: 1
     maxItems: 2
 
   dma-coherent: true
 
+  non-pixel:
+    type: object
+    additionalProperties: false
+
+    description:
+      Non pixel context bank is needed when video hardware have distinct iommus
+      for non pixel buffers. Non pixel buffers are mainly compressed and
+      internal buffers.
+
+    properties:
+      iommus:
+        maxItems: 1
+
+      memory-region:
+        maxItems: 1
+
+    required:
+      - iommus
+      - memory-region
+
   operating-points-v2: true
 
   opp-table:
@@ -86,6 +107,7 @@ required:
 
 allOf:
   - $ref: qcom,venus-common.yaml#
+  - $ref: /schemas/reserved-memory/reserved-memory.yaml
   - if:
       properties:
         compatible:
@@ -117,6 +139,16 @@ examples:
     #include <dt-bindings/power/qcom-rpmpd.h>
     #include <dt-bindings/power/qcom,rpmhpd.h>
 
+    reserved-memory {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      iris_resv: reservation-iris {
+        iommu-addresses = <&iris_non_pixel 0x0 0x0 0x0 0x25800000>,
+                          <&iris_non_pixel 0x0 0xe0000000 0x0 0x20000000>;
+      };
+    };
+
     video-codec@aa00000 {
         compatible = "qcom,sm8550-iris";
         reg = <0x0aa00000 0xf0000>;
@@ -144,12 +176,16 @@ examples:
         resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
         reset-names = "bus";
 
-        iommus = <&apps_smmu 0x1940 0x0000>,
-                 <&apps_smmu 0x1947 0x0000>;
+        iommus = <&apps_smmu 0x1947 0x0000>;
         dma-coherent;
 
         operating-points-v2 = <&iris_opp_table>;
 
+        iris_non_pixel: non-pixel {
+            iommus = <&apps_smmu 0x1940 0x0000>;
+            memory-region = <&iris_resv>;
+        };
+
         iris_opp_table: opp-table {
             compatible = "operating-points-v2";
 

-- 
2.34.1

[PATCH v3 2/5] media: iris: register and configure non-pixel node as platform device

[Vikash Garodia]

Register "non_pixel" child node as a platform device, and configure its
DMA via of_dma_configure(). This ensures proper IOMMU attachment and DMA
setup for the "non_pixel" device.
All non pixel memory, i.e bitstream buffers, HFI queues and internal
buffers related to bitstream processing, would be managed by non_pixel
device.

Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
 drivers/media/platform/qcom/iris/iris_core.h  |  2 ++
 drivers/media/platform/qcom/iris/iris_probe.c | 50 ++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h
index aeeac32a1f6d9a9fa7027e8e3db4d95f021c552e..757efd16870876bd2b1d5b1e4103b2d2326b5f49 100644
--- a/drivers/media/platform/qcom/iris/iris_core.h
+++ b/drivers/media/platform/qcom/iris/iris_core.h
@@ -65,6 +65,7 @@ struct icc_info {
  * @sys_error_handler: a delayed work for handling system fatal error
  * @instances: a list_head of all instances
  * @inst_fw_caps: an array of supported instance capabilities
+ * @np_dev: device to represent non pixel node
  */
 
 struct iris_core {
@@ -105,6 +106,7 @@ struct iris_core {
 	struct delayed_work			sys_error_handler;
 	struct list_head			instances;
 	struct platform_inst_fw_cap		inst_fw_caps[INST_FW_CAP_MAX];
+	struct device				*np_dev;
 };
 
 int iris_core_init(struct iris_core *core);
diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
index 9a7ce142f7007ffcda0bd422c1983f2374bb0d92..8fe87e30bd40f3c67ec41305c7d73520fbc9db7b 100644
--- a/drivers/media/platform/qcom/iris/iris_probe.c
+++ b/drivers/media/platform/qcom/iris/iris_probe.c
@@ -6,6 +6,8 @@
 #include <linux/clk.h>
 #include <linux/interconnect.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
@@ -127,6 +129,46 @@ static int iris_init_resets(struct iris_core *core)
 				     core->iris_platform_data->controller_rst_tbl_size);
 }
 
+static int iris_init_non_pixel_node(struct iris_core *core)
+{
+	struct platform_device_info info;
+	struct platform_device *pdev;
+	struct device_node *np_node;
+	int ret;
+
+	np_node = of_get_child_by_name(core->dev->of_node, "non_pixel");
+	if (!np_node)
+		return 0;
+
+	memset(&info, 0, sizeof(info));
+	info.fwnode = &np_node->fwnode;
+	info.parent = core->dev;
+	info.name = np_node->name;
+	info.dma_mask = DMA_BIT_MASK(32);
+
+	pdev = platform_device_register_full(&info);
+	if (IS_ERR(pdev)) {
+		of_node_put(np_node);
+		return PTR_ERR(pdev);
+	}
+	pdev->dev.of_node = np_node;
+
+	ret = of_dma_configure(&pdev->dev, np_node, true);
+	if (ret)
+		goto err_unregister;
+
+	core->np_dev = &pdev->dev;
+
+	of_node_put(np_node);
+
+	return 0;
+
+err_unregister:
+	platform_device_unregister(pdev);
+	of_node_put(np_node);
+	return ret;
+}
+
 static int iris_init_resources(struct iris_core *core)
 {
 	int ret;
@@ -143,7 +185,11 @@ static int iris_init_resources(struct iris_core *core)
 	if (ret)
 		return ret;
 
-	return iris_init_resets(core);
+	ret = iris_init_resets(core);
+	if (ret)
+		return ret;
+
+	return iris_init_non_pixel_node(core);
 }
 
 static int iris_register_video_device(struct iris_core *core)
@@ -188,6 +234,8 @@ static void iris_remove(struct platform_device *pdev)
 
 	iris_core_deinit(core);
 
+	platform_device_unregister(to_platform_device(core->np_dev));
+
 	video_unregister_device(core->vdev_dec);
 
 	v4l2_device_unregister(&core->v4l2_dev);

-- 
2.34.1

[PATCH v3 3/5] media: iris: use np_dev as preferred DMA device in HFI queue management

[Vikash Garodia]

Update HFI interface queues to use np_dev(preferred non-pixel device)
for DMA memory allocation and deallocation if available. This allows
platforms with separate DMA domain for non-pixel to use the appropriate
device handle when managing HFI queues and SFR regions.

Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
 drivers/media/platform/qcom/iris/iris_hfi_queue.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/iris/iris_hfi_queue.c b/drivers/media/platform/qcom/iris/iris_hfi_queue.c
index fac7df0c4d1aec647aeca275ab19651c9ba23733..a31ebe947f525f0d7c09f8b786939d01b62532c3 100644
--- a/drivers/media/platform/qcom/iris/iris_hfi_queue.c
+++ b/drivers/media/platform/qcom/iris/iris_hfi_queue.c
@@ -247,24 +247,27 @@ static void iris_hfi_queue_deinit(struct iris_iface_q_info *iface_q)
 int iris_hfi_queues_init(struct iris_core *core)
 {
 	struct iris_hfi_queue_table_header *q_tbl_hdr;
+	struct device *dev;
 	u32 queue_size;
 
+	dev = core->np_dev ? core->np_dev : core->dev;
+
 	/* Iris hardware requires 4K queue alignment */
 	queue_size = ALIGN((sizeof(*q_tbl_hdr) + (IFACEQ_QUEUE_SIZE * IFACEQ_NUMQ)), SZ_4K);
-	core->iface_q_table_vaddr = dma_alloc_attrs(core->dev, queue_size,
+	core->iface_q_table_vaddr = dma_alloc_attrs(dev, queue_size,
 						    &core->iface_q_table_daddr,
 						    GFP_KERNEL, DMA_ATTR_WRITE_COMBINE);
 	if (!core->iface_q_table_vaddr) {
-		dev_err(core->dev, "queues alloc and map failed\n");
+		dev_err(dev, "queues alloc and map failed\n");
 		return -ENOMEM;
 	}
 
-	core->sfr_vaddr = dma_alloc_attrs(core->dev, SFR_SIZE,
+	core->sfr_vaddr = dma_alloc_attrs(dev, SFR_SIZE,
 					  &core->sfr_daddr,
 					  GFP_KERNEL, DMA_ATTR_WRITE_COMBINE);
 	if (!core->sfr_vaddr) {
-		dev_err(core->dev, "sfr alloc and map failed\n");
-		dma_free_attrs(core->dev, sizeof(*q_tbl_hdr), core->iface_q_table_vaddr,
+		dev_err(dev, "sfr alloc and map failed\n");
+		dma_free_attrs(dev, sizeof(*q_tbl_hdr), core->iface_q_table_vaddr,
 			       core->iface_q_table_daddr, DMA_ATTR_WRITE_COMBINE);
 		return -ENOMEM;
 	}
@@ -292,6 +295,7 @@ int iris_hfi_queues_init(struct iris_core *core)
 
 void iris_hfi_queues_deinit(struct iris_core *core)
 {
+	struct device *dev;
 	u32 queue_size;
 
 	if (!core->iface_q_table_vaddr)
@@ -301,7 +305,9 @@ void iris_hfi_queues_deinit(struct iris_core *core)
 	iris_hfi_queue_deinit(&core->message_queue);
 	iris_hfi_queue_deinit(&core->command_queue);
 
-	dma_free_attrs(core->dev, SFR_SIZE, core->sfr_vaddr,
+	dev = core->np_dev ? core->np_dev : core->dev;
+
+	dma_free_attrs(dev, SFR_SIZE, core->sfr_vaddr,
 		       core->sfr_daddr, DMA_ATTR_WRITE_COMBINE);
 
 	core->sfr_vaddr = NULL;
@@ -310,7 +316,7 @@ void iris_hfi_queues_deinit(struct iris_core *core)
 	queue_size = ALIGN(sizeof(struct iris_hfi_queue_table_header) +
 		(IFACEQ_QUEUE_SIZE * IFACEQ_NUMQ), SZ_4K);
 
-	dma_free_attrs(core->dev, queue_size, core->iface_q_table_vaddr,
+	dma_free_attrs(dev, queue_size, core->iface_q_table_vaddr,
 		       core->iface_q_table_daddr, DMA_ATTR_WRITE_COMBINE);
 
 	core->iface_q_table_vaddr = NULL;

-- 
2.34.1

[PATCH v3 4/5] media: iris: select appropriate DMA device for internal buffers

[Vikash Garodia]

When a non-pixel device (np_dev) exists, it is preferred for DMA
operations for internal buffers which are specific to bitstream data
processing. DPB(decoded picture buffer) buffers are internal buffers
associated with pixel buffers, hence they are not part of "non_pixel"
device.

Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
 drivers/media/platform/qcom/iris/iris_buffer.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c
index e5c5a564fcb81e77746df8c4797a10a07f2ae946..0bf6041936175d03a51985be148e78894fc3e990 100644
--- a/drivers/media/platform/qcom/iris/iris_buffer.c
+++ b/drivers/media/platform/qcom/iris/iris_buffer.c
@@ -265,6 +265,7 @@ static int iris_create_internal_buffer(struct iris_inst *inst,
 	struct iris_buffers *buffers = &inst->buffers[buffer_type];
 	struct iris_core *core = inst->core;
 	struct iris_buffer *buffer;
+	struct device *dev;
 
 	if (!buffers->size)
 		return 0;
@@ -280,7 +281,11 @@ static int iris_create_internal_buffer(struct iris_inst *inst,
 	buffer->dma_attrs = DMA_ATTR_WRITE_COMBINE | DMA_ATTR_NO_KERNEL_MAPPING;
 	list_add_tail(&buffer->list, &buffers->list);
 
-	buffer->kvaddr = dma_alloc_attrs(core->dev, buffer->buffer_size,
+	dev = core->np_dev ? core->np_dev : core->dev;
+	if (buffer->type == BUF_DPB)
+		dev = core->dev;
+
+	buffer->kvaddr = dma_alloc_attrs(dev, buffer->buffer_size,
 					 &buffer->device_addr, GFP_KERNEL, buffer->dma_attrs);
 	if (!buffer->kvaddr)
 		return -ENOMEM;
@@ -367,9 +372,15 @@ int iris_queue_internal_buffers(struct iris_inst *inst, u32 plane)
 int iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buffer)
 {
 	struct iris_core *core = inst->core;
+	struct device *dev;
+
+	dev = core->np_dev ? core->np_dev : core->dev;
+
+	if (buffer->type == BUF_DPB)
+		dev = core->dev;
 
 	list_del(&buffer->list);
-	dma_free_attrs(core->dev, buffer->buffer_size, buffer->kvaddr,
+	dma_free_attrs(dev, buffer->buffer_size, buffer->kvaddr,
 		       buffer->device_addr, buffer->dma_attrs);
 	kfree(buffer);
 

-- 
2.34.1

[PATCH v3 5/5] media: iris: configure DMA device for vb2 queue on OUTPUT plane

[Vikash Garodia]

While setting up the vb2 queues, assign "non_pixel" device to manage
OUTPUT plane buffers i.e bitstream buffers incase of decoder. It prefers
the non_pixel device(np_dev) when available, falling back to core->dev
otherwise.

Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
 drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c b/drivers/media/platform/qcom/iris/iris_vb2.c
index cdf11feb590b5cb7804db3fcde7282fb1f9f1a1e..01cc337970400d48063c558c1ac039539dbcbaba 100644
--- a/drivers/media/platform/qcom/iris/iris_vb2.c
+++ b/drivers/media/platform/qcom/iris/iris_vb2.c
@@ -159,6 +159,10 @@ int iris_vb2_queue_setup(struct vb2_queue *q,
 	*num_planes = 1;
 	sizes[0] = f->fmt.pix_mp.plane_fmt[0].sizeimage;
 
+	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT ||
+	    q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
+		q->dev = core->np_dev ? core->np_dev : core->dev;
+
 unlock:
 	mutex_unlock(&inst->lock);
 

-- 
2.34.1

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

[Bryan O'Donoghue]

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

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Bryan O'Donoghue]

On 27/06/2025 16:48, Vikash Garodia wrote:
> Existing definition 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. 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.
> 
> With this, both iris and non-pixel device can have IOVA range of 0-4GiB
> individually. Certain video usecases like higher video concurrency needs
> IOVA higher than 4GiB.
> 
> Add reference to the reserve-memory schema, which defines reserved IOVA
> regions that are *excluded* from addressable range. Video hardware
> generates different stream IDs based on the predefined range of IOVA
> addresses. Thereby IOVA addresses for firmware and data buffers need to
> be non overlapping. For ex. 0x0-0x25800000 address range is reserved for
> firmware stream-ID, while non-pixel (bitstream) stream-ID can be
> generated by hardware only when bitstream buffers IOVA address is from
> 0x25800000-0xe0000000.
> Non-pixel stream-ID can now be part of the new sub-node, hence iommus in
> iris node can have either 1 entry for pixel stream-id or 2 entries for
> pixel and non-pixel stream-ids.
> 
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>   .../bindings/media/qcom,sm8550-iris.yaml           | 40 ++++++++++++++++++++--
>   1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> index c79bf2101812d83b99704f38b7348a9f728dff44..4dda2c9ca1293baa7aee3b9ee10aff38d280fe05 100644
> --- a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> @@ -65,10 +65,31 @@ properties:
>         - const: core
>   
>     iommus:
> +    minItems: 1
>       maxItems: 2
>   
>     dma-coherent: true
>   
> +  non-pixel:
> +    type: object
> +    additionalProperties: false
> +
> +    description:
> +      Non pixel context bank is needed when video hardware have distinct iommus
> +      for non pixel buffers. Non pixel buffers are mainly compressed and
> +      internal buffers.

You do a better job in the cover letter of describing what this is, why 
its needed etc.

Not asking for this verbatim but its clearer:

"All non pixel buffers, i.e bitstream, HFI queues
and internal buffers related to bitstream processing, would be managed
by this non_pixel device."

Where does the term "non-pixel" come from if its a meaningful name wrt 
to the firmware then non-pixel is fine but, consider a name such as 
"out-of-band" or "oob"

out-of-band is a common term as is "sideband" but sideband I think has a 
different meaning, really this non-data/non-pixel data stuff is out-of-band.

At least for the way the language pack I have installed in my brain 
right now, "oob" or "out-of-band" is a more intuitive name. Its really 
up to you though the main point would be to enumerate the description 
here with some of the detail you've put into the cover letter.

> +
> +    properties:
> +      iommus:
> +        maxItems: 1
> +
> +      memory-region:
> +        maxItems: 1
> +
> +    required:
> +      - iommus
> +      - memory-region
> +
>     operating-points-v2: true

>     opp-table:
> @@ -86,6 +107,7 @@ required:
>   
>   allOf:
>     - $ref: qcom,venus-common.yaml#
> +  - $ref: /schemas/reserved-memory/reserved-memory.yaml
>     - if:
>         properties:
>           compatible:
> @@ -117,6 +139,16 @@ examples:
>       #include <dt-bindings/power/qcom-rpmpd.h>
>       #include <dt-bindings/power/qcom,rpmhpd.h>
>   
> +    reserved-memory {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      iris_resv: reservation-iris {
> +        iommu-addresses = <&iris_non_pixel 0x0 0x0 0x0 0x25800000>,
> +                          <&iris_non_pixel 0x0 0xe0000000 0x0 0x20000000>;
> +      };
> +    };

iris_oob would be less text in the end.

> +
>       video-codec@aa00000 {
>           compatible = "qcom,sm8550-iris";
>           reg = <0x0aa00000 0xf0000>;
> @@ -144,12 +176,16 @@ examples:
>           resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>           reset-names = "bus";
>   
> -        iommus = <&apps_smmu 0x1940 0x0000>,
> -                 <&apps_smmu 0x1947 0x0000>;
> +        iommus = <&apps_smmu 0x1947 0x0000>;
>           dma-coherent;
>   
>           operating-points-v2 = <&iris_opp_table>;
>   
> +        iris_non_pixel: non-pixel {
> +            iommus = <&apps_smmu 0x1940 0x0000>;
> +            memory-region = <&iris_resv>;
> +        };
> +
>           iris_opp_table: opp-table {
>               compatible = "operating-points-v2";
>   
> 

So I was trying to think of a way to catch you out with an ABI break 
but, I don't see how you add minItems: 1 to the iommus declaration above 
so dts prior to this change should still be valid.

I think this adds up but, consider oob instead of non-pixel.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

---
bod

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

[Vikash Garodia]

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

Re: [PATCH v3 2/5] media: iris: register and configure non-pixel node as platform device

[Bryan O'Donoghue]

On 27/06/2025 16:48, Vikash Garodia wrote:
> Register "non_pixel" child node as a platform device, and configure its
> DMA via of_dma_configure(). This ensures proper IOMMU attachment and DMA
> setup for the "non_pixel" device.
> All non pixel memory, i.e bitstream buffers, HFI queues and internal
> buffers related to bitstream processing, would be managed by non_pixel
> device.
> 
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>   drivers/media/platform/qcom/iris/iris_core.h  |  2 ++
>   drivers/media/platform/qcom/iris/iris_probe.c | 50 ++++++++++++++++++++++++++-
>   2 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h
> index aeeac32a1f6d9a9fa7027e8e3db4d95f021c552e..757efd16870876bd2b1d5b1e4103b2d2326b5f49 100644
> --- a/drivers/media/platform/qcom/iris/iris_core.h
> +++ b/drivers/media/platform/qcom/iris/iris_core.h
> @@ -65,6 +65,7 @@ struct icc_info {
>    * @sys_error_handler: a delayed work for handling system fatal error
>    * @instances: a list_head of all instances
>    * @inst_fw_caps: an array of supported instance capabilities
> + * @np_dev: device to represent non pixel node
>    */
>   
>   struct iris_core {
> @@ -105,6 +106,7 @@ struct iris_core {
>   	struct delayed_work			sys_error_handler;
>   	struct list_head			instances;
>   	struct platform_inst_fw_cap		inst_fw_caps[INST_FW_CAP_MAX];
> +	struct device				*np_dev;
>   };
>   
>   int iris_core_init(struct iris_core *core);
> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
> index 9a7ce142f7007ffcda0bd422c1983f2374bb0d92..8fe87e30bd40f3c67ec41305c7d73520fbc9db7b 100644
> --- a/drivers/media/platform/qcom/iris/iris_probe.c
> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
> @@ -6,6 +6,8 @@
>   #include <linux/clk.h>
>   #include <linux/interconnect.h>
>   #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>   #include <linux/pm_domain.h>
>   #include <linux/pm_opp.h>
>   #include <linux/pm_runtime.h>
> @@ -127,6 +129,46 @@ static int iris_init_resets(struct iris_core *core)
>   				     core->iris_platform_data->controller_rst_tbl_size);
>   }
>   
> +static int iris_init_non_pixel_node(struct iris_core *core)
> +{
> +	struct platform_device_info info;
> +	struct platform_device *pdev;
> +	struct device_node *np_node;
> +	int ret;
> +
> +	np_node = of_get_child_by_name(core->dev->of_node, "non_pixel");
> +	if (!np_node)
> +		return 0;
> +
> +	memset(&info, 0, sizeof(info));
> +	info.fwnode = &np_node->fwnode;
> +	info.parent = core->dev;
> +	info.name = np_node->name;
> +	info.dma_mask = DMA_BIT_MASK(32);
> +
> +	pdev = platform_device_register_full(&info);
> +	if (IS_ERR(pdev)) {
> +		of_node_put(np_node);
> +		return PTR_ERR(pdev);
> +	}
> +	pdev->dev.of_node = np_node;
> +
> +	ret = of_dma_configure(&pdev->dev, np_node, true);
> +	if (ret)
> +		goto err_unregister;
> +
> +	core->np_dev = &pdev->dev;
> +
> +	of_node_put(np_node);
> +
> +	return 0;
> +
> +err_unregister:
> +	platform_device_unregister(pdev);
> +	of_node_put(np_node);
> +	return ret;
> +}
> +
>   static int iris_init_resources(struct iris_core *core)
>   {
>   	int ret;
> @@ -143,7 +185,11 @@ static int iris_init_resources(struct iris_core *core)
>   	if (ret)
>   		return ret;
>   
> -	return iris_init_resets(core);
> +	ret = iris_init_resets(core);
> +	if (ret)
> +		return ret;
> +
> +	return iris_init_non_pixel_node(core);
>   }
>   
>   static int iris_register_video_device(struct iris_core *core)
> @@ -188,6 +234,8 @@ static void iris_remove(struct platform_device *pdev)
>   
>   	iris_core_deinit(core);
>   
> +	platform_device_unregister(to_platform_device(core->np_dev));
> +
>   	video_unregister_device(core->vdev_dec);
>   
>   	v4l2_device_unregister(&core->v4l2_dev);
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH v3 3/5] media: iris: use np_dev as preferred DMA device in HFI queue management

[Bryan O'Donoghue]

On 27/06/2025 16:48, Vikash Garodia wrote:
> Update HFI interface queues to use np_dev(preferred non-pixel device)
> for DMA memory allocation and deallocation if available. This allows
> platforms with separate DMA domain for non-pixel to use the appropriate
> device handle when managing HFI queues and SFR regions.
> 
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>   drivers/media/platform/qcom/iris/iris_hfi_queue.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_queue.c b/drivers/media/platform/qcom/iris/iris_hfi_queue.c
> index fac7df0c4d1aec647aeca275ab19651c9ba23733..a31ebe947f525f0d7c09f8b786939d01b62532c3 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_queue.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_queue.c
> @@ -247,24 +247,27 @@ static void iris_hfi_queue_deinit(struct iris_iface_q_info *iface_q)
>   int iris_hfi_queues_init(struct iris_core *core)
>   {
>   	struct iris_hfi_queue_table_header *q_tbl_hdr;
> +	struct device *dev;
>   	u32 queue_size;
>   
> +	dev = core->np_dev ? core->np_dev : core->dev;
> +

dev = core->dev;
if (core->np_dev)
     dev = core->np_dev;

Is much easier to read.

>   	/* Iris hardware requires 4K queue alignment */
>   	queue_size = ALIGN((sizeof(*q_tbl_hdr) + (IFACEQ_QUEUE_SIZE * IFACEQ_NUMQ)), SZ_4K);
> -	core->iface_q_table_vaddr = dma_alloc_attrs(core->dev, queue_size,
> +	core->iface_q_table_vaddr = dma_alloc_attrs(dev, queue_size,
>   						    &core->iface_q_table_daddr,
>   						    GFP_KERNEL, DMA_ATTR_WRITE_COMBINE);
>   	if (!core->iface_q_table_vaddr) {
> -		dev_err(core->dev, "queues alloc and map failed\n");
> +		dev_err(dev, "queues alloc and map failed\n");
>   		return -ENOMEM;
>   	}
>   
> -	core->sfr_vaddr = dma_alloc_attrs(core->dev, SFR_SIZE,
> +	core->sfr_vaddr = dma_alloc_attrs(dev, SFR_SIZE,
>   					  &core->sfr_daddr,
>   					  GFP_KERNEL, DMA_ATTR_WRITE_COMBINE);
>   	if (!core->sfr_vaddr) {
> -		dev_err(core->dev, "sfr alloc and map failed\n");
> -		dma_free_attrs(core->dev, sizeof(*q_tbl_hdr), core->iface_q_table_vaddr,
> +		dev_err(dev, "sfr alloc and map failed\n");
> +		dma_free_attrs(dev, sizeof(*q_tbl_hdr), core->iface_q_table_vaddr,
>   			       core->iface_q_table_daddr, DMA_ATTR_WRITE_COMBINE);
>   		return -ENOMEM;
>   	}
> @@ -292,6 +295,7 @@ int iris_hfi_queues_init(struct iris_core *core)
>   
>   void iris_hfi_queues_deinit(struct iris_core *core)
>   {
> +	struct device *dev;
>   	u32 queue_size;
>   
>   	if (!core->iface_q_table_vaddr)
> @@ -301,7 +305,9 @@ void iris_hfi_queues_deinit(struct iris_core *core)
>   	iris_hfi_queue_deinit(&core->message_queue);
>   	iris_hfi_queue_deinit(&core->command_queue);
>   
> -	dma_free_attrs(core->dev, SFR_SIZE, core->sfr_vaddr,
> +	dev = core->np_dev ? core->np_dev : core->dev;

and again

> +
> +	dma_free_attrs(dev, SFR_SIZE, core->sfr_vaddr,
>   		       core->sfr_daddr, DMA_ATTR_WRITE_COMBINE);
>   
>   	core->sfr_vaddr = NULL;
> @@ -310,7 +316,7 @@ void iris_hfi_queues_deinit(struct iris_core *core)
>   	queue_size = ALIGN(sizeof(struct iris_hfi_queue_table_header) +
>   		(IFACEQ_QUEUE_SIZE * IFACEQ_NUMQ), SZ_4K);
>   
> -	dma_free_attrs(core->dev, queue_size, core->iface_q_table_vaddr,
> +	dma_free_attrs(dev, queue_size, core->iface_q_table_vaddr,
>   		       core->iface_q_table_daddr, DMA_ATTR_WRITE_COMBINE);
>   
>   	core->iface_q_table_vaddr = NULL;
> 
Other than that.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH v3 4/5] media: iris: select appropriate DMA device for internal buffers

[Bryan O'Donoghue]

On 27/06/2025 16:48, Vikash Garodia wrote:
> When a non-pixel device (np_dev) exists, it is preferred for DMA
> operations for internal buffers which are specific to bitstream data
> processing. DPB(decoded picture buffer) buffers are internal buffers
> associated with pixel buffers, hence they are not part of "non_pixel"
> device.
> 
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>   drivers/media/platform/qcom/iris/iris_buffer.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c
> index e5c5a564fcb81e77746df8c4797a10a07f2ae946..0bf6041936175d03a51985be148e78894fc3e990 100644
> --- a/drivers/media/platform/qcom/iris/iris_buffer.c
> +++ b/drivers/media/platform/qcom/iris/iris_buffer.c
> @@ -265,6 +265,7 @@ static int iris_create_internal_buffer(struct iris_inst *inst,
>   	struct iris_buffers *buffers = &inst->buffers[buffer_type];
>   	struct iris_core *core = inst->core;
>   	struct iris_buffer *buffer;
> +	struct device *dev;
>   
>   	if (!buffers->size)
>   		return 0;
> @@ -280,7 +281,11 @@ static int iris_create_internal_buffer(struct iris_inst *inst,
>   	buffer->dma_attrs = DMA_ATTR_WRITE_COMBINE | DMA_ATTR_NO_KERNEL_MAPPING;
>   	list_add_tail(&buffer->list, &buffers->list);
>   
> -	buffer->kvaddr = dma_alloc_attrs(core->dev, buffer->buffer_size,
> +	dev = core->np_dev ? core->np_dev : core->dev;
> +	if (buffer->type == BUF_DPB)
> +		dev = core->dev;
> +
> +	buffer->kvaddr = dma_alloc_attrs(dev, buffer->buffer_size,
>   					 &buffer->device_addr, GFP_KERNEL, buffer->dma_attrs);
>   	if (!buffer->kvaddr)
>   		return -ENOMEM;
> @@ -367,9 +372,15 @@ int iris_queue_internal_buffers(struct iris_inst *inst, u32 plane)
>   int iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buffer)
>   {
>   	struct iris_core *core = inst->core;
> +	struct device *dev;
> +
> +	dev = core->np_dev ? core->np_dev : core->dev;
> +
> +	if (buffer->type == BUF_DPB)
> +		dev = core->dev;

Again I think the hooking of dev is clearer with if/else instead of 
ternary operators.

Its not against the coding standard to my knowledge ..

>   
>   	list_del(&buffer->list);
> -	dma_free_attrs(core->dev, buffer->buffer_size, buffer->kvaddr,
> +	dma_free_attrs(dev, buffer->buffer_size, buffer->kvaddr,
>   		       buffer->device_addr, buffer->dma_attrs);
>   	kfree(buffer);
>   
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH v3 5/5] media: iris: configure DMA device for vb2 queue on OUTPUT plane

[Bryan O'Donoghue]

On 27/06/2025 16:48, Vikash Garodia wrote:
> While setting up the vb2 queues, assign "non_pixel" device to manage
> OUTPUT plane buffers i.e bitstream buffers incase of decoder. It prefers
> the non_pixel device(np_dev) when available, falling back to core->dev
> otherwise.
> 
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>   drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c b/drivers/media/platform/qcom/iris/iris_vb2.c
> index cdf11feb590b5cb7804db3fcde7282fb1f9f1a1e..01cc337970400d48063c558c1ac039539dbcbaba 100644
> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
> @@ -159,6 +159,10 @@ int iris_vb2_queue_setup(struct vb2_queue *q,
>   	*num_planes = 1;
>   	sizes[0] = f->fmt.pix_mp.plane_fmt[0].sizeimage;
>   
> +	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT ||
> +	    q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +		q->dev = core->np_dev ? core->np_dev : core->dev;
> +
>   unlock:
>   	mutex_unlock(&inst->lock);
>   
> 

q->dev = core->dev;

if (thing || thing_else)
     q->dev = core->np_dev;

---
bod

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Vikash Garodia]

On 6/27/2025 10:01 PM, Bryan O'Donoghue wrote:
> On 27/06/2025 16:48, Vikash Garodia wrote:
>> Existing definition 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. 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.
>>
>> With this, both iris and non-pixel device can have IOVA range of 0-4GiB
>> individually. Certain video usecases like higher video concurrency needs
>> IOVA higher than 4GiB.
>>
>> Add reference to the reserve-memory schema, which defines reserved IOVA
>> regions that are *excluded* from addressable range. Video hardware
>> generates different stream IDs based on the predefined range of IOVA
>> addresses. Thereby IOVA addresses for firmware and data buffers need to
>> be non overlapping. For ex. 0x0-0x25800000 address range is reserved for
>> firmware stream-ID, while non-pixel (bitstream) stream-ID can be
>> generated by hardware only when bitstream buffers IOVA address is from
>> 0x25800000-0xe0000000.
>> Non-pixel stream-ID can now be part of the new sub-node, hence iommus in
>> iris node can have either 1 entry for pixel stream-id or 2 entries for
>> pixel and non-pixel stream-ids.
>>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
>>   .../bindings/media/qcom,sm8550-iris.yaml           | 40 ++++++++++++++++++++--
>>   1 file changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> index
>> c79bf2101812d83b99704f38b7348a9f728dff44..4dda2c9ca1293baa7aee3b9ee10aff38d280fe05 100644
>> --- a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> @@ -65,10 +65,31 @@ properties:
>>         - const: core
>>       iommus:
>> +    minItems: 1
>>       maxItems: 2
>>       dma-coherent: true
>>   +  non-pixel:
>> +    type: object
>> +    additionalProperties: false
>> +
>> +    description:
>> +      Non pixel context bank is needed when video hardware have distinct iommus
>> +      for non pixel buffers. Non pixel buffers are mainly compressed and
>> +      internal buffers.
> 
> You do a better job in the cover letter of describing what this is, why its
> needed etc.
> 
> Not asking for this verbatim but its clearer:
> 
> "All non pixel buffers, i.e bitstream, HFI queues
> and internal buffers related to bitstream processing, would be managed
> by this non_pixel device."
> 
> Where does the term "non-pixel" come from if its a meaningful name wrt to the
> firmware then non-pixel is fine but, consider a name such as "out-of-band" or "oob"
It is in-sync with firmware and we would be seeing it more when we extend it for
secure usecases.

Regards,
Vikash
> out-of-band is a common term as is "sideband" but sideband I think has a
> different meaning, really this non-data/non-pixel data stuff is out-of-band.
> 
> At least for the way the language pack I have installed in my brain right now,
> "oob" or "out-of-band" is a more intuitive name. Its really up to you though the
> main point would be to enumerate the description here with some of the detail
> you've put into the cover letter.
> 
>> +
>> +    properties:
>> +      iommus:
>> +        maxItems: 1
>> +
>> +      memory-region:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - iommus
>> +      - memory-region
>> +
>>     operating-points-v2: true
> 
>>     opp-table:
>> @@ -86,6 +107,7 @@ required:
>>     allOf:
>>     - $ref: qcom,venus-common.yaml#
>> +  - $ref: /schemas/reserved-memory/reserved-memory.yaml
>>     - if:
>>         properties:
>>           compatible:
>> @@ -117,6 +139,16 @@ examples:
>>       #include <dt-bindings/power/qcom-rpmpd.h>
>>       #include <dt-bindings/power/qcom,rpmhpd.h>
>>   +    reserved-memory {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +
>> +      iris_resv: reservation-iris {
>> +        iommu-addresses = <&iris_non_pixel 0x0 0x0 0x0 0x25800000>,
>> +                          <&iris_non_pixel 0x0 0xe0000000 0x0 0x20000000>;
>> +      };
>> +    };
> 
> iris_oob would be less text in the end.
> 
>> +
>>       video-codec@aa00000 {
>>           compatible = "qcom,sm8550-iris";
>>           reg = <0x0aa00000 0xf0000>;
>> @@ -144,12 +176,16 @@ examples:
>>           resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>>           reset-names = "bus";
>>   -        iommus = <&apps_smmu 0x1940 0x0000>,
>> -                 <&apps_smmu 0x1947 0x0000>;
>> +        iommus = <&apps_smmu 0x1947 0x0000>;
>>           dma-coherent;
>>             operating-points-v2 = <&iris_opp_table>;
>>   +        iris_non_pixel: non-pixel {
>> +            iommus = <&apps_smmu 0x1940 0x0000>;
>> +            memory-region = <&iris_resv>;
>> +        };
>> +
>>           iris_opp_table: opp-table {
>>               compatible = "operating-points-v2";
>>  
> 
> So I was trying to think of a way to catch you out with an ABI break but, I
> don't see how you add minItems: 1 to the iommus declaration above so dts prior
> to this change should still be valid.
> 
> I think this adds up but, consider oob instead of non-pixel.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 
> ---
> bod

Re: [PATCH v3 5/5] media: iris: configure DMA device for vb2 queue on OUTPUT plane

[Vikash Garodia]

On 6/27/2025 10:38 PM, Bryan O'Donoghue wrote:
> On 27/06/2025 16:48, Vikash Garodia wrote:
>> While setting up the vb2 queues, assign "non_pixel" device to manage
>> OUTPUT plane buffers i.e bitstream buffers incase of decoder. It prefers
>> the non_pixel device(np_dev) when available, falling back to core->dev
>> otherwise.
>>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c
>> b/drivers/media/platform/qcom/iris/iris_vb2.c
>> index
>> cdf11feb590b5cb7804db3fcde7282fb1f9f1a1e..01cc337970400d48063c558c1ac039539dbcbaba 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
>> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
>> @@ -159,6 +159,10 @@ int iris_vb2_queue_setup(struct vb2_queue *q,
>>       *num_planes = 1;
>>       sizes[0] = f->fmt.pix_mp.plane_fmt[0].sizeimage;
>>   +    if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT ||
>> +        q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>> +        q->dev = core->np_dev ? core->np_dev : core->dev;
>> +
>>   unlock:
>>       mutex_unlock(&inst->lock);
>>  
> 
> q->dev = core->dev;
> 
> if (thing || thing_else)
>     q->dev = core->np_dev;
when IF condition is not met, q->dev assignment would be unnecessary i.e for
plane types other than V4L2_BUF_TYPE_VIDEO_OUTPUT. Refer [1] for the dev
assignment to queue.

Regards,
Vikash

[1]
https://elixir.bootlin.com/linux/v6.15.3/source/drivers/media/platform/qcom/iris/iris_vidc.c#L106
> 
> ---
> bod

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[neil.armstrong]

On 27/06/2025 17:48, Vikash Garodia wrote:
> Existing definition 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. 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.
> 
> With this, both iris and non-pixel device can have IOVA range of 0-4GiB
> individually. Certain video usecases like higher video concurrency needs
> IOVA higher than 4GiB.
> 
> Add reference to the reserve-memory schema, which defines reserved IOVA
> regions that are *excluded* from addressable range. Video hardware
> generates different stream IDs based on the predefined range of IOVA
> addresses. Thereby IOVA addresses for firmware and data buffers need to
> be non overlapping. For ex. 0x0-0x25800000 address range is reserved for
> firmware stream-ID, while non-pixel (bitstream) stream-ID can be
> generated by hardware only when bitstream buffers IOVA address is from
> 0x25800000-0xe0000000.
> Non-pixel stream-ID can now be part of the new sub-node, hence iommus in
> iris node can have either 1 entry for pixel stream-id or 2 entries for
> pixel and non-pixel stream-ids.
> 
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>   .../bindings/media/qcom,sm8550-iris.yaml           | 40 ++++++++++++++++++++--
>   1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> index c79bf2101812d83b99704f38b7348a9f728dff44..4dda2c9ca1293baa7aee3b9ee10aff38d280fe05 100644
> --- a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> @@ -65,10 +65,31 @@ properties:
>         - const: core
>   
>     iommus:
> +    minItems: 1
>       maxItems: 2
>   
>     dma-coherent: true
>   
> +  non-pixel:
> +    type: object
> +    additionalProperties: false
> +
> +    description:
> +      Non pixel context bank is needed when video hardware have distinct iommus
> +      for non pixel buffers. Non pixel buffers are mainly compressed and
> +      internal buffers.
> +
> +    properties:
> +      iommus:
> +        maxItems: 1
> +
> +      memory-region:
> +        maxItems: 1
> +
> +    required:
> +      - iommus
> +      - memory-region
> +
>     operating-points-v2: true
>   
>     opp-table:
> @@ -86,6 +107,7 @@ required:
>   
>   allOf:
>     - $ref: qcom,venus-common.yaml#
> +  - $ref: /schemas/reserved-memory/reserved-memory.yaml
>     - if:
>         properties:
>           compatible:
> @@ -117,6 +139,16 @@ examples:
>       #include <dt-bindings/power/qcom-rpmpd.h>
>       #include <dt-bindings/power/qcom,rpmhpd.h>
>   
> +    reserved-memory {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      iris_resv: reservation-iris {
> +        iommu-addresses = <&iris_non_pixel 0x0 0x0 0x0 0x25800000>,
> +                          <&iris_non_pixel 0x0 0xe0000000 0x0 0x20000000>;
> +      };
> +    };
> +
>       video-codec@aa00000 {
>           compatible = "qcom,sm8550-iris";
>           reg = <0x0aa00000 0xf0000>;
> @@ -144,12 +176,16 @@ examples:
>           resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>           reset-names = "bus";
>   
> -        iommus = <&apps_smmu 0x1940 0x0000>,
> -                 <&apps_smmu 0x1947 0x0000>;
> +        iommus = <&apps_smmu 0x1947 0x0000>;
>           dma-coherent;
>   
>           operating-points-v2 = <&iris_opp_table>;
>   
> +        iris_non_pixel: non-pixel {

You can drop the label for this node.

Neil

> +            iommus = <&apps_smmu 0x1940 0x0000>;
> +            memory-region = <&iris_resv>;
> +        };
> +
>           iris_opp_table: opp-table {
>               compatible = "operating-points-v2";
>   
> 

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

[neil.armstrong]

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,

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Neil Armstrong]

On 30/06/2025 17:48, neil.armstrong@linaro.org wrote:
> On 27/06/2025 17:48, Vikash Garodia wrote:
>> Existing definition 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. 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.
>>
>> With this, both iris and non-pixel device can have IOVA range of 0-4GiB
>> individually. Certain video usecases like higher video concurrency needs
>> IOVA higher than 4GiB.
>>
>> Add reference to the reserve-memory schema, which defines reserved IOVA
>> regions that are *excluded* from addressable range. Video hardware
>> generates different stream IDs based on the predefined range of IOVA
>> addresses. Thereby IOVA addresses for firmware and data buffers need to
>> be non overlapping. For ex. 0x0-0x25800000 address range is reserved for
>> firmware stream-ID, while non-pixel (bitstream) stream-ID can be
>> generated by hardware only when bitstream buffers IOVA address is from
>> 0x25800000-0xe0000000.
>> Non-pixel stream-ID can now be part of the new sub-node, hence iommus in
>> iris node can have either 1 entry for pixel stream-id or 2 entries for
>> pixel and non-pixel stream-ids.
>>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
>>   .../bindings/media/qcom,sm8550-iris.yaml           | 40 ++++++++++++++++++++--
>>   1 file changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> index c79bf2101812d83b99704f38b7348a9f728dff44..4dda2c9ca1293baa7aee3b9ee10aff38d280fe05 100644
>> --- a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> @@ -65,10 +65,31 @@ properties:
>>         - const: core
>>     iommus:
>> +    minItems: 1
>>       maxItems: 2
>>     dma-coherent: true
>> +  non-pixel:
>> +    type: object
>> +    additionalProperties: false
>> +
>> +    description:
>> +      Non pixel context bank is needed when video hardware have distinct iommus
>> +      for non pixel buffers. Non pixel buffers are mainly compressed and
>> +      internal buffers.
>> +
>> +    properties:
>> +      iommus:
>> +        maxItems: 1
>> +
>> +      memory-region:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - iommus
>> +      - memory-region
>> +
>>     operating-points-v2: true
>>     opp-table:
>> @@ -86,6 +107,7 @@ required:
>>   allOf:
>>     - $ref: qcom,venus-common.yaml#
>> +  - $ref: /schemas/reserved-memory/reserved-memory.yaml
>>     - if:
>>         properties:
>>           compatible:
>> @@ -117,6 +139,16 @@ examples:
>>       #include <dt-bindings/power/qcom-rpmpd.h>
>>       #include <dt-bindings/power/qcom,rpmhpd.h>
>> +    reserved-memory {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +
>> +      iris_resv: reservation-iris {
>> +        iommu-addresses = <&iris_non_pixel 0x0 0x0 0x0 0x25800000>,
>> +                          <&iris_non_pixel 0x0 0xe0000000 0x0 0x20000000>;
>> +      };
>> +    };
>> +
>>       video-codec@aa00000 {
>>           compatible = "qcom,sm8550-iris";
>>           reg = <0x0aa00000 0xf0000>;
>> @@ -144,12 +176,16 @@ examples:
>>           resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>>           reset-names = "bus";
>> -        iommus = <&apps_smmu 0x1940 0x0000>,
>> -                 <&apps_smmu 0x1947 0x0000>;
>> +        iommus = <&apps_smmu 0x1947 0x0000>;
>>           dma-coherent;
>>           operating-points-v2 = <&iris_opp_table>;
>> +        iris_non_pixel: non-pixel {
> 
> You can drop the label for this node.

Sorry forget this....

> 
> Neil
> 
>> +            iommus = <&apps_smmu 0x1940 0x0000>;
>> +            memory-region = <&iris_resv>;
>> +        };
>> +
>>           iris_opp_table: opp-table {
>>               compatible = "operating-points-v2";
>>
> 

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

[neil.armstrong]

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

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

[Konrad Dybcio]

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

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

[Vikash Garodia]

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

Re: [PATCH v3 5/5] media: iris: configure DMA device for vb2 queue on OUTPUT plane

[Konrad Dybcio]

On 27-Jun-25 19:08, Bryan O'Donoghue wrote:
> On 27/06/2025 16:48, Vikash Garodia wrote:
>> While setting up the vb2 queues, assign "non_pixel" device to manage
>> OUTPUT plane buffers i.e bitstream buffers incase of decoder. It prefers
>> the non_pixel device(np_dev) when available, falling back to core->dev
>> otherwise.
>>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/iris/iris_vb2.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c b/drivers/media/platform/qcom/iris/iris_vb2.c
>> index cdf11feb590b5cb7804db3fcde7282fb1f9f1a1e..01cc337970400d48063c558c1ac039539dbcbaba 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
>> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
>> @@ -159,6 +159,10 @@ int iris_vb2_queue_setup(struct vb2_queue *q,
>>       *num_planes = 1;
>>       sizes[0] = f->fmt.pix_mp.plane_fmt[0].sizeimage;
>>   +    if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT ||
>> +        q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>> +        q->dev = core->np_dev ? core->np_dev : core->dev;
>> +
>>   unlock:
>>       mutex_unlock(&inst->lock);
>>  
> 
> q->dev = core->dev;
> 
> if (thing || thing_else)
>     q->dev = core->np_dev;

q->dev = core->np_dev ?: core->dev;

Konrad

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

[Neil Armstrong]

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

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

[Vikash Garodia]

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
> 

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

[Neil Armstrong]

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

Re: [PATCH v3 2/5] media: iris: register and configure non-pixel node as platform device

[Krzysztof Kozlowski]

On 27/06/2025 17:48, Vikash Garodia wrote:
>  
> +static int iris_init_non_pixel_node(struct iris_core *core)
> +{
> +	struct platform_device_info info;
> +	struct platform_device *pdev;
> +	struct device_node *np_node;
> +	int ret;
> +
> +	np_node = of_get_child_by_name(core->dev->of_node, "non_pixel");

Never tested.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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

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

[Krzysztof Kozlowski]

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

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Krzysztof Kozlowski]

On 27/06/2025 17:48, Vikash Garodia wrote:
> Existing definition 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. 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.
> 
> With this, both iris and non-pixel device can have IOVA range of 0-4GiB
> individually. Certain video usecases like higher video concurrency needs
> IOVA higher than 4GiB.
> 
> Add reference to the reserve-memory schema, which defines reserved IOVA

No. That schema is always selected. This makes no sense at all.

> regions that are *excluded* from addressable range. Video hardware
> generates different stream IDs based on the predefined range of IOVA
> addresses. Thereby IOVA addresses for firmware and data buffers need to
> be non overlapping. For ex. 0x0-0x25800000 address range is reserved for
> firmware stream-ID, while non-pixel (bitstream) stream-ID can be
> generated by hardware only when bitstream buffers IOVA address is from
> 0x25800000-0xe0000000.
> Non-pixel stream-ID can now be part of the new sub-node, hence iommus in
> iris node can have either 1 entry for pixel stream-id or 2 entries for
> pixel and non-pixel stream-ids.
> 
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>  .../bindings/media/qcom,sm8550-iris.yaml           | 40 ++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> index c79bf2101812d83b99704f38b7348a9f728dff44..4dda2c9ca1293baa7aee3b9ee10aff38d280fe05 100644
> --- a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> @@ -65,10 +65,31 @@ properties:
>        - const: core
>  
>    iommus:
> +    minItems: 1
>      maxItems: 2

No, why hardware suddenly has different amount?

>  
>    dma-coherent: true
>  
> +  non-pixel:

Why EXISTING hardware grows?

> +    type: object
> +    additionalProperties: false
> +
> +    description:
> +      Non pixel context bank is needed when video hardware have distinct iommus
> +      for non pixel buffers. Non pixel buffers are mainly compressed and
> +      internal buffers.
> +
> +    properties:
> +      iommus:
> +        maxItems: 1
> +
> +      memory-region:
> +        maxItems: 1
> +
> +    required:
> +      - iommus
> +      - memory-region
> +
>    operating-points-v2: true
>  
>    opp-table:
> @@ -86,6 +107,7 @@ required:
>  
>  allOf:
>    - $ref: qcom,venus-common.yaml#
> +  - $ref: /schemas/reserved-memory/reserved-memory.yaml

This makes no sense. how is this device a reserved memory?

>    - if:
>        properties:
>          compatible:
> @@ -117,6 +139,16 @@ examples:
>      #include <dt-bindings/power/qcom-rpmpd.h>
>      #include <dt-bindings/power/qcom,rpmhpd.h>
>  
> +    reserved-memory {
> +      #address-cells = <2>;
> +      #size-cells = <2>;

Why do you need this?

> +
> +      iris_resv: reservation-iris {

Mixing MMIO and non-MMIO is not the way to go. This is also not relevant
here. Don't embed other things into your binding example.


> +        iommu-addresses = <&iris_non_pixel 0x0 0x0 0x0 0x25800000>,
> +                          <&iris_non_pixel 0x0 0xe0000000 0x0 0x20000000>;
> +      };
> +    };
> +
>      video-codec@aa00000 {
>          compatible = "qcom,sm8550-iris";
>          reg = <0x0aa00000 0xf0000>;
> @@ -144,12 +176,16 @@ examples:
>          resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>          reset-names = "bus";
>  
> -        iommus = <&apps_smmu 0x1940 0x0000>,
> -                 <&apps_smmu 0x1947 0x0000>;
> +        iommus = <&apps_smmu 0x1947 0x0000>;

Why did the device or hardware change? Nothing explains in commit msg
what is wrong with existing device and existing binding.

>          dma-coherent;
>  
>          operating-points-v2 = <&iris_opp_table>;
>  
> +        iris_non_pixel: non-pixel {
> +            iommus = <&apps_smmu 0x1940 0x0000>;
> +            memory-region = <&iris_resv>;
> +        };
> +
>          iris_opp_table: opp-table {
>              compatible = "operating-points-v2";
>  
> 


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Krzysztof Kozlowski]

On 27/06/2025 17:48, Vikash Garodia wrote:
> +
>      video-codec@aa00000 {
>          compatible = "qcom,sm8550-iris";
>          reg = <0x0aa00000 0xf0000>;
> @@ -144,12 +176,16 @@ examples:
>          resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>          reset-names = "bus";
>  
> -        iommus = <&apps_smmu 0x1940 0x0000>,
> -                 <&apps_smmu 0x1947 0x0000>;
> +        iommus = <&apps_smmu 0x1947 0x0000>;

I missed, that's technically ABI break and nothing in commit msg
explains that. You need to clearly explain the reasons and impact.

Best regards,
Krzysztof

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Vikash Garodia]

On 7/2/2025 4:43 PM, Krzysztof Kozlowski wrote:
> On 27/06/2025 17:48, Vikash Garodia wrote:
>> Existing definition 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. 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.
>>
>> With this, both iris and non-pixel device can have IOVA range of 0-4GiB
>> individually. Certain video usecases like higher video concurrency needs
>> IOVA higher than 4GiB.
>>
>> Add reference to the reserve-memory schema, which defines reserved IOVA
> 
> No. That schema is always selected. This makes no sense at all.
I could not get this, are you suggesting to drop this reference ?
> 
>> regions that are *excluded* from addressable range. Video hardware
>> generates different stream IDs based on the predefined range of IOVA
>> addresses. Thereby IOVA addresses for firmware and data buffers need to
>> be non overlapping. For ex. 0x0-0x25800000 address range is reserved for
>> firmware stream-ID, while non-pixel (bitstream) stream-ID can be
>> generated by hardware only when bitstream buffers IOVA address is from
>> 0x25800000-0xe0000000.
>> Non-pixel stream-ID can now be part of the new sub-node, hence iommus in
>> iris node can have either 1 entry for pixel stream-id or 2 entries for
>> pixel and non-pixel stream-ids.
>>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
>>  .../bindings/media/qcom,sm8550-iris.yaml           | 40 ++++++++++++++++++++--
>>  1 file changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> index c79bf2101812d83b99704f38b7348a9f728dff44..4dda2c9ca1293baa7aee3b9ee10aff38d280fe05 100644
>> --- a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> @@ -65,10 +65,31 @@ properties:
>>        - const: core
>>  
>>    iommus:
>> +    minItems: 1
>>      maxItems: 2
> 
> No, why hardware suddenly has different amount?
Its not about hardware started to have a new stream-ID. You can look for the
description in the commit which explains the need for a new device and hence the
split of stream-IDs in iris device OR non-pixel device.
> 
>>  
>>    dma-coherent: true
>>  
>> +  non-pixel:
> 
> Why EXISTING hardware grows?
Same here, the commit describes the limitation of existing design and also
explains the need for having the non-pixel device. Its not that the hardware is
growing here, rather the hardware stream-IDs are utilized differently to get
higher device addressable range.
> 
>> +    type: object
>> +    additionalProperties: false
>> +
>> +    description:
>> +      Non pixel context bank is needed when video hardware have distinct iommus
>> +      for non pixel buffers. Non pixel buffers are mainly compressed and
>> +      internal buffers.
>> +
>> +    properties:
>> +      iommus:
>> +        maxItems: 1
>> +
>> +      memory-region:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - iommus
>> +      - memory-region
>> +
>>    operating-points-v2: true
>>  
>>    opp-table:
>> @@ -86,6 +107,7 @@ required:
>>  
>>  allOf:
>>    - $ref: qcom,venus-common.yaml#
>> +  - $ref: /schemas/reserved-memory/reserved-memory.yaml
> 
> This makes no sense. how is this device a reserved memory?
Again, explained the "excluded" portion from IOVA part in commit description.
For such excluded region, reserved memory would be needed. I have followed the
adsp example in the reserved-memory schema[1], its same for iris.

[1]
https://github.com/devicetree-org/dt-schema/blame/main/dtschema/schemas/reserved-memory/reserved-memory.yaml
> 
>>    - if:
>>        properties:
>>          compatible:
>> @@ -117,6 +139,16 @@ examples:
>>      #include <dt-bindings/power/qcom-rpmpd.h>
>>      #include <dt-bindings/power/qcom,rpmhpd.h>
>>  
>> +    reserved-memory {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
> 
> Why do you need this?
Was planning to drop this, as the reserved-memory region have it defined.
> 
>> +
>> +      iris_resv: reservation-iris {
> 
> Mixing MMIO and non-MMIO is not the way to go. This is also not relevant
> here. Don't embed other things into your binding example.
> 
> 
>> +        iommu-addresses = <&iris_non_pixel 0x0 0x0 0x0 0x25800000>,
>> +                          <&iris_non_pixel 0x0 0xe0000000 0x0 0x20000000>;
>> +      };
>> +    };
>> +
>>      video-codec@aa00000 {
>>          compatible = "qcom,sm8550-iris";
>>          reg = <0x0aa00000 0xf0000>;
>> @@ -144,12 +176,16 @@ examples:
>>          resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>>          reset-names = "bus";
>>  
>> -        iommus = <&apps_smmu 0x1940 0x0000>,
>> -                 <&apps_smmu 0x1947 0x0000>;
>> +        iommus = <&apps_smmu 0x1947 0x0000>;
> 
> Why did the device or hardware change? Nothing explains in commit msg
> what is wrong with existing device and existing binding.
Same query here, the commit well explains the limitation with existing device
and how adding a new sub node mitigates the situation.

Regards,
Vikash
> 
>>          dma-coherent;
>>  
>>          operating-points-v2 = <&iris_opp_table>;
>>  
>> +        iris_non_pixel: non-pixel {
>> +            iommus = <&apps_smmu 0x1940 0x0000>;
>> +            memory-region = <&iris_resv>;
>> +        };
>> +
>>          iris_opp_table: opp-table {
>>              compatible = "operating-points-v2";
>>  
>>
> 
> 
> Best regards,
> Krzysztof

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

[Vikash Garodia]

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

Re: [PATCH v3 2/5] media: iris: register and configure non-pixel node as platform device

[Vikash Garodia]

On 7/2/2025 4:34 PM, Krzysztof Kozlowski wrote:
> On 27/06/2025 17:48, Vikash Garodia wrote:
>>  
>> +static int iris_init_non_pixel_node(struct iris_core *core)
>> +{
>> +	struct platform_device_info info;
>> +	struct platform_device *pdev;
>> +	struct device_node *np_node;
>> +	int ret;
>> +
>> +	np_node = of_get_child_by_name(core->dev->of_node, "non_pixel");
> 
> Never tested.
Yes, thats correct, but it have been sorted out now.

> 
> Best regards,
> Krzysztof

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Vikash Garodia]

On 7/2/2025 4:53 PM, Krzysztof Kozlowski wrote:
> On 27/06/2025 17:48, Vikash Garodia wrote:
>> +
>>      video-codec@aa00000 {
>>          compatible = "qcom,sm8550-iris";
>>          reg = <0x0aa00000 0xf0000>;
>> @@ -144,12 +176,16 @@ examples:
>>          resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>>          reset-names = "bus";
>>  
>> -        iommus = <&apps_smmu 0x1940 0x0000>,
>> -                 <&apps_smmu 0x1947 0x0000>;
>> +        iommus = <&apps_smmu 0x1947 0x0000>;
> 
> I missed, that's technically ABI break and nothing in commit msg
> explains that. You need to clearly explain the reasons and impact.
No, it is not. Interface works well with old or new approach.

> 
> Best regards,
> Krzysztof

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Krzysztof Kozlowski]

On 02/07/2025 13:32, Vikash Garodia wrote:
> 
> On 7/2/2025 4:43 PM, Krzysztof Kozlowski wrote:
>> On 27/06/2025 17:48, Vikash Garodia wrote:
>>> Existing definition 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. 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.
>>>
>>> With this, both iris and non-pixel device can have IOVA range of 0-4GiB
>>> individually. Certain video usecases like higher video concurrency needs
>>> IOVA higher than 4GiB.
>>>
>>> Add reference to the reserve-memory schema, which defines reserved IOVA
>>
>> No. That schema is always selected. This makes no sense at all.
> I could not get this, are you suggesting to drop this reference ?

What does the schema says?

  7 title: /reserved-memory Child Node Common


Is this the binding for reserved-memory node? Not sure, your subject
does not have proper prefix, but diff suggested that not.

Maybe I missed something.


>>
>>> regions that are *excluded* from addressable range. Video hardware
>>> generates different stream IDs based on the predefined range of IOVA
>>> addresses. Thereby IOVA addresses for firmware and data buffers need to
>>> be non overlapping. For ex. 0x0-0x25800000 address range is reserved for
>>> firmware stream-ID, while non-pixel (bitstream) stream-ID can be
>>> generated by hardware only when bitstream buffers IOVA address is from
>>> 0x25800000-0xe0000000.
>>> Non-pixel stream-ID can now be part of the new sub-node, hence iommus in
>>> iris node can have either 1 entry for pixel stream-id or 2 entries for
>>> pixel and non-pixel stream-ids.
>>>
>>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>>> ---
>>>  .../bindings/media/qcom,sm8550-iris.yaml           | 40 ++++++++++++++++++++--
>>>  1 file changed, 38 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>>> index c79bf2101812d83b99704f38b7348a9f728dff44..4dda2c9ca1293baa7aee3b9ee10aff38d280fe05 100644
>>> --- a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>>> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>>> @@ -65,10 +65,31 @@ properties:
>>>        - const: core
>>>  
>>>    iommus:
>>> +    minItems: 1
>>>      maxItems: 2
>>
>> No, why hardware suddenly has different amount?
> Its not about hardware started to have a new stream-ID. You can look for the
> description in the commit which explains the need for a new device and hence the
> split of stream-IDs in iris device OR non-pixel device.

But this is not a new device! This is sm8550. Existing device.

>>
>>>  
>>>    dma-coherent: true
>>>  
>>> +  non-pixel:
>>
>> Why EXISTING hardware grows?
> Same here, the commit describes the limitation of existing design and also
> explains the need for having the non-pixel device. Its not that the hardware is
> growing here, rather the hardware stream-IDs are utilized differently to get
> higher device addressable range.

You are not doing this for a new device. There is no new device here at
all. Nowhere here is a new device.

Changes for a new device COME TOGETHER with the new device.

What you are doing here is changing existing hardware without any
explanation why.

>>
>>> +    type: object
>>> +    additionalProperties: false
>>> +
>>> +    description:
>>> +      Non pixel context bank is needed when video hardware have distinct iommus
>>> +      for non pixel buffers. Non pixel buffers are mainly compressed and
>>> +      internal buffers.
>>> +
>>> +    properties:
>>> +      iommus:
>>> +        maxItems: 1
>>> +
>>> +      memory-region:
>>> +        maxItems: 1
>>> +
>>> +    required:
>>> +      - iommus
>>> +      - memory-region
>>> +
>>>    operating-points-v2: true
>>>  
>>>    opp-table:
>>> @@ -86,6 +107,7 @@ required:
>>>  
>>>  allOf:
>>>    - $ref: qcom,venus-common.yaml#
>>> +  - $ref: /schemas/reserved-memory/reserved-memory.yaml
>>
>> This makes no sense. how is this device a reserved memory?
> Again, explained the "excluded" portion from IOVA part in commit description.
> For such excluded region, reserved memory would be needed. I have followed the
> adsp example in the reserved-memory schema[1], its same for iris.
> 
> [1]
> https://github.com/devicetree-org/dt-schema/blame/main/dtschema/schemas/reserved-memory/reserved-memory.yaml

Read the title there.


>>
>>>    - if:
>>>        properties:
>>>          compatible:
>>> @@ -117,6 +139,16 @@ examples:
>>>      #include <dt-bindings/power/qcom-rpmpd.h>
>>>      #include <dt-bindings/power/qcom,rpmhpd.h>
>>>  
>>> +    reserved-memory {
>>> +      #address-cells = <2>;
>>> +      #size-cells = <2>;
>>
>> Why do you need this?
> Was planning to drop this, as the reserved-memory region have it defined.
>>
>>> +
>>> +      iris_resv: reservation-iris {
>>
>> Mixing MMIO and non-MMIO is not the way to go. This is also not relevant
>> here. Don't embed other things into your binding example.
>>
>>
>>> +        iommu-addresses = <&iris_non_pixel 0x0 0x0 0x0 0x25800000>,
>>> +                          <&iris_non_pixel 0x0 0xe0000000 0x0 0x20000000>;
>>> +      };
>>> +    };
>>> +
>>>      video-codec@aa00000 {
>>>          compatible = "qcom,sm8550-iris";
>>>          reg = <0x0aa00000 0xf0000>;
>>> @@ -144,12 +176,16 @@ examples:
>>>          resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>>>          reset-names = "bus";
>>>  
>>> -        iommus = <&apps_smmu 0x1940 0x0000>,
>>> -                 <&apps_smmu 0x1947 0x0000>;
>>> +        iommus = <&apps_smmu 0x1947 0x0000>;
>>
>> Why did the device or hardware change? Nothing explains in commit msg
>> what is wrong with existing device and existing binding.
> Same query here, the commit well explains the limitation with existing device
> and how adding a new sub node mitigates the situation.

I read it and still do not get what is wrong with existing device. Which
hardware emits different stream-ID? How does it affect users? How can I
reproduce the problem?

Remember, that you are now affecting ABI.

Best regards,
Krzysztof

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Krzysztof Kozlowski]

On 02/07/2025 13:45, Vikash Garodia wrote:
> 
> On 7/2/2025 4:53 PM, Krzysztof Kozlowski wrote:
>> On 27/06/2025 17:48, Vikash Garodia wrote:
>>> +
>>>      video-codec@aa00000 {
>>>          compatible = "qcom,sm8550-iris";
>>>          reg = <0x0aa00000 0xf0000>;
>>> @@ -144,12 +176,16 @@ examples:
>>>          resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>>>          reset-names = "bus";
>>>  
>>> -        iommus = <&apps_smmu 0x1940 0x0000>,
>>> -                 <&apps_smmu 0x1947 0x0000>;
>>> +        iommus = <&apps_smmu 0x1947 0x0000>;
>>
>> I missed, that's technically ABI break and nothing in commit msg
>> explains that. You need to clearly explain the reasons and impact.
> No, it is not. Interface works well with old or new approach.


You changed the order of IOMMUs, so yes it is. Which interface works
well - FreeBSD? Or other? You are changing ABI for every user.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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

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

[Krzysztof Kozlowski]

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

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Vikash Garodia]

On 7/2/2025 5:17 PM, Krzysztof Kozlowski wrote:
> On 02/07/2025 13:45, Vikash Garodia wrote:
>>
>> On 7/2/2025 4:53 PM, Krzysztof Kozlowski wrote:
>>> On 27/06/2025 17:48, Vikash Garodia wrote:
>>>> +
>>>>      video-codec@aa00000 {
>>>>          compatible = "qcom,sm8550-iris";
>>>>          reg = <0x0aa00000 0xf0000>;
>>>> @@ -144,12 +176,16 @@ examples:
>>>>          resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>>>>          reset-names = "bus";
>>>>  
>>>> -        iommus = <&apps_smmu 0x1940 0x0000>,
>>>> -                 <&apps_smmu 0x1947 0x0000>;
>>>> +        iommus = <&apps_smmu 0x1947 0x0000>;
>>>
>>> I missed, that's technically ABI break and nothing in commit msg
>>> explains that. You need to clearly explain the reasons and impact.
>> No, it is not. Interface works well with old or new approach.
> 
> 
> You changed the order of IOMMUs, so yes it is. Which interface works
> well - FreeBSD? Or other? You are changing ABI for every user.
Why do i need to change, when without changing would work as well ?

> 
> Best regards,
> Krzysztof

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Krzysztof Kozlowski]

On 02/07/2025 13:55, Vikash Garodia wrote:
> 
> 
> On 7/2/2025 5:17 PM, Krzysztof Kozlowski wrote:
>> On 02/07/2025 13:45, Vikash Garodia wrote:
>>>
>>> On 7/2/2025 4:53 PM, Krzysztof Kozlowski wrote:
>>>> On 27/06/2025 17:48, Vikash Garodia wrote:
>>>>> +
>>>>>      video-codec@aa00000 {
>>>>>          compatible = "qcom,sm8550-iris";
>>>>>          reg = <0x0aa00000 0xf0000>;
>>>>> @@ -144,12 +176,16 @@ examples:
>>>>>          resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>>>>>          reset-names = "bus";
>>>>>  
>>>>> -        iommus = <&apps_smmu 0x1940 0x0000>,
>>>>> -                 <&apps_smmu 0x1947 0x0000>;
>>>>> +        iommus = <&apps_smmu 0x1947 0x0000>;
>>>>
>>>> I missed, that's technically ABI break and nothing in commit msg
>>>> explains that. You need to clearly explain the reasons and impact.
>>> No, it is not. Interface works well with old or new approach.
>>
>>
>> You changed the order of IOMMUs, so yes it is. Which interface works
>> well - FreeBSD? Or other? You are changing ABI for every user.
> Why do i need to change, when without changing would work as well ?
? I don't understand. I made a statement, not a question. You are doing
this - you are changing the ABI.

Which item was the first IOMMU before and which was second?

Which item is the first IOMMU now?

Best regards,
Krzysztof

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

[Vikash Garodia]

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

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

[Krzysztof Kozlowski]

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

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

[Bryan O'Donoghue]

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

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Vikash Garodia]

On 7/2/2025 5:28 PM, Krzysztof Kozlowski wrote:
> On 02/07/2025 13:55, Vikash Garodia wrote:
>>
>>
>> On 7/2/2025 5:17 PM, Krzysztof Kozlowski wrote:
>>> On 02/07/2025 13:45, Vikash Garodia wrote:
>>>>
>>>> On 7/2/2025 4:53 PM, Krzysztof Kozlowski wrote:
>>>>> On 27/06/2025 17:48, Vikash Garodia wrote:
>>>>>> +
>>>>>>      video-codec@aa00000 {
>>>>>>          compatible = "qcom,sm8550-iris";
>>>>>>          reg = <0x0aa00000 0xf0000>;
>>>>>> @@ -144,12 +176,16 @@ examples:
>>>>>>          resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>>>>>>          reset-names = "bus";
>>>>>>  
>>>>>> -        iommus = <&apps_smmu 0x1940 0x0000>,
>>>>>> -                 <&apps_smmu 0x1947 0x0000>;
>>>>>> +        iommus = <&apps_smmu 0x1947 0x0000>;
>>>>>
>>>>> I missed, that's technically ABI break and nothing in commit msg
>>>>> explains that. You need to clearly explain the reasons and impact.
>>>> No, it is not. Interface works well with old or new approach.
>>>
>>>
>>> You changed the order of IOMMUs, so yes it is. Which interface works
>>> well - FreeBSD? Or other? You are changing ABI for every user.
>> Why do i need to change, when without changing would work as well ?
> ? I don't understand. I made a statement, not a question. You are doing
> this - you are changing the ABI.
> 
> Which item was the first IOMMU before and which was second?
> 
> Which item is the first IOMMU now?
Old approach - max 2 iommus interface - <SID-A, SID-B>
New approach - min 1/max 2, iommu interface - <SID-B>, child - <SID-A>

If both works, how is interchanging impacting any existing hardware OR breaking
ABI ?

> 
> Best regards,
> Krzysztof

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Krzysztof Kozlowski]

On 02/07/2025 14:08, Vikash Garodia wrote:
> 
> On 7/2/2025 5:28 PM, Krzysztof Kozlowski wrote:
>> On 02/07/2025 13:55, Vikash Garodia wrote:
>>>
>>>
>>> On 7/2/2025 5:17 PM, Krzysztof Kozlowski wrote:
>>>> On 02/07/2025 13:45, Vikash Garodia wrote:
>>>>>
>>>>> On 7/2/2025 4:53 PM, Krzysztof Kozlowski wrote:
>>>>>> On 27/06/2025 17:48, Vikash Garodia wrote:
>>>>>>> +
>>>>>>>      video-codec@aa00000 {
>>>>>>>          compatible = "qcom,sm8550-iris";
>>>>>>>          reg = <0x0aa00000 0xf0000>;
>>>>>>> @@ -144,12 +176,16 @@ examples:
>>>>>>>          resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>>>>>>>          reset-names = "bus";
>>>>>>>  
>>>>>>> -        iommus = <&apps_smmu 0x1940 0x0000>,
>>>>>>> -                 <&apps_smmu 0x1947 0x0000>;
>>>>>>> +        iommus = <&apps_smmu 0x1947 0x0000>;
>>>>>>
>>>>>> I missed, that's technically ABI break and nothing in commit msg
>>>>>> explains that. You need to clearly explain the reasons and impact.
>>>>> No, it is not. Interface works well with old or new approach.
>>>>
>>>>
>>>> You changed the order of IOMMUs, so yes it is. Which interface works
>>>> well - FreeBSD? Or other? You are changing ABI for every user.
>>> Why do i need to change, when without changing would work as well ?
>> ? I don't understand. I made a statement, not a question. You are doing
>> this - you are changing the ABI.
>>
>> Which item was the first IOMMU before and which was second?
>>
>> Which item is the first IOMMU now?
> Old approach - max 2 iommus interface - <SID-A, SID-B>
> New approach - min 1/max 2, iommu interface - <SID-B>, child - <SID-A>

So you changed first IOMMU entry, first IOMMU master and that is
technically ABI break.

> 
> If both works, how is interchanging impacting any existing hardware OR breaking
> ABI ?

Because you change the entries. The ordering of lists - not iommus which
do not matter for Linux here - was discussed many times, so just refer
to that discussions.



Best regards,
Krzysztof

Re: [PATCH v3 2/5] media: iris: register and configure non-pixel node as platform device

[Konrad Dybcio]

On 6/27/25 5:48 PM, Vikash Garodia wrote:
> Register "non_pixel" child node as a platform device, and configure its
> DMA via of_dma_configure(). This ensures proper IOMMU attachment and DMA
> setup for the "non_pixel" device.
> All non pixel memory, i.e bitstream buffers, HFI queues and internal
> buffers related to bitstream processing, would be managed by non_pixel
> device.
> 
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---

[...]

> +	memset(&info, 0, sizeof(info));
> +	info.fwnode = &np_node->fwnode;
> +	info.parent = core->dev;
> +	info.name = np_node->name;
> +	info.dma_mask = DMA_BIT_MASK(32);

I'm not 1000% sure, but I fear that with the current description:

iris_resv: reservation-iris {
	iommu-addresses = <&iris_non_pixel 0x0 0x0 0x0 0x25800000>,
	                  <&iris_non_pixel 0x0 0xe0000000 0x0 0x20000000>;
};

this only works by luck, and once we introduce a platform that needs >32b
address space access, a change here will break the existing platforms, as
the higher parts are not forbidden.

We can work around it like the Tegra folks by filling out the upper size
dword, but I think it only further makes the iommu-addresses binding look
silly..

I'll submit a patch to (in my view) improve it soon

Konrad

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

[Vikash Garodia]

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

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Konrad Dybcio]

On 7/2/25 1:46 PM, Krzysztof Kozlowski wrote:
> On 02/07/2025 13:32, Vikash Garodia wrote:
>>
>> On 7/2/2025 4:43 PM, Krzysztof Kozlowski wrote:
>>> On 27/06/2025 17:48, Vikash Garodia wrote:
>>>> Existing definition 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. 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.
>>>>
>>>> With this, both iris and non-pixel device can have IOVA range of 0-4GiB
>>>> individually. Certain video usecases like higher video concurrency needs
>>>> IOVA higher than 4GiB.
>>>>
>>>> Add reference to the reserve-memory schema, which defines reserved IOVA

[...]

>>>>    dma-coherent: true
>>>>  
>>>> +  non-pixel:
>>>
>>> Why EXISTING hardware grows?
>> Same here, the commit describes the limitation of existing design and also
>> explains the need for having the non-pixel device. Its not that the hardware is
>> growing here, rather the hardware stream-IDs are utilized differently to get
>> higher device addressable range.
> 
> You are not doing this for a new device. There is no new device here at
> all. Nowhere here is a new device.
> 
> Changes for a new device COME TOGETHER with the new device.
> 
> What you are doing here is changing existing hardware without any
> explanation why.

This is bindings getting a reality check.. this goes as far back as Venus
existed (msm8974/2012)

We unfortunately have to expect a number of similar updates for all
multimedia peripherals (GPU/Camera/Display etc.), as certain mappings
must be done through certain SIDs (which are deemed 'secure') and some
hardware has general addressing limitations that may have been causing
silent issues all along

Konrad

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Krzysztof Kozlowski]

On 02/07/2025 15:11, Konrad Dybcio wrote:
> On 7/2/25 1:46 PM, Krzysztof Kozlowski wrote:
>> On 02/07/2025 13:32, Vikash Garodia wrote:
>>>
>>> On 7/2/2025 4:43 PM, Krzysztof Kozlowski wrote:
>>>> On 27/06/2025 17:48, Vikash Garodia wrote:
>>>>> Existing definition 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. 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.
>>>>>
>>>>> With this, both iris and non-pixel device can have IOVA range of 0-4GiB
>>>>> individually. Certain video usecases like higher video concurrency needs
>>>>> IOVA higher than 4GiB.
>>>>>
>>>>> Add reference to the reserve-memory schema, which defines reserved IOVA
> 
> [...]
> 
>>>>>    dma-coherent: true
>>>>>  
>>>>> +  non-pixel:
>>>>
>>>> Why EXISTING hardware grows?
>>> Same here, the commit describes the limitation of existing design and also
>>> explains the need for having the non-pixel device. Its not that the hardware is
>>> growing here, rather the hardware stream-IDs are utilized differently to get
>>> higher device addressable range.
>>
>> You are not doing this for a new device. There is no new device here at
>> all. Nowhere here is a new device.
>>
>> Changes for a new device COME TOGETHER with the new device.
>>
>> What you are doing here is changing existing hardware without any
>> explanation why.
> 
> This is bindings getting a reality check.. this goes as far back as Venus
> existed (msm8974/2012)

This won't fly. This is a new binding after long reviews and
discussions, why Qualcomm does not want to extend existing Venus but
needs completely new Iris. Well, if you get completely new Iris, you
cannot use argument of "legacy". We insisted on growing existing
solution, but choice of abandoning it and coming with a new one means
you were supposed to do it right since there is no legacy.

> 
> We unfortunately have to expect a number of similar updates for all
> multimedia peripherals (GPU/Camera/Display etc.), as certain mappings
> must be done through certain SIDs (which are deemed 'secure') and some
> hardware has general addressing limitations that may have been causing
> silent issues all along
> 
Isn't all this commit msg here about non-pixel stuff just not really
describing the real problem at all? This commit msg is very vague and
silent on actual use cases and actual firmware, so even multiple
readings of commit msg did not help me. Stephan Gerhold now nicely
summarized what was never told in commit msg - there is a gap in address
space which is reserved for firmware and no allocations can be done from
that?

Also commit msg says "Existing definition limits the IOVA to an
addressable range of 4GiB, and" but I do not see such definition in the
binding at all. So what does it refer to?



Best regards,
Krzysztof

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Vikash Garodia]

On 7/2/2025 7:29 PM, Krzysztof Kozlowski wrote:
> On 02/07/2025 15:11, Konrad Dybcio wrote:
>> On 7/2/25 1:46 PM, Krzysztof Kozlowski wrote:
>>> On 02/07/2025 13:32, Vikash Garodia wrote:
>>>>
>>>> On 7/2/2025 4:43 PM, Krzysztof Kozlowski wrote:
>>>>> On 27/06/2025 17:48, Vikash Garodia wrote:
>>>>>> Existing definition 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. 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.
>>>>>>
>>>>>> With this, both iris and non-pixel device can have IOVA range of 0-4GiB
>>>>>> individually. Certain video usecases like higher video concurrency needs
>>>>>> IOVA higher than 4GiB.
>>>>>>
>>>>>> Add reference to the reserve-memory schema, which defines reserved IOVA
>>
>> [...]
>>
>>>>>>    dma-coherent: true
>>>>>>  
>>>>>> +  non-pixel:
>>>>>
>>>>> Why EXISTING hardware grows?
>>>> Same here, the commit describes the limitation of existing design and also
>>>> explains the need for having the non-pixel device. Its not that the hardware is
>>>> growing here, rather the hardware stream-IDs are utilized differently to get
>>>> higher device addressable range.
>>>
>>> You are not doing this for a new device. There is no new device here at
>>> all. Nowhere here is a new device.
>>>
>>> Changes for a new device COME TOGETHER with the new device.
>>>
>>> What you are doing here is changing existing hardware without any
>>> explanation why.
>>
>> This is bindings getting a reality check.. this goes as far back as Venus
>> existed (msm8974/2012)
> 
> This won't fly. This is a new binding after long reviews and
> discussions, why Qualcomm does not want to extend existing Venus but
> needs completely new Iris. Well, if you get completely new Iris, you
> cannot use argument of "legacy". We insisted on growing existing
> solution, but choice of abandoning it and coming with a new one means
> you were supposed to do it right since there is no legacy.
> 
>>
>> We unfortunately have to expect a number of similar updates for all
>> multimedia peripherals (GPU/Camera/Display etc.), as certain mappings
>> must be done through certain SIDs (which are deemed 'secure') and some
>> hardware has general addressing limitations that may have been causing
>> silent issues all along
>>
> Isn't all this commit msg here about non-pixel stuff just not really
> describing the real problem at all? This commit msg is very vague and
> silent on actual use cases and actual firmware, so even multiple
> readings of commit msg did not help me. Stephan Gerhold now nicely
> summarized what was never told in commit msg - there is a gap in address
> space which is reserved for firmware and no allocations can be done from
> that?
Yes precisely that. Thanks to Stephan for clarifying.

An existing example which is defined in reserve-memory schema here
https://github.com/devicetree-org/dt-schema/blame/main/dtschema/schemas/reserved-memory/reserved-memory.yaml#L149

> 
> Also commit msg says "Existing definition limits the IOVA to an
> addressable range of 4GiB, and" but I do not see such definition in the
> binding at all. So what does it refer to?
Processors based out of 32 bit OS, can serve addresses in range 0-31, which
implies 4GiB (2pow31).
> 
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Krzysztof Kozlowski]

On 02/07/2025 18:36, Vikash Garodia wrote:
>>
>> Also commit msg says "Existing definition limits the IOVA to an
>> addressable range of 4GiB, and" but I do not see such definition in the
>> binding at all. So what does it refer to?
> Processors based out of 32 bit OS, can serve addresses in range 0-31, which
> implies 4GiB (2pow31).
You are not replying to statements. Your commit msg said "existing
definition". Point me to the binding part saying that.

Best regards,
Krzysztof

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

[Dmitry Baryshkov]

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

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

[Krzysztof Kozlowski]

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

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Krzysztof Kozlowski]

On 02/07/2025 13:13, Krzysztof Kozlowski wrote:
>> ---
>>  .../bindings/media/qcom,sm8550-iris.yaml           | 40 ++++++++++++++++++++--
>>  1 file changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> index c79bf2101812d83b99704f38b7348a9f728dff44..4dda2c9ca1293baa7aee3b9ee10aff38d280fe05 100644
>> --- a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>> @@ -65,10 +65,31 @@ properties:
>>        - const: core
>>  
>>    iommus:
>> +    minItems: 1
>>      maxItems: 2
> 
> No, why hardware suddenly has different amount?
> 
>>  
>>    dma-coherent: true
>>  
>> +  non-pixel:
> 
> Why EXISTING hardware grows?

One more comment here - this entire node is not needed, there is no
device here being described. Just solve the problem in the main device node.

Best regards,
Krzysztof

Re: [PATCH v3 1/5] media: dt-bindings: add non-pixel property in iris schema

[Konrad Dybcio]

On 02-Jul-25 15:59, Krzysztof Kozlowski wrote:
> On 02/07/2025 15:11, Konrad Dybcio wrote:
>> On 7/2/25 1:46 PM, Krzysztof Kozlowski wrote:
>>> On 02/07/2025 13:32, Vikash Garodia wrote:
>>>>
>>>> On 7/2/2025 4:43 PM, Krzysztof Kozlowski wrote:
>>>>> On 27/06/2025 17:48, Vikash Garodia wrote:
>>>>>> Existing definition 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. 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.
>>>>>>
>>>>>> With this, both iris and non-pixel device can have IOVA range of 0-4GiB
>>>>>> individually. Certain video usecases like higher video concurrency needs
>>>>>> IOVA higher than 4GiB.
>>>>>>
>>>>>> Add reference to the reserve-memory schema, which defines reserved IOVA
>>
>> [...]
>>
>>>>>>    dma-coherent: true
>>>>>>  
>>>>>> +  non-pixel:
>>>>>
>>>>> Why EXISTING hardware grows?
>>>> Same here, the commit describes the limitation of existing design and also
>>>> explains the need for having the non-pixel device. Its not that the hardware is
>>>> growing here, rather the hardware stream-IDs are utilized differently to get
>>>> higher device addressable range.
>>>
>>> You are not doing this for a new device. There is no new device here at
>>> all. Nowhere here is a new device.
>>>
>>> Changes for a new device COME TOGETHER with the new device.
>>>
>>> What you are doing here is changing existing hardware without any
>>> explanation why.
>>
>> This is bindings getting a reality check.. this goes as far back as Venus
>> existed (msm8974/2012)
> 
> This won't fly. This is a new binding after long reviews and
> discussions, why Qualcomm does not want to extend existing Venus but
> needs completely new Iris. Well, if you get completely new Iris, you
> cannot use argument of "legacy". We insisted on growing existing
> solution, but choice of abandoning it and coming with a new one means
> you were supposed to do it right since there is no legacy.

So maybe I worded this in an unfortunate way.. I meant Venus the HW
block, not Venus the driver. Even the ancient msm8974 has the same
addressing limitations and a separate IOMMU domain for
non_pixel/secure/etc.

>> We unfortunately have to expect a number of similar updates for all
>> multimedia peripherals (GPU/Camera/Display etc.), as certain mappings
>> must be done through certain SIDs (which are deemed 'secure') and some
>> hardware has general addressing limitations that may have been causing
>> silent issues all along
>>
> Isn't all this commit msg here about non-pixel stuff just not really
> describing the real problem at all? This commit msg is very vague and
> silent on actual use cases and actual firmware, so even multiple
> readings of commit msg did not help me. Stephan Gerhold now nicely
> summarized what was never told in commit msg - there is a gap in address
> space which is reserved for firmware and no allocations can be done from
> that?
> 
> Also commit msg says "Existing definition limits the IOVA to an
> addressable range of 4GiB, and" but I do not see such definition in the
> binding at all. So what does it refer to?

Yeah, there's many parts to this. The solution that this patchset
proposes will essentially need to be copypasted a couple times if
this is going to go in as-is.. see qcom,msm-vidc,context-bank (lol)
subnodes, each describing some combination of the above problems:

https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/tags/android-11.0.0_r0.56/qcom/scuba-vidc.dtsi#58

Konrad

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

[Konrad Dybcio]

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
> 

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

[Krzysztof Kozlowski]

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

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

[Konrad Dybcio]

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

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

[Bryan O'Donoghue]

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

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

[Dmitry Baryshkov]

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

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

[Bryan O'Donoghue]

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

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

[Konrad Dybcio]

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

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

[Dmitry Baryshkov]

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

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

[Vikash Garodia]

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/

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

[Bryan O'Donoghue]

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

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

[Prakash Gupta]

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

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

[Konrad Dybcio]

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