Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1437710 > unrolled thread

Re: [PATCH v3 0/9] Add MT8173 Video Decoder Driver

Started byHans Verkuil <hverkuil@xs4all.nl>
First post2016-07-06 15:30 +0200
Last post2016-07-08 13:50 +0200
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 0/9] Add MT8173 Video Decoder Driver Hans Verkuil <hverkuil@xs4all.nl> - 2016-07-06 15:30 +0200
    Re: [PATCH v3 0/9] Add MT8173 Video Decoder Driver tiffany lin <tiffany.lin@mediatek.com> - 2016-07-07 12:20 +0200
      Re: [PATCH v3 0/9] Add MT8173 Video Decoder Driver Hans Verkuil <hansverk@cisco.com> - 2016-07-07 13:20 +0200
      Re: [PATCH v3 0/9] Add MT8173 Video Decoder Driver Hans Verkuil <hverkuil@xs4all.nl> - 2016-07-08 13:50 +0200

#1437710 — Re: [PATCH v3 0/9] Add MT8173 Video Decoder Driver

FromHans Verkuil <hverkuil@xs4all.nl>
Date2016-07-06 15:30 +0200
SubjectRe: [PATCH v3 0/9] Add MT8173 Video Decoder Driver
Message-ID<rRV3P-1jb-1@gated-at.bofh.it>
Hi Tiffany,

I plan to review this patch series on Friday, but one obvious question is
what the reason for these failures is:

> Input/Output configuration ioctls:
>         test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
>         test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
>         test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
>         test VIDIOC_G/S_EDID: OK (Not Supported)
> 
>         Control ioctls:
>                 test VIDIOC_QUERYCTRL/MENU: OK
>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-controls.cpp(357): g_ctrl returned an error (11)
>                 test VIDIOC_G/S_CTRL: FAIL
>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-controls.cpp(579): g_ext_ctrls returned an error (11)
>                 test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-controls.cpp(721): subscribe event for control 'User Controls' failed
>                 test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
>                 test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
>                 Standard Controls: 2 Private Controls: 0
> 
>         Format ioctls:
>                 test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
>                 test VIDIOC_G/S_PARM: OK (Not Supported)
>                 test VIDIOC_G_FBUF: OK (Not Supported)
>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-formats.cpp(405): expected EINVAL, but got 11 when getting format for buftype 9
>                 test VIDIOC_G_FMT: FAIL
>                 test VIDIOC_TRY_FMT: OK (Not Supported)
>                 test VIDIOC_S_FMT: OK (Not Supported)
>                 test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> 
>         Codec ioctls:
>                 test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
>                 test VIDIOC_G_ENC_INDEX: OK (Not Supported)
>                 test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
> 
>         Buffer ioctls:
>                 test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-buffers.cpp(500): q.has_expbuf(node)
>                 test VIDIOC_EXPBUF: FAIL
> 
> 
> Total: 38, Succeeded: 33, Failed: 5, Warnings: 0

If it is due to a bug in v4l2-compliance, then let me know and I'll fix it. If not,
then it should be fixed in the driver.

Frankly, it was the presence of these failures that made me think this patch series
wasn't final. Before a v4l2 driver can be accepted in the kernel, v4l2-compliance must pass.

Regards,

	Hans

[toc] | [next] | [standalone]

#1438497

Fromtiffany lin <tiffany.lin@mediatek.com>
Date2016-07-07 12:20 +0200
Message-ID<rSezv-5yn-7@gated-at.bofh.it>
In reply to#1437710
Hi Hans,


