drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V)

[Krzysztof Halasa]

Any feedback?

Does the silence mean I'm to send it straight to Linus, first class? :-)

> 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