[PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state

[PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state

[Daniel Vetter]

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

Re: [Intel-gfx] [PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state

[Daniel Vetter]

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

Re: [PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state

[Ville Syrjälä]

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

Re: [Intel-gfx] [PATCH] drm/i915: Don't take fp 0/1 selector into account for pll state

[Daniel Vetter]

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