Re: [Intel-gfx] [PATCH 03/13] drm/i915: add pipe_config-timings_set

2013-03-27 Thread Jesse Barnes 
On Wed, 27 Mar 2013 00:44:52 +0100
Daniel Vetter daniel.vet...@ffwll.ch wrote:

 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.vet...@ffwll.ch
 ---
  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 56ff8a5..3e22305 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -3970,7 +3970,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
 @@ -7532,6 +7532,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 4cc6625..054032a 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 *);
 @@ -203,6 +201,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 6ff145f..a2c516c 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

Re: [Intel-gfx] [PATCH 03/13] drm/i915: add pipe_config-timings_set

2013-03-27 Thread Daniel Vetter 
On Wed, Mar 27, 2013 at 09:49:52AM -0700, Jesse Barnes wrote:
 Changelog and summary could be better and mention the new
 compute_config function and how it replaces the mode_fixup one.
 
 Also, TIMINGS_SET probably wasn't a very good name in the first place,
 since it really deals with letter/pillar box configs.  But that's not
 really a problem with your patch and could be changed in a follow-on.
 
 Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org

Well, I've missed to add a commit addition discussed with Ville:


 
 -- 
 Jesse Barnes, Intel Open Source Technology Center
 ___
 Intel-gfx mailing list
 Intel-gfx@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/13] drm/i915: add pipe_config-timings_set

2013-03-27 Thread Jesse Barnes 
On Wed, 27 Mar 2013 00:44:52 +0100
Daniel Vetter daniel.vet...@ffwll.ch wrote:

 + 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 *);
 @@ -203,6 +201,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;

The compute_config function could actually use some kdoc instead of
putting it over the timings_set function.  It'll need to be expanded to
cover all the pipe_config bits eventually, what they mean and when they
should be set.

-- 
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/13] drm/i915: add pipe_config-timings_set

2013-03-27 Thread Daniel Vetter 
On Wed, Mar 27, 2013 at 5:49 PM, Jesse Barnes jbar...@virtuousgeek.org wrote:
 Changelog and summary could be better and mention the new
 compute_config function and how it replaces the mode_fixup one.

 Also, TIMINGS_SET probably wasn't a very good name in the first place,
 since it really deals with letter/pillar box configs.  But that's not
 really a problem with your patch and could be changed in a follow-on.

 Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org

I've forgotten to add a commit addition discussed with Ville:

Note that since the lvds code unconditionally sets the crtc timings, we
can also unconditionally set the respective flag and not just when we set
special timings like the old code did.

I'll smash another paragraph for you on top (which also address an
issue raised by Paulo):

This requires that we pass the pipe config around to encoders, so
that they can set special attributes and set constraints. To do so
introduce a new -compute_config encoder callback, which is called in
stead of the drm crtc helper's -mode_fixup.

To avoid massive churn all over the codebase we don't want to convert
all existing -mode_fixup functions. Instead I've opted to convert
them on an as-needed basis (mostly to cut down on rebase conflicts and
to have more freedom to experiment around while developing the
patches).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/13] drm/i915: add pipe_config-timings_set

2013-03-27 Thread Daniel Vetter 
On Wed, Mar 27, 2013 at 5:59 PM, Jesse Barnes jbar...@virtuousgeek.org wrote:
 @@ -203,6 +201,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;

 The compute_config function could actually use some kdoc instead of
 putting it over the timings_set function.  It'll need to be expanded to
 cover all the pipe_config bits eventually, what they mean and when they
 should be set.

Now I very much like to claim the opposite, but this isn't designed
but very much organically grown code. So imo documentation doesn't
make too much sense before things settle a bit more (the auto fdi link
dither at the end will introduce quite a bit of fun still ...).

I've promised though in my pipe_config intro a few weeks ago that I'll
create a nice blog post and doc patch once the basic stuff is settled.
I still intend to deliver on that. Is that good enough?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/13] drm/i915: add pipe_config-timings_set

2013-03-27 Thread Jesse Barnes 
On Wed, 27 Mar 2013 18:06:44 +0100
Daniel Vetter daniel.vet...@ffwll.ch wrote:

 On Wed, Mar 27, 2013 at 5:59 PM, Jesse Barnes jbar...@virtuousgeek.org 
 wrote:
  @@ -203,6 +201,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;
 
  The compute_config function could actually use some kdoc instead of
  putting it over the timings_set function.  It'll need to be expanded to
  cover all the pipe_config bits eventually, what they mean and when they
  should be set.
 
 Now I very much like to claim the opposite, but this isn't designed
 but very much organically grown code. So imo documentation doesn't
 make too much sense before things settle a bit more (the auto fdi link
 dither at the end will introduce quite a bit of fun still ...).
 
 I've promised though in my pipe_config intro a few weeks ago that I'll
 create a nice blog post and doc patch once the basic stuff is settled.
 I still intend to deliver on that. Is that good enough?

I guess so... incrementally adding to the compute_config kdoc with the
new pipe_config bits as added is too much rebase pain?

-- 
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx