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

[Bloomfield, Jon]

> -----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 :-)