Linux ARM-MSM sub-architecture
 help / color / mirror / Atom feed
* [PATCH v2 0/6] media: iris: misc fixes for fluster, seek and concurrency issues
@ 2025-12-29  6:31 Dikshita Agarwal
  2025-12-29  6:31 ` [PATCH v2 1/6] media: iris: Add buffer to list only after successful allocation Dikshita Agarwal
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Dikshita Agarwal @ 2025-12-29  6:31 UTC (permalink / raw)
  To: Vikash Garodia, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue,
	Dikshita Agarwal, Vishnu Reddy

This series contains a set of fixes addressing issues uncovered during
fluster test, resolution-change, seek and concurrency on Iris.

Changes in v2:
- Added missing fixes tags (Bryan)
- Link to v1: https://lore.kernel.org/r/20251224-iris-fixes-v1-0-5f79861700ec@oss.qualcomm.com

Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
---
Dikshita Agarwal (5):
      media: iris: Add buffer to list only after successful allocation
      media: iris: Skip resolution set on first IPSC
      media: iris: gen1: Destroy internal buffers after FW releases
      Revert "media: iris: Add sanity check for stop streaming"
      media: iris: gen2: Add sanity check for session stop

Vishnu Reddy (1):
      media: iris: Prevent output buffer queuing before stream-on completes

 drivers/media/platform/qcom/iris/iris_buffer.c         |  7 +++++--
 .../media/platform/qcom/iris/iris_hfi_gen1_command.c   |  4 +++-
 .../media/platform/qcom/iris/iris_hfi_gen2_command.c   |  3 +++
 drivers/media/platform/qcom/iris/iris_vb2.c            | 18 ++++++++++--------
 4 files changed, 21 insertions(+), 11 deletions(-)
---
base-commit: 1f2353f5a1af995efbf7bea44341aa0d03460b28
change-id: 20251216-iris-fixes-4be01def1c7f

Best regards,
-- 
Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 1/6] media: iris: Add buffer to list only after successful allocation
  2025-12-29  6:31 [PATCH v2 0/6] media: iris: misc fixes for fluster, seek and concurrency issues Dikshita Agarwal
@ 2025-12-29  6:31 ` Dikshita Agarwal
  2026-01-07 10:26   ` Vikash Garodia
  2025-12-29  6:31 ` [PATCH v2 2/6] media: iris: Skip resolution set on first IPSC Dikshita Agarwal
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Dikshita Agarwal @ 2025-12-29  6:31 UTC (permalink / raw)
  To: Vikash Garodia, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue,
	Dikshita Agarwal

Move `list_add_tail()` to after `dma_alloc_attrs()` succeeds when creating
internal buffers. Previously, the buffer was enqueued in `buffers->list`
before the DMA allocation. If the allocation failed, the function returned
`-ENOMEM` while leaving a partially initialized buffer in the list, which
could lead to inconsistent state and potential leaks.

By adding the buffer to the list only after `dma_alloc_attrs()` succeeds,
we ensure the list contains only valid, fully initialized buffers.

Fixes: 73702f45db81 ("media: iris: allocate, initialize and queue internal buffers")
Reviewed-by: Bryan O'Donoghue <bod@kernel.org>
Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
---
 drivers/media/platform/qcom/iris/iris_buffer.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c
index b89b1ee06cce151e7c04a80956380d154643c116..f1f003a787bf22db6f048c9e682ba8ed2f39bc21 100644
--- a/drivers/media/platform/qcom/iris/iris_buffer.c
+++ b/drivers/media/platform/qcom/iris/iris_buffer.c
@@ -351,12 +351,15 @@ static int iris_create_internal_buffer(struct iris_inst *inst,
 	buffer->index = index;
 	buffer->buffer_size = buffers->size;
 	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,
 					 &buffer->device_addr, GFP_KERNEL, buffer->dma_attrs);
-	if (!buffer->kvaddr)
+	if (!buffer->kvaddr) {
+		kfree(buffer);
 		return -ENOMEM;
+	}
+
+	list_add_tail(&buffer->list, &buffers->list);
 
 	return 0;
 }

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 2/6] media: iris: Skip resolution set on first IPSC
  2025-12-29  6:31 [PATCH v2 0/6] media: iris: misc fixes for fluster, seek and concurrency issues Dikshita Agarwal
  2025-12-29  6:31 ` [PATCH v2 1/6] media: iris: Add buffer to list only after successful allocation Dikshita Agarwal
@ 2025-12-29  6:31 ` Dikshita Agarwal
  2026-01-07 10:27   ` Vikash Garodia
  2025-12-29  6:31 ` [PATCH v2 3/6] media: iris: gen1: Destroy internal buffers after FW releases Dikshita Agarwal
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Dikshita Agarwal @ 2025-12-29  6:31 UTC (permalink / raw)
  To: Vikash Garodia, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue,
	Dikshita Agarwal

The resolution property is not supposed to be set during reconfig.
Existing iris_drc_pending(inst) check is insufficient, as it doesn't
cover the first port setting change.

Extend the conditional check to also skip resolution setting when
the instance is in IRIS_INST_SUB_FIRST_IPSC.

Fixes: caf205548769 ("media: iris: Avoid updating frame size to firmware during reconfig")
Reviewed-by: Bryan O'Donoghue <bod@kernel.org>
Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
---
 drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
index 52da7ef7bab08fb1cb2ac804ccc6e3c7f9677890..5087e51daa842515e9d62730680fb237bf274efa 100644
--- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
+++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
@@ -733,7 +733,7 @@ static int iris_hfi_gen1_set_resolution(struct iris_inst *inst, u32 plane)
 	struct hfi_framesize fs;
 	int ret;
 
-	if (!iris_drc_pending(inst)) {
+	if (!iris_drc_pending(inst) && !(inst->sub_state & IRIS_INST_SUB_FIRST_IPSC)) {
 		fs.buffer_type = HFI_BUFFER_INPUT;
 		fs.width = inst->fmt_src->fmt.pix_mp.width;
 		fs.height = inst->fmt_src->fmt.pix_mp.height;

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 3/6] media: iris: gen1: Destroy internal buffers after FW releases
  2025-12-29  6:31 [PATCH v2 0/6] media: iris: misc fixes for fluster, seek and concurrency issues Dikshita Agarwal
  2025-12-29  6:31 ` [PATCH v2 1/6] media: iris: Add buffer to list only after successful allocation Dikshita Agarwal
  2025-12-29  6:31 ` [PATCH v2 2/6] media: iris: Skip resolution set on first IPSC Dikshita Agarwal
@ 2025-12-29  6:31 ` Dikshita Agarwal
  2026-01-07 10:27   ` Vikash Garodia
  2025-12-29  6:31 ` [PATCH v2 4/6] Revert "media: iris: Add sanity check for stop streaming" Dikshita Agarwal
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Dikshita Agarwal @ 2025-12-29  6:31 UTC (permalink / raw)
  To: Vikash Garodia, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue,
	Dikshita Agarwal

After the firmware releases internal buffers, the driver was not
destroying them. This left stale allocations that were no longer used,
especially across resolution changes where new buffers are allocated per
the updated requirements. As a result, memory was wasted until session
close.

Destroy internal buffers once the release response is received from the
firmware.

Fixes: 73702f45db81 ("media: iris: allocate, initialize and queue internal buffers")
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
---
 drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
index 5087e51daa842515e9d62730680fb237bf274efa..5ff71e25597b61587c674142feb99626e402c893 100644
--- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
+++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
@@ -441,6 +441,8 @@ static int iris_hfi_gen1_session_unset_buffers(struct iris_inst *inst, struct ir
 		goto exit;
 
 	ret = iris_wait_for_session_response(inst, false);
+	if (!ret)
+		ret = iris_destroy_internal_buffer(inst, buf);
 
 exit:
 	kfree(pkt);

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 4/6] Revert "media: iris: Add sanity check for stop streaming"
  2025-12-29  6:31 [PATCH v2 0/6] media: iris: misc fixes for fluster, seek and concurrency issues Dikshita Agarwal
                   ` (2 preceding siblings ...)
  2025-12-29  6:31 ` [PATCH v2 3/6] media: iris: gen1: Destroy internal buffers after FW releases Dikshita Agarwal
@ 2025-12-29  6:31 ` Dikshita Agarwal
  2025-12-30 10:25   ` Bryan O'Donoghue
  2026-01-07 10:29   ` Vikash Garodia
  2025-12-29  6:31 ` [PATCH v2 5/6] media: iris: gen2: Add sanity check for session stop Dikshita Agarwal
  2025-12-29  6:31 ` [PATCH v2 6/6] media: iris: Prevent output buffer queuing before stream-on completes Dikshita Agarwal
  5 siblings, 2 replies; 19+ messages in thread
From: Dikshita Agarwal @ 2025-12-29  6:31 UTC (permalink / raw)
  To: Vikash Garodia, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue,
	Dikshita Agarwal

Revert the check that skipped stop_streaming when the instance was in
IRIS_INST_ERROR, as it caused multiple regressions:

1. Buffers were not returned to vb2 when the instance was already in
   error state, triggering warnings in the vb2 core because buffer
   completion was skipped.

2. If a session failed early (e.g. unsupported configuration), the
   instance transitioned to IRIS_INST_ERROR. When userspace attempted
   to stop streaming for cleanup, stop_streaming was skipped due to the
   added check, preventing proper teardown and leaving the firmware
   in an inconsistent state.

Fixes: ad699fa78b59 ("media: iris: Add sanity check for stop streaming")
Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
---
 drivers/media/platform/qcom/iris/iris_vb2.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c b/drivers/media/platform/qcom/iris/iris_vb2.c
index db8768d8a8f61c9ceb04e423d0a769d35114e20e..139b821f7952feb33b21a7045aef9e8a4782aa3c 100644
--- a/drivers/media/platform/qcom/iris/iris_vb2.c
+++ b/drivers/media/platform/qcom/iris/iris_vb2.c
@@ -231,8 +231,6 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
 		return;
 
 	mutex_lock(&inst->lock);
-	if (inst->state == IRIS_INST_ERROR)
-		goto exit;
 
 	if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
 	    !V4L2_TYPE_IS_CAPTURE(q->type))
@@ -243,10 +241,10 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
 		goto exit;
 
 exit:
-	if (ret) {
-		iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
+	iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
+	if (ret)
 		iris_inst_change_state(inst, IRIS_INST_ERROR);
-	}
+
 	mutex_unlock(&inst->lock);
 }
 

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 5/6] media: iris: gen2: Add sanity check for session stop
  2025-12-29  6:31 [PATCH v2 0/6] media: iris: misc fixes for fluster, seek and concurrency issues Dikshita Agarwal
                   ` (3 preceding siblings ...)
  2025-12-29  6:31 ` [PATCH v2 4/6] Revert "media: iris: Add sanity check for stop streaming" Dikshita Agarwal
@ 2025-12-29  6:31 ` Dikshita Agarwal
  2025-12-30 10:33   ` Bryan O'Donoghue
  2026-01-12 11:09   ` Vikash Garodia
  2025-12-29  6:31 ` [PATCH v2 6/6] media: iris: Prevent output buffer queuing before stream-on completes Dikshita Agarwal
  5 siblings, 2 replies; 19+ messages in thread
From: Dikshita Agarwal @ 2025-12-29  6:31 UTC (permalink / raw)
  To: Vikash Garodia, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue,
	Dikshita Agarwal

In iris_kill_session, inst->state is set to IRIS_INST_ERROR and
session_close is executed, which will kfree(inst_hfi_gen2->packet).
If stop_streaming is called afterward, it will cause a crash.

Add a NULL check for inst_hfi_gen2->packet before sendling STOP packet
to firmware to fix that.

Fixes: 11712ce70f8e ("media: iris: implement vb2 streaming ops")
Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
---
 drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c | 3 +++
 1 file changed, 3 insertions(+)

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 6a772db2ec33fb002d8884753a41dc98b3a8439d..59e41adcce9aadd7c60bb1d369d68a4954f62aef 100644
--- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
+++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
@@ -963,6 +963,9 @@ static int iris_hfi_gen2_session_stop(struct iris_inst *inst, u32 plane)
 	struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst);
 	int ret = 0;
 
+	if (!inst_hfi_gen2->packet)
+		return -EINVAL;
+
 	reinit_completion(&inst->completion);
 
 	iris_hfi_gen2_packet_session_command(inst,

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 6/6] media: iris: Prevent output buffer queuing before stream-on completes
  2025-12-29  6:31 [PATCH v2 0/6] media: iris: misc fixes for fluster, seek and concurrency issues Dikshita Agarwal
                   ` (4 preceding siblings ...)
  2025-12-29  6:31 ` [PATCH v2 5/6] media: iris: gen2: Add sanity check for session stop Dikshita Agarwal
@ 2025-12-29  6:31 ` Dikshita Agarwal
  2026-01-12 11:09   ` Vikash Garodia
  5 siblings, 1 reply; 19+ messages in thread
From: Dikshita Agarwal @ 2025-12-29  6:31 UTC (permalink / raw)
  To: Vikash Garodia, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue,
	Dikshita Agarwal, Vishnu Reddy

From: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>

During normal playback, stream-on for input is followed by output, and
only after input stream-on does actual streaming begin. However, when
gst-play performs a seek, both input and output streams are stopped,
and on restart, output stream-on occurs first. At this point, firmware
has not yet started streaming. Queuing output buffers before the firmware
begins streaming causes it to process buffers in an invalid state, leading
to an error response. These buffers are returned to the driver as errors,
forcing the driver into an error state and stopping playback.

Fix this by deferring output buffer queuing until stream-on completes.
Input buffers can still be queued before stream-on as required.

Fixes: 92e007ca5ab6 ("media: iris: Add V4L2 streaming support for encoder video device")
Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
---
 drivers/media/platform/qcom/iris/iris_vb2.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c b/drivers/media/platform/qcom/iris/iris_vb2.c
index 139b821f7952feb33b21a7045aef9e8a4782aa3c..bf0b8400996ece5c9d449b99609a302da726bf9a 100644
--- a/drivers/media/platform/qcom/iris/iris_vb2.c
+++ b/drivers/media/platform/qcom/iris/iris_vb2.c
@@ -193,10 +193,14 @@ int iris_vb2_start_streaming(struct vb2_queue *q, unsigned int count)
 	buf_type = iris_v4l2_type_to_driver(q->type);
 
 	if (inst->domain == DECODER) {
-		if (inst->state == IRIS_INST_STREAMING)
+		if (buf_type == BUF_INPUT)
+			ret = iris_queue_deferred_buffers(inst, BUF_INPUT);
+
+		if (!ret && inst->state == IRIS_INST_STREAMING) {
 			ret = iris_queue_internal_deferred_buffers(inst, BUF_DPB);
-		if (!ret)
-			ret = iris_queue_deferred_buffers(inst, buf_type);
+			if (!ret)
+				ret = iris_queue_deferred_buffers(inst, BUF_OUTPUT);
+		}
 	} else {
 		if (inst->state == IRIS_INST_STREAMING) {
 			ret = iris_queue_deferred_buffers(inst, BUF_INPUT);

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 4/6] Revert "media: iris: Add sanity check for stop streaming"
  2025-12-29  6:31 ` [PATCH v2 4/6] Revert "media: iris: Add sanity check for stop streaming" Dikshita Agarwal
@ 2025-12-30 10:25   ` Bryan O'Donoghue
  2026-01-05  9:51     ` Dikshita Agarwal
  2026-01-07 10:29   ` Vikash Garodia
  1 sibling, 1 reply; 19+ messages in thread
From: Bryan O'Donoghue @ 2025-12-30 10:25 UTC (permalink / raw)
  To: Dikshita Agarwal, Vikash Garodia, Abhinav Kumar,
	Bryan O'Donoghue, Mauro Carvalho Chehab, Hans Verkuil,
	Stefan Schmidt, Hans Verkuil, Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel

On 29/12/2025 06:31, Dikshita Agarwal wrote:
> Revert the check that skipped stop_streaming when the instance was in
> IRIS_INST_ERROR, as it caused multiple regressions:
> 
> 1. Buffers were not returned to vb2 when the instance was already in
>     error state, triggering warnings in the vb2 core because buffer
>     completion was skipped.
> 
> 2. If a session failed early (e.g. unsupported configuration), the
>     instance transitioned to IRIS_INST_ERROR. When userspace attempted
>     to stop streaming for cleanup, stop_streaming was skipped due to the
>     added check, preventing proper teardown and leaving the firmware
>     in an inconsistent state.
> 
> Fixes: ad699fa78b59 ("media: iris: Add sanity check for stop streaming")
> Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
> ---
>   drivers/media/platform/qcom/iris/iris_vb2.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c b/drivers/media/platform/qcom/iris/iris_vb2.c
> index db8768d8a8f61c9ceb04e423d0a769d35114e20e..139b821f7952feb33b21a7045aef9e8a4782aa3c 100644
> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
> @@ -231,8 +231,6 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>   		return;
>   
>   	mutex_lock(&inst->lock);
> -	if (inst->state == IRIS_INST_ERROR)
> -		goto exit;
>   
>   	if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
>   	    !V4L2_TYPE_IS_CAPTURE(q->type))
> @@ -243,10 +241,10 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>   		goto exit;
>   
>   exit:
> -	if (ret) {
> -		iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
> +	iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
> +	if (ret)
>   		iris_inst_change_state(inst, IRIS_INST_ERROR);
> -	}
> +
>   	mutex_unlock(&inst->lock);
>   }
>   
> 

This revert looks strange, should be something like:

commit 9b6b11d31918722b4522b8982141d7b9646c0e48 (HEAD -> next-6.19-camss-v2)
Author: Bryan O'Donoghue <bod@kernel.org>
Date:   Tue Dec 30 10:20:01 2025 +0000

     Revert "media: iris: Add sanity check for stop streaming"

     This reverts commit ad699fa78b59241c9d71a8cafb51525f3dab04d4.

     Everything is broken I give up.

     Signed-off-by: Bryan O'Donoghue <bod@kernel.org>

diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c 
b/drivers/media/platform/qcom/iris/iris_vb2.c
index db8768d8a8f61..139b821f7952f 100644
--- a/drivers/media/platform/qcom/iris/iris_vb2.c
+++ b/drivers/media/platform/qcom/iris/iris_vb2.c
@@ -231,8 +231,6 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
                 return;

         mutex_lock(&inst->lock);
-       if (inst->state == IRIS_INST_ERROR)
-               goto exit;

         if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
             !V4L2_TYPE_IS_CAPTURE(q->type))
@@ -243,10 +241,10 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
                 goto exit;

  exit:
-       if (ret) {
-               iris_helper_buffers_done(inst, q->type, 
VB2_BUF_STATE_ERROR);
+       iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
+       if (ret)
                 iris_inst_change_state(inst, IRIS_INST_ERROR);
-       }
+
         mutex_unlock(&inst->lock);
  }

Just `git revert ad699fa78b59241c9d71a8cafb51525f3dab04d4` and add your 
commit log ?!

---
bod

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 5/6] media: iris: gen2: Add sanity check for session stop
  2025-12-29  6:31 ` [PATCH v2 5/6] media: iris: gen2: Add sanity check for session stop Dikshita Agarwal
@ 2025-12-30 10:33   ` Bryan O'Donoghue
  2026-01-07 10:06     ` Dikshita Agarwal
  2026-01-12 11:09   ` Vikash Garodia
  1 sibling, 1 reply; 19+ messages in thread
From: Bryan O'Donoghue @ 2025-12-30 10:33 UTC (permalink / raw)
  To: Dikshita Agarwal, Vikash Garodia, Abhinav Kumar,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue

On 29/12/2025 06:31, Dikshita Agarwal wrote:
> In iris_kill_session, inst->state is set to IRIS_INST_ERROR and
> session_close is executed, which will kfree(inst_hfi_gen2->packet).
> If stop_streaming is called afterward, it will cause a crash.
> 
> Add a NULL check for inst_hfi_gen2->packet before sendling STOP packet
> to firmware to fix that.
> 
> Fixes: 11712ce70f8e ("media: iris: implement vb2 streaming ops")
> Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
> ---
>   drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> 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 6a772db2ec33fb002d8884753a41dc98b3a8439d..59e41adcce9aadd7c60bb1d369d68a4954f62aef 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> @@ -963,6 +963,9 @@ static int iris_hfi_gen2_session_stop(struct iris_inst *inst, u32 plane)
>   	struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst);
>   	int ret = 0;
> 
> +	if (!inst_hfi_gen2->packet)
> +		return -EINVAL;
> +
>   	reinit_completion(&inst->completion);
> 
>   	iris_hfi_gen2_packet_session_command(inst,
> 
> --
> 2.34.1
> 

Are you sure this NULL check is concurrency safe ?

i.e. that ->session_stop() and ->session_close() cannot be executed 
concurrently ?

---
bod

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 4/6] Revert "media: iris: Add sanity check for stop streaming"
  2025-12-30 10:25   ` Bryan O'Donoghue
@ 2026-01-05  9:51     ` Dikshita Agarwal
  2026-01-05 12:50       ` Bryan O'Donoghue
  0 siblings, 1 reply; 19+ messages in thread
From: Dikshita Agarwal @ 2026-01-05  9:51 UTC (permalink / raw)
  To: Bryan O'Donoghue, Vikash Garodia, Abhinav Kumar,
	Bryan O'Donoghue, Mauro Carvalho Chehab, Hans Verkuil,
	Stefan Schmidt, Hans Verkuil, Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel



On 12/30/2025 3:55 PM, Bryan O'Donoghue wrote:
> On 29/12/2025 06:31, Dikshita Agarwal wrote:
>> Revert the check that skipped stop_streaming when the instance was in
>> IRIS_INST_ERROR, as it caused multiple regressions:
>>
>> 1. Buffers were not returned to vb2 when the instance was already in
>>     error state, triggering warnings in the vb2 core because buffer
>>     completion was skipped.
>>
>> 2. If a session failed early (e.g. unsupported configuration), the
>>     instance transitioned to IRIS_INST_ERROR. When userspace attempted
>>     to stop streaming for cleanup, stop_streaming was skipped due to the
>>     added check, preventing proper teardown and leaving the firmware
>>     in an inconsistent state.
>>
>> Fixes: ad699fa78b59 ("media: iris: Add sanity check for stop streaming")
>> Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
>> ---
>>   drivers/media/platform/qcom/iris/iris_vb2.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c
>> b/drivers/media/platform/qcom/iris/iris_vb2.c
>> index
>> db8768d8a8f61c9ceb04e423d0a769d35114e20e..139b821f7952feb33b21a7045aef9e8a4782aa3c 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
>> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
>> @@ -231,8 +231,6 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>>           return;
>>         mutex_lock(&inst->lock);
>> -    if (inst->state == IRIS_INST_ERROR)
>> -        goto exit;
>>         if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
>>           !V4L2_TYPE_IS_CAPTURE(q->type))
>> @@ -243,10 +241,10 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>>           goto exit;
>>     exit:
>> -    if (ret) {
>> -        iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
>> +    iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
>> +    if (ret)
>>           iris_inst_change_state(inst, IRIS_INST_ERROR);
>> -    }
>> +
>>       mutex_unlock(&inst->lock);
>>   }
>>  
> 
> This revert looks strange, should be something like:
> 
> commit 9b6b11d31918722b4522b8982141d7b9646c0e48 (HEAD -> next-6.19-camss-v2)
> Author: Bryan O'Donoghue <bod@kernel.org>
> Date:   Tue Dec 30 10:20:01 2025 +0000
> 
>     Revert "media: iris: Add sanity check for stop streaming"
> 
>     This reverts commit ad699fa78b59241c9d71a8cafb51525f3dab04d4.
> 
>     Everything is broken I give up.
> 
>     Signed-off-by: Bryan O'Donoghue <bod@kernel.org>
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c
> b/drivers/media/platform/qcom/iris/iris_vb2.c
> index db8768d8a8f61..139b821f7952f 100644
> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
> @@ -231,8 +231,6 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>                 return;
> 
>         mutex_lock(&inst->lock);
> -       if (inst->state == IRIS_INST_ERROR)
> -               goto exit;
> 
>         if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
>             !V4L2_TYPE_IS_CAPTURE(q->type))
> @@ -243,10 +241,10 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>                 goto exit;
> 
>  exit:
> -       if (ret) {
> -               iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
> +       iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
> +       if (ret)
>                 iris_inst_change_state(inst, IRIS_INST_ERROR);
> -       }
> +
>         mutex_unlock(&inst->lock);
>  }
> 
> Just `git revert ad699fa78b59241c9d71a8cafb51525f3dab04d4` and add your
> commit log ?!

Yeah I did the same, revert and changed the commit message.

BTW, I don't see any difference in my change and your commit, anything I am
missing here?

Thanks,
Dikshita

> 
> ---
> bod

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 4/6] Revert "media: iris: Add sanity check for stop streaming"
  2026-01-05  9:51     ` Dikshita Agarwal
@ 2026-01-05 12:50       ` Bryan O'Donoghue
  2026-01-06  5:47         ` Dikshita Agarwal
  0 siblings, 1 reply; 19+ messages in thread
From: Bryan O'Donoghue @ 2026-01-05 12:50 UTC (permalink / raw)
  To: Dikshita Agarwal, Vikash Garodia, Abhinav Kumar,
	Bryan O'Donoghue, Mauro Carvalho Chehab, Hans Verkuil,
	Stefan Schmidt, Hans Verkuil, Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel

On 05/01/2026 09:51, Dikshita Agarwal wrote:
> 
> 
> On 12/30/2025 3:55 PM, Bryan O'Donoghue wrote:
>> On 29/12/2025 06:31, Dikshita Agarwal wrote:
>>> Revert the check that skipped stop_streaming when the instance was in
>>> IRIS_INST_ERROR, as it caused multiple regressions:
>>>
>>> 1. Buffers were not returned to vb2 when the instance was already in
>>>      error state, triggering warnings in the vb2 core because buffer
>>>      completion was skipped.
>>>
>>> 2. If a session failed early (e.g. unsupported configuration), the
>>>      instance transitioned to IRIS_INST_ERROR. When userspace attempted
>>>      to stop streaming for cleanup, stop_streaming was skipped due to the
>>>      added check, preventing proper teardown and leaving the firmware
>>>      in an inconsistent state.
>>>
>>> Fixes: ad699fa78b59 ("media: iris: Add sanity check for stop streaming")
>>> Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
>>> ---
>>>    drivers/media/platform/qcom/iris/iris_vb2.c | 8 +++-----
>>>    1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c
>>> b/drivers/media/platform/qcom/iris/iris_vb2.c
>>> index
>>> db8768d8a8f61c9ceb04e423d0a769d35114e20e..139b821f7952feb33b21a7045aef9e8a4782aa3c 100644
>>> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
>>> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
>>> @@ -231,8 +231,6 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>>>            return;
>>>          mutex_lock(&inst->lock);
>>> -    if (inst->state == IRIS_INST_ERROR)
>>> -        goto exit;
>>>          if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
>>>            !V4L2_TYPE_IS_CAPTURE(q->type))
>>> @@ -243,10 +241,10 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>>>            goto exit;
>>>      exit:
>>> -    if (ret) {
>>> -        iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
>>> +    iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
>>> +    if (ret)
>>>            iris_inst_change_state(inst, IRIS_INST_ERROR);
>>> -    }
>>> +
>>>        mutex_unlock(&inst->lock);
>>>    }
>>>   
>>
>> This revert looks strange, should be something like:
>>
>> commit 9b6b11d31918722b4522b8982141d7b9646c0e48 (HEAD -> next-6.19-camss-v2)
>> Author: Bryan O'Donoghue <bod@kernel.org>
>> Date:   Tue Dec 30 10:20:01 2025 +0000
>>
>>      Revert "media: iris: Add sanity check for stop streaming"
>>
>>      This reverts commit ad699fa78b59241c9d71a8cafb51525f3dab04d4.
>>
>>      Everything is broken I give up.
>>
>>      Signed-off-by: Bryan O'Donoghue <bod@kernel.org>
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c
>> b/drivers/media/platform/qcom/iris/iris_vb2.c
>> index db8768d8a8f61..139b821f7952f 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
>> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
>> @@ -231,8 +231,6 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>>                  return;
>>
>>          mutex_lock(&inst->lock);
>> -       if (inst->state == IRIS_INST_ERROR)
>> -               goto exit;
>>
>>          if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
>>              !V4L2_TYPE_IS_CAPTURE(q->type))
>> @@ -243,10 +241,10 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>>                  goto exit;
>>
>>   exit:
>> -       if (ret) {
>> -               iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
>> +       iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
>> +       if (ret)
>>                  iris_inst_change_state(inst, IRIS_INST_ERROR);
>> -       }
>> +
>>          mutex_unlock(&inst->lock);
>>   }
>>
>> Just `git revert ad699fa78b59241c9d71a8cafb51525f3dab04d4` and add your
>> commit log ?!
> 
> Yeah I did the same, revert and changed the commit message.
> 
> BTW, I don't see any difference in my change and your commit, anything I am
> missing here?

Take this example, I believe the "This reverts commit xxx" is added by 
the revert command and its best practice to include it.

commit afb9917d9b374ecb77d478c2a052e20875c6e232
Author: Christian Brauner <brauner@kernel.org>
Date:   Fri Dec 5 13:50:31 2025 +0100

     Revert "net/socket: convert sock_map_fd() to FD_ADD()"

     This reverts commit 245f0d1c622b0183ce4f44b3e39aeacf78fae594.

I can just add it back in with the PR though its NBD.

---
bod

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 4/6] Revert "media: iris: Add sanity check for stop streaming"
  2026-01-05 12:50       ` Bryan O'Donoghue
@ 2026-01-06  5:47         ` Dikshita Agarwal
  0 siblings, 0 replies; 19+ messages in thread
From: Dikshita Agarwal @ 2026-01-06  5:47 UTC (permalink / raw)
  To: Bryan O'Donoghue, Vikash Garodia, Abhinav Kumar,
	Bryan O'Donoghue, Mauro Carvalho Chehab, Hans Verkuil,
	Stefan Schmidt, Hans Verkuil, Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel



On 1/5/2026 6:20 PM, Bryan O'Donoghue wrote:
> On 05/01/2026 09:51, Dikshita Agarwal wrote:
>>
>>
>> On 12/30/2025 3:55 PM, Bryan O'Donoghue wrote:
>>> On 29/12/2025 06:31, Dikshita Agarwal wrote:
>>>> Revert the check that skipped stop_streaming when the instance was in
>>>> IRIS_INST_ERROR, as it caused multiple regressions:
>>>>
>>>> 1. Buffers were not returned to vb2 when the instance was already in
>>>>      error state, triggering warnings in the vb2 core because buffer
>>>>      completion was skipped.
>>>>
>>>> 2. If a session failed early (e.g. unsupported configuration), the
>>>>      instance transitioned to IRIS_INST_ERROR. When userspace attempted
>>>>      to stop streaming for cleanup, stop_streaming was skipped due to the
>>>>      added check, preventing proper teardown and leaving the firmware
>>>>      in an inconsistent state.
>>>>
>>>> Fixes: ad699fa78b59 ("media: iris: Add sanity check for stop streaming")
>>>> Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
>>>> ---
>>>>    drivers/media/platform/qcom/iris/iris_vb2.c | 8 +++-----
>>>>    1 file changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c
>>>> b/drivers/media/platform/qcom/iris/iris_vb2.c
>>>> index
>>>> db8768d8a8f61c9ceb04e423d0a769d35114e20e..139b821f7952feb33b21a7045aef9e8a4782aa3c 100644
>>>> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
>>>> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
>>>> @@ -231,8 +231,6 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>>>>            return;
>>>>          mutex_lock(&inst->lock);
>>>> -    if (inst->state == IRIS_INST_ERROR)
>>>> -        goto exit;
>>>>          if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
>>>>            !V4L2_TYPE_IS_CAPTURE(q->type))
>>>> @@ -243,10 +241,10 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>>>>            goto exit;
>>>>      exit:
>>>> -    if (ret) {
>>>> -        iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
>>>> +    iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
>>>> +    if (ret)
>>>>            iris_inst_change_state(inst, IRIS_INST_ERROR);
>>>> -    }
>>>> +
>>>>        mutex_unlock(&inst->lock);
>>>>    }
>>>>   
>>>
>>> This revert looks strange, should be something like:
>>>
>>> commit 9b6b11d31918722b4522b8982141d7b9646c0e48 (HEAD ->
>>> next-6.19-camss-v2)
>>> Author: Bryan O'Donoghue <bod@kernel.org>
>>> Date:   Tue Dec 30 10:20:01 2025 +0000
>>>
>>>      Revert "media: iris: Add sanity check for stop streaming"
>>>
>>>      This reverts commit ad699fa78b59241c9d71a8cafb51525f3dab04d4.
>>>
>>>      Everything is broken I give up.
>>>
>>>      Signed-off-by: Bryan O'Donoghue <bod@kernel.org>
>>>
>>> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c
>>> b/drivers/media/platform/qcom/iris/iris_vb2.c
>>> index db8768d8a8f61..139b821f7952f 100644
>>> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
>>> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
>>> @@ -231,8 +231,6 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>>>                  return;
>>>
>>>          mutex_lock(&inst->lock);
>>> -       if (inst->state == IRIS_INST_ERROR)
>>> -               goto exit;
>>>
>>>          if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
>>>              !V4L2_TYPE_IS_CAPTURE(q->type))
>>> @@ -243,10 +241,10 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>>>                  goto exit;
>>>
>>>   exit:
>>> -       if (ret) {
>>> -               iris_helper_buffers_done(inst, q->type,
>>> VB2_BUF_STATE_ERROR);
>>> +       iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
>>> +       if (ret)
>>>                  iris_inst_change_state(inst, IRIS_INST_ERROR);
>>> -       }
>>> +
>>>          mutex_unlock(&inst->lock);
>>>   }
>>>
>>> Just `git revert ad699fa78b59241c9d71a8cafb51525f3dab04d4` and add your
>>> commit log ?!
>>
>> Yeah I did the same, revert and changed the commit message.
>>
>> BTW, I don't see any difference in my change and your commit, anything I am
>> missing here?
> 
> Take this example, I believe the "This reverts commit xxx" is added by the
> revert command and its best practice to include it.
> 
> commit afb9917d9b374ecb77d478c2a052e20875c6e232
> Author: Christian Brauner <brauner@kernel.org>
> Date:   Fri Dec 5 13:50:31 2025 +0100
> 
>     Revert "net/socket: convert sock_map_fd() to FD_ADD()"
> 
>     This reverts commit 245f0d1c622b0183ce4f44b3e39aeacf78fae594.
> 
> I can just add it back in with the PR though its NBD.

Ah ok, got it. Thanks!

> 
> ---
> bod

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 5/6] media: iris: gen2: Add sanity check for session stop
  2025-12-30 10:33   ` Bryan O'Donoghue
@ 2026-01-07 10:06     ` Dikshita Agarwal
  0 siblings, 0 replies; 19+ messages in thread
From: Dikshita Agarwal @ 2026-01-07 10:06 UTC (permalink / raw)
  To: Bryan O'Donoghue, Vikash Garodia, Abhinav Kumar,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue



On 12/30/2025 4:03 PM, Bryan O'Donoghue wrote:
> On 29/12/2025 06:31, Dikshita Agarwal wrote:
>> In iris_kill_session, inst->state is set to IRIS_INST_ERROR and
>> session_close is executed, which will kfree(inst_hfi_gen2->packet).
>> If stop_streaming is called afterward, it will cause a crash.
>>
>> Add a NULL check for inst_hfi_gen2->packet before sendling STOP packet
>> to firmware to fix that.
>>
>> Fixes: 11712ce70f8e ("media: iris: implement vb2 streaming ops")
>> Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
>> ---
>>   drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> 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
>> 6a772db2ec33fb002d8884753a41dc98b3a8439d..59e41adcce9aadd7c60bb1d369d68a4954f62aef 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
>> @@ -963,6 +963,9 @@ static int iris_hfi_gen2_session_stop(struct
>> iris_inst *inst, u32 plane)
>>       struct iris_inst_hfi_gen2 *inst_hfi_gen2 =
>> to_iris_inst_hfi_gen2(inst);
>>       int ret = 0;
>>
>> +    if (!inst_hfi_gen2->packet)
>> +        return -EINVAL;
>> +
>>       reinit_completion(&inst->completion);
>>
>>       iris_hfi_gen2_packet_session_command(inst,
>>
>> -- 
>> 2.34.1
>>
> 
> Are you sure this NULL check is concurrency safe ?
> 
> i.e. that ->session_stop() and ->session_close() cannot be executed
> concurrently ?

Yes, this is concurrency safe.

Both iris_hfi_gen2_session_stop() and iris_kill_session() are invoked while
holding inst->lock. For instance, iris_vb2_stop_streaming() locks the mutex
before calling session_stop(), and iris_kill_session() / session_close()
also grab the same lock before freeing inst_hfi_gen2->packet.

That ensures they never run concurrently, so the NULL-check protects
against the packet having been freed earlier without racing.

Thanks,
Dikshita

> 
> ---
> bod

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/6] media: iris: Add buffer to list only after successful allocation
  2025-12-29  6:31 ` [PATCH v2 1/6] media: iris: Add buffer to list only after successful allocation Dikshita Agarwal
@ 2026-01-07 10:26   ` Vikash Garodia
  0 siblings, 0 replies; 19+ messages in thread
From: Vikash Garodia @ 2026-01-07 10:26 UTC (permalink / raw)
  To: Dikshita Agarwal, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue


