Patchwork drm/i915: don't change DRM configuration when releasing load detect pipe

login
register
mail settings
Submitter Jesse Barnes
Date Feb. 18, 2010, 10:51 p.m.
Message ID <20100218145154.7aa7d557@jbarnes-piketon>
Download mbox | patch
Permalink /patch/80474/
State Deferred, archived
Headers show

Comments

Jesse Barnes - July 9, 2010, 3:30 p.m.
On Mon, 08 Mar 2010 21:59:29 +0100
Brice Goglin <Brice.Goglin@gmail.com> wrote:

> Eric Anholt wrote:
> > On Thu, 18 Feb 2010 14:51:54 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >   
> >> When we get a CRTC to use for load detection, we restore its DPMS state
> >> if needed.  We shouldn't, however, change the DRM configuration by
> >> calling drm_helper_disable_unused_functions when we release the CRTC.
> >> Doing so can cause problems with resume, since at suspend or lid close
> >> time, X may choose to probe outputs.  If it doesn't re-probe them at
> >> open or resume time, LVDS won't be restored, since
> >> drm_helper_disable_unused_functions will have turned it off, preventing
> >> the mode set at lid open from restoring it.
> >>     
> >
> > It seems to me like the bug is that drm_helper_disable_unused_functions
> > whacks the configuration.  Why should it do that?
> >
> > 		if (connector->status == connector_status_disconnected)
> > 			connector->encoder = NULL;
> >
> > This function back in UMS used to be about getting the system from
> > unknown bootup state to other junk being turned off after the initial
> > modeset (which was of course broken, since having the other junk turned
> > on during modeset could break things, so really it should have been
> > intel_turn_everything_off_thanks() before doing a modeset).
> >
> > Once that's fixed, it seems like it'd be a replacement for the
> > turn-off-after-load-detect logic in the load detect pipe release
> > function.
> >   
> 
> Did anything get applied to fix this issue in the end ? Jesse's initial
> patch fixed blank screens on my i 945 so I am still applying it to my
> kernels for now.

Arg, totally dropped this one.

Dave, please comment on the above.
Jesse Barnes - July 9, 2010, 6:13 p.m.
On Fri, 9 Jul 2010 08:30:11 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Mon, 08 Mar 2010 21:59:29 +0100
> Brice Goglin <Brice.Goglin@gmail.com> wrote:
> 
> > Eric Anholt wrote:
> > > On Thu, 18 Feb 2010 14:51:54 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > >   
> > >> When we get a CRTC to use for load detection, we restore its DPMS state
> > >> if needed.  We shouldn't, however, change the DRM configuration by
> > >> calling drm_helper_disable_unused_functions when we release the CRTC.
> > >> Doing so can cause problems with resume, since at suspend or lid close
> > >> time, X may choose to probe outputs.  If it doesn't re-probe them at
> > >> open or resume time, LVDS won't be restored, since
> > >> drm_helper_disable_unused_functions will have turned it off, preventing
> > >> the mode set at lid open from restoring it.
> > >>     
> > >
> > > It seems to me like the bug is that drm_helper_disable_unused_functions
> > > whacks the configuration.  Why should it do that?
> > >
> > > 		if (connector->status == connector_status_disconnected)
> > > 			connector->encoder = NULL;
> > >
> > > This function back in UMS used to be about getting the system from
> > > unknown bootup state to other junk being turned off after the initial
> > > modeset (which was of course broken, since having the other junk turned
> > > on during modeset could break things, so really it should have been
> > > intel_turn_everything_off_thanks() before doing a modeset).
> > >
> > > Once that's fixed, it seems like it'd be a replacement for the
> > > turn-off-after-load-detect logic in the load detect pipe release
> > > function.
> > >   
> > 
> > Did anything get applied to fix this issue in the end ? Jesse's initial
> > patch fixed blank screens on my i 945 so I am still applying it to my
> > kernels for now.
> 
> Arg, totally dropped this one.
> 
> Dave, please comment on the above.

I take that back; this shouldn't be required now that we
unconditionally return connected from the LVDS detect hook.

Brice, are you seeing something different?  I.e. is this patch required
for you even on current kernels?

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a483f41..cbb58e5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3758,7 +3758,6 @@  struct drm_crtc *intel_get_load_detect_pipe(struct intel_output *intel_output,
 void intel_release_load_detect_pipe(struct intel_output *intel_output, int dpms_mode)
 {
 	struct drm_encoder *encoder = &intel_output->enc;
-	struct drm_device *dev = encoder->dev;
 	struct drm_crtc *crtc = encoder->crtc;
 	struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
 	struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
@@ -3768,7 +3767,6 @@  void intel_release_load_detect_pipe(struct intel_output *intel_output, int dpms_
 		intel_output->base.encoder = NULL;
 		intel_output->load_detect_temp = false;
 		crtc->enabled = drm_helper_crtc_in_use(crtc);
-		drm_helper_disable_unused_functions(dev);
 	}
 
 	/* Switch crtc and output back off if necessary */