On 23-09-15 02:56, Junghak Sung wrote:
>
>
> On 09/22/2015 10:44 PM, Hans Verkuil wrote:
>> Hi Junghak,
>>
>> On 22-09-15 15:30, Junghak Sung wrote:
>>> Replace struct v4l2_format * with vb2_format * to make queue_setup()
>>> for common use.
>>>
>>> struct vb2_format {
>>> unsigned int type;
>>> unsigned int pixelformat;
>>> unsigned int width;
>>> unsigned int height;
>>> unsigned int num_planes;
>>> unsigned int bytesperline[VIDEO_MAX_PLANES];
>>> unsigned int req_sizes[VIDEO_MAX_PLANES];
>>> };
>>
>> Why would you need all the other fields besides req_sizes[]?
>>
>> Which drivers actually need those other fields? Drivers like exynos4-is/fimc-lite.c
>> don't actually use anything but req_sizes if you read the code carefully.
>>
>> I suspect any driver that uses more than req_sizes is actually buggy or
>> written carelessly.
>>
>> I wish you'd checked with me before making this struct...
>>
>> Be aware that I'm abroad (vacation/conferences) from tomorrow until October 10,
>> so I won't be able to do in-depth reviews during that time (well, I'm able,
>> but I don't want to!)
>>
>> Regards,
>>
>> Hans
>
> Hi Hans,
>
> I added the member of struct vb2_format, if the member is used by any
> device driver.
> These are the usecases for each field besides req_sizes[].
>
> [platform/s3c-camif/camif-capture.c] pixelformat
> [platform/exynos4-is/fimc-capture.c] pixelformat, width, height
> [platform/exynos4-is/fimc-isp-video.c] pixelformat, width, height
> [platform/exynos4-is/fimc-lite.c] pixelformat, width, height
> [platform/soc_camera/mx3_camera.c] pixelformat, width, bytesperline,
> height
> [platform/soc_camera/rcar_vin.c] pixelformat, width, bytesperline,
> height
> [platform/soc_camera/sh_mobile_ceu_camera.c] pixelformat, width,
> bytesperline,
> height
> [platform/sh_veu.c] type, pixelformat, width, height, byteperline
> [platform/vivid/vivid-vid-cap.c] num_planes, pixelformat
> [platform/vivid/vivid-vid-out.c] num_planes
> [platform/vsp2/vsp1_video.c] width, height, pixelformat, byteperline,
> num_planes
>
> And I can not understand that fimc-lite.c do not actually use anything
> but req_sizes[]. In original source code, it seems that fimc_lite.c
> should use the other fields - pixelformat, width, height - if a user
> set the v4l2_format.
> Would you please explain a little bit more?
There are two different things going on here: validation of the format and
using the sizeimage value in queue_setup.
The queue_setup callback should only receive requested sizes, the format validation
should take place on the v4l2 side of create_buffers.
I'm OK if this is a bit hackish (I'm thinking a separate callback somewhere
where v4l2 drivers can do their format validation). I want to discuss this more
during the upcoming workshop. I always thought that it would be better if
the v4l2 core would call try_format first and leave the validation to that
function. But this needs to be discussed first.
Regards,
Hans
>
> Thank you for your review.
>
> Best regards,
> Junghak
>
>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
