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;
IronPort-SDR: QdKj1qXAPN1A8+Rh1Px6eUx26iXu7wnUrMVIpxpfco2x7XrtsRVbIF4oA8vIY9DT+sxIqM82rS
 5BzCjr0b6KHg==
IronPort-SDR: cJDv/L0tfPx9bJ4fkZ1bsmZhxw7AiOgksUX2g5eH3BzEnvhub4FHEwNlujc4FtO8Vd2BOnWG/l
 sxsVVmHUs+gg==
From:   Jani Nikula <jani.nikula@intel.com>
To:     Lyude Paul <lyude@redhat.com>, dri-devel@lists.freedesktop.org,
        intel-gfx@lists.freedesktop.org
Cc:     thaytan@noraisin.net, Vasily Khoruzhick <anarsoul@gmail.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Ville =?utf-8?B?U3lyasOkbMOk?= <ville.syrjala@linux.intel.com>,
        Uma Shankar <uma.shankar@intel.com>,
        Imre Deak <imre.deak@intel.com>,
        Ramalingam C <ramalingam.c@intel.com>,
        Anshuman Gupta <anshuman.gupta@intel.com>,
        Dave Airlie <airlied@redhat.com>,
        Lucas De Marchi <lucas.demarchi@intel.com>,
        Chris Wilson <chris@chris-wilson.co.uk>,
        Hans de Goede <hdegoede@redhat.com>,
        Arnd Bergmann <arnd@arndb.de>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 1/4] drm/i915: Keep track of pwm-related backlight hooks separately
In-Reply-To: <20210113235426.2190684-2-lyude@redhat.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
References: <20210113235426.2190684-1-lyude@redhat.com> <20210113235426.2190684-2-lyude@redhat.com>
Date:   Thu, 14 Jan 2021 09:12:08 +0200
Message-ID: <871reornzr.fsf@intel.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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.

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@aborted.html
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?

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)

-- 
Jani Nikula, Intel Open Source Graphics Center