On Wed, 2016-07-06 at 15:19 +0200, Hans Verkuil wrote:
> Hi Tiffany,
> 
> I plan to review this patch series on Friday, but one obvious question is
> what the reason for these failures is:
> 
> > Input/Output configuration ioctls:
> >         test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
> >         test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> >         test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> >         test VIDIOC_G/S_EDID: OK (Not Supported)
> > 
> >         Control ioctls:
> >                 test VIDIOC_QUERYCTRL/MENU: OK
> >                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-controls.cpp(357): g_ctrl returned an error (11)
> >                 test VIDIOC_G/S_CTRL: FAIL
> >                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-controls.cpp(579): g_ext_ctrls returned an error (11)
> >                 test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
These fails are because VIDIOC_G_CTRL and VIDIOC_G_EXT_CTRLS return
V4L2_CID_MIN_BUFFERS_FOR_CAPTURE only when dirver in MTK_STATE_HEADER
state, or it will return EAGAIN.
This could help user space get correct value, not default value that may
changed base on media content.

> >                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-controls.cpp(721): subscribe event for control 'User Controls' failed
> >                 test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
Driver do not support subscribe event for control 'User Controls' for
now.
Do we need to support this?

> >                 test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> >                 Standard Controls: 2 Private Controls: 0
> > 
> >         Format ioctls:
> >                 test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> >                 test VIDIOC_G/S_PARM: OK (Not Supported)
> >                 test VIDIOC_G_FBUF: OK (Not Supported)
> >                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-formats.cpp(405): expected EINVAL, but got 11 when getting format for buftype 9
> >                 test VIDIOC_G_FMT: FAIL
This is because vidioc_vdec_g_fmt only succeed when context is in
MTK_STATE_HEADER state, or user space cannot get correct format data
using this function.

> >                 test VIDIOC_TRY_FMT: OK (Not Supported)
> >                 test VIDIOC_S_FMT: OK (Not Supported)
> >                 test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> > 
> >         Codec ioctls:
> >                 test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
> >                 test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> >                 test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
> > 
> >         Buffer ioctls:
> >                 test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> >                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-buffers.cpp(500): q.has_expbuf(node)
Our OUTPUT and CAPTURE queue support both VB2_DMABUF and VB2_MMAP, user
space can select which to use in runtime.
So our driver default support v4l2_m2m_ioctl_expbuf functionality.
In v4l2-compliance test, it will check v4l2_m2m_ioctl_expbuf only valid
when node->valid_memorytype is V4L2_MEMORY_MMAP.
So when go through node->valid_memorytype is V4L2_MEMORY_DMABUF, it
fail.


best regards,
Tiffany



> >                 test VIDIOC_EXPBUF: FAIL
> > 
> > 
> > Total: 38, Succeeded: 33, Failed: 5, Warnings: 0
> 
> If it is due to a bug in v4l2-compliance, then let me know and I'll fix it. If not,
> then it should be fixed in the driver.
> 
> Frankly, it was the presence of these failures that made me think this patch series
> wasn't final. Before a v4l2 driver can be accepted in the kernel, v4l2-compliance must pass.
> 
> Regards,
> 
> 	Hans

[toc] | [prev] | [next] | [standalone]

#1438528

FromHans Verkuil <hansverk@cisco.com>
Date2016-07-07 13:20 +0200
Message-ID<rSfvz-69Q-3@gated-at.bofh.it>
In reply to#1438497
Hi Tiffany,

On 07/07/16 12:16, tiffany lin wrote:
> Hi Hans,
> 
> 
> On Wed, 2016-07-06 at 15:19 +0200, Hans Verkuil wrote:
>> Hi Tiffany,
>>
>> I plan to review this patch series on Friday, but one obvious question is
>> what the reason for these failures is:
>>
>>> Input/Output configuration ioctls:
>>>         test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
>>>         test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
>>>         test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
>>>         test VIDIOC_G/S_EDID: OK (Not Supported)
>>>
>>>         Control ioctls:
>>>                 test VIDIOC_QUERYCTRL/MENU: OK
>>>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-controls.cpp(357): g_ctrl returned an error (11)
>>>                 test VIDIOC_G/S_CTRL: FAIL
>>>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-controls.cpp(579): g_ext_ctrls returned an error (11)
>>>                 test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
> These fails are because VIDIOC_G_CTRL and VIDIOC_G_EXT_CTRLS return
> V4L2_CID_MIN_BUFFERS_FOR_CAPTURE only when dirver in MTK_STATE_HEADER
> state, or it will return EAGAIN.
> This could help user space get correct value, not default value that may
> changed base on media content.

So this should be improved in v4l2-compliance.

> 
>>>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-controls.cpp(721): subscribe event for control 'User Controls' failed
>>>                 test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
> Driver do not support subscribe event for control 'User Controls' for
> now.
> Do we need to support this?

Let me check: this might be a knock-on effect from the previous error.

Which controls does your driver have? (v4l2-ctl -l)

> 
>>>                 test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
>>>                 Standard Controls: 2 Private Controls: 0
>>>
>>>         Format ioctls:
>>>                 test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
>>>                 test VIDIOC_G/S_PARM: OK (Not Supported)
>>>                 test VIDIOC_G_FBUF: OK (Not Supported)
>>>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-formats.cpp(405): expected EINVAL, but got 11 when getting format for buftype 9
>>>                 test VIDIOC_G_FMT: FAIL
> This is because vidioc_vdec_g_fmt only succeed when context is in
> MTK_STATE_HEADER state, or user space cannot get correct format data
> using this function.

I'll take a look at this as well.

> 
>>>                 test VIDIOC_TRY_FMT: OK (Not Supported)
>>>                 test VIDIOC_S_FMT: OK (Not Supported)
>>>                 test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
>>>
>>>         Codec ioctls:
>>>                 test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
>>>                 test VIDIOC_G_ENC_INDEX: OK (Not Supported)
>>>                 test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
>>>
>>>         Buffer ioctls:
>>>                 test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>>>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-buffers.cpp(500): q.has_expbuf(node)

Don't use v4l-utils-1.6: just use the latest master branch. That way you can
be certain that the latest fixes are in. Besides, 1.6.0 is really old (almost
two years!). If you used the same old version for the encoder driver as well,
then please retest that encoder driver!

> Our OUTPUT and CAPTURE queue support both VB2_DMABUF and VB2_MMAP, user
> space can select which to use in runtime.
> So our driver default support v4l2_m2m_ioctl_expbuf functionality.
> In v4l2-compliance test, it will check v4l2_m2m_ioctl_expbuf only valid
> when node->valid_memorytype is V4L2_MEMORY_MMAP.
> So when go through node->valid_memorytype is V4L2_MEMORY_DMABUF, it
> fail.

That sounds like a bug. I'll take a look. For all I know, this is fixed in the
master branch.

Regards,

	Hans

> 
> 
> best regards,
> Tiffany
> 
> 
> 
>>>                 test VIDIOC_EXPBUF: FAIL
>>>
>>>
>>> Total: 38, Succeeded: 33, Failed: 5, Warnings: 0
>>
>> If it is due to a bug in v4l2-compliance, then let me know and I'll fix it. If not,
>> then it should be fixed in the driver.
>>
>> Frankly, it was the presence of these failures that made me think this patch series
>> wasn't final. Before a v4l2 driver can be accepted in the kernel, v4l2-compliance must pass.
>>
>> Regards,
>>
>> 	Hans
> 
>

[toc] | [prev] | [next] | [standalone]

#1439342

FromHans Verkuil <hverkuil@xs4all.nl>
Date2016-07-08 13:50 +0200
Message-ID<rSCs9-4e3-23@gated-at.bofh.it>
In reply to#1438497
On 07/07/2016 12:16 PM, tiffany lin wrote:
> Hi Hans,
> 
> 
> On Wed, 2016-07-06 at 15:19 +0200, Hans Verkuil wrote:
>> Hi Tiffany,
>>
>> I plan to review this patch series on Friday, but one obvious question is
>> what the reason for these failures is:
>>
>>> Input/Output configuration ioctls:
>>>         test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
>>>         test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
>>>         test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
>>>         test VIDIOC_G/S_EDID: OK (Not Supported)
>>>
>>>         Control ioctls:
>>>                 test VIDIOC_QUERYCTRL/MENU: OK
>>>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-controls.cpp(357): g_ctrl returned an error (11)
>>>                 test VIDIOC_G/S_CTRL: FAIL
>>>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-controls.cpp(579): g_ext_ctrls returned an error (11)
>>>                 test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
> These fails are because VIDIOC_G_CTRL and VIDIOC_G_EXT_CTRLS return
> V4L2_CID_MIN_BUFFERS_FOR_CAPTURE only when dirver in MTK_STATE_HEADER
> state, or it will return EAGAIN.
> This could help user space get correct value, not default value that may
> changed base on media content.

OK, I really don't like this. I also looked what the s5p-mfc-dec driver does (the only other
driver currently implementing this), and that returns -EINVAL.

My proposal would be to change this. If this information isn't known yet, why not
just return 0 as the value? The doc would have to be updated and (preferably) also
the s5p-mfc-dec driver. I've added Samsung devs to the Cc list, let me know what you
think.

> 
>>>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-controls.cpp(721): subscribe event for control 'User Controls' failed
>>>                 test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
> Driver do not support subscribe event for control 'User Controls' for
> now.
> Do we need to support this?

I don't see why this would fail. It's OK to subscribe to such controls, although
you'll never get an event.

>>>                 test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
>>>                 Standard Controls: 2 Private Controls: 0
>>>
>>>         Format ioctls:
>>>                 test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
>>>                 test VIDIOC_G/S_PARM: OK (Not Supported)
>>>                 test VIDIOC_G_FBUF: OK (Not Supported)
>>>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-formats.cpp(405): expected EINVAL, but got 11 when getting format for buftype 9
>>>                 test VIDIOC_G_FMT: FAIL
> This is because vidioc_vdec_g_fmt only succeed when context is in
> MTK_STATE_HEADER state, or user space cannot get correct format data
> using this function.

Comparing this to s5p-mfc-dec I see that -EINVAL is returned in that case.

I am not opposed to using EAGAIN in s5p-mfc-dec as well. Marek, Kamil, what is
your opinion?

> 
>>>                 test VIDIOC_TRY_FMT: OK (Not Supported)
>>>                 test VIDIOC_S_FMT: OK (Not Supported)
>>>                 test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
>>>
>>>         Codec ioctls:
>>>                 test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
>>>                 test VIDIOC_G_ENC_INDEX: OK (Not Supported)
>>>                 test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
>>>
>>>         Buffer ioctls:
>>>                 test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>>>                 fail: ../../../v4l-utils-1.6.0/utils/v4l2-compliance/v4l2-test-buffers.cpp(500): q.has_expbuf(node)
> Our OUTPUT and CAPTURE queue support both VB2_DMABUF and VB2_MMAP, user
> space can select which to use in runtime.
> So our driver default support v4l2_m2m_ioctl_expbuf functionality.
> In v4l2-compliance test, it will check v4l2_m2m_ioctl_expbuf only valid
> when node->valid_memorytype is V4L2_MEMORY_MMAP.
> So when go through node->valid_memorytype is V4L2_MEMORY_DMABUF, it
> fail.

valid_memorytype should have both MMAP and DMABUF flags.

But v4l-utils-1.6.0 is way too old to be certain it isn't some v4l2-compliance bug
that has since been fixed.

Regards,

	Hans

> 
> 
> best regards,
> Tiffany
> 
> 
> 
>>>                 test VIDIOC_EXPBUF: FAIL
>>>
>>>
>>> Total: 38, Succeeded: 33, Failed: 5, Warnings: 0
>>
>> If it is due to a bug in v4l2-compliance, then let me know and I'll fix it. If not,
>> then it should be fixed in the driver.
>>
>> Frankly, it was the presence of these failures that made me think this patch series
>> wasn't final. Before a v4l2 driver can be accepted in the kernel, v4l2-compliance must pass.
>>
>> Regards,
>>
>> 	Hans
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web