On 02/07/2024 16:22, Maxime Ripard wrote: > Hi, > > On Mon, Jul 01, 2024 at 10:29:55AM GMT, Hans Verkuil wrote: >> Hi Maxime, >> >> On 28/06/2024 10:50, Maxime Ripard wrote: >>> Hi Hans, >>> >>> I've been playing with the unicam driver and the TC358743 HDMI -> CSI >>> bridge recently. >>> >>> The program I was testing it with had a (arguably suboptimal) pattern >>> where it would (in a non-blocking way): >>> >>> In a loop: >>> - set EDID >>> - In a loop: >>> - call query_dv_timings >>> - if we have a timing matching the mode we expect: >>> - break the loop >>> >>> - Call s_dv_timings >>> - Call s_fmt >>> - Call reqbufs >>> - Query and Queue all requested buffers >>> - Call streamon >>> - In a loop: >>> - Dequeue the events >>> - If there's a resolution change: >>> - Call streamoff >>> - Call reqbufs with 0 buffers to clear all buffers >>> - Restart the entire sequence >>> - Dequeue a buffer >>> - Queue it again >>> >>> This works mostly fine, but when trying to capture the boot of a device >>> connected on the other end, I'm always getting at some point an >>> resolution change event in the very first iteration. >>> >>> The event itself looks fine: there's no remaining events at any point, >>> the sequence is correct, etc. However, this puts the s_edid call super >>> close to streamoff and the next s_edid call. >>> >>> And it looks like the tc358743 driver doesn't like that very much and >>> the HPD pin ends up in the wrong state on the next iteration: both the >>> driver itself and the device at the other reports the hotplug pin as >>> being low, and thus, not connected. >>> >>> I'm not entirely sure what is the reason, but I suspect a race in: >>> https://elixir.bootlin.com/linux/v6.9.3/source/drivers/media/i2c/tc358743.c#L403 >>> >>> Possibly due to the 100ms delay? >>> >>> I've attached a kernel log with debug logs from both v4l2 and the driver >>> enabled. The relevant part is here: [ 149.457319] Starting new Test [ 149.460387] tc358743 10-000f: tc358743_s_edid, pad 0, start block 0, blocks 2 [ 149.460396] tc358743 10-000f: tc358743_disable_edid: HPD is pulled low here. [ 149.486259] tc358743 10-000f: tc358743_enable_edid: [ 149.486268] tc358743 10-000f: tc358743_enable_interrupts: cable connected = 1 Here the delayed work is started. [ 149.488760] video0: VIDIOC_S_EDID [ 149.495353] tc358743 10-000f: tc358743_query_dv_timings: 1280x720p60.00 (1650x750) But here the tc358743 accepts a query_dv_timings call, even though the source should have stopped transmitting because the HPD went low. [ 149.502929] video0: VIDIOC_QUERY_DV_TIMINGS ... [ 149.555039] Starting new Test And the new test started within 100 ms of the previous test, so we never saw the tc358743_delayed_work_enable_hotplug call that pulls the HPD high. [ 149.558153] tc358743 10-000f: tc358743_s_edid, pad 0, start block 0, blocks 2 [ 149.558163] tc358743 10-000f: tc358743_disable_edid: HPD is pulled low here. [ 149.584032] tc358743 10-000f: tc358743_enable_edid: [ 149.584041] tc358743 10-000f: tc358743_enable_interrupts: cable connected = 1 [ 149.586526] video0: VIDIOC_S_EDID [ 149.587052] tc358743 10-000f: tc358743_get_detected_timings: no valid signal [ 149.587057] video0: VIDIOC_QUERY_DV_TIMINGS: error -67 [ 149.687340] tc358743 10-000f: tc358743_delayed_work_enable_hotplug: HPD is pulled high here (about 100 ms later). I think the solution might be ensure that tc358743_get_detected_timings() returns -ENOLINK if the HPD is low. So add: if (!(i2c_rd8(sd, HPD_CTL) & MASK_HPD_OUT0)) return -ENOLINK; Regards, Hans >> >> You forgot to attach the logs :-) > > Of course I did :) > > It should be attached this time > >> I don't off-hand see a race condition. > > Yeah, me neither. The code looked sane to me, hence that mail. > >> But there is an important thing to remember: the HPD is only pulled >> high if the 5V line from the source is also high. I.e., if no source >> is detected, then the HPD remains low. >> >> I don't think you state what the source device is, but make sure it >> has 5V high. If it is low, or it toggles the 5V for some reason, then >> that might be related to the problem. But without logs it is hard to >> tell. > > It's a RaspberryPi. I was looking at the register and it doesn't detect > HPD being high, but I'll try to see if I can find a testpoint to read > the level. > > Maxime
