[Intel-gfx] [RESEND PATCH 1/5] drm/i915/backlight: Restore backlight on resume, v2.

[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.

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}.

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)

-- 
Jani Nikula, Intel Open Source Graphics Center