On 12/29/2025 12:01 PM, Dikshita Agarwal wrote:
> Move `list_add_tail()` to after `dma_alloc_attrs()` succeeds when creating
> internal buffers. Previously, the buffer was enqueued in `buffers->list`
> before the DMA allocation. If the allocation failed, the function returned
> `-ENOMEM` while leaving a partially initialized buffer in the list, which
> could lead to inconsistent state and potential leaks.
> 
> By adding the buffer to the list only after `dma_alloc_attrs()` succeeds,
> we ensure the list contains only valid, fully initialized buffers.
> 
> Fixes: 73702f45db81 ("media: iris: allocate, initialize and queue internal buffers")
> Reviewed-by: Bryan O'Donoghue <bod@kernel.org>
> Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
> ---
>   drivers/media/platform/qcom/iris/iris_buffer.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c
> index b89b1ee06cce151e7c04a80956380d154643c116..f1f003a787bf22db6f048c9e682ba8ed2f39bc21 100644
> --- a/drivers/media/platform/qcom/iris/iris_buffer.c
> +++ b/drivers/media/platform/qcom/iris/iris_buffer.c
> @@ -351,12 +351,15 @@ static int iris_create_internal_buffer(struct iris_inst *inst,
>   	buffer->index = index;
>   	buffer->buffer_size = buffers->size;
>   	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,
>   					 &buffer->device_addr, GFP_KERNEL, buffer->dma_attrs);
> -	if (!buffer->kvaddr)
> +	if (!buffer->kvaddr) {
> +		kfree(buffer);
>   		return -ENOMEM;
> +	}
> +
> +	list_add_tail(&buffer->list, &buffers->list);
>   
>   	return 0;
>   }
> 

Reviewed-by: Vikash Garodia<vikash.garodia@oss.qualcomm.com>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/6] media: iris: Skip resolution set on first IPSC
  2025-12-29  6:31 ` [PATCH v2 2/6] media: iris: Skip resolution set on first IPSC Dikshita Agarwal
@ 2026-01-07 10:27   ` Vikash Garodia
  0 siblings, 0 replies; 19+ messages in thread
From: Vikash Garodia @ 2026-01-07 10:27 UTC (permalink / raw)
  To: Dikshita Agarwal, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue


On 12/29/2025 12:01 PM, Dikshita Agarwal wrote:
> The resolution property is not supposed to be set during reconfig.
> Existing iris_drc_pending(inst) check is insufficient, as it doesn't
> cover the first port setting change.
> 
> Extend the conditional check to also skip resolution setting when
> the instance is in IRIS_INST_SUB_FIRST_IPSC.
> 
> Fixes: caf205548769 ("media: iris: Avoid updating frame size to firmware during reconfig")
> Reviewed-by: Bryan O'Donoghue <bod@kernel.org>
> Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
> ---
>   drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> index 52da7ef7bab08fb1cb2ac804ccc6e3c7f9677890..5087e51daa842515e9d62730680fb237bf274efa 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> @@ -733,7 +733,7 @@ static int iris_hfi_gen1_set_resolution(struct iris_inst *inst, u32 plane)
>   	struct hfi_framesize fs;
>   	int ret;
>   
> -	if (!iris_drc_pending(inst)) {
> +	if (!iris_drc_pending(inst) && !(inst->sub_state & IRIS_INST_SUB_FIRST_IPSC)) {
>   		fs.buffer_type = HFI_BUFFER_INPUT;
>   		fs.width = inst->fmt_src->fmt.pix_mp.width;
>   		fs.height = inst->fmt_src->fmt.pix_mp.height;
> 

Reviewed-by: Vikash Garodia<vikash.garodia@oss.qualcomm.com>


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 3/6] media: iris: gen1: Destroy internal buffers after FW releases
  2025-12-29  6:31 ` [PATCH v2 3/6] media: iris: gen1: Destroy internal buffers after FW releases Dikshita Agarwal
@ 2026-01-07 10:27   ` Vikash Garodia
  0 siblings, 0 replies; 19+ messages in thread
From: Vikash Garodia @ 2026-01-07 10:27 UTC (permalink / raw)
  To: Dikshita Agarwal, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue


On 12/29/2025 12:01 PM, Dikshita Agarwal wrote:
> After the firmware releases internal buffers, the driver was not
> destroying them. This left stale allocations that were no longer used,
> especially across resolution changes where new buffers are allocated per
> the updated requirements. As a result, memory was wasted until session
> close.
> 
> Destroy internal buffers once the release response is received from the
> firmware.
> 
> Fixes: 73702f45db81 ("media: iris: allocate, initialize and queue internal buffers")
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
> ---
>   drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> index 5087e51daa842515e9d62730680fb237bf274efa..5ff71e25597b61587c674142feb99626e402c893 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> @@ -441,6 +441,8 @@ static int iris_hfi_gen1_session_unset_buffers(struct iris_inst *inst, struct ir
>   		goto exit;
>   
>   	ret = iris_wait_for_session_response(inst, false);
> +	if (!ret)
> +		ret = iris_destroy_internal_buffer(inst, buf);
>   
>   exit:
>   	kfree(pkt);
> 

Reviewed-by: Vikash Garodia<vikash.garodia@oss.qualcomm.com>


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 4/6] Revert "media: iris: Add sanity check for stop streaming"
  2025-12-29  6:31 ` [PATCH v2 4/6] Revert "media: iris: Add sanity check for stop streaming" Dikshita Agarwal
  2025-12-30 10:25   ` Bryan O'Donoghue
@ 2026-01-07 10:29   ` Vikash Garodia
  1 sibling, 0 replies; 19+ messages in thread
From: Vikash Garodia @ 2026-01-07 10:29 UTC (permalink / raw)
  To: Dikshita Agarwal, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue


On 12/29/2025 12:01 PM, Dikshita Agarwal wrote:
> Revert the check that skipped stop_streaming when the instance was in
> IRIS_INST_ERROR, as it caused multiple regressions:
> 
> 1. Buffers were not returned to vb2 when the instance was already in
>     error state, triggering warnings in the vb2 core because buffer
>     completion was skipped.
> 
> 2. If a session failed early (e.g. unsupported configuration), the
>     instance transitioned to IRIS_INST_ERROR. When userspace attempted
>     to stop streaming for cleanup, stop_streaming was skipped due to the
>     added check, preventing proper teardown and leaving the firmware
>     in an inconsistent state.
> 
> Fixes: ad699fa78b59 ("media: iris: Add sanity check for stop streaming")
> Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
> ---
>   drivers/media/platform/qcom/iris/iris_vb2.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c b/drivers/media/platform/qcom/iris/iris_vb2.c
> index db8768d8a8f61c9ceb04e423d0a769d35114e20e..139b821f7952feb33b21a7045aef9e8a4782aa3c 100644
> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
> @@ -231,8 +231,6 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>   		return;
>   
>   	mutex_lock(&inst->lock);
> -	if (inst->state == IRIS_INST_ERROR)
> -		goto exit;
>   
>   	if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
>   	    !V4L2_TYPE_IS_CAPTURE(q->type))
> @@ -243,10 +241,10 @@ void iris_vb2_stop_streaming(struct vb2_queue *q)
>   		goto exit;
>   
>   exit:
> -	if (ret) {
> -		iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
> +	iris_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
> +	if (ret)
>   		iris_inst_change_state(inst, IRIS_INST_ERROR);
> -	}
> +
>   	mutex_unlock(&inst->lock);
>   }
>   
> 

Reviewed-by: Vikash Garodia<vikash.garodia@oss.qualcomm.com>



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 5/6] media: iris: gen2: Add sanity check for session stop
  2025-12-29  6:31 ` [PATCH v2 5/6] media: iris: gen2: Add sanity check for session stop Dikshita Agarwal
  2025-12-30 10:33   ` Bryan O'Donoghue
@ 2026-01-12 11:09   ` Vikash Garodia
  1 sibling, 0 replies; 19+ messages in thread
From: Vikash Garodia @ 2026-01-12 11:09 UTC (permalink / raw)
  To: Dikshita Agarwal, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue


On 12/29/2025 12:01 PM, Dikshita Agarwal wrote:
> In iris_kill_session, inst->state is set to IRIS_INST_ERROR and
> session_close is executed, which will kfree(inst_hfi_gen2->packet).
> If stop_streaming is called afterward, it will cause a crash.
> 
> Add a NULL check for inst_hfi_gen2->packet before sendling STOP packet
> to firmware to fix that.
> 
> Fixes: 11712ce70f8e ("media: iris: implement vb2 streaming ops")
> Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
> ---
>   drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> 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 6a772db2ec33fb002d8884753a41dc98b3a8439d..59e41adcce9aadd7c60bb1d369d68a4954f62aef 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> @@ -963,6 +963,9 @@ static int iris_hfi_gen2_session_stop(struct iris_inst *inst, u32 plane)
>   	struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst);
>   	int ret = 0;
>   
> +	if (!inst_hfi_gen2->packet)
> +		return -EINVAL;
> +
>   	reinit_completion(&inst->completion);
>   
>   	iris_hfi_gen2_packet_session_command(inst,
> 

Reviewed-by: Vikash Garodia<vikash.garodia@oss.qualcomm.com>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/6] media: iris: Prevent output buffer queuing before stream-on completes
  2025-12-29  6:31 ` [PATCH v2 6/6] media: iris: Prevent output buffer queuing before stream-on completes Dikshita Agarwal
@ 2026-01-12 11:09   ` Vikash Garodia
  0 siblings, 0 replies; 19+ messages in thread
