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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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)


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx