[Intel-gfx] [PATCH] drm/i915: handle input/output sdvo timings separately in mode_set

[Daniel Vetter]

I've picked this up for -fixes, with Jesse's irc r-b added.
-Daniel

On Tue, Apr 10, 2012 at 07:13:59PM +0200, Daniel Vetter 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 at 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.
> 
> Due to questions raised in review, below a more elaborate analysis of
> the bug at hand:
> 
> Sdvo seems to have two timings, one is the output timing which will be
> sent over whatever is connected on the other side of the sdvo chip (panel,
> hdmi screen, tv), the other is the input timing which will be generated by
> the gmch pipe. It looks like sdvo is expected to scale between the two.
> 
> To make things slightly more complicated, we have a bunch of special
> cases:
> - For lvds panel we always use a fixed output timing, namely
>   intel_sdvo->sdvo_lvds_fixed_mode, hence that special case.
> - Sdvo has an interface to generate a preferred input timing for a given
>   output timing. This is the confusing thing that I've tried to clear up
>   with the follow-on patches.
> - A special requirement is that the input pixel clock needs to be between
>   100MHz and 200MHz (likely to keep it within the electromechanical design
>   range of PCIe), 270MHz on later gen4+. Lower pixel clocks are
>   doubled/quadrupled.
> 
> The thing this patch tries to fix is that the pipe needs to be
> explicitly instructed to double/quadruple the pixels and needs the
> correspondingly higher pixel clock, whereas the sdvo adaptor seems to
> do that itself and needs the unadjusted pixel clock. For the sdvo
> encode side we already set the pixel mutliplier with a different
> command (0x21).
> 
> This patch tries to fix this mess by:
> - Keeping the output mode timing in the unadjusted plain mode, safe
>   for the lvds case.
> - Storing the input timing in the adjusted_mode with the adjusted
>   pixel clock. This way we don't need to frob around with the core
>   crtc mode set code.
> - Fixing up the pixelclock when constructing the sdvo dtd timing
>   struct. This is why the first hunk of the patch is an integral part
>   of the series.
> - Dropping the is_tv special case because input_dtd is equivalent to
>   adjusted_mode after these changes. Follow-up patches clear this up
>   further (by simply ripping out intel_sdvo->input_dtd because it's
>   not needed).
> 
> v2: Extend commit message with an in-depth bug analysis.
> 
> Reported-and-Tested-by: Bernard Blackham <b-linuxgit at largestprime.net>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48157
> Signed-off-by: Daniel Vetter <daniel.vetter at 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) |
> @@ -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) {
> -- 
> 1.7.9.1
> 

-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48