Re: [PATCH v5 11/14] staging: media: starfive: Add ISP params video device

[Hans Verkuil]

On 10/07/2024 15:07, Jacopo Mondi wrote:
> Hi Changhuang
> 
>    + Hans for one question on the vb2 queue mem_ops to use.
> 
> On Tue, Jul 09, 2024 at 01:38:21AM GMT, Changhuang Liang wrote:
>> Add ISP params video device to write ISP parameters for 3A.
>>
>> Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
>> ---
>>  drivers/staging/media/starfive/camss/Makefile |   2 +
>>  .../staging/media/starfive/camss/stf-camss.c  |  23 +-
>>  .../staging/media/starfive/camss/stf-camss.h  |   3 +
>>  .../media/starfive/camss/stf-isp-params.c     | 240 ++++++++++++++++++
>>  .../staging/media/starfive/camss/stf-isp.h    |   4 +
>>  .../staging/media/starfive/camss/stf-output.c |  83 ++++++
>>  .../staging/media/starfive/camss/stf-output.h |  22 ++
>>  7 files changed, 376 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/staging/media/starfive/camss/stf-isp-params.c
>>  create mode 100644 drivers/staging/media/starfive/camss/stf-output.c
>>  create mode 100644 drivers/staging/media/starfive/camss/stf-output.h
>>

<snip>

>> +int stf_isp_params_register(struct stfcamss_video *video,
>> +			    struct v4l2_device *v4l2_dev,
>> +			    const char *name)
>> +{
>> +	struct video_device *vdev = &video->vdev;
>> +	struct vb2_queue *q;
>> +	struct media_pad *pad = &video->pad;
>> +	int ret;
>> +
>> +	mutex_init(&video->q_lock);
>> +	mutex_init(&video->lock);
> 
> are two mutexes required for the vb2 queue and the video node ? I see,
> in example, rkisp1-params.c uses a single one
> 
>> +
>> +	q = &video->vb2_q;
>> +	q->drv_priv = video;
>> +	q->mem_ops = &vb2_dma_contig_memops;
> 
> Now, I might be wrong, but unless you need to allocate memory from a
> DMA-capable area, you shouldn't need to use vb2_dma_contig_memops.
> 
> Looking at the next patches you apply configuration parameters to the
> ISP by inspecting the user supplied parameters one by one, not by
> transfering the whole parameters buffer to a memory area. Hans what do
> you think ?

Yes, if the data is not DMAed to the hardware, then use vb2_vmalloc_memops.

Regards,

	Hans