| Message ID | 20181217095024.2340-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,1/5] drm/i915/backlight: Restore backlight on resume, v2. | expand |
On Mon, 17 Dec 2018, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > Restore our saved values for backlight. This way even with fastset on > S4 resume we will correctly restore the backlight to the active values. I think this is a non-trivial commit that requires more explanation in the commit message and comments. What does this do for non-fastset resume? It seems to me this enables/disables backlight twice in that case. This doesn't take panel power sequencing into account at all. You can't just go ahead and enable the PWM with no consideration of how that is fed to the panel. That in turn is encoder code, so I'm not sure how that could be bolted on here. So I'm afraid I'm not convinced this will work. One more detail in-line below. > > Changes since v1: > - Call enable_backlight() when backlight.level is set. On suspend > backlight.enabled is always cleared, this makes it not a good > indicator. Also check for crtc->state->active. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Tolga Cakir <cevelnet@gmail.com> > Cc: Basil Eric Rabi <ericbasil.rabi@gmail.com> > Cc: Hans de Goede <jwrdegoede@fedoraproject.org> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 2 ++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_panel.c | 30 ++++++++++++++++++++++++++++ > 3 files changed, 33 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2c3f3f68d506..86e7b145fd98 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15887,7 +15887,9 @@ void intel_display_resume(struct drm_device *dev) > if (!ret) > ret = __intel_display_resume(dev, state, &ctx); > > + intel_panel_restore_backlight(dev_priv); > intel_enable_ipc(dev_priv); > + > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index cb3a055f18c8..a0551742bcf4 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -2003,6 +2003,7 @@ int intel_panel_init(struct intel_panel *panel, > struct drm_display_mode *fixed_mode, > struct drm_display_mode *downclock_mode); > void intel_panel_fini(struct intel_panel *panel); > +void intel_panel_restore_backlight(struct drm_i915_private *dev_priv); > void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, > struct drm_display_mode *adjusted_mode); > void intel_pch_panel_fitting(struct intel_crtc *crtc, > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index ee3e0842d542..799284fcd57d 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -1903,6 +1903,36 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel) > } > } > > +void intel_panel_restore_backlight(struct drm_i915_private *dev_priv) > +{ > + struct intel_connector *connector; > + struct drm_connector_list_iter conn_iter; > + > + /* Kill all the work that may have been queued by hpd. */ > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > + for_each_intel_connector_iter(connector, &conn_iter) { > + struct intel_panel *panel = &connector->panel; > + const struct drm_connector_state *conn_state = > + connector->base.state; > + > + if (!panel->backlight.present) > + continue; > + > + if (panel->backlight.level && conn_state->crtc && panel->backlight.level is not necessarily 0-based, it's between panel->backlight.{min,max}. BR, Jani. > + conn_state->crtc->state->active) { > + const struct intel_crtc_state *crtc_state = > + to_intel_crtc_state(conn_state->crtc->state); > + > + intel_panel_enable_backlight(crtc_state, conn_state); > + } else { > + WARN(panel->backlight.enabled, "Backlight enabled without crtc\n"); > + > + intel_panel_disable_backlight(conn_state); > + } > + } > + drm_connector_list_iter_end(&conn_iter); > +} > + > int intel_panel_init(struct intel_panel *panel, > struct drm_display_mode *fixed_mode, > struct drm_display_mode *downclock_mode)
Op 28-12-2018 om 11:10 schreef Jani Nikula: > On Mon, 17 Dec 2018, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: >> Restore our saved values for backlight. This way even with fastset on >> S4 resume we will correctly restore the backlight to the active values. > I think this is a non-trivial commit that requires more explanation in > the commit message and comments. > > What does this do for non-fastset resume? It seems to me this > enables/disables backlight twice in that case. > > This doesn't take panel power sequencing into account at all. You can't > just go ahead and enable the PWM with no consideration of how that is > fed to the panel. That in turn is encoder code, so I'm not sure how that > could be bolted on here. > > So I'm afraid I'm not convinced this will work. Neither, I've seen a very nice encoder update_pipe hook for fastset. I think using this for S4 resume would solve the issue. https://patchwork.kernel.org/patch/10738879/ > One more detail in-line below. > >> Changes since v1: >> - Call enable_backlight() when backlight.level is set. On suspend >> backlight.enabled is always cleared, this makes it not a good >> indicator. Also check for crtc->state->active. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Tolga Cakir <cevelnet@gmail.com> >> Cc: Basil Eric Rabi <ericbasil.rabi@gmail.com> >> Cc: Hans de Goede <jwrdegoede@fedoraproject.org> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 2 ++ >> drivers/gpu/drm/i915/intel_drv.h | 1 + >> drivers/gpu/drm/i915/intel_panel.c | 30 ++++++++++++++++++++++++++++ >> 3 files changed, 33 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 2c3f3f68d506..86e7b145fd98 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -15887,7 +15887,9 @@ void intel_display_resume(struct drm_device *dev) >> if (!ret) >> ret = __intel_display_resume(dev, state, &ctx); >> >> + intel_panel_restore_backlight(dev_priv); >> intel_enable_ipc(dev_priv); >> + >> drm_modeset_drop_locks(&ctx); >> drm_modeset_acquire_fini(&ctx); >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index cb3a055f18c8..a0551742bcf4 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -2003,6 +2003,7 @@ int intel_panel_init(struct intel_panel *panel, >> struct drm_display_mode *fixed_mode, >> struct drm_display_mode *downclock_mode); >> void intel_panel_fini(struct intel_panel *panel); >> +void intel_panel_restore_backlight(struct drm_i915_private *dev_priv); >> void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, >> struct drm_display_mode *adjusted_mode); >> void intel_pch_panel_fitting(struct intel_crtc *crtc, >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >> index ee3e0842d542..799284fcd57d 100644 >> --- a/drivers/gpu/drm/i915/intel_panel.c >> +++ b/drivers/gpu/drm/i915/intel_panel.c >> @@ -1903,6 +1903,36 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel) >> } >> } >> >> +void intel_panel_restore_backlight(struct drm_i915_private *dev_priv) >> +{ >> + struct intel_connector *connector; >> + struct drm_connector_list_iter conn_iter; >> + >> + /* Kill all the work that may have been queued by hpd. */ >> + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); >> + for_each_intel_connector_iter(connector, &conn_iter) { >> + struct intel_panel *panel = &connector->panel; >> + const struct drm_connector_state *conn_state = >> + connector->base.state; >> + >> + if (!panel->backlight.present) >> + continue; >> + >> + if (panel->backlight.level && conn_state->crtc && > panel->backlight.level is not necessarily 0-based, it's between > panel->backlight.{min,max}. Yeah, this part can be removed. > BR, > Jani. > >> + conn_state->crtc->state->active) { >> + const struct intel_crtc_state *crtc_state = >> + to_intel_crtc_state(conn_state->crtc->state); >> + >> + intel_panel_enable_backlight(crtc_state, conn_state); >> + } else { >> + WARN(panel->backlight.enabled, "Backlight enabled without crtc\n"); >> + >> + intel_panel_disable_backlight(conn_state); >> + } >> + } >> + drm_connector_list_iter_end(&conn_iter); >> +} >> + >> int intel_panel_init(struct intel_panel *panel, >> struct drm_display_mode *fixed_mode, >> struct drm_display_mode *downclock_mode)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2c3f3f68d506..86e7b145fd98 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15887,7 +15887,9 @@ void intel_display_resume(struct drm_device *dev) if (!ret) ret = __intel_display_resume(dev, state, &ctx); + intel_panel_restore_backlight(dev_priv); intel_enable_ipc(dev_priv); + drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index cb3a055f18c8..a0551742bcf4 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -2003,6 +2003,7 @@ int intel_panel_init(struct intel_panel *panel, struct drm_display_mode *fixed_mode, struct drm_display_mode *downclock_mode); void intel_panel_fini(struct intel_panel *panel); +void intel_panel_restore_backlight(struct drm_i915_private *dev_priv); void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode); void intel_pch_panel_fitting(struct intel_crtc *crtc, diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index ee3e0842d542..799284fcd57d 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1903,6 +1903,36 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel) } } +void intel_panel_restore_backlight(struct drm_i915_private *dev_priv) +{ + struct intel_connector *connector; + struct drm_connector_list_iter conn_iter; + + /* Kill all the work that may have been queued by hpd. */ + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); + for_each_intel_connector_iter(connector, &conn_iter) { + struct intel_panel *panel = &connector->panel; + const struct drm_connector_state *conn_state = + connector->base.state; + + if (!panel->backlight.present) + continue; + + if (panel->backlight.level && conn_state->crtc && + conn_state->crtc->state->active) { + const struct intel_crtc_state *crtc_state = + to_intel_crtc_state(conn_state->crtc->state); + + intel_panel_enable_backlight(crtc_state, conn_state); + } else { + WARN(panel->backlight.enabled, "Backlight enabled without crtc\n"); + + intel_panel_disable_backlight(conn_state); + } + } + drm_connector_list_iter_end(&conn_iter); +} + int intel_panel_init(struct intel_panel *panel, struct drm_display_mode *fixed_mode, struct drm_display_mode *downclock_mode)
Restore our saved values for backlight. This way even with fastset on S4 resume we will correctly restore the backlight to the active values. Changes since v1: - Call enable_backlight() when backlight.level is set. On suspend backlight.enabled is always cleared, this makes it not a good indicator. Also check for crtc->state->active. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Tolga Cakir <cevelnet@gmail.com> Cc: Basil Eric Rabi <ericbasil.rabi@gmail.com> Cc: Hans de Goede <jwrdegoede@fedoraproject.org> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 2 ++ drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_panel.c | 30 ++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+)