Re: [PATCH] drm/i915: fastboot support for HSW and BDW

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 13 Jan 2014 17:55:27 -0200
Paulo Zanoni <przanoni@xxxxxxxxx> wrote:

> 2014/1/8 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>:
> > No idea if this is correct or not, all the WRPLL programming is new to
> > me.  Paulo, can you take a look?  At least it doesn't complain on my BDW
> > here...
> 
> As a side note: can't we add some debugfs interface that, when read,
> would trigger a HW state readout, and then would compare it against
> what we have? It would be the perfect thing to validate patches like
> the one you wrote... And it would make QA easier (just write a test
> case that sets modes and reads the debugfs interface! Or just modify
> kms_setmode/testdisplay to do it everywhere).
> 
> Some comments:
> 
> - I guess the commit message could be improved :)
> - We probably need to do something about the VGA output, which uses
> the old intel_crt encoder instead of intel_digital_port. If this patch
> doesn't regress anything, we could do it on a separate patch.
> - Don't we also need to patch intel_pipe_config_compare() so it checks
> the now-used adjusted_mode.crtc_clock?
> - Based on that, don't we also need HW readout support for port_clock?
> - You should probably compare your patch against "[PATCH 2/2]
> drm/i915: add fast boot support for Haswell" sent by Furquan Shaikh in
> August, and also check the reviews we gave to it.
> - I tried booting it on my HSW machine that only has an eDP output,
> and the driver doesn't load:
> 
> I get:
> 
> [   62.635700] WARNING: CPU: 0 PID: 1120 at
> drivers/gpu/drm/i915/intel_ddi.c:431
> intel_ddi_get_crtc_encoder+0x95/0xa0 [i915]()
> [   62.635703] 0 encoders on crtc for pipe A
> 
> Then later I see:
> 
> [   62.636200] kernel BUG at drivers/gpu/drm/i915/intel_ddi.c:433!
> [   62.636249] invalid opcode: 0000 [#1] SMP
> 
> Which is the "BUG_ON(ret == NULL);" line
> 
> Please notice that I'm not using the i915_fastboot command line argument.
> 
> More below:
> 
> >
> > Thanks,
> > Jesse
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      |  6 ++++
> >  drivers/gpu/drm/i915/intel_ddi.c     |  2 +-
> >  drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  4 files changed, 67 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index a699efd..644e4f9 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,8 +5318,13 @@
> >  #define  WRPLL_PLL_SELECT_LCPLL_2700   (0x03<<28)
> >  /* WRPLL divider programming */
> >  #define  WRPLL_DIVIDER_REFERENCE(x)    ((x)<<0)
> > +#define  WRPLL_DIVIDER_REF_MASK                (0xff)
> >  #define  WRPLL_DIVIDER_POST(x)         ((x)<<8)
> > +#define  WRPLL_DIVIDER_POST_MASK       (0x3f<<8)
> > +#define  WRPLL_DIVIDER_POST_SHIFT      8
> >  #define  WRPLL_DIVIDER_FEEDBACK(x)     ((x)<<16)
> > +#define  WRPLL_DIVIDER_FB_SHIFT                16
> > +#define  WRPLL_DIVIDER_FB_MASK         (0xff<<16)
> >
> >  /* Port clock selection */
> >  #define PORT_CLK_SEL_A                 0x46100
> > @@ -5332,6 +5337,7 @@
> >  #define  PORT_CLK_SEL_WRPLL1           (4<<29)
> >  #define  PORT_CLK_SEL_WRPLL2           (5<<29)
> >  #define  PORT_CLK_SEL_NONE             (7<<29)
> > +#define  PORT_CLK_SEL_MASK             (7<<29)
> >
> >  /* Transcoder clock selection */
> >  #define TRANS_CLK_SEL_A                        0x46140
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 1488b28..f3d7b42 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -413,7 +413,7 @@ static void intel_ddi_mode_set(struct intel_encoder *encoder)
> >         }
> >  }
> >
> > -static struct intel_encoder *
> > +struct intel_encoder *
> >  intel_ddi_get_crtc_encoder(struct drm_crtc *crtc)
> >  {
> >         struct drm_device *dev = crtc->dev;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 74137d5..f87244d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -48,6 +48,8 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
> >                                 struct intel_crtc_config *pipe_config);
> >  static void ironlake_pch_clock_get(struct intel_crtc *crtc,
> >                                    struct intel_crtc_config *pipe_config);
> > +static void haswell_ddi_clock_get(struct intel_crtc *crtc,
> > +                                 struct intel_crtc_config *pipe_config);
> >
> >  static int intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode,
> >                           int x, int y, struct drm_framebuffer *old_fb);
> > @@ -6994,10 +6996,11 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >                 tmp = I915_READ(FDI_RX_CTL(PIPE_A));
> >                 pipe_config->fdi_lanes = ((FDI_DP_PORT_WIDTH_MASK & tmp) >>
> >                                           FDI_DP_PORT_WIDTH_SHIFT) + 1;
> > -
> > -               ironlake_get_fdi_m_n_config(crtc, pipe_config);
> >         }
> >
> > +       ironlake_get_fdi_m_n_config(crtc, pipe_config);
> 
> This is a little confusing, probably wrong, because FDI is only used
> when has_pch_encoder is true, but you're calling this code on all
> cases.
> 
> Function ironlake_get_fdi_m_n_config() calls
> intel_cpu_transcoder_get_m_n() for eDP/DP/HDMI.
> However, we have encoder->get_config which calls
> intel_ddi_get_config() which calls intel_dp_get_m_n(), which will call
> intel_cpu_transcoder_get_m_n() again on the same cases. So we call the
> same thing twice.
> 
> I would imagine you probably don't need to move this line above, but I
> may be wrong. If something is wrong, you probably need to fix some
> other code that's assuming that "gen5+ DP m/n registers are on the
> PCH" or something like that.
> 
> 
> > +       haswell_ddi_clock_get(crtc, pipe_config);
> > +
> >         intel_get_pipe_timings(crtc, pipe_config);
> >
> >         pfit_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> > @@ -8034,6 +8037,60 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
> >                                          &pipe_config->fdi_m_n);
> >  }
> >
> > +#define LC_FREQ 2700
> > +
> > +static int intel_ddi_calc_wrpll_link(u32 wrpll)
> > +{
> > +       int n, p, r;
> > +
> > +       r = wrpll & WRPLL_DIVIDER_REF_MASK;
> > +       p = (wrpll & WRPLL_DIVIDER_POST_MASK) >> WRPLL_DIVIDER_POST_SHIFT;
> > +       n = (wrpll & WRPLL_DIVIDER_FB_MASK) >> WRPLL_DIVIDER_FB_SHIFT;
> > +
> > +       return (LC_FREQ * n) / (p * r);
> > +}
> > +
> > +static void haswell_ddi_clock_get(struct intel_crtc *crtc,
> > +                                 struct intel_crtc_config *pipe_config)
> > +{
> > +       struct intel_encoder *intel_encoder =
> > +               intel_ddi_get_crtc_encoder(&crtc->base);
> > +       struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > +       enum port port = intel_ddi_get_encoder_port(intel_encoder);
> > +       int link_clock;
> > +       u32 val, pll;
> > +
> > +       val = I915_READ(PORT_CLK_SEL(port));
> > +       switch (val & PORT_CLK_SEL_MASK) {
> > +       case PORT_CLK_SEL_LCPLL_810:
> > +               link_clock = 81000;
> > +               break;
> > +       case PORT_CLK_SEL_LCPLL_1350:
> > +               link_clock = 135000;
> > +               break;
> > +       case PORT_CLK_SEL_LCPLL_2700:
> > +               link_clock = 270000;
> > +               break;
> > +       case PORT_CLK_SEL_WRPLL1:
> > +               pll = I915_READ(WRPLL_CTL1);
> > +               link_clock = intel_ddi_calc_wrpll_link(pll);
> > +               break;
> > +       case PORT_CLK_SEL_WRPLL2:
> > +               pll = I915_READ(WRPLL_CTL2);
> > +               link_clock = intel_ddi_calc_wrpll_link(pll);
> > +               break;
> > +       case PORT_CLK_SEL_SPLL:
> > +               link_clock = 135000;
> > +               break;
> > +       default:
> > +               WARN(1, "bad port clock sel\n");
> > +               return;
> > +       }
> > +
> > +       pipe_config->adjusted_mode.crtc_clock =
> > +               intel_dotclock_calculate(link_clock, &pipe_config->fdi_m_n);
> > +}
> > +
> >  /** Returns the currently programmed mode of the given pipe. */
> >  struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
> >                                              struct drm_crtc *crtc)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 46aea6c..7bfc19a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -604,6 +604,7 @@ void intel_prepare_ddi(struct drm_device *dev);
> >  void hsw_fdi_link_train(struct drm_crtc *crtc);
> >  void intel_ddi_init(struct drm_device *dev, enum port port);
> >  enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder);
> > +struct intel_encoder *intel_ddi_get_crtc_encoder(struct drm_crtc *crtc);
> >  bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);
> >  int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv);
> >  void intel_ddi_pll_init(struct drm_device *dev);
> > --

Ok, I only tried it on BDW, I'll check again w/o the fastboot param and
see what happens.

Agree on the other points.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux