* [PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state @ 2014-06-15 12:55 Daniel Vetter 2014-06-16 8:32 ` [Intel-gfx] " Ville Syrjälä 0 siblings, 1 reply; 5+ messages in thread From: Daniel Vetter @ 2014-06-15 12:55 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter, Aleks, stable It changes at runtime and so should be ignored for pipe state checks. Note that we don't yet read out the full DRRS state, so there's some gaps. Bu DRRS is also not yet enabled for LVDS by default. Cc: Aleks <aleks@slobodensoftver.org.mk> Reported-by: Aleks <aleks@slobodensoftver.org.mk> Cc: stable@vger.kernel.org Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ba1d9aac3958..1ccf660e67d9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8549,6 +8549,9 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc, else fp = pipe_config->dpll_hw_state.fp1; + /* We don't compute the FPA 0/1 selector. */ + dpll &= ~DISPLAY_RATE_SELECT_FPA1; + clock.m1 = (fp & FP_M1_DIV_MASK) >> FP_M1_DIV_SHIFT; if (IS_PINEVIEW(dev)) { clock.n = ffs((fp & FP_N_PINEVIEW_DIV_MASK) >> FP_N_DIV_SHIFT) - 1; -- 2.0.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state 2014-06-15 12:55 [PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state Daniel Vetter @ 2014-06-16 8:32 ` Ville Syrjälä 2014-06-16 18:01 ` Daniel Vetter 0 siblings, 1 reply; 5+ messages in thread From: Ville Syrjälä @ 2014-06-16 8:32 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development, Aleks, stable On Sun, Jun 15, 2014 at 02:55:42PM +0200, Daniel Vetter wrote: > It changes at runtime and so should be ignored for pipe state checks. > Note that we don't yet read out the full DRRS state, so there's some > gaps. Bu DRRS is also not yet enabled for LVDS by default. > > Cc: Aleks <aleks@slobodensoftver.org.mk> > Reported-by: Aleks <aleks@slobodensoftver.org.mk> > Cc: stable@vger.kernel.org > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index ba1d9aac3958..1ccf660e67d9 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8549,6 +8549,9 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc, > else > fp = pipe_config->dpll_hw_state.fp1; > > + /* We don't compute the FPA 0/1 selector. */ > + dpll &= ~DISPLAY_RATE_SELECT_FPA1; > + But we still compute port_clock based on the currently active FPA register. Won't that make the clock checks fail as well? > clock.m1 = (fp & FP_M1_DIV_MASK) >> FP_M1_DIV_SHIFT; > if (IS_PINEVIEW(dev)) { > clock.n = ffs((fp & FP_N_PINEVIEW_DIV_MASK) >> FP_N_DIV_SHIFT) - 1; > -- > 2.0.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state 2014-06-16 8:32 ` [Intel-gfx] " Ville Syrjälä @ 2014-06-16 18:01 ` Daniel Vetter 2014-06-16 18:20 ` Ville Syrjälä 0 siblings, 1 reply; 5+ messages in thread From: Daniel Vetter @ 2014-06-16 18:01 UTC (permalink / raw) To: Ville Syrjälä Cc: Daniel Vetter, Intel Graphics Development, Aleks, stable On Mon, Jun 16, 2014 at 11:32:39AM +0300, Ville Syrjälä wrote: > On Sun, Jun 15, 2014 at 02:55:42PM +0200, Daniel Vetter wrote: > > It changes at runtime and so should be ignored for pipe state checks. > > Note that we don't yet read out the full DRRS state, so there's some > > gaps. Bu DRRS is also not yet enabled for LVDS by default. > > > > Cc: Aleks <aleks@slobodensoftver.org.mk> > > Reported-by: Aleks <aleks@slobodensoftver.org.mk> > > Cc: stable@vger.kernel.org > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/intel_display.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index ba1d9aac3958..1ccf660e67d9 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -8549,6 +8549,9 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc, > > else > > fp = pipe_config->dpll_hw_state.fp1; > > > > + /* We don't compute the FPA 0/1 selector. */ > > + dpll &= ~DISPLAY_RATE_SELECT_FPA1; > > + > > But we still compute port_clock based on the currently active FPA > register. Won't that make the clock checks fail as well? Well yeah, but that should get solved as part of the DRRS stuff I think. Imo ignore the frequency selector for the dpll state is the right thing. DRRS with state readout is still in-flux and unsolved wrt fastbooting. Should I add a caveat to the commit message that this isn't everything or not worth it as-is? -Daniel > > > clock.m1 = (fp & FP_M1_DIV_MASK) >> FP_M1_DIV_SHIFT; > > if (IS_PINEVIEW(dev)) { > > clock.n = ffs((fp & FP_N_PINEVIEW_DIV_MASK) >> FP_N_DIV_SHIFT) - 1; > > -- > > 2.0.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state 2014-06-16 18:01 ` Daniel Vetter @ 2014-06-16 18:20 ` Ville Syrjälä 2014-06-16 18:45 ` [Intel-gfx] " Daniel Vetter 0 siblings, 1 reply; 5+ messages in thread From: Ville Syrjälä @ 2014-06-16 18:20 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, Aleks, stable On Mon, Jun 16, 2014 at 08:01:26PM +0200, Daniel Vetter wrote: > On Mon, Jun 16, 2014 at 11:32:39AM +0300, Ville Syrjälä wrote: > > On Sun, Jun 15, 2014 at 02:55:42PM +0200, Daniel Vetter wrote: > > > It changes at runtime and so should be ignored for pipe state checks. > > > Note that we don't yet read out the full DRRS state, so there's some > > > gaps. Bu DRRS is also not yet enabled for LVDS by default. > > > > > > Cc: Aleks <aleks@slobodensoftver.org.mk> > > > Reported-by: Aleks <aleks@slobodensoftver.org.mk> > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index ba1d9aac3958..1ccf660e67d9 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -8549,6 +8549,9 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc, > > > else > > > fp = pipe_config->dpll_hw_state.fp1; > > > > > > + /* We don't compute the FPA 0/1 selector. */ > > > + dpll &= ~DISPLAY_RATE_SELECT_FPA1; > > > + > > > > But we still compute port_clock based on the currently active FPA > > register. Won't that make the clock checks fail as well? > > Well yeah, but that should get solved as part of the DRRS stuff I think. > Imo ignore the frequency selector for the dpll state is the right thing. > > DRRS with state readout is still in-flux and unsolved wrt fastbooting. > > Should I add a caveat to the commit message that this isn't everything or > not worth it as-is? Well, after a better look I see that this patch does absolutely nothing. You already picked the FPA register before you cleared the select bit. So you need to clear it a bit earlier in this function, or even go as far as clearing it when we read out the dpll state. Maybe the latter is better in case we want to start checking the entire dpll state? -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state 2014-06-16 18:20 ` Ville Syrjälä @ 2014-06-16 18:45 ` Daniel Vetter 0 siblings, 0 replies; 5+ messages in thread From: Daniel Vetter @ 2014-06-16 18:45 UTC (permalink / raw) To: Ville Syrjälä Cc: Daniel Vetter, Daniel Vetter, Intel Graphics Development, Aleks, stable On Mon, Jun 16, 2014 at 09:20:35PM +0300, Ville Syrjälä wrote: > On Mon, Jun 16, 2014 at 08:01:26PM +0200, Daniel Vetter wrote: > > On Mon, Jun 16, 2014 at 11:32:39AM +0300, Ville Syrjälä wrote: > > > On Sun, Jun 15, 2014 at 02:55:42PM +0200, Daniel Vetter wrote: > > > > It changes at runtime and so should be ignored for pipe state checks. > > > > Note that we don't yet read out the full DRRS state, so there's some > > > > gaps. Bu DRRS is also not yet enabled for LVDS by default. > > > > > > > > Cc: Aleks <aleks@slobodensoftver.org.mk> > > > > Reported-by: Aleks <aleks@slobodensoftver.org.mk> > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index ba1d9aac3958..1ccf660e67d9 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -8549,6 +8549,9 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc, > > > > else > > > > fp = pipe_config->dpll_hw_state.fp1; > > > > > > > > + /* We don't compute the FPA 0/1 selector. */ > > > > + dpll &= ~DISPLAY_RATE_SELECT_FPA1; > > > > + > > > > > > But we still compute port_clock based on the currently active FPA > > > register. Won't that make the clock checks fail as well? > > > > Well yeah, but that should get solved as part of the DRRS stuff I think. > > Imo ignore the frequency selector for the dpll state is the right thing. > > > > DRRS with state readout is still in-flux and unsolved wrt fastbooting. > > > > Should I add a caveat to the commit message that this isn't everything or > > not worth it as-is? > > Well, after a better look I see that this patch does absolutely nothing. > You already picked the FPA register before you cleared the select bit. > So you need to clear it a bit earlier in this function, or even go as > far as clearing it when we read out the dpll state. Maybe the latter > is better in case we want to start checking the entire dpll state? Hm, I guess I hide in shame and will leave this topic to people with clue ;-) Thanks for looking at this anyway. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-06-16 18:45 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-15 12:55 [PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state Daniel Vetter 2014-06-16 8:32 ` [Intel-gfx] " Ville Syrjälä 2014-06-16 18:01 ` Daniel Vetter 2014-06-16 18:20 ` Ville Syrjälä 2014-06-16 18:45 ` [Intel-gfx] " Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.