On Thu, Oct 18, 2012 at 06:21:35PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> Because the PIPECONF register is actually part of the CPU transcoder,
> not the CPU pipe.
>
> Ideally we would also rename PIPECONF to TRANSCONF to remind people
> that they should use the transcoder instead of the pipe, but let's
> keep it like this for now since most Gens still name it PIPECONF.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
I think as a general rule it makes sense to not convert codepaths that are
never run on haswell to cpu_transcoder, since that thing really doesn't
exist that much on earlier platforms. Also, it makes the patch smaller ;-)
Below some comments about which hunks I think we can drop.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_irq.c | 4 ++-
> drivers/gpu/drm/i915/i915_reg.h | 2 +-
> drivers/gpu/drm/i915/intel_crt.c | 6 ++--
> drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++--------------
> drivers/gpu/drm/i915/intel_sprite.c | 3 +-
> drivers/gpu/drm/i915/intel_tv.c | 4 +--
> 6 files changed, 49 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d07c787..c9b186d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -123,7 +123,9 @@ static int
> i915_pipe_enabled(struct drm_device *dev, int pipe)
> {
> drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> - return I915_READ(PIPECONF(pipe)) & PIPECONF_ENABLE;
> + enum transcoder cpu_transcoder = pipe_to_cpu_transcoder(dev_priv, pipe);
> +
> + return I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE;
> }
Oh, how I hate our vblank code and it's insistency to deal with pipes
instead of crtc numbers.
>
> /* Called from drm generic code, passed a 'crtc', which
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 72a61b5..9fecd3b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2714,7 +2714,7 @@
> #define PIPE_12BPC (3 << 5)
>
> #define PIPESRC(pipe) _PIPE(pipe, _PIPEASRC, _PIPEBSRC)
> -#define PIPECONF(pipe) _PIPE(pipe, _PIPEACONF, _PIPEBCONF)
> +#define PIPECONF(tran) _TRANSCODER(tran, _PIPEACONF, _PIPEBCONF)
> #define PIPEDSL(pipe) _PIPE(pipe, _PIPEADSL, _PIPEBDSL)
> #define PIPEFRAME(pipe) _PIPE(pipe, _PIPEAFRAMEHIGH, _PIPEBFRAMEHIGH)
> #define PIPEFRAMEPIXEL(pipe) _PIPE(pipe, _PIPEAFRAMEPIXEL, _PIPEBFRAMEPIXEL)
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 53f3e87..2a2c976 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -467,7 +467,9 @@ intel_crt_load_detect(struct intel_crt *crt)
> {
> struct drm_device *dev = crt->base.base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - uint32_t pipe = to_intel_crtc(crt->base.base.crtc)->pipe;
> + struct intel_crtc *intel_crtc = to_intel_crtc(crt->base.base.crtc);
> + enum pipe pipe = intel_crtc->pipe;
> + enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
> uint32_t save_bclrpat;
> uint32_t save_vtotal;
> uint32_t vtotal, vactive;
> @@ -489,7 +491,7 @@ intel_crt_load_detect(struct intel_crt *crt)
> vtotal_reg = VTOTAL(pipe);
> vblank_reg = VBLANK(pipe);
> vsync_reg = VSYNC(pipe);
> - pipeconf_reg = PIPECONF(pipe);
> + pipeconf_reg = PIPECONF(cpu_transcoder);
> pipe_dsl_reg = PIPEDSL(pipe);
>
> save_bclrpat = I915_READ(bclrpat_reg);
Load detect is only used by gen2/3 vga connectors and by the tv out
connector, nothing else. So I think we can just leave this as-is.
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 827c5ba..dc93c39 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1008,9 +1008,10 @@ void intel_wait_for_vblank(struct drm_device *dev, int pipe)
> void intel_wait_for_pipe_off(struct drm_device *dev, int pipe)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> + enum transcoder cpu_transcoder = pipe_to_cpu_transcoder(dev_priv, pipe);
>
> if (INTEL_INFO(dev)->gen >= 4) {
> - int reg = PIPECONF(pipe);
> + int reg = PIPECONF(cpu_transcoder);
>
> /* Wait for the Pipe State to go off */
> if (wait_for((I915_READ(reg) & I965_PIPECONF_ACTIVE) == 0,
> @@ -1222,12 +1223,13 @@ void assert_pipe(struct drm_i915_private *dev_priv,
> int reg;
> u32 val;
> bool cur_state;
> + enum transcoder cpu_transcoder = pipe_to_cpu_transcoder(dev_priv, pipe);
>
> /* if we need the pipe A quirk it must be always on */
> if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
> state = true;
>
> - reg = PIPECONF(pipe);
> + reg = PIPECONF(cpu_transcoder);
> val = I915_READ(reg);
> cur_state = !!(val & PIPECONF_ENABLE);
> WARN(cur_state != state,
> @@ -1661,6 +1663,7 @@ static void intel_enable_transcoder(struct drm_i915_private *dev_priv,
> int reg;
> u32 val, pipeconf_val;
> struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> + enum transcoder cpu_transcoder = pipe_to_cpu_transcoder(dev_priv, pipe);
>
> /* PCH only available on ILK+ */
> BUG_ON(dev_priv->info->gen < 5);
Do we really need this on hsw for the pch vga port? Since the only pch
port we have is vga, and that is currently restricted to pipe 0 I'd prefer
if we fix this up once we really put the code to some use (i.e. lift the
pipe == 0 restriction).
> @@ -1680,7 +1683,7 @@ static void intel_enable_transcoder(struct drm_i915_private *dev_priv,
> }
> reg = TRANSCONF(pipe);
> val = I915_READ(reg);
> - pipeconf_val = I915_READ(PIPECONF(pipe));
> + pipeconf_val = I915_READ(PIPECONF(cpu_transcoder));
>
> if (HAS_PCH_IBX(dev_priv->dev)) {
> /*
> @@ -1745,6 +1748,7 @@ static void intel_disable_transcoder(struct drm_i915_private *dev_priv,
> static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
> bool pch_port)
> {
> + enum transcoder cpu_transcoder = pipe_to_cpu_transcoder(dev_priv, pipe);
> int reg;
> u32 val;
>
> @@ -1764,7 +1768,7 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
> /* FIXME: assert CPU port conditions for SNB+ */
> }
>
> - reg = PIPECONF(pipe);
> + reg = PIPECONF(cpu_transcoder);
> val = I915_READ(reg);
> if (val & PIPECONF_ENABLE)
> return;
> @@ -1788,6 +1792,7 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
> static void intel_disable_pipe(struct drm_i915_private *dev_priv,
> enum pipe pipe)
> {
> + enum transcoder cpu_transcoder = pipe_to_cpu_transcoder(dev_priv, pipe);
> int reg;
> u32 val;
>
> @@ -1801,7 +1806,7 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
> if (pipe == PIPE_A && (dev_priv->quirks & QUIRK_PIPEA_FORCE))
> return;
>
> - reg = PIPECONF(pipe);
> + reg = PIPECONF(cpu_transcoder);
> val = I915_READ(reg);
> if ((val & PIPECONF_ENABLE) == 0)
> return;
> @@ -2679,6 +2684,7 @@ static void ironlake_fdi_pll_enable(struct intel_crtc *intel_crtc)
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> int pipe = intel_crtc->pipe;
> + enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
> u32 reg, temp;
Dito for all the fdi stuff here, I think that's better done when we
enable/fix-up VGA.
>
> /* Write the TU size bits so error detection works */
> @@ -2690,7 +2696,7 @@ static void ironlake_fdi_pll_enable(struct intel_crtc *intel_crtc)
> temp = I915_READ(reg);
> temp &= ~((0x7 << 19) | (0x7 << 16));
> temp |= (intel_crtc->fdi_lanes - 1) << 19;
> - temp |= (I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK) << 11;
> + temp |= (I915_READ(PIPECONF(cpu_transcoder)) & PIPE_BPC_MASK) << 11;
> I915_WRITE(reg, temp | FDI_RX_PLL_ENABLE);
>
> POSTING_READ(reg);
> @@ -2764,6 +2770,7 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc)
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int pipe = intel_crtc->pipe;
> + enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
> u32 reg, temp;
>
> /* disable CPU FDI tx and PCH FDI rx */
> @@ -2775,7 +2782,7 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc)
> reg = FDI_RX_CTL(pipe);
> temp = I915_READ(reg);
> temp &= ~(0x7 << 16);
> - temp |= (I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK) << 11;
> + temp |= (I915_READ(PIPECONF(cpu_transcoder)) & PIPE_BPC_MASK) << 11;
> I915_WRITE(reg, temp & ~FDI_RX_ENABLE);
>
> POSTING_READ(reg);
> @@ -2809,7 +2816,7 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc)
> }
> /* BPC in FDI rx is consistent with that in PIPECONF */
> temp &= ~(0x07 << 16);
> - temp |= (I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK) << 11;
> + temp |= (I915_READ(PIPECONF(cpu_transcoder)) & PIPE_BPC_MASK) << 11;
> I915_WRITE(reg, temp);
>
> POSTING_READ(reg);
> @@ -2971,6 +2978,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int pipe = intel_crtc->pipe;
> + enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
> u32 reg, temp;
>
> assert_transcoder_disabled(dev_priv, pipe);
> @@ -3027,7 +3035,8 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
> if (HAS_PCH_CPT(dev) &&
> (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) ||
> intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
> - u32 bpc = (I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK) >> 5;
> + u32 bpc = (I915_READ(PIPECONF(cpu_transcoder)) &
> + PIPE_BPC_MASK) >> 5;
> reg = TRANS_DP_CTL(pipe);
> temp = I915_READ(reg);
> temp &= ~(TRANS_DP_PORT_SEL_MASK |
> @@ -4383,6 +4392,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int pipe = intel_crtc->pipe;
> int plane = intel_crtc->plane;
> + enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
> int refclk, num_connectors = 0;
> intel_clock_t clock, reduced_clock;
> u32 dspcntr, pipeconf;
> @@ -4463,7 +4473,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> num_connectors);
i9xx stuff, I think pipeconf is totally ok for these ;-)
>
> /* setup pipeconf */
> - pipeconf = I915_READ(PIPECONF(pipe));
> + pipeconf = I915_READ(PIPECONF(cpu_transcoder));
>
> /* Set up the display plane register */
> dspcntr = DISPPLANE_GAMMA_ENABLE;
> @@ -4535,8 +4545,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> (mode->hdisplay - 1));
> I915_WRITE(DSPPOS(plane), 0);
>
> - I915_WRITE(PIPECONF(pipe), pipeconf);
> - POSTING_READ(PIPECONF(pipe));
> + I915_WRITE(PIPECONF(cpu_transcoder), pipeconf);
> + POSTING_READ(PIPECONF(cpu_transcoder));
> intel_enable_pipe(dev_priv, pipe, false);
>
> intel_wait_for_vblank(dev, pipe);
> @@ -4704,10 +4714,10 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
> {
> struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - int pipe = intel_crtc->pipe;
> + enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
> uint32_t val;
ilk_set_pipeconf is not called on hsw, we have the special version now.
>
> - val = I915_READ(PIPECONF(pipe));
> + val = I915_READ(PIPECONF(cpu_transcoder));
>
> val &= ~PIPE_BPC_MASK;
> switch (intel_crtc->bpp) {
> @@ -4738,8 +4748,8 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
> else
> val |= PIPECONF_PROGRESSIVE;
>
> - I915_WRITE(PIPECONF(pipe), val);
> - POSTING_READ(PIPECONF(pipe));
> + I915_WRITE(PIPECONF(cpu_transcoder), val);
> + POSTING_READ(PIPECONF(cpu_transcoder));
> }
>
> static void haswell_set_pipeconf(struct drm_crtc *crtc,
> @@ -4748,10 +4758,10 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc,
> {
> struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - int pipe = intel_crtc->pipe;
> + enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
> uint32_t val;
>
> - val = I915_READ(PIPECONF(pipe));
> + val = I915_READ(PIPECONF(cpu_transcoder));
>
> val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
> if (dither)
> @@ -4763,8 +4773,8 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc,
> else
> val |= PIPECONF_PROGRESSIVE;
>
> - I915_WRITE(PIPECONF(pipe), val);
> - POSTING_READ(PIPECONF(pipe));
> + I915_WRITE(PIPECONF(cpu_transcoder), val);
> + POSTING_READ(PIPECONF(cpu_transcoder));
> }
>
> static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> @@ -5238,7 +5248,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> WARN(num_connectors != 1, "%d connectors attached to pipe %c\n",
> num_connectors, pipe_name(pipe));
>
> - WARN_ON(I915_READ(PIPECONF(pipe)) &
> + WARN_ON(I915_READ(PIPECONF(intel_crtc->cpu_transcoder)) &
> (PIPECONF_ENABLE | I965_PIPECONF_ACTIVE));
>
> WARN_ON(I915_READ(DSPCNTR(plane)) & DISPLAY_PLANE_ENABLE);
> @@ -8416,7 +8426,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> u32 reg, val;
>
> /* Clear any frame start delays used for debugging left by the BIOS */
> - reg = PIPECONF(crtc->pipe);
> + reg = PIPECONF(crtc->cpu_transcoder);
> I915_WRITE(reg, I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
>
> /* We need to sanitize the plane -> pipe mapping first because this will
> @@ -8579,7 +8589,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev)
> for_each_pipe(pipe) {
> crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>
> - tmp = I915_READ(PIPECONF(pipe));
> + tmp = I915_READ(PIPECONF(crtc->cpu_transcoder));
> if (tmp & PIPECONF_ENABLE)
> crtc->active = true;
> else
> @@ -8773,6 +8783,7 @@ intel_display_capture_error_state(struct drm_device *dev)
> {
> drm_i915_private_t *dev_priv = dev->dev_private;
> struct intel_display_error_state *error;
> + enum transcoder cpu_transcoder;
> int i;
>
> error = kmalloc(sizeof(*error), GFP_ATOMIC);
> @@ -8780,6 +8791,8 @@ intel_display_capture_error_state(struct drm_device *dev)
> return NULL;
>
> for_each_pipe(i) {
> + cpu_transcoder = pipe_to_cpu_transcoder(dev_priv, i);
> +
> error->cursor[i].control = I915_READ(CURCNTR(i));
> error->cursor[i].position = I915_READ(CURPOS(i));
> error->cursor[i].base = I915_READ(CURBASE(i));
> @@ -8794,7 +8807,7 @@ intel_display_capture_error_state(struct drm_device *dev)
> error->plane[i].tile_offset = I915_READ(DSPTILEOFF(i));
> }
>
> - error->pipe[i].conf = I915_READ(PIPECONF(i));
> + error->pipe[i].conf = I915_READ(PIPECONF(cpu_transcoder));
> error->pipe[i].source = I915_READ(PIPESRC(i));
> error->pipe[i].htotal = I915_READ(HTOTAL(i));
> error->pipe[i].hblank = I915_READ(HBLANK(i));
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 7644f31..f2ca943 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -422,6 +422,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> struct intel_framebuffer *intel_fb;
> struct drm_i915_gem_object *obj, *old_obj;
> int pipe = intel_plane->pipe;
> + enum transcoder cpu_transcoder = pipe_to_cpu_transcoder(dev_priv, pipe);
I think we can leave that until we enable sprite support on hsw, too.
> int ret = 0;
> int x = src_x >> 16, y = src_y >> 16;
> int primary_w = crtc->mode.hdisplay, primary_h = crtc->mode.vdisplay;
> @@ -436,7 +437,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> src_h = src_h >> 16;
>
> /* Pipe must be running... */
> - if (!(I915_READ(PIPECONF(pipe)) & PIPECONF_ENABLE))
> + if (!(I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE))
> return -EINVAL;
>
> if (crtc_x >= primary_w || crtc_y >= primary_h)
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index d2c5c8f..fcdbb66 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -941,7 +941,7 @@ intel_tv_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
> const struct video_levels *video_levels;
> const struct color_conversion *color_conversion;
> bool burst_ena;
> - int pipe = intel_crtc->pipe;
> + enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
TV-out is non-existing on hsw ...
>
> if (!tv_mode)
> return; /* can't happen (mode_prepare prevents this) */
> @@ -1085,7 +1085,7 @@ intel_tv_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
> ((video_levels->black << TV_BLACK_LEVEL_SHIFT) |
> (video_levels->blank << TV_BLANK_LEVEL_SHIFT)));
> {
> - int pipeconf_reg = PIPECONF(pipe);
> + int pipeconf_reg = PIPECONF(cpu_transcoder);
> int dspcntr_reg = DSPCNTR(intel_crtc->plane);
> int pipeconf = I915_READ(pipeconf_reg);
> int dspcntr = I915_READ(dspcntr_reg);
> --
> 1.7.11.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
