[lkml]   [2012]   [May]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    Patches in this message
    Subject[ 107/167] [PATCH] drm/i915: handle input/output sdvo timings separately in mode_set
    3.2-stable review patch.  If anyone has any objections, please let me know.


    From: Daniel Vetter <>

    commit 6651819b4b4fc3caa6964c5d825eb4bb996f3905 upstream.

    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 <>
    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
    - 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
    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 <>
    Reviewed-by: Jesse Barnes <>
    Signed-off-by: Daniel Vetter <>
    [bwh: Indented the hunk quoted above so quilt doesn't try to apply it]
    Signed-off-by: Ben Hutchings <>
    drivers/gpu/drm/i915/intel_sdvo.c | 34 ++++++++++++++++++----------------
    1 file changed, 18 insertions(+), 16 deletions(-)
    diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
    index e36b171..232d77d 100644
    --- a/drivers/gpu/drm/i915/intel_sdvo.c
    +++ b/drivers/gpu/drm/i915/intel_sdvo.c
    @@ -731,6 +731,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;
    @@ -745,7 +746,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) |
    @@ -996,7 +1001,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;

    @@ -1021,20 +1026,13 @@ 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) {
    - 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))
    @@ -1052,6 +1050,10 @@ 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.
    + */
    + intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
    (void) intel_sdvo_set_input_timing(intel_sdvo, &input_dtd);

    switch (pixel_multiplier) {

     \ /
      Last update: 2012-05-09 09:01    [W:0.033 / U:8.028 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site