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

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Hans de Goede <jwrdegoede@fedoraproject.org>,
	Tolga Cakir <cevelnet@gmail.com>,
	Basil Eric Rabi <ericbasil.rabi@gmail.com>
Subject: Re: [RESEND PATCH 1/5] drm/i915/backlight: Restore backlight on resume, v2.
Date: Fri, 28 Dec 2018 12:10:17 +0200	[thread overview]
Message-ID: <871s62vu5y.fsf@intel.com> (raw)
In-Reply-To: <20181217095024.2340-1-maarten.lankhorst@linux.intel.com>

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)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx