> -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] > Sent: Wednesday, June 7, 2017 1:16 AM > To: Deak, Imre <imre.deak@xxxxxxxxx>; Bloomfield, Jon > <jon.bloomfield@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Mustaffa, Mustamin B > <mustamin.b.mustaffa@xxxxxxxxx> > Subject: Re: [PATCH] drm/i915/bxt: Enable VBT based BL control > for DP > > On Tue, 06 Jun 2017, Imre Deak <imre.deak@xxxxxxxxx> wrote: > > On Tue, Jun 06, 2017 at 05:58:43PM +0300, Bloomfield, Jon wrote: > >> > -----Original Message----- > >> > From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On > Behalf > >> > Of Imre Deak > >> > Sent: Tuesday, June 6, 2017 5:34 AM > >> > To: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > >> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Mustaffa, Mustamin B > >> > <mustamin.b.mustaffa@xxxxxxxxx> > >> > Subject: Re: [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@xxxxxxxxx> 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? Yes, I was having doubts about this yesterday too. I can find nothing the BSpec to indicate any relationship at all between PWM and PPS. Mustamin is from IOTG and this for a specific product they're working on. The correct fix is probably to extend VBT to include a separate PPS select field and then key off that. As this is a new product, there should be no issues with updating the VBT (I hope), but how does that sit with you guys ? Hardcoding is certainly plain wrong, even if all current released products do need 0. > > I just involved Imre here because the existing code is, I think, > unnecessarily hard to follow. _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
