| Message ID | 20211006024018.320394-2-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/dp, drm/i915: Finish basic PWM support for VESA backlight helpers | expand |
On Tue, Oct 05, 2021 at 10:40:14PM -0400, Lyude Paul wrote: > This simply adds proper support for panel backlights that can be controlled > via VESA's backlight control protocol, but which also require that we > enable and disable the backlight via PWM instead of via the DPCD interface. > We also enable this by default, in order to fix some people's backlights > that were broken by not having this enabled. > > For reference, backlights that require this and use VESA's backlight > interface tend to be laptops with hybrid GPUs, but this very well may > change in the future. > > Signed-off-by: Lyude Paul <lyude@redhat.com> > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/3680 > Fixes: fe7d52bccab6 ("drm/i915/dp: Don't use DPCD backlights that need PWM enable/disable") > Cc: <stable@vger.kernel.org> # v5.12+ > --- > .../drm/i915/display/intel_dp_aux_backlight.c | 24 ++++++++++++++----- > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > index 569d17b4d00f..594fdc7453ca 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > @@ -293,6 +293,10 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state, > struct intel_panel *panel = &connector->panel; > struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > > + if (!panel->backlight.edp.vesa.info.aux_enable) > + panel->backlight.pwm_funcs->enable(crtc_state, conn_state, > + panel->backlight.pwm_level_max); What't the story here with the non-inverted max vs. pontetially inverted 0 in the counterpart? > + > drm_edp_backlight_enable(&intel_dp->aux, &panel->backlight.edp.vesa.info, level); > } > > @@ -304,6 +308,10 @@ static void intel_dp_aux_vesa_disable_backlight(const struct drm_connector_state > struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > > drm_edp_backlight_disable(&intel_dp->aux, &panel->backlight.edp.vesa.info); > + > + if (!panel->backlight.edp.vesa.info.aux_enable) > + panel->backlight.pwm_funcs->disable(old_conn_state, > + intel_backlight_invert_pwm_level(connector, 0)); > } > > static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector, enum pipe pipe) > @@ -321,6 +329,15 @@ static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector, > if (ret < 0) > return ret; > > + if (!panel->backlight.edp.vesa.info.aux_enable) { > + ret = panel->backlight.pwm_funcs->setup(connector, pipe); > + if (ret < 0) { > + drm_err(&i915->drm, > + "Failed to setup PWM backlight controls for eDP backlight: %d\n", > + ret); > + return ret; > + } > + } > panel->backlight.max = panel->backlight.edp.vesa.info.max; > panel->backlight.min = 0; > if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) { > @@ -340,12 +357,7 @@ intel_dp_aux_supports_vesa_backlight(struct intel_connector *connector) > struct intel_dp *intel_dp = intel_attached_dp(connector); > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > - /* TODO: We currently only support AUX only backlight configurations, not backlights which > - * require a mix of PWM and AUX controls to work. In the mean time, these machines typically > - * work just fine using normal PWM controls anyway. > - */ > - if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) && > - drm_edp_backlight_supported(intel_dp->edp_dpcd)) { > + if (drm_edp_backlight_supported(intel_dp->edp_dpcd)) { > drm_dbg_kms(&i915->drm, "AUX Backlight Control Supported!\n"); > return true; > } > -- > 2.31.1
On Tue, 2021-10-19 at 21:09 +0300, Ville Syrjälä wrote: > On Tue, Oct 05, 2021 at 10:40:14PM -0400, Lyude Paul wrote: > > This simply adds proper support for panel backlights that can be > > controlled > > via VESA's backlight control protocol, but which also require that we > > enable and disable the backlight via PWM instead of via the DPCD > > interface. > > We also enable this by default, in order to fix some people's backlights > > that were broken by not having this enabled. > > > > For reference, backlights that require this and use VESA's backlight > > interface tend to be laptops with hybrid GPUs, but this very well may > > change in the future. > > > > Signed-off-by: Lyude Paul <lyude@redhat.com> > > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/3680 > > Fixes: fe7d52bccab6 ("drm/i915/dp: Don't use DPCD backlights that need PWM > > enable/disable") > > Cc: <stable@vger.kernel.org> # v5.12+ > > --- > > .../drm/i915/display/intel_dp_aux_backlight.c | 24 ++++++++++++++----- > > 1 file changed, 18 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > index 569d17b4d00f..594fdc7453ca 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > @@ -293,6 +293,10 @@ intel_dp_aux_vesa_enable_backlight(const struct > > intel_crtc_state *crtc_state, > > struct intel_panel *panel = &connector->panel; > > struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > > > > + if (!panel->backlight.edp.vesa.info.aux_enable) > > + panel->backlight.pwm_funcs->enable(crtc_state, conn_state, > > + panel- > > >backlight.pwm_level_max); > > What't the story here with the non-inverted max vs. pontetially inverted > 0 in the counterpart? OH! Nice catch, I wonder if this explains some of the weirdness with samus- fi-bdw… Anyway-unfortunately I don't know the precise answer to if we're supposed to be inverting the panel backlight level or not, so I'd say we should probably just go with whatever the Intel HDR AUX interface is currently doing - which is inverting the panel PWM level when needed. Will fix this in a respin shortly > > > + > > drm_edp_backlight_enable(&intel_dp->aux, &panel- > > >backlight.edp.vesa.info, level); > > } > > > > @@ -304,6 +308,10 @@ static void intel_dp_aux_vesa_disable_backlight(const > > struct drm_connector_state > > struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > > > > drm_edp_backlight_disable(&intel_dp->aux, &panel- > > >backlight.edp.vesa.info); > > + > > + if (!panel->backlight.edp.vesa.info.aux_enable) > > + panel->backlight.pwm_funcs->disable(old_conn_state, > > + > > intel_backlight_invert_pwm_level(connector, 0)); > > } > > > > static int intel_dp_aux_vesa_setup_backlight(struct intel_connector > > *connector, enum pipe pipe) > > @@ -321,6 +329,15 @@ static int intel_dp_aux_vesa_setup_backlight(struct > > intel_connector *connector, > > if (ret < 0) > > return ret; > > > > + if (!panel->backlight.edp.vesa.info.aux_enable) { > > + ret = panel->backlight.pwm_funcs->setup(connector, pipe); > > + if (ret < 0) { > > + drm_err(&i915->drm, > > + "Failed to setup PWM backlight controls > > for eDP backlight: %d\n", > > + ret); > > + return ret; > > + } > > + } > > panel->backlight.max = panel->backlight.edp.vesa.info.max; > > panel->backlight.min = 0; > > if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) { > > @@ -340,12 +357,7 @@ intel_dp_aux_supports_vesa_backlight(struct > > intel_connector *connector) > > struct intel_dp *intel_dp = intel_attached_dp(connector); > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > > - /* TODO: We currently only support AUX only backlight > > configurations, not backlights which > > - * require a mix of PWM and AUX controls to work. In the mean > > time, these machines typically > > - * work just fine using normal PWM controls anyway. > > - */ > > - if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) && > > - drm_edp_backlight_supported(intel_dp->edp_dpcd)) { > > + if (drm_edp_backlight_supported(intel_dp->edp_dpcd)) { > > drm_dbg_kms(&i915->drm, "AUX Backlight Control > > Supported!\n"); > > return true; > > } > > -- > > 2.31.1 >
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c index 569d17b4d00f..594fdc7453ca 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c @@ -293,6 +293,10 @@ intel_dp_aux_vesa_enable_backlight(const struct intel_crtc_state *crtc_state, struct intel_panel *panel = &connector->panel; struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); + if (!panel->backlight.edp.vesa.info.aux_enable) + panel->backlight.pwm_funcs->enable(crtc_state, conn_state, + panel->backlight.pwm_level_max); + drm_edp_backlight_enable(&intel_dp->aux, &panel->backlight.edp.vesa.info, level); } @@ -304,6 +308,10 @@ static void intel_dp_aux_vesa_disable_backlight(const struct drm_connector_state struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); drm_edp_backlight_disable(&intel_dp->aux, &panel->backlight.edp.vesa.info); + + if (!panel->backlight.edp.vesa.info.aux_enable) + panel->backlight.pwm_funcs->disable(old_conn_state, + intel_backlight_invert_pwm_level(connector, 0)); } static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector, enum pipe pipe) @@ -321,6 +329,15 @@ static int intel_dp_aux_vesa_setup_backlight(struct intel_connector *connector, if (ret < 0) return ret; + if (!panel->backlight.edp.vesa.info.aux_enable) { + ret = panel->backlight.pwm_funcs->setup(connector, pipe); + if (ret < 0) { + drm_err(&i915->drm, + "Failed to setup PWM backlight controls for eDP backlight: %d\n", + ret); + return ret; + } + } panel->backlight.max = panel->backlight.edp.vesa.info.max; panel->backlight.min = 0; if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) { @@ -340,12 +357,7 @@ intel_dp_aux_supports_vesa_backlight(struct intel_connector *connector) struct intel_dp *intel_dp = intel_attached_dp(connector); struct drm_i915_private *i915 = dp_to_i915(intel_dp); - /* TODO: We currently only support AUX only backlight configurations, not backlights which - * require a mix of PWM and AUX controls to work. In the mean time, these machines typically - * work just fine using normal PWM controls anyway. - */ - if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) && - drm_edp_backlight_supported(intel_dp->edp_dpcd)) { + if (drm_edp_backlight_supported(intel_dp->edp_dpcd)) { drm_dbg_kms(&i915->drm, "AUX Backlight Control Supported!\n"); return true; }
This simply adds proper support for panel backlights that can be controlled via VESA's backlight control protocol, but which also require that we enable and disable the backlight via PWM instead of via the DPCD interface. We also enable this by default, in order to fix some people's backlights that were broken by not having this enabled. For reference, backlights that require this and use VESA's backlight interface tend to be laptops with hybrid GPUs, but this very well may change in the future. Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/3680 Fixes: fe7d52bccab6 ("drm/i915/dp: Don't use DPCD backlights that need PWM enable/disable") Cc: <stable@vger.kernel.org> # v5.12+ --- .../drm/i915/display/intel_dp_aux_backlight.c | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-)