Re: [PATCH 11/12] drm/i915/bxt: Modify BXT BLC according to VBT changes

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 25 May 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 22 May 2015, Uma Shankar <uma.shankar@intel.com> wrote:
>> From: Sunil Kamath <sunil.kamath@intel.com>
>>
>> Latest VBT mentions which set of registers will be used for BLC,
>> as controller number field. Making use of this field in BXT
>> BLC implementation. Also, the registers are used in case control
>> pin indicates display DDI. Adding a check for this.
>> According to Bspec, BLC_PWM_*_2 uses the display utility pin for output.
>> To use backlight 2, enable the utility pin with mode = PWM
>>    v2: Jani's review comments
>>    addressed
>>        - Add a prefix _ to BXT BLC registers definitions.
>>        - Add "bxt only" comment for u8 controller
>>        - Remove control_pin check for DDI controller
>>        - Check for valid controller values
>>        - Set pipe bits in UTIL_PIN_CTL
>>        - Enable/Disable UTIL_PIN_CTL in enable/disable_backlight()
>>        - If BLC 2 is used, read active_low_pwm from UTIL_PIN polarity
>>    Satheesh's review comment addressed
>>        - If UTIL PIN is already enabled, BIOS would have programmed it. No
>>        need to disable and enable again.
>>    v3: Jani's review comments
>>        - add UTIL_PIN_PIPE_MASK and UTIL_PIN_MODE_MASK
>>        - Disable UTIL_PIN if controller 1 is used
>>        - Mask out UTIL_PIN_PIPE_MASK and UTIL_PIN_MODE_MASK before enabling
>>        UTIL_PIN
>>        - check valid controller value in intel_bios.c
>>        - add backlight.util_pin_active_low
>>        - disable util pin before enabling
>>    v4: Change for BXT-PO branch:
>>    Stubbed unwanted definition which was existing before
>>    because of DC6 patch.
>>    UTIL_PIN_MODE_PWM     (0x1b << 24)
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> Signed-off-by: Sunil Kamath <sunil.kamath@intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h    |   28 +++++++---
>>  drivers/gpu/drm/i915/intel_drv.h   |    2 +
>>  drivers/gpu/drm/i915/intel_panel.c |  100 ++++++++++++++++++++++++++++++------
>>  3 files changed, 106 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index f96f049..5e3958f 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3509,17 +3509,29 @@ enum skl_disp_power_wells {
>>  #define UTIL_PIN_CTL		0x48400
>>  #define   UTIL_PIN_ENABLE	(1 << 31)
>>  
>> +#define   UTIL_PIN_PIPE(x)     ((x) << 29)
>> +#define   UTIL_PIN_PIPE_MASK   (3 << 29)
>> +#define   UTIL_PIN_MODE_PWM    (1 << 24)
>> +#define   UTIL_PIN_MODE_MASK   (0xf << 24)
>> +#define   UTIL_PIN_POLARITY    (1 << 22)
>> +
>>  /* BXT backlight register definition. */
>> -#define BXT_BLC_PWM_CTL1			0xC8250
>> +#define _BXT_BLC_PWM_CTL1			0xC8250
>>  #define   BXT_BLC_PWM_ENABLE			(1 << 31)
>>  #define   BXT_BLC_PWM_POLARITY			(1 << 29)
>> -#define BXT_BLC_PWM_FREQ1			0xC8254
>> -#define BXT_BLC_PWM_DUTY1			0xC8258
>> -
>> -#define BXT_BLC_PWM_CTL2			0xC8350
>> -#define BXT_BLC_PWM_FREQ2			0xC8354
>> -#define BXT_BLC_PWM_DUTY2			0xC8358
>> -
>> +#define _BXT_BLC_PWM_FREQ1			0xC8254
>> +#define _BXT_BLC_PWM_DUTY1			0xC8258
>> +
>> +#define _BXT_BLC_PWM_CTL2			0xC8350
>> +#define _BXT_BLC_PWM_FREQ2			0xC8354
>> +#define _BXT_BLC_PWM_DUTY2			0xC8358
>> +
>> +#define BXT_BLC_PWM_CTL(controller)    _PIPE(controller, \
>> +					_BXT_BLC_PWM_CTL1, _BXT_BLC_PWM_CTL2)
>> +#define BXT_BLC_PWM_FREQ(controller)   _PIPE(controller, \
>> +					_BXT_BLC_PWM_FREQ1, _BXT_BLC_PWM_FREQ2)
>> +#define BXT_BLC_PWM_DUTY(controller)   _PIPE(controller, \
>> +					_BXT_BLC_PWM_DUTY1, _BXT_BLC_PWM_DUTY2)
>>  
>>  #define PCH_GTC_CTL		0xe7000
>>  #define   PCH_GTC_ENABLE	(1 << 31)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 47bc729..ee9eb2b 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -182,7 +182,9 @@ struct intel_panel {
>>  		bool enabled;
>>  		bool combination_mode;	/* gen 2/4 only */
>>  		bool active_low_pwm;
>> +		bool util_pin_active_low;	/* bxt+ */
>>  		struct backlight_device *device;
>> +		u8 controller;		/* bxt+ only */
>>  	} backlight;
>>  
>>  	void (*backlight_power)(struct intel_connector *, bool enable);
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index 7d83527..23847f2 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -32,7 +32,10 @@
>>  
>>  #include <linux/kernel.h>
>>  #include <linux/moduleparam.h>
>> +#include <drm/drm_panel.h>
>>  #include "intel_drv.h"
>> +#include "intel_dsi.h"
>> +#include "i915_drv.h"
>>  
>>  void
>>  intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
>> @@ -539,9 +542,10 @@ static u32 vlv_get_backlight(struct intel_connector *connector)
>>  static u32 bxt_get_backlight(struct intel_connector *connector)
>>  {
>>  	struct drm_device *dev = connector->base.dev;
>> +	struct intel_panel *panel = &connector->panel;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  
>> -	return I915_READ(BXT_BLC_PWM_DUTY1);
>> +	return I915_READ(BXT_BLC_PWM_DUTY(panel->backlight.controller));
>>  }
>>  
>>  static u32 intel_panel_get_backlight(struct intel_connector *connector)
>> @@ -628,8 +632,9 @@ static void bxt_set_backlight(struct intel_connector *connector, u32 level)
>>  {
>>  	struct drm_device *dev = connector->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_panel *panel = &connector->panel;
>>  
>> -	I915_WRITE(BXT_BLC_PWM_DUTY1, level);
>> +	I915_WRITE(BXT_BLC_PWM_DUTY(panel->backlight.controller), level);
>>  }
>>  
>>  static void
>> @@ -761,12 +766,20 @@ static void bxt_disable_backlight(struct intel_connector *connector)
>>  {
>>  	struct drm_device *dev = connector->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	u32 tmp;
>> +	struct intel_panel *panel = &connector->panel;
>> +	u32 tmp, val;
>>  
>>  	intel_panel_actually_set_backlight(connector, 0);
>>  
>> -	tmp = I915_READ(BXT_BLC_PWM_CTL1);
>> -	I915_WRITE(BXT_BLC_PWM_CTL1, tmp & ~BXT_BLC_PWM_ENABLE);
>> +	tmp = I915_READ(BXT_BLC_PWM_CTL(panel->backlight.controller));
>> +	I915_WRITE(BXT_BLC_PWM_CTL(panel->backlight.controller),
>> +			tmp & ~BXT_BLC_PWM_ENABLE);
>> +
>> +	if (panel->backlight.controller == 1) {
>> +		val = I915_READ(UTIL_PIN_CTL);
>> +		val &= ~UTIL_PIN_ENABLE;
>> +		I915_WRITE(UTIL_PIN_CTL, val);
>> +	}
>>  }
>>  
>>  void intel_panel_disable_backlight(struct intel_connector *connector)
>> @@ -980,16 +993,39 @@ static void bxt_enable_backlight(struct intel_connector *connector)
>>  	struct drm_device *dev = connector->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_panel *panel = &connector->panel;
>> -	u32 pwm_ctl;
>> +	enum pipe pipe = intel_get_pipe_from_connector(connector);
>> +	u32 pwm_ctl, val;
>>  
>> -	pwm_ctl = I915_READ(BXT_BLC_PWM_CTL1);
>> +	/* To use 2nd set of backlight registers, utility pin has to be
>> +	 * enabled with PWM mode.
>> +	 * The field should only be changed when the utility pin is disabled
>> +	 */
>> +	if (panel->backlight.controller == 1) {
>> +		val = I915_READ(UTIL_PIN_CTL);
>> +		if (val & UTIL_PIN_ENABLE) {
>> +			DRM_DEBUG_KMS("util pin already enabled\n");
>> +			val &= ~UTIL_PIN_ENABLE;
>> +			I915_WRITE(UTIL_PIN_CTL, val);
>> +		}
>> +		/* mask out UTIL_PIN_PIPE and UTIL_PIN_MODE */
>> +		val &= ~(UTIL_PIN_PIPE_MASK | UTIL_PIN_MODE_MASK);
>> +		I915_WRITE(UTIL_PIN_CTL, val);
>> +		if (panel->backlight.util_pin_active_low)
>> +			val |= UTIL_PIN_POLARITY;
>> +		I915_WRITE(UTIL_PIN_CTL, val | UTIL_PIN_PIPE(pipe) |
>> +				UTIL_PIN_MODE_PWM | UTIL_PIN_ENABLE);
>> +	}
>> +
>> +	pwm_ctl = I915_READ(BXT_BLC_PWM_CTL(panel->backlight.controller));
>>  	if (pwm_ctl & BXT_BLC_PWM_ENABLE) {
>>  		DRM_DEBUG_KMS("backlight already enabled\n");
>>  		pwm_ctl &= ~BXT_BLC_PWM_ENABLE;
>> -		I915_WRITE(BXT_BLC_PWM_CTL1, pwm_ctl);
>> +		I915_WRITE(BXT_BLC_PWM_CTL(panel->backlight.controller),
>> +				pwm_ctl);
>>  	}
>>  
>> -	I915_WRITE(BXT_BLC_PWM_FREQ1, panel->backlight.max);
>> +	I915_WRITE(BXT_BLC_PWM_FREQ(panel->backlight.controller),
>> +			panel->backlight.max);
>>  
>>  	intel_panel_actually_set_backlight(connector, panel->backlight.level);
>>  
>> @@ -997,9 +1033,10 @@ static void bxt_enable_backlight(struct intel_connector *connector)
>>  	if (panel->backlight.active_low_pwm)
>>  		pwm_ctl |= BXT_BLC_PWM_POLARITY;
>>  
>> -	I915_WRITE(BXT_BLC_PWM_CTL1, pwm_ctl);
>> -	POSTING_READ(BXT_BLC_PWM_CTL1);
>> -	I915_WRITE(BXT_BLC_PWM_CTL1, pwm_ctl | BXT_BLC_PWM_ENABLE);
>> +	I915_WRITE(BXT_BLC_PWM_CTL(panel->backlight.controller), pwm_ctl);
>> +	POSTING_READ(BXT_BLC_PWM_CTL(panel->backlight.controller));
>> +	I915_WRITE(BXT_BLC_PWM_CTL(panel->backlight.controller),
>> +			pwm_ctl | BXT_BLC_PWM_ENABLE);
>>  }
>>  
>>  void intel_panel_enable_backlight(struct intel_connector *connector)
>> @@ -1008,6 +1045,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_panel *panel = &connector->panel;
>>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>> +	struct intel_dsi *intel_dsi = NULL;
>> +	struct drm_crtc *crtc = NULL;
>> +	struct intel_encoder *encoder = NULL;
>>  
>>  	if (!panel->backlight.present)
>>  		return;
>> @@ -1027,7 +1067,18 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>>  						 panel->backlight.device->props.max_brightness);
>>  	}
>>  
>> -	dev_priv->display.enable_backlight(connector);
>> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>> +		for_each_encoder_on_crtc(dev, crtc, encoder) {
>> +			if (encoder->type == INTEL_OUTPUT_DSI)
>> +				intel_dsi = enc_to_intel_dsi(&encoder->base);
>> +		}
>> +	}
>> +
>> +	if (IS_BROXTON(dev) && intel_dsi->panel->funcs->enable)
>> +		intel_dsi->panel->funcs->enable(intel_dsi->panel);
>> +	else
>> +		dev_priv->display.enable_backlight(connector);
>> +
>
> This is backwards. The DSI code should call intel_panel_enable_backlight
> and intel_panel_disable_backlight, not the other way round.

The code should also call setup backlight. Presumably there won't (at
least initially) be a setup with both eDP and DSI.

BR,
Jani.

>
> BR,
> Jani.
>
>
>>  	panel->backlight.enabled = true;
>>  	if (panel->backlight.device)
>>  		panel->backlight.device->props.power = FB_BLANK_UNBLANK;
>> @@ -1362,10 +1413,27 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused)
>>  	struct intel_panel *panel = &connector->panel;
>>  	u32 pwm_ctl, val;
>>  
>> -	pwm_ctl = I915_READ(BXT_BLC_PWM_CTL1);
>> -	panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY;
>> +	/* For BXT hard coding the Backlight controller to 0.
>> +	 * ToDo : Read the controller value from VBT and generalize
>> +	 */
>> +	panel->backlight.controller = 0;
>> +
>> +	pwm_ctl = I915_READ(BXT_BLC_PWM_CTL(panel->backlight.controller));
>> +
>> +	/* Keeping the check if controller 1 is to be programmed. 
>> +	 * This will come into affect once the VBT parsing
>> +	 * is fixed for controller selection, and controller 1 is used
>> +	 * for a prticular display configuration.
>> +	 */ 
>> +	if (panel->backlight.controller == 1) {
>> +		val = I915_READ(UTIL_PIN_CTL);
>> +		panel->backlight.util_pin_active_low =
>> +			val & UTIL_PIN_POLARITY;
>> +	}
>>  
>> -	panel->backlight.max = I915_READ(BXT_BLC_PWM_FREQ1);
>> +	panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY;
>> +	panel->backlight.max = I915_READ(
>> +				BXT_BLC_PWM_FREQ(panel->backlight.controller));
>>  	if (!panel->backlight.max)
>>  		return -ENODEV;
>>  
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

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