On Thu, Oct 11, 2012 at 05:36:35PM -0600, Stephen Warren wrote:
> On 10/11/2012 02:07 PM, Thierry Reding wrote:
> > Hi,
> >
> > I've finally managed to get HDMI working on Tegra20. Unfortunately
> > the xf86-video-modesetting driver doesn't work on top of it yet,
> > but I think that's a different error somewhere else and I'm still
> > trying to figure out what exactly is going wrong. However, a
> > framebuffer console can be run on top of it, as does a small test
> > program that I've written for testing.
>
> I should start out by saying that this is all very excellent news!
> Thanks so much for your hard work on tegra-drm.
>
> I have managed to get Harmony HDMI working, mostly just by copying the
> obvious device tree entries from tegra20-trimslice.dts. This didn't
> quite work immediately though, and while debugging this I found a few
> issues...
>
> (most files I mention below are in drivers/gpu/drm/tegra/)
>
> 1)
>
> clock.c:tegra_clock_setup() calls clk_set_rate() on the PLL. However,
> there is no error-checking, so any failure is completely silent and
> simply results in the display not working without much clue as to why.
> In my case, my monitor's preferred mode was 1920x1080 with a 154MHz
> pixel clock, which required a 616MHz clock for pll_d. However,
> mach-tegra/tegra20_clocks_data.c didn't have an entry for that rate,
> so clk_set_rate() failed.
I'm not entirely happy with how this is currently implemented. One thing
I was going to experiment with now that I have a working reference is to
move clock setup into the respective outputs in order to get rid of the
various if branches. This may turn out to not be much better, but it
will allow the clock setup to be more closely located to the actual user
of the code and therefore increase maintainability.
Even if that doesn't work out, error checking is still a must.
> For reference, the following entry in tegra_pll_d_freq_table[] solves
> this for me:
>
> { 12000000, 616000000, 616, 12, 1, 8},
>
> (although I have no idea if the "cpcon" field there is correct)
>
> I imagine that output.c:tegra_connector_mode_valid() should be
> querying the clock API to determine whether the desired mode clock is
> actually possible, and rejecting the mode if not. In my case, I assume
> that would have caused the code to fall back to some other mode, and
> hopefully found one that did (my monitor apparently lists 51 modes in
> the EDID after all).
Yes, that's definitely something we should cleanup. If the output can
determine that the clock can't be set to the rate required by the mode
it should mark the mode as invalide.
> 2)
>
> The code in clock.c uses lots of clk_get_sys() calls with hard-coded
> clock names. We really should be using the common clock DT bindings
> for this instead of hard-coding names. This is especially true since
> the names differ on different SoCs, so there's a ton of
> of_device_is_compatible(output->dev->of_node, "nvidia,tegra30-hdmi")
> in this code.
I don't quite see how that is supposed to work. Wouldn't that mean that
we needed to setup various entries in the clock tables to be able to
look the clocks up by generic names? Like pll_d would need to be the
"parent" clock of "tegra-hdmi" or similar. In that case we could call
clk_get(hdmi->dev, "parent") instead. Is that what you had in mind?
> 3)
>
> It sucks that our clock driver can't just calculate the PLL m and n
> values; without this, we will end up with an almost infinitely large
> tegra_pll_d_freq_table[]. I'll try and find out whether this can be fixed.
If this can be calculated it is definitely the preferred way.
Thierry
Attachment:
pgp1EzLmzwzWS.pgp
Description: PGP signature
