On Tue, 24 Apr 2012 17:37:56 +0200, Daniel Vetter <daniel at ffwll.ch> wrote: > On Tue, Apr 24, 2012 at 04:11:43PM +0100, Chris Wilson wrote: > > On Tue, 24 Apr 2012 15:41:37 +0200, Daniel Vetter <daniel.vetter at ffwll.ch> wrote: > > > the only places we actually need the crtc timings is in the mode_set > > > function. > > > > > > So we can now safely rip out all the remaining calls to set_crtcinfo > > > left in the driver and clean up this confusion. > > > > I have a little flicker of doubt because these tend to end up being > > passed into the core drm as well as used during modeset. Even looking > > through the instances of drm_mode_set_crtcinfo() in the core, I'm left > > no the wiser as to which functions expect crtc_* to be filled in. As far > > I can see the only place where set_crtcinfo() must be called is prior to > > the final modeset, and so the crtc_* values are only ever used on the > > adjusted_mode. Hence why the drm usage leaves me slightly puzzled. > > I guess the idea of the drm core is that every time it creates a drm mode, > it also sets the timings. But afaics it never uses them, safe for the > precise vblank timestamp code (but that can only run on active modes, i.e. > after our mode_fixup functions have been called). The problem is that drm > core always sets CRTC_INTERLACE_HALVE_V, so the timings are pretty much > bogus for us anyway (at least with interlaced support). > > So I guess it's the drivers job that every active modes needs to have crtc > timings that suits it, and with these patches we should have that. drm > core doesn't seem to care about modes that just get passed around. So just capture that explanation in the commit message and get QA's seal of approval and job done. The patch itself looks good and complete afaict, so r-b with the above. -Chris -- Chris Wilson, Intel Open Source Technology Centre
