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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Sun, Mar 03, 2013 at 07:01:12PM +0100, Daniel Vetter wrote:
> 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."

Right, I should have actually read the code with some thought instead
of blindly looking at the changes. I think amending the commit message
with the proposed text would be enough.

-- 
Ville Syrjälä
Intel OTC