[Intel-gfx] [PATCH 1/3] drm/i915: hw state readout&check support for cpu_transcoder

Paulo Zanoni przanoni at gmail.com
Mon May 27 22:45:53 CEST 2013


2013/5/21 Daniel Vetter <daniel.vetter at ffwll.ch>:
> This allows us to drop a bunch of ugly hacks and finally implement
> what
>
> commit cc464b2a17c59adedbdc02cc54341d630354edc3
> Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Date:   Fri Jan 25 16:59:16 2013 -0200
>
>     drm/i915: set TRANSCODER_EDP even earlier
>
> tried to achieve, but that was reverted again in
>
> commit bba2181c49f1dddf8b592804a1b53cc1a3cf408a
> Author: Daniel Vetter <daniel.vetter at ffwll.ch>
> Date:   Fri Mar 22 10:53:40 2013 +0100
>
>     Revert "drm/i915: set TRANSCODER_EDP even earlier"
>
> Now we should always have a consistent cpu_transcoder in the
> pipe_config.
>
> v2: Fix up the code as spotted by Paulo:
> - read the register for real
> - assign the right pipes
> - break out if the hw state doesn't make sense
>
> v3: Shut up gcc.
>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |  4 ++
>  drivers/gpu/drm/i915/intel_display.c | 90 +++++++++++++-----------------------
>  2 files changed, 37 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 1fdd3b0..9649df8 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1291,9 +1291,13 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>                                      struct intel_crtc_config *pipe_config)
>  {
>         int type = encoder->type;
> +       int port = intel_ddi_get_encoder_port(encoder);
>
>         WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
>
> +       if (port == PORT_A)
> +               pipe_config->cpu_transcoder = TRANSCODER_EDP;
> +
>         if (type == INTEL_OUTPUT_HDMI)
>                 return intel_hdmi_compute_config(encoder, pipe_config);
>         else
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f9ec773..a42a7fd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3567,12 +3567,6 @@ static void ironlake_crtc_off(struct drm_crtc *crtc)
>
>  static void haswell_crtc_off(struct drm_crtc *crtc)
>  {
> -       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -
> -       /* Stop saying we're using TRANSCODER_EDP because some other CRTC might
> -        * start using it. */
> -       intel_crtc->config.cpu_transcoder = (enum transcoder) intel_crtc->pipe;
> -
>         intel_ddi_put_crtc_pll(crtc);
>  }
>
> @@ -5023,6 +5017,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         uint32_t tmp;
>
> +       pipe_config->cpu_transcoder = crtc->pipe;
> +
>         tmp = I915_READ(PIPECONF(crtc->pipe));
>         if (!(tmp & PIPECONF_ENABLE))
>                 return false;
> @@ -5745,8 +5741,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
>              "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
>
> -       intel_crtc->config.cpu_transcoder = pipe;
> -
>         ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
>                                      &has_reduced_clock, &reduced_clock);
>         if (!ok) {
> @@ -5882,6 +5876,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         uint32_t tmp;
>
> +       pipe_config->cpu_transcoder = crtc->pipe;
> +
>         tmp = I915_READ(PIPECONF(crtc->pipe));
>         if (!(tmp & PIPECONF_ENABLE))
>                 return false;
> @@ -5958,11 +5954,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>                 num_connectors++;
>         }
>
> -       if (is_cpu_edp)
> -               intel_crtc->config.cpu_transcoder = TRANSCODER_EDP;
> -       else
> -               intel_crtc->config.cpu_transcoder = pipe;
> -
>         /* We are not sure yet this won't happen. */
>         WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n",
>              INTEL_PCH_TYPE(dev));
> @@ -6016,15 +6007,37 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  {
>         struct drm_device *dev = crtc->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
>         enum intel_display_power_domain pfit_domain;
>         uint32_t tmp;
>
> +       pipe_config->cpu_transcoder = crtc->pipe;
> +       tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
> +       if (tmp & TRANS_DDI_FUNC_ENABLE) {
> +               enum pipe trans_edp_pipe;
> +               switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
> +               default:
> +                       WARN(1, "unknown pipe linked to edp transcoder\n");

Since there's no "break" we'll assign PIPE_A and then we'll probably
break pipe A on this case. OTOH this case should never really happen,
so I'm not sure if we care. So this is an optional bikeshedding.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

Also briefly tested, seems to work.

> +               case TRANS_DDI_EDP_INPUT_A_ONOFF:
> +               case TRANS_DDI_EDP_INPUT_A_ON:
> +                       trans_edp_pipe = PIPE_A;
> +                       break;
> +               case TRANS_DDI_EDP_INPUT_B_ONOFF:
> +                       trans_edp_pipe = PIPE_B;
> +                       break;
> +               case TRANS_DDI_EDP_INPUT_C_ONOFF:
> +                       trans_edp_pipe = PIPE_C;
> +                       break;
> +               }
> +
> +               if (trans_edp_pipe == crtc->pipe)
> +                       pipe_config->cpu_transcoder = TRANSCODER_EDP;
> +       }
> +
>         if (!intel_display_power_enabled(dev,
> -                       POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
> +                       POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder)))
>                 return false;
>
> -       tmp = I915_READ(PIPECONF(cpu_transcoder));
> +       tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder));
>         if (!(tmp & PIPECONF_ENABLE))
>                 return false;
>
> @@ -6033,7 +6046,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>          * 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(cpu_transcoder));
> +       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;
> @@ -7788,6 +7801,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>
>         drm_mode_copy(&pipe_config->adjusted_mode, mode);
>         drm_mode_copy(&pipe_config->requested_mode, mode);
> +       pipe_config->cpu_transcoder = to_intel_crtc(crtc)->pipe;
>
>         plane_bpp = pipe_config_set_bpp(crtc, fb, pipe_config);
>         if (plane_bpp < 0)
> @@ -8038,6 +8052,8 @@ intel_pipe_config_compare(struct drm_device *dev,
>                 return false; \
>         }
>
> +       PIPE_CONF_CHECK_I(cpu_transcoder);
> +
>         PIPE_CONF_CHECK_I(has_pch_encoder);
>         PIPE_CONF_CHECK_I(fdi_lanes);
>         PIPE_CONF_CHECK_I(fdi_m_n.gmch_m);
> @@ -8189,7 +8205,6 @@ intel_modeset_check_state(struct drm_device *dev)
>                      "crtc's computed enabled state doesn't match tracked enabled state "
>                      "(expected %i, found %i)\n", enabled, crtc->base.enabled);
>
> -               pipe_config.cpu_transcoder = crtc->config.cpu_transcoder;
>                 active = dev_priv->display.get_pipe_config(crtc,
>                                                            &pipe_config);
>                 WARN(crtc->active != active,
> @@ -8252,12 +8267,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>          * to set it here already despite that we pass it down the callchain.
>          */
>         if (modeset_pipes) {
> -               enum transcoder tmp = to_intel_crtc(crtc)->config.cpu_transcoder;
>                 crtc->mode = *mode;
>                 /* mode_set/enable/disable functions rely on a correct pipe
>                  * config. */
>                 to_intel_crtc(crtc)->config = *pipe_config;
> -               to_intel_crtc(crtc)->config.cpu_transcoder = tmp;
>         }
>
>         /* Only after disabling all output pipelines that will be changed can we
> @@ -8683,7 +8696,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>         /* Swap pipes & planes for FBC on pre-965 */
>         intel_crtc->pipe = pipe;
>         intel_crtc->plane = pipe;
> -       intel_crtc->config.cpu_transcoder = pipe;
>         if (IS_MOBILE(dev) && IS_GEN3(dev)) {
>                 DRM_DEBUG_KMS("swapping pipes & planes for FBC\n");
>                 intel_crtc->plane = !pipe;
> @@ -9549,50 +9561,14 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         enum pipe pipe;
> -       u32 tmp;
>         struct drm_plane *plane;
>         struct intel_crtc *crtc;
>         struct intel_encoder *encoder;
>         struct intel_connector *connector;
>
> -       if (HAS_DDI(dev)) {
> -               tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
> -
> -               if (tmp & TRANS_DDI_FUNC_ENABLE) {
> -                       switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
> -                       case TRANS_DDI_EDP_INPUT_A_ON:
> -                       case TRANS_DDI_EDP_INPUT_A_ONOFF:
> -                               pipe = PIPE_A;
> -                               break;
> -                       case TRANS_DDI_EDP_INPUT_B_ONOFF:
> -                               pipe = PIPE_B;
> -                               break;
> -                       case TRANS_DDI_EDP_INPUT_C_ONOFF:
> -                               pipe = PIPE_C;
> -                               break;
> -                       default:
> -                               /* A bogus value has been programmed, disable
> -                                * the transcoder */
> -                               WARN(1, "Bogus eDP source %08x\n", tmp);
> -                               intel_ddi_disable_transcoder_func(dev_priv,
> -                                               TRANSCODER_EDP);
> -                               goto setup_pipes;
> -                       }
> -
> -                       crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> -                       crtc->config.cpu_transcoder = TRANSCODER_EDP;
> -
> -                       DRM_DEBUG_KMS("Pipe %c using transcoder EDP\n",
> -                                     pipe_name(pipe));
> -               }
> -       }
> -
> -setup_pipes:
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list,
>                             base.head) {
> -               enum transcoder tmp = crtc->config.cpu_transcoder;
>                 memset(&crtc->config, 0, sizeof(crtc->config));
> -               crtc->config.cpu_transcoder = tmp;
>
>                 crtc->active = dev_priv->display.get_pipe_config(crtc,
>                                                                  &crtc->config);
> --
> 1.8.1.4
>



-- 
Paulo Zanoni



More information about the Intel-gfx mailing list