Re: [PATCH 2/2] media: rockchip: rkcif: comply with minimum number of buffers requirement

[Michael Riesch <michael.riesch@collabora.com>]

Hi Laurent,

On 2/19/26 10:13, Laurent Pinchart wrote:
> On Mon, Feb 16, 2026 at 02:49:57PM +0100, Michael Riesch via B4 Relay wrote:
>> From: Michael Riesch <michael.riesch@collabora.com>
>>
>> Each stream requires CIF_REQ_BUFS_MIN=1 buffers to enable streaming.
>> However, it failed with only one buffer provided.
>>
>> Comply with the minimum number of buffers requirement and accept
>> exactly one buffer.
>>
>> Fixes: 501802e2ad51 ("media: rockchip: rkcif: add abstraction for dma blocks")
>> Cc: stable@kernel.org
>> Signed-off-by: Michael Riesch <michael.riesch@collabora.com>
>> ---
>>  .../media/platform/rockchip/rkcif/rkcif-stream.c   | 41 +++++++++++-----------
>>  1 file changed, 21 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/media/platform/rockchip/rkcif/rkcif-stream.c b/drivers/media/platform/rockchip/rkcif/rkcif-stream.c
>> index e00010a91e8b..5a5ab9e7e86e 100644
>> --- a/drivers/media/platform/rockchip/rkcif/rkcif-stream.c
>> +++ b/drivers/media/platform/rockchip/rkcif/rkcif-stream.c
>> @@ -106,19 +106,6 @@ static int rkcif_stream_init_buffers(struct rkcif_stream *stream)
>>  {
>>  	struct v4l2_pix_format_mplane *pix = &stream->pix;
>>  
>> -	stream->buffers[0] = rkcif_stream_pop_buffer(stream);
>> -	if (!stream->buffers[0])
>> -		goto err_buff_0;
>> -
>> -	stream->buffers[1] = rkcif_stream_pop_buffer(stream);
>> -	if (!stream->buffers[1])
>> -		goto err_buff_1;
>> -
>> -	if (stream->queue_buffer) {
>> -		stream->queue_buffer(stream, 0);
>> -		stream->queue_buffer(stream, 1);
>> -	}
>> -
>>  	stream->dummy.size = pix->num_planes * pix->plane_fmt[0].sizeimage;
>>  	stream->dummy.vaddr =
>>  		dma_alloc_attrs(stream->rkcif->dev, stream->dummy.size,
>> @@ -132,16 +119,30 @@ static int rkcif_stream_init_buffers(struct rkcif_stream *stream)
>>  			stream->dummy.buffer.buff_addr[i - 1] +
>>  			pix->plane_fmt[i - 1].bytesperline * pix->height;
>>  
>> -	return 0;
>> +	stream->buffers[0] = rkcif_stream_pop_buffer(stream);
>> +	if (!stream->buffers[0])
>> +		goto err_buff_0;
> 
> Why do you move this after allocation of the dummy buffer, to then add
> dma_free_attrs() in the err_buff_0 error path ?

To keep the two rkcif_stream_pop_buffer calls together. We need to
allocate the dummy in any case, but in case the second pop fails we use
it -- this was not the case before.

> 
>>  
>> -err_dummy:
>> -	rkcif_stream_return_buffer(stream->buffers[1], VB2_BUF_STATE_QUEUED);
>> -	stream->buffers[1] = NULL;
>> +	stream->buffers[1] = rkcif_stream_pop_buffer(stream);
>> +	if (!stream->buffers[1]) {
>> +		stream->buffers[stream->frame_phase] = &stream->dummy.buffer;
>> +		stream->buffers[stream->frame_phase]->is_dummy = true;
>> +	}
>> +
>> +	if (stream->queue_buffer) {
>> +		stream->queue_buffer(stream, 0);
>> +		stream->queue_buffer(stream, 1);
>> +	}
>> +
>> +	return 0;
>>  
>> -err_buff_1:
>> -	rkcif_stream_return_buffer(stream->buffers[0], VB2_BUF_STATE_QUEUED);
>> -	stream->buffers[0] = NULL;
>>  err_buff_0:
>> +	dma_free_attrs(stream->rkcif->dev, stream->dummy.size,
>> +		       stream->dummy.vaddr,
>> +		       stream->dummy.buffer.buff_addr[0],
>> +		       DMA_ATTR_NO_KERNEL_MAPPING);
>> +	stream->dummy.vaddr = NULL;
>> +err_dummy:
>>  	return -EINVAL;
> 
> You can drop the err_dummy label and return -EINVAL directly. Except you
> should probably return -ENOMEM as the failure comes from
> dma_alloc_attrs().

Makes sense, will fix.

Best regards,
Michael

> 
>>  }
>>  
>