Re: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 06 Jun 2017, Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com> wrote:
> Currently, BXT_PP is hardcoded with value '0'.
> It practically disabled eDP backlight on MRB (BXT) platform.
>
> This patch will tell which BXT_PP registers (there are two set of PP_CONTROL in the spec)
> to be used as defined in VBT (Video Bios Timing table) and this will enabled eDP
> backlight controller on MRB (BXT) platform.

Didn't look at the spec. Is there a 1:1 mapping/routing with the two PPS
and backlight PWM?

Does this hold for Geminilake too, as your change impacts that as well?

> Change-Id: I42242d8def30d09298b629e3cd4828016189c3fa

Drop this.

> Signed-off-by: Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com>
> Tracked-On: https://jira01.devtools.intel.com/browse/OAM-46254

Drop this. Even I can't access this.

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d1670b8..124f58b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -591,13 +591,8 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp)
>  	/* We should never land here with regular DP ports */
>  	WARN_ON(!is_edp(intel_dp));
>  
> -	/*
> -	 * TODO: BXT has 2 PPS instances. The correct port->PPS instance
> -	 * mapping needs to be retrieved from VBT, for now just hard-code to
> -	 * use instance #0 always.
> -	 */
>  	if (!intel_dp->pps_reset)
> -		return 0;
> +		return dev_priv->vbt.backlight.controller;

So the existing code around here looks a bit convoluted, not least
because now pretty much all PPS access first does

 - intel_pps_get_registers(), which calls
 - bxt_power_sequencer_idx(), which calls
 - intel_dp_init_panel_power_sequencer_registers(), which calls
 - intel_pps_get_registers()...

With your change, for controller == 1 and pps_reset == true, the first
time the registers are needed, we'll initialize the correct controller 1
registers, but controller 0 registers are returned. From there on, we'll
keep returning controller 1 registers until pps_reset is set to true
again.

Cc: Imre as author of commits 78597996370c ("drm/i915/bxt: Fix PPS lost
state after suspend breaking eDP link training") and 8e8232d51878
("drm/i915: Deduplicate PPS register retrieval") which I think create
the loop.


BR,
Jani.



>  
>  	intel_dp->pps_reset = false;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx