| Message ID | 1342016944-23395-10-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 11, 2012 at 04:27:52PM +0200, Daniel Vetter wrote: > LVDS is the first output where dpms on/off and prepare/commit don't > perfectly match. Now the idea behind this special case seems to be > that for simple resolution changes on the LVDS we don't need to stop > the pipe, because (at least on newer chips) we can adjust the panel > fitter on the fly. > > There are a few problems with the current code though: > - We still stop and restart the pipe unconditionally, because the crtc > helper code isn't flexible enough. > - We show some ugly flickering, especially when changing crtcs (this > the crtc helper would actually take into account, but we don't > implement the encoder->get_crtc callback required to make this work > properly). > > So it doesn't even work as advertised. I agree that it would be nice > to do resolution changes on LVDS (and also eDP) whithout blacking the > screen where the panel fitter allows to do that. But imo we should > implement this as a special case a few layers up in the mode set code, > akin to how we already detect simple framebuffer changes (and only > update the required registers with ->mode_set_base). > > Until this is all in place, make our lives easier and just rip it out. > > Also note that this seems to fix actual bugs with enabling the lvds > output, see: > > http://lists.freedesktop.org/archives/intel-gfx/2012-July/018614.html > > Cc: Takashi Iwai <tiwai@suse.de> > Cc: Giacomo Comes <comes@naic.edu> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> I've merged this with Chris' irc-ack (under the condition that the modeset-rework leads to reinstated lvds fastpath that actually works eventually). I've stalled a bit for tested-bys from Takashi, but he seems to be awol atm. -Daniel
At Sun, 22 Jul 2012 16:52:54 +0200, Daniel Vetter wrote: > > On Wed, Jul 11, 2012 at 04:27:52PM +0200, Daniel Vetter wrote: > > LVDS is the first output where dpms on/off and prepare/commit don't > > perfectly match. Now the idea behind this special case seems to be > > that for simple resolution changes on the LVDS we don't need to stop > > the pipe, because (at least on newer chips) we can adjust the panel > > fitter on the fly. > > > > There are a few problems with the current code though: > > - We still stop and restart the pipe unconditionally, because the crtc > > helper code isn't flexible enough. > > - We show some ugly flickering, especially when changing crtcs (this > > the crtc helper would actually take into account, but we don't > > implement the encoder->get_crtc callback required to make this work > > properly). > > > > So it doesn't even work as advertised. I agree that it would be nice > > to do resolution changes on LVDS (and also eDP) whithout blacking the > > screen where the panel fitter allows to do that. But imo we should > > implement this as a special case a few layers up in the mode set code, > > akin to how we already detect simple framebuffer changes (and only > > update the required registers with ->mode_set_base). > > > > Until this is all in place, make our lives easier and just rip it out. > > > > Also note that this seems to fix actual bugs with enabling the lvds > > output, see: > > > > http://lists.freedesktop.org/archives/intel-gfx/2012-July/018614.html > > > > Cc: Takashi Iwai <tiwai@suse.de> > > Cc: Giacomo Comes <comes@naic.edu> > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > I've merged this with Chris' irc-ack (under the condition that the > modeset-rework leads to reinstated lvds fastpath that actually works > eventually). I've stalled a bit for tested-bys from Takashi, but he seems > to be awol atm. Sorry, I was too busy for other tasks after vacation. Tested on a few machines and it worked as expected. Tested-by: Takashi Iwai <tiwai@suse.de> Takashi
On Sun, Jul 22, 2012 at 6:32 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Sun, 22 Jul 2012 16:52:54 +0200, > Daniel Vetter wrote: >> >> On Wed, Jul 11, 2012 at 04:27:52PM +0200, Daniel Vetter wrote: >> > LVDS is the first output where dpms on/off and prepare/commit don't >> > perfectly match. Now the idea behind this special case seems to be >> > that for simple resolution changes on the LVDS we don't need to stop >> > the pipe, because (at least on newer chips) we can adjust the panel >> > fitter on the fly. >> > >> > There are a few problems with the current code though: >> > - We still stop and restart the pipe unconditionally, because the crtc >> > helper code isn't flexible enough. >> > - We show some ugly flickering, especially when changing crtcs (this >> > the crtc helper would actually take into account, but we don't >> > implement the encoder->get_crtc callback required to make this work >> > properly). >> > >> > So it doesn't even work as advertised. I agree that it would be nice >> > to do resolution changes on LVDS (and also eDP) whithout blacking the >> > screen where the panel fitter allows to do that. But imo we should >> > implement this as a special case a few layers up in the mode set code, >> > akin to how we already detect simple framebuffer changes (and only >> > update the required registers with ->mode_set_base). >> > >> > Until this is all in place, make our lives easier and just rip it out. >> > >> > Also note that this seems to fix actual bugs with enabling the lvds >> > output, see: >> > >> > http://lists.freedesktop.org/archives/intel-gfx/2012-July/018614.html >> > >> > Cc: Takashi Iwai <tiwai@suse.de> >> > Cc: Giacomo Comes <comes@naic.edu> >> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> I've merged this with Chris' irc-ack (under the condition that the >> modeset-rework leads to reinstated lvds fastpath that actually works >> eventually). I've stalled a bit for tested-bys from Takashi, but he seems >> to be awol atm. > > Sorry, I was too busy for other tasks after vacation. > Tested on a few machines and it worked as expected. > > Tested-by: Takashi Iwai <tiwai@suse.de> No problem ;-) and thanks for putting the patches through it's paces on a few bothersome machines. Tested-by added to the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 9b706a5..73d0079 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -409,13 +409,7 @@ static void intel_lvds_prepare(struct drm_encoder *encoder) { struct intel_lvds *intel_lvds = to_intel_lvds(encoder); - /* - * Prior to Ironlake, we must disable the pipe if we want to adjust - * the panel fitter. However at all other times we can just reset - * the registers regardless. - */ - if (!HAS_PCH_SPLIT(encoder->dev) && intel_lvds->pfit_dirty) - intel_lvds_disable(intel_lvds); + intel_lvds_disable(intel_lvds); } static void intel_lvds_commit(struct drm_encoder *encoder)
LVDS is the first output where dpms on/off and prepare/commit don't perfectly match. Now the idea behind this special case seems to be that for simple resolution changes on the LVDS we don't need to stop the pipe, because (at least on newer chips) we can adjust the panel fitter on the fly. There are a few problems with the current code though: - We still stop and restart the pipe unconditionally, because the crtc helper code isn't flexible enough. - We show some ugly flickering, especially when changing crtcs (this the crtc helper would actually take into account, but we don't implement the encoder->get_crtc callback required to make this work properly). So it doesn't even work as advertised. I agree that it would be nice to do resolution changes on LVDS (and also eDP) whithout blacking the screen where the panel fitter allows to do that. But imo we should implement this as a special case a few layers up in the mode set code, akin to how we already detect simple framebuffer changes (and only update the required registers with ->mode_set_base). Until this is all in place, make our lives easier and just rip it out. Also note that this seems to fix actual bugs with enabling the lvds output, see: http://lists.freedesktop.org/archives/intel-gfx/2012-July/018614.html Cc: Takashi Iwai <tiwai@suse.de> Cc: Giacomo Comes <comes@naic.edu> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_lvds.c | 8 +------- 1 files changed, 1 insertions(+), 7 deletions(-)