| Message ID | 1413484055-20669-3-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 16, 2014 at 09:27:28PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > eDP ports need the power seqeuncer whenever the port is active. Warn if > we accidentally steal the power sequener from an active eDP port. This > should not happen unless there's a bug somewhere else, but it's best to > scream loudly if it happens to help with debugging. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index de919e9..7ac5c47 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -2606,6 +2606,10 @@ static void vlv_steal_power_sequencer(struct drm_device *dev, > DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n", > pipe_name(pipe), port_name(port)); > > + WARN(encoder->connectors_active, This only checks for the pipe actually running, i.e. dpms on. But I guess we actually want to check for logically enabled (i.e. no matter the dpms state) to make sure we can always transition from dpms off to on again. That should check for encoder->base.crtc instead. I'll punt on this patch until this is clarified. -Daniel > + "stealing pipe %c power sequencer from active eDP port %c\n", > + pipe_name(pipe), port_name(port)); > + > /* make sure vdd is off before we steal it */ > edp_panel_vdd_off_sync(intel_dp); > > -- > 2.0.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Oct 28, 2014 at 09:10:13AM +0100, Daniel Vetter wrote: > On Thu, Oct 16, 2014 at 09:27:28PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > eDP ports need the power seqeuncer whenever the port is active. Warn if > > we accidentally steal the power sequener from an active eDP port. This > > should not happen unless there's a bug somewhere else, but it's best to > > scream loudly if it happens to help with debugging. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index de919e9..7ac5c47 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -2606,6 +2606,10 @@ static void vlv_steal_power_sequencer(struct drm_device *dev, > > DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n", > > pipe_name(pipe), port_name(port)); > > > > + WARN(encoder->connectors_active, > > This only checks for the pipe actually running, i.e. dpms on. But I guess > we actually want to check for logically enabled (i.e. no matter the dpms > state) to make sure we can always transition from dpms off to on again. > That should check for encoder->base.crtc instead. We can allow stealing the power sequencer whenever the port is not running. When the port gets re-enabled the power seqeuencer gets stolen back. > > I'll punt on this patch until this is clarified. > -Daniel > > > + "stealing pipe %c power sequencer from active eDP port %c\n", > > + pipe_name(pipe), port_name(port)); > > + > > /* make sure vdd is off before we steal it */ > > edp_panel_vdd_off_sync(intel_dp); > > > > -- > > 2.0.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Oct 28, 2014 at 10:14:54AM +0200, Ville Syrjälä wrote: > On Tue, Oct 28, 2014 at 09:10:13AM +0100, Daniel Vetter wrote: > > On Thu, Oct 16, 2014 at 09:27:28PM +0300, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > eDP ports need the power seqeuncer whenever the port is active. Warn if > > > we accidentally steal the power sequener from an active eDP port. This > > > should not happen unless there's a bug somewhere else, but it's best to > > > scream loudly if it happens to help with debugging. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index de919e9..7ac5c47 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -2606,6 +2606,10 @@ static void vlv_steal_power_sequencer(struct drm_device *dev, > > > DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n", > > > pipe_name(pipe), port_name(port)); > > > > > > + WARN(encoder->connectors_active, > > > > This only checks for the pipe actually running, i.e. dpms on. But I guess > > we actually want to check for logically enabled (i.e. no matter the dpms > > state) to make sure we can always transition from dpms off to on again. > > That should check for encoder->base.crtc instead. > > We can allow stealing the power sequencer whenever the port is not > running. When the port gets re-enabled the power seqeuencer gets > stolen back. Hm, but how does this work if we run out of power sequencers? Generally dpms on isn't allowed to fail, so if there's any shared resources we need to keep them reserved. Or am I super-dense again? -Daniel
On Tue, Oct 28, 2014 at 09:34:40AM +0100, Daniel Vetter wrote: > On Tue, Oct 28, 2014 at 10:14:54AM +0200, Ville Syrjälä wrote: > > On Tue, Oct 28, 2014 at 09:10:13AM +0100, Daniel Vetter wrote: > > > On Thu, Oct 16, 2014 at 09:27:28PM +0300, ville.syrjala@linux.intel.com wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > eDP ports need the power seqeuncer whenever the port is active. Warn if > > > > we accidentally steal the power sequener from an active eDP port. This > > > > should not happen unless there's a bug somewhere else, but it's best to > > > > scream loudly if it happens to help with debugging. > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > > index de919e9..7ac5c47 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -2606,6 +2606,10 @@ static void vlv_steal_power_sequencer(struct drm_device *dev, > > > > DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n", > > > > pipe_name(pipe), port_name(port)); > > > > > > > > + WARN(encoder->connectors_active, > > > > > > This only checks for the pipe actually running, i.e. dpms on. But I guess > > > we actually want to check for logically enabled (i.e. no matter the dpms > > > state) to make sure we can always transition from dpms off to on again. > > > That should check for encoder->base.crtc instead. > > > > We can allow stealing the power sequencer whenever the port is not > > running. When the port gets re-enabled the power seqeuencer gets > > stolen back. > > Hm, but how does this work if we run out of power sequencers? Generally > dpms on isn't allowed to fail, so if there's any shared resources we need > to keep them reserved. > > Or am I super-dense again? Currently it shouldn't fail. We have two power sequencers and at most two eDP ports. If that should change we need to rethink this code.
On Tue, Oct 28, 2014 at 11:07:27AM +0200, Ville Syrjälä wrote: > On Tue, Oct 28, 2014 at 09:34:40AM +0100, Daniel Vetter wrote: > > On Tue, Oct 28, 2014 at 10:14:54AM +0200, Ville Syrjälä wrote: > > > On Tue, Oct 28, 2014 at 09:10:13AM +0100, Daniel Vetter wrote: > > > > On Thu, Oct 16, 2014 at 09:27:28PM +0300, ville.syrjala@linux.intel.com wrote: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > eDP ports need the power seqeuncer whenever the port is active. Warn if > > > > > we accidentally steal the power sequener from an active eDP port. This > > > > > should not happen unless there's a bug somewhere else, but it's best to > > > > > scream loudly if it happens to help with debugging. > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_dp.c | 4 ++++ > > > > > 1 file changed, 4 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > > > index de919e9..7ac5c47 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > @@ -2606,6 +2606,10 @@ static void vlv_steal_power_sequencer(struct drm_device *dev, > > > > > DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n", > > > > > pipe_name(pipe), port_name(port)); > > > > > > > > > > + WARN(encoder->connectors_active, > > > > > > > > This only checks for the pipe actually running, i.e. dpms on. But I guess > > > > we actually want to check for logically enabled (i.e. no matter the dpms > > > > state) to make sure we can always transition from dpms off to on again. > > > > That should check for encoder->base.crtc instead. > > > > > > We can allow stealing the power sequencer whenever the port is not > > > running. When the port gets re-enabled the power seqeuencer gets > > > stolen back. > > > > Hm, but how does this work if we run out of power sequencers? Generally > > dpms on isn't allowed to fail, so if there's any shared resources we need > > to keep them reserved. > > > > Or am I super-dense again? > > Currently it shouldn't fail. We have two power sequencers and at most two > eDP ports. If that should change we need to rethink this code. Ok I'm convinced. Added a summary of our discussion here and merged your patch. Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index de919e9..7ac5c47 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2606,6 +2606,10 @@ static void vlv_steal_power_sequencer(struct drm_device *dev, DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n", pipe_name(pipe), port_name(port)); + WARN(encoder->connectors_active, + "stealing pipe %c power sequencer from active eDP port %c\n", + pipe_name(pipe), port_name(port)); + /* make sure vdd is off before we steal it */ edp_panel_vdd_off_sync(intel_dp);