From: "Bloomfield, Jon" <jon.bloomfield@intel.com>
To: "Deak, Imre" <imre.deak@intel.com>,
Jani Nikula <jani.nikula@linux.intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>,
"Mustaffa, Mustamin B" <mustamin.b.mustaffa@intel.com>
Subject: Re: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP
Date: Tue, 6 Jun 2017 14:58:43 +0000 [thread overview]
Message-ID: <AD48BB7FB99B174FBCC69E228F58B3B63906D39B@fmsmsx120.amr.corp.intel.com> (raw)
In-Reply-To: <20170606123422.GA10592@ideak-desk.fi.intel.com>
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Imre Deak
> Sent: Tuesday, June 6, 2017 5:34 AM
> To: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Mustaffa, Mustamin B
> <mustamin.b.mustaffa@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@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 :-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2017-06-06 15:02 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-06 7:09 [PATCH] drm/i915/bxt: Enable VBT based BL control for DP Mustamin B Mustaffa
2017-06-06 7:28 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-06-06 9:24 ` [PATCH] " Jani Nikula
2017-06-06 12:34 ` Imre Deak
2017-06-06 14:58 ` Bloomfield, Jon [this message]
2017-06-06 15:26 ` Imre Deak
2017-06-07 8:16 ` Jani Nikula
2017-06-07 14:12 ` Bloomfield, Jon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AD48BB7FB99B174FBCC69E228F58B3B63906D39B@fmsmsx120.amr.corp.intel.com \
--to=jon.bloomfield@intel.com \
--cc=imre.deak@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=mustamin.b.mustaffa@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.