[Intel-gfx] [PATCH 3/3] drm/i915: add basic pipe config dump support

Paulo Zanoni przanoni at gmail.com
Mon May 27 23:30:17 CEST 2013


2013/5/21 Daniel Vetter <daniel.vetter at 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 at 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
>



-- 
Paulo Zanoni



More information about the Intel-gfx mailing list