[Intel-gfx] [RESEND PATCH 1/5] drm/i915/backlight: Restore backlight on resume, v2.
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Wed Jan 2 10:14:58 UTC 2019
Op 28-12-2018 om 11:10 schreef Jani Nikula:
> On Mon, 17 Dec 2018, Maarten Lankhorst <maarten.lankhorst at 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 at linux.intel.com>
>> Cc: Tolga Cakir <cevelnet at gmail.com>
>> Cc: Basil Eric Rabi <ericbasil.rabi at gmail.com>
>> Cc: Hans de Goede <jwrdegoede at fedoraproject.org>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Reported-by: Ville Syrjälä <ville.syrjala at 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)
More information about the Intel-gfx
mailing list