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
