Re: [PATCH v4 3/5] drm/dp: Disable unsupported features in DP_EDP_BACKLIGHT_MODE_SET_REGISTER

[Lyude Paul]

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