[PATCH 03/10] drm/i915: add pipe_config->timings_set

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

 



On Tue, Feb 26, 2013 at 02:23:23PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/2/21 Daniel Vetter <daniel.vetter at ffwll.ch>:
> > Only used by the lvds encoder. Note that we shouldn't do the same
> > simple conversion with the FORCE_6BPC flag, since that's much better
> > handled by moving all the pipe_bpc computation around.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
> In addition to Ville's question (which makes sense):
> 
> This patch does 2 things: it adds pipe_config->timings_set, but it
> also adds the encoder->compute_config function. Even though I can see
> intel_encoder->compute_config is just like drm_encoder->mode_fixup but
> with different arguments, I don't think this is a trivial thing to
> notice (imagine a future code reader, not someone looking at this
> specific patch). It would be really nice if you could add the
> compute_config callback on a separate patch and give a detailed
> documentation of the expected behavior of the function for our future
> readers. Bonus points if you could also add documentation for all the
> other intel_encoder callbacks you've created and document their
> relationship with the drm callbacks.
> 
> Also, if we agree to create a separate patch for the compute_config
> callbacks, why don't we just convert all encoders to use
> compute_config instead of mode_fixup in the very first patch? Looks
> simpler to me.

The patch set is pretty old (well, a few months at least), and when I've
started it I kinda expected it to not get merged right away. So I eshewed
doing tree-wide refactorings if not required.

The second reason is that it's almost exactly the same callback as before,
the only difference is that the mode/adjusted_mode argumentes are grouped
together into pipe_config. With a few things tacked on top. So
documentation is "it's like mode_fixup, but better".

Last but not least the current pipe configuration computation code is
really hapzardous and there's a lot of interdependent stuff going on. So
right now I think nothing short of just reading the entire modeset code is
documentation enough. So I don't think adding documentation for this in
the middle of the core reorg makes much sense. I've tried to add useful
documentation for most of the new pipe_config attributes though.

Can I bribe you with the promise that I'll supply a nice blog post and
documenatation patches once the basic infrastructure has settled instead?

Cheers, Daniel

> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++-
> >  drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++----
> >  drivers/gpu/drm/i915/intel_lvds.c    | 19 +++++++++----------
> >  3 files changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0f61008..ad03b7f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3930,7 +3930,7 @@ static bool intel_crtc_compute_config(struct drm_crtc *crtc,
> >         /* All interlaced capable intel hw wants timings in frames. Note though
> >          * that intel_lvds_mode_fixup does some funny tricks with the crtc
> >          * timings, so we need to be careful not to clobber these.*/
> > -       if (!(adjusted_mode->private_flags & INTEL_MODE_CRTC_TIMINGS_SET))
> > +       if (!pipe_config->timings_set)
> >                 drm_mode_set_crtcinfo(adjusted_mode, 0);
> >
> >         /* WaPruneModeWithIncorrectHsyncOffset: Cantiga+ cannot handle modes
> > @@ -7509,6 +7509,16 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> >
> >                 if (&encoder->new_crtc->base != crtc)
> >                         continue;
> > +
> > +               if (encoder->compute_config) {
> > +                       if (!(encoder->compute_config(encoder, pipe_config))) {
> > +                               DRM_DEBUG_KMS("Encoder config failure\n");
> > +                               goto fail;
> > +                       }
> > +
> > +                       continue;
> > +               }
> > +
> >                 encoder_funcs = encoder->base.helper_private;
> >                 if (!(encoder_funcs->mode_fixup(&encoder->base,
> >                                                 &pipe_config->requested_mode,
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index eca75b6..edafbef 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -105,10 +105,6 @@
> >  #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)
> > -/* This flag must be set by the encoder's mode_fixup if it changes the crtc
> > - * timings in the mode to prevent the crtc fixup from overwriting them.
> > - * Currently only lvds needs that. */
> > -#define INTEL_MODE_CRTC_TIMINGS_SET (0x20)
> >  /*
> >   * Set when limited 16-235 (as opposed to full 0-255) RGB color range is
> >   * to be used.
> > @@ -158,6 +154,8 @@ struct intel_encoder {
> >         bool cloneable;
> >         bool connectors_active;
> >         void (*hot_plug)(struct intel_encoder *);
> > +       bool (*compute_config)(struct intel_encoder *,
> > +                              struct intel_crtc_config *);
> >         void (*pre_pll_enable)(struct intel_encoder *);
> >         void (*pre_enable)(struct intel_encoder *);
> >         void (*enable)(struct intel_encoder *);
> > @@ -202,6 +200,10 @@ struct intel_connector {
> >  struct intel_crtc_config {
> >         struct drm_display_mode requested_mode;
> >         struct drm_display_mode adjusted_mode;
> > +       /* This flag must be set by the encoder's compute_config callback if it
> > +        * changes the crtc timings in the mode to prevent the crtc fixup from
> > +        * overwriting them.  Currently only lvds needs that. */
> > +       bool timings_set;
> >  };
> >
> >  struct intel_crtc {
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 3d1d974..1616f53 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -261,8 +261,6 @@ centre_horizontally(struct drm_display_mode *mode,
> >
> >         mode->crtc_hsync_start = mode->crtc_hblank_start + sync_pos;
> >         mode->crtc_hsync_end = mode->crtc_hsync_start + sync_width;
> > -
> > -       mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
> >  }
> >
> >  static void
> > @@ -284,8 +282,6 @@ centre_vertically(struct drm_display_mode *mode,
> >
> >         mode->crtc_vsync_start = mode->crtc_vblank_start + sync_pos;
> >         mode->crtc_vsync_end = mode->crtc_vsync_start + sync_width;
> > -
> > -       mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
> >  }
> >
> >  static inline u32 panel_fitter_scaling(u32 source, u32 target)
> > @@ -301,15 +297,17 @@ static inline u32 panel_fitter_scaling(u32 source, u32 target)
> >         return (FACTOR * ratio + FACTOR/2) / FACTOR;
> >  }
> >
> > -static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
> > -                                 const struct drm_display_mode *mode,
> > -                                 struct drm_display_mode *adjusted_mode)
> > +static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
> > +                                     struct intel_crtc_config *pipe_config)
> >  {
> > -       struct drm_device *dev = encoder->dev;
> > +       struct drm_device *dev = intel_encoder->base.dev;
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -       struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(encoder);
> > +       struct intel_lvds_encoder *lvds_encoder =
> > +               to_lvds_encoder(&intel_encoder->base);
> >         struct intel_connector *intel_connector =
> >                 &lvds_encoder->attached_connector->base;
> > +       struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> > +       struct drm_display_mode *mode = &pipe_config->requested_mode;
> >         struct intel_crtc *intel_crtc = lvds_encoder->base.new_crtc;
> >         u32 pfit_control = 0, pfit_pgm_ratios = 0, border = 0;
> >         int pipe;
> > @@ -359,6 +357,7 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
> >                 I915_WRITE(BCLRPAT(pipe), 0);
> >
> >         drm_mode_set_crtcinfo(adjusted_mode, 0);
> > +       pipe_config->timings_set = true;
> >
> >         switch (intel_connector->panel.fitting_mode) {
> >         case DRM_MODE_SCALE_CENTER:
> > @@ -661,7 +660,6 @@ static int intel_lvds_set_property(struct drm_connector *connector,
> >  }
> >
> >  static const struct drm_encoder_helper_funcs intel_lvds_helper_funcs = {
> > -       .mode_fixup = intel_lvds_mode_fixup,
> >         .mode_set = intel_lvds_mode_set,
> >  };
> >
> > @@ -1102,6 +1100,7 @@ bool intel_lvds_init(struct drm_device *dev)
> >         intel_encoder->enable = intel_enable_lvds;
> >         intel_encoder->pre_enable = intel_pre_enable_lvds;
> >         intel_encoder->pre_pll_enable = intel_pre_pll_enable_lvds;
> > +       intel_encoder->compute_config = intel_lvds_compute_config;
> >         intel_encoder->disable = intel_disable_lvds;
> >         intel_encoder->get_hw_state = intel_lvds_get_hw_state;
> >         intel_connector->get_hw_state = intel_connector_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