Date:   Tue, 28 Sep 2021 17:04:16 -0400
From:   Lyude Paul <lyude@...hat.com>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Intel Graphics <intel-gfx@...ts.freedesktop.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Rajeev Nandan <rajeevny@...eaurora.org>,
        Satadru Pramanik <satadru@...il.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Ben Skeggs <bskeggs@...hat.com>,
        Ville Syrjälä 
        <ville.syrjala@...ux.intel.com>, Sean Paul <seanpaul@...omium.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS" 
        <nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH 2/3] drm/dp, drm/i915: Add support for VESA backlights
 using PWM for brightness control

On Tue, 2021-09-28 at 13:00 -0700, Doug Anderson wrote:
> Hi,
> 
> 
> I'm not sure I understand the comment above. You say "enabled/disabled
> via PWM" and that doesn't make sense w/ my mental model. Normally I
> think of a PWM allowing you to adjust the brightness and there being a
> separate GPIO that's in charge of enable/disable. To some extent you

Oops - you're completely right, it is a GPIO pin and I got myself
confused since in i915 I'm the chipset-specific callbacks for turning
the panel on are intertwined with the PWM callbacks.

> could think of a PWM as being "disabled" when its duty cycle is 0%,
> but usually there's separate "enable" logic that really has nothing to
> do with the PWM itself.
> 
> In general, it seems like the options are:
> 
> 1. DPCD controls PWM and the "enable" logic.
> 
> 2. DPCD controls PWM but requires an external "enable" GPIO.
> 
> 3. We require an external PWM but DPCD controls the "enable" logic.
> 
> Maybe you need a second "capability" to describe whether the client of
> your code knows how to control an enable GPIO? ...or perhaps better
> you don't need a capability and you can just assume that if the client
> needs to set an "enable" GPIO that it will do so. That would match how
> things work today. AKA:
> 
> a) Client calls the AUX backlight code to "enable"
> 
> b) AUX backlight code will set the "enable" bit if supported.
> 
> c) Client will set the "enable" GPIO if it knows about one.
> 
> Presumably only one of b) or c) will actually do something. If neither
> does something then this panel simply isn't compatible with this
> board.

I will definitely take note from this explanation and rewrite some of
the helper docs I'm updating to reflect this, thank you!

Being that I think panel drivers are basically the only other user of
this driver, if you think this is the way to go I'm OK with this. My
original reasoning for having a cap for this was worrying about the ARM
drivers handling this, along with potentially changing backlight
behavior in nouveau. I'm realizing now though that those are probably
problems for nouveau and not the helper, and I could probably avoid
hitting any issues by just adding some additional DPCD checks for GPIO
enabling/PWM passthrough in nouveau itself.

So I'll drop the cap in the next respin of this
> 
> 
> > +/**
> > + * drm_edp_backlight_supported() - Check an eDP DPCD for VESA backlight
> > support
> > + * @aux: The AUX channel, only used for debug logging
> > + * @edp_dpcd: The DPCD to check
> > + * @caps: The backlight capabilities this driver supports
> > + *
> > + * Returns: %True if @edp_dpcd indicates that VESA backlight controls are
> > supported, %false
> > + * otherwise
> > + */
> > +bool drm_edp_backlight_supported(struct drm_dp_aux *aux,
> > +                                const u8
> > edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE],
> > +                                enum drm_edp_backlight_driver_caps caps)
> > +{
> > +       if (!(edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP))
> > +               return false;
> > +
> > +       if (!(caps & DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM) &&
> > +           (!(edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) ||
> > +            !(edp_dpcd[2] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))) {
> 
> Elsewhere you match DP_EDP_BACKLIGHT_AUX_ENABLE_CAP against
> edp_dpcd[1]. Here you match against [2]. Are you sure that's correct?

absolutely not! thank you for catching this

> 
> 
> >  /*
> >   * DisplayPort AUX channel
> >   */
> > @@ -2200,7 +2182,11 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc,
> > enum drm_dp_quirk quirk)
> >   * @pwm_freq_pre_divider: The PWM frequency pre-divider value being used
> > for this backlight, if any
> >   * @max: The maximum backlight level that may be set
> >   * @lsb_reg_used: Do we also write values to the
> > DP_EDP_BACKLIGHT_BRIGHTNESS_LSB register?
> > - * @aux_enable: Does the panel support the AUX enable cap?
> > + * @aux_enable: Does the panel support the AUX enable cap? Always %false
> > when the driver doesn't
> > + * support %DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM
> 
> Why is aux_enable always false if it doesn't support
> DRM_EDP_BACKLIGHT_DRIVER_CAP_PWM? It doesn't seem like the code
> enforces this and I'm not sure why it would. Am I confused?

This was mainly just to keep the behavior identical for drivers that
didn't support controlling backlights like this, but re: the response I
wrote up above I think we can just totally drop the caps.

> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Powered by blists - more mailing lists