Re: [PATCH 1/4] drm/i915: handle input/output sdvo timings separately in mode_set

[Jesse Barnes <jbarnes@virtuousgeek.org>]

[-- Attachment #1.1: Type: text/plain, Size: 5716 bytes --]

On Tue, 10 Apr 2012 13:55:46 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> We seem to have a decent confusion between the output timings and the
> input timings of the sdvo encoder. If I understand the code correctly,
> we use the original mode unchanged for the output timings, safe for
> the lvds case. And we should use the adjusted mode for input timings.
> 
> Clarify the situation by adding an explicit output_dtd to the sdvo
> mode_set function and streamline the code-flow by moving the input and
> output mode setting in the sdvo encode together.
> 
> Furthermore testing showed that the sdvo input timing needs the
> unadjusted dotclock, the sdvo chip will automatically compute the
> required pixel multiplier to get a dotclock above 100 MHz.
> 
> Fix this up when converting a drm mode to an sdvo dtd.
> 
> This regression was introduced in
> 
> commit c74696b9c890074c1e1ee3d7496fc71eb3680ced
> Author: Pavel Roskin <proski@gnu.org>
> Date:   Thu Sep 2 14:46:34 2010 -0400
> 
>     i915: revert some checks added by commit 32aad86f
> 
> particularly the following hunk:
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
> b/drivers/gpu/drm/i915/intel_sdvo.c
> index 093e914..62d22ae 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1122,11 +1123,9 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
> 
>      /* We have tried to get input timing in mode_fixup, and filled into
>         adjusted_mode */
> -    if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
> -        intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> +    intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> +    if (intel_sdvo->is_tv || intel_sdvo->is_lvds)
>          input_dtd.part2.sdvo_flags = intel_sdvo->sdvo_flags;
> -    } else
> -        intel_sdvo_get_dtd_from_mode(&input_dtd, mode);
> 
>      /* If it's a TV, we already set the output timing in mode_fixup.
>       * Otherwise, the output timing is equal to the input timing.
> 
> Reported-and-Tested-by: Bernard Blackham <b-linuxgit@largestprime.net>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48157
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_sdvo.c |   34 ++++++++++++++++++----------------
>  1 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 6898145..ab47c1e 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -733,6 +733,7 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
>  	uint16_t width, height;
>  	uint16_t h_blank_len, h_sync_len, v_blank_len, v_sync_len;
>  	uint16_t h_sync_offset, v_sync_offset;
> +	int mode_clock;
>  
>  	width = mode->crtc_hdisplay;
>  	height = mode->crtc_vdisplay;
> @@ -747,7 +748,11 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
>  	h_sync_offset = mode->crtc_hsync_start - mode->crtc_hblank_start;
>  	v_sync_offset = mode->crtc_vsync_start - mode->crtc_vblank_start;
>  
> -	dtd->part1.clock = mode->clock / 10;
> +	mode_clock = mode->clock;
> +	mode_clock /= intel_mode_get_pixel_multiplier(mode) ?: 1;
> +	mode_clock /= 10;
> +	dtd->part1.clock = mode_clock;
> +
>  	dtd->part1.h_active = width & 0xff;
>  	dtd->part1.h_blank = h_blank_len & 0xff;
>  	dtd->part1.h_high = (((width >> 8) & 0xf) << 4) |

This hunk looks good.

> @@ -998,7 +1003,7 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>  	struct intel_sdvo *intel_sdvo = to_intel_sdvo(encoder);
>  	u32 sdvox;
>  	struct intel_sdvo_in_out_map in_out;
> -	struct intel_sdvo_dtd input_dtd;
> +	struct intel_sdvo_dtd input_dtd, output_dtd;
>  	int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
>  	int rate;
>  
> @@ -1023,20 +1028,13 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>  					  intel_sdvo->attached_output))
>  		return;
>  
> -	/* We have tried to get input timing in mode_fixup, and filled into
> -	 * adjusted_mode.
> -	 */
> -	if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
> -		input_dtd = intel_sdvo->input_dtd;
> -	} else {
> -		/* Set the output timing to the screen */
> -		if (!intel_sdvo_set_target_output(intel_sdvo,
> -						  intel_sdvo->attached_output))
> -			return;
> -
> -		intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> -		(void) intel_sdvo_set_output_timing(intel_sdvo, &input_dtd);
> -	}
> +	/* lvds has a special fixed output timing. */
> +	if (intel_sdvo->is_lvds)
> +		intel_sdvo_get_dtd_from_mode(&output_dtd,
> +					     intel_sdvo->sdvo_lvds_fixed_mode);
> +	else
> +		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> +	(void) intel_sdvo_set_output_timing(intel_sdvo, &output_dtd);
>  
>  	/* Set the input timing to the screen. Assume always input 0. */
>  	if (!intel_sdvo_set_target_input(intel_sdvo))
> @@ -1054,6 +1052,10 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>  	    !intel_sdvo_set_tv_format(intel_sdvo))
>  		return;
>  
> +	/* We have tried to get input timing in mode_fixup, and filled into
> +	 * adjusted_mode.
> +	 */
> +	intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
>  	(void) intel_sdvo_set_input_timing(intel_sdvo, &input_dtd);
>  
>  	switch (pixel_multiplier) {

But seems mostly separate from this hunk, which I don't really
understand, not being an SDVO expert.

What happened to the tv check?  Is input_dtd already set to the right
value here after the change?


-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx