On Thu, 2021-10-28 at 11:27 -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Oct 26, 2021 at 3:09 PM Lyude Paul <lyude@xxxxxxxxxx> wrote:
> >
> > As it turns out, apparently some machines will actually leave additional
> > backlight functionality like dynamic backlight control on before the OS
> > loads. Currently we don't take care to disable unsupported features when
> > writing back the backlight mode, which can lead to some rather strange
> > looking behavior when adjusting the backlight.
> >
> > So, let's fix this by ensuring we only keep supported features enabled for
> > panel backlights - which should fix some of the issues we were seeing from
> > this on fi-bdw-samus.
> >
> > Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> > Fixes: 867cf9cd73c3 ("drm/dp: Extract i915's eDP backlight code into DRM
> > helpers")
> > ---
> > drivers/gpu/drm/drm_dp_helper.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index ada0a1ff262d..8f2032a955cf 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -3372,7 +3372,9 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux,
> > const struct drm_edp_backli
> > return ret < 0 ? ret : -EIO;
> > }
> >
> > - new_dpcd_buf = dpcd_buf;
> > + /* Disable any backlight functionality we don't support that might
> > be on */
> > + new_dpcd_buf = dpcd_buf & (DP_EDP_BACKLIGHT_CONTROL_MODE_MASK |
> > + DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE);
>
> My first thought when reading the above was: if we're masking so much
> stuff out, why do we bother reading the old value back out at all?
>
> I guess the two places you use the old value for are:
>
> 1. You avoid setting the "DP_EDP_PWMGEN_BIT_COUNT" if the backlight
> was already configured for DPCD mode.
>
> 2. You avoid writing the register if you didn't change it.
>
> I would actually argue that use #1 is probably a bug. If you're
> worried about the firmware leaving the backlight configured in a
> strange way, it could very well have left the backlight configured in
> DPCD mode but set a different "bit count" than you want, right? Maybe
> you should just always set the bit count?
>
> Use #2 is fine, but does it buy you anything? Are writes to the DCPD
> bus somehow more expensive than reads? ...or maybe you're expecting
> that a display will glitch / act badly if you write the same value
> that's already there?
>
>
> So I guess my instinct here is that you should avoid reading all
> together and just program the value you want.
Good point, will respin this in a little bit
>
> -Doug
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
