Re: [PATCH 1/5] drm/i915: Fix DDI port_clock for VGA output

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Feb 28, 2014 at 07:09:51PM -0300, Paulo Zanoni wrote:
> 2014-02-27 9:23 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@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@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.

I have no objections to improving the docs.

> Our
> hardware also does a bunch of *2 multiplications on PLLs and clocks,
> and this gets things even more confusing.

The 2x is for PLL output->bitclock, and the /5 would be for
PLL output->symbol clock. So maybe it would be nicer to use
<real PLL frequency>/5 in the DDI get_config. It would match
how we compute things on VLV at least.

> 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@intel.com>
> 
> >         return true;
> >  }
> >
> > --
> > 1.8.3.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC