Re: [Intel-gfx] [PATCH] drm/i915/bxt: Fix HDMI DPLL configuration

[Daniel Vetter]

On Mon, Sep 26, 2016 at 06:02:20PM +0300, Jani Nikula wrote:
> On Mon, 26 Sep 2016, Imre Deak  wrote:
> > a277ca7dc01d should've been a no-functional-change commit, but it
> > removed the initialization of the dpll_hw_state for HDMI outputs,
> > resulting in state mismatches and a failed modeset with blank
> > screen. Fix this by reinstating the dpll_hw_state initialization.
> >
> > Fixes: a277ca7dc01d ("drm/i915: Split bxt_ddi_pll_select()")
> > Signed-off-by: Imre Deak 
> 
> Cc: Manasi Navare 
> Cc: Ander Conselvan de Oliveira 
> Cc: Durgadoss R 
> Cc: Rodrigo Vivi 

Aka pls update your dim copy, it'll generate this automatically.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 21 -
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c 
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index c26d18a..e8bf838 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -1694,21 +1694,32 @@ bool bxt_ddi_dp_set_dpll_hw_state(int clock,
> > return bxt_ddi_set_dpll_hw_state(clock, _div, dpll_hw_state);
> >  }
> >  
> > +bool bxt_ddi_hdmi_set_dpll_hw_state(struct intel_crtc *intel_crtc,
> > +   struct intel_crtc_state *crtc_state,
> > +   int clock,
> > +   struct intel_dpll_hw_state *dpll_hw_state)
> > +{
> > +   struct bxt_clk_div clk_div = { };
> > +
> > +   bxt_ddi_hdmi_pll_dividers(intel_crtc, crtc_state, clock, _div);
> > +
> > +   return bxt_ddi_set_dpll_hw_state(clock, _div, dpll_hw_state);
> > +}
> > +
> >  static struct intel_shared_dpll *
> >  bxt_get_dpll(struct intel_crtc *crtc,
> > struct intel_crtc_state *crtc_state,
> > struct intel_encoder *encoder)
> >  {
> > -   struct bxt_clk_div clk_div = {0};
> > -   struct intel_dpll_hw_state dpll_hw_state = {0};
> > +   struct intel_dpll_hw_state dpll_hw_state = { };
> > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > struct intel_digital_port *intel_dig_port;
> > struct intel_shared_dpll *pll;
> > int i, clock = crtc_state->port_clock;
> >  
> > -   if (encoder->type == INTEL_OUTPUT_HDMI
> > -   && !bxt_ddi_hdmi_pll_dividers(crtc, crtc_state,
> > - clock, _div))
> > +   if (encoder->type == INTEL_OUTPUT_HDMI &&
> > +   !bxt_ddi_hdmi_set_dpll_hw_state(crtc, crtc_state, clock,
> > +   _hw_state))
> > return NULL;
> >  
> > if ((encoder->type == INTEL_OUTPUT_DP ||
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/bxt: Fix HDMI DPLL configuration

[Imre Deak]

On ti, 2016-09-27 at 16:04 +0300, Ander Conselvan De Oliveira wrote:
> On Tue, 2016-09-27 at 14:26 +0300, Imre Deak wrote:
> > On ti, 2016-09-27 at 11:03 +0300, Ander Conselvan De Oliveira
> > wrote:
> > > 
> > > On Mon, 2016-09-26 at 17:54 +0300, Imre Deak wrote:
> > > > 
> > > > a277ca7dc01d should've been a no-functional-change commit, but
> > > > it
> > > > removed the initialization of the dpll_hw_state for HDMI
> > > > outputs,
> > > > resulting in state mismatches and a failed modeset with blank
> > > > screen. Fix this by reinstating the dpll_hw_state
> > > > initialization.
> > > > 
> > > > Fixes: a277ca7dc01d ("drm/i915: Split bxt_ddi_pll_select()")
> > > > Signed-off-by: Imre Deak 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 21 ---
> > > > --
> > > >  1 file changed, 16 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > index c26d18a..e8bf838 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > @@ -1694,21 +1694,32 @@ bool bxt_ddi_dp_set_dpll_hw_state(int
> > > > clock,
> > > >     return bxt_ddi_set_dpll_hw_state(clock, _div,
> > > > dpll_hw_state);
> > > >  }
> > > >  
> > > > +bool bxt_ddi_hdmi_set_dpll_hw_state(struct intel_crtc
> > > > *intel_crtc,
> > > > +   struct intel_crtc_state
> > > > *crtc_state,
> > > > +   int clock,
> > > > +   struct intel_dpll_hw_state
> > > > *dpll_hw_state)
> > > > +{
> > > > +   struct bxt_clk_div clk_div = { };
> > > > +
> > > > +   bxt_ddi_hdmi_pll_dividers(intel_crtc, crtc_state,
> > > > clock,
> > > > _div);
> > > > +
> > > > +   return bxt_ddi_set_dpll_hw_state(clock, _div,
> > > > dpll_hw_state);
> > > > +}
> > > > +
> > > >  static struct intel_shared_dpll *
> > > >  bxt_get_dpll(struct intel_crtc *crtc,
> > > >     struct intel_crtc_state *crtc_state,
> > > >     struct intel_encoder *encoder)
> > > >  {
> > > > -   struct bxt_clk_div clk_div = {0};
> > > > -   struct intel_dpll_hw_state dpll_hw_state = {0};
> > > > +   struct intel_dpll_hw_state dpll_hw_state = { };
> > > >     struct drm_i915_private *dev_priv = to_i915(crtc-
> > > > > 
> > > > > base.dev);
> > > >     struct intel_digital_port *intel_dig_port;
> > > >     struct intel_shared_dpll *pll;
> > > >     int i, clock = crtc_state->port_clock;
> > > >  
> > > > -   if (encoder->type == INTEL_OUTPUT_HDMI
> > > > -   && !bxt_ddi_hdmi_pll_dividers(crtc, crtc_state,
> > > > -     clock, _div))
> > > > +   if (encoder->type == INTEL_OUTPUT_HDMI &&
> > > > +   !bxt_ddi_hdmi_set_dpll_hw_state(crtc, crtc_state,
> > > > clock,
> > > > +   _hw_state))
> > > In my original patch there was just a straight call
> > > to bxt_ddi_set_dpll_hw_state() after this and the DP if condition
> > > (which also only calculated dividers).
> > That would require exporting bxt_ddi_dp_pll_dividers(), so I just
> > left
> > it as-is.
> 
> Not necessarily, bxt_ddi_dp_set_dpll_hw_state() could still be
> exported for the
> caller.

Ok, that would've worked. I was thinking along having the same calls
from intel_ddi_get_link_dpll() and bxt_get_dpll() for DPLL setup 
to keep things more unified.

> But the current solution is fine.
> 
> An even better solution would be to have a somewhat platform
> independent entry
> point for force enabling a DPLL independent of modeset with a given
> clock for
> DP. The caller of bxt_ddi_dp_set_dpll_hw_state() will immediately
> enable the
> DPLL anyway, so it could instead just ask the DPLL code to do the
> right thing
> and enable the PLL and leave as much of the platform specific part
> out of the
> caller.

There is no user yet for intel_ddi_get_link_dpll(), but having a
platform independent way to turn DPLLs on/off makes sense to me.

--Imre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/bxt: Fix HDMI DPLL configuration

[Ander Conselvan De Oliveira]

On Tue, 2016-09-27 at 14:26 +0300, Imre Deak wrote:
> On ti, 2016-09-27 at 11:03 +0300, Ander Conselvan De Oliveira wrote:
> > 
> > On Mon, 2016-09-26 at 17:54 +0300, Imre Deak wrote:
> > > 
> > > a277ca7dc01d should've been a no-functional-change commit, but it
> > > removed the initialization of the dpll_hw_state for HDMI outputs,
> > > resulting in state mismatches and a failed modeset with blank
> > > screen. Fix this by reinstating the dpll_hw_state initialization.
> > > 
> > > Fixes: a277ca7dc01d ("drm/i915: Split bxt_ddi_pll_select()")
> > > Signed-off-by: Imre Deak 
> > > ---
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 21 -
> > >  1 file changed, 16 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index c26d18a..e8bf838 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -1694,21 +1694,32 @@ bool bxt_ddi_dp_set_dpll_hw_state(int
> > > clock,
> > >   return bxt_ddi_set_dpll_hw_state(clock, _div,
> > > dpll_hw_state);
> > >  }
> > >  
> > > +bool bxt_ddi_hdmi_set_dpll_hw_state(struct intel_crtc *intel_crtc,
> > > + struct intel_crtc_state
> > > *crtc_state,
> > > + int clock,
> > > + struct intel_dpll_hw_state
> > > *dpll_hw_state)
> > > +{
> > > + struct bxt_clk_div clk_div = { };
> > > +
> > > + bxt_ddi_hdmi_pll_dividers(intel_crtc, crtc_state, clock,
> > > _div);
> > > +
> > > + return bxt_ddi_set_dpll_hw_state(clock, _div,
> > > dpll_hw_state);
> > > +}
> > > +
> > >  static struct intel_shared_dpll *
> > >  bxt_get_dpll(struct intel_crtc *crtc,
> > >   struct intel_crtc_state *crtc_state,
> > >   struct intel_encoder *encoder)
> > >  {
> > > - struct bxt_clk_div clk_div = {0};
> > > - struct intel_dpll_hw_state dpll_hw_state = {0};
> > > + struct intel_dpll_hw_state dpll_hw_state = { };
> > >   struct drm_i915_private *dev_priv = to_i915(crtc-
> > > > 
> > > > base.dev);
> > >   struct intel_digital_port *intel_dig_port;
> > >   struct intel_shared_dpll *pll;
> > >   int i, clock = crtc_state->port_clock;
> > >  
> > > - if (encoder->type == INTEL_OUTPUT_HDMI
> > > - && !bxt_ddi_hdmi_pll_dividers(crtc, crtc_state,
> > > -   clock, _div))
> > > + if (encoder->type == INTEL_OUTPUT_HDMI &&
> > > + !bxt_ddi_hdmi_set_dpll_hw_state(crtc, crtc_state,
> > > clock,
> > > + _hw_state))
> > In my original patch there was just a straight call
> > to bxt_ddi_set_dpll_hw_state() after this and the DP if condition
> > (which also only calculated dividers).
> That would require exporting bxt_ddi_dp_pll_dividers(), so I just left
> it as-is.

Not necessarily, bxt_ddi_dp_set_dpll_hw_state() could still be exported for the
caller. But the current solution is fine.

An even better solution would be to have a somewhat platform independent entry
point for force enabling a DPLL independent of modeset with a given clock for
DP. The caller of bxt_ddi_dp_set_dpll_hw_state() will immediately enable the
DPLL anyway, so it could instead just ask the DPLL code to do the right thing
and enable the PLL and leave as much of the platform specific part out of the
caller.

Ander


>  I forgot to make bxt_ddi_hdmi_set_dpll_hw_state() static, I
> fixed that up while applying.
> 
> Pushed to -dinq, thanks for the review.
> 
> > 
> > But whatever fixes the issue:
> > Reviewed-by: Ander Conselvan de Oliveira 
> > 
> > 
> > > 
> > >   return NULL;
> > >  
> > >   if ((encoder->type == INTEL_OUTPUT_DP ||
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/bxt: Fix HDMI DPLL configuration

[Imre Deak]

On ti, 2016-09-27 at 11:03 +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-09-26 at 17:54 +0300, Imre Deak wrote:
> > a277ca7dc01d should've been a no-functional-change commit, but it
> > removed the initialization of the dpll_hw_state for HDMI outputs,
> > resulting in state mismatches and a failed modeset with blank
> > screen. Fix this by reinstating the dpll_hw_state initialization.
> > 
> > Fixes: a277ca7dc01d ("drm/i915: Split bxt_ddi_pll_select()")
> > Signed-off-by: Imre Deak 
> > ---
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 21 -
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index c26d18a..e8bf838 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -1694,21 +1694,32 @@ bool bxt_ddi_dp_set_dpll_hw_state(int
> > clock,
> >     return bxt_ddi_set_dpll_hw_state(clock, _div,
> > dpll_hw_state);
> >  }
> >  
> > +bool bxt_ddi_hdmi_set_dpll_hw_state(struct intel_crtc *intel_crtc,
> > +   struct intel_crtc_state
> > *crtc_state,
> > +   int clock,
> > +   struct intel_dpll_hw_state
> > *dpll_hw_state)
> > +{
> > +   struct bxt_clk_div clk_div = { };
> > +
> > +   bxt_ddi_hdmi_pll_dividers(intel_crtc, crtc_state, clock,
> > _div);
> > +
> > +   return bxt_ddi_set_dpll_hw_state(clock, _div,
> > dpll_hw_state);
> > +}
> > +
> >  static struct intel_shared_dpll *
> >  bxt_get_dpll(struct intel_crtc *crtc,
> >     struct intel_crtc_state *crtc_state,
> >     struct intel_encoder *encoder)
> >  {
> > -   struct bxt_clk_div clk_div = {0};
> > -   struct intel_dpll_hw_state dpll_hw_state = {0};
> > +   struct intel_dpll_hw_state dpll_hw_state = { };
> >     struct drm_i915_private *dev_priv = to_i915(crtc-
> > >base.dev);
> >     struct intel_digital_port *intel_dig_port;
> >     struct intel_shared_dpll *pll;
> >     int i, clock = crtc_state->port_clock;
> >  
> > -   if (encoder->type == INTEL_OUTPUT_HDMI
> > -   && !bxt_ddi_hdmi_pll_dividers(crtc, crtc_state,
> > -     clock, _div))
> > +   if (encoder->type == INTEL_OUTPUT_HDMI &&
> > +   !bxt_ddi_hdmi_set_dpll_hw_state(crtc, crtc_state,
> > clock,
> > +   _hw_state))
> 
> In my original patch there was just a straight call
> to bxt_ddi_set_dpll_hw_state() after this and the DP if condition
> (which also only calculated dividers).

That would require exporting bxt_ddi_dp_pll_dividers(), so I just left
it as-is. I forgot to make bxt_ddi_hdmi_set_dpll_hw_state() static, I
fixed that up while applying.

Pushed to -dinq, thanks for the review.

> But whatever fixes the issue:
> Reviewed-by: Ander Conselvan de Oliveira 
> 
> 
> >     return NULL;
> >  
> >     if ((encoder->type == INTEL_OUTPUT_DP ||
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/bxt: Fix HDMI DPLL configuration

[Ander Conselvan De Oliveira]

On Mon, 2016-09-26 at 17:54 +0300, Imre Deak wrote:
> a277ca7dc01d should've been a no-functional-change commit, but it
> removed the initialization of the dpll_hw_state for HDMI outputs,
> resulting in state mismatches and a failed modeset with blank
> screen. Fix this by reinstating the dpll_hw_state initialization.
> 
> Fixes: a277ca7dc01d ("drm/i915: Split bxt_ddi_pll_select()")
> Signed-off-by: Imre Deak 
> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 21 -
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index c26d18a..e8bf838 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -1694,21 +1694,32 @@ bool bxt_ddi_dp_set_dpll_hw_state(int clock,
>   return bxt_ddi_set_dpll_hw_state(clock, _div, dpll_hw_state);
>  }
>  
> +bool bxt_ddi_hdmi_set_dpll_hw_state(struct intel_crtc *intel_crtc,
> + struct intel_crtc_state *crtc_state,
> + int clock,
> + struct intel_dpll_hw_state
> *dpll_hw_state)
> +{
> + struct bxt_clk_div clk_div = { };
> +
> + bxt_ddi_hdmi_pll_dividers(intel_crtc, crtc_state, clock, _div);
> +
> + return bxt_ddi_set_dpll_hw_state(clock, _div, dpll_hw_state);
> +}
> +
>  static struct intel_shared_dpll *
>  bxt_get_dpll(struct intel_crtc *crtc,
>   struct intel_crtc_state *crtc_state,
>   struct intel_encoder *encoder)
>  {
> - struct bxt_clk_div clk_div = {0};
> - struct intel_dpll_hw_state dpll_hw_state = {0};
> + struct intel_dpll_hw_state dpll_hw_state = { };
>   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>   struct intel_digital_port *intel_dig_port;
>   struct intel_shared_dpll *pll;
>   int i, clock = crtc_state->port_clock;
>  
> - if (encoder->type == INTEL_OUTPUT_HDMI
> - && !bxt_ddi_hdmi_pll_dividers(crtc, crtc_state,
> -   clock, _div))
> + if (encoder->type == INTEL_OUTPUT_HDMI &&
> + !bxt_ddi_hdmi_set_dpll_hw_state(crtc, crtc_state, clock,
> + _hw_state))

In my original patch there was just a straight call
to bxt_ddi_set_dpll_hw_state() after this and the DP if condition (which also
only calculated dividers). But whatever fixes the issue:

Reviewed-by: Ander Conselvan de Oliveira 


>   return NULL;
>  
>   if ((encoder->type == INTEL_OUTPUT_DP ||
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/bxt: Fix HDMI DPLL configuration

[Jani Nikula]

On Mon, 26 Sep 2016, Imre Deak  wrote:
> a277ca7dc01d should've been a no-functional-change commit, but it
> removed the initialization of the dpll_hw_state for HDMI outputs,
> resulting in state mismatches and a failed modeset with blank
> screen. Fix this by reinstating the dpll_hw_state initialization.
>
> Fixes: a277ca7dc01d ("drm/i915: Split bxt_ddi_pll_select()")
> Signed-off-by: Imre Deak 

Cc: Manasi Navare 
Cc: Ander Conselvan de Oliveira 
Cc: Durgadoss R 
Cc: Rodrigo Vivi 

> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 21 -
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c 
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index c26d18a..e8bf838 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -1694,21 +1694,32 @@ bool bxt_ddi_dp_set_dpll_hw_state(int clock,
>   return bxt_ddi_set_dpll_hw_state(clock, _div, dpll_hw_state);
>  }
>  
> +bool bxt_ddi_hdmi_set_dpll_hw_state(struct intel_crtc *intel_crtc,
> + struct intel_crtc_state *crtc_state,
> + int clock,
> + struct intel_dpll_hw_state *dpll_hw_state)
> +{
> + struct bxt_clk_div clk_div = { };
> +
> + bxt_ddi_hdmi_pll_dividers(intel_crtc, crtc_state, clock, _div);
> +
> + return bxt_ddi_set_dpll_hw_state(clock, _div, dpll_hw_state);
> +}
> +
>  static struct intel_shared_dpll *
>  bxt_get_dpll(struct intel_crtc *crtc,
>   struct intel_crtc_state *crtc_state,
>   struct intel_encoder *encoder)
>  {
> - struct bxt_clk_div clk_div = {0};
> - struct intel_dpll_hw_state dpll_hw_state = {0};
> + struct intel_dpll_hw_state dpll_hw_state = { };
>   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>   struct intel_digital_port *intel_dig_port;
>   struct intel_shared_dpll *pll;
>   int i, clock = crtc_state->port_clock;
>  
> - if (encoder->type == INTEL_OUTPUT_HDMI
> - && !bxt_ddi_hdmi_pll_dividers(crtc, crtc_state,
> -   clock, _div))
> + if (encoder->type == INTEL_OUTPUT_HDMI &&
> + !bxt_ddi_hdmi_set_dpll_hw_state(crtc, crtc_state, clock,
> + _hw_state))
>   return NULL;
>  
>   if ((encoder->type == INTEL_OUTPUT_DP ||

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx