[PATCH] drm/i915/bxt: Enable VBT based BL control for DP (v2)

[PATCH] drm/i915/bxt: Enable VBT based BL control for DP (v2)

[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.

Signed-off-by: Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d1670b8..4c55ef0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -585,19 +585,15 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	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(!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;
 
@@ -607,7 +603,7 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp)
 	 */
 	intel_dp_init_panel_power_sequencer_registers(dev, 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

[PATCH] drm/i915/bxt: Enable VBT based BL control for DP (v2)

[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.

Signed-off-by: Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 09f2744..e00569f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -640,19 +640,15 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	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(!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;
 
@@ -662,7 +658,7 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
 	 */
 	intel_dp_init_panel_power_sequencer_registers(dev, 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

✗ Fi.CI.BAT: failure for drm/i915/bxt: Enable VBT based BL control for DP (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915/bxt: Enable VBT based BL control for DP (rev4)
URL   : https://patchwork.freedesktop.org/series/25323/
State : failure

== Summary ==

Applying: drm/i915/bxt: Enable VBT based BL control for DP (v2)
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_dp.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_dp.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_dp.c
Patch failed at 0001 drm/i915/bxt: Enable VBT based BL control for DP (v2)
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP (v2)

[Jani Nikula]

On Wed, 10 Jan 2018, Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com> wrote:
> 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.
>
> Signed-off-by: Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 09f2744..e00569f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -640,19 +640,15 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	int backlight_controller = dev_priv->vbt.backlight.controller;

You need to look at connector->panel->backlight->controller
instead. This gets initialized to the vbt value, but we only use the vbt
information in bxt_setup_backlight().

BR,
Jani.

>  
>  	lockdep_assert_held(&dev_priv->pps_mutex);
>  
>  	/* 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 backlight_controller;
>  
>  	intel_dp->pps_reset = false;
>  
> @@ -662,7 +658,7 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>  	 */
>  	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, false);
>  
> -	return 0;
> +	return backlight_controller;
>  }
>  
>  typedef bool (*vlv_pipe_check)(struct drm_i915_private *dev_priv,

-- 
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: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP (v2)

[Mustaffa, Mustamin B]

Hi Jani, 

May I know why the need to use connector as connector wasn't initialized in parent function ' intel_pps_get_registers'? 

While ' dev_priv' already initialized which also already initialized to the VBT value. So it make sense to me to use 'dev_priv' structure to read the VBT value instead of connector. 

Or did I missed something? Please let me know. 

Best regard 

Mustamin


-----Original Message-----
From: Jani Nikula [mailto:jani.nikula@linux.intel.com] 
Sent: Tuesday, January 16, 2018 6:03 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] [PATCH] drm/i915/bxt: Enable VBT based BL control for DP (v2)

On Wed, 10 Jan 2018, Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com> wrote:
> 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.
>
> Signed-off-by: Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> b/drivers/gpu/drm/i915/intel_dp.c index 09f2744..e00569f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -640,19 +640,15 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	int backlight_controller = dev_priv->vbt.backlight.controller;

You need to look at connector->panel->backlight->controller
instead. This gets initialized to the vbt value, but we only use the vbt information in bxt_setup_backlight().

BR,
Jani.

>  
>  	lockdep_assert_held(&dev_priv->pps_mutex);
>  
>  	/* 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 backlight_controller;
>  
>  	intel_dp->pps_reset = false;
>  
> @@ -662,7 +658,7 @@ static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
>  	 */
>  	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, false);
>  
> -	return 0;
> +	return backlight_controller;
>  }
>  
>  typedef bool (*vlv_pipe_check)(struct drm_i915_private *dev_priv,

--
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: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP (v2)

[Jani Nikula]

On Fri, 09 Feb 2018, "Mustaffa, Mustamin B" <mustamin.b.mustaffa@intel.com> wrote:
> May I know why the need to use connector as connector wasn't
> initialized in parent function ' intel_pps_get_registers'?

Oh, right, I overlooked that.

> While ' dev_priv' already initialized which also already initialized
> to the VBT value. So it make sense to me to use 'dev_priv' structure
> to read the VBT value instead of connector.

Okay, I think the right thing to do in the long run is to refactor the
code to initialize the value in the connector earlier. We'll want to use
the VBT only for initialization once. But for now, I think this is
fine. Please rebase the patch against current upstream so we get fresh
CI results, and resend.

Thanks,
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: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP (v2)

[Mustaffa, Mustamin B]

Hi Jani, 

I able to figure out using intel_dp structure instead of dev_priv. 

Should I continue using dev_priv or intel_dp?

-       int backlight_controller = dev_priv->vbt.backlight.controller;
+       int backlight_controller = intel_dp->attached_connector->panel.backlight.controller;

Best regard 

Mustamin

-----Original Message-----
From: Jani Nikula [mailto:jani.nikula@linux.intel.com] 
Sent: Tuesday, February 13, 2018 11:53 PM
To: Mustaffa, Mustamin B <mustamin.b.mustaffa@intel.com>; intel-gfx@lists.freedesktop.org
Subject: RE: [Intel-gfx] [PATCH] drm/i915/bxt: Enable VBT based BL control for DP (v2)

On Fri, 09 Feb 2018, "Mustaffa, Mustamin B" <mustamin.b.mustaffa@intel.com> wrote:
> May I know why the need to use connector as connector wasn't 
> initialized in parent function ' intel_pps_get_registers'?

Oh, right, I overlooked that.

> While ' dev_priv' already initialized which also already initialized 
> to the VBT value. So it make sense to me to use 'dev_priv' structure 
> to read the VBT value instead of connector.

Okay, I think the right thing to do in the long run is to refactor the code to initialize the value in the connector earlier. We'll want to use the VBT only for initialization once. But for now, I think this is fine. Please rebase the patch against current upstream so we get fresh CI results, and resend.

Thanks,
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: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP (v2)

[Jani Nikula]

On Wed, 14 Feb 2018, "Mustaffa, Mustamin B" <mustamin.b.mustaffa@intel.com> wrote:
> Hi Jani, 
>
> I able to figure out using intel_dp structure instead of dev_priv. 
>
> Should I continue using dev_priv or intel_dp?
>
> -       int backlight_controller = dev_priv->vbt.backlight.controller;
> +       int backlight_controller = intel_dp->attached_connector->panel.backlight.controller;

Point was, connector->panel.backlight.controller gets initialized in
intel_panel_setup_backlight() which is called much later than
intel_dp_pps_init() in intel_edp_init_connector().


BR,
Jani.

>
> Best regard 
>
> Mustamin
>
> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com] 
> Sent: Tuesday, February 13, 2018 11:53 PM
> To: Mustaffa, Mustamin B <mustamin.b.mustaffa@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH] drm/i915/bxt: Enable VBT based BL control for DP (v2)
>
> On Fri, 09 Feb 2018, "Mustaffa, Mustamin B" <mustamin.b.mustaffa@intel.com> wrote:
>> May I know why the need to use connector as connector wasn't 
>> initialized in parent function ' intel_pps_get_registers'?
>
> Oh, right, I overlooked that.
>
>> While ' dev_priv' already initialized which also already initialized 
>> to the VBT value. So it make sense to me to use 'dev_priv' structure 
>> to read the VBT value instead of connector.
>
> Okay, I think the right thing to do in the long run is to refactor the code to initialize the value in the connector earlier. We'll want to use the VBT only for initialization once. But for now, I think this is fine. Please rebase the patch against current upstream so we get fresh CI results, and resend.
>
> Thanks,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Technology Center

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