From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 1/5] drm/i915: Fix DDI port_clock for VGA output Date: Mon, 3 Mar 2014 12:09:55 +0200 Message-ID: <20140303100955.GW3852@intel.com> References: <1393503796-31342-1-git-send-email-ville.syrjala@linux.intel.com> <1393503796-31342-2-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id B0915FB057 for ; Mon, 3 Mar 2014 02:10:26 -0800 (PST) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Paulo Zanoni Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Fri, Feb 28, 2014 at 07:09:51PM -0300, Paulo Zanoni wrote: > 2014-02-27 9:23 GMT-03:00 : > > From: Ville Syrj=E4l=E4 > > > > 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=3D74955 > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > 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/in= tel_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 =3D 24; > > > > + /* FDI must always be 2.7 GHz */ > > + if (HAS_DDI(dev)) > > + pipe_config->port_clock =3D 135000 * 2; > > + > = > > 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 /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? > > = > Still, I have reviewed your patch, and despite all my own confusions, > it looks correct, so: > Reviewed-by: Paulo Zanoni > = > > 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=E4l=E4 Intel OTC