Re: [PATCH 1/4] drm/i915: fixup interlaced vertical timings confusion

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jan 26, 2012 at 11:11:30PM +0100, Daniel Vetter wrote:
> On Thu, Jan 26, 2012 at 10:03:02PM +0000, Chris Wilson wrote:
> > On Thu, 26 Jan 2012 22:01:30 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > -	/* XXX some encoders set the crtcinfo, others don't.
> > > -	 * Obviously we need some form of conflict resolution here...
> > > -	 */
> > > -	if (adjusted_mode->crtc_htotal == 0)
> > > +	/* gen2 needs vertical crtc timing information in fields because that's
> > > +	 * what dvo outputs want - the chip itself can't do interlaced. All
> > > +	 * later generations can do interlaced natively and want timings in
> > > +	 * full frames. */
> > > +	if (IS_GEN2(dev))
> > > +		drm_mode_set_crtcinfo(adjusted_mode, CRTC_INTERLACE_HALVE_V);
> > > +	else
> > >  		drm_mode_set_crtcinfo(adjusted_mode, 0);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> > > index 6eda1b5..020a7d7 100644
> > > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > > @@ -157,7 +157,6 @@ static bool intel_dvo_mode_fixup(struct drm_encoder *encoder,
> > >  		C(vsync_end);
> > >  		C(vtotal);
> > >  		C(clock);
> > > -		drm_mode_set_crtcinfo(adjusted_mode, CRTC_INTERLACE_HALVE_V);
> > >  #undef C
> > >  	}
> > 
> > Removing drm_mode_set_crtcinfo() scares me because of the above comment.
> > We need to make sure that the adjusted_mode is initialised along
> > some path, and the fixup in intel_crtc_mode_fixup is just a hack.
> > 
> > commit 897493504addc5609f04a2c4f73c37ab972c29b2
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Sun Sep 12 18:25:19 2010 +0100
> > 
> >     drm/i915: Ensure that the crtcinfo is populated during mode_fixup()
> >     
> >     This should fix the mysterious mode setting failures reported during
> >     boot up and after resume, generally for i8xx class machines.
> >     
> >     Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=16478
> >     Reported-and-tested-by: Xavier Chantry <chantry.xavier@gmail.com>
> >     Buzilla: https://bugs.freedesktop.org/show_bug.cgi?id=29413
> >     Tested-by: Daniel Vetter <daniel@ffwll.ch>
> >     Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >     Cc: stable@kernel.org
> > 
> > If you can work out exactly where it should be initialised, you'll be my
> > hero!
> 
> Afaik drm core calling set_crtcinfo is just a side-effect of some random
> actions and it's not guaranteed to happen. The issue is that drm core puts
> the wrong stuff in there. Also note that I'm now unconditionally doing the
> fixup, so the particular bug above shouldn't show up again.
> 
> I'll check whether this is indeed the case on my dear old i855gm - let's
> see whether I'm provoking the wrath of some ill-tempered gods here ;-)

Seems the volcano gods haven't noticed yet what I'm doing here, i.e. works
on my i855gm.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48