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

Hans Verkuil
2024-07-15T07:12:10Z

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@starfivetech.com>
>> ---
>> 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