Re: [PATCH v9 5/5] media: iris: Add BUF_ROIMB_DELTAQP metadata buffer for ROI delta QP
From: Deepa Guthyappa Madivalara
Date: Fri Oct 16 2026 - 19:02:46 EST
On 10/9/2026 1:36 AM, Bryan O'Donoghue wrote:
On 05/10/2026 22:38, Deepa Guthyappa Madivalara wrote:It is needed to add HFI_PROP_ROI_INFO into the buffer header and it is part of iris_hfi_gen2_defines.h
- Introduce BUF_ROIMB_DELTAQP as a new internal buffer type to carry
per-macroblock ROI delta QP data to the firmware. This buffer is
allocated on demand when the application sets ROI parameters via
V4L2_CID_MPEG_VIDEO_ROI_MB_DELTA_QP, and is queued alongside each
raw input buffer as an HFI_BUFFER_METADATA packet on HFI_PORT_RAW.
- The metadata buffer layout follows the firmware-defined metabuf_header
and metapayload_header structures. The s8 per-MB delta QP values
supplied by userspace are widened to s16 as required by the firmware.
Reviewed-by: Vishnu Reddy <busanna.reddy@xxxxxxxxxxxxxxxx>
Signed-off-by: Deepa Guthyappa Madivalara <deepa.madivalara@xxxxxxxxxxxxxxxx>
---
drivers/media/platform/qcom/iris/iris_buffer.c | 114 +++++++++++++++++++++
drivers/media/platform/qcom/iris/iris_buffer.h | 4 +
drivers/media/platform/qcom/iris/iris_ctrls.c | 28 +++++
drivers/media/platform/qcom/iris/iris_ctrls.h | 1 +
drivers/media/platform/qcom/iris/iris_hfi_gen2.c | 1 +
.../platform/qcom/iris/iris_hfi_gen2_command.c | 24 +++++
.../platform/qcom/iris/iris_hfi_gen2_response.c | 2 +
drivers/media/platform/qcom/iris/iris_vidc.c | 2 +
8 files changed, 176 insertions(+)
diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c
index eb8de60c1177f5e1ab83b90a3c8c80e0b4d1f02e..a7f1d74360d6a490959b27936fa5bbc61a7d9358 100644
--- a/drivers/media/platform/qcom/iris/iris_buffer.c
+++ b/drivers/media/platform/qcom/iris/iris_buffer.c
@@ -5,11 +5,13 @@
#include <media/v4l2-event.h>
#include <media/v4l2-mem2mem.h>
+#include <linux/slab.h>
#include "iris_buffer.h"
#include "iris_instance.h"
#include "iris_power.h"
#include "iris_vpu_buffer.h"
+#include "iris_hfi_gen2_defines.h"
Why pull in gen2 defines to generic code ?
Done.
#define PIXELS_4K 4096
#define MAX_WIDTH 4096
@@ -705,6 +707,20 @@ int iris_destroy_dequeued_internal_buffers(struct iris_inst *inst, u32 plane)
return iris_destroy_internal_buffers(inst, plane, false);
}
+int iris_destroy_roi_metadata_buffers(struct iris_inst *inst)
+{
+ struct iris_buffer *buf, *next;
+ struct iris_buffers *buffers;
+
+ if (inst->domain == ENCODER) {
+ buffers = &inst->buffers[BUF_ROIMB_DELTAQP];
+ list_for_each_entry_safe(buf, next, &buffers->list, list)
+ iris_destroy_internal_buffer(inst, buf);
+ }
+
+ return 0;
+}
+
static int iris_release_internal_buffers(struct iris_inst *inst,
enum iris_buffer_type buffer_type)
{
@@ -928,3 +944,101 @@ int iris_vb2_buffer_done(struct iris_inst *inst, struct iris_buffer *buf)
return 0;
}
+
+static int iris_fill_roi_data(struct iris_inst *inst, struct iris_buffer *buffer)
+{
+ s8 *p_array = (s8 *)inst->fw_caps[ROI_PARAMS].p_array;
+ u32 payload_offset, lcu_width, lcu_height, row_size;
+ u32 array_size = inst->fw_caps[ROI_PARAMS].elems;
+ struct metabuf_header *mbuf_hdr = buffer->kvaddr;
+ struct metapayload_header *mbuf_payload_hdr;
+ struct v4l2_format *f = inst->fmt_src;
+ s16 *p_16, *p_temp;
+
+ memset(mbuf_hdr, 0, sizeof(struct metabuf_header));
Just memeset the buffer for its agrregate extent instead of memsetting at an offset again laster.
Updated in V10.+ mbuf_hdr->count = 1;
+ mbuf_hdr->size = sizeof(struct metabuf_header) +
+ sizeof(struct metapayload_header);
+ mbuf_hdr->version = BIT(16);
+ mbuf_hdr++;
+ mbuf_payload_hdr = (struct metapayload_header *)(mbuf_hdr);
+ payload_offset = sizeof(struct metabuf_header) +
+ sizeof(struct metapayload_header);
+
+ memset(mbuf_payload_hdr, 0, sizeof(struct metapayload_header));
+ mbuf_payload_hdr->type = HFI_PROP_ROI_INFO;
+ mbuf_payload_hdr->size = array_size * sizeof(s16);
+ mbuf_payload_hdr->version = 1 << 16;
BIT(16) not 1 << 16
You've already done the right thing nine line lines earlier..
Seems redundant. updated in V10.+ mbuf_payload_hdr->offset = ALIGN(payload_offset, (u32)256);
Why the cast to u32 ?
Updated in V10.
+ mbuf_payload_hdr->flags = 0;
flags is already zero from the memset();
Updated in V10.+
+ if (inst->codec == V4L2_PIX_FMT_HEVC) {
+ lcu_width = (f->fmt.pix_mp.width + 31) >> 5;
+ lcu_height = (f->fmt.pix_mp.height + 31) >> 5;
This code looks a bit odd.
lcu_width = (f->fmt.pix_mp.width + 31) >> 5;
->
DIV_ROUND_UP(f->fmt.pix_mp.width, 32);
Updated in V10.+ } else {
+ lcu_width = (f->fmt.pix_mp.width + 15) >> 4;
+ lcu_height = (f->fmt.pix_mp.height + 15) >> 4;
DIV_ROUND_UP()
Updated in V10.+ }
+ /* Firmware expects each row to align to 8 LCU */
+ row_size = (((lcu_width + 7) >> 3) << 3);
ALIGN(x, 8);
Updated in V10. Ideally expect the client to set right array_size but added a check here as well.+ /* Firmware expects 2bytes of delta_Qp, int16_t */
+ p_16 = buffer->kvaddr + mbuf_payload_hdr->offset;
+ for (int j = 0; j < lcu_height; j++) {
mbuf_payload_hdr->size = array_size * sizeof(s16) is the size of the buffer - lcu_height is not.
Updated in V10+ p_temp = p_16 + j * row_size;
+ for (int i = 0; i < lcu_width; i++)
+ *p_temp++ = (1 << 11) | (((u8)p_array[j * lcu_width + i]
+ & 0x3F) << 4);
u8 is not required here.
BIT(11) not (1 << 11)
Redundant, removed+ }
+
+ return 0;
+}
+
+int iris_hfi_gen2_session_alloc_roi_metadata_buffer(struct iris_inst *inst)
+{
+ struct iris_buffers *buffers = &inst->buffers[BUF_ROIMB_DELTAQP];
+ struct iris_core *core = inst->core;
+ struct iris_buffer *buffer, *first_buffer, *next;
+ bool found = false;
+ int ret = 0;
+
+ if (!buffers->size)
+ return 0;
+
+ list_for_each_entry_safe(buffer, next, &buffers->list, list) {
+ if (buffer->attr & BUF_ATTR_DEQUEUED) {
+ buffer->attr &= ~BUF_ATTR_DEQUEUED;
+ list_move(&buffer->list, &buffers->list);
+ found = true;
+ break;
+ }
+ }
+ if (!found) {
+ buffer = kzalloc_obj(*buffer);
+ if (!buffer)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(&buffer->list);
+ buffer->type = BUF_ROIMB_DELTAQP;
+ buffer->index++;
This is equivalent to buffer->index = 1;
But why ?
Updated in V10.+ buffer->buffer_size = buffers->size;
+ buffer->dma_attrs = DMA_ATTR_WRITE_COMBINE;
+
+ buffer->kvaddr = dma_alloc_attrs(core->dev, buffer->buffer_size,
+ &buffer->device_addr, GFP_KERNEL,
+ buffer->dma_attrs);
+
+ if (!buffer->kvaddr) {
+ kfree(buffer);
+ return -ENOMEM;
+ }
+ list_add(&buffer->list, &buffers->list);
+ }
+
+ first_buffer = list_first_entry(&buffers->list, struct iris_buffer, list);
+ ret = iris_fill_roi_data(inst, first_buffer);
+ if (ret)
+ return ret;
You could drop first_buffer and just use buffer here;
Updated in V10.+
+ return ret;
if (ret)
return ret;
return 0;
Verified with some print statements for the buffer size and data being set as same.+}
diff --git a/drivers/media/platform/qcom/iris/iris_buffer.h b/drivers/media/platform/qcom/iris/iris_buffer.h
index ee2d24bb69c57220b0a735d9b4aae4434a33daf6..df9d018493f3ec37fd5e6b0ba049dba0b5ae76e9 100644
--- a/drivers/media/platform/qcom/iris/iris_buffer.h
+++ b/drivers/media/platform/qcom/iris/iris_buffer.h
@@ -28,6 +28,7 @@ struct iris_inst;
* @BUF_SCRATCH_2: buffer to store encoding context data for HW
* @BUF_VPSS: buffer to store VPSS context data for HW
* @BUF_PARTIAL: buffer for AV1 IBC data
+ * @BUF_ROIMB_DELTAQP: metadata buffer for ROI MB DeltaQp
* @BUF_TYPE_MAX: max buffer types
*/
enum iris_buffer_type {
@@ -44,6 +45,7 @@ enum iris_buffer_type {
BUF_SCRATCH_2,
BUF_VPSS,
BUF_PARTIAL,
+ BUF_ROIMB_DELTAQP,
BUF_TYPE_MAX,
};
@@ -139,5 +141,7 @@ int iris_queue_buffer(struct iris_inst *inst, struct iris_buffer *buf);
int iris_queue_deferred_buffers(struct iris_inst *inst, enum iris_buffer_type buf_type);
int iris_vb2_buffer_done(struct iris_inst *inst, struct iris_buffer *buf);
void iris_vb2_queue_error(struct iris_inst *inst);
+int iris_hfi_gen2_session_alloc_roi_metadata_buffer(struct iris_inst *inst);
+int iris_destroy_roi_metadata_buffers(struct iris_inst *inst);
#endif
diff --git a/drivers/media/platform/qcom/iris/iris_ctrls.c b/drivers/media/platform/qcom/iris/iris_ctrls.c
index cd5597b2cbb69f0608fb8f21bd813e11c1f97693..fc6499598926af071fbd9f989c01e638d98efa29 100644
--- a/drivers/media/platform/qcom/iris/iris_ctrls.c
+++ b/drivers/media/platform/qcom/iris/iris_ctrls.c
@@ -1716,3 +1716,31 @@ int iris_set_metadata_delivery(struct iris_inst *inst, u32 plane)
return hfi_ops->session_subscribe_metadata_delivery(inst, plane);
}
+
+int iris_set_roi_params(struct iris_inst *inst, u32 plane)
+{
+ struct iris_buffers *buffers = &inst->buffers[BUF_ROIMB_DELTAQP];
+ u32 height = inst->fmt_src->fmt.pix_mp.height;
+ u32 width = inst->fmt_src->fmt.pix_mp.width;
+ u32 metadata_header_bytes = 256;
+ u32 lcu_height = 0;
+ u32 lcu_width = 0;
+ u32 lcu_size = 16;
+ u32 n_shift = 0;
+ u32 size = 0;
+
+ if (!inst->fw_caps[ROI_PARAMS].p_array)
+ return -EINVAL;
+
+ if (inst->codec == V4L2_PIX_FMT_HEVC)
+ lcu_size = 32;
+
+ n_shift = ilog2(lcu_size);
+ lcu_width = (width + (lcu_size - 1)) >> n_shift;
+ lcu_height = (height + (lcu_size - 1)) >> n_shift;
+ size = (((lcu_width + 7) >> 3) << 3) * lcu_height * 2;
+ size += metadata_header_bytes;
+ buffers->size = ALIGN(size, 4096);
So you set the size here but, this doesn't correlate to the size in the alloc() path.
Its possible then for a value to be set that overflows the allocated buffer on the kernel side ?
I am using buffers->size in iris_hfi_gen2_session_alloc_roi_metadata_buffer to allocate the buffer.
+
+ return iris_hfi_gen2_session_alloc_roi_metadata_buffer(inst);
+}
diff --git a/drivers/media/platform/qcom/iris/iris_ctrls.h b/drivers/media/platform/qcom/iris/iris_ctrls.h
index ef2c09485ad93719e4acc7f395899d7f67cb5b2d..d5fb33a8e2a19035a0ff5eb412ffb8167e6098f3 100644
--- a/drivers/media/platform/qcom/iris/iris_ctrls.h
+++ b/drivers/media/platform/qcom/iris/iris_ctrls.h
@@ -52,5 +52,6 @@ int iris_set_time_delta_based_rc(struct iris_inst *inst, enum platform_inst_fw_c
int iris_set_slice_count(struct iris_inst *inst, enum platform_inst_fw_cap_type cap_id);
int iris_set_properties(struct iris_inst *inst, u32 plane);
int iris_set_metadata_delivery(struct iris_inst *inst, u32 plane);
+int iris_set_roi_params(struct iris_inst *inst, u32 plane);
#endif
diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2.c b/drivers/media/platform/qcom/iris/iris_hfi_gen2.c
index 0f4c6f955f4f408c45c50796332b47c116f52982..36d2517140ce67821962c200093a9887e5b6b392 100644
--- a/drivers/media/platform/qcom/iris/iris_hfi_gen2.c
+++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2.c
@@ -1822,6 +1822,7 @@ static const struct platform_inst_fw_cap inst_fw_cap_sm8550_enc[] = {
.hfi_id = HFI_PROP_ROI_INFO,
.flags = CAP_FLAG_INPUT_PORT | CAP_FLAG_CUSTOM |
CAP_FLAG_DYNAMIC_ALLOWED,
+ .set = iris_set_roi_params,
},
{
.cap_id = MB_SIZE,
diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
index cf88dbe11e8e9faef826e661d8e9909fd7f16b94..a1b0c4982f0d789a807f22a1dffdf61decd20d28 100644
--- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
+++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
@@ -134,6 +134,7 @@ static u32 iris_hfi_gen2_get_port_from_buf_type(struct iris_inst *inst,
switch (buffer_type) {
case BUF_INPUT:
case BUF_VPSS:
+ case BUF_ROIMB_DELTAQP:
return HFI_PORT_RAW;
case BUF_OUTPUT:
case BUF_BIN:
@@ -1267,6 +1268,8 @@ static u32 iris_hfi_gen2_buf_type_from_driver(u32 domain, enum iris_buffer_type
return HFI_BUFFER_VPSS;
case BUF_PARTIAL:
return HFI_BUFFER_PARTIAL_DATA;
+ case BUF_ROIMB_DELTAQP:
+ return HFI_BUFFER_METADATA;
default:
return 0;
}
@@ -1328,6 +1331,7 @@ static struct iris_buffer *iris_queue_metadata_buffers(struct iris_inst *inst,
static int iris_hfi_gen2_session_queue_buffer(struct iris_inst *inst, struct iris_buffer *buffer)
{
struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst);
+ struct iris_hfi_buffer hfi_meta_buffer;
struct iris_hfi_buffer hfi_buffer;
u32 port;
int ret;
@@ -1349,6 +1353,26 @@ static int iris_hfi_gen2_session_queue_buffer(struct iris_inst *inst, struct iri
&hfi_buffer,
sizeof(hfi_buffer));
+ /* check if any metadata buffer is available not queued, queue it */
+ if (port == HFI_PORT_RAW) {
+ buffer = iris_queue_metadata_buffers(inst, BUF_ROIMB_DELTAQP, buffer->index);
+ if (buffer) {
+ iris_hfi_gen2_get_buffer(inst->domain, buffer, &hfi_meta_buffer);
+ port = iris_hfi_gen2_get_port_from_buf_type(inst, buffer->type);
+ iris_hfi_gen2_create_packet(inst_hfi_gen2->packet,
+ HFI_CMD_BUFFER,
+ HFI_HOST_FLAGS_INTR_REQUIRED,
+ HFI_PAYLOAD_STRUCTURE,
+ port,
+ inst->core->packet_id++,
+ &hfi_meta_buffer,
+ sizeof(hfi_meta_buffer));
+
+ buffer->attr |= BUF_ATTR_QUEUED;
+ buffer->attr &= ~BUF_ATTR_DEQUEUED;
+ }
+ }
+
return iris_hfi_queue_cmd_write(inst->core, inst_hfi_gen2->packet,
inst_hfi_gen2->packet->size);
}
diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
index ea1fc96077fdce83db9c6a6c1f753ec7978b0673..4f8f0b78f7b7559d985ea627f9a3224f36fb6f18 100644
--- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
+++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
@@ -58,6 +58,8 @@ static u32 iris_hfi_gen2_buf_type_to_driver(struct iris_inst *inst,
return BUF_PARTIAL;
case HFI_BUFFER_VPSS:
return BUF_VPSS;
+ case HFI_BUFFER_METADATA:
+ return BUF_ROIMB_DELTAQP;
default:
return 0;
}
diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c
index 05aab6e3005d5c3788f5dfe5857f2d6b782e7a83..ba56aca5c0d102869dcf7ad6c050579283000cd7 100644
--- a/drivers/media/platform/qcom/iris/iris_vidc.c
+++ b/drivers/media/platform/qcom/iris/iris_vidc.c
@@ -199,6 +199,7 @@ int iris_open(struct file *filp)
INIT_LIST_HEAD(&inst->buffers[BUF_SCRATCH_2].list);
INIT_LIST_HEAD(&inst->buffers[BUF_VPSS].list);
INIT_LIST_HEAD(&inst->buffers[BUF_PARTIAL].list);
+ INIT_LIST_HEAD(&inst->buffers[BUF_ROIMB_DELTAQP].list);
init_completion(&inst->completion);
init_completion(&inst->flush_completion);
@@ -321,6 +322,7 @@ int iris_close(struct file *filp)
iris_destroy_all_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
iris_check_num_queued_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
iris_check_num_queued_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
+ iris_destroy_roi_metadata_buffers(inst);
iris_remove_session(inst);
mutex_unlock(&inst->lock);
iris_inst_put(inst);
--
2.34.1