[{"id":1773994,"web_url":"http://patchwork.ozlabs.org/comment/1773994/","msgid":"<58433b8c-5e72-feb1-4515-9396d075e350@xs4all.nl>","list_archive_url":null,"date":"2017-09-23T07:30:29","subject":"Re: [PATCH 3/4] media: i2c: Add TDA1997x HDMI receiver driver","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"Hi Tim,\n\nOn 23/09/17 00:24, Tim Harvey wrote:\n> Add support for the TDA1997x HDMI receivers.\n\nI did a very quick high-level scan and found a few blockers:\n\n1) You *must* implement the get/set_edid ops. I won't accept\n the driver without that. You can use v4l2-ctl to set/get the\n EDID (see v4l2-ctl --help-edid).\n\n2) The dv_timings_cap and enum_dv_timings ops are missing: those\n must be implemented as well.\n\n3) Drop the deprecated g_mbus_config op.\n\n4) Do a proper implementation of query_dv_timings. It appears you\n change the timings in the irq function: that's wrong. The sequence\n should be the following:\n\n a) the irq handler detects that timings have changed and sends\n a V4L2_EVENT_SOURCE_CHANGE event to userspace.\n b) when userspace receives that event it will stop streaming,\n call VIDIOC_QUERY_DV_TIMINGS and if new valid timings are\n detected it will call VIDIOC_S_DV_TIMINGS, allocate the new\n buffers and start streaming again.\n\n The driver shall never switch timings on its own, this must be\n directed from userspace. Different timings will require different\n configuration through the whole stack (other HW in the video pipeline,\n DMA engines, userspace memory allocations, etc, etc). Only userspace\n can do the reconfiguration.\n\nGeneral note: if you want to implement CEC and/or HDCP, contact me first.\nI can give pointers on how to do that.\n\nThis is just a quick scan. I won't have time to do an in-depth review\nfor the next two weeks. Ideally you'll have a v2 ready by then with the\nissues mentioned above fixed.\n\nDid you run the v4l2-compliance utility to test this driver? For a v2\nplease run it and add the output to the cover letter of the patch series.\n\nYou say \"TDA19972 support (2 inputs)\": I assume that that means that there\nare 2 inputs, but only one is active at a time. Right?\n\nRegards,\n\n\tHans\n\n> \n> Signed-off-by: Tim Harvey \n> ---\n> drivers/media/i2c/Kconfig | 9 +\n> drivers/media/i2c/Makefile | 1 +\n> drivers/media/i2c/tda1997x.c | 3065 ++++++++++++++++++++++++++++++++++\n> include/dt-bindings/media/tda1997x.h | 78 +\n> include/media/i2c/tda1997x.h | 53 +\n> 5 files changed, 3206 insertions(+)\n> create mode 100644 drivers/media/i2c/tda1997x.c\n> create mode 100644 include/dt-bindings/media/tda1997x.h\n> create mode 100644 include/media/i2c/tda1997x.h\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xzhpQ5RHmz9t3C\n\tfor ;\n\tSat, 23 Sep 2017 17:30:38 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750778AbdIWHad (ORCPT\n\t);\n\tSat, 23 Sep 2017 03:30:33 -0400","from lb2-smtp-cloud8.xs4all.net ([194.109.24.25]:37415 \"EHLO\n\tlb2-smtp-cloud8.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1750762AbdIWHad (ORCPT\n\t); Sat, 23 Sep 2017 03:30:33 -0400","from [192.168.1.10] ([80.101.105.217])\n\tby smtp-cloud8.xs4all.net with ESMTPA\n\tid vetRd69fab4gvvetSd0X5B; Sat, 23 Sep 2017 09:30:31 +0200"],"Subject":"Re: [PATCH 3/4] media: i2c: Add TDA1997x HDMI receiver driver","To":"Tim Harvey , linux-media@vger.kernel.org","References":"<1506119053-21828-1-git-send-email-tharvey@gateworks.com>\n\t<1506119053-21828-4-git-send-email-tharvey@gateworks.com>","Cc":"devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tshawnguo@kernel.org, Steve Longerbeam ,\n\tPhilipp Zabel ,\n\tHans Verkuil , \n\tMauro Carvalho Chehab ","From":"Hans Verkuil ","Message-ID":"<58433b8c-5e72-feb1-4515-9396d075e350@xs4all.nl>","Date":"Sat, 23 Sep 2017 09:30:29 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101\n\tThunderbird/45.7.1","MIME-Version":"1.0","In-Reply-To":"<1506119053-21828-4-git-send-email-tharvey@gateworks.com>","Content-Type":"text/plain; charset=windows-1252","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfGvpdf1FC+57MFHGpOOs1CToWq4g+oGTtQs5lbyl8zSAkW7OayYYaiouXIBwr2cVq8Vxv8+A3vDP4MeTO3Jwl1xPhghxGYPkVkwnufwW3fYrVz9/wV5F\n\thDTj3RzjVOGtwB68eDDNok0zag8Vc5cLJKwHoly4PalGSbUf7k/DxqjDad4gaAFHkcEEDVERBbwD9sXf0GOjni4QK4KaGaLcW2csIYXTT6305dgplQlDDfp/\n\tDgf6stUFFBeXwhEglB9HZxp5CBT2LnUv7/QrdmSNHF0lc/xLUiB8PYqSqbJiypxjotaz7YkZ64hV44TNJYC/Dc+H50XhX45EUjVrFYHrc73r5MwejN5Rwv+W\n\tk5PqXDGJH5/BVwAHQu5cW47r3aiGPBmxTCK89Z+nXFF6swO6U0sXQqmBoke7S3URM8A9ymiQCgh4XpZ/4N2GqUhzsCamOw==","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1774969,"web_url":"http://patchwork.ozlabs.org/comment/1774969/","msgid":"","list_archive_url":null,"date":"2017-09-25T20:44:51","subject":"Re: [PATCH 3/4] media: i2c: Add TDA1997x HDMI receiver driver","submitter":{"id":41730,"url":"http://patchwork.ozlabs.org/api/people/41730/","name":"Tim Harvey","email":"tharvey@gateworks.com"},"content":"On Sat, Sep 23, 2017 at 12:30 AM, Hans Verkuil wrote:\n> Hi Tim,\n>\n> On 23/09/17 00:24, Tim Harvey wrote:\n>> Add support for the TDA1997x HDMI receivers.\n>\n\nHans,\n\nThanks for the quick review!\n\n> I did a very quick high-level scan and found a few blockers:\n>\n> 1) You *must* implement the get/set_edid ops. I won't accept\n> the driver without that. You can use v4l2-ctl to set/get the\n> EDID (see v4l2-ctl --help-edid).\n\nok\n\nWhen I add hooks for get_edid/set_edid I'm getting 'VIDIOC_S_EDID:\nfailed: Inappropriate ioctl for device'. v4l2-ctl doesn't operate\ndirectly on subdevs so I'm operating on the imx-media-capture device\nthat the tda1997x is linked to via media-ctl. Am I missing something?\n\n>\n> 2) The dv_timings_cap and enum_dv_timings ops are missing: those\n> must be implemented as well.\n\nok\n\nThe v4l2_dv_timings_cap contains a v4l2_bt_timings_cap and a type but\nit looks like the only type currently available is\nV4L2_DV_BT_656_1120. As the tda1997x can output raw RGB and raw YUV\nwhich are not technically BT656/1120 what should I be setting for\nthose?\n\n>\n> 3) Drop the deprecated g_mbus_config op.\n\nok\n\n>\n> 4) Do a proper implementation of query_dv_timings. It appears you\n> change the timings in the irq function: that's wrong. The sequence\n> should be the following:\n>\n> a) the irq handler detects that timings have changed and sends\n> a V4L2_EVENT_SOURCE_CHANGE event to userspace.\n> b) when userspace receives that event it will stop streaming,\n> call VIDIOC_QUERY_DV_TIMINGS and if new valid timings are\n> detected it will call VIDIOC_S_DV_TIMINGS, allocate the new\n> buffers and start streaming again.\n>\n> The driver shall never switch timings on its own, this must be\n> directed from userspace. Different timings will require different\n> configuration through the whole stack (other HW in the video pipeline,\n> DMA engines, userspace memory allocations, etc, etc). Only userspace\n> can do the reconfiguration.\n\nok, that makes a ton of sense. I was curious how userspace was\nnotified and handled this.\n\n>\n> General note: if you want to implement CEC and/or HDCP, contact me first.\n> I can give pointers on how to do that.\n>\n> This is just a quick scan. I won't have time to do an in-depth review\n> for the next two weeks. Ideally you'll have a v2 ready by then with the\n> issues mentioned above fixed.\n\nyes, I should have a v2 ready in a week or so after waiting for some\nmore feedback.\n\nI do have some general questions perhaps you can answer for me.\n\nThe TDA1997x video output port can drive HS/VS/DE either directly from\nthe HDMI input data (which creates a HS/VS pulses) or it can use an\ninternal VHREF timing generator that generates VREF/HREF signals\n(active during the entire frame/row). What sync signalling is\ntypically required by SoC's video input?\n\nThe IMX6 which I'm using needs HREF and VS so I've defaulted the sync\nconfiguration to that and not allowed sync type configuration via\ndevice-tree (although I did allow sync type configuration via a\nplatform data struct). I'm not clear if I need to allow all the sync\nconfiguration possibilities to be defined in device-tree at this point\nand haven't run across any other video decoders that expose this level\nof flexibility via dts.\n\nIn order for HREF/VREF to be generated the TDA1997x has an internal\nVHREF timing generator that works off of pixel counting. I'm currently\nconfiguring this based off of hard-coded values (units of pixel clock)\nfor href_start, href_end, vref_f1_start, vref_f1_width, vref_f2_start,\nvref_f2_width, fieldref_f1_start, fieldref_f2_start and field_polarity\nthat were provided in some vendor-specific sample code. I didn't see\nanything that would let me pull these values from the list of possible\ntimings in the kernel, but perhaps I should try calculate these based\non struct v4l2_bt_timings fields instead?\n\n>\n> Did you run the v4l2-compliance utility to test this driver? For a v2\n> please run it and add the output to the cover letter of the patch series.\n\nI did run v4l2-compliance but I wasn't clear how to interpret the\nresults and filter out compliance issues between the v4l2-subdev and\nthe capture driver it's linked to.\n\nI will post this info on the v2 cover page as requested but if\ninterested here's what it shows with my v0 RFC:\n\nHere's my pipline configuration for a GW54xx IMX6Q where tda19971 is\non IPU1_CSI0 via 16bit data bus supporting YUV422 (single clock cycle)\n\n# links\nmedia-ctl --reset\nmedia-ctl --links '\"tda19971 2-0048\":0 -> \"ipu1_csi0_mux\":1[1]'\nmedia-ctl --links '\"ipu1_csi0_mux\":2 -> \"ipu1_csi0\":0[1]'\nmedia-ctl --links '\"ipu1_csi0\":2 -> \"ipu1_csi0 capture\":0[1]'\n# format\nmedia-ctl --set-v4l2 \"'tda19971 2-0048':0[fmt:UYVY8_1X16/1280x720]\"\nmedia-ctl --set-v4l2 \"'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1280x720]\"\nmedia-ctl --set-v4l2 \"'ipu1_csi0':0[fmt:UYVY8_1X16/1280x720]\"\n\n# capture 1 frame\nv4l2-ctl -d4 --set-fmt-video=width=1280,height=720,pixelformat=UYVY\nv4l2-ctl -d4 --stream-mmap --stream-skip=1 --stream-to=/tmp/x.raw\n--stream-count=1\n\nmedia-ctl topology (.txt, .dot, .png) is at\nhttp://dev.gateworks.com/images/tharvey/tda1997x/gw54xx-imx6q/\n\nRunning v4l2-compliance on /dev/video4 shows:\nroot@ventana:~# v4l2-compliance -d /dev/video4\nDriver Info:\n Driver name : imx-media-captu\n Card type : imx-media-capture\n Bus info : platform:ipu1_csi0\n Driver version: 4.13.0\n Capabilities : 0x84200001\n Video Capture\n Streaming\n Extended Pix Format\n Device Capabilities\n Device Caps : 0x04200001\n Video Capture\n Streaming\n Extended Pix Format\n\nCompliance test for device /dev/video4 (not using libv4l2):\n\nRequired ioctls:\n test VIDIOC_QUERYCAP: OK\n\nAllow for multiple opens:\n test second video open: OK\n test VIDIOC_QUERYCAP: OK\n test VIDIOC_G/S_PRIORITY: OK\n\nDebug ioctls:\n test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)\n test VIDIOC_LOG_STATUS: OK (Not Supported)\n\nInput ioctls:\n test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)\n test VIDIOC_G/S_FREQUENCY: OK (Not Supported)\n test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)\n test VIDIOC_ENUMAUDIO: OK (Not Supported)\n fail: v4l2-test-input-output.cpp(418): G_INPUT not\nsupported for a capture device\n test VIDIOC_G/S/ENUMINPUT: FAIL\n test VIDIOC_G/S_AUDIO: OK (Not Supported)\n Inputs: 0 Audio Inputs: 0 Tuners: 0\n\nOutput ioctls:\n test VIDIOC_G/S_MODULATOR: OK (Not Supported)\n test VIDIOC_G/S_FREQUENCY: OK (Not Supported)\n test VIDIOC_ENUMAUDOUT: OK (Not Supported)\n test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)\n test VIDIOC_G/S_AUDOUT: OK (Not Supported)\n Outputs: 0 Audio Outputs: 0 Modulators: 0\n\nInput/Output configuration ioctls:\n test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)\n test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)\n test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)\n test VIDIOC_G/S_EDID: OK (Not Supported)\n\n Control ioctls:\n test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK\n test VIDIOC_QUERYCTRL: OK\n test VIDIOC_G/S_CTRL: OK\n test VIDIOC_G/S/TRY_EXT_CTRLS: OK\n fail: v4l2-test-controls.cpp(782): subscribe event for\ncontrol 'User Controls' failed\n test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL\n test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)\n Standard Controls: 1 Private Controls: 7\n\n Format ioctls:\n test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK\n test VIDIOC_G/S_PARM: OK\n test VIDIOC_G_FBUF: OK (Not Supported)\n test VIDIOC_G_FMT: OK\n test VIDIOC_TRY_FMT: OK\n test VIDIOC_S_FMT: OK\n test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)\n test Cropping: OK (Not Supported)\n test Composing: OK (Not Supported)\n test Scaling: OK (Not Supported)\n\n Codec ioctls:\n test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)\n test VIDIOC_G_ENC_INDEX: OK (Not Supported)\n test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)\n\n Buffer ioctls:\n test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK\n test VIDIOC_EXPBUF: OK\n\nTest input 0:\n\n\nTotal: 42, Succeeded: 40, Failed: 2, Warnings: 0\n\n\n>\n> You say \"TDA19972 support (2 inputs)\": I assume that that means that there\n> are 2 inputs, but only one is active at a time. Right?\n>\n\nThere are two inputs, both which can detect/report signal changes but\nyes only 1 can be selected at a time and output to a single video\noutput port.\n\nWhat is the correct subdev api for implementing multiple inputs?\n\nRegards,\n\nTim\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gateworks-com.20150623.gappssmtp.com\n\theader.i=@gateworks-com.20150623.gappssmtp.com header.b=\"Il/6Yly6\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y1GL16hhJz9t2Q\n\tfor ;\n\tTue, 26 Sep 2017 06:44:57 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S936244AbdIYUo4 (ORCPT );\n\tMon, 25 Sep 2017 16:44:56 -0400","from mail-wr0-f177.google.com ([209.85.128.177]:46177 \"EHLO\n\tmail-wr0-f177.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S936243AbdIYUoy (ORCPT\n\t); Mon, 25 Sep 2017 16:44:54 -0400","by mail-wr0-f177.google.com with SMTP id o42so9999025wrb.3\n\tfor ;\n\tMon, 25 Sep 2017 13:44:53 -0700 (PDT)","by 10.28.168.209 with HTTP; Mon, 25 Sep 2017 13:44:51 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gateworks-com.20150623.gappssmtp.com; s=20150623;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=6ld5Sm9bLnXQHZimgtBOMAzoMpB3Y64ISOeeCXHCIJs=;\n\tb=Il/6Yly6N7c2fuCBxxsPXEMTMeebHgUsw4ovjDYIfKT/NT041rrt93gPXzndZWCJJr\n\tSgYaT93dKaXQWijX/bRpoCYdok81Agef5QkqsnguINWk3Dm9Xx6kiyzCpJTaSdxpVG4/\n\thn4ULQr8Wc9UWtHU/UjTS+m4+Swp83a+oxQAX7aVg32GgWp+2AlI5dAf2UmiCKNFUdbR\n\tiUCy8SDbVncIeWAHZvsWSFOkFrg7v0NDU5NKKqq+6/1EHmLDTrc7nTaAuH46PtEi+5HH\n\tLGDmTeITJ0pXYoDk789k/QkM0I1F/wPqhyeYvjoNKBmQV6frNGvvynLLR1oO0C4Vra/4\n\tP+uw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=6ld5Sm9bLnXQHZimgtBOMAzoMpB3Y64ISOeeCXHCIJs=;\n\tb=rt4nebWkz0ypxZtWsDLOQEhN2TS5/luxTOvevc17A4nAJ8EnUMgktsZ1wppJglbfqB\n\t/5te2kCH0CaKJqZ1PQJih7GnX6JspIeu/jbPo4p+tsfMDjJLoDi5cGLoihOGP3+5ftIk\n\ttoguTwLWdqkhCSUAR8CSlJKQBcDXSPw4Jn0zYUTF5CJ9ZSJVEmR5TeBjEibxFybZT/cJ\n\tD92dRhrJpN/5730ZwIZPYDKZJSnWzDcIIQnol0c1HAdL+eBfMtgJ+2/F6WztkH7t4NUq\n\tXCf8osTP5vQguMBzTdSzAb8udm/YmnxDb+c9WrcdihMW/5OuqoP0hNPUJShMNhe6RugY\n\tKYTA==","X-Gm-Message-State":"AHPjjUjO99s2n6YTaH3ongzQUSx7dMD9d0wG2R4fRojTFDasHkGpWHyG\n\t7giepPjgsgz3a5wLqx6bE/SO2ym89EiXr+6U+kk0eQ==","X-Google-Smtp-Source":"AOwi7QCRSh7SlDJZfYsVjB4Yhf6bcp3QECX4EpZAc2TNR1V2j8TF+ZeMY17IeYt3JVsXqjlEuf95tCz8J9YXZOc6be8=","X-Received":"by 10.223.144.71 with SMTP id h65mr7377949wrh.41.1506372292486; \n\tMon, 25 Sep 2017 13:44:52 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<58433b8c-5e72-feb1-4515-9396d075e350@xs4all.nl>","References":"<1506119053-21828-1-git-send-email-tharvey@gateworks.com>\n\t<1506119053-21828-4-git-send-email-tharvey@gateworks.com>\n\t<58433b8c-5e72-feb1-4515-9396d075e350@xs4all.nl>","From":"Tim Harvey ","Date":"Mon, 25 Sep 2017 13:44:51 -0700","Message-ID":"","Subject":"Re: [PATCH 3/4] media: i2c: Add TDA1997x HDMI receiver driver","To":"Hans Verkuil ","Cc":"linux-media ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tShawn Guo ,\n\tSteve Longerbeam , \n\tPhilipp Zabel ,\n\tHans Verkuil , \n\tMauro Carvalho Chehab ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}}]