Re: [PATCH v8 0/7] TDA1997x HDMI video reciver

From: Hans Verkuil
Date: Wed Feb 07 2018 - 03:22:50 EST

Next message: Kirill A. Shutemov: "Re: [PATCH 2/3] x86/tme: Detect if TME and MKTME is activated by BIOS"
Previous message: Paul E. McKenney: "Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()"
In reply to: Tim Harvey: "Re: [PATCH v8 0/7] TDA1997x HDMI video reciver"
Next in thread: Hans Verkuil: "Re: [PATCH v8 0/7] TDA1997x HDMI video reciver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 02/07/2018 12:29 AM, Tim Harvey wrote:
> On Tue, Feb 6, 2018 at 1:21 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>> On 02/06/2018 09:27 PM, Tim Harvey wrote:
>>
>> <snip>
>>
>>> v4l2-compliance test results:
>>> - with the following kernel patches:
>>> v4l2-subdev: clear reserved fields
>>> . v4l2-subdev: without controls return -ENOTTY
>>>
>>> v4l2-compliance SHA : b2f8f9049056eb6f9e028927dacb2c715a062df8
>>> Media Driver Info:
>>> Driver name : imx-media
>>> Model : imx-media
>>> Serial :
>>> Bus info :
>>> Media version : 4.15.0
>>> Hardware revision: 0x00000000 (0)
>>> Driver version : 4.15.0
>>> Interface Info:
>>> ID : 0x0300008f
>>> Type : V4L Sub-Device
>>> Entity Info:
>>> ID : 0x00000003 (3)
>>> Name : tda19971 2-0048
>>> Function : Unknown
>>
>> This is missing. It should be one of these:
>>
>> https://hverkuil.home.xs4all.nl/spec/uapi/mediactl/media-types.html#media-entity-type
>>
>> However, we don't have a proper function defined.
>>
>> I would suggest adding a new MEDIA_ENT_F_DTV_DECODER analogous to MEDIA_ENT_F_ATV_DECODER.
>>
>> It would be a new patch adding this + documentation.
>
> Hows this look for adding to my next series:
>
> Author: Tim Harvey <tharvey@xxxxxxxxxxxxx>
> Date: Tue Feb 6 14:12:52 2018 -0800
>
> [media] add digital video decoder video interface entity functions
>
> Add a new media entity function definition for digital TV decoders:
> MEDIA_ENT_F_DTV_DECODER
>
> Signed-off-by: Tim Harvey <tharvey@xxxxxxxxxxxxx>
>
> --- a/Documentation/media/uapi/mediactl/media-types.rst
> +++ b/Documentation/media/uapi/mediactl/media-types.rst
> @@ -321,6 +321,17 @@ Types and flags used to represent the media graph elements
> MIPI CSI-2, ...), and outputs them on its source pad to an output
> video bus of another type (eDP, MIPI CSI-2, parallel, ...).
>
> + - .. row 31
> +
> + .. _MEDIA-ENT-F-DTV-DECODER:
> +
> + - ``MEDIA_ENT_F_DTV_DECODER``
> +
> + - Digital video decoder. The basic function of the video decoder is
> + to accept digital video from a wide variety of sources
> + and output it in some digital video standard, with appropriate
> + timing signals.
> +
> .. tabularcolumns:: |p{5.5cm}|p{12.0cm}|
>
> .. _media-entity-flag:
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index b9b9446..6653e88 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -152,6 +152,7 @@ struct media_device_info {
> * MEDIA_ENT_F_IF_VID_DECODER and/or MEDIA_ENT_F_IF_AUD_DECODER.
> */
> #define MEDIA_ENT_F_TUNER (MEDIA_ENT_F_OLD_SUBDEV_BASE + 5)
> +#define MEDIA_ENT_F_DTV_DECODER
> (MEDIA_ENT_F_OLD_SUBDEV_BASE + 6)

Don't use MEDIA_ENT_F_OLD_SUBDEV_BASE for new functions.

Use this instead:

/*
* Analog video decoder functions
*/
#define MEDIA_ENT_F_DTV_DECODER (MEDIA_ENT_F_BASE + 0x6001)

Other than this, this patch looks great.

>
> #define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN MEDIA_ENT_F_OLD_SUBDEV_BASE
>
>
> Assigning this now shows a function but does not resolve the media
> compliance results:
>
> --- a/drivers/media/i2c/tda1997x.c
> +++ b/drivers/media/i2c/tda1997x.c
> @@ -2586,6 +2586,7 @@ static int tda1997x_probe(struct i2c_client *client,
> id->name, i2c_adapter_id(client->adapter),
> client->addr);
> sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> + sd->entity.function = MEDIA_ENT_F_DTV_DECODER;
> sd->entity.ops = &tda1997x_media_ops;
>
> /* set allowed mbus modes based on chip, bus-type, and bus-width */
>
>
> root@ventana:~# v4l2-compliance -u1
> v4l2-compliance SHA : b2f8f9049056eb6f9e028927dacb2c715a062df8
> Media Driver Info:
> Driver name : imx-media
> Model : imx-media
> Serial :
> Bus info :
> Media version : 4.15.0
> Hardware revision: 0x00000000 (0)
> Driver version : 4.15.0
> Interface Info:
> ID : 0x0300008f
> Type : V4L Sub-Device
> Entity Info:
> ID : 0x00000003 (3)
> Name : tda19971 2-0048
> Function : Unknown (00020006)

Actually it does. The value above is now the new function.

> Pad 0x01000004 : Source
> Link 0x0200006f: to remote pad 0x1000063 of entity
> 'ipu1_csi0_mux': Data
>
> ...
>
> root@ventana:~# v4l2-compliance -m0 -M
> v4l2-compliance SHA : b2f8f9049056eb6f9e028927dacb2c715a062df8
> Media Driver Info:
> Driver name : imx-media
> Model : imx-media
> Serial :
> Bus info :
> Media version : 4.15.0
> Hardware revision: 0x00000000 (0)
> Driver version : 4.15.0
>
> Compliance test for device /dev/media0:
>
> Required ioctls:
> test MEDIA_IOC_DEVICE_INFO: OK
>
> Allow for multiple opens:
> test second /dev/media0 open: OK
> test MEDIA_IOC_DEVICE_INFO: OK
> test for unlimited opens: OK
>
> Media Controller ioctls:
> fail: v4l2-test-media.cpp(141): ent.function ==
> MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN

Weird, this shouldn't happen. I'll look into this a bit more.

> test MEDIA_IOC_G_TOPOLOGY: FAIL
> fail: v4l2-test-media.cpp(256):
> v2_entities_set.find(ent.id) == v2_entities_set.end()

This is a v4l2-compliance bug that I have fixed. Just do another git pull.

Regards,

Hans

> test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL
> test MEDIA_IOC_SETUP_LINK: OK
>
> Total: 7, Succeeded: 5, Failed: 2, Warnings: 0
>
>>
> <snip>
>>>
>>> Sub-Device ioctls (Source Pad 0):
>>> test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
>>> test Try VIDIOC_SUBDEV_G/S_FMT: OK
>>> test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported)
>>> test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
>>> test Active VIDIOC_SUBDEV_G/S_FMT: OK
>>> test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported)
>>> test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported)
>>>
>>> Control ioctls:
>>> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
>>> test VIDIOC_QUERYCTRL: OK (Not Supported)
>>> test VIDIOC_G/S_CTRL: OK (Not Supported)
>>> test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
>>> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
>>> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
>>> Standard Controls: 0 Private Controls: 0
>>
>> Why doesn't this show anything? You have at least one control, so this should
>> reflect that. Does 'v4l2-ctl -d /dev/v4l-subdev1 -l' show any controls?
>>
>> I think sd->ctrl_handler is never set to the v4l2_ctrl_handler pointer.
>>
>> Have you ever tested the controls?
>>
>> Looking closer I also notice that the control handler is never freed. Or
>> checked for errors when it is created in the probe function. Hmm, I should
>> have caught that earlier.
>>
>
> Yes thanks - I missed this also:
>
> --- a/drivers/media/i2c/tda1997x.c
> +++ b/drivers/media/i2c/tda1997x.c
> @@ -2726,6 +2726,12 @@ static int tda1997x_probe(struct i2c_client *client,
> &tda1997x_ctrl_ops,
> V4L2_CID_DV_RX_RGB_RANGE, V4L2_DV_RGB_RANGE_FULL, 0,
> V4L2_DV_RGB_RANGE_AUTO);
> + state->sd.ctrl_handler = hdl;
> + if (hdl->error) {
> + ret = hdl->error;
> + goto err_free_handler;
> + }
> + v4l2_ctrl_handler_setup(hdl);
>
> /* initialize source pads */
> state->pads[TDA1997X_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> @@ -2774,6 +2780,8 @@ static int tda1997x_probe(struct i2c_client *client,
>
> err_free_media:
> media_entity_cleanup(&sd->entity);
> +err_free_handler:
> + v4l2_ctrl_handler_free(&state->hdl);
> err_free_mutex:
> cancel_delayed_work(&state->delayed_work_enable_hpd);
> mutex_destroy(&state->page_lock);
> @@ -2801,6 +2809,7 @@ static int tda1997x_remove(struct i2c_client *client)
>
> v4l2_async_unregister_subdev(sd);
> media_entity_cleanup(&sd->entity);
> + v4l2_ctrl_handler_free(&state->hdl);
> regulator_bulk_disable(TDA1997X_NUM_SUPPLIES, state->supplies);
> i2c_unregister_device(state->client_cec);
> cancel_delayed_work(&state->delayed_work_enable_hpd);
>
> root@ventana:~# v4l2-ctl -d /dev/v4l-subdev1 --list-ctrls
>
> Digital Video Controls
>
> power_present 0x00a00964 (bitmask): max=0x00000001
> default=0x00000000 value=0x00000000 flags=read-only
> rx_rgb_quantization_range 0x00a00965 (menu) : min=0 max=2
> default=0 value=2
> rx_it_content_type 0x00a00966 (menu) : min=0 max=4
> default=4 value=0 flags=read-only, volatile
>
> And various sets/gets appear to work as designed (found that I wasn't
> updating the csc when quantiation range was changed via ctrl and fixed
> it).
>
> Regards,
>
> Tim
>

Next message: Kirill A. Shutemov: "Re: [PATCH 2/3] x86/tme: Detect if TME and MKTME is activated by BIOS"
Previous message: Paul E. McKenney: "Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()"
In reply to: Tim Harvey: "Re: [PATCH v8 0/7] TDA1997x HDMI video reciver"
Next in thread: Hans Verkuil: "Re: [PATCH v8 0/7] TDA1997x HDMI video reciver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]