[Intel-gfx] [PATCH 1/5] drm/i915: Fix DDI port_clock for VGA output

[Paulo Zanoni]

2014-02-27 9:23 GMT-03:00  <ville.syrjala at linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> On DDI there's no PLL as such to generate the pixel clock for VGA.
> Instead we derive the pixel clock from the FDI link frequency. So
> to make .compute_config match what .get_config does, we need to
> set the port_clock based on the FDI link frequency.
>
> Note that we don't even check the port_clock when selecting the
> PLL for VGA output. We just assume SPLL at 1.35GHz is what we want,
> and that does match with the asumption of FDI frequency of 2.7Ghz
> we have in intel_fdi_link_freq().
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74955
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 9864aa1..02e2b02 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -262,6 +262,10 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder,
>         if (HAS_PCH_LPT(dev))
>                 pipe_config->pipe_bpp = 24;
>
> +       /* FDI must always be 2.7 GHz */
> +       if (HAS_DDI(dev))
> +               pipe_config->port_clock = 135000 * 2;
> +

<rant>
Whenever I have to review patches touching the HW state
readout/compute I get completely confused. We have a bunch of
compute_config/get_config/get_hw_state functions, with no real
documentation of which fields exactly each function should get. Our
hardware also does a bunch of *2 multiplications on PLLs and clocks,
and this gets things even more confusing. Also, we have similar
information (like all these "clocks", from mode, adjusted_mode, port,
pipe, hw, etc) with slightly different meanings. I really wish we had
Kernel documentation describing *exactly* what is expected to be
computed from each function, and what are the differences between all
those similar things with slightly different meanings. Maybe we could
also organize struct intel_crtc_config, adding small sub-structs that
better define the domain of each field?
</rant>

Still, I have reviewed your patch, and despite all my own confusions,
it looks correct, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

>         return true;
>  }
>
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni