On Thu, 17 Sept 2020 at 03:19, Lyude Paul <lyude@xxxxxxxxxx> 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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
