On Tue, Apr 10, 2012 at 09:17:37AM -0700, Jesse Barnes wrote:
> On Tue, 10 Apr 2012 13:55:46 +0200
> Daniel Vetter <daniel.vetter@xxxxxxxx> 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@xxxxxxx>
> > 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@xxxxxxxxxxxxxxxx>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48157
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > ---
> > 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.
We need both hunks to fix the regression, which is why I've smashed them
into the same patch.
> > @@ -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?
Well, neither do I have a clue about sdvo, but I think I somewhat
self-consistent explanation for what's going on.
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). Lower pixel clocks are doubled/quadrupled.
The thing this patch tries to fix is that the pipe needs to be explicit
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.
This patch tries to fix this mess by
- keeping the output mode timing in the unadjusted plain mode, safe for
the lvds case.
- store 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.
- fixup the pixelclock when constructing the sdvo dtd timing struct. This
is why the first part of the patch is an integral part of the series.
- the is_tv special case can be dropped 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).
Hopefully this clears things up a bit.
Cheers, Daniel
--
Daniel Vetter
Mail: daniel@xxxxxxxx
Mobile: +41 (0)79 365 57 48
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
