Re: [PATCH v4 2/2] drm/i915/opregion: update cadl based on actually active outputs

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 29-06-16 om 17:36 schreef Jani Nikula:
> Previously we've just shoved the first eight devices in DIDL to CADL
> (list of active outputs). Some of the active outputs may have been left
> outside of CADL. The problem is, some BIOS implementations prevent
> laptop brightness hotkey propagation if the flat panel is not active.
>
> Now that we have connector to acpi device id mapping covered, we can
> update CADL based on which outputs are actually active.
>
> v3: actually git add the dev->dev_priv change.
>
> v4: update cadl in intel_shared_dpll_commit() if intel_state->modeset
>     (Maarten)
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-and-tested-by: Peter Wu <peter@lekensteyn.nl>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  2 +
>  drivers/gpu/drm/i915/intel_display.c  |  4 ++
>  drivers/gpu/drm/i915/intel_opregion.c | 70 ++++++++++++++++++-----------------
>  3 files changed, 43 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 724d34b00196..64ab52529be8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3692,6 +3692,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>  extern int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv,
>  					 pci_power_t state);
>  extern int intel_opregion_get_panel_type(struct drm_i915_private *dev_priv);
> +extern void intel_opregion_update_cadl(struct drm_i915_private *dev_priv);
>  #else
>  static inline int intel_opregion_setup(struct drm_i915_private *dev) { return 0; }
>  static inline void intel_opregion_register(struct drm_i915_private *dev_priv) { }
> @@ -3713,6 +3714,7 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev)
>  {
>  	return -ENODEV;
>  }
> +static inline void intel_opregion_update_cadl(struct drm_i915_private *dev_priv) { }
>  #endif
>  
>  /* intel_acpi.c */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d902a70edb84..4f404900f610 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13953,6 +13953,10 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	dev_priv->wm.distrust_bios_wm = false;
>  	dev_priv->wm.skl_results = intel_state->wm_results;
>  	intel_shared_dpll_commit(state);
> +
> +	if (intel_state->modeset)
> +		intel_opregion_update_cadl(dev_priv);
> +
>  	intel_atomic_track_fbs(state);
>  
>  	if (nonblock)
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 632f0178c2b0..8b3f7e6ae4bb 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -642,24 +642,6 @@ static struct notifier_block intel_opregion_notifier = {
>   * (version 3)
>   */
>  
> -static u32 get_did(struct intel_opregion *opregion, int i)
> -{
> -	u32 did;
> -
> -	if (i < ARRAY_SIZE(opregion->acpi->didl)) {
> -		did = opregion->acpi->didl[i];
> -	} else {
> -		i -= ARRAY_SIZE(opregion->acpi->didl);
> -
> -		if (WARN_ON(i >= ARRAY_SIZE(opregion->acpi->did2)))
> -			return 0;
> -
> -		did = opregion->acpi->did2[i];
> -	}
> -
> -	return did;
> -}
> -
>  static void set_did(struct intel_opregion *opregion, int i, u32 val)
>  {
>  	if (i < ARRAY_SIZE(opregion->acpi->didl)) {
> @@ -674,6 +656,14 @@ static void set_did(struct intel_opregion *opregion, int i, u32 val)
>  	}
>  }
>  
> +static void set_cad(struct intel_opregion *opregion, int i, u32 val)
> +{
> +	if (WARN_ON(i >= ARRAY_SIZE(opregion->acpi->cadl)))
> +		return;
> +
> +	opregion->acpi->cadl[i] = val;
> +}
> +
>  static u32 acpi_display_type(struct intel_connector *connector)
>  {
>  	u32 display_type;
> @@ -759,22 +749,36 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
>  		set_did(opregion, i, 0);
>  }
>  
> -static void intel_setup_cadls(struct drm_i915_private *dev_priv)
> +/* Update CADL to reflect active outputs. */
> +void intel_opregion_update_cadl(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_opregion *opregion = &dev_priv->opregion;
> -	int i = 0;
> -	u32 disp_id;
> -
> -	/* Initialize the CADL field by duplicating the DIDL values.
> -	 * Technically, this is not always correct as display outputs may exist,
> -	 * but not active. This initialization is necessary for some Clevo
> -	 * laptops that check this field before processing the brightness and
> -	 * display switching hotkeys. Just like DIDL, CADL is NULL-terminated if
> -	 * there are less than eight devices. */
> -	do {
> -		disp_id = get_did(opregion, i);
> -		opregion->acpi->cadl[i] = disp_id;
> -	} while (++i < 8 && disp_id != 0);
> +	struct intel_crtc *crtc;
> +	int i = 0, max_active = ARRAY_SIZE(opregion->acpi->cadl);
> +
> +	for_each_intel_crtc(dev_priv->dev, crtc) {
> +		struct intel_encoder *encoder;
> +
> +		if (!crtc->active)
> +			continue;
Argh, don't use crtc->active!
We have intel_state->active_crtcs + for_each_intel_crtc_mask for this, it should fix the issue Koenig mentioned.

If fixed,

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx