On Mon, Sep 21, 2020 at 6:35 PM Lyude Paul <lyude@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> > From: Sean Paul <seanpaul@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@xxxxxxxxx>
> > Cc: "Ville Syrjälä" <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Cc: Kevin Chowski <chowski@xxxxxxxxxxxx>>
> > Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> > ---
> > .../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
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
