| Message ID | 20200918002845.32766-1-sean@poorly.run (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: Tweak initial dpcd backlight.enabled value | expand |
So if I understand this correctly, it sounds like that some Pixelbooks boot up with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the panel actually having DPCD backlight controls enabled? If I'm understanding that correctly, then this patch looks good to me: Reviewed-by: Lyude Paul <lyude@redhat.com> On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote: > From: Sean Paul <seanpaul@chromium.org> > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in > DPCD control mode"), we fixed the brightness level when DPCD control was > not active to max brightness. This is as good as we can guess since most > backlights go on full when uncontrolled. > > However in doing so we changed the semantics of the initial > 'backlight.enabled' value. At least on Pixelbooks, they were relying > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on > boot such that enabled would be false. This causes the device to be > enabled when the brightness is set. Without this, brightness control > doesn't work. So by changing brightness to max, we also flipped enabled > to be true on boot. > > To fix this, make enabled a function of brightness and backlight control > mechanism. > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD > control mode") > Cc: Lyude Paul <lyude@redhat.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Kevin Chowski <chowski@chromium.org>> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++------- > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp > *intel_dp, bool enable) > } > } > > -/* > - * Read the current backlight value from DPCD register(s) based > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > - */ > -static u32 intel_dp_aux_get_backlight(struct intel_connector *connector) > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector > *connector) > { > struct intel_dp *intel_dp = intel_attached_dp(connector); > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > - u8 read_val[2] = { 0x0 }; > u8 mode_reg; > - u16 level = 0; > > if (drm_dp_dpcd_readb(&intel_dp->aux, > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct > intel_connector *connector) > drm_dbg_kms(&i915->drm, > "Failed to read the DPCD register 0x%x\n", > DP_EDP_BACKLIGHT_MODE_SET_REGISTER); > - return 0; > + return false; > } > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) == > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; > +} > + > +/* > + * Read the current backlight value from DPCD register(s) based > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > + */ > +static u32 intel_dp_aux_get_backlight(struct intel_connector *connector) > +{ > + struct intel_dp *intel_dp = intel_attached_dp(connector); > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > + u8 read_val[2] = { 0x0 }; > + u16 level = 0; > + > /* > * If we're not in DPCD control mode yet, the programmed brightness > * value is meaningless and we should assume max brightness > */ > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) > + if (!intel_dp_aux_backlight_dpcd_mode(connector)) > return connector->panel.backlight.max; > > if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct > intel_connector *connector, > > panel->backlight.min = 0; > panel->backlight.level = intel_dp_aux_get_backlight(connector); > - panel->backlight.enabled = panel->backlight.level != 0; > + panel->backlight.enabled = intel_dp_aux_backlight_dpcd_mode(connector) > && > + panel->backlight.level != 0; > > return 0; > }
On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote: > > So if I understand this correctly, it sounds like that some Pixelbooks boot up > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the > panel actually having DPCD backlight controls enabled? It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set backlight.enabled = false. By changing backlight.level = max, backlight.enabled is now set to true. This results in losing backlight control on boot (since the enable routine is no longer invoked). Sean > > If I'm understanding that correctly, then this patch looks good to me: > > Reviewed-by: Lyude Paul <lyude@redhat.com> > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote: > > From: Sean Paul <seanpaul@chromium.org> > > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in > > DPCD control mode"), we fixed the brightness level when DPCD control was > > not active to max brightness. This is as good as we can guess since most > > backlights go on full when uncontrolled. > > > > However in doing so we changed the semantics of the initial > > 'backlight.enabled' value. At least on Pixelbooks, they were relying > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on > > boot such that enabled would be false. This causes the device to be > > enabled when the brightness is set. Without this, brightness control > > doesn't work. So by changing brightness to max, we also flipped enabled > > to be true on boot. > > > > To fix this, make enabled a function of brightness and backlight control > > mechanism. > > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD > > control mode") > > Cc: Lyude Paul <lyude@redhat.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Kevin Chowski <chowski@chromium.org>> > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > --- > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++------- > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp > > *intel_dp, bool enable) > > } > > } > > > > -/* > > - * Read the current backlight value from DPCD register(s) based > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > > - */ > > -static u32 intel_dp_aux_get_backlight(struct intel_connector *connector) > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector > > *connector) > > { > > struct intel_dp *intel_dp = intel_attached_dp(connector); > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > - u8 read_val[2] = { 0x0 }; > > u8 mode_reg; > > - u16 level = 0; > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct > > intel_connector *connector) > > drm_dbg_kms(&i915->drm, > > "Failed to read the DPCD register 0x%x\n", > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER); > > - return 0; > > + return false; > > } > > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) == > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; > > +} > > + > > +/* > > + * Read the current backlight value from DPCD register(s) based > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > > + */ > > +static u32 intel_dp_aux_get_backlight(struct intel_connector *connector) > > +{ > > + struct intel_dp *intel_dp = intel_attached_dp(connector); > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > + u8 read_val[2] = { 0x0 }; > > + u16 level = 0; > > + > > /* > > * If we're not in DPCD control mode yet, the programmed brightness > > * value is meaningless and we should assume max brightness > > */ > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) > > + if (!intel_dp_aux_backlight_dpcd_mode(connector)) > > return connector->panel.backlight.max; > > > > if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct > > intel_connector *connector, > > > > panel->backlight.min = 0; > > panel->backlight.level = intel_dp_aux_get_backlight(connector); > > - panel->backlight.enabled = panel->backlight.level != 0; > > + panel->backlight.enabled = intel_dp_aux_backlight_dpcd_mode(connector) > > && > > + panel->backlight.level != 0; > > > > return 0; > > } > -- > Cheers, > Lyude Paul (she/her) > Software Engineer at Red Hat >
On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote: > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote: > > So if I understand this correctly, it sounds like that some Pixelbooks > > boot up > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the > > panel actually having DPCD backlight controls enabled? > > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set > backlight.enabled = false. By changing backlight.level = max, > backlight.enabled is now set to true. This results in losing backlight > control on boot (since the enable routine is no longer invoked). > Ahhh ok, I'm fine with that - review still stands :) > Sean > > > If I'm understanding that correctly, then this patch looks good to me: > > > > Reviewed-by: Lyude Paul <lyude@redhat.com> > > > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote: > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in > > > DPCD control mode"), we fixed the brightness level when DPCD control was > > > not active to max brightness. This is as good as we can guess since most > > > backlights go on full when uncontrolled. > > > > > > However in doing so we changed the semantics of the initial > > > 'backlight.enabled' value. At least on Pixelbooks, they were relying > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on > > > boot such that enabled would be false. This causes the device to be > > > enabled when the brightness is set. Without this, brightness control > > > doesn't work. So by changing brightness to max, we also flipped enabled > > > to be true on boot. > > > > > > To fix this, make enabled a function of brightness and backlight control > > > mechanism. > > > > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD > > > control mode") > > > Cc: Lyude Paul <lyude@redhat.com> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Kevin Chowski <chowski@chromium.org>> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > --- > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++------- > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp > > > *intel_dp, bool enable) > > > } > > > } > > > > > > -/* > > > - * Read the current backlight value from DPCD register(s) based > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > > > - */ > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector > > > *connector) > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector > > > *connector) > > > { > > > struct intel_dp *intel_dp = intel_attached_dp(connector); > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > - u8 read_val[2] = { 0x0 }; > > > u8 mode_reg; > > > - u16 level = 0; > > > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct > > > intel_connector *connector) > > > drm_dbg_kms(&i915->drm, > > > "Failed to read the DPCD register 0x%x\n", > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER); > > > - return 0; > > > + return false; > > > } > > > > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) == > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; > > > +} > > > + > > > +/* > > > + * Read the current backlight value from DPCD register(s) based > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > > > + */ > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector > > > *connector) > > > +{ > > > + struct intel_dp *intel_dp = intel_attached_dp(connector); > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > + u8 read_val[2] = { 0x0 }; > > > + u16 level = 0; > > > + > > > /* > > > * If we're not in DPCD control mode yet, the programmed > > > brightness > > > * value is meaningless and we should assume max brightness > > > */ > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector)) > > > return connector->panel.backlight.max; > > > > > > if (drm_dp_dpcd_read(&intel_dp->aux, > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct > > > intel_connector *connector, > > > > > > panel->backlight.min = 0; > > > panel->backlight.level = intel_dp_aux_get_backlight(connector); > > > - panel->backlight.enabled = panel->backlight.level != 0; > > > + panel->backlight.enabled = > > > intel_dp_aux_backlight_dpcd_mode(connector) > > > && > > > + panel->backlight.level != 0; > > > > > > return 0; > > > } > > -- > > Cheers, > > Lyude Paul (she/her) > > Software Engineer at Red Hat > >
On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote: > > On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote: > > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote: > > > So if I understand this correctly, it sounds like that some Pixelbooks > > > boot up > > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the > > > panel actually having DPCD backlight controls enabled? > > > > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set > > backlight.enabled = false. By changing backlight.level = max, > > backlight.enabled is now set to true. This results in losing backlight > > control on boot (since the enable routine is no longer invoked). > > > Ahhh ok, I'm fine with that - review still stands :) Pinging intel maintainers, could someone please apply this? Sean > > > Sean > > > > > If I'm understanding that correctly, then this patch looks good to me: > > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com> > > > > > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote: > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in > > > > DPCD control mode"), we fixed the brightness level when DPCD control was > > > > not active to max brightness. This is as good as we can guess since most > > > > backlights go on full when uncontrolled. > > > > > > > > However in doing so we changed the semantics of the initial > > > > 'backlight.enabled' value. At least on Pixelbooks, they were relying > > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on > > > > boot such that enabled would be false. This causes the device to be > > > > enabled when the brightness is set. Without this, brightness control > > > > doesn't work. So by changing brightness to max, we also flipped enabled > > > > to be true on boot. > > > > > > > > To fix this, make enabled a function of brightness and backlight control > > > > mechanism. > > > > > > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD > > > > control mode") > > > > Cc: Lyude Paul <lyude@redhat.com> > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Kevin Chowski <chowski@chromium.org>> > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > --- > > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++------- > > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp > > > > *intel_dp, bool enable) > > > > } > > > > } > > > > > > > > -/* > > > > - * Read the current backlight value from DPCD register(s) based > > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > > > > - */ > > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector > > > > *connector) > > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector > > > > *connector) > > > > { > > > > struct intel_dp *intel_dp = intel_attached_dp(connector); > > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > > - u8 read_val[2] = { 0x0 }; > > > > u8 mode_reg; > > > > - u16 level = 0; > > > > > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct > > > > intel_connector *connector) > > > > drm_dbg_kms(&i915->drm, > > > > "Failed to read the DPCD register 0x%x\n", > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER); > > > > - return 0; > > > > + return false; > > > > } > > > > > > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) == > > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; > > > > +} > > > > + > > > > +/* > > > > + * Read the current backlight value from DPCD register(s) based > > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > > > > + */ > > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector > > > > *connector) > > > > +{ > > > > + struct intel_dp *intel_dp = intel_attached_dp(connector); > > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > > + u8 read_val[2] = { 0x0 }; > > > > + u16 level = 0; > > > > + > > > > /* > > > > * If we're not in DPCD control mode yet, the programmed > > > > brightness > > > > * value is meaningless and we should assume max brightness > > > > */ > > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != > > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) > > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector)) > > > > return connector->panel.backlight.max; > > > > > > > > if (drm_dp_dpcd_read(&intel_dp->aux, > > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, > > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct > > > > intel_connector *connector, > > > > > > > > panel->backlight.min = 0; > > > > panel->backlight.level = intel_dp_aux_get_backlight(connector); > > > > - panel->backlight.enabled = panel->backlight.level != 0; > > > > + panel->backlight.enabled = > > > > intel_dp_aux_backlight_dpcd_mode(connector) > > > > && > > > > + panel->backlight.level != 0; > > > > > > > > return 0; > > > > } > > > -- > > > Cheers, > > > Lyude Paul (she/her) > > > Software Engineer at Red Hat > > > > -- > Cheers, > Lyude Paul (she/her) > Software Engineer at Red Hat >
On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote: > On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote: > > On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote: > > > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote: > > > > So if I understand this correctly, it sounds like that some Pixelbooks > > > > boot up > > > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without > > > > the > > > > panel actually having DPCD backlight controls enabled? > > > > > > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set > > > backlight.enabled = false. By changing backlight.level = max, > > > backlight.enabled is now set to true. This results in losing backlight > > > control on boot (since the enable routine is no longer invoked). > > > > > Ahhh ok, I'm fine with that - review still stands :) > > Pinging intel maintainers, could someone please apply this? oops, sorry about that. I can go ahead and push this > > > Sean > > > > Sean > > > > > > > If I'm understanding that correctly, then this patch looks good to me: > > > > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com> > > > > > > > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote: > > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in > > > > > DPCD control mode"), we fixed the brightness level when DPCD control > > > > > was > > > > > not active to max brightness. This is as good as we can guess since > > > > > most > > > > > backlights go on full when uncontrolled. > > > > > > > > > > However in doing so we changed the semantics of the initial > > > > > 'backlight.enabled' value. At least on Pixelbooks, they were relying > > > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on > > > > > boot such that enabled would be false. This causes the device to be > > > > > enabled when the brightness is set. Without this, brightness control > > > > > doesn't work. So by changing brightness to max, we also flipped > > > > > enabled > > > > > to be true on boot. > > > > > > > > > > To fix this, make enabled a function of brightness and backlight > > > > > control > > > > > mechanism. > > > > > > > > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in > > > > > DPCD > > > > > control mode") > > > > > Cc: Lyude Paul <lyude@redhat.com> > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Cc: Kevin Chowski <chowski@chromium.org>> > > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > --- > > > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++---- > > > > > --- > > > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct > > > > > intel_dp > > > > > *intel_dp, bool enable) > > > > > } > > > > > } > > > > > > > > > > -/* > > > > > - * Read the current backlight value from DPCD register(s) based > > > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > > > > > - */ > > > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector > > > > > *connector) > > > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector > > > > > *connector) > > > > > { > > > > > struct intel_dp *intel_dp = intel_attached_dp(connector); > > > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > > > - u8 read_val[2] = { 0x0 }; > > > > > u8 mode_reg; > > > > > - u16 level = 0; > > > > > > > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct > > > > > intel_connector *connector) > > > > > drm_dbg_kms(&i915->drm, > > > > > "Failed to read the DPCD register 0x%x\n", > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER); > > > > > - return 0; > > > > > + return false; > > > > > } > > > > > > > > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) == > > > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; > > > > > +} > > > > > + > > > > > +/* > > > > > + * Read the current backlight value from DPCD register(s) based > > > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > > > > > + */ > > > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector > > > > > *connector) > > > > > +{ > > > > > + struct intel_dp *intel_dp = intel_attached_dp(connector); > > > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > > > + u8 read_val[2] = { 0x0 }; > > > > > + u16 level = 0; > > > > > + > > > > > /* > > > > > * If we're not in DPCD control mode yet, the programmed > > > > > brightness > > > > > * value is meaningless and we should assume max brightness > > > > > */ > > > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != > > > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) > > > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector)) > > > > > return connector->panel.backlight.max; > > > > > > > > > > if (drm_dp_dpcd_read(&intel_dp->aux, > > > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, > > > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct > > > > > intel_connector *connector, > > > > > > > > > > panel->backlight.min = 0; > > > > > panel->backlight.level = intel_dp_aux_get_backlight(connector); > > > > > - panel->backlight.enabled = panel->backlight.level != 0; > > > > > + panel->backlight.enabled = > > > > > intel_dp_aux_backlight_dpcd_mode(connector) > > > > > && > > > > > + panel->backlight.level != 0; > > > > > > > > > > return 0; > > > > > } > > > > -- > > > > Cheers, > > > > Lyude Paul (she/her) > > > > Software Engineer at Red Hat > > > > > > -- > > Cheers, > > Lyude Paul (she/her) > > Software Engineer at Red Hat > >
Just pushed this, but it's not in drm-tip because it would seem that rebuilding drm-tip has failed, and the conflict doesn't appear to be from any of the patches I pushed so I'm getting the feeling from the DRM maintainer docs I should probably let one of the drm-misc-folks handle the conflict. On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote: > On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote: > > On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote: > > > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote: > > > > So if I understand this correctly, it sounds like that some Pixelbooks > > > > boot up > > > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without > > > > the > > > > panel actually having DPCD backlight controls enabled? > > > > > > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set > > > backlight.enabled = false. By changing backlight.level = max, > > > backlight.enabled is now set to true. This results in losing backlight > > > control on boot (since the enable routine is no longer invoked). > > > > > Ahhh ok, I'm fine with that - review still stands :) > > Pinging intel maintainers, could someone please apply this? > > > Sean > > > > Sean > > > > > > > If I'm understanding that correctly, then this patch looks good to me: > > > > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com> > > > > > > > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote: > > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not in > > > > > DPCD control mode"), we fixed the brightness level when DPCD control > > > > > was > > > > > not active to max brightness. This is as good as we can guess since > > > > > most > > > > > backlights go on full when uncontrolled. > > > > > > > > > > However in doing so we changed the semantics of the initial > > > > > 'backlight.enabled' value. At least on Pixelbooks, they were relying > > > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on > > > > > boot such that enabled would be false. This causes the device to be > > > > > enabled when the brightness is set. Without this, brightness control > > > > > doesn't work. So by changing brightness to max, we also flipped > > > > > enabled > > > > > to be true on boot. > > > > > > > > > > To fix this, make enabled a function of brightness and backlight > > > > > control > > > > > mechanism. > > > > > > > > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in > > > > > DPCD > > > > > control mode") > > > > > Cc: Lyude Paul <lyude@redhat.com> > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Cc: Kevin Chowski <chowski@chromium.org>> > > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > --- > > > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++---- > > > > > --- > > > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct > > > > > intel_dp > > > > > *intel_dp, bool enable) > > > > > } > > > > > } > > > > > > > > > > -/* > > > > > - * Read the current backlight value from DPCD register(s) based > > > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > > > > > - */ > > > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector > > > > > *connector) > > > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector > > > > > *connector) > > > > > { > > > > > struct intel_dp *intel_dp = intel_attached_dp(connector); > > > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > > > - u8 read_val[2] = { 0x0 }; > > > > > u8 mode_reg; > > > > > - u16 level = 0; > > > > > > > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct > > > > > intel_connector *connector) > > > > > drm_dbg_kms(&i915->drm, > > > > > "Failed to read the DPCD register 0x%x\n", > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER); > > > > > - return 0; > > > > > + return false; > > > > > } > > > > > > > > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) == > > > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; > > > > > +} > > > > > + > > > > > +/* > > > > > + * Read the current backlight value from DPCD register(s) based > > > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > > > > > + */ > > > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector > > > > > *connector) > > > > > +{ > > > > > + struct intel_dp *intel_dp = intel_attached_dp(connector); > > > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > > > + u8 read_val[2] = { 0x0 }; > > > > > + u16 level = 0; > > > > > + > > > > > /* > > > > > * If we're not in DPCD control mode yet, the programmed > > > > > brightness > > > > > * value is meaningless and we should assume max brightness > > > > > */ > > > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != > > > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) > > > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector)) > > > > > return connector->panel.backlight.max; > > > > > > > > > > if (drm_dp_dpcd_read(&intel_dp->aux, > > > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, > > > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct > > > > > intel_connector *connector, > > > > > > > > > > panel->backlight.min = 0; > > > > > panel->backlight.level = intel_dp_aux_get_backlight(connector); > > > > > - panel->backlight.enabled = panel->backlight.level != 0; > > > > > + panel->backlight.enabled = > > > > > intel_dp_aux_backlight_dpcd_mode(connector) > > > > > && > > > > > + panel->backlight.level != 0; > > > > > > > > > > return 0; > > > > > } > > > > -- > > > > Cheers, > > > > Lyude Paul (she/her) > > > > Software Engineer at Red Hat > > > > > > -- > > Cheers, > > Lyude Paul (she/her) > > Software Engineer at Red Hat > >
> On Oct 12, 2020, at 2:47 PM, Lyude Paul <lyude@redhat.com> wrote: > > Just pushed this, but it's not in drm-tip because it would seem that rebuilding > drm-tip has failed, and the conflict doesn't appear to be from any of the > patches I pushed so I'm getting the feeling from the DRM maintainer docs I > should probably let one of the drm-misc-folks handle the conflict. conflicts solved. feel free to push now. For the drm-misc I simply went with the drm-misc-next solution and for the drm-intel I went with the drm-intel-next-queued one. > > On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote: >> On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote: >>> On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote: >>>> On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote: >>>>> So if I understand this correctly, it sounds like that some Pixelbooks >>>>> boot up >>>>> with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without >>>>> the >>>>> panel actually having DPCD backlight controls enabled? >>>> >>>> It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set >>>> backlight.enabled = false. By changing backlight.level = max, >>>> backlight.enabled is now set to true. This results in losing backlight >>>> control on boot (since the enable routine is no longer invoked). >>>> >>> Ahhh ok, I'm fine with that - review still stands :) >> >> Pinging intel maintainers, could someone please apply this? >> >> >> Sean >> >>>> Sean >>>> >>>>> If I'm understanding that correctly, then this patch looks good to me: >>>>> >>>>> Reviewed-by: Lyude Paul <lyude@redhat.com> >>>>> >>>>> On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote: >>>>>> From: Sean Paul <seanpaul@chromium.org> >>>>>> >>>>>> In commit 79946723092b ("drm/i915: Assume 100% brightness when not in >>>>>> DPCD control mode"), we fixed the brightness level when DPCD control >>>>>> was >>>>>> not active to max brightness. This is as good as we can guess since >>>>>> most >>>>>> backlights go on full when uncontrolled. >>>>>> >>>>>> However in doing so we changed the semantics of the initial >>>>>> 'backlight.enabled' value. At least on Pixelbooks, they were relying >>>>>> on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on >>>>>> boot such that enabled would be false. This causes the device to be >>>>>> enabled when the brightness is set. Without this, brightness control >>>>>> doesn't work. So by changing brightness to max, we also flipped >>>>>> enabled >>>>>> to be true on boot. >>>>>> >>>>>> To fix this, make enabled a function of brightness and backlight >>>>>> control >>>>>> mechanism. >>>>>> >>>>>> Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in >>>>>> DPCD >>>>>> control mode") >>>>>> Cc: Lyude Paul <lyude@redhat.com> >>>>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>>>> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> >>>>>> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com> >>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>>>> Cc: Kevin Chowski <chowski@chromium.org>> >>>>>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>>>>> --- >>>>>> .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++---- >>>>>> --- >>>>>> 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644 >>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c >>>>>> @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct >>>>>> intel_dp >>>>>> *intel_dp, bool enable) >>>>>> } >>>>>> } >>>>>> >>>>>> -/* >>>>>> - * Read the current backlight value from DPCD register(s) based >>>>>> - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported >>>>>> - */ >>>>>> -static u32 intel_dp_aux_get_backlight(struct intel_connector >>>>>> *connector) >>>>>> +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector >>>>>> *connector) >>>>>> { >>>>>> struct intel_dp *intel_dp = intel_attached_dp(connector); >>>>>> struct drm_i915_private *i915 = dp_to_i915(intel_dp); >>>>>> - u8 read_val[2] = { 0x0 }; >>>>>> u8 mode_reg; >>>>>> - u16 level = 0; >>>>>> >>>>>> if (drm_dp_dpcd_readb(&intel_dp->aux, >>>>>> DP_EDP_BACKLIGHT_MODE_SET_REGISTER, >>>>>> @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct >>>>>> intel_connector *connector) >>>>>> drm_dbg_kms(&i915->drm, >>>>>> "Failed to read the DPCD register 0x%x\n", >>>>>> DP_EDP_BACKLIGHT_MODE_SET_REGISTER); >>>>>> - return 0; >>>>>> + return false; >>>>>> } >>>>>> >>>>>> + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) == >>>>>> + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; >>>>>> +} >>>>>> + >>>>>> +/* >>>>>> + * Read the current backlight value from DPCD register(s) based >>>>>> + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported >>>>>> + */ >>>>>> +static u32 intel_dp_aux_get_backlight(struct intel_connector >>>>>> *connector) >>>>>> +{ >>>>>> + struct intel_dp *intel_dp = intel_attached_dp(connector); >>>>>> + struct drm_i915_private *i915 = dp_to_i915(intel_dp); >>>>>> + u8 read_val[2] = { 0x0 }; >>>>>> + u16 level = 0; >>>>>> + >>>>>> /* >>>>>> * If we're not in DPCD control mode yet, the programmed >>>>>> brightness >>>>>> * value is meaningless and we should assume max brightness >>>>>> */ >>>>>> - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != >>>>>> - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) >>>>>> + if (!intel_dp_aux_backlight_dpcd_mode(connector)) >>>>>> return connector->panel.backlight.max; >>>>>> >>>>>> if (drm_dp_dpcd_read(&intel_dp->aux, >>>>>> DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, >>>>>> @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct >>>>>> intel_connector *connector, >>>>>> >>>>>> panel->backlight.min = 0; >>>>>> panel->backlight.level = intel_dp_aux_get_backlight(connector); >>>>>> - panel->backlight.enabled = panel->backlight.level != 0; >>>>>> + panel->backlight.enabled = >>>>>> intel_dp_aux_backlight_dpcd_mode(connector) >>>>>> && >>>>>> + panel->backlight.level != 0; >>>>>> >>>>>> return 0; >>>>>> } >>>>> -- >>>>> Cheers, >>>>> Lyude Paul (she/her) >>>>> Software Engineer at Red Hat >>>>> >>> -- >>> Cheers, >>> Lyude Paul (she/her) >>> Software Engineer at Red Hat >>> > -- > Sincerely, > Lyude Paul (she/her) > Software Engineer at Red Hat > > Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've > asked me a question, are waiting for a review/merge on a patch, etc. and I > haven't responded in a while, please feel free to send me another email to check > on my status. I don't bite! >
On Mon, 2020-10-12 at 22:02 +0000, Vivi, Rodrigo wrote: > > On Oct 12, 2020, at 2:47 PM, Lyude Paul <lyude@redhat.com> wrote: > > > > Just pushed this, but it's not in drm-tip because it would seem that > > rebuilding > > drm-tip has failed, and the conflict doesn't appear to be from any of the > > patches I pushed so I'm getting the feeling from the DRM maintainer docs I > > should probably let one of the drm-misc-folks handle the conflict. > > conflicts solved. feel free to push now. Thank you! > > For the drm-misc I simply went with the drm-misc-next solution and for the > drm-intel > I went with the drm-intel-next-queued one. > > > On Mon, 2020-10-12 at 13:50 -0400, Sean Paul wrote: > > > On Tue, Sep 22, 2020 at 11:36 AM Lyude Paul <lyude@redhat.com> wrote: > > > > On Tue, 2020-09-22 at 09:39 -0400, Sean Paul wrote: > > > > > On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@redhat.com> wrote: > > > > > > So if I understand this correctly, it sounds like that some > > > > > > Pixelbooks > > > > > > boot up > > > > > > with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, > > > > > > without > > > > > > the > > > > > > panel actually having DPCD backlight controls enabled? > > > > > > > > > > It boots with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB == 0, which used to set > > > > > backlight.enabled = false. By changing backlight.level = max, > > > > > backlight.enabled is now set to true. This results in losing backlight > > > > > control on boot (since the enable routine is no longer invoked). > > > > > > > > > Ahhh ok, I'm fine with that - review still stands :) > > > > > > Pinging intel maintainers, could someone please apply this? > > > > > > > > > Sean > > > > > > > > Sean > > > > > > > > > > > If I'm understanding that correctly, then this patch looks good to > > > > > > me: > > > > > > > > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com> > > > > > > > > > > > > On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote: > > > > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > > > > > In commit 79946723092b ("drm/i915: Assume 100% brightness when not > > > > > > > in > > > > > > > DPCD control mode"), we fixed the brightness level when DPCD > > > > > > > control > > > > > > > was > > > > > > > not active to max brightness. This is as good as we can guess > > > > > > > since > > > > > > > most > > > > > > > backlights go on full when uncontrolled. > > > > > > > > > > > > > > However in doing so we changed the semantics of the initial > > > > > > > 'backlight.enabled' value. At least on Pixelbooks, they were > > > > > > > relying > > > > > > > on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 > > > > > > > on > > > > > > > boot such that enabled would be false. This causes the device to > > > > > > > be > > > > > > > enabled when the brightness is set. Without this, brightness > > > > > > > control > > > > > > > doesn't work. So by changing brightness to max, we also flipped > > > > > > > enabled > > > > > > > to be true on boot. > > > > > > > > > > > > > > To fix this, make enabled a function of brightness and backlight > > > > > > > control > > > > > > > mechanism. > > > > > > > > > > > > > > Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in > > > > > > > DPCD > > > > > > > control mode") > > > > > > > Cc: Lyude Paul <lyude@redhat.com> > > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > > > > > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com> > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > Cc: Kevin Chowski <chowski@chromium.org>> > > > > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > > --- > > > > > > > .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++--- > > > > > > > - > > > > > > > --- > > > > > > > 1 file changed, 20 insertions(+), 11 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 acbd7eb66cbe..036f504ac7db 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > > > > > @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct > > > > > > > intel_dp > > > > > > > *intel_dp, bool enable) > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > -/* > > > > > > > - * Read the current backlight value from DPCD register(s) based > > > > > > > - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > > > > > > > - */ > > > > > > > -static u32 intel_dp_aux_get_backlight(struct intel_connector > > > > > > > *connector) > > > > > > > +static bool intel_dp_aux_backlight_dpcd_mode(struct > > > > > > > intel_connector > > > > > > > *connector) > > > > > > > { > > > > > > > struct intel_dp *intel_dp = intel_attached_dp(connector); > > > > > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > > > > > - u8 read_val[2] = { 0x0 }; > > > > > > > u8 mode_reg; > > > > > > > - u16 level = 0; > > > > > > > > > > > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, > > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > > > > > @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct > > > > > > > intel_connector *connector) > > > > > > > drm_dbg_kms(&i915->drm, > > > > > > > "Failed to read the DPCD register > > > > > > > 0x%x\n", > > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER); > > > > > > > - return 0; > > > > > > > + return false; > > > > > > > } > > > > > > > > > > > > > > + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) == > > > > > > > + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; > > > > > > > +} > > > > > > > + > > > > > > > +/* > > > > > > > + * Read the current backlight value from DPCD register(s) based > > > > > > > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported > > > > > > > + */ > > > > > > > +static u32 intel_dp_aux_get_backlight(struct intel_connector > > > > > > > *connector) > > > > > > > +{ > > > > > > > + struct intel_dp *intel_dp = intel_attached_dp(connector); > > > > > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > > > > > + u8 read_val[2] = { 0x0 }; > > > > > > > + u16 level = 0; > > > > > > > + > > > > > > > /* > > > > > > > * If we're not in DPCD control mode yet, the programmed > > > > > > > brightness > > > > > > > * value is meaningless and we should assume max brightness > > > > > > > */ > > > > > > > - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != > > > > > > > - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) > > > > > > > + if (!intel_dp_aux_backlight_dpcd_mode(connector)) > > > > > > > return connector->panel.backlight.max; > > > > > > > > > > > > > > if (drm_dp_dpcd_read(&intel_dp->aux, > > > > > > > DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, > > > > > > > @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct > > > > > > > intel_connector *connector, > > > > > > > > > > > > > > panel->backlight.min = 0; > > > > > > > panel->backlight.level = > > > > > > > intel_dp_aux_get_backlight(connector); > > > > > > > - panel->backlight.enabled = panel->backlight.level != 0; > > > > > > > + panel->backlight.enabled = > > > > > > > intel_dp_aux_backlight_dpcd_mode(connector) > > > > > > > && > > > > > > > + panel->backlight.level != 0; > > > > > > > > > > > > > > return 0; > > > > > > > } > > > > > > -- > > > > > > Cheers, > > > > > > Lyude Paul (she/her) > > > > > > Software Engineer at Red Hat > > > > > > > > > > -- > > > > Cheers, > > > > Lyude Paul (she/her) > > > > Software Engineer at Red Hat > > > > > > -- > > Sincerely, > > Lyude Paul (she/her) > > Software Engineer at Red Hat > > > > Note: I deal with a lot of emails and have a lot of bugs on my plate. If > > you've > > asked me a question, are waiting for a review/merge on a patch, etc. and I > > haven't responded in a while, please feel free to send me another email to > > check > > on my status. I don't bite! > >
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 acbd7eb66cbe..036f504ac7db 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable) } } -/* - * Read the current backlight value from DPCD register(s) based - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported - */ -static u32 intel_dp_aux_get_backlight(struct intel_connector *connector) +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector *connector) { struct intel_dp *intel_dp = intel_attached_dp(connector); struct drm_i915_private *i915 = dp_to_i915(intel_dp); - u8 read_val[2] = { 0x0 }; u8 mode_reg; - u16 level = 0; if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct intel_connector *connector) drm_dbg_kms(&i915->drm, "Failed to read the DPCD register 0x%x\n", DP_EDP_BACKLIGHT_MODE_SET_REGISTER); - return 0; + return false; } + return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) == + DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; +} + +/* + * Read the current backlight value from DPCD register(s) based + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported + */ +static u32 intel_dp_aux_get_backlight(struct intel_connector *connector) +{ + struct intel_dp *intel_dp = intel_attached_dp(connector); + struct drm_i915_private *i915 = dp_to_i915(intel_dp); + u8 read_val[2] = { 0x0 }; + u16 level = 0; + /* * If we're not in DPCD control mode yet, the programmed brightness * value is meaningless and we should assume max brightness */ - if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != - DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) + if (!intel_dp_aux_backlight_dpcd_mode(connector)) return connector->panel.backlight.max; if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector, panel->backlight.min = 0; panel->backlight.level = intel_dp_aux_get_backlight(connector); - panel->backlight.enabled = panel->backlight.level != 0; + panel->backlight.enabled = intel_dp_aux_backlight_dpcd_mode(connector) && + panel->backlight.level != 0; return 0; }