On Thu, Mar 28, 2019 at 10:59 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote: > > On 3/28/19 8:20 AM, Tomasz Figa wrote: > > <snip> > > >> +static int rockchip_vpu_buf_out_validate(struct vb2_buffer *vb) > >> +{ > >> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > >> + > >> + vbuf->field = V4L2_FIELD_NONE; > > > > Hmm, "validate" in the name of this callback would suggest that we > > should just check the contents, not change them. Hans, what was the > > intention when adding this callback? Are we missing the const > > specifier in the argument? > > See the original commit log: > > https://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg143163.html > > It is allowed to either just validate and return an error if wrong, > or change it to something known to be valid. In particular, userspace > can set this to FIELD_ANY, and in that case the driver must replace > it with something valid. > > Most drivers just support FIELD_NONE, and just set it. > > That said, I think we should tighten the spec for this as this is > not well documented. > > I propose that if vbuf->field == FIELD_ANY, then replace it with something > sane. Otherwise validate it and return an error if the field value > is not supported. > > And FIELD_ALTERNATE is never allowed here, that's always wrong. Thanks for clarifying. Makes sense. Best regards, Tomasz