| Message ID | 1519109569-13211-1-git-send-email-mustamin.b.mustaffa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Mustamin B Mustaffa (2018-02-20 06:52:49) > 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. > > v2: > - Remove unnecessary information in commit message. > - Assign vbt.backlight.controller to a backlight_controller variable and > return the variable value. > v3: > - Rebased to latest code base. > - updated commit title. > > Signed-off-by: Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 1868f73..f9b922d 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -655,18 +655,15 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv) > { > struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp)); > Ah, a purveyor of newlines. > + int backlight_controller = dev_priv->vbt.backlight.controller; Isn't the vbt->backlight.controller sanitized to the panel->backlight.controller at this point? -Chris
Hi Chris, Would you rather for me to use following line instead? + int backlight_controller = intel_dp->attached_connector->panel.backlight.controller; Best regard Mustamin -----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Tuesday, February 20, 2018 4:39 PM To: Mustaffa, Mustamin B <mustamin.b.mustaffa@intel.com>; intel-gfx@lists.freedesktop.org Cc: Mustaffa, Mustamin B <mustamin.b.mustaffa@intel.com> Subject: Re: [Intel-gfx] [V3] drm/i915: Enable VBT based BL control for DP Quoting Mustamin B Mustaffa (2018-02-20 06:52:49) > 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. > > v2: > - Remove unnecessary information in commit message. > - Assign vbt.backlight.controller to a backlight_controller variable and > return the variable value. > v3: > - Rebased to latest code base. > - updated commit title. > > Signed-off-by: Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c index 1868f73..f9b922d 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -655,18 +655,15 @@ static enum pipe vlv_find_free_pps(struct > drm_i915_private *dev_priv) { > struct drm_i915_private *dev_priv = > to_i915(intel_dp_to_dev(intel_dp)); > Ah, a purveyor of newlines. > + int backlight_controller = dev_priv->vbt.backlight.controller; Isn't the vbt->backlight.controller sanitized to the panel->backlight.controller at this point? -Chris
Quoting Mustaffa, Mustamin B (2018-02-20 08:44:45) > Hi Chris, > > Would you rather for me to use following line instead? > > + int backlight_controller = intel_dp->attached_connector->panel.backlight.controller; I think so, Jani would be best to answer the question about how vbt/panel tie together with backlight and which is meant to be used. -Chris
On Tue, 20 Feb 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Mustaffa, Mustamin B (2018-02-20 08:44:45) >> Hi Chris, >> >> Would you rather for me to use following line instead? >> >> + int backlight_controller = intel_dp->attached_connector->panel.backlight.controller; > > I think so, Jani would be best to answer the question about how > vbt/panel tie together with backlight and which is meant to be used. We discussed this previously, and the following sequence would lead to panel.backlight.controller being used uninitialized in bxt_power_sequencer_idx because it gets set at intel_panel_setup_backlight: - intel_edp_init_connector +- intel_dp_pps_init | +- intel_dp_init_panel_power_sequencer_registers | +- intel_pps_get_registers | +- bxt_power_sequencer_idx +- intel_panel_setup_backlight I decided it wasn't worth blocking a reasonable fix (that might warrant cc: stable actually) on a refactoring. We can (and should) do the refactoring later though. BR, Jani.
On Tue, Feb 20, 2018 at 11:43:57AM +0200, Jani Nikula wrote: > On Tue, 20 Feb 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Mustaffa, Mustamin B (2018-02-20 08:44:45) > >> Hi Chris, > >> > >> Would you rather for me to use following line instead? > >> > >> + int backlight_controller = intel_dp->attached_connector->panel.backlight.controller; > > > > I think so, Jani would be best to answer the question about how > > vbt/panel tie together with backlight and which is meant to be used. > > We discussed this previously, and the following sequence would lead to > panel.backlight.controller being used uninitialized in > bxt_power_sequencer_idx because it gets set at > intel_panel_setup_backlight: > > - intel_edp_init_connector > +- intel_dp_pps_init > | +- intel_dp_init_panel_power_sequencer_registers > | +- intel_pps_get_registers > | +- bxt_power_sequencer_idx > +- intel_panel_setup_backlight > > I decided it wasn't worth blocking a reasonable fix (that might warrant > cc: stable actually) on a refactoring. We can (and should) do the > refactoring later though. Oh! please ignore the email I just sent... I hadn't noticed this discussion here before... So, with this info here: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 1868f73..f9b922d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -655,18 +655,15 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv) { struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp)); + int backlight_controller = dev_priv->vbt.backlight.controller; + lockdep_assert_held(&dev_priv->pps_mutex); /* We should never land here with regular DP ports */ WARN_ON(!intel_dp_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 backlight_controller; intel_dp->pps_reset = false; @@ -676,7 +673,7 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv) */ intel_dp_init_panel_power_sequencer_registers(intel_dp, false); - return 0; + return backlight_controller; } typedef bool (*vlv_pipe_check)(struct drm_i915_private *dev_priv,
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. v2: - Remove unnecessary information in commit message. - Assign vbt.backlight.controller to a backlight_controller variable and return the variable value. v3: - Rebased to latest code base. - updated commit title. Signed-off-by: Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)