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@xxxxxxxx> 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@xxxxxxxx>
> > 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@xxxxxxxxxxxxxxxx>
> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
