drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V)

[Krzysztof Halasa]

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>

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 57cea01..2acfc88 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1520,7 +1520,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 
 		mode = drm_mode_create(dev);
 		drm_crtc_convert_umode(mode, &crtc_req->mode);
-		drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
+		drm_mode_set_crtcinfo(mode, 0);
 	}
 
 	if (crtc_req->count_connectors == 0 && mode) {
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 9b2a541..aa38c98 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -146,7 +146,7 @@ prune:
 	list_for_each_entry_safe(mode, t, &connector->modes, head) {
 		mode->vrefresh = drm_mode_vrefresh(mode);
 
-		drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
+		drm_mode_set_crtcinfo(mode, 0);
 		drm_mode_debug_printmodeline(mode);
 	}
 
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 7196620..e890c98 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1053,7 +1053,7 @@ create_mode:
 				    cmdline_mode->refresh_specified ? cmdline_mode->refresh : 60,
 				    cmdline_mode->interlace,
 				    cmdline_mode->margins);
-	drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
+	drm_mode_set_crtcinfo(mode, 0);
 	list_add(&mode->head, &fb_helper_conn->connector->modes);
 	return mode;
 }

-- 
Krzysztof Halasa