From: Vikash Garodia @ 2026-01-12 11:09 UTC (permalink / raw)
  To: Dikshita Agarwal, Abhinav Kumar, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Hans Verkuil, Stefan Schmidt, Hans Verkuil,
	Wangao Wang
  Cc: linux-media, linux-arm-msm, linux-kernel, Bryan O'Donoghue,
	Vishnu Reddy


On 12/29/2025 12:01 PM, Dikshita Agarwal wrote:
> From: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
> 
> During normal playback, stream-on for input is followed by output, and
> only after input stream-on does actual streaming begin. However, when
> gst-play performs a seek, both input and output streams are stopped,
> and on restart, output stream-on occurs first. At this point, firmware
> has not yet started streaming. Queuing output buffers before the firmware
> begins streaming causes it to process buffers in an invalid state, leading
> to an error response. These buffers are returned to the driver as errors,
> forcing the driver into an error state and stopping playback.
> 
> Fix this by deferring output buffer queuing until stream-on completes.
> Input buffers can still be queued before stream-on as required.
> 
> Fixes: 92e007ca5ab6 ("media: iris: Add V4L2 streaming support for encoder video device")
> Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
> Signed-off-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
> ---
>   drivers/media/platform/qcom/iris/iris_vb2.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c b/drivers/media/platform/qcom/iris/iris_vb2.c
> index 139b821f7952feb33b21a7045aef9e8a4782aa3c..bf0b8400996ece5c9d449b99609a302da726bf9a 100644
> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
> @@ -193,10 +193,14 @@ int iris_vb2_start_streaming(struct vb2_queue *q, unsigned int count)
>   	buf_type = iris_v4l2_type_to_driver(q->type);
>   
>   	if (inst->domain == DECODER) {
> -		if (inst->state == IRIS_INST_STREAMING)
> +		if (buf_type == BUF_INPUT)
> +			ret = iris_queue_deferred_buffers(inst, BUF_INPUT);
> +
> +		if (!ret && inst->state == IRIS_INST_STREAMING) {
>   			ret = iris_queue_internal_deferred_buffers(inst, BUF_DPB);
> -		if (!ret)
> -			ret = iris_queue_deferred_buffers(inst, buf_type);
> +			if (!ret)
> +				ret = iris_queue_deferred_buffers(inst, BUF_OUTPUT);
> +		}
>   	} else {
>   		if (inst->state == IRIS_INST_STREAMING) {
>   			ret = iris_queue_deferred_buffers(inst, BUF_INPUT);
> 

Reviewed-by: Vikash Garodia<vikash.garodia@oss.qualcomm.com>

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2026-01-12 11:09 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-29  6:31 [PATCH v2 0/6] media: iris: misc fixes for fluster, seek and concurrency issues Dikshita Agarwal
2025-12-29  6:31 ` [PATCH v2 1/6] media: iris: Add buffer to list only after successful allocation Dikshita Agarwal
2026-01-07 10:26   ` Vikash Garodia
2025-12-29  6:31 ` [PATCH v2 2/6] media: iris: Skip resolution set on first IPSC Dikshita Agarwal
2026-01-07 10:27   ` Vikash Garodia
2025-12-29  6:31 ` [PATCH v2 3/6] media: iris: gen1: Destroy internal buffers after FW releases Dikshita Agarwal
2026-01-07 10:27   ` Vikash Garodia
2025-12-29  6:31 ` [PATCH v2 4/6] Revert "media: iris: Add sanity check for stop streaming" Dikshita Agarwal
2025-12-30 10:25   ` Bryan O'Donoghue
2026-01-05  9:51     ` Dikshita Agarwal
2026-01-05 12:50       ` Bryan O'Donoghue
2026-01-06  5:47         ` Dikshita Agarwal
2026-01-07 10:29   ` Vikash Garodia
2025-12-29  6:31 ` [PATCH v2 5/6] media: iris: gen2: Add sanity check for session stop Dikshita Agarwal
2025-12-30 10:33   ` Bryan O'Donoghue
2026-01-07 10:06     ` Dikshita Agarwal
2026-01-12 11:09   ` Vikash Garodia
2025-12-29  6:31 ` [PATCH v2 6/6] media: iris: Prevent output buffer queuing before stream-on completes Dikshita Agarwal
2026-01-12 11:09   ` Vikash Garodia

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox