Hi
2012/10/18 Daniel Vetter <daniel at ffwll.ch>:
> 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 ;-)
Just making the discussion public...
I also thought about that, but my fear is that as we move code from
one place to another, in the future we might end moving/copying some
code and making Haswell use pipe instead of transcoder.
I will write the smaller version with only haswell-specific bits and send V2.
>
> 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
--
Paulo Zanoni
