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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Feb 22, 2013 at 03:51:15PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 22, 2013 at 12:56:47AM +0100, Daniel Vetter 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.vetter@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(-)
> > 
> <snip>
> > @@ -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;
> 
> This changes the behaviour a bit. Previously the flag was only set from
> the centering funcs, but now it's set always. Is that intentional?

The crtc code also calls drm_mode_set_crtcinfo(adjusted_mode, 0) if
->timings_set is false, so doesn't result in any behaviour change. Hence
I've figure that fewer lines of code should be better. Want me to change
it back or just add a bit of text to the commit message? E.g.

"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."

Cheers, Daniel

> 
> >  
> >  	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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

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