Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200916171855.129511-1-lyude@redhat.com> <20200916171855.129511-4-lyude@redhat.com>
In-Reply-To: <20200916171855.129511-4-lyude@redhat.com>
From:   Dave Airlie <airlied@gmail.com>
Date:   Thu, 26 Nov 2020 11:03:39 +1000
Message-ID: <CAPM=9tzLK3caEW661tCeSPhn0G0NihuztCdgyABDgtvZRvV4xA@mail.gmail.com>
Subject: Re: [Intel-gfx] [RFC v2 3/8] drm/i915: Keep track of pwm-related
 backlight hooks separately
To:     Lyude Paul <lyude@redhat.com>
Cc:     Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
        Lucas De Marchi <lucas.demarchi@intel.com>,
        David Airlie <airlied@linux.ie>,
        open list <linux-kernel@vger.kernel.org>,
        Chris Wilson <chris@chris-wilson.co.uk>,
        Vasily Khoruzhick <anarsoul@gmail.com>,
        Sean Paul <seanpaul@chromium.org>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Wambui Karuga <wambui.karugax@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 17 Sept 2020 at 03:19, Lyude Paul <lyude@redhat.com> wrote:
>
> Currently, every different type of backlight hook that i915 supports is
> pretty straight forward - you have a backlight, probably through PWM
> (but maybe DPCD), with a single set of platform-specific hooks that are
> used for controlling it.
>
> HDR backlights, in particular VESA and Intel's HDR backlight
> implementations, can end up being more complicated. With Intel's
> proprietary interface, HDR backlight controls always run through the
> DPCD. When the backlight is in SDR backlight mode however, the driver
> may need to bypass the TCON and control the backlight directly through
> PWM.
>
> So, in order to support this we'll need to split our backlight callbacks
> into two groups: a set of high-level backlight control callbacks in
> intel_panel, and an additional set of pwm-specific backlight control
> callbacks. This also implies a functional changes for how these
> callbacks are used:
>
> * We now keep track of two separate backlight level ranges, one for the
>   high-level backlight, and one for the pwm backlight range
> * We also keep track of backlight enablement and PWM backlight
>   enablement separately
> * Since the currently set backlight level might not be the same as the
>   currently programmed PWM backlight level, we stop setting
>   panel->backlight.level with the currently programmed PWM backlight
>   level in panel->backlight.pwm_funcs.setup(). Instead, we rely
>   on the higher level backlight control functions to retrieve the
>   current PWM backlight level (in this case, intel_pwm_get_backlight()).
>   Note that there are still a few PWM backlight setup callbacks that
>   do actually need to retrieve the current PWM backlight level, although
>   we no longer save this value in panel->backlight.level like before.
> * panel->backlight.pwm_funcs.enable()/disable() both accept a PWM
>   brightness level, unlike their siblings
>   panel->backlight.enable()/disable(). This is so we can calculate the
>   actual PWM brightness level we want to set on disable/enable in the
>   higher level backlight enable()/disable() functions, since this value
>   might be scaled from a brightness level that doesn't come from PWM.

Oh this patch is a handful, I can see why people stall out here.

I'm going to be annoying maintainer and see if you can clean this up a
bit in advance
of this patch.

1) move the callbacks out of struct intel_panel.backlight into a separate struct
and use const static object tables, having fn ptrs and data co-located
in a struct
isn't great.

strcut intel_panel_backlight_funcs {

};
struct intel_panel {
    struct {
        struct intel_panel_backlight_funcs *funcs;
    };
};

type of thing.

I think you could reuse the backlight funcs struct for the pwm stuff
as well. (maybe with an assert on hz_to_pwm for the old hooks).

2) change the apis to pass 0 down in a separate patch, this modifies a
bunch of apis to pass in an extra level parameter, do that
first in a separate patch that doesn't change anything but hands 0
down the chain. Then switch over in another patch.

3) One comment in passing below.
>
>
> -       if (cpu_mode)
> -               val = pch_get_backlight(connector);
> -       else
> -               val = lpt_get_backlight(connector);
> -       val = intel_panel_compute_brightness(connector, val);
> -       panel->backlight.level = clamp(val, panel->backlight.min,
> -                                      panel->backlight.max);
>
>         if (cpu_mode) {
> +               val = intel_panel_sanitize_pwm_level(connector, pch_get_backlight(connector));
> +
>                 drm_dbg_kms(&dev_priv->drm,
>                             "CPU backlight register was enabled, switching to PCH override\n");
>
>                 /* Write converted CPU PWM value to PCH override register */
> -               lpt_set_backlight(connector->base.state, panel->backlight.level);
> +               lpt_set_backlight(connector->base.state, val);
>                 intel_de_write(dev_priv, BLC_PWM_PCH_CTL1,
>                                pch_ctl1 | BLM_PCH_OVERRIDE_ENABLE);
>
The change here confused me since it no longer calls lpt_get_backlight
in this path, the commit msg might explain this, but it didn't explain
is so I could figure out if that was a mistake or intentional.

Dave.