On Mon, Feb 5, 2018 at 11:34 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 02/06/2018 08:16 AM, Tim Harvey wrote:
>> On Sat, Feb 3, 2018 at 7:56 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
<snip>
>>
>> With regards to the 3 failures:
>>
>> 1. test VIDIOC_G/S_EDID: FAIL
>> This is a valid catch where I was returning -EINVAL when the caller
>> was simply querying the edid - I've fixed it in my driver
>>
>> 2. test VIDIOC_DV_TIMINGS_CAP: FAIL
>> fail: v4l2-test-io-config.cpp(375): doioctl(node,
>> VIDIOC_DV_TIMINGS_CAP, &timingscap) != EINVAL
>> fail: v4l2-test-io-config.cpp(392): EDID check failed for source pad 0.
>>
>> It looks like the purpose of the test is to do an ioctl with an
>> invalid pad setting to ensure -EINVAL is returned. However by the time
>> the VIDIOC_DV_TIMINGS_CAP ioctl used here gets routed to a
>
> No, VIDIOC_SUBDEV_DV_TIMINGS_CAP == VIDIOC_DV_TIMINGS_CAP, i.e. the
> v4l-subdev API reuses the same ioctl as is used in the 'main' V4L2 API.
> See include/uapi/linux/v4l2-subdev.h at the end for a list of 'alias'
> ioctls.
Ah... thanks - I realized that was happening somehow but couldn't see how :)
>
> Looking at v7 your tda1997x_get_dv_timings_cap() function doesn't check
> for a valid pad field. Same for tda1997x_enum_dv_timings().
Right - I noticed this right off as well - I've added pad validation
but that didn't resolve the failure.
The test failing is:
memset(&timingscap, 0, sizeof(timingscap));
timingscap.pad = node->is_subdev() ? node->entity.pads : 1;
^^^^ this sets pad=node->entity.pads=1 which is invalid
fail_on_test(doioctl(node, VIDIOC_DV_TIMINGS_CAP, &timingscap)
!= EINVAL);
^^^^ tda1997x_get_dv_timings_cap() gets called with cap->pad = 0 which
is valid to returns 0. I don't understand how the userspace pad=1 get
changed to pad=0 in my handler.
>
>> VIDIOC_SUBDEV_DV_TIMINGS_CAP the pad is changed to 0 which is valid.
>> I'm not following what's going on here.
>>
>> 3. test Try VIDIOC_SUBDEV_G/S_FMT: FAIL
>> fail: v4l2-test-subdevs.cpp(303): fmt.code == 0 || fmt.code == ~0U
>> fail: v4l2-test-subdevs.cpp(342): checkMBusFrameFmt(node, fmt.format)
>>
>> This is reporting that a VIDIOC_SUBDEV_G_FMT with
>> which=V4L2_SUBDEV_FORMAT_TRY returns format->code = 0. The following
>> is my set_format:
>>
>> static int tda1997x_get_format(struct v4l2_subdev *sd,
>> struct v4l2_subdev_pad_config *cfg,
>> struct v4l2_subdev_format *format)
>> {
>> struct tda1997x_state *state = to_state(sd);
>>
>> v4l_dbg(1, debug, state->client, "%s pad=%d which=%d\n",
>> __func__, format->pad, format->which);
>> if (format->pad != TDA1997X_PAD_SOURCE)
>> return -EINVAL;
>>
>> tda1997x_fill_format(state, &format->format);
>>
>> if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>> struct v4l2_mbus_framefmt *fmt;
>>
>> fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
>> format->format.code = fmt->code;
>> } else {
>> format->format.code = state->mbus_code;
>> }
>>
>> return 0;
>> }
>>
>> I don't at all understand the V4L2_SUBDEV_FORMAT_TRY logic here which
>> I took from other subdev drivers as boilerplate. Is the test valid?
>
> The test is valid, this really shouldn't return a 0 code. But I am not
> sure what the right logic is. I'll need to do some digging myself.
I got lost in the v4l2_subdev_get_try_format implementation but closer
inspection shows me that v4l2_subdev_get_try_format returns the
try_fmt field of the v4l2_subdev_pad_config used for storing subdev
pad info. What I'm missing is how that struct/field gets initialized?
Do I need to implement an init_cfg op?
Regards,
Tim
