drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V)

[Jesse Barnes]

On Sat, 28 Aug 2010 19:38:45 +0200
Krzysztof Halasa <khc at pm.waw.pl> 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/xorg@lists.freedesktop.org/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 <khc at pm.waw.pl>

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