Re: [Intel-gfx] [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value

[Lyude Paul <lyude@redhat.com>]

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!
> > 
-- 
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!

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