On Tue, May 20, 2014 at 11:47:35AM +0100, Damien Lespiau wrote:
> On Thu, Apr 24, 2014 at 11:55:33PM +0200, Daniel Vetter wrote:
> > To make things a bit more manageable extract a new function for
> > reading out common ddi port state. This means a bit of duplication
> > between encoders and the core since both look at the same registers,
> > but doesn't seem worth to make a fuzz about.
> >
> > We can also remove the state readout code in intel_ddi_setup_hw_pll_state.
> > That code is only called from the hardware take over and not the cross
> > check code, and only after the crtc state is reconstructed. So we can
> > rely on an accurate value of crtc->config.ddi_pll_sel already.
> >
> > Compared to the old code also trust the hw state more and don't
> > special-case port A - we want to cross-check the actual-state, not
> > bake in our own assumptions about how this is supposed to all be
> > linked up.
> >
> > v2: Make use of the read-out ddi_pll_sel in intel_ddi_clock_get.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
> Moving things out of intel_ddi.c to intel_display.c seems to be the
> opposite of what we want. I don't see why we couldn't leave the DDI
> readout function inside intel_ddi.c?
Fewer forward decls and non-static functions. If you have a file with a
bunch of functions but they're all called from somewhere else, then the
split-up is imo not functionally sound. intel_ddi.c has a bit that taste
for me.
We could fix this I guess by moving a lot more into intel_ddi.c, like all
the hsw+ modeset code.
> Reviewed-by: Damien Lespiau <damien.lespiau@xxxxxxxxx>
>
> --
> Damien
>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > drivers/gpu/drm/i915/intel_ddi.c | 40 +-----------------------------
> > drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++++++++++++++++------------
> > 3 files changed, 34 insertions(+), 55 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 64d40f22e708..4c1cefb5f3eb 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5366,6 +5366,7 @@ enum punit_power_well {
> > #define TRANS_DDI_FUNC_ENABLE (1<<31)
> > /* Those bits are ignored by pipe EDP since it can only connect to DDI A */
> > #define TRANS_DDI_PORT_MASK (7<<28)
> > +#define TRANS_DDI_PORT_SHIFT 28
> > #define TRANS_DDI_SELECT_PORT(x) ((x)<<28)
> > #define TRANS_DDI_PORT_NONE (0<<28)
> > #define TRANS_DDI_MODE_SELECT_MASK (7<<24)
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 2adcc917806e..571cfe431558 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -481,11 +481,10 @@ static void intel_ddi_clock_get(struct intel_encoder *encoder,
> > struct intel_crtc_config *pipe_config)
> > {
> > struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> > - enum port port = intel_ddi_get_encoder_port(encoder);
> > int link_clock = 0;
> > u32 val, pll;
> >
> > - val = I915_READ(PORT_CLK_SEL(port));
> > + val = pipe_config->ddi_pll_sel;
> > switch (val & PORT_CLK_SEL_MASK) {
> > case PORT_CLK_SEL_LCPLL_810:
> > link_clock = 81000;
> > @@ -987,40 +986,6 @@ static bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
> > return intel_ddi_get_port_state(encoder, pipe, port);
> > }
> >
> > -static uint32_t intel_ddi_get_crtc_pll(struct drm_i915_private *dev_priv,
> > - enum pipe pipe)
> > -{
> > - uint32_t temp, ret;
> > - enum port port = I915_MAX_PORTS;
> > - enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
> > - pipe);
> > - int i;
> > -
> > - if (cpu_transcoder == TRANSCODER_EDP) {
> > - port = PORT_A;
> > - } else {
> > - temp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
> > - temp &= TRANS_DDI_PORT_MASK;
> > -
> > - for (i = PORT_B; i <= PORT_E; i++)
> > - if (temp == TRANS_DDI_SELECT_PORT(i))
> > - port = i;
> > - }
> > -
> > - if (port == I915_MAX_PORTS) {
> > - WARN(1, "Pipe %c enabled on an unknown port\n",
> > - pipe_name(pipe));
> > - ret = PORT_CLK_SEL_NONE;
> > - } else {
> > - ret = I915_READ(PORT_CLK_SEL(port));
> > - DRM_DEBUG_KMS("Pipe %c connected to port %c using clock "
> > - "0x%08x\n", pipe_name(pipe), port_name(port),
> > - ret);
> > - }
> > -
> > - return ret;
> > -}
> > -
> > void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -1039,9 +1004,6 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
> > continue;
> > }
> >
> > - intel_crtc->config.ddi_pll_sel = intel_ddi_get_crtc_pll(dev_priv,
> > - pipe);
> > -
> > switch (intel_crtc->config.ddi_pll_sel) {
> > case PORT_CLK_SEL_WRPLL1:
> > dev_priv->ddi_plls.wrpll1_refcount++;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 728b5a25cb80..1601da1b57a1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7006,6 +7006,35 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> > return 0;
> > }
> >
> > +static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
> > + struct intel_crtc_config *pipe_config)
> > +{
> > + struct drm_device *dev = crtc->base.dev;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + enum port port;
> > + uint32_t tmp;
> > +
> > + tmp = I915_READ(TRANS_DDI_FUNC_CTL(pipe_config->cpu_transcoder));
> > +
> > + port = (tmp & TRANS_DDI_PORT_MASK) >> TRANS_DDI_PORT_SHIFT;
> > +
> > + pipe_config->ddi_pll_sel = I915_READ(PORT_CLK_SEL(port));
> > + /*
> > + * Haswell has only FDI/PCH transcoder A. It is which is connected to
> > + * DDI E. So just check whether this pipe is wired to DDI E and whether
> > + * the PCH transcoder is on.
> > + */
> > + if ((port == PORT_E) && I915_READ(LPT_TRANSCONF) & TRANS_ENABLE) {
> > + pipe_config->has_pch_encoder = true;
> > +
> > + 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);
> > + }
> > +}
> > +
> > static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> > struct intel_crtc_config *pipe_config)
> > {
> > @@ -7051,22 +7080,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> > if (!(tmp & PIPECONF_ENABLE))
> > return false;
> >
> > - /*
> > - * Haswell has only FDI/PCH transcoder A. It is which is connected to
> > - * DDI E. So just check whether this pipe is wired to DDI E and whether
> > - * the PCH transcoder is on.
> > - */
> > - tmp = I915_READ(TRANS_DDI_FUNC_CTL(pipe_config->cpu_transcoder));
> > - if ((tmp & TRANS_DDI_PORT_MASK) == TRANS_DDI_SELECT_PORT(PORT_E) &&
> > - I915_READ(LPT_TRANSCONF) & TRANS_ENABLE) {
> > - pipe_config->has_pch_encoder = true;
> > -
> > - 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);
> > - }
> > + haswell_get_ddi_port_state(crtc, pipe_config);
> >
> > intel_get_pipe_timings(crtc, pipe_config);
> >
> > @@ -9521,6 +9535,8 @@ intel_pipe_config_compare(struct drm_device *dev,
> >
> > PIPE_CONF_CHECK_I(double_wide);
> >
> > + PIPE_CONF_CHECK_X(ddi_pll_sel);
> > +
> > PIPE_CONF_CHECK_I(shared_dpll);
> > PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
> > PIPE_CONF_CHECK_X(dpll_hw_state.dpll_md);
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
