Re: [PATCH] drm/i915: fix pfit regression for non-autoscaled resolutions

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jul 12, 2013 at 08:04:16AM -0700, Jesse Barnes wrote:
> On Fri, 12 Jul 2013 08:07:30 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > I.e. for letter/pillarboxing. For those cases we need to adjust the
> > mode a bit, but Jesse gmch pfit refactoring in
> > 
> > commit 2dd24552cab40ea829ba3fda890eeafd2c4816d8
> > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Date:   Thu Apr 25 12:55:01 2013 -0700
> > 
> >     drm/i915: factor out GMCH panel fitting code and use for eDP v3
> > 
> > broke that by reordering the computation of the gmch pfit state with
> > the block of code that prepared the adjusted mode for it and told the
> > modeset core not to overwrite the adjusted mode with default settings.
> > 
> > We might want to switch around the core code to just fill in defaults,
> > but this code predates the pipe_config modeset rework. And in the old
> > crtc helpers we did not have a suitable spot to do this.
> > 
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Cc: Hans de Bruin <jmdebruin@xmsnet.nl>
> > Reported-and-tested-by: Hans de Bruin <jmdebruin@xmsnet.nl>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_lvds.c  | 5 +----
> >  drivers/gpu/drm/i915/intel_panel.c | 3 +++
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index b0e1088..0536c9b 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -297,14 +297,11 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
> >  
> >  		intel_pch_panel_fitting(intel_crtc, pipe_config,
> >  					intel_connector->panel.fitting_mode);
> > -		return true;
> >  	} else {
> >  		intel_gmch_panel_fitting(intel_crtc, pipe_config,
> >  					 intel_connector->panel.fitting_mode);
> > -	}
> >  
> > -	drm_mode_set_crtcinfo(adjusted_mode, 0);
> > -	pipe_config->timings_set = true;
> > +	}
> >  
> >  	/*
> >  	 * XXX: It would be nice to support lower refresh rates on the
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 80bea1d..45010bb 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -194,6 +194,9 @@ void intel_gmch_panel_fitting(struct intel_crtc *intel_crtc,
> >  	    adjusted_mode->vdisplay == mode->vdisplay)
> >  		goto out;
> >  
> > +	drm_mode_set_crtcinfo(adjusted_mode, 0);
> > +	pipe_config->timings_set = true;
> > +
> >  	switch (fitting_mode) {
> >  	case DRM_MODE_SCALE_CENTER:
> >  		/*
> 
> This code is a bit confusing, but this looks better than what was there
> before (clobbering the adjusted_mode after calling gmch_panel_fitting
> definitely seems wrong).  Only nit is that the timings_set flag isn't
> very descriptive, but that's not the fault of this patch.

Yeah, that's just an artifact from the old crtc helper code, with the
new compute_config callbacks we could precompute sane crtc timings. I'll
fix this up for 3.12

> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Picked up for -fixes, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch