RE: [PATCH v4 1/2] v4l: Add memory-to-memory device helper framework for videobuf.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

thanks for the review. My responses below.


> Hiremath, Vaibhav <hvaibhav@xxxxxx> wrote:
>
>> -----Original Message-----
>> From: Pawel Osciak [mailto:p.osciak@xxxxxxxxxxx]
>> Sent: Monday, April 19, 2010 6:00 PM
>> To: linux-media@xxxxxxxxxxxxxxx
>> Cc: p.osciak@xxxxxxxxxxx; m.szyprowski@xxxxxxxxxxx;
>> kyungmin.park@xxxxxxxxxxx; Hiremath, Vaibhav
>> Subject: [PATCH v4 1/2] v4l: Add memory-to-memory device helper framework
>> for videobuf.


>[Hiremath, Vaibhav] Add one line here.
>

Ok...

[snip]

>> +/**
>> + * v4l2_m2m_querybuf() - multi-queue-aware QUERYBUF multiplexer
>> + *
>> + * See v4l2_m2m_mmap() documentation for details.
>> + */
>> +int v4l2_m2m_querybuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
>> +                   struct v4l2_buffer *buf)
>> +{
>> +     struct videobuf_queue *vq;
>> +     int ret;
>> +
>> +     vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
>> +     ret = videobuf_querybuf(vq, buf);
>> +
>> +     if (buf->memory == V4L2_MEMORY_MMAP
>> +         && vq->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>> +             buf->m.offset += DST_QUEUE_OFF_BASE;
>> +     }
>[Hiremath, Vaibhav] Don't you think we should check for ret value also here? Should it be something -
>
>if (!ret && buf->memory == V4L2_MEMORY_MMAP
>                        && vq->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>        buf->m.offset += DST_QUEUE_OFF_BASE;
>}
>

I think it should stay like this. The offset should never be different
depending on whether an error is being reported or not. The unmodified offset
could confuse userspace applications or even conflict with the other buffer
type (although in case of errors userspace should not be using those offsets
anyway).

[snip]

>> +/**
>> + * v4l2_m2m_dqbuf() - dequeue a source or destination buffer, depending on
>> + * the type
>> + */
>> +int v4l2_m2m_dqbuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
>> +                struct v4l2_buffer *buf)
>> +{
>> +     struct videobuf_queue *vq;
>> +
>> +     vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
>
>
>[Hiremath, Vaibhav] Does it make sense to check the return value here?
>

Well, videobuf does not check it either. I mean, it would be important if
there was a possibility that userspace passes malicious data. But a NULL
here would mean a driver error.

Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center


--
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

[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux