On Thu, 2020-11-26 at 11:03 +1000, Dave Airlie wrote:
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.
Not annoying at all :), I was hoping there'd be a good bit of criticism on this patch series since it's been hard to figure out if I'm even implementing things in the right way or not (especially because I really don't know what the HDR side of this is going to look like, although I assume it's probably going to be pretty hands-off in the kernel).
JFYI too for folks on the list, any suggestions about the HDR side of this are super appreciated. I'm barely familiar with such things.
- 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).
- 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.
- 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.
Will address these in the next respin, thanks for the review!
Dave.