On Sat, 28 Aug 2010 19:38:45 +0200 Krzysztof Halasa <[email protected]> wrote:
> Hi, > > It seems linux commit 0cc4d4300c broke i915 interlaced mode support > while fixing another issue (broken by my patch supporting interlaced > mode). Yep, I agree resetting the mode isn't the best idea (though it's > what several drivers do) and should not be needed in the first place. > I wonder if this is all working around a completely unneeded "feature". > > The "core" modesetting Linux code does the following: > drm_mode_set_crtcinfo(adjusted_mode, CRTC_INTERLACE_HALVE_V) > What is it good for, there in the core code? > > The (any) driver (or output) either supports interlaced mode, which > forces it to revert this core operation with > "drm_mode_set_crtcinfo(adjusted_mode, 0)" (which my i915 patch did and > which may and do break special customizations), or it doesn't support > interlaced mode and then this flag isn't used at all. > > Does the following patch (+ probably removing then unneeded calls in > e.g. radeon driver) fixes all these problems for good? > > At least makes my i915 working again in interlaced mode. I could also > test on radeon, though I don't think this change could break it. > > BTW interlaced mode on i915 requires X.org patch as well, see > http://www.mail-archive.com/[email protected]/msg11512.html > (only the userland driver patch and the kernel fix (below) is needed, > the main kernel part is already in place). > Perhaps someone in charge could apply the userland patch as well? > This is needed mainly for AV applications (e.g. connecting TV-alike > displays). > > Obviously I'm open for suggestions, I'm not an X.org nor drivers/gpu > expert. > > BTW2 is there some doc, note explaining all those "adjusted_mode" > magics? Why can't individual drivers mess with such things internally > when they need so? > > BTW3 :-) I think the drm_mode_set_crtcinfo(x, CRTC_INTERLACE_HALVE_V) > logic has another flaw: > > if (p->flags & DRM_MODE_FLAG_INTERLACE) { > if (adjust_flags & CRTC_INTERLACE_HALVE_V) { > p->crtc_vdisplay /= 2; > p->crtc_vsync_start /= 2; > p->crtc_vsync_end /= 2; > p->crtc_vtotal /= 2; > } > > p->crtc_vtotal |= 1; > } > > The last line should only be applied when we don't do > CRTC_INTERLACE_HALVE_V (i.e. the total number of lines in interlaced > mode has to be odd (X lines for odd field + X lines for even field + two > half-lines) - and only when we don't count these half-lines as full ones > (and we do, most of the time). If we do CRTC_INTERLACE_HALVE_V, we got > something like 288 or 240 field lines (instead of 576 or 480 for full > frame) and forcing the odd value makes no sense. > I'm not fixing this bug since I think we should remove this > CRTC_INTERLACE_HALVE_V completely (but I'll wait for comments to the > patch below first). > > Thanks. > > Signed-off-by: Krzysztof Hałasa <[email protected]> I think this is ok; I didn't reply earlier because I didn't have the sources handy to do a full review. IIRC there were lots of good cleanups that could be done to our mode handling code, so the only issue I have with this one is that it may not go far enough in fixing up these functions to make them easier to read/use. Maybe Dave can provide suggestions along those lines. -- Jesse Barnes, Intel Open Source Technology Center _______________________________________________ [email protected]: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: [email protected]
