On Thu, 2021-01-14 at 09:12 +0200, Jani Nikula wrote:
On Wed, 13 Jan 2021, 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.
Additionally, we drop the call to lpt_get_backlight() in lpt_setup_backlight(), and avoid unconditionally writing the PWM value that we get from it and only write it back if we're in CPU mode, and switching to PCH mode. The reason for this is because in the original codepath for this, it was expected that the intel_panel_bl_funcs->setup() hook would be responsible for fetching the initial backlight level. On lpt systems, the only time we could ever be in PCH backlight mode is during the initial driver load - meaning that outside of the setup() hook, lpt_get_backlight() will always be the callback used for retrieving the current backlight level. After this patch we still need to fetch and write-back the PCH backlight value if we're switching from CPU mode to PCH, but because intel_pwm_setup_backlight() will retrieve the backlight level after setup() using the get() hook, which always ends up being lpt_get_backlight(). Thus
- an additional call to lpt_get_backlight() in lpt_setup_backlight() is
made redundant.
v7:
- Use panel->backlight.pwm_funcs->get() to get the backlight level in
intel_pwm_setup_backlight(), lest we upset lockdep
I think this change is wrong, as it now bypasses intel_panel_invert_pwm_level(). Please explain. I don't see anything in there that could trigger a lockdep warning.
yeah-this was definitely me misunderstanding what the issue we were hitting here was.
Perhaps it's the below you're referring to, but I think the root cause is different?
@@ -1788,22 +1780,17 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965; ctl = intel_de_read(dev_priv, VLV_BLC_PWM_CTL(pipe)); - panel->backlight.max = ctl >> 16; + panel->backlight.pwm_level_max = ctl >> 16; - if (!panel->backlight.max) - panel->backlight.max = get_backlight_max_vbt(connector); + if (!panel->backlight.pwm_level_max) + panel->backlight.pwm_level_max = get_backlight_max_vbt(connector); - if (!panel->backlight.max) + if (!panel->backlight.pwm_level_max) return -ENODEV; - panel->backlight.min = get_backlight_min_vbt(connector); + panel->backlight.pwm_level_min = get_backlight_min_vbt(connector); - val = _vlv_get_backlight(dev_priv, pipe);
Turns out this is a meaningful change, as the higher level vlv_get_backlight() function that will be called instead hits:
<4>[ 12.870202] i915 0000:00:02.0: drm_WARN_ON(!drm_modeset_is_locked(&dev-
mode_config.connection_mutex))
in intel_connector_get_pipe(connector).
It's a real problem. See this, it's obvious (in retrospect):
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19348/fi-bsw-kefka/igt@ru...
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19348/fi-bsw-kefka/boot0....
I don't have a quick answer how this could be handled neatly. Perhaps the ->get call (or rather, intel_pwm_get_backlight) to set panel->backlight.level needs to be spread out to the end of each pwm_funcs->setup function after all? Though it's at the wrong abstraction level wrt level being a higher level, uh, level.
I don't think it's enough to just grab connection_mutex around setup (and even checking if we can do that is a bunch of digging) - I think it's likely intel_connector_get_pipe() returns INVALID_PIPE at that point.
Okay, here's a clumsy suggestion that I think works around this and unblocks the series until we figure out a better way:
- At the end of vlv_setup_backlight():
/* add fixme comment about how wrong this is */ panel->backlight.level = intel_panel_invert_pwm_level(connector, _vlv_get_backlight());
- In intel_pwm_setup_backlight() only set level if ->setup didn't:
if (!panel->backlight.level) panel->backlight.level = intel_pwm_get_backlight(connector);
What do you think?
Hm, I might have a better idea. Why not just adjust struct intel_panel_bl_funcs so that it accepts an enum pipe, since we're already being passed a pipe in -
setup(). Then in places where we call ->get() we can just make retrieving the
currently set pipe from the atomic state or somewhere else the responsibility of the caller. I think I'm going to add an additional patch to give this a shot and see how it goes.
BR, Jani.
- val = intel_panel_compute_brightness(connector, val); - panel->backlight.level = clamp(val, panel->backlight.min, - panel->backlight.max);
- panel->backlight.enabled = ctl2 & BLM_PWM_ENABLE; + panel->backlight.pwm_enabled = ctl2 & BLM_PWM_ENABLE; return 0; } @@ -1828,24 +1815,18 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused)