Re: [Nouveau] [PATCH 2/3] drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control

[Doug Anderson <dianders@chromium.org>]

Hi,

On Mon, Sep 27, 2021 at 1:12 PM Lyude Paul <lyude@redhat.com> wrote:
>
> @@ -3305,11 +3313,10 @@ EXPORT_SYMBOL(drm_edp_backlight_enable);
>   * @bl: Backlight capability info from drm_edp_backlight_init()
>   *
>   * This function handles disabling DPCD backlight controls on a panel over AUX. Note that some
> - * panels have backlights that are enabled/disabled by other means, despite having their brightness
> - * values controlled through DPCD. On such panels &drm_edp_backlight_info.aux_enable will be set to
> - * %false, this function will become a no-op (and we will skip updating
> - * %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must take care to perform it's own
> - * implementation specific step for disabling the backlight.
> + * panels have backlights that are enabled/disabled via PWM. On such panels
> + * &drm_edp_backlight_info.aux_enable will be set to %false, this function will become a no-op (and
> + * we will skip updating %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must handle disabling the
> + * backlight via PWM.

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


> +/**
> + * 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?


>  /*
>   * 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?