[Intel-gfx] [PATCH] drm/i915/bxt: Enable VBT based BL control for DP

[Jani Nikula]

On Tue, 06 Jun 2017, Imre Deak <imre.deak at intel.com> wrote:
> On Tue, Jun 06, 2017 at 05:58:43PM +0300, Bloomfield, Jon wrote:
>> > -----Original Message-----
>> > From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf
>> > Of Imre Deak
>> > Sent: Tuesday, June 6, 2017 5:34 AM
>> > To: Jani Nikula <jani.nikula at linux.intel.com>
>> > Cc: intel-gfx at lists.freedesktop.org; Mustaffa, Mustamin B
>> > <mustamin.b.mustaffa at intel.com>
>> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/bxt: Enable VBT based BL control
>> > for DP
>> > 
>> > On Tue, Jun 06, 2017 at 12:24:30PM +0300, Jani Nikula wrote:
>> > > On Tue, 06 Jun 2017, Mustamin B Mustaffa
>> > <mustamin.b.mustaffa at intel.com> wrote:
>> > > > [...]
>> > > > 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.
>> > 
>> > A loop would be prevented by the pps->reset check, but agree the code
>> > isn't nice, I guess I overlooked this when I wrote it. To make things
>> > clearer we could factor out a helper from
>> > intel_dp_init_panel_power_sequencer_registers() that takes pps_registers
>> > and call this helper from bxt_power_sequencer_idx().
>> > 
>> > So how about something like the following:
>> 
>> Just checking what the intention is here because your proposed change
>> ommits the VBT fix...  Are you going to post these changes as a new
>> baseline for Mustaffa's patch ? Or are you asking Mustaffa to fold
>> these changes into his patch ? Hoping it's the former :-)
>
> The change is just to make the code clearer, unrelated to the VBT fix,
> so it should be a separate patch. I don't mind doing this as a follow-up
> to Mustaffa's patchset. What his patch here would need is just to return 
> the correct index from bxt_power_sequencer_idx() in all cases.

I think we might need to backport Mustaffa's patch to stable so we need
to do that first as a standalone change. After it has been fixed
according to Imre's and my feedback. Oh, and I'd still like someone(tm)
to check if the PPS-PWM mapping is fixed 1:1, or can we cross connect
them?

I just involved Imre here because the existing code is, I think,
unnecessarily hard to follow.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center