| Message ID | 1352118507-6933-3-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi 2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>: > ... with is_dual_link_lvds introduced in > > commit b03543857fd75876b96e10d4320b775e95041bb7 > Author: Takashi Iwai <tiwai@suse.de> > Date: Tue Mar 20 13:07:05 2012 +0100 > > drm/i915: Check VBIOS value for determining LVDS dual channel mode, too > > All these checks predate this commit and have simply been overlooked. > Since we don't support switching between single-link and dual-link > modes anyway, this different checks could at best only get in the way > of refactorings, and in the worst case cause inconsistencies. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 1ad6d34..0973391 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -690,8 +690,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc, > intel_clock_t clock; > int err = target; > > - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) && > - (I915_READ(LVDS)) != 0) { > + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) { This chunk doesn't seem to do exactly what the commit message says. I tried to git-blame the lines and got a little confused. Maybe this chunk deserves its own commit with an explanation. Maybe what you really want to do is to revert commit 832cc28d5bc676331e6376d940ae45d5937aa688 instead of removing the line? If you really want to remove the line, you may also update the comment immediately below? The chunks below look correct. > /* > * For LVDS, if the panel is on, just rely on its current > * settings for dual-channel. We haven't figured out how to > @@ -766,8 +765,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc, > lvds_reg = PCH_LVDS; > else > lvds_reg = LVDS; > - if ((I915_READ(lvds_reg) & LVDS_CLKB_POWER_MASK) == > - LVDS_CLKB_POWER_UP) > + if (is_dual_link_lvds(dev_priv, lvds_reg)) > clock.p2 = limit->p2.p2_fast; > else > clock.p2 = limit->p2.p2_slow; > @@ -5360,7 +5358,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, > if (is_lvds) { > if ((intel_panel_use_ssc(dev_priv) && > dev_priv->lvds_ssc_freq == 100) || > - (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP) > + is_dual_link_lvds(dev_priv, PCH_LVDS)) > factor = 25; > } else if (is_sdvo && is_tv) > factor = 20; > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Nov 16, 2012 at 5:37 PM, Paulo Zanoni <przanoni@gmail.com> wrote: >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 1ad6d34..0973391 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -690,8 +690,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc, >> intel_clock_t clock; >> int err = target; >> >> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) && >> - (I915_READ(LVDS)) != 0) { >> + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) { > > This chunk doesn't seem to do exactly what the commit message says. I > tried to git-blame the lines and got a little confused. Maybe this > chunk deserves its own commit with an explanation. Maybe what you > really want to do is to revert commit > 832cc28d5bc676331e6376d940ae45d5937aa688 instead of removing the line? > If you really want to remove the line, you may also update the comment > immediately below? Afaict the LVDS register check is only to make sure that we don't read garbage. Iow the code doesn't handle the more robust dual-link mode checking introduced in b03543857fd75876b96e10d4320b77 If we switch over to that method to check for dual-link, we also don't need to do the (rather bogus imo) register check and hence can just drop it. I've dug around in the commit history, and the last patch to touch this code predates b03543857fd75876b, hence why I think we should just drop the register check and use the new is-dual-link function, assuming that this has simple been overlooked in the conversion. Does that make sense? -Daniel > > The chunks below look correct. > >> /* >> * For LVDS, if the panel is on, just rely on its current >> * settings for dual-channel. We haven't figured out how to >> @@ -766,8 +765,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc, >> lvds_reg = PCH_LVDS; >> else >> lvds_reg = LVDS; >> - if ((I915_READ(lvds_reg) & LVDS_CLKB_POWER_MASK) == >> - LVDS_CLKB_POWER_UP) >> + if (is_dual_link_lvds(dev_priv, lvds_reg)) >> clock.p2 = limit->p2.p2_fast;
Hi 2012/11/16 Daniel Vetter <daniel.vetter@ffwll.ch>: > On Fri, Nov 16, 2012 at 5:37 PM, Paulo Zanoni <przanoni@gmail.com> wrote: >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>> index 1ad6d34..0973391 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -690,8 +690,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc, >>> intel_clock_t clock; >>> int err = target; >>> >>> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) && >>> - (I915_READ(LVDS)) != 0) { >>> + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) { >> >> This chunk doesn't seem to do exactly what the commit message says. I >> tried to git-blame the lines and got a little confused. Maybe this >> chunk deserves its own commit with an explanation. Maybe what you >> really want to do is to revert commit >> 832cc28d5bc676331e6376d940ae45d5937aa688 instead of removing the line? >> If you really want to remove the line, you may also update the comment >> immediately below? > > Afaict the LVDS register check is only to make sure that we don't read > garbage. Iow the code doesn't handle the more robust dual-link mode > checking introduced in b03543857fd75876b96e10d4320b77 > > If we switch over to that method to check for dual-link, we also don't > need to do the (rather bogus imo) register check and hence can just > drop it. > > I've dug around in the commit history, and the last patch to touch > this code predates b03543857fd75876b, hence why I think we should just > drop the register check and use the new is-dual-link function, > assuming that this has simple been overlooked in the conversion. Well, take a look at patch 832cc28d5bc676331e6376d940ae45d5937aa688 and then read the 4-line comment that's inside the "if" statement. If we're going to completely remove the I915_READ line, shouldn't we also update the comment ("if the panel is on") ? > > Does that make sense? > -Daniel > > >> >> The chunks below look correct. >> >>> /* >>> * For LVDS, if the panel is on, just rely on its current >>> * settings for dual-channel. We haven't figured out how to >>> @@ -766,8 +765,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc, >>> lvds_reg = PCH_LVDS; >>> else >>> lvds_reg = LVDS; >>> - if ((I915_READ(lvds_reg) & LVDS_CLKB_POWER_MASK) == >>> - LVDS_CLKB_POWER_UP) >>> + if (is_dual_link_lvds(dev_priv, lvds_reg)) >>> clock.p2 = limit->p2.p2_fast; > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Fri, Nov 16, 2012 at 6:07 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > Well, take a look at patch 832cc28d5bc676331e6376d940ae45d5937aa688 > and then read the 4-line comment that's inside the "if" statement. If > we're going to completely remove the I915_READ line, shouldn't we also > update the comment ("if the panel is on") ? Yeah, that comment needs to be updated. And that commit message is wrong, since there are clearly bios versions out there which do not unconditionally set the dual-link bits in the lvds reg. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1ad6d34..0973391 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -690,8 +690,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc, intel_clock_t clock; int err = target; - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) && - (I915_READ(LVDS)) != 0) { + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) { /* * For LVDS, if the panel is on, just rely on its current * settings for dual-channel. We haven't figured out how to @@ -766,8 +765,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc, lvds_reg = PCH_LVDS; else lvds_reg = LVDS; - if ((I915_READ(lvds_reg) & LVDS_CLKB_POWER_MASK) == - LVDS_CLKB_POWER_UP) + if (is_dual_link_lvds(dev_priv, lvds_reg)) clock.p2 = limit->p2.p2_fast; else clock.p2 = limit->p2.p2_slow; @@ -5360,7 +5358,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, if (is_lvds) { if ((intel_panel_use_ssc(dev_priv) && dev_priv->lvds_ssc_freq == 100) || - (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP) + is_dual_link_lvds(dev_priv, PCH_LVDS)) factor = 25; } else if (is_sdvo && is_tv) factor = 20;
... with is_dual_link_lvds introduced in commit b03543857fd75876b96e10d4320b775e95041bb7 Author: Takashi Iwai <tiwai@suse.de> Date: Tue Mar 20 13:07:05 2012 +0100 drm/i915: Check VBIOS value for determining LVDS dual channel mode, too All these checks predate this commit and have simply been overlooked. Since we don't support switching between single-link and dual-link modes anyway, this different checks could at best only get in the way of refactorings, and in the worst case cause inconsistencies. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)