| Message ID | 1344446134-3704-8-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > 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. BR, Jani. > + if (i == ARRAY_SIZE(wrpll_tmds_clock_table)) > + i--; > > - valid = 1; > - break; > - } > - } > + if (wrpll_tmds_clock_table[i].clock != crtc->mode.clock) > + DRM_INFO("WR PLL: using settings for %dKHz on %dKHz mode\n", > + wrpll_tmds_clock_table[i].clock, crtc->mode.clock); > > - if (!valid) { > - DRM_ERROR("Unable to find WR PLL clock settings for %dKHz refresh rate\n", > - crtc->mode.clock); > - return; > - } > + p = wrpll_tmds_clock_table[i].p; > + n2 = wrpll_tmds_clock_table[i].n2; > + r2 = wrpll_tmds_clock_table[i].r2; > > /* Enable LCPLL if disabled */ > temp = I915_READ(LCPLL_CTL); > -- > 1.7.11.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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. > > BR, > Jani. > >> + if (i == ARRAY_SIZE(wrpll_tmds_clock_table)) >> + i--; >> >> - valid = 1; >> - break; >> - } >> - } >> + if (wrpll_tmds_clock_table[i].clock != crtc->mode.clock) >> + DRM_INFO("WR PLL: using settings for %dKHz on %dKHz mode\n", >> + wrpll_tmds_clock_table[i].clock, crtc->mode.clock); >> >> - if (!valid) { >> - DRM_ERROR("Unable to find WR PLL clock settings for %dKHz refresh rate\n", >> - crtc->mode.clock); >> - return; >> - } >> + p = wrpll_tmds_clock_table[i].p; >> + n2 = wrpll_tmds_clock_table[i].n2; >> + r2 = wrpll_tmds_clock_table[i].r2; >> >> /* Enable LCPLL if disabled */ >> temp = I915_READ(LCPLL_CTL); >> -- >> 1.7.11.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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
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. */ 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); + if (i == ARRAY_SIZE(wrpll_tmds_clock_table)) + i--; - valid = 1; - break; - } - } + if (wrpll_tmds_clock_table[i].clock != crtc->mode.clock) + DRM_INFO("WR PLL: using settings for %dKHz on %dKHz mode\n", + wrpll_tmds_clock_table[i].clock, crtc->mode.clock); - if (!valid) { - DRM_ERROR("Unable to find WR PLL clock settings for %dKHz refresh rate\n", - crtc->mode.clock); - return; - } + p = wrpll_tmds_clock_table[i].p; + n2 = wrpll_tmds_clock_table[i].n2; + r2 = wrpll_tmds_clock_table[i].r2; /* Enable LCPLL if disabled */ temp = I915_READ(LCPLL_CTL);