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 <pro...@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-linux...@largestprime.net> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48157 > Signed-off-by: Daniel Vetter <daniel.vet...@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: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel