| Message ID | 1369176624-5918-3-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/5/21 Daniel Vetter <daniel.vetter@ffwll.ch>: > All this pipe config abstraction adds another layer of complexity, so > it's good to have better visibility into what's going on exactly. > Doesn't dump out everything yet, and some bits are a bit duplicated > but this should be a good start. > > v2: Remove a few more now redudant debug output lines. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++++++--------------- > 1 file changed, 44 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index a42a7fd..5efb00d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3621,18 +3621,12 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc) > if (!crtc->config.gmch_pfit.control) > return; > > - WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE); > - assert_pipe_disabled(dev_priv, crtc->pipe); > - > /* > - * Enable automatic panel scaling so that non-native modes > - * fill the screen. The panel fitter should only be > - * adjusted whilst the pipe is disabled, according to > - * register description and PRM. > + * The panel fitter should only be adjusted whilst the pipe is disabled, > + * according to register description and PRM. > */ > - DRM_DEBUG_KMS("applying panel-fitter: %x, %x\n", > - pipe_config->gmch_pfit.control, > - pipe_config->gmch_pfit.pgm_ratios); > + WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE); > + assert_pipe_disabled(dev_priv, crtc->pipe); > > I915_WRITE(PFIT_PGM_RATIOS, pipe_config->gmch_pfit.pgm_ratios); > I915_WRITE(PFIT_CONTROL, pipe_config->gmch_pfit.control); > @@ -4955,9 +4949,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > dspcntr |= DISPPLANE_SEL_PIPE_B; > } > > - DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe)); > - drm_mode_debug_printmodeline(mode); > - > intel_set_pipe_timings(intel_crtc, mode, adjusted_mode); > > /* pipesrc and dspsize control the size that is scaled from, > @@ -5759,9 +5750,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > /* Ensure that the cursor is valid for the new mode before changing... */ > intel_crtc_update_cursor(crtc, true); > > - DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe)); > - drm_mode_debug_printmodeline(mode); > - > /* CPU eDP is the only output that doesn't need a PCH PLL of its own. */ > if (intel_crtc->config.has_pch_encoder) { > struct intel_pch_pll *pll; > @@ -5972,9 +5960,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, > /* Ensure that the cursor is valid for the new mode before changing... */ > intel_crtc_update_cursor(crtc, true); > > - DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe)); > - drm_mode_debug_printmodeline(mode); > - > if (intel_crtc->config.has_dp_encoder) > intel_dp_set_m_n(intel_crtc); > > @@ -7783,6 +7768,35 @@ pipe_config_set_bpp(struct drm_crtc *crtc, > return bpp; > } > > +static void intel_dump_pipe_config(struct intel_crtc *crtc, > + struct intel_crtc_config *pipe_config, > + const char *context) > +{ > + DRM_DEBUG_KMS("[CRTC:%d]%s config for pipe %c\n", crtc->base.base.id, > + context, pipe_name(crtc->pipe)); > + > + DRM_DEBUG_KMS("cpu_transcoder: %i\n", pipe_name(pipe_config->cpu_transcoder)); You should use %c, not %i. We also have the transcoder_name macro, which will be useful if we ever decide we want to print "EDP" for the EDP transcoder. > + DRM_DEBUG_KMS("pipe bpp: %i, dithering: %i\n", > + pipe_config->pipe_bpp, pipe_config->dither); > + DRM_DEBUG_KMS("fdi/pch: %i, lanes: %i, gmch_m: %i, gmch_n %i, link_m: %i, link_n: %i, tu: %i\n", You're missing a ':' character after gmch_n. > + pipe_config->has_pch_encoder, > + pipe_config->fdi_lanes, > + pipe_config->fdi_m_n.gmch_m, pipe_config->fdi_m_n.gmch_n, > + pipe_config->fdi_m_n.link_m, pipe_config->fdi_m_n.link_n, These 4 are unsigned, so maybe %u or 0x%x? > + pipe_config->fdi_m_n.tu); > + DRM_DEBUG_KMS("requested mode:\n"); > + drm_mode_debug_printmodeline(&pipe_config->requested_mode); > + DRM_DEBUG_KMS("adjusted mode:\n"); > + drm_mode_debug_printmodeline(&pipe_config->adjusted_mode); > + DRM_DEBUG_KMS("gmch pfit: control: 0x%08x, ratios: 0x%08x, lvds border: 0x%08x\n", > + pipe_config->gmch_pfit.control, > + pipe_config->gmch_pfit.pgm_ratios, > + pipe_config->gmch_pfit.lvds_border_bits); > + DRM_DEBUG_KMS("pch pfit: pos: 0x%08x, size: 0x%08x\n", > + pipe_config->pch_pfit.pos, > + pipe_config->pch_pfit.size); You could maybe break pos and size into x and y. Or just wait until someone fully splits the struct itself into more pieces, as Ville suggested a few days ago. > +} > + > static struct intel_crtc_config * > intel_modeset_pipe_config(struct drm_crtc *crtc, > struct drm_framebuffer *fb, > @@ -7853,8 +7867,6 @@ encoder_retry: > goto encoder_retry; > } > > - DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id); > - > pipe_config->dither = pipe_config->pipe_bpp != plane_bpp; > DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n", > plane_bpp, pipe_config->pipe_bpp, pipe_config->dither); > @@ -8211,9 +8223,14 @@ intel_modeset_check_state(struct drm_device *dev) > "crtc active state doesn't match with hw state " > "(expected %i, found %i)\n", crtc->active, active); > > - WARN(active && > - !intel_pipe_config_compare(dev, &crtc->config, &pipe_config), > - "pipe state doesn't match!\n"); > + if (active && > + !intel_pipe_config_compare(dev, &crtc->config, &pipe_config)) { > + WARN(1, "pipe state doesn't match!\n"); > + intel_dump_pipe_config(crtc, &pipe_config, > + "[hw state]"); > + intel_dump_pipe_config(crtc, &crtc->config, > + "[sw state]"); > + } > } > } > > @@ -8253,6 +8270,8 @@ static int __intel_set_mode(struct drm_crtc *crtc, > > goto out; > } > + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, > + "[modeset]"); > } > > for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc) > @@ -8609,12 +8628,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > goto fail; > > if (config->mode_changed) { > - if (set->mode) { > - DRM_DEBUG_KMS("attempting to set mode from" > - " userspace\n"); > - drm_mode_debug_printmodeline(set->mode); > - } > - > ret = intel_set_mode(set->crtc, set->mode, > set->x, set->y, set->fb); > } else if (config->fb_changed) { Patch doesn't apply anymore due to a recent code change here. Besides this, I also booted a kernel with this patch, and on the first time we print these things, almost everything is zero, even stuff like pipe_bpp, even for enabled pipes. Is this expected? > @@ -9629,6 +9642,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]); > intel_sanitize_crtc(crtc); > + intel_dump_pipe_config(crtc, &crtc->config, "[setup_hw_state]"); > } > > if (force_restore) { > -- > 1.8.1.4 >
On Mon, May 27, 2013 at 06:30:17PM -0300, Paulo Zanoni wrote: > 2013/5/21 Daniel Vetter <daniel.vetter@ffwll.ch>: > Besides this, I also booted a kernel with this patch, and on the first > time we print these things, almost everything is zero, even stuff like > pipe_bpp, even for enabled pipes. Is this expected? Lack of hw state readout support for those. One of the goals of this is that it's easier to see what pipe config the kernel is actually dealing with, and knowing that we have no clue about the pipe bpp is useuful. I'll respin the patch with your other comments fixed up. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a42a7fd..5efb00d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3621,18 +3621,12 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc) if (!crtc->config.gmch_pfit.control) return; - WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE); - assert_pipe_disabled(dev_priv, crtc->pipe); - /* - * Enable automatic panel scaling so that non-native modes - * fill the screen. The panel fitter should only be - * adjusted whilst the pipe is disabled, according to - * register description and PRM. + * The panel fitter should only be adjusted whilst the pipe is disabled, + * according to register description and PRM. */ - DRM_DEBUG_KMS("applying panel-fitter: %x, %x\n", - pipe_config->gmch_pfit.control, - pipe_config->gmch_pfit.pgm_ratios); + WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE); + assert_pipe_disabled(dev_priv, crtc->pipe); I915_WRITE(PFIT_PGM_RATIOS, pipe_config->gmch_pfit.pgm_ratios); I915_WRITE(PFIT_CONTROL, pipe_config->gmch_pfit.control); @@ -4955,9 +4949,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, dspcntr |= DISPPLANE_SEL_PIPE_B; } - DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe)); - drm_mode_debug_printmodeline(mode); - intel_set_pipe_timings(intel_crtc, mode, adjusted_mode); /* pipesrc and dspsize control the size that is scaled from, @@ -5759,9 +5750,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, /* Ensure that the cursor is valid for the new mode before changing... */ intel_crtc_update_cursor(crtc, true); - DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe)); - drm_mode_debug_printmodeline(mode); - /* CPU eDP is the only output that doesn't need a PCH PLL of its own. */ if (intel_crtc->config.has_pch_encoder) { struct intel_pch_pll *pll; @@ -5972,9 +5960,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, /* Ensure that the cursor is valid for the new mode before changing... */ intel_crtc_update_cursor(crtc, true); - DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe)); - drm_mode_debug_printmodeline(mode); - if (intel_crtc->config.has_dp_encoder) intel_dp_set_m_n(intel_crtc); @@ -7783,6 +7768,35 @@ pipe_config_set_bpp(struct drm_crtc *crtc, return bpp; } +static void intel_dump_pipe_config(struct intel_crtc *crtc, + struct intel_crtc_config *pipe_config, + const char *context) +{ + DRM_DEBUG_KMS("[CRTC:%d]%s config for pipe %c\n", crtc->base.base.id, + context, pipe_name(crtc->pipe)); + + DRM_DEBUG_KMS("cpu_transcoder: %i\n", pipe_name(pipe_config->cpu_transcoder)); + DRM_DEBUG_KMS("pipe bpp: %i, dithering: %i\n", + pipe_config->pipe_bpp, pipe_config->dither); + DRM_DEBUG_KMS("fdi/pch: %i, lanes: %i, gmch_m: %i, gmch_n %i, link_m: %i, link_n: %i, tu: %i\n", + pipe_config->has_pch_encoder, + pipe_config->fdi_lanes, + pipe_config->fdi_m_n.gmch_m, pipe_config->fdi_m_n.gmch_n, + pipe_config->fdi_m_n.link_m, pipe_config->fdi_m_n.link_n, + pipe_config->fdi_m_n.tu); + DRM_DEBUG_KMS("requested mode:\n"); + drm_mode_debug_printmodeline(&pipe_config->requested_mode); + DRM_DEBUG_KMS("adjusted mode:\n"); + drm_mode_debug_printmodeline(&pipe_config->adjusted_mode); + DRM_DEBUG_KMS("gmch pfit: control: 0x%08x, ratios: 0x%08x, lvds border: 0x%08x\n", + pipe_config->gmch_pfit.control, + pipe_config->gmch_pfit.pgm_ratios, + pipe_config->gmch_pfit.lvds_border_bits); + DRM_DEBUG_KMS("pch pfit: pos: 0x%08x, size: 0x%08x\n", + pipe_config->pch_pfit.pos, + pipe_config->pch_pfit.size); +} + static struct intel_crtc_config * intel_modeset_pipe_config(struct drm_crtc *crtc, struct drm_framebuffer *fb, @@ -7853,8 +7867,6 @@ encoder_retry: goto encoder_retry; } - DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id); - pipe_config->dither = pipe_config->pipe_bpp != plane_bpp; DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n", plane_bpp, pipe_config->pipe_bpp, pipe_config->dither); @@ -8211,9 +8223,14 @@ intel_modeset_check_state(struct drm_device *dev) "crtc active state doesn't match with hw state " "(expected %i, found %i)\n", crtc->active, active); - WARN(active && - !intel_pipe_config_compare(dev, &crtc->config, &pipe_config), - "pipe state doesn't match!\n"); + if (active && + !intel_pipe_config_compare(dev, &crtc->config, &pipe_config)) { + WARN(1, "pipe state doesn't match!\n"); + intel_dump_pipe_config(crtc, &pipe_config, + "[hw state]"); + intel_dump_pipe_config(crtc, &crtc->config, + "[sw state]"); + } } } @@ -8253,6 +8270,8 @@ static int __intel_set_mode(struct drm_crtc *crtc, goto out; } + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, + "[modeset]"); } for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc) @@ -8609,12 +8628,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set) goto fail; if (config->mode_changed) { - if (set->mode) { - DRM_DEBUG_KMS("attempting to set mode from" - " userspace\n"); - drm_mode_debug_printmodeline(set->mode); - } - ret = intel_set_mode(set->crtc, set->mode, set->x, set->y, set->fb); } else if (config->fb_changed) { @@ -9629,6 +9642,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]); intel_sanitize_crtc(crtc); + intel_dump_pipe_config(crtc, &crtc->config, "[setup_hw_state]"); } if (force_restore) {
All this pipe config abstraction adds another layer of complexity, so it's good to have better visibility into what's going on exactly. Doesn't dump out everything yet, and some bits are a bit duplicated but this should be a good start. v2: Remove a few more now redudant debug output lines. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 30 deletions(-)