On Wed, Oct 10, 2018 at 03:45:35PM +0200, Hans Verkuil wrote: > On 10/10/18 14:09, ektor5 wrote: > > Hi Hans, > > > > On Sat, Oct 06, 2018 at 11:54:38AM +0200, Hans Verkuil wrote: > >> Hi Ettore, > >> > >> On 10/05/2018 07:33 PM, ektor5 wrote: > >>> This series of patches aims to add CEC functionalities to SECO > >>> devices, in particular UDOO X86. > >>> > >>> The communication is achieved via Braswell SMBus (i2c-i801) to the > >>> onboard STM32 microcontroller that handles the CEC signals. The driver > >>> use direct access to the PCI addresses, due to the limitations of the > >>> specific driver in presence of ACPI calls. > >>> > >>> The basic functionalities are tested with success with cec-ctl and > >>> cec-compliance. > >> > >> This series looks good to me. But can you do one more test: > >> > >> Update your kernel to the latest media_tree master and also update your > >> v4l-utils repo to the latest master code. > >> > >> With all that in place please run: > >> > >> cec-compliance -A > >> > >> (have the HDMI output connected to a CEC-capable TV when running this test). > >> > >> Please report back the output of cec-compliance. > >> > >> A bunch of CEC bug fixes and improvements were merged yesterday, and the > >> cec-compliance adapter test is improved to check for issues that were hard > >> to find in the past. > >> > >> So it will be good to have a final check of this driver. > > > > Here it is, compiled media-tree and latest v4l-utils: > > > > udoo@udoo-UDOO-x86:~/v4l-utils/utils/cec-compliance$ uname -a > > Linux udoo-UDOO-x86 4.19.0-041900rc7-generic #201810071631+cec SMP Tue Oct 9 17:36:11 CEST 2018 x86_64 x86_64 x86_64 GNU/Linux > > udoo@udoo-UDOO-x86:~/v4l-utils/utils/cec-compliance$ ./cec-compliance -A > > cec-compliance SHA : 06ad469e966aafaf39c1cc76e6e0953ec7d4f9c9 > > Driver Info: > > Driver Name : secocec > > Adapter Name : CEC00001:00 > > Capabilities : 0x0000000e > > Logical Addresses > > Transmit > > Passthrough > > Driver version : 4.19.0 > > Available Logical Addresses: 1 > > Physical Address : 3.0.0.0 > > Logical Address Mask : 0x0010 > > CEC Version : 2.0 > > Vendor ID : 0x000c03 (HDMI) > > OSD Name : Playback > > Logical Addresses : 1 > > > > Logical Address : 4 (Playback Device 1) > > Primary Device Type : Playback > > Logical Address Type : Playback > > All Device Types : Playback > > RC TV Profile : None > > Device Features : > > None > > > > Compliance test for device /dev/cec0: > > > > The test results mean the following: > > OK Supported correctly by the device. > > OK (Not Supported) Not supported and not mandatory for the device. > > OK (Presumed) Presumably supported. Manually check to confirm. > > OK (Unexpected) Supported correctly but is not expected to be supported for this device. > > OK (Refused) Supported by the device, but was refused. > > FAIL Failed and was expected to be supported by this device. > > > > Find remote devices: > > Polling: OK > > > > CEC API: > > CEC_ADAP_G_CAPS: OK > > CEC_DQEVENT: OK > > CEC_ADAP_G/S_PHYS_ADDR: OK > > CEC_ADAP_G/S_LOG_ADDRS: OK > > CEC_TRANSMIT: OK > > CEC_RECEIVE: OK > > CEC_TRANSMIT/RECEIVE (non-blocking): OK (Presumed) > > CEC_G/S_MODE: OK > > fail: cec-test-adapter.cpp(1042): There were 142 pending messages for 83 transmitted messages > > That's not good. > > If you look in the kernel log, do you see 'timed out' cec messages? No, actually I see a lot of 'cec_transmit_msg_fh: transmit queue full' messages. > > Note: that message is a warning since commit 7ec2b3b941a666a942859684281b5f6460a0c234. > Before that you first need to enable debugging: > > echo 1 >/sys/module/cec/parameters/debug > > My guess is that this might be a fw bug. Does the firmware handle Signal Free Time > correctly? My guess is that it doesn't do that and that this test causes what is > effectively a 'denial of service' situation: the transmitter gets blocked waiting > for sufficient signal free time. > > I have updated cec-compliance to give better information about what was received, > so can you update cec-compliance and run again? Yes, I've recompiled the two, in attachment there are both outputs plus the relative kernel messages. > > Also, run 'cec-ctl -m >cec.log' at the same time and mail me that log. > > There are two types of CEC adapters: those that handle retransmits automatically > (and they determine the Signal Free Time themselves) and those that don't do > automatic retransmits, and there you normally need to program the Signal Free Time > before starting the transmit. > > This driver falls in the second category, but the SFT isn't set anywhere. > > This particular adapter test actually tests this, and I have seen this > symptom before if the SFT wasn't set correctly. I didn't know that, there is a place to put that parameter (or there is a default)? There is a way to evaluate a good value for that? Thanks a lot, Ettore > > Regards, > > Hans > > > CEC_EVENT_LOST_MSGS: FAIL > > > > Network topology: > > System Information for device 0 (TV) from device 4 (Playback Device 1): > > CEC Version : 1.4 > > Physical Address : 0.0.0.0 > > Primary Device Type : TV > > Vendor ID : 0x00e091 > > OSD Name : Tx, OK, Rx, Timeout > > Menu Language : kor > > Power Status : On > > > > Total: 10, Succeeded: 9, Failed: 1, Warnings: 0 > > > > Thanks, > > Ettore > > > >> > >> Regards, > >> > >> Hans > >> > >>> > >>> v2: > >>> - Removed useless debug prints > >>> - Added DMI && PCI to dependences > >>> - Removed useless ifdefs > >>> - Renamed all irda references to ir > >>> - Fixed SPDX clause > >>> - Several style fixes > >>> > >>> Ettore Chimenti (2): > >>> media: add SECO cec driver > >>> seco-cec: add Consumer-IR support > >>> > >>> MAINTAINERS | 6 + > >>> drivers/media/platform/Kconfig | 22 + > >>> drivers/media/platform/Makefile | 2 + > >>> drivers/media/platform/seco-cec/Makefile | 1 + > >>> drivers/media/platform/seco-cec/seco-cec.c | 829 +++++++++++++++++++++ > >>> drivers/media/platform/seco-cec/seco-cec.h | 141 ++++ > >>> 6 files changed, 1001 insertions(+) > >>> create mode 100644 drivers/media/platform/seco-cec/Makefile > >>> create mode 100644 drivers/media/platform/seco-cec/seco-cec.c > >>> create mode 100644 drivers/media/platform/seco-cec/seco-cec.h > >>> > >> >