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

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

 



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
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux