On 09/11/2015 01:07 PM, Junghak Sung wrote:
>
> Hello Hans,
>
> First of all, thank you for your review.
>
>
> On 09/11/2015 05:52 PM, Hans Verkuil wrote:
>> On 09/09/2015 01:19 PM, Junghak Sung wrote:
>>> Replace struct v4l2_format * with void * to make queue_setup()
>>> for common use.
>>> And then, modify all device drivers related with this change.
>>>
>>> Signed-off-by: Junghak Sung <jh1009.sung@xxxxxxxxxxx>
>>> Signed-off-by: Geunyoung Kim <nenggun.kim@xxxxxxxxxxx>
>>> Acked-by: Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx>
>>> Acked-by: Inki Dae <inki.dae@xxxxxxxxxxx>
>>
>> OK, so I never liked this void * change. After I thought about it some
>> more I came to the conclusion that this should be changed.
>>
>> For probably all drivers all that they do with the fmt argument is to
>> check the sizeimage field. So I would replace the v4l2_format pointer
>> by an 'unsigned int *req_sizes' argument. This contains the requested
>> per-plane sizes. That is also nicely generic and makes much more sense
>> in videobuf2-core.c.
>>
>> A vb2_v4l2_create_bufs helper function will just call the internal
>> vb2-core function with the correct requested sizes. Note: these sizes
>> will depend on the type of v4l2_format, so the helper will have to do
>> a bit more work.
>>
>> If a driver needs to do more checking for the format, then it can do
>> that before calling the vb2_v4l2_create_bufs helper.
>>
>> When called from reqbufs the req_sizes argument will be NULL, just like
>> fmt is NULL today.
>>
>> I believe this is a much better solution.
>>
>> Regards,
>>
>> Hans
>>
>
> I'm afraid but it seems very hard to implement your idea.
> Some device drivers use not only the sizeimage field but also other
> fields.
> For example, type, fmt.pix_mp of v4l2_format are used by
> vid_out_queue_setup() function in vivid-vid-out.c
> And, fmt.vbi.samples_per_line and fmt.vbi.count are used by
> vbi_queue_setup() function in au0828-vbi.c.
If you look carefully in both cases these fields are used to determine
the size of each plane. So again, it is just the per-plane requested
size that is relevant here.
So the vb2_v4l2_create_bufs() helper function would look something like
this:
unsigned req_sizes[VB2_MAX_PLANES];
switch (fmt->type) {
case V4L2_BUF_TYPE_VIDEO_CAPTURE:
case V4L2_BUF_TYPE_VIDEO_OUTPUT:
req_sizes[0] = fmt->fmt.pix.sizeimage;
break;
case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
for (i = 0; i < fmt->fmt.pix_mp.length; i++)
req_sizes[i] = fmt->fmt.pix_mp.plane_fmt[i].sizeimage;
break;
case V4L2_BUF_TYPE_VBI_CAPTURE:
case V4L2_BUF_TYPE_VBI_OUTPUT:
req_sizes[0] = fmt->fmt.vbi.samples_per_line *
(fmt->fmt.vbi.count[0] + fmt->fmt.vbi.count[1]);
break;
// etc. For sliced vbi it is io_size, for sdr is it buffersize
}
vb2_core_create_bufs(...., req_sizes, ...)
> Even if you put that only these two example, it will be very
> hard to make vb2_v4l2_create_bufs helper function to use something
> common instead of v4l2_format, IMHO.
All queue_setup is interested in is the requested plane sizes obtained
from v4l2_format. So I don't think this is a problem at all.
Regards,
Hans
>
> 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
