On Thu, 2021-01-14 at 09:12 +0200, Jani Nikula wrote: > On Wed, 13 Jan 2021, 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. > > > > 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@runner@xxxxxxxxxxxx > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19348/fi-bsw-kefka/boot0.txt > > 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: > > 1. 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()); > > > 2. 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) > -- Sincerely, Lyude Paul (she/her) Software Engineer at Red Hat Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've asked me a question, are waiting for a review/merge on a patch, etc. and I haven't responded in a while, please feel free to send me another email to check on my status. I don't bite! _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
