Re: [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Aug 09, 2012 at 02:30:21PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2012/8/9 Jani Nikula <jani.nikula@linux.intel.com>:
> > On Wed, 08 Aug 2012, Paulo Zanoni <przanoni@gmail.com> wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> If we don't find the exact refresh rate, go with the next one. This
> >> makes some modes work for me. They won't have the best settings, but
> >> will at least have something. Just returning from this function when
> >> we don't find the perfect settings does not help us at all.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_ddi.c | 33 ++++++++++++++-------------------
> >>  1 file changed, 14 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index ff03a3a..db242cf 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -267,7 +267,8 @@ struct wrpll_tmds_clock {
> >>       u16 r2;         /* Reference divider */
> >>  };
> >>
> >> -/* Table of matching values for WRPLL clocks programming for each frequency */
> >> +/* Table of matching values for WRPLL clocks programming for each frequency.
> >> + * The code assumes this table is sorted. */
> >
> > I spotted some duplicate lines in the table. Perhaps you could remove
> > them while at it?
> 
> Good catch. I will write a V2 removing the 3 duplicated lines.
> 
> >
> >>  static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
> >>       {19750, 38,     25,     18},
> >>       {20000, 48,     32,     18},
> >> @@ -658,7 +659,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> >>       struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> >>       int port = intel_hdmi->ddi_port;
> >>       int pipe = intel_crtc->pipe;
> >> -     int p, n2, r2, valid=0;
> >> +     int p, n2, r2;
> >>       u32 temp, i;
> >>
> >>       /* On Haswell, we need to enable the clocks and prepare DDI function to
> >> @@ -666,26 +667,20 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
> >>        */
> >>       DRM_DEBUG_KMS("Preparing HDMI DDI mode for Haswell on port %c, pipe %c\n", port_name(port), pipe_name(pipe));
> >>
> >> -     for (i=0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++) {
> >> -             if (crtc->mode.clock == wrpll_tmds_clock_table[i].clock) {
> >> -                     p = wrpll_tmds_clock_table[i].p;
> >> -                     n2 = wrpll_tmds_clock_table[i].n2;
> >> -                     r2 = wrpll_tmds_clock_table[i].r2;
> >> +     for (i = 0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++)
> >> +             if (crtc->mode.clock <= wrpll_tmds_clock_table[i].clock)
> >> +                     break;
> >>
> >> -                     DRM_DEBUG_KMS("WR PLL clock: found settings for %dKHz refresh rate: p=%d, n2=%d, r2=%d\n",
> >> -                                     crtc->mode.clock,
> >> -                                     p, n2, r2);
> >
> > You drop this debug message. Is it intentional? The below DRM_INFO will
> > only be printed if an exact match is not found.
> 
> Yes. It had way more than 80 columns so I started feeling extremely
> uncomfortable while reading the code and I didn't know why, until I
> realized it :) </joke>
> 
> The thing is that if we actually find the mode on the table it means
> we're on the "happy case", so we don't really need to pollute dmesg
> even more. The refresh rate is print by drm_mode_debug_printmodeline
> (or by the DRM_INFO in the unhappy case) and in case you really need
> to know the extremely meaningful p, n2 and r2 values you can always
> check the code. If we really need this I can always add it back... But
> leaving only the "bad case" for dmesg makes it easier to spot while
> reading the tons of messages we print.

For tricky code that has different ways to get to a working state (or
different reasons to fail) I like it when every case has a debug output.
Since users tend to report bugs with all kinds of funny kernels, it's
easier to be sure what's going on if the dmesg contains a log entry to
confirm things. Maybe differentiate the two with "found exact settings"
and "using approximate mode" ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48