On Thu, Aug 09, 2012 at 02:30:21PM -0300, Paulo Zanoni wrote:
> Hi
>
> 2012/8/9 Jani Nikula <jani.nikula at linux.intel.com>:
> > On Wed, 08 Aug 2012, Paulo Zanoni <przanoni at gmail.com> wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni at 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 at 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 at ffwll.ch
Mobile: +41 (0)79 365 57 48
