[Intel-gfx] [PATCH 13/13] drm/i915: clean up crtc timings computation

[Daniel Vetter]

On Fri, Jul 26, 2013 at 03:40:52PM -0300, Rodrigo Vivi wrote:
> On Sun, Jul 21, 2013 at 4:37 PM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> > In the old days of the crtc helpers we've only had the encoder and
> > crtc ->mode_fixup callbacks. So when the lvds connector wanted to
> > adjust the crtc timings it had to set a driver-private mode flag to
> > tell the crtc mode fixup code to not overwrite them with the generic
> > ones.
> >
> > When converting things to the new infrastructure I've kept the entire
> > logic and only moved the flag to pipe_config->timings_set. But this
> > logic is pretty tricky and already caused regressions:
> >
> > commit 21d8a4756af5fdf4a42e79a77cf3b6f52678d443
> > Author: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Date:   Fri Jul 12 08:07:30 2013 +0200
> >
> >     drm/i915: fix pfit regression for non-autoscaled resolutions
> >
> > So take advantage of the flexibility our own modeset infrastructure
> > affords us and prefill default crtc timings. This allows us to rip out
> > ->timings_set. Note that we overwrite things again when retrying the
> > pipe config computation due to bandwidth constraints to avoid bogus
> > crtc timings if the encoder only does relative adjustments (which is
> > how the pfit code works). Only a theoretical concern though since
> > platforms where we retry (pch-split platforms) do not need
> > adjustements (since only the old gmch pfit needs that). But let's
> > better be safe than sorry.
> >
> > Cc: Jesse Barnes <jbarnes at virtuousgeek.org>
> > Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 9 +++------
> >  drivers/gpu/drm/i915/intel_drv.h     | 4 ----
> >  drivers/gpu/drm/i915/intel_panel.c   | 3 ---
> >  3 files changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b07f891e..aebdadc 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4108,12 +4108,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
> >                         return -EINVAL;
> >         }
> >
> > -       /* All interlaced capable intel hw wants timings in frames. Note though
> > -        * that intel_lvds_compute_config does some funny tricks with the crtc
> > -        * timings, so we need to be careful not to clobber these.*/
> > -       if (!pipe_config->timings_set)
> > -               drm_mode_set_crtcinfo(adjusted_mode, 0);
> > -
> >         /* Cantiga+ cannot handle modes with a hsync front porch of 0.
> >          * WaPruneModeWithIncorrectHsyncOffset:ctg,elk,ilk,snb,ivb,vlv,hsw.
> >          */
> > @@ -7882,6 +7876,9 @@ encoder_retry:
> >         pipe_config->port_clock = 0;
> >         pipe_config->pixel_multiplier = 1;
> >
> > +       /* Fill in default crtc timings, allow encoders to overwrite them. */
> > +       drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, 0);
> > +
> >         /* Pass our mode to the connectors and the CRTC to give them a chance to
> >          * adjust it according to limitations or connector properties, and also
> >          * a chance to reject the mode entirely.
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 31087ff..a9eca0e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -208,10 +208,6 @@ 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;
> >         /* Whether to set up the PCH/FDI. Note that we never allow sharing
> >          * between pch encoders and cpu encoders. */
> >         bool has_pch_encoder;
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 67e2c1f..01b5a51 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -194,9 +194,6 @@ void intel_gmch_panel_fitting(struct intel_crtc *intel_crtc,
> >             adjusted_mode->vdisplay == mode->vdisplay)
> >                 goto out;
> >
> > -       drm_mode_set_crtcinfo(adjusted_mode, 0);
> 
> I didn't get why this set here isn't needed anymore..

The drm_mode_set_crtcinfo in intel_crtc_compute_config happens before we
call down into encoder->compute_config functions. Since the gmch pfit code
here is only called from those callbacks (for lvds + edp on vlv) we can
drop this one here and simplify the logic a bit.
-Daniel

> 
> > -       pipe_config->timings_set = true;
> > -
> >         switch (fitting_mode) {
> >         case DRM_MODE_SCALE_CENTER:
> >                 /*
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

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