[PATCH 04/10] drm/i915: add pipe_config->pixel_multiplier

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 26, 2013 at 02:27:36PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/2/21 Daniel Vetter <daniel.vetter at ffwll.ch>:
> > Used by SDVO (and hopefully, eventually HDMI, if we ever get around
> > to fixing up the low dotclock CEA modes ...).
> >
> > This required adding a new encoder->mode_set callback to be able to
> > pass around the intel_crtc_config.
> 
> Again, can we please separate the callback in another patch and add
> some nice documentation explaining what's this callback, why it's
> called and what's its relationship with the other mode_set callbacks
> (again, think about future code readers, not people reading the
> patch)? Also, why not just convert every encoder to this callback
> right now?

Same reasons as for the ->compute_config part - less potential for
conflicts and it's really the same function. The only difference is that
the old ->mode_set only gets mode&adjusted mode, but since without further
changes that's all they're interested in.

On a quick survey of the final state after all patches only sdvo/tv
encoders have such a new mode_set callback. For the compute_config
callback only dvo still uses ->mode_fixup.

I guess we could convert all the leftover ones to the new interfaces once
things settle a bit. But for now I'd like to avoid changing things just
because to avoid needless conflicts.
-Daniel

> 
> The pixel_multiplier part of the patch looks correct.
> 
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 80 +++++++++++++++++++-----------------
> >  drivers/gpu/drm/i915/intel_drv.h     | 19 ++-------
> >  drivers/gpu/drm/i915/intel_sdvo.c    | 39 +++++++++---------
> >  3 files changed, 66 insertions(+), 72 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ad03b7f..3446e2b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4280,14 +4280,15 @@ static void i9xx_update_pll_dividers(struct drm_crtc *crtc,
> >  }
> >
> >  static void vlv_update_pll(struct drm_crtc *crtc,
> > -                          struct drm_display_mode *mode,
> > -                          struct drm_display_mode *adjusted_mode,
> >                            intel_clock_t *clock, intel_clock_t *reduced_clock,
> >                            int num_connectors)
> >  {
> >         struct drm_device *dev = crtc->dev;
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +       struct drm_display_mode *adjusted_mode =
> > +               &intel_crtc->config.adjusted_mode;
> > +       struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
> >         int pipe = intel_crtc->pipe;
> >         u32 dpll, mdiv, pdiv;
> >         u32 bestn, bestm1, bestm2, bestp1, bestp2;
> > @@ -4354,11 +4355,11 @@ static void vlv_update_pll(struct drm_crtc *crtc,
> >
> >         temp = 0;
> >         if (is_sdvo) {
> > -               temp = intel_mode_get_pixel_multiplier(adjusted_mode);
> > -               if (temp > 1)
> > -                       temp = (temp - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> > -               else
> > -                       temp = 0;
> > +               temp = 0;
> > +               if (intel_crtc->config.pixel_multiplier > 1) {
> > +                       temp = (intel_crtc->config.pixel_multiplier - 1)
> > +                               << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> > +               }
> >         }
> >         I915_WRITE(DPLL_MD(pipe), temp);
> >         POSTING_READ(DPLL_MD(pipe));
> > @@ -4384,14 +4385,15 @@ static void vlv_update_pll(struct drm_crtc *crtc,
> >  }
> >
> >  static void i9xx_update_pll(struct drm_crtc *crtc,
> > -                           struct drm_display_mode *mode,
> > -                           struct drm_display_mode *adjusted_mode,
> >                             intel_clock_t *clock, intel_clock_t *reduced_clock,
> >                             int num_connectors)
> >  {
> >         struct drm_device *dev = crtc->dev;
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +       struct drm_display_mode *adjusted_mode =
> > +               &intel_crtc->config.adjusted_mode;
> > +       struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
> >         struct intel_encoder *encoder;
> >         int pipe = intel_crtc->pipe;
> >         u32 dpll;
> > @@ -4408,11 +4410,12 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
> >                 dpll |= DPLLB_MODE_LVDS;
> >         else
> >                 dpll |= DPLLB_MODE_DAC_SERIAL;
> > +
> >         if (is_sdvo) {
> > -               int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> > -               if (pixel_multiplier > 1) {
> > -                       if (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev))
> > -                               dpll |= (pixel_multiplier - 1) << SDVO_MULTIPLIER_SHIFT_HIRES;
> > +               if ((intel_crtc->config.pixel_multiplier > 1) &&
> > +                   (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev))) {
> > +                       dpll |= (intel_crtc->config.pixel_multiplier - 1)
> > +                               << SDVO_MULTIPLIER_SHIFT_HIRES;
> >                 }
> >                 dpll |= DPLL_DVO_HIGH_SPEED;
> >         }
> > @@ -4477,11 +4480,11 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
> >         if (INTEL_INFO(dev)->gen >= 4) {
> >                 u32 temp = 0;
> >                 if (is_sdvo) {
> > -                       temp = intel_mode_get_pixel_multiplier(adjusted_mode);
> > -                       if (temp > 1)
> > -                               temp = (temp - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> > -                       else
> > -                               temp = 0;
> > +                       temp = 0;
> > +                       if (intel_crtc->config.pixel_multiplier > 1) {
> > +                               temp = (intel_crtc->config.pixel_multiplier - 1)
> > +                                       << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> > +                       }
> >                 }
> >                 I915_WRITE(DPLL_MD(pipe), temp);
> >         } else {
> > @@ -4691,11 +4694,11 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> >                                 has_reduced_clock ? &reduced_clock : NULL,
> >                                 num_connectors);
> >         else if (IS_VALLEYVIEW(dev))
> > -               vlv_update_pll(crtc, mode, adjusted_mode, &clock,
> > +               vlv_update_pll(crtc, &clock,
> >                                 has_reduced_clock ? &reduced_clock : NULL,
> >                                 num_connectors);
> >         else
> > -               i9xx_update_pll(crtc, mode, adjusted_mode, &clock,
> > +               i9xx_update_pll(crtc, &clock,
> >                                 has_reduced_clock ? &reduced_clock : NULL,
> >                                 num_connectors);
> >
> > @@ -5407,17 +5410,18 @@ int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp)
> >         return bps / (link_bw * 8) + 1;
> >  }
> >
> > -static void ironlake_set_m_n(struct drm_crtc *crtc,
> > -                            struct drm_display_mode *mode,
> > -                            struct drm_display_mode *adjusted_mode)
> > +static void ironlake_set_m_n(struct drm_crtc *crtc)
> >  {
> >         struct drm_device *dev = crtc->dev;
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +       struct drm_display_mode *adjusted_mode =
> > +               &intel_crtc->config.adjusted_mode;
> > +       struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
> >         enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
> >         struct intel_encoder *intel_encoder, *edp_encoder = NULL;
> >         struct intel_link_m_n m_n = {0};
> > -       int target_clock, pixel_multiplier, lane, link_bw;
> > +       int target_clock, lane, link_bw;
> >         bool is_dp = false, is_cpu_edp = false;
> >
> >         for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
> > @@ -5435,7 +5439,6 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
> >         }
> >
> >         /* FDI link */
> > -       pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> >         lane = 0;
> >         /* CPU eDP doesn't require FDI link, so just set DP M/N
> >            according to current link config */
> > @@ -5466,8 +5469,8 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
> >
> >         intel_crtc->fdi_lanes = lane;
> >
> > -       if (pixel_multiplier > 1)
> > -               link_bw *= pixel_multiplier;
> > +       if (intel_crtc->config.pixel_multiplier > 1)
> > +               link_bw *= intel_crtc->config.pixel_multiplier;
> >         intel_link_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw, &m_n);
> >
> >         I915_WRITE(PIPE_DATA_M1(cpu_transcoder), TU_SIZE(m_n.tu) | m_n.gmch_m);
> > @@ -5477,7 +5480,6 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
> >  }
> >
> >  static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> > -                                     struct drm_display_mode *adjusted_mode,
> >                                       intel_clock_t *clock, u32 fp)
> >  {
> >         struct drm_crtc *crtc = &intel_crtc->base;
> > @@ -5485,7 +5487,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct intel_encoder *intel_encoder;
> >         uint32_t dpll;
> > -       int factor, pixel_multiplier, num_connectors = 0;
> > +       int factor, num_connectors = 0;
> >         bool is_lvds = false, is_sdvo = false, is_tv = false;
> >         bool is_dp = false, is_cpu_edp = false;
> >
> > @@ -5536,9 +5538,9 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> >         else
> >                 dpll |= DPLLB_MODE_DAC_SERIAL;
> >         if (is_sdvo) {
> > -               pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> > -               if (pixel_multiplier > 1) {
> > -                       dpll |= (pixel_multiplier - 1) << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
> > +               if (intel_crtc->config.pixel_multiplier > 1) {
> > +                       dpll |= (intel_crtc->config.pixel_multiplier - 1)
> > +                               << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
> >                 }
> >                 dpll |= DPLL_DVO_HIGH_SPEED;
> >         }
> > @@ -5642,7 +5644,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> >                 fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
> >                         reduced_clock.m2;
> >
> > -       dpll = ironlake_compute_dpll(intel_crtc, adjusted_mode, &clock, fp);
> > +       dpll = ironlake_compute_dpll(intel_crtc, &clock, fp);
> >
> >         DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
> >         drm_mode_debug_printmodeline(mode);
> > @@ -5696,7 +5698,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> >
> >         /* Note, this also computes intel_crtc->fdi_lanes which is used below in
> >          * ironlake_check_fdi_lanes. */
> > -       ironlake_set_m_n(crtc, mode, adjusted_mode);
> > +       ironlake_set_m_n(crtc);
> >
> >         fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
> >
> > @@ -5812,7 +5814,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> >         intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
> >
> >         if (!is_dp || is_cpu_edp)
> > -               ironlake_set_m_n(crtc, mode, adjusted_mode);
> > +               ironlake_set_m_n(crtc);
> >
> >         haswell_set_pipeconf(crtc, adjusted_mode, dither);
> >
> > @@ -5865,8 +5867,12 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
> >                         encoder->base.base.id,
> >                         drm_get_encoder_name(&encoder->base),
> >                         mode->base.id, mode->name);
> > -               encoder_funcs = encoder->base.helper_private;
> > -               encoder_funcs->mode_set(&encoder->base, mode, adjusted_mode);
> > +               if (encoder->mode_set) {
> > +                       encoder->mode_set(encoder);
> > +               } else {
> > +                       encoder_funcs = encoder->base.helper_private;
> > +                       encoder_funcs->mode_set(&encoder->base, mode, adjusted_mode);
> > +               }
> >         }
> >
> >         return 0;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index edafbef..ef5111b 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -102,8 +102,6 @@
> >  #define INTEL_DVO_CHIP_TVOUT 4
> >
> >  /* drm_display_mode->private_flags */
> > -#define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0)
> > -#define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT)
> >  #define INTEL_MODE_DP_FORCE_6BPC (0x10)
> >  /*
> >   * Set when limited 16-235 (as opposed to full 0-255) RGB color range is
> > @@ -111,20 +109,6 @@
> >   */
> >  #define INTEL_MODE_LIMITED_COLOR_RANGE (0x40)
> >
> > -static inline void
> > -intel_mode_set_pixel_multiplier(struct drm_display_mode *mode,
> > -                               int multiplier)
> > -{
> > -       mode->clock *= multiplier;
> > -       mode->private_flags |= multiplier;
> > -}
> > -
> > -static inline int
> > -intel_mode_get_pixel_multiplier(const struct drm_display_mode *mode)
> > -{
> > -       return (mode->private_flags & INTEL_MODE_PIXEL_MULTIPLIER_MASK) >> INTEL_MODE_PIXEL_MULTIPLIER_SHIFT;
> > -}
> > -
> >  struct intel_framebuffer {
> >         struct drm_framebuffer base;
> >         struct drm_i915_gem_object *obj;
> > @@ -159,6 +143,7 @@ struct intel_encoder {
> >         void (*pre_pll_enable)(struct intel_encoder *);
> >         void (*pre_enable)(struct intel_encoder *);
> >         void (*enable)(struct intel_encoder *);
> > +       void (*mode_set)(struct intel_encoder *intel_encoder);
> >         void (*disable)(struct intel_encoder *);
> >         void (*post_disable)(struct intel_encoder *);
> >         /* Read out the current hw state of this connector, returning true if
> > @@ -204,6 +189,8 @@ struct intel_crtc_config {
> >          * changes the crtc timings in the mode to prevent the crtc fixup from
> >          * overwriting them.  Currently only lvds needs that. */
> >         bool timings_set;
> > +       /* Used by SDVO (and if we ever fix it, HDMI). */
> > +       unsigned pixel_multiplier;
> >  };
> >
> >  struct intel_crtc {
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index 33b46d9..06bb2e1 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -788,7 +788,6 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
> >         v_sync_offset = mode->vsync_start - mode->vdisplay;
> >
> >         mode_clock = mode->clock;
> > -       mode_clock /= intel_mode_get_pixel_multiplier(mode) ?: 1;
> >         mode_clock /= 10;
> >         dtd->part1.clock = mode_clock;
> >
> > @@ -1039,12 +1038,12 @@ intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> >         return true;
> >  }
> >
> > -static bool intel_sdvo_mode_fixup(struct drm_encoder *encoder,
> > -                                 const struct drm_display_mode *mode,
> > -                                 struct drm_display_mode *adjusted_mode)
> > +static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
> > +                                     struct intel_crtc_config *pipe_config)
> >  {
> > -       struct intel_sdvo *intel_sdvo = to_intel_sdvo(encoder);
> > -       int multiplier;
> > +       struct intel_sdvo *intel_sdvo = to_intel_sdvo(&encoder->base);
> > +       struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> > +       struct drm_display_mode *mode = &pipe_config->requested_mode;
> >
> >         /* We need to construct preferred input timings based on our
> >          * output timings.  To do that, we have to set the output
> > @@ -1071,8 +1070,9 @@ static bool intel_sdvo_mode_fixup(struct drm_encoder *encoder,
> >         /* Make the CRTC code factor in the SDVO pixel multiplier.  The
> >          * SDVO device will factor out the multiplier during mode_set.
> >          */
> > -       multiplier = intel_sdvo_get_pixel_multiplier(adjusted_mode);
> > -       intel_mode_set_pixel_multiplier(adjusted_mode, multiplier);
> > +       pipe_config->pixel_multiplier =
> > +               intel_sdvo_get_pixel_multiplier(adjusted_mode);
> > +       adjusted_mode->clock *= pipe_config->pixel_multiplier;
> >
> >         if (intel_sdvo->color_range_auto) {
> >                 /* See CEA-861-E - 5.1 Default Encoding Parameters */
> > @@ -1089,19 +1089,19 @@ static bool intel_sdvo_mode_fixup(struct drm_encoder *encoder,
> >         return true;
> >  }
> >
> > -static void intel_sdvo_mode_set(struct drm_encoder *encoder,
> > -                               struct drm_display_mode *mode,
> > -                               struct drm_display_mode *adjusted_mode)
> > +static void intel_sdvo_mode_set(struct intel_encoder *intel_encoder)
> >  {
> > -       struct drm_device *dev = encoder->dev;
> > +       struct drm_device *dev = intel_encoder->base.dev;
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -       struct drm_crtc *crtc = encoder->crtc;
> > +       struct drm_crtc *crtc = intel_encoder->base.crtc;
> >         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -       struct intel_sdvo *intel_sdvo = to_intel_sdvo(encoder);
> > +       struct drm_display_mode *adjusted_mode =
> > +               &intel_crtc->config.adjusted_mode;
> > +       struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
> > +       struct intel_sdvo *intel_sdvo = to_intel_sdvo(&intel_encoder->base);
> >         u32 sdvox;
> >         struct intel_sdvo_in_out_map in_out;
> >         struct intel_sdvo_dtd input_dtd, output_dtd;
> > -       int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> >         int rate;
> >
> >         if (!mode)
> > @@ -1161,7 +1161,7 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
> >                 DRM_INFO("Setting input timings on %s failed\n",
> >                          SDVO_NAME(intel_sdvo));
> >
> > -       switch (pixel_multiplier) {
> > +       switch (intel_crtc->config.pixel_multiplier) {
> >         default:
> >         case 1: rate = SDVO_CLOCK_RATE_MULT_1X; break;
> >         case 2: rate = SDVO_CLOCK_RATE_MULT_2X; break;
> > @@ -1205,7 +1205,8 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
> >         } else if (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev)) {
> >                 /* done in crtc_mode_set as it lives inside the dpll register */
> >         } else {
> > -               sdvox |= (pixel_multiplier - 1) << SDVO_PORT_MULTIPLY_SHIFT;
> > +               sdvox |= (intel_crtc->config.pixel_multiplier - 1)
> > +                       << SDVO_PORT_MULTIPLY_SHIFT;
> >         }
> >
> >         if (input_dtd.part2.sdvo_flags & SDVO_NEED_TO_STALL &&
> > @@ -2041,8 +2042,6 @@ done:
> >  }
> >
> >  static const struct drm_encoder_helper_funcs intel_sdvo_helper_funcs = {
> > -       .mode_fixup = intel_sdvo_mode_fixup,
> > -       .mode_set = intel_sdvo_mode_set,
> >  };
> >
> >  static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
> > @@ -2781,7 +2780,9 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
> >
> >         drm_encoder_helper_add(&intel_encoder->base, &intel_sdvo_helper_funcs);
> >
> > +       intel_encoder->compute_config = intel_sdvo_compute_config;
> >         intel_encoder->disable = intel_disable_sdvo;
> > +       intel_encoder->mode_set = intel_sdvo_mode_set;
> >         intel_encoder->enable = intel_enable_sdvo;
> >         intel_encoder->get_hw_state = intel_sdvo_get_hw_state;
> >
> > --
> > 1.7.11.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux