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
