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

[Rodrigo Vivi]

On Tue, Feb 20, 2018 at 11:43:57AM +0200, Jani Nikula wrote:
> On Tue, 20 Feb 2018, Chris Wilson  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 

> 
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Tue, 20 Feb 2018, Chris Wilson  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.


-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Mustaffa, Mustamin B]

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:ch...@chris-wilson.co.uk] 
Sent: Tuesday, February 20, 2018 4:39 PM
To: Mustaffa, Mustamin B <mustamin.b.musta...@intel.com>; 
intel-gfx@lists.freedesktop.org
Cc: Mustaffa, Mustamin B <mustamin.b.musta...@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.musta...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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 
> ---
>  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Mustamin B Mustaffa]

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 
---
 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));
 
+   int backlight_controller = dev_priv->vbt.backlight.controller;
+
lockdep_assert_held(_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,
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Mustamin B Mustaffa]

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 
---
 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));
 
+   int backlight_controller = dev_priv->vbt.backlight.controller;
+
lockdep_assert_held(_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,
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx