[linux-sunxi] Re: HDMI/DVI spurious failure

[Jernej Škrabec]

Dne ponedeljek, 21. januar 2019 ob 16:07:28 CET je Priit Laes napisal(a):
> On Mon, Jan 21, 2019 at 02:25:17PM +0100, Maxime Ripard wrote:
> > On Fri, Jan 18, 2019 at 02:51:26PM +0000, Priit Laes wrote:
> > > On Fri, Jan 18, 2019 at 03:04:18PM +0100, Maxime Ripard wrote:
> > > > On Fri, Jan 18, 2019 at 10:10:53AM +0000, Priit Laes wrote:
> > > > > > > > > > It doesn't look related to the clock rate itself, since it
> > > > > > > > > > doesn't
> > > > > > > > > > change between the two cases. However, in one case the DDC
> > > > > > > > > > clock is
> > > > > > > > > > enabled and in the other it's disabled.
> > > > > > > > > > 
> > > > > > > > > > Was it taken at the same time? Maybe you can try with that
> > > > > > > > > > patch?
> > > > > > > > > > http://code.bulix.org/z7jmkm-555344?raw
> > > > > > > > > 
> > > > > > > > > Thanks, after doing ~50+ boots I haven't seen a single
> > > > > > > > > failure.
> > > > > > > > > 
> > > > > > > > > Previously I had following failure cases which are now both
> > > > > > > > > fixed:
> > > > > > > > > 
> > > > > > > > > a) Linux without u-boot HDMI, where one in every 6-7 boots
> > > > > > > > > failed.
> > > > > > > > > b) u--boot with hdmi enabled switching to simplefb and then
> > > > > > > > > switching
> > > > > > > > > to kms, where previously all boots ended up with garbled
> > > > > > > > > screen.
> > > > > > > > 
> > > > > > > > So it's not really a fix, but it really looks like the clock
> > > > > > > > is not
> > > > > > > > enabled when it should.
> > > > > > > > 
> > > > > > > > Can you describe your test scenario a bit more? What are you
> > > > > > > > doing
> > > > > > > > exactly, just booting? When do you start using the display?
> > > > > > > > When did
> > > > > > > > you capture the debugfs output that you pasted?
> > > > > > > 
> > > > > > > Display is already connected via HDMI to the board. I don't
> > > > > > > really
> > > > > > > remove it, I just boot the device and let it start Xorg.
> > > > > > > Meanwhile I just ssh into the device and capture debugfs output.
> > > > > > > See my 3 testing scenarios below.
> > > > > > > 
> > > > > > > Kernel also includes one extra patch to fall back to DDC, in
> > > > > > > case HPD
> > > > > > > fails. Mostly the same I already submitted last November [1].
> > > > > > 
> > > > > > Do you have the same issue without that patch?
> > > > > 
> > > > > Can't really test this display without this patch and I do not have
> > > > > other
> > > > > HDMI/DVI screens. And this issue does not happen with other HDMI
> > > > > displays
> > > > > that I have here.
> > > > 
> > > > Can't you just force the monitor to be reported as present? It's not
> > > > great and we don't want to merge it, but that would allow you to test
> > > > that setup without too many interferences.
> > > 
> > > Baseline is clean u-boot / linux. U-boot does not detect/enable display.
> > > 
> > > 1) Booting Linux with drm.debug=0xe
> > > 
> > > * Linux does not detect/enable display
> > > 
> > > 2) Booting with drm.debug=0xe video=HDMI-A-1:640x480 at 60e
> > > 
> > > * Linux detects display, but display is garbled, and proper edid data is
> > > detected:
> > > 
> > > [snip]
> > > pll-video1                     0        0        0   327000000         
> > > 0     0  50000> > 
> > >    pll-video1-2x               0        0        0   654000000         
> > >    0     0  50000> >    
> > >       hdmi-tmds                0        0        0    25153846         
> > >       0     0  50000> >       
> > >          hdmi-ddc              0        0        0       89835         
> > >          0     0  50000> > 
> > > [/snip]
> > > 
> > > 3) Booting with drm.debug=0xe video=HDMI-A-1:640x480 at 60e
> > > And also one extra patch for Linux where HDMI DDC clock marked as
> > > critical
> > > 
> > > Linux detects and initializes display properly:
> > > [snip]
> > > pll-video1                     1        1        0   327000000         
> > > 0     0  50000> > 
> > >    pll-video1-2x               1        1        0   654000000         
> > >    0     0  50000> >    
> > >       hdmi-tmds                1        1        0    25153846         
> > >       0     0  50000> >       
> > >          hdmi-ddc              1        1        0       89835         
> > >          0     0  50000> > 
> > > [/snip]
> > 
> > I guess you'll need to track down when the hdmi-tmds and hdmi-ddc are
> > enabled and disabled, and if it makes sense :/
> 
> OK, figured out the cause.
> 
> Apparently, for each ddc poll we enable ddc clock which is a child of TMDS
> clock. After transfer is done, we disable the clock and this also tears down
> the parent because its only user has gone missing.. :(
> 
> 
> So basically, patch below also works, but I guess we should override
> the sun4i_tmds_ops.disable to properly account for tmds clock use.
> 

As far as I can see, nobody called clk_prepare_enable() on tmds clock. I had 
similar issue with TMDS clock for H3, since I based code on sun4i tmds clk 
driver. Although clock doesn't have enable bit, it still has to be done to 
have proper reference count and to prevent issues like this.

BR,
Jernej

> ---
> @@ -225,7 +235,7 @@ int sun4i_tmds_create(struct sun4i_hdmi *hdmi)
>         init.ops = &sun4i_tmds_ops;
>         init.parent_names = parents;
>         init.num_parents = 2;
> -       init.flags = CLK_SET_RATE_PARENT;
> +       init.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL;
> 
>         tmds->hdmi = hdmi;
>         tmds->hw.init = &init;
> 
> > Maxime