Op 28-12-2018 om 11:10 schreef Jani Nikula: > On Mon, 17 Dec 2018, Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> >> Cc: Tolga Cakir <cevelnet@xxxxxxxxx> >> Cc: Basil Eric Rabi <ericbasil.rabi@xxxxxxxxx> >> Cc: Hans de Goede <jwrdegoede@xxxxxxxxxxxxxxxxx> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> Reported-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> --- >> 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) _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
